feat: add AttachmentCard component with generic async upload lifecycle#142
feat: add AttachmentCard component with generic async upload lifecycle#142
Conversation
Introduce a reusable AttachmentCard API with image/file previews, menu actions, and optional async upload handling so products can implement upload transport externally while getting consistent loading and error UI. Made-with: Cursor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /** Called when an async upload fails. */ | ||
| onUploadError?: (ctx: { file: File; error: Error }) => void; | ||
| /** Optional retry handler used by consumers for error recovery strategies. */ | ||
| onRetryUpload?: (item: AttachmentItem) => Promise<AttachmentItem>; |
There was a problem hiding this comment.
[1/4 — High] onRetryUpload is a dead public API prop
onRetryUpload is declared, exported, and documented, but it is never destructured from props or called anywhere in AttachmentCard.tsx. Consumers who pass this prop expecting retry UX will silently get nothing.
Either:
- Wire it up in the error-handling path in
handleUpload(replace thesetLocalItemsremoval with a status update to"error"and invokeonRetryUploadon the error tile), or - Remove it entirely from the type and docs before this ships.
Shipping a documented, exported prop that does nothing is a footgun — consumers will trust the docs, write retry handlers, and debug why they're never called.
| /** List of attachments to render. */ | ||
| items?: AttachmentItem[]; | ||
| /** Called when files are selected or dropped. */ | ||
| onUpload?: (files: File[]) => void; |
There was a problem hiding this comment.
[2/4 — Medium] onUpload is silently swallowed when uploadFile is provided
When both onUpload and uploadFile are passed, onUpload is silently ignored (lines 92–94 in AttachmentCard.tsx). There is no runtime warning and no TypeScript constraint that prevents this combination.
The docs table labels onUpload as "Legacy controlled upload callback" — but this is brand-new code and "legacy" is misleading. The mutual-exclusion contract should be enforced one of these ways:
- Enforce with types — use a discriminated union props type so
uploadFileandonUploadcannot be passed together:type AttachmentCardProps = BaseProps & ({ onUpload: (files: File[]) => void; uploadFile?: never } | { uploadFile: (file: File) => Promise(AttachmentItem); onUpload?: never });
- Document clearly in JSDoc — at minimum, note that
onUploadis ignored whenuploadFileis present.
Without this, consumers who pass both during a migration will lose all upload events from the onUpload path with no indication of why.
| } | ||
|
|
||
| const temporaryItems = files.map(createTemporaryUploadItem); | ||
| setLocalItems((prev) => [...temporaryItems.map((entry) => entry.item), ...prev]); |
There was a problem hiding this comment.
[3/4 — Medium] Object URL leak on component unmount during active upload
createTemporaryUploadItem creates a blob: URL via URL.createObjectURL for image previews. The finally block in handleUpload correctly revokes it after resolution — but only if the Promise settles while the component is still mounted. If the component unmounts mid-upload (e.g., user navigates away), the async Promise.all continues running in the background, setLocalItems calls trigger React warnings, and critically the finally block still runs — but there's nothing preventing further issues from state updates on the unmounted component.
The larger risk is if uploadFile never settles (e.g., hung request) — the blob URL is held indefinitely.
Fix: add an isMounted guard or AbortController signal, and revoke all pending object URLs on cleanup:
React.useEffect(() => {
return () => {
// Revoke any blob URLs still held by uploading items
setLocalItems((prev) => {
prev.filter(i => i.status === "uploading" && i.previewUrl?.startsWith("blob:"))
.forEach(i => URL.revokeObjectURL(i.previewUrl!));
return prev;
});
};
}, []);| /** Optional preview URL for image attachments. */ | ||
| previewUrl?: string; | ||
| /** Lifecycle status used for upload rendering. */ | ||
| status?: "ready" | "uploading" | "error"; |
There was a problem hiding this comment.
[4/4 — Low] status: "error" and errorMessage are declared but never rendered
AttachmentItem.status has a "error" variant and errorMessage is documented as "Optional upload error message for UI/telemetry", but the component only checks for status === "uploading". Items with status: "error" render identically to "ready" items — no error overlay, no visual indicator, no use of errorMessage.
This creates two problems:
- Consumers who set
status: "error"on items initems[]will see normal tiles, not error tiles. - The
errorMessagefield misleads consumers into thinking it surfaces somewhere in the UI.
If error-state rendering is not planned, remove "error" from the status union and drop errorMessage from the public type before shipping. Unexplained fields in a public interface are a long-term maintenance cost.
Remove dead public upload fields, enforce mutually exclusive upload modes at the type level, and harden async object URL lifecycle cleanup to avoid unmount-related leaks while keeping behavior docs and tests in sync. Made-with: Cursor
commit: |
|
/review |
1 participant
Introduce a reusable AttachmentCard API with image/file previews, menu actions, and optional async upload handling so products can implement upload transport externally while getting consistent loading and error UI.