ci: parallel jobs + PR-gate + jest-junit + report (mirror of API #672)#1908
Merged
ci: parallel jobs + PR-gate + jest-junit + report (mirror of API #672)#1908
Conversation
Mirror the structure peanut-api-ts shipped in PR #672 (devx/test-overhaul). Same wins, smaller absolute speedup because UI suite is smaller (38 jest files, 17 Playwright specs vs API's 1100+ tests). Changes to .github/workflows/tests.yml: - **Trigger**: `on: push: branches: ['**']` → `on: pull_request` against main/dev/develop. Stops firing the full suite on every feature-branch push that doesn't have a PR open. - **Split** the single serial job into 5 parallel jobs: - `lint` — prettier check + advisory validate-links - `typecheck` — `next typegen` then `tsc --noEmit` - `unit` — `pnpm test:unit:ci` (with --coverage, jest-junit reporter) - `e2e` — Playwright (advisory; same continue-on-error as before) - `report` — depends on unit; downloads coverage + JUnit artifacts, merges via nyc, posts an idempotent PR comment with status emoji, failing-tests-by-file, top-10 slowest cases, and coverage totals. Same `<!-- ui-test-report -->` tag pattern as code-analysis.yml. - **Add `cache: 'pnpm'`** to setup-node — was missing; saved time per job. - **Switch `pnpm install` → `pnpm install --frozen-lockfile`** for determinism in CI. - **Align Node 21.1.0 → 20** with peanut-api-ts. - **`permissions: checks: write`** so dorny/test-reporter can publish per-test inline annotations. package.json: - New `test:unit:ci` script (`jest --coverage` + jest-junit reporter). - `jest-junit@^16.0.0` to devDependencies. - `jest-junit` reporter config block (output to `test-results/junit.xml`). .gitignore: - `test-results/` and `coverage/` (generated on every run). Wins (estimated): - p50 wall-clock: ~167s → ~max(lint ~45s, typecheck ~70s, unit ~50s, e2e advisory) ≈ 70s. ~58% faster. - Stops wasted runs on non-PR pushes. - Failing tests now surface as inline PR-diff annotations + a clear status emoji at the top of the auto-comment.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Caution Review failedPull request was closed or merged during review WalkthroughCI workflow re-scoped to run on pull requests, split into lint/typecheck/unit/e2e/report/ci-success jobs with Node 20 and pnpm caching; unit job emits JUnit and coverage artifacts, e2e runs continue-on-error; report merges coverage and posts an idempotent PR comment. Added CI Jest config and extra ignore patterns. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Code-analysis diffPainscore total: 5402.52 → 5402.52 (0) 🆕 New findings (13)
✅ Resolved (12)
|
Contributor
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yml:
- Around line 172-175: The report job currently lists only needs: [unit] but
posts a misleading "✅ all green" status; update the report job (named report) so
its needs array includes lint and typecheck (needs: [unit, lint, typecheck]) to
ensure it waits for those results before posting, OR change the posted
status/message to clearly state it's reporting unit-only results (e.g., "✅ unit
tests passed") instead of "all green"; adjust the workflow step that emits the
message to reference the new wording if you choose the latter.
- Around line 307-313: The current lookup uses github.rest.issues.listComments
which only returns the first page (30) so the tagged report can be missed;
update the code that builds comments (the call around
github.rest.issues.listComments and the variables tag/existing) to fetch all
pages using Octokit's pagination (e.g. github.paginate with
github.rest.issues.listComments and the same params: owner, repo, issue_number)
and then search the resulting full list for the tag to set existing, ensuring
the follow-up update is idempotent instead of creating duplicates.
- Around line 215-246: The report job currently depends only on unit (needs:
[unit]) so update the report job's needs to include lint and typecheck (needs:
[unit, lint, typecheck]) so the PR comment reflects all checks; also fix the
comment-update logic that uses listComments by adding per_page and implementing
pagination (loop through pages until no more) when fetching existing comments so
the code can locate and update an existing report instead of creating
duplicates—refer to the report job's needs declaration and the listComments
usage in the comment-update routine when making these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a139784-2efd-4294-ac10-82518cadf070
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.github/workflows/tests.yml.gitignorepackage.json
Same pattern as peanut-api-ts #672. Branch protection requires only `ci-success` instead of listing every job — auto-gates new jobs added to `needs:` here, no ruleset edits required. `if: always()` + explicit `failure || cancelled` check handles the edge case where an upstream `skipped` would otherwise let the umbrella silently pass (GitHub treats `skipped` as neutral).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/tests.yml (2)
172-175:⚠️ Potential issue | 🟡 MinorReport job claims "all green" but only checks unit tests.
The
needs: [unit]dependency means the report runs after only unit tests complete. However, line 260 displays "✅ all green" which is misleading since lint and typecheck may still be failing. Either expandneedsto includelintandtypecheck, or clarify the message to reflect unit-only results.Option A: Expand needs array
report: runs-on: ubuntu-latest - needs: [unit] + needs: [unit, lint, typecheck] if: always()Option B: Clarify message as unit-only
- const status = p.failed.length === 0 ? '✅ all green' : `🔴 ${p.failed.length} failing`; + const status = p.failed.length === 0 ? '✅ unit green' : `🔴 ${p.failed.length} unit failing`;Also applies to: 260-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 172 - 175, The report job's dependency only lists "unit" causing the "✅ all green" message (line with that text) to be misleading; either expand the report job's needs array to include lint and typecheck (so the job waits for unit, lint, and typecheck before reporting) or change the report job's success message to explicitly state it reflects only unit test results; locate the `report:` job block and update the `needs:` array to `needs: [unit, lint, typecheck]` or edit the displayed message text ("✅ all green") to "✅ unit tests passed" (or similar) to accurately reflect scope.
307-313:⚠️ Potential issue | 🟡 MinorPaginate PR comments to avoid duplicates on busy PRs.
listCommentsreturns only the first page (30 items by default). On PRs with many comments, the tagged report may not be found, causing duplicates instead of updates.🔁 Proposed fix using Octokit pagination
const tag = '<!-- ui-test-report -->'; - const { data: comments } = await github.rest.issues.listComments({ - issue_number: context.issue.number, - owner: context.repo.owner, - repo: context.repo.repo, - }); + const comments = await github.paginate( + github.rest.issues.listComments, + { + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + per_page: 100, + } + ); const existing = comments.find(c => c.body && c.body.includes(tag));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 307 - 313, The code uses github.rest.issues.listComments which only returns the first page, causing missed tagged reports; replace that single-page call with Octokit pagination (use github.paginate) to retrieve all comments before searching for the tag variable, i.e. call github.paginate(github.rest.issues.listComments, { issue_number: context.issue.number, owner: context.repo.owner, repo: context.repo.repo }) to populate the comments array and then run the existing find (existing = comments.find(...)) so duplicates are avoided.
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
155-156: Consider usingpnpm execinstead ofnpxfor Playwright browser install.After
pnpm install, usingnpx playwright installmay resolve to a globally cached version rather than the project's locked version. Usingpnpm execensures the locally installed Playwright is used.♻️ Suggested change
- name: Install Playwright browsers - run: npx playwright install --with-deps chromium + run: pnpm exec playwright install --with-deps chromium🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 155 - 156, The GitHub Actions step "Install Playwright browsers" currently uses npx which can pick a global binary; update the step to invoke the project's Playwright via pnpm by replacing the run command "npx playwright install --with-deps chromium" with a pnpm-based invocation (e.g., "pnpm exec playwright install --with-deps chromium") so the workflow uses the locked local Playwright package version after pnpm install.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yml:
- Around line 220-222: The decode function's replacement for ' is wrong:
change the `.replaceAll(''','\\'')` occurrence inside function decode to
replace ' with a single-quote character instead of a backslash-escaped
string; update the replacement argument so it is a single-quote (e.g., use a
literal "'" as the replacement) ensuring proper quoting in the workflow/YAML
context so the resulting JavaScript uses .replaceAll(''', "'").
---
Duplicate comments:
In @.github/workflows/tests.yml:
- Around line 172-175: The report job's dependency only lists "unit" causing the
"✅ all green" message (line with that text) to be misleading; either expand the
report job's needs array to include lint and typecheck (so the job waits for
unit, lint, and typecheck before reporting) or change the report job's success
message to explicitly state it reflects only unit test results; locate the
`report:` job block and update the `needs:` array to `needs: [unit, lint,
typecheck]` or edit the displayed message text ("✅ all green") to "✅ unit tests
passed" (or similar) to accurately reflect scope.
- Around line 307-313: The code uses github.rest.issues.listComments which only
returns the first page, causing missed tagged reports; replace that single-page
call with Octokit pagination (use github.paginate) to retrieve all comments
before searching for the tag variable, i.e. call
github.paginate(github.rest.issues.listComments, { issue_number:
context.issue.number, owner: context.repo.owner, repo: context.repo.repo }) to
populate the comments array and then run the existing find (existing =
comments.find(...)) so duplicates are avoided.
---
Nitpick comments:
In @.github/workflows/tests.yml:
- Around line 155-156: The GitHub Actions step "Install Playwright browsers"
currently uses npx which can pick a global binary; update the step to invoke the
project's Playwright via pnpm by replacing the run command "npx playwright
install --with-deps chromium" with a pnpm-based invocation (e.g., "pnpm exec
playwright install --with-deps chromium") so the workflow uses the locked local
Playwright package version after pnpm install.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee9b5f7e-849a-4dc6-97d6-de637e00d672
📒 Files selected for processing (1)
.github/workflows/tests.yml
Comment on lines
+220
to
+222
| function decode(s) { | ||
| return s.replaceAll('&','&').replaceAll('<','<').replaceAll('>','>').replaceAll('"','\"').replaceAll(''','\\''); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the current escaping behavior vs the fix
echo "=== Current escaping (produces backslash) ==="
node -e "console.log('test'.replaceAll('t','\\\\'))"
echo ""
echo "=== Correct escaping for single quote ==="
node -e "console.log('test'.replaceAll('t',\"'\"))"Repository: peanutprotocol/peanut-ui
Length of output: 167
🏁 Script executed:
cd .github/workflows && head -n 225 tests.yml | tail -n 15Repository: peanutprotocol/peanut-ui
Length of output: 877
Escaping bug in decode function: ' replacement produces backslash instead of single quote.
The current code .replaceAll(''','\\'') replaces XML ' entities with a backslash character (\), not a single quote ('). In JavaScript, '\\' is a string containing one backslash.
Fix by using "'" inside the double-quoted shell context:
🐛 Proposed fix
function decode(s) {
- return s.replaceAll('&','&').replaceAll('<','<').replaceAll('>','>').replaceAll('"','\"').replaceAll(''','\\'');
+ return s.replaceAll('&','&').replaceAll('<','<').replaceAll('>','>').replaceAll('"','\"').replaceAll(''',"'");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/tests.yml around lines 220 - 222, The decode function's
replacement for ' is wrong: change the `.replaceAll(''','\\'')`
occurrence inside function decode to replace ' with a single-quote
character instead of a backslash-escaped string; update the replacement argument
so it is a single-quote (e.g., use a literal "'" as the replacement) ensuring
proper quoting in the workflow/YAML context so the resulting JavaScript uses
.replaceAll(''', "'").
…back) Same fix as peanut-api-ts/devx/test-overhaul commit 9dc1b2dc: 1. report's needs was [unit] only — header "✅ all green" misled when lint/typecheck/e2e statuses weren't represented. Now needs: [lint, typecheck, unit, e2e]. 2. listComments wasn't paginated — would silently start posting duplicates once PR has >30 comments. Switched to github.paginate with per_page=100.
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
1 participant
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.
Companion to peanut-api-ts #672 (devx/test-overhaul). Same structural CI improvements applied to peanut-ui.
Why
peanut-ui's
Testsworkflow had the same shape the API had before #672:on: push: branches: ['**']— fired on every push to every branch, even ones without an open PRcache: 'pnpm'on setup-node — paid full install cost every runpnpm install(not--frozen-lockfile) — non-deterministic CI installsChanges
.github/workflows/tests.ymllint,typecheck,unit,e2e,report. PR-gate trigger.cache: 'pnpm'.--frozen-lockfile. Node 20. dorny/test-reporter for inline annotations. Newreportjob posts an idempotent PR comment with status, failing tests, coverage, slowest cases.package.jsontest:unit:ciscript (jest --coverage+ jest-junit reporter).jest-junit@^16.0.0devDep.jest-junitreporter config block..gitignoretest-results/+coverage/Wall-clock estimate
What's NOT in this PR
continue-on-error: true). WiringTEST_HARNESS_SECRETto make it blocking is a separate decision.engineering/projects/test-infra-overhaul/TODO.mdin the mono companion PR).Test plan
lint,typecheck,unit,e2e,report) appear and run in parallelreportjob posts a PR comment with non-zero coverage + suite resultsTestsworkflow does NOT fire (was firing on every push before)