[ENG-604] Create node flow #387
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 Discourse node creation to the Obsidian Tldraw canvas: new panel and tool UI, drag-and-drop creation, a node-creation flow with a modal, asset store support to inject wikilink blockrefs into canvas files, tldraw data/file utilities and canvas creation helpers, minor shape update, and a white SVG icon constant. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Panel as DiscourseNodePanel
participant TLE as Tldraw Editor
participant Flow as openCreateDiscourseNodeAt
participant Modal as CreateNodeModal
participant DG as createDiscourseNode
participant Assets as addWikilinkBlockrefForFile
participant Vault as Obsidian Vault
User->>Panel: Click/Drag node type
Panel->>TLE: Compute page position
Panel->>Flow: openCreateDiscourseNodeAt({ plugin, canvasFile, tldrawEditor, position, initialNodeType? })
Flow->>Modal: Open with nodeTypes and callbacks
User->>Modal: Confirm node details
Modal->>DG: createDiscourseNode(plugin, nodeType, text)
alt file created
DG-->>Modal: TFile
Modal->>Assets: addWikilinkBlockrefForFile({ app, canvasFile, linkedFile })
Assets->>Vault: vault.process(write wikilink + ^blockref)
Assets-->>Modal: src = asset:obsidian.blockref.<id>
else no file
Modal-->>Flow: src = ""
end
Flow->>TLE: create shape (type: "discourse-node", props: { title, src, nodeTypeId })
TLE-->>User: Shape selected
note over Flow,TLE: On error: console.log + Notice
sequenceDiagram
actor User
participant TVC as TldrawViewComponent
participant TLE as Tldraw Editor
participant Flow as openCreateDiscourseNodeAt
User->>TVC: Drag node type onto canvas
TVC->>TLE: screenToPage(clientX, clientY)
TVC->>Flow: openCreateDiscourseNodeAt({ ..., position, initialNodeType })
Flow-->>TVC: Modal-driven creation (see above)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/obsidian/src/components/canvas/TldrawViewComponent.tsx (3)
100-107: Stop regenerating TLData UUID on every save (causes needless writes).
saveChangescreates a new UUID on each call, sostringifiedDataalways differs and you rewrite the file even when nothing changed.Apply these diffs to reuse a stable UUID parsed once from the file (and cached), and to build
newDataonly after reading the current content:const saveChanges = useCallback(async () => { - const newData = getTLDataTemplate({ - pluginVersion: plugin.manifest.version, - tldrawFile: createRawTldrawFile(currentStore), - uuid: window.crypto.randomUUID(), - }); - const stringifiedData = JSON.stringify(newData, null, "\t"); + // Read current content first to obtain a stable file-level UUID + const currentContent = await plugin.app.vault.read(file); + if (!currentContent) { + console.error("Could not read file content"); + return; + } + // Initialize and cache a stable UUID from the existing TLData block + if (!fileUuidRef.current) { + const m = currentContent.match( + new RegExp( + `${escapeRegExp(TLDATA_DELIMITER_START)}\\s*([\\s\\S]*?)\\s*${escapeRegExp(TLDATA_DELIMITER_END)}` + ), + ); + if (m?.[1]) { + try { + const parsed = JSON.parse(m[1]); + fileUuidRef.current = parsed?.meta?.uuid ?? null; + } catch {/* ignore */} + } + } + const newData = getTLDataTemplate({ + pluginVersion: plugin.manifest.version, + tldrawFile: createRawTldrawFile(currentStore), + uuid: fileUuidRef.current ?? (fileUuidRef.current = window.crypto.randomUUID()), + }); + const stringifiedData = JSON.stringify(newData, null, "\t"); - if (stringifiedData === lastSavedDataRef.current) { - return; - } + if (stringifiedData === lastSavedDataRef.current) return; - - const currentContent = await plugin.app.vault.read(file); - if (!currentContent) { - console.error("Could not read file content"); - return; - }Add these near the other refs:
const lastSavedDataRef = useRef<string>(""); + const fileUuidRef = useRef<string | null>(null);And define the small helper once in this file (top-level):
function escapeRegExp(s: string) { return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); }Also applies to: 112-118
127-135: Escape delimiters in RegExp and fix product name.Avoid raw interpolation in regex; also use “Tldraw” casing.
- const verifyMatch = verifyContent.match( - new RegExp( - `${TLDATA_DELIMITER_START}\\s*([\\s\\S]*?)\\s*${TLDATA_DELIMITER_END}`, - ), - ); + const verifyMatch = verifyContent.match( + new RegExp( + `${escapeRegExp(TLDATA_DELIMITER_START)}\\s*([\\s\\S]*?)\\s*${escapeRegExp(TLDATA_DELIMITER_END)}` + ), + ); ... - throw new Error("Failed to verify saved TLDraw data"); + throw new Error("Failed to verify saved Tldraw data");
141-156: Escape delimiters in the recovery RegExp.Same regex safety improvement for the reload path.
- const match = fileContent.match( - new RegExp( - `${TLDATA_DELIMITER_START}([\\s\\S]*?)${TLDATA_DELIMITER_END}`, - ), - ); + const match = fileContent.match( + new RegExp( + `${escapeRegExp(TLDATA_DELIMITER_START)}([\\s\\S]*?)${escapeRegExp(TLDATA_DELIMITER_END)}` + ), + );
🧹 Nitpick comments (9)
apps/obsidian/src/constants.ts (1)
75-75: Confirm debounce interval is appropriate for canvas size.500ms may be overly chatty on large canvases. If this governs autosave/debounced persistence, consider 1000–1500ms to reduce disk churn; otherwise, keep as-is.
apps/obsidian/src/components/canvas/stores/assetStore.ts (2)
32-38: Precompute link text safely; minor: guard unusual titles.
fileToLinktextis correct. If a filename contains]or newline, Obsidian handles it, but adding a simple sanitize ensures robust wikilinks.- const linkText = app.metadataCache.fileToLinktext( + const rawLinkText = app.metadataCache.fileToLinktext( linkedFile, canvasFile.path, ); - const content = `[[${linkText}]]\n^${blockRefId}`; + const linkText = rawLinkText.replace(/\n/g, " ").replace(/\]\]/g, ""); + const content = `[[${linkText}]]\n^${blockRefId}`;
53-54: Return a strongly typed asset src.Narrow the return type for easier call-site validation and autocomplete.
- return `asset:${ASSET_PREFIX}${blockRefId}`; + return `asset:${ASSET_PREFIX}${blockRefId}` as const;apps/obsidian/src/components/canvas/DiscourseNodeTool.ts (2)
6-8: Reset cursor on exit to avoid lingering cross cursor.Set the cursor back when leaving the tool.
override onEnter = () => { this.editor.setCursor({ type: "cross" }); }; + override onExit = () => { + this.editor.setCursor({ type: "default" }); + };
1-1: Import source matches TLDraw v3; keep imports consistent project-wide.Other files import
Editorfromtldraw. Prefer a single source (tldrawor@tldraw/editor) across the codebase to avoid duplication/mismatched versions.apps/obsidian/src/components/canvas/DiscourseNodePanel.tsx (2)
31-33: Handle empty node type lists gracefully.If
plugin.settings.nodeTypesis empty, render a hint instead of an empty list.- const nodeTypes = plugin.settings.nodeTypes; + const nodeTypes = plugin.settings.nodeTypes ?? [];And conditionally render a placeholder message.
41-50: Tighten DnD semantics.If you don’t support move, set
effectAllowed = "copy"and add a plain-text fallback for cross-app drops.- if (e.dataTransfer) { - e.dataTransfer.effectAllowed = "copyMove"; - } + if (e.dataTransfer) e.dataTransfer.effectAllowed = "copy"; + e.dataTransfer?.setData("text/plain", nodeType.id);apps/obsidian/src/components/canvas/utils/nodeCreationFlow.ts (1)
57-60: Improve failure notice with error context.Surface basic context to help users/debug logs.
- new Notice("Failed to create discourse node"); + new Notice("Failed to create discourse node. See console for details.");apps/obsidian/src/components/canvas/TldrawViewComponent.tsx (1)
53-53: Use a browser-safe timeout type.
NodeJS.Timeoutcan be incorrect in browser builds; prefer the inferred type.- const saveTimeoutRef = useRef<NodeJS.Timeout>(); + const saveTimeoutRef = useRef<ReturnType<typeof setTimeout>>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
apps/obsidian/src/components/canvas/DiscourseNodePanel.tsx(1 hunks)apps/obsidian/src/components/canvas/DiscourseNodeTool.ts(1 hunks)apps/obsidian/src/components/canvas/TldrawViewComponent.tsx(5 hunks)apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx(0 hunks)apps/obsidian/src/components/canvas/shapes/discourseNodeShapeUtils.ts(0 hunks)apps/obsidian/src/components/canvas/stores/assetStore.ts(1 hunks)apps/obsidian/src/components/canvas/utils/nodeCreationFlow.ts(1 hunks)apps/obsidian/src/components/canvas/utils/tldraw.ts(1 hunks)apps/obsidian/src/constants.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/obsidian/src/components/canvas/shapes/discourseNodeShapeUtils.ts
- apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx
🧰 Additional context used
🪛 ast-grep (0.38.6)
apps/obsidian/src/components/canvas/utils/tldraw.ts
[warning] 190-192: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
${TLDATA_DELIMITER_START}([\\s\\S]*?)${TLDATA_DELIMITER_END},
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (3)
apps/obsidian/src/components/canvas/TldrawViewComponent.tsx (2)
79-91: Theming-aware icon updates look solid.Listener registration and cleanup via
offrefare correct; data URL generation is efficient.
236-248: Tool wiring LGTM; confirm icon key binding.The custom tool id matches usage; UI integration is consistent. Verify that
assetUrls.icons.discourseNodeIconis recognized by your Tldraw version forTldrawUiMenuItem.If needed, search your Tldraw version docs for “custom icons via assetUrls.icons and TldrawUiMenuItem.icon”.
Also applies to: 249-284
apps/obsidian/src/components/canvas/utils/tldraw.ts (1)
156-179: Canvas creation flow looks good.Folder creation, unique naming, file write, and opening the file are handled correctly; error surfacing via Notice is appropriate.
trangdoan982
force-pushed
the
08-28-eng-604_create_node_flow
branch
2 times, most recently
from
9a430bf to
153be6d
Compare
apps/obsidian/src/components/canvas/shapes/discourseNodeShapeUtils.ts
Outdated
Show resolved
Hide resolved
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Good stuff! A few change requests.
trangdoan982
force-pushed
the
08-28-eng-604_create_node_flow
branch
2 times, most recently
from
6ee110d to
1b445e7
Compare
|
@mdroidian ready for re-review. I saw that the Vercel build has failed but running turbo build locally i don't see any errors. can you help me check? |
|
Yeah, very weird, as this PR isn't changing any website stuff. Seeing as this isn't merging into main, maybe we ignore it for now (I don't love this idea, but we can catch it again when merging the tldraw PR) |
| ); | ||
| }; | ||
|
|
||
| const NodeTypeButton = ({ |
There was a problem hiding this comment.
The hover bg is off, it is too narrow
It should look like this
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
I cannot add a node to the canvas via the tool.
As a user, I would expect to:
- click on dg logo
- click on claim
- click on the canvas where I want to add my claim
trangdoan982
force-pushed
the
08-28-eng-604_create_node_flow
branch
from
1b445e7 to
3a43d20
Compare
|
@mdroidian pushed up the changes. my bad i was focusing too hard on the dragging interaction and forgot about the clicking 😅 |
| useEffect(() => { | ||
| if (!focusedNodeTypeId) return; | ||
|
|
||
| const handlePointerDown = (e: PointerEvent) => { |
There was a problem hiding this comment.
We should use the "tool" part of the tldraw SDK for this https://tldraw.dev/docs/tools
There are a few benefits of using the recommended SDK from tldraw. Mostly it is the fact that they have already done most of the hard work when it comes to different use cases of screens, devices, interactions, etc etc.
In this case, we can use their "tool" SDK to handle the "on click". Because that's what all of their shapes use. If we use their SDK, we don't have to create all of these if's , useeffects and listeners to handle edge cases and get ourself twisted in our code.
We also use the keyboard shortcuts feature of the tools to allows users to use the same keyboard shortcuts to activate said tools.
Unless there's a specific reason you'd were thinking that not using their SDK would be a good idea? If so, I'm all ears 👍 If not, feel free to defer this to another ticket, but we should update it pretty quickly.
There was a problem hiding this comment.
yes good point. i updated the implementation so that it uses the onPointerDown from StateNode.
for keyboard shortcuts, because we don't have that setting in Obsidian yet I'm gonna defer it. Created a ticket to track this in the backlog ENG-817: Create keyboard shortcut for the DiscourseNodeTool
trangdoan982
force-pushed
the
08-28-eng-604_create_node_flow
branch
from
3a43d20 to
0d69cd7
Compare
* eng-604: create node flow * pwd
* eng-604: create node flow * pwd
* eng-604: create node flow * pwd
* eng-604: create node flow * pwd
* [ENG-495] Tldraw obsidian setup (#285) * cleaned * sm * address PR comments * [ENG-598] Data persistence for tldraw (#303) * data persistence to the file * error handling * address PR comments * address some PR comments * address other PR comments * address PR comments * [ENG-624] TLDraw Obsidian asset store (#326) * current state * works now * clean up * address PR comments * address PR reviews * cleanup * fix styling issues * address PR comments * correct styles * [ENG-599] Discourse node shape (#341) * current state * works now * clean up * address PR comments * address PR reviews * fix styling issues * latest progress * update new shape * shape defined * address PR comments * sm address PR review * current progress * reorg * address other PR comments * clean * simplify flow * address PR comments * [ENG-604] Create node flow (#387) * eng-604: create node flow * pwd * [ENG-658] Add existing node flow (#389) * eng-658-add-existing-nodes-flow * address PR comments * small changes * [ENG-601] Create settings for canvas and attachment default folder (#338) * add new settings * small add * ENG-600: Discourse Relation shape definition (#408) * ENG-605: Add new relation flow (#411) * [ENG-603] Add existing relations (#412) https://www.loom.com/share/3641f2a642714b0d849262344e8c6ee5?sid=0614c657-e541-4bfd-92df-9b1aa60945b6 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Added a Relations overlay on the canvas that shows a “Relations” button when a discourse node is selected. - Introduced a Relations panel to view and manage relations for the selected node, including adding or removing links, with clear loading/error states. - Overlay appears above the canvas without disrupting existing tools. - Chores - Consolidated relation-type lookup into shared utilities and updated imports. No user-facing changes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> * [ENG-844] Add color setting for relation types (#429) * add color setting * address PR reviews * address PR commens * fix icons * ENG-812 Update of database cli tools (#401) * eng-812 : Update database cli tools: supabase, vercel, cucumber. * newer cucumber constrains node * [ENG-495] Tldraw obsidian setup (#285) * cleaned * sm * address PR comments * [ENG-598] Data persistence for tldraw (#303) * data persistence to the file * error handling * address PR comments * address some PR comments * address other PR comments * address PR comments * switch to pnpm * delete wrong rebase file * fix pnpm lock * fix type checks * address all the PR comments * delete redundant files * fix types * shift click to open file on the right split (#485) * address PR comments * final lint cleanup --------- Co-authored-by: Marc-Antoine Parent <maparent@acm.org>
https://www.loom.com/share/54a51ece280d411aa4da615fc08bc137
Summary by CodeRabbit
New Features
Style
Bug Fixes