Skip to content

Clean up one-time wall startup after page teardown - #1313

Closed
nedtwigg wants to merge 3 commits into
mainfrom
audit-one-time-page-teardown
Closed

nedtwigg wants to merge 3 commits into
mainfrom
audit-one-time-page-teardown

Conversation

@nedtwigg

@nedtwigg nedtwigg commented Oct 21, 2026 •

Copy link
Copy Markdown
Member

Withdrawn after review: the shipping one-time page does not unmount its React root during a connection, and the real OneTimeClient.close contract resolves an in-flight connection as ended. The scripted tests exercised a late successful result that the real client prevents. These additional guards do not protect a reachable shipping flow and are being removed from the audit integration checkout.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Deploying mouseterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: e9b100a
Status: ✅  Deploy successful!
Preview URL: https://49219bdb.mouseterm.pages.dev
Branch Preview URL: https://audit-one-time-page-teardown.mouseterm.pages.dev

View logs

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I can't find anything in a shipping build that reaches the states these guards handle, so the change may not carry its weight:

  • Page teardown. one-time-app/main.tsx renders <OneTimeApp> into a root that is never unmounted. A real page teardown is navigation or reloadOnNewLink's reload, and React runs no effect cleanup then. Strict Mode's extra cleanup runs once at first mount, before any tap, so it can't land between connectOnce and mountRemoteWall. OneTimeApp.stories.tsx renders only the leaf screens. That leaves the tests as the only thing that unmounts the page mid-startup.
  • An ok after an ending. OneTimeClient.close() is documented to make a pending connectOnce resolve with ONE_TIME_ENDED_MESSAGE. close() sets #failure, and connectOnce re-checks it after the outcome and again in #race around the direct-path wait, so the ok return can't follow a close(). Cancel and unmount both call close(), so the first new check only ever sees a not-ok result. Before this PR that path already did nothing: end() returns early once endedRef is set, and after an unmount it's a setPhase on a dead component. The it.each test passes only because its scripted client resolves ok after close(), which the real client never does.

Unless a caller I missed unmounts the page, consider dropping mountedRef, the two checks, and the spec clause. Otherwise, name that caller in the PR and, if it's a real path, cover it with a test that drives the real client's close() contract rather than a scripted ok.

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The new late-teardown branch skips disposeAllSessions() so it won't clear "another page's" registry. But by the time mountRemoteWall resolves, installRemoteAdapter (in lib/src/remote/pocket-app/remote-wall.ts) has already run setPlatform(adapter) and disposeAllSessions(). So any other owner's registry is already gone and its platform replaced before this branch runs. The test passes because mountRemoteWall is mocked. Its expect(fake.disposeAllSessions).not.toHaveBeenCalled() therefore pins a protection that the real mount defeats.

This doesn't change behavior: the registry is empty here either way. The cost is a comment and a test assertion that describe an isolation the code doesn't provide. Either reword the comment as suggested and drop that assertion, or drop the late-teardown handling as the earlier review proposed.

Comment on lines +257 to +258
// This wall never rendered, so it owns no terminal entries. Release only
// its adapter; another page may already own the global terminal registry.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

installRemoteAdapter already replaced the platform and cleared the registry before this point, so this branch protects no other owner. Suggest stating only what holds:

Suggested change
// This wall never rendered, so it owns no terminal entries. Release only
// its adapter; another page may already own the global terminal registry.
// This wall never rendered, so it built no terminals; releasing its
// adapter is all that is left.

@nedtwigg nedtwigg closed this Oct 21, 2026
nedtwigg added a commit that referenced this pull request Oct 21, 2026
…idents

Drop rules AGENTS.md and running-tend already carry, and replace general
advice with an evidence-to-action table, a ledger template, and the
commands for combined validation and landing checks. Cite the incidents
behind the specific lessons (#1313, #1320, #1283/#1309), name the
harness's one-window and real-Tauri limits, and point visual checks at
Argos. Allow read-only sub-agents for per-feature review.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

2 participants