Fix multiple download issues on Mobile Safari and desktop versions. #37131
Conversation
…x/35189-multi-downloads-external-link
…x/35189-multi-downloads-external-link
…x/35189-multi-downloads-external-link
|
@ishpaul777 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| @@ -0,0 +1,243 @@ | |||
| import type {DownloadItem, Event, SaveDialogOptions, Session, WebContents} from 'electron'; | |||
There was a problem hiding this comment.
what does this file does? Can you write TLDR for code for some context ?
There was a problem hiding this comment.
@ishpaul777 the comment is bellow. Should we move it to the top?
App/src/libs/downloadQueue/electronDownloadManager.ts
Lines 5 to 11 in 7349cb6
There was a problem hiding this comment.
I meant how this works internally what does each function does, also add jsDocs for functions in the file
There was a problem hiding this comment.
@ishpaul777 I've revised the code to include additional comments from the original author and added new comments for new functions.
The changes made involve converting the code from JavaScript to TypeScript, replacing 'interface' with 'type', and eliminating dependencies on external libraries such as unused-filename, pupa, and ext-name.
I believe it is beneficial to keep the currently unused functionalities for potential future requirements, as it minimizes re-testing of the replicated package and facilitates referencing the original package in case of similar issues.
There was a problem hiding this comment.
I believe it is beneficial to keep the currently unused functionalities for potential future requirements
following the YAGNI principle we should only keep the functionalities that are necessary for the current requirement we can always add them back in future when required
There was a problem hiding this comment.
@ishpaul777 the PR has been updated with removal of unused functionalities and type.
There was a problem hiding this comment.
@ishpaul777 any other feedback?
There was a problem hiding this comment.
@ishpaul777 @techievivek friendly bump
…x/35189-multi-downloads-external-link
Sure, I hope we can achieve a faster turnaround time. |
@wildan-m I made some more comments. If done correctly, I think this would be the last one. Thank you for your patience with this. I fully understand and acknowledge the hard work you're putting into this. I'll try my best to provide feedback as fast as possible. |
…actor processor name
…x/35189-multi-downloads-external-link
desktop/createDownloadQueue.ts
Outdated
| }; | ||
|
|
||
| electronDownload(item.win, item.url, options); | ||
| item.win.webContents.session.on('will-download', listener); |
There was a problem hiding this comment.
Can we not save the listener in a variable? Would be a little more readable.
There was a problem hiding this comment.
Did you mean to create the listener in a separate function?
There was a problem hiding this comment.
No, use it directly like:
item.win.webContents.session.on('will-download', () => {
// code for the listener
});
There was a problem hiding this comment.
how can we perform cleanup without the listener variable?
There was a problem hiding this comment.
Ah, forgot about that. Never mind then.
There was a problem hiding this comment.
Please do not approve yet. I have noticed that the file has been renamed to download.html, and I am investigating it.
There was a problem hiding this comment.
Please do not approve yet. I have noticed that the file has been renamed to download.html, and I am investigating it.
@allroundexperts ok it's fixed, please help review the recent changes
| const downloadItemProcessor = (item: DownloadItem): Promise<void> => | ||
| new Promise((resolve, reject) => { | ||
| item.win.webContents.downloadURL(item.url); | ||
|
|
There was a problem hiding this comment.
Should we set a timeout here as well and reject the promise if the listener does not get called within that timeout period?
There was a problem hiding this comment.
@allroundexperts What is the expected behavior if the loading spinner timeout is reached? Should it stop downloading or continue downloading even after the spinner has stopped? Rejecting here will process the next item, resulting in some files not being downloaded due to the original issue in this PR.
There was a problem hiding this comment.
I'm not sure if I understand your question. In the processor, if the listener is not called after a certain amount of time, then that really means something is wrong. Isn't that the case?
There was a problem hiding this comment.
then that really means something is wrong
@allroundexperts Not always. It can also indicate that the download may take longer than usual, possibly due to network speed/latency or attachment size.
Currently, we only use setTimeout in fileDownload to ensure that the loading spinner stops, but it doesn't halt the actual download process.
If we set a timeout in the processor, then the next item will be processed. However, if the user takes no action, the file save dialog will remain open, preventing the next processed item from downloading, It's a similar situation when we do not implement this PR, but with timeout.
There was a problem hiding this comment.
I think we are not on the same page here. My understanding is that item.win.webContents.session.on('will-download', listener); would be called immediately on download start. You're suggesting that it would be called once the download completes?
There was a problem hiding this comment.
@wildan-m You should remove the timeout if listener gets called. That was the whole point of adding it.
There was a problem hiding this comment.
const downloadItemProcessor = (item: DownloadItem): Promise<void> =>
new Promise((resolve, reject) => {
item.win.webContents.downloadURL(item.url);
// Add a timeout to resolve the promise after 5 seconds
const timeout = setTimeout(() => {
item.win.webContents.session.removeListener('will-download', listener);
resolve();
}, 5000);
const listener = (event: Electron.Event, electronDownloadItem: Electron.DownloadItem) => {
clearTimeout(timeout)
.................
item.win.webContents.send(ELECTRON_EVENTS.DOWNLOAD_STARTED, {url: item.url});
};
item.win.webContents.session.on('will-download', listener);
});
There was a problem hiding this comment.
@allroundexperts clearing the timeout will not resolve the issue I mentioned earlier in the video.
There was a problem hiding this comment.
Have you tried it? Why doesn't it solve the issue? Does the listener not get called even after 5s?
There was a problem hiding this comment.
@allroundexperts I apologize for the incorrect assumption. I thought that will-download would not be called when the saveAs dialog opens. I have made the updates, please review them.
…x/35189-multi-downloads-external-link
| return new Promise((resolve) => { | ||
| // This sets a timeout that will resolve the promise after 5 seconds to prevent indefinite hanging | ||
| const downloadTimeout = setTimeout(() => { | ||
| resolve(); |
There was a problem hiding this comment.
We should reject here and let the user know that something went wrong.
There was a problem hiding this comment.
@allroundexperts how should it look? Native fileDownload use RN alert to display errors/success, but it's not implemented on the web/desktop. I believe this could be considered a feature request.
| resolve(); | ||
| }, CONST.DOWNLOADS_TIMEOUT); | ||
|
|
||
| window.electron.on(ELECTRON_EVENTS.DOWNLOAD_STARTED, (...args: unknown[]) => { |
There was a problem hiding this comment.
I think we've discussed this a lot, but I don't think that relying on DOWNLOAD_STARTED event would work here. If the queue is already processing a large file, we'll always hit the downloadTimeout. A better way would be to listen for a new event called DOWNLOAD_QUEUED and emit it whenever the download item is queued.
There was a problem hiding this comment.
@allroundexperts Another point to consider why it's better to start the loader on onPress instead of DOWNLOAD_QUEUED is that the spinner is used to prevent rapid clicks on the same download link. If we start the loader when it's queued, although minimal delay, it might allow the user to download the same file multiple times.
Have a look at this video:
Kapture.2024-05-13.at.08.48.47.mp4
Please re-consider.
applied changes
diff --git a/desktop/ELECTRON_EVENTS.ts b/desktop/ELECTRON_EVENTS.ts
index cb0b05f097..fd0952b083 100644
--- a/desktop/ELECTRON_EVENTS.ts
+++ b/desktop/ELECTRON_EVENTS.ts
@@ -11,6 +11,7 @@ const ELECTRON_EVENTS = {
UPDATE_DOWNLOADED: 'update-downloaded',
DOWNLOAD: 'download',
DOWNLOAD_STARTED: 'download-started',
+ DOWNLOAD_QUEUED: 'download-queued',
SILENT_UPDATE: 'silent-update',
} as const;
diff --git a/desktop/contextBridge.ts b/desktop/contextBridge.ts
index e9c61e6953..5aadbc4651 100644
--- a/desktop/contextBridge.ts
+++ b/desktop/contextBridge.ts
@@ -26,6 +26,7 @@ const WHITELIST_CHANNELS_MAIN_TO_RENDERER = [
ELECTRON_EVENTS.FOCUS,
ELECTRON_EVENTS.BLUR,
ELECTRON_EVENTS.DOWNLOAD_STARTED,
+ ELECTRON_EVENTS.DOWNLOAD_QUEUED,
] as const;
const getErrorMessage = (channel: string): string => `Electron context bridge cannot be used with channel '${channel}'`;
diff --git a/desktop/createDownloadQueue.ts b/desktop/createDownloadQueue.ts
index 29ce595fc5..c32fa5ad9f 100644
--- a/desktop/createDownloadQueue.ts
+++ b/desktop/createDownloadQueue.ts
@@ -108,6 +108,8 @@ const createDownloadQueue = () => {
const enqueueDownloadItem = (item: DownloadItem): void => {
queue.enqueue(item);
+ item.win.webContents.send(ELECTRON_EVENTS.DOWNLOAD_QUEUED, {url: item.url});
+
};
return {enqueueDownloadItem, dequeueDownloadItem: queue.dequeue};
};
diff --git a/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx b/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx
index d6bdd241dc..ad51aa8c5d 100644
--- a/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx
+++ b/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx
@@ -48,8 +48,7 @@ function BaseAnchorForAttachmentsOnly({style, source = '', displayName = '', dow
if (isDownloading || isOffline) {
return;
}
- Download.setDownload(sourceID, true);
- fileDownload(sourceURLWithAuth, displayName, '', Browser.isMobileSafari()).then(() => Download.setDownload(sourceID, false));
+ fileDownload(sourceURLWithAuth, displayName, '', Browser.isMobileSafari(), () => { Download.setDownload(sourceID, true) }).then(() => Download.setDownload(sourceID, false));
}}
onPressIn={onPressIn}
onPressOut={onPressOut}
diff --git a/src/libs/fileDownload/index.desktop.ts b/src/libs/fileDownload/index.desktop.ts
index 90faa1285b..8e1c213883 100644
--- a/src/libs/fileDownload/index.desktop.ts
+++ b/src/libs/fileDownload/index.desktop.ts
@@ -6,7 +6,7 @@ import type {FileDownload} from './types';
/**
* The function downloads an attachment on desktop platforms.
*/
-const fileDownload: FileDownload = (url, fileName) => {
+const fileDownload: FileDownload = (url, fileName, successMessage = '', shouldOpenExternalLink = false, startDownload = () => {}) => {
const options: Options = {
filename: fileName,
saveAs: true,
@@ -18,6 +18,17 @@ const fileDownload: FileDownload = (url, fileName) => {
resolve();
}, CONST.DOWNLOADS_TIMEOUT);
+ window.electron.on(ELECTRON_EVENTS.DOWNLOAD_QUEUED, (...args: unknown[]) => {
+ const arg = Array.isArray(args) ? args[0] : null;
+ const eventUrl = arg && typeof arg === 'object' && 'url' in arg ? arg.url : null;
+
+ // This event is triggered for all active download instances. We intentionally keep other promises waiting.
+ // Early resolution or rejection of other promises could prematurely stop the loading spinner or prevent the promise from being resolved.
+ if (eventUrl === url) {
+ startDownload();
+ }
+ });
+
window.electron.on(ELECTRON_EVENTS.DOWNLOAD_STARTED, (...args: unknown[]) => {
const arg = Array.isArray(args) ? args[0] : null;
const eventUrl = arg && typeof arg === 'object' && 'url' in arg ? arg.url : null;
diff --git a/src/libs/fileDownload/types.ts b/src/libs/fileDownload/types.ts
index fcc210c1c4..99763dfc42 100644
--- a/src/libs/fileDownload/types.ts
+++ b/src/libs/fileDownload/types.ts
@@ -1,6 +1,6 @@
import type {Asset} from 'react-native-image-picker';
-type FileDownload = (url: string, fileName?: string, successMessage?: string, shouldOpenExternalLink?: boolean) => Promise<void>;
+type FileDownload = (url: string, fileName?: string, successMessage?: string, shouldOpenExternalLink?: boolean, startDownload?: () => void) => Promise<void>;
type ImageResolution = {width: number; height: number};
type GetImageResolution = (url: File | Asset) => Promise<ImageResolution>;
There was a problem hiding this comment.
Can we just disable it once clicked, then show a loader once the DOWNLOAD_QUEUED event is received?
There was a problem hiding this comment.
@allroundexperts may I ask why we should initiate the loading spinner within the DOWNLOAD_QUEUE instead of placing it in the onPress function? What major advantage does this approach offer?
There was a problem hiding this comment.
I don't think there's any great advantage. It's just that it seems like a more event driven approach. Let's skip that if you feel too strongly about it.
There was a problem hiding this comment.
@allroundexperts I would like to keep the loading spinner and disable logic one together to minimize overhead. Therefore, the pending changes are to change DOWNLOAD_STARTED to DOWNLOAD_COMPLETED, DOWNLOAD_FAILED, and DOWNLOAD_CANCELED. Is that correct?
There was a problem hiding this comment.
New changes have been applied
…x/35189-multi-downloads-external-link
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid build not working Android: mWeb ChromeScreen.Recording.2024-05-21.at.8.56.25.PM.moviOS: NativeScreen.Recording.2024-05-21.at.8.48.49.PM.moviOS: mWeb SafariScreen.Recording.2024-05-21.at.8.42.55.PM.movMacOS: Chrome / SafariScreen.Recording.2024-05-21.at.8.36.52.PM.movMacOS: DesktopScreen.Recording.2024-05-21.at.8.39.26.PM.mov |
There was a problem hiding this comment.
Works great. Thanks for your patience and hard work @wildan-m!
|
@techievivek when will the merge freeze end? |
|
Thank you for your patience and hard work. We are good to merge now. |
|
🚀 Deployed to staging by https://github.com/techievivek in version: 1.4.77-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.77-11 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.77-11 🚀
|
@ishpaul777 @techievivek
Details
Fix multiple download issues on Mobile Safari and desktop versions.
Fixed Issues
$ #35189
PROPOSAL: #35189 (comment)
Tests
Pre-requisite: The user must have sent at least 2 files in the chat. The file types should not be supported for preview (e.g. docx, csv, .zip)
Mobile Safari
Other Platforms
Offline tests
Users should not be able to download files when offline; the download button should be disabled.
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
GMT20240223-140921_Clip_Wildan.M.s.Clip.02_23_2024.mp4
Android: mWeb Chrome
GMT20240223-140632_Clip_Wildan.M.s.Clip.02_23_2024.mp4
iOS: Native
GMT20240223-134318_Clip_Wildan.M.s.Clip.02_23_2024.mp4
iOS: mWeb Safari
GMT20240223-134522_Clip_Wildan.M.s.Clip.02_23_2024.mp4
MacOS: Chrome / Safari
GMT20240223-100835_Clip_Wildan.M.s.Clip.02_23_2024-2.mp4
MacOS: Desktop
GMT20240223-100432_Clip_Wildan.M.s.Clip.02_23_2024.mp4