Skip to content

feat(desktop): add keyboard navigation to clarify choices - #64346

Closed
mapu-og wants to merge 1 commit into
NousResearch:mainfrom
mapu-og:feat/desktop-clarify-keyboard-navigation
Closed

feat(desktop): add keyboard navigation to clarify choices#64346
mapu-og wants to merge 1 commit into
NousResearch:mainfrom
mapu-og:feat/desktop-clarify-keyboard-navigation

Conversation

@mapu-og

@mapu-og mapu-og commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a visual keyboard cursor to the Desktop clarify choices
  • support circular ArrowUp / ArrowDown navigation across every choice and the trailing Other field
  • support direct numeric selection with 19, while preserving the existing A/B/C shortcuts
  • let Enter confirm the selected answer or the currently highlighted row
  • keep global shortcuts inactive while the user is typing in an input, textarea, or editable field

Motivation

The Desktop clarify card already supports mouse input and letter shortcuts, but keyboard-heavy workflows still require users to target a specific letter or reach for the mouse. This makes the card behave like the other keyboard-oriented pickers in the app without changing the clarify.respond protocol or any backend behavior.

Behavior

  • arrow navigation wraps between the first choice and Other
  • moving the cursor clears any stale selection so the highlighted row is the source of truth
  • numeric and letter shortcuts select the corresponding answer
  • the number/letter after the final choice focuses Other
  • Enter submits an existing answer; with no explicit selection it submits the highlighted choice or focuses Other
  • shortcuts stand down while an input, textarea, or content-editable element has focus

Test plan

  • npx vitest run src/components/assistant-ui/clarify-tool.test.tsx — 9 tests passed
  • npm run typecheck
  • npx eslint src/components/assistant-ui/clarify-tool.tsx src/components/assistant-ui/clarify-tool.test.tsx — 0 errors
  • npx prettier --check src/components/assistant-ui/clarify-tool.tsx src/components/assistant-ui/clarify-tool.test.tsx
  • npm run build

Scope

Desktop renderer only. No tool schema, gateway, session, profile, or backend changes.

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) labels Jul 14, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused Desktop keyboard-navigation work. The feature is still absent on current main: apps/desktop/src/components/assistant-ui/clarify-tool.tsx:308-353 supports letter shortcuts only, with Enter limited to an existing answer.

Problems

  • The new unqualified Enter path at apps/desktop/src/components/assistant-ui/clarify-tool.tsx:400-406 is registered once per mounted clarify card. Current main mounts chat surfaces for the primary and every tile (apps/desktop/src/app/chat/index.tsx:428-431) and scopes each card to its own session request (clarify-tool.tsx:188-192). One body-level Enter could therefore answer multiple pending clarifications.
  • The new tests mount one card only (clarify-tool.test.tsx:67-80), so this multi-session path is untested.

Suggested changes

  • Gate shortcut handling to the focused clarify/session surface and add a primary-plus-tile regression test asserting only that request receives clarify.respond.
  • Salvage onto current main's session-scoped clarify implementation; GitHub currently reports this PR as dirty against main.

Automated hermes-sweeper review.

}

if (event.key === 'Enter' && pendingAnswer) {
if (event.key === 'Enter') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On current main, clarify cards can be mounted simultaneously for the primary surface and session tiles (apps/desktop/src/app/chat/index.tsx:428-431), each with a session-scoped request. Every ready card receives this window-level Enter and can call activateActive(), so a single Enter can answer multiple requests. Gate this to the focused clarify/session surface and add a multi-card regression test.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 16, 2026
@OutThisLife

Copy link
Copy Markdown
Collaborator

Superseded by #69799 — cherry-picked (authorship preserved) and re-authored onto the merged ChoiceButton + sanitized choice pipeline. Arrow/number/letter nav, Enter-to-confirm, and the full a11y wiring are all carried over with your tests. Thanks @mapu-og! Closing in favor of the rebased version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants