-
Notifications
You must be signed in to change notification settings - Fork 4
ENG-879 A node can have no specification or format #703
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
ENG-879 A node can have no specification or format #703
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. 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
🤖 Fix all issues with AI agents
In `@apps/roam/src/components/settings/NodeConfig.tsx`:
- Around line 234-243: The enabled assignment uses a malformed expression
causing an operator-precedence bug; replace the current logic in the enabled
declaration (which references getSubTree, getBasicTreeByParentUid, and
specificationUid) with a clear boolean check such as testing uid?.length !== 0
or coercing to boolean (e.g., !!(getSubTree(...)? .uid?.length)) so enabled
accurately reflects whether a uid exists; update that single line so it no
longer contains the useless "|| 0 !== 0" fragment.
🧹 Nitpick comments (1)
apps/roam/src/components/settings/NodeConfig.tsx (1)
233-275: MissingspecificationUidin useCallback dependency array.The
validatecallback referencesspecificationUidbut has an empty dependency array. WhilespecificationUidis stable for the lifetime of this component (derived fromnode.typewhich doesn't change), adding it to the dependency array is more correct and makes the intent explicit.♻️ Suggested fix
- }, []); + }, [specificationUid]);
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/roam/src/components/settings/DiscourseNodeSpecification.tsxapps/roam/src/components/settings/NodeConfig.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Follow responsive design principles
Prefertypeoverinterfacefor TypeScript
Use explicit return types for functions in TypeScript
Avoidanytypes in TypeScript 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
Prefer small, focused functions over inline code
Extract complex logic into well-named functions
Function names should describe their purpose clearly
Choose descriptive function names that make comments unnecessary
Break down complex operations into smaller, meaningful functions
Prefer early returns over nested conditionals for better readability
Prefer util functions for reusable logic and common operations
Add comments only when necessary; descriptive names should minimize the need for comments
Explain the why, not the what in comments, focusing on reasoning, trade-offs, and approaches
Document limitations, known bugs, or edge cases where behavior may not align with expectations
Files:
apps/roam/src/components/settings/DiscourseNodeSpecification.tsxapps/roam/src/components/settings/NodeConfig.tsx
{packages/ui/**,apps/**}/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use platform-native UI components first with shadcn/ui as a fallback
Files:
apps/roam/src/components/settings/DiscourseNodeSpecification.tsxapps/roam/src/components/settings/NodeConfig.tsx
apps/roam/**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (apps/roam/AGENTS.md)
Prefer existing dependencies from package.json
Files:
apps/roam/src/components/settings/DiscourseNodeSpecification.tsxapps/roam/src/components/settings/NodeConfig.tsx
apps/roam/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/roam/AGENTS.md)
Use BlueprintJS 3 components and Tailwind CSS for platform-native UI
Files:
apps/roam/src/components/settings/DiscourseNodeSpecification.tsxapps/roam/src/components/settings/NodeConfig.tsx
apps/roam/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/roam/AGENTS.md)
apps/roam/**/*.{ts,tsx,js,jsx}: Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj for API integration
Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU for extension development
Files:
apps/roam/src/components/settings/DiscourseNodeSpecification.tsxapps/roam/src/components/settings/NodeConfig.tsx
🧠 Learnings (6)
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/roam/src/components/settings/DiscourseNodeSpecification.tsx
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Applied to files:
apps/roam/src/components/settings/DiscourseNodeSpecification.tsx
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 372
File: apps/roam/src/components/DiscourseNodeMenu.tsx:116-116
Timestamp: 2025-08-25T15:53:21.799Z
Learning: In apps/roam/src/components/DiscourseNodeMenu.tsx, when handling tag insertion, multiple leading hashtags (like ##foo) should be preserved as they represent user intent, not normalized to a single hashtag. The current regex /^#/ is correct as it only removes one leading # before adding one back, maintaining any additional hashtags the user intended.
Applied to files:
apps/roam/src/components/settings/NodeConfig.tsx
📚 Learning: 2025-08-27T02:23:45.696Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 377
File: packages/ui/tsconfig.json:5-7
Timestamp: 2025-08-27T02:23:45.696Z
Learning: When reviewing TypeScript configuration changes, check if settings like "declaration" are inherited from base configs before suggesting to add them explicitly. Many monorepo packages extend base configurations that already include necessary compiler options.
Applied to files:
apps/roam/src/components/settings/NodeConfig.tsx
📚 Learning: 2025-06-22T10:40:21.679Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:15-16
Timestamp: 2025-06-22T10:40:21.679Z
Learning: In the getAllDiscourseNodesSince function in apps/roam/src/utils/getAllDiscourseNodesSince.ts, date validation is performed before the function is called, so additional date validation within the function is not needed.
Applied to files:
apps/roam/src/components/settings/NodeConfig.tsx
📚 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/NodeConfig.tsx
🪛 Biome (2.1.2)
apps/roam/src/components/settings/NodeConfig.tsx
[error] 238-238: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
🔇 Additional comments (3)
apps/roam/src/components/settings/DiscourseNodeSpecification.tsx (2)
15-19: LGTM - Clean callback prop addition.The optional
parentSetEnabledprop is properly typed and allows parent components to react to enabled state changes without tight coupling.
98-106: LGTM - Proper async callback invocation.The
parentSetEnabledcallback is correctly invoked after the async block operations complete, ensuring the parent receives accurate state notifications.apps/roam/src/components/settings/NodeConfig.tsx (1)
366-368: LGTM - Callback triggers re-validation on specification state change.The callback appropriately triggers validation when the specification enabled state changes. While the
enabledboolean parameter fromDiscourseNodeSpecificationis not used (validation re-fetches the state), this approach is acceptable since it ensures consistency with the actual persisted state.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
792bbdb to
09c7637
Compare
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.
Some nit, but this is probably fine as is because Migration of Roam Extension Settings to Section-Sharded Props (zod-typed) is coming soon.
apps/roam/src/components/settings/DiscourseNodeSpecification.tsx
Outdated
Show resolved
Hide resolved
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@CodeRabbit why is the review taking so long? |
|
Let me trigger a fresh review for you now. This should process the changes in your PR and provide specific feedback on the code modifications. ✅ Actions performedReview triggered.
|
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
https://linear.app/discourse-graphs/issue/ENG-879/a-node-can-have-no-specification-or-format
https://www.loom.com/share/22b8b9ced45e49c5a42efab46785d2b3
Added an error message if format and specification are both empty.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.