Skip to content

test(browser): let a route's handlers finish before unrouting - #831

Merged
devswha merged 1 commit into
mainfrom
test/drain-routes-before-unroute
Oct 11, 2026
Merged

devswha merged 1 commit into
mainfrom
test/drain-routes-before-unroute

Conversation

@devswha

@devswha devswha commented Oct 11, 2026

Copy link
Copy Markdown
Owner

Change

scripts/ui-regression.ts could fail with every check passing. On #824 the first CI attempt ended with

451 |     await route.fulfill({ response, json: body });
error: fulfill: Route is already handled!

after the check that owns that route had printed PASS; a rerun of the same commit passed.

The handlers in question await route.fetch() before they answer, and the app polls /api/machines on its own, so a call can be in flight whenever a check ends. page.unroute does not wait for it. When the route taken away is the page's last one, Playwright drops the page's interceptor and continues the request on the server side; the handler's own fulfill then finds the route handled. The rejection travels through Playwright's event listener, outside every assertion, and Bun exits 1.

scripts/drained-route.ts adds routeDrained(page, url, handler): it registers the route and returns an unroute that first waits for the calls in flight (15 s deadline), then removes that handler. It is used at:

  • ui-regression.ts: the held stale poll, the BG status check, and the plan check on the desktop page and on the phone (its context is closed right after).
  • tab-menu-regression.ts: the pending-selection check, which unrouted first and waited for its handlers afterwards.

Not used, on purpose: unrouteAll({ behavior: "wait" }) has the same race between two calls in flight (playwright-core 1.63) and removes unrelated routes; "ignoreErrors" and a .catch(() => {}) would hide a handler's real failure. A handler that throws still fails the unroute here.

Left alone: tab-menu-regression.ts:292 and terminal-copy-regression.ts:176 only race in a failure's cleanup, and five scripts close a context with a fetched route registered (a different error, if any). #806 moves the ui-regression.ts sites into ui-regression-palette.ts; it needs this carried over when it is rebased.

Validation

  • Reproduction (throwaway script, local Chrome): two requests in flight, then page.unroute → fulfill: Route is already handled! twice as unhandled rejections; the same with routeDrained → both fulfilled, no rejection; a handler that throws → the unroute rejects with that error.
  • TMPDIR=/tmp CHROME_PATH=<lockfile Chromium> bun run check run --build bun scripts/ui-regression.ts: exit 0, 115 PASS lines, on an isolated herdr (this includes checkTabMenu).
  • bun run typecheck: passed.
  • The diagnosis and the fix were reviewed read-only against playwright-core's source (Route.removeHandler continuing the request, _startHandling's assertion).

The original failure is rare (one of the last 26 failed CI runs), so a passing run does not prove its absence; the reproduction is what shows the race gone.

Release

Test scripts only; no app, contract or runtime change.

page.unroute does not wait for a handler still awaiting route.fetch().
Taking a page's last route away continues the request on the server
side, the handler's own fulfill then throws "Route is already handled!",
and that rejection lands outside every assertion: ui-regression.ts
exited 1 on #824 with every check passed.

routeDrained registers the route and returns an unroute that waits for
the calls in flight, with a deadline, and still fails on a handler's own
error.
@coderabbitai

coderabbitai Bot commented Oct 11, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 9 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: devswha/herdr-web-ui/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: b113252a-2dfe-44a8-9688-4c51cc81bcf0

📥 Commits

Reviewing files that changed from the base of the PR and between 3291eb4 and b802bb4.


📒 Files selected for processing (3)
  • scripts/drained-route.ts
  • scripts/tab-menu-regression.ts
  • scripts/ui-regression.ts

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@devswha
devswha merged commit 3c50127 into main Oct 11, 2026
9 checks passed
@devswha
devswha deleted the test/drain-routes-before-unroute branch October 11, 2026 19:04
devswha added a commit that referenced this pull request Oct 11, 2026
## Change

Release 0.4.7, from main at 80ca9bb (47 PRs since v0.4.6).

- `package.json` and `herdr-plugin.toml`: 0.4.7.
- CHANGELOG: `[Unreleased]` becomes `[0.4.7] - 2026-10-12`, with the
compare links.
- #513's entry moves out of the 0.4.0 section, where its branch's merge
left it, and gets its credit; it says the pace is a setting, off by
default (#834).
- A Maintenance section covers the PRs that had no entry: #743, #756,
#767, #795, #805, #810, #813, #821, #826, #831.
- The note under the Hermes entry that asked for a runtime bump at
release now says what a remote PC needs.
- `release-summaries.json`: 0.4.7 in en/ko/ja/zh/tr (6 new, 6 improved,
5 fixed). The Turkish lines are not checked by a native speaker.
- `REMOTE_BUNDLE_VERSION` 25 -> 26. Since v0.4.5's runtime the bridge
gained Hermes transcripts (#604), conversation pushes (#689) and
Grok/Antigravity activity (#816), so `remote-v26` is published before
the app release.

Not in this release: #606.

## Validation

- `bun scripts/release-notes.ts 0.4.7` renders the summaries and the
full changelog (235 lines).
- `bun run typecheck` and `bun run build`: pass.
- `TMPDIR=/tmp bun run test:unit`: one failure, `server/voice.test.ts`
("audio.webm" vs "clip.webm"), which fails the same way on unmodified
`main` on this Mac (Bun 1.4.0); CI runs Bun 1.4.2.
- Every PR merged since v0.4.6 was checked against the changelog
section: all 47 are linked.

## Release

After this merges and its CI on `main` passes: push `remote-v26`, wait
for its release, then run the Release workflow with `0.4.7`.
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