ENG-1456: Migrate personal boolean flag consumers (getSetting → getPersonalSetting)#814
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| import sendErrorEmail from "~/utils/sendErrorEmail"; | ||
| import { getSetting } from "~/utils/extensionSettings"; | ||
| import { DISALLOW_DIAGNOSTICS } from "~/data/userSettings"; | ||
| import { getPersonalSetting } from "~/components/settings/utils/accessors"; |
There was a problem hiding this comment.
🔴 Circular dependency between internalError and getPersonalSetting degrades error-handler robustness
internalError is the application's error handler of last resort — it must never throw. This PR replaces the simple getSetting(DISALLOW_DIAGNOSTICS, false) call with getPersonalSetting<boolean>(["Disable product diagnostics"]), which introduces a circular module dependency and a significantly more complex runtime path.
Root cause and impact
Circular dependency: internalError.ts now imports getPersonalSetting from accessors.ts, while accessors.ts imports internalError from internalError.ts. The old code imported getSetting from extensionSettings.ts which did NOT import internalError, so no cycle existed.
More complex call chain: The new getPersonalSetting call goes through isDualReadEnabled() → getFeatureFlag() → getFeatureFlags() → getBlockPropBasedSettings() → getBlockUidByTextOnPage() + getBlockProps() + FeatureFlagsSchema.parse() — all BEFORE reaching the legacy getSetting fallback. Each additional step is a potential throw site.
If any of these calls fail (e.g., Roam data queries are in a bad state — which is more likely when errors are already occurring), internalError itself throws, meaning:
- The original error being reported is silently lost
- Error telemetry (PostHog + email) never fires
- The caller crashes with an unrelated settings-read error
The old getSetting was a single extensionAPI.settings.get() call with a hardcoded false default — far fewer points of failure.
Prompt for agents
In apps/roam/src/utils/internalError.ts, the call to getPersonalSetting on line 25 should be wrapped in a try-catch to ensure internalError never throws due to a settings-read failure. Replace the getPersonalSetting call at line 25 with a safe wrapper, for example:
let diagnosticsDisabled = false;
try {
diagnosticsDisabled = getPersonalSetting<boolean>(["Disable product diagnostics"]) ?? false;
} catch {
// If we can't read settings, default to allowing diagnostics so errors get reported
diagnosticsDisabled = false;
}
Alternatively, consider keeping the simpler getSetting import from extensionSettings.ts for internalError specifically, to avoid the circular module dependency between internalError.ts and accessors.ts (since accessors.ts already imports internalError).
Was this helpful? React with 👍 or 👎 to provide feedback.
📝 WalkthroughWalkthroughThis PR migrates feature flag and preference retrievals from global extension settings (via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
getPersonalSettingis called outside thetry/catch— a throw causes a silent unhandled rejection.In both
toggleDiscourseContextOverlay(lines 147–149) andtoggleQueryMetadata(lines 171–174),getPersonalSettingis called before thetryblock. 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
getPersonalSettingcalls inside thetry/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
getPersonalSettinginside the per-shapeafterCreateside-effect can throw on every canvas interaction.This handler fires for every shape creation. If
getPersonalSettingthrows (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
getPersonalSettingcalls ininitObserversrun beforeinitSchema()— same ordering risk asindex.ts
initObserversis invoked atindex.tsline 101, beforeinitSchema()at line 158. All threegetPersonalSettingcalls here (lines 207, 230, 232) will throw for any unset setting whenisDualReadEnabled()is true, causinginitObserversto 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.
| const isOverlayEnabled = useMemo( | ||
| () => getSetting(DISCOURSE_CONTEXT_OVERLAY_IN_CANVAS_KEY, false), | ||
| () => getPersonalSetting<boolean>(["Overlay in canvas"]), | ||
| [], | ||
| ); |
There was a problem hiding this comment.
getPersonalSetting inside useMemo can throw during render
When isDualReadEnabled() is true and the "Overlay in canvas" setting is unset, getPersonalSetting throws, propagating as an unhandled render error (crashes the shape component unless an error boundary catches it).
Additionally, the empty dependency array means live setting changes during a session are silently ignored until the canvas remounts. If that's intentional, a comment is warranted; otherwise the call should move outside the component or listen to a settings context.
🐛 Proposed defensive fallback
- const isOverlayEnabled = useMemo(
- () => getPersonalSetting<boolean>(["Overlay in canvas"]),
- [],
- );
+ const isOverlayEnabled = useMemo(
+ () => getPersonalSetting<boolean>(["Overlay in canvas"]) ?? false,
+ [],
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isOverlayEnabled = useMemo( | |
| () => getSetting(DISCOURSE_CONTEXT_OVERLAY_IN_CANVAS_KEY, false), | |
| () => getPersonalSetting<boolean>(["Overlay in canvas"]), | |
| [], | |
| ); | |
| const isOverlayEnabled = useMemo( | |
| () => getPersonalSetting<boolean>(["Overlay in canvas"]) ?? false, | |
| [], | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/roam/src/components/canvas/DiscourseNodeUtil.tsx` around lines 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.
| const autoCanvasRelations = getPersonalSetting<boolean>([ | ||
| "Auto canvas relations", | ||
| ]); | ||
| if (autoCanvasRelations) { | ||
| try { |
There was a problem hiding this comment.
getPersonalSetting for autoCanvasRelations is outside the try/catch — unhandled throw
The getPersonalSetting call at line 515 sits before the try block at line 519. If it throws (dual-read path, unset setting), the error bypasses the renderToast error handler and propagates uncaught through the onSuccess callback.
🐛 Move the read inside the guarded block
- const autoCanvasRelations = getPersonalSetting<boolean>([
- "Auto canvas relations",
- ]);
- if (autoCanvasRelations) {
- try {
+ try {
+ const autoCanvasRelations = getPersonalSetting<boolean>([
+ "Auto canvas relations",
+ ]);
+ if (autoCanvasRelations) {
const relationIds = getRelationIds();
this.deleteRelationsInCanvas({ shape, relationIds });
await this.createExistingRelations({
shape,
relationIds,
finalUid: uid,
});
- } catch (error) {
- renderToast({
- id: `discourse-node-error-${Date.now()}`,
- intent: "danger",
- content: (
- <span>Error creating relations: {String(error)}</span>
- ),
- });
}
+ } catch (error) {
+ renderToast({
+ id: `discourse-node-error-${Date.now()}`,
+ intent: "danger",
+ content: (
+ <span>Error creating relations: {String(error)}</span>
+ ),
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const autoCanvasRelations = getPersonalSetting<boolean>([ | |
| "Auto canvas relations", | |
| ]); | |
| if (autoCanvasRelations) { | |
| try { | |
| try { | |
| const autoCanvasRelations = getPersonalSetting<boolean>([ | |
| "Auto canvas relations", | |
| ]); | |
| if (autoCanvasRelations) { | |
| const relationIds = getRelationIds(); | |
| this.deleteRelationsInCanvas({ shape, relationIds }); | |
| await this.createExistingRelations({ | |
| shape, | |
| relationIds, | |
| finalUid: uid, | |
| }); | |
| } | |
| } catch (error) { | |
| renderToast({ | |
| id: `discourse-node-error-${Date.now()}`, | |
| intent: "danger", | |
| content: ( | |
| <span>Error creating relations: {String(error)}</span> | |
| ), | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/roam/src/components/canvas/DiscourseNodeUtil.tsx` around lines 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.
| const discourseToolCombo = getPersonalSetting<IKeyCombo>([ | ||
| "Discourse tool shortcut", | ||
| ]) || { | ||
| key: "", | ||
| modifiers: 0, | ||
| }) as IKeyCombo; | ||
| }; | ||
|
|
||
| // For discourse tool, just use the key directly since we don't allow modifiers | ||
| const discourseToolShortcut = discourseToolCombo?.key?.toUpperCase() || ""; |
There was a problem hiding this comment.
getPersonalSetting can throw here — the || fallback only handles falsy returns, not exceptions; a throw during rendering breaks the canvas.
The || on line 275 handles undefined but not a thrown error. If getPersonalSetting throws inside the tools override (called during tldraw rendering), the entire canvas UI fails to render.
Additionally, getPersonalSetting<IKeyCombo> is a TypeScript-only type assertion — the stored JSON value may not actually match IKeyCombo at runtime (e.g., if legacy settings stored just a string). The .key access on line 281 would then silently return undefined.
🛡️ Proposed fix
- const discourseToolCombo = getPersonalSetting<IKeyCombo>([
- "Discourse tool shortcut",
- ]) || {
- key: "",
- modifiers: 0,
- };
+ let discourseToolCombo: IKeyCombo = { key: "", modifiers: 0 };
+ try {
+ const stored = getPersonalSetting<IKeyCombo>(["Discourse tool shortcut"]);
+ if (stored && typeof stored === "object" && "key" in stored) {
+ discourseToolCombo = stored;
+ }
+ } catch {
+ // setting unavailable; use default
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const discourseToolCombo = getPersonalSetting<IKeyCombo>([ | |
| "Discourse tool shortcut", | |
| ]) || { | |
| key: "", | |
| modifiers: 0, | |
| }) as IKeyCombo; | |
| }; | |
| // For discourse tool, just use the key directly since we don't allow modifiers | |
| const discourseToolShortcut = discourseToolCombo?.key?.toUpperCase() || ""; | |
| let discourseToolCombo: IKeyCombo = { key: "", modifiers: 0 }; | |
| try { | |
| const stored = getPersonalSetting<IKeyCombo>(["Discourse tool shortcut"]); | |
| if (stored && typeof stored === "object" && "key" in stored) { | |
| discourseToolCombo = stored; | |
| } | |
| } catch { | |
| // setting unavailable; use default | |
| } | |
| // For discourse tool, just use the key directly since we don't allow modifiers | |
| const discourseToolShortcut = discourseToolCombo?.key?.toUpperCase() || ""; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/roam/src/components/canvas/uiOverrides.tsx` around lines 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).
| if (getPersonalSetting<boolean>(["Hide feedback button"])) { | ||
| floatingMenuAnchor.classList.add("hidden"); | ||
| } |
There was a problem hiding this comment.
Unguarded getPersonalSetting at install time — a throw would prevent the floating menu from rendering.
getPersonalSetting throws when dual-read is enabled and the setting path resolves to undefined (see accessors.ts line 759). If this occurs during extension load (e.g., settings page not yet created), installDiscourseFloatingMenu aborts before ReactDOM.render is reached and no UI feedback is given.
🛡️ Proposed fix
- if (getPersonalSetting<boolean>(["Hide feedback button"])) {
+ if (getPersonalSetting<boolean>(["Hide feedback button"]) === true) {Actually the safer fix is wrapping:
+ let hideFeedback = false;
+ try { hideFeedback = getPersonalSetting<boolean>(["Hide feedback button"]) ?? false; }
+ catch { /* default to visible */ }
+ if (hideFeedback) {
- if (getPersonalSetting<boolean>(["Hide feedback button"])) {
floatingMenuAnchor.classList.add("hidden");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (getPersonalSetting<boolean>(["Hide feedback button"])) { | |
| floatingMenuAnchor.classList.add("hidden"); | |
| } | |
| let hideFeedback = false; | |
| try { | |
| hideFeedback = getPersonalSetting<boolean>(["Hide feedback button"]) ?? false; | |
| } catch { | |
| /* default to visible */ | |
| } | |
| if (hideFeedback) { | |
| floatingMenuAnchor.classList.add("hidden"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/roam/src/components/DiscourseFloatingMenu.tsx` around lines 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.
| const disallowDiagnostics = getPersonalSetting<boolean>([ | ||
| "Disable product diagnostics", | ||
| ]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Step 1: Find isDualReadEnabled definition and understand its behavior
echo "=== isDualReadEnabled definition ==="
rg -n "isDualReadEnabled" --type ts -A 5
echo ""
echo "=== initSchema definition and its behavior ==="
# Find initSchema to see if it populates defaults
rg -n "initSchema" --type ts -A 10 | head -50
echo ""
echo "=== Execution order in index.ts ==="
# Confirm execution order in index.ts
grep -n "initObservers\|initSchema\|getPersonalSetting\|runExtension" apps/roam/src/index.ts | head -30
echo ""
echo "=== getPersonalSetting implementation ==="
rg -n "getPersonalSetting" --type ts -B 2 -A 15 | head -80Repository: DiscourseGraphs/discourse-graph
Length of output: 9768
🏁 Script executed:
#!/bin/bash
# Get full getPersonalSetting implementation
echo "=== Full getPersonalSetting implementation ==="
rg -n "export const getPersonalSetting" --type ts -A 15
echo ""
echo "=== readPathValue implementation ==="
rg -n "readPathValue" --type ts -B 2 -A 10 | head -40
echo ""
echo "=== getGlobalSettings implementation ==="
rg -n "getGlobalSettings" --type ts -A 8 | head -30
echo ""
echo "=== What initSchema actually does ==="
cat -n apps/roam/src/components/settings/utils/init.ts | head -50Repository: DiscourseGraphs/discourse-graph
Length of output: 12078
getPersonalSetting called before settings are initialized — will throw on first load when dual-read is enabled
Both getPersonalSetting calls (lines 47–49 and 92–94) execute inside runExtension before initSchema() at line 158. When isDualReadEnabled() returns true, getPersonalSetting throws an Error if the setting value is undefined (e.g., a new user with no saved preferences). This aborts the entire extension load.
Two options:
- Move the calls after
initSchema()(preferred — ensures settings are bootstrapped). - Add a fallback at each call site.
Option 2 — safe fallback for both reads
- const disallowDiagnostics = getPersonalSetting<boolean>([
- "Disable product diagnostics",
- ]);
+ const disallowDiagnostics =
+ getPersonalSetting<boolean>(["Disable product diagnostics"]) ?? false;- const isStreamlineStylingEnabled = getPersonalSetting<boolean>([
- "Streamline styling",
- ]);
+ const isStreamlineStylingEnabled =
+ getPersonalSetting<boolean>(["Streamline styling"]) ?? false;Note: initObservers (line 101) is called between these two groups and has the same problem — see the comment on initializeObserversAndListeners.ts.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const disallowDiagnostics = getPersonalSetting<boolean>([ | |
| "Disable product diagnostics", | |
| ]); | |
| const disallowDiagnostics = | |
| getPersonalSetting<boolean>(["Disable product diagnostics"]) ?? false; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/roam/src/index.ts` around lines 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.
| const handleOpenInSidebar = (uid: string) => { | ||
| if (extensionAPI?.settings.get("disable-sidebar-open")) return; | ||
| openBlockInSidebar(uid); | ||
| if (getPersonalSetting<boolean>(["Disable sidebar open"])) return; | ||
| void openBlockInSidebar(uid); |
There was a problem hiding this comment.
A throw from getPersonalSetting inside handleOpenInSidebar would abort node creation mid-flight.
The node may already have been created in Roam (e.g. at line 117 after createBlock) before handleOpenInSidebar is called. If getPersonalSetting throws here, the async createDiscourseNode call rejects, leaving the caller thinking creation failed even though the node exists.
🛡️ Proposed fix
const handleOpenInSidebar = (uid: string) => {
- if (getPersonalSetting<boolean>(["Disable sidebar open"])) return;
+ try {
+ if (getPersonalSetting<boolean>(["Disable sidebar open"])) return;
+ } catch {
+ // Setting unavailable; proceed to open sidebar
+ }
void openBlockInSidebar(uid);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/roam/src/utils/createDiscourseNode.ts` around lines 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.
| const nodeCreationPopoverListener = debounce(() => { | ||
| const isTextSelectionPopupEnabled = | ||
| onloadArgs.extensionAPI.settings.get("text-selection-popup") !== false; | ||
| getPersonalSetting<boolean>(["Text selection popup"]) !== false; | ||
|
|
There was a problem hiding this comment.
!== false opt-out semantics break in dual-read path
"Text selection popup" is intentionally opt-out (enabled by default). This relies on getPersonalSetting returning undefined for an unset setting (undefined !== false === true). However, when isDualReadEnabled() is true, getPersonalSetting throws instead of returning undefined, so the default-enabled behaviour is lost entirely.
The correct fallback here is ?? true (not ?? false), to preserve opt-out semantics:
🐛 Preserve opt-out default
const isTextSelectionPopupEnabled =
- getPersonalSetting<boolean>(["Text selection popup"]) !== false;
+ (getPersonalSetting<boolean>(["Text selection popup"]) ?? true) !== false;Or more readably:
const isTextSelectionPopupEnabled =
- getPersonalSetting<boolean>(["Text selection popup"]) !== false;
+ getPersonalSetting<boolean>(["Text selection popup"]) ?? true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const nodeCreationPopoverListener = debounce(() => { | |
| const isTextSelectionPopupEnabled = | |
| onloadArgs.extensionAPI.settings.get("text-selection-popup") !== false; | |
| getPersonalSetting<boolean>(["Text selection popup"]) !== false; | |
| const nodeCreationPopoverListener = debounce(() => { | |
| const isTextSelectionPopupEnabled = | |
| (getPersonalSetting<boolean>(["Text selection popup"]) ?? true) !== false; |
| const nodeCreationPopoverListener = debounce(() => { | |
| const isTextSelectionPopupEnabled = | |
| onloadArgs.extensionAPI.settings.get("text-selection-popup") !== false; | |
| getPersonalSetting<boolean>(["Text selection popup"]) !== false; | |
| const nodeCreationPopoverListener = debounce(() => { | |
| const isTextSelectionPopupEnabled = | |
| getPersonalSetting<boolean>(["Text selection popup"]) ?? 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 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).
| if ( | ||
| getSetting(DISALLOW_DIAGNOSTICS, false) || | ||
| getPersonalSetting<boolean>(["Disable product diagnostics"]) || | ||
| (process.env.NODE_ENV === "development" && forceSendInDev !== true) | ||
| ) { | ||
| console.error(error, context); |
There was a problem hiding this comment.
internalError can itself throw — wrap getPersonalSetting in a try/catch here.
getPersonalSetting throws a new Error(...) whenever isDualReadEnabled() is true and the resolved value is undefined (see accessors.ts lines 759-761). Even if the Zod schema provides defaults making this unlikely in production, the call is inside the error-handling utility that the rest of the codebase relies on to be unconditionally safe:
internalErroris called byvalidateSettingValue, which is called bysetPersonalSetting/setGlobalSetting. Any error during a settings write now triggers a path that calls back into the same settings infrastructure.- If
getPersonalSettingthrows before Roam block-props are ready (e.g., very early in extension init),internalErrorpropagates instead of swallowing.
🐛 Proposed fix — guard against throw
if (
- getPersonalSetting<boolean>(["Disable product diagnostics"]) ||
+ (() => {
+ try { return getPersonalSetting<boolean>(["Disable product diagnostics"]); }
+ catch { return false; }
+ })() ||
(process.env.NODE_ENV === "development" && forceSendInDev !== true)
) {Or extract it above the if:
+ let diagnosticsDisabled = false;
+ try {
+ diagnosticsDisabled = getPersonalSetting<boolean>(["Disable product diagnostics"]) ?? false;
+ } catch {
+ // settings not ready; fall through to full error reporting
+ }
if (
- getPersonalSetting<boolean>(["Disable product diagnostics"]) ||
+ diagnosticsDisabled ||
(process.env.NODE_ENV === "development" && forceSendInDev !== true)
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| getSetting(DISALLOW_DIAGNOSTICS, false) || | |
| getPersonalSetting<boolean>(["Disable product diagnostics"]) || | |
| (process.env.NODE_ENV === "development" && forceSendInDev !== true) | |
| ) { | |
| console.error(error, context); | |
| let diagnosticsDisabled = false; | |
| try { | |
| diagnosticsDisabled = getPersonalSetting<boolean>(["Disable product diagnostics"]) ?? false; | |
| } catch { | |
| // settings not ready; fall through to full error reporting | |
| } | |
| if ( | |
| diagnosticsDisabled || | |
| (process.env.NODE_ENV === "development" && forceSendInDev !== true) | |
| ) { | |
| console.error(error, context); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/roam/src/utils/internalError.ts` around lines 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.
| export const initPostHog = (): void => { | ||
| const disabled = getSetting(DISALLOW_DIAGNOSTICS, false); | ||
| const disabled = getPersonalSetting<boolean>(["Disable product diagnostics"]); | ||
| if (!disabled) { | ||
| doInitPostHog(); | ||
| } |
There was a problem hiding this comment.
getPersonalSetting call in initPostHog can throw during early initialization — no error guard.
initPostHog is called at extension startup before settings block-props may be fully available. If getPersonalSetting throws (dual-read enabled, settings page not yet created), doInitPostHog is never called and PostHog remains uninitialized with no log or fallback.
🛡️ Proposed fix
export const initPostHog = (): void => {
- const disabled = getPersonalSetting<boolean>(["Disable product diagnostics"]);
- if (!disabled) {
+ let disabled = false;
+ try {
+ disabled = getPersonalSetting<boolean>(["Disable product diagnostics"]) ?? false;
+ } catch {
+ // settings not ready; default to enabled (non-invasive)
+ }
+ if (!disabled) {
doInitPostHog();
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const initPostHog = (): void => { | |
| const disabled = getSetting(DISALLOW_DIAGNOSTICS, false); | |
| const disabled = getPersonalSetting<boolean>(["Disable product diagnostics"]); | |
| if (!disabled) { | |
| doInitPostHog(); | |
| } | |
| export const initPostHog = (): void => { | |
| let disabled = false; | |
| try { | |
| disabled = getPersonalSetting<boolean>(["Disable product diagnostics"]) ?? false; | |
| } catch { | |
| // settings not ready; default to enabled (non-invasive) | |
| } | |
| if (!disabled) { | |
| doInitPostHog(); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/roam/src/utils/posthog.ts` around lines 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.
1 participant
Summary by CodeRabbit