Fix layout persistence when switching between orthogonal, flight, and oblique mode #8177
Conversation
WalkthroughThe pull request introduces several enhancements and fixes to the WEBKNOSSOS platform. Key features include the addition of metadata to annotations for Trees and Segments, improved slider functionality, and expanded search capabilities for unnamed segments. The changelog has been updated to reflect these changes, along with bug fixes related to dataset uploads and sharing tokens. Additionally, modifications have been made to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx
Outdated
Show resolved
Hide resolved
![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 (4)
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx (1)
Line range hint
1-450: Consider architectural improvements for layout management.While the immediate issue is fixed, consider the following improvements to make the layout management more robust and maintainable:
- Extract layout persistence logic into a dedicated service/hook to reduce complexity in the component
- Create a utility function for layout type determination to avoid repetition
- Add error handling for layout persistence failures
Example structure:
// useLayoutManager.ts export function useLayoutManager(commandType, viewMode, is2d) { const layoutType = determineLayout(commandType, viewMode, is2d); const saveLayout = async (model, layoutName) => { try { await storeLayoutConfig(model, layoutType, layoutName); } catch (error) { // Handle persistence failure } }; return { layoutType, saveLayout, // ... other layout management methods }; }frontend/javascripts/oxalis/view/layouting/flex_layout_wrapper.tsx (3)
210-215: LGTM! This change fixes the layout persistence issue.The use of setState callback ensures that
onLayoutChangeis called only after the model state has been fully updated. This fixes the timing issue that was causing empty viewports when switching between orthogonal, flight, and oblique modes.This pattern is particularly important for layout updates where the order of operations is critical. Consider documenting this requirement in a code comment to prevent future regressions.
Line range hint
401-404: Consider addressing TypeScript type issues.The code uses
@ts-ignoreto suppress TypeScript errors. While this works, it would be better to properly type these instances.For example, replace:
// @ts-ignore Typescript doesn't infer the type of side to "left" | "right" but only string, instead ([side, isOpen]: [BorderOpenStatusKeys, boolean])with:
([side, isOpen]: [keyof BorderOpenStatus, boolean])
Line range hint
279-282: Add error handling for missing viewport IDs.The error case for unknown viewport IDs only logs to console. Consider adding proper error handling or fallback behavior.
default: { - console.error(`The settings tab with id ${id} is unknown.`); - return null; + console.error(`Unknown viewport ID: ${id}`); + return <div className="error-viewport">Invalid Viewport Configuration</div>; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/layouting/flex_layout_wrapper.tsx(1 hunks)frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx(1 hunks)
🔇 Additional comments (4)
CHANGELOG.unreleased.md (1)
44-44: LGTM! Well-documented changelog entry.
The changelog entry accurately describes the bug fix, follows the proper format, and aligns with the PR objectives. The description clearly communicates both the issue and the conditions that trigger it.
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx (2)
197-206: LGTM! Fixed layout persistence issue.
The change correctly addresses the layout persistence bug by ensuring saveCurrentLayout is called only after the state update is complete. This fixes the issue where switching between viewing modes (orthogonal, flight, oblique) resulted in empty viewports.
Previously, saveCurrentLayout accessed stale state since it was called before the state update was processed. The new implementation using setState callback guarantees that the latest layout is saved.
197-206: Verify layout persistence implementation.
Let's verify that the layout persistence implementation is complete and consistent across the codebase.
✅ Verification successful
Based on the shell script results, I can now provide a final response:
Layout persistence implementation verified successfully
The implementation shows consistent patterns across the codebase:
- Layout saving is properly handled through
saveCurrentLayoutwith correct state management determineLayoutis consistently used to determine layout types based on command type, view mode, and dimensionality- The setState callback ensuring layout persistence after state update is correctly implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify layout persistence implementation
# Test 1: Check for other instances where layouts might be saved without proper state handling
echo "Checking for potential layout persistence issues..."
rg -A 2 "saveCurrentLayout|storeLayoutConfig" --type ts
# Test 2: Verify layout type determination consistency
echo "Checking layout type determination usage..."
ast-grep --pattern 'determineLayout($_, $_, $_)'
# Test 3: Check for proper setState usage with callbacks
echo "Checking setState usage patterns..."
ast-grep --pattern 'setState($_, $_)'
Length of output: 5435
frontend/javascripts/oxalis/view/layouting/flex_layout_wrapper.tsx (1)
210-215: Verify similar setState patterns in related layout components.
Let's check if other layout-related components follow the same pattern for state updates.
✅ Verification successful
The setState pattern with onLayoutChange callback is consistent with the codebase
The verification shows that:
- The setState pattern in flex_layout_wrapper.tsx is properly integrated with the layout system
- The onLayoutChange callback is consistently used across layout components
- The change is part of a well-structured layout persistence mechanism where:
- TracingLayoutView manages the high-level layout state
- FlexLayoutWrapper handles layout model updates with proper state management
- Changes are properly propagated through the component hierarchy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for setState calls in layout-related files to ensure consistent patterns
echo "Searching for setState patterns in layout files..."
rg -l 'setState.*=>' frontend/javascripts/oxalis/view/layouting/
Length of output: 177
Script:
#!/bin/bash
# Let's try a more comprehensive search for setState patterns
# First, let's see what files exist in the layouting directory
echo "Files in layouting directory:"
fd . frontend/javascripts/oxalis/view/layouting/
echo -e "\nSearching for setState patterns:"
# Search for setState with context to see the actual usage
rg "setState" frontend/javascripts/oxalis/view/layouting/ -A 3 -B 1
echo -e "\nSearching for onLayoutChange patterns:"
# Also search for onLayoutChange since it's related to the layout update
rg "onLayoutChange" frontend/javascripts/oxalis/view/layouting/ -A 2 -B 2
Length of output: 8921
frontend/javascripts/oxalis/view/layouting/flex_layout_wrapper.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx
Outdated
Show resolved
Hide resolved
On master, switching between the modes reliably leads to empty viewports, because the wrong layout is chosen (e.g. the orthogonal layout, although flight mode is active)
URL of deployed dev instance (used for testing):
Steps to test:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes