Single modal image upload and confirm navigation

[benjaminleonard]

The image upload flow has long been need of updating. I can excuse the poor UX, but ugliness?!

This replaces that with a single modal that has two states. It's still possible to return back to the form from the upload state, like before. It also adds a notice whilst cleanup is happening (though note a pre-existing issue #1788).

Before:

After:

---

Now for the nav guard block modal when a user leaves the form. The UX I think is fine, it's just the visual treatment that is awkward. The relationship between the regular modal and the side one. Here's my alternative proposal:

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
console	Ready	Preview	Jun 9, 2026 10:14pm

[david-crespo]

Working on this to close #1859, which Jatinder ran into on colo.

[david-crespo]

Did that in #3227 and rebased this on main to incorporate it.

[benjaminleonard]

Need to make sure the text here is not overflowing

Do you have a good way to test these errors locally? My usual method of messing with the MSW handler is a bit awkward.

[david-crespo]

That screenshot is from an e2e test. Look at the test changes in #3227.

[david-crespo]

Pretty good review. Will have to go through it.

🤖 review by Fable 5 in CC

Review of #3222. The single-modal design is a clear improvement, the ConfirmModal portal approach is clean, and the e2e tests are better than what they replace. But there's one confirmed broken behavior and a couple of real races in the cancel/retry paths.

1. Nav guard on the upload form never fires (confirmed empirically)

handleDismiss reads form.formState.isDirty inside the event handler (app/forms/image-upload.tsx:577). react-hook-form's formState is a Proxy that only tracks properties read during render — this exact gotcha is documented in this repo at app/components/form/SideModalForm.tsx:80-81 ("inlining form.formState.isDirty does not work"). Since isDirty is never read in render, it stays false forever.

I verified with a throwaway e2e test: fill the Name field, press Escape — the form closes with no "Leave form?" guard. Data loss with no warning. Fix: destructure const { isDirty, isSubmitting } = form.formState at render level, like SideModalForm does.

Related: the test comment in image-upload.e2e.ts ("react-hook-form treats post-submit state as clean, so this skips the nav guard") is a wrong rationalization of this bug — RHF does not clear isDirty on submit. Once fixed, that test will start hitting the nav guard after cancel-upload → dismiss, and the author needs to decide whether that's the desired flow (probably yes, since dismissing discards the kept form values).

2. Esc bypasses the "can no longer be canceled" guard

The footer "Cancel upload" button is correctly disabled once createImage is pending/succeeded (cancelDisabledReason), but handleDismiss (Esc, X, scrim) unconditionally opens the cancel guard on the running path (image-upload.tsx:595). Confirming during that window aborts at the post-createImage checkpoint: cleanup deletes the snapshot and disk, but the image stays — while the guard text says "All progress will be lost." The user then resubmits and gets "Image name already exists."

There's also a stale-state tail: if the abort lands after the last throwIfAborted (during the final deletes), setAllDone(true) fires after backToForm has already reset to the form phase, so a resubmit opens the progress phase with the Done button enabled immediately.

Suggested fix: early-return in handleDismiss when cancelDisabledReason is set, and also auto-close the cancel guard when createImage starts (the existing effect only closes it on allDone/modalError — a user can open the guard early and sit on it past the point of no return).

3. Retry race during error cleanup

modalError is set before cleanup() runs, so the "Retry from here" / "Back to form" footer appears while cleanup (several sequential API calls) is still in flight. Clicking Retry immediately:

• cleanup() reads disk.current after awaits, so the old run's cleanup can delete the new run's temp disk mid-upload.
• The old run's finally { abortController.current = null } (image-upload.tsx:510) clobbers the new run's controller, making the new upload uncancelable (all throwIfAborted calls become no-ops via ?.).

Also, the "Cleaning up…" spinner only renders in the form-phase footer; the error footer shows nothing during cleanup. Disabling both error-footer buttons while isCleaningUp (with the spinner shown there too) would fix the race and the missing feedback at once.

Smaller points

• "Retry from here" is misleading — it resets everything and restarts from disk creation. "Retry upload" or just "Retry" would be accurate.
• Stale-progress regression: the old code reset state at submit time, with a comment explaining why — after a cancel, in-flight chunk completions can set uploadProgress after backToForm's reset, so a resubmit can briefly show the previous run's progress. Narrow window, minor, but the deleted comment described exactly this.
• bytesToGiB(file.size) in SummaryHeader shows "0 GiB" for files under ~5 MB and forces GiB for everything. formatBytes picks the right unit and is already imported.
• SideModal.Body border change (SideModal.tsx:129-140): removing useIsOverflow makes the top border always-on for every side modal in the app, not just this one. Presumably intentional from the author, but worth confirming it was meant as a global change.
• ConfirmModal exit animations are dead code — the component returns null when closed with no AnimatePresence, so exit={{ opacity: 0 }} never runs.
• Nits: Step has a leftover <div className=""> with a now-stale "padding on icon" comment; duplicate aria-label="Upload progress" on both the steps container and the Progress bar; ConfirmModal's focus-out comment references the native confirm() dialog that this PR removes.

What's good

The phase-swap-in-one-modal structure, keeping the form mounted (hidden) so values survive cancel, focus management (focus the first step on phase change, restore on return), the guard auto-dismiss on terminal states, aria-current="step", and replacing native confirm() with a proper dialog all read well. The deleted scrim/focus-out regression tests are legitimately moot since there's no longer a stacked document-level modal.

Items 1–3 are worth blocking on; 1 is a straight bug with a one-line fix (plus one test-behavior decision).