From f18bc568c88dabcd305595f365838ab6470b975f Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Fri, 28 Aug 2026 08:27:46 +0200 Subject: [PATCH] feat(admin): shift-click range selection in the photo grid (#1212) (#1213) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(admin): shift-click range selection in the photo grid (#1212) Selecting photos was one tile at a time. Select All is all-or-one, so 're-assign these two hundred' meant two hundred clicks — which is how #1209 ran into it, re-categorising a large imported set. Shift-click now selects the span from the last plain click to the tile under the cursor, the way a file manager does. It extends the selection rather than replacing it: the grid already lets you accumulate tiles one at a time, so a range is another addition to that set. And it only ever adds — deselecting by dragging a range back over itself is a different gesture, and guessing at it would let a mis-aimed shift-click destroy a selection instead of growing it. The anchor stays put across repeated shift-clicks, so the second one re-aims the same span from the original point instead of walking along behind the cursor. The anchor carries the id of the tile it was set on, not just the index. An index means a different photo after a filter or a re-sort, and a range measured from a stale anchor would select the wrong span with nothing to show for it; the write checks the anchor still points where it was set and falls back to a plain toggle when it does not. Validating at use rather than clearing on every list change means a background refetch, which hands back an equal list, leaves the anchor usable. Seven tests, four of which fail without the change; the other three pin the plain-click and no-anchor behaviour that must not move. * fix(admin): clear the range anchor whenever the selection is cleared (#1212) External review. Cancel Selection, Deselect All and a successful bulk move or delete all emptied selectedPhotos and left the anchor behind. The anchor is invisible, and the list is usually unchanged, so it stayed valid — the next shift-click reached back into a selection session the user had already ended and selected a range they never started. Cleared at all four reset points now. Test fails against the un-fixed code. --------- Co-authored-by: Paul Nothaft --- .../src/components/admin/AdminPhotoGrid.tsx | 50 ++++- .../AdminPhotoGrid.rangeSelect.test.tsx | 208 ++++++++++++++++++ 2 files changed, 255 insertions(+), 3 deletions(-) create mode 100644 frontend/src/components/admin/__tests__/AdminPhotoGrid.rangeSelect.test.tsx 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]); + }); +});