Implimentaion file catche #6731
Conversation
WalkthroughIntroduces a new media type "other", exports getExtension with expanded extension resolution, and changes file download to check a media cache before downloading, using getMediaCache and downloadMediaFile instead of direct FileSystem.downloadAsync. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant FD as fileDownload()
participant MC as getMediaCache
participant DM as downloadMediaFile
C->>FD: request download (url, mimeType, messageId, ...)
FD->>MC: lookup cached URI (type: "other", mimeType, url)
alt cache hit
MC-->>FD: cachedUri
FD-->>C: return cachedUri
else cache miss
MC-->>FD: not found
FD->>DM: downloadMediaFile({ messageId, type:"other", downloadUrl })
DM-->>FD: downloadedUri
FD-->>C: return downloadedUri
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/lib/methods/helpers/fileDownload.ts (1)
22-29: Consider extracting the media type constant.The
'other' as constis repeated twice. For better maintainability, consider extracting it to a constant at the top of the file.+const FILE_MEDIA_TYPE = 'other' as const; + export const getLocalFilePathFromFile = (localPath: string, attachment: IAttachment): string => `${localPath}${attachment.title}`; export const fileDownload = async (url: string, attachment?: IAttachment, fileName?: string): Promise<string> => { let path = `${FileSystem.documentDirectory}`; if (fileName) { path = `${path}${sanitizeFileName(fileName)}`; } if (attachment?.title) { path = `${path}${sanitizeFileName(attachment.title)}`; } - const cache = await getMediaCache({ type: 'other' as const, mimeType: attachment?.format, urlToCache: url }); + const cache = await getMediaCache({ type: FILE_MEDIA_TYPE, mimeType: attachment?.format, urlToCache: url }); if (cache?.exists) { return cache.uri; } const option = { messageId: url, - type: 'other' as const, + type: FILE_MEDIA_TYPE, downloadUrl: url };app/lib/methods/handleMediaDownload.ts (1)
77-90: Refactor repetitive extension checks.The extension checks for doc, docx, and pdf follow the same pattern and can be consolidated to reduce code duplication.
Apply this diff to refactor the extension checks:
- // support url with doc extension - if (url?.split('.').pop() === 'doc') { - return 'doc'; - } - - // support url with docx extension - if (url?.split('.').pop() === 'docx') { - return 'docx'; - } - - // support url with pdf extension - if (url?.split('.').pop() === 'pdf') { - return 'pdf'; - } + // support document extensions from URL + const urlExtension = url?.split('.').pop(); + if (urlExtension && ['doc', 'docx', 'pdf'].includes(urlExtension)) { + return urlExtension; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/lib/methods/handleMediaDownload.ts(3 hunks)app/lib/methods/helpers/fileDownload.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/lib/methods/helpers/fileDownload.ts (1)
app/lib/methods/handleMediaDownload.ts (2)
getMediaCache(162-187)downloadMediaFile(268-315)
🔇 Additional comments (3)
app/lib/methods/helpers/fileDownload.ts (1)
9-9: LGTM!The import additions are correct and all imported functions are properly utilized in the caching flow.
app/lib/methods/handleMediaDownload.ts (2)
71-71: LGTM!Exporting
getExtensionenables broader reuse across the codebase and supports the new caching functionality.
71-90: Verify extension resolution priority is correct.The current implementation prioritizes URL extensions (gif, doc, docx, pdf) over
mimeType. This means if a URL ends withmimeType(e.g.,application/msword), it will return'pdf'instead of using the mimeType-based resolution. Ensure this behavior aligns with your requirements, especially for cases where URL extensions might be misleading or incorrect.Consider whether mimeType should take precedence for more accurate file type detection, or if the current URL-first approach is intentional for your use case.
| import { headers } from './helpers/fetch'; | ||
|
|
||
| export type MediaTypes = 'audio' | 'image' | 'video'; | ||
| export type MediaTypes = 'audio' | 'image' | 'video' | 'other'; |
There was a problem hiding this comment.
Add default extension for 'other' media type.
The MediaTypes union now includes 'other', but the defaultType object (lines 20-24) doesn't have an entry for it. This means getExtension (line 92) will return undefined when type is 'other' and mimeType is undefined, which could break filename generation in getFilename.
Apply this diff to add a default extension for the 'other' type:
const defaultType = {
audio: 'mp3',
image: 'jpg',
- video: 'mp4'
+ video: 'mp4',
+ other: 'bin'
};Alternatively, if the 'other' type should always have a mimeType, add validation to ensure it's provided.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type MediaTypes = 'audio' | 'image' | 'video' | 'other'; | |
| const defaultType = { | |
| audio: 'mp3', | |
| image: 'jpg', | |
| video: 'mp4', | |
| other: 'bin' | |
| }; |
🤖 Prompt for AI Agents
In app/lib/methods/handleMediaDownload.ts around lines 18–24 and 92, the
MediaTypes union now includes 'other' but the defaultType mapping lacks an entry
for it, causing getExtension to return undefined for type 'other' when mimeType
is missing; add a default extension for 'other' in the defaultType object (for
example 'bin' or 'dat') so getExtension always returns a sane value, or
alternatively add validation at the start of getFilename/getExtension that
throws or requires a mimeType when type === 'other'.
| const cache = await getMediaCache({ type: 'other' as const, mimeType: attachment?.format, urlToCache: url }); | ||
|
|
||
| if (cache?.exists) { | ||
| return cache.uri; | ||
| } | ||
| const option = { | ||
| messageId: url, | ||
| type: 'other' as const, | ||
| downloadUrl: url | ||
| }; | ||
|
|
||
| const uri = await downloadMediaFile(option); | ||
| return uri; |
There was a problem hiding this comment.
Critical issues with messageId and encryption handling.
This implementation has two critical problems:
-
Line 28: Passing
urlasmessageIdis incorrect. ThedownloadMediaFilefunction callspersistMessage(messageId, ...)which attempts to look up the message by this ID in the database. Using the URL instead of an actual message ID will cause the persistence to fail silently, potentially corrupting the message state. -
Lines 27-31: The
optionobject is missingencryptionandoriginalChecksumparameters. This means encrypted attachments won't be decrypted properly. Looking atfileDownloadAndPreview(lines 44-49), encryption is handled separately there, butdownloadMediaFilealso handles encryption (lines 300-302 in handleMediaDownload.ts), creating an inconsistency.
The fileDownload function needs to accept messageId as a parameter and pass encryption details when available:
-export const fileDownload = async (url: string, attachment?: IAttachment, fileName?: string): Promise<string> => {
+export const fileDownload = async (url: string, attachment?: IAttachment, fileName?: string, messageId?: string): Promise<string> => {
let path = `${FileSystem.documentDirectory}`;
if (fileName) {
path = `${path}${sanitizeFileName(fileName)}`;
}
if (attachment?.title) {
path = `${path}${sanitizeFileName(attachment.title)}`;
}
const cache = await getMediaCache({ type: 'other' as const, mimeType: attachment?.format, urlToCache: url });
if (cache?.exists) {
return cache.uri;
}
const option = {
- messageId: url,
+ messageId: messageId || url,
type: 'other' as const,
- downloadUrl: url
+ downloadUrl: url,
+ ...(attachment?.encryption && attachment?.hashes?.sha256 && {
+ encryption: attachment.encryption,
+ originalChecksum: attachment.hashes.sha256
+ })
};
const uri = await downloadMediaFile(option);
return uri;
};Then update the call in fileDownloadAndPreview:
if (!file.startsWith('file://')) {
- file = await fileDownload(file, attachment);
+ file = await fileDownload(file, attachment, undefined, messageId);
- if (attachment.encryption) {
- if (!attachment.hashes?.sha256) {
- throw new Error('Missing checksum');
- }
- await Encryption.addFileToDecryptFileQueue(messageId, file, attachment.encryption, attachment.hashes?.sha256);
- }
+ // Encryption is now handled by downloadMediaFile
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cache = await getMediaCache({ type: 'other' as const, mimeType: attachment?.format, urlToCache: url }); | |
| if (cache?.exists) { | |
| return cache.uri; | |
| } | |
| const option = { | |
| messageId: url, | |
| type: 'other' as const, | |
| downloadUrl: url | |
| }; | |
| const uri = await downloadMediaFile(option); | |
| return uri; | |
| // app/lib/methods/helpers/fileDownload.ts | |
| export const fileDownload = async ( | |
| url: string, | |
| attachment?: IAttachment, | |
| fileName?: string, | |
| messageId?: string | |
| ): Promise<string> => { | |
| let path = `${FileSystem.documentDirectory}`; | |
| if (fileName) { | |
| path = `${path}${sanitizeFileName(fileName)}`; | |
| } | |
| if (attachment?.title) { | |
| path = `${path}${sanitizeFileName(attachment.title)}`; | |
| } | |
| const cache = await getMediaCache({ | |
| type: 'other' as const, | |
| mimeType: attachment?.format, | |
| urlToCache: url | |
| }); | |
| if (cache?.exists) { | |
| return cache.uri; | |
| } | |
| const option = { | |
| messageId: messageId || url, | |
| type: 'other' as const, | |
| downloadUrl: url, | |
| ...(attachment?.encryption && attachment?.hashes?.sha256 && { | |
| encryption: attachment.encryption, | |
| originalChecksum: attachment.hashes.sha256 | |
| }) | |
| }; | |
| const uri = await downloadMediaFile(option); | |
| return uri; | |
| }; | |
| // … later in fileDownloadAndPreview (around lines 44–49): | |
| if (!file.startsWith('file://')) { | |
| file = await fileDownload(file, attachment, undefined, messageId); | |
| // Encryption is now handled inside downloadMediaFile, | |
| // so we no longer need to enqueue decryption here. | |
| } |
🤖 Prompt for AI Agents
In app/lib/methods/helpers/fileDownload.ts around lines 22 to 34, the function
wrongly uses the URL as messageId and omits encryption and originalChecksum in
the download options; change the function signature to accept a real messageId
parameter (and encryption/originalChecksum when available), pass that messageId
(not the URL) into the option.messageId field, and include option.encryption and
option.originalChecksum populated from the attachment or caller arguments; also
update the caller (fileDownloadAndPreview) to forward the correct messageId and
any encryption/originalChecksum values when invoking this fileDownload helper.
1 participant
Proposed changes
Implemented download caching for chat attachments to improve user experience by reducing download times and saving data usage. Files are now cached locally after first download, and subsequent access uses the cached version instead of re-downloading the same file.
Issue(s)
Closes #6756
How to test or reproduce
Screenshots
without_cache.mp4
with_cache.mp4
Types of changes
Checklist
Further comments
The implementation leverages the existing
getMediaCachefunction to check if files are already cached before initiating downloads. This approach maintains consistency with the existing caching system while providing the performance benefits of file caching for attachments.