-
Notifications
You must be signed in to change notification settings - Fork 4
ENG-1105: zod schema for all the settings #664
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds two new TypeScript files to the Roam settings component: Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/roam/src/components/settings/utils/zodSchema.ts (3)
194-204: Recursive schema definition works but could be reordered for clarity.The
RoamBlockNodeSchemareferencesRoamBlockNodetype before its declaration. This works due to TypeScript hoisting, but placing the type definition before its usage would improve readability.🔎 Suggested reordering
+type RoamBlockNode = { + text: string; + children?: RoamBlockNode[]; +}; + const RoamBlockNodeSchema: z.ZodType<RoamBlockNode> = z.lazy(() => z.object({ text: z.string(), children: z.array(RoamBlockNodeSchema).optional(), }), ); - -type RoamBlockNode = { - text: string; - children?: RoamBlockNode[]; -};
50-54: Consider more specific types instead ofz.unknown()for better type safety.The
specification,template, andindexfields usez.array(z.unknown())orz.unknown(). While this provides flexibility, it reduces compile-time type checking. If the structure of these fields is known (e.g., from Roam API docs), consider defining stricter schemas.Also applies to: 56-60, 74-74
27-27: Remove unusedAttributesSchemaor refactor to use it.
AttributesSchemais defined on line 27 but never used. Theattributesfield inDiscourseNodeSchema(lines 68-71) duplicates the schema definition inline instead of reusing it. Either remove the unused schema or refactorattributesto useAttributesSchemafor consistency.apps/roam/src/components/settings/utils/zodSchema.example.ts (1)
359-377: Consider adding runtime validation tests for examples.These example constants serve as documentation, but they could also be used in unit tests to verify schema parsing works correctly. Consider adding tests that parse each example through its corresponding schema.
Example test approach:
import { QueryBlockSchema } from "./zodSchema"; import { exampleQueryBlockMinimal } from "./zodSchema.example"; test("exampleQueryBlockMinimal validates against QueryBlockSchema", () => { expect(() => QueryBlockSchema.parse(exampleQueryBlockMinimal)).not.toThrow(); });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/roam/src/components/settings/utils/zodSchema.example.tsapps/roam/src/components/settings/utils/zodSchema.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/utils/zodSchema.tsapps/roam/src/components/settings/utils/zodSchema.example.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/utils/zodSchema.tsapps/roam/src/components/settings/utils/zodSchema.example.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/utils/zodSchema.tsapps/roam/src/components/settings/utils/zodSchema.example.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/utils/zodSchema.tsapps/roam/src/components/settings/utils/zodSchema.example.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/utils/zodSchema.tsapps/roam/src/components/settings/utils/zodSchema.example.ts
🧠 Learnings (5)
📓 Common learnings
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
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
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/**/*.{js,ts,tsx,jsx,json} : Prefer existing dependencies from package.json when working on the Roam Research extension
📚 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/utils/zodSchema.tsapps/roam/src/components/settings/utils/zodSchema.example.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/utils/zodSchema.ts
📚 Learning: 2025-12-19T16:59:40.640Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-12-19T16:59:40.640Z
Learning: In this codebase, type assertions at system boundaries (e.g., Roam datalog queries) are acceptable when the output shape is known from the query structure, such as when using `as Array<[string, string]>` after datalog queries with `:find ?uid ?title` clauses.
Applied to files:
apps/roam/src/components/settings/utils/zodSchema.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/utils/zodSchema.example.ts
🧬 Code graph analysis (1)
apps/roam/src/components/settings/utils/zodSchema.example.ts (1)
apps/roam/src/components/settings/utils/zodSchema.ts (16)
CanvasSettings(243-243)SuggestiveRules(244-244)DiscourseNodeSettings(245-245)FeatureFlags(246-246)ExportSettings(247-247)PageGroup(248-248)SuggestiveModeGlobalSettings(249-251)LeftSidebarGlobalSettings(252-254)GlobalSettings(255-255)PersonalSection(256-256)LeftSidebarPersonalSettings(257-259)QueryFilter(260-260)QuerySettings(261-261)PersonalSettings(262-262)GithubSettings(263-263)RoamBlock(268-268)
🔇 Additional comments (5)
apps/roam/src/components/settings/utils/zodSchema.ts (2)
29-41: Well-designed helper functions for nullable defaults.The
stringWithDefaultandbooleanWithDefaulthelpers provide a clean pattern for handling nullable/optional fields with fallback values. Good abstraction.
243-268: LGTM - Type exports follow best practices.Using
z.infer<typeof Schema>for all type exports ensures types stay synchronized with schemas. The re-export ofRoamBlockNodeasRoamBlockprovides a cleaner public API.apps/roam/src/components/settings/utils/zodSchema.example.ts (3)
335-357: Excellent documentation for complex query block structure.The documentation comment clearly explains the hierarchical structure of query blocks with examples of each component. This will be valuable for developers working with the query system.
1-19: LGTM - Clean import structure.All type imports are correctly pulled from the schema file using
import type, which ensures no runtime overhead and proper tree-shaking.
330-333: Mock token pattern is appropriate for examples.The
"ghp_xxxxxxxxxxxxxxxxxxxx"pattern clearly indicates a placeholder. This is fine for example data.
mdroidian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start!
Some notes:
- Let's see if we can be more specific on the
unknown's - take a look through the samepage definitions for query builder, there's some good examples there
- are specific
uid's required? - let's consolidate the naming structure for personal/global settings
- right now there is kebab-case and Title Case (key-image / Enable Left Sidebar)
- note, for UI facing we've decided to go with Sentence case
c6aee0a to
0dbcfa6
Compare

Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.