Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { HOTKEYS, type HotkeyId } from "../../registry";
import { useHotkeyOverridesStore } from "../../stores/hotkeyOverridesStore";
import { useKeyboardLayoutStore } from "../../stores/keyboardLayoutStore";
import { useKeyboardPreferencesStore } from "../../stores/keyboardPreferencesStore";
import type { ShortcutBinding } from "../../types";
import { bindingToDispatchChord } from "../../utils/binding";

Expand Down Expand Up @@ -33,8 +34,7 @@ export function getBinding(id: HotkeyId): ShortcutBinding | null {
* the bound handler on non-US layouts.
*/
export function getDispatchChord(id: HotkeyId): string | null {
return bindingToDispatchChord(
getBinding(id),
useKeyboardLayoutStore.getState().map,
);
const adaptive = useKeyboardPreferencesStore.getState().adaptiveLayoutEnabled;
const layoutMap = adaptive ? useKeyboardLayoutStore.getState().map : null;
return bindingToDispatchChord(getBinding(id), layoutMap);
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,22 @@ import { useMemo } from "react";
import { formatHotkeyDisplay } from "../../display";
import { PLATFORM } from "../../registry";
import { useKeyboardLayoutStore } from "../../stores/keyboardLayoutStore";
import { useKeyboardPreferencesStore } from "../../stores/keyboardPreferencesStore";
import type { HotkeyDisplay, ShortcutBinding } from "../../types";
import { bindingToDispatchChord } from "../../utils/binding";
import { useBinding } from "../useBinding";

/** Effective layout map for display: null when the user has disabled
* adaptive layout mapping, so glyphs match the authored chord. */
function useActiveLayoutMap(): ReadonlyMap<string, string> | null {
const layoutMap = useKeyboardLayoutStore((s) => s.map);
const adaptive = useKeyboardPreferencesStore((s) => s.adaptiveLayoutEnabled);
return adaptive ? layoutMap : null;
}
Comment on lines +12 to +16
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.

P2 Unnecessary layout-store subscription when adaptive is off

useActiveLayoutMap always subscribes to useKeyboardLayoutStore, so every keyboard-layout change (e.g. user switching input sources) triggers a re-render of every useHotkeyDisplay/useFormatBinding consumer even when adaptive layout is disabled and the return value is always null. Moving the selector condition inside the store subscription avoids these phantom re-renders.

Suggested change
function useActiveLayoutMap(): ReadonlyMap<string, string> | null {
const layoutMap = useKeyboardLayoutStore((s) => s.map);
const adaptive = useKeyboardPreferencesStore((s) => s.adaptiveLayoutEnabled);
return adaptive ? layoutMap : null;
}
function useActiveLayoutMap(): ReadonlyMap<string, string> | null {
const adaptive = useKeyboardPreferencesStore((s) => s.adaptiveLayoutEnabled);
const layoutMap = useKeyboardLayoutStore((s) => (adaptive ? s.map : null));
return layoutMap;
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/hotkeys/hooks/useHotkeyDisplay/useHotkeyDisplay.ts
Line: 12-16

Comment:
**Unnecessary layout-store subscription when adaptive is off**

`useActiveLayoutMap` always subscribes to `useKeyboardLayoutStore`, so every keyboard-layout change (e.g. user switching input sources) triggers a re-render of every `useHotkeyDisplay`/`useFormatBinding` consumer even when adaptive layout is disabled and the return value is always `null`. Moving the selector condition inside the store subscription avoids these phantom re-renders.

```suggestion
function useActiveLayoutMap(): ReadonlyMap<string, string> | null {
	const adaptive = useKeyboardPreferencesStore((s) => s.adaptiveLayoutEnabled);
	const layoutMap = useKeyboardLayoutStore((s) => (adaptive ? s.map : null));
	return layoutMap;
}
```

How can I resolve this? If you propose a fix, please make it concise.


export function useHotkeyDisplay(id: string): HotkeyDisplay {
const binding = useBinding(id as Parameters<typeof useBinding>[0]);
const layoutMap = useKeyboardLayoutStore((s) => s.map);
const layoutMap = useActiveLayoutMap();
const chord = bindingToDispatchChord(binding, layoutMap);
return useMemo(
() => formatHotkeyDisplay(chord, PLATFORM, layoutMap),
Expand All @@ -25,7 +34,7 @@ export function useHotkeyDisplay(id: string): HotkeyDisplay {
export function useFormatBinding(
binding: ShortcutBinding | null,
): HotkeyDisplay {
const layoutMap = useKeyboardLayoutStore((s) => s.map);
const layoutMap = useActiveLayoutMap();
const chord = bindingToDispatchChord(binding, layoutMap);
return useMemo(
() => formatHotkeyDisplay(chord, PLATFORM, layoutMap),
Expand Down
5 changes: 4 additions & 1 deletion apps/desktop/src/renderer/hotkeys/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ export {
useRecordHotkeys,
} from "./hooks";
export { HOTKEYS, type HotkeyId, PLATFORM } from "./registry";
export { useHotkeyOverridesStore } from "./stores";
export {
useHotkeyOverridesStore,
useKeyboardPreferencesStore,
} from "./stores";
export type {
BindingMode,
HotkeyCategory,
Expand Down
1 change: 1 addition & 0 deletions apps/desktop/src/renderer/hotkeys/stores/index.ts
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
export { useHotkeyOverridesStore } from "./hotkeyOverridesStore";
export { useKeyboardPreferencesStore } from "./keyboardPreferencesStore";
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { create } from "zustand";
import { createJSONStorage, persist } from "zustand/middleware";

interface KeyboardPreferencesState {
/** Opt-in: when true, logical bindings are translated through the OS
* keyboard layout — e.g. `⌘Z` dispatches to physical KeyY on QWERTZ.
* Defaults to false so bindings dispatch and display as if on US-ANSI
* regardless of the current input source. */
adaptiveLayoutEnabled: boolean;
setAdaptiveLayoutEnabled: (enabled: boolean) => void;
}

export const useKeyboardPreferencesStore = create<KeyboardPreferencesState>()(
persist(
(set) => ({
adaptiveLayoutEnabled: false,
setAdaptiveLayoutEnabled: (enabled) =>
set({ adaptiveLayoutEnabled: enabled }),
}),
{
name: "keyboard-preferences",
storage: createJSONStorage(() => localStorage),
partialize: (state) => ({
adaptiveLayoutEnabled: state.adaptiveLayoutEnabled,
}),
},
),
);
29 changes: 23 additions & 6 deletions apps/desktop/src/renderer/hotkeys/utils/resolveHotkeyFromEvent.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,19 @@
import { HOTKEYS, type HotkeyId } from "../registry";
import { useHotkeyOverridesStore } from "../stores/hotkeyOverridesStore";
import { useKeyboardLayoutStore } from "../stores/keyboardLayoutStore";
import { useKeyboardPreferencesStore } from "../stores/keyboardPreferencesStore";
import type { ShortcutBinding } from "../types";
import { bindingToDispatchChord } from "./binding";

/** Layout map for the resolver — null when the user has disabled adaptive
* layout mapping, so logical bindings keep their authored chord. */
function activeLayoutMap(): ReadonlyMap<string, string> | null {
if (!useKeyboardPreferencesStore.getState().adaptiveLayoutEnabled) {
return null;
}
return useKeyboardLayoutStore.getState().map;
}

/**
* KeyboardEvent → registered {@link HotkeyId}, or `null` if unbound. Uses the
* same `event.code` normalization as react-hotkeys-hook so the reverse index
Expand Down Expand Up @@ -134,21 +144,28 @@ function buildRegisteredAppChords(
return map;
}

// Reassigned on each override OR layout change; `let` is required so the
// subscribe callbacks can replace the reference the resolver reads.
// Reassigned on each override, layout, OR adaptive-layout-toggle change;
// `let` is required so the subscribe callbacks can replace the reference
// the resolver reads.
let registeredAppChords = buildRegisteredAppChords(
useHotkeyOverridesStore.getState().overrides,
useKeyboardLayoutStore.getState().map,
activeLayoutMap(),
);
useHotkeyOverridesStore.subscribe((state) => {
registeredAppChords = buildRegisteredAppChords(
state.overrides,
useKeyboardLayoutStore.getState().map,
activeLayoutMap(),
);
});
useKeyboardLayoutStore.subscribe(() => {
registeredAppChords = buildRegisteredAppChords(
useHotkeyOverridesStore.getState().overrides,
activeLayoutMap(),
);
});
useKeyboardLayoutStore.subscribe((state) => {
useKeyboardPreferencesStore.subscribe(() => {
registeredAppChords = buildRegisteredAppChords(
useHotkeyOverridesStore.getState().overrides,
state.map,
activeLayoutMap(),
);
});
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import {
import { Button } from "@superset/ui/button";
import { Input } from "@superset/ui/input";
import { Kbd, KbdGroup } from "@superset/ui/kbd";
import { Label } from "@superset/ui/label";
import { toast } from "@superset/ui/sonner";
import { Switch } from "@superset/ui/switch";
import { cn } from "@superset/ui/utils";
import { createFileRoute } from "@tanstack/react-router";
import { useMemo, useState } from "react";
Expand All @@ -22,6 +24,7 @@ import {
useFormatBinding,
useHotkeyDisplay,
useHotkeyOverridesStore,
useKeyboardPreferencesStore,
useRecordHotkeys,
} from "renderer/hotkeys";

Expand Down Expand Up @@ -137,6 +140,13 @@ function KeyboardShortcutsPage() {
const resetAll = useHotkeyOverridesStore((s) => s.resetAll);
const setOverride = useHotkeyOverridesStore((s) => s.setOverride);

const adaptiveLayoutEnabled = useKeyboardPreferencesStore(
(s) => s.adaptiveLayoutEnabled,
);
const setAdaptiveLayoutEnabled = useKeyboardPreferencesStore(
(s) => s.setAdaptiveLayoutEnabled,
);

useRecordHotkeys(recordingId, {
// New printable bindings follow the printed character (matches what the
// user sees on their keyboard). F-keys / named keys are forced to
Expand Down Expand Up @@ -215,6 +225,25 @@ function KeyboardShortcutsPage() {
</Button>
</div>

{/* Preferences */}
<div className="mb-8 flex items-center justify-between gap-4">
<div className="space-y-0.5">
<Label htmlFor="adaptive-layout" className="text-sm font-medium">
Adaptive layout mapping
</Label>
<p className="text-xs text-muted-foreground">
Remap shortcuts to your current keyboard layout (e.g. ⌘Z dispatches
to physical KeyY on QWERTZ). When off, shortcuts always match the
keys you bound.
</p>
</div>
<Switch
id="adaptive-layout"
checked={adaptiveLayoutEnabled}
onCheckedChange={setAdaptiveLayoutEnabled}
/>
</div>

{/* Search */}
<div className="relative mb-6">
<HiMagnifyingGlass className="absolute left-3 top-1/2 -translate-y-1/2 h-4 w-4 text-muted-foreground" />
Expand Down
Loading