-
Notifications
You must be signed in to change notification settings - Fork 127
refactor: Streams to make homepage startup instantly #1232
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
Conversation
ckeshava
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ckniffen , thank you for your work. The initial page latency has reduced drastically. This is a significant refactor and your work has been very helpful.
Let me know what you think of my observations.
| onMouseMove={(e) => | ||
| missing && missing.length && showTooltip('missing', e, { missing }) | ||
| } | ||
| className={status.trustedCount < status.missing.length ? 'missed' : ''} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this piece of logic has not been changed in this PR. However, it is incorrect even in the current staging branch of the code.
| className={status.trustedCount < status.missing.length ? 'missed' : ''} | |
| className={ status.missing.length > 0 ? 'missed' : ''} | |
| onMouseMove={(e) => { | |
| const { missing } = status | |
| missing.length && showTooltip('missing', e, { missing }) | |
| }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue is fixed in this commit: 5ed5e58
| @@ -1,146 +1,121 @@ | |||
| import { useContext, useState } from 'react' | |||
| import axios from 'axios' | |||
| import './css/style.scss' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, I'm unable to load the Network > Validators page in this build of the Explorer. I get a blank screen.
Here is the associated error in the console:
Warning: Each child in a list should have a unique "key" prop.
Check the render method of `ValidatorsTable`. See https://reactjs.org/link/warning-keys for more information.
at tr
at ValidatorsTable (http://localhost:3000/containers/Network/ValidatorsTable.tsx:54:23)
at div
at div
at Validators (http://localhost:3000/containers/Network/Validators.tsx:45:25)
at VHSValidatorsProvider (http://localhost:3000/containers/shared/components/VHSValidators/VHSValidatorsProvider.tsx:25:41)
at StreamsProvider (http://localhost:3000/containers/shared/components/Streams/StreamsProvider.tsx:47:35)
at ValidatorsPage
at RenderedRoute (http://localhost:3000/@fs/Users/ckeshavabs/explorer/node_modules/.vite/deps/chunk-IUD5A7RD.js?v=ab94eb5d:4082:5)
at Outlet (http://localhost:3000/@fs/Users/ckeshavabs/explorer/node_modules/.vite/deps/chunk-IUD5A7RD.js?v=ab94eb5d:4559:26)
at div
at NetworkProvider (http://localhost:3000/containers/shared/NetworkContext.tsx:36:3)
at SocketProvider (http://localhost:3000/containers/shared/SocketContext.tsx:55:3)
at _c (http://localhost:3000/containers/App/App.tsx:24:11)
at RenderedRoute (http://localhost:3000/@fs/Users/ckeshavabs/explorer/node_modules/.vite/deps/chunk-IUD5A7RD.js?v=ab94eb5d:4082:5)
at Routes (http://localhost:3000/@fs/Users/ckeshavabs/explorer/node_modules/.vite/deps/chunk-IUD5A7RD.js?v=ab94eb5d:4623:5)
at div
at AppErrorBoundary (http://localhost:3000/containers/App/AppErrorBoundary.tsx:5:1)
at div
at QueryClientProvider2 (http://localhost:3000/@fs/Users/ckeshavabs/explorer/node_modules/.vite/deps/react-query.js?v=ab94eb5d:2646:21)
at _a (http://localhost:3000/@fs/Users/ckeshavabs/explorer/node_modules/.vite/deps/react-helmet-async.js?v=ab94eb5d:625:5)
at AppWrapper (http://localhost:3000/containers/App/index.tsx:64:26)
at Router (http://localhost:3000/@fs/Users/ckeshavabs/explorer/node_modules/.vite/deps/chunk-IUD5A7RD.js?v=ab94eb5d:4566:15)
at BrowserRouter (http://localhost:3000/@fs/Users/ckeshavabs/explorer/node_modules/.vite/deps/react-router-dom.js?v=ab94eb5d:557:5)
at I18nextProvider (http://localhost:3000/@fs/Users/ckeshavabs/explorer/node_modules/.vite/deps/react-i18next.js?v=ab94eb5d:782:3)
at Suspense
Uncaught Error: Missing ":identifier" param
at invariant (chunk-IUD5A7RD.js?v=ab94eb5d:202:11)
at chunk-IUD5A7RD.js?v=ab94eb5d:639:7
at Array.map (<anonymous>)
at generatePath (chunk-IUD5A7RD.js?v=ab94eb5d:629:38)
at buildPath (routing.tsx:35:10)
at ExplorerLink (routing.tsx:73:16)
I suspect this has something to do with the removal of the key parameter in some of the React components in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI tests are also failing for the Validators page: https://github.com/ripple/explorer/actions/runs/17074812387/job/48413223332?pr=1232#step:6:3161
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because the signing_key is not always populated. One of my last commits introduced this error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue has been fixed in the commit: b748b23
Co-authored-by: Chenna Keshava B S <[email protected]>
|
As far as tests are concerned, I'll need to write |
2. Refactor the LedgerPages tests to accomodate the refactored StreamsProvider 3. Update LedgerEntryValidator to retain previous behavior (display validator signing key in class-name) 4. Fix the crashing behavior inside StreamsProvider.processValidationQueue (set up default hashes[])
… functional update in this commit.
|
@ckniffen Thank you very much for your work. Let me know if you'd like to include any other ideas into this PR. |
| "react18-json-view": "^0.2.8", | ||
| "recharts": "^2.15.3", | ||
| "ripple-address-codec": "^5.0.0", | ||
| "ripple-binary-codec": "^2.4.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package changes should be in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I have reverted the changes pertaining to package.json and package-lock.json file in this commit: 9a3be32
I'm referring to this practice as prop-drilling in react: https://react.dev/learn/passing-data-deeply-with-context Yes, I believe the new context based approach contributes significantly to the low startup time of the page. The There are also other optimizations thrown in (like pre-fetching the latest validated ledger after initialization of the |
…xplorer into refactor/streams-take-3
…hange which is slated to be removed in a few days
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include this irrelevant change in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I think we should have a separate PR for this so that the team is explicitly aware that we will remove it, since this was a higher up ask
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR will need a few days for all the reviewers to read+approve the changes. Retaining the LedgerCountdownBanner code will need a non-trivial usage of the StreamsProvider component.
I would like to instead wait for the removal of the LedgerCountdownBanner and then merge this PR.
|
|
||
| expect(wrapper.find('.validation').length).toBe(0) | ||
| expect(wrapper.find('.txn').length).toBe(0) | ||
| // Recent refactor of the StreamsProvider eagerly fetches the latest validated ledger. The objective is to reduce the latency of displaying the first validated ledger. Consequently, loading the page fetches the latest validated ledger. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate these comments for the lengthier tests!
|
nit: since this PR contains a lot of different refactors/cleanups, could you list them in the PR description? Would make it easier to follow specifically what is changing intentionally and why |
| refetchQuorum() | ||
| } | ||
|
|
||
| // TODO: Set fields before getting full ledger info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this todo still relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, removed it in 2cc86ce
| txCount: ledgerSummary.transactions.length, | ||
| closeTime: convertRippleDate(ledgerSummary.ledger_time), | ||
| transactions: ledgerSummary.transactions, | ||
| totalFees: ledgerSummary.total_fees, // fix type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, is the typing fix still relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, updated in 2cc86ce
achowdhry-ripple
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Note: The changes in this PR can be viewed in the |
ckeshava
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing my "Request Changes" older PR review because it is blocking the merge of this PR
|
Thank you @ckniffen and the PR reviewers for your help with this work. |
High Level Overview of Change
Switch streams logic to a context/provider based architecture. This reduces a bunch of redundant logic and cleans up the state change tree. The moment the page loads the most recent validated ledger is loaded in to make the page look populated from the get go.
Summary of Changes in this PR
StreamsProvider.tsxandVHSValidatorsProvider.tsxfiles. The new provider/context hook is used in the rest of the React components by replacing the existing property-drilled implementations. These changes are implemented in the first 12 commits of this PR. TheStreamsProviderandVHSValidatorsProvidercomponents supply the explorer with Validation information from two different sources, i.e Validation-stream and the VHS APIs respectively.cc @achowdhry-ripple @kuan121
Type of Change
Codebase Modernization