Add keyboard navigation and jump-to-message modal for long conversations#2361
Add keyboard navigation and jump-to-message modal for long conversations#2361Basit-Balogun10 wants to merge 12 commits into
Conversation
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/code/src/renderer/features/sessions/components/MessageJumpPicker.tsx:88-93
Superfluous alias: `allEntries` is always identical to `visibleEntries` and used only once in `handleSelect`. The intermediate name adds no clarity and violates the "no superfluous parts" rule.
```suggestion
const handleSelect = useCallback(
(id: string | null) => {
if (id === null) return;
const entry = visibleEntries.find((e) => e.id === id);
```
### Issue 2 of 3
apps/code/src/renderer/constants/keyboard-shortcuts.ts:282-305
**Dual source of truth for configurability**
`CONFIGURABLE_SHORTCUT_IDS` and the `configurable: true` flag on each `KeyboardShortcut` entry must be kept in sync manually. The flag controls which UI renderer (`ShortcutRecorder` vs `ShortcutKeys`) is shown in the sheet; the const array drives the TypeScript type and store logic. Adding a new shortcut requires updating both, but there is no compile-time or runtime guard that catches a mismatch — a shortcut could show `ShortcutRecorder` in the UI while the store type rejects its ID, or vice versa. Consider deriving one from the other: either generate `CONFIGURABLE_SHORTCUT_IDS` by filtering `KEYBOARD_SHORTCUTS` where `configurable === true`, or drop the `configurable` flag and compute it from membership in `CONFIGURABLE_SHORTCUT_IDS`.
### Issue 3 of 3
apps/code/src/renderer/features/sessions/components/ConversationView.tsx:217-221
**Jump picker doesn't anchor subsequent keyboard navigation**
`handleJumpToIndex` scrolls the list but never calls `setKeyboardFocusedMessageId`. After using `Cmd/Ctrl+J` to jump to a message, pressing `Alt+Up` / `Alt+Down` continues from whatever `keyboardFocusedMessageId` was before the picker opened (or from the first/last message if it was `null`), not from the just-jumped-to turn. A user who jump-navigates and then tries to step forward from that position will get a disorienting result. Consider calling `setKeyboardFocusedMessageId` with the target message's ID inside `handleJumpToIndex` (or pass the setter down to the picker).
Reviews (1): Last reviewed commit: "feat(code): add conversation message nav..." | Re-trigger Greptile |
| const allEntries = visibleEntries; | ||
|
|
||
| const handleSelect = useCallback( | ||
| (id: string | null) => { | ||
| if (id === null) return; | ||
| const entry = allEntries.find((e) => e.id === id); |
There was a problem hiding this comment.
Superfluous alias:
allEntries is always identical to visibleEntries and used only once in handleSelect. The intermediate name adds no clarity and violates the "no superfluous parts" rule.
| const allEntries = visibleEntries; | |
| const handleSelect = useCallback( | |
| (id: string | null) => { | |
| if (id === null) return; | |
| const entry = allEntries.find((e) => e.id === id); | |
| const handleSelect = useCallback( | |
| (id: string | null) => { | |
| if (id === null) return; | |
| const entry = visibleEntries.find((e) => e.id === id); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/sessions/components/MessageJumpPicker.tsx
Line: 88-93
Comment:
Superfluous alias: `allEntries` is always identical to `visibleEntries` and used only once in `handleSelect`. The intermediate name adds no clarity and violates the "no superfluous parts" rule.
```suggestion
const handleSelect = useCallback(
(id: string | null) => {
if (id === null) return;
const entry = visibleEntries.find((e) => e.id === id);
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| editor: "Editor", | ||
| }; | ||
|
|
||
| export const CONFIGURABLE_SHORTCUT_IDS = [ | ||
| "command-menu", | ||
| "new-task", | ||
| "settings", | ||
| "shortcuts", | ||
| "inbox", | ||
| "prev-task", | ||
| "next-task", | ||
| "space-up", | ||
| "space-down", | ||
| "go-back", | ||
| "go-forward", | ||
| "toggle-left-sidebar", | ||
| "toggle-review-panel", | ||
| "close-tab", | ||
| "open-in-editor", | ||
| "copy-path", | ||
| "toggle-focus", | ||
| "file-picker", | ||
| "message-prev", | ||
| "message-next", |
There was a problem hiding this comment.
Dual source of truth for configurability
CONFIGURABLE_SHORTCUT_IDS and the configurable: true flag on each KeyboardShortcut entry must be kept in sync manually. The flag controls which UI renderer (ShortcutRecorder vs ShortcutKeys) is shown in the sheet; the const array drives the TypeScript type and store logic. Adding a new shortcut requires updating both, but there is no compile-time or runtime guard that catches a mismatch — a shortcut could show ShortcutRecorder in the UI while the store type rejects its ID, or vice versa. Consider deriving one from the other: either generate CONFIGURABLE_SHORTCUT_IDS by filtering KEYBOARD_SHORTCUTS where configurable === true, or drop the configurable flag and compute it from membership in CONFIGURABLE_SHORTCUT_IDS.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/constants/keyboard-shortcuts.ts
Line: 282-305
Comment:
**Dual source of truth for configurability**
`CONFIGURABLE_SHORTCUT_IDS` and the `configurable: true` flag on each `KeyboardShortcut` entry must be kept in sync manually. The flag controls which UI renderer (`ShortcutRecorder` vs `ShortcutKeys`) is shown in the sheet; the const array drives the TypeScript type and store logic. Adding a new shortcut requires updating both, but there is no compile-time or runtime guard that catches a mismatch — a shortcut could show `ShortcutRecorder` in the UI while the store type rejects its ID, or vice versa. Consider deriving one from the other: either generate `CONFIGURABLE_SHORTCUT_IDS` by filtering `KEYBOARD_SHORTCUTS` where `configurable === true`, or drop the `configurable` flag and compute it from membership in `CONFIGURABLE_SHORTCUT_IDS`.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| useHotkeys(previousMessageKey, () => handleNavigateMessage(-1), { | ||
| enableOnFormTags: true, | ||
| enableOnContentEditable: true, | ||
| preventDefault: true, |
There was a problem hiding this comment.
Jump picker doesn't anchor subsequent keyboard navigation
handleJumpToIndex scrolls the list but never calls setKeyboardFocusedMessageId. After using Cmd/Ctrl+J to jump to a message, pressing Alt+Up / Alt+Down continues from whatever keyboardFocusedMessageId was before the picker opened (or from the first/last message if it was null), not from the just-jumped-to turn. A user who jump-navigates and then tries to step forward from that position will get a disorienting result. Consider calling setKeyboardFocusedMessageId with the target message's ID inside handleJumpToIndex (or pass the setter down to the picker).
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/sessions/components/ConversationView.tsx
Line: 217-221
Comment:
**Jump picker doesn't anchor subsequent keyboard navigation**
`handleJumpToIndex` scrolls the list but never calls `setKeyboardFocusedMessageId`. After using `Cmd/Ctrl+J` to jump to a message, pressing `Alt+Up` / `Alt+Down` continues from whatever `keyboardFocusedMessageId` was before the picker opened (or from the first/last message if it was `null`), not from the just-jumped-to turn. A user who jump-navigates and then tries to step forward from that position will get a disorienting result. Consider calling `setKeyboardFocusedMessageId` with the target message's ID inside `handleJumpToIndex` (or pass the setter down to the picker).
How can I resolve this? If you propose a fix, please make it concise.fdae5d7 to
ec8adb4
Compare
Users can now remap any of the 17 configurable shortcuts via Settings > Shortcuts (or the ⌘/ sheet). Custom bindings fully replace all defaults (including alternates) and multiple custom combos per action are supported. Bindings persist across sessions via electronStorage. - Add `configurable` flag + `DEFAULT_KEYBINDINGS` map to keyboard-shortcuts.ts - New `keybindingsStore` (persist + electronStorage) with array-based custom combos, conflict detection helper, and individual/bulk reset - New `useShortcut(id)` hook — reactive Zustand selector, feeds useHotkeys - New `Keycap` component extracted to avoid circular imports - New `ShortcutRecorder` component: click + to enter recording mode, captures keydown, shows conflict toast, per-binding × remove, per-shortcut ↩ reset - Update all useHotkeys call sites (GlobalEventHandlers, SpaceSwitcher, usePanelKeyboardShortcuts, ExternalAppsOpener) to use useShortcut() - KeyboardShortcutsSheet: configurable rows render ShortcutRecorder instead of static keycaps; "Reset all shortcuts" button shown when customisations exist Generated-By: PostHog Code Task-Id: 80405bf7-239f-4b60-a1cf-5a4777fb7218
Bare letter keys (e.g. just "k") would fire every time that character is typed anywhere in the app. Require at least mod/ctrl/alt to be held. Generated-By: PostHog Code Task-Id: 80405bf7-239f-4b60-a1cf-5a4777fb7218
24 tests covering resolveKey, addKeybinding, removeKeybinding, resetShortcut, resetAll, getKey, and findConflict — including conflict detection against comma-separated default alternates. Generated-By: PostHog Code Task-Id: 80405bf7-239f-4b60-a1cf-5a4777fb7218
- KeyboardShortcutsSheet header now reads the "shortcuts" key via useShortcut() so the trigger keycap updates when remapped - ExternalAppsOpener dropdown labels for open-in-editor and copy-path now derive from useShortcut() + formatHotkeyParts() instead of hardcoded Mac-only symbols test(e2e): add Playwright shortcut sheet tests Covers sheet open/close, category sections, hover controls, recording mode entry/cancellation, bare-key rejection, saving bindings, conflict detection, removing bindings, per-shortcut reset, and reset-all. Generated-By: PostHog Code Task-Id: 80405bf7-239f-4b60-a1cf-5a4777fb7218
Hardcoded Cmd glyphs were leaking onto Windows in the send-messages dropdown and the tiptap paste hint, and two handlers were gated on metaKey only so the corresponding shortcut never fired on Windows (mod+1..9 task switching, Cmd/Ctrl-click multi-select in the inbox). Generated-By: PostHog Code Task-Id: 80405bf7-239f-4b60-a1cf-5a4777fb7218
- Add prompt-history-prev/next to CONFIGURABLE_SHORTCUT_IDS and DEFAULT_KEYBINDINGS so they appear in the shortcuts sheet and can be rebound like any other shortcut - Add tiptapEventToCombo() — accepts shift-only combos (no Ctrl/Meta required) so shift+up/down can be matched against live bindings - Fix eventToCombo() to normalise Arrow-prefixed key names (ArrowUp to up) - Wire useTiptapEditor to resolve prompt-history keys from the store instead of hardcoding event.shiftKey - Fix paste hint toast to show the live paste-as-file binding instead of the hardcoded mod+shift+v string - Fix noStaticElementInteractions lint on recording modal backdrop - Rewrite E2E shortcut tests to match the current recording modal UI (chips + right-click context menu) rather than the old hover-button and inline-input design
- Deduplicate in updateKeybinding — conflict detection excludes the shortcut being edited so editing one binding to match another on the same shortcut could produce ["ctrl+q","ctrl+q"], duplicate React keys and broken chip reconciliation - Remove ArrowUp/Down gate around prompt-history navigation so custom non-arrow bindings (e.g. Ctrl+K) actually fire when pressed, not just when the physical key is an arrow - Remove obvious section-divider comments and redundant JSX labels (Header, Scrollable list, Sticky footer); keep non-obvious rationale comments (window-level capture, backdrop dismiss, canAddMore budget, dedup note, ArrowKey gate explanation)
ec8adb4 to
beb8ecd
Compare
Users can now filter conversation messages by date range directly in the jump picker. The filter row sits between the search input and the message list, showing two compact date inputs (From / To) with a clear button that appears when a filter is active. The message count in the footer reflects the active filter and notes when date filtering is on. Bounds for each input are derived from the earliest/latest message timestamps so the native date picker calendar starts at a sensible range.
Problem
Long conversations become hard to navigate once they grow beyond a few turns. Search (
Ctrl+F) can find text, but it cannot move between messages or jump to a specific turn in the conversation thread.Closes #2360. Builds on #2321 to make the shortcuts configurable
Changes
Alt/Option+UpandAlt/Option+Down.⌥↑and⌥↓.How did you test this?
Validated the touched renderer files with targeted static checks (
get_errors).Manual verification flow:
Option+UpandOption+Downto move backward and forward through user messages.Alt+UpandAlt+Downto verify the same navigation path.Cmd/Ctrl+Jto open the message jump picker.Demo
chat-timeline-modal-and-keyboard-navigation-posthog-code.webm
Publish to changelog?
no