Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 8 additions & 10 deletions apps/roam/src/components/DiscourseFloatingMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
import { FeedbackWidget } from "./BirdEatsBugs";
import { render as renderSettings } from "~/components/settings/Settings";
import posthog from "posthog-js";
import { getPersonalSetting } from "./settings/utils/accessors";
import { type SettingsSnapshot } from "./settings/utils/accessors";
import { PERSONAL_KEYS } from "./settings/utils/settingKeys";

type DiscourseFloatingMenuProps = {
Expand Down Expand Up @@ -118,26 +118,23 @@ export const showDiscourseFloatingMenu = () => {

export const installDiscourseFloatingMenu = (
onLoadArgs: OnloadArgs,
props: DiscourseFloatingMenuProps = {
position: "bottom-right",
theme: "bp3-light",
buttonTheme: "bp3-light",
},
snapshot: SettingsSnapshot,
) => {
let floatingMenuAnchor = document.getElementById(ANCHOR_ID);
if (!floatingMenuAnchor) {
floatingMenuAnchor = document.createElement("div");
floatingMenuAnchor.id = ANCHOR_ID;
document.getElementById("app")?.appendChild(floatingMenuAnchor);
}
if (getPersonalSetting<boolean>([PERSONAL_KEYS.hideFeedbackButton])) {
if (snapshot.personalSettings[PERSONAL_KEYS.hideFeedbackButton]) {
floatingMenuAnchor.classList.add("hidden");
}
// eslint-disable-next-line react/no-deprecated
ReactDOM.render(
<DiscourseFloatingMenu
position={props.position}
theme={props.theme}
buttonTheme={props.buttonTheme}
position="bottom-right"
theme="bp3-light"
buttonTheme="bp3-light"
onloadArgs={onLoadArgs}
/>,
floatingMenuAnchor,
Expand All @@ -148,6 +145,7 @@ export const removeDiscourseFloatingMenu = () => {
const anchor = document.getElementById(ANCHOR_ID);
if (anchor) {
try {
// eslint-disable-next-line react/no-deprecated
ReactDOM.unmountComponentAtNode(anchor);
} catch (e) {
// no-op: unmount best-effort
Expand Down
53 changes: 37 additions & 16 deletions apps/roam/src/components/LeftSidebarView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
getPersonalSettings,
setGlobalSetting,
setPersonalSetting,
type SettingsSnapshot,
} from "~/components/settings/utils/accessors";
import {
PERSONAL_KEYS,
Expand Down Expand Up @@ -336,14 +337,16 @@ const GlobalSection = ({ config }: { config: LeftSidebarConfig["global"] }) => {

// TODO(ENG-1471): Remove old-system merge when migration complete — just use accessor values directly.
// See mergeGlobalSectionWithAccessor/mergePersonalSectionsWithAccessor for why the merge exists.
const buildConfig = (): LeftSidebarConfig => {
const buildConfig = (snapshot?: SettingsSnapshot): LeftSidebarConfig => {
// Read VALUES from accessor (handles flag routing + mismatch detection)
const globalValues = getGlobalSetting<LeftSidebarGlobalSettings>([
GLOBAL_KEYS.leftSidebar,
]);
const personalValues = getPersonalSetting<
ReturnType<typeof getPersonalSettings>[typeof PERSONAL_KEYS.leftSidebar]
>([PERSONAL_KEYS.leftSidebar]);
const globalValues = snapshot
? snapshot.globalSettings[GLOBAL_KEYS.leftSidebar]
: getGlobalSetting<LeftSidebarGlobalSettings>([GLOBAL_KEYS.leftSidebar]);
const personalValues = snapshot
? snapshot.personalSettings[PERSONAL_KEYS.leftSidebar]
: getPersonalSetting<
ReturnType<typeof getPersonalSettings>[typeof PERSONAL_KEYS.leftSidebar]
>([PERSONAL_KEYS.leftSidebar]);

// Read UIDs from old system (needed for fold CRUD during dual-write)
const oldConfig = getCurrentLeftSidebarConfig();
Expand All @@ -364,8 +367,8 @@ const buildConfig = (): LeftSidebarConfig => {
};
};

export const useConfig = () => {
const [config, setConfig] = useState(() => buildConfig());
export const useConfig = (initialSnapshot?: SettingsSnapshot) => {
const [config, setConfig] = useState(() => buildConfig(initialSnapshot));
useEffect(() => {
const handleUpdate = () => {
setConfig(buildConfig());
Expand Down Expand Up @@ -504,8 +507,14 @@ const FavoritesPopover = ({ onloadArgs }: { onloadArgs: OnloadArgs }) => {
);
};

const LeftSidebarView = ({ onloadArgs }: { onloadArgs: OnloadArgs }) => {
const { config } = useConfig();
const LeftSidebarView = ({
onloadArgs,
initialSnapshot,
}: {
onloadArgs: OnloadArgs;
initialSnapshot?: SettingsSnapshot;
}) => {
const { config } = useConfig(initialSnapshot);

return (
<>
Expand Down Expand Up @@ -610,10 +619,15 @@ const migrateFavorites = async () => {
refreshConfigTree();
};

export const mountLeftSidebar = async (
wrapper: HTMLElement,
onloadArgs: OnloadArgs,
): Promise<void> => {
export const mountLeftSidebar = async ({
wrapper,
onloadArgs,
initialSnapshot,
}: {
wrapper: HTMLElement;
onloadArgs: OnloadArgs;
initialSnapshot?: SettingsSnapshot;
}): Promise<void> => {
if (!wrapper) return;

const id = "dg-left-sidebar-root";
Expand All @@ -630,7 +644,14 @@ export const mountLeftSidebar = async (
} else {
root.className = "starred-pages";
}
ReactDOM.render(<LeftSidebarView onloadArgs={onloadArgs} />, root);
// eslint-disable-next-line react/no-deprecated
ReactDOM.render(
<LeftSidebarView
onloadArgs={onloadArgs}
initialSnapshot={initialSnapshot}
/>,
root,
);
};

export default LeftSidebarView;
13 changes: 8 additions & 5 deletions apps/roam/src/components/settings/QueryPagesPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type { OnloadArgs } from "roamjs-components/types";
import {
getPersonalSetting,
setPersonalSetting,
type SettingsSnapshot,
} from "~/components/settings/utils/accessors";
import {
PERSONAL_KEYS,
Expand All @@ -13,11 +14,13 @@ import {

// Legacy extensionAPI stored query-pages as string | string[] | Record<string, string>.
// Coerce to string[] for backward compatibility with old stored formats.
export const getQueryPages = (): string[] => {
const value = getPersonalSetting<string[] | string | Record<string, string>>([
PERSONAL_KEYS.query,
QUERY_KEYS.queryPages,
]);
export const getQueryPages = (snapshot?: SettingsSnapshot): string[] => {
const value: string[] | string | Record<string, string> | undefined = snapshot
? snapshot.personalSettings[PERSONAL_KEYS.query][QUERY_KEYS.queryPages]
: getPersonalSetting<string[] | string | Record<string, string>>([
PERSONAL_KEYS.query,
QUERY_KEYS.queryPages,
]);
return typeof value === "string"
? [value]
: Array.isArray(value)
Expand Down
63 changes: 61 additions & 2 deletions apps/roam/src/components/settings/utils/accessors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -863,8 +863,10 @@ export const setGlobalSetting = (keys: string[], value: json): void => {
});
};

export const getAllRelations = (): DiscourseRelation[] => {
const settings = getGlobalSettings();
export const getAllRelations = (
snapshot?: SettingsSnapshot,
): DiscourseRelation[] => {
const settings = snapshot ? snapshot.globalSettings : getGlobalSettings();

return Object.entries(settings.Relations).flatMap(([id, relation]) =>
relation.ifConditions.map((ifCondition) => ({
Expand Down Expand Up @@ -909,6 +911,63 @@ export const getPersonalSetting = <T = unknown>(
return blockPropsValue as T | undefined;
};

export type SettingsSnapshot = {
featureFlags: FeatureFlags;
globalSettings: GlobalSettings;
personalSettings: PersonalSettings;
};

export const bulkReadSettings = (): SettingsSnapshot => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe getPersonalSetting was checking if isNewSettingsStoreEnabled, but it doesn't look like bulkReadSettings is doing this anymore.

Specifically example would be: const disallowDiagnostics = settingsSnapshot.personalSettings[PERSONAL_KEYS.disableProductDiagnostics]; If the new store settings aren't enabled, would the migration have happened yet?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

added the check, I missed it.

const pageResult = window.roamAlphaAPI.pull(
"[{:block/children [:block/string :block/props]}]",
[":node/title", DG_BLOCK_PROP_SETTINGS_PAGE_TITLE],
) as Record<string, json> | null;

const children = (pageResult?.[":block/children"] ?? []) as Record<
string,
json
>[];
const personalKey = getPersonalSettingsKey();
let featureFlagsProps: json = {};
let globalProps: json = {};
let personalProps: json = {};

for (const child of children) {
const text = child[":block/string"];
if (typeof text !== "string") continue;
const rawBlockProps = child[":block/props"];
const blockProps =
rawBlockProps && typeof rawBlockProps === "object"
? normalizeProps(rawBlockProps)
: {};
if (text === TOP_LEVEL_BLOCK_PROP_KEYS.featureFlags) {
featureFlagsProps = blockProps;
} else if (text === TOP_LEVEL_BLOCK_PROP_KEYS.global) {
globalProps = blockProps;
} else if (text === personalKey) {
personalProps = blockProps;
}
}

const featureFlags = FeatureFlagsSchema.parse(featureFlagsProps || {});

if (!featureFlags["Use new settings store"]) {
return {
featureFlags,
globalSettings: GlobalSettingsSchema.parse(readAllLegacyGlobalSettings()),
personalSettings: PersonalSettingsSchema.parse(
readAllLegacyPersonalSettings(),
),
};
Comment on lines +954 to +961
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 bulkReadSettings applies strict zod parse to legacy data that old code paths returned raw

When the new settings store is disabled (the default), bulkReadSettings() calls PersonalSettingsSchema.parse(readAllLegacyPersonalSettings()) which applies strict zod schema validation to legacy data. The old individual-read code path (getPersonalSetting) explicitly bypassed schema validation for legacy data, returning raw values (getLegacyPersonalSetting(keys) as T). The codebase documents that legacy query pages can be stored as string | string[] | Record<string, string> (apps/roam/src/components/settings/QueryPagesPanel.tsx:14), but QuerySettingsSchema defines "Query pages": z.array(z.string()) which would reject a Record<string, string> or bare string, causing .parse() to throw. Since bulkReadSettings() is called at the very start of extension loading (apps/roam/src/index.ts:51), before migration runs (apps/roam/src/index.ts:192), this would crash the extension on load for affected users.

Affected code path in bulkReadSettings

The legacy fallback at accessors.ts:954-961 uses .parse() (throws on failure) rather than .safeParse() with graceful fallback:

if (!featureFlags["Use new settings store"]) {
  return {
    featureFlags,
    globalSettings: GlobalSettingsSchema.parse(readAllLegacyGlobalSettings()),
    personalSettings: PersonalSettingsSchema.parse(
      readAllLegacyPersonalSettings(),
    ),
  };
}

The migration code (migrateLegacyToBlockProps.ts:73) correctly uses .safeParse() for this exact reason.

Prompt for agents
In bulkReadSettings() at accessors.ts lines 954-961, the legacy fallback path uses PersonalSettingsSchema.parse() and GlobalSettingsSchema.parse() on raw legacy data. This can throw for users with non-standard legacy formats (e.g., query pages stored as Record<string,string> or bare string, which the codebase explicitly handles in getQueryPages). The individual getPersonalSetting/getGlobalSetting functions bypass schema validation for legacy data, but bulkReadSettings does not.

The fix should use .safeParse() with a graceful fallback instead of .parse(). If parsing fails, fall back to schema defaults (e.g., GlobalSettingsSchema.parse({}) and PersonalSettingsSchema.parse({})) or log the issue without crashing. The migration code in migrateLegacyToBlockProps.ts already demonstrates the safeParse pattern. Alternatively, apply the same coercion logic that getQueryPages uses before parsing.
Open in Devin Review

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

}

return {
featureFlags,
globalSettings: GlobalSettingsSchema.parse(globalProps || {}),
personalSettings: PersonalSettingsSchema.parse(personalProps || {}),
};
};

export const setPersonalSetting = (keys: string[], value: json): void => {
if (keys.length === 0) {
internalError({
Expand Down
8 changes: 3 additions & 5 deletions apps/roam/src/components/settings/utils/init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,9 @@ export type InitSchemaResult = {
nodePageUids: Record<string, string>;
};

// On-demand dual-read comparison. Not called automatically on init —
// invoke from the console via window.dgDualReadLog() to inspect the legacy
// settings tree vs. the block-prop store.
const logDualReadComparison = (): void => {
if (!isNewSettingsStoreEnabled()) return;

Expand Down Expand Up @@ -415,11 +418,6 @@ export const initSchema = async (): Promise<InitSchemaResult> => {
await migrateGraphLevel(blockUids);
const nodePageUids = await initDiscourseNodePages();
await migratePersonalSettings(blockUids);
try {
logDualReadComparison();
} catch (e) {
console.warn("[DG Dual-Read] Comparison failed:", e);
}
(window as unknown as Record<string, unknown>).dgDualReadLog =
logDualReadComparison;
return { blockUids, nodePageUids };
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import getBlockProps from "~/utils/getBlockProps";
import type { json } from "~/utils/getBlockProps";
import setBlockProps from "~/utils/setBlockProps";
import getBlockUidByTextOnPage from "roamjs-components/queries/getBlockUidByTextOnPage";
import getPageUidByPageTitle from "roamjs-components/queries/getPageUidByPageTitle";
import { createBlock } from "roamjs-components/writes";
import { getSetting, setSetting } from "~/utils/extensionSettings";
Expand Down Expand Up @@ -29,11 +28,8 @@ const GRAPH_MIGRATION_MARKER = "Block props migrated";
const PERSONAL_MIGRATION_MARKER = "dg-personal-settings-migrated";
const MAX_ERROR_CONTEXT_LENGTH = 5000;

const hasGraphMigrationMarker = (): boolean =>
!!getBlockUidByTextOnPage({
text: GRAPH_MIGRATION_MARKER,
title: DG_BLOCK_PROP_SETTINGS_PAGE_TITLE,
});
const hasGraphMigrationMarker = (blockMap: Record<string, string>): boolean =>
!!blockMap[GRAPH_MIGRATION_MARKER];

const isPropsValid = (
schema: z.ZodTypeAny,
Expand Down Expand Up @@ -182,7 +178,7 @@ export const migrateGraphLevel = async (
return;
}

if (hasGraphMigrationMarker()) {
if (hasGraphMigrationMarker(blockUids)) {
console.log(`${LOG_PREFIX} graph-level: skipped (already migrated)`);
return;
}
Expand Down
Loading
Loading