fix(sidenav): align indicators and preserve width across Settings - #371
Conversation
Align row indicators and section controls, fade overflowing section names, and enforce a 220px minimum sidebar width. Keep Settings width consistent across navigation, narrow windows, and storage failures, and retain personal Profile access. Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review (via Wes’s account)
Reviewed head 279d6fc900823ea6bb6a16e39113d4da2b099481 against base 5ce7836b197fc4f19b9bd4c23d3f8cd56495df3b. This is a COMMENT, not approval or merge authorization.
Findings
Two actionable findings are attached inline: the narrow overlay does not close for channel/Settings-section navigation, and the Settings test migration removes the held-publication keyboard/failure/retry regression journey without equivalent browser coverage. The latter is a coverage-contract regression, not a claim that profile publication is currently broken.
Assessment: minimalness 9/10, elegance 9/10, correctness 8/10 pending the drawer repair and preservation of the affected recovery contract. Width clamping/session fallback and extraction of the existing fading-label behavior reuse existing owners appropriately. Groot’s independent navigation/accessibility source review was reconciled against the same snapshot.
Optional improvements
- Preserve a screen-reader-accessible level-one Settings heading.
Settings.module.css:104–106hides the old sidebar including theh1inSettings.tsx:182; the replacement sidebar has grouph2s and the detail headers default toh2. The region still has its Settings name, so this is heading-navigation polish, not an unnamed-region defect. Ansr-onlyheading in the detail pane would preserve that landmark in heading traversal. - Update
docs/shell-design.mdto describe the Settings replacement sidebar and all-page narrow drawer; its persistent-channel-sidebar/Settings-only disclosure descriptions now contradict the implementation.
Evidence and limits
- Read the full 37-file diff, relevant owners/callers, repository instructions and design/vision documents. Reviewed success/cancel separately from error/retry/focus paths. The retained real-wheel Settings journey at
tests/browser/layout.spec.mjs:640–680addresses representative wheel input; the 200% case is explicitly programmatic, as the description discloses. - Reviewed the public description, all six attached images and all six commit messages. No additional public-material disclosure finding in those inputs or the changed source/configuration/fixtures. The synthetic browser images compare
3a19fa43toe1060411, not this head. - Isolated exact-head source archive: 1,649 Git blobs verified, then SHA-256 contents rechecked unchanged; no dirty source inputs or live-checkout changes.
- Source-only: no PR code, tests, builds, installs or app launches executed. One hosted CI snapshot for run 36493396900 reports JavaScript, Rust/tool integration, all six Chromium/WebKit journey shards, browser measurements and CI required passing at this head; Windows native validation was skipped. Author-reported local checks were not independently rerun. Green CI does not establish the missing behavior.
- Current-head native geometry, browser focus/recovery behavior and human acceptance remain unverified. The description itself leaves final native captures/recording and human visual acceptance outstanding; this review does not attest the readiness checklist or
buzz-review-completed.
Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
…sh-followup * origin/main: Add message-level read and unread controls (#352) feat: add per-category notification alert sounds with app-owned playback (#356) test(relay): stabilize per-channel replay boundary coverage (#378) fix(design-system): keep button labels single-line and corners capsule-shaped (#357) Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No blocking findings at 0c7356f0d5591e541f60591a4dd2249c9a1e8a98. This is a follow-up COMMENT, not approval or merge authorization. Minimalness, elegance and correctness: 9/10 each.
- Both prior P2 findings are resolved: navigation attempts close the phone drawer and hand off focus, including Settings-section reselection; the gated profile publication failure/keyboard-retry journey is restored. The accessible Settings heading and shell documentation are also corrected. Independent navigation and profile/persistence source-review lanes agree.
- Independently ran the complete
settings.spec.mjsandsettings-sidebar-width.spec.mjsfiles in Chromium and WebKit on macOS at this clean head: 16/16 passed (20.6s). This covers drawer focus, width persistence/reload/storage failure and profile rejection/retry. One optional P3 remains inline: restore the distinct-local-default reload guard. It is a coverage weakness, not a demonstrated runtime defect. - Current-head hosted CI is still in progress; native visual acceptance and the final native captures/recording remain outstanding as disclosed in the description. Browser checks do not certify those. Reviewed the full base-to-head diff, description, attached media and commit text for publication hygiene; no internal-information or accidental committed review-artifact finding in those materials.
Code review is non-blocking. Finish hosted checks and the disclosed human/native acceptance before claiming the readiness checklist complete.
| await page.getByRole("menuitem", { name: "Settings", exact: true }).click(); | ||
| await expect( | ||
| page.getByRole("menu", { name: "Updated community profile" }), | ||
| page.getByRole("menu", { name: "Updated local profile" }), |
There was a problem hiding this comment.
[P3, optional] Restore the distinct local-default reload guard
The migrated test no longer changes the saved local profile before reload. A successful community save also updates that local default (src/app/ProfileSettings.tsx:270–277), so both now contain “Updated community profile”: the reload assertions can pass even if Settings/the header incorrectly use the local seed instead of confirmed community state. The base deliberately set saved.profile.name = "Local default only" before reloading to distinguish them.
Please restore that injection and check the actual menu name here (Updated community profile). Updated local profile is not supplied anywhere else in the current src/tests inputs, so this hidden assertion currently passes without proving dismissal. This is lost regression coverage, not evidence that runtime profile loading is broken.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review (via Wes’s account)
Reviewed head 0c7356f0d5591e541f60591a4dd2249c9a1e8a98 against base 423f7215e124aa68b828c85f9040703a3fe9913d. This is a COMMENT, not approval or merge authorization.
Follow-up result
Both prior P2 findings are addressed in source; no new actionable finding in the repairs. This follow-up preserves the previous review’s exit criteria rather than reopening unrelated portions of the change.
- Narrow drawer navigation:
App.tsx:157supplies the existing navigation-attempt ID toAppShell.tsx:56–62. The controller starts a fresh attempt for channel/Settings-section selection, reselection and ordinary retry, while in-place target normalization retains the ID. The drawer therefore closes on navigation, not on unrelated resolution updates. Focus is handed to the main region only when the narrow toggle is visible; desktop section focus is not changed by this effect. Reviewed the supported navigation callers, failure/retry presentation and composer mount-focus ownership.settings-sidebar-width.spec.mjs:76–116now asserts channel closure and typing, Settings-section closure and main focus, and same-section reselection. The shared helpers no longer hide the drawer on the implementation’s behalf. - Profile publication coverage:
settings.spec.mjs:408–573restores an explicit request gate, pending busy/disabled assertions, repeated Enter, rejection with retained Save focus, keyboard retry, success/Cancel focus recovery and exactly two publication requests. Both held requests are released infinally. These assertions match the existing form, publication receipt and focus-cleanup owners; this resolves the earlier coverage-contract finding, not a claim of newly repaired publication code. - The previously optional accessible Settings heading and shell-documentation corrections are also present.
Source assessment: minimalness 9/10, elegance 9/10, correctness 9/10. The repair reuses the navigation owner rather than adding another route tracker. Groot’s bounded independent navigation/accessibility lane is complete and reconciled against this snapshot.
Evidence and limits
- Reviewed the follow-up changes and affected owners/callers against repository instructions, contribution/test-layer rules, shell design and the relevant Buzz vision documents. Success/cancel and error/retry focus paths were considered separately.
- Inspected the public description, all six attached images, all twelve PR commit messages and the changed publication surface. No additional disclosure finding in those reviewed inputs. The synthetic images compare
3a19fa43toe1060411, not this head. - Isolated exact-head archive: 1,679 Git blobs verified, then SHA-256 contents rechecked unchanged; no dirty source inputs or live-checkout changes.
- Source-only: no PR code, tests, builds, installs or app launches executed. The single hosted CI snapshot for run 36501498676 showed browser measurements, DCO, Semgrep and zizmor passing; JavaScript, Rust/tool integration and all six browser-journey shards were still in progress. Windows native validation was skipped. This is a snapshot, not a current CI-green claim; author-reported local runs were not independently rerun.
- Current-head browser focus/recovery behavior, native geometry and human acceptance remain independently unverified. The description still leaves final native captures/recording and human visual acceptance outstanding. This review does not attest the readiness checklist or
buzz-review-completed.
Why
The sidenav allowed unread dots to sit outside row highlights, clipped section names, and changed width when opening Settings. Section controls and row highlights also had inconsistent spacing.
What
How
Reuse the existing sidebar width clamping and community/viewer storage scope. Share the existing overflow-label component between rows and section names. Keep channel labels full-width unless a DM name accessory needs space.
Risk
This touches app navigation, shared shell sizing, and sidebar geometry. At windows no wider than 650px, navigation opens as a 220px drawer over the content and starts collapsed on destination changes. If storage fails, width recovery lasts only for the current app session.
Testing
Current locally checked commit:
0c7356f0(hosted CI pending). Previous hosted green commit:279d6fc9. Original polish checks below were ate1060411.CI follow-up
Hosted CI passed at
279d6fc9: all six Chromium/WebKit journey shards, JavaScript, Rust/tool integration, and browser measurements. Hosted DCO passed; Windows native validation was skipped by the workflow.Migrated Settings journeys to the replacement sidebar, its Back action, and drawer controls. The pending-modal test now exercises the channel drawer directly; its uncertain-publication, Escape, modal-inertness, and focus assertions remain.
Extended the width journey to check that opening the phone drawer does not shrink the content and that desktop width is restored. Existing 320px/200% text-size coverage exercises pointer access to Settings controls. No browser cases were added or removed in this follow-up.
Accounted for the exact native scrollbar gutter in the row-fill assertion and waited for applied resize geometry without changing tolerances. Restored the real wheel-scroll journey against the current Settings scroll container.
Local Chromium/WebKit batch: 165/170 passed before the final corrections. Full affected-file reruns then passed 64 cases, followed by all 10 Settings cases after updating the narrow Back assertion. These reruns cover all failures from that batch. Commands used
pnpm test:browser <affected files> --project chromium --project webkit --no-depson macOS; broad hosted CI remains authoritative for Linux.After integrating main and resolving the agent-activity test overlap, all 16 agent-activity/channel-leave browser cases passed in Chromium and WebKit at
bd1af890.The startup-style journey now opens the phone drawer before inspecting navigation controls and closes it afterward; all styling assertions remain. The full file passed in Chromium and WebKit (2 cases, 6.4 seconds) at
279d6fc9. The preceding hosted run had only this stale-test failure in each engine; its other shards passed.At the pushed head, mandatory TypeScript, 71 related unit tests, and all design guards passed. All PR commits have DCO sign-offs.
The original JavaScript failure was a timeout in unchanged relay replay coverage; it is tracked independently in Emergency main fix: bound live replay test runtime #372.
Review follow-up
finally; final request counts guard duplicate publication.settings.spec.mjs,notification-settings.spec.mjs, andplugin-import.spec.mjspassed in Chromium and WebKit on macOS (32.0 seconds) at9b114052. No cases added or removed.8b9ff420). Coverage includes keyboard channel switching and typing, Settings-section/reselection focus, width restoration, and existing profile publication failure/retry. No browser cases added or removed.0c7356f0.Before and after
Matched WebKit fixture captures using synthetic data, light mode, a 1200×800 viewport, and 2× pixel density. Before: base
3a19fa43; after:e1060411. These illustrate browser layout; native visual acceptance remains outstanding.Unread row — 260px sidebar
Same Alice Fixture row, hovered in both captures.
Section label and actions — 220px sidebar
Same long section label, with its actions revealed in both captures.
Opening Settings — 360px sidebar
Both captures start with the sidebar resized to 360px before opening Settings. The updated Settings navigation occupies that same sidebar width.
Remaining before ready for review
Generated with Codex