fix: use GitHub App token to trigger e2e tests after lint/i18n commits - #8464
Closed
DrJKL wants to merge 22 commits into
Closed
fix: use GitHub App token to trigger e2e tests after lint/i18n commits#8464DrJKL wants to merge 22 commits into
DrJKL wants to merge 22 commits into
Conversation
Amp-Thread-ID: https://ampcode.com/threads/T-019c0c25-c0e9-71c2-ac29-b80252d9fb6f Co-authored-by: Amp <amp@ampcode.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! Comment |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/30/2026, 07:32:51 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests:
|
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 26 kB (baseline 26 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 974 kB (baseline 974 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 471 kB (baseline 471 kB) • 🟢 -8 BConfiguration panels, inspectors, and settings screens
Status: 12 added / 12 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.89 kB (baseline 2.89 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.7 kB (baseline 33.7 kB) • ⚪ 0 BReusable component library chunks
Status: 4 added / 4 removed Data & Services — 2.71 MB (baseline 2.71 MB) • 🔴 +1 BStores, services, APIs, and repositories
Status: 8 added / 8 removed Utilities & Hooks — 25.3 kB (baseline 25.3 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 7 added / 7 removed Vendor & Third-Party — 10.7 MB (baseline 10.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.1 MB (baseline 7.1 MB) • 🟢 -198 BBundles that do not match a named category
Status: 34 added / 34 removed |
Contributor
|
Amazing idea! (this should work, and currently CI have scopes problem need to fix..) Ref:Token differences: When to Use Which Use the GITHUB_TOKEN by default when your workflow only needs to interact with resources within the same repository (e.g., creating an issue, commenting on a pull request, or pushing code). Use a GitHub App token when you need your workflow to access resources in different repositories, perform actions that the default token is not allowed to do (like triggering another workflow), or build a long-lived, non-user-dependent integration. You can create a GitHub App on the GitHub Developer settings page and use an action like the tibdex/github-app-token action to generate the token within your workflow. |
## Summary Was enabled by default for 1.38 during nightly, but should be reverted back now. In the future we can now just use `isNightly` flag for this (ref: #8149) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8444-make-new-queue-panel-disabled-by-default-2f76d73d365081139b54f76d9325101d) by [Unito](https://www.unito.io)
The Help Center popover wasn't working in linear mode because the `#graph-canvas-container` is hidden. This is fixed by instead setting the target to body. This matches the [default behaviour used by portals in rekai-ui](https://github.com/unovue/reka-ui/blob/v2/packages/core/src/Teleport/Teleport.vue) <img width="476" height="677" alt="image" src="https://github.com/user-attachments/assets/cca46648-3bce-4b72-a5be-3727e2358217" /> I've got a working branch for moving the Help Center to our reka-ui Popover component, but that'll require a much larger surface area of code changes. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8438-Fix-Help-Center-display-in-linear-mode-2f76d73d36508112abedf1160f7c3a90) by [Unito](https://www.unito.io)
…8433) ## Summary Refactors the model assets cache in `assetsStore.ts` to be keyed by category (e.g., 'checkpoints', 'loras') instead of nodeType (e.g., 'CheckpointLoaderSimple'). ## Changes - Rename `modelStateByKey` to `modelStateByCategory` - Add `resolveCategory()` helper to translate nodeType to category for cache lookup - Multiple node types sharing the same category now share one cache entry - Add `invalidateCategory()` method for cache invalidation - Maintain backwards-compatible public API accepting nodeType - Update tests for new category-keyed behavior ## Benefits 1. **Deduplication**: Same category = same cache entry = single API call 2. **Simple invalidation**: Delete asset with tag 'checkpoints' then invalidate cache 3. **Cleaner mental model**: Store to View reactive flow works naturally ## Testing - All existing tests pass with updates - Added new tests for category-keyed cache sharing, invalidateCategory, and unknown node type handling ## Part of Stack This is **PR 1 of 2** in a stacked PR series: 1. **This PR**: Refactor asset cache to category-keyed (architectural improvement) 2. **[PR 2 #8434](#8434: Fix deletion invalidation ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8433-refactor-change-asset-cache-from-nodeType-keyed-to-category-keyed-2f76d73d365081999b7fda12c9706ab5) by [Unito](https://www.unito.io) --------- Co-authored-by: Subagent 5 <subagent@example.com> Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: GitHub Action <action@github.com>
Patch version increment to 1.39.2 **Base branch:** `main` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8447-1-39-2-2f86d73d3650819e8daccc9c5fbc3a3b) by [Unito](https://www.unito.io) Co-authored-by: christian-byrne <72887196+christian-byrne@users.noreply.github.com>
…8415) ## Summary Shows tab-specific empty state messages in Node Manager instead of generic "No search results found" message. ## Changes - Added computed properties to determine empty state messages based on current tab and search state - Display tab-specific messages when a tab is empty without active search (e.g., "No Missing Nodes" for Missing tab) - Fall back to search-related messages only when there's an active search query - Added Korean translations for empty state messages | Tab | Empty State Title | |-----|-------------------| | All Installed | No Extensions Installed | | Update Available | All Up to Date | | Conflicting | No Conflicts Detected | | Workflow | No Extensions in Workflow | | Missing | No Missing Nodes | ## Review Focus - Verify i18n key structure matches existing patterns ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8415-feat-Show-context-appropriate-empty-state-messages-in-Manager-tabs-2f76d73d3650817ab8a0d41b45df3411) by [Unito](https://www.unito.io)
#8187) ## Summary When removing a SubgraphNode via `LGraph.remove()`: - Fire `onRemoved` for all nodes inside the subgraph - Fire `onNodeRemoved` callback on the subgraph for each inner node - Remove subgraph definition from `rootGraph.subgraphs` when no other nodes reference it (checks both root graph nodes and nodes inside other subgraphs) This ensures proper cleanup of subgraph definitions and lifecycle callbacks for nested nodes when subgraph nodes are deleted. ## Changes ### LGraph.ts Added SubgraphNode-specific cleanup in `remove()` method that: 1. Iterates inner nodes and fires their `onRemoved` callbacks 2. Fires `onNodeRemoved` on the subgraph for downstream listeners (e.g., minimap) 3. Garbage collects the subgraph definition when no other nodes reference it ### SubgraphNode.ts Fixed `graph` property to match `LGraphNode` lifecycle contract. Previously it was declared as `override readonly graph` via constructor parameter promotion, which prevented `LGraph.remove()` from setting `node.graph = null`. Changed to a regular mutable property with null guard in `rootGraph` getter. ### LGraph.test.ts Added 4 tests: - `removing SubgraphNode fires onRemoved for inner nodes` - `removing SubgraphNode fires onNodeRemoved callback` - `subgraph definition is removed when last referencing node is removed` - `subgraph definition is retained when other nodes still reference it` ## Related - Fixes #8145 - Part of the subgraph lifecycle cleanup plan (Slice 2: Definition garbage collection)
## Summary Fix shift+click range selection not properly deselecting assets when selecting a smaller range, and improve selection performance. ## Changes - **Bug Fix**: Shift+click now replaces selection with the new range instead of combining with existing selection - **Performance**: Remove unnecessary `.every()` check in `setSelection` (O(n) → O(1)) - **Tests**: Add 23 unit tests for asset selection logic ## Test Plan - [x] Click 1st asset → only 1st selected - [x] Shift+click 3rd asset → items 1-3 selected - [x] Shift+click 1st asset → only 1st selected (was broken, now fixed) - [x] All 23 new unit tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8396-bugfix-Fix-shift-click-deselection-in-asset-panel-2f76d73d3650814ca060d1e6a40cf6d4) by [Unito](https://www.unito.io)
## Summary See Title. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8450-Fix-Hide-Jobs-in-Assets-Panel-when-Queue-V2-is-disabled-2f86d73d3650810c8155c1fea92fc0aa) by [Unito](https://www.unito.io)
The extra `linf` check was made in 878c8c0. I'm no longer able to replicate the cloning bug the check was introduced for. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8442-Fix-paste-with-links-breaking-autogrow-connections-2f76d73d3650817aa99cc3b9e4e6412c) by [Unito](https://www.unito.io)
Previously the save keybind action would - apply the new keybind - wait for a network request to persist the change - close the dialogue regardless of the results of the above changes During this network request, the dialog would show a warning that the keybind is invalid because the dialogue "contains a modified keybind which conflicts with an existing keybind" <img width="506" height="261" alt="image" src="https://github.com/user-attachments/assets/e46150ce-9349-4f8e-b3b5-fb0b20dd3db9" /> This PR changes the order these actions are applied in. - The dialogue is immediately closed - The keybinding is updated if valid - The keybinding is persisted. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8435-Fix-invalid-keybind-flash-2f76d73d3650815c9657f35e77d331fe) by [Unito](https://www.unito.io)
## Summary Adds support for creating Chatterbox TTS nodes when clicking Chatterbox models in the Cloud asset browser. ## Changes ### modelToNodeStore.ts - Add `findProvidersWithFallback()` helper for hierarchical model type lookups (e.g., `parent/child` falls back to `parent`) - Register 4 Chatterbox model directories with empty widget keys: - `chatterbox/chatterbox` → `FL_ChatterboxTTS` - `chatterbox/chatterbox_turbo` → `FL_ChatterboxTurboTTS` - `chatterbox/chatterbox_multilingual` → `FL_ChatterboxMultilingualTTS` - `chatterbox/chatterbox_vc` → `FL_ChatterboxVC` ### createModelNodeFromAsset.ts - Skip widget assignment when `provider.key` is empty (for nodes that auto-load models without a widget selector) ### Tests - Add tests for hierarchical fallback behavior - Add tests for empty widget key (auto-load nodes) - Add Chatterbox node types to mock data ## Notes - Empty `key` convention: Chatterbox nodes auto-load their models and don't have a model selector widget, so we register them with `key: ''` and skip the widget assignment step - Hierarchical fallback only goes one level deep (`a/b/c` → `a`, not `a/b`) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8418-feat-add-Chatterbox-model-support-for-Cloud-asset-browser-2f76d73d365081be822bc369b155f099) by [Unito](https://www.unito.io) --------- Co-authored-by: Subagent 5 <subagent@example.com> Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: Alexander Brown <drjkl@comfy.org> Co-authored-by: GitHub Action <action@github.com>
, domains) (#8441) ## Summary Adds structured test tags to all 54 Playwright test files to enable flexible test filtering during development and CI. ## Tags Added | Tag | Count | Purpose | |-----|-------|---------| | `@screenshot` | 32 files | Tests with visual assertions (`toHaveScreenshot`) | | `@smoke` | 5 files | Quick essential tests for fast validation | | `@slow` | 5 files | Long-running tests (templates, subgraph, featureFlags) | | `@canvas` | 15 files | Canvas/graph rendering tests | | `@node` | 10 files | Node behavior tests | | `@ui` | 8 files | UI component tests | | `@widget` | 5 files | Widget-specific tests | | `@workflow` | 3 files | Workflow operations | | `@subgraph` | 1 file | Subgraph functionality | | `@keyboard` | 2 files | Keyboard shortcuts | | `@settings` | 2 files | Settings/preferences | ## Usage Examples ```bash # Quick validation (~16 tests, ~30s) pnpm test:browser -- --grep @smoke # Skip slow tests for faster CI feedback pnpm test:browser -- --grep-invert @slow # Skip visual tests (useful for local development without snapshots) pnpm test:browser -- --grep-invert @screenshot # Run only canvas-related tests pnpm test:browser -- --grep @canvas # Combine filters pnpm test:browser -- --grep @smoke --grep-invert @screenshot ``` ## Implementation Details - Uses Playwright's native tag syntax: `test.describe('Name', { tag: '@tag' }, ...)` - Tags inherit from describe blocks to child tests - Preserves existing project-level tags: `@mobile`, `@2x`, `@0.5x` - Multiple tags supported: `{ tag: ['@screenshot', '@smoke'] }` ## Test Plan - [x] All existing tests pass unchanged - [x] Tag filtering works with `--grep` and `--grep-invert` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8441-test-add-Playwright-test-tags-for-filtering-smoke-slow-screenshot-domains-2f76d73d36508184990ec859c8fd7629) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: GitHub Action <action@github.com>
…xy compatibility (#8440) ## Summary Migrates ECMAScript private fields (`#`) to TypeScript private (`private`) across LiteGraph to fix Vue Proxy reactivity incompatibility. ## Problem ES private fields (`#field`) are incompatible with Vue's Proxy-based reactivity system - accessing `#field` through a Proxy throws `TypeError: Cannot read private member from an object whose class did not declare it`. ## Solution - Converted all `#field` to `private _field` across 10 phases - Added `toJSON()` methods to `LGraph`, `NodeSlot`, `NodeInputSlot`, and `NodeOutputSlot` to prevent circular reference errors during serialization (TypeScript private fields are visible to `JSON.stringify` unlike true ES private fields) - Made `DragAndScale.element.data` non-enumerable to break canvas circular reference chain ## Testing - All 4027 unit tests pass - Added 9 new serialization tests to catch future circular reference issues - Browser tests (undo/redo, save workflows) verified working ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8440-refactor-migrate-ES-private-fields-to-TypeScript-private-for-Vue-Proxy-compatibility-2f76d73d365081a3bd82d429a3e0fcb7) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com>
## Summary
This PR removes unsafe type assertions ("as unknown as Type") from test
files and improves type safety across the codebase.
### Key Changes
#### Type Safety Improvements
- Removed improper `as unknown as Type` patterns from 17 test files in
Group 8 part 7
- Replaced with proper TypeScript patterns using factory functions and
Mock types
- Fixed createTestingPinia usage in test files (was incorrectly using
createPinia)
- Fixed vi.hoisted pattern for mockSetDirty in viewport tests
- Fixed vi.doMock lint issues with vi.mock and vi.hoisted pattern
- Retained necessary `as unknown as` casts only for complex mock objects
where direct type assertions would fail
### Files Changed
Test files (Group 8 part 7 - services, stores, utils):
- src/services/nodeOrganizationService.test.ts
- src/services/providers/algoliaSearchProvider.test.ts
- src/services/providers/registrySearchProvider.test.ts
- src/stores/comfyRegistryStore.test.ts
- src/stores/domWidgetStore.test.ts
- src/stores/executionStore.test.ts
- src/stores/firebaseAuthStore.test.ts
- src/stores/modelToNodeStore.test.ts
- src/stores/queueStore.test.ts
- src/stores/subgraphNavigationStore.test.ts
- src/stores/subgraphNavigationStore.viewport.test.ts
- src/stores/subgraphStore.test.ts
- src/stores/systemStatsStore.test.ts
- src/stores/workspace/nodeHelpStore.test.ts
- src/utils/colorUtil.test.ts
- src/utils/executableGroupNodeChildDTO.test.ts
Source files:
- src/stores/modelStore.ts - Improved type handling
### Testing
- All TypeScript type checking passes (`pnpm typecheck`)
- All affected test files pass (`pnpm test:unit`)
- Linting passes without errors (`pnpm lint`)
- Code formatting applied (`pnpm format`)
Part of the "Road to No Explicit Any" initiative, cleaning up type
casting issues from branch `fix/remove-any-types-part8`.
### Previous PRs in this series:
- Part 2: #7401
- Part 3: #7935
- Part 4: #7970
- Part 5: #8064
- Part 6: #8083
- Part 7: #8092
- Part 8 Group 1: #8253
- Part 8 Group 2: #8258
- Part 8 Group 3: #8304
- Part 8 Group 4: #8314
- Part 8 Group 5: #8329
- Part 8 Group 6: #8344
- Part 8 Group 7: #8459 (this PR)
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-8459-Road-to-No-explicit-any-Group-8-part-7-test-files-2f86d73d36508114ad28d82e72a3a5e9)
by [Unito](https://www.unito.io)
## Summary Migrate keybindings domain to `src/platform/keybindings/` following DDD principles. ## Changes - **What**: Consolidate keybinding-related code (types, store, service, defaults, reserved keys) into a single domain module with flat structure - Extracted `KeyComboImpl` and `KeybindingImpl` classes into separate files - Updated all consumers to import from new location - Colocated tests with source files - Updated stores/README.md and services/README.md to remove migrated entries ## Review Focus - Verify all import paths were updated correctly - Check that the flat structure is appropriate (vs nested core/data/ui layers) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8369-refactor-migrate-keybindings-to-DDD-structure-2f66d73d36508120b169dc737075fb45) by [Unito](https://www.unito.io) --------- Co-authored-by: Subagent 5 <subagent@example.com> Co-authored-by: Amp <amp@ampcode.com>
## Summary Improve Template search input performance issue #8134 This was caused by the search logic running too frequently (throttled at 50ms), causing the main thread to block on every few keystrokes. ## Changes Use debouncing that wait until you stop typing for a specific time (300ms) before running. It makes the searching function more smoothly. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8343-Improve-template-search-input-performance-issue-2f56d73d36508144bdf9fa5e0cd76818) by [Unito](https://www.unito.io) --------- Co-authored-by: Alexander Brown <drjkl@comfy.org>
…#8469) ## Summary Fixes a bug where socket map data was not properly removed when sockets are dynamically added/removed via DynamicCombo widgets in Vue mode (Nodes 2.0). ## Problem When DynamicCombo widgets (e.g., `should_remesh` on Meshy nodes) change their selection, inputs are dynamically added/removed. The Vue `v-for` loop in `NodeSlots.vue` was using array index as the `:key`, causing Vue to **reuse** slot components instead of properly unmounting them. This led to: - Socket map entries leaking (never cleaned up) - Socket positions becoming desynced - Stale cached offset data ## Solution 1. **Use slot `name` as Vue key** instead of array index in `NodeSlots.vue` - Slot names are unique per node (enforced by ComfyUI backend) - When a slot is removed, Vue sees the key disappear and properly unmounts the component - `onUnmounted` cleanup in `useSlotElementTracking` now runs correctly 2. **Add defensive cleanup** in `useSlotElementTracking.ts` - Before registering a new slot, check if a stale entry exists with the same key - Clean up stale entry to handle any edge cases ## Related - Fixes COM-12970 - Related to #7837 (fixed LiteGraph version of this bug, but not Vue mode) ## Testing - Quality checks pass (typecheck, lint, format) - Manual testing with DynamicCombo nodes (Meshy, nodes_logic) recommended ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8469-fix-Vue-mode-socket-map-data-not-cleaned-up-on-dynamic-input-changes-2f86d73d365081e599eadca4f15e6b6e) by [Unito](https://www.unito.io) --------- Co-authored-by: Subagent 5 <subagent@example.com> Co-authored-by: Amp <amp@ampcode.com>
## Summary - Fix `[object Object]` display in error report logs section ## Changes - Add runtime type check for `serverLogs` in error report template - JSON stringify object logs with proper formatting ## Test plan - [x] Verify string logs still display correctly - [x] Verify object logs are properly stringified instead of showing `[object Object]` Fixes #8463 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8460-fix-handle-non-string-serverLogs-in-error-report-2f86d73d36508179af5afcdeec025a75) by [Unito](https://www.unito.io) Co-authored-by: Subagent 5 <subagent@example.com>
## Summary Add documentation comment explaining the Vite preload error handler with a link to official documentation. ## Changes - **What**: Added a 2-line comment above the `vite:preloadError` event listener in App.vue explaining its purpose and linking to https://vite.dev/guide/build#load-error-handling Fixes COM-14132 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8475-docs-add-Vite-preload-error-handling-documentation-comment-2f86d73d3650815f9731f30bd9c5eb57) by [Unito](https://www.unito.io) Co-authored-by: Amp <amp@ampcode.com>
Replace GitHub App token generation with PR_GH_TOKEN to match the pattern used consistently across all other workflows in the repository. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use GitHub App token instead of GITHUB_TOKEN so that commits pushed by lint and i18n workflows trigger downstream e2e tests.
┆Issue is synchronized with this Notion page by Unito