ENG-1189: Prod: Init block prop based schema#666
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughIntroduces a configuration constants file and an initialization module for Discourse Graph settings. The config file defines UI label constants and setting key names. The initialization module orchestrates creation of settings infrastructure, user-specific settings blocks, and discourse node pages, while persisting block properties via schema validation. Changes
Sequence DiagramsequenceDiagram
participant System as System/Init
participant SettingsPage as Settings Page
participant PersonalBlock as Personal Settings Block
participant NodePages as Discourse Node Pages
participant BlockMap as Block Property Map
participant Console as Console Reporter
System->>SettingsPage: Ensure settings page exists
SettingsPage-->>System: Settings page UID
System->>PersonalBlock: Fetch or create personal settings block
PersonalBlock-->>System: Personal block UID + properties
System->>BlockMap: Build map of blocks under settings page
BlockMap-->>System: Block UID → property mapping
System->>PersonalBlock: Parse & initialize properties (schema validation)
alt Properties missing
PersonalBlock->>PersonalBlock: Persist default values
end
Note over System,NodePages: For each Discourse Node:
System->>NodePages: Ensure node page exists
NodePages-->>System: Node page UID
System->>NodePages: Create/fetch template, index, specification blocks
NodePages-->>BlockMap: Block UIDs
System->>BlockMap: Parse & initialize node properties (DiscourseNodeSchema)
BlockMap->>BlockMap: Link template/index/specification UIDs
alt Properties missing
BlockMap->>BlockMap: Persist missing UID references
end
System->>Console: Aggregate all settings and print structured report
Console-->>System: Report printed
System-->>System: Return InitSchemaResult (block & node page UIDs)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Pre-merge checks✅ Passed checks (3 passed)
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: 0
🧹 Nitpick comments (9)
apps/roam/src/components/settings/utils/init.ts (9)
24-30: Add explicit return type and consider error handling.Per coding guidelines, functions should have explicit return types. Additionally, falling back to an empty string if
window.roamAlphaAPI.user.uid()returns null/undefined could cause issues if an empty string is not a valid personal settings key.🔎 Suggested improvements
-const getPersonalSettingsKey = (): string => { +const getPersonalSettingsKey = (): string => { if (cachedPersonalSettingsKey !== null) { return cachedPersonalSettingsKey; } - cachedPersonalSettingsKey = window.roamAlphaAPI.user.uid() || ""; + const uid = window.roamAlphaAPI.user.uid(); + if (!uid) { + console.warn("Unable to retrieve user UID, using empty string as fallback"); + } + cachedPersonalSettingsKey = uid || ""; return cachedPersonalSettingsKey; };
32-34: Add explicit return type.Per coding guidelines, add an explicit return type to this function.
🔎 Suggested improvement
-const getDiscourseNodePageTitle = (nodeLabel: string): string => { +const getDiscourseNodePageTitle = (nodeLabel: string): string => { return `${DISCOURSE_NODE_PAGE_PREFIX}${nodeLabel}`; };
36-48: Add error handling for page creation.The
createPagecall lacks error handling, which could result in unhandled promise rejections if the Roam API call fails. Consider wrapping async operations in try-catch blocks.🔎 Suggested improvement
const ensurePageExists = async (pageTitle: string): Promise<string> => { let pageUid = getPageUidByPageTitle(pageTitle); if (!pageUid) { - pageUid = window.roamAlphaAPI.util.generateUID(); - await createPage({ - title: pageTitle, - uid: pageUid, - }); + try { + pageUid = window.roamAlphaAPI.util.generateUID(); + await createPage({ + title: pageTitle, + uid: pageUid, + }); + } catch (error) { + console.error(`Failed to create page "${pageTitle}":`, error); + throw error; + } } return pageUid; };
50-76: Consider named parameters and error handling.Per coding guidelines, functions with more than 2 parameters should use named parameters (object destructuring). Additionally, the
Promise.allfor block creation lacks error handling.🔎 Suggested improvements
-const ensureBlocksExist = async ( - pageUid: string, - blockTexts: string[], - existingBlockMap: Record<string, string>, -): Promise<Record<string, string>> => { +const ensureBlocksExist = async ({ + pageUid, + blockTexts, + existingBlockMap, +}: { + pageUid: string; + blockTexts: string[]; + existingBlockMap: Record<string, string>; +}): Promise<Record<string, string>> => { const missingBlocks = blockTexts.filter( (blockText) => !existingBlockMap[blockText], ); if (missingBlocks.length > 0) { - const createdBlocks = await Promise.all( - missingBlocks.map(async (blockText) => { - const uid = await createBlock({ - parentUid: pageUid, - node: { text: blockText }, - }); - return { text: blockText, uid }; - }), - ); + try { + const createdBlocks = await Promise.all( + missingBlocks.map(async (blockText) => { + const uid = await createBlock({ + parentUid: pageUid, + node: { text: blockText }, + }); + return { text: blockText, uid }; + }), + ); + + createdBlocks.forEach((block) => { + existingBlockMap[block.text] = block.uid; + }); + } catch (error) { + console.error("Failed to create blocks:", error); + throw error; + } + } - createdBlocks.forEach((block) => { - existingBlockMap[block.text] = block.uid; - }); - } - return existingBlockMap; };Note: If you apply this refactor, you'll need to update call sites to use object syntax:
await ensureBlocksExist({ pageUid, blockTexts: topLevelBlocks, existingBlockMap: blockMap });
78-94: Add error handling for block creation.The
createBlockcall lacks error handling. Consider wrapping it in a try-catch block.🔎 Suggested improvement
const ensurePersonalBlockExists = async ( pageUid: string, existingBlockMap: Record<string, string>, ): Promise<{ key: string; uid: string }> => { const personalKey = getPersonalSettingsKey(); if (existingBlockMap[personalKey]) { return { key: personalKey, uid: existingBlockMap[personalKey] }; } - const uid = await createBlock({ - parentUid: pageUid, - node: { text: personalKey }, - }); + try { + const uid = await createBlock({ + parentUid: pageUid, + node: { text: personalKey }, + }); + return { key: personalKey, uid }; + } catch (error) { + console.error("Failed to create personal settings block:", error); + throw error; + } - - return { key: personalKey, uid }; };
96-103: Add explicit return type.Per coding guidelines, add an explicit return type.
🔎 Suggested improvement
-const buildBlockMap = (pageUid: string): Record<string, string> => { +const buildBlockMap = (pageUid: string): Record<string, string> => { const existingChildren = getShallowTreeByParentUid(pageUid); const blockMap: Record<string, string> = {}; existingChildren.forEach((child) => { blockMap[child.text] = child.uid; }); return blockMap; };
152-199: Consider extracting UID update logic and reviewing type casting.The conditional logic at Lines 174-196 handles two distinct scenarios: (1) initializing all props when empty, and (2) updating missing UID references. This logic is clear but could benefit from extraction into smaller helper functions for better maintainability.
Additionally, Line 189 casts to
Record<string, json>, which bypasses Zod's type safety. Based on retrieved learnings, type validation is expected at the caller side. Ensure the schema output type is compatible.🔎 Suggested refactoring
+const initializeNodeProps = ( + pageUid: string, + node: (typeof INITIAL_NODE_VALUES)[number], + templateUid: string, + indexUid: string, + specificationUid: string, +): void => { + const nodeData = DiscourseNodeSchema.parse({ + text: node.text, + type: node.type, + format: node.format || "", + shortcut: node.shortcut || "", + tag: node.tag || "", + graphOverview: node.graphOverview ?? false, + canvasSettings: node.canvasSettings || {}, + templateUid, + indexUid, + specificationUid, + backedBy: "user", + }); + setBlockProps(pageUid, nodeData as Record<string, json>, false); +}; + +const updateMissingUids = ( + pageUid: string, + templateUid: string, + indexUid: string, + specificationUid: string, +): void => { + setBlockProps(pageUid, { templateUid, indexUid, specificationUid }, true); +}; + const initSingleDiscourseNode = async ( node: (typeof INITIAL_NODE_VALUES)[number], ): Promise<{ label: string; pageUid: string } | null> => { if (!node.text) return null; const pageUid = await ensureDiscourseNodePageExists(node.text); const existingProps = getBlockProps(pageUid); const blockMap = buildBlockMap(pageUid); for (const key of Object.values(DISCOURSE_NODE_BLOCK_KEYS)) { if (!blockMap[key]) { blockMap[key] = await createBlock({ parentUid: pageUid, node: { text: key }, }); } } const templateUid = blockMap[DISCOURSE_NODE_BLOCK_KEYS.template]; const indexUid = blockMap[DISCOURSE_NODE_BLOCK_KEYS.index]; const specificationUid = blockMap[DISCOURSE_NODE_BLOCK_KEYS.specification]; if (!existingProps || Object.keys(existingProps).length === 0) { - const nodeData = DiscourseNodeSchema.parse({ - text: node.text, - type: node.type, - format: node.format || "", - shortcut: node.shortcut || "", - tag: node.tag || "", - graphOverview: node.graphOverview ?? false, - canvasSettings: node.canvasSettings || {}, - templateUid, - indexUid, - specificationUid, - backedBy: "user", - }); - - setBlockProps(pageUid, nodeData as Record<string, json>, false); + initializeNodeProps(pageUid, node, templateUid, indexUid, specificationUid); } else if ( !existingProps.templateUid || !existingProps.indexUid || !existingProps.specificationUid ) { - setBlockProps(pageUid, { templateUid, indexUid, specificationUid }, true); + updateMissingUids(pageUid, templateUid, indexUid, specificationUid); } return { label: node.text, pageUid }; };
216-322: Refactor the logging function for better maintainability.The
printAllSettingsfunction is over 100 lines and handles multiple concerns (feature flags, global settings, personal settings, each with nested subsections). Consider extracting helper functions for each settings category.🔎 Suggested refactoring approach
const printFeatureFlags = (featureFlags: Record<string, json> | null): void => { if (featureFlags) { console.group("🚩 Feature Flags"); console.table(featureFlags); console.groupEnd(); } }; const printGlobalSettings = (globalSettings: Record<string, json> | null): void => { if (globalSettings) { console.group("🌍 Global Settings"); console.log("Trigger:", globalSettings?.Trigger || "(empty)"); console.log("Canvas Page Format:", globalSettings?.["Canvas Page Format"] || "(empty)"); if (globalSettings?.["Left Sidebar"]) { console.group("📂 Left Sidebar"); console.log(globalSettings["Left Sidebar"]); console.groupEnd(); } // ... other subsections console.groupEnd(); } }; const printPersonalSettings = (personalSettings: Record<string, json> | null): void => { if (personalSettings) { console.group("👤 Personal Settings"); // ... personal settings logic console.groupEnd(); } }; const printAllSettings = (blockMap: Record<string, string>): void => { const featureFlagsUid = blockMap[TOP_LEVEL_BLOCK_PROP_KEYS.featureFlags]; const globalUid = blockMap[TOP_LEVEL_BLOCK_PROP_KEYS.global]; const personalKey = getPersonalSettingsKey(); const personalUid = blockMap[personalKey]; const featureFlags = featureFlagsUid ? getBlockProps(featureFlagsUid) : null; const globalSettings = globalUid ? getBlockProps(globalUid) : null; const personalSettings = personalUid ? getBlockProps(personalUid) : null; console.group("🔧 Discourse Graph Settings Initialized"); printFeatureFlags(featureFlags); printGlobalSettings(globalSettings); printPersonalSettings(personalSettings); console.groupEnd(); };
329-336: Add error handling to the main initialization function.The
initSchemafunction orchestrates multiple async operations but lacks error handling. If any initialization step fails, it could leave the system in an inconsistent state.🔎 Suggested improvement
export const initSchema = async (): Promise<InitSchemaResult> => { - const blockUids = await initSettingsPageBlocks(); - const nodePageUids = await initDiscourseNodePages(); + try { + const blockUids = await initSettingsPageBlocks(); + const nodePageUids = await initDiscourseNodePages(); - printAllSettings(blockUids); + printAllSettings(blockUids); - return { blockUids, nodePageUids }; + return { blockUids, nodePageUids }; + } catch (error) { + console.error("Failed to initialize Discourse Graph schema:", error); + throw error; + } };
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/roam/src/components/settings/data/blockPropsSettingsConfig.tsapps/roam/src/components/settings/utils/init.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
apps/roam/src/components/settings/data/blockPropsSettingsConfig.tsapps/roam/src/components/settings/utils/init.ts
apps/roam/**/*.{js,ts,tsx,jsx,json}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Prefer existing dependencies from package.json when working on the Roam Research extension
Files:
apps/roam/src/components/settings/data/blockPropsSettingsConfig.tsapps/roam/src/components/settings/utils/init.ts
apps/roam/**/*.{ts,tsx,jsx,js,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Files:
apps/roam/src/components/settings/data/blockPropsSettingsConfig.tsapps/roam/src/components/settings/utils/init.ts
apps/roam/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
apps/roam/**/*.{ts,tsx,js,jsx}: Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Files:
apps/roam/src/components/settings/data/blockPropsSettingsConfig.tsapps/roam/src/components/settings/utils/init.ts
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/components/settings/data/blockPropsSettingsConfig.tsapps/roam/src/components/settings/utils/init.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-06-25T22:56:17.522Z
Learning: In the Roam discourse-graph system, the existence of the configuration page (identified by DISCOURSE_CONFIG_PAGE_TITLE) and its corresponding UID is a system invariant. The code can safely assume this page will always exist, so defensive null checks are not needed when using `getPageUidByPageTitle(DISCOURSE_CONFIG_PAGE_TITLE)`.
📚 Learning: 2025-12-22T05:43:09.803Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 630
File: apps/roam/src/utils/settingsUsingBlockProps.ts:64-66
Timestamp: 2025-12-22T05:43:09.803Z
Learning: In `apps/roam/src/utils/settingsUsingBlockProps.ts`, the `setBlockPropBasedSettings` function expects callers to ensure that the `value` parameter matches the expected type for the given `keys` path. Type validation is handled at the caller side rather than within the utility function.
Applied to files:
apps/roam/src/components/settings/data/blockPropsSettingsConfig.tsapps/roam/src/components/settings/utils/init.ts
📚 Learning: 2025-11-06T13:48:35.007Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-06T13:48:35.007Z
Learning: In apps/roam/src/utils/createReifiedBlock.ts, the `setBlockProps` function does not need await as it is synchronous or fire-and-forget.
Applied to files:
apps/roam/src/components/settings/data/blockPropsSettingsConfig.ts
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{ts,tsx,js,jsx} : Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Applied to files:
apps/roam/src/components/settings/data/blockPropsSettingsConfig.ts
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{ts,tsx,js,jsx} : Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Applied to files:
apps/roam/src/components/settings/data/blockPropsSettingsConfig.tsapps/roam/src/components/settings/utils/init.ts
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Applied to files:
apps/roam/src/components/settings/data/blockPropsSettingsConfig.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (3)
apps/roam/src/components/settings/data/blockPropsSettingsConfig.ts (1)
1-19: LGTM! Clean configuration structure.The constants are well-defined and follow TypeScript best practices with
as constassertions for proper type narrowing. The ESLint rule suspension is appropriately scoped and justified for user-facing strings with spaces.apps/roam/src/components/settings/utils/init.ts (2)
1-20: LGTM! Imports are well-organized.All necessary dependencies are properly imported and organized by source (external packages, local utils, data, schemas, config).
105-111: Remove the type casting concern.The cast to
Record<string, json>is safe and necessary for type compatibility. The schemas (FeatureFlagsSchema, GlobalSettingsSchema, PersonalSettingsSchema) produce objects with properties that are all JSON-compatible values (booleans, strings, numbers, nested objects, arrays). These specific object types are assignable toRecord<string, json>without hiding type mismatches. Zod's validation already occurs at theparse()call; the cast simply makes the type compatibility explicit for thesetBlockPropsfunction's expected parameter type.
sid597
force-pushed
the
eng-1189-prod-init-schema
branch
3 times, most recently
from
0ca99fc to
7ea650f
Compare
sid597
force-pushed
the
eng-1105-base-zod-based-schema
branch
from
c6aee0a to
0dbcfa6
Compare
sid597
force-pushed
the
eng-1105-base-zod-based-schema
branch
from
0dbcfa6 to
e86bf02
Compare
sid597
force-pushed
the
eng-1189-prod-init-schema
branch
from
0365026 to
76d487b
Compare
sid597
force-pushed
the
eng-1189-prod-init-schema
branch
from
76d487b to
3c7d025
Compare
sid597
force-pushed
the
eng-1105-base-zod-based-schema
branch
from
e86bf02 to
14525b4
Compare
sid597
changed the base branch from
main
to
eng-1217-personal-left-sidebar-update-settings-and-view-for-personal
sid597
force-pushed
the
eng-1189-prod-init-schema
branch
from
72ce14e to
578ceb4
Compare
sid597
changed the base branch from
graphite-base/666
to
eng-1330-add-zod-schema-for-the-discourse-relations
|
@sid597 when should I expect this to be sent back to me for review? |
|
@mdroidian 8-9hrs from now, today i was afk got back at night |
sid597
force-pushed
the
eng-1189-prod-init-schema
branch
2 times, most recently
from
3e2abe6 to
6db17d0
Compare
|
Part1 (Used my account accidentally): Part 2: https://www.loom.com/share/4b7162b4c8c14fb797144e2631f20da4 |
Part 1 404's for me |
apps/roam/src/components/settings/data/blockPropsSettingsConfig.ts
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/data/defaultRelationsBlockProps.ts
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/data/defaultRelationsBlockProps.ts
Outdated
Show resolved
Hide resolved
sid597
force-pushed
the
eng-1189-prod-init-schema
branch
2 times, most recently
from
f75ab1a to
23823af
Compare
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Looks like CI is failing
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Just quickly went over the changes you've already made to expedite things. Also don't forget the CI 👍
| }; | ||
|
|
||
| const hasNonDefaultNodes = (): boolean => { | ||
| const results = window.roamAlphaAPI.q(` |
There was a problem hiding this comment.
Let's use the existing pattern:
const nodes = getDiscourseNodes().filter(excludeDefaultNodes);
if (nodes.length === 0) {It's a single line and if we ever change how discourse nodes are defined, it won't break hasNonDefaultNodes().
There was a problem hiding this comment.
getDiscourseNodes uses the discourseConfigRef.tree which underneath uses the roam block data and we are migrating from that, therefore we have this
There was a problem hiding this comment.
@sid597 We'll need to recreate a proper getDiscourseNodes() then.
There was a problem hiding this comment.
I have this same function as the getDiscourseNodes() in the accessors pr, by proper you mean smth similar to the existing one or only in performance? I think we should discuss this in the accessors pr #669
There was a problem hiding this comment.
I mean getDiscourseNodes() is used in many places across the repo. When we move from blocks to props, we'll need a getDiscourseNodes() that uses the props.
Regardless of where the new getDiscourseNodes() is created, it should be used in hasNonDefaultNodes() to make sure we DRY.
As an aside, hasNonDefaultNodes (or the new getDiscourseNodes()) should use fast.q and return the props in the single query, not an additional pull for each result
sid597
force-pushed
the
eng-1189-prod-init-schema
branch
from
23823af to
8ea4e10
Compare
sid597
force-pushed
the
eng-1189-prod-init-schema
branch
from
8ea4e10 to
ed28e5d
Compare
sid597
force-pushed
the
eng-1189-prod-init-schema
branch
from
ed28e5d to
8a707cf
Compare
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Let's just make sure we add a comment or create a task to change hasNonDefaultNodes to use the new getDiscourseNodes when it is created.
|
Yeah we are doing this in #669, should use |
2 participants
On load, if the schema page or blocks do not exist but should exist, we create them.
Currently we have a function
printAllSettingsthat logs the default values in the console. We will remove this after review and before merge.I would recommend first loading the plugin locally, see the console log and then move on to the code review.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.