[feat] Implement media asset workflow actions with shared utilities #6696
Conversation
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
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 11/17/2025, 11:56:30 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 11/18/2025, 12:06:19 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
viva-jinyi
force-pushed
the
feature/media-asset-action
branch
from
7430819 to
91af228
Compare
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.14 MB (baseline 3.13 MB) • 🔴 +8.37 kBMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 789 kB (baseline 789 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 8.03 kB (baseline 8.03 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 307 kB (baseline 307 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 136 kB (baseline 136 kB) • ⚪ 0 BReusable component library chunks
Status: 6 added / 6 removed Data & Services — 12.6 kB (baseline 12.6 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 3 added / 3 removed Utilities & Hooks — 5.87 kB (baseline 5.87 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 1 added / 1 removed Vendor & Third-Party — 5.32 MB (baseline 5.32 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Status: 3 added / 3 removed Other — 3.94 MB (baseline 3.94 MB) • ⚪ 0 BBundles that do not match a named category
Status: 21 added / 21 removed |
viva-jinyi
force-pushed
the
feature/media-asset-action
branch
from
81ff174 to
b242969
Compare
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
Member
Author
|
This PR is being split into smaller, more reviewable PRs:
Each PR will be independent and can be reviewed/merged separately. Closing this PR in favor of the new ones. |
6 tasks
viva-jinyi
requested review from
a team,
KarryCharon,
Yorha4D and
shinshin86
as code owners
dosubot
bot
added
the
size:XL
viva-jinyi
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // This supports PNG, WEBP, FLAC, and other formats with metadata | ||
| try { | ||
| const fileUrl = getAssetUrl(asset) | ||
| const response = await fetch(fileUrl) |
There was a problem hiding this comment.
[architecture] medium Priority
Issue: Network request without proper error handling could expose internal errors to users
Context: fetch() call on line 43 could expose network errors that reveal internal system information
Suggestion: Add proper error handling and sanitize error messages before user-facing display
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: [feat] Implement media asset workflow actions with shared utilities (#6696)
Impact: 588 additions, 110 deletions across 12 files
Issue Distribution
- Critical: 0
- High: 0
- Medium: 3
- Low: 2
Category Breakdown
- Architecture: 2 issues
- Security: 1 issue
- Performance: 1 issue
- Code Quality: 1 issue
Key Findings
Architecture & Design
This PR follows good architectural patterns by:
- Creating shared utility functions to eliminate code duplication (~150+ lines eliminated)
- Using proper composable patterns with useMediaAssetActions
- Implementing clean separation of concerns between UI components and business logic
- Following established error handling patterns with toast notifications
The new utilities (assetTypeUtil, assetUrlUtil, workflowActionsService) are well-designed and reusable. The workflow extraction pattern is particularly well-architected, supporting both output assets with metadata and input assets with embedded workflows.
Security Considerations
- Minor concern about filename validation in URL construction - while encodeURIComponent provides basic protection, additional validation would be safer
- Network error handling could potentially expose internal system information to users
- Overall security posture is good with proper use of existing validation utilities
Performance Impact
- Promise.all usage in bulk operations has some error swallowing that could mask partial failures
- Network requests lack timeout handling, which could impact user experience
- Bundle impact is positive due to code deduplication and shared utilities
- No significant performance regressions expected
Integration Points
- Good integration with existing stores (assetsStore, workflowStore)
- Proper use of existing composables (useCopyToClipboard, useToast)
- Clean integration with the node creation system via litegraphService
- Maintains compatibility with both cloud and OSS environments
Positive Observations
- Excellent code deduplication: The refactoring eliminates ~150+ lines of duplicate code
- Consistent error handling: Uses established toast notification patterns
- Good TypeScript usage: Proper typing throughout, no any types
- Internationalization: All user-facing strings properly localized with i18n keys
- Clean utilities: Well-documented utility functions with good JSDoc comments
- Composable architecture: Follows Vue 3 best practices with proper composable patterns
- Proper testing considerations: Code is structured to be testable
References
Next Steps
- Address medium priority issues in error handling and Promise.all patterns
- Consider adding filename validation for enhanced security
- Add timeout handling for network requests
- Consider adding unit tests for the new utility functions
- Update documentation if needed
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
github-actions
bot
removed
the
claude-review
viva-jinyi
force-pushed
the
feature/media-asset-action
branch
from
225a14a to
d3070b4
Compare
DrJKL
reviewed
Nov 17, 2025
DrJKL
reviewed
Nov 17, 2025
| export function useMediaAssetActions() { | ||
| const toast = useToast() | ||
| const dialogStore = useDialogStore() | ||
| const mediaContext = inject(MediaAssetKey, null) |
Contributor
There was a problem hiding this comment.
It looks like a lot of things depend on this being present. Could it be a required argument to the composable instead of being Provided?
DrJKL
reviewed
Nov 17, 2025
| life: 3000 | ||
| }) | ||
| } | ||
| } catch (error) { |
Contributor
There was a problem hiding this comment.
half-nit, half-actual-concern:
When there are so many things happening in the try block, it's easy for the operations to have partially completed which can leave the store in an invalid state.
Could you use multiple smaller try blocks, maybe tracking which pieces specifically failed?
DrJKL
reviewed
Nov 17, 2025
Contributor
There was a problem hiding this comment.
Very nit, but this seems like it could be a single module, src/platform/assets/utils.ts that exports multiple utility functions (each of which would then be tested in src/platform/assets/utils.test.ts)
DrJKL
reviewed
Nov 17, 2025
Comment on lines
+43
to
+45
| ): Promise<{ | ||
| success: boolean | ||
| error?: string |
Contributor
There was a problem hiding this comment.
nit: I like a good Result<T> monad, but this seems more like it could be an Optional<Error> with absence indicating success, a la form validation.
DrJKL
reviewed
Nov 17, 2025
| * supportsWorkflowMetadata('image.png') // true | ||
| * supportsWorkflowMetadata('image.jpg') // false | ||
| */ | ||
| export function supportsWorkflowMetadata(filename: string): boolean { |
Contributor
There was a problem hiding this comment.
quest: I think there's a similar list elsewhere in the codebase...
(Also, I think there are a lot more file formats that support the metadata than you'd think)
DrJKL
approved these changes
Nov 17, 2025
Contributor
DrJKL
left a comment
DrJKL
left a comment
There was a problem hiding this comment.
I have a few comments, but I don't think anything that couldn't be a follow-up.
Implemented 4 missing workflow features for media assets: - copyJobId: Fixed to properly extract promptId from metadata - addWorkflow: Add loader nodes (LoadImage/LoadVideo/LoadAudio) to canvas - openWorkflow: Open workflow from asset metadata/embedded PNG in new tab - exportWorkflow: Export workflow as JSON file Created shared utilities to eliminate code duplication: - assetTypeUtil.ts: Extract asset type with fallback (replaced 6 duplications) - assetUrlUtil.ts: Construct asset URLs (replaced 3 duplications) - workflowActionsService.ts: Shared workflow export/open operations - workflowExtractionUtil.ts: Extract workflows from jobs/assets - loaderNodeUtil.ts: Detect loader node types from filenames Refactored existing code: - Used formatUtil.getMediaTypeFromFilename() in loaderNodeUtil - Extracted deleteAssetApi() helper to reduce deletion logic duplication - Moved isResultItemType type guard to shared typeGuardUtil.ts - Added 9 i18n strings for proper localization - Added @comfyorg/shared-frontend-utils dependency Input assets now support workflow features where applicable: - All media files (JPEG/PNG/MP4/etc) can be added to current workflow - PNG/WEBP/FLAC files with embedded metadata support open/export workflow 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
In OSS environments, asset.id is the promptId directly. In Cloud environments, promptId is in user_metadata. Updated copyJobId to check both sources (same as deletion logic). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Refactored downloadAsset and downloadMultipleAssets functions to use the shared getAssetUrl utility instead of duplicating URL construction logic. Changes: - Added getAssetUrl import from assetUrlUtil - Replaced inline api.apiURL construction in downloadAsset (lines 74-77) - Replaced inline api.apiURL construction in downloadMultipleAssets (lines 112-115) Benefits: - Removes code duplication (2 occurrences) - Centralizes URL construction logic - Improves maintainability - Ensures consistent asset type handling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixed issue where partial deletion failures were not reported to users.
Previously, if some assets failed to delete, the UI would still show
"all assets deleted successfully" which could mislead users about data loss.
Changes:
- Replaced Promise.all with Promise.allSettled in deleteMultipleAssets
- Track individual success/failure for each asset deletion
- Show different toast messages based on results:
- All success: success toast with count
- All failed: error toast
- Partial success: warning toast with succeeded/failed counts
- Improved error logging with asset names for debugging
Added i18n:
- mediaAsset.selection.partialDeleteSuccess: "{succeeded} deleted successfully, {failed} failed"
This addresses PR review comment #2 (Medium Priority).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
viva-jinyi
force-pushed
the
feature/media-asset-action
branch
from
629eead to
3e9f906
Compare
Member
Author
|
Since this feature is currently missing on main, I’ll create an additional refactor branch to add it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Implements 4 missing media asset workflow features and creates shared utilities to eliminate code duplication.
Implemented Features
1. Copy Job ID ✅
getOutputAssetMetadatauseCopyToClipboardcomposable2. Add to Current Workflow ✅
detectNodeTypeFromFilenameutility3. Open Workflow in New Tab ✅
4. Export Workflow ✅
Code Refactoring
Created Shared Utilities:
assetTypeUtil.ts-getAssetType()function eliminates 6 instances ofasset.tags?.[0] || 'output'assetUrlUtil.ts-getAssetUrl()function consolidates 3 URL construction patternsworkflowActionsService.ts- Shared service for workflow export/open operationsworkflowExtractionUtil.ts- Extract workflows from jobs/assetsloaderNodeUtil.ts- Detect loader node types from filenamesImprovements to Existing Code:
formatUtil.getMediaTypeFromFilename()deleteAssetApi()helper to reduce deletion logic duplication (~40 lines)isResultItemTypetype guard to sharedtypeGuardUtil.ts@comfyorg/shared-frontend-utilsdependencyInput Assets Support
Improved input assets to support workflow features where applicable:
Impact
Testing
✅ TypeScript typecheck passed
✅ ESLint passed
✅ Knip passed
🤖 Generated with Claude Code
┆Issue is synchronized with this Notion page by Unito