You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
add a reproducible headless Chrome/CDP benchmark for plan UI cold loads
serve the built plan HTML with a representative Pi /api/plan response
report usable-render, navigation, JavaScript/task, long-task, document-transfer, and heap metrics
include browser/CDP provenance and the measured HTML SHA-256
support custom Chrome/HTML paths and machine-readable JSON output
Motivation
This provides a repeatable baseline for investigating the per-open delay reported in #926 and for comparing future bundle-splitting or server-delivery experiments against the same artifact and environment.
Each run creates a fresh page target, clears its HTTP cache, waits until both the plan content and approval control are rendered, and tears down its isolated Chrome profile.
TLDR: needs one small change, then merge. The harness hangs for the full 30s timeout on macOS, the platform #926 was filed from, because the readiness probe waits for the "Approve" label, which is hidden below the 768px breakpoint, and Chrome's default macOS window is 756px wide. Pinning --window-size=1280,900 fixes it (verified) and is worth having anyway since an unpinned viewport makes numbers incomparable across machines. Everything else is minor, and the safety, cleanup, and provenance discipline here are genuinely good for a first contribution. Welcome.
Detail (AI review findings, skim as needed):
The blocker mechanics: waitUntilUsable requires innerText.includes("Approve"), but below md the approve control renders as "OK" (ApproveDropdown.tsx, hidden md:flex). With the window pinned, macOS produces plausible numbers matching the shape of your Linux table scaled by hardware (~420-460ms usable, transfer bytes matching the file plus headers, and the reported sha256 matching an independent hash of the served file, so the provenance is genuine). Consider also switching the probe to a stable DOM hook rather than a responsive label so the readiness definition is viewport-independent.
Worth taking in the same pass: the measured window is not hermetic. The app fires a live request to api.github.com (the update check) inside every run, which makes a reproducible benchmark depend on network conditions and the anonymous rate limit. Blocking non-loopback resolution (--host-resolver-rules="MAP * ~NOTFOUND, EXCLUDE 127.0.0.1") and documenting it would tighten the claim. And since the issue thread names gzip as the first optimization this harness will judge, a --gzip serving flag would let it A/B the exact experiment it was built for.
Noted as fine or follow-up: the script needs Node 22+ for global WebSocket (an engines field or README line would save someone a bare ReferenceError); the fixture omits a few fields of the real /api/plan response (renders fine, slightly lighter than production); usableMs is quantized by the 25ms poll and the heap number swings with GC timing, both worth a caveat line; the free-port probe has a harmless race a --remote-debugging-port=0 plus DevToolsActivePort read would remove; parseArgs sits outside the try so unknown flags print a stack trace instead of the friendly error.
Verified clean, for the record: both embedded servers bind loopback only; no external URLs in the script itself; Chrome flags contain no sandbox or web-security weakening; the throwaway profile is mkdtemp'd and removed in a finally with zero leftovers after success and failure paths both; teardown escalates TERM then KILL; zero new dependencies (CDP over Node's built-in WebSocket, exactly the right call this week); not referenced by any CI workflow and invisible to bun test, so it is manual-only as it should be; the package.json delta is one script line and the lockfile is untouched; README accurate; no em dashes; single commit on current main, mergeable.
Both benchmark modes completed successfully with external traffic blocked. The gzip run reported 6.98 MiB served versus 21.97 MiB identity for the same HTML SHA-256.
TLDR: merge as-is. All findings fixed and verified empirically, the turnaround fixed root causes rather than symptoms, and your own harness already produced its first real result: gzip makes the loopback load about 46ms slower, not faster.
Detail (AI review findings, skim as needed):
The macOS failure is fixed properly: the viewport is pinned for comparability AND the probe is viewport-independent on its own merits (verified by forcing a 700px window, where it correctly matches the mobile OK control; the regex also correctly rejects the "Approving..." loading state and accepts the split-button variant). The network block works with the right semantics: the update-check still executes and fails locally at DNS with no packet leaving the machine, so the app's real code path is preserved while the network variable is removed; all seven loopback requests still flow. The gzip A/B byte accounting is correct (31.77 percent, agreeing with Vite's own gzip report for the same artifact to within rounding), the encoding is labeled in both output modes, and the sha256 correctly stays the hash of the uncompressed artifact so provenance identifies the build rather than the transport.
The substantive result: identity averaged 426ms usable, gzip 473ms. Over loopback the transfer saving buys nothing while decompression costs real time, which is direct evidence against the gzip experiment as a perceived-load improvement for the local server case. One methodological caveat if this number gets cited: the harness pre-compresses at startup, so server-side compression CPU is excluded; the A/B measures transfer plus decompression only. Remote sessions (where transfer is real) remain the case where compression could still pay.
Still clean: zero dependencies (node:zlib is a builtin), loopback-only with the resolver rule tightening posture, CI-invisible, zero leftover profiles or processes after six more launches, README accurate including the Node 22 floor and measurement caveats, no em dashes, mergeable on current main.
Cosmetic nits, fine as-is or follow-up: the provenance viewport string reports the window size rather than measured innerWidth/innerHeight; parseArgs sits outside the try so unknown flags stack-trace; the free-port probe has a benign race; the fixture omits a few production response fields.
Approving the workflow runs now so CI has real signal. Quality first contribution, twice over.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/api/planresponseMotivation
This provides a repeatable baseline for investigating the per-open delay reported in #926 and for comparing future bundle-splitting or server-delivery experiments against the same artifact and environment.
Each run creates a fresh page target, clears its HTTP cache, waits until both the plan content and approval control are rendered, and tears down its isolated Chrome profile.
Validation
bun run build:reviewbun run build:hookbun run typechecknode tests/bench-plan-load.mjs --runs 3 --chrome /usr/bin/chromiumbun test: 2863 passed; two existing environment/dependency errors remain because the installed@pierre/diffs@1.3.2build does not exportEditProviderCloses #926 only if maintainers consider the benchmark sufficient; this PR does not attempt the performance fix itself.