fix(ui): notification a11y/state + rail-pager gesture follow-ups (23 verified bugs) - #11316
Merged
Conversation
…d bugs) Adversarial fable review of the merged per-surface NotificationCenter (#11174) surfaced 6 confirmed in-scope defects; all fixed here: 1. Focus management: the sheet/panel are hand-rolled role=dialog shells portaled to body but did nothing for keyboard/AT — no aria-modal, no focus-in, no trap, no focus return. Added aria-modal, save/restore activeElement across open, a Tab-wrap handler, and tabIndex=-1 on the invisible backdrops so the first Tab lands on a real control. 2. Bell popover stayed open after a deep-link row navigated (uncontrolled Radix Popover). Made it controlled (bellOpen) so a row click closes it after nav. 3. Filter chips claimed role=tablist/tab+aria-selected without any tabs keyboard semantics. Switched to role=group + aria-pressed toggle buttons, matching the adjacent sort toggle. 4. Stale category filter could silently resurrect: the drain-to-all fallback was computed-only, so a later notification of the drained category snapped the open shell back to the old filter. Now committed to state so it is sticky. 5. Empty state showed the definitive "all caught up" before the inbox hydrated (conflating not-loaded with empty). Gate on the store hydrated flag; show "Loading" until it settles. 6. Overlay z-indexes were raw z-[9500]/[9501] off the floating-layers scale, tying/colliding with Z_TUTORIAL. Added Z_NOTIFICATION_BACKDROP/OVERLAY (9550/9560, above the pointer-events:none tutorial spotlight, below the system-banner band) and mirrored them with the standard comment. +3 unit tests (a11y dialog focus flow, hydration-gated empty state, aria-pressed filter toggles) + a __setHydratedForTests store seam. 13 notification + 19 home/store tests green; ui typecheck + biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarial fable review of the merged single-owner 50%-threshold pager surfaced several gesture defects — two are regressions from the 50% change itself: 1. Release velocity, not gesture average (regression): the flick escape-hatch used dx/total-elapsed, so the common "drag slowly to ~40%, then flick" read as slow and sprang back. Now a trailing 100ms sample window derives RELEASE velocity; falls back to the average for tap-flicks/single-sample. Direction-guarded so a drag-forward-then-fling-back release can't commit the wrong page. The same release velocity now feeds the momentum-settle duration. 2. Edge-swipe-home needed a half-screen drag (regression): the right-drag-home at page 0 is damped by EDGE_RESISTANCE, so the 50% raw-distance floor demanded a half-screen pull that read as "you can't do this". It now commits at the 64px MIN threshold; inter-page swipes keep the 50% floor. 3. Committed-swipe click suppression only covered rail commits (regression from the single-owner refactor): inner launcher paging and the edge-swipe-home path could still tap-launch the element under the release. Moved suppression into the hook (armed on every commit) and exposed handlers.onClickCapture; Launcher + HomeLauncherSurface use it, replacing HomeLauncherSurface's local ref. 4. Mouse-button guards: pointerdown ignores non-primary (right/middle/pen-barrel) buttons; a mouse/pen move with buttons=0 after a button-down abandons the stale drag (released off-surface before capture → no pointerup → hover was panning the rail). Gated on hadButtons so synthetic/touch events aren't spuriously abandoned. 5. Settle-teleport: touching down during a momentum settle grabbed the rail at the animation's END position; now it reads the live transform and catches the rail under the finger. +9 pager unit tests (release-velocity flick + direction guard, edge-swipe-home reduced threshold on/off, non-primary-button ignore, stale-hover abandon, click-suppression) — 17 pager + 112 launcher/home tests green; ui typecheck + biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…unch route (5 verified bugs) Continued fable-review cleanup across the launcher/notification/rail surfaces: 1. Reduced-motion was silently defeated: the pager writes an inline `style.transition`, which always overrides the `motion-reduce:transition-none` CSS class. Gate it in writeOffset (one seam for settle/snap-back/abandon/edge- buttons/mount) via a fresh prefers-reduced-motion read, and drop the now-dead motion-reduce classes from both rails. 2. Offscreen home↔launcher rail half was aria-hidden but still keyboard-focusable — added `inert` to each half (matches the Launcher's inert page pattern) so Tab can't reach controls hidden behind the visible page. 3. Notification category-filter chip bar (a horizontal scroll container) got vertically crushed when the list overflowed the flex-column shell — added `shrink-0` so only the intended list `ul` is the flex scroller. 4. Launcher curation dedup rewrote a winning alias to the canonical id but kept the alias `path`, and handleLaunch prefers `path` — so an alias win navigated to the alias route while recents/telemetry recorded the canonical id. Drop the path when (and only when) the id is rewritten, so launch falls back to /apps/<canonicalId> and all three agree. 5. Viewport resize DURING a drag left the rail permanently mis-offset after snap-back — finish()/abandonDrag() now re-measure width for the settle geometry (the commit threshold still uses the gesture's original width). +4 unit tests (reduced-motion transition drop, alias-path drop + non-alias path kept). 128 launcher/pager/notification/home tests green; ui typecheck + biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cs (5 verified bugs) Final batch of the fable-review cleanup on the launcher/notification surfaces: 1. WS notification boundary was a double-cast that trusted every field but `id`. Replace with validateWsNotification: require id + title (drop otherwise), coerce category/priority to their canonical union defaults, createdAt→now, and pass optional string/timestamp fields through only when well-typed. 2. Standalone page clamp read `layout.pages.length` while the RENDERED pages come from `curatedPages` in grouped mode — wrong count source. Clamp against `pages` (the actually-rendered list) and move the effect below its memo. 3. Notification sheet grabber implied a drag-to-dismiss gesture that didn't exist; made it a real dismiss button (tap the pill closes the sheet) — no whole-sheet pull gesture, which would fight the list's own scroll. 4. LAUNCHER_PAGE_SIZE doc claimed a fixed 4-col grid though the launcher is grid-cols-4 → sm:grid-cols-5; corrected the comment (value unchanged). 5. Launcher header doc claimed "fully token-themed" though the springboard intentionally uses a fixed white-on-wallpaper treatment; corrected the doc and clarified that production (LauncherSurface) renders read-only curated pages. +3 tests (WS drop-on-missing + enum coercion) + a __getStateForTests store seam. 123 launcher/notification/store/home tests green; ui typecheck + biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…esture follow-ups) Home-screen e2e passes end-to-end on the rebased tree: mobile sheet + desktop panel render on-screen, read-only launcher, swipe-back returns home, 0 page errors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Member
Author
|
QA (subagent-verified): 10+ of the 23 claims spot-checked in code and all real (velocity window+direction guard, edge-swipe MIN_DISTANCE commit, hook-owned click suppression with the duplicate deleted, WS payload validation replacing the double-cast, focus-trap/restore, aria group/pressed). No #11174 regression — 0.5 threshold and claimPagerPointer arbitration untouched, and the boot-free home-screen e2e passes end-to-end with real CDP touch. 116 unit tests green, pager suite doubled. Nit on record: z-index classes mirror rather than consume the new constants (Tailwind constraint). Merging. |
Contributor
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
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.
Summary
Follow-up bug-fix sweep on the launcher / notification / rail surfaces after the per-surface NotificationCenter + single-owner 50%-swipe landed (#11174). An adversarial fable review (6 finders → per-finding verify) surfaced 25 confirmed in-scope defects; 23 are fixed here across 4 commits, each with tests. Two are deliberately deferred (below).
Notifications — a11y + state correctness
aria-modal, focus moves in on open, Tab is trapped, focus returns to the opener on close; the invisible backdrops are no longer Tab stops.role=tablist/tab) with none of the tabs keyboard model → nowrole=group+aria-pressedtoggle buttons, matching the sort toggle.floating-layersscale (Z_NOTIFICATION_BACKDROP/OVERLAY, above the pointer-events-none tutorial spotlight, below the system-banner band) — were rawz-[9500]/[9501]tyingZ_TUTORIAL.id.shrink-0so it isn't vertically crushed when the list overflows.Rail / pager — gesture + a11y (incl. regressions from the 50% change)
handlers.onClickCapture), so inner launcher paging + the edge-swipe-home path no longer tap-launch the element under the release (was rail-only after the single-owner refactor).writeOffset(an inlinetransitioncan't be overridden by the CSS class); deadmotion-reduce:transition-noneclasses removed.inert, not justaria-hidden, so Tab can't reach controls behind the visible page.Launcher
path, so launch navigates to/apps/<canonicalId>— matching the id that recents/telemetry record (was navigating to the alias route).pagescount, notlayout.pages(wrong source in grouped mode).LAUNCHER_PAGE_SIZE(responsive 4→5 col), Launcher header (fixed white-on-wallpaper treatment; production renders read-only curated pages).Tests / evidence
output-home/02-mobile-notification-sheet.png(centered, on-screen),output-home/09-desktop-notification-panel.png(right-anchored, on-screen). Rebased onto latestdevelop.Deferred (honest, out of scope for a bug-fix PR)
pageGroups→ read-only curated pages). The verifier flagged ~200 lines as dead, but that machinery is still exercised (and passing) by the standalone launcher-e2e + stories — it's a disconnected feature, and deleting it removes user-facing drag-to-reorder (a product decision, Interaction QA is shallow/larpy — de-larp touch, mouse, XR & TUI testing with real-input e2e, recordings, per-step validation & fuzz #10722). Surfacing rather than deleting unilaterally.🤖 Generated with Claude Code