fix(shell): switch route when toggle requested for open menu & prevent XF86PowerOff key repeat - #6508
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Fixes menu route toggling and power-key repetition, alongside several shell reliability changes.
Changes:
- Adds route-aware menu toggling and power-key repeat handling.
- Coalesces concurrent scans and guard evaluations.
- Adjusts notification icons, provider errors, bar injection, and style refreshes.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
shell/shell.qml |
Switches routes when toggling an open menu. |
shell/services/AppLibrary.qml |
Changes scan shells and queues icon rescans. |
shell/plugins/notifications/components/NotificationCard.qml |
Resolves file-backed notification icons. |
shell/plugins/model-usage/providers/Claude.qml |
Suppresses expected missing-file errors. |
shell/plugins/menu/Menu.qml |
Queues concurrent guard evaluations. |
shell/plugins/bar/Bar.qml |
Guards dynamic property injection. |
shell/Commons/Style.qml |
Prevents restarting active processes. |
default/hypr/bindings/utilities.lua |
Attempts to disable power-key repetition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| o.bind("SUPER + SHIFT + code:201", "Omarchy menu", "omarchy-menu toggle root") | ||
| o.bind("SUPER + ESCAPE", "System menu", "omarchy-menu toggle system") | ||
| o.bind("XF86PowerOff", "Power menu", "omarchy-menu toggle system", { locked = true }) | ||
| o.bind("XF86PowerOff", "Power menu", "omarchy-menu toggle system", { locked = true, repeat = false }) |
| var filePath = value.indexOf("file://") === 0 ? value.substring(7) : (value.charAt(0) === "/" ? value : "") | ||
| if (filePath.length > 0) return Quickshell.iconPath(filePath, true) |
| command: ["bash", "-c", root.hiddenEntryScanCommand()] | ||
| stdout: SplitParser { onRead: function(line) { hiddenEntryOutput.text += line + "\n" } } | ||
| onStarted: hiddenEntryOutput.text = "" | ||
| onExited: root.loadDesktopHiddenEntries(hiddenEntryOutput.text) | ||
| } | ||
|
|
||
| property bool pendingIconIndexRescan: false | ||
|
|
||
| Process { | ||
| id: iconIndexScan | ||
| command: ["bash", "-lc", root.iconIndexScanCommand()] | ||
| command: ["bash", "-c", root.iconIndexScanCommand()] |
| var targetRoute = payload.initialMenu || payload.menu | ||
| if (targetRoute && loader.item.activeMenu !== undefined && loader.item.activeMenu !== targetRoute) { | ||
| return summon(id, payloadJson) | ||
| } |
45fad93 to
dda1de0
Compare
dda1de0 to
6c4a037
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
shell/shell.qml:524
resolveRoute()expands aliases but does not follow link entries;Menu.openRoute()follows those separately (Menu.qml:865-868). As a result, openinginstall.stylesetsactiveMenutostyle.install, and togglinginstall.styleagain sees two different IDs and reopens the menu instead of closing it. Canonicalize link targets before comparing routes.
var resolvedTarget = loader.item.resolveRoute(targetRoute)
var resolvedActive = loader.item.resolveRoute(loader.item.activeMenu)
if (resolvedTarget && resolvedActive && resolvedTarget !== resolvedActive) {
return summon(id, payloadJson)
shell/shell.qml:512
- The new route-switching branch has no regression coverage: existing shell tests exercise summon/hide, while the acceptance menu test only summons
root. Add a test that opens one route, toggles a different route and confirms the menu remains open on the requested route, then toggles that route again and confirms it closes; include a link route to cover canonicalization.
if (!isPluginOpen(id)) return summon(id, payloadJson)
| try { | ||
| var payload = JSON.parse(payloadJson) | ||
| var targetRoute = payload.initialMenu || payload.menu | ||
| if (targetRoute && typeof loader.item.resolveRoute === "function" && typeof loader.item.activeMenu === "string") { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
shell/shell.qml:521
resolveRoute()only normalizes aliases; it does not return the route thatopenRoute()will actually display. For example,install.styleredirects tostyle.install, and an unknown route falls back toroot. If that effective route is already active, this comparison still callssummon()and resets the menu instead of toggling it closed. Canonicalize redirects and missing items before comparing withactiveMenu.
var resolvedTarget = loader.item.resolveRoute(targetRoute)
var resolvedActive = loader.item.resolveRoute(loader.item.activeMenu)
if (resolvedTarget && resolvedActive && resolvedTarget !== resolvedActive) {
shell/shell.qml:523
- This new toggle branch has no regression coverage, although
test/acceptance.d/menu-test.shalready exercises menu routing. Add cases that open one route and toggle to another, then toggle the same effective route closed (including an alias/redirect such asinstall.style→style.install) so route canonicalization and close behavior are both verified.
if (resolvedTarget && resolvedActive && resolvedTarget !== resolvedActive) {
return summon(id, payloadJson)
default/hypr/bindings/utilities.lua:8
- The power-key regression is not covered by the existing Hyprland binding test, which currently discards binding options. Extend
test/shell.d/hyprland-default-config-test.shto record theXF86PowerOffoptions and assert thatlockedremains true andrepeatingis false; otherwise a future binding edit can silently restore the rapid menu toggling.
o.bind("XF86PowerOff", "Power menu", "omarchy-menu toggle system", { locked = true, repeating = false })
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
shell/shell.qml:512
- The new stateful toggle behavior is not covered by the shell tests: the existing acceptance test only summons
systemdirectly, so it would still pass if this branch always hid the menu. Add a focused test that opensroot, togglessystem, and verifies the menu remains open with the system route visible; also cover toggling the already-active route to close it.
if (!isPluginOpen(id)) return summon(id, payloadJson)
…t XF86PowerOff key repeat - Update shell toggle() to switch sub-routes via summon() when an open menu is toggled with a different canonical sub-route, resolving aliases and links. - Guard route-switching to regular menu mode so dmenu requests (select/input) are cleanly hidden on toggle rather than clobbering active request state. - Add repeating = false to XF86PowerOff keybinding in utilities.lua and add regression test in hyprland-default-config-test.sh.
9666adb to
70eb26f
Compare
…tead of re-summoning resolveRoute only expanded aliases; openRoute additionally follows link targets and falls back to root for unknown ids. Toggling to the effective route of an already-open menu compared raw ids, so opening install.style and toggling it again re-summoned instead of closing. Menu now exposes canonicalRoute() mirroring the open path, shell toggle compares these, and link-follow fallback remains for third-party menu panels.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/shell.d/app-search-test.sh:150
- These assertions only match source-code tokens, so they can pass even if
canonicalRoute()returns the wrong route ortoggle()still hides/summons incorrectly. The PR's core route-switching behavior therefore has no executable regression coverage. Please exercisetoggle()with a loaded fake/menu item and assert different-route summon, same-route hide, alias/link handling, and dmenu cancellation (for example in a QML shell fixture).
/function toggle\(pluginId, payloadJson\) \{[\s\S]*?canonicalRoute[\s\S]*?resolvedTarget[\s\S]*?resolvedActive[\s\S]*?resolvedTarget !== resolvedActive[\s\S]*?return summon\(id, payloadJson\)/.test(shellQml),
'shell toggle compares canonical routes so summoning the already-active route closes the menu instead'
)
assert(
/function toggle[\s\S]*?kind === "link"[\s\S]*?ent\.target[\s\S]*?\}[\s\S]*?return hide/.test(shellQml),
'shell toggle falls back to menu link resolution when a panel lacks canonicalRoute'
shell.qml inlined the toggle route-switch decision, so its tests could only assert source tokens. Extract canonicalRoute() and shouldSummonForRoute() into MenuModel.js, delegate Menu.qml and shell.qml to them, and cover the decisions against a real menu heap: alias and link canonicalization, hiding an already-active route (including via alias/link), summoning only when routes differ, and never clobbering an active dmenu request.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
shell/plugins/menu/MenuModel.js:176
- “purecallers” is missing a space; use “pure callers” so the new API comment reads correctly.
// runtime alias resolver; purecallers without aliases omit it.
Summary
omarchy-menu toggle systemis invoked while the menu is already open at a different route (e.g.rootorapps),shell.qml'stoggle()used to immediately close the menu instead of navigating to the requested route (system).XF86PowerOff, rapidly toggling the menu open and closed 40 times per second.Fix
shell.qml'stoggle()to check if the menu is active at a different sub-route, callingsummon()to switch routes rather than closing.repeat = falseto theXF86PowerOffbinding inutilities.luato suppress key repeat.Review follow-up
Initial review noted
resolveRoute()expanded aliases but didn't follow link entries, so toggling a link route (e.g.install.style→style.install) re-summoned instead of closing. Also missing regression coverage.Menu.canonicalRoute()mirroring exactly what an open will display: aliases → link targets →rootfallback for unknown ids.shell.qmltoggle()now compares canonical routes (with a link-follow fallback for third-party menu panels).XF86PowerOffbinding test now assertslocked = trueandrepeating = false(test/shell.d/hyprland-default-config-test.sh).Round 2 (post-re-review)
MenuModel.canonicalRoute()/MenuModel.shouldSummonForRoute()), andMenu.qml+shell.qmldelegate to them.test/shell.d/menu-test.shagainst a real menu heap: alias and link canonicalization; toggling the already-active route (including via alias or link) hides; a different route summons; an active dmenu request is never clobbered; third-party panels with onlyresolveRoute/itemsget the same link-follow behavior.Testing: menu-test.sh 98 ok · app-search-test.sh green · hyprland-default-config-test.sh green · full
./test/shellgreen against the same single pre-existing environment baseline.