Skip to content

ENG-1272 Migrate all small global settings components #688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sid597 wants to merge 8 commits into
base: main
Choose a base branch
from
+83 −67
Open

ENG-1272 Migrate all small global settings components #688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d06d8e7
ENG-1272 Migrate all small global settings components
sid597 Jan 12, 2026
224a20d
suggestive mode flags migrate
sid597 Jan 12, 2026
dcd3b6c
prettier
sid597 Jan 12, 2026
78b5481
address review
sid597 Feb 6, 2026
d9aef91
add comments for testing
sid597 Feb 6, 2026
6b5704f
add comment for testing
sid597 Feb 6, 2026
30e4424
remove comments
sid597 Feb 9, 2026
15f3a6b
remove init stub
sid597 Feb 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 28 additions & 21 deletions apps/roam/src/components/settings/ExportSettings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import React from "react";
import { getFormattedConfigTree } from "~/utils/discourseConfigRef";
import FlagPanel from "roamjs-components/components/ConfigPanels/FlagPanel";
import NumberPanel from "roamjs-components/components/ConfigPanels/NumberPanel";
import MultiTextPanel from "roamjs-components/components/ConfigPanels/MultiTextPanel";
import SelectPanel from "roamjs-components/components/ConfigPanels/SelectPanel";
import {
GlobalFlagPanel,
GlobalNumberPanel,
GlobalMultiTextPanel,
GlobalSelectPanel,
} from "./components/BlockPropSettingPanels";

const DiscourseGraphExport = ({}: {}) => {
const settings = getFormattedConfigTree();
Expand All @@ -12,69 +14,74 @@ const DiscourseGraphExport = ({}: {}) => {
return (
<div className="flex flex-col gap-4 p-1">
<div>
<FlagPanel
<GlobalFlagPanel
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}
Copy link
Collaborator Author

@sid597 sid597 Feb 6, 2026

Choose a reason for hiding this comment

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

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

order={1}
uid={exportSettings.removeSpecialCharacters.uid}
parentUid={parentUid}
value={exportSettings.removeSpecialCharacters.value || false}
/>

<FlagPanel
<GlobalFlagPanel
title="resolve block references"
description="Replaces block references in the markdown content with the block's content"
settingKeys={["Export", "Resolve Block References"]}
defaultValue={exportSettings.optsRefs.value}
order={3}
uid={exportSettings.optsRefs.uid}
parentUid={parentUid}
value={exportSettings.optsRefs.value || false}
/>
<FlagPanel
<GlobalFlagPanel
title="resolve block embeds"
description="Replaces block embeds in the markdown content with the block's content tree"
settingKeys={["Export", "Resolve Block Embeds"]}
defaultValue={exportSettings.optsEmbeds.value}
order={4}
uid={exportSettings.optsEmbeds.uid}
parentUid={parentUid}
value={exportSettings.optsEmbeds.value || false}
/>

<FlagPanel
<GlobalFlagPanel
title="append referenced node"
description="If a referenced node is defined in a node's format, it will be appended to the discourse context"
settingKeys={["Export", "Append Referenced Node"]}
defaultValue={exportSettings.appendRefNodeContext.value}
order={6}
uid={exportSettings.appendRefNodeContext.uid}
parentUid={parentUid}
value={exportSettings.appendRefNodeContext.value || false}
/>
</div>
<div className="link-type-select-wrapper">
<SelectPanel
<GlobalSelectPanel
title="link type"
description="How to format links that appear in your export."
settingKeys={["Export", "Link Type"]}
defaultValue={exportSettings.linkType.value || "alias"}
order={5}
options={{
items: ["alias", "wikilinks", "roam url"],
}}
options={["alias", "wikilinks", "roam url"]}
uid={exportSettings.linkType.uid}
parentUid={parentUid}
value={exportSettings.linkType.value || "alias"}
/>
</div>
<NumberPanel
<GlobalNumberPanel
title="max filename length"
description="Set the maximum name length for markdown file exports"
settingKeys={["Export", "Max Filename Length"]}
defaultValue={exportSettings.maxFilenameLength.value || 64}
order={0}
uid={exportSettings.maxFilenameLength.uid}
parentUid={parentUid}
value={exportSettings.maxFilenameLength.value || 64}
/>
<MultiTextPanel
<GlobalMultiTextPanel
title="frontmatter"
description="Specify all the lines that should go to the Frontmatter of the markdown file"
settingKeys={["Export", "Frontmatter"]}
defaultValue={exportSettings.frontmatter.values}
order={2}
uid={exportSettings.frontmatter.uid}
parentUid={parentUid}
value={exportSettings.frontmatter.values || []}
/>
</div>
);
Expand Down
29 changes: 13 additions & 16 deletions apps/roam/src/components/settings/GeneralSettings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import React, { useMemo, useState } from "react";
import TextPanel from "roamjs-components/components/ConfigPanels/TextPanel";
import FlagPanel from "roamjs-components/components/ConfigPanels/FlagPanel";
import { getFormattedConfigTree } from "~/utils/discourseConfigRef";
import refreshConfigTree from "~/utils/refreshConfigTree";
import { DEFAULT_CANVAS_PAGE_FORMAT } from "~/index";
import { Alert, Intent } from "@blueprintjs/core";
import { GlobalTextPanel, FeatureFlagPanel } from "./components/BlockPropSettingPanels";

const DiscourseGraphHome = () => {
const settings = useMemo(() => {
Expand All @@ -13,39 +12,37 @@ const DiscourseGraphHome = () => {
}, []);

const [isAlertOpen, setIsAlertOpen] = useState(false);

return (
<div className="flex flex-col gap-4 p-1">
<TextPanel
<GlobalTextPanel
title="trigger"
description="The trigger to create the node menu."
settingKeys={["Trigger"]}
defaultValue={settings.trigger.value || "\\"}
order={0}
uid={settings.trigger.uid}
parentUid={settings.settingsUid}
value={settings.trigger.value}
/>
<TextPanel
<GlobalTextPanel
title="Canvas Page Format"
description="The page format for canvas pages"
settingKeys={["Canvas Page Format"]}
defaultValue={settings.canvasPageFormat.value || DEFAULT_CANVAS_PAGE_FORMAT}
order={1}
uid={settings.canvasPageFormat.uid}
parentUid={settings.settingsUid}
value={settings.canvasPageFormat.value}
defaultValue={DEFAULT_CANVAS_PAGE_FORMAT}
/>
<FlagPanel
<FeatureFlagPanel
title="(BETA) Left Sidebar"
description="Whether or not to enable the left sidebar."
featureKey="Enable Left Sidebar"
order={2}
uid={settings.leftSidebarEnabled.uid}
parentUid={settings.settingsUid}
value={settings.leftSidebarEnabled.value || false}
options={{
onChange: (checked: boolean) => {
if (checked) {
setIsAlertOpen(true);
}
},
onAfterChange={(checked: boolean) => {
if (checked) {
setIsAlertOpen(true);
}
}}
/>
<Alert
Expand Down
25 changes: 10 additions & 15 deletions apps/roam/src/components/settings/SuggestiveModeSettings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@
import React, { useEffect, useState } from "react";
import { Button, Intent, Tabs, Tab, TabId } from "@blueprintjs/core";
import { getFormattedConfigTree } from "~/utils/discourseConfigRef";
import FlagPanel from "roamjs-components/components/ConfigPanels/FlagPanel";
import PageGroupsPanel from "./PageGroupPanel";
import createBlock from "roamjs-components/writes/createBlock";
import getPageUidByPageTitle from "roamjs-components/queries/getPageUidByPageTitle";
import { DISCOURSE_CONFIG_PAGE_TITLE } from "~/utils/renderNodeConfigPage";
import { createOrUpdateDiscourseEmbedding } from "~/utils/syncDgNodesToSupabase";
import { render as renderToast } from "roamjs-components/components/Toast";
import { GlobalFlagPanel } from "./components/BlockPropSettingPanels";

const SuggestiveModeSettings = () => {
const settings = getFormattedConfigTree();
Expand All @@ -34,7 +34,7 @@ const SuggestiveModeSettings = () => {
}, [suggestiveModeUid]);

const effectiveSuggestiveModeUid =
suggestiveModeUid || settings.suggestiveMode.parentUid;
suggestiveModeUid || settings.suggestiveMode.parentUid;

const [selectedTabId, setSelectedTabId] = useState<TabId>("page-groups");

Expand Down Expand Up @@ -64,35 +64,30 @@ const SuggestiveModeSettings = () => {
panel={
<div className="flex flex-col gap-4 p-1">
<div className="sync-config-settings">
<FlagPanel
<GlobalFlagPanel
Copy link
Contributor

@mdroidian mdroidian Feb 11, 2026

Choose a reason for hiding this comment

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

Why is there no value prop on this component?

Copy link
Contributor

@mdroidian mdroidian Feb 11, 2026

Choose a reason for hiding this comment

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

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

title="Include Current Page Relations"
description="Include relations from pages referenced on the current page"
settingKeys={["Suggestive Mode", "Include Current Page Relations"]}
defaultValue={settings.suggestiveMode.includePageRelations.value}
order={0}
uid={settings.suggestiveMode.includePageRelations.uid}
parentUid={effectiveSuggestiveModeUid}
value={includePageRelations}
options={{
onChange: (checked: boolean) => {
setIncludePageRelations(checked);
},
}}
onChange={setIncludePageRelations}
/>

<FlagPanel
<GlobalFlagPanel
title="Include Parent And Child Blocks"
description={
includePageRelations
? "Include relations from parent and child blocks (automatically enabled when including page relations)"
: "Include relations from parent and child blocks"
}
settingKeys={["Suggestive Mode", "Include Parent And Child Blocks"]}
defaultValue={settings.suggestiveMode.includeParentAndChildren.value}
Copy link
Contributor

@mdroidian mdroidian Feb 11, 2026

Choose a reason for hiding this comment

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

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

overrideValue={includePageRelations ? true : undefined}
Copy link
Contributor

@mdroidian mdroidian Feb 11, 2026

Choose a reason for hiding this comment

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

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.

order={1}
uid={settings.suggestiveMode.includeParentAndChildren.uid}
parentUid={effectiveSuggestiveModeUid}
value={
includePageRelations
? true
: settings.suggestiveMode.includeParentAndChildren.value
}
disabled={includePageRelations}
/>
</div>
Expand Down
41 changes: 28 additions & 13 deletions apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type BaseFlagPanelProps = {
getter: FlagGetter;
setter: FlagSetter;
defaultValue?: boolean;
overrideValue?: boolean;
disabled?: boolean;
onBeforeChange?: (checked: boolean) => Promise<boolean>;
onChange?: (checked: boolean) => void;
Expand Down Expand Up @@ -98,20 +99,22 @@ const BaseTextPanel = ({
settingKeys,
getter,
setter,
defaultValue = "",
defaultValue,
placeholder,
parentUid,
uid,
order,
}: BaseTextPanelProps) => {
const [value, setValue] = useState(() => getter(settingKeys) ?? defaultValue);
const [value, setValue] = useState(
() => defaultValue ?? getter(settingKeys) ?? "",
Copy link
Collaborator Author

@sid597 sid597 Feb 6, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@mdroidian mdroidian Feb 11, 2026

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@mdroidian mdroidian Feb 11, 2026

Choose a reason for hiding this comment

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

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"

);
const hasBlockSync = parentUid !== undefined && order !== undefined;
const { onChange: rawSyncToBlock } = useSingleChildValue({
title,
parentUid: parentUid ?? "",
order: order ?? 0,
uid,
defaultValue,
defaultValue: defaultValue ?? "",
transform: (s: string) => s,
toStr: (s: string) => s,
});
Expand Down Expand Up @@ -143,15 +146,18 @@ const BaseFlagPanel = ({
settingKeys,
getter,
setter,
defaultValue = false,
defaultValue,
overrideValue,
disabled = false,
onBeforeChange,
onChange,
parentUid,
uid: initialBlockUid,
order,
}: BaseFlagPanelProps) => {
const [value, setValue] = useState(() => getter(settingKeys) ?? defaultValue);
const [value, setValue] = useState(
() => defaultValue ?? getter(settingKeys) ?? false,
Copy link
Contributor

@mdroidian mdroidian Feb 11, 2026

Choose a reason for hiding this comment

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

same as above comment

);
const blockUidRef = useRef(initialBlockUid);

const syncFlagToBlock = useCallback(
Expand Down Expand Up @@ -193,7 +199,7 @@ const BaseFlagPanel = ({

return (
<Checkbox
checked={value}
checked={overrideValue ?? value}
onChange={(e) => void handleChange(e)}
disabled={disabled}
labelElement={
Expand All @@ -212,21 +218,23 @@ const BaseNumberPanel = ({
settingKeys,
getter,
setter,
defaultValue = 0,
defaultValue,
min,
max,
parentUid,
uid,
order,
}: BaseNumberPanelProps) => {
const [value, setValue] = useState(() => getter(settingKeys) ?? defaultValue);
const [value, setValue] = useState(
() => defaultValue ?? getter(settingKeys) ?? 0,
);
const hasBlockSync = parentUid !== undefined && order !== undefined;
const { onChange: rawSyncToBlock } = useSingleChildValue({
title,
parentUid: parentUid ?? "",
order: order ?? 0,
uid,
defaultValue,
defaultValue: defaultValue ?? 0,
transform: (s: string) => parseInt(s, 10),
toStr: (v: number) => `${v}`,
});
Expand Down Expand Up @@ -267,7 +275,7 @@ const BaseSelectPanel = ({
order,
}: BaseSelectPanelProps) => {
const [value, setValue] = useState(
() => getter(settingKeys) ?? defaultValue ?? options[0],
() => defaultValue ?? getter(settingKeys) ?? options[0],
);
const hasBlockSync = parentUid !== undefined && order !== undefined;
const { onChange: rawSyncToBlock } = useSingleChildValue({
Expand Down Expand Up @@ -308,13 +316,13 @@ const BaseMultiTextPanel = ({
settingKeys,
getter,
setter,
defaultValue = [],
defaultValue,
parentUid,
uid: initialBlockUid,
order,
}: BaseMultiTextPanelProps) => {
const [values, setValues] = useState<string[]>(
() => getter(settingKeys) ?? defaultValue,
() => defaultValue ?? getter(settingKeys) ?? [],
);
const [inputValue, setInputValue] = useState("");
const hasBlockSync = parentUid !== undefined && order !== undefined;
Expand Down Expand Up @@ -472,19 +480,23 @@ export const FeatureFlagPanel = ({
featureKey,
onBeforeEnable,
onAfterChange,
parentUid,
uid,
order,
}: {
title: string;
description: string;
featureKey: keyof FeatureFlags;
onBeforeEnable?: () => Promise<boolean>;
onAfterChange?: (checked: boolean) => void;
}) => {
} & RoamBlockSyncProps) => {
const handleBeforeChange:
| ((checked: boolean) => Promise<boolean>)
| undefined = onBeforeEnable
? async (checked) => {
if (checked) {
return onBeforeEnable();

}
return true;
}
Expand All @@ -499,6 +511,9 @@ export const FeatureFlagPanel = ({
setter={featureFlagSetter}
onBeforeChange={handleBeforeChange}
onChange={onAfterChange}
parentUid={parentUid}
uid={uid}
order={order}
/>
);
};
Expand Down
Loading