ENG-1272 Migrate all small global settings components

[sid597]

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

---

[linear]

ENG-1272 Migrate all small global settings component

[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]

• ENG-1272 Migrate all small global settings components #688 : 2 dependent PRs (#690 , #691 ) 👈 (View in Graphite)
• ENG-1345: Modify existing RoamJS component onChange to also change props #745 : 1 other dependent PR (#684 )
• ENG-1190 Replace existing roam-js components with blockProp based ones #689
• ENG-1188: prod: pull watchers for prop based settings #682
• ENG-1187: Prod: Implement get and set for block prop based settings #669
• ENG-1189: Prod: Init block prop based schema #666
• ENG-1330: zod schema for discourse relations #719
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[devin-ai-integration]

Devin Review found 2 new potential issues.

View issues and 9 additional flags in Devin Review.

[devin-ai-integration]

Devin Review found 1 potential issue.

View issue and 4 additional flags in Devin Review.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 6 additional findings in Devin Review.

[devin-ai-integration]

🟡 Debug console.log statements left in production code

Multiple debug console.log statements were added inside setTimeout callbacks throughout the BlockPropSettingPanels.tsx file. These appear to have been added for testing purposes (per commit message "add comments for testing") but should be removed before production.

Affected Locations

The debug statements appear at:

• apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx:128-130 (BaseTextPanel)
• apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx:201-203 (BaseFlagPanel)
• apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx:254-256 (BaseNumberPanel)
• apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx:306-308 (BaseSelectPanel)
• apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx:386-388 (BaseMultiTextPanel handleAdd)
• apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx:408-410 (BaseMultiTextPanel handleRemove)

Example pattern:

setTimeout(() => {
  console.log(`[FlagPanel] "${title}" after set, blockProp:`, getter(settingKeys));
}, 500);

Impact: These statements will log to the browser console in production, potentially exposing internal state and cluttering the console output. They also create unnecessary timers.

Prompt for agents

Remove all debug console.log statements wrapped in setTimeout callbacks from BlockPropSettingPanels.tsx. There are 6 occurrences:
1. Lines 128-131 in BaseTextPanel handleChange
2. Lines 201-204 in BaseFlagPanel handleChange
3. Lines 254-257 in BaseNumberPanel handleChange
4. Lines 306-309 in BaseSelectPanel handleChange
5. Lines 386-389 in BaseMultiTextPanel handleAdd
6. Lines 408-411 in BaseMultiTextPanel handleRemove

Each follows the pattern:
setTimeout(() => {
  console.log(`[PanelType] "${title}" after set/add/remove, blockProp:`, getter(settingKeys));
}, 500);

Simply delete these setTimeout blocks entirely.

---

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

[sid597]

Yes we need to do extensive testing before we merge so we will remove this later.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 9 additional findings in Devin Review.

[devin-ai-integration]

🔴 defaultValue takes precedence over stored settings, causing persisted values to be ignored

The new panel components initialize state with defaultValue ?? getter(settingKeys) ?? fallback, which means if defaultValue is provided (even if it's false), the stored value from block props will be ignored.

Root Cause and Impact

In the usage at ExportSettings.tsx:21, GeneralSettings.tsx:21, etc., defaultValue is set from the old config tree system (e.g., exportSettings.removeSpecialCharacters.value). Since the nullish coalescing operator (??) only falls through for null or undefined, any truthy or false value from the old system will take precedence.

This creates a migration problem:

1. User opens settings - sees value from old system (defaultValue)
2. User changes value - new value stored in block props via setter
3. User reopens settings - sees value from old system again because defaultValue overrides getter

The user's changes appear to be lost on reload because the component always shows the old config tree value instead of reading the newly persisted block props value.

Expected: getter(settingKeys) ?? defaultValue ?? fallback - stored values should take precedence
Actual: defaultValue ?? getter(settingKeys) ?? fallback - defaultValue always wins if defined

Prompt for agents

Fix the state initialization priority in all Base*Panel components in BlockPropSettingPanels.tsx. The getter should be checked first, then fall back to defaultValue. Change the following patterns:

In BaseTextPanel (line 108-110): Change `defaultValue ?? getter(settingKeys) ?? ""` to `getter(settingKeys) ?? defaultValue ?? ""`

In BaseFlagPanel (line 161-163): Change `defaultValue ?? getter(settingKeys) ?? false` to `getter(settingKeys) ?? defaultValue ?? false`

In BaseNumberPanel (line 234-236): Change `defaultValue ?? getter(settingKeys) ?? 0` to `getter(settingKeys) ?? defaultValue ?? 0`

In BaseSelectPanel (line 286-288): Change `defaultValue ?? getter(settingKeys) ?? options[0]` to `getter(settingKeys) ?? defaultValue ?? options[0]`

In BaseMultiTextPanel (line 336-338): Change `defaultValue ?? getter(settingKeys) ?? []` to `getter(settingKeys) ?? defaultValue ?? []`

This ensures that stored block prop values take precedence over the default values passed from the old config tree system.

---

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

[sid597]

Here we are going with defaultValue i.e if there is a default value then use it instead of using the gette() the getter gets from the block props but we don't want to read from there. defaultValue is passed from the caller side (mentioned later) and these defaults use the existing block structure.

[graphite-app]

Debug console.log statement left in production code. This will spam the console every time a text setting is changed and should be removed:

// Remove these lines:
setTimeout(() => {
  console.log(`[TextPanel] "${title}" after set, blockProp:`, getter(settingKeys));
}, 500);

Spotted by Graphite Agent



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

Debug console.log statement left in production code. This will spam the console every time a flag setting is toggled and should be removed:

// Remove these lines:
setTimeout(() => {
  console.log(`[FlagPanel] "${title}" after set, blockProp:`, getter(settingKeys));
}, 500);

Spotted by Graphite Agent



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

Debug console.log statement left in production code. This will spam the console every time a select setting is changed and should be removed:

// Remove these lines:
setTimeout(() => {
  console.log(`[SelectPanel] "${title}" after set, blockProp:`, getter(settingKeys));
}, 500);

Spotted by Graphite Agent



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

Debug console.log statement left in production code. This will spam the console every time an item is added to a multi-text setting and should be removed:

// Remove these lines:
setTimeout(() => {
  console.log(`[MultiTextPanel] "${title}" after add, blockProp:`, getter(settingKeys));
}, 500);

Spotted by Graphite Agent



Is this helpful? React 👍 or 👎 to let us know.

[sid597]

We get the settings on line 11, 12, and use that to pass the default value from here.

[mdroidian]

I haven't had a change to review this yet, but please go through and remove any testing code not meant to merge and resolve any unresolved review comments.

[sid597]

I put them explicitly because I thought we were going to test them extensively before merging and these logs will help us do that. Should I remove them and after review merge the pr?