fix: use fileId for upload queue identity instead of file.name#3785
fix: use fileId for upload queue identity instead of file.name#3785
Conversation
The reducer matches queue items by file.name, so two files with the same name (e.g. from different sub-folders) collide: an action targeting one updates both. These 3 tests demonstrate the bug. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The reducer matched queue items by file.name, causing two files with the same name (e.g. from different sub-folders) to collide. Switching to a dedicated fileId field (defaults to file.name for backward compat) fixes this and prepares for folder upload flattening. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Crash--
requested review from
JF-Cozy,
doubleface,
lethemanh,
rezk2ll and
zatteo
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughQueue items now include a normalized Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
src/modules/upload/index.js
Outdated
| } | ||
|
|
||
| const { file, entry, isDirectory } = item | ||
| const { file, fileId, entry, isDirectory } = item |
There was a problem hiding this comment.
I think we should merge this fix as is.
i'm working on improving things in an other PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/modules/upload/index.js (2)
485-487: Good backward-compatible fallback inremoveFileToUploadQueue.The
fileId ?? file.namefallback ensures callers that don't yet providefileIdwill continue to work.Minor: Prettier flagged the formatting. Consider expanding to multiple lines.
Formatting fix
export const removeFileToUploadQueue = (file, fileId) => async dispatch => { - dispatch({ type: RECEIVE_UPLOAD_SUCCESS, fileId: fileId ?? file.name, file, isUpdate: true }) + dispatch({ + type: RECEIVE_UPLOAD_SUCCESS, + fileId: fileId ?? file.name, + file, + isUpdate: true + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/upload/index.js` around lines 485 - 487, The function removeFileToUploadQueue currently uses a compact one-line arrow that Prettier flagged; refactor it into a multi-line arrow function to improve formatting and readability: locate export const removeFileToUploadQueue and expand the async dispatch => block so the dispatch({ type: RECEIVE_UPLOAD_SUCCESS, fileId: fileId ?? file.name, file, isUpdate: true }) call is on its own line(s) with proper indentation and trailing semicolon, preserving the fallback behavior (fileId ?? file.name) and the action type RECEIVE_UPLOAD_SUCCESS.
225-225: Minor: Format the inline dispatch object.ESLint/Prettier flagged this line for formatting.
Suggested fix
- dispatch({ type: RECEIVE_UPLOAD_SUCCESS, fileId, file, uploadedItem: newDir }) + dispatch({ + type: RECEIVE_UPLOAD_SUCCESS, + fileId, + file, + uploadedItem: newDir + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/upload/index.js` at line 225, The inline dispatch call dispatch({ type: RECEIVE_UPLOAD_SUCCESS, fileId, file, uploadedItem: newDir }) is flagged by ESLint/Prettier for formatting; reformat the object literal passed to dispatch (the call in which RECEIVE_UPLOAD_SUCCESS, fileId, file, and uploadedItem: newDir are used) into a properly indented multi-line object with each property on its own line and appropriate trailing commas to satisfy the project's linting rules.src/modules/upload/index.spec.js (1)
679-679: Minor: Format the inline object across multiple lines.ESLint/Prettier flagged this line for formatting.
Suggested fix
- const result3 = queue(result2, { type: 'RECEIVE_UPLOAD_ERROR', fileId, file }) + const result3 = queue(result2, { + type: 'RECEIVE_UPLOAD_ERROR', + fileId, + file + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/upload/index.spec.js` at line 679, The inline object passed to queue in the test is currently on one line (const result3 = queue(result2, { type: 'RECEIVE_UPLOAD_ERROR', fileId, file })) and needs to be formatted across multiple lines to satisfy ESLint/Prettier; update the call to queue(result2, { ... }) so the object properties (type, fileId, file) are each on their own lines (and keep result3 and result2 identifiers unchanged) ensuring commas and indentation follow the project's formatting rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/upload/index.js`:
- Around line 56-61: itemInitialState currently assumes item.file is non-null
and accesses item.file.name which throws for directory entries from
extractFilesEntries (where item.getAsFile() returns null); update
itemInitialState to derive fileId safely using optional chaining and a fallback
to the entry name (e.g. use item.file?.name ?? item.name or similar) so
directories get a valid fileId without accessing .name on null; ensure this
change is applied to the itemInitialState function so the reducer call that
initializes queue entries no longer causes a TypeError.
---
Nitpick comments:
In `@src/modules/upload/index.js`:
- Around line 485-487: The function removeFileToUploadQueue currently uses a
compact one-line arrow that Prettier flagged; refactor it into a multi-line
arrow function to improve formatting and readability: locate export const
removeFileToUploadQueue and expand the async dispatch => block so the dispatch({
type: RECEIVE_UPLOAD_SUCCESS, fileId: fileId ?? file.name, file, isUpdate: true
}) call is on its own line(s) with proper indentation and trailing semicolon,
preserving the fallback behavior (fileId ?? file.name) and the action type
RECEIVE_UPLOAD_SUCCESS.
- Line 225: The inline dispatch call dispatch({ type: RECEIVE_UPLOAD_SUCCESS,
fileId, file, uploadedItem: newDir }) is flagged by ESLint/Prettier for
formatting; reformat the object literal passed to dispatch (the call in which
RECEIVE_UPLOAD_SUCCESS, fileId, file, and uploadedItem: newDir are used) into a
properly indented multi-line object with each property on its own line and
appropriate trailing commas to satisfy the project's linting rules.
In `@src/modules/upload/index.spec.js`:
- Line 679: The inline object passed to queue in the test is currently on one
line (const result3 = queue(result2, { type: 'RECEIVE_UPLOAD_ERROR', fileId,
file })) and needs to be formatted across multiple lines to satisfy
ESLint/Prettier; update the call to queue(result2, { ... }) so the object
properties (type, fileId, file) are each on their own lines (and keep result3
and result2 identifiers unchanged) ensuring commas and indentation follow the
project's formatting rules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c5b1bce-0c0f-4212-9847-ed698d27e06f
📒 Files selected for processing (2)
src/modules/upload/index.jssrc/modules/upload/index.spec.js
- Fix Prettier formatting violations (line wrapping) - Use optional chaining in itemInitialState to handle directory entries where file is null: item.file?.name ?? item.entry?.name Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
BundleMonFiles updated (2)
Unchanged files (19)
Total files change +408B +0.01% Groups updated (1)
Unchanged groups (2)
Final result: ✅ View report in BundleMon website ➡️ |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/upload/index.js`:
- Line 160: The reducer matches queue items by action.fileId but UploadUtils.ts
dispatches RECEIVE_UPLOAD_SUCCESS and RECEIVE_UPLOAD_ERROR without fileId, so
add fileId to those dispatched actions; in UploadUtils.ts update the dispatch
calls for RECEIVE_UPLOAD_SUCCESS and RECEIVE_UPLOAD_ERROR to include fileId
(e.g. fileId: file.fileId or the same identifier used when the upload was
enqueued) so action.fileId matches the reducer's comparison. Ensure the
dispatched payload keys exactly match the reducer (action.fileId) so the item(i,
action) branch runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05c17e4f-854c-40e9-b746-3a123c7f2757
📒 Files selected for processing (2)
src/modules/upload/index.jssrc/modules/upload/index.spec.js
✅ Files skipped from review due to trivial changes (1)
- src/modules/upload/index.spec.js
The Flagship upload path (UploadUtils.ts) dispatches RECEIVE_UPLOAD_SUCCESS and RECEIVE_UPLOAD_ERROR without fileId. The reducer now falls back to matching on file.name when fileId is absent, ensuring these dispatches still update the correct queue item. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Crash--
force-pushed
the
fix/upload-queue-fileId-identity
branch
from
b360e9d to
20a8482
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Extract getUploadErrorStatus() and handleConflictOverwrite() to lower cyclomatic complexity flagged by CodeScene. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Crash--
force-pushed
the
fix/upload-queue-fileId-identity
branch
from
20a8482 to
fa56779
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/upload/index.js`:
- Around line 56-59: itemInitialState currently falls back to
file.name/entry.name which allows collisions; ensure the enqueueing code
(extractFilesEntries and UploadUtils) populates a stable item.fileId before
dispatching ADD_TO_UPLOAD_QUEUE instead of relying on itemInitialState fallback.
Fix by generating/assigning a deterministic unique id (e.g., UUID or
content-based hash or monotonic counter) for each file/entry when creating the
item in extractFilesEntries / the UploadUtils enqueue path and set item.fileId
there so ADD_TO_UPLOAD_QUEUE receives items with a stable fileId. Ensure the
generated id is used by itemInitialState if present so identical basenames no
longer collide.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3889db3e-308a-4aa7-b165-956105405726
📒 Files selected for processing (1)
src/modules/upload/index.js
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
extractFilesEntries now sets fileId from entry.fullPath, and generateForQueue uses file.filePath for Flagship uploads. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Crash--
force-pushed
the
fix/upload-queue-fileId-identity
branch
from
5546e10 to
133e44b
Compare
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Passed
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| index.js | 8.37 → 8.79 | Complex Method |
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
1 participant
Summary
file.name, causing two files with the same name (e.g.summer/photo.jpgandwinter/photo.jpg) to collide — an action targeting one incorrectly updates bothfileIdfield on queue items (defaults tofile.namefor backward compatibility) used as the identity key in the reducerTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests