Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis change adds DOM lifecycle management to ensure React components rendered into overlay mount nodes are properly cleaned up when those nodes are removed from the DOM, using a MutationObserver singleton pattern to automatically unmount and track mounted containers. Changes
Sequence DiagramsequenceDiagram
participant Render as Render Call
participant Tracker as Tracked Set
participant Observer as MutationObserver
participant DOM as DOM Node
participant Component as React Component
Render->>Tracker: Add parent element to set
Render->>Observer: Observe tracked elements
Note over DOM: Node exists in document
DOM->>Observer: Node disconnected
Observer->>Tracker: Check tracked elements
alt Element disconnected
Observer->>Component: Unmount React component
Component-->>Observer: Component unmounted
Observer->>Tracker: Remove element from set
Tracker-->>Observer: Element removed
end
alt No tracked containers remain
Observer->>Observer: Disconnect observer
Observer->>Tracker: Clear singleton reference
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/roam/src/components/DiscourseContextOverlay.tsx (1)
372-388: 💤 Low valueAdd explicit return type per coding guidelines.
The function is missing an explicit return type annotation.
Suggested fix
-const ensureObserver = () => { +const ensureObserver = (): void => {As per coding guidelines: "Use explicit return types for functions in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/roam/src/components/DiscourseContextOverlay.tsx` around lines 372 - 388, The function ensureObserver lacks an explicit TypeScript return type; update its declaration (const ensureObserver) to include a return type of void (e.g., const ensureObserver = (): void => { ... }) so it conforms to the project's coding guideline; keep the existing logic touching cleanupObserver, trackedContainers, MutationObserver and ReactDOM.unmountComponentAtNode unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/roam/src/components/DiscourseContextOverlay.tsx`:
- Around line 372-388: The function ensureObserver lacks an explicit TypeScript
return type; update its declaration (const ensureObserver) to include a return
type of void (e.g., const ensureObserver = (): void => { ... }) so it conforms
to the project's coding guideline; keep the existing logic touching
cleanupObserver, trackedContainers, MutationObserver and
ReactDOM.unmountComponentAtNode unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fb9db357-60ba-4add-ab5e-e04fa9fbc35f
📒 Files selected for processing (1)
apps/roam/src/components/DiscourseContextOverlay.tsx
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Approving this direction. Tracking the overlay containers and unmounting when Roam removes them is the right fix for the leak.
One preference: instead of adding a local trackedContainers + MutationObserver, I’d lean on roamjs-components/util/renderWithUnmount here. We already use it elsewhere in the extension, and it gives us the same DOM-removal cleanup while also registering/unregistering the React root and observer with the RoamJS extension registry. It also returns an explicit unmount function, which is useful when we intentionally remove overlays, e.g. when the setting is toggled off.
So I’m good with the lifecycle fix, but I’d prefer implementing it via renderWithUnmount to avoid maintaining a second cleanup mechanism.
maparent
deleted the
eng-1687-memory-leak-unmounting-the-discoursecontext-overlay-leaves
branch
2 participants
https://linear.app/discourse-graphs/issue/ENG-1687/memory-leak-unmounting-the-discoursecontext-overlay-leaves-it-dangling
Problem: https://www.loom.com/share/b7207d93abd942839133327de691aa86
Demonstrating the solution: https://www.loom.com/share/5d6a22757ca24905b44f921b87faa9ce
Again the diagnosis is mine but the solution is Claude's.
Summary by CodeRabbit