ENG-1472: Refactor BlockPropSettingPanels to add accessor-backed default reads

[sid597]

https://www.loom.com/share/1aaa5bcdb1344013af27c045e1f84b2c

main
    └── migration-block-init-staging-branch  (staging base)
         └── eng-1455 (#812)
              └── eng-1472  (this PR)

---

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-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
• ENG-1472: Refactor BlockPropSettingPanels to add accessor-backed default reads #813 👈 (View in Graphite)
• 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: 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
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title clearly summarizes the main change: refactoring BlockPropSettingPanels to add accessor-backed default reads with fallback logic.
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]

🧹 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.