hotfix: stabilize flaky workflow sidebar browser tests #7280
Conversation
- Add retry logic for menu hover in headless mode (Topbar.ts) - Add waitForDialogMaskHidden helper for dialog timing (Topbar.ts) - Handle confirmation dialog detection in _saveWorkflow (Topbar.ts) - Wait for workflow service completion in ConfirmDialog.click (ComfyPage.ts) - Add nextFrame after syncWorkflows in setupWorkflowsDirectory (ComfyPage.ts) - Wait for workflow service in renameWorkflow (SidebarTab.ts) - Add explicit waits in drop workflow test (workflows.spec.ts)
dosubot
bot
added
the
size:M
📝 WalkthroughWalkthroughThe changes improve test reliability across ComfyUI's browser test suite by adding explicit waits for async operations, polling for workflow system state, implementing retry logic for flaky menu interactions, and converting time-based assertions to retryable polling-based checks. Changes
Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx,js,jsx,vue,json}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,vue}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-11-24T19:47:22.909ZApplied to files:
📚 Learning: 2025-12-09T20:22:23.581ZApplied to files:
📚 Learning: 2025-12-09T20:22:23.581ZApplied to files:
📚 Learning: 2025-11-24T19:48:03.270ZApplied to files:
📚 Learning: 2025-11-24T19:47:22.909ZApplied to files:
📚 Learning: 2025-11-24T19:48:03.270ZApplied to files:
📚 Learning: 2025-12-09T03:39:54.501ZApplied 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). (1)
🔇 Additional comments (2)
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 |
🎭 Playwright Test Results⏰ Completed at: 12/10/2025, 12:39:12 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 12/09/2025, 11:54:01 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.22 MB (baseline 3.22 MB) • ⚪ 0 BMain entry bundles and manifests
Graph Workspace — 988 kB (baseline 988 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 — 298 kB (baseline 298 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
UI Components — 176 kB (baseline 176 kB) • ⚪ 0 BReusable component library chunks
Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Utilities & Hooks — 3.18 kB (baseline 3.18 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 8.56 MB (baseline 8.56 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 3.81 MB (baseline 3.81 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
browser_tests/fixtures/components/Topbar.ts (1)
200-219: Unreachable code after loop changes.The
await currentMenu.click()at line 218 is now unreachable because the loop always returns early wheni === path.length - 1(line 211). Additionally, thecurrentMenureassignment at line 216 is no longer used.Clean up the unreachable code:
let currentMenu = topLevelMenu for (let i = 1; i < path.length; i++) { const commandName = path[i] const menuItem = submenu .locator(`.p-tieredmenu-item:has-text("${commandName}")`) .first() await menuItem.waitFor({ state: 'visible' }) // For the last item, click it if (i === path.length - 1) { await menuItem.click() return } // Otherwise, hover to open nested submenu await menuItem.hover() - currentMenu = menuItem } - await currentMenu.click() }Or simplify by removing the
currentMenuvariable entirely since it's only used for the initial hover.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
browser_tests/fixtures/ComfyPage.ts(2 hunks)browser_tests/fixtures/components/SidebarTab.ts(1 hunks)browser_tests/fixtures/components/Topbar.ts(3 hunks)browser_tests/tests/sidebar/workflows.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript exclusively (no new JavaScript)
Files:
browser_tests/fixtures/ComfyPage.tsbrowser_tests/fixtures/components/SidebarTab.tsbrowser_tests/tests/sidebar/workflows.spec.tsbrowser_tests/fixtures/components/Topbar.ts
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,vue}: Do not useanytype in TypeScript code
Do not useas anytype assertions in TypeScript code; fix the underlying type issue
Style formatting: 2 space indent, single quotes, no trailing semicolons, 80 character width
Import statements should be sorted and grouped by plugin; runpnpm formatbefore committing
ESLint rules: no floating promises, no unused imports, i18n raw text restrictions in templates
Files:
browser_tests/fixtures/ComfyPage.tsbrowser_tests/fixtures/components/SidebarTab.tsbrowser_tests/tests/sidebar/workflows.spec.tsbrowser_tests/fixtures/components/Topbar.ts
**/*.{ts,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Use vue-i18n in Composition API for string literals and place new translation entries in
src/locales/en/main.json
Files:
browser_tests/fixtures/ComfyPage.tsbrowser_tests/fixtures/components/SidebarTab.tsbrowser_tests/tests/sidebar/workflows.spec.tsbrowser_tests/fixtures/components/Topbar.ts
browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (browser_tests/CLAUDE.md)
browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx}: Test user workflows in browser tests
Use Playwright fixtures for browser tests
Follow naming conventions for browser tests
Check assets/ directory for test data when writing tests
Prefer specific selectors in browser tests
Test across multiple viewports
Files:
browser_tests/tests/sidebar/workflows.spec.ts
browser_tests/**/*.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
Write E2E tests in
browser_tests/**/*.spec.tsusing Playwright
Files:
browser_tests/tests/sidebar/workflows.spec.ts
🧠 Learnings (6)
📓 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} : Test user workflows in 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} : 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: Create browser tests as appropriate after code changes
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} : Prefer specific selectors in browser tests
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T02:44:38.234Z
Learning: Write tests for all changes, especially bug fixes to catch future regressions
📚 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:
browser_tests/fixtures/ComfyPage.tsbrowser_tests/fixtures/components/SidebarTab.tsbrowser_tests/tests/sidebar/workflows.spec.tsbrowser_tests/fixtures/components/Topbar.ts
📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: tests-ui/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:48:03.270Z
Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Write tests for new features
Applied to files:
browser_tests/fixtures/ComfyPage.tsbrowser_tests/tests/sidebar/workflows.spec.tsbrowser_tests/fixtures/components/Topbar.ts
📚 Learning: 2025-12-09T03:39:54.501Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7169
File: src/platform/remote/comfyui/jobs/jobTypes.ts:1-107
Timestamp: 2025-12-09T03:39:54.501Z
Learning: In the ComfyUI_frontend project, Zod is on v3.x. Do not suggest Zod v4 standalone validators (z.uuid, z.ulid, z.cuid2, z.nanoid) until an upgrade to Zod 4 is performed. When reviewing TypeScript files (e.g., src/platform/remote/comfyui/jobs/jobTypes.ts) validate against Zod 3 capabilities and avoid introducing v4-specific features; flag any proposal to upgrade or incorporate v4-only validators and propose staying with compatible 3.x patterns.
Applied to files:
browser_tests/fixtures/ComfyPage.tsbrowser_tests/fixtures/components/SidebarTab.tsbrowser_tests/tests/sidebar/workflows.spec.tsbrowser_tests/fixtures/components/Topbar.ts
📚 Learning: 2025-12-09T02:44:38.234Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T02:44:38.234Z
Learning: Applies to browser_tests/**/*.spec.ts : Write E2E tests in `browser_tests/**/*.spec.ts` using Playwright
Applied to files:
browser_tests/tests/sidebar/workflows.spec.ts
📚 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} : Follow naming conventions for browser tests
Applied to files:
browser_tests/tests/sidebar/workflows.spec.ts
⏰ 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: test
- GitHub Check: setup
- GitHub Check: lint-and-format
- GitHub Check: collect
🔇 Additional comments (6)
browser_tests/fixtures/components/SidebarTab.ts (1)
140-147: Good addition for workflow service synchronization.The wait for
workflow.isBusyto become false ensures the rename operation completes before the test proceeds. This pattern aligns with similar waits added inComfyPage.tsandTopbar.ts.browser_tests/fixtures/ComfyPage.ts (2)
129-143: Well-structured dialog synchronization.The defensive check for mask count before waiting prevents unnecessary failures when no mask is present. The combined wait for both mask disappearance and workflow service idle state ensures proper sequencing.
259-262: Good use of frame synchronization for Vue reactivity.Waiting for the next animation frame after
syncWorkflows()gives Vue time to process reactive updates and re-render the workflow list before tests assert on the UI state.browser_tests/tests/sidebar/workflows.spec.ts (2)
343-348: Good explicit synchronization before drag operation.Waiting for the workflow item to be visible before interacting with it is the correct approach for test stability. This replaces implicit timing assumptions with explicit state verification.
367-370: Appropriate wait for node insertion.The
nextFrame()call after drag-and-drop allows the application to process the drop event and insert nodes before the assertion runs.browser_tests/fixtures/components/Topbar.ts (1)
109-121: Good defensive menu state management.Resetting the menu state when it's already open ensures predictable behavior regardless of prior test actions or flaky states. This prevents cascading failures from stale menu states.
| // Hover over top-level menu with retry logic for flaky submenu appearance | ||
| const submenu = this.getVisibleSubmenu() |
There was a problem hiding this comment.
Why is the appearance nondeterministic?
DrJKL
left a comment
DrJKL
left a comment
There was a problem hiding this comment.
I have some structural comments about how we're using Playwright in here.
|
Failing unit tests are my fault -- fixing now in #7281 |
Yes but I broke browser tests too 😀 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
browser_tests/fixtures/ComfyPage.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript exclusively (no new JavaScript)
Files:
browser_tests/fixtures/ComfyPage.ts
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,vue}: Do not useanytype in TypeScript code
Do not useas anytype assertions in TypeScript code; fix the underlying type issue
Style formatting: 2 space indent, single quotes, no trailing semicolons, 80 character width
Import statements should be sorted and grouped by plugin; runpnpm formatbefore committing
ESLint rules: no floating promises, no unused imports, i18n raw text restrictions in templates
Files:
browser_tests/fixtures/ComfyPage.ts
**/*.{ts,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Use vue-i18n in Composition API for string literals and place new translation entries in
src/locales/en/main.json
Files:
browser_tests/fixtures/ComfyPage.ts
🧠 Learnings (5)
📓 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} : Test user workflows in 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} : Use Playwright fixtures for browser tests
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T02:44:38.234Z
Learning: Applies to browser_tests/**/*.spec.ts : Write E2E tests in `browser_tests/**/*.spec.ts` using Playwright
📚 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:
browser_tests/fixtures/ComfyPage.ts
📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: tests-ui/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:48:03.270Z
Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Write tests for new features
Applied to files:
browser_tests/fixtures/ComfyPage.ts
📚 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:
browser_tests/fixtures/ComfyPage.ts
📚 Learning: 2025-12-09T03:39:54.501Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7169
File: src/platform/remote/comfyui/jobs/jobTypes.ts:1-107
Timestamp: 2025-12-09T03:39:54.501Z
Learning: In the ComfyUI_frontend project, Zod is on v3.x. Do not suggest Zod v4 standalone validators (z.uuid, z.ulid, z.cuid2, z.nanoid) until an upgrade to Zod 4 is performed. When reviewing TypeScript files (e.g., src/platform/remote/comfyui/jobs/jobTypes.ts) validate against Zod 3 capabilities and avoid introducing v4-specific features; flag any proposal to upgrade or incorporate v4-only validators and propose staying with compatible 3.x patterns.
Applied to files:
browser_tests/fixtures/ComfyPage.ts
⏰ 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)
browser_tests/fixtures/ComfyPage.ts (1)
259-261: LGTM!Adding
nextFrame()aftersyncWorkflows()is a solid fix for ensuring Vue has completed its reactive updates before proceeding with test assertions. This pattern effectively addresses the test isolation issues mentioned in the PR objectives.
| // Check if a confirmation dialog appeared (e.g., "Overwrite existing file?") | ||
| // If so, return early to let the test handle the confirmation | ||
| const confirmationDialog = this.page.locator( | ||
| '.p-dialog:has-text("Overwrite")' |
There was a problem hiding this comment.
- Remove commented-out waitForDialogMaskHidden code - Add re-hover logic after menu retry to properly trigger submenu - Use Playwright expect assertions instead of manual waitFor - Use getPersistedItem helper for user-facing locator - Make node count assertion retryable with expect.poll
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)
browser_tests/fixtures/components/Topbar.ts (1)
205-216: Remove unreachable dead code.The early return on line 208 when
i === path.length - 1means line 215 (await currentMenu.click()) can never be executed. This code should be removed to improve clarity.Apply this diff to remove the dead code:
// Otherwise, hover to open nested submenu await menuItem.hover() currentMenu = menuItem } - await currentMenu.click() } }
♻️ Duplicate comments (1)
browser_tests/fixtures/components/Topbar.ts (1)
98-100: Consider using semantic locators for the confirmation dialog.The current selector
.p-dialog:has-text("Overwrite")works, but Playwright best practices recommend using more semantic locators likegetByRoleorgetByTextfor better accessibility and specificity.Apply this diff to use a more semantic locator:
- const confirmationDialog = this.page.locator( - '.p-dialog:has-text("Overwrite")' - ) + const confirmationDialog = this.page.getByRole('dialog').filter({ + hasText: 'Overwrite' + })Based on learnings: Follow Playwright best practices for E2E tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
browser_tests/fixtures/components/Topbar.ts(2 hunks)browser_tests/tests/sidebar/workflows.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (browser_tests/CLAUDE.md)
browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx}: Test user workflows in browser tests
Use Playwright fixtures for browser tests
Follow naming conventions for browser tests
Check assets/ directory for test data when writing tests
Prefer specific selectors in browser tests
Test across multiple viewports
Files:
browser_tests/tests/sidebar/workflows.spec.ts
**/*.{ts,tsx,js,jsx,vue,json}
📄 CodeRabbit inference engine (AGENTS.md)
Code style: Use 2-space indentation, single quotes, no trailing semicolons, and 80-character line width (see
.prettierrc)
Files:
browser_tests/tests/sidebar/workflows.spec.tsbrowser_tests/fixtures/components/Topbar.ts
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,vue}: Imports must be sorted and grouped by plugin; runpnpm formatbefore committing
Use TypeScript for type safety; never useanytype - use proper TypeScript types
Never useas anytype assertions; fix the underlying type issue instead
Use es-toolkit for utility functions
Write code that is expressive and self-documenting; avoid comments unless absolutely necessary; do not add or retain redundant comments
Keep functions short and functional
Minimize nesting in code (e.g., deeply nestediforforstatements); apply the Arrow Anti-Pattern principle
Avoid mutable state; prefer immutability and assignment at point of declaration
Favor pure functions, especially testable ones
Files:
browser_tests/tests/sidebar/workflows.spec.tsbrowser_tests/fixtures/components/Topbar.ts
browser_tests/**/*.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
browser_tests/**/*.spec.ts: E2E test files must be namedbrowser_tests/**/*.spec.tsand use Playwright
Follow Playwright best practices described in the official documentation for E2E tests
Do not usewaitForTimeoutin Playwright tests; use Locator actions and retrying assertions instead
Use tags like@mobileand@2xin Playwright tests for relevant test scenarios
Files:
browser_tests/tests/sidebar/workflows.spec.ts
🧠 Learnings (16)
📓 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} : Test user workflows in browser tests
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.581Z
Learning: Applies to browser_tests/**/*.spec.ts : Do not use `waitForTimeout` in Playwright tests; use Locator actions and retrying assertions instead
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-09T20:22:23.581Z
Learning: Applies to browser_tests/**/*.spec.ts : Follow Playwright best practices described in the official documentation for E2E tests
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.581Z
Learning: Applies to browser_tests/**/*.spec.ts : Use tags like `mobile` and `2x` in Playwright tests for relevant test scenarios
📚 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:
browser_tests/tests/sidebar/workflows.spec.tsbrowser_tests/fixtures/components/Topbar.ts
📚 Learning: 2025-12-09T20:22:23.581Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.581Z
Learning: Applies to browser_tests/**/*.spec.ts : Do not use `waitForTimeout` in Playwright tests; use Locator actions and retrying assertions instead
Applied to files:
browser_tests/tests/sidebar/workflows.spec.tsbrowser_tests/fixtures/components/Topbar.ts
📚 Learning: 2025-12-09T20:22:23.581Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.581Z
Learning: Applies to browser_tests/**/*.spec.ts : Follow Playwright best practices described in the official documentation for E2E tests
Applied to files:
browser_tests/tests/sidebar/workflows.spec.tsbrowser_tests/fixtures/components/Topbar.ts
📚 Learning: 2025-12-09T20:22:23.581Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.581Z
Learning: Applies to **/*.test.ts : Write tests for all changes, especially bug fixes to catch future regressions
Applied to files:
browser_tests/tests/sidebar/workflows.spec.ts
📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: tests-ui/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:48:03.270Z
Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Write tests for new features
Applied to files:
browser_tests/tests/sidebar/workflows.spec.tsbrowser_tests/fixtures/components/Topbar.ts
📚 Learning: 2025-11-24T19:47:56.371Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : Use provided test helpers `createTestSubgraph` and `createTestSubgraphNode` from `./fixtures/subgraphHelpers` for consistent subgraph test setup
Applied to files:
browser_tests/tests/sidebar/workflows.spec.ts
📚 Learning: 2025-12-09T20:22:23.581Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.581Z
Learning: Applies to **/*.test.ts : Aim for behavioral coverage of critical and new features in unit tests
Applied to files:
browser_tests/tests/sidebar/workflows.spec.ts
📚 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:
browser_tests/tests/sidebar/workflows.spec.ts
📚 Learning: 2025-12-09T20:22:23.581Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.581Z
Learning: Applies to **/*.test.ts : Do not write change detector tests that just assert default values
Applied to files:
browser_tests/tests/sidebar/workflows.spec.ts
📚 Learning: 2025-11-24T19:47:56.371Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{ts,tsx} : Type assertions are an absolute last resort. In almost all cases, they are a crutch that leads to brittle code
Applied to files:
browser_tests/tests/sidebar/workflows.spec.ts
📚 Learning: 2025-12-09T20:22:23.581Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.581Z
Learning: Applies to browser_tests/**/*.spec.ts : E2E test files must be named `browser_tests/**/*.spec.ts` and use Playwright
Applied to files:
browser_tests/tests/sidebar/workflows.spec.ts
📚 Learning: 2025-12-09T20:22:23.581Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.581Z
Learning: Applies to **/*.test.ts : Do not write tests that just test the mocks; ensure tests fail when code behaves unexpectedly
Applied to files:
browser_tests/tests/sidebar/workflows.spec.ts
📚 Learning: 2025-12-09T20:22:23.581Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.581Z
Learning: Applies to **/*.{ts,tsx,vue} : Never use `as any` type assertions; fix the underlying type issue instead
Applied to files:
browser_tests/tests/sidebar/workflows.spec.ts
📚 Learning: 2025-12-09T03:39:54.501Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7169
File: src/platform/remote/comfyui/jobs/jobTypes.ts:1-107
Timestamp: 2025-12-09T03:39:54.501Z
Learning: In the ComfyUI_frontend project, Zod is on v3.x. Do not suggest Zod v4 standalone validators (z.uuid, z.ulid, z.cuid2, z.nanoid) until an upgrade to Zod 4 is performed. When reviewing TypeScript files (e.g., src/platform/remote/comfyui/jobs/jobTypes.ts) validate against Zod 3 capabilities and avoid introducing v4-specific features; flag any proposal to upgrade or incorporate v4-only validators and propose staying with compatible 3.x patterns.
Applied to files:
browser_tests/tests/sidebar/workflows.spec.tsbrowser_tests/fixtures/components/Topbar.ts
📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: tests-ui/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:48:03.270Z
Learning: Check tests-ui/README.md for test guidelines
Applied to files:
browser_tests/fixtures/components/Topbar.ts
⏰ 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: setup
- GitHub Check: test
🔇 Additional comments (3)
browser_tests/tests/sidebar/workflows.spec.ts (2)
343-346: LGTM! Proper synchronization before drag-and-drop.The explicit visibility wait ensures the workflow item is present and visible before attempting the drag-and-drop operation, addressing the flakiness mentioned in the PR objectives. This follows Playwright best practices by using retrying assertions instead of fixed timeouts.
367-370: LGTM! Retryable assertion addresses flakiness.Using
expect.poll()is the correct Playwright pattern for waiting on async operations. This allows the assertion to retry until the nodes are inserted or the timeout is reached, which properly addresses the test flakiness mentioned in the PR objectives.Based on learnings: This follows Playwright best practices and avoids
waitForTimeoutas recommended.browser_tests/fixtures/components/Topbar.ts (1)
182-195: LGTM! Pragmatic retry logic for flaky submenu appearance.The retry logic properly handles cases where the submenu doesn't appear on the first hover attempt. The re-hover after menu reset (line 193) correctly addresses previous review feedback. While this treats the symptom rather than the root cause, it's an appropriate pragmatic solution for a hotfix focused on test stability.
Note: The same hardcoded click position concern from lines 106-113 applies here.
| async openTopbarMenu() { | ||
| // If menu is already open, close it first to reset state | ||
| const isAlreadyOpen = await this.menuLocator.isVisible() | ||
| if (isAlreadyOpen) { | ||
| // Click outside the menu to close it properly | ||
| await this.page.locator('body').click({ position: { x: 500, y: 300 } }) | ||
| await this.menuLocator.waitFor({ state: 'hidden', timeout: 1000 }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a more reliable method to close the menu.
Clicking on body at hardcoded coordinates { x: 500, y: 300 } works but may be fragile across different viewport sizes or layouts. This same pattern appears in the retry logic as well.
Consider one of these alternatives:
- Click on a specific element known to be outside the menu (e.g., a canvas or sidebar element)
- Use
page.mouse.click()at a calculated position relative to the menu bounds - Press the Escape key if the menu supports keyboard dismissal
Example:
// If menu is already open, close it first to reset state
const isAlreadyOpen = await this.menuLocator.isVisible()
if (isAlreadyOpen) {
// Press Escape to close the menu
await this.page.keyboard.press('Escape')
await this.menuLocator.waitFor({ state: 'hidden', timeout: 1000 })
}🤖 Prompt for AI Agents
In browser_tests/fixtures/components/Topbar.ts around lines 106 to 113, the code
uses a hardcoded body.click at { x: 500, y: 300 } to close the menu which is
fragile; replace that with a reliable dismissal strategy (choose one): press the
Escape key via page.keyboard.press('Escape') and wait for menuLocator to be
hidden, or click a stable off-menu element (e.g., a known canvas or sidebar
locator) using its locator.click(), or compute an off-menu coordinate using
this.menuLocator.boundingBox() and use page.mouse.click() at a position
calculated relative to the menu bounds; apply the same replacement to the retry
logic elsewhere so all hardcoded-body clicks are removed and waiting for
menuLocator to be hidden is preserved.
#7294) ## Summary Move color/bgcolor initialization from class field overrides to constructor to preserve LGraphNodeProperties getter/setter instrumentation. Class field overrides were replacing the reactive property descriptors set by the parent constructor, preventing change events from firing. issue found while tesing in #3449 ## Screenshots Before https://github.com/user-attachments/assets/04499a3a-15c2-44fd-9819-6dd5f6849f20 After https://github.com/user-attachments/assets/ba93278b-9761-4d45-abb3-2a57ff95a900 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7294-fix-Note-MarkdownNote-node-color-change-not-reactive-in-vueNodes-mode-2c46d73d3650818f8ee6f6f0c0e61d39) by [Unito](https://www.unito.io)
## Summary Standing on the shoulders of giants. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7295-Testing-section-update-2c46d73d3650818f935bcb2ac65d9830) by [Unito](https://www.unito.io)
## Summary Adds a dropdown filter to the model browser that allows users to filter assets by ownership (All, My models, Public models), based on the `is_immutable` property. ## Changes - **Filter UI**: Added ownership dropdown in [AssetFilterBar.vue](src/platform/assets/components/AssetFilterBar.vue#L30-L38) that only appears when user has uploaded models - **Filter Logic**: Implemented `filterByOwnership` function in [useAssetBrowser.ts](src/platform/assets/composables/useAssetBrowser.ts#L38-L45) to filter by `is_immutable` property - **i18n**: Added translation strings for ownership filter options - **Tests**: Added comprehensive tests for ownership filtering in both composable and component test files ## Review Focus - The ownership filter visibility logic correctly checks for mutable assets (`!is_immutable`) - Default filter value is 'all' to show all models initially - Filter integrates cleanly with existing file format and base model filters 🤖 Generated with [Claude Code](https://claude.com/claude-code) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7201-feat-Add-ownership-filter-to-model-browser-2c16d73d365081f280f6d1e42e5400af) by [Unito](https://www.unito.io) --------- Co-authored-by: Claude <[email protected]> Co-authored-by: GitHub Action <[email protected]>
## Summary Move the interrupt control into the actionbar so cancellation sits with the run controls. ## Changes - add a cancel button to the actionbar with the existing interrupt tooltip and disabled state - remove the cancel button and related execution wiring from the top menu section to avoid duplication ## Review Focus - spacing/hover states of the new cancel control in both docked and floating modes ## Screenshots (if applicable) - n/a Tests: pnpm typecheck; pnpm lint:fix ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7297-Move-cancel-button-into-actionbar-2c46d73d36508198b00cf011390289f6) by [Unito](https://www.unito.io)
dosubot
bot
added
size:XL
dosubot
bot
added
size:M
|
Can you address review comments in future PRs? We should merge this for now if it restores a clean CI signal for all other devs. |
christian-byrne
added
needs-backport
## Summary - Fix flaky workflow sidebar browser tests that were failing in headless mode - Add retry logic for menu hover operations in Topbar - Add proper timing/wait helpers for dialog masks and workflow service completion - Fix test isolation issues in setupWorkflowsDirectory and drop workflow test ## Test plan - [x] Run `pnpm test:browser -- browser_tests/tests/sidebar/workflows.spec.ts` multiple times - [x] Verify the 3 previously failing tests now pass consistently: - "Can overwrite other workflows with save as" - "Can rename nested workflow from opened workflow item" - "Can drop workflow from workflows sidebar" ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7280-hotfix-stabilize-flaky-workflow-sidebar-browser-tests-2c46d73d365081c5b3badfafe35a63dc) by [Unito](https://www.unito.io) --------- Co-authored-by: Terry Jia <[email protected]> Co-authored-by: Alexander Brown <[email protected]> Co-authored-by: Luke Mino-Altherr <[email protected]> Co-authored-by: Claude <[email protected]> Co-authored-by: GitHub Action <[email protected]> Co-authored-by: Benjamin Lu <[email protected]>
github-actions
bot
removed
the
needs-backport
… tests (#7504) Backport of #7280 to `core/1.34` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7504-backport-core-1-34-hotfix-stabilize-flaky-workflow-sidebar-browser-tests-2ca6d73d3650810ab183d3f0e9316b70) by [Unito](https://www.unito.io) Co-authored-by: Johnpaul Chiwetelu <[email protected]> Co-authored-by: Terry Jia <[email protected]> Co-authored-by: Alexander Brown <[email protected]> Co-authored-by: Luke Mino-Altherr <[email protected]> Co-authored-by: Claude <[email protected]> Co-authored-by: GitHub Action <[email protected]> Co-authored-by: Benjamin Lu <[email protected]>
## Summary - Fix flaky workflow sidebar browser tests that were failing in headless mode - Add retry logic for menu hover operations in Topbar - Add proper timing/wait helpers for dialog masks and workflow service completion - Fix test isolation issues in setupWorkflowsDirectory and drop workflow test ## Test plan - [x] Run `pnpm test:browser -- browser_tests/tests/sidebar/workflows.spec.ts` multiple times - [x] Verify the 3 previously failing tests now pass consistently: - "Can overwrite other workflows with save as" - "Can rename nested workflow from opened workflow item" - "Can drop workflow from workflows sidebar" ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7280-hotfix-stabilize-flaky-workflow-sidebar-browser-tests-2c46d73d365081c5b3badfafe35a63dc) by [Unito](https://www.unito.io) --------- Co-authored-by: Terry Jia <[email protected]> Co-authored-by: Alexander Brown <[email protected]> Co-authored-by: Luke Mino-Altherr <[email protected]> Co-authored-by: Claude <[email protected]> Co-authored-by: GitHub Action <[email protected]> Co-authored-by: Benjamin Lu <[email protected]>
## Summary - Fix flaky workflow sidebar browser tests that were failing in headless mode - Add retry logic for menu hover operations in Topbar - Add proper timing/wait helpers for dialog masks and workflow service completion - Fix test isolation issues in setupWorkflowsDirectory and drop workflow test ## Test plan - [x] Run `pnpm test:browser -- browser_tests/tests/sidebar/workflows.spec.ts` multiple times - [x] Verify the 3 previously failing tests now pass consistently: - "Can overwrite other workflows with save as" - "Can rename nested workflow from opened workflow item" - "Can drop workflow from workflows sidebar" ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7280-hotfix-stabilize-flaky-workflow-sidebar-browser-tests-2c46d73d365081c5b3badfafe35a63dc) by [Unito](https://www.unito.io) --------- Co-authored-by: Terry Jia <[email protected]> Co-authored-by: Alexander Brown <[email protected]> Co-authored-by: Luke Mino-Altherr <[email protected]> Co-authored-by: Claude <[email protected]> Co-authored-by: GitHub Action <[email protected]> Co-authored-by: Benjamin Lu <[email protected]>
Summary
Test plan
pnpm test:browser -- browser_tests/tests/sidebar/workflows.spec.tsmultiple times┆Issue is synchronized with this Notion page by Unito