Skip to content

Commit d293028

Browse files
committed
fix: Address code review feedback for multi-select
- 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
1 parent 7499e4c commit d293028

File tree

3 files changed

+42
-8
lines changed

3 files changed

+42
-8
lines changed

src/components/sidebar/tabs/AssetsSidebarTab.vue

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,13 @@
100100
/>
101101
<span
102102
v-else
103-
class="cursor-pointer px-3 text-sm"
103+
role="button"
104+
tabindex="0"
105+
:aria-label="$t('mediaAsset.selection.deselectAll')"
106+
class="cursor-pointer px-3 text-sm focus:ring-2 focus:ring-primary focus:outline-none"
104107
@mouseenter="isHoveringSelectionCount = true"
108+
@keydown.enter="clearSelection"
109+
@keydown.space.prevent="clearSelection"
105110
>
106111
{{
107112
$t('mediaAsset.selection.selectedCount', { count: selectedCount })
@@ -142,7 +147,7 @@
142147
<script setup lang="ts">
143148
import ProgressSpinner from 'primevue/progressspinner'
144149
import { useToast } from 'primevue/usetoast'
145-
import { computed, ref, watch } from 'vue'
150+
import { computed, onUnmounted, ref, watch } from 'vue'
146151
147152
import IconTextButton from '@/components/button/IconTextButton.vue'
148153
import TextButton from '@/components/button/TextButton.vue'
@@ -196,7 +201,8 @@ const {
196201
hasSelection,
197202
selectedCount,
198203
clearSelection,
199-
getSelectedAssets
204+
getSelectedAssets,
205+
reset: resetSelection
200206
} = useAssetSelection()
201207
202208
// Asset actions - will be used for individual assets later
@@ -341,6 +347,11 @@ const exitFolderView = () => {
341347
clearSelection()
342348
}
343349
350+
// Clean up selection when component unmounts
351+
onUnmounted(() => {
352+
resetSelection()
353+
})
354+
344355
const copyJobId = async () => {
345356
if (folderPromptId.value) {
346357
try {

src/platform/assets/composables/useAssetSelection.ts

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,18 +24,26 @@ export function useAssetSelection() {
2424
index: number,
2525
allAssets: AssetItem[]
2626
) {
27+
// Input validation
28+
if (!asset?.id || index < 0 || index >= allAssets.length) {
29+
console.warn('Invalid asset selection parameters')
30+
return
31+
}
32+
2733
const assetId = asset.id
2834

2935
// Shift + Click: Range selection
3036
if (shiftKey.value && selectionStore.lastSelectedIndex >= 0) {
3137
const start = Math.min(selectionStore.lastSelectedIndex, index)
3238
const end = Math.max(selectionStore.lastSelectedIndex, index)
3339

34-
// Get IDs of assets in range
40+
// Batch operation for better performance
3541
const rangeIds = allAssets.slice(start, end + 1).map((a) => a.id)
42+
const existingIds = Array.from(selectionStore.selectedAssetIds)
43+
const combinedIds = [...new Set([...existingIds, ...rangeIds])]
3644

37-
// Add range to selection (keep existing selections)
38-
rangeIds.forEach((id) => selectionStore.addToSelection(id))
45+
// Single update instead of multiple forEach operations
46+
selectionStore.setSelection(combinedIds)
3947

4048
// Don't update lastSelectedIndex for shift selection
4149
return
@@ -84,6 +92,7 @@ export function useAssetSelection() {
8492
selectAll,
8593
clearSelection: () => selectionStore.clearSelection(),
8694
getSelectedAssets,
95+
reset: () => selectionStore.reset(),
8796

8897
// Key states (for UI feedback)
8998
shiftKey,

src/platform/assets/composables/useAssetSelectionStore.ts

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,14 @@ export const useAssetSelectionStore = defineStore('assetSelection', () => {
2121
}
2222

2323
function setSelection(assetIds: string[]) {
24-
selectedAssetIds.value = new Set(assetIds)
24+
// Only update if there's an actual change to prevent unnecessary re-renders
25+
const newSet = new Set(assetIds)
26+
if (
27+
newSet.size !== selectedAssetIds.value.size ||
28+
!assetIds.every((id) => selectedAssetIds.value.has(id))
29+
) {
30+
selectedAssetIds.value = newSet
31+
}
2532
}
2633

2734
function clearSelection() {
@@ -45,6 +52,12 @@ export const useAssetSelectionStore = defineStore('assetSelection', () => {
4552
lastSelectedIndex.value = index
4653
}
4754

55+
// Reset function for cleanup
56+
function reset() {
57+
selectedAssetIds.value.clear()
58+
lastSelectedIndex.value = -1
59+
}
60+
4861
return {
4962
// State
5063
selectedAssetIds: computed(() => selectedAssetIds.value),
@@ -62,6 +75,7 @@ export const useAssetSelectionStore = defineStore('assetSelection', () => {
6275
clearSelection,
6376
toggleSelection,
6477
isSelected,
65-
setLastSelectedIndex
78+
setLastSelectedIndex,
79+
reset
6680
}
6781
})

0 commit comments

Comments
 (0)