fix: remove LHCI js-yaml 3 path#186
Conversation
|
@codex review |
saagpatel
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60d2c12343
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (let run = 1; run <= numberOfRuns; run += 1) { | ||
| console.log(`\n== Lighthouse ${url} run ${run}/${numberOfRuns} ==`); | ||
| const lhr = runLighthouse(url, run); | ||
| allResults.push(...evaluate(lhr, `${safeName(url)}-${run}`)); |
There was a problem hiding this comment.
With the current .lighthouserc*.json files using numberOfRuns: 3 and no aggregationMethod, @lhci/cli did not evaluate each run separately; its default optimistic aggregation picks the best score for minScore and the lowest value for maxNumericValue before deciding an error. This line records each run's result independently, so one noisy CLS/LCP/performance sample now fails pnpm perf:lhci(:prod) even when the old gate would pass, making the replacement materially stricter/flaky instead of preserving the existing budget.
Useful? React with 👍 / 👎.
| }); | ||
|
|
||
| if (result.status !== 0) { | ||
| process.exit(result.status ?? 1); |
There was a problem hiding this comment.
When pnpm exec lighthouse exits nonzero after the preview server has been started (Chrome missing, audit crash, or page fetch failure), calling process.exit here bypasses the surrounding finally, so stopServer(server) is never reached. Because the server was spawned detached on Linux/macOS, Vite preview can be left running on port 4104 and poison later retries in the same job/session; throw or return the status and let the outer cleanup run instead.
Useful? React with 👍 / 👎.
|
Addressed both Codex P2s:
Local validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f15afbb809
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const cleanupTimer = setTimeout(() => { | ||
| reject( | ||
| new Error( | ||
| `Timed out waiting for Lighthouse server readiness pattern: ${readyPattern}`, | ||
| ), |
There was a problem hiding this comment.
When the preview command starts but never prints the configured readiness text (for example a changed Vite banner or a bad .lighthouserc startServerReadyPattern), this timeout rejects before waitForServer resolves, so the outer finally still has server === undefined and cannot terminate the spawned pnpm preview; that can leave port 4104 occupied and poison later Lighthouse runs. Please kill the child on this timeout path, or otherwise make the child available to the existing cleanup, before rejecting.
Useful? React with 👍 / 👎.
|
Addressed the readiness-timeout cleanup finding. Validation added locally:
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
What
@lhci/clidependency path with a repo-owned Lighthouse budget runner.pnpm perf:lhciandpnpm perf:lhci:prodcommand contracts as aliases to the new runner.@lhci/utils -> js-yaml@3.15.0present..prettierignoreforpnpm-lock.yamlso dependency PRs do not churn the pnpm lockfile through generic YAML formatting.Why
js-yaml@3.15.0through@lhci/utils@0.15.1, and forcing js-yaml v4 under LHCI would be brittle because LHCI utilities still use the old v3 API.How
scripts/perf/run-lighthouse-budget.mjsreads the existing.lighthouserc*.jsonfiles, starts the configured preview server, runs Lighthouse for the configured URLs/runs, and enforces category/audit assertions with warn/error semantics.package.jsonnow exposesperf:lighthouse*commands and keepsperf:lhci*aliases for compatibility.pnpm-workspace.yamlandpnpm-lock.yamldrop the LHCI/js-yaml-v3 override and dependency entries.Testing
pnpm why js-yaml-> onlyjs-yaml@4.3.0remains.rg -n '@lhci|js-yaml@3|js-yaml: 3\.15\.0|lighthouse@12\.6\.1|"@lhci/cli"' package.json pnpm-workspace.yaml pnpm-lock.yaml-> no matches.pnpm audit --audit-level high-> pass; still reports one unrelated moderate OpenTelemetry advisory through direct Lighthouse.pnpm exec prettier --check package.json pnpm-workspace.yaml scripts/perf/run-lighthouse-budget.mjs-> pass.pnpm exec eslint scripts/perf/run-lighthouse-budget.mjs-> pass.pnpm perf:build-> pass.pnpm perf:bundle-> pass.pnpm perf:assets-> pass.pnpm perf:memory-> pass.pnpm perf:lhci-> pass, 3 Lighthouse runs, all configured assertions passed.pnpm perf:lhci:prod-> pass, 3 Lighthouse runs, all production assertions passed.Performance impact
Risk/Notes
minScorechecks and auditmaxNumericValuechecks. If future configs use more LHCI assertion operators, the runner should be extended deliberately.pnpm audit --audit-level highpasses, but directlighthouse@13.3.0still brings one moderate OpenTelemetry advisory. That is separate from the js-yaml/LHCI path fixed here.Screenshots (UI only)
Lockfile rationale
@lhci/cli,@lhci/utils,js-yaml@3.15.0, and the oldlighthouse@12.6.1path that existed only through LHCI.