Skip to content

Defer backend detection until Measure is clicked#12

Merged
hoainho merged 1 commit into
hoainho:mainfrom
AjTheSpidey:fix/defer-backend-probe
May 21, 2026
Merged

Defer backend detection until Measure is clicked#12
hoainho merged 1 commit into
hoainho:mainfrom
AjTheSpidey:fix/defer-backend-probe

Conversation

@AjTheSpidey

Copy link
Copy Markdown
Contributor

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:

  • pnpm --filter @ohmyperf/website... typecheck
  • pnpm --filter @ohmyperf/website lint
  • pnpm --filter @ohmyperf/website build
  • pnpm --dir apps/website exec playwright test smoke.spec.ts -g "measure route does not probe"

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 hoainho left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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 useEffect mount-side effects — the probe is now entirely on-demand
  • New test measure route does not probe local runner before submit is 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.

@hoainho
hoainho merged commit 1526a2f into hoainho:main May 21, 2026
hoainho added a commit that referenced this pull request May 21, 2026
…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).
@AjTheSpidey
AjTheSpidey deleted the fix/defer-backend-probe branch May 21, 2026 15:48
hoainho added a commit that referenced this pull request May 22, 2026
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)
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.

website: backend-detector probe leaks CORS errors to console on /measure

2 participants