Defer backend detection until Measure is clicked#12
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the backend detection logic to be on-demand rather than automatic upon component mounting. The BackendCard component now retrieves the backend state from the store instead of performing its own detection, and the handleMeasure function in the measure page has been updated to trigger detection if no backend is active. A new smoke test ensures that local runner health checks are not initiated before a measurement is submitted. I have no feedback to provide.
hoainho
left a comment
There was a problem hiding this comment.
Thank you @AjTheSpidey — this is a clean, focused fix and exactly matches the design we proposed in #11 (defer probe to on-click). Excellent work for a first contribution.
Review summary
Audit:
- 3 files changed, +49/-25 LOC, surgical scope
- No security concerns (no eval/exec/fetch/atob/crypto/require/dynamic-import added)
- No leftover
useEffectmount-side effects — the probe is now entirely on-demand - New test
measure route does not probe local runner before submitis well-written and catches the exact behavior we wanted to verify
Verification (run locally on this PR branch):
- ✅
pnpm --filter @ohmyperf/website typecheck— clean - ✅
pnpm --filter @ohmyperf/website lint— clean (--max-warnings=0) - ✅
OHMYPERF_BASE_PATH=/ohmyperf pnpm --filter '@ohmyperf/website...' build— 8 static pages, all assets correctly prefixed - ✅
pnpm exec playwright test smoke.spec.ts -g 'measure route does not probe'— pass (3.8s) - ✅ Other 4 smoke tests pass; 1 pre-existing failure (
landing renders hero + form) is mine from a recent landing-page rewrite, not yours - ✅ Merge dry-run against current main is clean — your branch is 7 commits behind but all auto-merge resolutions are correct and preserve all in-flight work
Small note (not blocking): detectBackend() inside handleMeasure doesn't pass an AbortSignal, so if the user navigates away mid-probe the request still resolves. Not worth holding up the merge for; happy to file a follow-up issue if you'd like.
About the Windows/Node 25 build error you mentioned (ERR_UNSUPPORTED_ESM_URL_SCHEME in packages/design-tokens): that's a known Windows file URL issue with Node 25; it should resolve on Node 22/24 LTS. Not something this PR needs to address.
Merging now. Thanks again — this lands as part of v0.3.
…p ci] PR #12 review surfaced a pre-existing smoke test failure (not caused by that PR). My landing rewrite at fb46810 moved 'OhMyPerf' from the h1 to the header brand; h1 now reads 'The first perf tool an LLM agent can actually fix your site with.' Update the smoke test to match: - h1 contains /LLM agent can actually fix/i - 'OhMyPerf' verified visible anywhere on page (header brand) Verified: 6/6 smoke tests pass (13.8s).
3-Angle review (Oracle correctness + 2× explore security/UX) on PR #13 returned 13 findings (2 blockers + 5 important + 6 nits). This commit addresses 11 of them; 2 deferred (B5 frame-ancestors needs HTTP header — static export limitation, doc'd as v0.3 work; A3 runner-late- upgrade is acceptable behavior change, doc'd in REGISTRY). Angle A (correctness, oracle bg_b527c677): - A5 BLOCKER: HMR window-stamp guard apps/website/lib/backend-detector.ts: stamp `window.__ohmyperfAnnounceInstalled` flag so HMR re-evaluation doesn't double-install listener - A6 BLOCKER: process.* skip rule was matching whole lines containing esbuild markers, hiding real offenders. Now strips marker substrings via .replace() then tests residue with the regex. apps/extension-chrome/tests/playwright-e2e/extension-load.spec.ts L4 - A4 IMPORTANT: prepare-e2e-fixtures.mjs now runs `playwright install chromium` if browsers cache absent (instead of cryptic "browser not found" error mid-spec) - A-extra NIT: removed redundant headless: false from playwright.config.ts (was overridden by spec's launchPersistentContext anyway) Angle B (security, explore bg_1f59e5a0): - B1 MEDIUM: extended apps/extension-chrome/.gitignore with .dev-keys/, extension-dist/, playwright-report/ (defense-in-depth; root .gitignore already protects but package-level redundancy prevents subtree-copy exposure) - B5 MEDIUM: SKIPPED THIS PR. Reason: frame-ancestors is ignored when delivered via <meta> (Chromium spec). Real fix needs HTTP header which static export to GitHub Pages cannot deliver without a CDN proxy layer. Tracked as v0.3 — when ohmyperf.dev moves to Cloudflare Pages, set frame-ancestors via _headers file. - B9 MEDIUM: --no-sandbox is now conditional (Linux root || CI), not unconditional; protects developer macOS/Linux machines from unnecessary sandbox-disable - B10 LOW: stale profile cleanup at beforeAll start (recovers from SIGKILL leaks) Angle C (real-user UX, explore bg_68650392): - C1+C4 IMPORTANT: introduced `detecting` state. While true and backend.kind === 'none', BackendCard shows "Detecting…" badge and NoBackendGuide is suppressed. Eliminates jarring layout shift when detection completes (NoBackendGuide → ExtensionReady). - C2 IMPORTANT: shared detection promise via detectionPromiseRef. handleMeasure now awaits the inflight effect promise instead of starting a duplicate detection run, eliminating click-race. - C3 NIT: BackendCard "none" state copy updated. When detectionRan=true, shows "Auto-detect ran just now and found neither..." instead of misleading "click Measure to check again". - C8 IMPORTANT: detectionRan flag passed to BackendCard. Shows users that auto-detect did execute (vs. "detection never ran" ambiguity). - C9 NIT: dev-mode console.warn on detection error (replaces silent catch). - C10 NIT: BackendCard ready states now show "— enter a URL above to measure" helper text after the version badge. CRITICAL CORRECTION discovered while running smoke test: - E2E spec passed but smoke `measure route does not probe local runner before submit` (PR #12 contract @AjTheSpidey) FAILED because em's initial fix called detectBackend (which probes BOTH extension AND runner). PR #12 explicitly forbade auto-probing runner. - Em added new export `detectExtensionOnly()` to backend-detector.ts that only runs pingExtension (no pingRunner). page.tsx auto-detect effect now uses detectExtensionOnly. Runner is still probed on-Measure-click via the original detectBackend. - This preserves PR #12 contract while still fixing Layer F handshake. - REGISTRY updated: Layer F invariant now explicitly says "detectExtensionOnly (NOT detectBackend)". Verification (Evidence Receipts): layer: validate:quick command: pnpm --filter @ohmyperf/extension-chrome typecheck && pnpm --filter @ohmyperf/extension-chrome lint && pnpm --filter @ohmyperf/website typecheck && pnpm --filter @ohmyperf/website lint exit_code: 0 relevant_assertion: All clean across changed packages layer: test:cross-cutting-allowlists command: bash scripts/check-cross-cutting-allowlists.sh exit_code: 0 stdout_tail: 3 systems checked, 0 failed layer: test:e2e:extension command: pnpm --filter @ohmyperf/extension-chrome e2e:extension exit_code: 0 stdout_tail: | [L3] extensionDetected=true guideVisible=false [L3] page console (1 entries): ← was 3 with ERR_CONNECTION_REFUSED, now clean ✓ L1 (4ms) ✓ L2 (1ms) ✓ L3 (9.5s) ✓ L4 (7ms) ✓ L5 (3ms) 5 passed (26.5s) layer: test:integration (smoke) command: PLAYWRIGHT_BROWSERS_PATH=/opt/playwright-browsers playwright test tests/smoke.spec.ts (in apps/website) exit_code: 0 stdout_tail: | ✓ landing renders hero + form (901ms) ✓ form submit routes to /measure with url query (3.5s) ✓ backend detector card renders "none" state without backend (601ms) ✓ measure route does not probe local runner before submit (899ms) ← THIS IS THE PR #12 CONTRACT TEST ✓ CSP meta tag present (691ms) ✓ private URL triggers soft-warn (548ms) 6 passed (19.6s) Counts: Net diff: +128 -19 = +109 lines across 8 files 3-Angle review findings addressed: 11/13 (2 deferred with rationale)
Fixes #11.
This moves backend detection out of the backend card mount path and into the Measure submit flow. Visiting /measure no longer probes localhost:5174 on page load, so first-time visitors do not get the local runner CORS noise just from opening the page.
I also updated the backend card copy so it no longer says the page will auto-detect immediately.
Tested:
One note: pnpm --filter @ohmyperf/website... build fails for me earlier in packages/design-tokens on Windows/Node 25 with ERR_UNSUPPORTED_ESM_URL_SCHEME before the website build runs. The website build itself passes.