ENG-1467: Port global setting consumer reads (→ getGlobalSetting)#821
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
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.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR migrates configuration and feature flag access throughout the codebase from local tree-based and extension settings to centralized feature-flag and global-setting accessors. It removes unnecessary initialValue props from UI components and eliminates memoization patterns, consolidating how application settings are retrieved. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/roam/src/components/settings/GeneralSettings.tsx (1)
12-68:⚠️ Potential issue | 🟡 MinorAdd an explicit return type for the component.
This file is TypeScript and the component function is missing an explicit return type.
✅ Suggested fix
-const DiscourseGraphHome = () => { +const DiscourseGraphHome = (): React.JSX.Element => {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/GeneralSettings.tsx` around lines 12 - 68, The DiscourseGraphHome component is missing an explicit return type; update its declaration to include one (e.g., change "const DiscourseGraphHome = () => {" to "const DiscourseGraphHome = (): JSX.Element => {" or use React.FC) so TypeScript has an explicit component return type; ensure any necessary React imports/types (JSX.Element or React) are available in the file and keep the existing function body unchanged.
🧹 Nitpick comments (2)
apps/roam/src/components/SuggestionsBody.tsx (1)
314-382:getFeatureFlag("Reified relation triples")evaluated twice in the same async callThe flag is read at line 315 to branch behaviour, then read again at line 379 for the PostHog payload. Since
handleCreateBlockisasyncand awaits writes between the two reads, storing the result once is both cleaner and guards against a (hypothetical) mid-call flag change.♻️ Proposed refactor
const handleCreateBlock = async (node: SuggestedNode) => { + const useReifiedRelations = getFeatureFlag("Reified relation triples"); - if (getFeatureFlag("Reified relation triples")) { + if (useReifiedRelations) { // ... } posthog.capture("Suggestive Mode: Suggestion Adopted", { tag, nodeType: node.type, nodeText: node.text, - useReifiedRelations: getFeatureFlag("Reified relation triples"), + useReifiedRelations, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/roam/src/components/SuggestionsBody.tsx` around lines 314 - 382, handleCreateBlock reads getFeatureFlag("Reified relation triples") twice across awaits which can lead to inconsistent behavior; capture the flag once at the start of handleCreateBlock (e.g., const useReifiedRelations = getFeatureFlag("Reified relation triples")) and use that variable everywhere in the function (the branching logic that decides between createReifiedRelation/createBlock and the posthog.capture payload), keeping existing calls to createReifiedRelation, createBlock, posthog.capture and state updates unchanged.apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx (1)
53-57: Merge duplicate imports from the same module
getFeatureFlag,setGlobalSetting, andgetGlobalSettingsall come from the same path but are split across twoimportstatements.♻️ Proposed consolidation
-import { getFeatureFlag } from "~/components/settings/utils/accessors"; import { + getFeatureFlag, setGlobalSetting, getGlobalSettings, } from "~/components/settings/utils/accessors";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx` around lines 53 - 57, The imports getFeatureFlag, setGlobalSetting, and getGlobalSettings are split into two statements from the same module; consolidate them into a single import declaration that imports all three symbols from "~/components/settings/utils/accessors" (so update the import lines that reference getFeatureFlag and the setGlobalSetting/getGlobalSettings pair to one combined import).
🤖 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/SuggestionsBody.tsx`:
- Around line 422-427: getGlobalSetting(["Suggestive mode", "Page groups"]) can
throw in dual-read mode so the current "?? []" fallback never runs; wrap the
call in a try/catch (or guard with isDualReadEnabled() and safe-read logic) so
that if getGlobalSetting throws you assign groups = [] instead of letting the
exception propagate. Update the code where groups is declared and where
groupsRecord is produced (the groups variable used in the reduce) to use the
caught/guarded value, and optionally log a warning when falling back so failures
are visible.
In `@apps/roam/src/utils/initializeObserversAndListeners.ts`:
- Line 255: initObservers currently calls getGlobalSetting synchronously (const
globalTrigger = (getGlobalSetting<string>(["Trigger"]) ?? "\\").trim()) which
can throw in dual-read mode and abort initObservers; wrap the getGlobalSetting
call in a defensive try/catch (inside initObservers) or use a safe helper that
returns undefined on error, then apply the ?? "\\" fallback and .trim() only on
a non-null string; refer to the initObservers function and the getGlobalSetting
call so the fix isolates the lookup, handles exceptions, and ensures
observers/listeners continue initializing when the new store key is missing.
In `@apps/roam/src/utils/isCanvasPage.ts`:
- Around line 5-7: getGlobalSetting can throw in dual-read mode so the current
`|| DEFAULT_CANVAS_PAGE_FORMAT` fallback is unreachable; wrap the call to
`getGlobalSetting<string>(["Canvas page format"])` inside a try/catch (in the
`isCanvasPage` utility) and on any exception return/assign
`DEFAULT_CANVAS_PAGE_FORMAT` so the fallback is honored; keep the rest of the
logic unchanged and reference `isDualReadEnabled`, `getGlobalSetting`, and
`DEFAULT_CANVAS_PAGE_FORMAT` to locate where to add the try/catch.
---
Outside diff comments:
In `@apps/roam/src/components/settings/GeneralSettings.tsx`:
- Around line 12-68: The DiscourseGraphHome component is missing an explicit
return type; update its declaration to include one (e.g., change "const
DiscourseGraphHome = () => {" to "const DiscourseGraphHome = (): JSX.Element =>
{" or use React.FC) so TypeScript has an explicit component return type; ensure
any necessary React imports/types (JSX.Element or React) are available in the
file and keep the existing function body unchanged.
---
Nitpick comments:
In `@apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx`:
- Around line 53-57: The imports getFeatureFlag, setGlobalSetting, and
getGlobalSettings are split into two statements from the same module;
consolidate them into a single import declaration that imports all three symbols
from "~/components/settings/utils/accessors" (so update the import lines that
reference getFeatureFlag and the setGlobalSetting/getGlobalSettings pair to one
combined import).
In `@apps/roam/src/components/SuggestionsBody.tsx`:
- Around line 314-382: handleCreateBlock reads getFeatureFlag("Reified relation
triples") twice across awaits which can lead to inconsistent behavior; capture
the flag once at the start of handleCreateBlock (e.g., const useReifiedRelations
= getFeatureFlag("Reified relation triples")) and use that variable everywhere
in the function (the branching logic that decides between
createReifiedRelation/createBlock and the posthog.capture payload), keeping
existing calls to createReifiedRelation, createBlock, posthog.capture and state
updates unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/roam/src/components/SuggestionsBody.tsxapps/roam/src/components/settings/AdminPanel.tsxapps/roam/src/components/settings/DiscourseRelationConfigPanel.tsxapps/roam/src/components/settings/ExportSettings.tsxapps/roam/src/components/settings/GeneralSettings.tsxapps/roam/src/components/settings/HomePersonalSettings.tsxapps/roam/src/components/settings/NodeConfig.tsxapps/roam/src/utils/initializeObserversAndListeners.tsapps/roam/src/utils/isCanvasPage.tsapps/roam/src/utils/registerDiscourseDatalogTranslators.ts
💤 Files with no reviewable changes (1)
- apps/roam/src/components/settings/ExportSettings.tsx
| key: "trigger", | ||
| defaultValue: "\\", | ||
| }).trim(); | ||
| const globalTrigger = (getGlobalSetting<string>(["Trigger"]) ?? "\\").trim(); |
There was a problem hiding this comment.
getGlobalSetting throw in dual-read mode can abort initObservers entirely
Line 255 sits in the synchronous body of initObservers before any await. When isDualReadEnabled() is true and the "Trigger" key has not yet been written to the new settings store, getGlobalSetting throws, the ?? "\\" fallback is never evaluated, and the entire initObservers promise rejects — leaving no observers, no listeners, and a non-functional extension for developers using dual-read mode.
This is the same root cause as the isCanvasPage.ts and SuggestionsBody.tsx issues, but with a higher blast radius.
🛡️ Proposed fix
- const globalTrigger = (getGlobalSetting<string>(["Trigger"]) ?? "\\").trim();
+ let globalTrigger: string;
+ try {
+ globalTrigger = (getGlobalSetting<string>(["Trigger"]) ?? "\\").trim();
+ } catch {
+ globalTrigger = "\\";
+ }📝 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 globalTrigger = (getGlobalSetting<string>(["Trigger"]) ?? "\\").trim(); | |
| let globalTrigger: string; | |
| try { | |
| globalTrigger = (getGlobalSetting<string>(["Trigger"]) ?? "\\").trim(); | |
| } catch { | |
| globalTrigger = "\\"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/roam/src/utils/initializeObserversAndListeners.ts` at line 255,
initObservers currently calls getGlobalSetting synchronously (const
globalTrigger = (getGlobalSetting<string>(["Trigger"]) ?? "\\").trim()) which
can throw in dual-read mode and abort initObservers; wrap the getGlobalSetting
call in a defensive try/catch (inside initObservers) or use a safe helper that
returns undefined on error, then apply the ?? "\\" fallback and .trim() only on
a non-null string; refer to the initObservers function and the getGlobalSetting
call so the fix isolates the lookup, handles exceptions, and ensures
observers/listeners continue initializing when the new store key is missing.
There was a problem hiding this comment.
thats the goal to figure out bugs explicitly this pr and the project is under testing so it makes sense for this phase of the project
There was a problem hiding this comment.
@sid597, understood! That makes perfect sense for a testing phase — letting the dual-read mode surface these issues explicitly will help ensure complete coverage when you eventually switch over. This way you'll have a clear inventory of every spot that needs the defensive handling.
Feel free to ping me if you'd like help cataloging or triaging any of the exceptions that come up during testing. Good luck with the migration! 🚀
🧠 Learnings used
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 377
File: packages/ui/tsconfig.json:5-7
Timestamp: 2025-08-27T02:23:45.696Z
Learning: When reviewing TypeScript configuration changes, check if settings like "declaration" are inherited from base configs before suggesting to add them explicitly. Many monorepo packages extend base configurations that already include necessary compiler options.
| const format = | ||
| getGlobalSetting<string>(["Canvas page format"]) || | ||
| DEFAULT_CANVAS_PAGE_FORMAT; |
There was a problem hiding this comment.
getGlobalSetting throws in dual-read mode — || DEFAULT_CANVAS_PAGE_FORMAT fallback is unreachable
When isDualReadEnabled() is true, getGlobalSetting throws an error (rather than returning undefined) if the key is absent. The || fallback only catches undefined/falsy values, not thrown exceptions. In dual-read mode with "Canvas page format" not yet written to the new settings store, every isCanvasPage call (triggered on each H1 page-title change) will throw, breaking page rendering.
isDualReadEnabled() is false in production so this doesn't affect end-users, but any developer enabling "Enable dual read" will hit this.
🛡️ Proposed fix — wrap in try/catch to honour the intended fallback
- const format =
- getGlobalSetting<string>(["Canvas page format"]) ||
- DEFAULT_CANVAS_PAGE_FORMAT;
+ let format: string;
+ try {
+ format =
+ getGlobalSetting<string>(["Canvas page format"]) ||
+ DEFAULT_CANVAS_PAGE_FORMAT;
+ } catch {
+ format = DEFAULT_CANVAS_PAGE_FORMAT;
+ }📝 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 format = | |
| getGlobalSetting<string>(["Canvas page format"]) || | |
| DEFAULT_CANVAS_PAGE_FORMAT; | |
| let format: string; | |
| try { | |
| format = | |
| getGlobalSetting<string>(["Canvas page format"]) || | |
| DEFAULT_CANVAS_PAGE_FORMAT; | |
| } catch { | |
| format = DEFAULT_CANVAS_PAGE_FORMAT; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/roam/src/utils/isCanvasPage.ts` around lines 5 - 7, getGlobalSetting can
throw in dual-read mode so the current `|| DEFAULT_CANVAS_PAGE_FORMAT` fallback
is unreachable; wrap the call to `getGlobalSetting<string>(["Canvas page
format"])` inside a try/catch (in the `isCanvasPage` utility) and on any
exception return/assign `DEFAULT_CANVAS_PAGE_FORMAT` so the fallback is honored;
keep the rest of the logic unchanged and reference `isDualReadEnabled`,
`getGlobalSetting`, and `DEFAULT_CANVAS_PAGE_FORMAT` to locate where to add the
try/catch.
There was a problem hiding this comment.
thats the goal to figure out bugs explicitly this pr and the project is under testing so it makes sense for this phase of the project
There was a problem hiding this comment.
@sid597, understood — letting it throw explicitly during testing makes sense for catching migration issues early. Thanks for clarifying the intent! 👍
🧠 Learnings used
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-06-25T22:56:17.522Z
Learning: In the Roam discourse-graph system, the existence of the configuration page (identified by DISCOURSE_CONFIG_PAGE_TITLE) and its corresponding UID is a system invariant. The code can safely assume this page will always exist, so defensive null checks are not needed when using `getPageUidByPageTitle(DISCOURSE_CONFIG_PAGE_TITLE)`.
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
sid597
force-pushed
the
eng-1473-port-non-boolean-personal-setting-consumers
branch
from
0b44d51 to
8317887
Compare
sid597
force-pushed
the
eng-1467-port-global-setting-consumer-reads-getglobalsetting
branch
from
85fe628 to
60c1f24
Compare
sid597
force-pushed
the
eng-1473-port-non-boolean-personal-setting-consumers
branch
from
8317887 to
670bf75
Compare
sid597
force-pushed
the
eng-1467-port-global-setting-consumer-reads-getglobalsetting
branch
from
60c1f24 to
617bcbf
Compare
sid597
force-pushed
the
eng-1473-port-non-boolean-personal-setting-consumers
branch
from
670bf75 to
67dae77
Compare
sid597
force-pushed
the
eng-1467-port-global-setting-consumer-reads-getglobalsetting
branch
from
617bcbf to
04d638f
Compare
sid597
force-pushed
the
eng-1473-port-non-boolean-personal-setting-consumers
branch
from
67dae77 to
5f3de8e
Compare
sid597
force-pushed
the
eng-1467-port-global-setting-consumer-reads-getglobalsetting
branch
from
04d638f to
57ed4ae
Compare
sid597
force-pushed
the
eng-1473-port-non-boolean-personal-setting-consumers
branch
from
5f3de8e to
009e271
Compare
sid597
force-pushed
the
eng-1467-port-global-setting-consumer-reads-getglobalsetting
branch
from
57ed4ae to
7ae0c4e
Compare
1 participant
Summary by CodeRabbit