fix: prevent garbage collection of notification objects and ensure proper initialization order#1622
Conversation
ShGKme
left a comment
ShGKme
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
Indeed, they are garbage collected...
Just some small comment, and we can have it in
| notification.on('close', () => { | ||
| notifications.delete(notification!) | ||
| }) |
There was a problem hiding this comment.
One thing I'm worried about is that the close event is not guaranteed to be fired, which may lead to a small memory leak... But after checking a number of Electron issues, I don't see any better solution =\
There was a problem hiding this comment.
The Set is used to prevent the Notification object from being garbage collected by Electron's main process while the OS is still displaying it. This is a known behavior where if no JavaScript reference is kept, the click event listeners are destroyed before they can be triggered. I am not sure if there is a better solution, either - but Download notifications are infrequent, so I guess this will not kill the app through memory leaks.
There was a problem hiding this comment.
The current solution prevents notification object from being garbage collected.
But it keeps it in the memory forever (there is no guarantee for the close event).
So in some cases notification object will live in the app memory until restart.
There was a problem hiding this comment.
Though, it seems there is no solution for this problem in Electron =\
There was a problem hiding this comment.
We will go with a better solution for update notifications from another contribution:
Could you revert the changes here, keeping only the /tag/?
Just to remove a conflict between PRs, since this code is to be removed.
There was a problem hiding this comment.
I've reverted the changes in src/app/githubReleaseNotification.service.js, keeping only the /tag/ fix in the URL as requested. This should resolve the overlap with PR #1592.
…oper initialization order - Added a persistent Set to store Notification objects in githubReleaseNotification.service.js and downloads.ts to prevent them from being garbage collected before user interaction. - Moved setupReleaseNotificationScheduler inside app.whenReady() in main.js to ensure the app is fully initialized before showing notifications. - Updated GitHub release URL format to use /tag/ prefix for better reliability. Resolves: nextcloud#1621 Signed-off-by: Enrique López Mañas <eenriquelopez@gmail.com>
Signed-off-by: Enrique López Mañas <eenriquelopez@gmail.com>
kikoso
force-pushed
the
fix/1621/notification-click-fix
branch
from
a800be4 to
ab315f6
Compare
|
@ShGKme , rebased and committed to fix the DCO. |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
2 participants
☑️ Resolves
🖼️ Screenshots
📝 Description
This PR fixes the issue where clicking on the "New version available" notification did nothing.
The root cause was identified as the Notification object being garbage collected by Electron's main process before the user could interact with it. Additionally, the notification scheduler was starting before the app was fully ready.
Changes:
Setto hold references to Notification objects in githubReleaseNotification.service.js and downloads.ts, preventing garbage collection until they are closed or clicked.app.whenReady()in main.js./tag/prefix.