fix: lazy-load large attachments in history response#5019
Conversation
…oading failure Co-Authored-By: Claude <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| do { | ||
| let base64 = try await fetchAttachmentData(port: port, attachmentId: attachmentId) | ||
| guard let data = Data(base64Encoded: base64) else { return } |
There was a problem hiding this comment.
🔴 isLoading never reset when base64 decode fails in openInExternalPlayer lazy-load path
When a lazy-loaded video attachment is opened in the external player and the fetched base64 data is invalid (i.e. Data(base64Encoded:) returns nil), the early return exits the Task closure without ever resetting isLoading to false. The UI gets permanently stuck showing the "Loading video..." spinner with no way for the user to recover.
Root Cause
In openInExternalPlayer(), the lazy-load path sets isLoading = true at line 161, then starts an async Task. Inside the do block, after successfully fetching the base64 string, line 165 has:
guard let data = Data(base64Encoded: base64) else { return }If the base64 decode fails, return exits the entire Task closure. The isLoading = false at line 169 (success path) is skipped, and the catch block at line 172 is not triggered because no error was thrown.
Compare with fetchAndPlay() at InlineVideoAttachmentView.swift:134-155, which correctly handles all paths: isLoading = false is set in both the MainActor.run success block and the catch block, and the base64 decode failure is handled inside playFromBase64 which sets failed = true.
Impact: The user sees an infinite loading spinner and cannot retry or interact with the video attachment. The only way to recover is to navigate away and back.
| do { | |
| let base64 = try await fetchAttachmentData(port: port, attachmentId: attachmentId) | |
| guard let data = Data(base64Encoded: base64) else { return } | |
| let base64 = try await fetchAttachmentData(port: port, attachmentId: attachmentId) | |
| guard let data = Data(base64Encoded: base64) else { | |
| await MainActor.run { isLoading = false; failed = true } | |
| return | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 640e401aed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard let token = readSessionToken() else { | ||
| throw URLError(.userAuthenticationRequired) | ||
| } | ||
| let url = URL(string: "http://localhost:\(port)/v1/attachments/\(attachmentId)")! | ||
| var request = URLRequest(url: url) | ||
| request.setValue("Bearer \(token)", forHTTPHeaderField: "Authorization") |
There was a problem hiding this comment.
fetchAttachmentData reads ~/.vellum/session-token via readSessionToken() and sends it as the HTTP bearer token, but the runtime HTTP server authenticates against its separate HTTP token (RUNTIME_PROXY_BEARER_TOKEN/http-token) as set in assistant/src/daemon/lifecycle.ts and checked in assistant/src/runtime/http-server.ts. Because those tokens are generated independently, lazy attachment requests will typically get 401 Unauthorized, so large history videos cannot load.
Useful? React with 👍 / 👎.
| guard let token = readSessionToken() else { | ||
| throw URLError(.userAuthenticationRequired) | ||
| } | ||
| let url = URL(string: "http://localhost:\(port)/v1/attachments/\(attachmentId)")! |
There was a problem hiding this comment.
This new request path always calls /v1/attachments/:id, which dispatches as assistantId=self in the runtime router, but desktop IPC sessions persist assistant outputs under this.assistantId ?? 'local-assistant' (assistant/src/daemon/session.ts). Even if auth succeeds, the scoped lookup in handleGetAttachment will return 404 for those history attachments, so lazy-load playback still fails; the request needs the correct assistant scope (or an unscoped retrieval path).
Useful? React with 👍 / 👎.
| data: a.dataBase64.length > MAX_INLINE_B64_SIZE ? '' : a.dataBase64, | ||
| ...(a.dataBase64.length > MAX_INLINE_B64_SIZE ? { sizeBytes: a.sizeBytes } : {}), |
There was a problem hiding this comment.
The history handler now strips base64 data for every attachment above 512KB, but only video attachments gained a lazy-fetch path in this commit. Existing image/file rendering in ChatView still depends on inline attachment.data (thumbnail/open-in-preview/file-size paths), so large non-video history attachments become unusable or misrepresented (for example, falling back to generic chips with 0 B). Limit omission to video MIME types for now, or add lazy fetch support for the other attachment UIs before dropping inline data.
Useful? React with 👍 / 👎.
|
Addressed in #5053 |
1 participant
Summary
🤖 Generated with Claude Code