[ENG-599] Discourse node shape #341
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
trangdoan982
force-pushed
the
eng-599-discourse-node-shape
branch
from
3301752 to
7054d19
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughIntroduces a custom DiscourseNode shape into the Obsidian–tldraw integration. Updates TL store initialization to accept plugin context and include custom shape utils. Adds robust asset/block-ref resolution helpers. Guards rendering when assetStore is missing. Implements a DiscourseNode shape util with metadata-driven rendering and subscriptions. Changes
Sequence Diagram(s)sequenceDiagram
participant View as TldrawViewComponent
participant TL as processInitialData
participant Store as TLStore
participant SU as DiscourseNodeUtil.configure(ctx)
View->>TL: processInitialData(data, assetStore, { app, canvasFile, plugin })
TL->>SU: configure(ctx)
TL->>Store: create({ records, assets, shapeUtils:[...defaultShapeUtils, SU] })
Note over Store: Store initialized with custom shape
sequenceDiagram
participant Shape as DiscourseNodeUtil
participant UI as DiscourseNodeContent
participant AS as resolveLinkedFileFromSrc
participant MC as app.metadataCache
participant DG as plugin.settings.nodeTypes
Shape->>UI: render({ app, canvasFile, plugin, src })
UI->>AS: resolveLinkedFileFromSrc(app, canvasFile, src)
AS-->>UI: TFile | null
UI->>MC: getFileCache(TFile)
MC-->>UI: frontmatter
UI->>DG: find nodeType by id from frontmatter
DG-->>UI: DiscourseNode | null
UI-->>Shape: Render with title/color/type
MC-->>UI: events (changed/resolved)
UI->>UI: refresh on relevant changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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: 1
🧹 Nitpick comments (14)
apps/obsidian/src/utils/assetStore.ts (2)
238-265: Use the shared parser for asset ids and support audio assets
- Parse asset ids consistently via
extractBlockRefIdto avoid subtle parsing drift.- Include
audio/*in directly resolvable media types.Apply this diff:
- const assetId = assetSrc.split(":")[1] as BlockRefAssetId; - if (!assetId) return null; + const blockRefId = extractBlockRefId(assetSrc); + if (!blockRefId) return null; + const assetId = `${ASSET_PREFIX}${blockRefId}` as BlockRefAssetId; @@ - if (mimeType.startsWith("image/") || mimeType.startsWith("video/")) { + if ( + mimeType.startsWith("image/") || + mimeType.startsWith("video/") || + mimeType.startsWith("audio/") + ) { return await this.proxy.getCached(assetId); }
143-163: Consider preserving the MIME type when creating Blob URLs
new Blob([assetData])defaults toapplication/octet-stream. Passing the known MIME type would improve compatibility for certain renderers/decoders.If you decide to implement this, one approach is to thread the
mimeTypedown togetCached(or detect it from the linked file’s extension) and construct the Blob with{ type: mimeType }. Happy to sketch a minimal change if you want to proceed.apps/obsidian/src/components/TldrawView.tsx (2)
5-5: Remove unused import
defaultShapeUtilsisn’t used in this module.-import { defaultShapeUtils, TLStore } from "tldraw"; +import { TLStore } from "tldraw";
138-141: Unreachable guard (duplicate asset store check)This guard will never run because you throw earlier in the same method for a missing
assetStore(Lines 133–134). Remove one of them; if you prefer the softer behavior, drop the earlierthrow.Apply this diff to keep the softer guard:
- if (!this.assetStore) - throw new Error("TldrawView not initialized: missing assetStore"); - if (!this.store) - throw new Error("TldrawView not initialized: missing store"); + if (!this.store) + throw new Error("TldrawView not initialized: missing store"); @@ if (!this.assetStore) { console.warn("Asset store is not set"); return; }apps/obsidian/src/utils/tldraw.ts (1)
16-16: Drop unused imports
AppandTFilearen’t referenced in this file.-import { App, Notice, TFile } from "obsidian"; +import { Notice } from "obsidian";apps/obsidian/src/components/TldrawViewComponent.tsx (1)
45-52: Memoize custom shape utils to avoid re-instantiation on each render
customShapeUtilsis recreated on every render; memoizing prevents unnecessary allocations and potential reconfiguration churn.Apply this diff:
- const customShapeUtils = [ - ...defaultShapeUtils, - DiscourseNodeUtil.configure({ - app: plugin.app, - canvasFile: file, - plugin, - }), - ]; + const customShapeUtils = useMemo( + () => [ + ...defaultShapeUtils, + DiscourseNodeUtil.configure({ + app: plugin.app, + canvasFile: file, + plugin, + }), + ], + [plugin.app, file, plugin], + );And add
useMemoto the React imports:// at line 1 import { useCallback, useEffect, useRef, useState, useMemo } from "react";apps/obsidian/src/utils/shapes/discourseNodeShapeUtils.ts (1)
33-41: Defend against unsetnodeTypesin settingsIf
plugin.settings.nodeTypesis undefined,.findwill throw. Default to an empty array.Apply this diff:
- return ( - plugin.settings.nodeTypes.find((nodeType) => nodeType.id === nodeTypeId) ?? - null - ); + return ( + (plugin.settings.nodeTypes ?? []).find( + (nodeType) => nodeType.id === nodeTypeId, + ) ?? null + );apps/obsidian/src/utils/shapes/DiscourseNodeShape.tsx (7)
74-86: Use FrontmatterRecord directly and drop unsafe castsYou’re force-casting frontmatter to/from
Record<string, unknown>. SincediscourseNodeShapeUtilsalready defines/exportsFrontmatterRecord, prefer that type end-to-end for clarity and safety.@@ - async getFrontmatter( + async getFrontmatter( shape: DiscourseNodeShape, ctx?: { app: App; canvasFile: TFile }, - ): Promise<Record<string, unknown> | null> { + ): Promise<FrontmatterRecord | null> { const app = ctx?.app ?? this.options.app; const file = await this.getFile(shape, ctx); if (!file) return null; - return getFrontmatterForFile(app, file) as unknown as Record< - string, - unknown - > | null; + return getFrontmatterForFile(app, file); } @@ async getDiscourseNodeType( shape: DiscourseNodeShape, ctx?: { app: App; canvasFile: TFile }, ): Promise<DiscourseNode | null> { const frontmatter = await this.getFrontmatter(shape, ctx); - const nodeTypeId = getNodeTypeIdFromFrontmatter( - frontmatter as unknown as Record<string, unknown> | null, - ); + const nodeTypeId = getNodeTypeIdFromFrontmatter(frontmatter); return getNodeTypeById(this.options.plugin, nodeTypeId); }Additionally add this type-only import at the top of the file:
// at the imports section import type { FrontmatterRecord } from "./discourseNodeShapeUtils";Also applies to: 87-97
125-147: Reset UI state when src is falsy or file resolution fails to prevent stale contentIf
srcbecomes null/empty or the linked file cannot be resolved, the previous title/type/color persist. Reset to defaults in those cases.useEffect(() => { let isCancelled = false; const run = async () => { try { - if (!src) return; + if (!src) { + if (isCancelled) return; + setLinkedFile(null); + setTitle("..."); + setNodeTypeName(""); + setNodeColor("transparent"); + return; + } const linked = await getLinkedFileFromSrc(app, canvasFile, src); - if (!linked) return; + if (!linked) { + if (isCancelled) return; + setLinkedFile(null); + setTitle("..."); + setNodeTypeName(""); + setNodeColor("transparent"); + return; + } if (isCancelled) return; setLinkedFile(linked); setTitle(linked.basename);
149-163: Avoid per-shape metadata subscriptions to reduce event churn on large canvasesEach node subscribes to
metadataCache(changedandresolved). With many nodes, this scales linearly and may cause redundant re-runs. Consider centralizing these subscriptions at the Tldraw view level and broadcasting a single signal (e.g., via a lightweight event bus or store) to all shapes, or pass a debounced notifier inDiscourseNodeUtilOptions.
47-59: Consider passing shape.id to HTMLContainer for better diagnostics and CSS targetingWhile optional, providing
id={shape.id}helps with debugging and targeted styling.- return ( - <HTMLContainer> + return ( + <HTMLContainer id={shape.id}> <DiscourseNodeContent
166-176: Verify Tailwind availability in the Obsidian viewThe
classNameuses Tailwind utilities (e.g.,flex,border-2). Ensure Tailwind (or equivalent styles) are present in the Obsidian canvas context; otherwise, these classes won’t take effect and you may need inline styles or a CSS module.
98-106: Type the relations return for future-proofing
getRelationsreturningunknown[]makes consumers fragile. If you have or plan a relation type (e.g.,{ type: string; target: TFile | string }[]), define and use it here. If not yet ready, add a TODO with the intended shape.
123-124: linkedFile state is currently unused
linkedFileis set but not used in rendering. If you plan to support actions (e.g., open on double-click), keep it; otherwise, remove to reduce state and re-renders.Also applies to: 133-134
📜 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 (6)
apps/obsidian/src/components/TldrawView.tsx(3 hunks)apps/obsidian/src/components/TldrawViewComponent.tsx(5 hunks)apps/obsidian/src/utils/assetStore.ts(5 hunks)apps/obsidian/src/utils/shapes/DiscourseNodeShape.tsx(1 hunks)apps/obsidian/src/utils/shapes/discourseNodeShapeUtils.ts(1 hunks)apps/obsidian/src/utils/tldraw.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
apps/obsidian/src/utils/shapes/discourseNodeShapeUtils.ts (1)
apps/obsidian/src/utils/assetStore.ts (1)
resolveLinkedFileFromSrc(69-77)
apps/obsidian/src/components/TldrawViewComponent.tsx (1)
apps/obsidian/src/utils/shapes/DiscourseNodeShape.tsx (1)
DiscourseNodeUtil(29-107)
apps/obsidian/src/utils/tldraw.ts (1)
apps/obsidian/src/utils/shapes/DiscourseNodeShape.tsx (2)
DiscourseNodeUtilOptions(23-27)DiscourseNodeUtil(29-107)
apps/obsidian/src/utils/shapes/DiscourseNodeShape.tsx (1)
apps/obsidian/src/utils/shapes/discourseNodeShapeUtils.ts (4)
getLinkedFileFromSrc(8-15)getFrontmatterForFile(17-23)getNodeTypeIdFromFrontmatter(25-30)getNodeTypeById(32-41)
🔇 Additional comments (13)
apps/obsidian/src/utils/assetStore.ts (2)
19-32: Solid, reusable block-ref extractorThe helper cleanly normalizes both
asset:and internal id inputs and centralizes the prefix logic. This will help keep parsing consistent across call sites.
69-77: Nice helper to unify src→TFile resolutionDelegating to
extractBlockRefIdkeeps the logic DRY and makes this reusable by shapes or other consumers.apps/obsidian/src/components/TldrawView.tsx (1)
109-114: Wiring ctx intoprocessInitialDatalooks correctPassing
{ app, canvasFile, plugin }matches the updated signature and enables your custom shape util configuration.apps/obsidian/src/utils/tldraw.ts (2)
41-50: Custom shape utils configuration is well-integratedExtending
defaultShapeUtilswithDiscourseNodeUtil.configure(ctx)and threading ctx through is the right approach to enable the custom shape in both code paths.
66-75: ConsistentshapeUtilsusage across store creation pathsGood attention to passing
shapeUtilswhether or notrecordsDataexists. This avoids mismatched shape configuration at initialization.apps/obsidian/src/components/TldrawViewComponent.tsx (2)
110-115: Fallback re-init path is correctly updatedThe error-path call to
processInitialDatanow passes ctx, keeping the custom shape configuration in sync after reload.
150-155: PassingshapeUtilsto the Tldraw component is essential for custom shapesThis ensures the editor can render and interact with the Discourse Node shape during runtime, not just at store creation.
apps/obsidian/src/utils/shapes/discourseNodeShapeUtils.ts (4)
8-15: Good delegation to the asset-store resolverKeeping link and block-ref resolution centralized in the asset utils avoids duplication and keeps behavior consistent.
17-23: Frontmatter retrieval is straightforward and safeReturning
nullwhen missing is a sensible default that composes well with the downstream helpers.
43-54: End-to-end mapping from src → file → frontmatter → nodeType is cleanThis composes your helpers well and gracefully returns
nullwhen data is missing.
56-61: Placeholder for relations is fineThe TODO is appropriate until the schema solidifies.
apps/obsidian/src/utils/shapes/DiscourseNodeShape.tsx (2)
1-178: Solid addition — clean shape util with clear data flowNice work on encapsulating file/metadata resolution in the util and rendering a simple, resilient HTMLContainer component. The effect lifecycle and error handling are sensible. With the small fixes above, this should be robust.
33-37: VerifyT.string.nullablesupport in your tldraw version
We’ve confirmed you’re on tldraw v3.14.2, but it’s unclear whether that release’s@tldraw/validateexports the instance‐method form (T.string.nullable). If you can’t locate any prior.nullableusage in your codebase or docs, please choose one of the following:• If the instance API is supported, update your prop schema to:
static props = { w: T.number, h: T.number, src: T.string.nullable, };and keep your TS type as
string | nullwith anulldefault.• Otherwise, use the always-available wrapper form:
static props = { w: T.number, h: T.number, src: T.nullable(T.string), };This also lets you default
srctonullwithout changing your TS definitions.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Good stuff so far. I think the biggest blocker is the useState/functions in the rendering of the shape. Let's try to store as much in shape props as possible to avoid performance bottlenecks.
| @@ -0,0 +1,172 @@ | |||
| import { BaseBoxShapeUtil, HTMLContainer, T, TLBaseShape } from "tldraw"; | |||
There was a problem hiding this comment.
There's lots of different ways to organize things, but one rule of thumb I use is "If it ends in tsx, it isn't a util" 😄
A negative of doing something like utils/shapes would be that it is kind of ambiguous. What does "shape" mean here? In this moment, we know it means "tldraw shape", but in 4 months it might not be that obvious to us.
I've been trying to think of a hierarchy for this to help make these decisions, and it goes something like this:
- if the function(s) are just used in a single location, co-locate in that file
- if the function(s) are used for only a specific component, co-locate in
components/{feature}folder - if the function(s) are used in multiple components in this
app/{app}, store inapp/{app}/utils - if the function(s) are used in multiple
apps/package, move topackage/{whateverIsApplicable}
There was a problem hiding this comment.
oooh these are helpful. Thank you
|
|
||
| const customShapeUtils = [ | ||
| ...defaultShapeUtils, | ||
| DiscourseNodeUtil.configure({ |
There was a problem hiding this comment.
Interesting choice to use .configure here. As I understand it, .configure is used to pass optional configuration that alters the default behavior of a shape. But here it is being used to pass required context for every instance of the shape. Another way would be to use a factory function to create the util.
There was a problem hiding this comment.
I think it's official API to parameterize a shape.
There was a problem hiding this comment.
Are these options ever going to be optional? Or are they basically always required each time this shape is instantiated?
There was a problem hiding this comment.
they are required each time this shape is instantiated. I don't think ShapeUtil.configure them here would make them optional. Out of all the props available for ShapeUtils I think we can only put these params in shapeUtils.options, bcz the other props are migrations, props, type, editor
There was a problem hiding this comment.
We could think outside of ShapeUtils for these types of required parameters.
|
How's this coming along? |
|
@mdroidian my bad i forgot to push my changes before going out this weekend. this simplified version is now ready for review |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Added some thoughts, and approved so you can be unblocked!
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx
Outdated
Show resolved
Hide resolved
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx
Outdated
Show resolved
Hide resolved
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx
Outdated
Show resolved
Hide resolved
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx
Outdated
Show resolved
Hide resolved
apps/obsidian/src/components/canvas/shapes/discourseNodeShapeUtils.ts
Outdated
Show resolved
Hide resolved
trangdoan982
force-pushed
the
eng-599-discourse-node-shape
branch
from
07275b7 to
bd9bbbe
Compare
* 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
* 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
* 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
* 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-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>
I don't have a Loom demo here because this PR mostly define the shape and its utility functions. For a demo of using the shape, see the follow up PR for creating new node shape
Summary by CodeRabbit
New Features
Bug Fixes
Refactor