Repository navigation
Show one context menu at a time - #447
Merged
Merged
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 13 minutes. View limit details
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. |
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.
Real frames from the web runs, stepped at 1.5 s each:
ci: test web
Right-click an agent, then right-click another agent, and both menus stay open. Add a page row and there are three. A left click on another agent left the first menu open too. #417 noted this and left it.
shadcn_ui puts every context menu and every region that opens one in a single tap group, so a press on another region doesn't count as a press outside the open menu. The group is a constant in the package, and a menu has to share it with its submenus, so it can't be split from outside.
ContextMenuRegionis a small wrapper that remembers the one open menu. A press on any region closes it, and so does opening a menu from code, like the three-dot button on a strategy. All 13 regions in the app now use it, and a test fails if a new one calls shadcn directly.The library already had a smaller version of this,
libraryContextMenuDismissalProvider, which closed strategy and folder menus on any press in the library. It didn't reach the library background menu, and one behavior shouldn't have two mechanisms, so I removed it. That's whyfolder_content.dartshows a re-indented block.What I saw in a running build
Windows release driver builds against the dev deployment, and Edge at 1600 x 900 on web builds against dev. Before and after differ only by this PR.
one-context-menu.json, new, 96 steps: 9 failures before, clean after. It covers agent to agent, left click on another agent, agent to text to smoke, page to page, page to agent, agent to page, strategy to strategy, and strategy to background both ways. It ends by opening the Weapon submenu.right-click-local.json(101 steps) andright-click-cloud.json(63 steps) from Right-click a page to rename or delete it, and an agent to delete it #417: clean.test/context_menu_region_test.dart. Taking the fix out fails the three that cover it. The submenu test guards against closing too much.I reran the new script and
right-click-local.jsonon a rebuild of the merged head, and both were clean. CI passed on the same commit.Not covered
right-click-viewer.jsonwasn't rerun. Viewers get no menu, and this PR doesn't change who gets one.The driver gained
expectText, which counts text on screen. It only compiles into driver builds.
The PR appears safe to merge, with no actionable issues found.Summary
Adds
ContextMenuRegionto keep one context menu open across the app.Static review found no actionable issues. No remaining references to the removed provider were found. Tests and UI captures were not rerun because Flutter and Dart are unavailable in this environment.
SunkenInTime explicitly left the lineup ability picker unchanged because it uses a dialog with a barrier. SunkenInTime also identified the browser’s second-right-click menu as a separate bug handled by another PR.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Press another region] --> B[Hide the tracked menu] C[Open a menu from code] --> D[Controller listener runs] D --> B D --> E[Track the newly opened menu] F[Menu closes or its widget is disposed] --> G[Clear its tracked controller]Reviews (1) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." · Reviewed by Greptile