ENG-1272 Migrate all small global settings components by sid597 · Pull Request #688 · DiscourseGraphs/discourse-graph

sid597 · 2026-01-12T14:08:25Z

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

linear · 2026-01-12T14:08:29Z

ENG-1272 Migrate all small global settings component

supabase · 2026-01-12T14:08:32Z

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 · 2026-01-12T14:08:54Z

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 1 new potential issue.

View 6 additional findings in Devin Review.

apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx

devin-ai-integration

Devin Review found 1 new potential issue.

View 9 additional findings in Devin Review.

apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx

sid597 · 2026-02-06T19:22:53Z

apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx

 }: BaseTextPanelProps) => {
-  const [value, setValue] = useState(() => getter(settingKeys) ?? defaultValue);
+  const [value, setValue] = useState(
+    () => defaultValue ?? getter(settingKeys) ?? "",


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.

I don't think this is a good pattern. That means we have to make sure that every time <BaseTextPanel> is called, it is using the default value. This is too error prone and requires manual checking of all callers.

Let's remove the getter() altogether. I'm not sure what purpose it is serving now anyway.

If you want to have some code in here so you don't have to remember to put it back, code it as if we were using a flag to switch between blocks/props. But have the flag null or false (so that it always uses blocks until we add the mechanism to switch the flag to use props.)

Oh I see, you are using defaultValue as initialValue. Let's change it to initialValue

Initial Value = the initial value we are passing to another component which is handling that value's state going forward (not meant to change)
Default Value = if there no value, use this value (not meant to change)
Value = the value we care about to update the UI, the value we keep in state. (this is meant to change)

so generally something like this:
[value, setvValue] = useState(initialValue ?? defaultValue)

But the "let's remove the getter() comment still stands"

apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx

sid597 · 2026-02-06T19:24:36Z

apps/roam/src/components/settings/ExportSettings.tsx

          title="remove special characters"
          description="Whether or not to remove the special characters in a file name"
+          settingKeys={["Export", "Remove Special Characters"]}
+          defaultValue={exportSettings.removeSpecialCharacters.value}


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 · 2026-02-07T16:41:59Z

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?

mdroidian · 2026-02-08T07:01:01Z

As we discussed: re: new system to stop testing/unwanted code merging into production

don't commit the code. Add it to the body of the PR,

even better, add it as suggested code in line in a GitHub comment, so that it can be co-located with where it should go, so the reviewer can see it as they review

sid597 · 2026-02-09T05:38:18Z

apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx

    const newValue = e.target.value;
    setValue(newValue);
    setter(settingKeys, newValue);
    syncToBlock?.(newValue);


For testing, add the following here

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

same goes for all the other base panels, we should add this console.log to all the base panels for testing.

sid597 · 2026-02-09T05:50:03Z

apps/roam/src/index.ts

  }

+  // For testing purposes
+  // await initSchema();


uncomment this for testing

sid597 · 2026-02-09T05:50:12Z

apps/roam/src/index.ts

  STREAMLINE_STYLING_KEY,
  DISALLOW_DIAGNOSTICS,
 } from "./data/userSettings";
+// import { initSchema } from "./components/settings/utils/init";


uncomment this for testing

mdroidian

So a few issues in this one:

let's remove the getter() if we aren't using it yet, or put it behind a null/false flag that is intended to be used in the future
changing defaultValue to initialValue (this can be deferred, if it is, please link to the created task in which it will be completed)
clarify / simplify the logic for suggestive mode
please format all files with prettier before tagging for review
remove any code not intended to be merged

mdroidian · 2026-02-11T04:06:19Z

apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx

 }: BaseTextPanelProps) => {
-  const [value, setValue] = useState(() => getter(settingKeys) ?? defaultValue);
+  const [value, setValue] = useState(
+    () => defaultValue ?? getter(settingKeys) ?? "",


I don't think this is a good pattern. That means we have to make sure that every time <BaseTextPanel> is called, it is using the default value. This is too error prone and requires manual checking of all callers.

Let's remove the getter() altogether. I'm not sure what purpose it is serving now anyway.

If you want to have some code in here so you don't have to remember to put it back, code it as if we were using a flag to switch between blocks/props. But have the flag null or false (so that it always uses blocks until we add the mechanism to switch the flag to use props.)

mdroidian · 2026-02-11T04:07:36Z

apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx

 }: BaseFlagPanelProps) => {
-  const [value, setValue] = useState(() => getter(settingKeys) ?? defaultValue);
+  const [value, setValue] = useState(
+    () => defaultValue ?? getter(settingKeys) ?? false,


same as above comment

mdroidian · 2026-02-11T04:11:05Z

apps/roam/src/components/settings/SuggestiveModeSettings.tsx

                  }
+                  settingKeys={["Suggestive Mode", "Include Parent And Child Blocks"]}
+                  defaultValue={settings.suggestiveMode.includeParentAndChildren.value}
+                  overrideValue={includePageRelations ? true : undefined}


What is this called overrideValue and not just value? What value is it overriding?

Generally it's
defaultValue and value where we use defaultValue if there is no value defined.

mdroidian · 2026-02-11T04:16:09Z

apps/roam/src/components/settings/SuggestiveModeSettings.tsx

                      : "Include relations from parent and child blocks"
                  }
+                  settingKeys={["Suggestive Mode", "Include Parent And Child Blocks"]}
+                  defaultValue={settings.suggestiveMode.includeParentAndChildren.value}


From this panel alone this is confusing, can you walk me through the logic here?

Is it:
The value is from includeParentandChildren, but if includePageRelations is true then it is overriden? Overriden how?

Let's try to just use defaultValue/value and do the logic explicitly above so it's easy for anyone to follow how we derive defaultValue and value

mdroidian · 2026-02-11T04:16:59Z

apps/roam/src/components/settings/SuggestiveModeSettings.tsx

            <div className="flex flex-col gap-4 p-1">
              <div className="sync-config-settings">
-                <FlagPanel
+                <GlobalFlagPanel


Why is there no value prop on this component?

Ah, it looks like you are using defaultValue like an initialValue. This is kind of confusing when defaultValue usually means

this is an uncontrolled component
or

if there is no value, use defaultValue

mdroidian · 2026-02-11T04:21:55Z

apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx

 }: BaseTextPanelProps) => {
-  const [value, setValue] = useState(() => getter(settingKeys) ?? defaultValue);
+  const [value, setValue] = useState(
+    () => defaultValue ?? getter(settingKeys) ?? "",


Oh I see, you are using defaultValue as initialValue. Let's change it to initialValue

Initial Value = the initial value we are passing to another component which is handling that value's state going forward (not meant to change)
Default Value = if there no value, use this value (not meant to change)
Value = the value we care about to update the UI, the value we keep in state. (this is meant to change)

so generally something like this:
[value, setvValue] = useState(initialValue ?? defaultValue)

But the "let's remove the getter() comment still stands"

apps/roam/src/index.ts

This was referenced Jan 12, 2026

ENG-1105: zod schema for all the settings #664

Merged

ENG-1189: Prod: Init block prop based schema #666

Merged

ENG-1187: Prod: Implement get and set for block prop based settings #669

Merged

ENG-1188: prod: pull watchers for prop based settings #682

Merged

sid597 mentioned this pull request Jan 12, 2026

ENG-1274: Migrate feature flags #684

Closed

sid597 force-pushed the eng-1274-add-feature-flags-in-settings branch from 29817b9 to 9cd0a2c Compare January 12, 2026 15:26

sid597 force-pushed the eng-1272-migrate-all-small-global-settings-component branch from 8f1c13c to 2d2bb77 Compare January 12, 2026 15:26

sid597 mentioned this pull request Jan 12, 2026

ENG-1190 Replace existing roam-js components with blockProp based ones #689

Merged

sid597 marked this pull request as ready for review January 12, 2026 15:27

sid597 force-pushed the eng-1272-migrate-all-small-global-settings-component branch from 2d2bb77 to f9de2fd Compare January 12, 2026 15:38

sid597 force-pushed the eng-1272-migrate-all-small-global-settings-component branch from d1b8faf to 9015611 Compare January 17, 2026 18:16

sid597 force-pushed the eng-1274-add-feature-flags-in-settings branch 2 times, most recently from 23cedec to 496a3ba Compare January 18, 2026 05:25

sid597 force-pushed the eng-1272-migrate-all-small-global-settings-component branch 2 times, most recently from 16b26f7 to dd2272b Compare January 19, 2026 04:51

sid597 force-pushed the eng-1274-add-feature-flags-in-settings branch 2 times, most recently from 0238148 to 8dbd24c Compare January 19, 2026 05:01

sid597 force-pushed the eng-1272-migrate-all-small-global-settings-component branch from dd2272b to 92ea54b Compare January 19, 2026 05:01

sid597 force-pushed the eng-1274-add-feature-flags-in-settings branch from 8dbd24c to 4c71862 Compare January 19, 2026 06:03

sid597 force-pushed the eng-1272-migrate-all-small-global-settings-component branch from 92ea54b to 5a165ac Compare January 19, 2026 06:03

sid597 force-pushed the eng-1274-add-feature-flags-in-settings branch from 4c71862 to 77d1c95 Compare January 19, 2026 06:34

sid597 force-pushed the eng-1272-migrate-all-small-global-settings-component branch from 94e3904 to fbede09 Compare February 6, 2026 18:59

sid597 force-pushed the graphite-base/688 branch from cf56730 to 2ac769a Compare February 6, 2026 18:59

sid597 changed the base branch from graphite-base/688 to main February 6, 2026 18:59

devin-ai-integration bot reviewed Feb 6, 2026

View reviewed changes

apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx Outdated Show resolved Hide resolved

sid597 requested a review from mdroidian February 6, 2026 19:19

devin-ai-integration bot reviewed Feb 6, 2026

View reviewed changes

apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx Show resolved Hide resolved

sid597 commented Feb 6, 2026

View reviewed changes

graphite-app bot reviewed Feb 6, 2026

View reviewed changes

sid597 commented Feb 6, 2026

View reviewed changes

mdroidian requested changes Feb 7, 2026

View reviewed changes

sid597 commented Feb 9, 2026

View reviewed changes

sid597 requested a review from mdroidian February 9, 2026 05:47

sid597 commented Feb 9, 2026

View reviewed changes

apps/roam/src/index.ts

}

// For testing purposes

// await initSchema();

Copy link

Collaborator Author

sid597 Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uncomment this for testing

sid597 commented Feb 9, 2026

View reviewed changes

sid597 force-pushed the eng-1272-migrate-all-small-global-settings-component branch from 53f1015 to 0ef04ee Compare February 9, 2026 07:44

sid597 added 8 commits February 9, 2026 14:00

ENG-1272 Migrate all small global settings components

d06d8e7

suggestive mode flags migrate

224a20d

prettier

dcd3b6c

address review

78b5481

add comments for testing

d9aef91

add comment for testing

6b5704f

remove comments

30e4424

remove init stub

15f3a6b

sid597 force-pushed the eng-1272-migrate-all-small-global-settings-component branch from 0ef04ee to 15f3a6b Compare February 9, 2026 09:46

mdroidian requested changes Feb 11, 2026

View reviewed changes

This was referenced Feb 11, 2026

ENG-1280: Port Discourse node: Query builder and editor component for block props #764

Open

ENG-1291: Port discourse node specification #765

Open

ENG-1290: Port discourse node config panel #769

Open

Conversation

sid597 commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linear bot commented Jan 12, 2026

Uh oh!

supabase bot commented Jan 12, 2026

Uh oh!

sid597 commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

so generally something like this: [value, setvValue] = useState(initialValue ?? defaultValue)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdroidian left a comment

Choose a reason for hiding this comment

Uh oh!

sid597 commented Feb 7, 2026

Uh oh!

mdroidian commented Feb 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sid597 Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdroidian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

so generally something like this: [value, setvValue] = useState(initialValue ?? defaultValue)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sid597 commented Jan 12, 2026 •

edited

Loading

sid597 commented Jan 12, 2026 •

edited

Loading

so generally something like this:
`[value, setvValue] = useState(initialValue ?? defaultValue)`

mdroidian commented Feb 8, 2026 •

edited

Loading

sid597 Feb 9, 2026 •

edited

Loading

so generally something like this:
`[value, setvValue] = useState(initialValue ?? defaultValue)`