fix: preserve overlay lifecycle and keyboard access - #5725
OskarEichler wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 9fcd60d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Resolved the draft blocker by updating the DOM snapshots for the intentional iframe title and native-button markup. I also added focused regressions for pending-message replacement, dismissal before load, reopening and iframe Escape handling, Trusted Types policy reuse, and encoded editor paths. The full browser overlay suite passes 35/35, the new lifecycle suite passes 3/3, all lint/type/format/spelling checks are clean, and the syntax-error fixture is restored. I’ve marked the PR ready for review. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. WalkthroughThe overlay now discards stale pending renders, reuses its Trusted Types policy, restores focus after dismissal, and handles Escape in the iframe. It adds iframe accessibility attributes and renders the editor action as a button. Editor filenames are URL-encoded. New client tests cover lifecycle behavior, policy reuse, and editor requests. E2E snapshots reflect the updated iframe and button markup. A patch release changeset was added. Merge Risk: ⚪ Minimal · up to This localized overlay lifecycle and accessibility update preserves existing security controls and adds focused coverage, with the stated browser tests, build, lint, and validation checks passing; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
client-src/overlay.jsESLint failed to execute (timeout). test/client/overlay-lifecycle.test.jsESLint skipped: the matched ESLint configuration already failed (timeout). 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 |
* fix: client, overlay, progress and server lifecycle defects Consolidates the actionable fixes from #5724, #5725, #5726, #5727, #5728, #5729, #5730 and #5732. Client: - honor `client.progress: "linear" | "circular"`; the resource query only recognized `"true"`, so both visual modes were silently disabled - parse the resource query with full `key=value` semantics (encoded keys, `+` as space, `=` inside values, malformed escapes ignored) - decode credentials taken from the current script tag so `formatURL` does not encode them twice - apply `client.overlay.warnings` / `.errors` filter functions to what the overlay renders, not only to the decision to render - apply the reconnect limit before the first connection attempt, so `client.reconnect: false` no longer retries when the socket never opens Overlay: - reuse the Trusted Types policy instead of re-creating it per open, which throws under a `trusted-types` CSP - keep only the newest queued render so messages are not duplicated when two batches arrive before the iframe loads - re-register the Escape handler on open; it was removed on first dismiss and never restored - encode the `open-editor` file name, render openable entries as buttons, and restore focus on dismiss Progress: - style the linear bar through `#progress`; the rules targeted `#bar`, which no template emits - clear the `disappear` class and the pending hide timer when a new build starts, so the indicator reappears - skip redundant `attributeChangedCallback` work and expose progressbar ARIA state and reduced-motion styles Server: - reject from `start()` on an occupied port or IPC path instead of throwing from an event handler, and release what setup allocated - fix `bonjour` protocol reporting (`||` bound tighter than the ternary) - only install the WebSocket `upgrade` listener in no-server mode, and remove it on close - skip incomplete interfaces and CIDRs in `findIp`, and hand the listening socket an unbracketed IPv6 address - wait for pending startup before shutting down in plugin mode - build the asset report from `toJson` with only the fields it prints, construct the `serve-index` middleware once, and serialize each broadcast once instead of per client - export `BaseServer` and type `webSocketServer.type` as its constructor Examples: - repair `api/plugin` (CommonJS in an ESM package), `ipc` (`http-proxy`), `proxy` and `general/proxy-simple` (options removed in v5) - serve the shared layout assets through `express.static`, which also works for the `hono` example, and read each README relative to its own directory - restore host and cross-origin checks in the `hono` example, whose `setupMiddlewares` replaces the built-in stack Co-authored-by: Oskar Eichler <62393985+OskarEichler@users.noreply.github.com> * fix: address review on the hono example and changeset bump - the example's cross-origin middleware must return early for a valid host, matching the built-in one; it was tagging every response with `Cross-Origin-Resource-Policy: same-origin` - bump `minor`: exporting `BaseServer` adds public API Co-authored-by: Oskar Eichler <62393985+OskarEichler@users.noreply.github.com> * test: cover the client fixes in a real browser Six puppeteer cases, each verified to fail against main's client: - linear progress renders a 4px green bar (main: 0px — the rules were keyed on `#bar`, which no template emits) and reports itself enabled in the startup banner (main: "Progress disabled") - circular progress renders the ring and its ARIA state - the indicator comes back on a later rebuild instead of staying faded - Escape dismisses a second overlay: `invalid` fires a DISMISS on every rebuild, so on main the first fix-then-break cycle tore down the key handler for the rest of the session - the overlay reopens under an enforced `trusted-types` policy name, where asking for the same name twice is a TypeError - a warning filter decides what the overlay renders, not just whether it opens Co-authored-by: Oskar Eichler <62393985+OskarEichler@users.noreply.github.com> * test: stop the heartbeat case racing its own compilation The 100ms sweep terminates a client that has not ponged yet, and a compilation can block the event loop for longer than that, so a healthy client was dropped before the `ok` stats message reached it. Seen on the macOS Node 24 shard; the same file was stabilized for neighbouring races in #5733. Wait for the build to settle before connecting. Co-authored-by: Oskar Eichler <62393985+OskarEichler@users.noreply.github.com> --------- Co-authored-by: Oskar Eichler <62393985+OskarEichler@users.noreply.github.com>
1 participant
Summary
The existing DOM snapshots are updated for the intentional iframe title and button markup. New focused lifecycle tests cover queued/stale renders, reopening and iframe Escape handling, Trusted Types policy reuse, and encoded editor paths.
Compatibility
No public API, engine, or dependency changes. Error-link markup becomes a button; iframe title/language and focus behavior are intentional accessibility improvements. Reserved filename characters now reach the editor intact.
Verification
git diff --checkpassType: fix
Summary by CodeRabbit
Bug Fixes
Accessibility