ENG-952 - inconsistent send nodes to canvas behavior #668
Conversation
…mprove canvas integration - Updated addToSelectedCanvas function to handle both legacy and new TLStore formats. - Introduced utility functions for managing discourse relations and nodes. - Enhanced error handling for canvas updates. - Export dialog now correctly updates the store schema and state.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughRestructures the Export flow to use TLStoreSnapshot-based store management for TLDraw content, detects and aborts on legacy stores, initializes/migrates TLStore snapshots for new canvases, performs shape operations against the snapshot store, and adds guarded Roam block updates that return explicit success/failure. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Export
participant TLStoreUtils as "TLStore Utils"
participant RoamAPI as "Roam API"
User->>Export: Start export/send to canvas
Export->>TLStoreUtils: Inspect input -> isTLStoreSnapshot?
alt Legacy store detected
Export->>User: Show legacy-format toast & abort
else TLStoreSnapshot or new canvas
Export->>TLStoreUtils: Initialize/create/migrate TLStoreSnapshot
TLStoreUtils-->>Export: tlStoreSnapshot ready
rect rgb(220, 240, 220)
note right of Export: Create/place shapes against tlStoreSnapshot.store
Export->>Export: Add discourse shapes, compute bounds, getPageKey
Export->>TLStoreUtils: loadSnapshot/commit changes
end
rect rgb(220, 220, 240)
note right of Export: Update Roam block with tldraw store/schema
Export->>RoamAPI: updateBlock(...)
alt updateBlock succeeds
RoamAPI-->>Export: success
Export-->>User: return true
else updateBlock fails
RoamAPI-->>Export: error
Export->>Export: log internalError
Export-->>User: return false
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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
Fix all issues with AI Agents 🤖
In @apps/roam/src/components/Export.tsx:
- Around line 425-442: The early exits in Export.tsx currently use "return
console.log(...)" which returns undefined; change these to return a boolean
(e.g., log then "return false") so the function's callers (like handleSendTo)
receive a consistent boolean result. Specifically, replace the two occurrences
around the tlStoreSnapshot check and the pageKey check (the block defining
getPageKey and the subsequent pageKey usage) to call console.log(...) or process
the log first and then "return false" to signal failure.
🧹 Nitpick comments (4)
apps/roam/src/components/Export.tsx (4)
267-303: Simplify legacy store detection.The
isLegacyStorecheck has redundant conditions sincetldrawis already defaulted to{}on line 265, making thenull/undefinedchecks unnecessary.🔎 Suggested simplification
- const isLegacyStore = - !isTLStore && - tldraw !== null && - tldraw !== undefined && - typeof tldraw === "object" && - Object.keys(tldraw).length !== 0; + const isLegacyStore = !isTLStore && Object.keys(tldraw).length !== 0;
416-418: Avoid mutating the input object.Line 416 mutates
tldraw.schemadirectly on what may be the original object fromblockProps. While this works in the!isTLStorebranch (wheretldrawis a fresh empty object or copy), consider assigning to a local variable to make the intent clearer and avoid potential side effects if the upstream data structure changes.🔎 Suggested refactor
The mutation is safe here since
tldrawdefaults to{}when not a TLStoreSnapshot, but the pattern could be cleaner:- tldraw.schema = tlStore.schema.serialize(); - tempTlStoreSnapshot = tlStore.getStoreSnapshot(); + tempTlStoreSnapshot = tlStore.getStoreSnapshot();Since
tempTlStoreSnapshotalready contains the schema fromgetStoreSnapshot(), the explicit schema assignment is redundant.
574-587: Consider limiting error context size.The error context includes the full
resultsarray andblockProps, which could be large and cause issues with error logging/reporting systems. Consider limiting to a sample or summary.🔎 Suggested improvement
internalError({ error: error as Error, type: "Failed to add to selected canvas", userMessage: "Failed to add to selected canvas. The team has been notified.", context: { pageUid, - results, - blockProps, + resultsCount: results.length, + sampleUids: results.slice(0, 5).map((r) => r.uid), }, });
249-251: Add explicit return type annotation.Per coding guidelines, functions should have explicit return types. After fixing the
console.logreturn statements, this function should returnPromise<boolean>.🔎 Suggested fix
- const addToSelectedCanvas = async (pageUid: string) => { + const addToSelectedCanvas = async (pageUid: string): Promise<boolean> => {Based on coding guidelines: "Use explicit return types for functions".
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/roam/src/components/Export.tsxapps/roam/src/components/canvas/useRoamStore.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/canvas/useRoamStore.tsapps/roam/src/components/Export.tsx
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/canvas/useRoamStore.tsapps/roam/src/components/Export.tsx
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/canvas/useRoamStore.tsapps/roam/src/components/Export.tsx
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/canvas/useRoamStore.tsapps/roam/src/components/Export.tsx
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/components/canvas/useRoamStore.tsapps/roam/src/components/Export.tsx
🧠 Learnings (2)
📚 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/canvas/useRoamStore.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/canvas/useRoamStore.tsapps/roam/src/components/Export.tsx
🔇 Additional comments (3)
apps/roam/src/components/canvas/useRoamStore.ts (1)
32-39: LGTM!Exporting
isTLStoreSnapshotis appropriate for enabling cross-module interoperability with the new TLStoreSnapshot-based export flow inExport.tsx. The type guard implementation is correct and well-typed.apps/roam/src/components/Export.tsx (2)
56-85: LGTM!The new imports are correctly structured to support the TLStoreSnapshot-based export flow. The separation between
tldrawand@tldraw/editorimports follows the established pattern.
635-638: LGTM!The
handleSendTofunction correctly awaits theaddToSelectedCanvasresult and uses early return to abort on failure, preventing the success toast from displaying when the canvas update fails.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @apps/roam/src/components/Export.tsx:
- Around line 305-418: The TLStore initialization block is duplicated; extract
it into a shared helper (e.g., createTLStoreForCanvas) that encapsulates calls
to getDiscourseRelations, createNodeShapeUtils, createAllRelationShapeUtils,
createAllReferencedNodeUtils, createAllRelationBindings,
createAllReferencedNodeBindings, createMigrations and createTLStore, and returns
the created tlStore and its snapshot (and optionally the serialized schema).
Replace the inline block in Export.tsx (which sets tldraw.schema and
tempTlStoreSnapshot) and the similar block in tldraw.tsx with a call to this
helper, assign tldraw.schema = returned.schema (or serialize tlStore.schema) and
tempTlStoreSnapshot = returned.tlStoreSnapshot, and update imports to use the
new utils/createTLStoreForCanvas export.
♻️ Duplicate comments (2)
apps/roam/src/components/Export.tsx (2)
249-251: Past review comment resolved.The function signature now correctly returns
Promise<boolean>, and the early return properly returnsfalseinstead of the previousreturn console.log(...)pattern that returnedundefined.
421-468: Past review comment resolved.The error handling now correctly uses
internalErrorwith explicitreturn falsestatements instead ofreturn console.log(...). This fixes the inconsistent return type issue flagged in the previous review.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/components/Export.tsx
🧰 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/Export.tsx
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/Export.tsx
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/Export.tsx
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/Export.tsx
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/components/Export.tsx
🧠 Learnings (2)
📚 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/Export.tsx
📚 Learning: 2025-10-18T18:58:16.100Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 504
File: apps/roam/src/utils/syncDgNodesToSupabase.ts:523-531
Timestamp: 2025-10-18T18:58:16.100Z
Learning: In `apps/roam/src/utils/syncDgNodesToSupabase.ts`, partial successes from `upsertNodesToSupabaseAsContent` and `addMissingEmbeddings` (indicated by numeric return values showing the count of successful operations) should NOT trigger backoff. Only complete failures (false) should trigger the exponential backoff mechanism. This design allows the sync process to continue making progress even when some items fail.
Applied to files:
apps/roam/src/components/Export.tsx
🧬 Code graph analysis (1)
apps/roam/src/components/Export.tsx (8)
apps/roam/src/components/canvas/Tldraw.tsx (2)
MAX_WIDTH(121-121)discourseContext(112-117)apps/roam/src/components/canvas/useRoamStore.ts (1)
isTLStoreSnapshot(32-39)apps/roam/src/utils/getDiscourseRelations.ts (1)
DiscourseRelation(12-19)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx (1)
AddReferencedNodeType(15-15)apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (1)
createNodeShapeUtils(138-155)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx (1)
createAllRelationShapeUtils(567-1048)apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationBindings.tsx (2)
createAllRelationBindings(45-51)createAllReferencedNodeBindings(36-44)apps/roam/src/components/canvas/DiscourseRelationShape/discourseRelationMigrations.ts (1)
createMigrations(19-148)
🔇 Additional comments (4)
apps/roam/src/components/Export.tsx (4)
53-85: LGTM!The new imports properly support the TLStore-based canvas integration. All imports are used in the refactored
addToSelectedCanvasfunction.
253-303: Excellent legacy store handling.The detection logic correctly identifies legacy stores and provides a user-friendly notification with a clickable link to the canvas page. Properly aborts the operation to prevent data corruption.
580-613: Excellent error handling.The try/catch block properly handles Roam API failures and returns explicit boolean success/failure signals. The error context includes all relevant debugging information.
661-664: LGTM!Properly checks the boolean return from
addToSelectedCanvasand aborts on failure, preventing a false success message to the user.
mdroidian
deleted the
eng-952-inconsistent-send-nodes-to-canvas-behavior
branch
https://www.loom.com/share/9b391029e9cf453eb728fb0fc72467e9
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.