Merged
Conversation
- Add addAssetTags (POST) and removeAssetTags (DELETE) in assetService - Add TagsOperationResult schema and type - Update updateAssetTags to compute diff with es-toolkit difference - Update cache with server response after operations - Pass asset object to updateAssetTags for current tags access Amp-Thread-ID: https://ampcode.com/threads/T-019be775-4085-750c-a7a8-190760deaae4 Co-authored-by: Amp <amp@ampcode.com>
Contributor
📝 WalkthroughWalkthroughAdds tag add/remove APIs and store logic with diffed optimistic updates, updates model info UI with copy/edit controls and pendingModelType handling, introduces a new button size variant Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant MIP as ModelInfoPanel
participant Store as AssetsStore
participant Service as AssetService
participant Server as Server
User->>MIP: request tag change (add/remove)
MIP->>Store: updateAssetTags(asset, newTags)
Store->>Store: compute diff (adds, removes)
Store->>Store: optimistic cache update (new tag set)
alt has adds
Store->>Service: addAssetTags(asset.id, adds)
Service->>Server: POST /assets/{id}/tags
Server-->>Service: TagsOperationResult
Service-->>Store: result
end
alt has removes
Store->>Service: removeAssetTags(asset.id, removes)
Service->>Server: DELETE /assets/{id}/tags
Server-->>Service: TagsOperationResult
Service-->>Store: result
end
Store->>Store: update cache with final server tag state
Store-->>MIP: success
alt failure
Store->>Store: revert cache to original tags
Store-->>MIP: error
end
Possibly related PRs
Suggested reviewers
✨ Finishing touches
Comment |
🎭 Playwright Tests:
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/23/2026, 12:06:56 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.7 kB (baseline 22.7 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 948 kB (baseline 948 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: 11 added / 11 removed Panels & Settings — 440 kB (baseline 440 kB) • 🟢 -8 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 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.83 kB (baseline 2.83 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.7 kB (baseline 33.7 kB) • 🔴 +15 BReusable component library chunks
Status: 8 added / 8 removed Data & Services — 3.17 MB (baseline 3.16 MB) • 🔴 +5.15 kBStores, services, APIs, and repositories
Status: 8 added / 8 removed Utilities & Hooks — 24 kB (baseline 24 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 10 added / 10 removed Vendor & Third-Party — 10.7 MB (baseline 10.7 MB) • 🔴 +42 BExternal libraries and shared vendor chunks
Status: 1 added / 1 removed Other — 6.36 MB (baseline 6.36 MB) • 🟢 -192 BBundles that do not match a named category
Status: 42 added / 42 removed |
src/stores/assetsStore.ts
Outdated
Comment on lines
494
to
502
| const removeResult = | ||
| tagsToRemove.length > 0 | ||
| ? await assetService.removeAssetTags(asset.id, tagsToRemove) | ||
| : undefined | ||
|
|
||
| const addResult = | ||
| tagsToAdd.length > 0 | ||
| ? await assetService.addAssetTags(asset.id, tagsToAdd) | ||
| : undefined |
Contributor
There was a problem hiding this comment.
these throw. Do we want to add protection?
Amp-Thread-ID: https://ampcode.com/threads/T-019be794-70c1-73c8-a600-f68093216734 Co-authored-by: Amp <amp@ampcode.com>
DrJKL
changed the title
… the editable name.
- Add try/catch to updateAssetMetadata and updateAssetTags - Roll back cache to original state on API failure - Change updateAssetMetadata to accept full asset object Amp-Thread-ID: https://ampcode.com/threads/T-019be7fe-630d-7478-ad0f-7568ab09afaa Co-authored-by: Amp <amp@ampcode.com>
dosubot
bot
added
the
size:L
coderabbitai
bot
requested review from
KarryCharon,
Yorha4D,
christian-byrne,
luke-mino-altherr,
shinshin86 and
viva-jinyi
| const originalMetadata = asset.user_metadata | ||
| updateAssetInCache(asset.id, { user_metadata: userMetadata }, cacheKey) | ||
|
|
||
| try { |
Contributor
There was a problem hiding this comment.
we dont really need the try here anymore but i guess it doesnt hurt
luke-mino-altherr
previously approved these changes
Jan 22, 2026
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
![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
🤖 Fix all issues with AI agents
In `@src/platform/assets/components/modelInfo/ModelInfoPanel.vue`:
- Around line 337-344: The getter for selectedModelType contains a redundant "??
undefined"; update the computed getter in selectedModelType to return
pendingModelType.value ?? getAssetModelType(asset) (remove the trailing "??
undefined") so the expression naturally yields undefined when both operands are
nullish; ensure this change is made in the computed definition that references
pendingModelType, getAssetModelType, and asset.
- Around line 148-156: The icon-only Button rendering the copy-all action lacks
an aria-label, so screen readers may not announce its purpose; update the Button
(the component instance with variant="muted-textonly" size="icon-sm"
:title="t('g.copyAll')" and `@click`="copyToClipboard(triggerPhrases.join(', '))")
to include an aria-label attribute (e.g., :aria-label="t('g.copyAll')" or
aria-label="Copy all") so the accessible name is explicit and matches the
tooltip.
- Around line 22-30: The icon-only edit Button (component <Button> in
ModelInfoPanel.vue controlling isEditingDisplayName and gated by isImmutable) is
missing an accessible label; add an aria-label prop to the Button (e.g.,
aria-label="Edit display name") so screen readers can announce its purpose, or
bind a dynamic label (e.g., :aria-label="isEditingDisplayName ? 'Cancel editing
display name' : 'Edit display name'") if you want state-aware text; keep the
existing v-if and click handler unchanged and ensure the label matches the icon
class "icon-[lucide--square-pen]".
♻️ Duplicate comments (1)
src/platform/assets/components/modelInfo/ModelInfoPanel.vue (1)
84-89: Use i18n for the 'Unknown' fallback text.The hardcoded
'Unknown'string at line 87 violates the coding guideline requiring vue-i18n for all user-facing strings. This was flagged in a previous review.Proposed fix
<div v-else class="p-2 text-sm text-muted-foreground"> {{ modelTypes.find((o) => o.value === selectedModelType)?.name ?? - 'Unknown' + t('g.unknown') }} </div>Add to
src/locales/en/main.json:"unknown": "Unknown"
Comment on lines
337
to
344
| const selectedModelType = computed({ | ||
| get: () => getAssetModelType(asset) ?? undefined, | ||
| get: () => pendingModelType.value ?? getAssetModelType(asset) ?? undefined, | ||
| set: (value: string | undefined) => { | ||
| if (value) debouncedSaveModelType(value) | ||
| if (!value) return | ||
| pendingModelType.value = value | ||
| debouncedSaveModelType(value) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove redundant ?? undefined.
The trailing ?? undefined on line 338 is redundant. If both pendingModelType.value and getAssetModelType(asset) are nullish, the expression already evaluates to undefined.
Proposed fix
const selectedModelType = computed({
- get: () => pendingModelType.value ?? getAssetModelType(asset) ?? undefined,
+ get: () => pendingModelType.value ?? getAssetModelType(asset),
set: (value: string | undefined) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const selectedModelType = computed({ | |
| get: () => getAssetModelType(asset) ?? undefined, | |
| get: () => pendingModelType.value ?? getAssetModelType(asset) ?? undefined, | |
| set: (value: string | undefined) => { | |
| if (value) debouncedSaveModelType(value) | |
| if (!value) return | |
| pendingModelType.value = value | |
| debouncedSaveModelType(value) | |
| } | |
| }) | |
| const selectedModelType = computed({ | |
| get: () => pendingModelType.value ?? getAssetModelType(asset), | |
| set: (value: string | undefined) => { | |
| if (!value) return | |
| pendingModelType.value = value | |
| debouncedSaveModelType(value) | |
| } | |
| }) |
🤖 Prompt for AI Agents
In `@src/platform/assets/components/modelInfo/ModelInfoPanel.vue` around lines 337
- 344, The getter for selectedModelType contains a redundant "?? undefined";
update the computed getter in selectedModelType to return pendingModelType.value
?? getAssetModelType(asset) (remove the trailing "?? undefined") so the
expression naturally yields undefined when both operands are nullish; ensure
this change is made in the computed definition that references pendingModelType,
getAssetModelType, and asset.
Amp-Thread-ID: https://ampcode.com/threads/T-019be819-5921-77e8-9464-0196a935dd17 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019be81f-4461-73c1-84bf-f2231fa872e7 Co-authored-by: Amp <amp@ampcode.com>
Myestery
reviewed
Jan 22, 2026
Myestery
previously approved these changes
Jan 23, 2026
Amp-Thread-ID: https://ampcode.com/threads/T-019be828-5bdf-7475-9cc9-7980a2ba90d7 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/platform/assets/components/modelInfo/ModelInfoPanel.test.ts`:
- Around line 10-14: Replace the inline mock in ModelInfoPanel.test.ts with a
hoisted mock so tests can assert clipboard calls: use vi.hoisted to create and
expose a shared mock function (e.g., mockCopyToClipboard) while still returning
useCopyToClipboard -> { copyToClipboard: mockCopyToClipboard }, then in your
tests import/reference that exposed mockCopyToClipboard, call
mockCopyToClipboard.mockClear() in beforeEach and assert expectations like
expect(mockCopyToClipboard).toHaveBeenCalledWith('trigger1') for single-phrase
clicks and expect(mockCopyToClipboard).toHaveBeenCalledWith('trigger1 trigger2')
for the copy-all button; target the useCopyToClipboard and copyToClipboard
symbols when making these changes.
Comment on lines
+10
to
+14
| vi.mock('@/composables/useCopyToClipboard', () => ({ | ||
| useCopyToClipboard: () => ({ | ||
| copyToClipboard: vi.fn() | ||
| }) | ||
| })) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using vi.hoisted() to enable assertions on clipboard calls.
The current mock prevents runtime errors but doesn't expose copyToClipboard for test assertions. Since the PR adds click-to-copy functionality, tests should verify the behavior works correctly.
♻️ Proposed refactor to enable per-test assertions
+const mockCopyToClipboard = vi.hoisted(() => vi.fn())
+
-vi.mock('@/composables/useCopyToClipboard', () => ({
- useCopyToClipboard: () => ({
- copyToClipboard: vi.fn()
- })
-}))
+vi.mock('@/composables/useCopyToClipboard', () => ({
+ useCopyToClipboard: () => ({
+ copyToClipboard: mockCopyToClipboard
+ })
+}))Then add tests for the copy functionality:
describe('Trigger Phrases Copy', () => {
beforeEach(() => {
mockCopyToClipboard.mockClear()
})
it('copies individual trigger phrase when clicked', async () => {
const asset = createMockAsset({
user_metadata: { trained_words: ['trigger1', 'trigger2'] }
})
const wrapper = mountPanel(asset)
// Find and click the copy button for a phrase
// Assert: expect(mockCopyToClipboard).toHaveBeenCalledWith('trigger1')
})
it('copies all trigger phrases when copy-all button clicked', async () => {
const asset = createMockAsset({
user_metadata: { trained_words: ['trigger1', 'trigger2'] }
})
const wrapper = mountPanel(asset)
// Find and click the copy-all button
// Assert: expect(mockCopyToClipboard).toHaveBeenCalledWith('trigger1 trigger2')
})
})Based on learnings, using vi.hoisted() allows per-test manipulation of mock state, which is essential for verifying clipboard interactions.
Would you like me to generate complete test cases for the new copy-to-clipboard functionality?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| vi.mock('@/composables/useCopyToClipboard', () => ({ | |
| useCopyToClipboard: () => ({ | |
| copyToClipboard: vi.fn() | |
| }) | |
| })) | |
| const mockCopyToClipboard = vi.hoisted(() => vi.fn()) | |
| vi.mock('@/composables/useCopyToClipboard', () => ({ | |
| useCopyToClipboard: () => ({ | |
| copyToClipboard: mockCopyToClipboard | |
| }) | |
| })) |
🤖 Prompt for AI Agents
In `@src/platform/assets/components/modelInfo/ModelInfoPanel.test.ts` around lines
10 - 14, Replace the inline mock in ModelInfoPanel.test.ts with a hoisted mock
so tests can assert clipboard calls: use vi.hoisted to create and expose a
shared mock function (e.g., mockCopyToClipboard) while still returning
useCopyToClipboard -> { copyToClipboard: mockCopyToClipboard }, then in your
tests import/reference that exposed mockCopyToClipboard, call
mockCopyToClipboard.mockClear() in beforeEach and assert expectations like
expect(mockCopyToClipboard).toHaveBeenCalledWith('trigger1') for single-phrase
clicks and expect(mockCopyToClipboard).toHaveBeenCalledWith('trigger1 trigger2')
for the copy-all button; target the useCopyToClipboard and copyToClipboard
symbols when making these changes.
Myestery
approved these changes
Jan 23, 2026
DrJKL
added
cloud/1.37
github-actions bot
pushed a commit
that referenced
this pull request
Jan 23, 2026
## Summary Model management improvements: refactored tag API, enhanced UX for trigger phrases and editing. ## Changes ### API Refactor - Add `addAssetTags` (POST) and `removeAssetTags` (DELETE) endpoints in assetService - `updateAssetTags` now computes diff and calls remove/add serially - Add `TagsOperationResult` schema; cache syncs with server response ### UX Improvements - **Trigger phrases**: click to copy individual phrases, copy-all button in header - **Display name**: show edit icon on hover instead of relying on double-click - **Model type**: show plain text when immutable instead of disabled select - **Tags input**: only show edit icon on hover - **Field labels**: updated styling to use muted foreground color - **Optimistic update** for model type selection --------- Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: GitHub Action <action@github.com>
Member
github-actions
bot
removed
the
needs-backport
DrJKL
added a commit
that referenced
this pull request
Jan 23, 2026
Backport of #8248 to `cloud/1.37` Automatically created by backport workflow. Co-authored-by: Alexander Brown <drjkl@comfy.org> Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: GitHub Action <action@github.com>
Myestery
pushed a commit
that referenced
this pull request
Jan 23, 2026
## Summary Model management improvements: refactored tag API, enhanced UX for trigger phrases and editing. ## Changes ### API Refactor - Add `addAssetTags` (POST) and `removeAssetTags` (DELETE) endpoints in assetService - `updateAssetTags` now computes diff and calls remove/add serially - Add `TagsOperationResult` schema; cache syncs with server response ### UX Improvements - **Trigger phrases**: click to copy individual phrases, copy-all button in header - **Display name**: show edit icon on hover instead of relying on double-click - **Model type**: show plain text when immutable instead of disabled select - **Tags input**: only show edit icon on hover - **Field labels**: updated styling to use muted foreground color - **Optimistic update** for model type selection --------- Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: GitHub Action <action@github.com>
This was referenced Jan 23, 2026
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
6 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.
Summary
Model management improvements: refactored tag API, enhanced UX for trigger phrases and editing.
Changes
API Refactor
addAssetTags(POST) andremoveAssetTags(DELETE) endpoints in assetServiceupdateAssetTagsnow computes diff and calls remove/add seriallyTagsOperationResultschema; cache syncs with server responseUX Improvements