ENG-1472: Refactor BlockPropSettingPanels to add accessor-backed default reads (with initialValue fallback)

[sid597]

---

Summary by CodeRabbit

• Refactor
  • Improved settings initialization mechanism with enhanced fallback support across global, personal, and feature flag setting panels. Changes ensure more robust setting retrieval while maintaining existing component behavior.

[linear]

ENG-1472 Refactor BlockPropSettingPanels to add accessor-backed default reads (with initialValue fallback)

[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-1473: Port non-boolean personal setting consumers #818
• ENG-1456: Migrate personal boolean flag consumers (getSetting → getPersonalSetting) #814
• ENG-1472: Refactor BlockPropSettingPanels to add accessor-backed default reads (with initialValue fallback) #813 👈 (View in Graphite)
• ENG-1455: Dual-read from old-system settings and from blockprops #812
• ENG-1465: Enable dual read feature flag #811
• ENG-1217: Port section component in left sidebar #804
• ENG-1328: Port small blueprint based misc settings into props based #805
• graphite-base/805

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: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional findings.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR refactors BlockPropSettingPanels.tsx to introduce a readWithFallback helper and replaces direct initialValue usage with calls to setting accessor functions (getGlobalSetting, getPersonalSetting, getFeatureFlag, getDiscourseNodeSetting), each with a fallback to existing initialValues.

Changes

Cohort / File(s)	Summary
BlockPropSettingPanels Refactoring apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx	Introduces readWithFallback helper and updates Global, Personal, and DiscourseNode setting panels to resolve initial values via dedicated accessor functions with fallback support. Adds imports for getGlobalSetting, getPersonalSetting, getFeatureFlag, and getDiscourseNodeSetting.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• ENG-1190 Replace existing roam-js components with blockProp based ones #689 — Modifies the same BlockPropSettingPanels.tsx components by replacing direct initialValue usage with readWithFallback and getXSetting helpers for initial value resolution.
• ENG-1187: Prod: Implement get and set for block prop based settings #669 — Implements the new accessor functions (getGlobalSetting, getPersonalSetting, getFeatureFlag, getDiscourseNodeSetting) that this PR depends on and utilizes.
• ENG-1188: prod: pull watchers for prop based settings #682 — Related to the settings plumbing infrastructure; implements pull-watchers and adjustments to the accessors used by these new helper functions.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: refactoring BlockPropSettingPanels to integrate accessor-backed default reads with initialValue fallback, which directly aligns with the changeset.
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]

🧹 Nitpick comments (1)

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

473-483: Consider logging caught errors for debuggability.

The bare catch silently swallows all exceptions — including unexpected ones (e.g., TypeError from a bug in readPathValue). This is a reasonable resilience trade-off for UI code, but a console.warn would make misconfigured keys or accessor bugs much easier to diagnose.

Proposed fix

 const readWithFallback = <T,>(
   reader: () => T | undefined,
   fallback?: T,
 ): T | undefined => {
   try {
     const value = reader();
     return value ?? fallback;
-  } catch {
+  } catch (e) {
+    console.warn("[readWithFallback] accessor threw, using fallback:", e);
     return fallback;
   }
 };

🤖 Prompt for AI Agents

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

In `@apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx`
around lines 473 - 483, The readWithFallback function currently swallows all
exceptions silently; update it to catch the error into a variable and call
console.warn (or processLogger if available) with a concise message including
the error and context (e.g., "readWithFallback failed" and the thrown error)
before returning the fallback; keep existing behavior of returning fallback on
exceptions and preserve the reader/fallback signature so callers of
readWithFallback continue to work unchanged.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx`:
- Around line 473-483: The readWithFallback function currently swallows all
exceptions silently; update it to catch the error into a variable and call
console.warn (or processLogger if available) with a concise message including
the error and context (e.g., "readWithFallback failed" and the thrown error)
before returning the fallback; keep existing behavior of returning fallback on
exceptions and preserve the reader/fallback signature so callers of
readWithFallback continue to work unchanged.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 10 additional findings in Devin Review.

[devin-ai-integration]

🔴 readWithFallback never falls back for FeatureFlagPanel because getFeatureFlag always returns boolean

The FeatureFlagPanel component uses readWithFallback(() => getFeatureFlag(featureKey), initialValue) to compute the initial value. However, getFeatureFlag always returns a boolean (never undefined or null) because FeatureFlagsSchema has .default(false) for every field. Since readWithFallback uses value ?? fallback, and false ?? initialValue evaluates to false, the caller's initialValue fallback is effectively dead code under normal conditions.

Root cause and impact

getFeatureFlag at apps/roam/src/components/settings/utils/accessors.ts:668-671 calls getFeatureFlags() which parses block props with FeatureFlagsSchema.parse(blockProps || {}). The schema defines all flags with .default(false), so even when no block props exist, the parse returns { "Enable left sidebar": false, ... }.

In readWithFallback at line 494-495:

const value = reader(); // returns false (not undefined)
return value ?? fallback; // false ?? initialValue → false

This means the legacy initialValue passed by callers (e.g., settings.leftSidebarEnabled.value in GeneralSettings.tsx:46) is ignored. For users who had a feature flag enabled in the legacy system but whose block props were initialized with schema defaults (false), the checkbox will incorrectly show as unchecked.

For example, a user with the left sidebar enabled via the legacy block tree will see the checkbox unchecked after this change, because block props return false and the legacy initialValue={true} is never used as fallback.

Impact: Feature flag checkboxes may display incorrect initial state for existing users whose flags haven't been migrated to block props, contradicting the PR's stated goal of "with initialValue fallback."

Prompt for agents

In apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx, the readWithFallback function at line 489-499 uses the nullish coalescing operator (??), which only falls back for undefined/null. For getFeatureFlag, which always returns a boolean (false as default from schema), the fallback never triggers.

The fix depends on the desired semantics:

Option A: If the intent is to prefer block props when they have been explicitly set, and fall back to initialValue otherwise, then getFeatureFlag needs to distinguish between "explicitly set to false" and "defaulted to false because no value exists." This would require changing getFeatureFlag (or adding a variant) in apps/roam/src/components/settings/utils/accessors.ts around line 668-671 to return undefined when the block props don't contain an explicit value for the key, rather than returning the schema default.

Option B: If block props are always the source of truth for feature flags (because setFeatureFlag writes to block props), then the initialValue prop on FeatureFlagPanel is unnecessary and should be documented as such, or removed from the FeatureFlagPanel interface to avoid confusion.

---

Was this helpful? React with 👍 or 👎 to provide feedback.