ENG-1455: Dual-read from old-system settings and from blockprops#812
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
| initializeSupabaseSync(); | ||
| } | ||
|
|
||
| const { extensionAPI } = onloadArgs; |
There was a problem hiding this comment.
For testing add
const debugWindow = window as unknown as { [key: string]: unknown };
debugWindow["__DG_DEBUG__"] = {
runDualReadProbe,
};
then go to browser console and do
const report = window.__DG_DEBUG__.runDualReadProbe();
report.summary; // this provides a summary
report.mismatches; // if there are any mismatches we will see them
report.rows; // will print all the values we read from the graph in personal/global/discourse node + blockprops form
| DISALLOW_DIAGNOSTICS, | ||
| } from "./data/userSettings"; | ||
| import { initSchema } from "./components/settings/utils/init"; | ||
| import { setupPullWatchOnSettingsPage } from "./components/settings/utils/pullWatchers"; |
There was a problem hiding this comment.
for testing add
import { runDualReadProbe } from "./components/settings/utils/accessors";
| } | ||
|
|
||
| return nodes; | ||
| }; |
There was a problem hiding this comment.
For testing add the following here
type DualReadProbeCase = { keys: string[] };
type DualReadProbeNodeCase = {
nodeType: string;
keys: string[];
};
type DualReadProbeItem = {
scope: "personal" | "global" | "discourseNode";
keys: string[];
nodeType?: string;
legacyValue?: unknown;
blockPropsValue?: unknown;
legacyError?: string;
blockPropsError?: string;
match: boolean;
};
const normalizeProbeError = (error: unknown): string => {
return error instanceof Error ? error.message : String(error);
};
const safeSerialize = (value: unknown): string => {
try {
return JSON.stringify(value);
} catch {
return String(value);
}
};
const normalizeForComparison = (value: unknown): unknown => {
if (Array.isArray(value)) {
return value.map(normalizeForComparison);
}
if (isRecord(value)) {
return Object.keys(value)
.sort()
.reduce<Record<string, unknown>>((acc, key) => {
acc[key] = normalizeForComparison(value[key]);
return acc;
}, {});
}
return value;
};
const runProbeRead = (
reader: () => unknown,
): {
value?: unknown;
error?: string;
} => {
try {
return { value: reader() };
} catch (error) {
return { error: normalizeProbeError(error) };
}
};
const hasProbeMatch = ({
legacyError,
blockPropsError,
legacyValue,
blockPropsValue,
}: {
legacyError?: string;
blockPropsError?: string;
legacyValue?: unknown;
blockPropsValue?: unknown;
}): boolean => {
if (legacyError || blockPropsError) return false;
return (
safeSerialize(normalizeForComparison(legacyValue)) ===
safeSerialize(normalizeForComparison(blockPropsValue))
);
};
const DEFAULT_DISCOURSE_NODE_PROBE_TYPE = "Claim";
const DEFAULT_DISCOURSE_NODE_PROBE_KEYS: string[][] = [
["description"],
["shortcut"],
["tag"],
["index"],
["format"],
["specification", "enabled"],
["specification", "query"],
["template"],
["attributes"],
["overlay"],
["canvasSettings", "color"],
["canvasSettings", "alias"],
["canvasSettings", "key-image"],
["canvasSettings", "key-image-option"],
["canvasSettings", "query-builder-alias"],
["graphOverview"],
["suggestiveRules", "embeddingRef"],
["suggestiveRules", "isFirstChild"],
];
export const runDualReadProbe = ({
personalCases = [
{ keys: ["Discourse context overlay"] },
{ keys: ["Suggestive mode overlay"] },
{ keys: ["Text selection popup"] },
{ keys: ["Disable sidebar open"] },
{ keys: ["Page preview"] },
{ keys: ["Hide feedback button"] },
{ keys: ["Auto canvas relations"] },
{ keys: ["Overlay in canvas"] },
{ keys: ["Streamline styling"] },
{ keys: ["Disable product diagnostics"] },
{ keys: ["Discourse tool shortcut"] },
{ keys: ["Personal node menu trigger"] },
{ keys: ["Node search menu trigger"] },
{ keys: ["Query"] },
{ keys: ["Query", "Hide query metadata"] },
{ keys: ["Query", "Default page size"] },
{ keys: ["Query", "Query pages"] },
{ keys: ["Query", "Default filters"] },
{ keys: ["Left sidebar"] },
],
globalCases = [
{ keys: ["Trigger"] },
{ keys: ["Canvas page format"] },
{ keys: ["Left sidebar"] },
{ keys: ["Left sidebar", "Children"] },
{ keys: ["Left sidebar", "Settings", "Collapsable"] },
{ keys: ["Left sidebar", "Settings", "Folded"] },
{ keys: ["Export"] },
{ keys: ["Export", "Remove special characters"] },
{ keys: ["Export", "Resolve block references"] },
{ keys: ["Export", "Resolve block embeds"] },
{ keys: ["Export", "Append referenced node"] },
{ keys: ["Export", "Link type"] },
{ keys: ["Export", "Max filename length"] },
{ keys: ["Export", "Frontmatter"] },
{ keys: ["Suggestive mode"] },
{ keys: ["Suggestive mode", "Include current page relations"] },
{ keys: ["Suggestive mode", "Include parent and child blocks"] },
{ keys: ["Suggestive mode", "Page groups"] },
{ keys: ["Relations"] },
],
discourseNodeCases = DEFAULT_DISCOURSE_NODE_PROBE_KEYS.map((keys) => ({
nodeType: DEFAULT_DISCOURSE_NODE_PROBE_TYPE,
keys,
})),
}: {
personalCases?: DualReadProbeCase[];
globalCases?: DualReadProbeCase[];
discourseNodeCases?: DualReadProbeNodeCase[];
} = {}) => {
const rows: DualReadProbeItem[] = [];
personalCases.forEach(({ keys }) => {
const legacy = runProbeRead(() => getLegacyPersonalSetting(keys));
const blockProps = runProbeRead(() =>
readPathValue(getPersonalSettings(), keys),
);
rows.push({
scope: "personal",
keys,
legacyValue: legacy.value,
blockPropsValue: blockProps.value,
legacyError: legacy.error,
blockPropsError: blockProps.error,
match: hasProbeMatch({
legacyError: legacy.error,
blockPropsError: blockProps.error,
legacyValue: legacy.value,
blockPropsValue: blockProps.value,
}),
});
});
globalCases.forEach(({ keys }) => {
const legacy = runProbeRead(() => getLegacyGlobalSetting(keys));
const blockProps = runProbeRead(() =>
readPathValue(getGlobalSettings(), keys),
);
rows.push({
scope: "global",
keys,
legacyValue: legacy.value,
blockPropsValue: blockProps.value,
legacyError: legacy.error,
blockPropsError: blockProps.error,
match: hasProbeMatch({
legacyError: legacy.error,
blockPropsError: blockProps.error,
legacyValue: legacy.value,
blockPropsValue: blockProps.value,
}),
});
});
discourseNodeCases.forEach(({ nodeType, keys }) => {
const legacy = runProbeRead(() =>
getLegacyDiscourseNodeSetting(nodeType, keys),
);
const blockProps = runProbeRead(() =>
readPathValue(getDiscourseNodeSettings(nodeType), keys),
);
rows.push({
scope: "discourseNode",
nodeType,
keys,
legacyValue: legacy.value,
blockPropsValue: blockProps.value,
legacyError: legacy.error,
blockPropsError: blockProps.error,
match: hasProbeMatch({
legacyError: legacy.error,
blockPropsError: blockProps.error,
legacyValue: legacy.value,
blockPropsValue: blockProps.value,
}),
});
});
const mismatches = rows.filter((row) => !row.match);
return {
dualReadEnabled: isDualReadEnabled(),
summary: {
total: rows.length,
matches: rows.filter((row) => row.match).length,
mismatches: mismatches.length,
},
rows,
mismatches,
};
};
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughIntroduces dual-read compatibility for settings by adding extensive legacy reading paths and helper functions. When dual-read is disabled, the system falls back to legacy settings resolution for Personal, Global, and Discourse Node configurations. Updates query-related default values and introduces a new dual-read feature flag. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/roam/src/components/settings/utils/zodSchema.ts (1)
98-103: Consider adding a type annotation todefaultNodeIndexValue.This ensures the constant stays in sync with
IndexSchemaif the schema evolves. Without the annotation, adding a new required field toIndexSchemawon't cause a compile error here.♻️ Proposed fix
-const defaultNodeIndexValue = { +const defaultNodeIndexValue: z.input<typeof IndexSchema> = { conditions: [], selections: [], custom: "", returnNode: "node", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/roam/src/components/settings/utils/zodSchema.ts` around lines 98 - 103, The constant defaultNodeIndexValue is missing a TypeScript type annotation and should be annotated to match the IndexSchema so future schema changes cause compile errors; update defaultNodeIndexValue to have the type inferred from IndexSchema (for example using z.infer<typeof IndexSchema> or an exported Index type) and ensure the object literal conforms to that type, adjusting fields or imports as needed.apps/roam/src/components/settings/utils/accessors.ts (1)
168-175:DEFAULT_LEGACY_QUERYduplicatesdefaultNodeIndexValuefromzodSchema.ts.This constant is identical to
defaultNodeIndexValuedefined inzodSchema.ts(lines 98-103). Consider exporting that constant fromzodSchema.tsand reusing it here to avoid the two drifting apart.♻️ Proposed fix
In
zodSchema.ts:-const defaultNodeIndexValue = { +export const DEFAULT_NODE_INDEX_VALUE = { conditions: [], selections: [], custom: "", returnNode: "node", -}; +} as const;In
accessors.ts:import { DG_BLOCK_PROP_SETTINGS_PAGE_TITLE, DISCOURSE_NODE_PAGE_PREFIX, TOP_LEVEL_BLOCK_PROP_KEYS, + DEFAULT_NODE_INDEX_VALUE, FeatureFlagsSchema, ... } from "./zodSchema"; -const DEFAULT_LEGACY_QUERY = { - conditions: [], - selections: [], - custom: "", - returnNode: "node", -}; +const DEFAULT_LEGACY_QUERY = DEFAULT_NODE_INDEX_VALUE;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/roam/src/components/settings/utils/accessors.ts` around lines 168 - 175, DEFAULT_LEGACY_QUERY duplicates the constant defaultNodeIndexValue in zodSchema.ts; export defaultNodeIndexValue from zodSchema.ts (if not already exported) and update accessors.ts to import and reuse defaultNodeIndexValue instead of declaring DEFAULT_LEGACY_QUERY, replacing references to DEFAULT_LEGACY_QUERY with the imported defaultNodeIndexValue (or rename the import to DEFAULT_LEGACY_QUERY where used) so there is a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/roam/src/components/settings/utils/accessors.ts`:
- Around line 331-346: getLegacyGlobalSetting uses || for the "Trigger" and
"Canvas page format" branches which treats empty strings as falsy and will
incorrectly fall back to DEFAULT_GLOBAL_SETTINGS; update these branches in
getLegacyGlobalSetting to use the nullish coalescing operator (??) instead of ||
so an explicit empty string value is preserved while still falling back to the
defaults only for null/undefined.
- Around line 697-710: The dual-read path throws on missing settings which
contradicts the T | undefined signature and causes inconsistent behavior; update
getGlobalSetting, getPersonalSetting, and getDiscourseNodeSetting so that when
isDualReadEnabled() is true they do not throw on value === undefined but instead
return undefined (i.e., remove the throw that uses getMissingSettingError and
simply return the readPathValue result), keeping legacy behavior unchanged and
ensuring callers see undefined in both modes.
In `@apps/roam/src/components/settings/utils/zodSchema.ts`:
- Around line 232-236: Update the Query settings so defaults and migration are
consistent: change the fallback used by getQueryPages() in QueryPagesPanel.tsx
from ["queries/*"] to match QuerySettingsSchema's default
["discourse-graph/queries/*"], and consolidate migration behavior so QueryPages
are migrated the same way as other settings (e.g., use schema defaults and
PERSONAL_OLD_DEFAULT_OVERRIDES or perform the migration inside
setQueryPages())—ensure you update references to getQueryPages(),
setQueryPages(), and QuerySettingsSchema so they all agree on the canonical
default and that the migration path is centralized.
---
Nitpick comments:
In `@apps/roam/src/components/settings/utils/accessors.ts`:
- Around line 168-175: DEFAULT_LEGACY_QUERY duplicates the constant
defaultNodeIndexValue in zodSchema.ts; export defaultNodeIndexValue from
zodSchema.ts (if not already exported) and update accessors.ts to import and
reuse defaultNodeIndexValue instead of declaring DEFAULT_LEGACY_QUERY, replacing
references to DEFAULT_LEGACY_QUERY with the imported defaultNodeIndexValue (or
rename the import to DEFAULT_LEGACY_QUERY where used) so there is a single
source of truth.
In `@apps/roam/src/components/settings/utils/zodSchema.ts`:
- Around line 98-103: The constant defaultNodeIndexValue is missing a TypeScript
type annotation and should be annotated to match the IndexSchema so future
schema changes cause compile errors; update defaultNodeIndexValue to have the
type inferred from IndexSchema (for example using z.infer<typeof IndexSchema> or
an exported Index type) and ensure the object literal conforms to that type,
adjusting fields or imports as needed.
| const getLegacyGlobalSetting = (keys: string[]): unknown => { | ||
| if (keys.length === 0) return undefined; | ||
|
|
||
| const settings = getFormattedConfigTree(); | ||
| const firstKey = keys[0]; | ||
|
|
||
| if (firstKey === "Trigger") { | ||
| return settings.trigger.value || DEFAULT_GLOBAL_SETTINGS.Trigger; | ||
| } | ||
|
|
||
| if (firstKey === "Canvas page format") { | ||
| return ( | ||
| settings.canvasPageFormat.value || | ||
| DEFAULT_GLOBAL_SETTINGS["Canvas page format"] | ||
| ); | ||
| } |
There was a problem hiding this comment.
|| vs ?? for Trigger and Canvas page format may suppress valid empty-string values.
Lines 338 and 343 use ||, which treats an empty string "" as falsy and falls back to the default. If a user explicitly sets Trigger to "" (to disable it) or Canvas page format to "", the legacy reader will silently override their choice with the schema default ("\\" or "Canvas/*").
The other fallback patterns in this function (e.g., lines 355, 358, 368-387) correctly use ?? for boolean/number fields. Consider whether || is intentional here for strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/roam/src/components/settings/utils/accessors.ts` around lines 331 - 346,
getLegacyGlobalSetting uses || for the "Trigger" and "Canvas page format"
branches which treats empty strings as falsy and will incorrectly fall back to
DEFAULT_GLOBAL_SETTINGS; update these branches in getLegacyGlobalSetting to use
the nullish coalescing operator (??) instead of || so an explicit empty string
value is preserved while still falling back to the defaults only for
null/undefined.
| export const getGlobalSetting = <T = unknown>( | ||
| keys: string[], | ||
| ): T | undefined => { | ||
| const settings = getGlobalSettings(); | ||
| if (!isDualReadEnabled()) { | ||
| return getLegacyGlobalSetting(keys) as T | undefined; | ||
| } | ||
|
|
||
| return keys.reduce<unknown>((current, key) => { | ||
| if (!isRecord(current) || !(key in current)) return undefined; | ||
| return current[key]; | ||
| }, settings) as T | undefined; | ||
| const settings = getGlobalSettings(); | ||
| const value = readPathValue(settings, keys) as T | undefined; | ||
| if (value === undefined) { | ||
| throw new Error(getMissingSettingError({ context: "Global", keys })); | ||
| } | ||
| return value; | ||
| }; |
There was a problem hiding this comment.
Throwing on missing settings contradicts the T | undefined return type and creates inconsistent behavior between modes.
When dual-read is ON, getGlobalSetting, getPersonalSetting, and getDiscourseNodeSetting throw an Error if the value is undefined. When dual-read is OFF (legacy), the same functions return undefined for unrecognized/missing paths. This creates two problems:
- Contract violation: The return type is
T | undefined, but the dual-read-ON path never returnsundefined— it throws instead. Callers that defensively check forundefinedwill never hit that branch; callers that don't try/catch will crash. - Mode-switching breakage: Toggling the feature flag changes the error-handling semantics for every call site, making it risky to enable dual-read gradually.
Consider either (a) returning undefined in both modes and letting callers decide how to handle missing values, or (b) throwing in both modes and updating the return type to T (non-optional). Option (a) is safer for a gradual rollout.
🐛 Option (a): Return undefined consistently in both modes
Example fix for getGlobalSetting (apply same pattern to getPersonalSetting and getDiscourseNodeSetting):
export const getGlobalSetting = <T = unknown>(
keys: string[],
): T | undefined => {
if (!isDualReadEnabled()) {
return getLegacyGlobalSetting(keys) as T | undefined;
}
const settings = getGlobalSettings();
const value = readPathValue(settings, keys) as T | undefined;
- if (value === undefined) {
- throw new Error(getMissingSettingError({ context: "Global", keys }));
- }
return value;
};Also applies to: 757-770, 831-852
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/roam/src/components/settings/utils/accessors.ts` around lines 697 - 710,
The dual-read path throws on missing settings which contradicts the T |
undefined signature and causes inconsistent behavior; update getGlobalSetting,
getPersonalSetting, and getDiscourseNodeSetting so that when isDualReadEnabled()
is true they do not throw on value === undefined but instead return undefined
(i.e., remove the throw that uses getMissingSettingError and simply return the
readPathValue result), keeping legacy behavior unchanged and ensuring callers
see undefined in both modes.
| export const QuerySettingsSchema = z.object({ | ||
| "Hide query metadata": z.boolean().default(false), | ||
| "Hide query metadata": z.boolean().default(true), | ||
| "Default page size": z.number().default(10), | ||
| "Query pages": z.array(z.string()).default([]), | ||
| "Query pages": z.array(z.string()).default(["discourse-graph/queries/*"]), | ||
| "Default filters": z.record(z.string(), StoredFiltersSchema).default({}), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how QuerySettingsSchema defaults are consumed across the codebase
rg -n --type=ts -C3 'QuerySettingsSchema|"Hide query metadata"|"Query pages"|"Default page size"' -g '!*.example.*' -g '!node_modules/**'Repository: DiscourseGraphs/discourse-graph
Length of output: 7983
🏁 Script executed:
#!/bin/bash
# Find getQueryPages function and understand how it handles defaults
rg -n 'getQueryPages|PERSONAL_OLD_DEFAULT_OVERRIDES|getLegacyPersonalSetting' --type=ts -B2 -A5
# Also look for any version tracking or migration logic
rg -n 'version|migration|migrate' --type=ts -g '!node_modules' | head -30Repository: DiscourseGraphs/discourse-graph
Length of output: 13950
🏁 Script executed:
#!/bin/bash
# Get the full QueryPagesPanel.tsx file to see complete getQueryPages implementation
cat -n apps/roam/src/components/settings/QueryPagesPanel.tsx
echo "---"
# Search for DEFAULT_PERSONAL_SETTINGS definition
rg -n 'DEFAULT_PERSONAL_SETTINGS' --type=ts -B2 -A20Repository: DiscourseGraphs/discourse-graph
Length of output: 7249
🏁 Script executed:
#!/bin/bash
# Check how extensionAPI settings are loaded and validated
rg -n 'getPersonalSettings|readPathValue|extensionAPI.settings' --type=ts -B2 -A3 | head -80
# Also check if there's settings initialization with schema
rg -n 'parse\(\|validate\(' --type=ts apps/roam/src/components/settings/utils/accessors.ts | head -20Repository: DiscourseGraphs/discourse-graph
Length of output: 6204
🏁 Script executed:
#!/bin/bash
# Find where setQueryPages is called
rg -n 'setQueryPages' --type=ts -B3 -A3
# Also check the main initialization entry point
fd 'index|main|load|onload' -t f --path '*/apps/roam/src/*' | grep -E '\.(ts|tsx)$' | head -5Repository: DiscourseGraphs/discourse-graph
Length of output: 1890
🏁 Script executed:
#!/bin/bash
# Check git history for the schema to understand what changed
git log --oneline -n 20 -- "apps/roam/src/components/settings/utils/zodSchema.ts" 2>/dev/null || echo "Git history not available"
# Also check if there are any comments about defaults in the schema file
rg -n 'default|Default|migration|Migration' apps/roam/src/components/settings/utils/zodSchema.ts -B1 -A1 | head -40Repository: DiscourseGraphs/discourse-graph
Length of output: 1311
Address inconsistent migration patterns and fallback mismatch for Query settings.
The default changes are mitigated by setQueryPages() (called during onload), which ensures "discourse-graph/queries/*" is added to existing user arrays. However, there's an inconsistency: getQueryPages() has a fallback of ["queries/*"] (line 17 of QueryPagesPanel.tsx) while the schema default is ["discourse-graph/queries/*"]. Additionally, the migration approach differs between settings—"Hide query metadata" uses schema defaults + PERSONAL_OLD_DEFAULT_OVERRIDES, while "Query pages" relies on active migration via setQueryPages(). Align the fallback value in getQueryPages() with the schema default and consider consolidating the migration strategy for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/roam/src/components/settings/utils/zodSchema.ts` around lines 232 - 236,
Update the Query settings so defaults and migration are consistent: change the
fallback used by getQueryPages() in QueryPagesPanel.tsx from ["queries/*"] to
match QuerySettingsSchema's default ["discourse-graph/queries/*"], and
consolidate migration behavior so QueryPages are migrated the same way as other
settings (e.g., use schema defaults and PERSONAL_OLD_DEFAULT_OVERRIDES or
perform the migration inside setQueryPages())—ensure you update references to
getQueryPages(), setQueryPages(), and QuerySettingsSchema so they all agree on
the canonical default and that the migration path is centralized.
| const settings = getGlobalSettings(); | ||
| const value = readPathValue(settings, keys) as T | undefined; | ||
| if (value === undefined) { | ||
| throw new Error(getMissingSettingError({ context: "Global", keys })); | ||
| } | ||
| return value; |
There was a problem hiding this comment.
🔴 Getter functions throw instead of returning undefined for missing dynamic record keys when dual-read is enabled
When dual-read is enabled, getGlobalSetting, getPersonalSetting, and getDiscourseNodeSetting throw an Error for any path that resolves to undefined, but their return type signature is T | undefined, indicating callers should expect undefined for missing values.
Root Cause and Impact
The Zod schemas use z.record() for dynamic-key collections like Relations (zodSchema.ts:211), Left sidebar personal sections (zodSchema.ts:233), and Default filters (zodSchema.ts:246). These records legitimately may not contain a requested key — e.g. getGlobalSetting(["Relations", "some-relation-id"]) for a relation that doesn't exist, or getPersonalSetting(["Left sidebar", "My Section"]) for a section name that hasn't been created yet.
The readPathValue helper correctly returns undefined when a key is absent (accessors.ts:117), but the callers at accessors.ts:706-707, accessors.ts:764-767, and accessors.ts:841-848 convert that undefined into a thrown exception:
if (value === undefined) {
throw new Error(getMissingSettingError({ context: "Global", keys }));
}Any caller using standard patterns like getGlobalSetting(keys) ?? fallback will get an unhandled exception instead of the expected undefined. This breaks the API contract declared by the return type T | undefined.
Impact: When dual-read is enabled, any access to a dynamic record path that doesn't exist will throw an unhandled exception, potentially crashing UI components or breaking features. The functions should return undefined for missing paths (matching the declared return type) or the return type should be changed to just T with callers updated accordingly.
Prompt for agents
In apps/roam/src/components/settings/utils/accessors.ts, three functions throw an Error when readPathValue returns undefined, but their return type is T | undefined. This breaks the contract for dynamic record paths (e.g. Relations, Left sidebar sections, Default filters) where a key may legitimately not exist.
Fix by removing the throw and returning undefined instead, in all three locations:
1. getGlobalSetting (lines 704-709): Remove the throw and just return value.
2. getPersonalSetting (lines 762-769): Remove the throw and just return value.
3. getDiscourseNodeSetting (lines 839-851): Remove the throw and just return value.
Alternatively, if the intent is to enforce that all paths must exist, change the return type to T (removing | undefined) and update all callers. But given these accessor functions read from z.record() schemas with dynamic keys, returning undefined for missing keys is the correct behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if (!isDualReadEnabled()) { | ||
| return getLegacyGlobalSetting(keys) as T | undefined; | ||
| } | ||
|
|
||
| return keys.reduce<unknown>((current, key) => { | ||
| if (!isRecord(current) || !(key in current)) return undefined; | ||
| return current[key]; | ||
| }, settings) as T | undefined; | ||
| const settings = getGlobalSettings(); | ||
| const value = readPathValue(settings, keys) as T | undefined; | ||
| if (value === undefined) { | ||
| throw new Error(getMissingSettingError({ context: "Global", keys })); | ||
| } | ||
| return value; |
There was a problem hiding this comment.
Critical Logic Error: Not actually implementing dual-read behavior
When isDualReadEnabled() is true, the code reads ONLY from the new system and throws an error if the value is undefined. This is not dual-read - it's a hard switch to the new system.
A true dual-read implementation should:
if (!isDualReadEnabled()) {
return getLegacyGlobalSetting(keys) as T | undefined;
}
const settings = getGlobalSettings();
const value = readPathValue(settings, keys) as T | undefined;
if (value === undefined) {
// Fall back to legacy instead of throwing
return getLegacyGlobalSetting(keys) as T | undefined;
}
return value;The current implementation will cause production crashes when dual-read is enabled for any setting that hasn't been migrated to the new blockprops system yet.
| if (!isDualReadEnabled()) { | |
| return getLegacyGlobalSetting(keys) as T | undefined; | |
| } | |
| return keys.reduce<unknown>((current, key) => { | |
| if (!isRecord(current) || !(key in current)) return undefined; | |
| return current[key]; | |
| }, settings) as T | undefined; | |
| const settings = getGlobalSettings(); | |
| const value = readPathValue(settings, keys) as T | undefined; | |
| if (value === undefined) { | |
| throw new Error(getMissingSettingError({ context: "Global", keys })); | |
| } | |
| return value; | |
| if (!isDualReadEnabled()) { | |
| return getLegacyGlobalSetting(keys) as T | undefined; | |
| } | |
| const settings = getGlobalSettings(); | |
| const value = readPathValue(settings, keys) as T | undefined; | |
| if (value === undefined) { | |
| // Fall back to legacy instead of throwing | |
| return getLegacyGlobalSetting(keys) as T | undefined; | |
| } | |
| return value; | |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
1 participant
https://www.loom.com/share/1894f6a6fef2493e97ad1bf06fed851f
Summary by CodeRabbit