Misc improvements to details table in skeleton and segments tab #8316
Conversation
disable old edit-name-input
…leton-details-follow-up
📝 WalkthroughWalkthroughThis pull request introduces enhancements to segment and tree group management in the WEBKNOSSOS frontend. The changes focus on improving user interaction by enabling detailed views of segment groups, supporting active group selection, and allowing inline editing of tree and group names. The modifications span multiple components, introducing new rendering logic, memoization for performance, and restructuring of import paths to better organize the codebase. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (3)
2017-2022: Improve error handling specificity.Consider providing more specific feedback about why the details couldn't be rendered. Split the condition to handle each case separately.
- if (!activeGroup || this.props.segments == null) { - return null; - } + if (!activeGroup) { + return <>Could not find the selected group.</>; + } + if (this.props.segments == null) { + return <>Segment data is not available.</>; + }
2024-2026: Consider memoizing segment count calculations.The segment count calculations, especially for all children, could be expensive for large groups. Consider memoizing these calculations to improve performance.
+ const memoizedGetSegmentCount = React.useMemo(() => { + return _.sum( + groupWithSubgroups.map((groupId) => groupToSegmentsMap[groupId]?.length ?? 0) + ); + }, [groupWithSubgroups, groupToSegmentsMap]);Also applies to: 2068-2071
2027-2077: Consider extracting segment count display into a separate component.The segment count display logic could be extracted into a separate component to improve readability and maintainability. This would also make it easier to reuse this visualization elsewhere.
+ const GroupSegmentCounts: React.FC<{ + groupId: number; + groupWithSubgroups: number[]; + groupToSegmentsMap: Record<number, Segment[]>; + }> = ({ groupId, groupWithSubgroups, groupToSegmentsMap }) => { + const directChildrenCount = groupToSegmentsMap[groupId]?.length ?? 0; + + if (groupWithSubgroups.length === 1) { + return ( + <tr> + <td>Segment Count</td> + <td colSpan={2}>{directChildrenCount}</td> + </tr> + ); + } + + return ( + <> + <tr> + <td>Segment Count (direct children)</td> + <td colSpan={2}>{directChildrenCount}</td> + </tr> + <tr> + <td>Segment Count (all children)</td> + <td colSpan={2}> + {_.sum( + groupWithSubgroups.map((gId) => groupToSegmentsMap[gId]?.length ?? 0) + )} + </td> + </tr> + </> + ); + };frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view_helpers.ts (1)
209-209: Consider adding JSDoc comments.Since this is a public API, consider adding JSDoc comments to document the function's purpose, parameters, and return type.
+/** + * Creates a memoized map of group IDs to their associated trees. + * @param trees - Map of tree IDs to Tree objects + * @returns Record mapping group IDs to arrays of associated trees + */ export const createGroupToTreesMap = memoizeOne(_createGroupToTreesMap);frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (1)
403-404: Consider memoizing expensive computations.The
createGroupToTreesMapandgetGroupByIdWithSubgroupscalls could be memoized to prevent unnecessary recalculations on re-renders.+const memoizedGroupToTreesMap = useMemo( + () => createGroupToTreesMap(trees), + [trees] +); +const memoizedGroupWithSubgroups = useMemo( + () => getGroupByIdWithSubgroups(treeGroups, activeGroupId), + [treeGroups, activeGroupId] +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.ts(3 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx(3 hunks)frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view_helpers.ts(2 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx(5 hunks)
🔇 Additional comments (6)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (1)
1984-1985: LGTM! Clean destructuring and condition check.The destructuring of selectedIds and the single segment selection check are well-implemented.
frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view_helpers.ts (1)
205-208: Good use of memoization for performance optimization.The
createGroupToTreesMapfunction is now memoized usingmemoizeOne, which will prevent redundant computations when the input trees haven't changed.frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.ts (1)
197-197: Verify the behavior of segment group assignment.The segment's
groupIdis now derived from the first selected group ID. This is a significant change in behavior that warrants verification:
- What happens if no group is selected?
- Is it intentional to always use the first selected group?
- Should there be a fallback value if
getSelectedIdsreturns an empty array?✅ Verification successful
The segment group assignment behavior is properly implemented and safe.
The implementation includes appropriate null handling and fallbacks:
- Returns null if no group is selected
- Allows explicit group assignments to override the default
- Cleans up invalid selections automatically
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of getSelectedIds to understand the fallback behavior # Search for getSelectedIds implementation ast-grep --pattern 'export function getSelectedIds($_) { $$$ }' # Search for other usages of getSelectedIds to understand the expected behavior rg 'getSelectedIds' -A 5Length of output: 4279
Script:
#!/bin/bash # Get the implementation of _getSelectedIds ast-grep --pattern 'function _getSelectedIds($_) { $$$ }' # Get the context of the groupId assignment in the reducer rg -B 5 -A 5 'groupId: getSelectedIds\(state\)\[0\]\.group' # Look for any validation or error handling rg '\[0\]\.group' -B 2 -A 2Length of output: 2305
Script:
#!/bin/bash # Get the implementation from the specific file cat frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts # Get the full reducer case context rg -B 20 -A 10 'groupId: getSelectedIds\(state\)\[0\]\.group' frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.ts # Look for initial state definition rg 'selectedIds.*=.*{' -A 3 frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.tsLength of output: 29705
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (2)
397-401: Good defensive programming.The code properly checks for the existence of the active group before attempting to display its details.
424-444: Well-structured conditional rendering for tree counts.The code elegantly handles both single group and hierarchical group cases, providing clear information about direct children and total descendant trees.
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx (1)
918-921: Good user communication about deprecated feature.The code properly handles the deprecation of the name input field by:
- Disabling the input to prevent confusion
- Adding a clear tooltip message about future removal
- Directing users to the new location in the details table
| <table className="metadata-table"> | ||
| <thead> | ||
| <tr> | ||
| <th>ID</th> | ||
| <th colSpan={2}>{activeGroup.groupId}</th> | ||
| </tr> | ||
| </thead> | ||
| <tbody> | ||
| <tr> |
There was a problem hiding this comment.
This and below is pretty similar to the code in the segments view. Maybe this could be extracted into an own component as we want to unify the behaviour for both group tables?
The component then would need props that include
- kind (segment | tree) to display the correct text
- name
- id
- optional directChildCount, totalChildCount or so
This would help us to keep the two tables in sync.
But I also see that they diverge quite some bit regarding logic. Thus, the logic to calculate the props and so would need to stay her / in segments_view.tsx.
What do you think about the suggestion?
There was a problem hiding this comment.
But I also see that they diverge quite some bit regarding logic. Thus, the logic to calculate the props and so would need to stay her / in segments_view.tsx.
I also thought about DRYing this, but the points you raise here stopped me from doing that. In the end, I'd much rather refactor the skeleton and segments tab so that their underlying datastructures are almost interchangable. Then, reusing the same views would be a breeze (and wouldn't require extracting so many props).
As a trade-off, I created a new <SimpleRow /> component that abstracts stuff like td colSpan={2} away. I think, the code is leaner now and the similarity of skeleton and segment code is more bearable imo. Hope you agree!
There was a problem hiding this comment.
Hmm well. It's at least something.
I also thought about DRYing this, but the points you raise here stopped me from doing that. In the end, I'd much rather refactor the skeleton and segments tab so that their underlying datastructures are almost interchangable. Then, reusing the same views would be a breeze (and wouldn't require extracting so many props).
That would be pretty nice 🎉
| @@ -1,5 +1,6 @@ | |||
| import type { DataNode } from "antd/es/tree"; | |||
| import _ from "lodash"; | |||
| import memoizeOne from "memoize-one"; | |||
There was a problem hiding this comment.
Regarding the whole file:
I would move this file into the trees_tab folder as its name suggests that it should be within this folder
There was a problem hiding this comment.
yeah, good idea. unfortunately, this file also contains at least one function related to the segments tab, but I moved it to the trees_tab folder, anyway, to make the right-borders-tab folder a bit more sparse.
…leton-details-follow-up
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view_helpers.ts (1)
Line range hint
144-150: Consider simplifying the return type.The function implementation only returns
TreeGroup | null, so the return type could be simplified fromTreeGroup | null | undefinedto justTreeGroup | null.-export function findGroup(groups: TreeGroup[], groupId: number): TreeGroup | null | undefined { +export function findGroup(groups: TreeGroup[], groupId: number): TreeGroup | null {frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx (1)
99-105: Consider adding validation for empty tree namesWhile the EditableTextLabel implementation looks good, consider adding validation to prevent empty tree names.
<EditableTextLabel value={tree.name} label="Tree Name" - onChange={(newValue) => Store.dispatch(setTreeNameAction(newValue, tree.treeId))} + onChange={(newValue) => { + const trimmedValue = newValue.trim(); + if (trimmedValue.length === 0) { + Toast.error("Tree name cannot be empty"); + return; + } + Store.dispatch(setTreeNameAction(trimmedValue, tree.treeId)); + }} hideEditIcon margin={0} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/components/editable_text_label.tsx(5 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx(5 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view_helpers.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (7)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view_helpers.ts (2)
3-3: LGTM! Good performance optimization.Adding memoization will help optimize expensive computations by caching results.
205-209: LGTM! Well-structured refactoring with performance optimization.The refactoring maintains the original functionality while adding memoization for better performance. The private function pattern with the underscore prefix is a good practice.
frontend/javascripts/oxalis/view/components/editable_text_label.tsx (3)
26-26: LGTM! Type definition enhancementThe addition of the
hideEditIconproperty provides good flexibility for controlling the component's UI behavior.
152-163: LGTM! Clean event handling implementationThe
onRenamefunction properly handles event propagation, state management, and optional callbacks.
Line range hint
202-223: LGTM! Consistent editing behaviorThe render method changes successfully implement double-click editing while maintaining consistency with icon-click behavior.
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx (1)
98-98: LGTM! Informative node count displayAdding the node count provides valuable information about tree size at a glance.
CHANGELOG.unreleased.md (1)
22-23: LGTM! Clear and accurate changelog entriesThe changelog entries effectively communicate the new features to users.
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx (1)
230-237:⚠️ Potential issueAdd error handling for group rename operation.
The implementation of the editable group name looks good, but it's missing error handling.
The error handling suggestion from the previous review still applies here. Apply this diff to handle potential errors:
<EditableTextLabel value={displayableName} label="Group Name" - onChange={(newValue) => api.tracing.renameSkeletonGroup(id, newValue)} + onChange={async (newValue) => { + try { + await api.tracing.renameSkeletonGroup(id, newValue); + } catch (error) { + Toast.error(`Failed to rename group: ${error.message}`); + } + }} hideEditIcon margin={0} />
🧹 Nitpick comments (3)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx (1)
98-105: Enhance tree node display with node count and editable name.The implementation looks good. The node count is displayed before the tree name, and the
EditableTextLabelcomponent allows for inline editing with proper action dispatching.However, consider adding error handling for the tree name change operation.
<EditableTextLabel value={tree.name} label="Tree Name" - onChange={(newValue) => Store.dispatch(setTreeNameAction(newValue, tree.treeId))} + onChange={async (newValue) => { + try { + Store.dispatch(setTreeNameAction(newValue, tree.treeId)); + } catch (error) { + Toast.error(`Failed to rename tree: ${error.message}`); + } + }} hideEditIcon margin={0} />frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (1)
397-447: Consider extracting group details into a separate component.The group details table implementation is good, but it shares similar structure with the tree details table above. This is an opportunity for code reuse.
Consider extracting both the tree and group details tables into a shared component to maintain consistency and reduce code duplication. The component could accept props like:
- kind: 'tree' | 'group'
- id: number
- name: string
- metadata?: MetadataEntryProto[]
- directChildCount?: number
- totalChildCount?: number
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx (1)
918-921: Consider removing the deprecated input field.Since this text field will be removed in a future update, consider:
- Adding a deprecation warning in the console
- Creating a migration guide for users
- Adding a more visible UI indication that this field is deprecated
<InputComponent onChange={() => {}} value={activeTreeName || activeGroupName} disabled - title="Edit the name in the details table below the tree list. Note: This text field will be removed in a future update." + title="DEPRECATED: Edit the name in the details table below the tree list. This field will be removed in a future update." + style={{ + width: "80%", + backgroundColor: "var(--ant-color-warning-bg)", + border: "1px solid var(--ant-color-warning)" + }} />Also, consider adding a console warning:
useEffect(() => { console.warn( "The tree/group name input field in the toolbar is deprecated and will be removed. " + "Please use the name field in the details table below the tree list instead." ); }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
frontend/javascripts/oxalis/api/api_latest.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts(1 hunks)frontend/javascripts/oxalis/model/helpers/compaction/compact_toggle_actions.ts(1 hunks)frontend/javascripts/oxalis/model/helpers/nml_helpers.ts(1 hunks)frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer.ts(1 hunks)frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.ts(3 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx(3 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx(2 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx(6 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx(5 hunks)frontend/javascripts/oxalis/view/version_entry.tsx(1 hunks)frontend/javascripts/test/api/api_skeleton_latest.spec.ts(1 hunks)frontend/javascripts/test/libs/nml.spec.ts(1 hunks)frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts(1 hunks)frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts(1 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts
- frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts
- frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration.spec.ts
- frontend/javascripts/oxalis/model/helpers/compaction/compact_toggle_actions.ts
- frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts
- frontend/javascripts/oxalis/view/version_entry.tsx
- frontend/javascripts/test/api/api_skeleton_latest.spec.ts
- frontend/javascripts/oxalis/api/api_latest.ts
- frontend/javascripts/test/libs/nml.spec.ts
- frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer.ts
- frontend/javascripts/oxalis/model/helpers/nml_helpers.ts
- frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx
- frontend/javascripts/oxalis/model/reducers/volumetracing_reducer.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (1)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (1)
334-338: Props enhancement looks good.The addition of
treeGroupsandactiveGroupIdprops is well-integrated with the existing props structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/dashboard/folders/metadata_table.tsx (1)
546-566: LGTM! Consider using TypeScript interface for props.The
SimpleRowcomponent is well-implemented with semantic table elements and proper handling of header/regular rows.Consider defining a TypeScript interface for the props:
interface SimpleRowProps { label: React.ReactNode; value: React.ReactNode; isTableHead?: boolean; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/dashboard/folders/metadata_table.tsx(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx(3 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (3)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (3)
371-387: Good use of SimpleRow component for consistent table structure.The refactoring to use
SimpleRowimproves consistency across the application.
395-438: Consider extracting shared group details logic into a separate component.The group details rendering logic is similar to the code in the segments view. Consider extracting this into a reusable component to maintain consistency between tree and segment group tables.
The component could accept props like:
interface GroupDetailsProps { kind: 'segment' | 'tree'; group: TreeGroup | SegmentGroup; itemCount: { direct: number; total: number; }; onRename: (id: number, name: string) => void; readOnly?: boolean; }
430-432: Verify performance impact of lodash sum operation.Using
_.sum()on an array of group lengths could be expensive for large tree hierarchies. Consider memoizing the calculation if performance becomes an issue.Run this script to check the size of tree hierarchies in the codebase:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (1)
Line range hint
1985-2071: Consider extracting common table components.The details table implementation is duplicated between the tree hierarchy and segments views. Consider extracting a common
DetailsTablecomponent to:
- Reduce code duplication
- Ensure consistent behavior
- Simplify maintenance
Example structure:
interface DetailsTableProps { id: number; name: string; onNameChange: (newName: string) => void; metadata?: MetadataEntryProto[]; counts?: { directChildren?: number; totalChildren?: number; }; readOnly?: boolean; } const DetailsTable: React.FC<DetailsTableProps> = ({ id, name, onNameChange, metadata, counts, readOnly, }) => { return ( <table className="metadata-table"> <thead> <SimpleRow isTableHead label="ID" value={id} /> </thead> <tbody> <SimpleRow label="Name" value={ <InputWithUpdateOnBlur value={name} onChange={onNameChange} disabled={readOnly} /> } /> {counts && ( <> {counts.directChildren != null && ( <SimpleRow label="Count (direct children)" value={counts.directChildren} /> )} {counts.totalChildren != null && ( <SimpleRow label="Count (all children)" value={counts.totalChildren} /> )} </> )} {metadata && ( <MetadataEntryTableRows item={{ metadata }} setMetadata={setMetadata} readOnly={readOnly} /> )} </tbody> </table> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx(4 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx(5 hunks)
🔇 Additional comments (5)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_view.tsx (3)
3-3: LGTM: New imports are correctly added.The new imports for
SimpleRowandlodashare properly added and will be used for rendering table rows and utility functions.Also applies to: 5-5
371-387: LGTM: Tree details table is properly structured.The table structure for displaying tree details is well-organized using the
SimpleRowcomponent, with proper handling of tree ID, name, and metadata.
395-438: LGTM: Group details table implementation is consistent.The implementation for displaying group details follows the same pattern as tree details, showing:
- Group ID and name
- Direct children count
- Total children count (when applicable)
The code properly handles null checks and uses appropriate conditional rendering.
Note: This implementation aligns with a past discussion about unifying the behavior for both group tables, as mentioned in the previous review comments.
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx (2)
1985-1986: LGTM: Segment details table is properly implemented.The implementation correctly handles:
- Selection state checks
- Segment existence validation
- Proper table structure with ID, name, and metadata
Also applies to: 1988-2014
2015-2071: LGTM: Group details table implementation mirrors tree hierarchy view.The implementation for group details follows the same pattern as the tree hierarchy view, maintaining consistency across the application. It properly handles:
- Group existence validation
- Display of group ID and name
- Segment counts (direct children and total)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/model/helpers/shader_editor.ts (1)
115-115: Sanitize 'shaderType' before injecting into HTML to prevent XSS vulnerabilitiesWhen setting
button.innerHTMLwith a dynamic value likeshaderType, it's important to sanitize it to prevent potential Cross-Site Scripting (XSS) attacks, especially ifshaderTypecan be influenced by user input.Consider using
textContentinstead ofinnerHTMLor sanitizeshaderTypebefore assignment:- button.innerHTML = `Update ${shaderType}`; + button.textContent = `Update ${shaderType}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts(1 hunks)frontend/javascripts/oxalis/model/helpers/shader_editor.ts(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataConverter.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/FindDataService.scala(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (8)
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.ts (2)
597-600: LGTM! Improved event handling for unmerged bucket data.The code now correctly clones the data before any in-place modifications occur, ensuring that the
unmergedBucketDataLoadedevent receives unmodified data.
597-600: Verify the impact on undo operations.The changes affect how unmerged bucket data is handled during undo operations. Let's verify that undo operations work correctly with these changes.
✅ Verification successful
Implementation safely handles data integrity for undo operations
The code correctly clones the data before triggering the event, ensuring that subsequent in-place merge operations don't affect the snapshot used by undo functionality. The promise-based flow and proper cleanup of temporary states prevent any race conditions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for undo-related tests that might be affected by these changes rg -l "undo.*bucket" --type ts --type test # Look for any existing issues related to bucket undo operations gh issue list --search "bucket undo" # Check for any error handling patterns around unmerged bucket data ast-grep --pattern 'catch($_) { $$$ unmergedBucketDataLoaded $$$ }'Length of output: 401
Script:
#!/bin/bash # Search for test files with correct extension rg -l "undo.*bucket" --type ts # Find usage of unmergedBucketDataLoaded event rg "unmergedBucketDataLoaded" -A 3 --type ts # Look for test coverage ast-grep --pattern 'describe($_, () => { $$$ bucket $$$ })'Length of output: 1479
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataConverter.scala (2)
50-51: Method name and signature accurately reflect the new behavior.The rename from
toUnsignedtotoMaybeUnsignedbetter represents that not all types are converted to unsigned, and the expanded return type correctly includesByte.
53-53: Verify the impact of returning signed bytes.The change to return byte arrays directly without converting to unsigned might affect consumers of this API.
Let's verify the usage of this method across the codebase:
✅ Verification successful
The change to return byte arrays directly is safe.
The byte array is properly handled by the calling code in FindDataService where it's part of a conversion chain for histogram calculations. No other direct consumers of this method were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for method calls to understand the impact rg -A 3 "toMaybeUnsigned|toUnsigned" --type scalaLength of output: 1280
Script:
#!/bin/bash # Get the full implementation of DataConverter ast-grep --pattern 'class DataConverter { $$$ }' # Get the implementation of calculateHistogramValues ast-grep --pattern 'def calculateHistogramValues($$$) { $$$ }' # Look for any other byte array conversions or processing rg -A 5 "Array\[Byte\]" --type scalaLength of output: 89786
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/FindDataService.scala (3)
142-142: Method signature correctly expanded to support signed bytes.The addition of
Byteto the union type aligns with the changes inDataConverter.scala.
166-167: Variable name improved for clarity.The rename from
intDatatouintDatabetter reflects the unsigned nature of the data.
193-193: Verify data conversion pipeline.The change from
toUnsignedtotoMaybeUnsignedaffects the histogram calculation pipeline.Let's verify the conversion pipeline:
✅ Verification successful
Conversion pipeline is consistently implemented
The change from
toUnsignedtotoMaybeUnsignedis consistently applied across the codebase. The conversion pipeline is properly integrated with the histogram calculation system, with the converted data being used directly incalculateHistogramValues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar conversion patterns to ensure consistency rg "filterZeroes.*toMaybeUnsigned|toMaybeUnsigned.*filterZeroes" --type scalaLength of output: 300
Script:
#!/bin/bash # Search for both conversion functions and their usage patterns echo "=== Searching for toUnsigned ===" rg "toUnsigned" --type scala -A 2 echo -e "\n=== Searching for toMaybeUnsigned ===" rg "toMaybeUnsigned" --type scala -A 2 echo -e "\n=== Searching for histogram-related code ===" rg "histogram" --type scala -A 2Length of output: 3501
frontend/javascripts/oxalis/model/helpers/shader_editor.ts (1)
41-89: Ace editor options are well-configuredThe Ace editor configuration options are comprehensive and appropriate for enhancing the user experience. They cover essential features like syntax highlighting, line numbers, and keyboard accessibility.
| case byteData: Array[Byte] => { | ||
| byteData.foreach(el => counts(el.toInt + 128) += 1) | ||
| extrema = (-128, 128) | ||
| } |
There was a problem hiding this comment.
Verify histogram calculation for signed bytes.
The implementation shifts byte values by +128 to map them into the 0-255 range of the counts array. However, the extrema range is set to (-128, 128) which might be incorrect as bytes range from -128 to 127.
Apply this fix:
- extrema = (-128, 128)
+ extrema = (-128, 127)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case byteData: Array[Byte] => { | |
| byteData.foreach(el => counts(el.toInt + 128) += 1) | |
| extrema = (-128, 128) | |
| } | |
| case byteData: Array[Byte] => { | |
| byteData.foreach(el => counts(el.toInt + 128) += 1) | |
| extrema = (-128, 127) | |
| } |
| `, | ||
| ); | ||
| input.addEventListener("keydown", (evt) => { | ||
| if ((evt.keyCode === 10 || evt.keyCode === 13) && (evt.ctrlKey || event.metaKey)) { | ||
| evt.preventDefault(); | ||
| overrideShader(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Fix typo and update key event handling for cross-browser compatibility
In the keydown event listener, event.metaKey should be evt.metaKey to correctly detect the Meta key (Command key on Mac). Also, consider using evt.key instead of evt.keyCode as keyCode is deprecated and may not be consistent across browsers.
Apply this diff to correct the code:
input.addEventListener("keydown", (evt) => {
- if ((evt.keyCode === 10 || evt.keyCode === 13) && (evt.ctrlKey || event.metaKey)) {
+ if (evt.key === "Enter" && (evt.ctrlKey || evt.metaKey)) {
evt.preventDefault();
overrideShader();
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `, | |
| ); | |
| input.addEventListener("keydown", (evt) => { | |
| if ((evt.keyCode === 10 || evt.keyCode === 13) && (evt.ctrlKey || event.metaKey)) { | |
| evt.preventDefault(); | |
| overrideShader(); | |
| } | |
| }); | |
| `, | |
| ); | |
| input.addEventListener("keydown", (evt) => { | |
| if (evt.key === "Enter" && (evt.ctrlKey || evt.metaKey)) { | |
| evt.preventDefault(); | |
| overrideShader(); | |
| } | |
| }); |
| s.onload = s.onreadystatechange = function (_, isAbort) { | ||
| if (isAbort || !s.readyState || s.readyState === "loaded" || s.readyState === "complete") { | ||
| s = s.onload = s.onreadystatechange = null; | ||
| if (!isAbort) loadCallback(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix incorrect event handler parameters and add error handling for script loading
The event handler assigned to s.onload and s.onreadystatechange incorrectly uses the isAbort parameter, which is not provided by these event handlers. Additionally, error handling for failed script loading is missing.
Apply this diff to correct the event handler and add error handling:
const load = function (path, loadCallback) {
const head = document.getElementsByTagName("head")[0];
let s = document.createElement("script");
s.src = "https://ajaxorg.github.io/ace-builds/src-noconflict" + "/" + path;
head.appendChild(s);
- s.onload = s.onreadystatechange = function (_, isAbort) {
- if (isAbort || !s.readyState || s.readyState === "loaded" || s.readyState === "complete") {
- s = s.onload = s.onreadystatechange = null;
- if (!isAbort) loadCallback();
- }
- };
+ s.onload = function () {
+ s.onload = s.onreadystatechange = null;
+ loadCallback();
+ };
+ s.onerror = function () {
+ s.onload = s.onerror = null;
+ console.error("Failed to load script: " + s.src);
+ };
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| s.onload = s.onreadystatechange = function (_, isAbort) { | |
| if (isAbort || !s.readyState || s.readyState === "loaded" || s.readyState === "complete") { | |
| s = s.onload = s.onreadystatechange = null; | |
| if (!isAbort) loadCallback(); | |
| } | |
| }; | |
| s.onload = function () { | |
| s.onload = s.onreadystatechange = null; | |
| loadCallback(); | |
| }; | |
| s.onerror = function () { | |
| s.onload = s.onerror = null; | |
| console.error("Failed to load script: " + s.src); | |
| }; |
philippotto
force-pushed
the
skeleton-details-follow-up
branch
2 times, most recently
from
076411d to
b2809d7
Compare
| @@ -929,10 +915,10 @@ class SkeletonTabView extends React.PureComponent<Props, State> { | |||
| <i className="fas fa-arrow-left" /> | |||
| </ButtonComponent> | |||
| <InputComponent | |||
| onChange={this.handleChangeName} | |||
| onChange={() => {}} | |||
There was a problem hiding this comment.
Oh I just saw this.
Optional: Maybe replace () => {} with _.noop
There was a problem hiding this comment.
good point 👍 I also replaced some other occurrences.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
CHANGELOG.unreleased.md (1)
24-24: Fix hyphenation in "double-clicking".For consistency with technical writing standards, "doubleclicking" should be hyphenated.
-- Segment and tree names can be edited by doubleclicking them. [#8316](https://github.com/scalableminds/webknossos/pull/8316) ++ Segment and tree names can be edited by double-clicking them. [#8316](https://github.com/scalableminds/webknossos/pull/8316)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx (1)
918-921: LGTM! Consider keyboard accessibility for the new editing approach.The transition to double-click editing is well-communicated through the tooltip. The use of
_.noopfor the disabled input is a clean approach.Consider ensuring that the new double-click editing functionality is also accessible via keyboard shortcuts for users who rely on keyboard navigation.
frontend/javascripts/oxalis/view/right-border-tabs/bounding_box_tab.tsx (1)
150-150: LGTM! Consider type-safe event handler.The change to use
_.noopimproves code clarity by replacing an empty arrow function. However, since this is TypeScript, we could make it even more type-safe.Consider using a typed version of
noopto match the event handler signature:- onExport={isExportEnabled ? () => handleExportBoundingBox(bb) : _.noop} + onExport={isExportEnabled ? () => handleExportBoundingBox(bb) : (() => void 0) as () => void}This ensures the
noopfunction exactly matches the type signature of the success case. However, this is just a nitpick as the current implementation is perfectly functional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/admin/job/job_hooks.ts(1 hunks)frontend/javascripts/components/pricing_enforcers.tsx(2 hunks)frontend/javascripts/components/slider.tsx(2 hunks)frontend/javascripts/libs/react_hooks.ts(2 hunks)frontend/javascripts/oxalis/view/right-border-tabs/bounding_box_tab.tsx(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (6)
frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx (1)
82-90: LGTM! Clean import organization.The reorganization of imports and movement of tree hierarchy related code to a dedicated location improves code organization.
frontend/javascripts/admin/job/job_hooks.ts (1)
4-4: LGTM! Good improvement in code clarity.Using
_.noopinstead of empty arrow functions for default callbacks makes the intent clearer and follows a consistent pattern. This is a good practice as it provides a standardized way to represent no-op functions.Also applies to: 11-12
frontend/javascripts/components/slider.tsx (1)
4-4: LGTM! Consistent use of _.noop for event handlers.Using
_.noopas the initial value for event handlers is appropriate here as these handlers are reassigned later when needed. This maintains type safety while improving code clarity.Also applies to: 47-48
frontend/javascripts/libs/react_hooks.ts (1)
1-1: LGTM! Clear intention with _.noop.Using
_.noopfor theonClickhandler aligns well with the comment explaining that the handler should do nothing to avoid double-triggering on touch start. This makes the intention even clearer.Also applies to: 136-136
frontend/javascripts/components/pricing_enforcers.tsx (2)
13-13: LGTM! Appropriate use of _.noop for disabled state.Using
_.noopfor theonChangehandler in the disabled state ofPricingEnforcedSwitchSettingis appropriate and makes the intention clear.Also applies to: 170-170
164-165: LGTM! Improved comment formatting.The comment formatting change improves readability while maintaining the same information about the wrapper requirement.
Some screenshots:
When hovering over the old rename-tree field:
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)