Restore the selected menu row when going back

[houz42]

Summary

• setActiveMenu() always reset selectedIndex to 0, so pressing Left (or Backspace with an empty filter) to go back up a level in the Omarchy menu always landed on the first row instead of the item you'd drilled in from.
• navStack now remembers { id, filterText, selectedItemId } for the menu being left when descending. goBack() restores that filter (reloading volatile search providers if needed) so rebuildDisplay() regenerates the same view, then looks the remembered row up by itemId via a new indexOfItemId() and re-runs settleCursor() on it. The row is keyed by id rather than a raw index because the parent may have been filtered (search results) when the row was activated, and a raw index would land on an unrelated row once that filter is restored or cleared. Re-settling rather than forcing the cursor onto the found index means a row that went disabled while its submenu was open still resolves to the nearest selectable row instead of parking the cursor somewhere Enter won't run.

Test plan

• ./test/shell.d/menu-test.sh passes, including regression assertions covering the remembered row/filter, the identity-based lookup, and that the restored index is actually applied through settleCursor() (not just looked up)
• ./test/all — no new failures (pre-existing failures on this machine are environment-only: missing omarchy-pkgs checkout, file permissions, and flaky screen-capture/smoke tests unrelated to this change)
• Manually verified in the running shell: Right into a submenu from a non-first row, then Left back out re-highlights the original row; two levels down and back twice restores both levels; searching, descending into a drilldown result, and going back restores both the search text and the selected row; closing and re-summoning the menu starts fresh at root (navStack is cleared on open, so nothing leaks across sessions)

[Copilot]

Pull request overview

Restores parent-menu cursor position after returning from a submenu.

Changes:

• Stores menu IDs with selected indices in navigation history.
• Restores saved indices in goBack().
• Adds regression assertions for navigation state.

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 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
shell/plugins/menu/Menu.qml	Saves and restores cursor positions during navigation.
test/shell.d/menu-test.sh	Adds assertions for index preservation.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

shell/plugins/menu/Menu.qml:782

• setActiveMenu() rebuilds and calls settleCursor() while selectedIndex is still 0, then this block overwrites that settled result and unconditionally activates the remembered row. If that row became disabled while its submenu was open, the cursor is restored onto a non-selectable row, so the stated fallback for changed rows does not run. Set the remembered index first, then settle again so it remains selected when valid and advances to the nearest selectable row otherwise.

      var restored = root.indexOfItemId(previous.selectedItemId)
      if (restored >= 0) {
        root.selectedIndex = restored
        root.cursorActive = true
      }

[omarchybot]

Reviewed on a disposable VM running this branch's shell. Nothing blocking — the fix does what it says, and I confirmed it in the running menu rather than only on paper.

What ran. The worker's compositor was pointed at this branch (quickshell -n -p .../omarchy-pr-7657/shell), then ./test/shell.d/menu-test.sh — 124 passed, 0 failed — plus the adjacent suites that read Menu.qml: menu-guards-test.sh (15), app-search-test.sh (22), row-border-stability-test.sh (2), powerprofiles-set-test.sh (10), crash-capture-test.sh (5), plugin-clone-test.sh (20). All green.

Verified live. Descend from a non-first row and come back: the row is restored. Two levels down, back twice: both levels restored. Search develop, descend into a drilldown result, come back: the filter and the row both return. Close the menu while deep and re-summon: fresh at root on row 0, since openExistingMenu() clears navStack — nothing leaks across opens. Left at root with an empty stack: no-op, no crash.

Three things worth your attention, none of them blocking.

1. The new tests assert source text, not behaviour. Replacing root.selectedIndex = restored with root.selectedIndex = 0 on the worker — which switches the whole feature off — still gives 124 passed, 0 failed. The goBack() regex stops at var restored = ..., so the line doing the actual work is unasserted. That matches how the rest of menu-test.sh is written, so it is not a defect in the PR, but the regression coverage the description claims is not there.

2. goBack() sets cursorActive = true without checking rowSelectable() (Menu.qml:779-782). settleCursor() deliberately leaves the menu with no cursor rather than one "parked on a row Enter won't run" (its comment at Menu.qml:528), and the restore overrides that after the fact. Unreachable in the shipped menu — I checked all 325 entries in omarchy-menu.jsonc, and every one of the 61 disabled guards sits on a leaf action row while only menu and link rows are ever remembered — but a user extension that disables a submenu row would park the cursor on it, where Enter silently does nothing.

3. The restore is identity-keyed only at the instant it happens. After that it is a bare index, and any later rebuildDisplay() can move it onto a different entry. Reproduced on the worker: restore the cursor onto Remove, then hide two rows above it with a live extension edit, and the cursor ends up on About. I then checked whether this PR causes that, and it does not — the control, arrowing down to Remove with no back-navigation at all, moves the cursor to About in exactly the same way. So this is #7083, which #7413 addresses only for dmenu. Mentioning it because your comment says the row is "keyed by itemId rather than index", which is true of the lookup but not of what happens to it afterwards.

One thing to fix. The PR description still describes the first commit — { id, selectedIndex }, and "settleCursor() still lands on the nearest selectable row". The final code keys by { id, filterText, selectedItemId } and overrides settleCursor() after it has run. Worth updating before a maintainer reads it, since the stale version describes exactly the index-based approach your second commit replaced.

Second opinion. codex at xhigh reviewed independently and landed on the same three points. It rated the third High, on the reasoning that the PR "does not safely deliver its stated identity-based restoration"; the control run above is why I disagree — the behaviour is identical without this PR. Its read scope is not currently confined away from this review's own working files, so its independence is not guaranteed: where it agreed, that is agreement rather than confirmation.

Merge collisions. #7653 also edits Menu.qml, but only at lines 1438-1465 (the empty-state card), so it is clear of this. #7610 touches lines 80, 550, 1126, 1164, 1351 and 1368 — also clear. Draft #7659 edits test/shell.d/menu-test.sh at line 487, inside the block this PR rewrites, so whichever lands second needs a small rebase there.

Nothing pushed to your branch. This is waiting on a maintainer.

[houz42]

Thanks for the thorough live review — pushed 0949894 to address the two actionable points:

1. Cursor forced onto a possibly-disabled restored row: goBack() now calls root.settleCursor() after setting selectedIndex = restored instead of unconditionally flipping cursorActive = true, so a row that went disabled while its submenu was open falls through to the nearest selectable row, matching the invariant settleCursor() already enforces everywhere else.
2. Test coverage gap: added an assertion that specifically checks root.selectedIndex = restored followed by root.settleCursor() inside the if (restored >= 0) block. I reproduced your sabotage case locally (swap the assignment for = 0) — it now fails as expected.

Also updated the PR description to describe the actual final design ({ id, filterText, selectedItemId }) instead of the stale first-commit version.

On point 3 (drift after a later rebuildDisplay()) — agreed this is pre-existing (#7083) and not something this PR introduces or needs to fix; leaving it out of scope here.

[omarchybot]

Re-reviewed at 0949894. It fixes the disabled-row problem, and I proved it rather than reading it: same worker setup as last time, shell running on this branch.

The fix works, and here is the A/B. I built a throwaway menu extension with four rows under a Zprobe submenu — Alpha (itself a submenu), Bravo, Charlie — descended into Alpha, then flipped Alpha to disabled from outside while its submenu was open, then pressed Left. Under 0949894 the cursor comes back on Bravo, with Alpha ✓ dimmed and unhighlighted. Under the previous commit's root.cursorActive = true — reverted on the worker and the shell restarted so the change actually took, which the running shell does not do on file change alone — the cursor comes back parked on the dimmed Alpha. So the defect was real and reachable, and this commit closes it.

I also checked the branch that does not go through your new call. When the remembered itemId is gone from the rebuilt list, restored is -1 and the block is skipped entirely — but setActiveMenu() has already run rebuildDisplay(), which ends in settleCursor() (Menu.qml:677), so the cursor was settled before goBack() looked anything up. The "index went out of range because the list shrank" case cannot arise at all: restored comes from a scan of the current displayModel, so it is either a valid index or -1. And when every row is disabled, nextSelectable() returns -1 and the menu ends with no cursor rather than a fake one. All four branches are covered.

Two things I confirmed did not regress: descending from Delta (row 4) and coming back still restores Delta, so settleCursor() on an already-selectable index is the identity it should be; and the deferred revealCursor() from rebuildDisplay() runs after goBack() returns, so it still scrolls to the final row.

The test still asserts source text, and I want to be plain about what that buys you. Your new assertion does catch the exact sabotage you reproduced — swapping in root.selectedIndex = 0 fails it, 95 passed / 1 failed on the worker. But it is a regex over the QML, so it only pins those two lines. I turned the entire feature off with a one-token edit elsewhere — if (pushHistory && id !== root.activeMenu) to if (false), which stops anything from ever being pushed onto navStack — and the suite reported 125 passed, 0 failed, with menu-guards-test.sh green alongside it. The converse holds too: a behaviourally identical refactor, such as renaming restored, would fail the assertion while nothing changed. That is how the whole of menu-test.sh is written, so it is not a defect in your PR and I am not asking you to change it — but the description's "regression assertions" is a stronger claim than the file can support, and worth knowing if you ever lean on it.

The PR description now matches the code. Point 3 is agreed as pre-existing #7083 and out of scope.

Second opinion. codex at xhigh reviewed the new commit independently and reached the same conclusion on all four branches. It added one thing I had not traced: when the restored view has a search filter, a now-disabled row is not merely dim, it is absent — matchesQuery() passes !root.isDisabled(entry) as its visibility argument (Menu.qml:510, MenuModel.js:325), so search omits disabled rows entirely and that case takes the restored < 0 fallback rather than the disabled-row branch. It also confirmed dmenu and input mode cannot reach goBack() at all, since openDmenu() clears navStack. Its read scope is not currently confined away from this review's own working files, so where it agreed, that is agreement rather than independent confirmation; the search-filter mechanism is its own contribution.

One thing for the maintainer, not for you. #7074 ("Remember cursor position when navigating back in the root menu", nickav, 16 Aug) is the same feature in the same two files. It stores a raw { id, index } and reapplies it in setActiveMenu() after loadProviderForMenu(), with no filter restoration, no identity keying, and no settleCursor() — so it hits both the wrong-row-after-a-filter problem and the disabled-row problem this PR now handles. Yours covers strictly more. Which lands is not my call and I have not touched that PR. The collisions from last time are unchanged: #7653 and #7610 are clear of these lines; draft #7659 still edits test/shell.d/menu-test.sh inside the block this PR rewrites, so whichever lands second needs a small rebase there.

Tests. menu-test.sh 125 passed / 0 failed and menu-guards-test.sh 15 passed / 0 failed on the worker's own compositor, against the unmodified branch, after every mutation above was reverted. Nothing pushed to your branch. This is waiting on a maintainer.