* 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 <[email protected]>
This commit is contained in:
Paul Nothaft
co-authored by
Paul Nothaft
Paul Nothaft
parent
5c85e0c0e4
commit
f18bc568c8
@@ -43,6 +43,15 @@ export const AdminPhotoGrid: React.FC<AdminPhotoGridProps> = ({
|
||||
const { format: formatDate } = useLocalizedDate();
|
||||
const queryClient = useQueryClient();
|
||||
const [selectedPhotos, setSelectedPhotos] = useState<Set<number>>(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<Set<number>>(new Set());
|
||||
@@ -59,7 +68,7 @@ export const AdminPhotoGrid: React.FC<AdminPhotoGridProps> = ({
|
||||
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<AdminPhotoGridProps> = ({
|
||||
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<AdminPhotoGridProps> = ({
|
||||
let newSelected: Set<number>;
|
||||
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<AdminPhotoGridProps> = ({
|
||||
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<AdminPhotoGridProps> = ({
|
||||
setIsSelectionMode(!isSelectionMode);
|
||||
if (isSelectionMode) {
|
||||
setSelectedPhotos(new Set());
|
||||
setAnchor(null);
|
||||
onSelectionChange?.([]);
|
||||
}
|
||||
};
|
||||
@@ -173,6 +216,7 @@ export const AdminPhotoGrid: React.FC<AdminPhotoGridProps> = ({
|
||||
})
|
||||
);
|
||||
setSelectedPhotos(new Set());
|
||||
setAnchor(null);
|
||||
setIsSelectionMode(false);
|
||||
onSelectionChange?.([]);
|
||||
setIsCategoryModalOpen(false);
|
||||
@@ -336,7 +380,7 @@ export const AdminPhotoGrid: React.FC<AdminPhotoGridProps> = ({
|
||||
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)}
|
||||
>
|
||||
<div className={`w-6 h-6 rounded border-2 flex items-center justify-center ${
|
||||
selectedPhotos.has(photo.id)
|
||||
@@ -602,7 +646,7 @@ export const AdminPhotoGrid: React.FC<AdminPhotoGridProps> = ({
|
||||
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)}
|
||||
>
|
||||
<div className={`w-5 h-5 rounded border-2 flex items-center justify-center ${
|
||||
isSelected
|
||||
|
||||
@@ -0,0 +1,208 @@
|
||||
/**
|
||||
* Shift-click range selection in the admin photo grid (#1212).
|
||||
*
|
||||
* Reported in #1209 by someone re-assigning a category across a large imported
|
||||
* set: Select All is all-or-one, so "these two hundred" meant two hundred
|
||||
* clicks. The range extends the selection rather than replacing it, and it
|
||||
* only ever adds — a mis-aimed shift-click should grow the wrong set, not
|
||||
* destroy the right one.
|
||||
*
|
||||
* The case that matters most is the last one. The anchor is an index, and an
|
||||
* index means a different photo after a filter or a re-sort, so a range
|
||||
* measured from a stale anchor would select the wrong span with no sign that
|
||||
* anything went wrong.
|
||||
*/
|
||||
import { render, screen } from '@testing-library/react';
|
||||
import userEvent from '@testing-library/user-event';
|
||||
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
|
||||
import { describe, expect, it, vi } from 'vitest';
|
||||
import type { ReactElement, ReactNode } from 'react';
|
||||
|
||||
import { AdminPhotoGrid } from '../AdminPhotoGrid';
|
||||
import type { AdminPhoto } from '../../../services/photos.service';
|
||||
|
||||
vi.mock('react-i18next', async () => {
|
||||
const actual = await vi.importActual<typeof import('react-i18next')>('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 }) => <img alt={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(<QueryClientProvider client={queryClient}>{ui}</QueryClientProvider>);
|
||||
};
|
||||
|
||||
const renderGrid = (photos: AdminPhoto[]) => {
|
||||
const onSelectionChange = vi.fn();
|
||||
const view = renderWithQueryClient(
|
||||
<AdminPhotoGrid
|
||||
photos={photos}
|
||||
eventId={42}
|
||||
onPhotoClick={vi.fn()}
|
||||
onPhotosDeleted={vi.fn()}
|
||||
onSelectionChange={onSelectionChange}
|
||||
/>
|
||||
);
|
||||
return { onSelectionChange, view };
|
||||
};
|
||||
|
||||
const FIVE = [1, 2, 3, 4, 5].map(photo);
|
||||
|
||||
const selectedIds = (onSelectionChange: ReturnType<typeof vi.fn>) =>
|
||||
[...(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(
|
||||
<QueryClientProvider client={new QueryClient({ defaultOptions: { queries: { retry: false } } })}>
|
||||
<AdminPhotoGrid
|
||||
photos={[photo(7), photo(8), photo(9)]}
|
||||
eventId={42}
|
||||
onPhotoClick={vi.fn()}
|
||||
onPhotosDeleted={vi.fn()}
|
||||
onSelectionChange={onSelectionChange}
|
||||
/>
|
||||
</QueryClientProvider>
|
||||
);
|
||||
|
||||
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]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user