Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
653bbd5
fix: video attachments stuck loading forever on iOS
OtavioStasiak Jul 29, 2026
cf2adbb
chore: code improvements
OtavioStasiak Jul 29, 2026
2fc4f68
chore: code improvements
OtavioStasiak Jul 29, 2026
1b5bf97
test: add coverage for encodeAttachmentUrl
OtavioStasiak Aug 3, 2026
39f01dd
Merge branch 'develop' into fix.video-not-playing
OtavioStasiak Aug 3, 2026
87ede68
Merge branch 'develop' into fix.video-not-playing
OtavioStasiak Aug 5, 2026
c9abe98
refactor: encode attachment urls via the URL parser
OtavioStasiak Aug 5, 2026
1f1cc90
fix: double-encoded attachment urls in inline media
OtavioStasiak Aug 6, 2026
5fdc044
Merge branch 'develop' into fix.video-not-playing
OtavioStasiak Aug 6, 2026
8d6a891
fix: double-encoded attachment urls in inline media
OtavioStasiak Aug 6, 2026
04149da
fix: encode local file uris for non-ascii filenames
OtavioStasiak Aug 7, 2026
eb86b19
Merge branch 'develop' into fix.video-not-playing
OtavioStasiak Aug 10, 2026
e5d20d9
remove comments
OtavioStasiak Aug 13, 2026
370355f
Merge branch 'develop' into fix.video-not-playing
OtavioStasiak Aug 14, 2026
c2671d3
fix: merge conflicts
OtavioStasiak Aug 14, 2026
dffddf6
chore: remove console.log from persistMessage empty-batch path
OtavioStasiak Aug 14, 2026
6b5b2a2
remove unused comment
OtavioStasiak Aug 14, 2026
84196a4
fix: encode attachment urls at the player sinks instead of formatAtta…
OtavioStasiak Aug 14, 2026
70aa4a0
fix: test
OtavioStasiak Aug 14, 2026
b8363ad
chore: simplify useFile
OtavioStasiak Sep 1, 2026
e608d12
Merge branch 'develop' into fix.video-not-playing
OtavioStasiak Sep 1, 2026
2d98cd4
Merge branch 'develop' into fix.video-not-playing
OtavioStasiak Sep 1, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { useUserPreferences } from '../../../../../lib/methods/userPreferences';
import { AUTOPLAY_GIFS_PREFERENCES_KEY } from '../../../../../lib/constants/keys';
import ImageBadge from './ImageBadge';
import log from '../../../../../lib/methods/helpers/log';
import { encodeAttachmentUrl } from '../../../../../lib/methods/helpers/formatAttachmentUrl';

export const MessageImage = ({ uri, status, encrypted = false, imagePreview, imageType }: IMessageImage) => {
const { colors } = useTheme();
Expand Down Expand Up @@ -69,7 +70,7 @@ export const MessageImage = ({ uri, status, encrypted = false, imagePreview, ima
<>
{showImage ? (
<View style={[containerStyle, borderStyle]}>
<Image autoplay={autoplayGifs} style={imageStyle} source={{ uri: encodeURI(uri) }} contentFit='cover' />
<Image autoplay={autoplayGifs} style={imageStyle} source={{ uri: encodeAttachmentUrl(uri) }} contentFit='cover' />
</View>
) : null}
{['loading', 'to-download'].includes(status) || (status === 'downloaded' && !showImage) ? (
Expand Down
107 changes: 0 additions & 107 deletions app/containers/message/hooks/__tests__/useFile.test.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -47,19 +47,6 @@ jest.mock('../../../../lib/methods/helpers/emitter', () => {
};
});

// The persisted/localFile logic is covered by useFile.test; here useFile is a plain
// state cell so setCurrentFile merges are observable through the returned currentFile.
jest.mock('../useFile', () => {
const { useState } = require('react');
return {
useFile: (file: IAttachment) => {
const [current, setCurrent] = useState(file);
const manage = (partial: Partial<IAttachment>) => setCurrent((prev: IAttachment) => ({ ...prev, ...partial }));
return [current, manage];
}
};
});

const mockDownloadMediaFile = downloadMediaFile as jest.Mock;
const mockGetMediaCache = getMediaCache as jest.Mock;
const mockIsDownloadActive = isDownloadActive as jest.Mock;
Expand Down Expand Up @@ -101,7 +88,14 @@ const renderMediaHook = ({
</MessageRoomProvider>
</Provider>
);
return renderHook(() => useMediaAutoDownload({ file, author, showAttachment }), { wrapper });
return renderHook(
(props: { file: IAttachment; author?: IUserMessage; showAttachment?: (file: IAttachment) => void }) =>
useMediaAutoDownload(props),
{
wrapper,
initialProps: { file, author, showAttachment }
}
);
};

describe('useMediaAutoDownload', () => {
Expand Down Expand Up @@ -172,6 +166,17 @@ describe('useMediaAutoDownload', () => {
expect(result.current.currentFile.title_link).toBe('file://cache');
});

it('keeps the local title_link when the file prop changes back to the remote url', async () => {
mockGetMediaCache.mockResolvedValue({ exists: true, uri: 'file://cache' });
const { result, rerender } = renderMediaHook({ file: { image_url: '/img', title: 'original.png' } });
await waitFor(() => expect(result.current.status).toBe('downloaded'));

rerender({ file: { image_url: '/img', title: 'renamed.png', title_link: '/remote' } });

expect(result.current.currentFile.title).toBe('renamed.png');
expect(result.current.currentFile.title_link).toBe('file://cache');
});

it('resumes an active download by registering the download listener and showing loading', async () => {
mockIsDownloadActive.mockReturnValue(true);
const { result } = renderMediaHook({ file: { image_url: '/img' } });
Expand Down
30 changes: 0 additions & 30 deletions app/containers/message/hooks/useFile.tsx

This file was deleted.

15 changes: 6 additions & 9 deletions app/containers/message/hooks/useMediaAutoDownload.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useCallback, useEffect, useReducer } from 'react';
import { useCallback, useEffect, useReducer, useState } from 'react';

import { type IAttachment, type IUserMessage } from '../../../definitions';
import { isImageBase64 } from '../../../lib/methods/isImageBase64';
Expand All @@ -15,7 +15,6 @@ import { emitter } from '../../../lib/methods/helpers/emitter';
import { formatAttachmentUrl } from '../../../lib/methods/helpers/formatAttachmentUrl';
import { useBaseUrl, useMessageUser } from '../stores/MessageRoomStore';
import { useMessageId } from '../stores/MessageStore';
import { useFile } from './useFile';

const getFileType = (file: IAttachment): MediaTypes | null => {
if (file.image_url) {
Expand Down Expand Up @@ -80,7 +79,9 @@ export const useMediaAutoDownload = ({
const baseUrl = useBaseUrl();
const user = useMessageUser();
const [status, dispatchDownloadEvent] = useReducer(downloadStatusReducer, 'to-download');
const [currentFile, setCurrentFile] = useFile(file, id ?? '');
// Local overrides (downloaded uri, decrypted state) win over the file prop, which may still carry the remote url
const [fileOverrides, setFileOverrides] = useState<Partial<IAttachment> | null>(null);
const currentFile = fileOverrides ? { ...file, ...fileOverrides } : file;
const originalUrl = getOriginalURL(file);
const url = formatAttachmentUrl(
file.title_link || getFileProperty(currentFile, fileType, 'url'),
Expand Down Expand Up @@ -152,17 +153,13 @@ export const useMediaAutoDownload = ({
};

const updateCurrentFile = (uri: string) => {
setCurrentFile({
title_link: uri
});
setFileOverrides(prev => ({ ...prev, title_link: uri }));
dispatchDownloadEvent('download_succeeded');
};

const setDecrypted = () => {
if (isEncrypted) {
setCurrentFile({
e2e: 'done'
});
setFileOverrides(prev => ({ ...prev, e2e: 'done' }));
}
};

Expand Down
27 changes: 26 additions & 1 deletion app/lib/methods/handleMediaDownload.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getFilename, matchDownloadUrl, persistMessage } from './handleMediaDownload';
import { getFilePath, getFilename, matchDownloadUrl, persistMessage } from './handleMediaDownload';
import database from '../database';
import { getMessageById } from '../database/services/Message';
import { getThreadById } from '../database/services/Thread';
Expand Down Expand Up @@ -33,6 +33,22 @@ jest.mock('../database/services/ThreadMessage', () => ({
getThreadMessageById: jest.fn()
}));

jest.mock('../store/auxStore', () => ({
store: { getState: () => ({ server: { server: 'https://server.com' } }) }
}));

describe('getFilePath', () => {
it('derives the cache filename from the unencoded url', () => {
expect(
getFilePath({
type: 'video',
mimeType: 'video/quicktime',
urlToCache: 'https://server.com/file-upload/abc/Screen Recording.mov'
})
).toContain('/Screen_Recording.mov');
});
});

describe('matchDownloadUrl', () => {
it('matches when downloadUrl contains image_url', () => {
expect(
Expand Down Expand Up @@ -67,6 +83,15 @@ describe('matchDownloadUrl', () => {
matchDownloadUrl({ image_url: '/file-upload/abc/photo.jpg' }, 'https://server.com/file-upload/abc/audio.mp3')
).toBeFalsy();
});

it('matches an attachment url with a space against the download url', () => {
expect(
matchDownloadUrl(
{ video_url: '/file-upload/abc/Screen Recording.mov' },
'https://server.com/file-upload/abc/Screen Recording.mov'
)
).toBeTruthy();
});
});

describe('Test the getFilename', () => {
Expand Down
61 changes: 61 additions & 0 deletions app/lib/methods/helpers/__tests__/formatAttachmentUrl.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { encodeAttachmentUrl } from '../formatAttachmentUrl';

describe('encodeAttachmentUrl', () => {
it('encodes an unencoded path', () => {
expect(encodeAttachmentUrl('https://open.rocket.chat/file-upload/1/Screen Recording.mov')).toBe(
'https://open.rocket.chat/file-upload/1/Screen%20Recording.mov'
);
});

it('leaves an already-encoded path untouched', () => {
expect(encodeAttachmentUrl('https://open.rocket.chat/file-upload/1/Screen%20Recording.mov')).toBe(
'https://open.rocket.chat/file-upload/1/Screen%20Recording.mov'
);
});

it('leaves an already-escaped reserved character in the path', () => {
expect(encodeAttachmentUrl('https://open.rocket.chat/file-upload/1/a%20video%20%232.mov')).toBe(
'https://open.rocket.chat/file-upload/1/a%20video%20%232.mov'
);
});

// Known limitation, not a regression: `#` is the fragment delimiter per the URL spec, so a raw one ends the
// path and the rest becomes the fragment — which HTTP drops, so the server sees a truncated path. The previous
// encodeURI behaved identically (it leaves `#` unescaped too). Only reachable if the server sends a raw `#`.
it('treats a raw reserved `#` in the path as a fragment', () => {
expect(encodeAttachmentUrl('https://open.rocket.chat/file-upload/1/a video #2.mov')).toBe(
'https://open.rocket.chat/file-upload/1/a%20video%20#2.mov'
);
});

it('preserves the query string', () => {
expect(encodeAttachmentUrl('https://open.rocket.chat/file-upload/1/Screen Recording.mov?rc_token=abc&rc_uid=123')).toBe(
'https://open.rocket.chat/file-upload/1/Screen%20Recording.mov?rc_token=abc&rc_uid=123'
);
});

// WHATWG URL passes a malformed escape through rather than throwing, so this exercises the try branch.
it('returns the raw url when it has a malformed escape', () => {
expect(encodeAttachmentUrl('https://open.rocket.chat/file-upload/1/%ZZ.mov')).toBe(
'https://open.rocket.chat/file-upload/1/%ZZ.mov'
);
});

// A non-absolute url is what actually throws — reachable when the server/CDN prefix is empty.
it('returns the raw url when it is not absolute', () => {
expect(encodeAttachmentUrl('/file-upload/1/Screen Recording.mov')).toBe('/file-upload/1/Screen Recording.mov');
});

// Cache filenames keep unicode letters, so local uris need encoding before they reach the native players.
it('encodes unicode letters in a local file uri', () => {
expect(encodeAttachmentUrl('file:///var/app/Documents/server/msg1/vídeo.mov')).toBe(
'file:///var/app/Documents/server/msg1/v%C3%ADdeo.mov'
);
});

it('leaves an already-encoded local file uri untouched', () => {
expect(encodeAttachmentUrl('file:///var/app/Documents/server/msg1/v%C3%ADdeo.mov')).toBe(
'file:///var/app/Documents/server/msg1/v%C3%ADdeo.mov'
);
});
});
10 changes: 9 additions & 1 deletion app/lib/methods/helpers/formatAttachmentUrl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,21 @@
return urlObj.toString();
}

export const encodeAttachmentUrl = (url: string): string => {
Comment thread
OtavioStasiak marked this conversation as resolved.
try {
return new URL(url).toString();
} catch {
return url;
}
};

export const formatAttachmentUrl = (
attachmentUrl: string | undefined,
userId: string,
token: string,
server: string,
_originalUrl?: string | null
): string => {

Check warning on line 27 in app/lib/methods/helpers/formatAttachmentUrl.ts

View workflow job for this annotation

GitHub Actions / format

eslint(max-params)

Arrow function has too many parameters (5). Maximum allowed is 4.

Check warning on line 27 in app/lib/methods/helpers/formatAttachmentUrl.ts

View workflow job for this annotation

GitHub Actions / ESLint and Test / run-eslint-and-test

eslint(max-params)

Arrow function has too many parameters (5). Maximum allowed is 4.
const protectFiles = store.getState().settings.FileUpload_ProtectFiles;

if ((attachmentUrl && isImageBase64(attachmentUrl)) || attachmentUrl?.startsWith('file://')) {
Expand All @@ -28,7 +36,7 @@
}

if (attachmentUrl.includes('rc_token')) {
return encodeURI(attachmentUrl);
return encodeAttachmentUrl(attachmentUrl);
}

if (protectFiles) return setParamInUrl({ url: attachmentUrl, token, userId });
Expand Down
1 change: 1 addition & 0 deletions app/views/AttachmentView.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ jest.mock('../lib/hooks/useAppSelector', () => ({

jest.mock('../lib/methods/helpers', () => ({
formatAttachmentUrl: (url: string) => url,
encodeAttachmentUrl: (url: string) => url,
isAndroid: false,
showErrorAlert: jest.fn()
}));
Expand Down
Loading
Loading