Skip to content

Commit a70aa5e

Browse files
committed
fix: address Copilot re-review feedback (round 5)
- Lift `commands` to GlobalKeybindings: previously CommandPalette and GlobalKeybindings each called `useCommandSources` independently. Now GlobalKeybindings owns the single Command[] instance and passes it to CommandPalette as a prop, so the palette, the shortcuts dialog, and the keydown listener all reference the exact same closures. - Move the "/" focus-search shortcut into useGlobalKeybindings' existing keydown listener so it inherits the established suppression rules: it now no-ops on touch-only devices via isMobileLike() and treats <select> as a typing target via isTypingTarget(), matching the rest of the global shortcuts. Removes the separate document-level listener that previously didn't share those guards.
1 parent c703b4f commit a70aa5e

3 files changed

Lines changed: 24 additions & 38 deletions

File tree

apps/web/src/components/command-palette/CommandPalette.tsx

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,16 @@ import { CommandPaletteFooter } from "./CommandPaletteFooter";
1818
import { CommandPaletteInput } from "./CommandPaletteInput";
1919
import { CommandPaletteList, getDefaultCommandPaletteRows } from "./CommandPaletteList";
2020
import { COMMAND_PALETTE_LISTBOX_ID, SEARCH_INPUT_ID } from "./constants";
21-
import { useCommandSources } from "./useCommandSources";
2221

2322
interface Props {
24-
/** Called when the user runs the "Show keyboard shortcuts" command. */
25-
onOpenShortcuts: () => void;
23+
/** Command list — provided by `GlobalKeybindings` so the palette, the
24+
* shortcuts dialog, and the global listener all share a single instance. */
25+
commands: Command[];
2626
}
2727

2828
const SEARCH_FALLBACK_ID = "search.fallback";
2929

30-
export function CommandPalette({ onOpenShortcuts }: Props) {
30+
export function CommandPalette({ commands }: Props) {
3131
const theme = useTheme();
3232
const isXs = useMediaQuery(theme.breakpoints.down("sm"));
3333
const t = useTranslations("commandPalette");
@@ -62,8 +62,6 @@ export function CommandPalette({ onOpenShortcuts }: Props) {
6262
});
6363
}, [close, query, setSearchQuery]);
6464

65-
const commands = useCommandSources({ openShortcutsDialog: onOpenShortcuts });
66-
6765
// Ranked filtered list when query is present, otherwise the raw command list.
6866
// When filtering, the synthetic "Search '<q>' on map" row is always appended so
6967
// it can be navigated by ↑↓ alongside real matches.

apps/web/src/components/command-palette/GlobalKeybindings.tsx

Lines changed: 4 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
"use client";
22

3-
import { useCommandPaletteStore } from "@openmapx/core";
4-
import { useCallback, useEffect, useState } from "react";
3+
import { useCallback, useState } from "react";
54
import { CommandPalette } from "./CommandPalette";
6-
import { SEARCH_INPUT_ID } from "./constants";
75
import { KeyboardShortcutsDialog } from "./KeyboardShortcutsDialog";
86
import { useCommandSources } from "./useCommandSources";
97
import { useGlobalKeybindings } from "./useGlobalKeybindings";
@@ -13,6 +11,8 @@ export function GlobalKeybindings() {
1311
// Stable references so useCommandSources' useMemo deps don't churn.
1412
const openShortcuts = useCallback(() => setShortcutsOpen(true), []);
1513
const closeShortcuts = useCallback(() => setShortcutsOpen(false), []);
14+
// Single source of truth — palette, shortcuts dialog, and listener all
15+
// share the same Command[] instance.
1616
const commands = useCommandSources({ openShortcutsDialog: openShortcuts });
1717

1818
useGlobalKeybindings({
@@ -21,36 +21,9 @@ export function GlobalKeybindings() {
2121
isShortcutsDialogOpen: shortcutsOpen,
2222
});
2323

24-
// Built-in: "/" focuses the SearchBar input. Suppressed while the palette
25-
// or shortcuts dialog is open so it doesn't focus the SearchBar behind a
26-
// modal, and (like the rest of the listener) while the user is typing.
27-
useEffect(() => {
28-
const onKey = (e: KeyboardEvent) => {
29-
if (e.key !== "/") return;
30-
// Plain "/" only — don't hijack browser/OS shortcuts like Ctrl+/, Cmd+/.
31-
// (Shift+/ produces "?" so it never reaches this branch on most layouts.)
32-
if (e.ctrlKey || e.metaKey || e.altKey) return;
33-
if (useCommandPaletteStore.getState().isOpen) return;
34-
if (shortcutsOpen) return;
35-
const target = e.target;
36-
if (target instanceof HTMLElement) {
37-
const tag = target.tagName;
38-
if (tag === "INPUT" || tag === "TEXTAREA" || target.isContentEditable) return;
39-
}
40-
const el = document.getElementById(SEARCH_INPUT_ID) as HTMLInputElement | null;
41-
if (el) {
42-
e.preventDefault();
43-
el.focus();
44-
el.select();
45-
}
46-
};
47-
document.addEventListener("keydown", onKey);
48-
return () => document.removeEventListener("keydown", onKey);
49-
}, [shortcutsOpen]);
50-
5124
return (
5225
<>
53-
<CommandPalette onOpenShortcuts={openShortcuts} />
26+
<CommandPalette commands={commands} />
5427
<KeyboardShortcutsDialog open={shortcutsOpen} onClose={closeShortcuts} commands={commands} />
5528
</>
5629
);

apps/web/src/components/command-palette/useGlobalKeybindings.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
useSidebarStore,
1515
} from "@openmapx/core";
1616
import { useEffect, useRef } from "react";
17+
import { SEARCH_INPUT_ID } from "./constants";
1718

1819
const SEQUENCE_TIMEOUT_MS = 1200;
1920

@@ -141,7 +142,21 @@ export function useGlobalKeybindings(opts: Options) {
141142
return;
142143
}
143144

144-
// 4. Match against registered command shortcuts (sequences allowed)
145+
// 4. Built-in "/" — focus the SearchBar. Plain "/" only; modifier
146+
// combos belong to the browser/OS. Inherits the typing-target,
147+
// dialog-open, and mobile-guard suppression rules above.
148+
if (event.key === "/" && !event.ctrlKey && !event.metaKey && !event.altKey) {
149+
const el = document.getElementById(SEARCH_INPUT_ID) as HTMLInputElement | null;
150+
if (el) {
151+
event.preventDefault();
152+
el.focus();
153+
el.select();
154+
}
155+
clearBuffer();
156+
return;
157+
}
158+
159+
// 5. Match against registered command shortcuts (sequences allowed)
145160
const sequences: KeySequence[] = [];
146161
const cmdsForSeq: Command[] = [];
147162
for (const c of commandsRef.current) {

0 commit comments

Comments
 (0)