feat: Add multi-select support for media assets #6256
Merged
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
🎭 Playwright Test Results⏰ Completed at: 10/29/2025, 07:21:47 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
viva-jinyi
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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: Add multi-select support for media assets (#6256)
Impact: 291 additions, 42 deletions across 5 files
Issue Distribution
- Critical: 1
- High: 2
- Medium: 3
- Low: 1
Category Breakdown
- Architecture: 2 issues
- Security: 1 issue
- Performance: 2 issues
- Code Quality: 3 issues
Key Findings
Architecture & Design
The multi-select implementation follows Vue 3 Composition API patterns well with proper separation of concerns through a dedicated Pinia store and composable. However, there are concerns about state management consistency between folder and normal views, and potential memory management issues with Set operations without proper cleanup lifecycle management.
Security Considerations
Input validation is missing for asset IDs and array indices, which could potentially allow out-of-bounds access or injection attacks in edge cases. Asset metadata operations need validation before use in selection logic.
Performance Impact
The range selection implementation has potential O(n) performance issues for large asset lists (1000+ items), which could cause UI freezing during shift-click operations. Additionally, reactive Set operations may cause excessive re-rendering without proper memoization.
Integration Points
The feature integrates well with existing media asset infrastructure and properly handles keyboard modifiers. However, the TODO placeholders for download/delete functionality should be addressed with proper implementation or issue tracking.
Positive Observations
- Excellent use of TypeScript with proper type definitions
- Good separation of concerns with dedicated store and composable
- Proper use of VueUse for keyboard modifier detection
- Clean i18n integration with appropriate translation keys
- Follows existing component patterns and architecture
- Comprehensive selection interactions (single, range, toggle)
References
Next Steps
- Address critical performance issue in range selection before merge
- Add input validation for asset operations
- Implement proper cleanup lifecycle management
- Consider accessibility improvements for keyboard navigation
- Replace TODO comments with proper implementation or issue tracking
- Add comprehensive unit tests for selection logic edge cases
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
requested review from
a team,
KarryCharon,
Yorha4D,
arjansingh and
shinshin86
as code owners
dosubot
bot
added
the
size:L
DrJKL
force-pushed
the
feature/media-asset-sidebar-tab
branch
from
d578518 to
208e522
Compare
viva-jinyi
force-pushed
the
feature/media-asset-sidebar-tab
branch
2 times, most recently
from
5780728 to
b778cde
Compare
dosubot
bot
added
size:XL
viva-jinyi
force-pushed
the
feature/asset-select
branch
from
9c64c8f to
5a494cb
Compare
dosubot
bot
added
size:L
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/29/2025, 07:02:59 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.31 MB (baseline 3.31 MB) • 🔴 +539 BMain entry bundles and manifests
Status: 2 added / 2 removed Graph Workspace — 716 kB (baseline 716 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 8.15 kB (baseline 8.15 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 294 kB (baseline 294 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 12.3 kB (baseline 12.3 kB) • ⚪ 0 BReusable component library chunks
Status: 1 added / 1 removed Data & Services — 10 kB (baseline 10 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 1 added / 1 removed Utilities & Hooks — 1.07 kB (baseline 1.07 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 5.36 MB (baseline 5.36 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 2.55 MB (baseline 2.55 MB) • ⚪ 0 BBundles that do not match a named category
|
dosubot
bot
added
size:XL
Member
Author
🎯 Multi-Select Improvements
🔒 Folder View Restrictions
⌨️ Keyboard Event Optimization
|
- Add multi-select interactions with Shift/Ctrl(Cmd) keys - Normal click: Single selection - Shift + click: Range selection - Ctrl/Cmd + click: Individual multi-selection - Create assetSelectionStore for managing selected assets - Encapsulate selection logic in useAssetSelection composable - Add selection count display and bulk action buttons in footer - Shows "Deselect all" on hover - Download/Delete buttons (actual implementation pending)
- Performance: Use batch operations for range selection instead of forEach - Memory: Add reset function to clear selection on component unmount - Security: Add input validation for asset operations - Accessibility: Add keyboard support for selection count interaction - Optimization: Prevent unnecessary re-renders in Set operations
- Clear isHoveringSelectionCount when deselecting all assets - Prevents 'Deselect all' text from staying visible after clearing selection - Improves UX by returning to normal selection count display
…a assets - Add bulk download/delete for multiple selected assets - Hide delete buttons in folder view - Activate keyboard shortcuts only when sidebar is open - Add activate/deactivate lifecycle methods to useAssetSelection
viva-jinyi
force-pushed
the
feature/asset-select
branch
from
de2695f to
f2ae87e
Compare
christian-byrne
approved these changes
Oct 29, 2025
christian-byrne
added a commit
that referenced
this pull request
Oct 30, 2025
Add missing English translation keys that were added in recent releases (PR #6256, #6112, #6187) but not backported to rh-test: - Media asset management (delete, selection, job ID toast) - Asset browser aria labels - Sidebar labels (console, menu, assets, imported, generated) - File management strings (no files found messages) - Resize handle tooltips - Basic UI strings (edit/delete image, chart, file, etc.) These will be translated in the next commit.
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 file explorer-style multi-selection functionality for media assets in the AssetsSidebarTab component.
Changes
Multi-Selection Interactions
State Management
assetSelectionStoreto manage selected asset IDs using SetuseAssetSelectioncomposable for selection logic and keyboard stateUI Enhancements
Translation Keys
Added new keys under
mediaAsset.selection:selectedCount: "{count} selected"deselectAll: "Deselect all"downloadSelected: "Download"deleteSelected: "Delete"Test Plan
Notes
screen-capture.webm