Repository navigation
Conversation
With autoAttach on, a file that failed or was paused and then retried or resumed uploaded fine but was never attached: the list item's retry/resume runs its own useChunkedUpload, so useFileUploadLogic's onUploadComplete never fired. Pick completed uploads in the target's scope up from the store instead, and drop them from the store once attached. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dq2w352FXrTDKwmGC9eyxH
🦋 Changeset detectedLatest commit: b3c2766 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
|
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
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.



Summary
Fixes an existing web bug: with
autoAttachon, a file that finishes uploading after a retry or resume is never attached to its target. On web this hits comment editing (CommentUpdate→FileUploadTrigger autoAttach). Pause an upload and resume it, or retry a failed one. The chip reaches "completed", but the file is not attached and is gone after a reload.Backend: N/A, frontend-only.
Template PR: none yet. The template picks this up when its submodule pointer next moves past this commit.
Cause
useFileUploadLogicattached late completions throughuseChunkedUpload({ onUploadComplete }). That callback belongs to the hook instance. The Retry and Resume buttons live inUploadingFileItem, which calls its ownuseChunkedUpload()with no options. So when a retried or resumed upload completed, nothing attached it.What changed
files/common/hooks/useFileUploadLogic:useShallowselector, for COMPLETED uploads in its target's scope that have a relay id, and attaches them. This doesn't depend on which component started the retry or resume.handleFilesSelectedis running, the batch attaches its own files and the watcher waits. When the batch ends, anything that completed in the meantime is attached. Ids already sent are still deduped throughattachedRef, so nothing is attached twice.resetKeystill bumps only for batches.Test plan
Q: Is the bug covered by a test?
A: Yes, the new
useFileUploadLogic.test.ts. Three of its four cases fail onmasterand pass here: late completion attaches; the late file leaves the store once attached; completion during a running batch is attached after it. The fourth (other scopes /autoAttach: falseare ignored) passes on both.Q: How do I check it by hand?
A: On web, edit a comment and attach a large file.
Q: Does it change the normal path?
A: No. A file that finishes inside its original batch is attached by the batch, exactly as before.
Tradeoffs
autoAttach.attachedRefis per hook instance. Two auto-attach triggers mounted for the same target at once could each attach the same late file. No screen does that today.🤖 Generated with Claude Code
https://claude.ai/code/session_01Dq2w352FXrTDKwmGC9eyxH