ENG-1280: Port Discourse node: Query builder and editor component for block props #764
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
sid597
changed the title
sid597
changed the title
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe changes implement synchronization of query builder state to discourse node block properties. A new optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QueryEditor
participant DebounceTimer
participant IndexSchema
participant DiscourseNode
User->>QueryEditor: Update conditions/selections/custom
QueryEditor->>DebounceTimer: Clear previous timeout
DebounceTimer->>DebounceTimer: Start new 500ms timer
DebounceTimer->>IndexSchema: Validate query state
IndexSchema-->>IndexSchema: safeParse(data)
alt Validation succeeds
IndexSchema-->>QueryEditor: Valid data
QueryEditor->>DiscourseNode: setDiscourseNodeSetting("index", validatedData)
DiscourseNode-->>QueryEditor: Updated
else Validation fails
IndexSchema-->>QueryEditor: Parse error (silently ignored)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/roam/src/components/settings/DiscourseNodeIndex.tsx (1)
27-68:⚠️ Potential issue | 🟠 MajorNo error handling on the
createBlockpromise — spinner may hang indefinitely on failure.If
createBlockrejects,setShowQuery(true)is never called, leaving the user stuck on a<Spinner />with no recovery path. Consider adding a.catch()to handle the failure gracefully.Proposed fix
void createBlock({ parentUid: initialQueryArgs.conditionsNodesUid, node: { text: "clause", children: [ { text: "source", children: [{ text: DEFAULT_RETURN_NODE }], }, { text: "relation", children: [{ text: "is a" }], }, { text: "target", children: [ { text: node.text, }, ], }, ], }, }).then(() => { setDiscourseNodeSetting(node.type, ["index"], { conditions: [ { type: "clause", source: DEFAULT_RETURN_NODE, relation: "is a", target: node.text, }, ], selections: [], }); setShowQuery(true); - }); + }).catch((error) => { + console.error("Failed to create initial query block:", error); + setShowQuery(true); + });
🧹 Nitpick comments (1)
apps/roam/src/components/settings/DiscourseNodeIndex.tsx (1)
53-63: Minor inconsistency: initial index write omitscustomfield.The
IndexSchemanow includescustom: z.string().default(""), andQueryEditor's sync logic serializescustom. This initial write tosetDiscourseNodeSettingomits it. It works because Zod fills in the default on read, but addingcustom: ""here would be consistent with the schema and the sync logic inQueryEditor.Proposed fix
setDiscourseNodeSetting(node.type, ["index"], { conditions: [ { type: "clause", source: DEFAULT_RETURN_NODE, relation: "is a", target: node.text, }, ], selections: [], + custom: "", });
| <ExtensionApiContextProvider {...onloadArgs}> | ||
| {showQuery ? <QueryBuilder pageUid={parentUid} /> : <Spinner />} | ||
| {showQuery ? ( | ||
| <QueryBuilder pageUid={parentUid} discourseNodeType={node.type} /> |
There was a problem hiding this comment.
nit: The pageUid/parentUid is the index block which is a child block of node.type. Seeing as we are passing the parentUid, we can just get the node.type uid based on that, so we don't need to pass it through 2 components.
EG: in Query Editor, where you want to use node.type, use the information that is there to get it (which we know is the ancestor with no parent (or the page uid it is in))
There was a problem hiding this comment.
Ah never mind, we wouldn't want to make an extra query/check for every query instance
| if (!discourseNodeType) return; | ||
|
|
||
| const stripped: unknown = JSON.parse( | ||
| JSON.stringify({ conditions, selections, custom }, (key, value: unknown) => |
2 participants
https://www.loom.com/share/92b399a36f4d4e3f8e49a95e5c65661b
Summary by CodeRabbit