feat(settings): two-layer takeover shell, migrate pages (PR1 of #604)#951
Merged
Conversation
Replace the old tab-router settings-page with a two-layer takeover shell (240px left nav + content region). Migrate the six existing pages by reuse: general/shortcuts/worktrees/memory wrap the existing settings-* components, and providers+models merge into a single "Models" page that stacks the existing SettingsProviders + SettingsModels. Remote and Integrations pages are scaffolded but hidden from the nav until their real content lands; their connection management stays available in the right-panel Connections for now. Fix Escape not closing settings: the old shell only ever closed via its Close button because a global keybind consumed Escape in the bubble phase. The new shell listens in the capture phase and defers to any open dialog, so Esc reliably closes the surface without dismissing a nested provider dialog. Migrate the settings tests to the new shell: the providers spec targets the Models tab, the smoke test asserts the surface region instead of a page h1, the memory contract test reads settings-shell, and a new settings-shell spec/snap locks nav items, page switching, and close behavior. Also retire the dead notification/sound tests left by #923 (those controls were collapsed into a single notify tri-state) and fix the theme-migration assertion for the current cache-overwrite behavior. Part of #604.
Contributor
|
Warning Review limit reached
More reviews will be available in 27 minutes and 43 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 (32)
📝 WalkthroughWalkthroughThis PR migrates the legacy ChangesSettings Shell Migration & Tab Reorganization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/components/rate-limit-card-wiring.tsx, packages/app/src/components/settings-memory.test.ts, packages/app/src/components/settings-page.tsx, packages/app/src/context/shell-surface.tsx, packages/app/src/i18n/en.ts, packages/app/src/i18n/zh.ts, packages/app/src/pages/layout.tsx, packages/app/src/pages/layout/shell-navigation.ts, packages/app/src/pages/settings/integrations.tsx, packages/app/src/pages/settings/models.tsx, packages/app/src/pages/settings/remote.tsx, packages/app/src/pages/settings/settings-shell.tsx)).
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.
There was a problem hiding this comment.
Code Review
This pull request replaces the old settings page with a new SettingsShell component, introducing a two-column layout with a left navigation bar and a right content area. It merges the "Providers" and "Models" tabs into a single "Models" tab, simplifies notification and sound settings into a single tri-state selector, and adds placeholders for future "Remote access" and "Integrations" tabs. Feedback focuses on fixing a critical SolidJS memory leak in SettingsShell caused by registering onCleanup inside onMount, and replacing flaky page.waitForTimeout() calls in E2E snapshot tests with robust assertions on DOM visibility.
Perf delta summaryComparator: pass
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/app/e2e/settings/settings.spec.ts (1)
450-457: ⚡ Quick winUse a direct localStorage read here instead of
expect.poll(...).For this persisted-settings path, writes are synchronous after the interaction; polling can hide write regressions.
Based on learnings: “In this repo’s Playwright Chromium E2E tests… `localStorage` write happens synchronously on `setStore`… Don’t recommend wrapping these `localStorage` assertions with `expect.poll()` for this storage path.”Suggested fix
- await expect - .poll(async () => { - return await page.evaluate((key) => { - const raw = localStorage.getItem(key) - return raw ? JSON.parse(raw) : null - }, settingsKey) - }) - .toMatchObject({ notify: "always" }) + const stored = await page.evaluate((key) => { + const raw = localStorage.getItem(key) + return raw ? JSON.parse(raw) : null + }, settingsKey) + expect(stored).toMatchObject({ notify: "always" })🤖 Prompt for 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. In `@packages/app/e2e/settings/settings.spec.ts` around lines 450 - 457, Replace the expect.poll usage with a direct synchronous read of localStorage after the interaction: call page.evaluate passing settingsKey to read localStorage.getItem(key), JSON.parse it (or return null) and then assert the result matches { notify: "always" } (same shape as the current toMatchObject). Update the block that currently uses expect.poll(...) in settings.spec.ts to perform the immediate page.evaluate(localStorage read) and assert its return value, referencing the existing settingsKey and the page.evaluate call used in the diff.
🤖 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/e2e/settings/settings-shell.spec.ts`:
- Around line 33-38: The test currently only checks for
'[data-component="settings-page"]' which can miss cases where openSettings
returned a dialog; update the assertions to verify the actual returned locator
`settings` closes after Escape/back, e.g. use Playwright expectations on the
`settings` locator such as expect(settings).toBeHidden() or
expect(settings).toBeDetached() (or both) instead of checking the generic
selector; apply the same change for the other case where `openSettings` is used
(the block around lines 43-48).
In `@packages/app/e2e/snap/settings-shell.snap.ts`:
- Around line 21-23: Replace the brittle sleep with a locator-based wait: after
calling settings.getByRole("tab", { name: tab }).click(), remove
page.waitForTimeout(300) and instead await a specific UI element that indicates
the tab finished rendering (e.g., the tabpanel or a unique content element)
using locator.waitFor() or Playwright expect(locator).toBeVisible(); then take
the screenshot (shots.push with settings.screenshot()). Update the code around
settings.getByRole("tab", { name: tab }).click(), settings.screenshot, and the
shots array to use that locator-based wait.
In `@packages/app/src/pages/layout.tsx`:
- Line 2458: SettingsShell is being mounted without the directory prop so
SettingsMemory receives undefined; update the SettingsShell invocation to pass
the current directory (e.g., directory={directory}) so SettingsMemory keeps
project-scoped behavior, ensuring the surrounding component uses the correct
variable (directory or currentDirectory) in scope and that the SettingsShell
prop signature accepts directory alongside active={settingsTab()}
onSelect={setSettingsTab} onClose={closeSettings}.
---
Nitpick comments:
In `@packages/app/e2e/settings/settings.spec.ts`:
- Around line 450-457: Replace the expect.poll usage with a direct synchronous
read of localStorage after the interaction: call page.evaluate passing
settingsKey to read localStorage.getItem(key), JSON.parse it (or return null)
and then assert the result matches { notify: "always" } (same shape as the
current toMatchObject). Update the block that currently uses expect.poll(...) in
settings.spec.ts to perform the immediate page.evaluate(localStorage read) and
assert its return value, referencing the existing settingsKey and the
page.evaluate call used in the diff.
🪄 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: c3ee1828-6e3d-4ed8-b606-53f2be23071b
📒 Files selected for processing (18)
packages/app/e2e/selectors.tspackages/app/e2e/settings/settings-providers.spec.tspackages/app/e2e/settings/settings-shell.spec.tspackages/app/e2e/settings/settings.spec.tspackages/app/e2e/snap/settings-shell.snap.tspackages/app/e2e/snap/settings-sounds.snap.tspackages/app/src/components/rate-limit-card-wiring.tsxpackages/app/src/components/settings-memory.test.tspackages/app/src/components/settings-page.tsxpackages/app/src/context/shell-surface.tsxpackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/layout.tsxpackages/app/src/pages/layout/shell-navigation.tspackages/app/src/pages/settings/integrations.tsxpackages/app/src/pages/settings/models.tsxpackages/app/src/pages/settings/remote.tsxpackages/app/src/pages/settings/settings-shell.tsx
💤 Files with no reviewable changes (3)
- packages/app/e2e/snap/settings-sounds.snap.ts
- packages/app/e2e/selectors.ts
- packages/app/src/components/settings-page.tsx
Settings now renders into the existing shell slots: the nav fills the sidebar slot and the page fills the main slot, inheriting their width/background/border so there is no alignment drift with the session layout. The session sidebar and main subtree stay mounted but hidden (inert + aria-hidden + invisible) to preserve terminal/scroll state. - Drop Kobalte Tabs (cannot span two slots); hand-roll role=tablist with roving tabindex + arrow/home/end keys and a shared active signal. - Fix settings opening blank: the sidebar button forwarded its click event as the tab arg; openSettingsSurface now validates the tab and the wiring drops the event. - Settings content canvas is bg-base; update the 4 sticky-header gradients from surface-raised to bg-base to match. - Align nav tabs to the session row visuals (px-2, gap-0.5 row gap, row-hover / row-active overlays). - Update e2e helpers/specs and snapshot; remove the close-on-nav spec (path removed by the takeover; covered by the shell-navigation unit test).
Tools always fold by default now; users expand them on demand. The two opt-in settings only ever flipped that default, so drop them along with their full plumbing: - settings schema entries, defaults, and getter/setters - the two settings-general rows and their i18n strings - the shellToolDefaultOpen/editToolDefaultOpen props threaded through AssistantParts, session-turn, message-timeline, and the playground - the snapshot fixture scenarios and perf-probe scenario that existed solely to exercise the removed setting Part of the settings rewrite (#604).
The settings toggle controlled auto-accept for the current session/directory, yet lived in global settings, so it sat inert whenever settings opened without an active session. The same control already exists in-session as the mod+shift+a command, making the settings row a confusing duplicate. - remove the settings row and its permission/params/decode64 plumbing - remove the orphaned permissions.autoApprove settings schema (no readers) - migrate the two e2e tests that enabled auto-accept through the toggle to press the command keybind instead; drop the switch-only "toggle works before first submit" test, whose logic stays covered by the permission-auto-respond unit tests Part of the settings rewrite (#604).
The settings takeover mounted SettingsContent without a directory, so SettingsMemory received undefined and created its SDK client with no project scope, losing project-scoped memory behaviour. Forward currentDir(). Addresses review feedback on #951.
- snap: wait for each tab's content to render instead of a fixed 300ms delay - spec: assert the locator returned by openSettings actually closes, rather than a hard-coded settings-page selector that passes even on a dialog fallback Addresses review feedback on #951.
remote and integrations were valid SettingsTab values and listed in
TAB_VALUES, yet SettingsContent has no content branch for them, so a
programmatic openSettings("remote") landed on an empty settings page.
They have no page yet, so remove them from both the type and TAB_VALUES
(isSettingsTab now rejects them and the open path falls back to general).
They return — type, TAB_VALUES, NAV_ITEMS and a content Match together —
when their pages land.
Addresses review feedback on #951.
…shell-content openSettings returns the shell-content slot ancestor, which also hosts the session view and never detaches, so expect(settings).toBeHidden() could never hold. The close tests are not @smoke-tagged, so PR CI (smoke-only) never ran them and the broken assertion went unnoticed. Key the close assertion on settings-page (mounted only while settings is open) like closeSettingsPanel.
PR CI runs the smoke suite only (--grep @smoke). The PR1 foundation-lock spec had no @smoke tag, so its nav-takeover / tab-switch / close tests never ran on PRs and could not guard the shell. Tag all three so the lock is real; they are local-only UI (no network), ~6s total, sharing the already-booted smoke worker.
…down is open The Escape capture listener runs before Kobalte's bubble-phase dropdown dismissal, so pressing Escape to close a Select (language / notify / theme) tore down the whole settings shell. Bail while a select-content layer is open (alongside dialog-overlay) so the popover consumes Escape first. Adds an e2e guard and registers it in the smoke inventory.
This was referenced Jun 1, 2026
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
PR1 of the #604 settings rewrite: replace the old tab-router settings page with the new two-layer takeover shell. Every existing page moves over by reuse — the surface is migration-first, with two deliberate settings removals called out below.
This is the "swap the foundation" PR — atomic by necessity. The moment layout mounts the new shell the old one is gone, so all six existing pages and their tests must move in the same change. Visual polish and new functionality land as follow-up additive PRs.
Intentional removals (not pure migration)
The migration preserves behaviour except for two settings we chose to drop here rather than carry over:
mod+shift+acommand, so the redundant (and confusing) settings entry is removed.general.shellToolPartsExpanded/editToolPartsExpandedsettings and their timeline plumbing (SessionTurn/AssistantParts) are removed.Upgrade impact: a user who had turned on "default-expand" will see those tool parts fold by default after upgrade — the stale localStorage values are ignored rather than migrated. This is the intended new default, not a regression we plan to undo.
Done
settings-shell.tsx: two-layer takeover, 240px left nav (Back-to-app + General / Shortcuts / Models / Worktrees / Memory + version foot).settings-*components; providers+models merge into a single Models page stackingSettingsProviders+SettingsModels.settings-page.tsx.SettingsMemorykeeps its project scope.Tests
mod+shift+akeybind instead of the removed settings toggle.Not in this PR (follow-up additive PRs)
Refs #604.
Summary by CodeRabbit