feat(rename): Rename Nodes #841

Jem256 · 2024-03-01T11:49:57Z

Closes #388

Description

Adds a rename node feature that allows users to rename a node.

Screenshots

Jem256 · 2024-03-10T08:02:08Z

@jamaljsr here is the code

Edit: the last commit fixed the issue of the ports undefined error. Although the visual connector of the node to the backend has disappeared as shown below.

jamaljsr · 2024-03-11T14:09:04Z

The code currently on Github is missing the lightning.renameNode() function and is throwing an error, so I don't see the issue you're having.

Jem256 · 2024-03-11T14:19:42Z

The code currently on Github is missing the lightning.renameNode() function and is throwing an error, so I don't see the issue you're having.

I've fixed all the issues. I kindly request for a review of my PR

jamaljsr

Thanks so much for working on this. When testing it, I found a few functional issues.

polar-node-rename.mp4

When the network is started and I enter a new name then click Save, the network is stopped, but the node isn't renamed. I have to click Save a second time to rename the node.
After the rename, the node's open channels are no longer displayed.
The file paths to the cert & macaroons are still showing the old name. We should rename this folder as well.
I think it would be better to use a modal to have the user type the new name in so that we can access this from the right-click menu on each node.
Rename is not available for the Bitcoin Core and TAP nodes. Shouldn't this be supported for all node types?

Jem256 · 2024-03-11T15:08:40Z

Thanks so much for working on this. When testing it, I found a few functional issues.
polar-node-rename.mp4

1. When the network is started and I enter a new name then click Save, the network is stopped, but the node isn't renamed. I have to click Save a second time to rename the node.

2. After the rename, the node's open channels are no longer displayed.

3. The file paths to the cert & macaroons are still showing the old name. We should rename this folder as well.

4. I think it would be better to use a modal to have the user type the new name in so that we can access this from the right-click menu on each node.

5. Rename is not available for the Bitcoin Core and TAP nodes. Shouldn't this be supported for all node types?

Thank so much for the review. I am going to address all these. I have a question though, about Number 4, should we remove the button and simply have the modal accessible through the right click menu or should we have both?

codecov-commenter · 2024-03-11T15:11:39Z

Codecov Report

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

Project coverage is 97.72%. Comparing base (05d0bb8) to head (4b67452).
Report is 67 commits behind head on master.

Files	Patch %	Lines
src/utils/network.ts	2.94%	33 Missing ⚠️
src/components/common/RenameNodeModal.tsx	4.16%	23 Missing ⚠️
src/store/models/designer.ts	0.00%	14 Missing ⚠️
src/store/models/network.ts	50.00%	11 Missing ⚠️
src/store/models/modals.ts	0.00%	6 Missing ⚠️
src/lib/docker/dockerService.ts	0.00%	5 Missing ⚠️
src/utils/files.ts	20.00%	4 Missing ⚠️
src/components/common/RenameNodeButton.tsx	87.50%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##            master     #841      +/-   ##
===========================================
- Coverage   100.00%   97.72%   -2.28%     
===========================================
  Files          140      142       +2     
  Lines         4428     4700     +272     
  Branches       862      874      +12     
===========================================
+ Hits          4428     4593     +165     
- Misses           0      107     +107

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

jamaljsr · 2024-03-11T16:01:48Z

I have a question though, about Number 4, should we remove the button and simply have the modal accessible through the right click menu or should we have both?

To be consistent with the other modals, I think you should remove the sidebar dropdown menu and add a new "Rename Node" button between the "Restart" and "Advanced Options" buttons in the Actions tab.

Jem256 · 2024-03-25T11:13:12Z

Hey @jamaljsr do you have an idea why this is happening? when the app is restarted, the paths update to the new node name but the node also shows this error.

jamaljsr · 2024-03-25T16:07:26Z

Hey @Jem256, for each node, there is a folder on the host machine that contains the node's data. It's located at ~/.polar/networks/<network-id>/volumes/<implementation>/<node-name>/. It looks like Polar isn't renaming this dir when the node is renamed.

Jem256 · 2024-03-25T16:16:18Z

~/.polar/networks//volumes///

how can I access this dir through the code to update the name? I can't seem to figure that out

jamaljsr · 2024-03-25T16:40:51Z

The DockerService.ensureDirs is currently the method that initially creates this dir when it starts the containers. This seems like the most appropriate place to rename the dir when the node is renamed. You can call it from the stores using injections.dockerService.

jamaljsr · 2024-04-24T04:36:19Z

Hey @Jem256 please let me know when you feel this is ready for review and I'll take a look at it.

Jem256 · 2024-04-24T07:13:49Z

Hey @Jem256 please let me know when you feel this is ready for review and I'll take a look at it.

Hey Jamal, It should be ready sometime this week. Finishing up a few things today. I'll ping you when it's ready

Jem256 · 2024-04-24T11:55:55Z

@jamaljsr please take a look and let me know what you think. The channel links are not yet working but I've tried to make a lot of other things work and would like to know your thoughts on my approach. The other thing I would like to highlight is that in case someone tries to rename the bitcoin nodes, the path to the node can't be updated because it doesn't have that property. Also on the issue of links, syncChart doesn't work in updating them. But the functions updateBackendLink and updateTapBackendLink worked for those corresponding links. Do you think we have to write a separate function like updateChannelLink for the channels

jamaljsr

Thanks so much for working on this feature. It has been requested many times in the past. You are off to a great start. I've left a bunch of feedback on where I see room for improvement.

One strange issue I noticed when testing was that I couldn't select the text in the input field of the modal. Instead, it was dragging the node on the canvas underneath the modal. I'm not sure how that's even possible but my hunch is it's related to where you have the Modal component being mounted. I mentioned some details about this in a comment below.

polar-rename.mp4

src/components/common/RenameNode.tsx

src/shared/types.ts

src/store/models/network.ts

src/store/models/designer.ts

src/store/models/lightning.ts

src/store/models/network.ts

src/i18n/locales/de-DE.json

This reverts commit a19a2ad.

jamaljsr

This is looking amazing! Great work 👍

I just have a bit more feedback on the latest changes.

Also, I noticed this error when trying to rename a Core Lightning node. Does it work fine for you?

src/lib/docker/dockerService.ts

src/store/models/designer.ts

src/i18n/locales/en-US.json

src/store/models/network.ts

src/components/common/RenameNodeModal.tsx

src/lib/docker/dockerService.ts

jamaljsr

Thanks so much for the additional updates. This is looking really good. I found a few remaining issues, but this should be complete once these are fixed.

The last things remaining would be to rebase this branch on master and add/update the unit tests to fix the failures and get the coverage back up. Testing all of the code paths can be pretty tricky sometimes so please let me know if you need any assistance with this.

jamaljsr · 2024-05-14T06:36:13Z

src/lib/docker/dockerService.ts

+    const volumeDir = (node as AnyNode).implementation
+      .toLocaleLowerCase()
+      .replace('-', '');


Apologies for being too specific in my last comment. You can use nodePath() for the newPath variable as well. We don't want to have this custom string manipulation here when the volume dir name is already spcified in the dockerConfig. This is what nodePath uses.

jamaljsr · 2024-05-14T06:40:53Z

src/i18n/locales/en-US.json

@@ -277,6 +290,7 @@
  "cmps.designer.lightning.LightningDetails.actions": "Actions",
  "cmps.designer.lightning.LightningDetails.waitingNotice": "Waiting for {{implementation}} to come online",
  "cmps.designer.lightning.LightningDetails.connectError": "Unable to connect to {{implementation}} node",
+  "cmps.designer.lightning.LightningDetails.menuRename": "Rename",


Looks like I missed this in my prior review. This key isn't used and can be removed.

jamaljsr · 2024-05-14T07:51:27Z

src/utils/files.ts

+    await rename(abs(oldPath), abs(newPath));
+    info(`File renamed successfully from ${oldPath} to ${newPath}`);
+  } catch (err) {
+    debug('Error occurred while renaming file:', err);


I ran into this error when testing. I created a new network with 1 LND node. I started the network then immediately stopped it. I just wanted the data dirs to be created. Then I renamed alice to alice2. It seemed to work correctly because I saw the success notification. But when I started the network up, the node failed to come online. I went to check the data dir and saw that it was still named alice. Then I found this message in my logs:

[electron] 01:51:55.172 › Error occurred while renaming file: Error: ENOENT: no such file or directory, rename '/Users/jamal/.polar/networks/1/volumes/lnd/alice2' -> '/Users/jamal/.polar/networks/1/volumes/lnd/alice2'

It looks like the oldPath being passed to this function contains the new name. I'm not sure how this is exactly happening, but one additional suggestion I have is to remove the try/catch here and let the error bubble up to the UI, so that users will be aware if the directory rename fails. it may be better to try to rename the dir first before updating the network & store state, otherwise you'll end up with data thats out of sync.

jamaljsr · 2024-05-14T07:52:47Z

src/utils/network.ts

+    case 'lightning':
+      switch (node.implementation) {
+        case 'LND':
+          updatedNode = network?.nodes.lightning.find(n => n.id === node.id) as LndNode;


nitpick: we don't need the ?. throughout this function because the network parameter will never be undefined.

jamaljsr · 2024-05-14T07:57:42Z

src/store/models/network.ts

+        actions.setNetworks([...networks]);
+        await actions.save();
+        await injections.dockerService.saveComposeFile(network);


These 3 lines don't need to be duplicated in both conditions. Just call them outside of the if and only have actions.start() called inside of the if block.

jamaljsr · 2024-05-14T08:00:10Z

src/store/models/network.ts

+      await renameNode(network, node, newName);
+      await getStoreActions().designer.renameNode({
+        name: newName,
+        nodeId: oldNodeName,
+      });
+
+      await injections.dockerService.renameNodeDir(network, node, newName);


Ah, i see why the new name is being passed to dockerService.renameNodeDir(). It's because the await renameNode(network, node, newName); is updating the name on the node first. You should be able to resolve the error I mentioned above by just moving the dockerService rename above the await renameNode.

feat(rename): add rename button

a19a2ad

Jem256 changed the title ~~feat(rename): add rename button~~ feat(rename): Rename Nodes Mar 1, 2024

Jem256 added 3 commits March 4, 2024 18:28

feat(rename): add rename node method

783c449

feat(rename): rename node method to network store

39c2a9c

feat(rename): update designer store

e1db91b

feat(rename): fix ports issue

42f5c7c

feat(rename): stop node before rename

6fb22ea

Jem256 marked this pull request as ready for review March 11, 2024 14:21

jamaljsr reviewed Mar 11, 2024

View reviewed changes

Jem256 added 6 commits March 14, 2024 01:22

Merge branch 'master' into rename-nodes

4a2bcc8

feat(rename): fix save issue

3aca069

feat(rename): remove side button and add a button to actions tab

5944968

feat(rename): add to rightclick menu

acacd89

feat(rename): rename bitcoin node

f464c47

feat(rename): rename taproot node

ddb963d

Jem256 marked this pull request as draft April 2, 2024 09:00

Jem256 added 3 commits April 3, 2024 13:26

feat(rename): rename lnd host directory

5f0a95b

feat(rename): update file paths

f5620fe

Merge branch 'master' into rename-nodes

5c7da51

feat(rename): update bitcoin node links and fix tap paths

0a0005b

Jem256 requested a review from jamaljsr April 24, 2024 11:49

Jem256 marked this pull request as ready for review April 24, 2024 11:50

jamaljsr reviewed Apr 27, 2024

View reviewed changes

Jem256 added 4 commits April 29, 2024 20:38

Revert "feat(rename): add rename button"

3440b01

This reverts commit a19a2ad.

feat(rename node): rename node modal

1d7a68d

feat(rename node): add update dir function to docker service

a3b21d0

feat(rename node): update node name

c0e943c

Jem256 requested a review from jamaljsr May 2, 2024 17:17

jamaljsr reviewed May 3, 2024

View reviewed changes

feat(rename node): dockerService, status update, file util

904037c

Jem256 requested a review from jamaljsr May 11, 2024 20:51

feat(rename node): modal title to include name

4b67452

jamaljsr reviewed May 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rename): Rename Nodes #841

feat(rename): Rename Nodes #841

Jem256 commented Mar 1, 2024 •

edited

Jem256 commented Mar 10, 2024 •

edited

jamaljsr commented Mar 11, 2024

Jem256 commented Mar 11, 2024

jamaljsr left a comment

Jem256 commented Mar 11, 2024 •

edited

codecov-commenter commented Mar 11, 2024 •

edited by codecov bot

jamaljsr commented Mar 11, 2024

Jem256 commented Mar 25, 2024

jamaljsr commented Mar 25, 2024

Jem256 commented Mar 25, 2024

jamaljsr commented Mar 25, 2024

jamaljsr commented Apr 24, 2024

Jem256 commented Apr 24, 2024

Jem256 commented Apr 24, 2024 •

edited

jamaljsr left a comment •

edited

jamaljsr left a comment

jamaljsr left a comment •

edited

jamaljsr May 14, 2024

jamaljsr May 14, 2024

jamaljsr May 14, 2024

jamaljsr May 14, 2024

jamaljsr May 14, 2024

jamaljsr May 14, 2024

feat(rename): Rename Nodes #841

Are you sure you want to change the base?

feat(rename): Rename Nodes #841

Conversation

Jem256 commented Mar 1, 2024 • edited

Description

Screenshots

Jem256 commented Mar 10, 2024 • edited

jamaljsr commented Mar 11, 2024

Jem256 commented Mar 11, 2024

jamaljsr left a comment

Choose a reason for hiding this comment

Jem256 commented Mar 11, 2024 • edited

codecov-commenter commented Mar 11, 2024 • edited by codecov bot

Codecov Report

jamaljsr commented Mar 11, 2024

Jem256 commented Mar 25, 2024

jamaljsr commented Mar 25, 2024

Jem256 commented Mar 25, 2024

jamaljsr commented Mar 25, 2024

jamaljsr commented Apr 24, 2024

Jem256 commented Apr 24, 2024

Jem256 commented Apr 24, 2024 • edited

jamaljsr left a comment • edited

Choose a reason for hiding this comment

jamaljsr left a comment

Choose a reason for hiding this comment

jamaljsr left a comment • edited

Choose a reason for hiding this comment

jamaljsr May 14, 2024

Choose a reason for hiding this comment

jamaljsr May 14, 2024

Choose a reason for hiding this comment

jamaljsr May 14, 2024

Choose a reason for hiding this comment

jamaljsr May 14, 2024

Choose a reason for hiding this comment

jamaljsr May 14, 2024

Choose a reason for hiding this comment

jamaljsr May 14, 2024

Choose a reason for hiding this comment

Jem256 commented Mar 1, 2024 •

edited

Jem256 commented Mar 10, 2024 •

edited

Jem256 commented Mar 11, 2024 •

edited

codecov-commenter commented Mar 11, 2024 •

edited by codecov bot

Jem256 commented Apr 24, 2024 •

edited

jamaljsr left a comment •

edited

jamaljsr left a comment •

edited