Skip to content

Conversation

@viva-jinyi
Copy link
Member

@viva-jinyi viva-jinyi commented Oct 20, 2025

Summary

  • Extract sidebar template into reusable AssetSidebarTemplate component
  • Replace PrimeVue Tabs with TextButton for better visual consistency
  • Improve job detail view header layout with better spacing

Changes

  • Created AssetSidebarTemplate.vue as a reusable template component
  • Replaced PrimeVue Tabs with TextButton components for tab navigation
  • Added i18n translation key for "Back to all assets" button
  • Improved spacing and layout in job detail view header
  • Maintained all existing functionality while cleaning up template structure

Test Plan

  • Verify tab switching between Imported and Generated tabs works correctly
  • Test job detail view displays properly with Job ID and execution time
  • Confirm "Back to all assets" button returns to main view
  • Check that all existing media asset features remain functional
  • Verify UI consistency with other sidebar tabs
screen-capture.webm

@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Oct 20, 2025
@github-actions
Copy link

github-actions bot commented Oct 20, 2025

🎭 Playwright Test Results

⚠️ Tests passed with flaky tests

⏰ Completed at: 10/23/2025, 02:24:14 PM UTC

📈 Summary

  • Total Tests: 498
  • Passed: 464 ✅
  • Failed: 0
  • Flaky: 3 ⚠️
  • Skipped: 31 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 455 / ❌ 0 / ⚠️ 3 / ⏭️ 31
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 6 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

@viva-jinyi viva-jinyi force-pushed the feature/media-asset-sidebar-tab branch from bee62d3 to 61eed48 Compare October 22, 2025 02:08
@viva-jinyi viva-jinyi changed the title refactor: Extract AssetsSidebarTab template and improve UI structure Extract AssetsSidebarTab template and improve UI structure Oct 22, 2025
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Oct 22, 2025
@viva-jinyi viva-jinyi force-pushed the feature/media-asset-sidebar-tab branch from ac427e5 to bd23ccc Compare October 22, 2025 03:06
@viva-jinyi viva-jinyi force-pushed the feature/job-detail-view branch from 915a9e9 to 39dad98 Compare October 22, 2025 03:11
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Oct 22, 2025
@viva-jinyi viva-jinyi force-pushed the feature/media-asset-sidebar-tab branch from 97d6a6f to 6769687 Compare October 22, 2025 12:51
@viva-jinyi viva-jinyi force-pushed the feature/job-detail-view branch from 354c05e to 98390f5 Compare October 22, 2025 12:52
@christian-byrne christian-byrne added the claude-review Add to trigger a PR code review from Claude Code label Oct 22, 2025
Copy link

@claude claude bot left a 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: Extract AssetsSidebarTab template and improve UI structure (#6164)
Impact: 729 additions, 62 deletions across 15 files

Issue Distribution

  • Critical: 0
  • High: 1
  • Medium: 5
  • Low: 2

Category Breakdown

  • Architecture: 2 issues
  • Security: 1 issue
  • Performance: 0 issues
  • Code Quality: 5 issues

Key Findings

Architecture & Design

The PR introduces custom tab components to replace PrimeVue's TabView, which diverges from the established component library ecosystem. While the implementation follows Vue 3 patterns correctly, it creates duplicate functionality that may be better addressed by extending PrimeVue components with custom styling. The AssetSidebarTemplate component shows good separation of concerns but could benefit from more flexible, composable design patterns.

Security Considerations

The code handles external URLs and user-provided metadata without sufficient validation. Direct assignment of output.url to preview_url poses potential XSS risks if malicious content is provided. Type assertions using 'as any[]' bypass TypeScript safety mechanisms for user-provided data.

Performance Impact

No significant performance concerns identified. The use of VirtualGrid for asset rendering and computed properties for reactive data are appropriate patterns. The component properly leverages Vue 3's reactivity system.

Integration Points

The PR maintains backward compatibility while introducing new folder view functionality. The integration with existing media asset systems appears solid, with proper error handling for toast notifications and clipboard operations.

Positive Observations

  • Excellent test coverage for the new UUID utility functions with comprehensive edge cases
  • Proper use of Vue 3 Composition API and TypeScript throughout
  • Good separation of concerns with template extraction
  • Appropriate use of semantic CSS tokens for theming
  • Proper i18n integration for new UI strings

References

Next Steps

  1. Address critical issues before merge (currently none)
  2. Consider architectural feedback for long-term maintainability
  3. Add proper validation for external URL handling
  4. Remove debug console.log statements
  5. Consider extending PrimeVue components instead of reimplementing tab functionality

This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.

@github-actions github-actions bot removed the claude-review Add to trigger a PR code review from Claude Code label Oct 22, 2025
@viva-jinyi viva-jinyi force-pushed the feature/media-asset-sidebar-tab branch from cefad39 to 13c5b44 Compare October 23, 2025 11:14
  - Add folder view to display all outputs from a single batch job
  - Show output count badge on assets with multiple batch outputs
  - Add job ID display and copy functionality in folder view header
  - Display execution time for batch jobs
  - Implement download functionality for output assets only
  - Add inspect action to asset more menu
  - Extract prompt ID from asset IDs using new UUID utility
  - Add comprehensive tests for UUID extraction utilities
  - Use existing formatDuration util from shared utils instead of custom
  implementation
  - Extract execution time formatting to computed property for cleaner
  template
  - Remove 'any' type assertion in MediaAssetActions, use proper types
  - Extract sidebar template into reusable AssetSidebarTemplate component
  - Replace PrimeVue Tabs with TextButton for better visual consistency
  - Add i18n key for "Back to all assets" button
  - Improve job detail view header layout with better spacing
  - Maintain existing functionality while cleaning up template structure
  - Add new Tab and TabList components with provide/inject pattern for
  state management
  - Replace TextButton-based tabs with new Tab components in
  AssetsSidebarTab
  - Update AssetSidebarTemplate to use semantic color tokens
  (bg-interface-panel-surface)
  - Improve tab styling with proper hover and focus states
@viva-jinyi viva-jinyi force-pushed the feature/job-detail-view branch from 492116b to 75c865c Compare October 23, 2025 11:44
- Replace type assertions with type guards and proper runtime checks
- Add OutputAssetMetadata interface for structured metadata typing
- Use uuid package instead of custom regex validation
- Add i18n support for toast messages (Job ID copy)
- Implement WAI-ARIA tab pattern with basic accessibility
- Remove complex keyboard navigation in favor of simple tab key navigation
- Add getAssetType utility function for consistent type handling
- Use test.for for parameterized UUID validation tests
- Simplify Tab components to focus on basic accessibility requirements
  validation

  - Replace any type with proper Record<string,
  unknown> | undefined
  - Fix nodeId type check to support both string
  and number types
  - Remove redundant object type checks in type
  guard
  - Remove unused isValidUuidV4 function from
  uuidUtil
  - Simplify metadata validation logic
@viva-jinyi
Copy link
Member Author

Thank you for the thorough review! I’ve addressed all feedback and implemented the requested changes.

Changes Made
1. Type Safety
• Removed all type assertions (as string, as number, as any[]) and replaced them with proper type guards and runtime checks.
• Introduced an OutputAssetMetadata interface to replace untyped Record<string, any>.
• Added a getAssetType() utility for consistent type handling across the codebase.
2. Code Quality
• Replaced the custom UUID regex with the standard uuid package (uuid.validate()).
• Added i18n for all user-facing strings (e.g., clipboard toast messages).
• Refactored tests to use test.for for parameterized UUID validation.
3. Accessibility
• Implemented the WAI-ARIA tab pattern with proper attributes (role="tablist", role="tab", aria-selected, aria-controls").
• Simplified keyboard navigation by using standard Tab navigation (removed complex arrow-key handling).
• Ensured all tabs have tabindex="0" for keyboard accessibility.
4. Fixes
• Restored missing folder-view functionality after rebase (now fetching all outputs instead of just preview).
• Added robust error handling for clipboard operations with a fallback mechanism.

Pending Item

The suggestion to use the provide/inject pattern for MediaAssetCard actions (from @DrJKL’s review) will be handled in a separate PR to keep this one focused on the current feedback.

All tests are passing, and the code is ready for review. Please let me know if anything needs clarification!

@christian-byrne christian-byrne merged commit b48636a into feature/media-asset-sidebar-tab Oct 23, 2025
20 checks passed
@christian-byrne christian-byrne deleted the feature/job-detail-view branch October 23, 2025 14:56
viva-jinyi added a commit that referenced this pull request Oct 24, 2025
## Summary
- Extract sidebar template into reusable AssetSidebarTemplate component
- Replace PrimeVue Tabs with TextButton for better visual consistency  
- Improve job detail view header layout with better spacing

## Changes
- Created `AssetSidebarTemplate.vue` as a reusable template component
- Replaced PrimeVue Tabs with TextButton components for tab navigation
- Added i18n translation key for "Back to all assets" button
- Improved spacing and layout in job detail view header
- Maintained all existing functionality while cleaning up template
structure

## Test Plan
- [ ] Verify tab switching between Imported and Generated tabs works
correctly
- [ ] Test job detail view displays properly with Job ID and execution
time
- [ ] Confirm "Back to all assets" button returns to main view
- [ ] Check that all existing media asset features remain functional
- [ ] Verify UI consistency with other sidebar tabs


[screen-capture.webm](https://github.com/user-attachments/assets/4ed192e1-a9f7-4fc1-a41e-f732741dd55d)
viva-jinyi added a commit that referenced this pull request Oct 24, 2025
## Summary
- Extract sidebar template into reusable AssetSidebarTemplate component
- Replace PrimeVue Tabs with TextButton for better visual consistency  
- Improve job detail view header layout with better spacing

## Changes
- Created `AssetSidebarTemplate.vue` as a reusable template component
- Replaced PrimeVue Tabs with TextButton components for tab navigation
- Added i18n translation key for "Back to all assets" button
- Improved spacing and layout in job detail view header
- Maintained all existing functionality while cleaning up template
structure

## Test Plan
- [ ] Verify tab switching between Imported and Generated tabs works
correctly
- [ ] Test job detail view displays properly with Job ID and execution
time
- [ ] Confirm "Back to all assets" button returns to main view
- [ ] Check that all existing media asset features remain functional
- [ ] Verify UI consistency with other sidebar tabs


[screen-capture.webm](https://github.com/user-attachments/assets/4ed192e1-a9f7-4fc1-a41e-f732741dd55d)
viva-jinyi added a commit that referenced this pull request Oct 24, 2025
## Summary
- Extract sidebar template into reusable AssetSidebarTemplate component
- Replace PrimeVue Tabs with TextButton for better visual consistency  
- Improve job detail view header layout with better spacing

## Changes
- Created `AssetSidebarTemplate.vue` as a reusable template component
- Replaced PrimeVue Tabs with TextButton components for tab navigation
- Added i18n translation key for "Back to all assets" button
- Improved spacing and layout in job detail view header
- Maintained all existing functionality while cleaning up template
structure

## Test Plan
- [ ] Verify tab switching between Imported and Generated tabs works
correctly
- [ ] Test job detail view displays properly with Job ID and execution
time
- [ ] Confirm "Back to all assets" button returns to main view
- [ ] Check that all existing media asset features remain functional
- [ ] Verify UI consistency with other sidebar tabs


[screen-capture.webm](https://github.com/user-attachments/assets/4ed192e1-a9f7-4fc1-a41e-f732741dd55d)
DrJKL pushed a commit that referenced this pull request Oct 29, 2025
## Summary
- Extract sidebar template into reusable AssetSidebarTemplate component
- Replace PrimeVue Tabs with TextButton for better visual consistency  
- Improve job detail view header layout with better spacing

## Changes
- Created `AssetSidebarTemplate.vue` as a reusable template component
- Replaced PrimeVue Tabs with TextButton components for tab navigation
- Added i18n translation key for "Back to all assets" button
- Improved spacing and layout in job detail view header
- Maintained all existing functionality while cleaning up template
structure

## Test Plan
- [ ] Verify tab switching between Imported and Generated tabs works
correctly
- [ ] Test job detail view displays properly with Job ID and execution
time
- [ ] Confirm "Back to all assets" button returns to main view
- [ ] Check that all existing media asset features remain functional
- [ ] Verify UI consistency with other sidebar tabs


[screen-capture.webm](https://github.com/user-attachments/assets/4ed192e1-a9f7-4fc1-a41e-f732741dd55d)
viva-jinyi added a commit that referenced this pull request Oct 29, 2025
## Summary
- Extract sidebar template into reusable AssetSidebarTemplate component
- Replace PrimeVue Tabs with TextButton for better visual consistency  
- Improve job detail view header layout with better spacing

## Changes
- Created `AssetSidebarTemplate.vue` as a reusable template component
- Replaced PrimeVue Tabs with TextButton components for tab navigation
- Added i18n translation key for "Back to all assets" button
- Improved spacing and layout in job detail view header
- Maintained all existing functionality while cleaning up template
structure

## Test Plan
- [ ] Verify tab switching between Imported and Generated tabs works
correctly
- [ ] Test job detail view displays properly with Job ID and execution
time
- [ ] Confirm "Back to all assets" button returns to main view
- [ ] Check that all existing media asset features remain functional
- [ ] Verify UI consistency with other sidebar tabs


[screen-capture.webm](https://github.com/user-attachments/assets/4ed192e1-a9f7-4fc1-a41e-f732741dd55d)
viva-jinyi added a commit that referenced this pull request Oct 29, 2025
## Summary
- Extract sidebar template into reusable AssetSidebarTemplate component
- Replace PrimeVue Tabs with TextButton for better visual consistency  
- Improve job detail view header layout with better spacing

## Changes
- Created `AssetSidebarTemplate.vue` as a reusable template component
- Replaced PrimeVue Tabs with TextButton components for tab navigation
- Added i18n translation key for "Back to all assets" button
- Improved spacing and layout in job detail view header
- Maintained all existing functionality while cleaning up template
structure

## Test Plan
- [ ] Verify tab switching between Imported and Generated tabs works
correctly
- [ ] Test job detail view displays properly with Job ID and execution
time
- [ ] Confirm "Back to all assets" button returns to main view
- [ ] Check that all existing media asset features remain functional
- [ ] Verify UI consistency with other sidebar tabs


[screen-capture.webm](https://github.com/user-attachments/assets/4ed192e1-a9f7-4fc1-a41e-f732741dd55d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants