fix: direct-upload on drop + URL-upload bug (#750 issue 1 completion)#97
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
UX team clarification: the drop-a-file-to-upload flow must reduce user
steps. Rapid-batch add-images-to-many-items should not require an extra
review/confirm click per item, and certainly should not flash a modal
dialog for each upload. The cropper stays in the edit-existing flow
where the user is already in an edit mindset.
Two changes land together:
1. ItemCardEditor direct-upload on drop
- handleDropZoneInput calls onUpload (file) or onUploadFromUrl (url)
directly — no dialog, no cropper, no confirm click.
- Drop-zone slot becomes a state machine with three visible states:
default drop zone, uploading (role="status" spinner + "Uploading
image..."), error (role="alert" message + "Try again" button).
- Edit-existing flow is untouched: the "Click to edit/replace"
hover overlay still opens the dialog in EditExisting phase.
- New props: onUploadFromUrl (required when URL inputs are possible),
onUploadError (optional, for hosts that also want a toast).
- No more pendingInput wiring from ItemCardEditor. The pendingInput
prop on ImageUploadDialog remains for consumers that still want
the crop-first-upload UX.
2. ImageUploadDialog empty-blob URL upload bug
- Previously the Uploading effect coerced URL inputs to `new Blob([])`
and called onUpload with 0 bytes — producing a silent 0-byte CDN
object. Silent data corruption.
- Now the effect routes string imageData through the new
onUploadFromUrl handler. If the host hasn't supplied one, the
dialog dispatches UPLOAD_ERROR with a clear "URL upload not
supported" message instead of silently uploading empty bytes.
- File/Blob inputs continue to use the existing onUpload path
unchanged.
Tests
- 9 new ItemCardEditor tests covering direct-upload success/failure/try-
again, missing onUpload/onUploadFromUrl graceful errors, spinner
rendering, error-input ignore, and unchanged edit-existing flow.
- 3 new ImageUploadDialog tests covering the URL-input upload paths
(success via onUploadFromUrl, UPLOAD_ERROR when missing, file-input
remains on onUpload).
- Obsolete "drop routes through dialog" tests (the short-lived 4.11.5
shape of issue 1) removed.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jmpicnic
force-pushed
the
jmpicnic/image-upload-feedback
branch
from
07e13c5 to
35ced9a
Compare
There was a problem hiding this comment.
Pull request overview
Updates the image upload flow to support direct inline uploads from ItemCardEditor’s empty-state drop zone (no dialog/cropper/confirm), and fixes a data-corruption bug where URL inputs in ImageUploadDialog were previously uploaded as an empty blob.
Changes:
- Add
onUploadFromUrlsupport and route URL uploads through it inImageUploadDialog(fail loudly if missing). - Change
ItemCardEditorempty-state drop zone to upload immediately with inline uploading/error UI, while keeping the edit-existing dialog flow. - Add/adjust unit tests for both the new URL-upload path and the direct-upload-on-drop behavior; update changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/canary/organisms/shared/image-upload-dialog/image-upload-dialog.tsx | Adds onUploadFromUrl and fixes URL upload handling to avoid 0-byte uploads. |
| src/components/canary/organisms/shared/image-upload-dialog/image-upload-dialog.test.tsx | Adds tests covering URL upload routing and missing-handler error behavior. |
| src/components/canary/organisms/item-card-editor/item-card-editor.tsx | Implements direct-upload-on-drop with spinner/error states; forwards new URL handler to dialog. |
| src/components/canary/organisms/item-card-editor/item-card-editor.test.tsx | Updates tests to validate direct upload behavior, errors, and edit-existing dialog unchanged. |
| CHANGELOG.md | Adds 4.11.6 entry describing the fixes/behavior changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1. Copilot #3 + CI build-and-test fix: ItemCardEditor's new direct-upload flow broke Storybook play functions that render the editor without an onUpload handler (e.g. image-display.stories.tsx and external-url. stories.tsx). Default onUpload to defaultUploadHandler and onUploadFromUrl to a new defaultUrlUploadHandler — matches the pattern ImageUploadDialog already used. Stubs return mock picsum.photos URLs after a 1.5s simulated upload. Production consumers pass real handlers. 2. Copilot #1: ImageUploadDialog's "URL upload without handler" error previously surfaced a technical message in the UploadError UI. Shortened to "URL upload not supported" for the user-facing dispatch; detail logged via console.error for ops/devs. 3. Copilot #2: Updated JSDoc on ItemCardEditor's onUploadFromUrl prop to match the actual behavior (falls back to defaultUrlUploadHandler, not "ignored"). Tests - Replaced the two obsolete "not configured" tests with tests asserting the new default fallback behavior (drop-file and drop-url both commit a picsum.photos URL without a host-supplied handler). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous commit addressed the file-drop Storybook failure but I missed that the same fix needs to apply to the dialog itself. The Storybook test runner was still failing on two stories (image-display.stories.tsx and external-url.stories.tsx) because the dialog's "URL upload without handler" path fired a console.error that the runner caught as a test failure: error: ImageUploadDialog URL upload requested without an onUploadFromUrl handler. Make the dialog parallel to ItemCardEditor: default onUploadFromUrl to the new defaultUrlUploadHandler stub. This eliminates the console.error path entirely — the defensive "URL upload not supported" dispatch is now unreachable code, so I removed it. Production consumers must pass a real handler; the stub's mock URL is a visible bug flag if missed. Tests - Added defaultUrlUploadHandler to the vi.mock block in image-upload-dialog.test.tsx (43 tests were failing to import). - Rewrote the "URL input without onUploadFromUrl" test to assert the new stub fallback (returns a picsum.photos mock URL) instead of the removed UPLOAD_ERROR dispatch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The image-display.stories.tsx EditFlowAutomated play function was
asserting /arda-uploaded/i in the final CDN URL — that seed came from
defaultUploadHandler (File-blob path). Before 4.11.6, URL inputs
silently uploaded an empty blob via defaultUploadHandler, which is why
the assertion matched.
With the empty-blob bug fixed in 4.11.6, URL inputs now route through
the new defaultUrlUploadHandler which returns a distinct seed
("arda-from-url-<len>"). Update the story's assertion to match the
prefix defaultUrlUploadHandler emits.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Completes Arda-cards/arda-frontend-app#750 issue 1 per UX team clarification, and fixes a silent data-corruption bug that was surfaced during the design review.
1.
ItemCardEditordirect-upload on dropThe rapid-batch add-images-to-many-items flow must not require a cropper review + confirm per item. The card's empty-state drop zone now uploads inline — no dialog, no cropper, no confirm click.
handleDropZoneInputcallsonUpload(for file) or newonUploadFromUrlprop (for URL) directly.role="status"spinner), error (role="alert"+ "Try again").EditExistingphase, which is the deliberate edit-mindset interaction where the cropper still lives.onUploadFromUrl?: (url: string) => Promise<string>,onUploadError?: (err: Error) => void.2.
ImageUploadDialogempty-blob URL upload bugPreviously the
Uploadingeffect coerced URL inputs tonew Blob([])and calledonUploadwith 0 bytes — producing a silent 0-byte CDN object. Silent data corruption surfaced only when someone later tried to render the image.The effect now routes string
imageDatathrough the newonUploadFromUrlprop. Missing handler →UPLOAD_ERRORwith "URL upload not supported" instead of a silent 0-byte upload.Test plan
ItemCardEditortests (direct-upload success/failure/try-again, missing-handler graceful errors, spinner visibility, edit-existing flow unchanged)ImageUploadDialogtests (URL →onUploadFromUrl, URL without handler →UPLOAD_ERROR, File input still usesonUpload)4.11.6entry under### Fixed🤖 Generated with Claude Code