fix(ui): persist sidebar open state and width to prevent flash on load #12743
Conversation
…retrieval to ensure consistency with current state management
|
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
WalkthroughThe initialization of sidebar-related LiveData observables in both the app sidebar and workbench modules was modified. Instead of always using hardcoded default values, the observables now initialize from the persisted state if available, falling back to the previous defaults only if no stored value exists. Additionally, a new test suite was added for the Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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)
packages/frontend/core/src/modules/workbench/entities/workbench.ts (3)
61-62: Handle undefined emissions insidebarOpen$watcher
The fallback?? falseapplies only to the initial value. IfglobalState.watchever emitsundefined, it will pass through. Wrap the watcher in.pipe(map(value => value ?? false))to ensure every emission falls back tofalse.
67-69: Guard against undefined watcher values for width
Similar tosidebarOpen$, consider adding.pipe(map(value => value ?? 320))on the watcher stream to apply the fallback on every update.
75-77: Ensure fallback for workspace selector watcher
Add.pipe(map(value => value ?? false))to the watcher so that anyundefinedemissions are converted back tofalse, maintaining consistency after the initial load.packages/frontend/core/src/modules/app-sidebar/entities/app-sidebar.ts (2)
50-53: AligngetCachedAppSidebarOpenStatewithopen$default
getCachedAppSidebarOpenStatereturns the raw stored value, which may beundefined. Consider using the same fallback (?? true) here to avoid exposingundefined.
58-61: Remove redundant return statement
The explicitreturn;insetOpenis unnecessary. You can simplify the method by omitting it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/frontend/core/src/modules/app-sidebar/entities/app-sidebar.ts(1 hunks)packages/frontend/core/src/modules/workbench/entities/workbench.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
packages/frontend/core/src/modules/app-sidebar/entities/app-sidebar.ts
[error] 27-27: Finnish notation should not be used here.
(rxjs/finnish)
packages/frontend/core/src/modules/workbench/entities/workbench.ts
[error] 66-66: Finnish notation should not be used here.
(rxjs/finnish)
[error] 74-74: Finnish notation should not be used here.
(rxjs/finnish)
🔇 Additional comments (5)
packages/frontend/core/src/modules/workbench/entities/workbench.ts (3)
59-62: Restore persisted sidebar open state
InitializingsidebarOpen$withthis.globalState.get(...) ?? falseensures the sidebar’s last state is used on load, preventing the flash. This aligns perfectly with the PR objective to persist UI state.🧰 Tools
🪛 ESLint
[error] 59-59: Finnish notation should not be used here.
(rxjs/finnish)
66-69: Restore persisted sidebar width
ThesidebarWidth$LiveData now correctly reads fromglobalStatewith a320fallback, matching the stored user preference on startup.🧰 Tools
🪛 ESLint
[error] 66-66: Finnish notation should not be used here.
(rxjs/finnish)
74-77: Restore persisted workspace selector state
InitializingworkspaceSelectorOpen$with the persisted value and falling back tofalsemeets the requirement to persist UI state across loads.🧰 Tools
🪛 ESLint
[error] 74-74: Finnish notation should not be used here.
(rxjs/finnish)
packages/frontend/core/src/modules/app-sidebar/entities/app-sidebar.ts (2)
20-25: Persist sidebar open state with fallback mapping
Using.watch<boolean>(...).pipe(map(value => value ?? true))alongside an initial fallback correctly reads stored state and handlesundefinedon every emission.🧰 Tools
🪛 ESLint
[error] 20-20: Finnish notation should not be used here.
(rxjs/finnish)
27-32: Persist sidebar width with fallback mapping
Thewidth$LiveData now uses.watch<number>(...).pipe(map(value => value ?? 248))and an initial fallback, ensuring both initial and subsequent undefined emissions map to248.🧰 Tools
🪛 ESLint
[error] 27-27: Finnish notation should not be used here.
(rxjs/finnish)
… ensure correct functionality and state management
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/frontend/core/src/modules/app-sidebar/entities/__tests__/app-sidebar.spec.ts(1 hunks)
🔇 Additional comments (3)
packages/frontend/core/src/modules/app-sidebar/entities/__tests__/app-sidebar.spec.ts (3)
1-18: LGTM! Solid test setup for sidebar persistence testing.The test setup correctly initializes the Framework with MemoryMemento to mock storage behavior, which aligns perfectly with testing the persistence fixes mentioned in the PR objectives.
20-25: LGTM! Default values test validates fallback behavior.This test correctly verifies that when no stored state exists (fresh MemoryMemento), the sidebar falls back to the expected default values (open: true, width: 248). This aligns with the PR's goal of reading from persistent storage with fallback defaults.
50-53: LGTM! Cached state test validates correct retrieval.The test correctly verifies that
getCachedAppSidebarOpenState()returns the current cached state value, ensuring the persistence mechanism works as expected.
packages/frontend/core/src/modules/app-sidebar/entities/__tests__/app-sidebar.spec.ts
Show resolved
Hide resolved
|
cool |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## canary #12743 +/- ##
==========================================
- Coverage 55.94% 55.63% -0.32%
==========================================
Files 2653 2655 +2
Lines 125303 125319 +16
Branches 19898 19834 -64
==========================================
- Hits 70105 69722 -383
+ Misses 53450 53290 -160
- Partials 1748 2307 +559
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Please run CI test. |
persist-sidebar-open-state.mp4
This PR change updates the sidebar initialization so it reads the open and width values from persistent storage. This prevents the UI from flashing open by default on startup.
Both left and right sidebars restore correctly from the last known state from storage.
Summary by CodeRabbit