diff --git a/frontend/src/components/admin/AdminPhotoGrid.tsx b/frontend/src/components/admin/AdminPhotoGrid.tsx index 800b3c04..ca009457 100644 --- a/frontend/src/components/admin/AdminPhotoGrid.tsx +++ b/frontend/src/components/admin/AdminPhotoGrid.tsx @@ -43,6 +43,15 @@ export const AdminPhotoGrid: React.FC = ({ const { format: formatDate } = useLocalizedDate(); const queryClient = useQueryClient(); const [selectedPhotos, setSelectedPhotos] = useState>(new Set()); + // Where a shift-click measures its range from: the last tile clicked without + // the shift key (#1212). The index is what a range needs — a span of the + // current ordering — but the id is carried with it so the anchor can prove + // it still points at the tile it was set on. Filtering or re-sorting leaves + // index 5 meaning a different photo, and a range measured from a stale + // anchor selects the wrong span silently, which is worse than not selecting + // at all. Validating at use beats clearing on every list change: a + // background refetch hands back an equal list and the anchor stays good. + const [anchor, setAnchor] = useState<{ index: number; photoId: number } | null>(null); const [isSelectionMode, setIsSelectionMode] = useState(false); const [isDeleting, setIsDeleting] = useState(false); const [deletingPhotos, setDeletingPhotos] = useState>(new Set()); @@ -59,7 +68,7 @@ export const AdminPhotoGrid: React.FC = ({ setPhotoViewMode(mode); }; - const handlePhotoSelect = (photoId: number, e?: React.MouseEvent) => { + const handlePhotoSelect = (photoId: number, e?: React.MouseEvent, index?: number) => { if (e) { e.stopPropagation(); } @@ -68,11 +77,38 @@ export const AdminPhotoGrid: React.FC = ({ setIsSelectionMode(true); } const newSelected = new Set(selectedPhotos); + + // Shift-click selects the span from the last plain click to here (#1212), + // the way every file manager does it. Re-assigning a category across a few + // hundred imported photos is otherwise a few hundred individual clicks. + // + // Extends rather than replaces: the grid already lets you accumulate tiles + // one at a time, so a range is another addition to that set, not a reset + // of it. And it only ever adds — dragging a range back over itself to + // deselect is a different gesture, and guessing at it would make a + // mis-aimed shift-click destroy a selection instead of growing it. + const anchorStillValid = anchor !== null && photos[anchor.index]?.id === anchor.photoId; + if (e?.shiftKey && anchorStillValid && index !== undefined) { + const from = Math.min(anchor.index, index); + const to = Math.max(anchor.index, index); + for (let i = from; i <= to; i++) { + const photo = photos[i]; + if (photo) newSelected.add(photo.id); + } + setSelectedPhotos(newSelected); + onSelectionChange?.(Array.from(newSelected)); + // Anchor deliberately left where it was, so a second shift-click + // re-aims the same range from the original point rather than walking + // the anchor along behind the cursor. + return; + } + if (newSelected.has(photoId)) { newSelected.delete(photoId); } else { newSelected.add(photoId); } + if (index !== undefined) setAnchor({ index, photoId }); setSelectedPhotos(newSelected); onSelectionChange?.(Array.from(newSelected)); }; @@ -81,6 +117,11 @@ export const AdminPhotoGrid: React.FC = ({ let newSelected: Set; if (selectedPhotos.size === photos.length) { newSelected = new Set(); + // Clearing the selection clears what a range would measure from (#1212 + // review). The anchor is invisible, so an anchor that outlived the + // selection made the next shift-click reach back into a session the user + // had already ended and select a range they never started. + setAnchor(null); } else { newSelected = new Set(photos.map(p => p.id)); } @@ -126,6 +167,7 @@ export const AdminPhotoGrid: React.FC = ({ await photosService.deletePhotos(eventId, selectedIds); toast.success(`${count} photo${count > 1 ? 's' : ''} deleted successfully`); setSelectedPhotos(new Set()); + setAnchor(null); setIsSelectionMode(false); onSelectionChange?.([]); onPhotosDeleted(); @@ -151,6 +193,7 @@ export const AdminPhotoGrid: React.FC = ({ setIsSelectionMode(!isSelectionMode); if (isSelectionMode) { setSelectedPhotos(new Set()); + setAnchor(null); onSelectionChange?.([]); } }; @@ -173,6 +216,7 @@ export const AdminPhotoGrid: React.FC = ({ }) ); setSelectedPhotos(new Set()); + setAnchor(null); setIsSelectionMode(false); onSelectionChange?.([]); setIsCategoryModalOpen(false); @@ -336,7 +380,7 @@ export const AdminPhotoGrid: React.FC = ({ className={`absolute top-2 right-2 z-20 transition-opacity ${ selectedPhotos.has(photo.id) ? 'opacity-100' : 'opacity-0 group-hover:opacity-100' }`} - onClick={(e) => handlePhotoSelect(photo.id, e)} + onClick={(e) => handlePhotoSelect(photo.id, e, index)} >
= ({ role="checkbox" aria-checked={isSelected} data-testid={`admin-photo-row-checkbox-${photo.id}`} - onClick={(e) => handlePhotoSelect(photo.id, e)} + onClick={(e) => handlePhotoSelect(photo.id, e, index)} >
{ + const actual = await vi.importActual('react-i18next'); + return { + ...actual, + useTranslation: () => ({ + t: (_key: string, fallback?: any) => (typeof fallback === 'string' ? fallback : _key), + i18n: { language: 'en' } + }) + }; +}); + +vi.mock('../AdminAuthenticatedImage', () => ({ + AdminAuthenticatedImage: ({ alt }: { alt: string }) => {alt} +})); + +vi.mock('../../../services/photos.service', () => ({ + photosService: { formatBytes: (n: number) => `${n} B` } +})); + +vi.mock('../PermissionGate', () => ({ + PermissionGate: ({ children }: { children: ReactNode }) => <>{children} +})); + +const photo = (id: number): AdminPhoto => ({ + id, filename: `p${id}.jpg`, path: `/p${id}.jpg`, url: `/p${id}.jpg`, + thumbnail_url: `/t/p${id}.jpg`, type: 'photo', + category_id: null, category_name: null, category_slug: null, + size: 1000 + id, uploaded_at: '2026-01-01T00:00:00Z' +} as AdminPhoto); + +const renderWithQueryClient = (ui: ReactElement) => { + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return render({ui}); +}; + +const renderGrid = (photos: AdminPhoto[]) => { + const onSelectionChange = vi.fn(); + const view = renderWithQueryClient( + + ); + return { onSelectionChange, view }; +}; + +const FIVE = [1, 2, 3, 4, 5].map(photo); + +const selectedIds = (onSelectionChange: ReturnType) => + [...(onSelectionChange.mock.calls.at(-1)?.[0] ?? [])].sort((a: number, b: number) => a - b); + +const checkbox = (id: number) => screen.getByTestId(`admin-photo-checkbox-${id}`); + +describe('shift-click range selection (#1212)', () => { + it('selects everything between the anchor and the shift-clicked tile', async () => { + const user = userEvent.setup(); + const { onSelectionChange } = renderGrid(FIVE); + + await user.click(checkbox(2)); + await user.keyboard('{Shift>}'); + await user.click(checkbox(4)); + await user.keyboard('{/Shift}'); + + expect(selectedIds(onSelectionChange)).toEqual([2, 3, 4]); + }); + + it('works when the range is drawn backwards', async () => { + const user = userEvent.setup(); + const { onSelectionChange } = renderGrid(FIVE); + + await user.click(checkbox(4)); + await user.keyboard('{Shift>}'); + await user.click(checkbox(2)); + await user.keyboard('{/Shift}'); + + expect(selectedIds(onSelectionChange)).toEqual([2, 3, 4]); + }); + + it('extends an existing selection rather than replacing it', async () => { + const user = userEvent.setup(); + const { onSelectionChange } = renderGrid(FIVE); + + await user.click(checkbox(1)); + await user.click(checkbox(3)); + await user.keyboard('{Shift>}'); + await user.click(checkbox(5)); + await user.keyboard('{/Shift}'); + + // 1 was picked before the range and stays picked. + expect(selectedIds(onSelectionChange)).toEqual([1, 3, 4, 5]); + }); + + it('re-aims from the original anchor on a second shift-click', async () => { + const user = userEvent.setup(); + const { onSelectionChange } = renderGrid(FIVE); + + await user.click(checkbox(1)); + await user.keyboard('{Shift>}'); + await user.click(checkbox(4)); + await user.click(checkbox(2)); + await user.keyboard('{/Shift}'); + + // Still measured from 1. The anchor does not walk along behind the cursor, + // so the second click narrows the intent rather than starting a new span + // at 4 — though what it already added stays added. + expect(selectedIds(onSelectionChange)).toEqual([1, 2, 3, 4]); + }); + + it('is a plain toggle when there is no anchor yet', async () => { + const user = userEvent.setup(); + const { onSelectionChange } = renderGrid(FIVE); + + await user.keyboard('{Shift>}'); + await user.click(checkbox(3)); + await user.keyboard('{/Shift}'); + + expect(selectedIds(onSelectionChange)).toEqual([3]); + }); + + it('leaves a plain click toggling one tile', async () => { + const user = userEvent.setup(); + const { onSelectionChange } = renderGrid(FIVE); + + await user.click(checkbox(2)); + await user.click(checkbox(4)); + expect(selectedIds(onSelectionChange)).toEqual([2, 4]); + + await user.click(checkbox(2)); + expect(selectedIds(onSelectionChange)).toEqual([4]); + }); + + it('forgets the anchor when the selection is cleared', async () => { + // The anchor is invisible. Left behind by Deselect All, it made the next + // shift-click reach back into a selection session the user had already + // ended — selecting a range they never started (#1212 review). + const user = userEvent.setup(); + const { onSelectionChange } = renderGrid(FIVE); + + await user.click(checkbox(1)); + // Select All then again to deselect — with one tile picked the button is + // still "Select All", so a single click would select rather than clear. + await user.click(screen.getByRole('button', { name: /select all/i })); + await user.click(screen.getByRole('button', { name: /deselect all/i })); + expect(selectedIds(onSelectionChange)).toEqual([]); + + await user.keyboard('{Shift>}'); + await user.click(checkbox(4)); + await user.keyboard('{/Shift}'); + + // A plain toggle of the tile that was clicked, not 1..4. + expect(selectedIds(onSelectionChange)).toEqual([4]); + }); + + it('refuses to measure a range from an anchor the list has moved under', async () => { + const user = userEvent.setup(); + const { onSelectionChange, view } = renderGrid(FIVE); + + // Anchor on the tile at index 1. + await user.click(checkbox(2)); + + // The list is re-filtered: index 1 is now a different photo entirely. + view.rerender( + + + + ); + + await user.keyboard('{Shift>}'); + await user.click(checkbox(9)); + await user.keyboard('{/Shift}'); + + // Falls back to a plain toggle. Selecting one tile the user pointed at is + // recoverable; silently selecting 7 and 8 as well is not. + expect(selectedIds(onSelectionChange)).toEqual([2, 9]); + }); +});