Repository navigation
Conversation
kesselb
changed the title
27747d2 to
660c560
Compare
|
@coderabbitai review |
Action performed
|
Findings
 |
| Layer / File(s) | Summary |
|---|---|
Track attachment upload state src/components/ComposerAttachments.vue, src/tests/unit/components/ComposerAttachments.vue.spec.js |
ComposerAttachments reports blocking state during uploads and when attachments have errors. It awaits uploads, updates the model after success, and reports non-aborted failures. Tests cover successful uploads, failures, and aborted uploads. |
Guard sending and save attachment drafts src/components/Composer.vue, src/components/NewMessageModal.vue, src/tests/unit/components/Composer.vue.spec.js |
Composer blocks sending when attachments prevent it and saves drafts when attachment data changes. NewMessageModal removes upload-completion coordination. Tests cover the send guard and draft saving. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant ComposerAttachments
participant User
participant Composer
ComposerAttachments->>Composer: Emit update:blocksSending(true)
User->>Composer: Invoke onSend
Composer->>Composer: Check canSend
Composer-->>User: Return without emitting send
Merge Risk: 
660c5
-
The change aims to stop messages from being sent while attachments are uploading or have failed. In a failed upload, in overlapping uploads, and after minimizing and reopening the composer, Send can still become available and the message goes out without the file. Fix these before merging.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: blocking message sending while attachments are uploading or have failed. Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 2 files. (2 skipped: 2 unsupported.)
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
-
-
- Source mode: Auto
- Repository: nextcloud/mail
- Branch: fix/disable-send-on-attachment-failure
- File: .coderabbit.yaml
- Commit: 660c560
- Layers: Repository YAML
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
ed46f5fa-3746-43c2-9702-408389da1cca
-
src/components/Composer.vuesrc/components/ComposerAttachments.vuesrc/components/NewMessageModal.vuesrc/tests/unit/components/Composer.vue.spec.jssrc/tests/unit/components/ComposerAttachments.vue.spec.js
-
- src/components/NewMessageModal.vue
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
-
| blocksSending() { | ||
| return this.uploading || this.hasAttachmentErrors | ||
| }, |
There was a problem hiding this comment.
Preserve failed-upload state when the composer reopens.
If an upload fails and the user minimizes the composer, the modal hides and destroys this component’s rendered subtree. A reopened ComposerAttachments rebuilds its attachments from modelValue, which contains only successful uploads. The failed file and its send block disappear, so Send becomes available without that file. Preserve the failure state across reopening, or require the user to remove the failed attachment before closing the composer. (raw.githubusercontent.com)
| attachment.error = true | ||
| showError(t('mail', 'Could not upload {fileName}', { fileName: file.name })) |
There was a problem hiding this comment.
Set upload errors through the reactive attachment.
If the attachment counter has already read hasAttachmentErrors, assigning attachment.error here mutates the original object rather than its Vue proxy. That assignment does not invalidate the cached computation. When uploading becomes false, blocksSending can emit false despite the failed upload, allowing a message without the file. Update the attachment through this.attachments or make the local attachment reactive. The current test reads the object directly, so it does not establish that the send block updates. (vuejs.org)
| await Promise.all(promises) | ||
| this.uploading = false |
There was a problem hiding this comment.
Keep sending blocked until every upload batch settles.
If a user starts a second upload before the first finishes, the first batch sets uploading to false while the second file is still pending. blocksSending then enables Send, and the message can omit that file. The previous modal coordination tracked upload promises; this change removes that wait. Track all active batches, including the size-limit return path, before clearing the block. (raw.githubusercontent.com)
660c560 to
bfd8ba0
Compare
Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Daniel Kesselberg <3902676+kesselb@users.noreply.github.com>
bfd8ba0 to
85b42c5
Compare
Summary
This PR addresses a CodeRabbit finding from #13992: if an attachment upload was still running or had failed, sending continued behind the "Preparing message…" toast and the message went out without the file.
The suggested fix (abort the send after the uploads settled) did not look right to me. Why allow sending at all when one of the attachments failed?
Instead, the send button stays disabled until all uploads are done, and a failed upload shows an error toast. That also makes the finding obsolete, because that state can no longer happen.
🤖 AI (if applicable)
Summary by CodeRabbit