refactor(ui): draw the last two overflow menus from the shared menu parts - #206
Merged
Conversation
…arts The snippet row's menu and the mobile terminal bar's panels menu were the two that did not merely position themselves by hand — they painted their own rows too, on the wrong tokens: a `--t-bg-modal` surface with a `--t-border` divider and `--t-bg-elevated` hover, where every other menu in the app uses `surface-float` with `--t-bg-card-hover`. Their rows were `py-1.5` with 12px icons against the shared 16px. Both now open a PickerSurface. The snippet row renders MenuItemList, which turns its two-step "move to folder / back" flow into a submenu and deletes the `movingToFolder` state along with the second menu. The mobile bar renders DropdownMenuItem, and gains an outside-tap dismiss it never had — until now only tapping an item closed it. Hosting MenuItemList inside PickerSurface needed the submenu to be legible to the surface that opened it: a submenu portals to the body, so a surface that dismisses on outside-mousedown would unmount the row before its click fired. The portal is marked `data-menu-portal` and PickerSurface skips those, and the portal moves to `z-10000` because a submenu that flips left overlaps its parent — which is the common case in the narrow right-hand panel. ContextMenu and PanelActionsMenu keep their own backdrop-based surfaces: that backdrop is what lets their submenus receive a click at all, and swapping it for PickerSurface would reintroduce the bug the comment there already warns about.
kipavy
added a commit
that referenced
this pull request
Sep 1, 2026
#206 replaced the terminal bar's hand-written menu buttons with DropdownMenuItem and dropped `data-mobile-panel={key}` with them. Nothing in the repo reads that attribute, but nothing reads `data-mobile-terminal-menu` or `data-mobile-session-chip` either — the whole family arrived together in 1817d08 as the selector surface for driving the Android build over CDP, and this is the one member that went missing. DropdownMenuItem takes a `dataAttrs` map so a shared row can carry the hook the hand-written button carried. It matters more here than elsewhere: the mobile shell only mounts when the backend reports `android`, so this menu cannot be reached from the desktop build and those selectors are how it gets exercised at all.
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
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.
Follow-up to #203 and #205, and the end of the sweep. These two were the menus
that did not merely position themselves by hand — they painted their own rows
too, on the wrong tokens: a
--t-bg-modalsurface with a--t-borderdividerand
--t-bg-elevatedhover, where every other menu usessurface-floatwith--t-bg-card-hover, andpy-1.5rows with 12px icons against the shared 16px.terminal/SnippetsPanelrendersMenuItemListinside aPickerSurface. Thatturns the two-step "move to folder / back" flow into a real submenu and deletes
the
movingToFolderstate along with the entire second menu — the file loses 118lines and gains 43.
mobile/MobileTerminalTopBarrendersDropdownMenuItemrows, and picks up anoutside-tap dismiss it never had: until now only tapping an item closed it.
The one non-obvious bit
Hosting
MenuItemListinsidePickerSurfaceneeded the submenu to be legible tothe surface that opened it. A submenu portals to the body, so it is not a DOM
descendant of that surface, and dismiss-on-outside-mousedown would unmount the row
before its click fired — the exact bug
ContextMenu's backdrop comment alreadywarns about. So the portal is marked
data-menu-portal,PickerSurfaceskipsmousedowns inside one, and the portal moves to
z-10000because a submenu thatflips left overlaps its parent. In the narrow right-hand panel that flip is the
normal case, not the edge case.
ContextMenuandPanelActionsMenukeep their own backdrop-based surfaces forthat same reason — swapping them for
PickerSurfacewould reintroduce the bug.Verification
tsc --noEmitclean; 205 tests acrossterminal,shared,mobileandconnectionspass.Driven in the headless build against a real snippet and a real folder:
surface-floatcard with 16px icons, a divider and ared Delete;
and draws above it;
Opsin that submenu moves the snippet and closes both menus — thedismissal hazard above, confirmed handled rather than assumed;
MobileTerminalTopBaris not live-verified: the mobile shell only mounts whenthe backend reports
android, so it is unreachable from the desktop build. It istypechecked and covered by the suite; a run on the emulator would need
iterating-on-voltius-android-emulator.