ENG-1438: Port Keyboard shortcut keys/triggers settings

[sid597]

https://www.loom.com/share/c49cc82577c847368c521e0a7680fc5e

---

Summary by CodeRabbit

• Refactor
  • Enhanced keyboard shortcut handling and persistence for menu and tool triggers.
  • Restructured keyboard shortcut configuration storage format for improved consistency and validation.
  • Streamlined shortcut input components to use normalized key combination representations.

[linear]

ENG-1438 Port Keyboard shortcut keys/triggers settings

[supabase]

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[sid597]

• ENG-1438: Port Keyboard shortcut keys/triggers settings #794 👈 (View in Graphite)
• ENG-1440: Port page groups settings in suggestive mode #793
• ENG-1281: Port discourse node block panel #784 : 2 other dependent PRs (#795 , #805 )
• ENG-1290: Port discourse node config panel #769
• ENG-1291: Port discourse node specification #765
• ENG-1280: Port Discourse node: Query builder and editor component for block props #764
• ENG-1225: Discourse node migration simple settings #696 : 1 other dependent PR (#697 )
• ENG-1273: Migrate small personal settings #691
• ENG-1272 Migrate all small global settings components #688 : 1 other dependent PR (#690 )
• ENG-1345: Modify existing RoamJS component onChange to also change props #745 : 1 other dependent PR (#684 )
• ENG-1190 Replace existing roam-js components with blockProp based ones #689
• ENG-1188: prod: pull watchers for prop based settings #682
• ENG-1187: Prod: Implement get and set for block prop based settings #669
• ENG-1189: Prod: Init block prop based schema #666
• ENG-1330: zod schema for discourse relations #719
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 11 additional findings in Devin Review.

[sid597]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request refactors keyboard shortcut handling to use a structured data format with modifiers and key properties instead of plain strings, adds persistence of shortcuts via personal settings storage, and centralizes shortcut string generation through a new comboToString() utility function.

Changes

Cohort / File(s)	Summary
Keyboard Shortcut Utilities apps/roam/src/components/DiscourseNodeMenu.tsx	Added comboToString() utility function to normalize key combo objects into readable shortcut strings; refactored shortcut generation and state management to use this utility.
Node Menu Persistence apps/roam/src/components/DiscourseNodeMenu.tsx	Extended onKeyDown handler to store combo in state and persist via both extensionAPI.settings.set() and setPersonalSetting() calls; updated clear button to persist empty values.
Node Search Menu Persistence apps/roam/src/components/DiscourseNodeSearchMenu.tsx	Added setPersonalSetting() call in NodeSearchMenuTriggerSetting onChange handler to persist trigger changes alongside extension settings updates.
Keyboard Shortcut Input Refactor apps/roam/src/components/settings/KeyboardShortcutInput.tsx	Introduced blockPropKey prop to enable per-setting persistence; replaced custom normalization logic with comboToString() utility; added setPersonalSetting() calls in all modification paths (set, clear, keydown).
Shortcut Settings Configuration apps/roam/src/components/settings/HomePersonalSettings.tsx	Added blockPropKey prop assignment to KeyboardShortcutInput component during rendering.
Schema Type Updates apps/roam/src/components/settings/utils/zodSchema.ts, apps/roam/src/components/settings/utils/zodSchema.example.ts	Changed structure of shortcut fields (Personal node menu trigger, Discourse tool shortcut) from plain strings to objects with { modifiers: number; key: string } shape; updated schema validation and default values to match new structure.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• ENG-1187: Prod: Implement get and set for block prop based settings #669: Introduces setPersonalSetting() utilities that this PR directly depends on for persisting keyboard shortcuts.
• ENG-1105: zod schema for all the settings #664: Modifies the same zod schema files to define the { modifiers, key } shortcut structure that this PR adopts.
• ENG-775 - Add keyboard shortcut for base discourse tool #378: Updates keyboard shortcut feature components and settings that are modified by this PR's refactoring and persistence additions.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main objective of the PR: porting keyboard shortcut keys/triggers settings. This directly aligns with the comprehensive changes across multiple components and schema files.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

apps/roam/src/components/settings/utils/zodSchema.ts (1)

233-242: Asymmetric schema for the two shortcut fields — is the union on "Personal node menu trigger" intentional?

"Personal node menu trigger" accepts z.union([object, z.literal("")]) while "Discourse tool shortcut" only accepts the object shape. This means consumers of "Personal node menu trigger" must do a type-narrowing check (typeof value === "string") before accessing .modifiers / .key, which is easy to miss and could cause runtime errors.

If the empty-string variant is for backward compatibility with previously stored string values, consider using .transform() to normalize "" into { modifiers: 0, key: "" } at parse time so the output type is always uniform. If this asymmetry is intentional (the two fields genuinely have different semantics), a brief comment explaining why would help future readers.

Example normalization

   "Personal node menu trigger": z
     .union([
       z.object({ modifiers: z.number(), key: z.string() }),
       z.literal(""),
     ])
-    .default({ modifiers: 0, key: "" }),
+    .default({ modifiers: 0, key: "" })
+    .transform((val) => (val === "" ? { modifiers: 0, key: "" } : val)),

This way the inferred type is always { modifiers: number; key: string }, matching "Discourse tool shortcut".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/roam/src/components/settings/utils/zodSchema.ts` around lines 233 - 242,
The "Personal node menu trigger" schema currently uses z.union([z.object(...),
z.literal("")]) which produces an inconsistent output type vs the "Discourse
tool shortcut" object; change the schema so the parsed output is always the
object shape (matching "Discourse tool shortcut") by handling the empty-string
legacy case with a transform or preprocess: accept the union but .transform()
(or z.preprocess) to map "" to { modifiers: 0, key: "" } so downstream consumers
of "Personal node menu trigger" can safely access .modifiers and .key without
type-narrowing; alternatively make both schemas identical objects if no legacy
string values must be supported, and add a short comment explaining the chosen
approach.

apps/roam/src/components/DiscourseNodeMenu.tsx (1)

410-415: Consider extracting comboToString (and related helpers) to a shared utility file.

comboToString, getModifiersFromCombo, normalizeKeyCombo, and the modifier/alias constants are pure utility logic with no component dependencies. Exporting them from DiscourseNodeMenu.tsx forces KeyboardShortcutInput.tsx (a settings component) to import from a menu component, creating an awkward cross-concern coupling.

Moving these to e.g. ~/utils/keyComboUtils.ts would improve module boundaries and make it easier to reuse across additional settings components.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/roam/src/components/DiscourseNodeMenu.tsx` around lines 410 - 415,
Extract the pure utility logic from DiscourseNodeMenu.tsx into a shared module
(e.g., create ~/utils/keyComboUtils.ts): move comboToString,
getModifiersFromCombo, normalizeKeyCombo and the modifier/alias constants into
that file and export them, then update DiscourseNodeMenu.tsx and
KeyboardShortcutInput.tsx to import these helpers from the new utils module;
ensure exports keep the same names/signatures so no other call sites need
changes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/roam/src/components/DiscourseNodeMenu.tsx`:
- Around line 460-464: The onClick clear handler uses an empty string "" for
extensionAPI.settings.set and setPersonalSetting but elsewhere
(KeyboardShortcutInput.handleClear) clears with the structured object {
modifiers: 0, key: "" }, causing validation and type inconsistencies; update the
onClick handler to persist the same structured object ({ modifiers: 0, key: ""
}) to extensionAPI.settings.set and setPersonalSetting and keep setComboKey({
modifiers: 0, key: "" }) so the clear behavior is consistent with
KeyboardShortcutInput.handleClear.

In `@apps/roam/src/components/DiscourseNodeSearchMenu.tsx`:
- Around line 631-632: The code writes the trigger to two different keys
(extensionAPI.settings.set("node-search-trigger", ...) and
setPersonalSetting(["Node search menu trigger"], ...)) while reads only use
getSetting("node-search-trigger", "@"), risking divergence; fix by consolidating
to a single canonical store: remove the redundant setPersonalSetting call and
persist only via extensionAPI.settings.set("node-search-trigger", trigger), or
if you must keep both, update read logic (where
getSetting("node-search-trigger", "@") is used) to also read and merge the
personal setting with a clear precedence. Ensure references to
setPersonalSetting and extensionAPI.settings.set are consistent and update any
docs/tests accordingly.

---

Nitpick comments:
In `@apps/roam/src/components/DiscourseNodeMenu.tsx`:
- Around line 410-415: Extract the pure utility logic from DiscourseNodeMenu.tsx
into a shared module (e.g., create ~/utils/keyComboUtils.ts): move
comboToString, getModifiersFromCombo, normalizeKeyCombo and the modifier/alias
constants into that file and export them, then update DiscourseNodeMenu.tsx and
KeyboardShortcutInput.tsx to import these helpers from the new utils module;
ensure exports keep the same names/signatures so no other call sites need
changes.

In `@apps/roam/src/components/settings/utils/zodSchema.ts`:
- Around line 233-242: The "Personal node menu trigger" schema currently uses
z.union([z.object(...), z.literal("")]) which produces an inconsistent output
type vs the "Discourse tool shortcut" object; change the schema so the parsed
output is always the object shape (matching "Discourse tool shortcut") by
handling the empty-string legacy case with a transform or preprocess: accept the
union but .transform() (or z.preprocess) to map "" to { modifiers: 0, key: "" }
so downstream consumers of "Personal node menu trigger" can safely access
.modifiers and .key without type-narrowing; alternatively make both schemas
identical objects if no legacy string values must be supported, and add a short
comment explaining the chosen approach.

[mdroidian]

Let's go!

One devin comment worth looking into. Can defer, not blocking.