ENG-1456: Migrate personal boolean flag consumers (getSetting → getPersonalSetting)

[sid597]

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

---

Summary by CodeRabbit

• Refactor
  • Migrated all user-configurable settings from global application storage to per-user personal settings. Individual preferences for feedback visibility, canvas overlays, keyboard shortcuts, diagnostic tracking, sidebar behavior, query metadata display, and styling options are now preserved uniquely for each user rather than being shared globally.

[linear]

ENG-1456 Migrate personal boolean flag consumers (getSetting → getPersonalSetting)

[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
• ENG-1456: Migrate personal boolean flag consumers (getSetting → getPersonalSetting) #814 👈 (View in Graphite)
• ENG-1472: Refactor BlockPropSettingPanels to add accessor-backed default reads #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.

[devin-ai-integration]

Devin Review found 1 potential issue.

View 6 additional findings in Devin Review.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR migrates feature flag and preference retrievals from global extension settings (via getSetting) to personal per-user settings (via getPersonalSetting). The changes affect 12 files across components, utilities, and settings UI, establishing personal settings as the single source of truth for user preferences.

Changes

Cohort / File(s)	Summary
Canvas & UI Components apps/roam/src/components/DiscourseFloatingMenu.tsx, apps/roam/src/components/canvas/DiscourseNodeUtil.tsx, apps/roam/src/components/canvas/Tldraw.tsx, apps/roam/src/components/canvas/uiOverrides.tsx	Replaced runtime extension API setting checks with getPersonalSetting calls for feature toggles: feedback button visibility, overlay/canvas relations preferences, and discourse tool keyboard shortcut.
Settings UI Configuration apps/roam/src/components/settings/HomePersonalSettings.tsx, apps/roam/src/components/settings/QuerySettings.tsx	Removed getSetting usage and initialValue props from PersonalFlagPanel components; initialization now relies on component defaults rather than explicit per-item defaults read from extension settings.
Settings Infrastructure apps/roam/src/components/settings/utils/accessors.ts	Removed legacy default overrides map (PERSONAL_OLD_DEFAULT_OVERRIDES) and simplified getLegacyPersonalSetting to always read defaults from DEFAULT_PERSONAL_SETTINGS.
Core Utilities & Initialization apps/roam/src/index.ts, apps/roam/src/utils/initializeObserversAndListeners.ts, apps/roam/src/utils/createDiscourseNode.ts	Replaced getSetting calls with getPersonalSetting for diagnostics, styling, overlay, and sidebar preferences; one location adds return statement to propagate creation promise in page action listener.
Error Handling & Analytics apps/roam/src/utils/internalError.ts, apps/roam/src/utils/posthog.ts	Replaced getSetting with getPersonalSetting for "Disable product diagnostics" flag in error reporting and PostHog initialization flows.
Command Palette apps/roam/src/utils/registerCommandPaletteCommands.ts	Updated toggle commands to read settings via getPersonalSetting, sync both extension and personal settings on update, and include error messages in toast notifications.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• ENG-1273: Migrate small personal settings #691: Directly related—makes identical code-level changes (replacing getSetting/extension API calls with getPersonalSetting) across the same files (DiscourseFloatingMenu, createDiscourseNode, initializeObserversAndListeners, HomePersonalSettings).
• ENG-1190 Replace existing roam-js components with blockProp based ones #689: Related—both modify the settings UI layer and PersonalFlagPanel component signatures for personal/global setting panels.
• ENG-636 Add auto canvas relations setting to user preferences #363: Related—both modify the AUTO_CANVAS_RELATIONS gating logic in DiscourseNodeUtil and Tldraw to conditionally create canvas relations based on user preference.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately summarizes the main change: migrating personal boolean flag consumers from getSetting to getPersonalSetting across multiple files.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ 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: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

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

146-168: ⚠️ Potential issue | 🟠 Major

getPersonalSetting is called outside the try/catch — a throw causes a silent unhandled rejection.

In both toggleDiscourseContextOverlay (lines 147–149) and toggleQueryMetadata (lines 171–174), getPersonalSetting is called before the try block. The callers invoke these via () => void fn(), so any rejected promise is swallowed silently — users get no feedback and the toggle does nothing.

Move the getPersonalSetting calls inside the try/catch (or add a guard before it) so failures surface as toast messages consistent with the rest of the error-handling here.

🛡️ Proposed fix for `toggleDiscourseContextOverlay`

  const toggleDiscourseContextOverlay = async () => {
-   const currentValue = getPersonalSetting<boolean>([
-     "Discourse context overlay",
-   ]);
-   const newValue = !currentValue;
    try {
+     const currentValue = getPersonalSetting<boolean>([
+       "Discourse context overlay",
+     ]);
+     const newValue = !currentValue;
      await extensionAPI.settings.set("discourse-context-overlay", newValue);
      setPersonalSetting(["Discourse context overlay"], newValue);
+     const overlayHandler = getOverlayHandler(onloadArgs);
+     onPageRefObserverChange(overlayHandler)(newValue);
+     renderToast({
+       id: "discourse-context-overlay-toggle",
+       content: `Discourse context overlay ${newValue ? "enabled" : "disabled"}`,
+     });
    } catch (error) {
      const e = error as Error;
      renderToast({
        id: "discourse-context-overlay-toggle-error",
        content: `Failed to toggle discourse context overlay: ${e.message}`,
      });
      return;
    }
-   const overlayHandler = getOverlayHandler(onloadArgs);
-   onPageRefObserverChange(overlayHandler)(newValue);
-   renderToast({
-     id: "discourse-context-overlay-toggle",
-     content: `Discourse context overlay ${newValue ? "enabled" : "disabled"}`,
-   });
  };

Apply the same pattern to toggleQueryMetadata.

🤖 Prompt for AI Agents

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

In `@apps/roam/src/utils/registerCommandPaletteCommands.ts` around lines 146 -
168, Both toggleDiscourseContextOverlay and toggleQueryMetadata call
getPersonalSetting outside their try/catch, which can throw and produce an
unhandled rejection; move the getPersonalSetting calls into the existing try
block (or add an explicit guard that catches errors) inside
toggleDiscourseContextOverlay and toggleQueryMetadata so any thrown error is
caught and the renderToast error path runs; update references to
currentValue/newValue accordingly inside the try so the subsequent settings.set,
setPersonalSetting, onPageRefObserverChange(onloadArgs) calls and
success/failure toasts behave consistently.

apps/roam/src/components/canvas/Tldraw.tsx (1)

1089-1101: ⚠️ Potential issue | 🟠 Major

getPersonalSetting inside the per-shape afterCreate side-effect can throw on every canvas interaction.

This handler fires for every shape creation. If getPersonalSetting throws (dual-read enabled, setting not yet available), tldraw's side effect system receives an unhandled exception on each new shape, potentially breaking canvas interactions repeatedly.

🛡️ Proposed fix

       editor.sideEffects.registerAfterCreateHandler("shape", (shape) => {
         const util = editor.getShapeUtil(shape);
         if (util instanceof BaseDiscourseNodeUtil) {
-          const autoCanvasRelations = getPersonalSetting<boolean>([
-            "Auto canvas relations",
-          ]);
+          let autoCanvasRelations = false;
+          try {
+            autoCanvasRelations = getPersonalSetting<boolean>(["Auto canvas relations"]) ?? false;
+          } catch {
+            // setting unavailable; skip auto-relations
+          }
           if (autoCanvasRelations) {

🤖 Prompt for AI Agents

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

In `@apps/roam/src/components/canvas/Tldraw.tsx` around lines 1089 - 1101, The
per-shape afterCreate handler registered via
editor.sideEffects.registerAfterCreateHandler ("shape") calls getPersonalSetting
synchronously and can throw, causing unhandled exceptions on every shape
creation; fix it by making the handler defensive: wrap the getPersonalSetting
call in a try/catch (or check for availability) and bail silently on error, and
only call util.createExistingRelations when the setting is safely read and true;
reference the removeAfterCreateHandler registration, the BaseDiscourseNodeUtil
check, getPersonalSetting call, and util.createExistingRelations to locate and
update the code.

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

207-235: ⚠️ Potential issue | 🟠 Major

getPersonalSetting calls in initObservers run before initSchema() — same ordering risk as index.ts

initObservers is invoked at index.ts line 101, before initSchema() at line 158. All three getPersonalSetting calls here (lines 207, 230, 232) will throw for any unset setting when isDualReadEnabled() is true, causing initObservers to reject and the extension to fail to load.

🐛 Add `?? false` fallbacks

-  if (getPersonalSetting<boolean>(["Suggestive mode overlay"])) {
+  if (getPersonalSetting<boolean>(["Suggestive mode overlay"]) ?? false) {
     addPageRefObserver(getSuggestiveOverlayHandler(onloadArgs));
   }

-  if (getPersonalSetting<boolean>(["Page preview"]))
+  if (getPersonalSetting<boolean>(["Page preview"]) ?? false)
     addPageRefObserver(previewPageRefHandler);
-  if (getPersonalSetting<boolean>(["Discourse context overlay"])) {
+  if (getPersonalSetting<boolean>(["Discourse context overlay"]) ?? false) {
     const overlayHandler = getOverlayHandler(onloadArgs);
     onPageRefObserverChange(overlayHandler)(true);
   }

🤖 Prompt for AI Agents

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

In `@apps/roam/src/utils/initializeObserversAndListeners.ts` around lines 207 -
235, The three calls to getPersonalSetting inside initObservers (the ones gating
addPageRefObserver(getSuggestiveOverlayHandler(onloadArgs)), the Page preview
branch that uses previewPageRefHandler, and the Discourse context overlay branch
that calls getOverlayHandler/onPageRefObserverChange) can throw when schema
isn't initialized; update each getPersonalSetting<boolean>(...) invocation to
safely coalesce to false (e.g., getPersonalSetting<boolean>([...]) ?? false) so
they never throw when isDualReadEnabled() or initSchema() haven't run yet.

🤖 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/canvas/DiscourseNodeUtil.tsx`:
- Around line 515-519: The call to getPersonalSetting that assigns
autoCanvasRelations is currently outside the try/catch so its exceptions can
bypass the renderToast error handler; move the
getPersonalSetting<boolean>(["Auto canvas relations"]) call inside the try block
that follows (the same guarded section that uses renderToast and the onSuccess
callback), so any thrown error is caught and handled by the existing catch which
calls renderToast; keep the same variable name autoCanvasRelations and ensure
the rest of the code in that try block uses the moved variable.
- Around line 433-436: The call to getPersonalSetting(["Overlay in canvas"])
inside the useMemo that defines isOverlayEnabled can throw during render
(especially when isDualReadEnabled() is true and the setting is unset) and the
empty dependency array prevents reacting to setting changes; change this by
moving the getPersonalSetting call out of a render-time exception-prone context
or wrap it in a safe try/catch that returns a sensible default (e.g., false) and
log/handle the error, and also replace the empty deps with a settings-dependent
value or subscribe to the settings/context so isOverlayEnabled updates when the
personal setting changes; look for the isOverlayEnabled constant, the useMemo
call, getPersonalSetting, and isDualReadEnabled to implement the defensive
fallback and dependency fix.

In `@apps/roam/src/components/canvas/uiOverrides.tsx`:
- Around line 273-281: Wrap the getPersonalSetting call in a try/catch and
validate its shape before accessing .key: catch any exceptions from
getPersonalSetting("Discourse tool shortcut") and fall back to a safe default
object (e.g., { key: "", modifiers: 0 }), then ensure the returned value
actually matches the expected shape (IKeyCombo-like) before using .key (e.g.,
check typeof combo === "object" && typeof combo.key === "string"); finally
compute discourseToolShortcut from that validated value so rendering won't throw
in the tools override (refer to discourseToolCombo, getPersonalSetting,
IKeyCombo, and discourseToolShortcut).

In `@apps/roam/src/components/DiscourseFloatingMenu.tsx`:
- Around line 122-124: installDiscourseFloatingMenu currently calls
getPersonalSetting([... "Hide feedback button"]) during extension install which
can throw (when dual-read is enabled and the setting path is undefined), causing
install to abort before ReactDOM.render; wrap the getPersonalSetting call inside
a try/catch (or otherwise guard it) in
installDiscourseFloatingMenu/DiscourseFloatingMenu initialization, treat
failures as the safe default (e.g., false), and only call
floatingMenuAnchor.classList.add("hidden") when the guarded result is true so
the floating menu still renders even if the accessor throws.

In `@apps/roam/src/index.ts`:
- Around line 47-49: The calls to getPersonalSetting (e.g., for
disallowDiagnostics) occur inside runExtension before initSchema(), so when
isDualReadEnabled() is true and the setting is undefined it can throw and abort
startup; move these getPersonalSetting calls (and any observer initialization
that reads settings such as initObservers/initializeObserversAndListeners) to
after initSchema() is awaited, or alternatively add a safe fallback at each call
site by checking isDualReadEnabled() and treating undefined as the desired
default (e.g., false) before using the value; update usages including the
disallowDiagnostics variable and any other pre-schema reads to either be
relocated after initSchema() or wrapped with the fallback check so the extension
won't throw on first load.

In `@apps/roam/src/utils/createDiscourseNode.ts`:
- Around line 35-37: handleOpenInSidebar currently calls getPersonalSetting
synchronously which, if it throws, can cause createDiscourseNode to reject after
the Roam node was already created; wrap the call to getPersonalSetting (and the
subsequent openBlockInSidebar invocation) in a try/catch inside
handleOpenInSidebar, default to treating the setting as "allow" when an error
occurs (i.e., proceed without opening the sidebar) or simply swallow the error
so node creation doesn't reject, and keep the existing void
openBlockInSidebar(uid) invocation in the try block to preserve fire-and-forget
behavior.

In `@apps/roam/src/utils/initializeObserversAndListeners.ts`:
- Around line 385-388: The current check in nodeCreationPopoverListener uses
getPersonalSetting(...) !== false which fails when getPersonalSetting throws in
the dual-read path; instead wrap the getPersonalSetting call in a try/catch to
treat thrown errors as unset, then apply nullish-coalescing to preserve opt-out
semantics: let val; try { val = getPersonalSetting<boolean>(["Text selection
popup"]); } catch { val = undefined; } const isTextSelectionPopupEnabled = val
?? true; update the logic in the nodeCreationPopoverListener closure accordingly
(referencing nodeCreationPopoverListener and getPersonalSetting).

In `@apps/roam/src/utils/internalError.ts`:
- Around line 24-28: The call to getPersonalSetting inside internalError can
throw (see isDualReadEnabled behavior) which can make internalError propagate;
wrap the getPersonalSetting<boolean>(["Disable product diagnostics"]) call in a
try/catch (or extract it above and catch errors) so that any thrown error is
swallowed/handled and treated as a safe default (e.g., false/undefined) before
proceeding with the existing conditional; modify internalError to use that safe
result so internalError itself cannot throw when invoked from
validateSettingValue, setPersonalSetting, or setGlobalSetting.

In `@apps/roam/src/utils/posthog.ts`:
- Around line 71-75: The call to getPersonalSetting inside initPostHog can throw
during early startup and currently prevents doInitPostHog from running; wrap the
getPersonalSetting<boolean>(["Disable product diagnostics"]) call in a
try/catch, log the caught error (using the module's logger or console.error),
treat the setting as not disabled on error (i.e., default to false) and ensure
doInitPostHog() is called in that fallback path so PostHog is initialized even
if settings are temporarily unavailable.

---

Outside diff comments:
In `@apps/roam/src/components/canvas/Tldraw.tsx`:
- Around line 1089-1101: The per-shape afterCreate handler registered via
editor.sideEffects.registerAfterCreateHandler ("shape") calls getPersonalSetting
synchronously and can throw, causing unhandled exceptions on every shape
creation; fix it by making the handler defensive: wrap the getPersonalSetting
call in a try/catch (or check for availability) and bail silently on error, and
only call util.createExistingRelations when the setting is safely read and true;
reference the removeAfterCreateHandler registration, the BaseDiscourseNodeUtil
check, getPersonalSetting call, and util.createExistingRelations to locate and
update the code.

In `@apps/roam/src/utils/initializeObserversAndListeners.ts`:
- Around line 207-235: The three calls to getPersonalSetting inside
initObservers (the ones gating
addPageRefObserver(getSuggestiveOverlayHandler(onloadArgs)), the Page preview
branch that uses previewPageRefHandler, and the Discourse context overlay branch
that calls getOverlayHandler/onPageRefObserverChange) can throw when schema
isn't initialized; update each getPersonalSetting<boolean>(...) invocation to
safely coalesce to false (e.g., getPersonalSetting<boolean>([...]) ?? false) so
they never throw when isDualReadEnabled() or initSchema() haven't run yet.

In `@apps/roam/src/utils/registerCommandPaletteCommands.ts`:
- Around line 146-168: Both toggleDiscourseContextOverlay and
toggleQueryMetadata call getPersonalSetting outside their try/catch, which can
throw and produce an unhandled rejection; move the getPersonalSetting calls into
the existing try block (or add an explicit guard that catches errors) inside
toggleDiscourseContextOverlay and toggleQueryMetadata so any thrown error is
caught and the renderToast error path runs; update references to
currentValue/newValue accordingly inside the try so the subsequent settings.set,
setPersonalSetting, onPageRefObserverChange(onloadArgs) calls and
success/failure toasts behave consistently.