feat(uploads): add option to error on name conflicts
Adds an option to display an error when uploading a file with a naming conflict instead of automatically overwriting or timestamping. After this change:
-
overwrite: true- Overwrites the file with a new version -
overwrite: false- Automatically adds a timestamp to the file (or useconflictCallbackto specify a new name) -
overwrite: 'error'- Show an error instead of trying to automatically resolve
Summary by CodeRabbit
-
New Features
- Added a new overwrite mode for uploads that treats file name conflicts as errors, immediately stopping the upload without retrying or renaming.
- Improved error handling in the upload interface: items with a "name in use" error are now removed from the queue with a clear cancel action.
- Updated tooltips and icons for error states to provide clearer feedback during upload conflicts.
-
Bug Fixes
- Enhanced error display and cancellation for uploads encountering file name conflicts.
-
Tests
- Added a test to verify correct error handling when uploads encounter a file name conflict with the new overwrite mode.
Walkthrough
The changes introduce a new explicit 'error' mode for the overwrite property in upload-related classes and components. This mode allows upload operations to treat name conflicts as errors, skipping retries or renaming. Type annotations and control flows are updated accordingly, and related UI components and tests are adjusted to handle the new behavior.
Changes
| File(s) | Change Summary |
|---|---|
| src/api/uploads/BaseUpload.js | Changed overwrite type from boolean to `boolean |
| src/api/uploads/MultiputUpload.js | Updated overwrite parameter type to `boolean |
| src/api/uploads/PlainUpload.js | Changed overwrite parameter type in upload method to `boolean |
| src/elements/content-uploader/ContentUploader.tsx | Updated overwrite prop type to `boolean |
| src/elements/content-uploader/ItemAction.tsx | Added conditional UI logic for STATUS_ERROR to show different icon and tooltip when error code matches name-in-use conflict. |
| src/api/uploads/tests/BaseUpload.test.js | Added test verifying that preflightErrorHandler calls error callback and skips retry when overwrite is 'error' and HTTP 409 conflict occurs. |
Sequence Diagram(s)
sequenceDiagram
participant User
participant ContentUploader
participant UploadClass
participant Server
User->>ContentUploader: Initiates upload (overwrite: "error")
ContentUploader->>UploadClass: Start upload with overwrite="error"
UploadClass->>Server: Preflight upload request
Server-->>UploadClass: Responds with 409 Conflict
UploadClass->>ContentUploader: Calls error callback (no retry/rename)
ContentUploader->>User: Displays error, removes item from queue
Suggested labels
ready-to-merge
Suggested reviewers
- tjuanitas
Poem
A bunny with bytes in the springtime air,
Now treats file name clashes with extra care.
If "overwrite" says "error", no retries ensueβ
The upload will stop, and the queue is anew.
With tests and new icons, the changes are clear,
Hopping forward with uploads, with nothing to fear!
πβ¨
π Recent review details
Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro
π₯ Commits
Reviewing files that changed from the base of the PR and between 3bf945c98607f9c89c504b376dc6f3ec77668cb4 and dd855c410e2fadd072ddf9c7a6d25fb64427f4f7.
π Files selected for processing (6)
-
src/api/uploads/BaseUpload.js(2 hunks) -
src/api/uploads/MultiputUpload.js(8 hunks) -
src/api/uploads/PlainUpload.js(3 hunks) -
src/api/uploads/__tests__/BaseUpload.test.js(1 hunks) -
src/elements/content-uploader/ContentUploader.tsx(4 hunks) -
src/elements/content-uploader/ItemAction.tsx(2 hunks)
π§ Files skipped from review as they are similar to previous changes (3)
- src/elements/content-uploader/ItemAction.tsx
- src/api/uploads/tests/BaseUpload.test.js
- src/elements/content-uploader/ContentUploader.tsx
π§° Additional context used
πͺ Biome (1.9.4)
src/api/uploads/BaseUpload.js
[error] 33-33: return types can only be used in TypeScript files
remove this type annotation
(parse)
src/api/uploads/MultiputUpload.js
[error] 186-186: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 186-186: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 186-186: Shouldn't redeclare 'overwrite'. Consider to delete it or rename it.
'overwrite' is defined here:
(lint/suspicious/noRedeclare)
[error] 232-232: expected , but instead found overwrite
Remove overwrite
(parse)
[error] 232-232: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 456-456: expected , but instead found overwrite
Remove overwrite
(parse)
[error] 456-456: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
src/api/uploads/PlainUpload.js
[error] 140-140: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 140-140: Shouldn't redeclare 'overwrite'. Consider to delete it or rename it.
'overwrite' is defined here:
(lint/suspicious/noRedeclare)
β° Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
π Additional comments (7)
src/api/uploads/PlainUpload.js (2)
119-119: Documentation accurately reflects the new overwrite behavior.The JSDoc comment correctly documents all three modes: overwrite (true), error on conflict ('error'), and rename via conflict callback (false). This provides clear guidance for API consumers.
131-132: Type annotation and FlowFixMe usage are appropriate.The union type
boolean | 'error'correctly supports the new overwrite mode, and the// $FlowFixMecomment is an acceptable workaround for Flow's limitation with union types and default parameters. This approach aligns with the project's migration from Flow to TypeScript.Also applies to: 140-140
src/api/uploads/BaseUpload.js (2)
33-33: Property type correctly updated to support new overwrite mode.The union type
boolean | 'error'properly extends the overwrite property to support the new error-on-conflict behavior.
162-165: Error handling logic correctly implements immediate failure on conflicts.The early return when
overwrite === 'error'ensures that conflicts trigger immediate error callbacks without attempting retries or conflict resolution. This matches the intended behavior described in the PR objectives.src/api/uploads/MultiputUpload.js (3)
176-177: Type annotations and FlowFixMe comments are consistently applied.The union type
boolean | 'error'is correctly applied across all method signatures (setFileInfo,upload,resume), and the// $FlowFixMecomments appropriately suppress Flow type errors for default parameter values. This maintains consistency with the other upload classes.Also applies to: 186-186, 221-222, 232-232, 446-447, 456-456
342-345: Error handling correctly prevents redundant retries.The early return when
overwrite === 'error'prevents the subsequent calls toresolveConflict()andcreateSessionRetry(), addressing the concern raised in previous reviews about duplicate error callbacks and unnecessary retry attempts.
608-608: Minor formatting improvement for method chaining.The consolidation of
.then().catch()into a single line improves readability without affecting functionality.
β¨ Finishing Touches
- [ ] π Generate Docstrings
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.
πͺ§ Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
-
I pushed a fix in commit <commit_id>, please review it. -
Explain this complex logic. -
Open a follow-up GitHub issue for this discussion.
-
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:-
@coderabbitai explain this code block. -
@coderabbitai modularize this function.
-
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:-
@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase. -
@coderabbitai read src/utils.ts and explain its main purpose. -
@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format. -
@coderabbitai help me debug CodeRabbit configuration file.
-
Support
Need help? Create a ticket on our support page for assistance with any issues or questions.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
-
@coderabbitai pauseto pause the reviews on a PR. -
@coderabbitai resumeto resume the paused reviews. -
@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository. -
@coderabbitai full reviewto do a full review from scratch and review all the files again. -
@coderabbitai summaryto regenerate the summary of the PR. -
@coderabbitai generate docstringsto generate docstrings for this PR. -
@coderabbitai generate sequence diagramto generate a sequence diagram of the changes in this PR. -
@coderabbitai resolveresolve all the CodeRabbit review comments. -
@coderabbitai configurationto show the current CodeRabbit configuration for the repository. -
@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
Documentation and Community
- Visit our Documentation for detailed information on how to use CodeRabbit.
- Join our Discord Community to get help, request features, and share feedback.
- Follow us on X/Twitter for updates and announcements.
@psevertson do you mind uploading before and after images of what this change looks like with the different states?
When using the ContentUploader, and you select files, here's what it looks like before hitting upload:
Without my changes, when you hit upload you'll see the files upload, then show a success checkmark. The only options are overwrite: true or overwrite: false, which will both result in a successful upload and either overwrite the file with a new version, or save as a new file with a timestamp
After my changes, there's a third option to set overwrite: error, which means on a file conflict to show an error instead of overwriting or timestamping. It shows the error message that the backend returns, with a X option to remove the file from the uploader.
@psevertson do you mind fixing the flow errors that is currently breaking the CI? this PR is pending internal review
@greg-in-a-box I'm trying to figure out the flow issues, but I'm not as familiar with flow as I am with Typescript.
It seems like it doesn't like how I'm using a union type between boolean and a literal string (boolean | 'error'), but as far as I can tell from Flow's docs, it should be allowed, and I can't replicate the issue in a playground.
It seems the repo is running an older version of Flow, so I'm wondering it this issue is because of that, or some sort of config that we're using on Flow.
Do you have any insights on this?
@greg-in-a-box I'm trying to figure out the flow issues, but I'm not as familiar with flow as I am with Typescript. It seems like it doesn't like how I'm using a union type between boolean and a literal string (
boolean | 'error'), but as far as I can tell from Flow's docs, it should be allowed, and I can't replicate the issue in a playground. It seems the repo is running an older version of Flow, so I'm wondering it this issue is because of that, or some sort of config that we're using on Flow. Do you have any insights on this?
you reminded me that we should have deleted those flow files in the ContentUploader, you should be able to rebase and it will be green
@tjuanitas @greg-in-a-box I'm not sure what else to try here. I tried what the AI suggested and it's not working.
@psevertson can you revert whatever coderabbit suggested and add a // $FlowFixMe comment to wherever the original issue is occurring? that should bypass the flow check
we're migrating Flow to TS so this will eventually be fixed β’οΈ
@tjuanitas Ok, should be good now
@mergify queue
queue
π This queue command has been cancelled by a dequeue command
@mergifyio refresh
refresh
β Pull request refreshed
@mergify dequeue
dequeue
β The pull request is not waiting to be queued anymore.
@mergify queue
queue
π Waiting for conditions to match
- [ ]
-closed[π queue requirement] - [ ] any of: [π queue conditions]
- [ ] all of: [π queue conditions of queue
Automatic boxmoji merge]- [ ]
author = boxmoji - [ ]
files ~= ^i18n/ - [ ]
title ~= ^(fix)\(i18n\)?:\supdate translations$ - [ ] any of: [π‘ GitHub branch protection]
- [ ]
check-neutral = Validate - [ ]
check-skipped = Validate - [ ]
check-success = Validate
- [ ]
- [X]
#approved-reviews-by >= 1[π‘ GitHub branch protection] - [X]
#changes-requested-reviews-by = 0[π‘ GitHub branch protection] - [X]
#review-threads-unresolved = 0[π‘ GitHub branch protection] - [X]
branch-protection-review-decision = APPROVED[π‘ GitHub branch protection] - [X]
label != do-not-merge - [X]
status-success = license/cla - [X]
status-success = lint_pull_request - [X]
status-success = lint_test_build - [X] any of: [π‘ GitHub branch protection]
- [X]
check-success = Summary - [ ]
check-neutral = Summary - [ ]
check-skipped = Summary
- [X]
- [X] any of: [π‘ GitHub branch protection]
- [X]
check-success = lint_test_build - [ ]
check-neutral = lint_test_build - [ ]
check-skipped = lint_test_build
- [X]
- [X] any of: [π‘ GitHub branch protection]
- [X]
check-success = license/cla - [ ]
check-neutral = license/cla - [ ]
check-skipped = license/cla
- [X]
- [ ]
- [ ] all of: [π queue conditions of queue
Automatic strict merge]- [ ] any of: [π‘ GitHub branch protection]
- [ ]
check-neutral = Validate - [ ]
check-skipped = Validate - [ ]
check-success = Validate
- [ ]
- [X]
#approved-reviews-by >= 1[π‘ GitHub branch protection] - [X]
#approved-reviews-by >= 2 - [X]
#changes-requested-reviews-by = 0 - [X]
#changes-requested-reviews-by = 0[π‘ GitHub branch protection] - [X]
#review-threads-unresolved = 0 - [X]
#review-threads-unresolved = 0[π‘ GitHub branch protection] - [X]
branch-protection-review-decision = APPROVED - [X]
branch-protection-review-decision = APPROVED[π‘ GitHub branch protection] - [X]
label != do-not-merge - [X]
label = ready-to-merge - [X]
status-success = license/cla - [X]
status-success = lint_pull_request - [X]
status-success = lint_test_build - [X]
title ~= ^(build|ci|chore|docs|feat|fix|perf|refactor|revert|style|test)(\([^)]+\))?:\s.+$ - [X] any of: [π‘ GitHub branch protection]
- [X]
check-success = Summary - [ ]
check-neutral = Summary - [ ]
check-skipped = Summary
- [X]
- [X] any of: [π‘ GitHub branch protection]
- [X]
check-success = lint_test_build - [ ]
check-neutral = lint_test_build - [ ]
check-skipped = lint_test_build
- [X]
- [X] any of: [π‘ GitHub branch protection]
- [X]
check-success = license/cla - [ ]
check-neutral = license/cla - [ ]
check-skipped = license/cla
- [X]
- [ ] any of: [π‘ GitHub branch protection]
- [ ] all of: [π queue conditions of queue
- [X]
-conflict[π queue requirement] - [X]
-draft[π queue requirement] - [X] any of: [π queue -> configuration change requirements]
- [X]
-mergify-configuration-changed - [ ]
check-success = Configuration changed
- [X]
for whatever reason, mergify is stuck on an old github branch protection configuration. same issue as: https://github.com/Mergifyio/mergify/issues/5131