Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(app): properly manage the ipcRenderer notify event emitter #14621

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Mar 11, 2024

Closes RQA-2459

Overview

Do not instantiate new notification ipcRenderer event listeners for each component that intends to subscribe to the notification server. Instead re-use the same ipcRenderer listener. This not only reduces the number of emitters created, but fixes a bug in which ipcRenderer listeners were not properly disposed.

Test Plan

  • Verified via Chrome console that there is no memory leak warning, which often appears after navigating to the devices page or the robot details page.
  • Verified via smoke test that notifications still work as expected.
  • Using this build of the app, update a robot on the devices tab via the robot update banner, click a lot, let the app sit on the device details page for a while, etc. etc., and verify the app doesn't whitescreen.

Changelog

  • Fixed a memory leak.

Review requests

  • Read the ticket if you'd like more context. I've asked QA to try and repro this as well. Regardless of whether or not this actually solves the whitescreen (although I think this does), this should make it into 7.2.1.

Risk assessment

medium-low. The code touches the app's ipcRenderer for notifications, but it's pretty straightforward: if notifications still work as expected, we're good.

Do not instantiate new notification ipcRenderer event listeners for each component that intends to
subscribe to the notification server. Instead re-use the same ipcRenderer listener. This not only
reduces the number of emitters created, but fixes a bug in which ipcRenderer listeners were not
properly disposed.
@mjhuff mjhuff requested a review from a team as a code owner March 11, 2024 16:23
@mjhuff
Copy link
Contributor Author

mjhuff commented Mar 11, 2024

We explicitly prevent testing of remote.ts, since it breaks all our Jest tests, meaning writing a unit test for appShellListener requires moving it out remote.ts and doing some import/export changes. This seems like a big workaround for Jest considering we no longer use it in edge, and we don't seem to have the same limitation of mocking the whole file path. My plan is to do the 7.2.1 mergeback into edge with the unit test, but I'm open to suggestions if we think it's worth changing the plan.

Copy link

codecov bot commented Mar 11, 2024

Codecov Report

Attention: Patch coverage is 19.04762% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 67.56%. Comparing base (be34672) to head (107173b).

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.2.1   #14621      +/-   ##
=======================================================
- Coverage                67.57%   67.56%   -0.02%     
=======================================================
  Files                     2521     2521              
  Lines                    72237    72251      +14     
  Branches                  9306     9311       +5     
=======================================================
- Hits                     48816    48815       -1     
- Misses                   21226    21240      +14     
- Partials                  2195     2196       +1     
Flag Coverage Δ
app 63.89% <19.04%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
app/src/resources/useNotifyService.ts 81.48% <80.00%> (-10.52%) ⬇️
app/src/redux/shell/remote.ts 0.00% <0.00%> (ø)

@mjhuff mjhuff requested a review from a team March 11, 2024 16:30
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, nice fix!

@mjhuff mjhuff merged commit 7f5a687 into chore_release-7.2.1 Mar 11, 2024
37 of 39 checks passed
@mjhuff mjhuff deleted the app_fix-ipcRenderer-memory-leak branch March 11, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants