CI: Use custom container for E2E tests #7625
Conversation
dosubot
bot
added
the
size:S
📝 WalkthroughWalkthroughTwo GitHub Actions workflows were modified to replace cache-based artifact management with explicit artifact uploads and downloads. Containerized test execution environments were introduced for both E2E testing and Playwright expectation update jobs, with frontend dependencies installed and browsers pre-configured within containers. Changes
Possibly related PRs
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (10)📓 Common learnings📚 Learning: 2025-12-12T23:02:37.473ZApplied to files:
📚 Learning: 2025-11-24T19:47:22.909ZApplied to files:
📚 Learning: 2025-11-24T19:47:22.909ZApplied to files:
📚 Learning: 2025-12-17T01:01:03.609ZApplied to files:
📚 Learning: 2025-12-17T01:01:03.609ZApplied to files:
📚 Learning: 2025-12-17T01:01:03.609ZApplied to files:
📚 Learning: 2025-11-24T19:47:56.371ZApplied to files:
📚 Learning: 2025-12-17T01:01:03.609ZApplied to files:
📚 Learning: 2025-12-16T17:41:58.464ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (6)
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 |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 12/20/2025, 06:13:46 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results❌ Some tests failed ⏰ Completed at: 12/20/2025, 06:27:26 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.21 MB (baseline 3.21 MB) • ⚪ 0 BMain entry bundles and manifests
Graph Workspace — 993 kB (baseline 993 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Views & Navigation — 6.54 kB (baseline 6.54 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Panels & Settings — 297 kB (baseline 297 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
UI Components — 185 kB (baseline 185 kB) • ⚪ 0 BReusable component library chunks
Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Utilities & Hooks — 1.41 kB (baseline 1.41 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 8.46 MB (baseline 8.46 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 3.44 MB (baseline 3.44 MB) • ⚪ 0 BBundles that do not match a named category
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci-tests-e2e.yaml (1)
47-66: Update container image to use Playwright v1.57.0 to match project dependency.The project uses Playwright v1.57.0, but the container image
mcr.microsoft.com/playwright:v1.52.0-nobleruns v1.52.0. If the Playwright version in the Docker image does not match the version in the project/tests, Playwright will be unable to locate browser executables. Change the container image tag fromv1.52.0-nobletov1.57.0-noble.Additionally, cache files are created by the runner (UID 1001 on ubuntu-latest) and mounted into the container, but the Playwright container's user 1001 may not have the necessary permissions if that user doesn't exist in the container. Consider using
--user rootor ensure the container image has a user with UID 1001 configured.
♻️ Duplicate comments (1)
.github/workflows/ci-tests-e2e.yaml (1)
96-98: Same permission and version concerns apply here.This job has the same container configuration and cache restoration pattern as
playwright-tests-chromium-sharded, so the permission mismatch and version compatibility concerns from lines 47-66 apply here as well.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-tests-e2e.yaml(7 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : Use Playwright tags like `mobile`, `2x` for relevant E2E tests - they are respected by config
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : E2E test files must use pattern `browser_tests/**/*.spec.ts` with Playwright
📚 Learning: 2025-12-12T23:02:37.473Z
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : Use Playwright tags like `mobile`, `2x` for relevant E2E tests - they are respected by config
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : E2E test files must use pattern `browser_tests/**/*.spec.ts` with Playwright
Applied to files:
.github/workflows/ci-tests-e2e.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup
- GitHub Check: lint-and-format
- GitHub Check: test
- GitHub Check: collect
🔇 Additional comments (1)
.github/workflows/ci-tests-e2e.yaml (1)
9-9: LGTM: Reasonable optimization to skip E2E tests for version bump PRs.This addition prevents running expensive E2E tests on automated version bump PRs, which is a sensible optimization.
Myestery
force-pushed
the
ci/playwright-docker-containers
branch
from
952e5cd to
ef5a55d
Compare
dosubot
bot
added
size:M
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/ci-tests-e2e.yaml (1)
147-149: Note: merge-reports container usage already flagged.This has already been reviewed in a previous comment. The full Playwright container (with browsers) is not necessary for the
merge-reportsjob, but using it ensures CLI version consistency. A lighter Node.js container would be more efficient if startup time is a concern.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-tests-e2e.yaml(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
📚 Learning: 2025-12-12T23:02:37.473Z
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : E2E test files must use pattern `browser_tests/**/*.spec.ts` with Playwright
Applied to files:
.github/workflows/ci-tests-e2e.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint-and-format
- GitHub Check: setup
- GitHub Check: test
- GitHub Check: collect
🔇 Additional comments (4)
.github/workflows/ci-tests-e2e.yaml (4)
9-9: LGTM: Skip E2E tests on version bump PRs.Adding
version-bump*to the ignore list is appropriate for automated version bumps that don't require E2E validation.
55-61: LGTM: Correct pattern for container-based jobs with composite actions.The checkout-then-download pattern is necessary because:
- Checkout provides access to
.github/actions(local composite actions)- Workspace download provides built artifacts from the setup job
This is the correct approach for container-based workflows using artifacts.
63-70: LGTM: Efficient test environment setup.The setup correctly:
- Launches the ComfyUI server for testing
- Reuses the built frontend from the workspace artifact
- Avoids redundant installation/build steps
- Relies on container-provided browsers
72-84: LGTM: Proper sharded test execution and report handling.The implementation correctly:
- Uses blob reporter for efficient report merging across shards
- Uploads reports even on test failure (
!cancelled()condition)- Uses appropriate 1-day retention for intermediate blob reports
- Properly names artifacts with shard indices
There was a problem hiding this comment.
For future PRs:
Unused files needing removal:
- .github/actions/setup-playwright/action.yml
Docs needing updates after this PR:
browser_tests/README.md - Remove pnpm exec playwright install chromium --with-depskeep this as it's docs for running locally- CONTRIBUTING.md - Update Playwright test section to mention Docker approach
|
We still have more issues to fix |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
.github/workflows/ci-tests-e2e.yaml (4)
29-38: Consider optimizing workspace artifact size to reduce CI overhead.Using artifacts instead of cache works correctly with containers, but uploading the entire workspace (except
.git) may create large artifacts if it includesnode_modulesand build outputs. This can slow upload/download times and consume storage quota unnecessarily.If the artifact size exceeds 500MB, consider either:
- Excluding
node_modulesand runningpnpm installin downstream jobs (with cache if possible)- Using a targeted artifact containing only
dist/and files essential for test jobsNote that file permissions are not preserved during artifact uploads.
44-46: Update Playwright container image to match project version.The container image (v1.52.0) lags behind the project's Playwright dependency (v1.57.0). Update the container image from
mcr.microsoft.com/playwright:v1.52.0-nobletomcr.microsoft.com/playwright:v1.57.0-nobleto ensure consistency between CI and development environments.🔎 Proposed fix
container: - image: mcr.microsoft.com/playwright:v1.52.0-noble + image: mcr.microsoft.com/playwright:v1.57.0-noble options: --user 1001
92-94: Update Playwright container image to match project version.Same version mismatch as in the chromium-sharded job. Update to v1.57.0.
🔎 Proposed fix
container: - image: mcr.microsoft.com/playwright:v1.52.0-noble + image: mcr.microsoft.com/playwright:v1.57.0-noble options: --user 1001
149-151: Consider lighter container for merge-reports job.The
merge-reportsjob only runsplaywright merge-reportsCLI commands and doesn't execute tests or require browsers. Using the full Playwright container (which includes browser binaries, ~1.3GB) may be excessive.However, using the same container ensures:
- Consistent Playwright CLI version across all jobs
- Simplified maintenance (single container version to update)
If you prioritize faster job startup and lower resource usage, consider using a lighter Node.js container with just
@playwright/testinstalled. Otherwise, the current approach is acceptable for consistency.💡 Alternative: Use lightweight Node container
merge-reports: needs: [playwright-tests-chromium-sharded] runs-on: ubuntu-latest - container: - image: mcr.microsoft.com/playwright:v1.52.0-noble - options: --user 1001 if: ${{ !cancelled() }} steps: - name: Checkout repository uses: actions/checkout@v5 - # Setup pnpm/node to run playwright merge-reports (browsers pre-installed in container) + # Setup pnpm/node to run playwright merge-reports - name: Setup frontend uses: ./.github/actions/setup-frontendThis would use the default ubuntu-latest runner, which should work since no browsers are needed.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/actions/setup-comfyui-server/action.yml(3 hunks).github/workflows/ci-tests-e2e.yaml(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
📚 Learning: 2025-12-12T23:02:37.473Z
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : E2E test files must use pattern `browser_tests/**/*.spec.ts` with Playwright
Applied to files:
.github/workflows/ci-tests-e2e.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint-and-format
- GitHub Check: test
- GitHub Check: setup
- GitHub Check: collect
🔇 Additional comments (8)
.github/actions/setup-comfyui-server/action.yml (2)
12-15: LGTM! Container mode input is well-defined.The new
container_modeinput properly distinguishes between host and container execution environments with appropriate defaults.
39-40: LGTM! Conditional Python setup is correct.The conditional logic correctly skips Python setup when running inside a container, as the container provides its own Python environment.
.github/workflows/ci-tests-e2e.yaml (6)
9-9: LGTM! Version bump branch exclusion is appropriate.Adding
version-bump*to branches-ignore prevents unnecessary E2E test runs on automated version bump PRs.
56-61: LGTM! Checkout and workspace download pattern is correct.The sequence of checking out the repository first (to get
.github/actions) then downloading the workspace artifact is the proper approach for container-based jobs.
68-68: LGTM! Container mode flag correctly passed.Setting
container_mode: trueensures the setup action skips redundant Python installation and uses the container's system Python.
102-108: LGTM! Checkout and workspace download pattern is correct.The pattern is consistent with the chromium-sharded job and properly structured.
115-115: LGTM! Container mode flag correctly passed.Consistent with the chromium-sharded job.
44-46: No action needed—--user 1001is the officially recommended pattern for this Playwright container image in GitHub Actions.Playwright's official CI documentation for GitHub Actions uses
--user 1001withmcr.microsoft.com/playwright:v1.57.0-noble. This configuration is verified as standard practice with no documented permission issues.
dosubot
bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/actions/setup-comfyui-server/action.yml (1)
55-58: Inconsistent pip invocation style (duplicate concern).This code mixes
python3 -m pip(line 55) withpip3(lines 56-58). As noted in a previous review, usingpython3 -m pipconsistently is more explicit and ensures you're using the pip module associated with the specific Python interpreter—especially important in containerized environments.🔎 Proposed fix for consistency
python3 -m pip install --upgrade pip --break-system-packages || python3 -m pip install --upgrade pip -pip3 install torch torchvision torchaudio --index-url https://download.pytorch.org/whl/cpu --break-system-packages || pip3 install torch torchvision torchaudio --index-url https://download.pytorch.org/whl/cpu -pip3 install -r requirements.txt --break-system-packages || pip3 install -r requirements.txt -pip3 install wait-for-it --break-system-packages || pip3 install wait-for-it +python3 -m pip install torch torchvision torchaudio --index-url https://download.pytorch.org/whl/cpu --break-system-packages || python3 -m pip install torch torchvision torchaudio --index-url https://download.pytorch.org/whl/cpu +python3 -m pip install -r requirements.txt --break-system-packages || python3 -m pip install -r requirements.txt +python3 -m pip install wait-for-it --break-system-packages || python3 -m pip install wait-for-it
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/actions/setup-comfyui-server/action.yml(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
📚 Learning: 2025-12-12T23:02:37.473Z
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Applied to files:
.github/actions/setup-comfyui-server/action.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup
- GitHub Check: lint-and-format
- GitHub Check: test
- GitHub Check: collect
🔇 Additional comments (3)
.github/actions/setup-comfyui-server/action.yml (3)
12-15: LGTM! Container mode input is well-designed.The
container_modeinput enables dual-mode operation while maintaining backward compatibility with existing workflows.
39-40: LGTM! Conditional Python setup is appropriate.Correctly skips the setup-python action when running in container mode where system Python is available.
65-65: LGTM! Correct use ofpython3.Explicitly using
python3ensures the correct interpreter is invoked in the container environment.
dosubot
bot
added
size:M
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
.github/actions/setup-comfyui-server/action.yml (2)
70-78: Usepython3 -m pipconsistently.Line 75 uses
python3 -m pipwhile lines 76-78 usepip3. For consistency and to ensure the correct interpreter's pip is used, usepython3 -m pipthroughout.🔎 Proposed fix
- name: Install Python requirements (host mode) if: ${{ inputs.container_mode != 'true' }} shell: bash working-directory: ComfyUI run: | python3 -m pip install --upgrade pip - pip3 install torch torchvision torchaudio --index-url https://download.pytorch.org/whl/cpu - pip3 install -r requirements.txt - pip3 install wait-for-it + python3 -m pip install torch torchvision torchaudio --index-url https://download.pytorch.org/whl/cpu + python3 -m pip install -r requirements.txt + python3 -m pip install wait-for-it
45-49: Use Python Playwright container image and remove this step.This step will fail because:
- User 1001 (non-root) cannot execute
apt-getcommands—permission denied- The workflow uses
mcr.microsoft.com/playwright:v1.57.0-noble, which is the Node.js-based image without PythonThe Python-enabled Playwright image includes Python and pip pre-installed. Update the workflow to use
mcr.microsoft.com/playwright/python:v1.57.0-nobleand remove this step entirely.🔎 Required fixes in both files
In
.github/workflows/ci-tests-e2e.yaml, update the container image:container: - image: mcr.microsoft.com/playwright:v1.57.0-noble + image: mcr.microsoft.com/playwright/python:v1.57.0-nobleApply this change to all three container specifications (lines 44-45, 91-92, and 147-148).
In
.github/actions/setup-comfyui-server/action.yml, remove the apt-get step:- - name: Install pip (container mode) - if: ${{ inputs.container_mode == 'true' }} - shell: bash - run: | - apt-get update && apt-get install -y python3-pip python3-venv -.github/workflows/ci-tests-e2e.yaml (2)
91-92: Use Python-enabled Playwright container.Same issue as lines 44-46: use
mcr.microsoft.com/playwright/python:v1.57.0-nobleinstead.
147-148: Use Python-enabled Playwright container.Same issue as previous container specifications: use
mcr.microsoft.com/playwright/python:v1.57.0-noble.Note: The merge-reports job doesn't require browsers or Python—it only runs
playwright merge-reports. Using the Python Playwright image works but is heavier than necessary. Consider this addressed for consistency, or optimize later with a lighter Node.js-only image if startup time becomes a concern.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/actions/setup-comfyui-server/action.yml(2 hunks).github/workflows/ci-tests-e2e.yaml(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : Use Playwright tags like `mobile`, `2x` for relevant E2E tests - they are respected by config
📚 Learning: 2025-12-12T23:02:37.473Z
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Applied to files:
.github/actions/setup-comfyui-server/action.yml.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : E2E test files must use pattern `browser_tests/**/*.spec.ts` with Playwright
Applied to files:
.github/workflows/ci-tests-e2e.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup
- GitHub Check: test
- GitHub Check: lint-and-format
- GitHub Check: collect
🔇 Additional comments (12)
.github/actions/setup-comfyui-server/action.yml (6)
12-15: LGTM: Clear input parameter.The
container_modeinput is well-defined with appropriate defaults and documentation.
39-43: LGTM: Clean conditional logic for host mode.The renamed step with conditional execution properly separates host and container workflows.
51-57: LGTM: Standard virtual environment setup.The venv creation and PATH configuration are correct. This will work once the Python Playwright container image is used.
59-68: LGTM: Consistent pip usage within venv.Using
pipdirectly after activating the virtual environment is correct and more concise thanpython -m pip.
80-87: LGTM: Proper venv activation before server start.The server startup correctly activates the virtual environment and uses the venv's Python executable.
89-95: LGTM: Host mode server startup is correct.Using
python3directly in host mode is appropriate since the setup-python action installs it aspython3..github/workflows/ci-tests-e2e.yaml (6)
9-9: LGTM: Skip E2E tests for version bumps.Adding
version-bump*to the ignore list is a sensible optimization for automated version bump PRs.
30-38: LGTM: Artifact-based workspace sharing works with containers.Switching from cache to artifacts is necessary for container-based workflows. The entire workspace upload ensures all dependencies are available.
54-60: LGTM: Checkout before download ensures actions are available.The explicit checkout step is necessary to access
.github/actionsbefore downloading the workspace artifact.
67-67: LGTM: Correct container_mode usage.Passing
container_mode: trueactivates the container-specific setup path in the action.
100-106: LGTM: Consistent checkout-then-download pattern.Same correct approach as the chromium-sharded job.
113-113: LGTM: Correct container_mode usage.Consistent with the chromium-sharded job.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci-tests-e2e.yaml (1)
89-90: Add security option to run container as non-root user.This container specification has the same security issue as lines 42-43: missing the
--user 1001option mentioned in the PR summary.🔎 Proposed fix
container: image: mcr.microsoft.com/playwright/python:v1.57.0-noble + options: --user 1001
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/actions/setup-comfyui-server/action.yml(2 hunks).github/workflows/ci-tests-e2e.yaml(6 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : Use Playwright tags like `mobile`, `2x` for relevant E2E tests - they are respected by config
📚 Learning: 2025-12-12T23:02:37.473Z
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Applied to files:
.github/workflows/ci-tests-e2e.yaml.github/actions/setup-comfyui-server/action.yml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : E2E test files must use pattern `browser_tests/**/*.spec.ts` with Playwright
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-16T17:41:58.464Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T17:41:58.464Z
Learning: Prerequisites for repository setup: Node.js >= 24, Git repository, available ports for dev server and storybook
Applied to files:
.github/workflows/ci-tests-e2e.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint-and-format
- GitHub Check: setup
- GitHub Check: collect
- GitHub Check: test
🔇 Additional comments (11)
.github/actions/setup-comfyui-server/action.yml (5)
12-15: LGTM: Container mode input enables flexible execution.The
container_modeinput cleanly separates container and host execution paths. The default of'false'maintains backward compatibility for existing callers.
39-43: LGTM: Host-only Python setup is correctly gated.The conditional execution ensures Python setup only runs on host runners where it's needed, avoiding redundant setup in containers that already provide Python.
45-53: Verify the security implications of--break-system-packages.The
--break-system-packagesflag bypasses PEP 668 protection for externally-managed Python environments. While necessary in some containerized setups, this approach:
- Allows pip to modify system-managed packages
- Could lead to dependency conflicts or version mismatches
- Is appropriate for ephemeral CI containers but should be intentional
Consider whether a virtual environment approach would be more robust, or confirm that this flag is the intended solution for the Playwright Python container environment.
55-63: LGTM: Host mode requirements installation is correctly configured.The host mode path correctly uses
python3 -m pipwithout--break-system-packages, which is appropriate for non-containerized environments where Python is managed byactions/setup-python.
70-70: LGTM: Explicitpython3command improves reliability.Using
python3instead ofpythonensures the correct interpreter is invoked consistently across both container and host execution modes..github/workflows/ci-tests-e2e.yaml (6)
9-9: LGTM: Skipping E2E tests for version bump PRs is sensible.Version bump PRs typically only update version numbers and don't require full E2E test coverage, reducing unnecessary CI overhead.
30-36: LGTM: Artifact-based distribution is cleaner than cache-based approach.Uploading only the built
dist/directory is more efficient than the full workspace. The 1-day retention is appropriate for CI artifacts that are only needed during the workflow run.
52-58: LGTM: Artifact-based workflow requires checkout before download.The addition of checkout and artifact download steps correctly establishes the workspace needed for containerized test execution. The path mapping (
frontend-dist→dist/) aligns with the setup job's artifact upload.
65-65: LGTM: Container mode flag correctly activates container-specific setup.Passing
container_mode: trueenables the appropriate Python installation logic with--break-system-packagesflags in the composite action.
66-67: LGTM: Step name accurately reflects its reduced scope.The rename to "Setup nodejs, pnpm" correctly indicates that frontend building is handled in the setup job, and this step only installs runtime dependencies.
141-152: LGTM: Merge-reports job correctly avoids unnecessary container overhead.Running the merge-reports job directly on
ubuntu-latestwithout a container is the right choice. It only needs the Playwright CLI (via pnpm) to merge blob reports and doesn't require browsers or Python, making the lightweight approach more efficient.
Myestery
force-pushed
the
ci/playwright-docker-containers
branch
from
94ec109 to
df91d2a
Compare
|
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.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pr-update-playwright-expectations.yaml(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : Use Playwright tags like `mobile`, `2x` for relevant E2E tests - they are respected by config
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : E2E test files must use pattern `browser_tests/**/*.spec.ts` with Playwright
📚 Learning: 2025-12-12T23:02:37.473Z
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Applied to files:
.github/workflows/pr-update-playwright-expectations.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint-and-format
- GitHub Check: collect
- GitHub Check: test
- GitHub Check: setup
🔇 Additional comments (7)
.github/workflows/pr-update-playwright-expectations.yaml (7)
66-73: LGTM: Artifact-based distribution is appropriate for container workflows.The switch from cache to artifacts with 1-day retention is correct since cache doesn't work reliably inside containers.
91-95: LGTM: Artifact download correctly replaces cache restoration.The artifact-based approach properly corresponds to the upload in the setup job and works reliably in container environments.
115-139: Good practices: Git configuration and debug output for container environments.The
git config --global --add safe.directoryconfiguration (line 123) is essential for container environments where the repository is owned by a different user. The extensive debug output will help troubleshoot container-specific issues.
224-232: LGTM: Robust handling of missing artifacts.The conditional check properly handles the case where no shards produced snapshot changes, providing clear feedback.
243-251: LGTM: Clean early exit when no artifacts to merge.The guard correctly handles the scenario where no shards produced changes, preventing unnecessary processing and potential errors.
97-102: No action needed—thecontainer_mode: trueparameter properly handles Python executable access in container execution environments.
103-105: The setup-frontend action already properly implements dependency caching. It uses actions/setup-node with cache: 'pnpm', which relies on actions/cache under the hood for caching global packages data. GitHub cache works in containers; caches are found and restored properly in containerized workflows. Dependency installation will benefit from caching across runs.Likely an incorrect or invalid review comment.
| container: | ||
| image: mcr.microsoft.com/playwright/python:v1.57.0-noble |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the workflow file
echo "=== Workflow file (.github/workflows/pr-update-playwright-expectations.yaml) ==="
head -100 .github/workflows/pr-update-playwright-expectations.yaml | tail -30
echo -e "\n=== Lines around 79-80 ==="
sed -n '75,85p' .github/workflows/pr-update-playwright-expectations.yaml
echo -e "\n=== Test execution command around line 109 ==="
sed -n '105,115p' .github/workflows/pr-update-playwright-expectations.yaml
echo -e "\n=== Package.json devDependencies for @playwright ==="
grep -A 5 '"@playwright' package.json || grep -A 5 'playwright' package.json | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2121
🏁 Script executed:
#!/bin/bash
# Check all workflows for container configurations
echo "=== All container: configurations in workflows ==="
grep -r "container:" .github/workflows/ -A 2 | grep -E "container:|image:|options:" || echo "No other container configs found"
# Check if --user is used anywhere in the repo
echo -e "\n=== Search for --user in workflows ==="
grep -r "\-\-user" .github/workflows/ || echo "No --user found in workflows"
# Check GitHub Actions documentation links
echo -e "\n=== Looking for any security context in workflow files ==="
grep -r "security\|--init" .github/workflows/ | head -10 || echo "No security directives found"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 741
🏁 Script executed:
#!/bin/bash
# Check ci-tests-e2e.yaml container configuration and test commands
echo "=== ci-tests-e2e.yaml container configuration and test commands ==="
grep -A 20 "container:" .github/workflows/ci-tests-e2e.yaml | grep -E "container:|image:|run:" | head -20
echo -e "\n=== Full context around container in ci-tests-e2e.yaml ==="
grep -B 5 -A 10 "mcr.microsoft.com/playwright/python" .github/workflows/ci-tests-e2e.yaml | head -30
echo -e "\n=== Check what tests run in ci-tests-e2e.yaml ==="
grep -E "pnpm|playwright" .github/workflows/ci-tests-e2e.yaml | head -10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1776
Use Node.js Playwright container instead of Python variant.
The workflow uses mcr.microsoft.com/playwright/python:v1.57.0-noble, but executes tests with pnpm exec playwright test, which requires the Node.js Playwright test runner. The Python container includes only Python bindings, not Node.js runtime or dependencies. Switch to mcr.microsoft.com/playwright:v1.57.0-noble.
container:
- image: mcr.microsoft.com/playwright/python:v1.57.0-noble
+ image: mcr.microsoft.com/playwright:v1.57.0-noble📝 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.
| container: | |
| image: mcr.microsoft.com/playwright/python:v1.57.0-noble | |
| container: | |
| image: mcr.microsoft.com/playwright:v1.57.0-noble |
🤖 Prompt for AI Agents
In .github/workflows/pr-update-playwright-expectations.yaml around lines 79 to
80, the workflow uses the Python Playwright container image but runs the Node
Playwright test runner; replace the container.image value
mcr.microsoft.com/playwright/python:v1.57.0-noble with the Node variant
mcr.microsoft.com/playwright:v1.57.0-noble so the Node runtime and Playwright
test runner are available, then commit the change and verify the workflow can
run pnpm exec playwright test successfully.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/pr-update-playwright-expectations.yaml (1)
79-80: Container image contradicts PR objectives.This workflow has the same critical issues as
.github/workflows/ci-tests-e2e.yaml:
- Not using Microsoft containers - contradicts PR title/objectives
- Using
:latesttag - non-deterministic builds- Missing security options - no
options: --user 1001Apply the same fix as recommended for
ci-tests-e2e.yamllines 42-43:🔎 Proposed fix
container: - image: ghcr.io/myestery/comfyui-test-action:latest + image: mcr.microsoft.com/playwright:v1.57.0-noble + options: --user 1001 # ... later in steps, add before "Start ComfyUI server": + - name: Install Python for ComfyUI + run: | + sudo apt-get update + sudo apt-get install -y python3 python3-pip python3-venv
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci-tests-e2e.yaml(6 hunks).github/workflows/pr-update-playwright-expectations.yaml(4 hunks)AGENTS.md(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7415
File: browser_tests/tests/mobileBaseline.spec.ts:17-22
Timestamp: 2025-12-13T05:54:35.779Z
Learning: In browser_tests tests for the Comfy-Org/ComfyUI_frontend repository, the `comfyPage.loadWorkflow()` method already handles all necessary synchronization and waiting. No additional `await comfyPage.nextFrame()` call is needed before taking screenshots after loading a workflow.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7358
File: src/components/dialog/content/signin/SignUpForm.vue:45-54
Timestamp: 2025-12-11T12:25:24.164Z
Learning: The project has CI automation that handles code formatting (pnpm format) automatically, so manual formatting suggestions should not be provided in code reviews for the Comfy-Org/ComfyUI_frontend repository.
📚 Learning: 2025-12-12T23:02:37.473Z
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Applied to files:
.github/workflows/ci-tests-e2e.yaml.github/workflows/pr-update-playwright-expectations.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml.github/workflows/pr-update-playwright-expectations.yaml
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : E2E test files must use pattern `browser_tests/**/*.spec.ts` with Playwright
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to tests-ui/**/*.test.ts : Write tests for all changes, especially bug fixes to catch future regressions
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : Use Playwright tags like `mobile`, `2x` for relevant E2E tests - they are respected by config
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-16T17:41:58.464Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T17:41:58.464Z
Learning: Prerequisites for repository setup: Node.js >= 24, Git repository, available ports for dev server and storybook
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Follow YAGNI, AHA, DRY, SOLID principles in code design
Applied to files:
AGENTS.md
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Maintain clean, stable public APIs and practice domain-driven design
Applied to files:
AGENTS.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint-and-format
- GitHub Check: setup
- GitHub Check: test
- GitHub Check: collect
🔇 Additional comments (7)
AGENTS.md (1)
212-212: LGTM: Documentation clarity improvement.The wording change from "SOLID" to "SOLID principles" improves clarity and consistency in the documentation.
.github/workflows/ci-tests-e2e.yaml (3)
9-9: LGTM: Skip E2E tests for version bump PRs.Adding
version-bump*to the branches-ignore list is a reasonable optimization that avoids running expensive E2E tests for automated version bump PRs.
30-36: LGTM: Efficient artifact-based workspace sharing.Switching from cache to artifacts for workspace sharing is correct (cache doesn't work with containers), and uploading only
dist/is more efficient than uploading the entire workspace.
141-152: LGTM: Efficient merge-reports job without container overhead.The
merge-reportsjob correctly runs without a container since it only needs the Playwright CLI to merge blob reports. This avoids the overhead of downloading the container image (~1GB+) when browsers aren't needed..github/workflows/pr-update-playwright-expectations.yaml (3)
67-73: LGTM: Consistent artifact-based approach.Switching to artifacts for
dist/sharing is consistent with the changes inci-tests-e2e.yamland necessary for container-based workflows.
116-140: LGTM: Good container-specific hardening and debugging.The additions provide essential container environment support:
- Line 123-124:
git config --global --add safe.directoryis necessary when running Git operations inside containers- Lines 126-140: Extensive debugging output helps troubleshoot container environment issues and makes the workflow more maintainable
These are good defensive programming practices for containerized workflows.
225-252: LGTM: Robust handling of empty artifact scenarios.The safety checks at lines 225-233 and 244-252 properly handle cases where no shards produce snapshot changes. This prevents failures when attempting to process non-existent directories and provides clear logging of the zero-change scenario.
| container: | ||
| image: ghcr.io/myestery/comfyui-test-action:latest |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and its line count
if [ -f .github/workflows/ci-tests-e2e.yaml ]; then
wc -l .github/workflows/ci-tests-e2e.yaml
echo "---"
# Read lines 35-100 to see context around lines 42-43 and 90-91
sed -n '35,100p' .github/workflows/ci-tests-e2e.yaml
else
echo "File not found"
ls -la .github/workflows/
fiRepository: Comfy-Org/ComfyUI_frontend
Length of output: 2241
Address container image versioning and security configuration issues.
The current container specifications use ghcr.io/myestery/comfyui-test-action:latest, which introduces two CI/CD best practice issues:
- Non-deterministic image tag - Using
:latestprevents reproducible builds. Pin to a specific version (e.g.,ghcr.io/myestery/comfyui-test-action:v1.0.0) - Missing security options - Add
options: --user 1001to match GitHub Actions' default runner user, preventing permission issues with mounted workspace
Update both container specifications at lines 42-43 and ~85-86:
container:
image: ghcr.io/myestery/comfyui-test-action:v1.0.0 # Replace with specific version
options: --user 1001If maintaining a custom container image, ensure the Dockerfile is in this repository with clear documentation of included tools (Playwright, Python, Node.js versions).
🤖 Prompt for AI Agents
.github/workflows/ci-tests-e2e.yaml lines 42-43 (and similarly update around
85-86): the workflow currently pins the container image to
ghcr.io/myestery/comfyui-test-action:latest and omits security options; change
the image tag to a specific release (e.g.,
ghcr.io/myestery/comfyui-test-action:v1.0.0 or the correct release you maintain)
and add options: --user 1001 under the container stanza to match the runner user
and avoid permission issues; ensure both container blocks are updated
consistently and, if using a custom image, document the Dockerfile and included
tool versions in the repo.
14f5ccc to
8525806
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
.github/workflows/ci-tests-e2e.yaml (2)
90-91: Apply the same container fixes from lines 42-43.This container specification has the same critical issues: using
:latesttag and missingoptions: --user 1001. Apply the same fixes suggested for lines 42-43.
42-43: Fix all container configurations: use digest pinning and add security options.Three workflows use the custom container with
:latesttag and missing security options. Docker images use digest pinning for security, as the latest tag is mutable and subject to change. Additionally, GitHub Actions use UID 1001, so containers should run as this user to avoid permission issues.Affected locations:
.github/workflows/ci-tests-e2e.yamllines 42-43, 90-91.github/workflows/pr-update-playwright-expectations.yaml(similar container usage)Issues:
- Using
:latesttag breaks reproducibility — once an image is built, its digest always refers to that specific build, ensuring immutability and consistency- Missing
options: --user 1001— required to match the GitHub Actions runner user and prevent permission issues with workspace files- External container from personal repository with no visibility into contents or build process
Fix:
Replace the image reference with a digest-pinned version and add user options:container: - image: ghcr.io/myestery/comfyui-test-action:latest + image: ghcr.io/myestery/comfyui-test-action@sha256:<digest> # Replace with actual digest + options: --user 1001Obtain the digest by running:
docker inspect --format='{{index .RepoDigests 0}}' ghcr.io/myestery/comfyui-test-action:latestAlternatively, document the container contents (Python, Node.js, pnpm, Playwright versions) or add the Dockerfile to this repository for transparency.
.github/workflows/pr-update-playwright-expectations.yaml (2)
79-80: Apply the same container fixes as ci-tests-e2e.yaml.This container specification has the same critical issues identified in
ci-tests-e2e.yaml(lines 42-43):
- Using
:latesttag instead of pinned version- Missing
options: --user 1001for securityApply the same fixes:
container: - image: ghcr.io/myestery/comfyui-test-action:latest + image: ghcr.io/myestery/comfyui-test-action:v1.0.0 # Replace with actual version + options: --user 1001
97-102: Apply the same server startup improvements as ci-tests-e2e.yaml.This server startup has the same issues identified in
ci-tests-e2e.yaml(lines 60-65): hardcoded paths, no error handling, and background process errors being lost. Apply the same error handling and verification improvements suggested there.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci-tests-e2e.yaml(6 hunks).github/workflows/pr-update-playwright-expectations.yaml(4 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7415
File: browser_tests/tests/mobileBaseline.spec.ts:17-22
Timestamp: 2025-12-13T05:54:35.779Z
Learning: In browser_tests tests for the Comfy-Org/ComfyUI_frontend repository, the `comfyPage.loadWorkflow()` method already handles all necessary synchronization and waiting. No additional `await comfyPage.nextFrame()` call is needed before taking screenshots after loading a workflow.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
📚 Learning: 2025-12-12T23:02:37.473Z
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Applied to files:
.github/workflows/ci-tests-e2e.yaml.github/workflows/pr-update-playwright-expectations.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml.github/workflows/pr-update-playwright-expectations.yaml
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to tests-ui/**/*.test.ts : Write tests for all changes, especially bug fixes to catch future regressions
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : Use Playwright tags like `mobile`, `2x` for relevant E2E tests - they are respected by config
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to browser_tests/**/*.spec.ts : E2E test files must use pattern `browser_tests/**/*.spec.ts` with Playwright
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-16T17:41:58.464Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T17:41:58.464Z
Learning: Prerequisites for repository setup: Node.js >= 24, Git repository, available ports for dev server and storybook
Applied to files:
.github/workflows/ci-tests-e2e.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup
- GitHub Check: test
- GitHub Check: lint-and-format
- GitHub Check: collect
🔇 Additional comments (6)
.github/workflows/ci-tests-e2e.yaml (3)
9-9: LGTM: Skip E2E tests for version bump PRs.Excluding
version-bump*branches from E2E tests is a reasonable optimization since version bump PRs typically don't require full E2E validation.
30-36: LGTM: Artifact-based frontend distribution.Uploading only the built
dist/as an artifact is the correct approach for container-based workflows, since cache restoration doesn't work inside containers. The 1-day retention is appropriate for ephemeral CI artifacts.
141-152: LGTM: Merge reports without container overhead.Correctly removed the container from the
merge-reportsjob since it only runs CLI commands and doesn't need browsers. This reduces startup time and resource usage..github/workflows/pr-update-playwright-expectations.yaml (3)
67-73: LGTM: Consistent artifact handling.The artifact upload matches the pattern in
ci-tests-e2e.yaml, which is the correct approach for container-based workflows.
116-134: LGTM: Good defensive programming for containers.The additions are well-thought-out for container environments:
- Git safe.directory configuration (lines 123-124) - required because container ownership differs from the checkout action
- Debug output (lines 126-135) - helps troubleshoot snapshot detection issues
- Error handling with
2>/dev/null || echo- prevents script failures from missing directoriesThese changes show proper understanding of container-specific challenges.
225-252: LGTM: Graceful handling of empty artifact scenarios.The enhanced merge logic correctly handles edge cases:
- Check for missing artifacts (lines 245-252) - gracefully exits when no shards produced changes, preventing errors from missing directories
- Verify target directory (lines 254-258) - fails fast with clear error if
browser_testsdoesn't exist- Informative output - reports "0 files" instead of failing, improving debugging experience
This defensive approach prevents spurious workflow failures when legitimately no snapshots need updating.
Based on learnings, the scoped snapshot handling aligns with the workflow's intentional design.
- Use ghcr.io/myestery/comfyui-test-action:latest container - Container includes: Playwright, Node.js, pnpm, Python, ComfyUI backend - Simplified setup: just copy devtools and start server directly - No more cloning ComfyUI or pip install in each job - Add version-bump* to branches-ignore
Myestery
force-pushed
the
ci/playwright-docker-containers
branch
from
8525806 to
087317e
Compare
| wait-for-it --service 127.0.0.1:8188 -t 600 | ||
| # Run sharded tests and upload sharded reports | ||
| - name: Install frontend deps |
There was a problem hiding this comment.
we might be able to do without this step. will check later
Summary
Use a pre-built container image with all dependencies for Playwright E2E tests, eliminating ~130s setup time per shard.
Changes
ghcr.io/myestery/comfyui-test-action:latestcontainer for test jobsversion-bump*tobranches-ignoreto skip E2E tests for version bump PRsBenefits
Container Image
Repository: https://github.com/Myestery/comfyui-test-action
Image:
ghcr.io/myestery/comfyui-test-action:latestTest plan