ENG-1473: Port non-boolean personal setting consumers

[sid597]

Summary by CodeRabbit

• Refactor
  • Restructured the settings management system to use a unified personal settings approach. This affects how keyboard triggers, filters, shortcuts, and query page preferences are saved and loaded, improving consistency and reliability of your configuration across sessions.

[linear]

ENG-1473 Port non-boolean personal setting consumers

ENG-1454 Base `getter()` task

[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]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• ENG-1468: Port node and relations tree based consumers #822
• ENG-1467: Port global setting consumer reads (→ getGlobalSetting) #821
• ENG-1473: Port non-boolean personal setting consumers #818 👈 (View in Graphite)
• ENG-1456: Migrate personal boolean flag consumers (getSetting → getPersonalSetting) #814
• ENG-1472: Refactor BlockPropSettingPanels to add accessor-backed default reads (with initialValue fallback) #813
• ENG-1455: Dual-read from old-system settings and from blockprops #812
• ENG-1465: Add "Use new settings store" feature flag  #811
• ENG-1217: Port section component in left sidebar #804
• ENG-1328: Port small blueprint based misc settings into props based #805
• eng-1438-keyboard-quick-shortcut-keystriggers

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

[sid597]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request migrates personal user settings retrieval from using extensionAPI.settings and getSetting to a new getPersonalSetting accessor pattern. The changes refactor how node menu triggers, search triggers, keyboard shortcuts, and query pages are initialized and persisted across multiple components and utility functions.

Changes

Cohort / File(s)	Summary
Node Menu Triggers apps/roam/src/components/DiscourseNodeMenu.tsx, apps/roam/src/components/DiscourseNodeSearchMenu.tsx	Replaced extensionAPI.settings and getSetting with getPersonalSetting for retrieving user-defined node menu and search menu trigger keybindings.
Settings UI Components apps/roam/src/components/settings/DefaultFilters.tsx, apps/roam/src/components/settings/KeyboardShortcutInput.tsx, apps/roam/src/components/settings/QueryPagesPanel.tsx	Migrated to getPersonalSetting for state initialization; added new getQueryPages() function in QueryPagesPanel that reads from personal settings and returns string[].
Settings Panel Configuration apps/roam/src/components/settings/QuerySettings.tsx	Removed initialValue props from numeric and multi-text panels, allowing components to initialize independently from personal settings.
Settings Accessor Utilities apps/roam/src/components/settings/utils/accessors.ts	Refactored getLegacyPersonalSetting to assign path lookup result to a variable before querying the map, with no behavioral changes.
Initialization and Utility Functions apps/roam/src/utils/initializeObserversAndListeners.ts, apps/roam/src/utils/setQueryPages.ts	Updated personal trigger and query page retrieval to use getPersonalSetting; adjusted logic to handle undefined settings gracefully.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• ENG-1190 Replace existing roam-js components with blockProp based ones #689: Introduces the personal settings accessor infrastructure (getPersonalSetting/setPersonalSetting) and blockProp-based setting wrappers that this PR depends on and consumes across multiple components.
• [ENG-299] Trigger setting for DG summoning menu #162: Modifies node-search trigger handling in DiscourseNodeSearchMenu with a configurable trigger UI; this PR updates the same component to use the new personal settings accessor pattern.
• Move personal settings to DG:settings dialog #86: Refactors personal node menu trigger handling in DiscourseNodeMenu; this PR updates the same component to retrieve triggers via getPersonalSetting instead of direct API access.

🚥 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 accurately identifies the main objective of the PR: porting non-boolean personal setting consumers to use the new getPersonalSetting accessor.
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: 1

♻️ Duplicate comments (6)

apps/roam/src/utils/setQueryPages.ts (1)

8-9: Duplicate — getPersonalSetting can throw before the fallback applies.

See the main comment in apps/roam/src/utils/initializeObserversAndListeners.ts (Line 265-271, Line 333-334, Line 388-389).

🤖 Prompt for AI Agents

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

In `@apps/roam/src/utils/setQueryPages.ts` around lines 8 - 9, getPersonalSetting
can throw before the fallback is applied, so replace the direct call to
getPersonalSetting in setQueryPages.ts with a safe call that catches exceptions:
wrap the getPersonalSetting(["Query", "Query pages"]) invocation in a try/catch,
assign queryPages from the try block and on error set queryPageArray = []; keep
the existing Array.isArray check (use queryPageArray = Array.isArray(queryPages)
? queryPages : []) but ensure queryPages is initialized to a safe default when
an exception occurs; reference getPersonalSetting, queryPages, and
queryPageArray when making the change.

apps/roam/src/components/settings/QueryPagesPanel.tsx (1)

10-15: Duplicate — getPersonalSetting can throw before the fallback applies.

See the main comment in apps/roam/src/utils/initializeObserversAndListeners.ts (Line 265-271, Line 333-334, Line 388-389).

🤖 Prompt for AI Agents

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

In `@apps/roam/src/components/settings/QueryPagesPanel.tsx` around lines 10 - 15,
getQueryPages calls getPersonalSetting directly which can throw before any
fallback is applied; wrap the getPersonalSetting(["Query","Query pages"])
invocation inside a try/catch (or use a safe getter helper) and return a
sensible default (e.g., []) on error, so getQueryPages always returns a string[]
without letting exceptions propagate; update the getQueryPages function to catch
errors from getPersonalSetting and return the fallback array while preserving
the existing signature.

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

427-433: Duplicate — getPersonalSetting can throw before the fallback applies.

See the main comment in apps/roam/src/utils/initializeObserversAndListeners.ts (Line 265-271, Line 333-334, Line 388-389).

🤖 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 427 - 433, The
call to getPersonalSetting inside the useState initializer for comboKey can
throw before the fallback object is applied; wrap the getPersonalSetting call in
a safe guard so the initializer for comboKey always returns a valid IKeyCombo
even if getPersonalSetting throws—e.g., in the DiscourseNodeMenu component
change the state initializer for comboKey (useState<IKeyCombo>(...)) to call
getPersonalSetting inside a try/catch (or use a safe helper) and return the
fallback { modifiers: 0, key: "" } on any error, and keep using setComboKey
elsewhere as before.

apps/roam/src/components/settings/KeyboardShortcutInput.tsx (1)

38-41: Duplicate — getPersonalSetting can throw before the fallback applies.

See the main comment in apps/roam/src/utils/initializeObserversAndListeners.ts (Line 265-271, Line 333-334, Line 388-389).

🤖 Prompt for AI Agents

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

In `@apps/roam/src/components/settings/KeyboardShortcutInput.tsx` around lines 38
- 41, The lazy initializer for useState in KeyboardShortcutInput uses
getPersonalSetting which can throw before the object fallback is applied; update
the initializer for comboKey (setComboKey) to wrap the
getPersonalSetting([blockPropKey]) call in a try/catch and return the fallback
IKeyCombo when an exception occurs (optionally log the error), ensuring
getPersonalSetting errors do not prevent the default { modifiers: 0, ... } from
being used.

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

715-717: Duplicate — getPersonalSetting can throw before the fallback applies.

See the main comment in apps/roam/src/utils/initializeObserversAndListeners.ts (Line 265-271, Line 333-334, Line 388-389).

🤖 Prompt for AI Agents

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

In `@apps/roam/src/components/DiscourseNodeSearchMenu.tsx` around lines 715 - 717,
The initialization of nodeSearchTrigger uses getPersonalSetting(...) directly
inside useState which can throw before the fallback "@" is applied; update the
DiscourseNodeSearchMenu initialization to call getPersonalSetting inside a
try/catch (or a safe wrapper) and return the fallback on error so
nodeSearchTrigger and setNodeSearchTrigger are seeded safely; specifically
protect the call to getPersonalSetting in the useState initializer (referencing
nodeSearchTrigger, setNodeSearchTrigger, and getPersonalSetting) and ensure any
thrown error results in the fallback "@" being used.

apps/roam/src/components/settings/DefaultFilters.tsx (1)

112-115: Duplicate — getPersonalSetting can throw before the fallback applies.

See the main comment in apps/roam/src/utils/initializeObserversAndListeners.ts (Line 265-271, Line 333-334, Line 388-389).

🤖 Prompt for AI Agents

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

In `@apps/roam/src/components/settings/DefaultFilters.tsx` around lines 112 - 115,
The call to getPersonalSetting(...) in DefaultFilters.tsx can throw before the
fallback is applied, so wrap the call in a try/catch and fall back to an empty
Record<string, StoredFilters> when an exception occurs; specifically, replace
the inline cast of getPersonalSetting in the initialization with a safe
retrieval (try { const v = getPersonalSetting<Record<string,
StoredFilters>>([...]); use v } catch { use {} }) so getPersonalSetting, the
returned StoredFilters record, and the fallback are handled deterministically.

🧹 Nitpick comments (1)

apps/roam/src/components/settings/QueryPagesPanel.tsx (1)

24-27: Add an explicit return type to persistQueryPages.

This keeps TypeScript intent clear and aligns with the repo’s TS guidelines.

♻️ Suggested change

-  const persistQueryPages = (newTexts: string[]) => {
+  const persistQueryPages = (newTexts: string[]): void => {
     setPersonalSetting(["Query", "Query pages"], newTexts);
     void extensionAPI.settings.set("query-pages", newTexts);
   };

As per coding guidelines, use explicit return types for functions in TypeScript.

🤖 Prompt for AI Agents

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

In `@apps/roam/src/components/settings/QueryPagesPanel.tsx` around lines 24 - 27,
The function persistQueryPages lacks an explicit return type; update its
signature to include an explicit TypeScript return type (e.g., change to
persistQueryPages = (newTexts: string[]): void => { ... }) so the function's
intent is clear and consistent with repo TS guidelines; make this change where
persistQueryPages is defined and ensure the compiler still accepts the calls to
setPersonalSetting and extensionAPI.settings.set inside the function.

🤖 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/utils/initializeObserversAndListeners.ts`:
- Around line 265-271: getPersonalSetting can throw when a path is absent,
preventing the fallback logic from running; wrap calls to getPersonalSetting
(e.g., the one assigned to personalTriggerCombo used by personalTrigger and
getModifiersFromCombo) in a safe accessor or try/catch that returns undefined on
error so the existing ?? / !== false fallbacks work. Update the
personalTriggerCombo assignment to use a helper like safeGetPersonalSetting(...)
or a try { return getPersonalSetting(...) } catch { return undefined }, and
apply the same pattern to the other new getPersonalSetting call sites in this
file (the additional personal setting lookups introduced in this PR) so
observer/listener initialization cannot be aborted by a missing key.

---

Duplicate comments:
In `@apps/roam/src/components/DiscourseNodeMenu.tsx`:
- Around line 427-433: The call to getPersonalSetting inside the useState
initializer for comboKey can throw before the fallback object is applied; wrap
the getPersonalSetting call in a safe guard so the initializer for comboKey
always returns a valid IKeyCombo even if getPersonalSetting throws—e.g., in the
DiscourseNodeMenu component change the state initializer for comboKey
(useState<IKeyCombo>(...)) to call getPersonalSetting inside a try/catch (or use
a safe helper) and return the fallback { modifiers: 0, key: "" } on any error,
and keep using setComboKey elsewhere as before.

In `@apps/roam/src/components/DiscourseNodeSearchMenu.tsx`:
- Around line 715-717: The initialization of nodeSearchTrigger uses
getPersonalSetting(...) directly inside useState which can throw before the
fallback "@" is applied; update the DiscourseNodeSearchMenu initialization to
call getPersonalSetting inside a try/catch (or a safe wrapper) and return the
fallback on error so nodeSearchTrigger and setNodeSearchTrigger are seeded
safely; specifically protect the call to getPersonalSetting in the useState
initializer (referencing nodeSearchTrigger, setNodeSearchTrigger, and
getPersonalSetting) and ensure any thrown error results in the fallback "@"
being used.

In `@apps/roam/src/components/settings/DefaultFilters.tsx`:
- Around line 112-115: The call to getPersonalSetting(...) in DefaultFilters.tsx
can throw before the fallback is applied, so wrap the call in a try/catch and
fall back to an empty Record<string, StoredFilters> when an exception occurs;
specifically, replace the inline cast of getPersonalSetting in the
initialization with a safe retrieval (try { const v =
getPersonalSetting<Record<string, StoredFilters>>([...]); use v } catch { use {}
}) so getPersonalSetting, the returned StoredFilters record, and the fallback
are handled deterministically.

In `@apps/roam/src/components/settings/KeyboardShortcutInput.tsx`:
- Around line 38-41: The lazy initializer for useState in KeyboardShortcutInput
uses getPersonalSetting which can throw before the object fallback is applied;
update the initializer for comboKey (setComboKey) to wrap the
getPersonalSetting([blockPropKey]) call in a try/catch and return the fallback
IKeyCombo when an exception occurs (optionally log the error), ensuring
getPersonalSetting errors do not prevent the default { modifiers: 0, ... } from
being used.

In `@apps/roam/src/components/settings/QueryPagesPanel.tsx`:
- Around line 10-15: getQueryPages calls getPersonalSetting directly which can
throw before any fallback is applied; wrap the
getPersonalSetting(["Query","Query pages"]) invocation inside a try/catch (or
use a safe getter helper) and return a sensible default (e.g., []) on error, so
getQueryPages always returns a string[] without letting exceptions propagate;
update the getQueryPages function to catch errors from getPersonalSetting and
return the fallback array while preserving the existing signature.

In `@apps/roam/src/utils/setQueryPages.ts`:
- Around line 8-9: getPersonalSetting can throw before the fallback is applied,
so replace the direct call to getPersonalSetting in setQueryPages.ts with a safe
call that catches exceptions: wrap the getPersonalSetting(["Query", "Query
pages"]) invocation in a try/catch, assign queryPages from the try block and on
error set queryPageArray = []; keep the existing Array.isArray check (use
queryPageArray = Array.isArray(queryPages) ? queryPages : []) but ensure
queryPages is initialized to a safe default when an exception occurs; reference
getPersonalSetting, queryPages, and queryPageArray when making the change.

---

Nitpick comments:
In `@apps/roam/src/components/settings/QueryPagesPanel.tsx`:
- Around line 24-27: The function persistQueryPages lacks an explicit return
type; update its signature to include an explicit TypeScript return type (e.g.,
change to persistQueryPages = (newTexts: string[]): void => { ... }) so the
function's intent is clear and consistent with repo TS guidelines; make this
change where persistQueryPages is defined and ensure the compiler still accepts
the calls to setPersonalSetting and extensionAPI.settings.set inside the
function.

---

ℹ️ Review info

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3e2cebd and 0b44d51.

📒 Files selected for processing (9)

• apps/roam/src/components/DiscourseNodeMenu.tsx
• apps/roam/src/components/DiscourseNodeSearchMenu.tsx
• apps/roam/src/components/settings/DefaultFilters.tsx
• apps/roam/src/components/settings/KeyboardShortcutInput.tsx
• apps/roam/src/components/settings/QueryPagesPanel.tsx
• apps/roam/src/components/settings/QuerySettings.tsx
• apps/roam/src/components/settings/utils/accessors.ts
• apps/roam/src/utils/initializeObserversAndListeners.ts
• apps/roam/src/utils/setQueryPages.ts

💤 Files with no reviewable changes (1)

• apps/roam/src/components/settings/QuerySettings.tsx