fix: reuse stale upload record instead of blocking re-send#7494
Conversation
GitHub Actions
Inactive
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
WalkthroughExisting upload records are checked for active uploads before blocking retries. Stale records have their error and progress reset and are reused, with tests covering active, stale, forced-retry, and new-record scenarios. ChangesUpload retry handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
GitHub Actions
Inactive
![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 (1)
app/lib/methods/sendFileMessage/sendFileMessage.ts (1)
33-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated stale-record reset logic into a shared helper.
The
db.write+uploadRecord.updateblock that resetserrorandprogressis identical toutils.tslines 83-88. If the reset fields or logic change, both sites must be updated in lockstep. Extract a shared helper inutils.tsand call it from bothcreateUploadRecordandsendFileMessage.♻️ Proposed shared helper in
utils.ts+export const resetStaleUploadRecord = async (uploadRecord: TUploadModel): Promise<void> => { + const db = database.active; + await db.write(async () => { + await uploadRecord.update(u => { + u.error = false; + u.progress = 0; + }); + }); +};Then in both call sites, replace the inline
db.writeblock:- // Record left behind by a crashed or failed upload: reset and reuse it. - await db.write(async () => { - await uploadRecord?.update(u => { - u.error = false; - u.progress = 0; - }); - }); + // Record left behind by a crashed or failed upload: reset and reuse it. + await resetStaleUploadRecord(uploadRecord);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/methods/sendFileMessage/sendFileMessage.ts` around lines 33 - 43, Extract the duplicated stale-upload reset operation into a shared helper in utils.ts, including the db.write wrapper and uploadRecord.update logic that clears error and resets progress. Update both createUploadRecord and sendFileMessage to call this helper instead of maintaining inline reset blocks, preserving the existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/methods/sendFileMessage/utils.test.ts`:
- Around line 57-65: Add an active upload entry to uploadQueue in the “reuses
the existing record when force-retry” test before calling createUploadRecord, so
isUploadActive would otherwise be true. Keep isForceTryAgain set to true and
retain the existing assertions, ensuring the test verifies that force retry
overrides the active-upload state.
---
Nitpick comments:
In `@app/lib/methods/sendFileMessage/sendFileMessage.ts`:
- Around line 33-43: Extract the duplicated stale-upload reset operation into a
shared helper in utils.ts, including the db.write wrapper and
uploadRecord.update logic that clears error and resets progress. Update both
createUploadRecord and sendFileMessage to call this helper instead of
maintaining inline reset blocks, preserving the existing behavior.
🪄 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: a2b48032-4205-4ff2-b000-a38b1e711586
📒 Files selected for processing (3)
app/lib/methods/sendFileMessage/sendFileMessage.tsapp/lib/methods/sendFileMessage/utils.test.tsapp/lib/methods/sendFileMessage/utils.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: E2E Build Android / android-build
- GitHub Check: E2E Build iOS / ios-build
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/methods/sendFileMessage/utils.tsapp/lib/methods/sendFileMessage/sendFileMessage.tsapp/lib/methods/sendFileMessage/utils.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
**/*.{ts,tsx}: Use TypeScript in strict mode, with imports resolved from theapp/baseUrl
Follow the repository's TypeScript/React Native code style enforced by Prettier: tabs, single quotes, 130-character width, no trailing commas, omit arrow-function parens when possible, and keep brackets on the same line
Files:
app/lib/methods/sendFileMessage/utils.tsapp/lib/methods/sendFileMessage/sendFileMessage.tsapp/lib/methods/sendFileMessage/utils.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the project's ESLint setup (
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins) for code quality and style compliance
Files:
app/lib/methods/sendFileMessage/utils.tsapp/lib/methods/sendFileMessage/sendFileMessage.tsapp/lib/methods/sendFileMessage/utils.test.ts
🧠 Learnings (2)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/methods/sendFileMessage/utils.tsapp/lib/methods/sendFileMessage/sendFileMessage.tsapp/lib/methods/sendFileMessage/utils.test.ts
📚 Learning: 2026-06-25T18:37:25.526Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.test.tsx:16-22
Timestamp: 2026-06-25T18:37:25.526Z
Learning: In Rocket.Chat ReactNative tests that mock selectors for `useAppSelector`, don’t require the mocked selector input to be typed as `IApplicationState` when the fixture only includes a partial Redux state slice (e.g., only `server` and `settings`). Requiring the full `IApplicationState` type in that scenario forces unsafe `as IApplicationState` casts and undermines type-safety. For these narrowly scoped selector-mock fixtures, use a less strict type (e.g., `any`) to keep the mock focused on the slice under test.
Applied to files:
app/lib/methods/sendFileMessage/utils.test.ts
🔇 Additional comments (2)
app/lib/methods/sendFileMessage/utils.ts (1)
77-88: LGTM!app/lib/methods/sendFileMessage/sendFileMessage.ts (1)
10-10: LGTM!
|
iOS Build Available Rocket.Chat 4.75.0.109348 |
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Error
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/methods/sendFileMessage/sendFileMessage.ts (1)
31-39: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the original upload path for error handling. Reassigning
fileInfo.pathhere makespersistUploadError(fileInfo.path, rid)search by the cached path, but the upload record was created with the original path. Use a localcachedPathfor the multipart body and leavefileInfo.pathunchanged.♻️ Proposed fix
- fileInfo.path = await copyFileToCacheDirectoryIfNeeded(fileInfo.path, fileInfo.name); + const cachedPath = await copyFileToCacheDirectoryIfNeeded(fileInfo.path, fileInfo.name); @@ - uri: fileInfo.path + uri: cachedPath🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/methods/sendFileMessage/sendFileMessage.ts` around lines 31 - 39, Preserve the original path in fileInfo.path for persistUploadError by storing the result of copyFileToCacheDirectoryIfNeeded in a local cachedPath variable. Use cachedPath as the multipart formData uri while leaving fileInfo.path unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/lib/methods/sendFileMessage/sendFileMessage.ts`:
- Around line 31-39: Preserve the original path in fileInfo.path for
persistUploadError by storing the result of copyFileToCacheDirectoryIfNeeded in
a local cachedPath variable. Use cachedPath as the multipart formData uri while
leaving fileInfo.path unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a87ec24-a704-4707-8b2d-7213f75af20c
📒 Files selected for processing (1)
app/lib/methods/sendFileMessage/sendFileMessage.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/methods/sendFileMessage/sendFileMessage.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
**/*.{ts,tsx}: Use TypeScript in strict mode, with imports resolved from theapp/baseUrl
Follow the repository's TypeScript/React Native code style enforced by Prettier: tabs, single quotes, 130-character width, no trailing commas, omit arrow-function parens when possible, and keep brackets on the same line
Files:
app/lib/methods/sendFileMessage/sendFileMessage.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the project's ESLint setup (
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins) for code quality and style compliance
Files:
app/lib/methods/sendFileMessage/sendFileMessage.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/methods/sendFileMessage/sendFileMessage.ts
An upload record left in the DB by a crashed or failed upload permanently blocked re-sending the same file, surfacing as "Couldn't create upload record". The guard treated "record exists in DB" as "upload in progress"; the real in-progress signal is the in-memory uploadQueue, which is empty after a crash. Bail with the in-progress alert only when the upload is actually active (isUploadActive); otherwise reset the stale record (error/progress) and reuse it so the send proceeds. Applied to both the V2 and legacy V1 paths.
73cdb97 to
05be42a
Compare
GitHub Actions
Waiting
GitHub Actions
Waiting
2 participants
Proposed changes
An upload record left behind in the local DB by a crashed or failed upload permanently blocked re-sending the same file. On the next (non–"try again") send of that file the app threw
Couldn't create upload recordand showed an "Upload in progress" alert, even though nothing was uploading.Root cause: the guard in
createUploadRecordtreated "a record with this path exists in the DB" as "an upload is in progress". That is wrong after a crash/restart — the DB row survives, but the real in-progress signal is the in-memoryuploadQueue, which is empty.Fix: show the "Upload in progress" alert and bail only when the upload is actually active (
isUploadActive). Otherwise the existing row is a stale orphan — reset itserror/progressand reuse it so the send proceeds. Applied to both the V2 path (createUploadRecord) and the legacy V1 path (sendFileMessage.ts, servers < 6.10).Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1422
How to test or reproduce
uploadsrow is left behind.Couldn't create upload record/ "Upload in progress". After: the file uploads normally.Screenshots
Types of changes
Checklist
Further comments
Two related items intentionally left out of scope: (1) the native ObjC-interop exception that can crash the app mid-upload (a separate de-interop/symbolication track — this PR only makes the JS side resilient to the orphan it leaves), and (2) the
catchbranch that logsUpload cancelledand silently swallows genuine early-failure errors without rethrowing.Summary by CodeRabbit
Bug Fixes
Tests