ENG-1164 Ability to add a canvas page to the clipboard to view the nodes on that canvas #627
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughExtract the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/roam/src/utils/getAllReferencesOnPage.ts (1)
12-24: Add validation for type assertions.The code uses type assertions (
as Array<[string, string]>) without validating the actual structure of the data returned from Roam queries. If the query returns unexpected data, this could lead to runtime errors.Consider adding runtime validation before the type assertions:
🔎 Suggested validation approach
const validateQueryResult = (result: unknown): result is Array<[string, string]> => { return ( Array.isArray(result) && result.every( (item) => Array.isArray(item) && item.length === 2 && typeof item[0] === "string" && typeof item[1] === "string" ) ); }; // Then use it: const oldCanvasContent = window.roamAlphaAPI.data.fast.q(/* query */); if (!validateQueryResult(oldCanvasContent)) { console.error("Unexpected query result format for oldCanvasContent"); return []; }Also applies to: 25-38, 41-51
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/components/canvas/Clipboard.tsx(2 hunks)apps/roam/src/utils/getAllReferencesOnPage.ts(1 hunks)apps/roam/src/utils/hyde.ts(1 hunks)
🧰 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/utils/getAllReferencesOnPage.tsapps/roam/src/utils/hyde.tsapps/roam/src/components/canvas/Clipboard.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/utils/getAllReferencesOnPage.tsapps/roam/src/utils/hyde.tsapps/roam/src/components/canvas/Clipboard.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/utils/getAllReferencesOnPage.tsapps/roam/src/utils/hyde.tsapps/roam/src/components/canvas/Clipboard.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/utils/getAllReferencesOnPage.tsapps/roam/src/utils/hyde.tsapps/roam/src/components/canvas/Clipboard.tsx
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/utils/getAllReferencesOnPage.tsapps/roam/src/utils/hyde.tsapps/roam/src/components/canvas/Clipboard.tsx
🧠 Learnings (7)
📚 Learning: 2025-11-05T21:57:14.909Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 534
File: apps/roam/src/utils/createReifiedBlock.ts:40-48
Timestamp: 2025-11-05T21:57:14.909Z
Learning: In the discourse-graph repository, the function `getPageUidByPageTitle` from `roamjs-components/queries/getPageUidByPageTitle` is a synchronous function that returns a string directly (the page UID or an empty string if not found), not a Promise. It should be called without `await`.
Applied to files:
apps/roam/src/utils/getAllReferencesOnPage.tsapps/roam/src/utils/hyde.tsapps/roam/src/components/canvas/Clipboard.tsx
📚 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/utils/getAllReferencesOnPage.tsapps/roam/src/utils/hyde.tsapps/roam/src/components/canvas/Clipboard.tsx
📚 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/utils/getAllReferencesOnPage.tsapps/roam/src/utils/hyde.tsapps/roam/src/components/canvas/Clipboard.tsx
📚 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/utils/hyde.tsapps/roam/src/components/canvas/Clipboard.tsx
📚 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/** : Implement the Discourse Graph protocol in the Roam Research extension
Applied to files:
apps/roam/src/utils/hyde.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,jsx,js,css,scss} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Applied to files:
apps/roam/src/components/canvas/Clipboard.tsx
📚 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/**/*.{js,ts,tsx,jsx,json} : Prefer existing dependencies from package.json when working on the Roam Research extension
Applied to files:
apps/roam/src/components/canvas/Clipboard.tsx
🔇 Additional comments (4)
apps/roam/src/components/canvas/Clipboard.tsx (1)
42-42: LGTM! Import updated correctly.The import path has been correctly updated to reference the newly extracted
getAllReferencesOnPageutility module.apps/roam/src/utils/hyde.ts (1)
9-9: LGTM! Import added correctly.The import correctly references the newly extracted
getAllReferencesOnPagemodule, maintaining the same functionality while improving code modularity.apps/roam/src/utils/getAllReferencesOnPage.ts (2)
6-8: Add explicit return type annotation.As per coding guidelines, functions should have explicit return types for better type safety and documentation.
🔎 Proposed fix
-const getAllReferencesOnPage = async ( - pageTitle: string, -): Promise<{ uid: string; text: string }[]> => { +const getAllReferencesOnPage = async ( + pageTitle: string +): Promise<Array<{ uid: string; text: string }>> => {⛔ Skipped due to learnings
Learnt from: maparent Repo: DiscourseGraphs/discourse-graph PR: 534 File: apps/roam/src/utils/createReifiedBlock.ts:40-48 Timestamp: 2025-11-05T21:57:14.909Z Learning: In the discourse-graph repository, the function `getPageUidByPageTitle` from `roamjs-components/queries/getPageUidByPageTitle` is a synchronous function that returns a string directly (the page UID or an empty string if not found), not a Promise. It should be called without `await`.Learnt from: CR Repo: DiscourseGraphs/discourse-graph PR: 0 File: .cursor/rules/main.mdc:0-0 Timestamp: 2025-11-25T00:52:27.779Z Learning: Applies to **/*.{ts,tsx} : Use explicit return types for functions
11-39: Verify whether Canvas pages can contain regular block references alongside canvas shapes.The current implementation assumes Canvas pages contain only tldraw shapes and never checks for regular
[[page-reference]]links on Canvas pages (lines 11-39), whereas non-Canvas pages do query for block references (lines 40-52). If Canvas pages can have both content types, the logic should query both tldraw shapes and:block/refsfor Canvas pages and merge the results.
| @@ -0,0 +1,56 @@ | |||
| import getExtensionAPI from "roamjs-components/util/extensionApiContext"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused import.
The getExtensionAPI import is not used in this module (only in the commented-out code at line 4).
🔎 Proposed fix
-import getExtensionAPI from "roamjs-components/util/extensionApiContext";
import normalizePageTitle from "roamjs-components/queries/normalizePageTitle";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import getExtensionAPI from "roamjs-components/util/extensionApiContext"; | |
| import normalizePageTitle from "roamjs-components/queries/normalizePageTitle"; |
🤖 Prompt for AI Agents
In apps/roam/src/utils/getAllReferencesOnPage.ts around line 1, the import
`getExtensionAPI` is unused (only referenced in commented-out code); remove the
unused import statement to clean up the module and update any related linter
warnings, or if the commented code will be re-enabled later, move the import
next to that code and keep it commented as well.
| import getExtensionAPI from "roamjs-components/util/extensionApiContext"; | ||
| import normalizePageTitle from "roamjs-components/queries/normalizePageTitle"; | ||
|
|
||
| // getExtensionAPI(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code.
The commented line appears to be a debugging artifact and should be removed to keep the codebase clean.
🔎 Proposed fix
import normalizePageTitle from "roamjs-components/queries/normalizePageTitle";
-// getExtensionAPI();
-
const getAllReferencesOnPage = async (🤖 Prompt for AI Agents
In apps/roam/src/utils/getAllReferencesOnPage.ts around line 4, remove the
commented-out debugging artifact "// getExtensionAPI();" to clean up the
codebase; simply delete that line so only active code remains and run
linting/formatting to ensure no trailing whitespace.
|
@CodeRabbit and @mdroidian |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1767 🏁 Script executed: Length of output: 35492 🏁 Script executed: Length of output: 14814 @maparent, that's a sound approach. When interfacing with external systems like Roam's datalog queries, type assertions at the boundary are appropriate since the ✏️ Learnings added
|
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
We need to grab the defined canvas page format, but looks good other than that.
Curious if this would have side effects on Hyde? I expect Hyde to never deal with Canvas pages.
Should be fine.
Also: Could a Canvas page also have non-canvas node references?
Currently, all references are dg node shapes.
Also: Can canvas references include non-node pages? I do not (yet) filter them out. Would that be required?
Yes. But this is already filtered out elsewhere.
| ): Promise<{ uid: string; text: string }[]> => { | ||
| pageTitle = normalizePageTitle(pageTitle); | ||
| let referencedPages: Array<[string, string]> = []; | ||
| if (pageTitle.startsWith("Canvas/")) { |
There was a problem hiding this comment.
Canvas page format is defined in settings, so this could change. Let's make sure we grab the up to date format.
|
|
||
| const getAllReferencesOnPage = async ( | ||
| pageTitle: string, | ||
| ): Promise<{ uid: string; text: string }[]> => { |
There was a problem hiding this comment.
nit: uid;text; is Result[], which is commonly used in the repo
| pageTitle = normalizePageTitle(pageTitle); | ||
| let referencedPages: Array<[string, string]> = []; | ||
| if (pageTitle.startsWith("Canvas/")) { | ||
| const oldCanvasContent = window.roamAlphaAPI.data.fast.q( |
There was a problem hiding this comment.
nit: we should see if we can reuse some code with CanvasReferences
https://linear.app/discourse-graphs/issue/ENG-1164/ability-to-add-a-canvas-page-to-the-clipboard-to-view-the-nodes-on
Allow the references in a Canvas page to be accessible through the Canvas clipboard.
Did this through simply expanding getAllReferencesOnPage to handle canvas pages.
Curious if this would have side effects on Hyde? I expect Hyde to never deal with Canvas pages.
Also: Could a Canvas page also have non-canvas node references?
Also: Can canvas references include non-node pages? I do not (yet) filter them out. Would that be required?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.