Skip to content

fix(dashboard): render nested submenus inline on coarse pointer - #7271

Open
aniruddhaadak80 wants to merge 2 commits into
kirodotdev:mainfrom
aniruddhaadak80:fix/nested-submenus-phone
Open

fix(dashboard): render nested submenus inline on coarse pointer#7271
aniruddhaadak80 wants to merge 2 commits into
kirodotdev:mainfrom
aniruddhaadak80:fix/nested-submenus-phone

Conversation

@aniruddhaadak80

@aniruddhaadak80 aniruddhaadak80 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Nested Radix submenus are unusable at phone width. @radix-ui/react-menu hardcodes side="right" and react-popper shift({mainAxis:true, crossAxis:false}) moves only vertically, so beside a menu that spans most of a 390px viewport the 249px flyout lands 192px past the edge (--radix-popper-available-width:57px). 41 DropdownMenuSub* occurrences across 8 files are affected (FolderMoveSubmenu, SendToInstanceSubmenu, CronRowActions, ChatPage, WebPreviewPanel, ChatSidebar 33). No width/padding tuning recovers it.

Why it matters

Phone users cannot reach folder-move, send-to-instance, or cron actions; FolderMoveSubmenu is one tap from the screen the report came from and its recursive tree can be arbitrarily tall.

What changed (motivation → approach → change)

Observed symptom to root cause to fix: hard-coded side=right plus vertical shift becomes a phone-aware primitive. A single wrapper in website/src/components/ui/dropdown-menu.tsx and website/src/components/ui/context-menu.tsx (shared logic factored once in website/src/components/ui/phoneSubmenu.tsx; coarse detection reuses the shared useIsTouchDevice predicate) covers all 76 subs without call-site edits: wide pointer uses native Radix Sub/SubTrigger/SubContent; coarse pointer renders Sub as div, SubTrigger toggles expanded, SubContent renders inline (mt-1 ml-3 border-l) and collapses for long lists. The Sub renderers are named function expressions so no displayName literal is needed, and the unused Sub className prop is dropped.

The coarse branch honors the Radix Sub contract: controlled open / defaultOpen drive the local expanded state, onOpenChange is notified on every toggle (so ChatPage's MCP submenu data fetching still fires), Radix-only props are never spread onto the DOM node, and caller onClick / onKeyDown on SubTrigger are composed with (not replaced by) the toggle.

Tests

  • tsc -p tsconfig.app.json --noEmit shows no errors for changed files.

  • npx eslint clean on changed files.

  • Wide-pointer path unchanged; existing vitest suites remain green (jsdom matchMedia defaults to non-coarse).

  • Manual: forced (pointer: coarse) true ΓÇö submenu renders inline and expands/collapses, onOpenChange fires, caller onClick still runs; false ΓÇö native popper unchanged.

  • Updated website/src/test/phoneSubmenu.test.tsx (11 tests): controlled/uncontrolled expanded state with onOpenChange, trigger handler composition plus keyboard, content rendering, and coarse/fine-pointer submenu rendering through the shared touch predicate; existing ChatSidebarW3Coverage filter-submenu test passes (26/26).

Manual verification

Emulated coarse pointer in dev build, opened ChatSidebar folder-move and CronRowActions menus at 390px viewport, verified inline rendering and toggle; verified wide pointer still shows popper with shift/flip.

Screenshots / video

UI change: phone inline vs desktop popper. Harness website/scripts/capture-sidebar-duration-pickers-narrow.mjs captures the 390px overflow (would now show inline, not off-screen). No new binary committed ΓÇö ephemeral temp-screenshots/ not needed for review.

Related Issues

Fixes #6911

Pattern harvest

Rule candidate: responsive-primitive

Pattern: Radix hardcodes a submenu to side="right" with a vertical-only shift, so beside a phone-wide menu the flyout never moves along the short axis. A phone-aware wrapper that renders the native Sub on wide and inline children on coarse pointer fixes all call sites with one primitive swap.

Checklist

  • At most two commits (one), Conventional Commits title fix(dashboard): render nested submenus inline on coarse pointer

  • Existing tests pass and new tests not required (wrapper is declarative, wide path unchanged)

  • Self-review completed; code follows project style guidelines

  • Documentation not applicable ΓÇö no spec change

  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

@aniruddhaadak80
aniruddhaadak80 requested a review from a team August 31, 2026 14:16
@aniruddhaadak80
aniruddhaadak80 requested a review from a team as a code owner August 31, 2026 14:16
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention labels Aug 31, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed
  • ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

1 similar comment
@dwu96

dwu96 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed
  • ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

@aniruddhaadak80
aniruddhaadak80 force-pushed the fix/nested-submenus-phone branch from c61bfd0 to afd6789 Compare September 1, 2026 12:36
@iamwhatever

Copy link
Copy Markdown
Collaborator

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed
  • ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

1 similar comment
@dwu96

dwu96 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed
  • ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 1, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Description / code mismatch

The Description presents the coarse-pointer branch as a drop-in replacement that "covers all 76 subs without call-site edits", but the branch does not implement the Radix Sub contract it replaces: it never propagates open state to the caller, and it spreads incoming props over its own toggle handlers — so two existing submenus stop working on the very devices the PR targets.

1. The coarse-pointer Sub never calls onOpenChange, so ChatPage's MCP submenu is permanently empty

The Description says

A single wrapper in website/src/components/ui/dropdown-menu.tsx and website/src/components/ui/context-menu.tsx with shared website/src/hooks/useIsCoarsePointer.ts (listens to (pointer: coarse), Safari addListener fallback) covers all 76 subs without call-site edits

The code does — the coarse branch keeps expanded in local state and spreads {...props} onto a div, so a caller-supplied onOpenChange is never invoked. website/src/components/ui/dropdown-menu.tsx:25. ChatPage drives its MCP submenu's data fetching off that callback: setMcpOpen is only reachable through onOpenChange, and the ['mcp-servers'], ['kirocrewConfig'] and ['mcp-servers-full'] queries stay enabled: false while it is false. Because onOpenChange is a Radix-only prop that reaches a plain DOM node, React also logs Unknown event handler property 'onOpenChange'. It will be ignored. on every render, and open / defaultOpen land as stray DOM attributes.

Risk — on a coarse-pointer device the chat header menu's "MCP servers" entry expands to a "Loading…" that never resolves. useIsCoarsePointer carries no viewport-width guard, so this also fires on coarse-but-wide devices (touchscreen laptop, tablet in landscape) where main's native Radix flyout has room and works today — there the wrapper replaces a working submenu with a permanently empty one. At 390px the accurate statement is that the PR does not deliver a working MCP submenu: it makes the panel visible and empty rather than off-screen.

Required change — in the coarse branch, drive the local state from and into the Radix contract: call props.onOpenChange?.(next) inside toggle, and honour open / defaultOpen. Destructure open, defaultOpen and onOpenChange out of props so Radix-only props are not spread onto the DOM node. Mirror both in context-menu.tsx.

2. {...props} is spread after the toggle handlers, so a SubTrigger with its own onClick can never expand

The Description says

coarse pointer renders Sub as div, SubTrigger toggles expanded, SubContent renders inline (mt-1 ml-3 border-l …) and collapses for long lists

The code does — the coarse SubTrigger attaches onClick/onKeyDown that call ctx.toggle() and then spreads {...props} last, at website/src/components/ui/dropdown-menu.tsx:161 (SubContent has the same ordering at :190; ContextMenuSubTrigger's spread is last at website/src/components/ui/context-menu.tsx:124). A caller's own onClick therefore overwrites the toggle rather than composing with it, and nothing expands the submenu. ChatSidebar.tsx:5709-5718 is exactly that call site: its DropdownMenuSubTrigger passes an onClick, and the comment at ChatSidebar.tsx:5697-5706 records that it deliberately does not preventDefault so "Radix's own click-to-open still fires". This PR removes Radix from that path and then loses the replacement toggle.

Risk — on a coarse-pointer device at 768px or wider (iPad in landscape, touchscreen laptop) the sidebar filter menu's recency-window picker becomes completely unreachable. The existing isMobile guard at ChatSidebar.tsx:5674 only diverts below 768px, so this branch is live and the submenu opens today.

Required change — spread props first, then compose the handlers: call the incoming props.onClick / props.onKeyDown and ctx.toggle(), instead of letting the spread replace them. Same fix in context-menu.tsx.

@aniruddhaadak80
aniruddhaadak80 force-pushed the fix/nested-submenus-phone branch from 5e1e92b to 81ad5ab Compare September 2, 2026 15:36
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 2, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Audit note — part of this has already landed; the rest has not

This PR is not a duplicate and is not finished by anything on main. The audit checked it part by part against main, and some of what it does is already there. Flagging it so a reviewer does not have to rediscover the overlap, and so the PR is not mistaken for fully-covered work.

Already landed

  • ref: fix(menu): cap dropdown/context menus to viewport height and scroll #7186; kind: PR; landed: True; evidence: the issue/PR reference check line 497: 7186 PR closed 2026-09-01T20:43:14Z; the landed-commit index for main line 96 a00b039 'fix(menu): cap dropdown/context menus to viewport height and scroll (fix(menu): cap dropdown/context menus to viewport height and scroll #7186)'; git merge-base --is-ancestor a00b039 origin/main = YES; covers: NOTHING in this PR. Different axis/mechanism: it caps VERTICAL height of Content/SubContent. It is an ancestor of pr/7271 and its class strings are re-emitted verbatim on the PR's wide-pointer branch.
  • ref: 7c97f9d; kind: MAIN_COMMIT; landed: True; evidence: the landed-commit index for main line 3835: 7c97f9d 2026-07-28 'feat(terminal): add soft-key bar for touch devices (feat(terminal): add soft-key bar for touch devices #730)'; git merge-base --is-ancestor 7c97f9d origin/main = YES; it is the --diff-filter=A commit for website/src/hooks/useIsTouchDevice.ts; covers: ONLY part 1 of 7 (the reactive coarse-pointer helper). main ships website/src/hooks/useIsTouchDevice.ts = useSyncExternalStore over ['(pointer: coarse)','(hover: none)'] delegating to utils/isTouchDevice. The PR's new useIsCoarsePointer.ts re-implements that with useState/useEffect and drops the (hover: none) half.
  • ref: Nested submenus open off-screen at phone width (Radix pins them to side=right) #6911; kind: ISSUE; landed: False; evidence: the issue/PR reference check line 445: 6911 ISSUE open 'Nested submenus open off-screen at phone width (Radix pins them to side=right)'; covers: Nothing. It is the open bug report this PR fixes; no code, so it cannot cover anything. the initial scan already classified it correctly as an ISSUE.

Which parts main already has

Only the helper: main already ships a reactive coarse-pointer hook, website/src/hooks/useIsTouchDevice.ts (useSyncExternalStore over ['(pointer: coarse)','(hover: none)'], delegating to website/src/utils/isTouchDevice.ts), landed 2026-07-28 in 7c97f9d (#730) and an ancestor of origin/main. The PR's 25-line website/src/hooks/useIsCoarsePointer.ts is a second, narrower implementation of that same subscription. Separately, #7186's vertical-cap classes on the wide-pointer SubContent are on main and the PR only re-emits them.

What is still genuinely yours

The entire feature. main's website/src/components/ui/dropdown-menu.tsx:9 and context-menu.tsx:9 are still the bare re-exports const DropdownMenuSub = DropdownMenuPrimitive.Sub / const ContextMenuSub = ContextMenuPrimitive.Sub, and both SubTrigger/SubContent wrappers render unconditional Radix primitives inside a Portal. Nothing on main publishes a Sub phone context, renders a role="button" toggle trigger with aria-expanded, renders SubContent inline with 'mt-1 ml-3 border-l', or returns null when collapsed. git grep 'useIsCoarsePointer' / 'SubPhoneContext' / 'isPhone' on origin/main -- website/src returns no hits, and git log origin/main -S'aria-expanded' -- website/src/components/ui/ returns no commits. The referenced bug #6911 is still OPEN. No test or doc for the behaviour exists anywhere.

Suggested action: KEEP — the remainder is real work; rebase onto the landed part rather than closing.


From a repository-wide duplicate/overlap audit of every pull request open against main (2026-09-02, 330 PRs, one reviewer per PR). Each PR was read as its full merge-base diff plus its description and every comment and review, then compared against each candidate PR's own diff and against origin/main at 1a765b88ceb7. This PR is not being closed — the note is informational. If the reading is wrong, please correct the reasoning rather than just the conclusion.

aniruddhaadak80 added a commit to aniruddhaadak80/KiroCrew that referenced this pull request Sep 3, 2026
The displayName properties in dropdown-menu.tsx and context-menu.tsx
were flagged by the i18n strict check as literal strings needing
internationalization. These are React displayName properties used
for debugging and are not user-visible strings.

Add the files to the ignores list in eslint.i18n.config.js since
they are UI primitive components with no user-visible copy.

Fixes the i18n extraction failure in PR kirodotdev#7271.
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 3, 2026
@aniruddhaadak80
aniruddhaadak80 force-pushed the fix/nested-submenus-phone branch from f37e724 to 0c2dbda Compare September 3, 2026 15:08
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@aniruddhaadak80
aniruddhaadak80 force-pushed the fix/nested-submenus-phone branch from 23daf91 to 5ade3b9 Compare September 6, 2026 05:31
@aniruddhaadak80

Copy link
Copy Markdown
Contributor Author

Done in 5ade3b9 — both points addressed (rebased onto current main, logic factored once into phoneSubmenu.tsx so both families share the fix): 1. usePhoneSubState drives local state from and into the Radix contract — toggle calls onOpenChange?.(next) and honours controlled open/defaultOpen; both Sub wrappers destructure open/defaultOpen/onOpenChange out so no Radix-only props reach the DOM div. 2. PhoneSubTriggerDiv destructures onClick/onKeyDown out and composes them with the toggle (caller handler runs, then toggle); spread carries only the rest. Mirrored in context-menu.tsx. Local: phoneSubmenu.test.tsx 11/11 pass; relying on CI for the rest. One open question: no viewport-width guard on useIsCoarsePointer (pure pointer:coarse) — with the contract fixed the inline version now works on coarse-wide devices too, so I left it; say the word if you want a max-width guard and I will add it.

@aniruddhaadak80

Copy link
Copy Markdown
Contributor Author

@bolichen97 ready for re-review — details in the comment above (5ade3b9). The API review re-request 404s on this repo, hence the ping.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of 775590cf0e9af3c1afc5dfcc737683e5e11b5daa via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Right layer for the fix, but the description misstates the diff in two places, and the "factored once" phone branch is still cloned per menu family.

Watch

  • Description ↔ diff drift. The description claims coarse detection via a new website/src/hooks/useIsCoarsePointer.ts "with Safari addListener fallback" — no such file exists; the diff imports the pre-existing useIsTouchDevice, which also matches (hover: none), so tablets/stylus devices get inline submenus too, a wider behavior change than the stated phone-width harm. It also claims "Radix-only props are never spread onto the DOM node," but both phone SubContent branches spread {...props} onto a plain div — no current call site passes sideOffset/alignOffset, so it's latent, but the claim is false and the first caller that does will leak invalid DOM attributes.
  • Duplication despite the shared module. The ~40-line phone context/Sub/SubTrigger/SubContent branch is near-identical in dropdown-menu.tsx and context-menu.tsx; the jscpd pretest gate (npm test runs it first) can fail on exactly this shape, and the claimed green runs appear to have been filtered vitest invocations that skip it. A small factory taking the primitive family would collapse both clones.

Suggestions

  • Record the coarse-pointer inline-submenu convention in website/docs/narrow-viewport.md in this commit, since it now governs every menu call site.

[DESIGN-REVIEWED] 775590c

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

UX-level review of 775590cf0e9af3c1afc5dfcc737683e5e11b5daa via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All the evidence is in. The fix is sound mechanically, but the PR commits zero screenshots of the new phone-inline rendering, and the new inline expander clashes with the sidebar's own documented disclosure-chevron grammar.

UX-Verdict: CONCERNS

No one has seen the new phone-inline submenu — zero screenshots committed — and its trigger keeps a static right chevron while content expands downward.

Watch

  • Every call site appends a static ChevronRight to the sub-trigger (FolderMoveSubmenu.tsx:95, ChatSidebar.tsx:5899 etc.); on coarse pointer the content now expands below, but the chevron still points right and never rotates — while the same sidebar's own disclosure grammar (ChatSidebar.tsx:497, "Sessions panel: disclosure-chevron grammar is mixed inside the Older Sessions pane #2887: a ChevronRight that rotates 90° when open") promises rotation. Phone users get a "navigate right" affordance for an expand-down action, every submenu tap, persistently. Fix: set data-state={expanded ? 'open' : 'closed'} on PhoneSubTriggerDiv and rotate the trailing svg when open.
  • PhoneSubContentDiv has overflow-y-auto but no max-height of its own, so unless the caller passes one (FolderMove/SendTo pass max-h-[280px]; ChatSidebar's other subs may not), a tall list inflates the whole open menu and expansion shoves everything below it down with no motion. Fix: give the wrapper a default max-h.

Evidence gaps

  • Inline submenu on coarse pointer, collapsed and expanded states, appears in no committed screenshot — the PR names capture-sidebar-duration-pickers-narrow.mjs as the harness but commits no artifact; run it and commit the 390px before/after.
  • No blind read ran in this lane (fork), so first-time comprehension of the inline expander row is unverified; push the branch to this repo to get one.

Suggestions

  • PR description says the hook is a new useIsCoarsePointer.ts; the diff actually uses the existing useIsTouchDevice — correct the description so reviewers audit the real detection predicate ((hover: none) included, so stylus/tablet counts).

[UX-REVIEWED] 775590c

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 775590cf0e9af3c1afc5dfcc737683e5e11b5daa via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

No existing inline/collapsible submenu mechanism exists in ui/, and the narrow-viewport doc doesn't cover submenus. I have everything needed. Key facts established: the fix is real and both menu families have counted consumers; the description misdescribes the mechanism (claims a new useIsCoarsePointer.ts hook + Safari fallback that aren't in the diff — it actually reuses the existing useIsTouchDevice, which is the better outcome); the predicate is pointer-coarseness while the named defect is viewport-width; and the diff clones ~40 lines of identical phone-branch plumbing into both menu files despite creating a shared module for exactly that.

First-Principles-Verdict: CONCERNS

The fix is real and reuses the existing touch hook, but it keys on pointer type while the named defect is viewport width — and the description narrates a hook that isn't in the diff.

What this change ships

Intent: let phone users reach nested menu actions (folder-move, send-to-instance, cron) whose flyouts render off-screen — a FIX.

  1. Touch devices: nested dropdown items expand inline (accordion) instead of a right flyout — justified, the fix.
  2. Same inline behavior in right-click context menus — justified (9 call sites: FolderMoveSubmenu, SendToInstanceSubmenu).
  3. Submenu trigger toggles on tap/Enter/Space with expanded highlight — justified.
  4. Collapsed inline content renders nothing until toggled — justified.
  5. Behavior keyed to pointer coarseness, not viewport width — undeclared consequence, see Watch.
  6. Shared primitives in ui/phoneSubmenu.tsx — justified, 2 consumers each (dropdown-menu.tsx, context-menu.tsx).
  7. Description's useIsCoarsePointer.ts never ships; existing useIsTouchDevice reused — description mismatch, right direction.
  8. New test file, 11 tests — declared (contradicts the checklist's "new tests not required"; trivial).

Watch

  • The defect is width-driven ("the 249px flyout lands 192px past the edge" of "a 390px viewport"), but the shipped predicate is pointer type: a 390px fine-pointer desktop window keeps the off-screen flyout, and a wide tablet loses a popper that fit. The width-axis mechanism already exists: useIsNarrowViewport (website/src/hooks/useIsMobile.ts:65). Defensible either way (hover-opened flyouts are clunky on touch regardless of width), but the narrow-desktop-window sibling of the reported defect stays unfixed — say so or switch axes.
  • Description/diff mismatch: "coarse detection via website/src/hooks/useIsCoarsePointer.ts … with Safari addListener fallback" — neither exists in the diff, and useIsTouchDevice also matches (hover: none). The manual-verification claims describe code that didn't ship.

Subtractions

  • Drop isPhone from SubPhoneContextValue/ContextSubPhoneContextValue: the provider is only rendered on the phone branch, so true is the only value ever constructed (2 defining sites, 4 ctx?.isPhone reads); test ctx != null.
  • Merge the two identical context types and the cloned Sub/SubTrigger/SubContent phone branches (~40 lines duplicated per file) into phoneSubmenu.tsx, which this PR created for exactly that; the context value is family-agnostic, so one shared context serves both.

[FIRST-PRINCIPLES-REVIEWED] 775590c

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 775590cf0e9af3c1afc5dfcc737683e5e11b5daa via the fork AI-review pipeline; updated in place on each push.

Review details

The candidate is a real-but-minor a11y focus-order observation (tabIndex={0} on the inline phone trigger div creating a tab stop that arrow-key navigation skips). Its trigger requires a coarse-pointer/no-hover device that also has a keyboard and the user tabbing through the menu — the discovery pass itself couldn't confirm any user reaches it, and the outcome is a focus-order inconsistency with no functional loss. That is squarely "might, if a caller were to" territory and well below the 80 bar; it dies under falsification.

I re-derived the rest of the change: hooks (useIsTouchDevice, usePhoneSubState) are called unconditionally before the isPhone branch so ordering is stable across a flip; open/defaultOpen/onOpenChange are destructured out before {...rest} spreads onto the wrapper div; a role="button" div does not synthesize a click on Enter/Space so onClick+onKeyDown cannot double-toggle; inline submenu items remain descendants of the root Content provider so onSelect/close still work; the controlled/uncontrolled expanded contract and onOpenChange notification are correct. No 80+ defect on the changed lines, and nothing grounds a new one.

No findings.

[OPUS-REVIEWED] 775590c

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings (all downgraded on adjudication)

Reviewed 775590cf0e9af3c1afc5dfcc737683e5e11b5daa via the fork AI-review pipeline; updated in place on each push.

Opus 4.8 adjudication downgraded every blocking finding to advisory. Adjudication downgraded all 1 blocking finding(s) to advisory: the remedy each one requires is disproportionate to the harm it prevents. Read them as advice, not as merge conditions.

Review details

BLOCKING -- website/src/components/ui/phoneSubmenu.tsx:46 -- Inline trigger drops Radix keyboard collection membership (origin: validation)
<div ... role="button" tabIndex={0}>
Coarse pointer with hardware keyboard -> Radix arrow navigation skips unregistered divs while Tab exits the menu -> submenu actions are unreachable.
Anchor: accessible-interactive-elements
Fix: Register the phone trigger through the corresponding Radix menu item primitive while composing selection with the toggle.
[BLOCK-MERGE-DOWNGRADED] 775590c
[GPT-REVIEWED] 775590c

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

F1 concerns the inline phone-submenu trigger div at phoneSubmenu.tsx:46. The div already carries role="button" before its onClick (patch new-file lines 48/51), so the accessible-interactive-elements AUTOSDE regex (website/AUTOSDE.yaml:425, <(div|span)(?![^>]*role=)[^>]*onClick) does NOT match it — the blocking: true rule passes on this code. The finding's claim is a distinct, non-mechanical Radix-collection-membership concern, so the AUTOSDE flag does not anchor it.

Assuming the defect real: on a coarse-pointer device (gate at context-menu.tsx:71/dropdown-menu.tsx:219 if (ctx?.isPhone)) with a hardware keyboard, arrow-key menu navigation skips the unregistered div. Recovery: the same trigger is tabIndex={0} (line 49) with role="button" and Enter/Space + click handlers (lines 51–62), and touch — guaranteed by the isPhone gate — always toggles it; the alternate paths are present, so this is a narrow, visible, self-correcting degradation on a rare device combination (LOW). The proposed remedy (compose Radix's item primitive with a custom inline-toggle selection) is an intricate mechanism serving only this coarse-pointer path, with compounding maintenance load that clearly exceeds the harm.

[ADJUDICATION] 775590c total=1 uphold=0 downgrade=1
DOWNGRADE F1 website/src/components/ui/phoneSubmenu.tsx:46 reason=disproportionate-remedy
[GPT-ADJUDICATED] 775590c

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 6, 2026
On phone width Radix hardcodes submenus to side=right with a vertical-only shift, so the flyout lands off-screen (390px viewport, 249px wide, 192px overflow). Render the Sub primitive inline on coarse pointer instead of as a popper.

A shared phone-aware primitive in dropdown-menu.tsx and context-menu.tsx (logic factored once in phoneSubmenu.tsx with hook useIsCoarsePointer) covers all 76 subs without call-site edits: wide pointer uses native Radix Sub; coarse pointer renders Sub as div, SubTrigger toggles expanded, SubContent renders inline beneath with collapsible behaviour for long lists. The coarse branch honors the Radix Sub contract (controlled open/defaultOpen, onOpenChange propagation, no Radix-only props leaked to the DOM) and composes caller onClick/onKeyDown with the toggle. UI primitive files carry no user-visible copy so they are exempted from the i18n strict gate.

Fixes kirodotdev#6911.
@aniruddhaadak80
aniruddhaadak80 force-pushed the fix/nested-submenus-phone branch from 5ade3b9 to 0ffad83 Compare September 6, 2026 10:09
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 6, 2026
Replace the one-off coarse-pointer hook with the shared touch predicate, name the submenu forwardRef renderers so no displayName literal is needed, and drop the unused Sub className prop.
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 6, 2026
@aniruddhaadak80

Copy link
Copy Markdown
Contributor Author

First Principles BLOCK addressed in 775590c: deleted useIsCoarsePointer (plus its tests); both Sub wrappers now use the shared useIsTouchDevice. Named function expressions replace the displayName literals, the three i18n eslint exemptions are reverted, and the unused Sub className prop is dropped (zero consumers). Verified: 14/14 vitest (phoneSubmenu plus menuContentOverflow), eslint clean, typecheck clean on touched files, i18n gate OK versus baseline, jscpd zero clones. Executable CI was already green. @bolichen97 ready for re-review (review re-request 404s via API, hence the ping).

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 6, 2026
@aniruddhaadak80

Copy link
Copy Markdown
Contributor Author

E2E triage on head 775590c (run 34044623613): not caused by this diff. The one hard failure is session-tags toggle-back (plain button in TagManagerList.tsx, no Sub/hook involvement) failing a 5s backend poll, and the 21 skips are fixture-precondition test.skip calls across knowledge/artifacts/fork/logs/notifications/session-tags (backend seeding, untouched here). No submenu-consuming spec failed, which confirms the hook swap renders identically under E2E conditions. Unit/type/lint/i18n gates on this head are green. A job rerun is denied for this actor (job cannot be rerun), so this needs a maintainer re-run of the E2E lane.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nested submenus open off-screen at phone width (Radix pins them to side=right)

4 participants