fix: video attachments not reproducing on iOS - #7530
Conversation
GitHub Actions
Error
|
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:
WalkthroughAttachment overrides now use local state. Attachment URL encoding is centralized to prevent double encoding. Media rendering uses formatted URLs directly. Empty message persistence batches are logged and skipped. ChangesAttachment handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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
Error
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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)
app/lib/methods/handleMediaDownload.ts (1)
252-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the empty-batch path.
Mock the three lookups to return no records and assert that no database write occurs; also cover the normal update path.
🤖 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/handleMediaDownload.ts` around lines 252 - 262, Add regression coverage around the empty-batch guard in handleMediaDownload: mock all three lookup operations to return no records, assert db.write and db.batch are not called, and retain a separate test verifying the normal path performs the expected database update.app/lib/methods/helpers/formatAttachmentUrl.ts (2)
13-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for
encodeAttachmentUrl.This is new, exported, and central to fixing the double-encoding bug (spaces, already-encoded paths, malformed escapes falling back to the raw url). No test file was included for it in this diff — worth covering the round-trip and fallback cases directly.
🤖 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/helpers/formatAttachmentUrl.ts` around lines 13 - 27, Add unit tests for the exported encodeAttachmentUrl function covering plain URLs with spaces, already percent-encoded paths without double-encoding, and malformed escape sequences returning the original URL unchanged. Use the project’s existing test conventions and assert each round-trip and fallback behavior directly.
45-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
protectFilesbranching logic.The
if (protectFiles) return encodeAttachmentUrl(setParamInUrl(...)); return encodeAttachmentUrl(...)pattern is repeated identically for the http and CDN/relative branches, differing only in the input url. Worth extracting to avoid the two copies drifting apart later.♻️ Proposed extraction
+ const finalizeUrl = (url: string): string => + protectFiles ? encodeAttachmentUrl(setParamInUrl({ url, token, userId })) : encodeAttachmentUrl(url); + if (attachmentUrl && attachmentUrl.startsWith('http')) { if (_originalUrl && !_originalUrl.startsWith(server)) { return _originalUrl; } if (attachmentUrl.includes('rc_token')) { return encodeAttachmentUrl(attachmentUrl); } - if (protectFiles) return encodeAttachmentUrl(setParamInUrl({ url: attachmentUrl, token, userId })); - return encodeAttachmentUrl(attachmentUrl); + return finalizeUrl(attachmentUrl); } let cdnPrefix = store?.getState().settings.CDN_PREFIX as string; cdnPrefix = cdnPrefix?.trim(); if (cdnPrefix && cdnPrefix.startsWith('http')) { server = cdnPrefix.replace(/\/+$/, ''); } - if (protectFiles) return encodeAttachmentUrl(setParamInUrl({ url: `${server}${attachmentUrl}`, token, userId })); - return encodeAttachmentUrl(`${server}${attachmentUrl}`); + return finalizeUrl(`${server}${attachmentUrl}`);🤖 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/helpers/formatAttachmentUrl.ts` around lines 45 - 58, Refactor the attachment URL handling in formatAttachmentUrl so the duplicated protectFiles branching is consolidated into one shared flow. First determine the input URL for the HTTP and CDN/relative cases, then apply the existing protected-URL transformation and encodeAttachmentUrl call once, preserving the current behavior for both branches.
🤖 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/handleMediaDownload.ts`:
- Around line 252-258: Update the no-message branch in handleMediaDownload to
stop logging the raw messageId. Use the project’s redacted or appropriately
leveled logger, or remove the identifier from the message while preserving the
existing return behavior.
---
Nitpick comments:
In `@app/lib/methods/handleMediaDownload.ts`:
- Around line 252-262: Add regression coverage around the empty-batch guard in
handleMediaDownload: mock all three lookup operations to return no records,
assert db.write and db.batch are not called, and retain a separate test
verifying the normal path performs the expected database update.
In `@app/lib/methods/helpers/formatAttachmentUrl.ts`:
- Around line 13-27: Add unit tests for the exported encodeAttachmentUrl
function covering plain URLs with spaces, already percent-encoded paths without
double-encoding, and malformed escape sequences returning the original URL
unchanged. Use the project’s existing test conventions and assert each
round-trip and fallback behavior directly.
- Around line 45-58: Refactor the attachment URL handling in formatAttachmentUrl
so the duplicated protectFiles branching is consolidated into one shared flow.
First determine the input URL for the HTTP and CDN/relative cases, then apply
the existing protected-URL transformation and encodeAttachmentUrl call once,
preserving the current behavior for both branches.
🪄 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 Plus
Run ID: 01c038b0-fec7-406f-a8e2-23bd86ebec52
📒 Files selected for processing (6)
app/containers/message/hooks/__tests__/useFile.test.tsapp/containers/message/hooks/useFile.tsxapp/containers/message/hooks/useMediaAutoDownload.tsxapp/lib/methods/handleMediaDownload.tsapp/lib/methods/helpers/formatAttachmentUrl.tsapp/views/AttachmentView.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 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/containers/message/hooks/useMediaAutoDownload.tsxapp/views/AttachmentView.tsxapp/lib/methods/handleMediaDownload.tsapp/containers/message/hooks/useFile.tsxapp/lib/methods/helpers/formatAttachmentUrl.tsapp/containers/message/hooks/__tests__/useFile.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
Files:
app/containers/message/hooks/useMediaAutoDownload.tsxapp/views/AttachmentView.tsxapp/lib/methods/handleMediaDownload.tsapp/containers/message/hooks/useFile.tsxapp/lib/methods/helpers/formatAttachmentUrl.tsapp/containers/message/hooks/__tests__/useFile.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Before committing changes to JavaScript or TypeScript files, runpnpm prettier-lintandTZ=UTC pnpm testfor the modified files.
Use the local-first data flow: the UI reads from WatermelonDB, while sagas synchronize data with the server.
Use Redux and Redux-Saga for global or server state, and use Zustand for feature-local stores; do not assume all state is in Redux.
Files:
app/containers/message/hooks/useMediaAutoDownload.tsxapp/views/AttachmentView.tsxapp/lib/methods/handleMediaDownload.tsapp/containers/message/hooks/useFile.tsxapp/lib/methods/helpers/formatAttachmentUrl.tsapp/containers/message/hooks/__tests__/useFile.test.ts
🧠 Learnings (4)
📚 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/containers/message/hooks/useMediaAutoDownload.tsxapp/views/AttachmentView.tsxapp/lib/methods/handleMediaDownload.tsapp/containers/message/hooks/useFile.tsxapp/lib/methods/helpers/formatAttachmentUrl.tsapp/containers/message/hooks/__tests__/useFile.test.ts
📚 Learning: 2026-06-25T18:37:44.793Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.tsx:101-141
Timestamp: 2026-06-25T18:37:44.793Z
Learning: In the Rocket.Chat React Native codebase, do not treat passing an `async` function directly to an event prop in React/React Native UI components (e.g., `onPress={async () => ...}` in TSX) as a “floating promises” CI-blocking lint issue—this repo does not enable the ESLint `no-floating-promises` rule (while `no-void` is enforced). Only raise robustness follow-ups when there are genuinely unhandled promise paths (e.g., fire-and-forget calls like `save()` that return a Promise that is neither awaited nor handled), and prefer making sure failure paths are explicitly handled/reported rather than blocking on lint-style floating-promise concerns.
Applied to files:
app/containers/message/hooks/useMediaAutoDownload.tsxapp/views/AttachmentView.tsxapp/containers/message/hooks/useFile.tsx
📚 Learning: 2026-06-24T22:58:43.390Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7157
File: app/views/MessagesView/index.tsx:392-392
Timestamp: 2026-06-24T22:58:43.390Z
Learning: When wrapping a React Native component (e.g., via `withSafeAreaInsets`) ensure `hoistNonReactStatics` is only required if the wrapped component actually defines static properties/methods that consumers rely on. If the component has no statics (as in `app/views/MessagesView/index.tsx`), you can omit `hoistNonReactStatics` for this case.
Applied to files:
app/views/AttachmentView.tsx
📚 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/containers/message/hooks/__tests__/useFile.test.ts
🔇 Additional comments (5)
app/lib/methods/handleMediaDownload.ts (1)
252-262: Run the required TypeScript checks before merging.Run
pnpm prettier-lintandTZ=UTC pnpm testfor the modified files, as required by the repository guidelines. As per coding guidelines, these checks are required for JavaScript and TypeScript changes.Source: Coding guidelines
app/containers/message/hooks/useFile.tsx (1)
16-26: 🎯 Functional CorrectnessVerify overrides can't leak across a genuinely different attachment, not just a cosmetic
fileprop update.The hook now merges
overridesunconditionally and never resets them, which the tests confirm is intentional when the same attachment's metadata changes (e.g., atitleedit) — the local download URI should still win. But there's no reset keyed off the attachment's own identity (e.g.title_link/video_url/image_url), so if the same component instance ever renders a different underlying attachment (e.g. a message edit that swaps the attached video while the message id/component instance is unchanged), the stale local override from the previous download would still be applied on top of the new, unrelated remote url.Can you confirm whether
useFile's call sites are always remounted (not reused) whenever the underlying attachment actually changes content, as opposed to just metadata? If reuse across different attachments is possible, consider resettingoverrideswhen a stable identity field offilechanges.app/containers/message/hooks/__tests__/useFile.test.ts (1)
6-46: LGTM!app/containers/message/hooks/useMediaAutoDownload.tsx (1)
85-85: LGTM!app/views/AttachmentView.tsx (1)
62-64: LGTM!Also applies to: 77-77
|
iOS Build Available Rocket.Chat 4.75.0.109454 |
OtavioStasiak
changed the title
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.75.0.109457 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNTegnMqFz9_7qNkhm2HLx1aaLyvIXa9iPKYnDgCmaLRZM2zoQZLyU_jV6RA8kapL7Le4Ybv4I5FxVkbci5u |
|
iOS Build Available Rocket.Chat 4.75.0.109458 |
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
|
iOS Build Available Rocket.Chat 4.76.0.109502 |
Code reviewThe Found 2 issues: 1. Inline message images still double-encode, and this PR breaks the one config that worked
The For
With 2.
if (_originalUrl && !_originalUrl.startsWith(server)) {
return encodeAttachmentUrl(_originalUrl);
}The Minor: the malformed-escape test asserts correct behavior, but not for the stated reason — WHATWG |
GitHub Actions
Waiting
|
iOS Build Available Rocket.Chat 4.76.0.109503 |
2 participants
Proposed changes
Media attachments could keep pointing at the remote URL even after the file was already cached on disk, so opening them streamed from the server instead of playing the local file. On iOS that surfaced as a video thumbnail that spins
forever with no error.
Root cause.
useFileonly merged local overrides for non-persisted messages, relying onpersistMessageto write the downloaded uri back to the database for everything else. That write silently no-ops whenever no row matches the message id, which is the case for forwarded messages and for the Files/Mentions/Starred/Pinned lists —MessagesViewbuilds attachments from the uploads REST payload, which has_idbut noidand no message id at all, souseMessageId()isundefined. Downloading from the Files tab therefore left the room's message row holding the remote URL while the file sat fully downloaded on disk.Why only iOS, and only some videos. The broken state is identical on both platforms; the difference is what each player does when asked to stream the file. An MP4/MOV can't start until the player has the
moovatom (sampletables, byte offsets, durations). macOS screen recordings and
az_recorderwritemoovat the end of the file — they can't know the sample table until recording stops — whereas the iPhone camera writes faststart (moovfirst).Reaching a trailing
moovover HTTP requires working Range requests. AVFoundation assumes Range works and simply waits: the item stays inAVPlayerItemStatusUnknown, so expo-av never firesonLoad(needsReadyToPlay) oronError(needsAVPlayerItemStatusFailed), andloadingstaystrue. ExoPlayer falls back to reading forward from byte 0 until it findsmoov— wasteful, but it terminates, so Android just loaded slowly. The absence of an error alert was the tell: a 404/403 would have popped the view withError_play_video.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1307
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Bug Fixes
Improvements
Tests