feat(desktop): polish sidebar resizing - #7794
klopez4212 wants to merge 1 commit into
Conversation
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🔐 Codex Security Review
|
|
🤖 ## Screenshots RestingThe resize gutter stays visually hidden at rest.
HoverHovering anywhere in the gutter reveals the centered resize affordance.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17f78307bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| onDoubleClick={(event) => { | ||
| onDoubleClick?.(event); | ||
| if (event.defaultPrevented) return; | ||
| setSidebarWidth(SIDEBAR_WIDTH_DEFAULT); | ||
| performSidebarDefaultHaptic(); |
There was a problem hiding this comment.
For keyboard-only users, the new reset action is unreachable because it is implemented only in onDoubleClick, while the rail remains outside the tab order with tabIndex={-1} and there is no keyboard handler or alternate reset control. The added ARIA description therefore advertises an action that assistive-technology users cannot invoke; expose a focusable keyboard path and cover it alongside the pointer test.
AGENTS.md reference: AGENTS.md:L232-L239
Useful? React with 👍 / 👎.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES
Reviewed: 77729abfb692b25a0f4ec4a69add86af2e32c0dd..17f78307bbed928dd5358f6d07e4a981e74624f5 (exact head 17f78307bbed928dd5358f6d07e4a981e74624f5)
Risk: medium — persistent, shared desktop layout and pointer lifecycle changes with app-wide cursor/selection effects.
Blocking findings
-
[P2] The new reset action is pointer-only.
desktop/src/shared/ui/sidebar.tsx:486-498keeps the resize control out of the tab order (tabIndex={-1}) and implements reset only inonDoubleClick, despite advertising “Drag to resize. Double-click to reset.” A keyboard or switch-control user cannot focus the control or restore a persisted non-default width. This conflicts with the repository’s input-modality and interactive-semantics rules (AGENTS.md:222-239). Make the control keyboard-operable with truthful adjustable semantics (current/min/max; arrow adjustment and Home/reset, or at minimum an operable focusable reset path), then add a regression test proving keyboard focus, resize/reset, and persisted width. -
[P2] Interrupted drag can strand global cursor and selection state. Drag start mutates
document.documentElement.style.cursor,document.body.style.userSelect, and providerisResizing(desktop/src/shared/ui/sidebar.tsx:519-533), which activates the app-widehtml[data-sidebar-resizing] *rule (desktop/src/shared/styles/globals/theme.css:144-148). Cleanup only runs from pointer up/cancel (sidebar.tsx:464-480,499-502,566-569); there is no lost-capture or unmount cleanup.AppSidebarcan unmount while its provider remains mounted when AppShell swaps to Settings (desktop/src/app/AppShell.tsx:825-831), leaving selection disabled andcol-resizeforced across the app. Centralize idempotent cleanup independent of a React event; invoke it for pointer up/cancel, lost pointer capture, and teardown (plus blur/visibility if that is the intended native recovery contract). Add a causal regression that begins dragging, forces capture loss/unmount, and verifies capture plus all global/provider state are restored. -
[P1 gate] The PR’s scrollbar change breaks the required smoke suite on all retries. Exact-head job Desktop Smoke E2E (1) fails both Buzz light and dark palette tests:
buzz-theme-screenshots.spec.ts:213still requires the sidebar thumb to equalsearchSurface, whilescrollbars.css:5-26now rendershsl(var(--foreground) / 0.2)(rgba(36,41,46,.2)light /rgba(225,228,232,.2)dark). Resolve the intended theme contract and update implementation/tests together; the required exact-head gate must pass.
Author action: resolve all three findings and provide a new head with focused keyboard and interrupted-drag lifecycle regressions; reconcile the scrollbar palette contract so required smoke CI passes.
Verification owner: author for code/tests and PR-caused smoke failure; reviewer for exact-head re-review and focused/full-package gates; reviewer/tooling for native WKWebView/Windows visual, zoom, and screen-reader follow-up.
Validation
- Confirmed live PR author
klopez4212, authenticated reviewerjedwards27, base/head above, and clean local tree. - PASS:
git diff --check 77729abf..17f78307. - PASS at matching clean HEAD: pinned-Hermit install; Desktop typecheck; 6,508 frontend tests;
pnpm build:e2e. - PASS at matching clean HEAD: focused integration Playwright — sidebar tooltip removal, non-toggle click, drag/persistence/snap/double-click reset, and delayed affordance (4/4). An independent full
sidebar.spec.tsrun passed 23/23. pnpm checkcompleted with pre-existing warnings outside this six-file diff; no changed-file lint finding.- CI: macOS/Windows builds and relay-backed integration passed; exact-head smoke shard 1 failed as described; Desktop Core and smoke shard 4 were still running at final review.
Manual/native evidence: supplied resting/hover screenshots show the hidden gutter and centered hover handle. No exact-head native drag interruption, keyboard/AX, zoom matrix, or cross-platform scrollbar journey was independently recorded.
Residual risk: custom-status shortcodes are now hidden while the shared query is pending and fall back after resolution, but this diff adds no delayed-query runtime regression. Native scrollbar appearance and app-level blur/capture-loss behavior remain to be exercised after the code defects are fixed.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested
P2: Make the new sidebar reset action keyboard-operable
desktop/src/shared/ui/sidebar.tsx:489–498 advertises reset but implements it only in onDoubleClick, while retaining tabIndex={-1}. The complete rail has no keyboard reset handler, and its production caller renders <SidebarRail /> without an alternative handler (AppSidebar.tsx:959). The provider’s existing shortcut only toggles visibility, not width.
Impact / source-derived reproduction: start with a non-default persisted sidebar width, then use only Tab/Enter/Space. The rail is skipped in tab order, and keyboard activation does not dispatch dblclick, so the newly introduced reset action cannot restore the default width without a pointer. An ARIA description does not provide the missing interaction. This conflicts with the repository’s WCAG 2.1 AA and first-class input-modality contracts.
Smallest fix: provide a focusable keyboard-operable reset action, either on the rail or as an alternate control, using the existing width setter. Cover keyboard activation and persistence of the default width. The added Playwright case currently exercises only pointer dblclick (sidebar.spec.ts:695–703). This independently confirms the existing unresolved Codex finding; it is one issue, not an additional duplicate defect.
Review scope: exact head 17f78307bbed928dd5358f6d07e4a981e74624f5 against exact base 77729abfb692b25a0f4ec4a69add86af2e32c0dd. Source-only review on the pinned Blox host, covering all six changed files and their relevant producers/consumers. No checkout, builds, tests, or PR-code execution. Existing CI was sampled once; Desktop Core and smoke shards were still running at that snapshot, so this is not a claim of green CI or runtime validation.
|
🤖 Closing this because the work belongs in the current Buzz app repository. Replacement: block/buzz-app#123 |
3 participants
Summary
Related issue
None found.
Testing
just ci: all non-ACP lanes passed; three load-sensitive ACP timer tests failed under suite contention and passed individuallyScreenshots
Resting gutter:
Hover affordance: