Skip to content

Conversation

@roomote
Copy link

@roomote roomote bot commented Oct 8, 2025

Description

This PR attempts to address Issue #8563 where users reported that clicking on checkpoints does nothing. The issue was caused by the portal container element not being available when the CheckpointMenu component tried to render the popover.

Problem

  • Clicking on checkpoints had no effect - the restore menu popover was not appearing
  • The portal container element ('roo-portal') might not be immediately available in the DOM when components mount

Solution

  1. Enhanced useRooPortal hook:

    • Replaced useMount with useEffect for better lifecycle management
    • Added MutationObserver to detect when the portal element is dynamically added to the DOM
    • Properly cleans up the observer on unmount to prevent memory leaks
  2. Fixed CheckpointMenu component:

    • Added fallback to undefined when portal container is null
    • Ensures PopoverContent receives the correct prop type

Testing

  • ✅ All existing tests pass (CheckpointSaved.spec.tsx)
  • ✅ Components using useRooPortal (ModeSelector, ApiConfigSelector) remain unaffected
  • ✅ Type checking passes
  • ✅ Linting passes

Related Issues

Fixes #8563

Feedback and guidance are welcome!


Important

Fixes checkpoint restore popover issue by enhancing useRooPortal with MutationObserver and handling null containers in CheckpointMenu.

  • Behavior:
  • Hooks:
    • useRooPortal in useRooPortal.ts now uses useEffect instead of useMount.
    • Adds MutationObserver to detect dynamic addition of portal element.
    • Cleans up observer on unmount to prevent memory leaks.
  • Testing:
    • All existing tests pass, including CheckpointSaved.spec.tsx.
    • Components using useRooPortal (e.g., ModeSelector, ApiConfigSelector) remain unaffected.

This description was created by Ellipsis for a42a50e. You can customize this summary. It will automatically update as commits are pushed.

… not immediately available

- Updated useRooPortal hook to use MutationObserver to watch for portal element
- Added fallback to undefined when portal container is null in CheckpointMenu
- Fixes issue where clicking on checkpoints does nothing

Fixes #8563
@roomote roomote bot requested review from cte, jr and mrubens as code owners October 8, 2025 08:43
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Oct 8, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Oct 8, 2025
Copy link
Author

@roomote roomote bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review: scanning my own diff like a mirror with lint—objectivity buffer at 0%, sarcasm cache at 100%.


export const useRooPortal = (id: string) => {
const [container, setContainer] = useState<HTMLElement>()
const [container, setContainer] = useState<HTMLElement | null>(null)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Changing the hook to return null introduces a null/undefined mismatch for consumers. Prefer returning undefined to align with common DOM APIs and existing prop types, avoiding the need for portalContainer || undefined at call sites.

Suggested change
const [container, setContainer] = useState<HTMLElement | null>(null)
const [container, setContainer] = useState<HTMLElement | undefined>(undefined)

const element = document.getElementById(id)
if (element) {
setContainer(element)
observer.disconnect()
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] After the first match, the observer disconnects. If the portal node is later removed and re-inserted, container may reference a detached node. Consider re-subscribing on removal or checking container?.isConnected when consuming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. size:M This PR changes 30-99 lines, ignoring generated files.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[BUG] Checkpoint exists but clicking does nothing

3 participants