fix(web): allow retrying failed attachment uploads - #1722
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
- [Major] Retrying a draft-restored failure reuses an ID that removal has already cancelled — restored
Fileobjects retain their attachment ID (web/src/lib/composer-attachment-drafts.ts:60,web/src/lib/attachmentAdapter.ts:52), while removal permanently adds that ID tocancelledAttachmentIds(web/src/lib/attachmentAdapter.ts:186). Re-adding the sameFileatweb/src/components/AssistantChat/AttachmentItem.tsx:151therefore reuses the cancelled ID and exits atattachmentAdapter.ts:91before the upload.
Suggested fix:const retryFile = new File([file], file.name, { type: file.type, lastModified: file.lastModified, }) await attachmentRuntime.remove() await composer.addAttachment(retryFile)
- [Minor] The truncation observer is not installed when a previewed image transitions into the error layout — the first render returns the preview branch with
filenameRef.current === null; because the effect depends only onname, the unchanged filename does not trigger it whenisErrorlater mounts the span (web/src/components/AssistantChat/AttachmentItem.tsx:134).
Suggested fix:}, [name, isError])
- [Minor] Oversized files receive a retry control even though every retry is guaranteed to fail — the adapter maps
file.size > MAX_UPLOAD_BYTESto the same incomplete/error state (web/src/lib/attachmentAdapter.ts:95), and the newshowRetrycondition exposes retry for all such states (web/src/components/AssistantChat/AttachmentItem.tsx:118).
Suggested fix:import { MAX_UPLOAD_BYTES } from '@/lib/attachmentAdapter' const isRetryableError = isError && file.size <= MAX_UPLOAD_BYTES const showRetry = isRetryableError && !isParking
Summary
- Review mode: initial
- One major correctness issue blocks retry for restored failed drafts; two minor gaps affect deterministic oversize failures and error-layout spacing.
Testing
- Not run (automation/security constraint: PR code was not executed). Add an adapter-integrated retry test using a pathless draft-restored
File, a render transition test from preview/running to incomplete/error with a long filename, and an oversized-file retry visibility test.
HAPI Bot
| setIsRetrying(true) | ||
| try { | ||
| await attachmentRuntime.remove() | ||
| await composer.addAttachment(file) |
There was a problem hiding this comment.
[MAJOR] Retrying a restored draft can be cancelled immediately. Draft-restored File objects retain their old attachment ID in composer-attachment-drafts.ts, and attachmentRuntime.remove() adds that ID to the adapter's permanent cancelledAttachmentIds set. Passing the same File back into addAttachment reuses the old ID, so attachmentAdapter.ts:91 returns before uploading. This makes retry silently remove pathless restored failures after a reload.
Suggested fix:
const retryFile = new File([file], file.name, {
type: file.type,
lastModified: file.lastModified,
})
await attachmentRuntime.remove()
await composer.addAttachment(retryFile)Also cover this with the real adapter: restore a pathless file with an existing ID, fail its upload, retry, and assert a second upload starts under a fresh ID.
| const observer = new ResizeObserver(updateTruncation) | ||
| observer.observe(element) | ||
| return () => observer.disconnect() | ||
| }, [name]) |
There was a problem hiding this comment.
[MINOR] Re-run this effect when the error branch mounts. A previewed image initially returns the image branch, so filenameRef.current is null and this effect exits. When that upload later becomes incomplete, the filename is unchanged; the span mounts but the [name] dependency prevents installing the observer, so the new truncated-name remove margin is never applied for this common transition.
Suggested fix:
}, [name, isError])Add a rerender test that starts with previewUrl + running status, then changes to incomplete/error with a long filename.
| const [isFilenameTruncated, setIsFilenameTruncated] = useState(false) | ||
| const isUploading = status.type === 'running' | ||
| const isError = status.type === 'incomplete' | ||
| const showRetry = isError && !isParking |
There was a problem hiding this comment.
[MINOR] Do not offer retry for the adapter's deterministic size failure. attachmentAdapter.ts:95 emits this same incomplete/error state when file.size > MAX_UPLOAD_BYTES; retrying cannot change that input, so the new control loops forever without a possible success.
Suggested fix:
import { MAX_UPLOAD_BYTES } from '@/lib/attachmentAdapter'
const isRetryableError = isError && file.size <= MAX_UPLOAD_BYTES
const showRetry = isRetryableError && !isParkingKeep a non-retry error indicator for oversized files and add a visibility test at the boundary.
|
Addressed in 48cde1d:
The updated targeted tests, typecheck, build, and terminal Playwright check pass. |
There was a problem hiding this comment.
Findings
- [Minor] Retry eligibility uses the normal chat upload ceiling instead of the active attachment adapter — the same component is used with the scratchlist adapter, whose per-file limit is configurable and defaults to 10 MB. A scratchlist rejection between 10–50 MB still exposes retry and repeats the full base64 upload, while a configured scratchlist limit above 50 MB suppresses retry for otherwise valid transient failures. Evidence:
web/src/components/AssistantChat/AttachmentItem.tsx:119,web/src/components/SessionChat.tsx:1659,shared/src/scratchlistAttachments.ts:22,hub/src/config/scratchlistAttachmentLimits.ts:20.
Suggested fix:
type ComposerAttachmentWithPreview = PendingAttachment & {
previewUrl?: string
retryable?: boolean
}
const showRetry = isError && retryable !== false && !isParking
Have each adapter set `retryable: false` for its own deterministic failures, including normal oversize and scratchlist size/MIME/quota validation:
```ts
yield {
id,
type: 'file',
name: file.name,
contentType,
file,
status: { type: 'incomplete', reason: 'error' },
retryable: false,
}
Summary
- Review mode: follow-up after new commits
- The three prior bot findings are addressed. One minor cross-adapter retry eligibility issue remains.
Testing
- Not run (automation/security constraint: PR code was not executed). Add coverage for a deterministic scratchlist rejection below 50 MB and an adapter-provided retryability flag.
HAPI Bot
| const [isFilenameTruncated, setIsFilenameTruncated] = useState(false) | ||
| const isUploading = status.type === 'running' | ||
| const isError = status.type === 'incomplete' | ||
| const isRetryableError = isError && file.size <= MAX_UPLOAD_BYTES |
There was a problem hiding this comment.
[MINOR] Use adapter-specific retryability here. This component is also rendered with createScratchlistAttachmentAdapter when scratchlist mode is active (SessionChat.tsx:1659). Scratchlist uploads default to 10 MB per file and can be configured independently (shared/src/scratchlistAttachments.ts:22, hub/src/config/scratchlistAttachmentLimits.ts:20), and size/MIME/quota rejections all become the same incomplete/error state in scratchlistAttachmentAdapter.ts:138. Therefore a deterministic 10–50 MB scratchlist failure still shows this retry button and repeats the full upload; conversely, an operator-configured limit above 50 MB hides retry after a transient failure.
Suggested fix:
type ComposerAttachmentWithPreview = PendingAttachment & {
previewUrl?: string
retryable?: boolean
}
const showRetry = isError && retryable !== false && !isParkingThen have each adapter emit retryable: false from its own deterministic size/MIME/quota failure branches instead of applying the chat adapter constant globally.
|
Addressed in 478adfc:
Validation on the new HEAD: 32 targeted tests passed, |
There was a problem hiding this comment.
Findings
-
[Minor] Retrying a failed attachment moves it to the end — the retry removes the old ID and adds a fresh ID, while the order reconciler appends new IDs. This changes user-visible and submitted attachment order. Evidence:
web/src/components/AssistantChat/AttachmentItem.tsx:155,web/src/lib/attachmentOrder.ts:15,web/src/lib/assistant-runtime.ts:930.Suggested fix:
// Preserve the old order index and replace it with the new retry id. const originalIndex = attachmentOrderRef.current.indexOf(oldId) await retryAttachment() replaceAttachmentIdAt(originalIndex, newId)
Summary
- Review mode: follow-up after new commits
- One minor ordering regression remains on the latest head.
Testing
- Not run (automation/security constraint: PR code was not executed).
- Add a regression test with three attachments: retry the first and middle items, then assert both rendered order and the order passed to send/scratchlist park.
- Add an adapter test using a thrown
ApiErrorwith a deterministic scratchlist code; the current new test covers only a resolvedsuccess: falseresult, while the real client throws on HTTP 400/413.
HAPI Bot
| lastModified: file.lastModified, | ||
| }) | ||
| await attachmentRuntime.remove() | ||
| await composer.addAttachment(retryFile) |
There was a problem hiding this comment.
[MINOR] Preserve the attachment's position when retrying. Removing the failed attachment and then adding a fresh-ID attachment makes the runtime append the retry as a new item. The order reconciler appends unknown IDs (web/src/lib/attachmentOrder.ts:15), and that reconciled order is used for outgoing attachments (web/src/lib/assistant-runtime.ts:930) and scratchlist parks (web/src/components/AssistantChat/HappyComposer.tsx:1163). Retrying the first or middle item therefore silently moves it to the end.
Suggested fix:
// Route retry through HappyComposer, which owns attachmentOrderRef.
const originalIndex = attachmentOrderRef.current.indexOf(oldId)
await remove()
await addAttachment(retryFile)
const newId = composer.getState().attachments.find(
(attachment) => !idsBeforeRetry.has(attachment.id),
)?.id
if (newId) {
const next = attachmentOrderRef.current.filter((id) => id !== newId)
next.splice(Math.max(0, originalIndex), 0, newId)
attachmentOrderRef.current = next
}
|
Addressed in 8ee0e98:
Validation on the new HEAD: 42 targeted tests passed, |
There was a problem hiding this comment.
Findings
-
[Minor] Restore the retry slot when the replacement first appears —
composer.addAttachment()resolves only after the adapter's async generator finishes, while both upload adapters await the network request. The replacement therefore sits at the reconciler-appended tail for the whole retry, and the late callback can overwrite a user reorder made during that upload. Evidence:web/src/components/AssistantChat/AttachmentItem.tsx:173,web/src/lib/attachmentAdapter.ts:144,web/src/lib/scratchlistAttachmentAdapter.ts:145.Suggested fix:
const unsubscribe = composer.subscribe(() => { const retry = composer.getState().attachments.find((item) => item.file === retryFile) if (!retry) return unsubscribe() props.onRetry?.(id, retry.id, originalIndex) }) try { await attachmentRuntime.remove() await composer.addAttachment(retryFile) } finally { unsubscribe() }
-
[Minor] Keep an accessible label for non-retryable upload failures — the visible "Upload failed" text was removed, but the fallback error icon has no role or accessible text. A screen reader now announces only the struck-through filename and remove control, not that the upload failed. Evidence:
web/src/components/AssistantChat/AttachmentItem.tsx:250.Suggested fix:
<> <span aria-hidden="true" className="text-red-500"><ErrorIcon /></span> <span className="sr-only">{t('attachment.uploadFailed')}</span> </>
Questions
- None.
Summary
Review mode: follow-up after new commits
The previous ordering finding is only partially resolved: final order is restored, but not during the retry upload. One accessibility regression also remains for non-retryable failures.
Testing
- Not run (automation/security constraint: PR code was not executed).
- Add a deferred-upload component test asserting the replacement occupies the original slot immediately and a user reorder during upload is not overwritten on completion.
- Add an accessibility assertion that a non-retryable failure exposes localized failure text.
HAPI Bot
| lastModified: file.lastModified, | ||
| }) | ||
| await attachmentRuntime.remove() | ||
| await composer.addAttachment(retryFile) |
There was a problem hiding this comment.
[MINOR] Restore the retry slot before the upload finishes.
In @assistant-ui/react@0.14.29, addAttachment() drains the adapter's async generator before resolving. Both HAPI adapters yield the new attachment first, then await the upload (web/src/lib/attachmentAdapter.ts:144, web/src/lib/scratchlistAttachmentAdapter.ts:145). Waiting here means the new ID is rendered at the reconciler-appended tail for the whole network request. Because running attachments remain draggable, the later onRetry also overwrites any reorder the user makes while that request is pending.
Suggested fix:
const unsubscribe = composer.subscribe(() => {
const retry = composer.getState().attachments.find((item) => item.file === retryFile)
if (!retry) return
unsubscribe()
props.onRetry?.(id, retry.id, originalIndex)
})
try {
await attachmentRuntime.remove()
await composer.addAttachment(retryFile)
} finally {
unsubscribe()
}| {isUploading ? <Spinner size="sm" label={null} className="text-[var(--app-hint)]" /> : null} | ||
| {isError ? ( | ||
| <span className="text-red-500"> | ||
| {isError && (!showRetry || isParking) ? ( |
There was a problem hiding this comment.
[MINOR] Preserve an accessible upload-failure label.
This branch replaces the previous "Upload failed" text with an unlabeled SVG. For a non-retryable failure there is no retry button either, so assistive technology receives the filename and remove control but no indication that the upload failed.
Suggested fix:
<>
<span aria-hidden="true" className="text-red-500"><ErrorIcon /></span>
<span className="sr-only">{t('attachment.uploadFailed')}</span>
</>Add attachment.uploadFailed to both locale files.
|
Addressed in 694c7e0:
Validation on the new HEAD: 42 targeted tests passed, |
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits
No issues found in the latest full diff. The prior ordering and accessible failure-label findings are addressed. Residual testing risk: retry ordering is verified with a mocked composer subscription, not a real assistant-ui runtime integration test.
Testing
- Not run (automation/security constraint: PR code was not executed).
- GitHub checks at review time: integration passed; test was still in progress.
- Added unit coverage exercises retry file identity, immediate slot replacement, adapter retryability, and the non-retryable accessible label.
HAPI Bot
heavygee
added
area:web
2 participants
Summary
Filefor retries so restored draft attachments receive a new upload ID.Validation
bun typecheck— passed.pwsh -NoProfile -File .\scripts\Invoke-HapiTaskPlaywright.ps1 -Name attachment-upload-retry -Suite Root -TestArgs 'terminal-wrap-fidelity.spec.ts'— 2 passed.bun --cwd web test src/components/AssistantChat/AttachmentItem.test.tsx src/components/AssistantChat/SortableComposerAttachments.test.tsx src/lib/attachmentAdapter.test.ts src/lib/scratchlistAttachmentAdapter.test.ts src/lib/attachmentOrder.test.ts— 42 passed.bun run build— passed.Related Issues
None
AI Assistance
OpenAI Codex with GPT-5.6 assisted with implementation, testing, and validation.