refactor: `Root` screen to functional component #502

tuliomir · 2024-01-31T00:31:00Z

Acceptance Criteria

Refactors the Root component inside App.js to a functional component
Refactors the Version component, called uniquely by Root, to a functional component
Refactors the VersionError component, called uniquely by Root, to a functional component
Adapts the event handling codes to work with useEffect()

Security Checklist

Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

codecov · 2024-01-31T00:34:06Z

Codecov Report

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

Project coverage is 9.20%. Comparing base (e09124a) to head (a160da3).
Report is 2 commits behind head on dev.

❗ Current head a160da3 differs from pull request most recent head 9d1755f. Consider uploading reports for the commit 9d1755f to get more accurate results

Files	Patch %	Lines
src/App.js	0.00%	44 Missing and 13 partials ⚠️
src/screens/VersionError.js	0.00%	6 Missing ⚠️
src/components/Version.js	0.00%	4 Missing ⚠️
src/screens/Server.js	0.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             dev    #502      +/-   ##
========================================
- Coverage   9.21%   9.20%   -0.02%     
========================================
  Files        112     112              
  Lines       5250    5259       +9     
  Branches     695     698       +3     
========================================
  Hits         484     484              
- Misses      4106    4112       +6     
- Partials     660     663       +3

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

tuliomir · 2024-02-02T19:05:38Z

src/App.js

    }
-  }
+  }, [isVersionAllowed, wallet]);


The version validation used to be inside the returnDefaultComponent function, but was moved up to the Root level to reduce the amount of times it was called during the application runtime.

alexruzenhack · 2024-02-06T18:23:00Z

src/App.js

+  if (!versionIsKnown) {
+    if (!isLockedScreen) {
+      return null;
    }


I didn't understand it. Shouldn't it return at least an error page?

This indeed is still confusing. I believe the outer navigation and outer wrappers should be refactored in a future moment for increased legibility.

Added some more documentation on ae60eeb, but in short: this screen should not render anything while the version is still unknown.

The response from the fullnode informing the version should be received in a fraction of a second, so this will normally not be an issue. Connection failures with the fullnode during this request should be intercepted by the <ErrorWrapper /> component

r4mmer · 2024-02-08T13:42:09Z

src/App.js

+    if (!IPC_RENDERER) {
+      return;
    }


In this case I think we should keep the IPC_RENDERER logic inside the if statement.
This is because we may want to add more logic to the startup and if we add after this it may cause some unintended issues.
Another reason is that the return of an useEffect is special, and if we add some cleanup logic in the return in the end it may not work all the time since we have an early return here.

That's a good point. Refactored on 74bab60.

r4mmer · 2024-02-08T13:49:16Z

src/App.js

-          });
-        }
-      });
+    // If there is no ledger connected, finish execution here


IPC_RENDERER does not signal if a ledger is connected.
electron usually runs with 2 processes a main (with nodeJS on the host) and a renderer (with the frontend code on a browser) and there is a special IPC (inter-process communication) channel between those 2, the main side will have a IPC_MAIN and the renderer side a IPC_RENDERER.

There is a case where this channel does not exist, when we start the renderer code on a normal browser instead of the electron one, this is why no IPC_RENDERER means no ledger, but no IPC_RENDERER does not mean no ledger connected

Improved the description on 74bab60.

r4mmer · 2024-02-08T13:53:22Z

src/App.js

+  useEffect(() => {
+    // Fetch the version information before rendering any screens, if possible
+    if (isVersionAllowed === undefined && wallet) {
+      helpersUtils.loadStorageState();


I think this is better kept in the initial startup.
What do you think?

This is already as close to the initial startup as I could move it while being in context, because it needs an initialized wallet to check the API version.

If I moved this into the ErrorWrapper, ErrorBoundary it would seem to be out of place, I think.
Do you agree?

r4mmer · 2024-02-08T13:56:44Z

src/App.js

+
+      // We already handle all js errors in general and open an error modal to the user
+      // so there is no need to catch the promise error below
+      versionUtils.checkApiVersion(wallet);


Can we check if this should also run when we change server?
Because the method logic does 2 things, validate that the fullnode connected is compatible with our wallet version and set the network.

It makes sense. On 51dc965 I modified the Server.js logic to reset the isVersionAllowed property, triggering a new validation from this effect.

r4mmer · 2024-02-08T15:48:59Z

src/screens/Server.js

+
+    // Forces the validation of the allowed version
+    dispatch(isVersionAllowedUpdate({ allowed: undefined }));


I like this solution but can you double check that the server change flow is not affected?
For instance, we have:

Change server from mainnet to another in mainnet

Change server from mainnet to testnet successfully

Change server from mainnet to testnet and fail the testnet confirm modal

Change server from testnet to another in testnet

Change server from testnet to mainnet

Since we are changing a variable that may change the current screen I think this is important.

All those were working OK, but the dispatch was not being done on an optimal place.
Refactored it to inside the executeServerChange() method on a160da3.

r4mmer · 2024-02-08T15:53:27Z

src/App.js

-    throw new Error('[LoadedWalletComponent] isVersionAllowed is undefined');
-    // return <Redirect to={{
-    //   pathname: '/loading_addresses/',
-    //   state: {path: match.url},
-    //   waitVersionCheck: true
-    // }} />;
+    // The version information is not yet available, no op
+    return;


Why this change?

Because when the ServerChange sets the isVersionAllowed to undefined to force a revalidation of the version, the LoadedWalletComponent would throw an exception and the whole application would come to a halt.

Instead, it should just wait a few more fractions of a second for this property to be populated and the app can continue working normally.

r4mmer · 2024-02-08T15:57:50Z

src/App.js

+  useEffect(() => {
+    // Fetch the version information before rendering any screens, if possible
+    if (isVersionAllowed === undefined && wallet) {
+      helpersUtils.loadStorageState();
+
+      // We already handle all js errors in general and open an error modal to the user
+      // so there is no need to catch the promise error below
+      versionUtils.checkApiVersion(wallet);
    }
+  }, [isVersionAllowed, wallet]);


checkApiVersion is an async method with a state change, I think we should write this useEffect more like whats described here what do you think?

As we discussed, this is an important change and will be made on a dedicaded PR. I'll open an issue for that.

This is being tracked on issue

refactor: Prevent race conditions within useEffect() #526

tuliomir added the refactor label Jan 31, 2024

tuliomir self-assigned this Jan 31, 2024

tuliomir changed the title ~~refactor: Functional root component~~ refactor: Root screen to functional component Jan 31, 2024

tuliomir force-pushed the refactor/functional-root branch from ff07415 to 009556c Compare February 2, 2024 18:44

tuliomir requested review from alexruzenhack and r4mmer February 2, 2024 18:49

tuliomir marked this pull request as ready for review February 2, 2024 18:49

tuliomir requested a review from pedroferreira1 as a code owner February 2, 2024 18:49

tuliomir removed the request for review from pedroferreira1 February 2, 2024 18:49

tuliomir commented Feb 2, 2024

View reviewed changes

alexruzenhack reviewed Feb 6, 2024

View reviewed changes

alexruzenhack approved these changes Feb 6, 2024

View reviewed changes

r4mmer requested changes Feb 8, 2024

View reviewed changes

tuliomir requested a review from r4mmer February 8, 2024 15:11

r4mmer reviewed Feb 8, 2024

View reviewed changes

r4mmer approved these changes Feb 8, 2024

View reviewed changes

r4mmer self-requested a review February 8, 2024 15:58

tuliomir added 11 commits February 8, 2024 15:28

refactor: Version component to functional

8d18ee5

refactor: VersionError component to functional

34e218e

refactor: Root component to functional

141d7fe

chore: removes debugging logs

e49f487

chore: removes TODOs

a11c31e

style: lint

e10d672

docs: increases clarity on version checking

9df082b

fix: allAddresses route

0bc71a1

refactor: improves ledger startup logic

96286f0

fix: updates isVersionAllowed on server change

c5e6761

refactor: improves version checking

a160da3

tuliomir force-pushed the refactor/functional-root branch from 01fbbbb to a160da3 Compare February 8, 2024 18:37

r4mmer approved these changes Feb 27, 2024

View reviewed changes

fix: loading screen shown on new wallet

9d1755f

tuliomir merged commit 5df5b77 into dev Feb 29, 2024

tuliomir deleted the refactor/functional-root branch February 29, 2024 12:58

tuliomir mentioned this pull request Mar 22, 2024

chore: Upgrades react-router to v6 #555

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: `Root` screen to functional component #502

refactor: `Root` screen to functional component #502

tuliomir commented Jan 31, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

tuliomir Feb 2, 2024

alexruzenhack Feb 6, 2024

tuliomir Feb 6, 2024

r4mmer Feb 8, 2024

tuliomir Feb 8, 2024

r4mmer Feb 8, 2024

tuliomir Feb 8, 2024

r4mmer Feb 8, 2024

tuliomir Feb 8, 2024

r4mmer Feb 8, 2024

tuliomir Feb 8, 2024

r4mmer Feb 8, 2024

tuliomir Feb 8, 2024 •

edited

Loading

r4mmer Feb 8, 2024

tuliomir Feb 8, 2024

r4mmer Feb 8, 2024

tuliomir Feb 8, 2024

tuliomir Feb 8, 2024


		// Forces the validation of the allowed version
		dispatch(isVersionAllowedUpdate({ allowed: undefined }));

refactor: Root screen to functional component #502

refactor: Root screen to functional component #502

Conversation

tuliomir commented Jan 31, 2024 • edited Loading

Acceptance Criteria

Security Checklist

codecov bot commented Jan 31, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuliomir Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor: `Root` screen to functional component #502

refactor: `Root` screen to functional component #502

tuliomir commented Jan 31, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

tuliomir Feb 8, 2024 •

edited

Loading