ci: Add Lighthouse Test as Nightly CI run#20850
Open
HazAT wants to merge 29 commits into
Open
Conversation
Adds the foundation for measuring Lighthouse performance scores across frontend SDKs and feature combinations. Introduces a new private workspace at dev-packages/lighthouse-tests/ with three pieces: - lighthouserc.js — shared LHCI config. Uses simulated throttling (default, most deterministic on shared CI runners), 5 runs per URL with aggregationMethod 'median-run' to halve variance, performance-only audit (skips a11y/SEO/PWA), and warn-only score assertion at minScore 0.5 so the job never blocks a merge. Branches between staticDistDir and startServerCommand at config-load time based on LIGHTHOUSE_SERVE_MODE so the same config powers both static SPAs and SSR apps. - lighthouse-matrix.mjs — generator that emits the 14 apps × 3 feature modes = 42 matrix cells consumed by the (to-be-added) GitHub Actions job. Each cell carries every field the workflow needs (app-dir, static-dir/start-cmd, ready-pattern) plus an env-var-name field that documents which bundler-specific prefix the app reads at build time (SENTRY_LIGHTHOUSE_MODE, PUBLIC_*, VITE_*, NEXT_PUBLIC_*, NUXT_PUBLIC_*). - package.json — minimal private workspace with volta.extends pointing at the root so Node/yarn/pnpm versions stay locked to the monorepo. Also registers dev-packages/lighthouse-tests under the root workspaces array next to the existing dev-packages entries. Refs: TODO-a4b18f49 (plan: .pi/plans/2026-05-12-lighthouse-ci/plan.md) Co-Authored-By: Claude claude-opus-4-5 <noreply@anthropic.com>
…ser, nextjs-16, react-router-7-spa Instrument three E2E test applications to conditionally initialize Sentry based on the SENTRY_LIGHTHOUSE_MODE build-time environment variable. This enables Lighthouse CI to measure performance across three feature levels: - no-sentry: dynamic import is skipped entirely, allowing treeshaking to remove all SDK code for a clean baseline measurement - init-only: Sentry.init() runs with no additional integrations (measures SDK core overhead) - tracing-replay: full integrations enabled (measures feature overhead) - unset/empty: preserves existing E2E behavior (all features enabled) Each app uses the env var prefix appropriate for its bundler: - default-browser (webpack): process.env.SENTRY_LIGHTHOUSE_MODE - nextjs-16 (Next.js): process.env.NEXT_PUBLIC_SENTRY_LIGHTHOUSE_MODE - react-router-7-spa (Vite): import.meta.env.PUBLIC_SENTRY_LIGHTHOUSE_MODE The async IIFE + dynamic import pattern is used (instead of top-level await) because webpack production builds may not support TLA. This also ensures clean treeshaking in no-sentry mode since the import itself is conditional. The default-browser build.mjs EnvironmentPlugin is changed from array form to object form with empty-string defaults so the build succeeds when SENTRY_LIGHTHOUSE_MODE is not set (existing E2E jobs don't set it). Bundle-output treeshake verification deferred to PR CI (the Lighthouse workflow job builds apps with the env var set and runs Lighthouse against the output). Ref: TODO-5805679c Co-Authored-By: Claude claude-opus-4-6 <noreply@anthropic.com>
…browser and nextjs-16 The previous commit (36c3f76) implemented the SENTRY_LIGHTHOUSE_MODE guard but did not actually wire up replayIntegration() in the 'tracing-replay' mode for default-browser, and did not wire up browserTracingIntegration() or replayIntegration() at all for nextjs-16. This made the 'tracing-replay' mode measure identically to 'init-only' for those two apps, defeating the purpose of the matrix. - default-browser: push Sentry.replayIntegration() when mode is 'tracing-replay'; also set replaysSessionSampleRate / replaysOnErrorSampleRate to 1.0 in that mode (0 otherwise). Existing E2E behavior (unset env var) intentionally still has no replay, matching the file before this work. - nextjs-16: push Sentry.browserTracingIntegration() and Sentry.replayIntegration() when mode is 'tracing-replay'. Also gate the thirdPartyErrorFilterIntegration to existing-E2E mode only so init-only and tracing-replay measure SDK overhead without app-specific noise. Note: nextjs-16 continues to use a static 'import * as Sentry' rather than dynamic import. This is a tracked limitation (plan Risk 5): Next.js's bundler does not reliably treeshake dynamic imports of @sentry/nextjs from instrumentation-client.ts. The 'no-sentry' mode will therefore measure a baseline that includes the SDK bundle (but does not initialize it). The deltas between init-only and tracing-replay remain meaningful; only the absolute no-sentry baseline is polluted. Ref: TODO-5805679c Co-Authored-By: Claude claude-opus-4-5 <noreply@anthropic.com>
…ld.yml Insert three new jobs between job_optional_e2e_tests and job_required_jobs_passed in the CI workflow: - job_lighthouse_matrix: generates the test matrix (apps × modes), gated on schedule (nightly) or 'ci:lighthouse' PR label - job_lighthouse: runs each matrix cell on ubuntu-24.04-large-js with continue-on-error: true and max-parallel: 15. Mirrors the job_e2e_tests prep recipe (pnpm 9.15.9, Node from app package.json, restore-cache, download build-tarball-output, yarn test:prepare, ci:copy-to-temp, ci:pnpm-overrides). Build step sets SENTRY_LIGHTHOUSE_MODE under every common bundler env prefix (NEXT_PUBLIC_, PUBLIC_, REACT_APP_) so each app reads whichever its bundler exposes. Uses treosh/lighthouse-ci-action@v12 with numberOfRuns: 5 and temporaryPublicStorage: true. - job_lighthouse_report: downloads all lighthouse artifacts and runs post-comment.mjs to generate a summary table / PR comment Extended on.pull_request to include types: [opened, synchronize, reopened, labeled] so the labeled event fires when ci:lighthouse is added to a PR. Security: NO pull_request_target, NO new permissions block, NO new secrets beyond GITHUB_TOKEN, NOT added to job_required_jobs_passed needs[] — lighthouse failures never block merges. Ref: TODO-1d15a08e, plan rev 2 Co-Authored-By: Claude claude-opus-4-6 <noreply@anthropic.com>
…thouserc.js The previous commit (ae9b8b8) added the three lighthouse jobs but referenced matrix field names that don't exist in our matrix generator output, and didn't wire the env vars that lighthouserc.js reads. As written, every matrix cell would have failed at the 'Set up Node' step because matrix.test-application is undefined. Fixes: 1. Matrix field references — rename to match dev-packages/lighthouse-tests/ lighthouse-matrix.mjs output: - matrix.test-application → matrix.app - matrix.build-command → dropped (always 'pnpm test:build') - matrix.serve-command → matrix.start-cmd (via env var) - matrix.url → dropped (always http://localhost:3000/) The matrix actually emits: app, app-dir, env-var-name, mode, ready-pattern, sdk, serve, start-cmd, static-dir. 2. Capture matrix output. The matrix generator step ran 'node lighthouse-matrix.mjs' but did NOT redirect to $GITHUB_OUTPUT, so its 'matrix=<JSON>' line was discarded and downstream jobs would expand to an empty matrix. Fixed to '>> "$GITHUB_OUTPUT"'. 3. Pass LIGHTHOUSE_* env vars to the treosh action. The lighthouserc.js in dev-packages/lighthouse-tests/ branches at config-load time on LIGHTHOUSE_SERVE_MODE (static vs server) and reads LIGHTHOUSE_STATIC_DIR / LIGHTHOUSE_START_CMD / LIGHTHOUSE_READY_PATTERN / LIGHTHOUSE_URL. The previous commit instead used the action's 'urls:' and 'startServerCommand:' inputs, which would have conflicted with the config's branching (staticDistDir would have been undefined). 4. Add the DSN env block at job level, mirroring job_e2e_tests (E2E_TEST_DSN + framework-prefixed variants). Apps gracefully handle missing DSN but cleanly setting them keeps Sentry.init() identical to real E2E runs. 5. Add 'yarn test:validate' step after test:prepare to match job_e2e_tests prep recipe. 6. Use node-version-file: 'package.json' (repo convention) instead of node-version: lts/* in job_lighthouse_matrix and job_lighthouse_report. 7. job_lighthouse_report: add job_build + job_lighthouse_matrix to needs[] so the restore-cache step has its dependency_cache_key, and the report waits for the matrix step. 8. job_lighthouse_report: pass IS_PR (boolean) and PR_NUMBER env vars to post-comment.mjs (matches Todo 4's spec). Security posture from ae9b8b8 is preserved: still no pull_request_target, no new permissions, no new secrets, lighthouse jobs still excluded from job_required_jobs_passed.needs[]. Ref: TODO-1d15a08e Co-Authored-By: Claude claude-opus-4-5 <noreply@anthropic.com>
…s as PR comment
Add a script that reads LHCI artifact directories (manifest.json + lhr-*.json),
extracts median-run performance scores for each app × mode combination, and
renders a markdown table with columns for No Sentry, Init Only, Tracing+Replay,
plus computed deltas (SDK overhead = init-only minus no-sentry, feature overhead =
tracing-replay minus init-only).
Mirrors the find-and-update PR comment pattern from size-limit-gh-action: searches
for an existing comment by heading prefix ('## 🔦 Lighthouse Report'), then creates
or updates accordingly. Handles 403 gracefully on fork PRs where GITHUB_TOKEN from
pull_request events is read-only — logs a warning and exits 0 instead of failing.
For nightly cron runs (IS_PR !== 'true'), the table is logged to stdout. A <50%
data completeness guard skips posting when too many cells are missing.
Co-Authored-By: Claude claude-opus-4-6 <noreply@anthropic.com>
The previous commit (0119764) wrapped each score in a markdown link pointing to entry.htmlPath from the LHCI manifest. That field is a path on the CI runner's local filesystem (e.g. /home/runner/_work/.../lhr-0.html), not a URL that a PR reader can open. GitHub renders the markdown but the link 404s. The temporaryPublicStorage URLs (where the reports actually live) are exposed by treosh/lighthouse-ci-action as its 'links' output, not via the manifest artifact, so they're not available in this script's current input. Piping them in would require a second artifact upload step in the workflow — out of scope for the MVP. For now, drop the link entirely (just show the score) and add a footer pointing PR readers to the workflow artifacts (lighthouse-<app>-<mode>), which are downloadable from the GitHub Actions UI. Wiring the public-storage URLs into the comment is tracked as a future improvement and can be done by: 1. capturing the action's 'links' output in job_lighthouse via 'id: lh' + step output, and 2. saving it as a per-cell artifact that post-comment.mjs merges in. Verified locally with a 42-cell fixture (full table renders without links, footer present) and a 3-cell fixture (<50% warning triggers, no comment posted). Ref: TODO-5844320c Co-Authored-By: Claude claude-opus-4-5 <noreply@anthropic.com>
The previous commit referenced treosh/lighthouse-ci-action by the floating @v12 tag. That tag is a moving target — if the treosh org's GitHub account is compromised or they push a malicious v12.x.y patch, our CI would execute it on the next workflow run. This action also internally npm-installs @lhci/cli, so the upstream supply chain includes both the action's own code AND its transitive npm deps. Pinning to a commit SHA locks both down (the action's repo has a committed package-lock.json, so the commit pins the npm tree too). Matches the existing repo policy for third-party actions (cf. pnpm/action-setup@fc06bc1257... which is similarly pinned to SHA). Co-Authored-By: Claude claude-opus-4-5 <noreply@anthropic.com>
…n report Two issues found by running the workflow end-to-end locally: 1. **default-browser webpack build was aborting in all 3 modes.** The dynamic-import refactor in 36c3f76 caused @sentry/browser to be emitted as a separate code-split chunk (~265 KiB raw). Webpack's default performance.hints='warning' fires AssetsOverSizeLimitWarning for assets > 244 KiB, and build.mjs treats any warning as fatal (process.exit(1)). The original static-import build came in at 138 KiB so the warning never fired before. Disabling performance hints in this app's webpack config is the right scope — bundle size is tracked separately via .size-limit.js at the repo root. 2. **post-comment.mjs reported only categories.performance.score, which tops out at 100 for fast static apps across all 3 modes.** Local measurement of default-browser confirmed: score is 100 for no-sentry, init-only, AND tracing-replay, even though LCP jumps from 916 ms to 1836 ms when the SDK is loaded. The score column alone would have rendered as a sea of '100 | 100 | 0 | 100 | 0' across all 14 apps, making the comment useless for fast frameworks. Now the report has four tables (one per metric): performance score, LCP, TBT, and total bytes downloaded. Each follows the same App × Mode × Δ shape. LCP is the primary SDK-overhead indicator; TBT captures runtime cost of integrations; bytes captures download cost. For slower apps where scores actually differ, the score table still carries signal; for fast static apps the metric tables show what's happening. Verified locally end-to-end: built default-browser in all 3 modes, ran lhci collect (2 runs each) + lhci upload --target=filesystem, staged the resulting manifests as the workflow would, and confirmed post-comment.mjs renders all four tables with correct deltas and units. Webpack's constant-folding successfully eliminates the dynamic @sentry/browser import entirely in no-sentry mode — the entry never references the SDK chunk, so the browser fetches just 3 KB total (clean baseline confirmed). Known limitation tracked in plan: dynamic import doesn't propagate treeshaking, so init-only and tracing-replay download the same 87 KB gzipped chunk. Only TBT differentiates them (runtime cost of integration instantiation). LCP / bytes are mostly indistinguishable between those two modes for now; can be improved later via per-mode entry files. Co-Authored-By: Claude claude-opus-4-5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
size-limit report 📦
|
These four apps were producing Lighthouse failures in CI due to framework-specific build/serve quirks that we don't want to invest in for the MVP. Their cells are removed from both lighthouse-matrix.mjs and post-comment.mjs's APPS list (the two must stay in sync). Matrix shape goes from 14 apps × 3 modes (42 cells) to 10 apps × 3 modes (30 cells). Remaining coverage: default-browser (browser) react-router-7-spa (react-router) react-19 (react) tanstackstart-react (tanstack-start) vue-3 (vue) nextjs-16 (nextjs) svelte-5 (svelte) nuxt-5 (nuxt) sveltekit-2 (sveltekit) astro-5 (astro) These can be added back later as a follow-up once we're confident the core pipeline is stable. Co-Authored-By: Claude claude-opus-4-5 <noreply@anthropic.com>
…le.exports
All 42 Lighthouse cells in the first CI run failed with:
##[error]Config missing top level 'ci' property
Root cause: LHCI's config loader uses Node `require()` to load the
config file. The parent package (`dev-packages/lighthouse-tests/`) sets
`"type": "module"` so `lighthouserc.js` was treated as ESM. On modern
Node, `require()` of an ESM module returns `{ default: <export> }`
instead of the export itself, so LHCI saw a config without a top-level
`ci` property and bailed out before running Lighthouse.
Fix: rename to `lighthouserc.cjs` (explicit CommonJS regardless of the
parent package's `type` field) and change `export default` to
`module.exports`. Update the workflow's `configPath` accordingly.
Verified locally:
- `require('./lighthouserc.cjs')` returns an object with `ci` at the top
level.
- Server-mode branching (LIGHTHOUSE_SERVE_MODE=server) still resolves the
expected `startServerCommand` / `url` config.
The other files in this package (`lighthouse-matrix.mjs`,
`post-comment.mjs`) are explicit-extension ESM modules invoked via
`node script.mjs`, so they're unaffected by this change.
Co-Authored-By: Claude claude-opus-4-5 <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
29 of 30 matrix cells failed with:
##[error]Failed to CreateArtifact: Received non-retryable error:
Failed request: (409) Conflict: an artifact with this name already
exists on the workflow run
Root cause: `treosh/lighthouse-ci-action` with `uploadArtifacts: true`
uploads `.lighthouseci/` to a default-named artifact (`lighthouse-reports`).
With 30 matrix cells running in the same workflow run, every cell after
the first one to finish hit a 409 name-collision and aborted the action
\u2014 even though the Lighthouse audit itself had already succeeded.
The one cell that did succeed (`svelte-5 init-only`) was simply the first
to call upload-artifact; everything else lost the race.
We already do our own per-cell artifact upload one step later with a
unique name (`lighthouse-${{ matrix.app }}-${{ matrix.mode }}`), so the
treosh action's upload is redundant. Drop it.
`temporaryPublicStorage: true` is kept \u2014 it uploads the HTML report to
Google's bucket and prints clickable URLs into the action log, which is
useful per-cell debug context independent of the artifact.
Co-Authored-By: Claude claude-opus-4-5 <noreply@anthropic.com>
Last CI run (b766f80) was a big step forward — 21 of 30 cells succeeded, proving the pipeline works end-to-end. Three apps still failed: 1. nextjs-16 — build-time TypeScript error: Type error: '"@sentry/nextjs"' has no exported member named 'Integration'. The previous SENTRY_LIGHTHOUSE_MODE refactor introduced const integrations: Sentry.Integration[] = [] but `@sentry/nextjs` doesn't re-export the `Integration` type. Fix by inlining a spread-based array literal in Sentry.init() so no explicit type annotation is needed. TypeScript infers integration types from the individual integration factories. 2. astro-5 — Lighthouse hit chrome-error://chromewebdata/ (CHROME_INTERSTITIAL_ERROR): The @astrojs/node adapter defaults to port 4321, not 3000. The server started fine and printed "Listening on localhost:4321" — matching our readyPattern: 'localhost' — but LHCI then navigated to http://localhost:3000/, where nothing was listening, and Chrome bounced to its error page. Fix: prefix the start command with PORT=3000 so the Astro server binds to the port Lighthouse audits. 3. react-router-7-spa — Lighthouse reported NO_FCP across all three modes: the bundle loads, no obvious error in the log, but the page never paints within the navigation timeout. Could be a 2-minute fix or a 2-hour debug — and we already have 7 frontend SDKs reporting cleanly (browser, react, vue, svelte, sveltekit, astro, nuxt, tanstack-start, nextjs). Drop it from the matrix to match the previous descope decision (angular, remix, ember, solidstart). Can be re-added once we figure out why React Router 7 SPA doesn't paint under headless Chrome with simulated throttling. Matrix shape goes from 10 apps × 3 modes (30 cells) to 9 apps × 3 modes (27 cells). Co-Authored-By: Claude claude-opus-4-5 <noreply@anthropic.com>
…Path output All 27 Lighthouse cells succeeded in CI run db8a4bb but NO PR comment was posted. The Lighthouse Report job logged: Only 0/27 Lighthouse cells have results (< 50%). Skipping comment. Root cause: the upload-artifact step had `path: .lighthouseci/` (in our repo root), but `lhci collect` writes to `.lighthouseci/` inside the treosh action's own working directory (`/home/runner/work/_actions/treosh/ lighthouse-ci-action/.../node_modules/lighthouse-ci/` or similar) \u2014 not our repo root. So the upload step found 0 files, and because of `if-no-files-found: ignore`, silently succeeded with no artifact uploaded. All 27 cells therefore reported "success" while uploading nothing. The treosh action exposes the actual path via its `resultsPath` output. Fix: - Add `id: lighthouse` to the action step. - Switch `path:` to `${{ steps.lighthouse.outputs.resultsPath }}`. - Gate the upload on `resultsPath` being set so a failed Lighthouse run doesn't try to upload nothing. - Switch `if-no-files-found` from `ignore` to `error` so a missing or empty dir is now a loud bug instead of a silent zero-upload. Co-Authored-By: Claude claude-opus-4-5 <noreply@anthropic.com>
…/ artifact
After the previous fix (pointing upload-artifact at the treosh action's
`resultsPath` output), Lighthouse cells started failing with:
##[error]No files were found with the provided path:
/home/runner/work/sentry-javascript/sentry-javascript/.lighthouseci.
No artifacts will be uploaded.
even though the treosh action's manifest output showed real files at
that path:
"htmlPath":"/home/runner/.../.lighthouseci/localhost-index_html-...html"
Root cause: `.lighthouseci` is a dot-prefixed directory, and
`actions/upload-artifact@v7` excludes hidden files by default. The
docs:
> include-hidden-files: If true, hidden files will be included in the
> artifact. If false, hidden files will be excluded from the artifact.
> Default: false
This also explains why the previous run (b29aada, with the same path
but `if-no-files-found: ignore`) produced zero `lighthouse-*` artifacts
on the workflow run \u2014 the dir was being skipped for the same reason,
just silently.
Fix: set `include-hidden-files: true` so the dot-prefixed parent dir
doesn't cause everything inside to be excluded.
Co-Authored-By: Claude claude-opus-4-5 <noreply@anthropic.com>
Contributor
🔦 Lighthouse ReportPerformance score
Largest Contentful Paint (LCP)
Total Blocking Time (TBT)
Bytes downloaded
Median of 5 runs · simulated throttling · localhost. Lower is better for LCP, TBT, and bytes. Higher is better for score. Full reports are attached as workflow artifacts ( |
… avoid E2E race Commit 36c3f76 wrapped default-browser/src/index.js in an async IIFE so the `@sentry/browser` import could be guarded behind `SENTRY_LIGHTHOUSE_MODE=no-sentry` and tree-shaken out of that Lighthouse build. As a side effect the click listeners on `#exception-button` and `#navigation-link` were also moved inside that IIFE, so they now attach only after `await import('@sentry/browser')` resolves. That created a race for the existing E2E tests: `page.goto('/')` is followed immediately by `exceptionButton.click()`, and the click can fire before the dynamic import resolves and the listener registers. Flagged as a HIGH-severity review-bot finding on PR #20850. Move the two listeners back to synchronous module-top-level so they're guaranteed attached before any user interaction. Only the Sentry init is left inside the async IIFE \u2014 it's still gated by `if (lighthouseMode !== 'no-sentry')` so the tree-shaking benefit is preserved. The thrown error from the exception button is still captured by Sentry's global `window.error` listener installed by `Sentry.init()` whenever the IIFE finishes running.
…browser, nextjs-16) The previous matrix listed nine apps but only two (default-browser, nextjs-16) actually branch on `SENTRY_LIGHTHOUSE_MODE` in their Sentry init code. The other seven (react-19, vue-3, svelte-5, sveltekit-2, astro-5, tanstackstart-react, nuxt-5) ran three identical builds for every mode \u2014 same SDK, same integrations \u2014 so the `\u0394 (SDK)` and `\u0394 (Features)` columns in the PR comment were just run-to-run measurement noise rather than real SDK overhead. Flagged as a MEDIUM-severity review-bot finding on PR #20850. Drop the seven uninstrumented apps from the matrix so the report only shows honest deltas. Matrix is now 2 apps \u00d7 3 modes = 6 cells. The follow-up todo TODO-aeab11f0 already exists to wire `SENTRY_LIGHTHOUSE_MODE` into the remaining apps; once an app is actually instrumented it can be added back here. react-router-7-spa is instrumented but stays out of the matrix until its Lighthouse NO_FCP failure is diagnosed. Also update the file-level docstring \u2014 it still claimed "14 apps \u00d7 3 modes = 42 cells" from the original planner output even though earlier commits had dropped the matrix down to nine apps.
Two changes that simplify the trigger surface and fix the HIGH-severity review-bot finding on PR #20850 about the workflow-level `labeled` trigger: 1. Remove `labeled` from `pull_request.types` in build.yml. With it included, adding ANY label to a PR (not just `ci:lighthouse`) re-triggered the whole workflow, and the workflow-level `concurrency.cancel-in-progress: true` block cancelled the in-progress build/test/e2e jobs and restarted them from scratch. Labels are no longer a CI control surface here. 2. Drop the `contains(labels, 'ci:lighthouse')` check on `job_lighthouse_matrix`. Lighthouse now runs on every PR push the same way the other test jobs do, plus on the nightly schedule. Simpler mental model, no per-PR opt-in. The Lighthouse jobs are still NOT referenced in `job_required_jobs_passed.needs`, so they never block merges \u2014 only surface measurements via the PR comment and (after the next commit) the workflow run Job Summary.
…matrix Two changes to make Lighthouse data visible on every run, regardless of trigger: 1. Always write the report markdown to `$GITHUB_STEP_SUMMARY` via `core.summary.addRaw().write()`. The Job Summary renders as a styled panel at the top of the workflow run page \u2014 visible for PR runs, nightly runs, and manual dispatches alike. For PR runs the existing PR comment is still posted/updated; for nightly runs the Job Summary becomes the only output (was previously just `console.log` to the workflow log, which nobody reads). 2. Trim the hardcoded APPS list from 9 entries down to 2 (default-browser, nextjs-16). Only those two apps actually branch on `SENTRY_LIGHTHOUSE_MODE` today \u2014 the other seven were carried over from the original planner output but were never instrumented. With the stale 9-entry list, the matrix only produced 6 of 27 expected cells, tripping the 50%-fill safety check on every run and skipping the report entirely. The MEDIUM-severity bugbot finding called this out. When more apps gain `SENTRY_LIGHTHOUSE_MODE` support (tracked in TODO-aeab11f0), add them to both APPS arrays at the same time.
| </SentryRoutes> | ||
| </BrowserRouter>, | ||
| ); | ||
| })(); |
There was a problem hiding this comment.
App rendering deferred behind async import breaks E2E
High Severity
The entire ReactDOM.createRoot().render() call is now inside an async IIFE that awaits a dynamic import('@sentry/react'). Previously, rendering was synchronous. During normal E2E runs (where lighthouseMode is undefined), the app still goes through this async path, meaning React won't render until the dynamic chunk loads. Vite production builds split dynamic imports into separate chunks fetched at runtime; the page's load event can fire before that fetch completes, so page.goto('/') may return before the app has rendered or Sentry has initialized. This risks breaking the existing pageload transaction and error-capture E2E tests — particularly the ones that waitForTransaction with op === 'pageload', since browserTracingIntegration is now initialized well after the navigation performance entries fire.
Reviewed by Cursor Bugbot for commit 8288dcb. Configure here.
Member
|
I don't think we should have this on every PR (the comment alone is massive and pretty noisy), I'd rather have this be a dedicated workflow that only runs nightly 🤔 |
Move the Lighthouse jobs out of build.yml into a new .github/workflows/lighthouse.yml that runs only on the nightly schedule and on workflow_dispatch. No pull_request trigger — per @mydea's review feedback on PR #20850, running on every PR was too noisy and the per-PR comment was unwanted. Concretely: - Delete job_lighthouse_matrix, job_lighthouse, job_lighthouse_report from build.yml. - Add .github/workflows/lighthouse.yml with three jobs that mirror the deleted set: * job_build does its own `yarn build:ci` + `yarn build:tarball` and uploads them as 'lighthouse-build-tarball-output' — fully decoupled from build.yml. * job_lighthouse matrix unchanged from before (2 apps × 3 modes = 6 cells on ubuntu-24.04-large-js with continue-on-error so a single bad cell doesn't tank the run). * job_lighthouse_report runs report.mjs and writes the result to the workflow Job Summary (added in the next commit). - Trim the build-time env-var prefix block down to the two prefixes we actually consume today (SENTRY_LIGHTHOUSE_MODE + NEXT_PUBLIC_SENTRY_LIGHTHOUSE_MODE). PUBLIC_*, VITE_*, NUXT_PUBLIC_*, REACT_APP_* are gone — they implied broader coverage than we actually have. Same for the DSN block. - Use a unique concurrency group (lighthouse-${{ run_id }}) with cancel-in-progress: false — nightly runs are independent and shouldn't cancel each other. Adds ~5 min of self-build overhead per nightly run; nothing waits on it.
Rename post-comment.mjs to report.mjs and remove all PR-commenting code paths (octokit, listComments + find-and-update, GITHUB_TOKEN handling, fork-403 fallback, IS_PR / PR_NUMBER env vars). The Lighthouse workflow no longer runs on pull_request triggers (see the previous commit), so this code was dead. The script now does one thing: read .lighthouseci/ artifacts, build the markdown report, and write it to $GITHUB_STEP_SUMMARY via core.summary. That panel renders at the top of the workflow run page and is the workflow's only public output. Also drop the @actions/github dependency from dev-packages/lighthouse-tests/package.json — it was only used by the deleted octokit calls. yarn.lock retains an orphaned "@actions/github@^5.0.0" entry that the existing 9.x usage in other gh-action packages doesn't touch; left for natural cleanup on the next clean install.
Mirrors the dynamic-import pattern from default-browser/src/index.js so the
react-19 E2E test app can serve as a third (CRA) cell in the Lighthouse
matrix alongside default-browser (webpack) and nextjs-16 (Next.js SSR).
CRA exposes only env vars prefixed REACT_APP_*, so the gate is
process.env.REACT_APP_SENTRY_LIGHTHOUSE_MODE. Three branches:
- 'no-sentry': sync render with no Sentry import; CRA dead-code-eliminates
the @sentry/react chunk entirely (the dynamic import in the else branch
becomes unreachable when the env var inlines to 'no-sentry').
- 'tracing-replay': Sentry.init + browserTracingIntegration + replayIntegration.
- 'init-only' / unset: Sentry.init with no integrations, error handlers
attached to createRoot (preserves the existing E2E behavior when the env
var is unset).
The createRoot call moves inside the async IIFE in the else branch because
the onUncaughtError / onCaughtError options need the dynamically-imported
Sentry.reactErrorHandler. React handles its own scheduling so the extra
microtask before render is invisible to Playwright tests.
….gg lab Replace the in-CI Lighthouse runs with a build-bundle-upload pipeline that ships prebuilt test apps to the dedicated Sentry Lighthouse lab service (https://lighthouse.sentry.gg). The lab runs Lighthouse on stable, dedicated hardware — eliminating the measurement noise we hit on shared GitHub-hosted runners — persists results, and ships every metric to Sentry under the lighthouse.* namespace. Companion to the spec at ~/Projects/sentry-lhci/docs/sentry-javascript-handoff.md. Concretely: - Rename dev-packages/lighthouse-tests/ to dev-packages/lighthouse-bundle/. The directory's job is no longer 'tests' — it's bundle preparation. - Delete the in-CI Lighthouse infrastructure: lighthouserc.cjs (lab owns the LHCI config now), report.mjs (Job Summary moves into the new script), and lighthouse-matrix.mjs (matrix is just a hardcoded constant in the upload script — no GitHub Actions matrix expansion needed anymore). - Add bundle-and-upload.mjs: zero-deps (Node 22 builtins only — fetch, FormData, Blob, system tar). For each (app, mode) cell, copies the app to a temp dir, applies the existing ci:pnpm-overrides helper, builds with the right SENTRY_LIGHTHOUSE_MODE / NEXT_PUBLIC_SENTRY_LIGHTHOUSE_MODE / REACT_APP_SENTRY_LIGHTHOUSE_MODE env vars (every prefix set at once — each app's bundler picks up whichever it knows), and tars the result. Static cells (default-browser, react-19) bundle only their build/ dir. SSR cell (nextjs-16) strips node_modules + pnpm-lock.yaml, copies the packed SDK tgzs into ./packed/ inside the bundle, and rewrites every workspace-absolute file: path in package.json (dependencies, devDependencies, pnpm.overrides) to file:./packed/... so pnpm install can resolve them after the lab extracts the tarball. All 9 bundles are POSTed as one multipart request to /api/builds; the script then polls /api/builds/:id every 15s for up to 25 minutes and appends a markdown summary table to $GITHUB_STEP_SUMMARY with per-cell status, median score, and Lighthouse-report links. - Strip dependencies from dev-packages/lighthouse-bundle/package.json. The script only needs Node builtins. - Rewrite .github/workflows/lighthouse.yml. Two jobs: * job_build — SDK build + tarball generation (unchanged). * job_bundle_and_upload — single non-matrix job; runs the script after restoring tarballs and seeding dev-packages/e2e-tests/packed/ via yarn test:prepare. Per-cell parallelism gained nothing once Lighthouse moved off the runner. Requires two new repo secrets in getsentry/sentry-javascript: - LIGHTHOUSE_LAB_URL (https://lighthouse.sentry.gg) - LIGHTHOUSE_UPLOAD_TOKEN (mirror of the lab's Northflank UPLOAD_TOKEN) Triggers: nightly cron at 00:00 UTC plus workflow_dispatch for ad-hoc runs.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
GitHub only registers the workflow_dispatch button (and accepts API dispatches) once the workflow file lives on the default branch. This PR's workflow file is only on feat/lighthouse-ci, so neither the Actions UI nor `gh workflow run` can launch it. Add a temporary push trigger for this branch only so we can verify the end-to-end flow against the live lab before merging to develop. Once we have one green run, drop this trigger — the long-term contract is schedule + workflow_dispatch only. Tracking: remove this commit (or just this block) before opening the PR for review on develop.
The previous commit renamed dev-packages/lighthouse-tests/ to dev-packages/lighthouse-bundle/ but missed the corresponding entry in the root package.json workspaces array. scripts/dependency-hash-key.js iterates `workspaces` and require()s each package.json — the stale entry pointing at the missing lighthouse-tests/package.json crashed the install-dependencies composite action with MODULE_NOT_FOUND on the first push of the new workflow.
| app: 'nextjs-16', | ||
| serve: 'server', | ||
| startCmd: 'pnpm start', | ||
| readyPattern: 'Ready in', |
Contributor
There was a problem hiding this comment.
Bug: The readyPattern for the Next.js 16 Lighthouse test is 'Ready in', but the actual server startup message is "ready - started server", which will cause a timeout.
Severity: HIGH
Suggested Fix
Change the readyPattern for the nextjs-16 definition in bundle-and-upload.mjs from 'Ready in' to a pattern that exists in the production startup log, such as 'ready - started server'. This will allow the Lighthouse lab to correctly identify when the server is ready for testing.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: dev-packages/lighthouse-bundle/bundle-and-upload.mjs#L54
Potential issue: The `readyPattern` in `bundle-and-upload.mjs` is configured as `'Ready
in'` for the Next.js 16 Lighthouse test application. However, the production server for
Next.js 16 outputs `"ready - started server on..."` upon successful startup. Since the
specified pattern is not present in the server's output, the Lighthouse lab's readiness
check will fail to detect that the server has started. This will lead to a timeout,
causing all Lighthouse tests for the `nextjs-16` application to fail during the CI
workflow.
The downstream `job_bundle_and_upload` job uses `.github/actions/restore-cache`, which unconditionally downloads a `build-output` artifact (the transpiled package outputs from Nx). My new `job_build` was only uploading the SDK tarballs, so the restore step failed with 'Artifact not found for name: build-output'. Mirror build.yml's job_build: compute the Nx build artifact paths via `yarn ci:print-build-artifact-paths` and upload them as `build-output` in addition to the existing `lighthouse-build-tarball-output`.
…den exec
Five tightly related cleanups to the upload script — all motivated by the
first end-to-end CI run (which uploaded successfully but had all three
nextjs-16 cells fail at the lab's `pnpm install` step) and by the review-bot
findings on the same commit:
1. Fire-and-forget upload (per maintainer feedback). The script no longer
polls the lab's `GET /api/builds/:id` endpoint or appends a Job Summary
table. As soon as the POST succeeds we log the dashboard URL and exit 0.
Cell results live in the Sentry dashboard, not the GH Actions UI.
2. Fix nextjs-16 install on the lab. The previous SSR bundle preserved
`@sentry-internal/test-utils: link:/abs/path/...` in devDependencies —
ciCopyToTemp rewrites workspace-relative `link:` deps to absolute paths
anchored at the runner's workspace, which obviously doesn't exist after
the bundle moves to the lab. `pnpm install` then exited with code 1.
Fix: `prepareSsrBundle` now deletes `devDependencies` wholesale before
writing package.json. The lab only needs to run `pnpm start` (no test
tools, no type-checker, no linter), and dropping devDeps eliminates the
workspace-link footgun entirely.
3. Replace every `execSync('cmd ' + var)` with `execFileSync('cmd', [argv])`.
CodeQL flagged four sites where shell commands were built from env-derived
strings (RUNNER_TEMP-rooted paths, app names). Even though inputs are
controlled, the argv-array form is the correct shape — no shell layer, no
interpolation, no escape-quoting concerns. As a bonus, paths with spaces
would now work.
4. Drop dead `envVarName` field from APPS entries. Three matrix definitions
carried this property but nothing reads it — the build step hardcodes all
three prefix variants. Cursor flagged this as a maintenance trap.
5. Drop unused imports (`setTimeout as sleep`, `appendFile`) now that the
polling and Job Summary code is gone.
| SENTRY_E2E_WORKSPACE_ROOT: WORKSPACE, | ||
| SENTRY_LIGHTHOUSE_MODE: mode, | ||
| NEXT_PUBLIC_SENTRY_LIGHTHOUSE_MODE: mode, | ||
| REACT_APP_SENTRY_LIGHTHOUSE_MODE: mode, |
There was a problem hiding this comment.
Missing PUBLIC_ prefix env var for Vite app
Medium Severity
The build env in bundle-and-upload.mjs sets SENTRY_LIGHTHOUSE_MODE, NEXT_PUBLIC_SENTRY_LIGHTHOUSE_MODE, and REACT_APP_SENTRY_LIGHTHOUSE_MODE, but omits PUBLIC_SENTRY_LIGHTHOUSE_MODE. The react-router-7-spa app reads import.meta.env.PUBLIC_SENTRY_LIGHTHOUSE_MODE (Vite with envPrefix: 'PUBLIC_'), so the mode will always be undefined when this app is used in lighthouse builds. The workflow env already sets PUBLIC_E2E_TEST_DSN, showing awareness of this prefix. The comment on line 118 says "all common bundler prefixes" are set, but the Vite PUBLIC_ prefix is missing.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 48c2db9. Configure here.
Member
Author
|
Okay i updated the pr so right now there is a new service that lives here: https://github.com/getsentry/sentry-lighthouse It publishes all the run metrics into a dedicated dashboard: https://sentry-sdks.sentry.io/dashboard/5335223/?interval=1d&project=4511381736128512&seerRunId=14062882&statsPeriod=24h And the workflow runs nightly and just like does something. We can set up alerts in Sentry for this. |
…hthouse gate
react-router-7-spa was instrumented for SENTRY_LIGHTHOUSE_MODE early in this
PR but never made it into the upload matrix (the app's static build serves an
empty FCP under Lighthouse — NO_FCP). The dynamic-import-gated initialisation
has therefore been dead code that:
- adds an async IIFE wrapper around a sync render path,
- keeps the unused PUBLIC_SENTRY_LIGHTHOUSE_MODE branch in source,
- and risks subtly changing existing E2E test behaviour for an app whose
Lighthouse coverage is on hold anyway.
Restore the file to its develop state. The lighthouse-bundle matrix is now
the only source of truth for which apps are touched, and it lists exactly
three: default-browser, react-19, nextjs-16.
The verification trigger was only needed because GitHub doesn't register the workflow_dispatch button (or accept REST dispatches) until the workflow file lives on the default branch. Now that the workflow has been exercised end-to-end (bundle build, upload, lab acceptance, fire-and-forget exit) the long-term contract — schedule cron + workflow_dispatch — is sufficient. After merge to develop, the workflow will fire nightly at 00:00 UTC and be manually triggerable from the Actions UI on any branch.
| }); | ||
| } | ||
|
|
||
| export const onRouterTransitionStart = lighthouseMode !== 'no-sentry' ? Sentry.captureRouterTransitionStart : undefined; |
Contributor
There was a problem hiding this comment.
Bug: Exporting onRouterTransitionStart as undefined in no-sentry mode will likely cause a TypeError during navigation in Next.js, crashing the test application.
Severity: MEDIUM
Suggested Fix
To fix this, avoid exporting onRouterTransitionStart as undefined. Instead, ensure that the module does not export onRouterTransitionStart at all when it's not needed. This allows the Next.js framework to correctly treat the hook as non-existent rather than attempting to call an undefined value.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
dev-packages/e2e-tests/test-applications/nextjs-16/instrumentation-client.ts#L42
Potential issue: In the `no-sentry` Lighthouse mode, `onRouterTransitionStart` is
exported as `undefined`. The Next.js framework, which consumes this hook, likely does
not check if the value is a function before calling it during router transitions. This
will cause a `TypeError: onRouterTransitionStart is not a function` at runtime, crashing
the application. This behavior is confined to the Lighthouse lab test environment and
will cause those specific tests to fail.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9ba92f4. Configure here.
| performance: { hints: false }, | ||
| plugins: [ | ||
| new webpack.EnvironmentPlugin(['E2E_TEST_DSN']), | ||
| new webpack.EnvironmentPlugin({ E2E_TEST_DSN: '', SENTRY_LIGHTHOUSE_MODE: '' }), |
There was a problem hiding this comment.
Missing DSN no longer fails the build
Low Severity
Changing webpack.EnvironmentPlugin(['E2E_TEST_DSN']) to webpack.EnvironmentPlugin({ E2E_TEST_DSN: '', SENTRY_LIGHTHOUSE_MODE: '' }) means a missing E2E_TEST_DSN no longer causes a build failure — it silently defaults to ''. With an empty DSN, Sentry.init won't capture events, so any E2E test using waitForError or waitForTransaction would time out instead of surfacing a clear build-time error. The SENTRY_LIGHTHOUSE_MODE default could be kept separate (e.g. a second EnvironmentPlugin call) to preserve the fail-fast behavior for the DSN.
Reviewed by Cursor Bugbot for commit 9ba92f4. Configure here.
| </div>, | ||
| ); | ||
| })(); | ||
| } |
There was a problem hiding this comment.
Async rendering changes E2E test app behavior
Low Severity
For normal E2E runs (env var unset), the react-19 app now renders the entire React tree inside an async IIFE after a dynamic import('@sentry/react'). Previously, rendering was synchronous. Since webpack code-splits the dynamic import into a separate chunk fetched over the network, the DOM stays empty until that fetch completes. Similarly, default-browser now initializes Sentry asynchronously, so window.onerror isn't hooked until the chunk loads. While Playwright's auto-wait mitigates this, it introduces a race between chunk loading and test interactions that could cause flaky timeouts under CI load.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9ba92f4. Configure here.
mydea
reviewed
May 13, 2026
| - v8 | ||
| - release/** | ||
| pull_request: | ||
| types: [opened, synchronize, reopened] |
Member
There was a problem hiding this comment.
unrelated to this here and can likely be removed from this PR?
mydea
reviewed
May 13, 2026
| # protocol in ~/Projects/sentry-lhci/docs/sentry-javascript-handoff.md. | ||
| # | ||
| # This workflow only builds + uploads — no Lighthouse runs on GitHub Actions. | ||
| # Never blocks merges (not in build.yml's required-jobs gate). |
Member
There was a problem hiding this comment.
Suggested change
| # Never blocks merges (not in build.yml's required-jobs gate). |
mydea
reviewed
May 13, 2026
| DISABLE_V8_COMPILE_CACHE: '1' | ||
|
|
||
| jobs: | ||
| job_build: |
Member
There was a problem hiding this comment.
IMHO we can just combine all of this into one job and avoid all the caching and artifact handling, making this much simpler!
mydea
reviewed
May 13, 2026
| // Sentry is loaded via dynamic `import()` so the `no-sentry` Lighthouse build can | ||
| // tree-shake the SDK out completely. Wrapped in an async IIFE because top-level await | ||
| // isn't supported by the webpack target used for this app's bundle. | ||
| if (lighthouseMode !== 'no-sentry') { |
Member
There was a problem hiding this comment.
hmm not a fan of this, this complicates/changes the core thing we are testing here, which is a really straightforward sentry setup. this is not really the recommended way to run sentry at all and we heavily discourage users from doing this - it will also have different performance implications and lighthouse scores compared to using sentry "normally".
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
I think we should start tracking lighthouse score to work towards improving the overhead we have.
Yes, I know our CI is already beefy, but right now, this is meant to run nightly or when adding the label
ci:lighthouseto post a comment.Right now this runs on every PR to track stats, we could change it that it runs nightly to remove some load from CI and push the results to our own https://github.com/GoogleChrome/lighthouse-ci/blob/main/docs/server.md that I would set up.