ENG-741 Allow Canvas to be opened in Sidebar #346
Conversation
…d sidebar contexts. Update utility functions to support sidebar canvas rendering.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughRefactors Tldraw canvas rendering to a generalized helper that patches target DOM regions using a shared data-attribute. Adds sidebar-specific rendering and detection. Updates initialization to render canvases in both main page and sidebar. Adjusts CSS to use a shared attribute and class selectors, exporting the attribute constant. Changes
Sequence Diagram(s)sequenceDiagram
participant Observer as PageTitleObserver
participant Utils as isCurrentPageCanvas / isSidebarCanvas
participant Tldraw as Tldraw module
participant Helper as renderTldrawCanvasHelper
participant DOM as DOM
participant Ctx as ExtensionApiContextProvider
Observer->>Utils: Check main-page canvas
alt Main-page is canvas
Observer->>Tldraw: renderTldrawCanvas({ title, onloadArgs })
Tldraw->>Helper: renderTldrawCanvasHelper(article selector, dims)
Helper->>DOM: Patch parent (set data-attr), wrap children
Helper->>Ctx: Render Tldraw with renderWithUnmount
end
Observer->>Utils: Check sidebar canvas
alt Sidebar is canvas
Observer->>Tldraw: renderTldrawCanvasInSidebar({ title, onloadArgs })
Tldraw->>Helper: renderTldrawCanvasHelper(sidebar selector, dims)
Helper->>DOM: Patch parent (set data-attr), wrap children
Helper->>Ctx: Render Tldraw with renderWithUnmount
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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: 3
🧹 Nitpick comments (2)
apps/roam/src/components/canvas/tldrawStyles.ts (1)
2-2: Consider using a data- attribute for HTML validityCustom non–data-* attributes are allowed in HTML5 and will work with attribute selectors, but using a data- attribute (e.g.,
data-dg-tldraw-canvas-wrapper) better communicates intent and avoids potential tooling or validator complaints. This would require updating both the constant and the selectors where it’s used.apps/roam/src/utils/initializeObserversAndListeners.ts (1)
80-82: Good: separate branches for article vs sidebar canvasesThe ordering avoids double-rendering. One note: both
renderTldrawCanvas*functions return cleanup functions, but they’re not captured/used here. IfrenderWithUnmountinternally handles DOM removal cleanly, this may be fine; otherwise, it could leak the wrapper or leave the attribute set. I’ve suggested a JS fix that scopes the patch to the specific H1’s container and removes the wrapper on cleanup.Would you like me to wire the returned cleanup functions into a lifecycle that unsets the attribute and removes the wrapper on page/sidebar unmount?
📜 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 (4)
apps/roam/src/components/canvas/Tldraw.tsx(3 hunks)apps/roam/src/components/canvas/tldrawStyles.ts(3 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(2 hunks)apps/roam/src/utils/isCanvasPage.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/roam/src/utils/initializeObserversAndListeners.ts (2)
apps/roam/src/utils/isCanvasPage.ts (2)
isCurrentPageCanvas(11-19)isSidebarCanvas(21-29)apps/roam/src/components/canvas/Tldraw.tsx (2)
renderTldrawCanvas(893-907)renderTldrawCanvasInSidebar(909-923)
apps/roam/src/components/canvas/Tldraw.tsx (1)
apps/roam/src/components/canvas/tldrawStyles.ts (1)
TLDRAW_DATA_ATTRIBUTE(2-2)
🔇 Additional comments (6)
apps/roam/src/utils/isCanvasPage.ts (1)
21-29: Sidebar detection utility looks good and mirrors current-page logicThe predicate is simple, consistent with
isCurrentPageCanvas, and reusesisCanvasPageas a gate. No issues spotted.apps/roam/src/components/canvas/tldrawStyles.ts (1)
22-24: Selector update from id to class is correctMatches the container’s updated className. This will ensure the “overflow visible” fix applies.
apps/roam/src/utils/initializeObserversAndListeners.ts (2)
9-12: New imports align with the split rendering pathsThe addition of
renderTldrawCanvasInSidebarpairs with the new sidebar detection logic.
18-18: Importing both canvas predicates is appropriateThis supports routing to the correct render path based on context. No issues.
apps/roam/src/components/canvas/Tldraw.tsx (2)
85-85: Importing the shared attribute constant is the right moveCentralizing the attribute value avoids drift between CSS and JS.
471-471: Container switched to class-based selector; matches CSS changeThe class
roamjs-tldraw-canvas-containeraligns with the updated CSS. No concerns.
… improved element selection.
https://www.loom.com/share/bf2e0a05c2b54f0fba6528eb02a489aa
Summary by CodeRabbit
New Features
Bug Fixes
Style