这是indexloc提供的服务,不要输入任何密码
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 30, 2025

Conversation

psevertson
Copy link
Contributor

@psevertson psevertson commented May 28, 2025

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 psevertson requested a review from a team as a code owner May 28, 2025 23:40
@CLAassistant
Copy link

CLAassistant commented May 28, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

coderabbitai bot commented May 28, 2025

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
Loading

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 3bf945c and dd855c4.

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/api/uploads/BaseUpload.js (1)

33-33: Consider type annotation consistency.

The type annotation is functionally correct, but uses double quotes while the corresponding TypeScript interface uses single quotes. Consider using single quotes for consistency across the codebase.

-    overwrite: boolean | "error";
+    overwrite: boolean | 'error';

Note: The static analysis hint about "return types can only be used in TypeScript files" is incorrect - this is valid Flow syntax for property type annotations in JavaScript files.

🧰 Tools
🪛 Biome (1.9.4)

[error] 33-33: return types can only be used in TypeScript files

remove this type annotation

(parse)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5a09f91 and 7f44dc8.

📒 Files selected for processing (7)
  • src/api/uploads/BaseUpload.js (2 hunks)
  • src/api/uploads/MultiputUpload.js (1 hunks)
  • src/api/uploads/PlainUpload.js (1 hunks)
  • src/api/uploads/__tests__/BaseUpload.test.js (1 hunks)
  • src/elements/content-uploader/ContentUploader.js.flow (1 hunks)
  • src/elements/content-uploader/ContentUploader.tsx (3 hunks)
  • src/elements/content-uploader/ItemAction.tsx (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/elements/content-uploader/ItemAction.tsx (1)
src/elements/common/messages.js (1)
  • messages (9-1092)
🪛 Biome (1.9.4)
src/api/uploads/PlainUpload.js

[error] 139-139: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 139-139: Shouldn't redeclare 'overwrite'. Consider to delete it or rename it.

'overwrite' is defined here:

(lint/suspicious/noRedeclare)

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] 185-185: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 185-185: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 185-185: 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 (9)
src/api/uploads/PlainUpload.js (1)

139-139: LGTM! Type annotation correctly updated to support new 'error' mode.

The union type boolean | 'error' properly reflects the new overwrite behavior described in the PR objectives, where 'error' will cause conflicts to be treated as immediate errors.

Note: The static analysis warnings about Flow syntax and variable redeclaration are false positives and can be safely ignored in this codebase.

🧰 Tools
🪛 Biome (1.9.4)

[error] 139-139: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 139-139: Shouldn't redeclare 'overwrite'. Consider to delete it or rename it.

'overwrite' is defined here:

(lint/suspicious/noRedeclare)

src/elements/content-uploader/ContentUploader.js.flow (1)

98-98: Correct prop type update for the new overwrite mode.

The union type boolean | 'error' properly aligns with the updated upload API classes and allows the ContentUploader component to support the new error handling mode for naming conflicts.

src/elements/content-uploader/ItemAction.tsx (2)

11-11: Appropriate import for the new error handling logic.

The ERROR_CODE_ITEM_NAME_IN_USE constant is correctly imported to support the conditional rendering logic for the new overwrite error mode.


74-80: Excellent UI logic for handling naming conflict errors.

The conditional logic properly distinguishes between naming conflict errors (ERROR_CODE_ITEM_NAME_IN_USE) and other upload errors:

  • Naming conflicts: Show XMark icon with cancel tooltip - appropriate since these errors are intentional and shouldn't be retried automatically
  • Other errors: Preserve original behavior with retry/resume functionality

This provides a clear visual distinction and appropriate user actions for the new overwrite: 'error' mode.

src/elements/content-uploader/ContentUploader.tsx (3)

37-37: LGTM - Required import for new error handling.

The import of ERROR_CODE_ITEM_NAME_IN_USE is correctly added to support the new error handling logic for name conflicts.


94-94: LGTM - Prop type updated to support new error mode.

The prop type change from boolean to boolean | 'error' correctly aligns with the new overwrite behavior options.


1101-1104: LGTM - Correct error handling for name conflicts.

The logic correctly handles the new error mode by immediately removing items with ERROR_CODE_ITEM_NAME_IN_USE and calling the cancel callback, instead of attempting retry or resume operations.

src/api/uploads/__tests__/BaseUpload.test.js (1)

299-311: LGTM - Comprehensive test coverage for new error mode.

The test case properly verifies that when overwrite is set to 'error' and a 409 conflict occurs, the error callback is invoked immediately without any retry logic. Good test coverage for the new functionality.

src/api/uploads/BaseUpload.js (1)

162-165: LGTM - Correct implementation of error mode handling.

The logic correctly handles the new 'error' mode by immediately calling the error callback and returning early, bypassing all conflict resolution logic. This implements the intended behavior of treating conflicts as immediate errors.

@greg-in-a-box
Copy link
Contributor

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

@psevertson
Copy link
Contributor Author

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

@greg-in-a-box
Copy link
Contributor

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

@psevertson
Copy link
Contributor Author

@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
Copy link
Contributor

greg-in-a-box commented Jun 12, 2025

@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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
src/api/uploads/MultiputUpload.js (1)

1243-1255: ⚠️ Potential issue

resolveConflict ignores the new 'error' mode – will overwrite instead of erroring

this.overwrite is treated as a truthy flag; when it contains 'error', the condition still passes and we proceed with the overwrite path, breaking the contract introduced by this PR.

-    async resolveConflict(data: Object): Promise<any> {
-        if (this.overwrite && data.context_info) {
+    async resolveConflict(data: Object): Promise<any> {
+        // Honour the new mode – surface the conflict immediately.
+        if (this.overwrite === 'error') {
+            this.errorCallback(data);
+            return;
+        }
+
+        if (this.overwrite && data.context_info) {
             this.fileId = data.context_info.conflicts.id;
             return;
         }

Without this guard, any chunked upload using 'error' will silently overwrite the existing file – the exact behaviour we’re trying to avoid.

🧰 Tools
🪛 Biome (1.9.4)

[error] 1245-1245: Illegal return statement outside of a function

(parse)


[error] 1249-1249: Illegal return statement outside of a function

(parse)

🧹 Nitpick comments (1)
src/api/uploads/PlainUpload.js (1)

139-141: JSDoc is now stale – update the param doc for overwrite
The type has changed to accept 'error', but the preceding JSDoc (lines 118-120) still reads @param {boolean}. Keeping docs in sync prevents confusion for integrators.

- * @param {boolean} [overwrite] - Should upload overwrite file with same name
+ * @param {boolean|'error'} [overwrite] - Conflict behaviour. true=overwrite, false=rename, 'error'=fail
🧰 Tools
🪛 Biome (1.9.4)

[error] 139-139: 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: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 141-141: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 139-139: Shouldn't redeclare 'overwrite'. Consider to delete it or rename it.

'overwrite' is defined here:

(lint/suspicious/noRedeclare)


[error] 140-140: Shouldn't redeclare 'progressCallback'. Consider to delete it or rename it.

'progressCallback' is defined here:

(lint/suspicious/noRedeclare)


[error] 141-141: Shouldn't redeclare 'successCallback'. Consider to delete it or rename it.

'successCallback' is defined here:

(lint/suspicious/noRedeclare)


[error] 140-140: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)


[error] 141-141: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6440a3e and 7bf4d46.

📒 Files selected for processing (6)
  • src/api/uploads/BaseUpload.js (2 hunks)
  • src/api/uploads/MultiputUpload.js (3 hunks)
  • src/api/uploads/PlainUpload.js (1 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] 185-185: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 185-185: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 185-185: Shouldn't redeclare 'overwrite'. Consider to delete it or rename it.

'overwrite' is defined here:

(lint/suspicious/noRedeclare)


[error] 230-230: expected , but instead found overwrite

Remove overwrite

(parse)


[error] 230-230: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 449-449: expected , but instead found overwrite

Remove overwrite

(parse)


[error] 449-449: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)

src/api/uploads/PlainUpload.js

[error] 139-139: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 139-139: 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 (5)
src/api/uploads/BaseUpload.js (2)

33-34: overwrite union type looks solid
Extending the Flow type to boolean | 'error' correctly models the new mode without breaking existing call-sites.

🧰 Tools
🪛 Biome (1.9.4)

[error] 33-33: return types can only be used in TypeScript files

remove this type annotation

(parse)


162-165: Early-exit correctly short-circuits retry logic
The explicit branch for overwrite === 'error' immediately surfaces the 409 to the consumer, preventing the subsequent overwrite / rename logic from executing. ✔️

src/api/uploads/MultiputUpload.js (3)

185-188: Type annotation aligned – good catch
overwrite?: boolean | 'error' keeps setFileInfo consistent with the new semantics.

🧰 Tools
🪛 Biome (1.9.4)

[error] 185-185: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 185-185: 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: 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] 187-187: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 187-187: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 188-188: Expected a parameter but instead found '}'.

Expected a parameter here.

(parse)


[error] 188-188: Expected a statement but instead found '): void'.

Expected a statement here.

(parse)


[error] 185-185: Shouldn't redeclare 'overwrite'. Consider to delete it or rename it.

'overwrite' is defined here:

(lint/suspicious/noRedeclare)


[error] 186-186: Shouldn't redeclare 'progressCallback'. Consider to delete it or rename it.

'progressCallback' is defined here:

(lint/suspicious/noRedeclare)


[error] 187-187: Shouldn't redeclare 'successCallback'. Consider to delete it or rename it.

'successCallback' is defined here:

(lint/suspicious/noRedeclare)


[error] 186-186: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)


[error] 187-187: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)


230-233: Ditto for the upload() overload
Signature now accepts 'error', maintaining parity across entry points.

🧰 Tools
🪛 Biome (1.9.4)

[error] 230-230: expected , but instead found overwrite

Remove overwrite

(parse)


[error] 230-230: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 231-231: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 232-232: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 233-233: Expected an expression but instead found '}'.

Expected an expression here.

(parse)


449-453: resume() signature updated – looks correct
All public APIs of MultiputUpload now share the same contract.

🧰 Tools
🪛 Biome (1.9.4)

[error] 449-449: expected , but instead found overwrite

Remove overwrite

(parse)


[error] 449-449: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 450-450: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 451-451: expected , but instead found :

Remove :

(parse)


[error] 452-452: expected , but instead found successCallback

Remove successCallback

(parse)


[error] 452-452: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 453-453: Expected an expression but instead found '}'.

Expected an expression here.

(parse)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
src/api/uploads/MultiputUpload.js (1)

339-346: ⚠️ Potential issue

Retry continues even after signalling fatal conflict – causes unwanted loops

createSessionErrorHandler always calls createSessionRetry() after resolveConflict().
When overwrite === 'error', resolveConflict (lines 1241-1244) already triggers errorCallback and should terminate the flow, but control returns to createSessionErrorHandler, which then schedules another retry.
This results in an unnecessary retry loop and multiple error callbacks.

@@
-        if (errorData && errorData.status === 409) {
-            this.resolveConflict(errorData);
-            this.createSessionRetry();
-            return;
-        }
+        if (errorData && errorData.status === 409) {
+            if (this.overwrite === 'error') {
+                this.errorCallback(errorData);
+                return;
+            }
+            this.resolveConflict(errorData);
+            this.createSessionRetry();
+            return;
+        }

Alternatively, make resolveConflict return a boolean indicating whether the caller should continue.

Also applies to: 1241-1244

🧹 Nitpick comments (2)
src/api/uploads/PlainUpload.js (1)

118-121: JSDoc no longer matches the new overwrite contract

The docblock still describes overwrite as boolean-only and omits the new 'error' mode, while the Flow annotation (line 139) correctly allows boolean | 'error'. Please update the comment so that consumer-facing documentation and IDE hints stay in sync.

- * @param {boolean} [overwrite] - Should upload overwrite file with same name
+ * @param {boolean|'error'} [overwrite] - true → overwrite, false → auto-rename, 'error' → fail on conflict

Also applies to: 137-140

src/api/uploads/MultiputUpload.js (1)

170-186: Outdated JSDoc after adding 'error' overwrite mode

All three public entry points (setFileInfo, upload, resume) now accept boolean | 'error', but the surrounding comments still document the parameter as a plain boolean. This creates confusion for integrators and future maintainers.

- * @param {boolean} [overwrite]
+ * @param {boolean|'error'} [overwrite]

Apply the same tweak to the other two blocks above.

Also applies to: 213-233, 433-451

🧰 Tools
🪛 Biome (1.9.4)

[error] 179-183: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 184-184: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 185-185: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 185-185: 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: 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] 184-184: Shouldn't redeclare 'folderId'. Consider to delete it or rename it.

'folderId' is defined here:

(lint/suspicious/noRedeclare)


[error] 185-185: Shouldn't redeclare 'overwrite'. Consider to delete it or rename it.

'overwrite' is defined here:

(lint/suspicious/noRedeclare)


[error] 186-186: Shouldn't redeclare 'progressCallback'. Consider to delete it or rename it.

'progressCallback' is defined here:

(lint/suspicious/noRedeclare)


[error] 180-180: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)


[error] 181-181: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)


[error] 186-186: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7bf4d46 and 72b8097.

📒 Files selected for processing (6)
  • src/api/uploads/BaseUpload.js (2 hunks)
  • src/api/uploads/MultiputUpload.js (5 hunks)
  • src/api/uploads/PlainUpload.js (1 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] 185-185: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 185-185: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 185-185: Shouldn't redeclare 'overwrite'. Consider to delete it or rename it.

'overwrite' is defined here:

(lint/suspicious/noRedeclare)


[error] 230-230: expected , but instead found overwrite

Remove overwrite

(parse)


[error] 230-230: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 449-449: expected , but instead found overwrite

Remove overwrite

(parse)


[error] 449-449: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 1243-1243: Illegal return statement outside of a function

(parse)

src/api/uploads/PlainUpload.js

[error] 139-139: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 139-139: 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 (1)
src/api/uploads/BaseUpload.js (1)

160-166: Early-exit for 'error' mode looks good

The short-circuit when overwrite === 'error' cleanly aborts the preflight retry path and surfaces the conflict to the caller. Implementation is consistent with existing handling for the MAX_RETRY branch, so no issues here.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
src/api/uploads/MultiputUpload.js (2)

221-231: Same duplication comment as above

See previous note – the same cast appears here.

🧰 Tools
🪛 Biome (1.9.4)

[error] 223-223: expected , but instead found :

Remove :

(parse)


[error] 224-224: expected , but instead found conflictCallback

Remove conflictCallback

(parse)


[error] 224-224: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 225-225: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 226-226: expected , but instead found :

Remove :

(parse)


[error] 227-227: expected , but instead found fileDescription

Remove fileDescription

(parse)


[error] 227-227: expected , but instead found :

Remove :

(parse)


[error] 228-228: expected , but instead found fileId

Remove fileId

(parse)


[error] 228-228: expected , but instead found :

Remove :

(parse)


[error] 229-229: expected , but instead found folderId

Remove folderId

(parse)


[error] 229-229: expected , but instead found :

Remove :

(parse)


[error] 230-230: expected , but instead found overwrite

Remove overwrite

(parse)


[error] 230-230: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 231-231: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


441-450: Same duplication comment as above

See previous note – the same cast appears here.

🧰 Tools
🪛 Biome (1.9.4)

[error] 443-443: expected , but instead found :

Remove :

(parse)


[error] 444-444: expected , but instead found conflictCallback

Remove conflictCallback

(parse)


[error] 444-444: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 445-445: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 446-446: expected , but instead found :

Remove :

(parse)


[error] 447-447: expected , but instead found fileId

Remove fileId

(parse)


[error] 447-447: expected , but instead found :

Remove :

(parse)


[error] 448-448: expected , but instead found folderId

Remove folderId

(parse)


[error] 448-448: expected , but instead found :

Remove :

(parse)


[error] 449-449: expected , but instead found overwrite

Remove overwrite

(parse)


[error] 449-449: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 450-450: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)

🧹 Nitpick comments (3)
src/api/uploads/BaseUpload.js (1)

33-34: Add doc-block/propType update for new union type

overwrite is now boolean | 'error', but that isn’t documented anywhere in the file header or in consumer-facing JSDoc. A quick comment explaining the three modes (overwrite, rename, hard-error) will make the API self-describing.

🧰 Tools
🪛 Biome (1.9.4)

[error] 33-33: return types can only be used in TypeScript files

remove this type annotation

(parse)

src/api/uploads/MultiputUpload.js (1)

176-186: Default-parameter cast is fine, but consider centralising

The (true: boolean | 'error') cast is now repeated in setFileInfo, upload, and resume. Extracting a small constant, e.g.:

const DEFAULT_OVERWRITE: boolean | 'error' = true;

would remove duplication and the slightly awkward Flow cast.

🧰 Tools
🪛 Biome (1.9.4)

[error] 176-176: expected ) but instead found :

Remove :

(parse)


[error] 176-176: expected , but instead found |

Remove |

(parse)


[error] 176-176: expected , but instead found )

Remove )

(parse)


[error] 176-176: Expected a class method body but instead found ','.

Expected a class method body here.

(parse)


[error] 177-177: expected a semicolon to end the class property, but found none

(parse)


[error] 177-177: Expected an identifier, a string literal, a number literal, a private field name, or a computed name but instead found ','.

Expected an identifier, a string literal, a number literal, a private field name, or a computed name here.

(parse)


[error] 178-178: expected a semicolon to end the class property, but found none

(parse)


[error] 178-178: Expected an identifier, a string literal, a number literal, a private field name, or a computed name but instead found ','.

Expected an identifier, a string literal, a number literal, a private field name, or a computed name here.

(parse)


[error] 179-179: Expected a statement but instead found ':'.

Expected a statement here.

(parse)


[error] 180-180: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 181-181: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 182-182: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)

src/api/uploads/PlainUpload.js (1)

131-132: Same ‘Flow cast’ duplication comment

The (true: boolean | 'error') pattern appears in three upload classes. Consider extracting a shared constant as noted in MultiputUpload.

🧰 Tools
🪛 Biome (1.9.4)

[error] 131-131: expected ) but instead found :

Remove :

(parse)


[error] 131-131: expected , but instead found |

Remove |

(parse)


[error] 131-131: expected , but instead found )

Remove )

(parse)


[error] 131-131: Expected a class method body but instead found ','.

Expected a class method body here.

(parse)


[error] 132-132: Expected a statement but instead found ':'.

Expected a statement here.

(parse)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 72b8097 and db85d08.

📒 Files selected for processing (6)
  • src/api/uploads/BaseUpload.js (2 hunks)
  • src/api/uploads/MultiputUpload.js (8 hunks)
  • src/api/uploads/PlainUpload.js (2 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] 176-176: expected ) but instead found :

Remove :

(parse)


[error] 176-176: expected , but instead found |

Remove |

(parse)


[error] 176-176: expected , but instead found )

Remove )

(parse)


[error] 176-176: Expected a class method body but instead found ','.

Expected a class method body here.

(parse)


[error] 220-220: Did you mean to use a :? An = can only follow a property name when the containing object literal is part of a destructuring pattern.

(parse)


[error] 220-220: expected ) but instead found :

Remove :

(parse)


[error] 220-220: expected , but instead found )

Remove )

(parse)


[error] 230-230: expected , but instead found overwrite

Remove overwrite

(parse)


[error] 230-230: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 440-440: Did you mean to use a :? An = can only follow a property name when the containing object literal is part of a destructuring pattern.

(parse)


[error] 440-440: expected ) but instead found :

Remove :

(parse)


[error] 440-440: expected , but instead found )

Remove )

(parse)


[error] 449-449: expected , but instead found overwrite

Remove overwrite

(parse)


[error] 449-449: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 1243-1243: Illegal return statement outside of a function

(parse)

src/api/uploads/PlainUpload.js

[error] 131-131: expected ) but instead found :

Remove :

(parse)


[error] 131-131: expected , but instead found |

Remove |

(parse)


[error] 131-131: expected , but instead found )

Remove )

(parse)


[error] 131-131: Expected a class method body but instead found ','.

Expected a class method body here.

(parse)


[error] 132-132: Expected a statement but instead found ':'.

Expected a statement here.

(parse)


[error] 133-133: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 134-134: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: lint_test_build
  • GitHub Check: Summary
🔇 Additional comments (2)
src/api/uploads/BaseUpload.js (1)

162-165: Early–exit logic looks correct ✅

Nice addition — immediately short-circuiting on overwrite === 'error' prevents the rename/overwrite code paths from running. No further action required here.

src/api/uploads/PlainUpload.js (1)

119-120: Good JSDoc extension

The param docs now accurately describe the 'error' mode. 👍

@psevertson
Copy link
Contributor Author

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

@tjuanitas
Copy link
Contributor

@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 ™️

@psevertson psevertson force-pushed the master branch 2 times, most recently from f9e378b to 99d3115 Compare June 17, 2025 16:20
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (3)
src/api/uploads/MultiputUpload.js (3)

221-233: Same suppression repeat – apply cast pattern
See previous comment; apply the same fix here.


443-453: Third occurrence – remove $FlowFixMe
Consistent fix as above.


341-345: Retry still scheduled after fatal conflict – duplicate error callbacks possible
resolveConflict now bails out early when overwrite === 'error', yet the caller immediately schedules createSessionRetry(). This results in:

  1. errorCallback firing once in resolveConflict
  2. A retry, followed by additional failures / callbacks.

Prevent the redundant retry:

-            if (errorData && errorData.status === 409) {
-                this.resolveConflict(errorData);
-                this.createSessionRetry();
-                return;
-            }
+            if (errorData && errorData.status === 409) {
+                if (this.overwrite === 'error') {
+                    this.errorCallback(errorData);
+                    return;
+                }
+                this.resolveConflict(errorData);
+                this.createSessionRetry();
+                return;
+            }
🧹 Nitpick comments (6)
src/api/uploads/BaseUpload.js (2)

33-35: Union type addition looks correct, but please document the tri-state clearly
The field now accepts boolean | 'error'; consider updating the file-level JSDoc (top of file) so other engineers immediately understand the three behaviours.


161-166: Short-circuit branch added – consider extracting literal 'error' to a constant
The early return correctly prevents retries when overwrite === 'error'. To avoid future typos and make IDE refactors easier, introduce a shared constant, e.g.

+export const OVERWRITE_MODE_ERROR = 'error';
...
- if (this.overwrite === 'error') {
+ if (this.overwrite === OVERWRITE_MODE_ERROR) {
src/api/uploads/MultiputUpload.js (1)

176-187: Flow suppression can be removed – cast the default instead of $FlowFixMe
Explicitly cast the default value to the union to keep CI green without suppressions:

-        // $FlowFixMe
-        overwrite = true,
+        overwrite: boolean | 'error' = (true: boolean),

Repeat for the other overloads below.

src/api/uploads/PlainUpload.js (3)

119-120: JSDoc updated – consider mirroring wording used in other upload classes
To keep docs consistent across upload helpers, reuse the phrasing from BaseUpload/MultiputUpload.


131-133: Avoid $FlowFixMe by casting default
Same pattern as MultiputUpload:

-        // $FlowFixMe
-        overwrite = true,
+        overwrite: boolean | 'error' = (true: boolean),

This keeps Flow happy without suppressions.


140-141: Parameter type redeclaration warning – harmless but avoidable
The type of overwrite is redeclared in the object-type annotation; removing it there eliminates Biome’s “noRedeclare” noise:

-        overwrite: boolean | 'error',
+        overwrite: boolean | 'error',

(only one declaration required)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between db85d08 and 99d3115.

📒 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] 452-452: expected , but instead found overwrite

Remove overwrite

(parse)


[error] 452-452: Expected an expression, or an assignment but instead found ':'.

Expected an expression, or an assignment here.

(parse)


[error] 1246-1246: Illegal return statement outside of a function

(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 (1)
  • GitHub Check: Summary
🔇 Additional comments (1)
src/api/uploads/MultiputUpload.js (1)

1244-1247: Early exit on conflict correctly implemented
Conflicts are now surfaced immediately when overwrite === 'error'; implementation looks good.

@psevertson
Copy link
Contributor Author

@tjuanitas Ok, should be good now

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/api/uploads/MultiputUpload.js (1)

176-187: Consider dropping the repeated // $FlowFixMe and using an explicit param type instead

Three different method signatures still rely on a // $FlowFixMe to silence Flow around the default value for overwrite. Since the parameter is now explicitly typed as boolean | 'error', Flow should accept the default without complaint on current builds. Removing the pragma keeps the codebase cleaner and makes future Flow/TS migrations easier.

-        // $FlowFixMe
-        overwrite = true,
+        overwrite: boolean | 'error' = true,

Repeat the same change in upload() and resume().

If the old Flow version still errors, a local type alias can be introduced instead of scatter-shot suppressions:

type OverwriteMode = boolean | 'error';

This trims noise while retaining type safety.

Also applies to: 221-233, 446-457

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 99d3115 and 35ecee0.

📒 Files selected for processing (1)
  • src/api/uploads/MultiputUpload.js (8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
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)

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: lint_test_build
  • GitHub Check: Summary
🔇 Additional comments (1)
src/api/uploads/MultiputUpload.js (1)

342-345: Short-circuit for 'error' overwrite looks correct

The early‐return prevents both resolveConflict() and createSessionRetry(), eliminating the duplicate network request that was previously called out in review. 👍

Copy link
Contributor

@tjuanitas tjuanitas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
src/api/uploads/PlainUpload.js (1)

131-133: Avoid blanket // $FlowFixMe; cast the default instead

overwrite already accepts a boolean, so Flow can be satisfied without suppressing the type checker:

-        // $FlowFixMe
-        overwrite = true,
+        overwrite = (true: boolean | 'error'),

This removes a global suppression comment, keeps the intent explicit, and prevents accidental masking of unrelated Flow errors in the future.

src/api/uploads/BaseUpload.js (1)

162-165: Consider cleaning up the upload object after emitting the conflict error

When overwrite === 'error' you invoke errorCallback and return, but the upload object (timers, XHR handles, workers) remains alive. This can leak resources if the consumer does not call cancel() manually.

-            if (this.overwrite === 'error') {
-                this.errorCallback(errorData);
-                return;
-            }
+            if (this.overwrite === 'error') {
+                this.errorCallback(errorData);
+                this.destroy(); // prevent stray retries / listeners
+                return;
+            }

Please verify this aligns with how callers expect the lifecycle to behave.

src/api/uploads/MultiputUpload.js (2)

176-178: Replace repeated // $FlowFixMe with a typed default

Same suggestion as for PlainUpload: casting the default value keeps the type-checker happy without suppressing it.

-        // $FlowFixMe
-        overwrite = true,
+        overwrite = (true: boolean | 'error'),

Apply to every constructor/overload where the pattern occurs to reduce noise and future maintenance risk.

Also applies to: 221-223, 446-448


342-345: Early-exit branch still leaves the session object alive

Mirroring the concern in BaseUpload, returning after errorCallback means any pending retry timers (createSessionTimeout) stay scheduled. Add a cleanup to avoid duplicate error surfaces and wasted bandwidth:

-                if (this.overwrite === 'error') {
-                    this.errorCallback(errorData);
-                    return;
-                }
+                if (this.overwrite === 'error') {
+                    this.errorCallback(errorData);
+                    this.destroy();
+                    return;
+                }

Double-check that calling destroy() here will not break UI expectations (e.g., cancel buttons, progress components).

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 35ecee0 and 9bdb06f.

📒 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

@psevertson psevertson force-pushed the master branch 2 times, most recently from 42d38a2 to 3bf945c Compare June 23, 2025 16:13
@tjuanitas tjuanitas requested a review from a team as a code owner June 30, 2025 18:28
@tjuanitas
Copy link
Contributor

@mergify queue

Copy link
Contributor

mergify bot commented Jun 30, 2025

queue

🛑 This queue command has been cancelled by a dequeue command

@tjuanitas
Copy link
Contributor

@Mergifyio refresh

Copy link
Contributor

mergify bot commented Jun 30, 2025

refresh

✅ Pull request refreshed

@tjuanitas
Copy link
Contributor

@mergify dequeue

Copy link
Contributor

mergify bot commented Jun 30, 2025

dequeue

✅ The pull request is not waiting to be queued anymore.

@tjuanitas
Copy link
Contributor

@mergify queue

Copy link
Contributor

mergify bot commented Jun 30, 2025

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
      • #approved-reviews-by >= 1 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • label != do-not-merge
      • status-success = license/cla
      • status-success = lint_pull_request
      • status-success = lint_test_build
      • any of: [🛡 GitHub branch protection]
        • check-success = Summary
        • check-neutral = Summary
        • check-skipped = Summary
      • any of: [🛡 GitHub branch protection]
        • check-success = lint_test_build
        • check-neutral = lint_test_build
        • check-skipped = lint_test_build
      • any of: [🛡 GitHub branch protection]
        • 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
      • #approved-reviews-by >= 1 [🛡 GitHub branch protection]
      • #approved-reviews-by >= 2
      • #changes-requested-reviews-by = 0
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved = 0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • branch-protection-review-decision = APPROVED
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • label != do-not-merge
      • label = ready-to-merge
      • status-success = license/cla
      • status-success = lint_pull_request
      • status-success = lint_test_build
      • title ~= ^(build|ci|chore|docs|feat|fix|perf|refactor|revert|style|test)(\([^)]+\))?:\s.+$
      • any of: [🛡 GitHub branch protection]
        • check-success = Summary
        • check-neutral = Summary
        • check-skipped = Summary
      • any of: [🛡 GitHub branch protection]
        • check-success = lint_test_build
        • check-neutral = lint_test_build
        • check-skipped = lint_test_build
      • any of: [🛡 GitHub branch protection]
        • check-success = license/cla
        • check-neutral = license/cla
        • check-skipped = license/cla
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

@tjuanitas
Copy link
Contributor

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

@tjuanitas tjuanitas merged commit 463839e into box:master Jun 30, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants