support keyboard-first navigation#3963
Conversation
✅ Deploy Preview for hyprnote-storybook canceled.
|
✅ Deploy Preview for hyprnote canceled.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| useHotkeys( | ||
| "mod+alt+left", | ||
| "meta+alt+left", |
There was a problem hiding this comment.
🔴 Changing mod to meta breaks top-level tab navigation on Windows/Linux
The hotkey for switching between top-level tabs was changed from mod+alt+left/right to meta+alt+left/right. In react-hotkeys-hook, mod is a cross-platform alias that maps to Meta (Cmd) on macOS and Control on Windows/Linux, whereas meta always maps to the Meta key specifically (Cmd on macOS, Windows key on Windows/Linux).
Root Cause and Impact
On macOS this change has no effect (both mod and meta resolve to Cmd). However, on Windows/Linux the shortcut changes from Ctrl+Alt+Left/Right to Win+Alt+Left/Right. The Win+Alt+Arrow combination is typically intercepted by the OS for window management, making the shortcut effectively unusable.
Every other hotkey in this file (index.tsx:851, index.tsx:868, index.tsx:879, index.tsx:915, etc.) consistently uses mod for cross-platform compatibility. The new sub-tab navigation shortcuts in ai.tsx:110 and settings.tsx:103 use ctrl+alt+left/right, which would have conflicted with the old mod+alt+left/right on Windows/Linux (since mod = Ctrl there). It appears the intent was to separate the two shortcuts on all platforms, but meta is the wrong choice for Windows/Linux — the top-level shortcut should likely remain mod+alt+left/right and the sub-tab shortcut should use a different key combination entirely.
Impact: Top-level tab navigation via keyboard is completely broken on Windows/Linux.
Prompt for agents
The root problem is that on Windows/Linux, `mod` resolves to `Ctrl`, so both the top-level tab navigation (`mod+alt+left/right`) and the new sub-tab navigation (`ctrl+alt+left/right`) would use the same physical keys. Simply reverting `meta` back to `mod` at lines 934 and 945 in index.tsx would restore Windows/Linux support but re-introduce the conflict with the sub-tab shortcuts in ai.tsx and settings.tsx. You need to pick a different key combination for one of the two levels. For example, keep `mod+alt+left/right` for top-level tabs (lines 934 and 945 in index.tsx) and change the sub-tab navigation in ai.tsx (lines 110, 125) and settings.tsx (lines 103, 118) to a non-conflicting shortcut such as `ctrl+shift+left/right` or `alt+left/right`.
Was this helpful? React with 👍 or 👎 to provide feedback.
Replace focus and aria-selected classes with data-selected attribute for better visual feedback when options are selected. This provides more consistent styling behavior across different interaction states.
Add Ctrl+Alt+Left/Right hotkeys to navigate between tabs in AI and Settings components. Import react-hotkeys-hook and implement tab switching logic that cycles through available sections. Change existing navigation hotkeys from 'mod' to 'meta' modifier in main body component to avoid conflicts with new tab navigation shortcuts.
Add visual highlight styling for active settings items during keyboard navigation. Implements background color, border radius, box shadow, and padding adjustments to improve accessibility and user experience when navigating settings with keyboard.
Implement keyboard navigation for settings panels with arrow key movement and space/enter activation. Add visual highlighting with smooth scrolling and click-to-select functionality. Include smart detection to disable navigation when editing fields or when popovers are open to prevent conflicts with existing interactions.
Add data-settings-item and data-settings-activate attributes to settings components and integrate useSettingsNavigation hook. The data attributes enable keyboard navigation and accessibility features for settings panels. The hook provides programmatic navigation between different settings items and their interactive elements like buttons and switches.
Remove URL validation and HTTPS API key requirement from aiProviderSchema. Change base_url from z.url() to z.string() and remove the refine validation that enforced API keys for HTTPS URLs to make the schema more flexible.
ComputelessComputer
force-pushed
the
keyboard-first
branch
from
f8cc6e2 to
da07a02
Compare
| useHotkeys( | ||
| "ctrl+alt+left", | ||
| () => { | ||
| if (currentIndex > 0) { | ||
| setActiveTab(enabledMenuKeys[currentIndex - 1]); | ||
| } | ||
| }, | ||
| { | ||
| preventDefault: true, | ||
| enableOnFormTags: true, | ||
| enableOnContentEditable: true, | ||
| }, | ||
| [currentIndex, setActiveTab], | ||
| ); | ||
|
|
||
| useHotkeys( | ||
| "ctrl+alt+right", | ||
| () => { | ||
| if (currentIndex >= 0 && currentIndex < enabledMenuKeys.length - 1) { | ||
| setActiveTab(enabledMenuKeys[currentIndex + 1]); | ||
| } | ||
| }, | ||
| { | ||
| preventDefault: true, | ||
| enableOnFormTags: true, | ||
| enableOnContentEditable: true, | ||
| }, | ||
| [currentIndex, setActiveTab], | ||
| ); |
There was a problem hiding this comment.
🚩 ctrl+alt+left/right hotkey collision between AI/Settings sub-tabs and note-input tabs
The new ctrl+alt+left/right hotkeys added here and in settings.tsx:104-131 use the same key combination as the pre-existing hotkeys in apps/desktop/src/components/main/body/sessions/note-input/index.tsx:352-394. In react-hotkeys-hook, all mounted handlers for the same key fire. If both the AI/Settings view and the note-input view are mounted simultaneously (e.g., multiple tabs rendered in the DOM), pressing ctrl+alt+left would trigger both handlers. However, based on the tab rendering pattern (conditional rendering with {activeTab === ...}), only the active tab's content component should be mounted, which likely prevents the conflict. This warrants investigation to confirm that inactive tab content is truly unmounted rather than hidden.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function hasOpenPopover(): boolean { | ||
| return ( | ||
| document.querySelector( | ||
| '[aria-expanded="true"], [data-state="open"][role="dialog"]', | ||
| ) !== null | ||
| ); | ||
| } |
There was a problem hiding this comment.
🚩 hasOpenPopover uses global document query — may match unrelated elements
The hasOpenPopover() function at line 23-29 queries the entire document for [aria-expanded="true"] or [data-state="open"][role="dialog"]. This is a global check — it will match any element in the document with these attributes, not just those within the settings panel. For example, a combobox in the header, a tooltip, or a dropdown in a different part of the app could cause hasOpenPopover() to return true, blocking keyboard navigation in settings even though the popover is unrelated. Scoping the query to the scrollRef container would be more precise, though it depends on whether Radix popovers portal outside the container (they typically do, rendering to document.body). This is a tradeoff: scoping would miss portaled popovers, but global matching creates false positives.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-Authored-By: john@hyprnote.com <john@hyprnote.com>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if (direction === "down") { | ||
| setActiveIndex(current < items.length - 1 ? current + 1 : current); | ||
| } else { | ||
| setActiveIndex(current > 0 ? current - 1 : 0); | ||
| } |
There was a problem hiding this comment.
Navigation logic bug when current is -1 (initial/no selection). Pressing the up arrow from no selection state (current === -1) sets activeIndex to 0 because -1 > 0 is false, taking the else branch setActiveIndex(0).
This creates unexpected behavior where up arrow always moves to first item from no-selection state, but down arrow also moves to first item (current + 1 = 0). Users expect up/down to navigate in opposite directions.
Fix:
if (direction === "down") {
setActiveIndex(current < 0 ? 0 : Math.min(current + 1, items.length - 1));
} else {
setActiveIndex(Math.max(current - 1, -1));
}| if (direction === "down") { | |
| setActiveIndex(current < items.length - 1 ? current + 1 : current); | |
| } else { | |
| setActiveIndex(current > 0 ? current - 1 : 0); | |
| } | |
| if (direction === "down") { | |
| setActiveIndex(current < 0 ? 0 : Math.min(current + 1, items.length - 1)); | |
| } else { | |
| setActiveIndex(Math.max(current - 1, -1)); | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
packages/store/src/zod.ts
Outdated
| export const aiProviderSchema = z.object({ | ||
| type: z.enum(["stt", "llm"]), | ||
| base_url: z.string(), | ||
| api_key: z.string(), | ||
| }); |
There was a problem hiding this comment.
🚩 aiProviderSchema validation significantly relaxed — intentional or accidental?
The base_url field was changed from z.url().min(1) to z.string(), and the .refine() that enforced API key requirements for HTTPS URLs was removed entirely. This schema is used as the onChange form validator in apps/desktop/src/components/settings/ai/shared/index.tsx:133 (validators: { onChange: aiProviderSchema }), meaning users can now save completely arbitrary strings as base URLs with no validation feedback.
I considered this as a potential bug but decided against it: the old z.url().min(1) would reject empty strings, and the default form value uses config.baseUrl ?? "" (index.tsx:123). This means on first open of an unconfigured provider, the empty default would immediately fail validation and block the onChange auto-submit listener (index.tsx:127-131). The relaxation likely fixes this UX issue. However, the complete removal of URL validation (not even a conditional check) means truly invalid values like "not a url" are now silently accepted and will cause runtime connection failures later.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "hover:bg-neutral-200! data-[selected=true]:bg-neutral-200!", | ||
| ])} |
There was a problem hiding this comment.
🚩 searchable-select removes focus: and aria-selected: styles, relying solely on data-[selected=true]
The CommandItem styling was changed from focus:bg-neutral-200! hover:bg-neutral-200! aria-selected:bg-transparent to hover:bg-neutral-200! data-[selected=true]:bg-neutral-200!. This assumes the Command component (likely from cmdk) sets data-selected="true" on the active item. Modern versions of cmdk do use this attribute, but the old code handled both focus: and aria-selected: states. If the underlying cmdk version uses aria-selected instead of data-selected, the keyboard highlight on items would break. This is likely fine if the project uses cmdk v1+, but worth verifying the cmdk version.
Was this helpful? React with 👍 or 👎 to provide feedback.
…efault behind popover check Co-Authored-By: john@hyprnote.com <john@hyprnote.com>
packages/store/src/zod.ts
Outdated
| export const aiProviderSchema = z.object({ | ||
| type: z.enum(["stt", "llm"]), | ||
| base_url: z.string(), | ||
| api_key: z.string(), | ||
| }); |
There was a problem hiding this comment.
Critical validation removed from aiProviderSchema. The schema no longer:
- Validates that
base_urlis a valid URL (changed fromz.url().min(1)toz.string()) - Enforces that
api_keyis non-empty for HTTPS URLs (removed refinement check)
This allows invalid URLs and empty API keys to pass validation, which will cause runtime failures when attempting to connect to AI providers.
// Should restore validation:
export const aiProviderSchema = z
.object({
type: z.enum(["stt", "llm"]),
base_url: z.string().url("Must be a valid URL"),
api_key: z.string(),
})
.refine(
(data) => !data.base_url.startsWith("https:") || data.api_key.length > 0,
{
message: "API key is required for HTTPS URLs",
path: ["api_key"],
},
);This change appears unrelated to keyboard navigation and may have been included accidentally.
| export const aiProviderSchema = z.object({ | |
| type: z.enum(["stt", "llm"]), | |
| base_url: z.string(), | |
| api_key: z.string(), | |
| }); | |
| export const aiProviderSchema = z | |
| .object({ | |
| type: z.enum(["stt", "llm"]), | |
| base_url: z.string().url("Must be a valid URL"), | |
| api_key: z.string(), | |
| }) | |
| .refine( | |
| (data) => !data.base_url.startsWith("https:") || data.api_key.length > 0, | |
| { | |
| message: "API key is required for HTTPS URLs", | |
| path: ["api_key"], | |
| }, | |
| ); |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| export function useSettingsNavigation( | ||
| scrollRef: RefObject<HTMLElement | null>, | ||
| panelKey: string, | ||
| ) { |
There was a problem hiding this comment.
🚩 Multiple useSettingsNavigation instances could conflict if tabs are simultaneously mounted
Both SettingsView (settings.tsx:133) and AIView (ai.tsx:140) call useSettingsNavigation, which registers document-level hotkeys for down, up, space, and enter. If the tab rendering system keeps both components mounted (e.g., for tab state preservation), both sets of hotkeys would fire simultaneously, potentially navigating items in an invisible panel.
The current rendering in settings.tsx:179 uses renderContent() which conditionally renders one panel at a time within the settings view. But the question is whether SettingsView and AIView themselves can coexist. This depends on the tab container implementation — if inactive tab contents are unmounted, there's no issue. If they're hidden via CSS (display: none), both hooks remain active.
Was this helpful? React with 👍 or 👎 to provide feedback.
…se_url Co-Authored-By: john@hyprnote.com <john@hyprnote.com>
| [data-settings-item][data-active="true"] { | ||
| background-color: rgb(245 245 245); | ||
| border-radius: 8px; | ||
| box-shadow: 0 0 0 2px rgb(163 163 163); | ||
| padding: 8px; | ||
| margin: -8px; | ||
| } |
There was a problem hiding this comment.
🟡 Highlight CSS overrides pre-existing padding on Container elements, causing layout shift
When keyboard navigation highlights a Container element (used in the Account settings page), the global CSS rule [data-settings-item][data-active="true"] applies padding: 8px; margin: -8px;. This overrides the existing p-4 (16px) Tailwind padding on the <section> element in Container, causing the content inside to visibly jump inward by 8px.
Root Cause
The CSS rule at apps/desktop/src/styles/globals.css:210-216 is unlayered (outside any @layer), which in the CSS cascade takes precedence over Tailwind's layered @layer utilities styles. The padding: 8px unconditionally overrides whatever padding the element already has.
The Container component at apps/desktop/src/components/settings/general/account.tsx:425-428 renders:
<section data-settings-item class="bg-neutral-50 p-4 rounded-lg flex flex-col gap-4">When this item receives data-active="true" via keyboard navigation, the padding drops from 16px to 8px, causing a visible content shift. The margin: -8px expands the element outward, but the internal content still jumps. Items without pre-existing padding (like SettingRow, PermissionRow) are unaffected because padding: 8px; margin: -8px nets zero layout change for them.
Impact: Visual glitch/layout shift when navigating to Account settings sections (Profile, Plan) using keyboard arrows.
Prompt for agents
The highlight CSS at apps/desktop/src/styles/globals.css lines 210-216 uses fixed padding: 8px and margin: -8px which conflicts with elements that already have padding (notably the Container component in apps/desktop/src/components/settings/general/account.tsx which has p-4 = 16px). Options to fix:
1. Use outline or box-shadow only for the highlight effect (remove padding/margin manipulation):
[data-settings-item][data-active="true"] {
background-color: rgb(245 245 245);
border-radius: 8px;
box-shadow: 0 0 0 2px rgb(163 163 163);
}
2. Or use an outline instead of box-shadow to avoid any layout impact:
[data-settings-item][data-active="true"] {
background-color: rgb(245 245 245);
border-radius: 8px;
outline: 2px solid rgb(163 163 163);
outline-offset: 4px;
}
Either approach avoids overriding element padding and prevents the layout shift on Container elements.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Uh oh!
There was an error while loading. Please reload this page.