Skip to content

Commit 79ee662

Browse files
adulbrichclaude
andcommitted
fix(preview): clear the selected image when its file is removed
Removing a file left `selectedImage` pointing at a path the form no longer contains. That was already wrong -- the mask preview kept describing a frame the user had deleted -- but dropping queued RAW conversions made it fail outright: the shared cache entry aborts, so the metadata promise derived from it rejects, and it is memoized on the path. `page.tsx`'s submit handler awaits that promise outside its try block, so pressing Run did nothing at all, with no toast and no recorded attempt, until a different preview image was selected. `lens-mask-input.tsx` reads the same promise with `use()` inside a Suspense with no ErrorBoundary above it. `onRemove` and `onRemoveIndex` now clear the selection when it points at a file being removed, which unmounts the `use()` consumer in the same commit, well before the deferred `AbortError` arrives. `useGenericImageMetadata` also attaches a swallowing handler where it memoizes the promise, so one created before the removal cannot surface as an unhandled rejection afterwards; the original promise is still returned, so a genuine conversion failure keeps rejecting rather than looking like an image with no dimensions. The spec and plan are corrected to the shipped code: `flags: EntryFlags` rather than flat fields, the drop check testing `done` as well as `started`, and the "no new unhandled-rejection surface" claim replaced by what is actually true -- that analysis enumerated two handlers and missed the metadata consumer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 2261aa6 commit 79ee662

6 files changed

Lines changed: 312 additions & 37 deletions

File tree

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
import { describe, expect, it } from "@jest/globals";
2+
import { act, fireEvent, render, screen } from "@testing-library/react";
3+
import { useForm } from "react-hook-form";
4+
5+
declare const jest: typeof import("@jest/globals").jest;
6+
7+
jest.mock("@tauri-apps/plugin-dialog", () => ({
8+
open: () => Promise.resolve(null),
9+
}));
10+
jest.mock("@tauri-apps/plugin-fs", () => ({
11+
readDir: () => Promise.resolve([]),
12+
stat: () => Promise.resolve({ isDirectory: false, isFile: true, size: 1024 }),
13+
}));
14+
15+
import {
16+
SelectedImageProvider,
17+
useSelectedImage,
18+
} from "../src/app/home-page/selected-image-context";
19+
import { ImageMatrixInput } from "../src/components/ui/image-matrix-input";
20+
import type { ImageSet } from "../src/components/ui/image-set-preview";
21+
import { TooltipProvider } from "../src/components/ui/tooltip";
22+
23+
interface FormValues {
24+
inputSets: ImageSet[];
25+
}
26+
27+
// JPEGs rather than RAW files, for the same reason as `image-matrix-lock`: a
28+
// CR2 would drag the TIFF decode worker into a test about which file stays
29+
// selected.
30+
const SCENE1_FIRST = "/photos/scene1/capt01.jpg";
31+
const SCENE1_SECOND = "/photos/scene1/capt02.jpg";
32+
const SCENE2_ONLY = "/photos/scene2/capt03.jpg";
33+
34+
const twoSets: ImageSet[] = [
35+
{ files: [SCENE1_FIRST, SCENE1_SECOND], name: "scene1" },
36+
{ files: [SCENE2_ONLY], name: "scene2" },
37+
];
38+
39+
const NOTHING_SELECTED = "none";
40+
41+
const REMOVE_SCENE1 = /remove image set scene1/i;
42+
const REMOVE_SCENE2 = /remove image set scene2/i;
43+
44+
function SelectionProbe() {
45+
const { selectedImage } = useSelectedImage();
46+
47+
return <p data-testid="selected">{selectedImage ?? NOTHING_SELECTED}</p>;
48+
}
49+
50+
function Harness() {
51+
const { control } = useForm<FormValues>({
52+
defaultValues: { inputSets: twoSets.map((set) => ({ ...set })) },
53+
});
54+
55+
return (
56+
<TooltipProvider>
57+
<SelectedImageProvider>
58+
<ImageMatrixInput control={control} name="inputSets" />
59+
<SelectionProbe />
60+
</SelectedImageProvider>
61+
</TooltipProvider>
62+
);
63+
}
64+
65+
const settle = () => act(() => new Promise((r) => setTimeout(r, 0)));
66+
67+
async function renderPanel() {
68+
// The file statistics resolve through a suspended child, so the first paint
69+
// is awaited rather than taken synchronously.
70+
let view: ReturnType<typeof render> | undefined;
71+
await act(() => {
72+
view = render(<Harness />);
73+
return Promise.resolve();
74+
});
75+
await settle();
76+
if (!view) {
77+
throw new Error("expected the panel to render");
78+
}
79+
80+
return view;
81+
}
82+
83+
function thumbnails(container: HTMLElement): Element[] {
84+
// One per file, in the order the rows render them, which is each set's files
85+
// sorted -- the same order `onRemoveIndex` resolves against.
86+
return Array.from(container.querySelectorAll(".generic-image-container"));
87+
}
88+
89+
function thumbnailAt(container: HTMLElement, index: number): Element {
90+
const thumbnail = thumbnails(container)[index];
91+
if (!thumbnail) {
92+
throw new Error(`expected a thumbnail at ${index}`);
93+
}
94+
95+
return thumbnail;
96+
}
97+
98+
async function selectThumbnail(container: HTMLElement, index: number) {
99+
fireEvent.click(thumbnailAt(container, index));
100+
await settle();
101+
}
102+
103+
async function removeThumbnail(container: HTMLElement, index: number) {
104+
// The context menu's trigger is the thumbnail wrapper, and a contextmenu
105+
// event only bubbles up.
106+
fireEvent.contextMenu(thumbnailAt(container, index));
107+
await settle();
108+
fireEvent.click(screen.getByText("Remove image"));
109+
await settle();
110+
}
111+
112+
function selected(): string {
113+
const probe = screen.getByTestId("selected").textContent;
114+
115+
return probe ?? "";
116+
}
117+
118+
describe("removing the file that is selected", () => {
119+
// Left selected, the mask preview keeps asking for the dimensions of a frame
120+
// the form no longer holds. For a RAW frame that is worse than stale: its
121+
// queued conversion is dropped along with it, so the metadata promise the
122+
// submit handler awaits rejects, and it is memoized on the path -- every
123+
// later run awaits the same permanently rejected promise.
124+
it("clears the selection when its whole set is removed", async () => {
125+
const { container } = await renderPanel();
126+
127+
await selectThumbnail(container, 0);
128+
expect(selected()).toBe(SCENE1_FIRST);
129+
130+
fireEvent.click(screen.getByRole("button", { name: REMOVE_SCENE1 }));
131+
await settle();
132+
133+
expect(selected()).toBe(NOTHING_SELECTED);
134+
});
135+
136+
it("clears the selection when that one frame is removed", async () => {
137+
const { container } = await renderPanel();
138+
139+
await selectThumbnail(container, 0);
140+
expect(selected()).toBe(SCENE1_FIRST);
141+
142+
await removeThumbnail(container, 0);
143+
144+
expect(selected()).toBe(NOTHING_SELECTED);
145+
});
146+
});
147+
148+
describe("removing a file that is not selected", () => {
149+
// The clear has to be as narrow as the removal, or every removal anywhere in
150+
// the panel would empty a preview the user is still working with.
151+
it("keeps the selection when another set is removed", async () => {
152+
const { container } = await renderPanel();
153+
154+
await selectThumbnail(container, 0);
155+
156+
fireEvent.click(screen.getByRole("button", { name: REMOVE_SCENE2 }));
157+
await settle();
158+
159+
expect(selected()).toBe(SCENE1_FIRST);
160+
});
161+
162+
it("keeps the selection when a sibling frame is removed", async () => {
163+
const { container } = await renderPanel();
164+
165+
await selectThumbnail(container, 0);
166+
167+
await removeThumbnail(container, 1);
168+
169+
expect(selected()).toBe(SCENE1_FIRST);
170+
});
171+
});

docs/superpowers/plans/2026-08-01-raw-conversion-cancellation.md

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -295,7 +295,9 @@ EOF
295295
|---|---|---|---|
296296
| Queued, never sent | `false` | `false` | Abort and forget |
297297
| In flight | `true` | `false` | Leave entirely alone |
298-
| Finished | `true` | `true` | Leave entirely alone |
298+
| Finished | `true` or `false` | `true` | Leave entirely alone |
299+
300+
`started` is not guaranteed on a finished frame: a converter may resolve without ever calling `onStart`, which is what #243's OPFS path does when it answers from a cached TIFF instead of converting. So `done` is what recognizes a finished entry, and the drop check has to test both flags rather than `started` alone.
299301

300302
Forgetting an in-flight entry takes it out of the map while its conversion is still running, so re-adding the same set queues a *second* conversion of the same frame — the duplication this module caches the promise, rather than the result, to prevent. Leaning on the existing `catch → forget` instead fails the other way: a queued frame dropped and instantly re-added would hit the surviving entry and inherit its pending `AbortError`, showing a broken thumbnail for a file the user just asked for.
301303

@@ -554,7 +556,11 @@ And the new export:
554556
* because forgetting it would make a re-added set a cache miss and convert
555557
* the same bytes a second time. A finished frame is kept too -- it costs
556558
* nothing the LRU budget does not already govern, and it makes re-adding the
557-
* same file instant.
559+
* same file instant. `flags.done` is what recognizes it: a converter is free
560+
* to resolve without ever calling `onStart` -- #243's OPFS path will do
561+
* exactly that when it answers from a cached TIFF instead of converting --
562+
* and such a converter would otherwise leave `flags.started` false on a
563+
* completed entry, indistinguishable from one still queued.
558564
*
559565
* Paths that were never converted, including every non-RAW one, match no key
560566
* and cost a scan.
@@ -565,7 +571,7 @@ export function dropRawConversions(paths: string[]): void {
565571
if (key !== path && !key.startsWith(`${path}|`)) {
566572
continue;
567573
}
568-
if (entry.flags.started) {
574+
if (entry.flags.started || entry.flags.done) {
569575
continue;
570576
}
571577
entry.controller.abort();
@@ -641,10 +647,11 @@ Add `dropRawConversions` to the file's existing import from `./raw-preview`.
641647

642648
- [ ] **Step 7: Verify the accounting case actually discriminates**
643649

644-
The last case must fail without its guard. Temporarily delete these three lines from `rawToTiff`'s `.then`:
650+
The last case must fail without its guard. Temporarily delete the identity check from `rawToTiff`'s `.then` (keeping whatever the rest of the block needs to still compile):
645651

646652
```ts
647-
if (cache.get(key) !== entry) {
653+
const live = cache.get(key);
654+
if (live?.flags !== flags) {
648655
return;
649656
}
650657
```

docs/superpowers/specs/2026-08-01-raw-conversion-cancellation-design.md

Lines changed: 81 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -154,16 +154,23 @@ inherit its pending `AbortError`, showing a failed thumbnail for a file the
154154
user just asked for. Forget-at-drop is right for queued and wrong for
155155
in-flight, so the two states have to be distinguishable.
156156

157-
`Entry` therefore gains three fields:
157+
`Entry` therefore gains a controller and two flags. The flags live in their own
158+
object rather than directly on the entry, because `convert` has to be handed
159+
something to write `started` into *before* the entry exists -- the entry needs
160+
the promise `convert` returns:
158161

159162
```ts
160-
interface Entry {
161-
bytes: number;
162-
controller: AbortController;
163+
interface EntryFlags {
163164
/** Set once the conversion settles, either way. */
164165
done: boolean;
165166
/** Set when the frame leaves the queue and reaches the worker. */
166167
started: boolean;
168+
}
169+
170+
interface Entry {
171+
bytes: number;
172+
controller: AbortController;
173+
flags: EntryFlags;
167174
tiff: Promise<Uint8Array<ArrayBuffer>>;
168175
}
169176
```
@@ -199,16 +206,26 @@ export function dropRawConversions(paths: string[]): void
199206

200207
For each path it matches cache keys by `key === path || key.startsWith(
201208
`${path}|`)`keys are `path` or `path|fingerprint`and then applies the
202-
table above: an entry that has not `started` is aborted and forgotten;
203-
anything else is left untouched.
209+
table above: an entry that has neither `started` nor `done` is aborted and
210+
forgotten; anything else is left untouched.
211+
212+
Both flags are tested, not `started` alone. A converter is free to resolve
213+
without ever calling `onStart` -- #243's OPFS path will do exactly that when it
214+
answers from a cached TIFF instead of converting -- and such a converter would
215+
leave `started` false on an entry that has already finished its work. Checking
216+
`started` alone would abort and forget that entry, throwing away a conversion
217+
that is complete.
204218

205219
A finished conversion is deliberately kept rather than evicted. It costs
206220
nothing beyond the LRU budget that already governs it, and keeping it makes
207221
re-adding the same file instant instead of a re-queue. Non-RAW paths match no
208222
key, so callers need not filter by extension.
209223

210-
`forget` becomes identity-aware, taking the entry it means to remove and
211-
deleting only if `cache.get(key) === entry`. Without that, a dropped frame's
224+
`forget` becomes identity-aware, taking the flags of the entry it means to
225+
remove and deleting only if `cache.get(key)?.flags === flags`. Identifying by
226+
the flags rather than by the entry lets a conversion's own `catch` call it
227+
without a forward reference to the entry it has not finished building; the two
228+
are one-to-one, so the check is the same. Without that check, a dropped frame's
212229
`AbortError` arriving at the `catch` on `:127` *after* the user has re-added
213230
the file would delete the replacement entry, orphaning a conversion that is
214231
already running and sending the next consumer to a third one.
@@ -245,18 +262,18 @@ The `.then` accounts only if the entry is still the live one for its key.
245262

246263
```ts
247264
.then((data) => {
248-
entry.done = true;
249-
if (cache.get(key) !== entry) {
265+
flags.done = true;
266+
const live = cache.get(key);
267+
if (live?.flags !== flags) {
250268
return; // dropped or evicted while converting
251269
}
252-
entry.bytes = data.byteLength;
270+
live.bytes = data.byteLength;
253271
held += data.byteLength;
254272
evictDownToBudget(key);
255273
})
256274
.catch(() => {
257-
// Already handled at `:127`; this sets `done` on the failure path and
258-
// prevents an unhandled rejection, as it did before.
259-
entry.done = true;
275+
// Handled at `:127`, which is also where `done` is set on the failure
276+
// path; this only prevents an unhandled rejection, as it did before.
260277
})
261278
```
262279

@@ -323,11 +340,46 @@ drop time, and the existing `catch → forget` at `raw-preview.ts:127` covers
323340
the rejection that follows. A frame dropped and then re-added converts afresh
324341
rather than inheriting a rejected promise.
325342

326-
No new unhandled-rejection surface: `raw-preview.ts:137` keeps a handler on
327-
the cached promise itself, and `tiff-image.tsx:50` already catches its derived
328-
promise because an aborted decode was always expected there. The thumbnail
329-
unmounts when its file is removed, so the `AbortError` does not reach the
330-
`ErrorBoundary` at `tiff-image.tsx:69`.
343+
**This section originally claimed there was no new unhandled-rejection
344+
surface, and it was wrong.** It enumerated two handlers -- `raw-preview.ts`'s
345+
own, on the cached promise, and `tiff-image.tsx:50`, on the derived decode
346+
promise -- and concluded that every consumer was covered. It missed a third
347+
consumer that also derives a promise from the cached one:
348+
`generic-image-metadata.ts`'s `getTiffImageMetadata`, which chains
349+
`rawToTiff(...).then(...)` to read the mask preview's dimensions. That promise
350+
had no rejection path at all, and making `AbortError` a routine outcome is
351+
exactly what turned the omission into two user-visible failures:
352+
353+
- The submit handler at `page.tsx:440` awaits that promise outside its own
354+
`try`, and it is memoized on the path -- so once it rejects, pressing Run did
355+
nothing at all, with no toast and no recorded attempt, for as long as the
356+
removed file stayed selected.
357+
- `lens-mask-input.tsx` and `fs-circular-mas-selection.tsx` read it with
358+
`use()` inside a `Suspense` that has no `ErrorBoundary` above it anywhere.
359+
360+
The cause underneath both is not the drop. It is that `selectedImage` was never
361+
cleared when the file it names is removed, so a path the form no longer
362+
contains went on driving the mask preview. What is true after the fix:
363+
364+
- `image-matrix-input.tsx` clears the selection in `onRemove` and
365+
`onRemoveIndex` when it points at a file being removed, so no consumer asks
366+
for the metadata of a removed frame in the first place. The clear is as
367+
narrow as the removal: removing some other set or sibling frame leaves the
368+
selection alone.
369+
- `useGenericImageMetadata` attaches a swallowing handler where it memoizes the
370+
promise, in the same spirit as `tiff-image.tsx:50`, so a metadata promise
371+
created before the removal cannot surface as an unhandled rejection after the
372+
selection has moved on. It returns the original promise, not the handled one:
373+
a genuine conversion failure must still reject rather than look like an image
374+
with no dimensions.
375+
376+
The thumbnail itself unmounts when its file is removed, so the `AbortError`
377+
still does not reach the `ErrorBoundary` at `tiff-image.tsx:69`.
378+
379+
No `ErrorBoundary` was added over the mask preview. One would be worth having
380+
on its own merits, but it addresses neither failure above -- the submit-handler
381+
one never throws during render -- and it is left as separate work rather than
382+
smuggled in as a fix for this.
331383

332384
## Testing
333385

@@ -356,9 +408,16 @@ In `raw-preview.test.ts`:
356408
unit test. The entry point is tests-only, but the guard it exercises is the
357409
same one the eviction path needs, and it fails against today's code.
358410

359-
The `image-matrix-input.tsx` wiring is not unit-tested; the behaviour worth
360-
pinning lives in the two library modules, and the component change is a
361-
two-line call plus the index fix.
411+
The `image-matrix-input.tsx` drop wiring itself is not unit-tested; the
412+
behaviour worth pinning there lives in the two library modules, and the
413+
component change is a two-line call plus the index fix.
414+
415+
The selection clear that removal now performs *is* tested, in
416+
`__tests__/image-matrix-selection-clear.test.tsx`, against the same
417+
RTL-plus-react-hook-form harness `image-matrix-lock.test.tsx` uses. Four cases:
418+
the selection is cleared when its whole set is removed and when that one frame
419+
is removed, and it survives the removal of another set and of a sibling frame.
420+
The last two are what stop an unconditional clear from passing.
362421

363422
## Out of scope
364423

src/app/home-page/selected-image-context.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,15 @@ import { createContext, useContext, useMemo, useState } from "react";
44

55
interface SelectedImageContextValue {
66
selectedImage: string | undefined;
7-
setSelectedImage: (image: string) => void;
7+
/**
8+
* Accepts `undefined` so a caller can clear the selection. Removing the file
9+
* that is selected has to be able to say so: a path the form no longer
10+
* contains must not go on driving the mask preview, whose metadata promise
11+
* would then be resolved against a frame the user threw away -- and, for a
12+
* RAW frame whose queued conversion was dropped with it, never resolve at
13+
* all.
14+
*/
15+
setSelectedImage: (image: string | undefined) => void;
816
}
917

1018
const selectedImageContext = createContext<

0 commit comments

Comments
 (0)