feat: add download button for attached videos#5042
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6db247ee90
ℹ️ 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 data = Data(base64Encoded: attachment.data) else { return } | ||
| try? data.write(to: destURL) |
There was a problem hiding this comment.
Avoid blocking UI while saving inline video data
In the non-lazy attachment path, both Data(base64Encoded:) and data.write(to:) run synchronously in the button action, which executes on the main thread. For larger inline videos, this can freeze the chat UI until decode+disk write finishes, making the app feel hung during save. Moving this branch to a background Task (as done for lazy-load saves) would prevent user-visible stalls.
Useful? React with 👍 / 👎.
|
|
||
| if attachment.isLazyLoad { | ||
| guard let port = daemonHttpPort, let attachmentId = attachment.id.isEmpty ? nil : attachment.id else { return } | ||
| isLoading = true |
There was a problem hiding this comment.
🟡 saveVideo() sets isLoading=true which hides the active video player during save
When a user is watching a lazy-loaded video and clicks the save button, saveVideo() sets isLoading = true at line 187. Because the view body uses an if/else if chain (InlineVideoAttachmentView.swift:32-42), the isLoading branch takes priority over the player/isPlaying branch, causing the video player to be replaced by a "Loading video..." spinner while the save is in progress.
Root Cause and Impact
The isLoading state is shared between two different operations: initial video loading for playback and the save/download operation. When saveVideo() sets isLoading = true at line 187, the view body at lines 34-36 shows loadingView instead of the VideoPlayerView:
} else if isLoading {
loadingView // <-- replaces the video player
} else if let player, isPlaying {
VideoPlayerView(player: player) // <-- hidden during saveThis means clicking "Save video" on an already-playing lazy-loaded video will:
- Hide the video player and show a loading spinner
- Re-fetch the attachment data from the server (even though it was already fetched for playback)
- After save completes, the video player reappears but the visual interruption is jarring
Impact: The video playback is visually disrupted every time the user saves a lazy-loaded video. The save operation should use a separate state (e.g. isSaving) or avoid setting isLoading altogether, since the save can happen in the background without affecting the UI.
Prompt for agents
In clients/macos/vellum-assistant/Features/Chat/MediaEmbeds/InlineVideoAttachmentView.swift, the saveVideo() function at line 187 sets isLoading = true, which causes the video player to be replaced by a loading spinner during the save operation. To fix this:
1. Add a new @State private var isSaving = false property (around line 22)
2. In saveVideo() (lines 187-199), replace all occurrences of isLoading with isSaving
3. Optionally show a subtle save progress indicator that doesn't replace the video player (e.g. overlay a small spinner on the save button area)
4. Update the save button visibility condition at line 44 to also check !isSaving if you want to hide the button during save
This ensures the save operation doesn't interfere with video playback state.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Addressed in #5058 |
Summary
🤖 Generated with Claude Code