Skip to content

Commit ced3db7

Browse files
adulbrichclaude
andauthored
Drop RAW conversions the user no longer wants (#248), and remove the frame they actually clicked (#251) (#254)
* docs: design dropping RAW conversions the user no longer wants (#248) Settles what "cancel" means before anything is built: a frame not yet handed to the worker is never handed to it, and a frame already converting runs to completion. Records why the other two definitions #248 offered were rejected. Corrects the assumption #248 closed with. It expected the persistent tier to make dropping queued frames pointless, on the grounds that a queued frame might be a millisecond cache hit. The lookup lives inside the worker, so it is only reached after the frame has waited its whole turn -- a cache hit still sits behind eight 1.9 s conversions. Resolving hits before they queue is a real improvement, and is scoped out to its own issue because it reworks the boundary #243 just established. Also records two pre-existing bugs found while reading the path, both of which this work makes reachable and so fixes rather than defers: `held` accounting leaks whenever an entry is forgotten while still pending, which dropping turns from rare into routine; and `onRemoveIndex` applies an index from the sorted file list to the unsorted one, so removing an image deletes the wrong frame. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: distinguish a queued frame from one already converting The first draft had two states where there are three. Dropping treated "pending" as one case and both aborted and forgot it, which is right for a frame still in the queue and wrong for one already converting: aborting is a no-op by then, but forgetting takes the entry out of the map while its conversion runs, so re-adding the same set queues a second conversion of the same frame. That is the duplication `rawToTiff` 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 immediately re-added would inherit the pending AbortError and show a failed thumbnail for a file the user just asked for. The two states have to be distinguishable, so the seam carries an `onStart` callback back from the queue and `forget` becomes identity-aware. Also: remove by file identity rather than writing the sorted array back, so fixing the index bug does not silently normalise stored order as well; name `workerTiffFor` as the default that has to forward the options; and add the test that discriminates the three-state model. Both follow-ups the spec defers now exist: #251 for the wrong-file removal, #252 for cache hits waiting behind the queue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: plan dropping RAW conversions in three tasks One task per module, each ending green and committable: the queue check and `onStart` in the worker client, the three entry states and `dropRawConversions` in the cache, then the UI wiring with the index fix it depends on. Also corrects the spec's reason for fixing the `held` accounting leak. The first draft said dropping made forgetting a pending entry routine, which was true of the two-state design but not of the three-state one that replaced it: only frames that never started are forgotten, and those reject rather than resolving, so dropping cannot reach the leak at all. What reaches it is the flow the feature serves -- two brackets in play at once, 673 MB each against a 768 MB budget, so eviction lands on a frame still converting. Same fix, honest reason, and a test that drives it through `clearRawPreviewCache` because the eviction path needs 768 MB of real allocation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: hold the entry flags in their own object Pre-flight scan of the plan found it mandating a cast a reviewer would rightly flag. `convert` needs somewhere to record that a frame has started before the entry exists, because the entry needs the promise `convert` returns -- so the plan reached for `undefined as unknown as Promise<...>` and assigned `tiff` afterwards. Putting the two mutable flags in their own object removes the circularity instead of casting around it, and `forget` identifying an entry by that object rather than by the entry drops a forward reference to `entry` from inside its own `catch`. Same three states, no cast, nothing for a reviewer to trip over. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(raw): let the queue skip a frame nobody wants any more `convertRawInWorker` takes an optional signal, checked once at the front of the queue rather than when the call was made: the work worth skipping is the work that has been sitting in it. A skipped frame still occupies and settles its link, so ordering and the one-frame-at-a-time invariant are untouched, and `send()` is never entered -- no worker message, no interaction with `abandon`, no termination. `onStart` fires immediately after that check, because the cache on the other side of the seam cannot otherwise tell a queued frame from one already converting, and it must: forgetting the second kind would let a re-added set convert the same frame twice. Refs #248 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: correct the plan's stale test baseline The 58-suite figure counted the #250 branch's own suite, which is not on this branch. True baseline is 57 suites / 384 tests, and the suite count must not be derived from --listTests, whose output picks up shell-completion noise on this machine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(raw): give up on conversions the user has removed `dropRawConversions(paths)` abandons frames still waiting in the queue. A frame is one of three things, and the middle one is handled opposite to the first: queued frames are aborted and forgotten, frames already converting are left entirely alone, and finished frames are kept. Forgetting an in-flight frame would take its entry out of the map while its conversion ran, so re-adding the same set would miss and convert the same bytes twice -- the duplication this module caches the promise rather than the result to prevent. Relying instead on the existing `catch -> forget` fails the other way round: a queued frame dropped and immediately re-added would find the entry still there and inherit its pending AbortError, showing a failed thumbnail for a file the user had just asked for. Telling the two apart is what `onStart` is for. `forget` now checks identity before deleting, so a dropped frame's AbortError arriving a turn later cannot delete the replacement entry a re-add created in the meantime. `dropRawConversions` keeps an entry when either `flags.started` or `flags.done` is set, not `started` alone: a converter that resolves without ever calling `onStart` -- `countingIo` in the tests -- otherwise leaves a finished entry indistinguishable from a queued one, and a completed conversion gets aborted and forgotten for no reason. Also fixes a pre-existing accounting leak this feature makes reachable. Both `forget` and `evictDownToBudget` subtract `entry.bytes`, which is 0 while a conversion is pending, so an entry that left the map before resolving added to `held` and never subtracted -- `held` drifting up for the rest of the session and the 768 MB budget evicting ever more eagerly. It needs two brackets in play at once, 673 MB each, which is precisely the swap-one-set- for-another flow this feature exists to serve. Refs #248 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(raw): pin the drop-and-instantly-re-add case, and fix two comment drifts Review found the drop-cancellation implementation correct but the test surface incomplete: no test exercised re-adding a frame in the same turn it was dropped, which is the only way to observe `forget`'s identity check and `dropRawConversions`'s eager `forget` actually doing their jobs -- the existing "forgets a frame dropped before it started" test awaits the rejection first, giving the ordinary catch -> forget path time to clean up before the re-add ever runs. Adds one test that drops and re-adds with no await between, confirmed by mutation to fail both ways: deleting `dropRawConversions`'s `forget` call lets the re-added promise inherit the dropped entry's AbortError, and reverting `forget`'s identity check lets the dropped entry's late, post-rejection forget delete the re-added entry's cache slot, surfacing as a third conversion of bytes already held. Also corrects two doc comments that had drifted from the code they describe: `tiffFor`'s claim that a converter skipping `onStart` leaves frames droppable "for their whole life" (false since the `started || done` widening -- droppable only until they finish), and `dropRawConversions`'s docstring naming the test fixture `countingIo` where it should state the underlying contract instead. Refs #248 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(raw): drop conversions when the user removes the frames Wires `dropRawConversions` into the two places that already know the user changed their mind. Removing a set two frames in no longer makes a newly added set wait about 15 s for frames nobody wants. Fixes the index those handlers removed by, which had to change for the wiring to be correct at all. The preview is handed a sorted copy and reports an index into it; `onRemoveIndex` applied that index to the unsorted stored array, so once `onAdd` appended a file that sorted earlier, removing an image deleted the wrong one -- and dropping the conversion for one frame while the form removed another would have left the cache and the UI disagreeing about which frame was gone. The index is now resolved against the array the user saw, and the file removed by identity so the stored order is left alone. Closes #248 Closes #251 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * 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> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent eafc0f4 commit ced3db7

10 files changed

Lines changed: 1904 additions & 32 deletions
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+
});

0 commit comments

Comments
 (0)