fix loading .svg files as React components in tests #2472
Conversation
…istency Run Playwright browser inside a Docker container so visual screenshot tests produce identical results on macOS (local dev) and Linux (CI). - Add docker-compose.visual.yml with Playwright server container - Update vitest config to connect to Docker browser via websocket when PW_TEST_CONNECT_WS_ENDPOINT env var is set - Add test:visual and test:visual:update npm scripts - Update CI workflow to use Docker Playwright server instead of bare Playwright install - Regenerate screenshot baselines from Linux container Closes PRD-6191 Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Add explicit failure handling if Playwright server doesn't start - Bind Docker port to 127.0.0.1 only (don't expose to network) - Align npx playwright version with Docker image (both 1.58.0) Co-Authored-By: Claude Opus 4.6 <[email protected]>
The @vitest/browser-playwright package requires a local Playwright install to initialize, even when the actual browser runs in Docker via connectOptions. Keep the install step alongside the Docker server. Co-Authored-By: Claude Opus 4.6 <[email protected]>
… mode Turbo v2 strict mode filters env vars not listed in turbo.json from child processes. The Playwright WebSocket endpoint was being silently dropped, causing vitest to fall back to local Chromium instead of the Docker server — producing mismatched screenshots in CI. Also pins docker-compose.visual.yml to linux/amd64 so local baselines match CI regardless of host architecture (see microsoft/playwright#13873), and fixes the Playwright cache restore-keys prefix mismatch. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…or state visual test
…and stable render wait
…ests The previous refactor removed the onUnhandledError handler, causing CI to fail with exit code 1 due to 3 known, unfixable Monaco Editor errors in Vitest browser mode: 1. "Cannot use import statement outside a module" - Monaco web workers cannot load ESM in the Vitest browser sandbox 2. "InvalidCharacterError" / "is not a valid name" - Monaco attempts createElement with an SVG data URI as the tag name 3. "Closing rpc while" - Vitest worker RPC shutdown race condition These errors were originally suppressed by Nick in #2046 and #2078 after investigation confirmed they are unfixable Monaco/Vitest internals that do not affect test correctness. Refs: #2046, #2078
The pixelmatch package is no longer imported after the vitest.config.ts refactor removed the custom tolerantPixelmatch comparator. Knip correctly flags it as unused.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 465cbb8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Ito Test Report ✅17 test cases ran. 17 passed. This verification run tested the agents-manage-ui visual test infrastructure changes from PR #2472, focusing on Dockerized visual regression tests with the vite-plugin-svgr migration. All verified functionality passed successfully: Docker image builds with the updated Dockerfile and agents-core dependency, SVG icons render correctly using the new ✅ Passed (17)
📋 View Recording |
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Medium
Delta Review Scope
This is a re-review scoped to changes since the previous automated review at commit 834084b3a.
The delta consists of 1 commit (76a5ec2f2 — "polish"):
| File | Change | Assessment |
|---|---|---|
AGENTS.md |
Changed code fence from ``` to ```dotenv |
✅ Correct — syntax highlighting improvement |
docker-compose.visual.yml |
Typo fix: etc). → etc.). |
✅ Correct — punctuation fix |
docker-compose.visual.yml |
Changed platform: linux/amd64 → platform: linux |
❌ Regression — breaks cross-architecture screenshot consistency |
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
docker-compose.visual.yml:11Platform change breaks cross-architecture screenshot consistency
Analysis
The change from platform: linux/amd64 to platform: linux is a regression that undermines the core purpose of this visual testing infrastructure. The prior review feedback specifically requested restoring the documentation comment explaining why linux/amd64 is required — this delta instead removes the architecture pin entirely.
Impact:
- On Apple Silicon Macs,
platform: linuxwill uselinux/arm64 - ARM Chromium produces different subpixel rendering than x86 Chromium
- Local screenshots will no longer match CI screenshots
- The AGENTS.md documentation explicitly states Docker is required to ensure screenshots match CI
This contradicts the Ito test report's ADV-2 finding: "Screenshot mismatches are present even on x86_64 due to font rendering environment differences" — if there are already differences on the same architecture due to fonts, removing the architecture pin will make this significantly worse.
🚫 REQUEST CHANGES
Summary: The platform: linux/amd64 → platform: linux change in docker-compose.visual.yml is a regression that breaks the cross-platform screenshot consistency this PR was designed to achieve. Please restore platform: linux/amd64 along with the explanatory comment (see 1-click suggestion in the inline comment). The AGENTS.md change (adding dotenv language identifier) is fine.
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
AGENTS.md:265 |
Code fence language change | Valid improvement — dotenv syntax highlighting is more accurate than generic |
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 1 | 0 | 1 |
Note: Delta review — no sub-reviewers dispatched as the delta was limited to 2 files with a single substantive change.
| # ARM and x86 Chromium produce different subpixel rendering — see | ||
| # https://github.com/microsoft/playwright/issues/13873 | ||
| platform: linux/amd64 | ||
| platform: linux |
There was a problem hiding this comment.
🟠 MAJOR: Platform change breaks cross-architecture screenshot consistency
Issue: Changing from platform: linux/amd64 to platform: linux removes the architecture pin that ensures screenshot consistency between local development and CI.
Why: ARM and x86 Chromium produce different subpixel rendering. On Apple Silicon Macs, platform: linux will pull the ARM image, causing local screenshots to differ from CI screenshots (which run on x86 Linux). This defeats the entire purpose of running the browser in a Docker container — the AGENTS.md documentation even states: "Docker is required — without it, tests fall back to local Chromium and screenshots won't match CI."
Fix: (1-click apply)
| platform: linux | |
| # Pin to amd64 so screenshots match CI (ubuntu-x86) regardless of host arch. | |
| # ARM and x86 Chromium produce different subpixel rendering — see | |
| # https://github.com/microsoft/playwright/issues/13873 | |
| platform: linux/amd64 |
Refs:
- microsoft/playwright#13873 — ARM vs x86 rendering differences
- AGENTS.md:139 — Documentation stating Docker is required for cross-platform consistency
- Prior review feedback — Original suggestion to restore this documentation
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Medium
Delta Review Scope
This is a re-review scoped to changes since the previous automated review at commit 76a5ec2f2.
The delta consists of 1 commit (645abc052 — "fix"):
| File | Change | Assessment |
|---|---|---|
__mocks__/monaco-editor.ts:25 |
Reverted from 'monaco-editor/esm/vs/editor/editor.main.js' back to 'monaco-editor' |
✅ Correct — reverting the specific ESM path that may have caused compatibility issues; simpler import works better with the test bundler |
package.json:46 |
Added --project '@inkeep/agents-manage-ui' filter and chained && pnpm test:visual |
✅ Correct — CI already starts the Playwright Docker server and sets PW_TEST_CONNECT_WS_ENDPOINT, so visual tests will run correctly |
Assessment: Both delta changes are valid fixes. No new issues introduced.
🕐 Pending Recommendations (1)
Prior review feedback that remains unaddressed (unchanged in this delta, but still blocking):
- 🟠
docker-compose.visual.yml:11Platform change breaks cross-architecture screenshot consistency — Theplatform: linux/amd64was changed toplatform: linuxin commit76a5ec2f2. On Apple Silicon Macs, this causes the Playwright container to use ARM instead of x86, producing different subpixel rendering. Local screenshots will no longer match CI screenshots. This was flagged as a MAJOR issue in the previous review and remains unresolved.
🚫 REQUEST CHANGES
Summary: The delta changes in this commit are fine — reverting the Monaco mock to a simpler import and chaining visual tests into the main test script are both valid fixes. However, the blocking issue from the previous review remains unresolved: the platform: linux/amd64 → platform: linux change in docker-compose.visual.yml will cause screenshot mismatches between local development (ARM on Apple Silicon) and CI (x86 Linux). Please restore platform: linux/amd64 with the explanatory comment as suggested in the inline comment.
Discarded (0)
No discarded findings.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Delta review — no sub-reviewers dispatched as the delta was limited to 2 files with minor fixes.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Medium
Delta Review Scope
This is a re-review scoped to changes since the previous automated review at commit 645abc052f39.
The delta consists of 3 commits:
| Commit | Change | Assessment |
|---|---|---|
e841d144a |
Merge branch 'main' into feat/dockerize-visual-tests-2 | ✅ Merge commit — brings in main branch updates |
c900173e8 |
run pnpm dedupe (#2953) |
✅ Lockfile optimization from main |
1b339c076 |
pnpm i | ✅ Lockfile update |
Delta Changes Analyzed
| File | Change | Assessment |
|---|---|---|
agents-docs/package.json |
React version: canary → ^19.2.4 | ✅ Stabilization from canary to release |
agents-manage-ui/package.json |
React version: canary → ^19.2.4 | ✅ Stabilization from canary to release |
packages/agents-email/package.json |
React peerDep/devDep version normalization | ✅ Cleanup |
pnpm-lock.yaml |
Deduplication and version updates | ✅ Expected lockfile changes |
Assessment: All delta changes are benign — merge commits, lockfile deduplication, and React version stabilization from canary to stable release. No new code changes requiring review.
🕐 Pending Recommendations (1)
Prior review feedback that remains unaddressed and is still blocking:
- 🟠
docker-compose.visual.yml:11Platform change breaks cross-architecture screenshot consistency — Theplatform: linux/amd64was changed toplatform: linuxin commit76a5ec2f2. On Apple Silicon Macs, this causes the Playwright container to use ARM instead of x86, producing different subpixel rendering. Local screenshots will no longer match CI screenshots (which run on x86 Linux). This was flagged as a MAJOR issue in the previous review and remains unresolved.
The fix is straightforward — restore the architecture pin with the explanatory comment:
playwright:
# Pin to amd64 so screenshots match CI (ubuntu-x86) regardless of host arch.
# ARM and x86 Chromium produce different subpixel rendering — see
# https://github.com/microsoft/playwright/issues/13873
platform: linux/amd64🚫 REQUEST CHANGES
Summary: The delta since last review contains only merge commits from main and lockfile updates — no issues in the new code. However, the blocking issue from the previous review remains unresolved: the platform: linux/amd64 → platform: linux change in docker-compose.visual.yml will cause screenshot mismatches between local development (ARM on Apple Silicon) and CI (x86 Linux). Please restore platform: linux/amd64 with the explanatory comment as suggested in the inline comment. This is a 1-line fix that preserves the core purpose of this visual testing infrastructure.
Discarded (0)
No discarded findings.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Delta review — no sub-reviewers dispatched as the delta contained only merge commits and lockfile changes with no new code to review.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Medium
Delta Review Scope
This is a re-review scoped to changes since the previous automated review at commit 1b339c076.
The delta consists of 1 commit (465cbb8b9 — "upd"):
| File | Change | Assessment |
|---|---|---|
diff-viewer.test.tsx |
Added cleanup import and afterEach(cleanup) |
✅ Correct — explicit cleanup prevents test pollution |
nodes.browser.test.tsx |
Added cleanup import, afterEach(cleanup), reduced timeout 30s→10s |
✅ Correct — cleanup and faster timeouts after worker mocking stabilized |
form.browser.test.tsx |
Added cleanup import/call, reduced timeouts to 10s |
✅ Correct — consistent cleanup pattern |
generic-prompt-editor.test.tsx |
Added cleanup import and afterEach(cleanup) |
✅ Correct — consistent cleanup pattern |
| Screenshot baselines (2 files) | Deleted | ✅ Expected — baselines regenerated with Docker/svgr changes |
Assessment: All delta changes are valid test hygiene improvements — adding explicit cleanup() calls ensures component unmounting between tests, and reduced timeouts reflect improved test stability now that Monaco worker mocking is properly configured. The deleted screenshot baselines will be regenerated on next visual test run.
🕐 Pending Recommendations (1)
Prior review feedback that remains unaddressed and is still blocking:
- 🟠
docker-compose.visual.yml:11Platform change breaks cross-architecture screenshot consistency — Theplatform: linux/amd64was changed toplatform: linuxin commit76a5ec2f2. On Apple Silicon Macs, this causes the Playwright container to use ARM instead of x86, producing different subpixel rendering. Local screenshots will no longer match CI screenshots (which run on x86 Linux). This was flagged as a MAJOR issue in the previous review and remains unresolved.
The fix is straightforward — restore the architecture pin with the explanatory comment:
playwright:
# Pin to amd64 so screenshots match CI (ubuntu-x86) regardless of host arch.
# ARM and x86 Chromium produce different subpixel rendering — see
# https://github.com/microsoft/playwright/issues/13873
platform: linux/amd64🚫 REQUEST CHANGES
Summary: The delta since last review contains valid test hygiene improvements — adding explicit cleanup() calls, reducing timeouts, and removing old screenshot baselines. However, the blocking issue from prior reviews remains unresolved: the platform: linux/amd64 → platform: linux change in docker-compose.visual.yml will cause screenshot mismatches between local development (ARM on Apple Silicon) and CI (x86 Linux). Please restore platform: linux/amd64 with the explanatory comment as suggested in the inline comment. This is a 1-line fix that preserves the core purpose of this visual testing infrastructure.
Discarded (0)
No discarded findings.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Delta review — no sub-reviewers dispatched as the delta contained only test cleanup changes (adding explicit cleanup() calls, reducing timeouts, deleting old baselines) with no new logic to review.
No description provided.