From 711c30fb008c2155d97336b3e448a07f69a8b9d6 Mon Sep 17 00:00:00 2001 From: Chouffe Date: Thu, 13 Aug 2026 18:50:27 +0200 Subject: [PATCH 1/3] feat(localize): leave the editor once the accepted boxes land MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Confirming the editor's "Accept boxes" popover settles every remaining frame of the object, so there is nothing left to do on that screen — but the editor stayed open, and the annotator had to press Escape to get back to a cockpit that had already moved on without them. The confirm paths (Enter and the popover's own button) now hand the page a callback, and the page fires it from the mutation's onSuccess. Closing on the write rather than on the gesture is what keeps a failed accept where it belongs: the editor stays open with its error toast still about the object in front of you. The close runs through the editor's own requestClose, so it shrinks back into the grid cell exactly as Escape does. The rail's copy of the same popover passes no callback — there is no editor to close there — and is untouched. --- .../localize/editor/LocalizeObjectEditor.tsx | 12 ++++-- frontend/src/pages/LocalizeAlertPage.tsx | 8 +++- .../editor/LocalizeObjectEditor.test.tsx | 43 +++++++++++++++++++ 3 files changed, 59 insertions(+), 4 deletions(-) diff --git a/frontend/src/components/localize/editor/LocalizeObjectEditor.tsx b/frontend/src/components/localize/editor/LocalizeObjectEditor.tsx index 60f6066e..fdf65538 100644 --- a/frontend/src/components/localize/editor/LocalizeObjectEditor.tsx +++ b/frontend/src/components/localize/editor/LocalizeObjectEditor.tsx @@ -113,8 +113,14 @@ export interface LocalizeObjectEditorProps { /** * Commit the winning model box on every frame of this object that has * none. Never overwrites a frame the annotator already decided. + * + * `onAccepted` fires once the write has landed, and the editor passes its + * own close in: accepting the remainder settles every frame, so there is + * nothing left to do here. Waiting for the write rather than closing on the + * gesture is what keeps a failed accept on screen, where its error toast is + * still about the object in front of you. */ - onAcceptRemaining: () => void; + onAcceptRemaining: (onAccepted?: () => void) => void; /** Hand this object's classification back to the classify cockpit. */ onReclassify: () => void; onClose: () => void; @@ -587,7 +593,7 @@ export function LocalizeObjectEditor({ ) return; if (!isAccepting) { - onAcceptRemaining(); + onAcceptRemaining(requestClose); setAcceptOpen(false); } } else { @@ -764,7 +770,7 @@ export function LocalizeObjectEditor({ gapCount={gapCount} isAccepting={isAccepting} onConfirm={() => { - onAcceptRemaining(); + onAcceptRemaining(requestClose); setAcceptOpen(false); }} onCancel={() => setAcceptOpen(false)} diff --git a/frontend/src/pages/LocalizeAlertPage.tsx b/frontend/src/pages/LocalizeAlertPage.tsx index 9df0b8ee..3bdc7c51 100644 --- a/frontend/src/pages/LocalizeAlertPage.tsx +++ b/frontend/src/pages/LocalizeAlertPage.tsx @@ -2297,7 +2297,13 @@ export default function LocalizeAlertPage({ mode }: LocalizeAlertPageProps = {}) onCommit={handleEditorCommit} onCommitGapFrame={handleEditorCommitGapFrame} onUnmaterialize={handleEditorUnmaterialize} - onAcceptRemaining={() => quickAcceptLane.mutate(modalContext.laneId)} + // The editor asks to be closed once the boxes are actually written, + // and only then: a failed accept keeps it open, with its own error + // toast still about the object on screen. The rail's copy of the + // same action passes no callback — there is no editor to close. + onAcceptRemaining={onAccepted => + quickAcceptLane.mutate(modalContext.laneId, { onSuccess: onAccepted }) + } onReclassify={() => handleReclassify(modalContext.laneId)} onNavigateToDetection={navigateModalTo} onClose={closeModal} diff --git a/frontend/tests/components/localize/editor/LocalizeObjectEditor.test.tsx b/frontend/tests/components/localize/editor/LocalizeObjectEditor.test.tsx index 9ad5e52d..9c678764 100644 --- a/frontend/tests/components/localize/editor/LocalizeObjectEditor.test.tsx +++ b/frontend/tests/components/localize/editor/LocalizeObjectEditor.test.tsx @@ -965,6 +965,49 @@ describe('LocalizeObjectEditor accept remaining', () => { expect(onCommit).not.toHaveBeenCalled(); }); + it('leaves the editor once the accepted boxes are written', () => { + // Accepting the remainder settles every frame of the object, so there is + // nothing left to do here — the editor hands the page a callback and the + // page fires it when the write lands. + const onAcceptRemaining = vi.fn(); + const onClose = vi.fn(); + renderEditor({ onAcceptRemaining, onClose }); + fireEvent.click(screen.getByTestId('editor-accept-remaining')); + + fireEvent.keyDown(window, { key: 'Enter' }); + + expect(onClose).not.toHaveBeenCalled(); + onAcceptRemaining.mock.calls[0][0](); + expect(onClose).toHaveBeenCalled(); + }); + + it('leaves the editor when confirmed by click too', () => { + const onAcceptRemaining = vi.fn(); + const onClose = vi.fn(); + renderEditor({ onAcceptRemaining, onClose }); + fireEvent.click(screen.getByTestId('editor-accept-remaining')); + + fireEvent.click(screen.getByTestId('accept-remaining-confirm')); + + onAcceptRemaining.mock.calls[0][0](); + expect(onClose).toHaveBeenCalled(); + }); + + it('stays put when the write never lands', () => { + // A failed accept leaves the object unfinished and a failure toast on + // screen — closing on the keypress alone would carry the annotator away + // from work that did not happen. + const onAcceptRemaining = vi.fn(); + const onClose = vi.fn(); + renderEditor({ onAcceptRemaining, onClose }); + fireEvent.click(screen.getByTestId('editor-accept-remaining')); + + fireEvent.keyDown(window, { key: 'Enter' }); + + expect(onAcceptRemaining).toHaveBeenCalledTimes(1); + expect(onClose).not.toHaveBeenCalled(); + }); + it('warns about frames no model found smoke on, without blocking', () => { // One frame has candidates, the other has none at all. renderEditor({ From 9a9b1cc984dab335d11146c938327510b385575f Mon Sep 17 00:00:00 2001 From: Chouffe Date: Thu, 13 Aug 2026 18:54:09 +0200 Subject: [PATCH 2/3] fix(localize): ignore a close asked for after the editor is gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Handing `requestClose` to the page made it reachable long after the gesture that armed it: the write can land once the annotator has already left by another door — browser Back, which keeps the cockpit page and its mutation mounted. The close then ran on an unmounted editor and navigated them back to it. `mountedRef` already guards the exit animation's own finish handler for the same reason; this takes it one step earlier, to the whole request. --- .../localize/editor/LocalizeObjectEditor.tsx | 4 ++++ .../editor/LocalizeObjectEditor.test.tsx | 16 ++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/frontend/src/components/localize/editor/LocalizeObjectEditor.tsx b/frontend/src/components/localize/editor/LocalizeObjectEditor.tsx index fdf65538..d2a80f8e 100644 --- a/frontend/src/components/localize/editor/LocalizeObjectEditor.tsx +++ b/frontend/src/components/localize/editor/LocalizeObjectEditor.tsx @@ -520,6 +520,10 @@ export function LocalizeObjectEditor({ const isClosingRef = useRef(false); const requestClose = useCallback(() => { if (isClosingRef.current) return; + // The accept handler hands this to the page, which fires it whenever the + // write lands — possibly after the annotator has already left by another + // door. Closing something that is gone would navigate them back to it. + if (!mountedRef.current) return; const root = rootRef.current; if (!root || typeof root.animate !== 'function') { onClose(); diff --git a/frontend/tests/components/localize/editor/LocalizeObjectEditor.test.tsx b/frontend/tests/components/localize/editor/LocalizeObjectEditor.test.tsx index 9c678764..5fac1ac1 100644 --- a/frontend/tests/components/localize/editor/LocalizeObjectEditor.test.tsx +++ b/frontend/tests/components/localize/editor/LocalizeObjectEditor.test.tsx @@ -1008,6 +1008,22 @@ describe('LocalizeObjectEditor accept remaining', () => { expect(onClose).not.toHaveBeenCalled(); }); + it('does not navigate when the write lands after the editor is gone', () => { + // Accept, then leave before it returns — browser Back, say, which keeps + // the cockpit page (and its mutation) mounted. The late callback must not + // drag the annotator back to wherever the editor had been. + const onAcceptRemaining = vi.fn(); + const onClose = vi.fn(); + const { unmount } = renderEditor({ onAcceptRemaining, onClose }); + fireEvent.click(screen.getByTestId('editor-accept-remaining')); + fireEvent.keyDown(window, { key: 'Enter' }); + + unmount(); + onAcceptRemaining.mock.calls[0][0](); + + expect(onClose).not.toHaveBeenCalled(); + }); + it('warns about frames no model found smoke on, without blocking', () => { // One frame has candidates, the other has none at all. renderEditor({ From 7c43b1418cf23ce871813d88f751c576e631b5a9 Mon Sep 17 00:00:00 2001 From: Chouffe Date: Thu, 13 Aug 2026 19:04:47 +0200 Subject: [PATCH 3/3] fix(localize): close into the frame the annotator is on, not the one they accepted from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things review turned up in the accept-close wiring. The close handed to the page was frozen at gesture time: `requestClose` closes over the open frame, and nothing stops an ArrowRight while the bulk upsert is in flight. The write would land and the editor shrink into the cell of a frame already left — possibly one scrolled out of view, so it flew off toward nothing. A ref resolves the callback to the current close instead, the same trick `clearRef` uses to keep the keyboard handler from re-binding. And "stays put when the write never lands" tested nothing: the failure semantics live in the page's `{ onSuccess }`, so an editor-level test could only re-assert what the test above it already pins. Swapping that for `onSettled` — which would close on a FAILED accept and bury the error toast — left the whole suite green. The guard now sits where the behavior does, in a page test that rejects the bulk write and asserts the editor is still on screen; its sibling covers the success close. Both were watched failing against that mutant first. --- .../localize/editor/LocalizeObjectEditor.tsx | 15 ++++++- .../editor/LocalizeObjectEditor.test.tsx | 41 +++++++++++------- .../tests/pages/LocalizeAlertPage.test.tsx | 42 +++++++++++++++++++ 3 files changed, 82 insertions(+), 16 deletions(-) diff --git a/frontend/src/components/localize/editor/LocalizeObjectEditor.tsx b/frontend/src/components/localize/editor/LocalizeObjectEditor.tsx index d2a80f8e..6f164138 100644 --- a/frontend/src/components/localize/editor/LocalizeObjectEditor.tsx +++ b/frontend/src/components/localize/editor/LocalizeObjectEditor.tsx @@ -564,6 +564,16 @@ export function LocalizeObjectEditor({ animation.addEventListener('cancel', done); }, [onClose, frameCellRect, peeked, detection]); + // What the accept hands the page. The page fires it whenever the write + // lands, and nothing stops the annotator arrowing on meanwhile — so it must + // resolve to the CURRENT `requestClose`, whose target cell is the frame + // they are on now. The frozen one would shrink into a frame they already + // left. Same ref trick `clearRef` uses to keep the keyboard handler from + // re-binding. + const requestCloseRef = useRef(requestClose); + requestCloseRef.current = requestClose; + const requestLatestClose = useCallback(() => requestCloseRef.current(), []); + // --- Keyboard ----------------------------------------------------------- useEffect(() => { @@ -597,7 +607,7 @@ export function LocalizeObjectEditor({ ) return; if (!isAccepting) { - onAcceptRemaining(requestClose); + onAcceptRemaining(requestLatestClose); setAcceptOpen(false); } } else { @@ -669,6 +679,7 @@ export function LocalizeObjectEditor({ shortcutsOpen, resetStageZoom, requestClose, + requestLatestClose, editable, isAccepting, onAcceptRemaining, @@ -774,7 +785,7 @@ export function LocalizeObjectEditor({ gapCount={gapCount} isAccepting={isAccepting} onConfirm={() => { - onAcceptRemaining(requestClose); + onAcceptRemaining(requestLatestClose); setAcceptOpen(false); }} onCancel={() => setAcceptOpen(false)} diff --git a/frontend/tests/components/localize/editor/LocalizeObjectEditor.test.tsx b/frontend/tests/components/localize/editor/LocalizeObjectEditor.test.tsx index 5fac1ac1..a22ac4ee 100644 --- a/frontend/tests/components/localize/editor/LocalizeObjectEditor.test.tsx +++ b/frontend/tests/components/localize/editor/LocalizeObjectEditor.test.tsx @@ -993,20 +993,11 @@ describe('LocalizeObjectEditor accept remaining', () => { expect(onClose).toHaveBeenCalled(); }); - it('stays put when the write never lands', () => { - // A failed accept leaves the object unfinished and a failure toast on - // screen — closing on the keypress alone would carry the annotator away - // from work that did not happen. - const onAcceptRemaining = vi.fn(); - const onClose = vi.fn(); - renderEditor({ onAcceptRemaining, onClose }); - fireEvent.click(screen.getByTestId('editor-accept-remaining')); - - fireEvent.keyDown(window, { key: 'Enter' }); - - expect(onAcceptRemaining).toHaveBeenCalledTimes(1); - expect(onClose).not.toHaveBeenCalled(); - }); + // Whether a FAILED write leaves the editor open is the page's half of this + // contract — it decides when to fire the callback — and is covered where it + // lives, in LocalizeAlertPage's "keeps the editor open when the write + // fails". An editor-level version could only re-assert that it does not + // close on the keypress, which the test above already pins. it('does not navigate when the write lands after the editor is gone', () => { // Accept, then leave before it returns — browser Back, say, which keeps @@ -1547,6 +1538,28 @@ describe('open/close transition', () => { expect(onClose).toHaveBeenCalledTimes(1); }); + it('shrinks into the frame it closes from, not the one the accept started on', () => { + // The accept's close is fired by the page whenever the write lands, and + // nothing stops the annotator arrowing on meanwhile. A close frozen at + // gesture time would fly into the cell of a frame they already left — + // possibly one scrolled out of view. + const { animate } = makeAnimateMock(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (Element.prototype as any).animate = animate; + const onAcceptRemaining = vi.fn(); + const frameCellRect = vi.fn(() => ({ left: 5, top: 6, width: 100, height: 60 })); + const { rerender } = renderEditor({ onAcceptRemaining, frameCellRect }); + + fireEvent.click(screen.getByTestId('editor-accept-remaining')); + fireEvent.keyDown(window, { key: 'Enter' }); + + // Write in flight; the annotator steps on to the next frame. + rerender(editorWith({ onAcceptRemaining, frameCellRect, detection: lastDetection })); + onAcceptRemaining.mock.calls[0][0](); + + expect(frameCellRect).toHaveBeenLastCalledWith(lastDetection.recorded_at); + }); + it('closes immediately when element.animate is unavailable', () => { const onClose = vi.fn(); renderEditor({ onClose, frameCellRect: () => ({ left: 0, top: 0, width: 1, height: 1 }) }); diff --git a/frontend/tests/pages/LocalizeAlertPage.test.tsx b/frontend/tests/pages/LocalizeAlertPage.test.tsx index ef2761cd..d644eb8c 100644 --- a/frontend/tests/pages/LocalizeAlertPage.test.tsx +++ b/frontend/tests/pages/LocalizeAlertPage.test.tsx @@ -101,6 +101,7 @@ vi.mock('@/components/localize/editor', () => ({ onCommit: (detection: Detection, items: unknown[]) => void; onCommitGapFrame?: (recordedAt: string, items: unknown[]) => void; onUnmaterialize?: (detection: Detection) => void; + onAcceptRemaining: (onAccepted?: () => void) => void; objectOverlays?: Array<{ color: string; label: string; boxes: unknown[] }>; }) => (
@@ -166,6 +167,13 @@ vi.mock('@/components/localize/editor', () => ({ + {/* The real editor's accept hands its own close in, for the page to + fire once the write lands — so the stand-in does the same. Whether + that callback runs on success only is the page's business, and this + button is what lets a test hold it to it. */} +
), })); @@ -2478,6 +2486,40 @@ describe('LocalizeAlertPage', () => { return await screen.findByTestId('accept-remaining-popover'); }; + // The same accept, reached from inside the editor. What the page owes the + // editor there is a close — but only once the boxes are actually written, + // which is the half no editor-level test can see. + describe('from the editor', () => { + const editorWrapper = makeWrapper('/localize/101/object/102/1002'); + + it('closes the editor once the boxes are written', async () => { + await renderAndSettle(, { wrapper: editorWrapper }); + expect(screen.getByTestId('image-modal')).toBeInTheDocument(); + + fireEvent.click(screen.getByRole('button', { name: 'Mock Accept Remaining' })); + + await waitFor(() => { + expect(screen.queryByTestId('image-modal')).not.toBeInTheDocument(); + }); + // Back to the object's own page, object still selected. + expect(screen.getByTestId('location')).toHaveTextContent( + /^\/localize\/101\/object\/102$/ + ); + }); + + it('keeps the editor open when the write fails, with the failure in view', async () => { + vi.mocked(apiClient.bulkUpsertDetectionAnnotations).mockRejectedValue(new Error('boom')); + await renderAndSettle(, { wrapper: editorWrapper }); + + fireEvent.click(screen.getByRole('button', { name: 'Mock Accept Remaining' })); + + expect(await screen.findByText(/failed to accept boxes/i)).toBeInTheDocument(); + // Closing here would carry the annotator away from work that did not + // happen, and bury the toast on a page that is no longer about it. + expect(screen.getByTestId('image-modal')).toBeInTheDocument(); + }); + }); + it('opens the popover instead of accepting immediately, previewing the active lane', async () => { await renderAndSettle(, { wrapper });