ENG-788 - Add Discourse Context Overlay to tldraw canvas #380
Conversation
…ionality - Introduced DISCOURSE_CONTEXT_OVERLAY_IN_CANVAS_KEY to manage the visibility of the Discourse Context overlay in the canvas. - Updated HomePersonalSettings to include a checkbox for enabling/disabling the overlay feature. - Integrated useMemo in DiscourseNodeUtil to optimize the overlay setting retrieval. - Added conditional rendering for the DiscourseContextOverlay in the canvas based on the overlay state.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
…handling - Updated DiscourseContextOverlay to accept uid prop, allowing for more flexible tag resolution. - Refactored getOverlayInfo call to utilize the new tag logic based on uid or tag. - Adjusted DiscourseNodeUtil to pass uid instead of deriving tag from it, enhancing clarity and functionality.
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds a canvas overlay feature for Discourse nodes gated by a new user setting. The overlay mounts on pointer enter, renders DiscourseContextOverlay, and now accepts either tag, uid, or both. Tag resolution supports UID-based lookup. A new user setting key is introduced and surfaced in HomePersonalSettings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant CN as CanvasNode (Discourse)
participant Set as Settings
participant UI as DiscourseContextOverlay
participant RG as Roam Graph
participant DF as getOverlayInfo
U->>CN: Pointer enter
CN->>Set: getSetting(DISCOURSE_CONTEXT_OVERLAY_IN_CANVAS_KEY)
alt setting enabled
CN->>CN: overlayMounted = true
CN->>UI: Mount with { uid, id }
par Resolve tag/uid
UI->>RG: getPageTitleByPageUid(uid)
UI->>RG: getPageUidByPageTitle(tag?) Note over UI,RG: Used if uid not provided
end
UI->>DF: getOverlayInfo(resolvedTag)
DF-->>UI: Overlay data (results, refs, score)
UI-->>CN: Render overlay
else disabled
CN-->>U: No overlay
end
Note over UI,CN: Overlay container stops pointer down propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/roam/src/components/DiscourseContextOverlay.tsx (1)
76-99: Fix stale-closure bug and add early-exit guard in getInfogetInfo closes over newTag/tagUid but they’re not in the dependency array, so the overlay can show stale data after the node’s uid/tag changes. Also add a short-circuit when resolution fails.
Apply this patch:
- const getInfo = useCallback( - () => - getOverlayInfo(newTag) - .then(({ refs, results }) => { - const discourseNode = findDiscourseNode(tagUid); - if (discourseNode) { - const attribute = getSettingValueFromTree({ - tree: getBasicTreeByParentUid(discourseNode.type), - key: "Overlay", - defaultValue: "Overlay", - }); - return deriveDiscourseNodeAttribute({ - uid: tagUid, - attribute, - }).then((score) => { - setResults(results); - setRefs(refs); - setScore(score); - }); - } - }) - .finally(() => setLoading(false)), - [tag, setResults, setLoading, setRefs, setScore], - ); + const getInfo = useCallback(() => { + if (!newTag || !tagUid) { + setResults([]); + setRefs(0); + setScore("-"); + setLoading(false); + return Promise.resolve(); + } + return getOverlayInfo(newTag) + .then(({ refs, results }) => { + const discourseNode = findDiscourseNode(tagUid); + if (discourseNode) { + const attribute = getSettingValueFromTree({ + tree: getBasicTreeByParentUid(discourseNode.type), + key: "Overlay", + defaultValue: "Overlay", + }); + return deriveDiscourseNodeAttribute({ + uid: tagUid, + attribute, + }).then((score) => { + setResults(results); + setRefs(refs); + setScore(score); + }); + } + }) + .finally(() => setLoading(false)); + }, [newTag, tagUid]);
🧹 Nitpick comments (6)
apps/roam/src/data/userSettings.ts (1)
8-9: New setting key looks goodConsistent naming and scoping with adjacent keys. Consider adding a short JSDoc describing where this key is consumed (canvas overlay) to aid discoverability.
apps/roam/src/components/settings/HomePersonalSettings.tsx (2)
17-23: Confirm intentional use of extensionSettings store vs extensionAPI.settingsExisting settings in this component persist via extensionAPI.settings, while the new overlay-in-canvas toggle uses getSetting/setSetting from extensionSettings. If this separation is intentional (e.g., because canvas code reads from extensionSettings), great—just confirming so we don’t end up with fragmented config stores. If not intentional, consider normalizing on one backing store.
182-201: UI wiring is correct; ensure live effect or communicate refresh requirementCheckbox correctly initializes/persists to DISCOURSE_CONTEXT_OVERLAY_IN_CANVAS_KEY. Note: the current canvas reader uses a memoized read of this key and won’t react to changes until re-rendered. I’m proposing a small change in DiscourseNodeUtil.tsx to re-read on hover so the toggle takes effect immediately. If you prefer not to change the canvas code, append “Must refresh after editing” to the Description here to set expectations.
apps/roam/src/components/DiscourseContextOverlay.tsx (2)
104-107: Trim effect dependencies to avoid redundant invokesrefresh already depends on getInfo; including both can cause duplicate re-runs. Keep only getInfo.
- useEffect(() => { - getInfo(); - }, [refresh, getInfo]); + useEffect(() => { + getInfo(); + }, [getInfo]);
116-139: Disable popover trigger when uid resolution failsPrevent passing an undefined uid into ContextContent and avoid enabling the button in that state.
- <ContextContent uid={tagUid} results={results} /> + <ContextContent uid={tagUid ?? ""} results={results} /> ... - minimal - disabled={loading} + minimal + disabled={loading || !tagUid}apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (1)
458-462: Make feature flag reactive on hover (read setting at interaction time)Currently isOverlayEnabled is read once via useMemo([]) and won’t reflect changes until a full remount. Re-read on pointer enter (cheap) so the toggle applies immediately without requiring a refresh.
- // eslint-disable-next-line react-hooks/rules-of-hooks - const isOverlayEnabled = useMemo( - () => getSetting(DISCOURSE_CONTEXT_OVERLAY_IN_CANVAS_KEY, false), - [], - ); + // eslint-disable-next-line react-hooks/rules-of-hooks + const [isOverlayEnabled, setIsOverlayEnabled] = useState<boolean>( + () => getSetting(DISCOURSE_CONTEXT_OVERLAY_IN_CANVAS_KEY, false), + ); ... - onPointerEnter={() => setOverlayMounted(true)} + onPointerEnter={() => { + setIsOverlayEnabled( + getSetting(DISCOURSE_CONTEXT_OVERLAY_IN_CANVAS_KEY, false), + ); + setOverlayMounted(true); + }}Also applies to: 516-516
📜 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/DiscourseContextOverlay.tsx(2 hunks)apps/roam/src/components/canvas/DiscourseNodeUtil.tsx(5 hunks)apps/roam/src/components/settings/HomePersonalSettings.tsx(2 hunks)apps/roam/src/data/userSettings.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-25T15:53:21.774Z
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#372
File: apps/roam/src/components/DiscourseNodeMenu.tsx:116-116
Timestamp: 2025-08-25T15:53:21.774Z
Learning: In apps/roam/src/components/DiscourseNodeMenu.tsx, when handling tag insertion, multiple leading hashtags (like ##foo) should be preserved as they represent user intent, not normalized to a single hashtag. The current regex /^#/ is correct as it only removes one leading # before adding one back, maintaining any additional hashtags the user intended.
Applied to files:
apps/roam/src/components/DiscourseContextOverlay.tsx
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/roam/src/components/DiscourseContextOverlay.tsx
🔇 Additional comments (2)
apps/roam/src/components/DiscourseContextOverlay.tsx (1)
81-89: Use of discourseNode.type as UID is correct heregetBasicTreeByParentUid(discourseNode.type) aligns with our convention that node.type stores the UID. Thanks for keeping that subtlety in mind.
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (1)
537-547: Overlay mount pattern is solidLazy-mount on hover, event suppression via onPointerDown, and unique id namespacing look good. No concerns.
mdroidian
deleted the
eng-788-add-discourse-context-overlay-to-tldraw-canvas
branch
BETA, WIP
https://www.loom.com/share/01f8363289bf43a3a7546dc3ff60212a
Summary by CodeRabbit
New Features
Settings
UX Improvements