Add CI performance budgets and observability checks#99
Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR introduces end-to-end performance budget testing and CI observability infrastructure, including a Playwright test suite to measure page navigation and LCP metrics, scripts to validate canary budgets via Prometheus and Tempo, a metrics publishing pipeline for Prometheus Pushgateway or JUnit reports, TypeScript type definitions for YAML parsing, and updated documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant PW as Playwright Test
participant Config as perf-budget.yml
participant Browser as Browser
participant Results as Results JSON
participant Metrics as Metrics Publisher
participant PG as Prometheus/JUnit
PW->>Config: Load YAML config
Note over PW: Parse journeys & metrics
loop For each journey iteration
PW->>Browser: Navigate from→to
PW->>Browser: Collect LCP & nav metrics
PW->>Results: Store run metrics
end
PW->>Results: Aggregate percentiles (p90/p95)
PW->>PW: Assert vs thresholds
Metrics->>Results: Read JSON files
alt PUSHGATEWAY_URL set
Metrics->>PG: POST Prometheus metrics
else
Metrics->>PG: Write JUnit report
end
sequenceDiagram
participant CI as CI Pipeline
participant Prom as Prometheus
participant Tempo as Tempo
participant Check as check_canary_budgets.py
CI->>Check: Invoke with config
Check->>Prom: Query current metrics
Check->>Prom: Query baseline metrics
Check->>Check: Compute regression & evaluate
alt Budget threshold exceeded
Check->>Check: Mark failed
end
opt Tempo enabled
Check->>Tempo: Query trace durations
Check->>Check: Check regression vs tolerance
end
Check->>CI: Report results & exit status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes The diff introduces substantial new functionality across multiple domains: a feature-rich Playwright test suite with custom metrics collection and configuration parsing; a CI observability script with Prometheus/Tempo integration and regression analysis; a metrics publishing pipeline; and TypeScript dependencies. While individual pieces follow clear patterns, the heterogeneous nature of changes (test infrastructure, CI scripts, publishing, config), multiple files with distinct logic, and new type definitions and public functions require careful cross-functional review. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Comment |
![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.
ℹ️ 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
| for (const journey of perfConfig.journeys) { | ||
| test(journey.id, async () => { | ||
| const runs: Array<{ navigation_duration_ms: number; largest_contentful_paint_ms: number }> = []; | ||
|
|
||
| for (let iteration = 0; iteration < runCount; iteration += 1) { | ||
| const metrics = await collectNavigationMetrics(browser, journey.from_url, journey.to_url, journey.waits); | ||
| runs.push({ |
There was a problem hiding this comment.
Each generated test calls collectNavigationMetrics(browser, …) but browser is never defined in this spec, so Playwright will throw ReferenceError: browser is not defined before any metrics are gathered. The fixture must be pulled from the test context (e.g. test(journey.id, async ({ browser }) => …) or retrieved in a beforeAll) so the performance job can run at all.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/package.json (2)
34-34: Remove duplicate@playwright/testdependency.The
@playwright/testpackage appears twice in devDependencies at lines 34 and 57, both specifying version^1.56.0. This duplication is likely unintentional and should be consolidated to a single entry.Apply this diff to remove the duplicate:
"@types/react-dom": "^18", "@types/js-yaml": "^4.0.9", "autoprefixer": "^10.0.1", "eslint": "^8", "eslint-config-next": "14.2.0", "gltfpack": "0.25.0", "jest": "^29.7.0", "jest-environment-jsdom": "^29.7.0", "meshoptimizer": "0.25.0", "postcss": "^8", "tailwindcss": "^3.3.0", "ts-jest": "^29.2.5", "typescript": "^5", "gltfpack": "0.25.0", "meshoptimizer": "0.25.0", "vitest": "^1.6.0", - "@playwright/test": "^1.56.0", "ts-node": "^10.9.2", "js-yaml": "^4.1.0"Also applies to: 57-57
54-55: Remove duplicategltfpackandmeshoptimizerdependencies.Both
gltfpackandmeshoptimizerappear twice in devDependencies at lines 46/54 and 49/55. Consolidate to single entries.Apply this diff:
"postcss": "^8", "tailwindcss": "^3.3.0", "ts-jest": "^29.2.5", "typescript": "^5", - "gltfpack": "0.25.0", - "meshoptimizer": "0.25.0", "vitest": "^1.6.0", "@playwright/test": "^1.56.0", "ts-node": "^10.9.2", "js-yaml": "^4.1.0"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.github/workflows/ci.yml(1 hunks)docs/release-checklist.md(1 hunks)frontend/package.json(2 hunks)frontend/tests/e2e/perf/perf-budget.spec.ts(1 hunks)mkdocs.yml(1 hunks)perf-budget.yml(1 hunks)scripts/ci/__init__.py(1 hunks)scripts/ci/check_canary_budgets.py(1 hunks)scripts/publish-perf-metrics.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/tests/e2e/perf/perf-budget.spec.ts (2)
scripts/publish-perf-metrics.mjs (3)
entries(16-16)metric(87-87)metric(95-95)backend/api/main.py (1)
metrics(105-107)
🪛 actionlint (1.7.8)
.github/workflows/ci.yml
155-155: could not parse as YAML: could not find expected ':'
(syntax-check)
🪛 Checkov (3.2.334)
.github/workflows/ci.yml
[low] 135-136: Base64 High Entropy String
(CKV_SECRET_6)
🪛 LanguageTool
docs/release-checklist.md
[grammar] ~1-~1: Use correct spacing
Context: # Release Checklist This checklist captures the operational ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~3-~3: Use correct spacing
Context: ...have a consistent view of system health. ## 1. Verify CI performance budgets 1. Ins...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~5-~5: There might be a mistake here.
Context: ...th. ## 1. Verify CI performance budgets 1. Inspect the perf-budget job in the C...
(QB_NEW_EN_OTHER)
[grammar] ~9-~9: Use a hyphen to join words.
Context: ...remediation plan is documented. 3. Cross check the JUnit report (or Pushgateway m...
(QB_NEW_EN_HYPHEN)
[grammar] ~9-~9: Use correct spacing
Context: ...fore approving the release branch merge. ## 2. Review canary latency and error budge...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~11-~11: There might be a mistake here.
Context: ... Review canary latency and error budgets 1. Open the observability-budgets job l...
(QB_NEW_EN_OTHER)
[grammar] ~14-~14: Use correct spacing
Context: ...roll back before continuing the release. ## 3. Validate Grafana alerts and on-call n...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~16-~16: There might be a mistake here.
Context: ...Grafana alerts and on-call notifications 1. Check that the Grafana dashboard for the...
(QB_NEW_EN_OTHER)
[grammar] ~20-~20: Use correct spacing
Context: ...rage changed since the previous release. ## 4. Preview environment gating 1. Before...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~22-~22: Use correct spacing
Context: ...lease. ## 4. Preview environment gating 1. Before cutting a release tag, verify tha...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~25-~25: There might be a mistake here.
Context: ...ecks load within the CI-defined budgets. - Error budgets are green in Grafana/Prome...
(QB_NEW_EN)
[style] ~28-~28: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... ticket to document the state of the UI prior to launch. ## 5. Final approval 1. Ensur...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[grammar] ~28-~28: Use correct spacing
Context: ...ent the state of the UI prior to launch. ## 5. Final approval 1. Ensure all checkli...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~30-~30: Use correct spacing
Context: ...I prior to launch. ## 5. Final approval 1. Ensure all checklist items above are mar...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
🪛 Ruff (0.14.0)
scripts/ci/check_canary_budgets.py
49-49: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
52-52: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
52-52: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
106-108: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
110-110: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
113-113: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
113-113: Avoid specifying long messages outside the exception class
(TRY003)
120-120: Avoid specifying long messages outside the exception class
(TRY003)
124-124: Avoid specifying long messages outside the exception class
(TRY003)
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 156-156: syntax error: could not find expected ':'
(syntax)
🔇 Additional comments (20)
scripts/ci/__init__.py (1)
1-1: LGTM!The module docstring clearly describes the purpose of the CI helper scripts package and aligns with the new observability tooling introduced in this PR.
scripts/ci/check_canary_budgets.py (5)
16-36: LGTM!The
BudgetResultdataclass is well-designed with clear fields and a usefulto_linemethod for human-readable output. The optional fields are handled correctly with conditional formatting.
43-66: Function logic is correct with proper error handling.The Prometheus query implementation correctly constructs the API request, handles timeouts, and validates the response structure. The static analysis warnings about URL schemes (S310) are false positives in this CI context where URLs are controlled by environment configuration.
68-102: LGTM!The budget evaluation logic correctly:
- Queries both current and baseline values
- Applies unit scaling
- Computes regression ratio with proper zero-division guard (line 86)
- Evaluates pass/fail based on threshold and regression tolerance
The handling of
previous_valueas a falsy check correctly prevents division by zero when the previous value is 0.0.
104-135: LGTM!The Tempo query implementation correctly:
- Posts JSON query to the search API
- Validates response structure and trace data
- Extracts duration values with type checking (line 122)
- Returns the maximum trace duration for budget comparison
Error handling appropriately exits on missing or invalid data.
137-198: LGTM!The main function correctly:
- Reads configuration from environment with sensible defaults
- Gracefully skips validation when Prometheus config is missing (lines 142-144)
- Evaluates both required budgets (latency, error rate)
- Conditionally includes Tempo evaluation when configured
- Aggregates results and exits with appropriate status code
The early return when configuration is missing (line 144) prevents false failures in environments where observability isn't configured.
mkdocs.yml (1)
36-36: LGTM!The addition of the Release Checklist to the documentation navigation is well-positioned in the logical flow, appearing after introductory content and before deployment details.
frontend/package.json (1)
42-42: LGTM!The additions of
js-yamland@types/js-yamlare appropriate for parsing theperf-budget.ymlconfiguration in the new performance test suite. The version 4.1.0 is the latest stable release with safe-by-default behavior.Based on learnings.
Also applies to: 59-59
perf-budget.yml (1)
44-61: LGTM!The new journey configuration is well-structured:
- Clear navigation path from homepage to configurator
- Appropriate wait condition for canvas element with reasonable timeout
- Meaningful metrics (navigation duration, LCP) with percentile-based aggregation
- Reasonable thresholds: 3s for P90 navigation, 4s for P95 LCP
The configuration aligns with the new perf-budget test suite and CI workflow.
.github/workflows/ci.yml (4)
106-149: LGTM!The perf-budget job setup correctly:
- Sequences after test jobs to ensure baseline quality
- Installs required dependencies (Node, Playwright with system deps)
- Creates appropriate development environment configuration
- Starts the Docker stack and polls for service readiness with timeouts
- Uses reasonable timeout values (60 iterations × 2s = 2 min max wait)
The Checkov warning about line 135-136 is a false positive—the value is a placeholder string, not an actual secret.
150-194: LGTM!The inline Python seeding script correctly:
- Uses proper heredoc syntax for embedding Python in YAML (
python - <<'PY')- Creates test fixtures (materials, modules) via API calls
- Includes appropriate error handling with detailed failure messages
- Validates responses and extracts required IDs for dependent resources
The static analysis syntax errors are false positives—the actionlint and yamllint tools misparse the heredoc delimiter inside the YAML
runblock. The syntax is valid for GitHub Actions.
196-228: LGTM!The test execution and cleanup steps correctly:
- Run the specific performance budget test suite
- Configure multiple reporters (junit, line) for visibility
- Publish metrics conditionally using environment-driven configuration
- Upload artifacts regardless of test outcome (
if: always())- Ensure Docker stack cleanup runs even on failure
The workflow properly handles both Pushgateway-based metric publishing and JUnit report generation as fallback.
229-249: LGTM!The observability-budgets job correctly:
- Sequences after perf-budget to validate metrics
- Uses secrets for sensitive endpoint URLs (Prometheus, Tempo)
- Uses repository variables for queries (appropriate for non-sensitive config)
- Provides sensible default thresholds (3s latency, 2% error rate, 15% regression tolerance)
- Executes the budget validation script with proper environment setup
The separation of secrets vs. variables is appropriate for security and maintainability.
docs/release-checklist.md (1)
1-34: LGTM!The release checklist provides comprehensive operational guidance that ties together:
- CI performance budget validation (perf-budget job)
- Canary budget evaluation (observability-budgets job)
- Alert configuration and on-call routing
- Preview environment gating
- Approval workflow
The structure is clear and actionable, providing specific steps for each gate. The checklist effectively documents the release governance process introduced by this PR's observability infrastructure.
scripts/publish-perf-metrics.mjs (2)
65-80: LGTM!The Pushgateway integration correctly:
- URL-encodes job and journey labels to prevent injection (line 69)
- Posts to the correct endpoint with proper content type
- Validates response status and includes error details
- Processes metrics for each journey independently
135-154: LGTM!The main flow correctly:
- Reads journey metrics from the results directory
- Gracefully skips when no results are found (lines 142-144)
- Conditionally publishes to Pushgateway or generates JUnit based on configuration
- Provides clear logging for each outcome
The error handling (lines 156-159) appropriately sets a non-zero exit code for failures.
frontend/tests/e2e/perf/perf-budget.spec.ts (4)
1-5: LGTM!Imports are appropriate for the performance testing requirements.
7-53: LGTM!Type definitions are well-structured and provide clear contracts for configuration and results.
55-68: LGTM!The percentile calculation correctly implements linear interpolation between sorted values, following standard statistical methods.
177-189: Ignore LCP extraction change
entry.startTimealready returns the correct LCP time (it yieldsrenderTimeif non-zero, otherwiseloadTime).Likely an incorrect or invalid review comment.
| const loadPerfBudget = (): PerfBudgetConfig => { | ||
| const configPath = process.env.PERF_BUDGET_FILE | ||
| ? path.resolve(process.cwd(), process.env.PERF_BUDGET_FILE) | ||
| : path.resolve(__dirname, '../../../../perf-budget.yml'); | ||
|
|
||
| const raw = fs.readFileSync(configPath, 'utf8'); | ||
| return yaml.load(raw) as PerfBudgetConfig; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Add error handling for file operations and config validation.
The function lacks error handling for:
- Missing or unreadable config files
- YAML parsing errors
- Invalid config structure after loading
Additionally, __dirname usage on line 73 may not be available in ES module contexts or certain TypeScript configurations.
Apply this diff to add error handling:
const loadPerfBudget = (): PerfBudgetConfig => {
const configPath = process.env.PERF_BUDGET_FILE
? path.resolve(process.cwd(), process.env.PERF_BUDGET_FILE)
: path.resolve(__dirname, '../../../../perf-budget.yml');
+ if (!fs.existsSync(configPath)) {
+ throw new Error(`Performance budget config not found at: ${configPath}`);
+ }
+
+ try {
const raw = fs.readFileSync(configPath, 'utf8');
- return yaml.load(raw) as PerfBudgetConfig;
+ const config = yaml.load(raw) as PerfBudgetConfig;
+
+ if (!config || typeof config !== 'object') {
+ throw new Error('Invalid config structure');
+ }
+
+ return config;
+ } catch (error) {
+ throw new Error(`Failed to load perf budget config: ${error instanceof Error ? error.message : String(error)}`);
+ }
};Run the following script to verify if __dirname is available in the TypeScript/build configuration:
🏁 Script executed:
#!/bin/bash
# Check if the project uses ES modules and how __dirname is handled
# Check package.json for module type
echo "=== Checking package.json for module type ==="
fd -t f '^package\.json$' frontend | head -1 | xargs cat | jq -r '.type // "commonjs"'
# Check tsconfig.json for module settings
echo -e "\n=== Checking TypeScript module configuration ==="
fd -t f '^tsconfig.*\.json$' frontend | head -3 | xargs -I {} sh -c 'echo "File: {}" && cat {} | jq -r ".compilerOptions.module // \"not set\""'
# Search for other usages of __dirname in test files
echo -e "\n=== Other __dirname usages in test files ==="
rg -t ts '__dirname' frontend/tests/ -C 2Length of output: 1122
Optional: Add error handling and validation in loadPerfBudget.
The loader may throw on missing files or invalid YAML—wrapping readFileSync and yaml.load in a try/catch and validating the parsed object can provide clearer failure messages.
🤖 Prompt for AI Agents
In frontend/tests/e2e/perf/perf-budget.spec.ts around lines 70 to 77, wrap the
file read and YAML parsing in a try/catch to surface a clear, descriptive error
when the PERF_BUDGET_FILE is missing or the YAML is invalid; before reading,
optionally check fs.existsSync(configPath) and throw a clear error if missing,
then call fs.readFileSync and yaml.load inside the try block, validate the
returned object matches the expected PerfBudgetConfig shape (required
fields/types) and throw a descriptive error if validation fails, and rethrow or
wrap parsing/file I/O errors with contextual messages so test failures indicate
exactly whether the file was missing, unreadable, or the YAML invalid.
| const toQuantile = (aggregation: string): number | undefined => { | ||
| const match = /^p(\d{1,2})$/i.exec(aggregation.trim()); | ||
| if (!match) { | ||
| return undefined; | ||
| } | ||
| const percentileNumber = Number(match[1]); | ||
| return percentileNumber / 100; | ||
| }; | ||
|
|
||
| const ensureResultsDir = (): string => { | ||
| const dir = process.env.PERF_RESULTS_DIR | ||
| ? path.resolve(process.cwd(), process.env.PERF_RESULTS_DIR) | ||
| : path.resolve(__dirname, '../../../../perf-results'); | ||
| fs.mkdirSync(dir, { recursive: true }); | ||
| return dir; | ||
| }; | ||
|
|
||
| const writeJourneyResult = (dir: string, result: JourneyResult): void => { | ||
| const filePath = path.join(dir, `${result.id}.json`); | ||
| fs.writeFileSync(filePath, JSON.stringify(result, null, 2), 'utf8'); | ||
| }; | ||
|
|
||
| const attachJourneyResult = async (result: JourneyResult): Promise<void> => { | ||
| await test.info().attach(`${result.id}-metrics.json`, { | ||
| body: Buffer.from(JSON.stringify(result, null, 2)), | ||
| contentType: 'application/json', | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding error handling to file write operations.
The writeJourneyResult function (lines 96-99) lacks error handling for file write failures. While not critical for test execution, it could cause silent failures in results persistence.
Apply this diff to add error handling:
const writeJourneyResult = (dir: string, result: JourneyResult): void => {
const filePath = path.join(dir, `${result.id}.json`);
- fs.writeFileSync(filePath, JSON.stringify(result, null, 2), 'utf8');
+ try {
+ fs.writeFileSync(filePath, JSON.stringify(result, null, 2), 'utf8');
+ } catch (error) {
+ console.warn(`Failed to write journey result to ${filePath}:`, error);
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const toQuantile = (aggregation: string): number | undefined => { | |
| const match = /^p(\d{1,2})$/i.exec(aggregation.trim()); | |
| if (!match) { | |
| return undefined; | |
| } | |
| const percentileNumber = Number(match[1]); | |
| return percentileNumber / 100; | |
| }; | |
| const ensureResultsDir = (): string => { | |
| const dir = process.env.PERF_RESULTS_DIR | |
| ? path.resolve(process.cwd(), process.env.PERF_RESULTS_DIR) | |
| : path.resolve(__dirname, '../../../../perf-results'); | |
| fs.mkdirSync(dir, { recursive: true }); | |
| return dir; | |
| }; | |
| const writeJourneyResult = (dir: string, result: JourneyResult): void => { | |
| const filePath = path.join(dir, `${result.id}.json`); | |
| fs.writeFileSync(filePath, JSON.stringify(result, null, 2), 'utf8'); | |
| }; | |
| const attachJourneyResult = async (result: JourneyResult): Promise<void> => { | |
| await test.info().attach(`${result.id}-metrics.json`, { | |
| body: Buffer.from(JSON.stringify(result, null, 2)), | |
| contentType: 'application/json', | |
| }); | |
| }; | |
| const writeJourneyResult = (dir: string, result: JourneyResult): void => { | |
| const filePath = path.join(dir, `${result.id}.json`); | |
| try { | |
| fs.writeFileSync(filePath, JSON.stringify(result, null, 2), 'utf8'); | |
| } catch (error) { | |
| console.warn(`Failed to write journey result to ${filePath}:`, error); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In frontend/tests/e2e/perf/perf-budget.spec.ts around lines 79 to 106, the
writeJourneyResult function (lines 96-99) writes to disk without error handling;
wrap the fs.writeFileSync call in a try/catch, build the same filePath before
writing, and on failure log a clear error (including filePath and the caught
error) and attach the error/details to the current test via test.info().attach
so the failure is recorded; do not let the exception crash the test run (do not
rethrow) unless you intentionally want the test to fail.
| const collectNavigationMetrics = async ( | ||
| browser: Browser, | ||
| fromUrl: string, | ||
| toUrl: string, | ||
| waits: WaitConfig[] | undefined, | ||
| ): Promise<{ navigation: number; lcp: number }> => { | ||
| const context = await browser.newContext(); | ||
|
|
||
| await context.addInitScript(() => { | ||
| (window as typeof window & { | ||
| __lcpEntries?: PerformanceEntry[]; | ||
| __lcpObserver?: PerformanceObserver; | ||
| }).__lcpEntries = []; | ||
| }); | ||
|
|
||
| const page = await context.newPage(); | ||
| await page.goto(fromUrl, { waitUntil: 'networkidle' }); | ||
|
|
||
| await page.evaluate(() => { | ||
| const globalWindow = window as typeof window & { | ||
| __lcpEntries?: PerformanceEntry[]; | ||
| __lcpObserver?: PerformanceObserver; | ||
| }; | ||
|
|
||
| globalWindow.__lcpEntries = []; | ||
| if (globalWindow.__lcpObserver) { | ||
| globalWindow.__lcpObserver.disconnect(); | ||
| } | ||
| globalWindow.__lcpObserver = new PerformanceObserver((entryList) => { | ||
| const entries = entryList.getEntries(); | ||
| globalWindow.__lcpEntries = [ | ||
| ...(globalWindow.__lcpEntries ?? []), | ||
| ...entries, | ||
| ]; | ||
| }); | ||
| globalWindow.__lcpObserver.observe({ type: 'largest-contentful-paint', buffered: true }); | ||
|
|
||
| performance.clearResourceTimings(); | ||
| performance.clearMeasures(); | ||
| performance.clearMarks(); | ||
| }); | ||
|
|
||
| await page.goto(toUrl, { waitUntil: 'networkidle' }); | ||
|
|
||
| if (waits) { | ||
| for (const wait of waits) { | ||
| if (wait.type === 'selector' && wait.selector) { | ||
| await page.waitForSelector(wait.selector, { | ||
| state: 'attached', | ||
| timeout: wait.timeout_ms ?? 30_000, | ||
| }); | ||
| } | ||
| if (wait.type === 'networkidle') { | ||
| await page.waitForLoadState('networkidle', { timeout: wait.timeout_ms ?? 30_000 }); | ||
| if (wait.idle_ms) { | ||
| await page.waitForTimeout(wait.idle_ms); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| await page.waitForTimeout(500); | ||
|
|
||
| const navigationDuration = await page.evaluate(() => { | ||
| const entries = performance.getEntriesByType('navigation'); | ||
| const last = entries[entries.length - 1] as PerformanceNavigationTiming | undefined; | ||
| return last ? last.duration : Number.NaN; | ||
| }); | ||
|
|
||
| const lcp = await page.evaluate(() => { | ||
| const globalWindow = window as typeof window & { | ||
| __lcpEntries?: PerformanceEntry[]; | ||
| }; | ||
| const entries = (globalWindow.__lcpEntries ?? []) as Array<PerformanceEntry & { renderTime?: number; loadTime?: number; startTime: number }>; | ||
| if (!entries.length) { | ||
| return Number.NaN; | ||
| } | ||
| return entries.reduce((acc, entry) => { | ||
| const candidate = entry.startTime ?? entry.renderTime ?? entry.loadTime ?? 0; | ||
| return Math.max(acc, candidate); | ||
| }, 0); | ||
| }); | ||
|
|
||
| await context.close(); | ||
|
|
||
| return { navigation: navigationDuration, lcp }; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add resource cleanup in error scenarios.
The function creates a browser context (line 114) but doesn't guarantee cleanup if an error occurs between context creation and the close call (line 191). This could lead to resource leaks in test failures.
Apply this diff to ensure cleanup:
): Promise<{ navigation: number; lcp: number }> => {
const context = await browser.newContext();
+
+ try {
+ await context.addInitScript(() => {
+ (window as typeof window & {
+ __lcpEntries?: PerformanceEntry[];
+ __lcpObserver?: PerformanceObserver;
+ }).__lcpEntries = [];
+ });
- await context.addInitScript(() => {
- (window as typeof window & {
- __lcpEntries?: PerformanceEntry[];
- __lcpObserver?: PerformanceObserver;
- }).__lcpEntries = [];
- });
-
- const page = await context.newPage();
+ const page = await context.newPage();
// ... rest of the function logic ...
- await context.close();
-
- return { navigation: navigationDuration, lcp };
+ return { navigation: navigationDuration, lcp };
+ } finally {
+ await context.close();
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const collectNavigationMetrics = async ( | |
| browser: Browser, | |
| fromUrl: string, | |
| toUrl: string, | |
| waits: WaitConfig[] | undefined, | |
| ): Promise<{ navigation: number; lcp: number }> => { | |
| const context = await browser.newContext(); | |
| await context.addInitScript(() => { | |
| (window as typeof window & { | |
| __lcpEntries?: PerformanceEntry[]; | |
| __lcpObserver?: PerformanceObserver; | |
| }).__lcpEntries = []; | |
| }); | |
| const page = await context.newPage(); | |
| await page.goto(fromUrl, { waitUntil: 'networkidle' }); | |
| await page.evaluate(() => { | |
| const globalWindow = window as typeof window & { | |
| __lcpEntries?: PerformanceEntry[]; | |
| __lcpObserver?: PerformanceObserver; | |
| }; | |
| globalWindow.__lcpEntries = []; | |
| if (globalWindow.__lcpObserver) { | |
| globalWindow.__lcpObserver.disconnect(); | |
| } | |
| globalWindow.__lcpObserver = new PerformanceObserver((entryList) => { | |
| const entries = entryList.getEntries(); | |
| globalWindow.__lcpEntries = [ | |
| ...(globalWindow.__lcpEntries ?? []), | |
| ...entries, | |
| ]; | |
| }); | |
| globalWindow.__lcpObserver.observe({ type: 'largest-contentful-paint', buffered: true }); | |
| performance.clearResourceTimings(); | |
| performance.clearMeasures(); | |
| performance.clearMarks(); | |
| }); | |
| await page.goto(toUrl, { waitUntil: 'networkidle' }); | |
| if (waits) { | |
| for (const wait of waits) { | |
| if (wait.type === 'selector' && wait.selector) { | |
| await page.waitForSelector(wait.selector, { | |
| state: 'attached', | |
| timeout: wait.timeout_ms ?? 30_000, | |
| }); | |
| } | |
| if (wait.type === 'networkidle') { | |
| await page.waitForLoadState('networkidle', { timeout: wait.timeout_ms ?? 30_000 }); | |
| if (wait.idle_ms) { | |
| await page.waitForTimeout(wait.idle_ms); | |
| } | |
| } | |
| } | |
| } | |
| await page.waitForTimeout(500); | |
| const navigationDuration = await page.evaluate(() => { | |
| const entries = performance.getEntriesByType('navigation'); | |
| const last = entries[entries.length - 1] as PerformanceNavigationTiming | undefined; | |
| return last ? last.duration : Number.NaN; | |
| }); | |
| const lcp = await page.evaluate(() => { | |
| const globalWindow = window as typeof window & { | |
| __lcpEntries?: PerformanceEntry[]; | |
| }; | |
| const entries = (globalWindow.__lcpEntries ?? []) as Array<PerformanceEntry & { renderTime?: number; loadTime?: number; startTime: number }>; | |
| if (!entries.length) { | |
| return Number.NaN; | |
| } | |
| return entries.reduce((acc, entry) => { | |
| const candidate = entry.startTime ?? entry.renderTime ?? entry.loadTime ?? 0; | |
| return Math.max(acc, candidate); | |
| }, 0); | |
| }); | |
| await context.close(); | |
| return { navigation: navigationDuration, lcp }; | |
| }; | |
| const collectNavigationMetrics = async ( | |
| browser: Browser, | |
| fromUrl: string, | |
| toUrl: string, | |
| waits: WaitConfig[] | undefined, | |
| ): Promise<{ navigation: number; lcp: number }> => { | |
| const context = await browser.newContext(); | |
| try { | |
| await context.addInitScript(() => { | |
| (window as typeof window & { | |
| __lcpEntries?: PerformanceEntry[]; | |
| __lcpObserver?: PerformanceObserver; | |
| }).__lcpEntries = []; | |
| }); | |
| const page = await context.newPage(); | |
| await page.goto(fromUrl, { waitUntil: 'networkidle' }); | |
| await page.evaluate(() => { | |
| const globalWindow = window as typeof window & { | |
| __lcpEntries?: PerformanceEntry[]; | |
| __lcpObserver?: PerformanceObserver; | |
| }; | |
| globalWindow.__lcpEntries = []; | |
| if (globalWindow.__lcpObserver) { | |
| globalWindow.__lcpObserver.disconnect(); | |
| } | |
| globalWindow.__lcpObserver = new PerformanceObserver((entryList) => { | |
| const entries = entryList.getEntries(); | |
| globalWindow.__lcpEntries = [ | |
| ...(globalWindow.__lcpEntries ?? []), | |
| ...entries, | |
| ]; | |
| }); | |
| globalWindow.__lcpObserver.observe({ type: 'largest-contentful-paint', buffered: true }); | |
| performance.clearResourceTimings(); | |
| performance.clearMeasures(); | |
| performance.clearMarks(); | |
| }); | |
| await page.goto(toUrl, { waitUntil: 'networkidle' }); | |
| if (waits) { | |
| for (const wait of waits) { | |
| if (wait.type === 'selector' && wait.selector) { | |
| await page.waitForSelector(wait.selector, { | |
| state: 'attached', | |
| timeout: wait.timeout_ms ?? 30_000, | |
| }); | |
| } | |
| if (wait.type === 'networkidle') { | |
| await page.waitForLoadState('networkidle', { timeout: wait.timeout_ms ?? 30_000 }); | |
| if (wait.idle_ms) { | |
| await page.waitForTimeout(wait.idle_ms); | |
| } | |
| } | |
| } | |
| } | |
| await page.waitForTimeout(500); | |
| const navigationDuration = await page.evaluate(() => { | |
| const entries = performance.getEntriesByType('navigation'); | |
| const last = entries[entries.length - 1] as PerformanceNavigationTiming | undefined; | |
| return last ? last.duration : Number.NaN; | |
| }); | |
| const lcp = await page.evaluate(() => { | |
| const globalWindow = window as typeof window & { | |
| __lcpEntries?: PerformanceEntry[]; | |
| }; | |
| const entries = (globalWindow.__lcpEntries ?? []) as Array< | |
| PerformanceEntry & { renderTime?: number; loadTime?: number; startTime: number } | |
| >; | |
| if (!entries.length) { | |
| return Number.NaN; | |
| } | |
| return entries.reduce((acc, entry) => { | |
| const candidate = entry.startTime ?? entry.renderTime ?? entry.loadTime ?? 0; | |
| return Math.max(acc, candidate); | |
| }, 0); | |
| }); | |
| return { navigation: navigationDuration, lcp }; | |
| } finally { | |
| await context.close(); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In frontend/tests/e2e/perf/perf-budget.spec.ts around lines 108 to 194, the
function creates a browser context at line 114 but only calls context.close() at
the end, so if an exception is thrown before line 191 the context (and page) may
never be closed; to fix this, allocate the context (and page) to variables
scoped outside the try, then wrap the navigation/measurement logic in try { ...
} finally { if (page) await page.close().catch(() => {}); if (context) await
context.close().catch(() => {}); } so the context and page are always cleaned up
even on errors.
| } | ||
| } | ||
|
|
||
| await page.waitForTimeout(500); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document or make the fixed delay configurable.
Line 169 introduces an arbitrary 500ms wait after processing configured waits. This magic number lacks explanation for why this specific duration is needed.
Consider either:
- Adding a comment explaining why this delay is necessary, or
- Making it configurable via the journey configuration, or
- Removing it if it's not essential
+ // Brief delay to ensure all performance entries are captured
await page.waitForTimeout(500);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await page.waitForTimeout(500); | |
| // Brief delay to ensure all performance entries are captured | |
| await page.waitForTimeout(500); |
🤖 Prompt for AI Agents
In frontend/tests/e2e/perf/perf-budget.spec.ts around line 169, there is a
hardcoded 500ms wait (await page.waitForTimeout(500)) acting as an unexplained
"magic" delay; replace or document it by either (A) adding a brief comment that
explains the purpose and why 500ms was chosen, (B) exposing the delay as a
configurable value in the journey/test config (e.g., a named timeout like
postWaitDelayMs) and using that config value here, or (C) remove the wait
entirely if verification shows it is unnecessary; implement the chosen option
and update any related tests or config defaults accordingly.
| if (lcpValues.length) { | ||
| const metricConfig = metricConfigById.get('largest-contentful-paint'); | ||
| const quantile = metricConfig ? toQuantile(metricConfig.aggregation) : 0.95; | ||
| const value = percentile(lcpValues, quantile ?? 0.95); | ||
| aggregated.largest_contentful_paint = { | ||
| quantile: metricConfig ? metricConfig.aggregation.toLowerCase() : 'p95', | ||
| value_ms: value, | ||
| threshold_ms: metricConfig?.threshold, | ||
| passed: metricConfig ? value <= metricConfig.threshold : true, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Critical: Property naming inconsistency with downstream script.
Line 246 uses largest_contentful_paint (with underscores) as the property name, but the publish metrics script at scripts/publish-perf-metrics.mjs line 94 expects largest-contentful-paint (with hyphens), causing the downstream script to receive undefined values.
Apply this diff to maintain consistency with the expected format:
if (lcpValues.length) {
const metricConfig = metricConfigById.get('largest-contentful-paint');
const quantile = metricConfig ? toQuantile(metricConfig.aggregation) : 0.95;
const value = percentile(lcpValues, quantile ?? 0.95);
- aggregated.largest_contentful_paint = {
+ aggregated['largest-contentful-paint'] = {
quantile: metricConfig ? metricConfig.aggregation.toLowerCase() : 'p95',
value_ms: value,
threshold_ms: metricConfig?.threshold,
passed: metricConfig ? value <= metricConfig.threshold : true,
};
}Also update the type definition to match:
type JourneyResult = {
id: string;
description?: string;
runs: Array<{
navigation_duration_ms: number;
largest_contentful_paint_ms: number;
}>;
aggregated: {
navigation_duration?: JourneyMetricSummary;
- largest_contentful_paint?: JourneyMetricSummary;
+ 'largest-contentful-paint'?: JourneyMetricSummary;
};
};And update line 270:
- if (aggregated.largest_contentful_paint && aggregated.largest_contentful_paint.threshold_ms !== undefined) {
+ if (aggregated['largest-contentful-paint'] && aggregated['largest-contentful-paint'].threshold_ms !== undefined) {
- expect(aggregated.largest_contentful_paint.value_ms, 'homepage to configurator LCP P95 should remain under budget').toBeLessThanOrEqual(
+ expect(aggregated['largest-contentful-paint'].value_ms, 'homepage to configurator LCP P95 should remain under budget').toBeLessThanOrEqual(
- aggregated.largest_contentful_paint.threshold_ms,
+ aggregated['largest-contentful-paint'].threshold_ms,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (lcpValues.length) { | |
| const metricConfig = metricConfigById.get('largest-contentful-paint'); | |
| const quantile = metricConfig ? toQuantile(metricConfig.aggregation) : 0.95; | |
| const value = percentile(lcpValues, quantile ?? 0.95); | |
| aggregated.largest_contentful_paint = { | |
| quantile: metricConfig ? metricConfig.aggregation.toLowerCase() : 'p95', | |
| value_ms: value, | |
| threshold_ms: metricConfig?.threshold, | |
| passed: metricConfig ? value <= metricConfig.threshold : true, | |
| }; | |
| } | |
| if (lcpValues.length) { | |
| const metricConfig = metricConfigById.get('largest-contentful-paint'); | |
| const quantile = metricConfig ? toQuantile(metricConfig.aggregation) : 0.95; | |
| const value = percentile(lcpValues, quantile ?? 0.95); | |
| aggregated['largest-contentful-paint'] = { | |
| quantile: metricConfig ? metricConfig.aggregation.toLowerCase() : 'p95', | |
| value_ms: value, | |
| threshold_ms: metricConfig?.threshold, | |
| passed: metricConfig ? value <= metricConfig.threshold : true, | |
| }; | |
| } |
| if (lcpValues.length) { | |
| const metricConfig = metricConfigById.get('largest-contentful-paint'); | |
| const quantile = metricConfig ? toQuantile(metricConfig.aggregation) : 0.95; | |
| const value = percentile(lcpValues, quantile ?? 0.95); | |
| aggregated.largest_contentful_paint = { | |
| quantile: metricConfig ? metricConfig.aggregation.toLowerCase() : 'p95', | |
| value_ms: value, | |
| threshold_ms: metricConfig?.threshold, | |
| passed: metricConfig ? value <= metricConfig.threshold : true, | |
| }; | |
| } | |
| type JourneyResult = { | |
| id: string; | |
| description?: string; | |
| runs: Array<{ | |
| navigation_duration_ms: number; | |
| largest_contentful_paint_ms: number; | |
| }>; | |
| aggregated: { | |
| navigation_duration?: JourneyMetricSummary; | |
| 'largest-contentful-paint'?: JourneyMetricSummary; | |
| }; | |
| }; |
| if (lcpValues.length) { | |
| const metricConfig = metricConfigById.get('largest-contentful-paint'); | |
| const quantile = metricConfig ? toQuantile(metricConfig.aggregation) : 0.95; | |
| const value = percentile(lcpValues, quantile ?? 0.95); | |
| aggregated.largest_contentful_paint = { | |
| quantile: metricConfig ? metricConfig.aggregation.toLowerCase() : 'p95', | |
| value_ms: value, | |
| threshold_ms: metricConfig?.threshold, | |
| passed: metricConfig ? value <= metricConfig.threshold : true, | |
| }; | |
| } | |
| if (aggregated['largest-contentful-paint'] && aggregated['largest-contentful-paint'].threshold_ms !== undefined) { | |
| expect( | |
| aggregated['largest-contentful-paint'].value_ms, | |
| 'homepage to configurator LCP P95 should remain under budget' | |
| ).toBeLessThanOrEqual( | |
| aggregated['largest-contentful-paint'].threshold_ms, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In frontend/tests/e2e/perf/perf-budget.spec.ts around lines 242 to 252, the test
populates aggregated.largest_contentful_paint (underscores) but downstream
scripts expect the hyphenated key 'largest-contentful-paint'; change the
property name to use 'largest-contentful-paint' when assigning the aggregated
metric, update the corresponding test/type definition so the aggregated object
type includes 'largest-contentful-paint' (string key with hyphens) instead of
largest_contentful_paint, and adjust the reference at line 270 to read the
hyphenated key as well so the test produces the same shape expected by
scripts/publish-perf-metrics.mjs.
| if (aggregated.navigation_duration && aggregated.navigation_duration.threshold_ms !== undefined) { | ||
| expect(aggregated.navigation_duration.value_ms, 'homepage to configurator navigation P90 should remain under budget').toBeLessThanOrEqual( | ||
| aggregated.navigation_duration.threshold_ms, | ||
| ); | ||
| } | ||
|
|
||
| if (aggregated.largest_contentful_paint && aggregated.largest_contentful_paint.threshold_ms !== undefined) { | ||
| expect(aggregated.largest_contentful_paint.value_ms, 'homepage to configurator LCP P95 should remain under budget').toBeLessThanOrEqual( | ||
| aggregated.largest_contentful_paint.threshold_ms, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix hardcoded assertion messages to reflect actual journey.
Lines 265 and 271 contain hardcoded assertion messages referring to "homepage to configurator" but this test runs for all journeys in a loop. The messages should dynamically reflect the actual journey being tested.
Apply this diff to use dynamic messages:
if (aggregated.navigation_duration && aggregated.navigation_duration.threshold_ms !== undefined) {
- expect(aggregated.navigation_duration.value_ms, 'homepage to configurator navigation P90 should remain under budget').toBeLessThanOrEqual(
+ expect(aggregated.navigation_duration.value_ms, `${journey.id} navigation ${aggregated.navigation_duration.quantile.toUpperCase()} should remain under budget`).toBeLessThanOrEqual(
aggregated.navigation_duration.threshold_ms,
);
}
if (aggregated.largest_contentful_paint && aggregated.largest_contentful_paint.threshold_ms !== undefined) {
- expect(aggregated.largest_contentful_paint.value_ms, 'homepage to configurator LCP P95 should remain under budget').toBeLessThanOrEqual(
+ expect(aggregated.largest_contentful_paint.value_ms, `${journey.id} LCP ${aggregated.largest_contentful_paint.quantile.toUpperCase()} should remain under budget`).toBeLessThanOrEqual(
aggregated.largest_contentful_paint.threshold_ms,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (aggregated.navigation_duration && aggregated.navigation_duration.threshold_ms !== undefined) { | |
| expect(aggregated.navigation_duration.value_ms, 'homepage to configurator navigation P90 should remain under budget').toBeLessThanOrEqual( | |
| aggregated.navigation_duration.threshold_ms, | |
| ); | |
| } | |
| if (aggregated.largest_contentful_paint && aggregated.largest_contentful_paint.threshold_ms !== undefined) { | |
| expect(aggregated.largest_contentful_paint.value_ms, 'homepage to configurator LCP P95 should remain under budget').toBeLessThanOrEqual( | |
| aggregated.largest_contentful_paint.threshold_ms, | |
| ); | |
| } | |
| if (aggregated.navigation_duration && aggregated.navigation_duration.threshold_ms !== undefined) { | |
| expect( | |
| aggregated.navigation_duration.value_ms, | |
| `${journey.id} navigation ${aggregated.navigation_duration.quantile.toUpperCase()} should remain under budget` | |
| ).toBeLessThanOrEqual( | |
| aggregated.navigation_duration.threshold_ms, | |
| ); | |
| } | |
| if (aggregated.largest_contentful_paint && aggregated.largest_contentful_paint.threshold_ms !== undefined) { | |
| expect( | |
| aggregated.largest_contentful_paint.value_ms, | |
| `${journey.id} LCP ${aggregated.largest_contentful_paint.quantile.toUpperCase()} should remain under budget` | |
| ).toBeLessThanOrEqual( | |
| aggregated.largest_contentful_paint.threshold_ms, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In frontend/tests/e2e/perf/perf-budget.spec.ts around lines 264 to 274, the
assertion messages are hardcoded as "homepage to configurator" but this test
runs for multiple journeys; replace those hardcoded strings with a dynamic
message that includes the current journey identifier (for example using the loop
variable like journey.name or journeyId) so the expect messages read something
like `${journey.name} navigation P90 should remain under budget` and
`${journey.name} LCP P95 should remain under budget`; update both expect calls
to use the dynamic journey-based message variable.
| const readJourneyFiles = async () => { | ||
| try { | ||
| const entries = await fs.readdir(RESULTS_DIR); | ||
| const jsonFiles = entries.filter((entry) => entry.endsWith('.json')); | ||
| const payloads = []; | ||
| for (const file of jsonFiles) { | ||
| const content = await fs.readFile(path.join(RESULTS_DIR, file), 'utf8'); | ||
| payloads.push(JSON.parse(content)); | ||
| } | ||
| return payloads; | ||
| } catch (error) { | ||
| if ((error instanceof Error && 'code' in error && error.code === 'ENOENT') || | ||
| (typeof error === 'object' && error !== null && 'code' in error && error.code === 'ENOENT')) { | ||
| return []; | ||
| } | ||
| throw error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify error handling for ENOENT.
The error handling at lines 25-26 has redundant checks. Modern Node.js errors from fs operations are always Error instances with a code property, so the complex typeof checks are unnecessary.
Apply this diff to simplify:
} catch (error) {
- if ((error instanceof Error && 'code' in error && error.code === 'ENOENT') ||
- (typeof error === 'object' && error !== null && 'code' in error && error.code === 'ENOENT')) {
+ if (error?.code === 'ENOENT') {
return [];
}
throw error;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const readJourneyFiles = async () => { | |
| try { | |
| const entries = await fs.readdir(RESULTS_DIR); | |
| const jsonFiles = entries.filter((entry) => entry.endsWith('.json')); | |
| const payloads = []; | |
| for (const file of jsonFiles) { | |
| const content = await fs.readFile(path.join(RESULTS_DIR, file), 'utf8'); | |
| payloads.push(JSON.parse(content)); | |
| } | |
| return payloads; | |
| } catch (error) { | |
| if ((error instanceof Error && 'code' in error && error.code === 'ENOENT') || | |
| (typeof error === 'object' && error !== null && 'code' in error && error.code === 'ENOENT')) { | |
| return []; | |
| } | |
| throw error; | |
| } | |
| }; | |
| const readJourneyFiles = async () => { | |
| try { | |
| const entries = await fs.readdir(RESULTS_DIR); | |
| const jsonFiles = entries.filter((entry) => entry.endsWith('.json')); | |
| const payloads = []; | |
| for (const file of jsonFiles) { | |
| const content = await fs.readFile(path.join(RESULTS_DIR, file), 'utf8'); | |
| payloads.push(JSON.parse(content)); | |
| } | |
| return payloads; | |
| } catch (error) { | |
| if (error?.code === 'ENOENT') { | |
| return []; | |
| } | |
| throw error; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In scripts/publish-perf-metrics.mjs around lines 14–31, the catch block uses
redundant runtime type checks to detect ENOENT; simplify the conditional to a
single check for the code property (e.g. if (error.code === 'ENOENT') return
[]), removing the unnecessary instanceof/typeof branches so the catch returns an
empty array on missing directory and rethrows otherwise.
| const formatPrometheusBody = (journey) => { | ||
| const labelsBase = `journey="${journey.id}"`; | ||
| const lines = [ | ||
| '# TYPE ci_navigation_duration_seconds summary', | ||
| ]; | ||
|
|
||
| if (journey.aggregated?.navigation_duration?.value_ms !== undefined) { | ||
| const navigationSeconds = journey.aggregated.navigation_duration.value_ms / 1000; | ||
| const quantileLabel = journey.aggregated.navigation_duration.quantile; | ||
| lines.push(`ci_navigation_duration_seconds{${labelsBase},quantile="${quantileLabel}"} ${navigationSeconds}`); | ||
| } | ||
|
|
||
| if (journey.aggregated?.largest_contentful_paint?.value_ms !== undefined) { | ||
| const lcpSeconds = journey.aggregated.largest_contentful_paint.value_ms / 1000; | ||
| const quantileLabel = journey.aggregated.largest_contentful_paint.quantile; | ||
| lines.push(`ci_lcp_seconds{${labelsBase},quantile="${quantileLabel}"} ${lcpSeconds}`); | ||
| } | ||
|
|
||
| const metaLabels = [`journey="${journey.id}"`]; | ||
| if (GIT_SHA) { | ||
| metaLabels.push(`git_sha="${GIT_SHA}"`); | ||
| } | ||
| if (GIT_REF) { | ||
| metaLabels.push(`git_ref="${GIT_REF}"`); | ||
| } | ||
|
|
||
| lines.push(`# TYPE ci_perf_build_info gauge`); | ||
| lines.push(`ci_perf_build_info{${metaLabels.join(',')}} 1`); | ||
|
|
||
| return lines.join('\n'); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding trailing newline for Prometheus exposition format.
While the current implementation is functional, the Prometheus exposition format typically expects a trailing newline. This ensures compatibility with all parser implementations.
Apply this diff:
lines.push(`# TYPE ci_perf_build_info gauge`);
lines.push(`ci_perf_build_info{${metaLabels.join(',')}} 1`);
- return lines.join('\n');
+ return lines.join('\n') + '\n';
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const formatPrometheusBody = (journey) => { | |
| const labelsBase = `journey="${journey.id}"`; | |
| const lines = [ | |
| '# TYPE ci_navigation_duration_seconds summary', | |
| ]; | |
| if (journey.aggregated?.navigation_duration?.value_ms !== undefined) { | |
| const navigationSeconds = journey.aggregated.navigation_duration.value_ms / 1000; | |
| const quantileLabel = journey.aggregated.navigation_duration.quantile; | |
| lines.push(`ci_navigation_duration_seconds{${labelsBase},quantile="${quantileLabel}"} ${navigationSeconds}`); | |
| } | |
| if (journey.aggregated?.largest_contentful_paint?.value_ms !== undefined) { | |
| const lcpSeconds = journey.aggregated.largest_contentful_paint.value_ms / 1000; | |
| const quantileLabel = journey.aggregated.largest_contentful_paint.quantile; | |
| lines.push(`ci_lcp_seconds{${labelsBase},quantile="${quantileLabel}"} ${lcpSeconds}`); | |
| } | |
| const metaLabels = [`journey="${journey.id}"`]; | |
| if (GIT_SHA) { | |
| metaLabels.push(`git_sha="${GIT_SHA}"`); | |
| } | |
| if (GIT_REF) { | |
| metaLabels.push(`git_ref="${GIT_REF}"`); | |
| } | |
| lines.push(`# TYPE ci_perf_build_info gauge`); | |
| lines.push(`ci_perf_build_info{${metaLabels.join(',')}} 1`); | |
| return lines.join('\n'); | |
| }; | |
| const formatPrometheusBody = (journey) => { | |
| const labelsBase = `journey="${journey.id}"`; | |
| const lines = [ | |
| '# TYPE ci_navigation_duration_seconds summary', | |
| ]; | |
| if (journey.aggregated?.navigation_duration?.value_ms !== undefined) { | |
| const navigationSeconds = journey.aggregated.navigation_duration.value_ms / 1000; | |
| const quantileLabel = journey.aggregated.navigation_duration.quantile; | |
| lines.push(`ci_navigation_duration_seconds{${labelsBase},quantile="${quantileLabel}"} ${navigationSeconds}`); | |
| } | |
| if (journey.aggregated?.largest_contentful_paint?.value_ms !== undefined) { | |
| const lcpSeconds = journey.aggregated.largest_contentful_paint.value_ms / 1000; | |
| const quantileLabel = journey.aggregated.largest_contentful_paint.quantile; | |
| lines.push(`ci_lcp_seconds{${labelsBase},quantile="${quantileLabel}"} ${lcpSeconds}`); | |
| } | |
| const metaLabels = [`journey="${journey.id}"`]; | |
| if (GIT_SHA) { | |
| metaLabels.push(`git_sha="${GIT_SHA}"`); | |
| } | |
| if (GIT_REF) { | |
| metaLabels.push(`git_ref="${GIT_REF}"`); | |
| } | |
| lines.push(`# TYPE ci_perf_build_info gauge`); | |
| lines.push(`ci_perf_build_info{${metaLabels.join(',')}} 1`); | |
| return lines.join('\n') + '\n'; | |
| }; |
🤖 Prompt for AI Agents
In scripts/publish-perf-metrics.mjs around lines 33 to 63, the Prometheus
exposition payload is built with lines.join('\n') but lacks a trailing newline;
update the return to append a final '\n' (e.g., return lines.join('\n') + '\n')
so the output conforms to Prometheus exposition format and is compatible with
all parsers.
| const toJUnit = (journeyMetrics) => { | ||
| const testcases = journeyMetrics.map((journey) => { | ||
| const name = journey.description ? `${journey.id} — ${journey.description}` : journey.id; | ||
| const lines = []; | ||
| if (journey.aggregated?.navigation_duration) { | ||
| const metric = journey.aggregated.navigation_duration; | ||
| lines.push(`navigation_${metric.quantile}=${metric.value_ms.toFixed(2)}ms`); | ||
| if (metric.threshold_ms !== undefined) { | ||
| lines.push(`navigation_threshold=${metric.threshold_ms}`); | ||
| } | ||
| lines.push(`navigation_passed=${metric.passed}`); | ||
| } | ||
| if (journey.aggregated?.largest_contentful_paint) { | ||
| const metric = journey.aggregated.largest_contentful_paint; | ||
| lines.push(`lcp_${metric.quantile}=${metric.value_ms.toFixed(2)}ms`); | ||
| if (metric.threshold_ms !== undefined) { | ||
| lines.push(`lcp_threshold=${metric.threshold_ms}`); | ||
| } | ||
| lines.push(`lcp_passed=${metric.passed}`); | ||
| } | ||
| const systemOut = lines.join('\n'); | ||
| const failure = Object.values(journey.aggregated ?? {}).some((metric) => metric && metric.threshold_ms !== undefined && !metric.passed); | ||
| return { | ||
| name, | ||
| systemOut, | ||
| failure, | ||
| failureMessage: failure ? 'Performance budget regression detected' : undefined, | ||
| }; | ||
| }); | ||
|
|
||
| const failures = testcases.filter((tc) => tc.failure).length; | ||
| const xmlParts = [ | ||
| '<?xml version="1.0" encoding="UTF-8"?>', | ||
| `<testsuites tests="${testcases.length}" failures="${failures}">`, | ||
| `<testsuite name="performance-budget" tests="${testcases.length}" failures="${failures}">`, | ||
| ]; | ||
|
|
||
| for (const testcase of testcases) { | ||
| xmlParts.push(`<testcase classname="performance-budget" name="${testcase.name}">`); | ||
| if (testcase.failure && testcase.failureMessage) { | ||
| xmlParts.push(`<failure message="${testcase.failureMessage}"/>`); | ||
| } | ||
| if (testcase.systemOut) { | ||
| xmlParts.push(`<system-out><![CDATA[${testcase.systemOut}]]></system-out>`); | ||
| } | ||
| xmlParts.push('</testcase>'); | ||
| } | ||
|
|
||
| xmlParts.push('</testsuite>'); | ||
| xmlParts.push('</testsuites>'); | ||
| return xmlParts.join(''); | ||
| }; |
There was a problem hiding this comment.
Add XML escaping for dynamic content in JUnit output.
The JUnit XML generation doesn't escape special XML characters in testcase.name (line 120) and testcase.failureMessage (line 122). If journey IDs or descriptions contain characters like <, >, &, or ", the generated XML will be malformed.
Add an XML escape helper:
+const escapeXml = (str) =>
+ str.replace(/[<>&"]/g, (char) => {
+ const entities = { '<': '<', '>': '>', '&': '&', '"': '"' };
+ return entities[char];
+ });
+
const toJUnit = (journeyMetrics) => {
const testcases = journeyMetrics.map((journey) => {
- const name = journey.description ? `${journey.id} — ${journey.description}` : journey.id;
+ const name = escapeXml(journey.description ? `${journey.id} — ${journey.description}` : journey.id);Then use it when outputting the attributes:
for (const testcase of testcases) {
- xmlParts.push(`<testcase classname="performance-budget" name="${testcase.name}">`);
+ xmlParts.push(`<testcase classname="performance-budget" name="${testcase.name}">`);
if (testcase.failure && testcase.failureMessage) {
- xmlParts.push(`<failure message="${testcase.failureMessage}"/>`);
+ xmlParts.push(`<failure message="${escapeXml(testcase.failureMessage)}"/>`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const toJUnit = (journeyMetrics) => { | |
| const testcases = journeyMetrics.map((journey) => { | |
| const name = journey.description ? `${journey.id} — ${journey.description}` : journey.id; | |
| const lines = []; | |
| if (journey.aggregated?.navigation_duration) { | |
| const metric = journey.aggregated.navigation_duration; | |
| lines.push(`navigation_${metric.quantile}=${metric.value_ms.toFixed(2)}ms`); | |
| if (metric.threshold_ms !== undefined) { | |
| lines.push(`navigation_threshold=${metric.threshold_ms}`); | |
| } | |
| lines.push(`navigation_passed=${metric.passed}`); | |
| } | |
| if (journey.aggregated?.largest_contentful_paint) { | |
| const metric = journey.aggregated.largest_contentful_paint; | |
| lines.push(`lcp_${metric.quantile}=${metric.value_ms.toFixed(2)}ms`); | |
| if (metric.threshold_ms !== undefined) { | |
| lines.push(`lcp_threshold=${metric.threshold_ms}`); | |
| } | |
| lines.push(`lcp_passed=${metric.passed}`); | |
| } | |
| const systemOut = lines.join('\n'); | |
| const failure = Object.values(journey.aggregated ?? {}).some((metric) => metric && metric.threshold_ms !== undefined && !metric.passed); | |
| return { | |
| name, | |
| systemOut, | |
| failure, | |
| failureMessage: failure ? 'Performance budget regression detected' : undefined, | |
| }; | |
| }); | |
| const failures = testcases.filter((tc) => tc.failure).length; | |
| const xmlParts = [ | |
| '<?xml version="1.0" encoding="UTF-8"?>', | |
| `<testsuites tests="${testcases.length}" failures="${failures}">`, | |
| `<testsuite name="performance-budget" tests="${testcases.length}" failures="${failures}">`, | |
| ]; | |
| for (const testcase of testcases) { | |
| xmlParts.push(`<testcase classname="performance-budget" name="${testcase.name}">`); | |
| if (testcase.failure && testcase.failureMessage) { | |
| xmlParts.push(`<failure message="${testcase.failureMessage}"/>`); | |
| } | |
| if (testcase.systemOut) { | |
| xmlParts.push(`<system-out><![CDATA[${testcase.systemOut}]]></system-out>`); | |
| } | |
| xmlParts.push('</testcase>'); | |
| } | |
| xmlParts.push('</testsuite>'); | |
| xmlParts.push('</testsuites>'); | |
| return xmlParts.join(''); | |
| }; | |
| // add this helper above `toJUnit` | |
| const escapeXml = (str) => | |
| str.replace(/[<>&"]/g, (char) => { | |
| const entities = { '<': '<', '>': '>', '&': '&', '"': '"' }; | |
| return entities[char]; | |
| }); | |
| const toJUnit = (journeyMetrics) => { | |
| const testcases = journeyMetrics.map((journey) => { | |
| const name = escapeXml( | |
| journey.description | |
| ? `${journey.id} — ${journey.description}` | |
| : journey.id | |
| ); | |
| const lines = []; | |
| if (journey.aggregated?.navigation_duration) { | |
| const metric = journey.aggregated.navigation_duration; | |
| lines.push(`navigation_${metric.quantile}=${metric.value_ms.toFixed(2)}ms`); | |
| if (metric.threshold_ms !== undefined) { | |
| lines.push(`navigation_threshold=${metric.threshold_ms}`); | |
| } | |
| lines.push(`navigation_passed=${metric.passed}`); | |
| } | |
| if (journey.aggregated?.largest_contentful_paint) { | |
| const metric = journey.aggregated.largest_contentful_paint; | |
| lines.push(`lcp_${metric.quantile}=${metric.value_ms.toFixed(2)}ms`); | |
| if (metric.threshold_ms !== undefined) { | |
| lines.push(`lcp_threshold=${metric.threshold_ms}`); | |
| } | |
| lines.push(`lcp_passed=${metric.passed}`); | |
| } | |
| const systemOut = lines.join('\n'); | |
| const failure = Object.values(journey.aggregated ?? {}) | |
| .some((metric) => metric && metric.threshold_ms !== undefined && !metric.passed); | |
| return { | |
| name, | |
| systemOut, | |
| failure, | |
| failureMessage: failure | |
| ? 'Performance budget regression detected' | |
| : undefined, | |
| }; | |
| }); | |
| const failures = testcases.filter((tc) => tc.failure).length; | |
| const xmlParts = [ | |
| '<?xml version="1.0" encoding="UTF-8"?>', | |
| `<testsuites tests="${testcases.length}" failures="${failures}">`, | |
| `<testsuite name="performance-budget" tests="${testcases.length}" failures="${failures}">`, | |
| ]; | |
| for (const testcase of testcases) { | |
| xmlParts.push( | |
| `<testcase classname="performance-budget" name="${testcase.name}">` | |
| ); | |
| if (testcase.failure && testcase.failureMessage) { | |
| xmlParts.push( | |
| `<failure message="${escapeXml(testcase.failureMessage)}"/>` | |
| ); | |
| } | |
| if (testcase.systemOut) { | |
| xmlParts.push( | |
| `<system-out><![CDATA[${testcase.systemOut}]]></system-out>` | |
| ); | |
| } | |
| xmlParts.push('</testcase>'); | |
| } | |
| xmlParts.push('</testsuite>'); | |
| xmlParts.push('</testsuites>'); | |
| return xmlParts.join(''); | |
| }; |
🤖 Prompt for AI Agents
In scripts/publish-perf-metrics.mjs around lines 82 to 133, the JUnit XML writer
inserts testcase.name and failureMessage directly into XML attributes which can
break the output if they contain characters like <, >, &, " or '. Add a small
xmlEscape helper that replaces & with &, < with <, > with >, " with
" and ' with ', and call it when interpolating testcase.name and
testcase.failureMessage into the attribute values (leave the CDATA-wrapped
systemOut unchanged).
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @shayancoin. * #99 (comment) The following files were modified: * `scripts/ci/check_canary_budgets.py`
…158) Docstrings generation was requested by @shayancoin. * #99 (comment) The following files were modified: * `scripts/ci/check_canary_budgets.py` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68f12ebe0cec8330a2ee49fc1503621d
Summary by CodeRabbit
New Features
Documentation
Chores