Shutdown Docker Containers when Closing Polar GUI #844

Abdulkbk · 2024-03-06T21:31:45Z

This commit introduces a new feature that ensures docker containers are gracefully stopped when the user exits the application.

Closes #521

Description

Intercept the user's action of clicking the "X" button to close Polar.
Send an IPC message between the main process and renderer to initiate the shutdown process for docker containers.
Wait for all network containers to properly shutdown.
Add a delay before sending an IPC message from the renderer to the main process to allow the application to exit gracefully.

Steps to Test

Open Polar and create one or more networks.
Start the networks (optionally, do some stuff).
Exit Polar.
Go to your terminal and run docker ps.
You'll find out that the Polar docker containers are shut

codecov-commenter · 2024-03-10T16:59:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (57e0491) to head (a12ddf7).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #844   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          139       140    +1     
  Lines         4402      4428   +26     
  Branches       858       862    +4     
=========================================
+ Hits          4402      4428   +26

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jamaljsr

Thanks for implementing this change. This is a great feature to add.

I have some feedback after testing this locally.

While waiting for the containers to stop, I think there should be a visual indication that the user should wait until the process finishes. Otherwise, they may think it is not working and repeatedly click the close button again. Since the DockerContainerShutdown is mounted at the app level, we could have a partially transparent overlay which displays a loading icon and a "Shutting down..." message.
On my Mac, the window doesn't close after the docker containers have completed shutting down. I think it's fine to remove this if and just make the app quit on all OS's.
The code coverage has dropped. Please add unit tests to get the coverage for the new functionality.

src/components/App.tsx

Abdulkbk · 2024-03-12T16:29:25Z

T

Thanks for implementing this change. This is a great feature to add.

I have some feedback after testing this locally.

While waiting for the containers to stop, I think there should be a visual indication that the user should wait until the process finishes. Otherwise, they may think it is not working and repeatedly click the close button again. Since the DockerContainerShutdown is mounted at the app level, we could have a partially transparent overlay which displays a loading icon and a "Shutting down..." message.

On my Mac, the window doesn't close after the docker containers have completed shutting down. I think it's fine to remove this if and just make the app quit on all OS's.

The code coverage has dropped. Please add unit tests to get the for the new functionality.

Thanks for the feedback. I would work on resolving all the points you raised

jamaljsr · 2024-03-15T04:47:49Z

Thanks for the updates! 👌

I noticed that if I have one network that is not started, there is still a few seconds delay before Polar will shutdown. Can you change it to close immediately if there aren't any running networks?

src/store/models/network.ts

Abdulkbk · 2024-03-15T14:30:26Z

Thanks for the updates! 👌

I noticed that if I have one network that is not started, there is still a few seconds delay before Polar will shutdown. Can you change it to close immediately if there aren't any running networks?

Thank you for the feedback and of course.

I will make the changes before the end of today.

Abdulkbk · 2024-03-19T16:01:51Z

Hi @jamaljsr, I've got 2 blockers. I'd appereciate your help here:

Regarding test coverage, I added tests but could not cover one specific line (ref: in the image below).

Secondly, it seems my branch is out-of-date with the base branch, do I update and sync the base branch with my changes in this PR?

I await your response. Thank you.

jamaljsr

Thanks for the updates.

Testing can be a royal pain in the but. It usually takes me a bit of trial and error to figure out what works to target specific code blocks. Testing timers is even more tricky. It does get a little easier with experiences. The most important thing to focus on is to ensure you're verifying the correct outcomes, otherwise bugs can slip through even though you have those code paths covered.

I genuinely wouldn't have known how to help you with the tests without figuring out what works myself. So here's a git patch (network-spec.patch) you can apply and commit to your branch.
Yes, please rebase this branch on the latest master just to be sure there won't be any issues merging this.

src/components/appExit/DockerContainerShutdown.tsx

src/store/models/network.ts

jamaljsr

It looks like you merged the master branch into this PR. It is better to use git rebase master so your commits are placed on top of the master branch, instead of duplicating the commits here. Just be sure your local master branch has the latest commits from the master branch on Github. Since this diff is pretty small, I'd be ok with squashing all of your commits into one. It doesn't make sense to have 21 commits for such a small amount of changes.

I found a couple more tweaks to make when looking at this again this morning.

src/components/appExit/DockerContainerShutdown.tsx

jamaljsr

Thanks for fixing the commits. This is almost good to go.

Just a couple small tweaks and this will be good to go.

src/components/common/DockerContainerShutdown.tsx

src/i18n/locales/en-US.json

jamaljsr · 2024-03-22T16:02:28Z

The code looks great! Can you just squash your commits? Then I'll merge this.

This commit introduces a fix that ensures docker containers are gracefully stopped when the user exits the application. fix: correct comment after mod in previous commit fix: ipcRenderer import fix(test): modify electron's mock to include ipcRenderer's .on and .removeAllListeners functions feat: add shutting down indicator, add test, and make app quit on all OS by removing "if" test: add test for stopAll function fix: call stopAll without passing any param. Immediately close app when no network is started refactor: add test for stopAll dunction, moved 'shutting down...' label to en-US.json refactor: move component to common folder. Move style to Styled mappings refactor: removed an inline styling. Change key to reflect component's folder

Abdulkbk · 2024-03-24T11:48:45Z

The code looks great! Can you just squash your commits? Then I'll merge this.

Done 👍

jamaljsr · 2024-03-25T06:06:32Z

Thanks so much for following through with this PR. I'm going to merge it now 🎉

Abdulkbk · 2024-03-26T04:48:33Z

Thanks so much for following through with this PR. I'm going to merge it now 🎉

Great!.

This is my first contribution to a major open source project in the bitcoin space. I learned so much.

Thanks @jamaljsr.

jamaljsr reviewed Mar 11, 2024

View reviewed changes

src/components/App.tsx Show resolved Hide resolved

jamaljsr reviewed Mar 15, 2024

View reviewed changes

src/store/models/network.ts Outdated Show resolved Hide resolved

jamaljsr reviewed Mar 20, 2024

View reviewed changes

src/components/appExit/DockerContainerShutdown.tsx Outdated Show resolved Hide resolved

src/store/models/network.ts Outdated Show resolved Hide resolved

jamaljsr reviewed Mar 20, 2024

View reviewed changes

src/components/appExit/DockerContainerShutdown.tsx Outdated Show resolved Hide resolved

src/components/appExit/DockerContainerShutdown.tsx Outdated Show resolved Hide resolved

Abdulkbk force-pushed the del-containers-onclose branch 3 times, most recently from b222549 to 5d5d462 Compare March 21, 2024 01:04

jamaljsr reviewed Mar 21, 2024

View reviewed changes

src/components/common/DockerContainerShutdown.tsx Outdated Show resolved Hide resolved

src/components/common/DockerContainerShutdown.tsx Outdated Show resolved Hide resolved

src/i18n/locales/en-US.json Outdated Show resolved Hide resolved

Abdulkbk force-pushed the del-containers-onclose branch 2 times, most recently from b29ab24 to 553e126 Compare March 22, 2024 14:16

Abdulkbk force-pushed the del-containers-onclose branch from 553e126 to a12ddf7 Compare March 22, 2024 16:41

jamaljsr merged commit 5cfacb2 into jamaljsr:master Mar 25, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shutdown Docker Containers when Closing Polar GUI #844

Shutdown Docker Containers when Closing Polar GUI #844

Abdulkbk commented Mar 6, 2024

codecov-commenter commented Mar 10, 2024 •

edited

Loading

jamaljsr left a comment •

edited

Loading

Abdulkbk commented Mar 12, 2024

jamaljsr commented Mar 15, 2024

Abdulkbk commented Mar 15, 2024

Abdulkbk commented Mar 19, 2024 •

edited

Loading

jamaljsr left a comment

jamaljsr left a comment

jamaljsr left a comment

jamaljsr commented Mar 22, 2024

Abdulkbk commented Mar 24, 2024

jamaljsr commented Mar 25, 2024

Abdulkbk commented Mar 26, 2024

Shutdown Docker Containers when Closing Polar GUI #844

Shutdown Docker Containers when Closing Polar GUI #844

Conversation

Abdulkbk commented Mar 6, 2024

Description

Steps to Test

codecov-commenter commented Mar 10, 2024 • edited Loading

Codecov Report

jamaljsr left a comment • edited Loading

Choose a reason for hiding this comment

Abdulkbk commented Mar 12, 2024

jamaljsr commented Mar 15, 2024

Abdulkbk commented Mar 15, 2024

Abdulkbk commented Mar 19, 2024 • edited Loading

jamaljsr left a comment

Choose a reason for hiding this comment

jamaljsr left a comment

Choose a reason for hiding this comment

jamaljsr left a comment

Choose a reason for hiding this comment

jamaljsr commented Mar 22, 2024

Abdulkbk commented Mar 24, 2024

jamaljsr commented Mar 25, 2024

Abdulkbk commented Mar 26, 2024

codecov-commenter commented Mar 10, 2024 •

edited

Loading

jamaljsr left a comment •

edited

Loading

Abdulkbk commented Mar 19, 2024 •

edited

Loading