diff --git a/.agents/skills/settings-ui-patterns/SKILL.md b/.agents/skills/settings-ui-patterns/SKILL.md index 58a894f7..2c731346 100644 --- a/.agents/skills/settings-ui-patterns/SKILL.md +++ b/.agents/skills/settings-ui-patterns/SKILL.md @@ -23,7 +23,7 @@ divs — use the primitives, and extend them (in the shared file) when a new shape is genuinely missing. - Flat hierarchy through spacing and typography; no cards, boxed backgrounds, or row chrome. -- Secondary helper text is hidden behind an info icon (`info` prop); the default view stays quiet. +- Secondary helper text is hidden behind an info icon (`info` prop) by default; the default view stays quiet. - Controls have one standard size (`h-9` / select `size="settings"`) and capped widths — no full-bleed inputs. - Layouts respond to the settings pane width via container queries (`@xl:` / `@3xl:`), never viewport `sm:`/`lg:` breakpoints (the pane is much narrower than the viewport inside the dialog). - Checkbox/radio state comes before labels; selected states are subtle and never shift layout. @@ -57,7 +57,8 @@ Do not introduce raw ``-based info icons, direct Remixicon components, ## Description Policy (info hints) -- Explanatory prose (what a feature does, when it applies) goes behind the info icon via the `info` prop — never as always-visible `description`. +- Explanatory prose goes behind the info icon via the `info` prop by default. +- When labels alone cannot explain the differences, consequences, or conditions needed to choose a setting, use a title, a visible description, then checkbox or radio controls. Large-text paste modes and send shortcuts with expanded-composer exceptions need this explanation. Having multiple options or a group title alone does not require a description; see `references/controls.md` for composition. - Stays visible: security/data-loss warnings, destructive consequences, required syntax/placeholder lists the user reads while typing, dynamic status, empty states, validation errors, active-flow wizard instructions. - Mixed text: keep the warning sentence visible, move the explanation to `info`. @@ -80,7 +81,7 @@ Dynamic entity rows normally are not indexed. Load `references/search.md` for ex ## Completion Criteria - Built from shared primitives; no ad-hoc page/section/row markup. -- Explanatory text hidden behind `info`; warnings/syntax/status still visible. +- Description placement follows the policy above; warnings/syntax/status remain visible. - Container-query (`@xl:`/`@3xl:`) responsiveness — no viewport breakpoints in pane content. - Controls use the standard size and width caps; no stretched full-width inputs. - Localized visible and accessibility text everywhere. diff --git a/.agents/skills/settings-ui-patterns/references/controls.md b/.agents/skills/settings-ui-patterns/references/controls.md index 08b36ada..2a2c0806 100644 --- a/.agents/skills/settings-ui-patterns/references/controls.md +++ b/.agents/skills/settings-ui-patterns/references/controls.md @@ -38,6 +38,9 @@ cells or when the control is wide; same `info` / `settingsItem` props. ## Boolean +For a self-explanatory enable/disable setting, use only a checkbox and label; +no separate group title or description is needed. + ```tsx - - + + + + + ``` Skip per-option descriptions when labels are self-explanatory. For short diff --git a/.agents/skills/settings-ui-patterns/references/layout.md b/.agents/skills/settings-ui-patterns/references/layout.md index 36007eac..f23cbea0 100644 --- a/.agents/skills/settings-ui-patterns/references/layout.md +++ b/.agents/skills/settings-ui-patterns/references/layout.md @@ -29,7 +29,7 @@ All primitives and class constants below live in | L2 | `SettingsSection` title (`SETTINGS_SECTION_TITLE_CLASS`) | Section | | L3 | `SettingsControlGroup` title (`SETTINGS_GROUP_TITLE_CLASS`) | Sub-cluster inside a section | | L4 | `SETTINGS_FIELD_LABEL_CLASS` | Field / control labels | -| Helper | `SETTINGS_HELPER_CLASS`, `SETTINGS_DESCRIPTION_CLASS` | Rare visible helper text (most goes behind `info`) | +| Helper | `SETTINGS_HELPER_CLASS`, `SETTINGS_DESCRIPTION_CLASS` | Rare visible helper text (most goes behind `info`; see the skill's Description Policy) | ## Navigation Placement @@ -63,5 +63,6 @@ pattern when touching nav. - Sections own vertical rhythm: divider + `py-8` come from `SettingsSection`. - Fields inside a column: `SETTINGS_FIELDS_STACK_CLASS` (`space-y-4`). - Checkbox/radio lists: `SETTINGS_OPTION_STACK_CLASS` (`space-y-1.5`). +- Groups requiring a title and visible description: separate them from preceding controls with `space-y-6` on the parent. Keep simple checkbox/radio lists compact. The title sits closer to its own description and controls than to the preceding group; use `SettingsControlGroup`'s internal spacing. - Two-column areas: `SettingsTwoColumn` (`@3xl:grid-cols-2`); use `SettingsStackedField` inside cells (a `SettingsFieldRow` overflows half-width columns). - No elevated backgrounds, rounded rows, or hover fills without explicit UX value. diff --git a/.agents/skills/theme-system/SKILL.md b/.agents/skills/theme-system/SKILL.md index 89e49a36..cb8225b6 100644 --- a/.agents/skills/theme-system/SKILL.md +++ b/.agents/skills/theme-system/SKILL.md @@ -61,6 +61,14 @@ Use `Button` from `packages/ui/src/components/ui/button.tsx`. Do not hardcode button height/padding when a size variant exists. Do not recreate selection/destructive styling with ad-hoc classes. +## Keyboard Navigation Contract + +- Menus, selects, and autocomplete pickers with ArrowDown/ArrowUp navigation must also support Ctrl+N/Ctrl+P, including submenus and searchable lists. +- Keep this behavior in shared components so callers inherit it. Use the keyboard mapping in `packages/ui/src/components/ui/dropdown-navigation.ts`; feature code must not duplicate key detection. +- Lists that own their active option or stop keyboard propagation must call the shared navigation helper at their own event boundary. Wrapping a custom list in a dropdown does not guarantee that its navigation events reach the wrapper. +- Route both key pairs through the same selection logic, preserving disabled-item skipping, boundary or wrap behavior, highlight, and scroll visibility. Consume each navigation event once, only while the menu or picker is active; preserve IME text entry and other modifier chords. +- Verify Ctrl+N/P alongside arrow keys in the real component, including search-input focus, submenus, and closed state. A key-mapping unit test alone does not verify event propagation or focus behavior. + ## Icon Contract ```tsx @@ -82,6 +90,7 @@ For any other technique, load `performance-engineering` and `scripts/perf/DOCUME - Animations are limited to `transform` and `opacity`, or their cost was measured and accepted. - No hardcoded/palette colors were introduced. - Buttons use shared variants and sizes. +- Menus and pickers satisfy the keyboard navigation contract without caller-specific key handling for standard shared components. - Icons use `Icon`/`IconName`, and generated sprite changes are intentional. - Hover, selection, primary, and status semantics are distinct. - Light/dark/high-contrast and long-text states remain legible. diff --git a/packages/ui/src/components/chat/ChatInput.tsx b/packages/ui/src/components/chat/ChatInput.tsx index fba574ef..dfa31029 100644 --- a/packages/ui/src/components/chat/ChatInput.tsx +++ b/packages/ui/src/components/chat/ChatInput.tsx @@ -51,6 +51,7 @@ import { parseAgentMentions } from '@/lib/messages/agentMentions'; import { CONTEXT_METADATA_KEY, draftFromContextPayload } from '@/lib/messages/contextParts'; import { ComposerStatusBar } from './ComposerStatusBar'; import { shouldSubmitEnter } from './composer/keyboardPolicy'; +import { getDropdownNavigationKey } from '@/components/ui/dropdown-navigation'; import { PendingChangesBar } from './PendingChangesBar'; import { useChatColumnSession } from './chatColumnSession'; import { useChatSurfaceMode } from './useChatSurfaceMode'; @@ -1873,40 +1874,17 @@ const ChatInputComponent: React.FC = ({ return; } - if (openAutocomplete === 'command' && commandRef.current) { - if (e.key === 'Enter' || e.key === 'ArrowUp' || e.key === 'ArrowDown' || e.key === 'Escape' || e.key === 'Tab') { - e.preventDefault(); - e.stopPropagation(); - commandRef.current.handleKeyDown(e.key); - return; - } - } - - if (openAutocomplete === 'skill' && skillRef.current) { - if (e.key === 'Enter' || e.key === 'ArrowUp' || e.key === 'ArrowDown' || e.key === 'Escape' || e.key === 'Tab') { - e.preventDefault(); - e.stopPropagation(); - skillRef.current.handleKeyDown(e.key); - return; - } - } - - if (openAutocomplete === 'snippet' && snippetRef.current) { - if (e.key === 'Enter' || e.key === 'ArrowUp' || e.key === 'ArrowDown' || e.key === 'Escape' || e.key === 'Tab') { - e.preventDefault(); - e.stopPropagation(); - snippetRef.current.handleKeyDown(e.key); - return; - } - } - - if (openAutocomplete === 'mention' && mentionRef.current) { - if (e.key === 'Enter' || e.key === 'ArrowUp' || e.key === 'ArrowDown' || e.key === 'Escape' || e.key === 'Tab') { - e.preventDefault(); - e.stopPropagation(); - mentionRef.current.handleKeyDown(e.key); - return; - } + const autocomplete = openAutocomplete === 'command' ? commandRef.current + : openAutocomplete === 'skill' ? skillRef.current + : openAutocomplete === 'snippet' ? snippetRef.current + : openAutocomplete === 'mention' ? mentionRef.current + : null; + const autocompleteKey = getDropdownNavigationKey(e) ?? e.key; + if (autocomplete && (autocompleteKey === 'Enter' || autocompleteKey === 'ArrowUp' || autocompleteKey === 'ArrowDown' || autocompleteKey === 'Escape' || autocompleteKey === 'Tab')) { + e.preventDefault(); + e.stopPropagation(); + autocomplete.handleKeyDown(autocompleteKey); + return; } if (isDesktopExpanded && e.key === 'Escape') { diff --git a/packages/ui/src/components/chat/composer/DOCUMENTATION.md b/packages/ui/src/components/chat/composer/DOCUMENTATION.md index 3f6ee746..0df78f2f 100644 --- a/packages/ui/src/components/chat/composer/DOCUMENTATION.md +++ b/packages/ui/src/components/chat/composer/DOCUMENTATION.md @@ -200,9 +200,13 @@ and the send path reading the same grammar. result as transient local state that resets on every close, and commits through the existing project-change flow only on explicit activation. Filtering changes the result area below the anchored input without moving - the search field. The - worktree Select and the mobile bottom sheets are unchanged. The selectors only - consume their shared prefix while the draft target UI is mounted. + the search field. The worktree picker remains a Select; mobile keeps its + bottom sheets. The selectors only consume their shared prefix while the + draft target UI is mounted. + Keyboard selection returns focus to the current form's composer, including + when the selected value is unchanged. +- `ChatInput.tsx` maps Ctrl+N/P to the active command, skill, snippet, or + mention picker after its IME guard. ## Input recall ownership @@ -259,10 +263,13 @@ suites that install module mocks are order-dependent. ## Enter preference -`keyboardPolicy.ts` owns the submission decision. Until the Chat setting is -changed, desktop Enter sends, mobile and focus mode require Ctrl/Cmd+Enter, -and Shift-modified Enter does not send. An explicit choice applies across -shared composers; Ctrl/Cmd+Enter sends in either configured mode. +`keyboardPolicy.ts` owns the submission decision. The expanded desktop composer +always inserts a newline with Enter, including Shift+Enter, and sends with +Ctrl/Cmd+Enter; it ignores the Enter-to-send preference. Outside expanded mode, +until the Chat setting is changed, desktop Enter sends, mobile requires +Ctrl/Cmd+Enter, and Shift-modified Enter does not send. An explicit choice +applies across the other shared composers; Ctrl/Cmd+Enter sends in either +configured mode. CodeMirror's deferred mobile Enter loses modifier information. Untouched settings restore Shift to keep the original policy. Once configured, with mobile diff --git a/packages/ui/src/components/chat/composer/keyboardPolicy.test.ts b/packages/ui/src/components/chat/composer/keyboardPolicy.test.ts index 90559804..71c7ccbd 100644 --- a/packages/ui/src/components/chat/composer/keyboardPolicy.test.ts +++ b/packages/ui/src/components/chat/composer/keyboardPolicy.test.ts @@ -26,6 +26,10 @@ const enterPolicyCases: Array<[string, Partial, boolean]> = ['configured enabled Shift+Enter inserts a newline', { enterToSendConfigured: true, enterToSend: true, shiftKey: true }, false], ['configured disabled Enter inserts a newline', { enterToSendConfigured: true, enterToSend: false }, false], ['configured disabled Shift+Enter sends', { enterToSendConfigured: true, enterToSend: false, shiftKey: true }, true], + ['expanded composer Enter inserts a newline when Enter-to-send is enabled', { isDesktopExpanded: true, enterToSendConfigured: true, enterToSend: true }, false], + ['expanded composer Shift+Enter inserts a newline when Enter-to-send is disabled', { isDesktopExpanded: true, enterToSendConfigured: true, enterToSend: false, shiftKey: true }, false], + ['expanded composer Ctrl+Enter sends despite Enter-to-send being disabled', { isDesktopExpanded: true, enterToSendConfigured: true, ctrlKey: true }, true], + ['expanded composer Cmd+Enter sends despite Enter-to-send being enabled', { isDesktopExpanded: true, enterToSendConfigured: true, enterToSend: true, metaKey: true }, true], ['configured Ctrl+Enter always sends', { enterToSendConfigured: true, isMobile: true, isDesktopExpanded: true, shiftKey: true, ctrlKey: true }, true], ['configured Meta+Enter always sends', { enterToSendConfigured: true, isMobile: true, isDesktopExpanded: true, shiftKey: true, metaKey: true }, true], ]; @@ -33,8 +37,9 @@ const enterPolicyCases: Array<[string, Partial, boolean]> = describe('Enter key policy', () => { for (const surface of [{}, { isMobile: true }, { isDesktopExpanded: true }]) { for (const modifiers of [{}, { ctrlKey: true }, { metaKey: true }, { ctrlKey: true, metaKey: true }]) { - test(`untouched Shift+Enter does not submit: ${JSON.stringify({ ...surface, ...modifiers })}`, () => { - expect(shouldSubmitEnter(policy({ ...surface, ...modifiers, shiftKey: true }))).toBe(false); + test(`untouched Shift+Enter only submits with a modifier in expanded mode: ${JSON.stringify({ ...surface, ...modifiers })}`, () => { + expect(shouldSubmitEnter(policy({ ...surface, ...modifiers, shiftKey: true }))) + .toBe(Boolean(surface.isDesktopExpanded && (modifiers.ctrlKey || modifiers.metaKey))); }); } } diff --git a/packages/ui/src/components/chat/composer/keyboardPolicy.ts b/packages/ui/src/components/chat/composer/keyboardPolicy.ts index f59081d9..27ab1f80 100644 --- a/packages/ui/src/components/chat/composer/keyboardPolicy.ts +++ b/packages/ui/src/components/chat/composer/keyboardPolicy.ts @@ -9,8 +9,10 @@ export interface EnterKeyPolicyInput { } export const shouldSubmitEnter = (input: EnterKeyPolicyInput): boolean => { - const enterSendsByDefault = !input.isMobile && !input.isDesktopExpanded; const isCtrlEnter = input.ctrlKey || input.metaKey; + if (input.isDesktopExpanded) return isCtrlEnter; + + const enterSendsByDefault = !input.isMobile; if (!input.enterToSendConfigured) { return !input.shiftKey && (enterSendsByDefault || isCtrlEnter); } diff --git a/packages/ui/src/components/chat/composer/ui/DraftTargetSelectors.tsx b/packages/ui/src/components/chat/composer/ui/DraftTargetSelectors.tsx index ab326fb6..abccd7d6 100644 --- a/packages/ui/src/components/chat/composer/ui/DraftTargetSelectors.tsx +++ b/packages/ui/src/components/chat/composer/ui/DraftTargetSelectors.tsx @@ -163,6 +163,10 @@ export function DraftTargetSelectors(props: DraftTargetProps) { const [projectFocusReturn, setProjectFocusReturn] = React.useState(false); const projectTriggerRef = React.useRef(null); const worktreeTriggerRef = React.useRef(null); + // Controlled Select closes can omit finalFocus's interaction type. + const keyboardCloseRef = React.useRef(false); + const getComposerInput = () => projectTriggerRef.current?.closest('form')?.querySelector('[data-chat-input="true"] .cm-content'); + const getFinalFocus = () => keyboardCloseRef.current ? getComposerInput() : true; const projectSearchRef = React.useRef(null); // Preserve Select's dialog portal and main-area containment. const [projectPortalContainer, setProjectPortalContainer] = React.useState(null); @@ -190,6 +194,7 @@ export function DraftTargetSelectors(props: DraftTargetProps) { if (openPicker === null || !shouldDismissDropdown(event)) return; event.preventDefault(); event.stopPropagation(); + keyboardCloseRef.current = true; setOpenPicker(null); }; @@ -279,10 +284,10 @@ export function DraftTargetSelectors(props: DraftTargetProps) { setOpenPicker(null); }} onOpenChangeComplete={(open) => { - // Return focus after Base UI finishes closing so the - // trigger itself, not document body, keeps keyboard flow. + // Return focus after Base UI finishes closing so typing + // continues in this form's composer, including reselection. if (!open && projectFocusReturn) { - projectTriggerRef.current?.focus(); + (getComposerInput() ?? projectTriggerRef.current)?.focus(); setProjectFocusReturn(false); } // Focus the search once the popup mounts; the opening @@ -403,7 +408,10 @@ export function DraftTargetSelectors(props: DraftTargetProps) {