feat(attachments): robust photo uploads — retry, per-photo progress, real XHR progress#1525
Draft
brokenalarms wants to merge 5 commits into
Draft
Conversation
The frontend was showing generic toast messages ("Couldn't create item",
"Failed to upload attachment") on API failures, hiding the real reason
the backend already provides in its `ErrorResponse { error, fields? }`
JSON shape.
Add `lib/requests/extract-error.ts` — a pure function that:
- Reads the backend's standard error/fields shape and prefers
per-field messages over the generic top-level error
- Strips Go validator boilerplate, turning
"Field validation for 'Name' failed on the 'required' tag" into
"Name is required"
- Handles legacy `[{ field, error }]` array responses, raw thrown
exceptions (e.g. network failure), and falls back through statusText
and HTTP status as last resorts
- Is fully covered by `extract-error.test.ts` (12 cases)
Apply it to the Create Item modal's item-create error path as the
first concrete consumer: validation failures now show the real reason
("Couldn't create item: Name is required") instead of just "Couldn't
create item". Other call sites will switch to the extractor in
follow-up PRs.
https://claude.ai/code/session_012aZhnQmF8RGDXAMdpKdoka
Co-authored-by: Claude <noreply@anthropic.com>
(cherry picked from commit 919fc0f)
…ge (#3) Mobile attachment uploads were silently failing on the item edit page: thrown `fetch` exceptions (network drop, server read timeout) never reached any error handler, so the dropzone gave no feedback and the user had to refresh to find out whether the upload landed. Server-side errors only ever surfaced as a generic "Failed to upload attachment" toast. Split the fix into two reusable layers: - `lib/requests/retry.ts` — pure `withRetries(call, options)` helper that wraps any `TResponse`-returning call in try/catch, retries transient failures (status 0 from thrown exceptions, 408, 429, 5xx) with configurable backoff, and gives up immediately on 4xx (won't get better). Returns a tagged-union `RetryResult` so callers handle one shape regardless of failure mode. Fully covered by 11 vitest cases (success/retry/give-up paths, backoff schedule, 4xx-no-retry, thrown-error surfacing). - `composables/use-attachment-upload.ts` — thin Vue wrapper exposing `uploadFile`, `uploadExternalLink`, and a reactive `uploading` counter for in-flight UI state. Errors are extracted via the `extract-error` helper added in the parent PR. Apply both to the item edit page's file dropzone and URL drop. The dropzone area now visibly switches to a spinning loader plus "Uploading attachment…" while a request is in flight, and failure toasts include the real reason ("Failed to upload attachment: failed to parse multipart form" instead of just "Failed to upload attachment"). The Create Item modal moves over in the next PR. https://claude.ai/code/session_012aZhnQmF8RGDXAMdpKdoka Co-authored-by: Claude <noreply@anthropic.com> (cherry picked from commit 6e40280)
The Create Item modal had three mobile-upload UX failures: a thrown fetch exception left the submit button stuck disabled forever (loading flag never reset); when a photo upload failed cleanly the item was created with no picture and no way to retry without re-creating it; and during the upload itself the UI gave only a fleeting toast — the disabled button with a static package icon looked dead. Switch the create flow to use the attachment-upload composable and restructure the handler: - The whole body is wrapped in try/catch/finally so loading is always reset and unexpected exceptions surface as a toast with the real message instead of vanishing. - Each photo upload now goes through the composable (retry on network/5xx, real error reason via extractErrorMessage). Per-photo progress is tracked in `uploadProgress` and rendered on the submit button as "Uploading 2 / 5…" — with a spinning MdiLoading icon replacing the static package icon while in flight. - On partial success — item created but one or more photos failed — the failed photos stay in `form.photos` and `pendingItemId` is stashed. The submit button label flips to "Retry photo upload" and the "Create and Add Another" button is hidden in that state. The retry path skips item creation entirely and only re-uploads the remaining photos to the existing item, so there's no risk of duplicate items. - Opening the modal fresh clears `pendingItemId` so stale retry state doesn't carry over between sessions. https://claude.ai/code/session_012aZhnQmF8RGDXAMdpKdoka Co-authored-by: Claude <noreply@anthropic.com> (cherry picked from commit 123b22e)
The previous two PRs added a spinner and a per-photo counter ("Uploading
2 / 5…") to the Create Item modal and the edit-page dropzone, but
neither showed a percentage — because `fetch()` doesn't expose a usable
upload-progress event. On a slow mobile connection the spinner can still
sit for 10+ seconds per photo with no signal of forward motion.
Add an XHR-based transport so call sites can subscribe to real bytes-
uploaded events:
- `lib/requests/xhr-upload.ts` — standalone helper that wraps
`XMLHttpRequest.upload.onprogress`, parses the response the same way
`Requests.do` does, and resolves to a `TResponse<T>`. Rejects only on
transport failure (network / abort / timeout) so callers can read
4xx/5xx via `result.error`. Covered by 8 vitest cases that drive a
fake XHR through every code path.
- `Requests.postWithProgress` — public method that wires the existing
auth token, base URL, and response interceptors into the XHR helper.
Mirrors `post`'s `TResponse<U>` shape so the rest of the stack is
unchanged.
- `AttachmentsAPI.addWithProgress` — same payload as `add`, just routes
through `postWithProgress` and exposes the progress callback. The
multipart-form construction is now extracted into a shared
`buildAttachmentForm` helper used by both `add` and `addWithProgress`.
Wire into the call sites:
- `useAttachmentUpload.uploadFile` takes an optional `onProgress`
callback. If passed, it routes through `addWithProgress` (XHR);
otherwise it stays on `add` (fetch) so consumers that don't care
pay no cost.
- CreateModal extends its `uploadProgress` shape with `percent` and
passes the callback to `uploadFile`. The submit button label becomes
"Uploading 2 / 5 (45%)" with the percentage updating live as bytes
leave the browser.
- The edit-page dropzone tracks a separate `attachmentProgress` ref
and renders "Uploading attachment… (45%)" while the file streams.
The percentage measures bytes leaving the browser, not bytes received
by the server. If the server crashes or times out mid-stream the bar
will reach 100% locally and then the failure toast fires — which the
retry layer can recover from.
https://claude.ai/code/session_012aZhnQmF8RGDXAMdpKdoka
Co-authored-by: Claude <noreply@anthropic.com>
(cherry picked from commit 2075565)
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The frontend CI runs `pnpm lint:ci` which is `eslint ... --max-warnings 1`, and main has accumulated 7 errors plus 2 warnings that block every PR regardless of what it touches. Clear them so the lint job goes green for this PR (and is a no-op once upstream merges any of these in any order). - components/Location/CreateModal.vue: drop unused EntityTypeSummary import. - lib/api/classes/items.ts: drop the `error: any` cast on getLocations' return; TypeScript infers a tighter type from the spread. - pages/collection/index/entity-types.vue: drop the unused `t` from useI18n and the now-unused useI18n import itself. - pages/item/[id]/index/edit.vue, pages/location/[id]/index/edit.vue: drop the dead `locations` computed and the locationStore wiring it dragged in. Nothing on the template reads either. - pages/location/[id]/index/index.vue: drop the dead `refresh` from useAsyncData and the dead locationStore. Replace `class="aspect-square ..."` with `aspect-[1/1]` so the tailwindcss/no-custom-classname rule stops flagging it (the rule's allowlist doesn't include `aspect-square`). - pages/reset-password.vue: collapse a multi-line Button attribute set to one line per the prettier suggestion. No runtime behavior changes — every removal is a value/import that was never read. Verified with `pnpm lint:ci` exit 0. https://claude.ai/code/session_012aZhnQmF8RGDXAMdpKdoka
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
2 participants
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.
What type of PR is this?
What this PR does / why we need it:
Makes attachment/photo uploads resilient and gives users real feedback during upload. Bundled as one cohesive feature because the parts share a common composable and error path.
frontend/lib/requests/extract-error.ts(+ test): reusableextractErrorMessage()that normalizes API/thrown errors into a human-readable string.frontend/lib/requests/retry.ts(+ test): generic retry helper for upload requests, usingextractErrorMessage()to report the failure reason.frontend/composables/use-attachment-upload.ts: shared upload composable with retry, applied to the item edit page (pages/item/[id]/index/edit.vue).frontend/components/Item/CreateModal.vue: retry + per-photo upload progress on item creation.frontend/lib/requests/xhr-upload.ts(+ test) andlib/api/classes/items.ts/lib/requests/requests.ts: real upload progress via XHR (replacing simulated progress) so the progress bar reflects actual bytes sent.Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
This combines four internally-staged changes (error extractor → upload composable+retry → create-item retry/progress → real XHR progress) into a single feature PR, since they build on each other. Happy to split into a stack if you'd prefer to review them separately.
Testing
extract-error.test.ts,retry.test.ts,xhr-upload.test.ts.eslintpasses clean on the changed files.