-
Notifications
You must be signed in to change notification settings - Fork 376
[Draft] Add QueueAssetCard presentation components #5878
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Create type-specific card components (Image, Video, Audio, Text) - Use IconButton and Card components from common library - Implement 1:1 aspect ratio for all CardTop components - Add responsive layouts without hardcoded dimensions - Support Input/Output contexts with jobId display - Add Storybook stories and unit tests 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/01/2025, 12:28:42 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 10/01/2025, 12:38:22 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
import CardContainer from '@/components/card/CardContainer.vue' | ||
import CardTop from '@/components/card/CardTop.vue' | ||
import type { AssetContext, AssetMeta } from '@/types/media.types' | ||
import { formatSize } from '@/utils/formatUtil' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[architecture] critical Priority
Issue: Incorrect import path for formatSize utility
Context: Components import formatSize from '@/utils/formatUtil' but the file exists at 'packages/shared-frontend-utils/src/formatUtil.ts'. This will cause build failures.
Suggestion: Either fix the import path or check if there should be a path alias mapping in the build configuration
> | ||
<i class="pi pi-copy text-xs" /> | ||
<!-- eslint-disable-next-line @intlify/vue-i18n/no-raw-text --> | ||
<span>MULTI</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[quality] medium Priority
Issue: Hardcoded text violates i18n guidelines
Context: The project has comprehensive i18n support but this component hardcodes 'MULTI' text
Suggestion: Use i18n translation key instead of hardcoded text, e.g., {{ ('asset.multi') }}
v-else | ||
class="w-full h-full flex items-center justify-center p-4 bg-gray-100 dark-theme:bg-gray-800 rounded-t-lg" | ||
:style="{ | ||
backgroundImage: 'url(/assets/images/default-template.png)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance] medium Priority
Issue: Hardcoded asset URL may cause loading issues
Context: Direct path '/assets/images/default-template.png' assumes specific asset location
Suggestion: Import the asset properly or use a dynamic import to ensure build tools can handle asset optimization and path resolution
> | ||
<video | ||
controls | ||
autoplay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[security] low Priority
Issue: Autoplay video without user consent
Context: Video autoplay can consume bandwidth, battery, and be accessibility unfriendly without explicit user consent
Suggestion: Remove autoplay or make it conditional based on user preferences/settings
'!rounded-lg' | ||
) | ||
|
||
function formatDuration(seconds: number): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[quality] medium Priority
Issue: Code duplication - formatDuration function
Context: Identical formatDuration function exists in both QueueVideoCard and QueueAudioCard components
Suggestion: Extract formatDuration to a shared utility function or use existing time formatting utilities from a library like date-fns
<template> | ||
<div class="flex items-center gap-1"> | ||
<span class="text-xs text-muted">{{ `Job: ${jobId}` }}</span> | ||
<button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[quality] medium Priority
Issue: Missing keyboard accessibility for JobIdSection button
Context: The copy button lacks proper keyboard event handlers
Suggestion: Add @keydown.enter and @keydown.space event handlers to support keyboard navigation
* Media types for Asset Library | ||
*/ | ||
|
||
export type MediaKind = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[quality] low Priority
Issue: Missing JSDoc documentation for public types
Context: New MediaKind type and AssetMeta interface would benefit from documentation for maintainability
Suggestion: Add JSDoc comments describing when each MediaKind should be used and what AssetMeta represents
:poster="asset.thumbnailUrl" | ||
@click.stop | ||
> | ||
<source :src="asset.videoUrl || asset.thumbnailUrl || ''" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance] low Priority
Issue: Inefficient fallback chain for video source
Context: The video source fallback uses 'asset.videoUrl || asset.thumbnailUrl || ""' which may load inappropriate content
Suggestion: Separate the logic - only use videoUrl for video source and handle missing URLs with proper error states
import type { AssetContext, AssetMeta, MediaKind } from '@/types/media.types' | ||
|
||
// Map media types to their specific card components | ||
const cardComponents = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[architecture] medium Priority
Issue: Inconsistent component mapping for media types
Context: Both 'webp' and 'gif' map to QueueImageCard, but 'pose' also maps to QueueImageCard which may not be semantically correct
Suggestion: Consider if 'pose' should have its own card component or be mapped to QueueTextCard based on its actual content type
@open-detail="emit('openDetail', $event)" | ||
@play="emit('play', $event)" | ||
@view="emit('view', $event)" | ||
@copy="emit('copy', $event)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[quality] medium Priority
Issue: Missing 'more' event handler in parent component
Context: QueueImageCard and QueueVideoCard emit 'more' events but QueueAssetCard doesn't handle or forward them
Suggestion: Add @more="emit('more', $event)" to the parent component's event handlers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: [Draft] Add QueueAssetCard presentation components (#5878)
Impact: 1499 additions, 0 deletions across 11 files
Issue Distribution
- Critical: 1
- High: 0
- Medium: 6
- Low: 3
Category Breakdown
- Architecture: 2 issues
- Security: 1 issues
- Performance: 2 issues
- Code Quality: 5 issues
Key Findings
Architecture & Design
The component architecture follows Vue 3 Composition API patterns well with proper type-specific card components. However, there's a critical import path issue that will cause build failures. The component mapping for 'pose' type needs reconsideration for semantic correctness.
Security Considerations
Video autoplay without user consent could impact bandwidth and accessibility. Consider implementing user preference controls.
Performance Impact
Hardcoded asset paths and inefficient video fallback chains may impact build optimization and runtime performance.
Integration Points
Missing event handler forwarding may break parent-child communication. The formatUtil import path mismatch is the most critical issue requiring immediate attention.
Positive Observations
- Excellent use of TypeScript with proper type definitions
- Good accessibility with ARIA labels and keyboard support
- Consistent use of Tailwind utility classes following project guidelines
- Comprehensive test coverage and Storybook stories
- Proper component composition with clear separation of concerns
- Good error and loading state handling
References
Next Steps
- Address critical import path issue before merge
- Fix missing event handler forwarding
- Extract duplicate formatDuration utility function
- Add i18n support for hardcoded text
- Consider accessibility improvements for keyboard navigation
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
Summary
Changes
QueueAssetCard
component that switches between type-specific cards based on media kindQueueImageCard
,QueueVideoCard
,QueueAudioCard
,QueueTextCard
QueueAssetThumbnail
component for media displayJobIdSection
component for output contextTest Plan
🤖 Generated with Claude Code
┆Issue is synchronized with this Notion page by Unito