fix(dashboard): render nested submenus inline on coarse pointer - #7271
fix(dashboard): render nested submenus inline on coarse pointer#7271aniruddhaadak80 wants to merge 2 commits into
Conversation
|
👋 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:
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
|
👋 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:
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. |
c61bfd0 to
afd6789
Compare
|
👋 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:
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
|
👋 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:
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. |
bolichen97
left a comment
There was a problem hiding this comment.
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.tsxandwebsite/src/components/ui/context-menu.tsxwith sharedwebsite/src/hooks/useIsCoarsePointer.ts(listens to(pointer: coarse), SafariaddListenerfallback) 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
Subasdiv,SubTriggertogglesexpanded,SubContentrenders 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.
5e1e92b to
81ad5ab
Compare
Audit note — part of this has already landed; the rest has notThis PR is not a duplicate and is not finished by anything on Already landed
Which parts
|
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.
f37e724 to
0c2dbda
Compare
23daf91 to
5ade3b9
Compare
|
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. |
|
@bolichen97 ready for re-review — details in the comment above (5ade3b9). The API review re-request 404s on this repo, hence the ping. |
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of 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
Suggestions
[DESIGN-REVIEWED] 775590c |
UX Review (Fable 5, fork) — 🟡 CONCERNSUX-level review of 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
Evidence gaps
Suggestions
[UX-REVIEWED] 775590c |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of No existing inline/collapsible submenu mechanism exists in 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 shipsIntent: let phone users reach nested menu actions (folder-move, send-to-instance, cron) whose flyouts render off-screen — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 775590c |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsThe candidate is a real-but-minor a11y focus-order observation ( I re-derived the rest of the change: hooks ( No findings. [OPUS-REVIEWED] 775590c |
GPT 5.6 Review (fork) — ✅ no blocking findings (all downgraded on adjudication)Reviewed 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 detailsBLOCKING -- website/src/components/ui/phoneSubmenu.tsx:46 -- Inline trigger drops Radix keyboard collection membership (origin: validation) Adjudication (Opus 4.8) — is blocking on each finding proportionate?F1 concerns the inline phone-submenu trigger div at Assuming the defect real: on a coarse-pointer device (gate at [ADJUDICATION] 775590c total=1 uphold=0 downgrade=1 |
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.
5ade3b9 to
0ffad83
Compare
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.
|
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). |
|
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. |
Problem / Motivation
Nested Radix submenus are unusable at phone width.
@radix-ui/react-menuhardcodesside="right"andreact-poppershift({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). 41DropdownMenuSub*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;
FolderMoveSubmenuis 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=rightplus vertical shift becomes a phone-aware primitive. A single wrapper inwebsite/src/components/ui/dropdown-menu.tsxandwebsite/src/components/ui/context-menu.tsx(shared logic factored once inwebsite/src/components/ui/phoneSubmenu.tsx; coarse detection reuses the shareduseIsTouchDevicepredicate) covers all 76 subs without call-site edits: wide pointer uses native RadixSub/SubTrigger/SubContent; coarse pointer rendersSubasdiv,SubTriggertogglesexpanded,SubContentrenders inline (mt-1 ml-3 border-l) and collapses for long lists. TheSubrenderers are named function expressions so nodisplayNameliteral is needed, and the unusedSubclassNameprop is dropped.The coarse branch honors the Radix
Subcontract: controlledopen/defaultOpendrive the local expanded state,onOpenChangeis notified on every toggle (so ChatPage's MCP submenu data fetching still fires), Radix-only props are never spread onto the DOM node, and calleronClick/onKeyDownonSubTriggerare composed with (not replaced by) the toggle.Tests
tsc -p tsconfig.app.json --noEmitshows no errors for changed files.npx eslintclean on changed files.Wide-pointer path unchanged; existing vitest suites remain green (jsdom
matchMediadefaults to non-coarse).Manual: forced
(pointer: coarse)true ΓÇö submenu renders inline and expands/collapses,onOpenChangefires, calleronClickstill runs; false ΓÇö native popper unchanged.Updated
website/src/test/phoneSubmenu.test.tsx(11 tests): controlled/uncontrolled expanded state withonOpenChange, trigger handler composition plus keyboard, content rendering, and coarse/fine-pointer submenu rendering through the shared touch predicate; existingChatSidebarW3Coveragefilter-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.mjscaptures the 390px overflow (would now show inline, not off-screen). No new binary committed ΓÇö ephemeraltemp-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 nativeSubon 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 pointerExisting 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