[integration][do-not-merge] M1 FE asset stack for BE integration testing#12411
Draft
dante01yoon wants to merge 47 commits into
Draft
[integration][do-not-merge] M1 FE asset stack for BE integration testing#12411dante01yoon wants to merge 47 commits into
dante01yoon wants to merge 47 commits into
Conversation
…ureFlags test util L1 prerequisite cleanup. Two type-preserving refactors that make subsequent L1 sub-issues mechanical. 1. Extract getAssetStoredFilename(asset) helper to collapse the duplicated `isCloud && asset.asset_hash ? asset.asset_hash : asset.name` branch from useMediaAssetActions (lines 302, 446) into a single helper. Once BE-933/934 emit file_path and the cloud spec sync brings it into generated types, only the helper changes. 2. Add mockFeatureFlags(overrides) test utility under src/test-utils/. L1 sub-issues will use this consistently instead of re-inventing the mock shape each time. Includes a FeatureFlags type export from useFeatureFlags.ts so the mock stays in sync. Also auto-fixed unrelated tailwind class-order lint errors in four files (VirtualGrid, RightSidePanel, Textarea, ModelInfoPanel) to keep CI green. No behavior change.
Collapse the OSS vs Cloud branching in the mask editor and painter so both runtimes use the same upload contract: POST to /upload/image with type "input" and no subfolder, then reference the result by filename only. - maskeditor: replace separate uploadMask + uploadImage helpers with a single uploadLayer; drop the /upload/mask call, the original_ref form field, and the clipspace subfolder - painter: drop the isCloud branch on type/subfolder/result string; always upload as type "input" with no subfolder - update painter test to match the unified result shape
Both uploadLayer (maskeditor) and the painter's serialize path previously fell through to the pre-upload layer.ref when the server returned 200 with no name or with a non-JSON body, logging only a console.warn. The caller then assigned the stale ref onto the saved widget value as if the upload had succeeded, producing graphs that pointed at files that were never written. - maskeditor: rewrite uploadLayer to throw on JSON parse failure and on missing data.name (was: console.warn + return layer.ref) - painter: add a missing-name guard alongside the existing JSON parse guard, surfacing the same toast/error path the JSON failure already uses - painter test: assert the upload FormData carries type=input with no subfolder, and add coverage for the two new error branches
After the upload-contract unification, the mask editor saves all four layers through /upload/image. The Playwright route interceptors for /upload/mask in the save-success and save-failure tests no longer fire. - save-success: remove the mask interceptor, tighten the count assertion from "> 0" to "=== 4" (one upload per layer) - save-failure: drop the unused mask interceptor; the image-route 500 is sufficient to keep the dialog open
Replace the hand-rolled { name?: string ... } shapes in the painter
and mask editor with UploadImageResponse from the codegen'd
ingest-types package. The runtime shape is unchanged - the type now
flows from the spec via the existing codegen pipeline instead of
being declared inline locally.
The runtime check for data.name remains: the spec marks the 200
response properties as optional, so the imported type still has
name?: string. The check enforces what the server actually returns
in practice; tightening the spec is a separate follow-up.
FE-729 (L1.2 — L1 central). Asset API will be always available post-BE-786 (`--enable-assets` removed from OSS). Delete the gating function, the user-facing setting, the experimental warning, and the toggle command. Simplify all caller sites to assume the asset API is available. Changes: - Delete isAssetAPIEnabled() from assetService.ts; simplify shouldUseAssetBrowser() to call isAssetBrowserEligible() directly. - Remove the EXPERIMENTAL_WARNING prefix from four asset-load error messages. - Drop the Comfy.Assets.UseAssetAPI setting from coreSettings.ts and its schema entry in apiSchema.ts. - Remove the Comfy.ToggleAssetAPI command outright. Keep Comfy.BrowseModelAssets but drop its setting-check / setting-set preamble and the "Experimental:" label prefix; the command now just opens the asset browser dialog. - modelStore: drop the useAssetAPI fork in createGetModelsFunc and loadModelFolders; always route through assetService. - sidebarTabStore: model-library tab click always routes through Comfy.BrowseModelAssets (no more setting check). - assetPreviewUtil.isAssetPreviewSupported() returns true unconditionally; caller-site simplification deferred. - WidgetSelect.vue: simplify isAssetMode to drop the isAssetAPIEnabled() arm of the disjunction. - Test mocks/expectations updated. Blocked-merge by BE-786 (OSS `--enable-assets` removal). PR opened as draft. Also auto-fixed unrelated tailwind class-order lint errors in four files (VirtualGrid, RightSidePanel, Textarea, ModelInfoPanel, WidgetSelectDefault) to keep CI green.
… callers
Follow-up to the previous FE-729 commit. After deleting
isAssetAPIEnabled, isAssetPreviewSupported() became a wrapper that
always returned true. Remove the function and simplify all callers.
Changes:
- Delete isAssetPreviewSupported() from assetPreviewUtil.ts.
- Media3DTop.vue: drop the isAssetPreviewSupported() arm of the
loadThumbnail guard (asset.name check is still required).
- saveMesh.ts: unwrap two `if (isAssetPreviewSupported()) { ... }`
blocks in applySaveGLBOutput and the SaveGLB beforeRegisterNodeDef
extension callback.
- FormDropdownMenuItem.vue: drop the early return from
resolveMeshPreview.
- useLoad3d.ts: drop the isAssetPreviewSupported() arm of the
modelReady guard.
- Tests: remove the dead "asset preview is unsupported" branches
(useLoad3d, Media3DTop, FormDropdownMenuItem) and clean up the
associated mocks and hoisted state.
Auto-fixed unrelated tailwind class-order lint errors in five files
(VirtualGrid, RightSidePanel, Textarea, ModelInfoPanel,
WidgetSelectDefault) to keep CI green.
Asset cards now render image dimensions from the asset response's metadata field (`asset.metadata.width` / `asset.metadata.height`) when available, falling back to the locally-computed naturalWidth / naturalHeight from the rendered <img> tag when not. This unifies the dimension display across runtimes that serve the original file vs. runtimes that serve a downscaled preview — the rendered <img> on the latter reports the preview size, not the source asset's size, so a metadata-first read is what makes the displayed label match the source on both runtimes. No behavior change in the absence of metadata: cards continue to display the locally-computed dimensions as they do today. Drops the now-unused isCloud import; the conditional it guarded collapses into the unified read-then-fallback path.
…ming Tightens the metadata-dimension read added in the previous commit based on review feedback: - Adds isValidDimension() guard: rejects NaN, Infinity, 0, negatives, and fractional values. typeof v === 'number' alone accepts all of those, any of which would render as garbage in the dimension label (e.g. "NaNxNaN", "0x0", "1024.5x768"). - Renames originalImageDimensions to displayImageDimensions. The fallback branch may return preview-sized values from the rendered <img>'s naturalWidth/Height — calling that "original" invites the next caller to wire it into a place where source-of-truth dimensions are required. - Restores meaning of metaInfo's existing truthy check by allowing displayImageDimensions to return undefined when neither path produces usable values (e.g. invalid metadata + image not yet loaded). Previously the truthy check was dead code because the computed always returned something. No behavior change in the happy path (valid metadata, valid local).
Move isValidDimension + the metadata-vs-fallback logic into getAssetMetadataDimensions in assetMetadataUtils, leaving the SFC with a one-line computed. Switches the block comments to JSDoc on the helper (per review feedback) and adds unit coverage for the validation cases — addresses codecov/patch failure on the previously SFC-internal logic.
Temporary unblock for FE-729 e2e while BE-786 is in flight. FE-729 removes the legacy /api/models fallback in modelStore, so the assets endpoint must be reachable in CI. BE-786 will make assets always-on in OSS core; once that ships in the CI ComfyUI image, this flag (and this commit) MUST be reverted before merging FE-729 to main.
After FE-729 the model-library sidebar tab routes to the Asset Browser dialog and never renders ModelLibrarySidebarTab.vue. The legacy sidebar-tree tests now validate dead code paths: - delete browser_tests/tests/sidebar/modelLibrary.spec.ts entirely (tab/folders/search/refresh/empty-state — all dead) - drop the model-library entry from the defaultKeybindings sidebar toggle table (KeyM no longer toggles a panel) - remove the Refresh-clears-resolved-missing-model case from errorsTabMissingModels.spec.ts — it waited for the legacy /experiment/models endpoint that modelStore no longer hits Dialog-flow coverage for the new entry point belongs to FE-732.
… fetcher FE-730 (L1.3). Both Cloud and OSS will serve /api/assets post-BE-786, so assetsStore collapses to a single asset-API code path. - Drop fetchInputFilesFromAPI (legacy /files/input caller); rename fetchInputFilesFromCloud to fetchInputFiles. - Remove the isCloud ternary on the input fetcher swap. - Unwrap getModelState()'s if(isCloud) body and delete its OSS no-op return object; the model pagination subsystem now runs unconditionally. - Drop now-dead isCloud + mapInputFileToAssetItem imports. - Delete mapInputFileToAssetItem + stripDirectoryAnnotation from assetMappers.ts (no remaining callers in src/); delete its test file. - Simplify assetsStore.test.ts: hardcode the distribution mock to isCloud: true, drop the mockIsCloud toggle helper and the dead mapper mock, rename the now-unconditional "(Cloud)" describe blocks. Blocked-merge by BE-786 (OSS --enable-assets removal) + FE-729 (isAssetAPIEnabled deletion). Opened as a Draft on the FE-729 stack. Auto-fixed unrelated tailwind class-order lint errors in five files via pnpm lint:fix to keep CI green.
FE-729 routes model widgets (ckpt_name, lora_name, etc.) through WidgetSelectDropdown on OSS as well as Cloud. That dropdown's trigger button has no aria-label — the widget name lives in a sibling label div ([data-testid="widget-layout-field-label"]). Grid-mode widgets (WidgetSelectDefault) still carry aria-label on the input directly. Update the helper to .or() the two strategies so existing aria-label lookups keep working while asset-mode dropdowns are reached via the visible label text + sibling trigger button.
- useComboWidget: drop the `if (isCloud)` wrap so shouldUseAssetBrowser and NODE_MEDIA_TYPE_MAP paths run unconditionally. Rename resolveCloudDefault -> resolveAssetDefault. - useAssetWidgetData: drop the `if (isCloud)` branch and its empty-data fallback; remove the "Cloud-only" docstring. - Tests: drop the isCloud mock + per-test toggles. Delete the isCloud=false desktop spec and the OSS LoadImage fall-through case that asserted the now-removed branch.
…Widget Previous .or() implementation acted as a union — both branches resolved for number widgets (e.g. 'seed' has aria-label on wrapper + a Decrement button under widget-layout-field-label), causing Playwright strict mode violations on tests that previously passed. Switch to a count-based check: prefer the aria-label match when it exists, otherwise fall back to the dropdown-trigger button under the visible label row. This restores grid/number widget behaviour while keeping asset-mode dropdown support.
…746) - Add `file_path` to AssetItem schema (nullable per BE-933/BE-934 wire shape) - Rewrite `getAssetDetectionNames`: use `file_path` alone when emitted, fall back to the legacy `asset_hash` / `name` / `subfolder + name` union when null (hash-only Core registrations, tagless Cloud rows, legacy data) - Drop `isCloud` parameter from `scanAllMediaCandidates`, `scanNodeMediaCandidates`, and `MediaVerificationOptions` — both backends consult the same asset listing oracle post-BE-933/934 - Remove the OSS synchronous-shortcut branch; all candidates now resolve asynchronously through `verifyMediaCandidates` against the unified listing - Rename `MediaVerificationOptions.isCloud` to `allowCompactSuffix` (N1-deferred annotation grammar flag, no longer a backend identity field) - Always call `assetService.getInputAssetsIncludingPublic`; retain internal `isCloud` only for the generated-assets oracle (Cloud /api/assets vs OSS history) — unifying that path is tracked separately - Update callers in app.ts, useErrorClearingHooks.ts, markDeletedAssetsAsMissingMedia.ts - Update tests with module-level `isCloud` mock for generated-asset oracle control; add `file_path`-primary matching test case Per RFC BE-808 v2 (Asset Identity Semantics). DO NOT MERGE until BE-933 (Comfy-Org/ComfyUI#14005) and BE-934 (Comfy-Org/cloud#3744) merge and ship — both backends must emit `file_path` for the new path to fire. Fixes FE-746
…, FE-748) FE-748: drop the isCloud branch in getMediaDisplayName. Resolve through the shared getAssetDisplayFilename helper so the missingMedia surface uses the same fallback chain (user_metadata.filename -> metadata.filename -> display_name -> asset.name) as the asset card and browser surfaces. FE-747: align assetMetadataUtils helper docstrings with the unified display_name shape emitted by both Core and Cloud per the BE-808 Asset Identity RFC. Add fixture tests covering the Cloud (hash + display_name) and OSS (filename + nullable display_name) response shapes.
…DisplayName Adds 4 cases against the in-file consumer of getMediaDisplayName that was previously untested: empty-on-missing-widget, Cloud-hash combo values mapped through the shared helper chain (display_name and metadata.filename), OSS filename pass-through, and the candidate-name filter. Caught by the FE-747/748 PR review pass.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughRemoves cloud/setting gating, unifies uploads to /upload/image and upload-response parsing, makes thumbnail persistence unconditional, defers missing-media detection to verification (allowCompactSuffix), adds asset filename/dimension helpers, refactors stores/composables/components, updates CI start flag, and updates/adds many tests. ChangesUnified asset & media pipeline
Estimated code review effort: Possibly related PRs:
Suggested labels: Suggested reviewers:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ❌ 1367 passed, 130 failed · 1 flaky❌ Failed Tests📊 Browser Reports
|
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #12411 +/- ##
===========================================
- Coverage 75.07% 60.37% -14.70%
===========================================
Files 1526 1417 -109
Lines 96710 72519 -24191
Branches 28279 19250 -9029
===========================================
- Hits 72602 43785 -28817
- Misses 23214 28260 +5046
+ Partials 894 474 -420
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1011 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
src/stores/workspace/sidebarTabStore.ts (1)
78-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid silent no-op when browse command is unavailable.
At Line 82, the early
returnruns even whenComfy.BrowseModelAssetsis not
registered, somodel-librarywon’t toggle at all in that case. Add a fallback
totoggleSidebarTab(tab.id).💡 Suggested change
- if (tab.id === 'model-library') { - await commandStore.commands - .find((cmd) => cmd.id === 'Comfy.BrowseModelAssets') - ?.function?.() - return - } + if (tab.id === 'model-library') { + const browseModelAssetsCommand = commandStore.commands.find( + (cmd) => cmd.id === 'Comfy.BrowseModelAssets' + ) + if (browseModelAssetsCommand?.function) { + await browseModelAssetsCommand.function() + return + } + } toggleSidebarTab(tab.id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/workspace/sidebarTabStore.ts` around lines 78 - 83, The current branch for tab.id === 'model-library' returns unconditionally, causing a silent no-op when the Comfy.BrowseModelAssets command is not registered; update the logic in the sidebarTabStore handler that checks commandStore.commands for id 'Comfy.BrowseModelAssets' so that you only return after successfully invoking its .function(), and otherwise call toggleSidebarTab(tab.id) as a fallback (i.e., if .find(...) or .function is undefined, invoke toggleSidebarTab with the tab id).
🧹 Nitpick comments (9)
src/platform/assets/services/assetService.ts (1)
230-232: ⚡ Quick winKeep Zod diagnostics in logs, not thrown user messages.
These thrown errors include full schema diagnostics. Prefer logging
fromZodError(...)and throwing a localized, user-friendly message instead.Proposed change
function validateAssetResponse(data: unknown): AssetResponse { const result = assetResponseSchema.safeParse(data) if (result.success) return result.data - const error = fromZodError(result.error) - throw new Error(`Invalid asset response against zod schema:\n${error}`) + console.error('Invalid asset response:', fromZodError(result.error)) + throw new Error( + st( + 'assetBrowser.errorInvalidAssetResponse', + 'Received an invalid asset response. Please try again.' + ) + ) } @@ - const error = result.error - ? fromZodError(result.error) - : 'Unknown validation error' - throw new Error(`Invalid asset response against zod schema:\n${error}`) + console.error( + 'Invalid asset details response:', + result.error ? fromZodError(result.error) : 'Unknown validation error' + ) + throw new Error( + st( + 'assetBrowser.errorInvalidAssetResponse', + 'Received an invalid asset response. Please try again.' + ) + )Based on learnings: "In asset service files under src/platform/assets/services, prefer using schema.safeParse()... convert the error with fromZodError(result.error) for logging while throwing user-friendly errors that do not reveal internal schema details."
Also applies to: 447-450
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/assets/services/assetService.ts` around lines 230 - 232, Replace throwing the full Zod diagnostics created by fromZodError(result.error) with logging the diagnostics and throwing a user-friendly message: call your logger (e.g., processLogger.error or existing logger) with fromZodError(result.error) to record full schema details, then throw a generic/localized Error like "Asset response validation failed" (do this in the block that currently uses fromZodError(result.error)). Apply the same change to the other occurrence around the second validation (the block referencing fromZodError at the later location).src/platform/missingMedia/missingMediaAssetResolver.test.ts (1)
23-27: ⚡ Quick winRemove redundant mock-explanation comments.
The surrounding test naming already communicates intent; these comments are extra upkeep without added signal.
As per coding guidelines: `**/*.{ts,tsx,vue,js,jsx}`: "Avoid new usage of code comments; do not add or retain redundant comments".♻️ Proposed cleanup
-// Mutable holder so each test can flip the runtime `isCloud` to drive the -// resolver's generated-assets oracle selection (Cloud /api/assets vs OSS -// job history). The named-import binding into the resolver re-reads the -// getter on each access (ESM live binding semantics). const isCloudHolder = vi.hoisted(() => ({ value: false }))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/missingMedia/missingMediaAssetResolver.test.ts` around lines 23 - 27, Remove the redundant explanatory comment block above the test helper and leave only the code; specifically delete the multi-line comment describing the mutable holder and ESM live binding semantics that precedes isCloudHolder, so keep the const isCloudHolder = vi.hoisted(() => ({ value: false })) line but remove the surrounding comment text.src/platform/missingMedia/missingMediaScan.test.ts (2)
864-870: ⚡ Quick winDrop the redundant "no pending candidates" test case.
This scenario is already covered by the prior "already resolved as true" test and does not add new behavioral coverage.
As per coding guidelines: `src/**/*.test.ts`: "Do not write redundant tests; follow composable testing patterns".♻️ Proposed cleanup
- it('skips entirely when no pending candidates', async () => { - const candidates = [makeCandidate('1', missingHash, { isMissing: true })] - - await verifyMediaCandidates(candidates, { allowCompactSuffix: true }) - - expect(mockGetInputAssetsIncludingPublic).not.toHaveBeenCalled() - })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/missingMedia/missingMediaScan.test.ts` around lines 864 - 870, Remove the redundant test case 'skips entirely when no pending candidates' in missingMediaScan.test.ts because its behavior is already covered by the earlier "already resolved as true" test; delete the it(...) block that constructs candidates with makeCandidate('1', missingHash, { isMissing: true }) and calls verifyMediaCandidates(...), and ensure any expectations against mockGetInputAssetsIncludingPublic in that block are removed so the suite relies on the existing test covering the same behavior.
31-34: ⚡ Quick winRemove redundant inline comments in test bodies.
These comments duplicate intent already captured by test names/assertions and add maintenance noise.
As per coding guidelines: `**/*.{ts,tsx,vue,js,jsx}`: "Avoid new usage of code comments; do not add or retain redundant comments".♻️ Proposed cleanup
-// Mutable runtime `isCloud` holder for tests that exercise the default -// resolver's generated-assets oracle (Cloud /api/assets vs OSS history). -// Tests with their own `resolveAssetSources` mock can ignore this. const isCloudHolder = vi.hoisted(() => ({ value: false })) @@ - // Legacy `name` and `asset_hash` deliberately diverge from the - // widget value; `file_path` is the sole reason the match succeeds. name: 'unrelated.png',Also applies to: 492-494
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/missingMedia/missingMediaScan.test.ts` around lines 31 - 34, Remove the redundant inline comments in test bodies that duplicate test names/assertions: delete the comment block above the isCloudHolder declaration (the mutable runtime `isCloud` holder) and remove the similar inline comments around the tests referenced near the bottom (the ones at 492-494) while leaving the isCloudHolder constant and the tests themselves unchanged so behavior/assertions remain intact.browser_tests/fixtures/helpers/BuilderSelectHelper.ts (1)
123-127: ⚡ Quick winRemove newly added implementation comment block
Please encode this intent in selector naming/helpers instead of inline comments to stay aligned with repo guidance.
As per coding guidelines, "Avoid new usage of code comments; do not add or retain redundant comments."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@browser_tests/fixtures/helpers/BuilderSelectHelper.ts` around lines 123 - 127, Remove the inline implementation comment block in BuilderSelectHelper.ts and instead encode the intent in a descriptive selector/helper name; e.g., replace the explanatory comment about grid-mode vs asset-mode widgets with a clear helper or selector like clickWidgetDropdownTriggerInRow or findWidgetByLabelOrDropdown that encapsulates the branching (WidgetSelectDefault vs WidgetSelectDropdown) and uses the data-testid "widget-layout-field-label" fallback internally, so the reasoning lives in the helper name and tests call that helper rather than relying on comments.src/renderer/extensions/vueNodes/widgets/composables/useComboWidget.ts (1)
108-111: ⚡ Quick winRemove newly introduced explanatory comments in this TS module.
Please keep the intent in naming/tests and drop these new comments to match repo conventions.
As per coding guidelines: "Avoid new usage of code comments; do not add or retain redundant comments."
Also applies to: 215-217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/extensions/vueNodes/widgets/composables/useComboWidget.ts` around lines 108 - 111, Remove the newly added explanatory comment blocks in the useComboWidget module: delete the comment that begins "Resolve the default value for an asset widget. Priority: inputSpec.default..." and the other recently introduced explanatory block later in the same file, leaving only intent expressed via clear naming and tests (i.e., do not add or retain these redundant comments in the useComboWidget/composables code).src/composables/useFeatureFlags.ts (1)
46-49: ⚡ Quick winDrop the newly added doc comment block here to match TS repo conventions.
Keep the exported type, but remove this new explanatory comment.
As per coding guidelines: "Avoid new usage of code comments; do not add or retain redundant comments."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/composables/useFeatureFlags.ts` around lines 46 - 49, Remove the newly added doc comment block above the exported type returned by useFeatureFlags so the file follows TS repo conventions; keep the exported type itself and any export statements intact, locating the comment immediately above the useFeatureFlags return type declaration and delete only that explanatory comment text.src/test-utils/mockFeatureFlags.ts (2)
8-18: ⚡ Quick winRemove the new explanatory block comment and keep the helper self-documenting.
This comment block is redundant with the function name/signature and adds maintenance overhead.
As per coding guidelines, "Avoid new usage of code comments; do not add or retain redundant comments."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test-utils/mockFeatureFlags.ts` around lines 8 - 18, Remove the redundant block comment above the mockFeatureFlags helper: delete the entire explanatory comment block that documents useFeatureFlags/mockFeatureFlags so the helper remains self-documenting via its function name and signature; locate the comment immediately preceding the mockFeatureFlags function in src/test-utils/mockFeatureFlags.ts and remove it, leaving only the implementation and any minimal inline comments that add non-redundant value.
42-47: ⚡ Quick winClarify
mockFeatureFlagsfeatureFlagmock (current tests don’t use it)
src/test-utils/mockFeatureFlags.tsis not imported/called by the test suite; existing tests mockuseFeatureFlagsdirectly withflagsonly, so the currentfeatureFlag: vi.fn()(returningundefined) won’t distort current assertions.- If this helper is intended for future use, make
featureFlagreturn acomputedconsistent withuseFeatureFlags(useflagswhen the path matches, otherwise return the provideddefaultValue).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test-utils/mockFeatureFlags.ts` around lines 42 - 47, The mockFeatureFlags helper currently returns featureFlag as vi.fn() which yields undefined; change mockFeatureFlags so its featureFlag returns a Vue computed that mirrors useFeatureFlags behavior: when called with a path string, return a computed that resolves to the value in the provided flags for that path (support nested lookup), and when a defaultValue is passed return that default when the path is missing; update references to featureFlag in the returned object so callers get a computed ref consistent with useFeatureFlags's featureFlag implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@browser_tests/fixtures/helpers/BuilderSelectHelper.ts`:
- Around line 128-137: In selectInputWidget, tighten ambiguous locators: change
byAriaLabel to use .first() (e.g., const byAriaLabel =
node.getByLabel(widgetName, { exact: true }).first()) so it always returns a
single locator, and make the fallback filter use exact text matching for the
row/button (e.g., locate the field-label row by matching the label/button text
exactly via getByText(..., { exact: true }) or a anchored RegExp like ^…$ then
.locator('..').locator('button').first()). Remove the explanatory comment block
above this logic so the code remains self-documenting; update references to
byAriaLabel and widgetLocator in the click call accordingly.
In `@src/composables/useCoreCommands.ts`:
- Line 1286: The command label is hardcoded as 'Browse Model Assets'—replace it
with a vue-i18n translation call (use the local t function from useI18n or the
i18n instance used in useCoreCommands) so the command object's label property
uses t('commands.browseModelAssets') (or equivalent) instead of the string, and
add the key "commands.browseModelAssets": "Browse Model Assets" to
src/locales/en/main.json; locate the label in the command definition inside
useCoreCommands and update the import/usage of useI18n if needed to ensure t is
available.
In `@src/stores/assetsStore.ts`:
- Around line 632-653: The functions updateAssetMetadata (and similarly
updateAssetTags) currently catch errors, perform a rollback via
updateAssetInCache, and then swallow the error; change them to rethrow the
original error (or return a typed failure) after the rollback so callers can
handle/display the failure. Concretely, inside the catch block for
updateAssetMetadata (the assetService.updateAsset call and its
updateAssetInCache rollback) preserve the caught error and throw it (or return a
standardized error result) after restoring originalMetadata; apply the same
pattern to updateAssetTags where assetService.updateAssetTags and its rollback
via updateAssetInCache are handled.
---
Outside diff comments:
In `@src/stores/workspace/sidebarTabStore.ts`:
- Around line 78-83: The current branch for tab.id === 'model-library' returns
unconditionally, causing a silent no-op when the Comfy.BrowseModelAssets command
is not registered; update the logic in the sidebarTabStore handler that checks
commandStore.commands for id 'Comfy.BrowseModelAssets' so that you only return
after successfully invoking its .function(), and otherwise call
toggleSidebarTab(tab.id) as a fallback (i.e., if .find(...) or .function is
undefined, invoke toggleSidebarTab with the tab id).
---
Nitpick comments:
In `@browser_tests/fixtures/helpers/BuilderSelectHelper.ts`:
- Around line 123-127: Remove the inline implementation comment block in
BuilderSelectHelper.ts and instead encode the intent in a descriptive
selector/helper name; e.g., replace the explanatory comment about grid-mode vs
asset-mode widgets with a clear helper or selector like
clickWidgetDropdownTriggerInRow or findWidgetByLabelOrDropdown that encapsulates
the branching (WidgetSelectDefault vs WidgetSelectDropdown) and uses the
data-testid "widget-layout-field-label" fallback internally, so the reasoning
lives in the helper name and tests call that helper rather than relying on
comments.
In `@src/composables/useFeatureFlags.ts`:
- Around line 46-49: Remove the newly added doc comment block above the exported
type returned by useFeatureFlags so the file follows TS repo conventions; keep
the exported type itself and any export statements intact, locating the comment
immediately above the useFeatureFlags return type declaration and delete only
that explanatory comment text.
In `@src/platform/assets/services/assetService.ts`:
- Around line 230-232: Replace throwing the full Zod diagnostics created by
fromZodError(result.error) with logging the diagnostics and throwing a
user-friendly message: call your logger (e.g., processLogger.error or existing
logger) with fromZodError(result.error) to record full schema details, then
throw a generic/localized Error like "Asset response validation failed" (do this
in the block that currently uses fromZodError(result.error)). Apply the same
change to the other occurrence around the second validation (the block
referencing fromZodError at the later location).
In `@src/platform/missingMedia/missingMediaAssetResolver.test.ts`:
- Around line 23-27: Remove the redundant explanatory comment block above the
test helper and leave only the code; specifically delete the multi-line comment
describing the mutable holder and ESM live binding semantics that precedes
isCloudHolder, so keep the const isCloudHolder = vi.hoisted(() => ({ value:
false })) line but remove the surrounding comment text.
In `@src/platform/missingMedia/missingMediaScan.test.ts`:
- Around line 864-870: Remove the redundant test case 'skips entirely when no
pending candidates' in missingMediaScan.test.ts because its behavior is already
covered by the earlier "already resolved as true" test; delete the it(...) block
that constructs candidates with makeCandidate('1', missingHash, { isMissing:
true }) and calls verifyMediaCandidates(...), and ensure any expectations
against mockGetInputAssetsIncludingPublic in that block are removed so the suite
relies on the existing test covering the same behavior.
- Around line 31-34: Remove the redundant inline comments in test bodies that
duplicate test names/assertions: delete the comment block above the
isCloudHolder declaration (the mutable runtime `isCloud` holder) and remove the
similar inline comments around the tests referenced near the bottom (the ones at
492-494) while leaving the isCloudHolder constant and the tests themselves
unchanged so behavior/assertions remain intact.
In `@src/renderer/extensions/vueNodes/widgets/composables/useComboWidget.ts`:
- Around line 108-111: Remove the newly added explanatory comment blocks in the
useComboWidget module: delete the comment that begins "Resolve the default value
for an asset widget. Priority: inputSpec.default..." and the other recently
introduced explanatory block later in the same file, leaving only intent
expressed via clear naming and tests (i.e., do not add or retain these redundant
comments in the useComboWidget/composables code).
In `@src/test-utils/mockFeatureFlags.ts`:
- Around line 8-18: Remove the redundant block comment above the
mockFeatureFlags helper: delete the entire explanatory comment block that
documents useFeatureFlags/mockFeatureFlags so the helper remains
self-documenting via its function name and signature; locate the comment
immediately preceding the mockFeatureFlags function in
src/test-utils/mockFeatureFlags.ts and remove it, leaving only the
implementation and any minimal inline comments that add non-redundant value.
- Around line 42-47: The mockFeatureFlags helper currently returns featureFlag
as vi.fn() which yields undefined; change mockFeatureFlags so its featureFlag
returns a Vue computed that mirrors useFeatureFlags behavior: when called with a
path string, return a computed that resolves to the value in the provided flags
for that path (support nested lookup), and when a defaultValue is passed return
that default when the path is missing; update references to featureFlag in the
returned object so callers get a computed ref consistent with useFeatureFlags's
featureFlag implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ac99015-b849-4d11-a4ac-dbb7ddd38426
📒 Files selected for processing (55)
.github/actions/start-comfyui-server/action.yamlbrowser_tests/fixtures/helpers/BuilderSelectHelper.tsbrowser_tests/tests/defaultKeybindings.spec.tsbrowser_tests/tests/maskEditor.spec.tsbrowser_tests/tests/propertiesPanel/errorsTabMissingModels.spec.tsbrowser_tests/tests/sidebar/modelLibrary.spec.tssrc/composables/graph/useErrorClearingHooks.test.tssrc/composables/graph/useErrorClearingHooks.tssrc/composables/maskeditor/useMaskEditorSaver.tssrc/composables/painter/usePainter.test.tssrc/composables/painter/usePainter.tssrc/composables/useCoreCommands.tssrc/composables/useFeatureFlags.tssrc/composables/useLoad3d.test.tssrc/composables/useLoad3d.tssrc/extensions/core/saveMesh.test.tssrc/extensions/core/saveMesh.tssrc/platform/assets/components/Media3DTop.test.tssrc/platform/assets/components/Media3DTop.vuesrc/platform/assets/components/MediaAssetCard.vuesrc/platform/assets/composables/media/assetMappers.test.tssrc/platform/assets/composables/media/assetMappers.tssrc/platform/assets/composables/useMediaAssetActions.tssrc/platform/assets/schemas/assetSchema.tssrc/platform/assets/services/assetService.test.tssrc/platform/assets/services/assetService.tssrc/platform/assets/utils/assetMetadataUtils.test.tssrc/platform/assets/utils/assetMetadataUtils.tssrc/platform/assets/utils/assetPreviewUtil.test.tssrc/platform/assets/utils/assetPreviewUtil.tssrc/platform/assets/utils/markDeletedAssetsAsMissingMedia.test.tssrc/platform/assets/utils/markDeletedAssetsAsMissingMedia.tssrc/platform/missingMedia/missingMediaAssetResolver.test.tssrc/platform/missingMedia/missingMediaAssetResolver.tssrc/platform/missingMedia/missingMediaScan.test.tssrc/platform/missingMedia/missingMediaScan.tssrc/platform/settings/constants/coreSettings.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetSelect.asset-mode.test.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetSelect.test.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetSelect.vuesrc/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownMenuItem.test.tssrc/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownMenuItem.vuesrc/renderer/extensions/vueNodes/widgets/composables/useAssetWidgetData.desktop.test.tssrc/renderer/extensions/vueNodes/widgets/composables/useAssetWidgetData.test.tssrc/renderer/extensions/vueNodes/widgets/composables/useAssetWidgetData.tssrc/renderer/extensions/vueNodes/widgets/composables/useComboWidget.test.tssrc/renderer/extensions/vueNodes/widgets/composables/useComboWidget.tssrc/schemas/apiSchema.tssrc/scripts/app.tssrc/stores/assetsStore.test.tssrc/stores/assetsStore.tssrc/stores/modelStore.test.tssrc/stores/modelStore.tssrc/stores/workspace/sidebarTabStore.tssrc/test-utils/mockFeatureFlags.ts
💤 Files with no reviewable changes (10)
- src/platform/assets/composables/media/assetMappers.test.ts
- src/extensions/core/saveMesh.test.ts
- src/platform/assets/utils/assetPreviewUtil.ts
- src/platform/settings/constants/coreSettings.ts
- browser_tests/tests/defaultKeybindings.spec.ts
- browser_tests/tests/sidebar/modelLibrary.spec.ts
- browser_tests/tests/propertiesPanel/errorsTabMissingModels.spec.ts
- src/schemas/apiSchema.ts
- src/renderer/extensions/vueNodes/widgets/composables/useAssetWidgetData.desktop.test.ts
- src/platform/assets/composables/media/assetMappers.ts
Comment on lines
+128
to
+137
| const byAriaLabel = node.getByLabel(widgetName, { exact: true }) | ||
| const widgetLocator = | ||
| (await byAriaLabel.count()) > 0 | ||
| ? byAriaLabel | ||
| : node | ||
| .getByTestId('widget-layout-field-label') | ||
| .filter({ hasText: widgetName }) | ||
| .locator('..') | ||
| .locator('button') | ||
| .first() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="browser_tests/fixtures/helpers/BuilderSelectHelper.ts"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "== Lines 90-170 =="
nl -ba "$FILE" | sed -n '90,170p'
echo
echo "== Extract function selectInputWidget (best-effort) =="
# Try to locate the function body without dumping huge output
rg -n "selectInputWidget" "$FILE" || true
START=$(rg -n "selectInputWidget" "$FILE" | head -n1 | cut -d: -f1 || true)
if [ -n "${START:-}" ]; then
# Print from start to next closing brace-ish region; keep bounded
END=$((START+140))
nl -ba "$FILE" | sed -n "${START},${END}p"
fiRepository: Comfy-Org/ComfyUI_frontend
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="browser_tests/fixtures/helpers/BuilderSelectHelper.ts"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "== Showing lines 110-160 =="
awk 'NR>=110 && NR<=160 {printf "%5d\t%s\n", NR, $0}' "$FILE"
echo
echo "== Finding selectInputWidget and widgetLocator usage =="
rg -n "selectInputWidget|widgetLocator" "$FILE" || true
echo
echo "== Finding any widgetLocator.click() usage =="
rg -n "widgetLocator\.click\(" "$FILE" || true
echoRepository: Comfy-Org/ComfyUI_frontend
Length of output: 2840
Fix strict-mode/ambiguity risks in selectInputWidget locator selection (and avoid extra comment)
- When
byAriaLabel.count() > 0,widgetLocatoris set tobyAriaLabelwithout narrowing to a single element; if the label matches multiple nodes,await widgetLocator.click({ force: true })can throw strict-mode errors. - The fallback
filter({ hasText: widgetName })can match the wrong “field-label” when widget names overlap; use exact text matching for the intended row/button. - Remove or refactor the explanatory comment block (123–127) to keep the code self-documenting per repo guidelines.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@browser_tests/fixtures/helpers/BuilderSelectHelper.ts` around lines 128 -
137, In selectInputWidget, tighten ambiguous locators: change byAriaLabel to use
.first() (e.g., const byAriaLabel = node.getByLabel(widgetName, { exact: true
}).first()) so it always returns a single locator, and make the fallback filter
use exact text matching for the row/button (e.g., locate the field-label row by
matching the label/button text exactly via getByText(..., { exact: true }) or a
anchored RegExp like ^…$ then .locator('..').locator('button').first()). Remove
the explanatory comment block above this logic so the code remains
self-documenting; update references to byAriaLabel and widgetLocator in the
click call accordingly.
| id: 'Comfy.BrowseModelAssets', | ||
| icon: 'pi pi-folder-open', | ||
| label: 'Experimental: Browse Model Assets', | ||
| label: 'Browse Model Assets', |
Contributor
There was a problem hiding this comment.
Localize the new command label via vue-i18n.
This command label is user-facing text and should use a translation key rather than a hardcoded string.
Proposed change
- label: 'Browse Model Assets',
+ label: () => t('commands.browseModelAssets'),As per coding guidelines: "Use vue-i18n for ALL user-facing strings, configured in src/locales/en/main.json."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/composables/useCoreCommands.ts` at line 1286, The command label is
hardcoded as 'Browse Model Assets'—replace it with a vue-i18n translation call
(use the local t function from useI18n or the i18n instance used in
useCoreCommands) so the command object's label property uses
t('commands.browseModelAssets') (or equivalent) instead of the string, and add
the key "commands.browseModelAssets": "Browse Model Assets" to
src/locales/en/main.json; locate the label in the command definition inside
useCoreCommands and update the import/usage of useI18n if needed to ensure t is
available.
Comment on lines
+632
to
+653
| async function updateAssetMetadata( | ||
| asset: AssetItem, | ||
| userMetadata: Record<string, unknown>, | ||
| cacheKey?: string | ||
| ) { | ||
| const originalMetadata = asset.user_metadata | ||
| updateAssetInCache(asset.id, { user_metadata: userMetadata }, cacheKey) | ||
|
|
||
| try { | ||
| const updatedAsset = await assetService.updateAsset(asset.id, { | ||
| user_metadata: userMetadata | ||
| }) | ||
| updateAssetInCache(asset.id, updatedAsset, cacheKey) | ||
| } catch (error) { | ||
| console.error('Failed to update asset metadata:', error) | ||
| updateAssetInCache( | ||
| asset.id, | ||
| { user_metadata: originalMetadata }, | ||
| cacheKey | ||
| ) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Propagate mutation failures to callers after rollback.
updateAssetMetadata and updateAssetTags currently log and swallow
failures. Callers can’t surface an error state/toast or trigger higher-level
recovery. After rollback/compensation, rethrow (or return a typed failure
result).
💡 Suggested change
async function updateAssetMetadata(
asset: AssetItem,
userMetadata: Record<string, unknown>,
cacheKey?: string
) {
const originalMetadata = asset.user_metadata
updateAssetInCache(asset.id, { user_metadata: userMetadata }, cacheKey)
try {
const updatedAsset = await assetService.updateAsset(asset.id, {
user_metadata: userMetadata
})
updateAssetInCache(asset.id, updatedAsset, cacheKey)
} catch (error) {
console.error('Failed to update asset metadata:', error)
updateAssetInCache(
asset.id,
{ user_metadata: originalMetadata },
cacheKey
)
+ throw error instanceof Error
+ ? error
+ : new Error('Failed to update asset metadata')
}
}
@@
async function updateAssetTags(
asset: AssetItem,
newTags: string[],
cacheKey?: string
) {
@@
} catch (error) {
console.error('Failed to update asset tags:', error)
updateAssetInCache(asset.id, { tags: originalTags }, cacheKey)
@@
}
+ throw error instanceof Error
+ ? error
+ : new Error('Failed to update asset tags')
}
}As per coding guidelines: "Implement proper error propagation".
Also applies to: 661-725
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/stores/assetsStore.ts` around lines 632 - 653, The functions
updateAssetMetadata (and similarly updateAssetTags) currently catch errors,
perform a rollback via updateAssetInCache, and then swallow the error; change
them to rethrow the original error (or return a typed failure) after the
rollback so callers can handle/display the failure. Concretely, inside the catch
block for updateAssetMetadata (the assetService.updateAsset call and its
updateAssetInCache rollback) preserve the caught error and throw it (or return a
standardized error result) after restoring originalMetadata; apply the same
pattern to updateAssetTags where assetService.updateAssetTags and its rollback
via updateAssetInCache are handled.
…ts-to-unified-file_path-based
…ase B) `getAssetDetectionNames` previously short-circuited on `asset.file_path`, returning only that key when emitted. That premise — `file_path` as the primary match key — doesn't match RFC BE-808 v2: §3 names `id` as the identity field and §4 explicitly calls `file_path` "a standalone locator/display string" emitted on a BEST EFFORT basis. Workflow widget values predate the BE-933 / BE-934 `file_path` rollout and may still be bare filenames, hashes, or annotated paths. A pre-BE workflow paired with a post-BE asset would false-negative under the early-return: the asset emits `file_path: "input/sub/photo.png"` while the widget value is still `"photo.png"`, and the previous code would mark the reference missing even though the file is present. Union `file_path`, `asset_hash`, `name`, and `subfolder + name` variants so workflows authored against any historical asset shape keep matching. Both backends round-trip `name` through the BE-792 deprecation window, so the legacy keys stay valid for the duration of the rollout. Adversarial review (Codex) flagged the false-negative; RFC re-read confirmed `id` (not `file_path`) is the identity field. Plan and docblock terminology updated to drop the misleading "canonical match key" framing. Adds a regression test for the bare-filename × file_path-emitting asset case.
…se.allSettled) `resolveMissingMediaAssetSources` previously ran the input-asset fetch (`/api/assets`) and the generated-asset fetch (Cloud `/api/assets` or OSS `/history`) under `Promise.all` with `abortSiblingsOnFailure`, so a single rejection took down both. With the FE-746 unification, the input fetch now always hits `assetService.getInputAssetsIncludingPublic` — including against backends that may legitimately fail (pre-BE-786 OSS without `/api/assets`, BE-934 partial deploys, transient network errors). Switch to `Promise.allSettled` and per-branch soft-degrade. Each oracle is independent; failure in one shouldn't cancel the other. On rejection, return an empty list from that branch; abort errors stay silent, real errors get a one-line console warning so we can spot them without swallowing the rest of the verification. Caller (`verifyMediaCandidates`) already treats an empty asset list as "nothing matches this candidate," which surfaces affected candidates as missing rather than silently dropping them via the previous toast-warn path. False-positive missing is recoverable (user sees the issue in the Errors tab); silent-fail was not. Adversarial review (Codex) flagged the cascade. RFC §4's BEST EFFORT framing for `file_path` already implies the contract has soft-fail expectations, so this aligns the resolver shape with the RFC's stated tolerance. Removes the now-unused `abortSiblingsOnFailure` helper. Replaces the abort-cascade regression test with two independence tests covering each oracle failing in isolation.
…ests (FE-746) Adds an e2e regression spec for the FE-746 behavior changes that unit tests can only assert with mocks: 1. Case B union — a workflow stored before BE-933/934 keeps its widget value as a bare filename. Once the backend starts emitting `file_path` on the same asset (a namespace-rooted locator that diverges from `name`), detection must still match via the `name` arm. The pre-correction Case A early-return would false-negative here. 2. BE-933 hash-only registration — assets registered via POST /assets/from-hash come back with `file_path: null` and `display_name: null`. The legacy `name` arm has to keep working through the BE-792 deprecation window. 3. Negative path — a listing that does not cover the widget value via any key still surfaces the missing-media overlay. Guards against an accidental "match everything" regression when the early-return was removed. 4. Soft-degrade — when `/api/assets` returns 500 (pre-BE-786 OSS without the endpoint, partial BE-934 deploys, transient errors), `Promise.allSettled` lets the verifier finish and mark the candidate missing instead of leaving `isMissing` stuck at undefined behind a silent toast. `Asset` from `@comfyorg/ingest-types` does not yet carry `file_path` / `display_name` (regen is BE-932). Spec extends the type locally so the mocked wire shape stays strongly typed. The companion workflow asset is the minimum LoadImage graph needed to fire the missing-media pipeline; widget value is a fixed sentinel filename used by every test.
# Conflicts: # src/platform/assets/utils/assetMetadataUtils.test.ts # src/platform/assets/utils/assetMetadataUtils.ts
Remove isCloud guards from the filter bar, asset browser modal, sidebar delete site, context-menu delete site, and input-asset delete throw — all five UI surfaces enumerated in FE-732 now render and execute unconditionally against the unified /api/assets surface. Drop the dead cloud-only docstring on AssetBrowserModal.overrideAssets and the dead shouldShowDeleteButton computed in AssetsSidebarTab (the latter was the last caller of the :show-delete-button pass-through). Fix the latent Vue Boolean prop-default-false trap in MediaAssetContextMenu by defaulting showDeleteButton to true via destructure default; the previous "propAllows = showDeleteButton ?? true" never fired because Vue normalizes absent Boolean props to false. The bug was masked while AssetsSidebarTab explicitly passed the prop. Drop the now-unused mediaAsset.deletingImportedFilesCloudOnly i18n key from all 12 locales. Tests: add OSS + Cloud input-asset deletion paths in useMediaAssetActions, add input-mode delete visibility coverage in MediaAssetContextMenu, add an @oss describe block in assetDeleteClearsLoadImage so both Playwright projects run the deletion flow. Refresh stale cloud-only comments in the filter and sort sidebar specs; their @cloud tags stay because /api/jobs is still cloud-only. L4 residual: filter-bar query params job_ids, asset_hash, and include_public will start being accepted by OSS once BE-886 and BE-891 merge — no FE change needed at that point. Stacked on top of FE-731. Depends on BE-786 (OSS removes --enable-assets) before this can ship to production. - Fixes FE-732
Lock the post-FE-732 contracts surfaced by the latent Boolean prop-default-true fix and the unconditional filter rendering: - MediaAssetContextMenu.test.ts — add an explicit showDeleteButton: false case so the opt-out path of the prop is asserted, complementing the input-mode default-true case from the parent commit. Reshape mountComponent to take an opts object. - MediaAssetFilterBar.test.ts — new component test verifies the filter button renders unconditionally and that the settings menu receives show-sort-options=true after the constant pin.
The pre-FE-732 sidebar logic hid the bulk Delete footer button on the Imported tab when isCloud was false. After removing the gate the button must render in both Cloud and OSS builds. Add a Playwright case that runs on the default chromium project — i.e. the OSS build — to lock the new invariant, mirroring the existing output-asset case in the same describe block.
4 tasks
…ode dropdown open (FE-732 fix) (#12465) https://github.com/user-attachments/assets/1baf8283-8170-4d50-aa22-25c05598874d Stacks on #12417 (FE-732). The isCloud guard removal in the M1 stack (FE-731/FE-732) exposed two latent regressions on the inline `FormDropdown` asset path; this PR fixes both. ## Symptoms (verified via CDP against `pr-3809.testenvs.comfy.org` cloud BE and a local `synap5e/assets-m1 --enable-assets` OSS BE) Reported by Simon in #m1-fe-integration testing against PR #12411: 1. **Model dropdown stale list** — `r` to refresh doesn't update the dropdown; the model list is fetched once on first dropdown open (`/api/assets?include_tags=models,checkpoints&limit=500&exclude_tags=missing`) and cached. Reopening shows stale data. Newly imported models don't appear until full page reload. 2. **`/api/jobs` per dropdown open** — each model dropdown open hits `/api/jobs?status=completed,failed,cancelled&limit=200&offset=0` for no reason. This is the OSS history path being triggered by the output-media refresh hook in the dropdown. ## Root cause - `useAssetWidgetData.watch(immediate:true)` is the only trigger for model fetch; once `assetsStore.hasAssetKey(...)` returns true, the watch short-circuits forever. Dropdown reopen has no refresh hook. - `WidgetSelectDropdown.handleIsOpenUpdate` calls `outputMediaAssets.refresh()` on every open regardless of widget kind. For asset-mode (model) dropdowns this is irrelevant — but the call still routes through `useAssetsApi('output')` → `assetsStore.updateHistory` → `api.getHistory` → `/jobs?status=completed,failed,cancelled` on OSS. Cloud distribution swaps the provider for `useFlatOutputAssets` which hits `/api/assets?include_tags=output` — still wasted work, just a different URL. The handler and cache guard are pre-existing (#6734, #8090), but were only reachable from cloud distributions before FE-731 unwrapped `if (isCloud)` in `useAssetWidgetData`. ## Fix - `useAssetWidgetData`: expose `refresh()` that re-invokes `assetsStore.updateModelsForNodeType` for the current node type, guarded against re-entry while a fetch is in flight. - `WidgetSelectDropdown.handleIsOpenUpdate`: branch on `props.isAssetMode` — asset-mode dropdowns refresh model assets via `assetData.refresh()`; non-asset-mode dropdowns continue to refresh `outputMediaAssets` as before (preserved cloud / OSS behavior). ## Verification **CDP — Vue Nodes 2.0 ON** Same dropdown opened twice (`qwen_image_vae` VAELoader widget): | Open # | `/api/jobs?status=...` | `/api/assets?include_tags=models,vae` | |---|---|---| | 1st | 0 | 1 (fresh fetch) | | 2nd | 0 | 1 (fresh fetch — newly added models would appear) | **Before the fix** (same setup): 1st open fired both `/api/jobs` and the model fetch; 2nd open fired only `/api/jobs` (model list stale). **Other paths preserved (verified empirically + by code)**: - OSS + non-asset-mode dropdown → `/api/jobs` still fires (existing OSS behavior). - Cloud + non-asset-mode dropdown → `/api/assets?include_tags=output` still fires (existing cloud behavior; branch in `WidgetSelectDropdown.vue` outputMediaAssets ternary is untouched). - WS-status queue/history polling (`limit=64` `/api/jobs`) still fires on page load — unrelated to dropdown. ## Test plan - [x] `pnpm vitest run src/renderer/extensions/vueNodes/widgets/composables/useAssetWidgetData.test.ts` — 10/10 (3 new tests for `refresh()`). - [x] `pnpm vitest run src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.test.ts` — 9/9 (4 new tests cover handleIsOpenUpdate branches: asset-mode skips outputMediaAssets.refresh, asset-mode reopen refetches model assets, non-asset-mode preserves outputMediaAssets.refresh, close event is no-op). - [x] `pnpm typecheck` clean. - [ ] Re-test in `m1-fe-integration` once stacked PRs merge: import a new model via cloud import flow → reopen dropdown → new model appears without page reload. Uploading Screen Recording 2026-05-27 at 12.43.13 AM.mov…
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
3 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.
Purpose
Test-only branch — do not merge. Stacks every open M1 Assets Foundation FE PR onto latest
mainso backend can run integration tests against the unified FE state without waiting for each PR to merge sequentially.Stacked PRs (current)
77514a4c83051c00254837307fac244b53d0117d01190+d8422691c(Case B + soft-degrade + e2e follow-up)4b0f6dc29d45e64197b50510f7d752452b90Base:
main. 13 merge commits total.M1 scope coverage (per Assets FE Divergence Survey)
L1 — Remove
!isCloudasset guards (10 sites)useMediaAssets.ts:14(deleted)mainassetService.ts:385assetsStore.ts:121,324useComboWidget.ts:216+useAssetWidgetData.ts:1-80MediaAssetFilterBar.vue:12,30+AssetBrowserModal.vue+AssetsSidebarTab.vue:287+useMediaAssetActions.ts:96+MediaAssetContextMenu.vue:124mockFeatureFlagsL2 — Capability flags (6 sites)
useMediaAssetActions.ts:120(ZIP export)useModelUpload.ts+useUploadModelWizard.ts+missingModelPipeline.ts:114,136,164+MissingModelCard.vue:61,66,87,142+MissingModelRow.vue:35,151L3 — Asset Identity RFC (3 sites)
missingMediaScan.tsassetMetadataUtils.ts:66-68,164-201useMissingMediaInteractions.ts:92L4 — OSS filter/sort param parity
Handled as residual inside FE-732. BE-886/BE-891 light up
job_ids/asset_hash/include_publicon OSS.L5 — Typed metadata (1 site)
MediaAssetCard.vue:286image dimensionsL6 — Field renames
Localized at helper layer by FE-733. No FE rename ticket needed; helpers absorb
asset_hash→hash,prompt_id→job_idaliasing.L7 — Image-editor upload (1 site)
usePainter.ts:634,635+useMaskEditorSaver.tsN1 — Out of M1 scope (3 sites)
Widget value formats —
useMediaAssetActions.ts:46,302,446+useComboWidget.ts:186+usePainter.ts:673+useMaskEditorSaver.ts:329.Verified
pnpm typecheck→ exit 0 (after FE-732 merge)mockFeatureFlags.tsflagged as unused due to stacking-order false-positive.BE gate matrix