fix(desktop): slash/@ menu keyboard nav — cycle all items + Esc dismiss#37999
Merged
kshitijk4poor merged 2 commits intoJun 3, 2026
Merged
Conversation
The existing slash-menu fix (PR NousResearch#37937) shipped a unit test that drove the keydown reducer directly. It did not exercise the actual DOM event path — specifically the keyup-driven `refreshTrigger` that was the root cause — so it would not have caught a regression in that path. This adds a faithful @testing-library reproduction that mounts the real `useLiveCompletionAdapter` plus the index.tsx trigger wiring and fires real `keyDown` + `keyUp` event pairs on a contentEditable. It asserts: - ArrowDown cycles through ALL items (0,1,2,3,4,0,1), not just the first two - Escape closes the menu and keyup does not reopen it Reverting the fix (always-refresh keyup + unconditional setTriggerActive(0)) makes this test fail with the highlight stuck at the top — confirming it guards the real bug.
Follow-up to NousResearch#37937. That fix guarded the composer's keyup with `shouldSkipTriggerRefreshOnKeyUp(key, trigger !== null)`. The `trigger !== null` check is timing-fragile for Escape: Escape's *keydown* sets `trigger = null` and closes the menu, but in a real browser the *keyup* fires after a re-render, so the handler closure sees `trigger === null`, the guard returns false, `refreshTrigger` runs, re-detects the still-present `/` in the input, and instantly reopens the menu. (jsdom batches state synchronously so a unit test could not observe this -- only the running app does.) Replace the value-based guard with a `triggerKeyConsumedRef` set synchronously in keydown whenever the open popover consumes a nav/control key (Arrow/Enter/Tab/Escape). keyup consults and clears that ref, so it is immune to the keydown->re-render->keyup timing. Applied to both the main composer (chat/composer/index.tsx) and the message-edit composer (assistant-ui/thread.tsx). Removes the now-unused `shouldSkipTriggerRefreshOnKeyUp` helper and its unit test. The real-DOM regression test now fires keydown+keyup pairs through the ref-based handlers and asserts Esc closes and stays closed. Verified by running a production renderer build (Vite v8) under Electron against a local backend: ArrowDown/ArrowUp cycle the full list and Esc dismisses the menu without reopening.
davidgut1982
pushed a commit
to davidgut1982/hermes-agent
that referenced
this pull request
Jun 5, 2026
…h-nav-dom-regression-test fix(desktop): slash/@ menu keyboard nav — cycle all items + Esc dismiss
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
2 participants
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.
Problem
In the desktop app, the
/command palette (and@mentions) had two keyboard bugs:Both trace to the composer's
onKeyUpunconditionally runningrefreshTrigger, which re-detects the trigger and resets the active index. PR #37937 addressed symptom (1) and part of (2), but its keyup guard —shouldSkipTriggerRefreshOnKeyUp(key, trigger !== null)— was timing-fragile for Escape:trigger = nulland closes the menu.trigger === null, the guard returns false,refreshTriggerruns, re-detects the still-present/, and reopens the menu.Fix
Replace the value-based guard with a
triggerKeyConsumedRefthat is set synchronously in keydown whenever the open popover consumes a nav/control key (ArrowUp/ArrowDown/Enter/Tab/Escape).keyupconsults and clears that ref — immune to the keydown → re-render → keyup timing that defeated the old check.chat/composer/index.tsx) and the message-edit composer (assistant-ui/thread.tsx).shouldSkipTriggerRefreshOnKeyUphelper and its unit test.Tests
slash-nav-dom-repro.test.tsx: a real-DOM@testing-libraryreproduction that mounts the actualuseLiveCompletionAdapter+ trigger wiring and fires realkeyDown+keyUpevent pairs through the ref-based handlers. Asserts ArrowDown cycles through all items ([0,1,2,3,4,0,1]) and Esc closes + stays closed.text-utils.test.ts: keepsdetectTriggercoverage.Verification
tsc -bclean;vitestcomposer suite green (7/7); eslint introduces no new errors (two pre-existing repo lint errors on untouched lines remain).