fix(app): close search dialog before navigating to prevent ghost overlay#2875
Conversation
🦋 Changeset detectedLatest commit: 438c40d The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughClose any open search-related native dialogs before route navigation and ensure overlay dialogs are explicitly closed on component unmount to avoid lingering open dialog state that can trap focus across route changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant SearchAdapter as OverviewSearchAdapter
participant DOM as Browser DOM
participant Router as Router
User->>SearchAdapter: trigger navigateTo(target)
SearchAdapter->>DOM: querySelectorAll('dialog[data-likec4-search]')
DOM-->>SearchAdapter: NodeList
SearchAdapter->>SearchAdapter: filter HTMLDialogElement && open===true
loop for each open dialog
SearchAdapter->>DOM: dialog.close()
end
SearchAdapter->>SearchAdapter: onClose()
SearchAdapter->>Router: navigate(target)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
3304ff8 to
5fb58d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/diagram/src/overlays/overlay/Overlay.tsx`:
- Around line 73-76: The cleanup currently calls dialogRef.current.close() which
can re-enter onClose via the dialog 'close' handler; before calling
dialog.close() set isClosingRef.current = true to prevent the
onCloseRef.current() path from running during teardown, then call
dialog.close(), and finally ensure the cleanup only clears the browser top-layer
entry (leave existing onClose handler intact). Update the effect teardown that
references dialogRef to set isClosingRef.current before invoking dialog.close()
and keep references to dialogRef and onCloseRef unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5e4baa8-97da-4eac-971b-38b523a67f15
📒 Files selected for processing (2)
packages/diagram/src/overlays/overlay/Overlay.tsxpackages/likec4/app/src/components/search/OverviewSearchAdapter.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/likec4/app/src/components/search/OverviewSearchAdapter.tsx
5fb58d3 to
954a5a7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/tests/overview-page.spec.ts (1)
79-86: Align the regression setup with the reported repro path (Ctrl+K).This test is solid, but on Line 81 it opens search via button click instead of the issue’s documented Ctrl+K path. Using the shortcut here would make the regression guard tighter for
#2353.Proposed tweak
- // Open search - await page.getByRole('button', { name: /Search/ }).click() + // Open search via the reported repro path + await page.keyboard.press('Control+k') await expect(page.getByRole('textbox', { name: /Search by title/ })).toBeVisible()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/overview-page.spec.ts` around lines 79 - 86, The test "search overlay closes after navigating to a view (`#2353`)" opens search via a button click but should use the Ctrl+K shortcut to match the reported repro; replace the call that clicks the Search button in the test with a keyboard shortcut invocation (e.g., page.keyboard.press('Control+K')) before asserting the search textbox visibility, keeping the subsequent navigation step (clicking the Landscape result and asserting URL) unchanged so the regression is guarded by the actual repro path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/tests/overview-page.spec.ts`:
- Around line 79-86: The test "search overlay closes after navigating to a view
(`#2353`)" opens search via a button click but should use the Ctrl+K shortcut to
match the reported repro; replace the call that clicks the Search button in the
test with a keyboard shortcut invocation (e.g.,
page.keyboard.press('Control+K')) before asserting the search textbox
visibility, keeping the subsequent navigation step (clicking the Landscape
result and asserting URL) unchanged so the regression is guarded by the actual
repro path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d97544d7-d0b6-4f90-b41d-3906f3989b5f
📒 Files selected for processing (3)
e2e/tests/overview-page.spec.tspackages/diagram/src/overlays/overlay/Overlay.tsxpackages/likec4/app/src/components/search/OverviewSearchAdapter.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/diagram/src/overlays/overlay/Overlay.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/likec4/app/src/components/search/OverviewSearchAdapter.tsx
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
954a5a7 to
bfb9945
Compare
|
Direct DOM operations from React might lead to unexpected results. Lets discuss on our call |
Replace direct DOM query (document.querySelectorAll) with Motion's onExitComplete callback to properly close the search dialog after exit animation. Uses a captured ref pattern to retain the dialog element beyond React unmount. Addresses review feedback from @davydkov. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bfb9945 to
438c40d
Compare
|
Replaced |
Summary
Fix for the search overlay persisting after navigating to a view via Ctrl+K search, trapping focus and blocking interaction with the destination diagram.
Closes #2353
Root Cause
When clicking a view in the search panel,
navigateTocalledonClose()(React state update) andnavigate()(route change) simultaneously. The route change could unmount theOverviewSearchcomponent before the<dialog>element was properly closed via the DOM API. SinceshowModal()places the dialog in the browser's top layer, unmounting without calling.close()first left the dialog's backdrop and focus trap active.Fix
Explicitly close all open search
<dialog>elements via the DOM before triggering the React state change and route navigation. This ensures the browser removes the dialog from the top layer before the component unmounts.Verified with Playwright
Files changed (1)
packages/likec4/app/src/components/search/OverviewSearchAdapter.tsx— 6 lines added