ENG-1189: init schema to check if page and top level blocks exist #631
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
sid597
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughIntroduces a configuration constant for top-level block properties and creates a new utility module to initialize block property settings by ensuring a settings page and required blocks exist in Roam. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 1
🧹 Nitpick comments (3)
apps/roam/src/data/blockPropsSettingsConfig.ts (1)
2-2: Consider adding an explicit type annotation for better type safety.The
TOP_LEVEL_BLOCK_PROP_KEYSconstant would benefit from an explicit type definition to improve IntelliSense, maintainability, and type safety when extending with additional top-level keys.🔎 Proposed type annotation
-export const TOP_LEVEL_BLOCK_PROP_KEYS = { featureFlags: "Feature Flags" }; +export const TOP_LEVEL_BLOCK_PROP_KEYS: Record<string, string> = { featureFlags: "Feature Flags" };Or for stricter typing:
+type TopLevelBlockPropKeys = { + featureFlags: string; +}; + -export const TOP_LEVEL_BLOCK_PROP_KEYS = { featureFlags: "Feature Flags" }; +export const TOP_LEVEL_BLOCK_PROP_KEYS: TopLevelBlockPropKeys = { featureFlags: "Feature Flags" };apps/roam/src/utils/initBlockPropSettings.ts (2)
9-21: Consider adding error handling for Roam API operations.The
createPagecall could fail due to various reasons (API errors, network issues, etc.). While error propagation to the caller may be intentional for initialization code, consider whether explicit error handling or logging would improve debuggability.🔎 Optional error handling pattern
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; };
23-42: Consider error handling and potential race conditions.Two observations:
Error handling: Similar to
ensurePageExists,createBlockoperations could fail. Consider whether explicit error handling would improve debuggability.Race condition risk: If
initSchemais called concurrently, the time gap between reading existing blocks (line 25) and creating missing blocks (lines 33-40) could lead to duplicate creation attempts or partial failures.If concurrent initialization is possible in your extension lifecycle, consider adding synchronization or making
initSchemaidempotent with a guard flag.🔎 Optional error handling
if (missingBlocks.length > 0) { - await Promise.all( - missingBlocks.map((blockText) => - createBlock({ - parentUid: pageUid, - node: { text: blockText }, - }), - ), - ); + try { + await Promise.all( + missingBlocks.map((blockText) => + createBlock({ + parentUid: pageUid, + node: { text: blockText }, + }), + ), + ); + } catch (error) { + console.error('Failed to create one or more blocks:', error); + throw error; + } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/roam/src/data/blockPropsSettingsConfig.tsapps/roam/src/utils/initBlockPropSettings.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/data/blockPropsSettingsConfig.tsapps/roam/src/utils/initBlockPropSettings.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/data/blockPropsSettingsConfig.tsapps/roam/src/utils/initBlockPropSettings.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/data/blockPropsSettingsConfig.tsapps/roam/src/utils/initBlockPropSettings.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/data/blockPropsSettingsConfig.tsapps/roam/src/utils/initBlockPropSettings.ts
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/data/blockPropsSettingsConfig.tsapps/roam/src/utils/initBlockPropSettings.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 630
File: apps/roam/src/utils/settingsUsingBlockProps.ts:64-66
Timestamp: 2025-12-22T05:43:01.282Z
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.
📚 Learning: 2025-12-22T05:43:01.282Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 630
File: apps/roam/src/utils/settingsUsingBlockProps.ts:64-66
Timestamp: 2025-12-22T05:43:01.282Z
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/data/blockPropsSettingsConfig.tsapps/roam/src/utils/initBlockPropSettings.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/data/blockPropsSettingsConfig.tsapps/roam/src/utils/initBlockPropSettings.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/data/blockPropsSettingsConfig.ts
🧬 Code graph analysis (1)
apps/roam/src/utils/initBlockPropSettings.ts (1)
apps/roam/src/data/blockPropsSettingsConfig.ts (2)
TOP_LEVEL_BLOCK_PROP_KEYS(2-2)DG_BLOCK_PROP_SETTINGS_PAGE_TITLE(1-1)
⏰ 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 (2)
apps/roam/src/utils/initBlockPropSettings.ts (2)
1-7: LGTM!Imports are well-organized and follow the project's conventions. The use of existing
roamjs-componentslibrary aligns with the coding guidelines.
44-47: Sequential execution is correct.The logic correctly ensures the page exists before attempting to create blocks. The sequential await pattern is appropriate since
ensureBlocksExistrequires the page UID.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.