refactor(app): extract layout shell frame - #1042
Conversation
|
Warning Review limit reached
More reviews will be available in 54 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR refactors the desktop shell architecture by introducing a new ChangesShell architecture refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/pages/layout.tsx, packages/app/src/pages/layout/layout-commands.test.ts, packages/app/src/pages/layout/layout-commands.ts, packages/app/src/pages/layout/layout-shell-frame.tsx, packages/app/src/shell-frame-contract.test.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the desktop layout by extracting the shell frame JSX into a new LayoutShellFrame component and decoupling the layout commands registration from specific context hooks via structured action callbacks. It also adds unit tests for the layout commands. The review feedback correctly points out a potential ReferenceError when accessing window directly during render in LayoutShellFrame, which could cause crashes in SSR or test environments, and suggests a defensive check.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/src/pages/layout/layout-shell-frame.tsx`:
- Around line 37-41: Compute a single normalized sidebar width and reuse it
everywhere instead of calling props.sidebar.width() directly: create a
normalized value that clamps the visible width between props.sidebar.minWidth
and props.sidebar.maxWidth (and yields 0 when props.sidebar.visible() is false),
use that normalized value to set the CSS variable "--dialog-left-margin" and to
provide the createMemo "side" value so all geometry paths share the same clamped
width and cannot overflow when restoring persisted state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 499f6be7-615f-41fa-af00-d148fe585312
📒 Files selected for processing (7)
packages/app/src/pages/layout.tsxpackages/app/src/pages/layout/layout-commands.test.tspackages/app/src/pages/layout/layout-commands.tspackages/app/src/pages/layout/layout-shell-frame-debug.tspackages/app/src/pages/layout/layout-shell-frame.test.tspackages/app/src/pages/layout/layout-shell-frame.tsxpackages/app/src/shell-frame-contract.test.ts
1 participant
Summary
Refactors the next layout governance slice after #1036:
registerLayoutCommandsinputs into command-facing capabilities and actions instead of passing a broad layout-owned parameter bag.LayoutShellFrame, keeping providers and state owners inlayout.tsx.Why
This continues the
layout.tsxowner extraction tracked by #606. PR #1036 moved command registration out oflayout.tsx; this PR tightens that owner boundary and then moves the render shell into a focused frame component without changing workspace, session, or startup behavior.Related Issue
Related: #606
Human Review Status
Pending
Review Focus
Please focus on whether the new
LayoutShellFrameprops are still a render-shell boundary rather than a leaked copy oflayout.tsxinternals, and whether the command capability groups keep command behavior unchanged.Risk Notes
Visible shell geometry and settings takeover are touched. The PR intentionally does not move workspace CRUD, session window/prefetch ownership, boot effects, or the context providers.
How To Verify
Screenshots or Recordings
Snap grids reviewed locally:
docs/design/preview/screenshots/app-shell.pngdocs/design/preview/screenshots/settings-shell.pngChecklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
New Features
Refactor