Fix bbox export via bbox menu #8152
Conversation
WalkthroughThe changes involve modifications to the Changes
Poem
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
🧹 Outside diff range and nitpick comments (1)
frontend/javascripts/oxalis/view/right-border-tabs/bounding_box_tab.tsx (1)
Line range hint
156-156: Consider adding tooltip for disabled export state.When
isExportEnabledis false, the export functionality silently falls back to a no-op. Consider adding a tooltip to explain why the export option is disabled, improving user experience.Example implementation:
onExport={isExportEnabled ? () => handleExportBoundingBox(bb) : () => {}} tooltipTitle={!isExportEnabled ? "Export is not available for this dataset type" : undefined}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- frontend/javascripts/oxalis/view/right-border-tabs/bounding_box_tab.tsx (1 hunks)
🔇 Additional comments (2)
frontend/javascripts/oxalis/view/right-border-tabs/bounding_box_tab.tsx (2)
93-95: LGTM! Clean implementation of export handler.The simplified implementation correctly handles the bounding box export by directly setting the state and cleaning up the UI.
Line range hint
273-281: Verify the export modal integration.The export flow looks correct, but let's verify the integration with DownloadModalView:
- Modal receives the correct bounding box ID
- Modal opens in export tab mode
- Modal properly cleans up on close
✅ Verification successful
Export modal integration is properly implemented
The verification confirms that the export flow is correctly integrated:
- The
handleExportBoundingBoxfunction properly sets the selected bounding box for export and shows the modal- The
DownloadModalViewreceives all required props:
isOpenfor modal visibilityinitialBoundingBoxIdwith the selected box's IDinitialTab="export"to open in export modeonClosehandler that cleans up by setting selected box to null- The modal includes proper validation through
isBoundingBoxExportableto ensure the export meets size constraintsThe implementation follows a consistent pattern seen in other usages of
DownloadModalViewthroughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for DownloadModalView usage patterns to verify consistent prop handling ast-grep --pattern 'DownloadModalView { isOpen $_ isAnnotation $_ onClose $_ initialBoundingBoxId $_ initialTab $_ }' # Verify export-related prop types and handlers rg -A 5 'type.*DownloadModalView.*Props'Length of output: 172
Script:
#!/bin/bash # Let's try a broader search for DownloadModalView usage and props rg -A 10 "DownloadModalView" # Search for the modal component definition fd -e tsx -e ts "DownloadModalView" # Look for export-related code in the bounding box tab rg -A 10 "handleExportBoundingBox|export.*boundingBox"Length of output: 15872
| @@ -90,7 +90,7 @@ export default function BoundingBoxTab() { | |||
| } | |||
|
|
|||
| function handleExportBoundingBox(bb: UserBoundingBox) { | |||
| _.partial(setSelectedBoundingBoxForExport, bb); | |||
There was a problem hiding this comment.
_.partial generates a new function which is callable and automatically calls setSelectedBoundingBoxForExport with bb as its first arg -> This returned function is never called
The fix simply calls the function
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.unreleased.md (1)
30-30: Enhance the changelog entry with more details.The current entry "Fixed bbox export menu item" is too brief. Consider expanding it to provide more context about the bug and its fix, similar to other entries in the changelog.
-Fixed bbox export menu item [#8152](https://github.com/scalableminds/webknossos/pull/8152) +Fixed an issue where the bounding box export functionality was not working correctly when triggered via the bbox menu. Users can now successfully export bounding boxes using the menu entry. [#8152](https://github.com/scalableminds/webknossos/pull/8152)
For report details see: https://scm.slack.com/archives/C5AKLAV0B/p1730111734807479
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
New Features
Bug Fixes