Skip to content

fix: skip I9kModal showModal when the dialog left the page - #39

Merged
ismail9k merged 2 commits into
mainfrom
posthog-self-driving/fixi9kmodal-skip-showmodal-when-the-e18691
Oct 26, 2026
Merged

ismail9k merged 2 commits into
mainfrom
posthog-self-driving/fixi9kmodal-skip-showmodal-when-the-e18691

Conversation

@posthog-eu

@posthog-eu posthog-eu Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Problem

  • One 9k.school learner on iOS Safari got an uncaught InvalidStateError ("Element is not connected") when they moved to the next lesson. The user saw nothing break. The cost is noise in error tracking.
  • show() runs after nextTick, from the open watcher and from the re-show in onNativeClose. If a route change removes the <dialog> first, the template ref still points at the old element, and showModal() throws.

Changes

  • show() returns early when the dialog is not connected to the document.
  • The native-dialog test stand-in for showModal() now throws on a disconnected element, the same as a browser does.
  • A new test detaches the dialog, then opens the modal. Without the guard, this test fails with InvalidStateError.

Risk

  • Low. A disconnected dialog cannot show, so the guard only stops a call that always throws.
  • The modal does not show again by itself if the same <dialog> element is attached again later. No current flow does this, so this PR does not add onActivated handling.

Verification

Check Result
npm test (full suite) pass
New test without the guard fails with InvalidStateError
npm run format:check pass
npm run lint pass
npm run typecheck pass
Library / Storybook / showcase builds not run

Follow-up

  • Update @9klabs/design in the 9k.school learner app after this merges.

Agent context

  • The inbox report was filed against the9klabs/promptdump, but promptdump does not use @9klabs/design. The fix belongs in this repository, so the PR is here.
  • The exact modal and route in the learner app are not confirmed, because its /_nuxt/ chunk has no source maps.

🤖 Generated with Claude Code

Created with PostHog Desktop from this inbox report.

A route change can remove the <dialog> before a show() queued by
nextTick runs. showModal() then throws InvalidStateError ("Element is
not connected"). show() now returns early for a disconnected dialog.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 67290182-7f62-443c-bc85-43c4a95fe52a

@ismail9k ismail9k left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed with Codex (code pass), checked against the code.

Findings: 0 Critical, 0 Important, 0 Minor.

Intent pass: skipped — the PR links no intent issue (no Closes # / Relates to #). Checked against the PR description instead: the guard and test do what it says.

Merge note — this branch conflicts with main. Since this branch was cut, main landed f259e60 (fix: never show I9kModal's dialog again once it unmounts), which fixes the same InvalidStateError for the route-change / <Suspense> unmount race with an unmounting flag in show() and onNativeClose(). The isConnected guard still adds value on top of it: it covers a dialog that leaves the document while the component stays mounted (a detached subtree, a <KeepAlive>-cached page getting an open update), where unmounting stays false. Resolve the conflict by keeping both checks:

if (unmounting || !element || !element.isConnected || isShown(element)) return;

main's showModal stand-in in tests/I9kModal.test.ts already throws on a disconnected element, so this PR's change to that mock is redundant — keep main's and add only the new test.

…l-when-the-e18691

Keep both guards in show(): main's `unmounting` flag for the Suspense
unmount race, and this branch's `isConnected` check for a dialog that
leaves the document while the modal stays mounted. Keep main's showModal
test stand-in, which already throws on a disconnected element.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ismail9k

Copy link
Copy Markdown
Member

Answering #39 (review):

  1. Merge conflict with main — fixed in 03830f5. origin/main is merged in, and show() now keeps both guards: if (unmounting || !element || !element.isConnected || isShown(element)) return;. Its comment now names the case isConnected alone covers: a dialog that leaves the document while the modal stays mounted. The showModal stand-in in the test is main's, which already throws on a disconnected element. This branch's new test sits next to main's <Suspense> regression test.

Verified on 03830f5: npm test (1122 passed), npm run format:check, npm run lint, npm run typecheck all pass.

@ismail9k
ismail9k marked this pull request as ready for review October 26, 2026 08:22
@ismail9k
ismail9k merged commit f427b56 into main Oct 26, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant