box-ui-elements icon indicating copy to clipboard operation
box-ui-elements copied to clipboard

feat(uploads): add option to error on name conflicts

Open psevertson opened this issue 8 months ago β€’ 10 comments

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 use conflictCallback to 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.

psevertson avatar May 28 '25 23:05 psevertson

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 28 '25 23:05 CLAassistant

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 // $FlowFixMe comment 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 // $FlowFixMe comments 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 to resolveConflict() and createSessionRetry(), 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.

❀️ Share
πŸͺ§ 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 @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in 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 pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file 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.

coderabbitai[bot] avatar May 28 '25 23:05 coderabbitai[bot]

@psevertson do you mind uploading before and after images of what this change looks like with the different states?

greg-in-a-box avatar May 29 '25 19:05 greg-in-a-box

When using the ContentUploader, and you select files, here's what it looks like before hitting upload: image

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 image

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. image

psevertson avatar May 29 '25 19:05 psevertson

@psevertson do you mind fixing the flow errors that is currently breaking the CI? this PR is pending internal review

greg-in-a-box avatar Jun 12 '25 18:06 greg-in-a-box

@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?

psevertson avatar Jun 12 '25 20:06 psevertson

@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

greg-in-a-box avatar Jun 12 '25 23:06 greg-in-a-box

@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 avatar Jun 17 '25 15:06 psevertson

@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 avatar Jun 17 '25 16:06 tjuanitas

@tjuanitas Ok, should be good now

psevertson avatar Jun 17 '25 16:06 psevertson

@mergify queue

tjuanitas avatar Jun 30 '25 18:06 tjuanitas

queue

πŸ›‘ This queue command has been cancelled by a dequeue command

mergify[bot] avatar Jun 30 '25 18:06 mergify[bot]

@mergifyio refresh

tjuanitas avatar Jun 30 '25 18:06 tjuanitas

refresh

βœ… Pull request refreshed

mergify[bot] avatar Jun 30 '25 18:06 mergify[bot]

@mergify dequeue

tjuanitas avatar Jun 30 '25 19:06 tjuanitas

dequeue

βœ… The pull request is not waiting to be queued anymore.

mergify[bot] avatar Jun 30 '25 19:06 mergify[bot]

@mergify queue

tjuanitas avatar Jun 30 '25 19:06 tjuanitas

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] any of: [πŸ›‘ GitHub branch protection]
        • [X] check-success = lint_test_build
        • [ ] check-neutral = lint_test_build
        • [ ] check-skipped = lint_test_build
      • [X] any of: [πŸ›‘ GitHub branch protection]
        • [X] check-success = license/cla
        • [ ] check-neutral = license/cla
        • [ ] check-skipped = license/cla
    • [ ] 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] any of: [πŸ›‘ GitHub branch protection]
        • [X] check-success = lint_test_build
        • [ ] check-neutral = lint_test_build
        • [ ] check-skipped = lint_test_build
      • [X] any of: [πŸ›‘ GitHub branch protection]
        • [X] check-success = license/cla
        • [ ] check-neutral = license/cla
        • [ ] check-skipped = license/cla
  • [X] -conflict [πŸ“Œ queue requirement]
  • [X] -draft [πŸ“Œ queue requirement]
  • [X] any of: [πŸ“Œ queue -> configuration change requirements]
    • [X] -mergify-configuration-changed
    • [ ] check-success = Configuration changed

mergify[bot] avatar Jun 30 '25 19:06 mergify[bot]

for whatever reason, mergify is stuck on an old github branch protection configuration. same issue as: https://github.com/Mergifyio/mergify/issues/5131

tjuanitas avatar Jun 30 '25 19:06 tjuanitas