fix(ci): Robust E2E & Unit Test Pipeline #1
Conversation
|
Warning Rate limit exceeded@flyingrobots has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 18 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughCI split into unit and E2E jobs: unit tests run via Changes
Sequence Diagram(s)sequenceDiagram
actor Developer
participant GH as GitHub Actions Runner
participant Repo as Repository
participant Node as Node 20 Env (E2E)
participant Docker as Docker container (Unit)
participant Playwright as Playwright tests
participant Artifact as Upload Artifact
rect rgb(230,240,255)
Developer->>Repo: push PR
Repo->>GH: trigger CI workflow
end
par Unit Tests (Docker)
GH->>Repo: checkout
GH->>Docker: run ./test/run-docker.sh
Docker-->>GH: unit test results
and E2E Tests (Playwright)
GH->>Repo: checkout
GH->>Node: setup-node (v20)
Node->>Node: npm ci
Node->>Node: install git & configure user
Node->>Playwright: npm run test:e2e (CI=true)
Playwright-->>Node: test report
Node->>Artifact: upload Playwright report (retain 30d)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @.github/workflows/ci.yml:
- Around line 16-20: Remove the stream-of-consciousness comment block in the CI
workflow (the consecutive commented lines starting with "We don't need
setup-node here..." through "the runner has npm."), leaving only concise,
purposeful comments or none at all; edit the .github workflow step that contains
those comment lines (look for the commented sentences about setup-node and npm)
and delete them so the YAML contains no internal deliberation comments.
- Around line 51-53: Remove the redundant PORT environment variable from the CI
workflow and make the server default match Playwright: delete the PORT: 4638
entry in the CI workflow env block, then update the server default in
src/server/index.js to use 4638 (e.g., change the default value used when
reading process.env.PORT or the hardcoded fallback) or add a clear comment
explaining why a different default is intentional so configs aren’t out of sync;
ensure references to PORT in playwright.config.js remain the single source of
truth for tests.
In @vitest.config.js:
- Around line 1-7: The exclude array in the test config contains redundant
pattern 'node_modules/**'; open the defineConfig call and remove
'node_modules/**' from the test.exclude array so it only lists custom exclusions
(e.g., keep 'test/e2e/**' and drop the node_modules entry).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
node_modules/.package-lock.jsonis excluded by!**/node_modules/**
📒 Files selected for processing (2)
.github/workflows/ci.ymlvitest.config.js
🔇 Additional comments (3)
.github/workflows/ci.yml (3)
55-60: Artifact upload configuration is solid.The use of
if: always()ensures the Playwright report is captured regardless of test outcome, which is exactly what you want for debugging failures. The 30-day retention is reasonable for CI artifacts.
28-28: No action required. Playwright v1.57.0 is the current stable release (released December 2025) and the container imagemcr.microsoft.com/playwright:v1.57.0-jammyis the latest available. Zero known vulnerabilities affect this version.
37-44: No changes needed—Git installation is necessary and properly justified.The E2E test setup requires Git because
test/e2e/global-setup.jsexplicitly callsexecFileSync('git', [...])to initialize an isolated test repository before running the Playwright tests. The setup creates a freshtest-repodirectory independent of the main repository checkout, ensuring proper test isolation without polluting the CI workspace. Git configuration is correctly applied globally in the container and used only for this isolated test environment.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @.github/workflows/ci.yml:
- Around line 32-33: Update the "Install Git" step in the CI workflow (the
`Install Git` job step in .github/workflows/ci.yml) to install a minimal Git
package and clean apt caches; replace `apt-get update && apt-get install -y git`
with `apt-get update && apt-get install -y --no-install-recommends git && rm -rf
/var/lib/apt/lists/*` and add a short inline comment above that step explaining
why Git is required in the Playwright container.
- Around line 44-47: In the "Run Playwright Tests" step remove the explicit
environment variable setting for CI (the env: CI: true line) since GitHub
Actions automatically sets CI=true; update the step that runs npm run test:e2e
(named "Run Playwright Tests") by deleting the env block or at least removing
the CI: true entry so the workflow remains cleaner without changing behavior.
In @src/server/index.js:
- Line 12: The repo still contains stale port references to 4637 that must be
changed to 4638: update docker-compose.yml entries that expose port 4637 and any
environment variable lines setting PORT=4637 to PORT=4638 (the expose and
environment/ports sections), and update README.md references (the example URL
http://localhost:4637) to http://localhost:4638 so documentation, Docker
orchestration, and the server constant (const PORT) are all consistent.
In @vitest.config.js:
- Around line 3-7: The config is replacing Vitest's built-in exclude patterns
instead of extending them; update the defineConfig call so test.exclude merges
Vitest's default exclude patterns with your extra pattern rather than
overwriting them (i.e., get the default exclude patterns and set test.exclude =
[...defaultExcludePatterns, 'test/e2e/**'] when building the config),
referencing defineConfig and the test.exclude property to locate the change.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.ymlsrc/server/index.jsvitest.config.js
🔇 Additional comments (4)
.github/workflows/ci.yml (4)
10-17: Unit test job: streamlined and correct.The deliberation comment cruft from the previous review is gone. The job does exactly one thing: run the Docker-based test script. No unnecessary
setup-node, no existential crises about npm availability. Acceptable.
22-23: Container image version is pinned correctly to match@playwright/testversion.
mcr.microsoft.com/playwright:v1.57.0-jammyaligns with@playwright/testversion1.57.0from the manifest. This prevents the delightful scenario where your local Playwright version and CI container browsers drift apart, causing phantom failures that make developers question their sanity.
35-39: Git user configuration: functional but paranoid aboutinit.defaultBranch.
user.emailanduser.nameare required for commits—fine. Butinit.defaultBranch mainis only relevant if tests callgit init. If they do, this is correct. If they don't, it's cargo cult configuration. Either way, it's harmless noise.
49-54: Artifact upload configuration: locked in.
if: always()captures reports on success or failure—essential for debugging flaky tests. 30-day retention is pragmatic. The pathplaywright-report/is confirmed to be Playwright's default HTML reporter output;playwright.config.jsuses the standardreporter: 'html'with no customoutputDiroverride. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
40-40: Update port reference to match server implementation change.Line 40 references
http://localhost:4637, but the PR changes the server default port to4638(persrc/server/index.js). The documentation now contradicts the actual implementation and will cause developers to fail when following setup instructions.🔎 Proposed fix
-The API and Admin UI will be available at `http://localhost:4637`. +The API and Admin UI will be available at `http://localhost:4638`.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/images/hero.pngis excluded by!**/*.png
📒 Files selected for processing (1)
README.md
🔇 Additional comments (1)
README.md (1)
3-3: Hero image addition is well-executed.The
<img>tag is properly formatted with appropriate attributes (width,alt,align) and logical placement. No issues.
Separates Docker-based unit tests and Playwright E2E tests into distinct jobs. Uses official Playwright container for E2E speed and reliability. Fixes CI failures.