fix: avoid redundant getNodeInfo() call on app start#3849
fix: avoid redundant getNodeInfo() call on app start#3849myxmaster wants to merge 1 commit intoZeusLN:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request optimizes the application's startup sequence by addressing an unnecessary API call. It refactors how node information is fetched, preventing a duplicate request during initialization and ensuring that critical data, like the current block height, is still updated efficiently and only when necessary, particularly for users leveraging LSP channels. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses a performance issue by removing a redundant getNodeInfo() call that occurred on application startup. The change involves removing a broad MobX reaction from NodeInfoStore that was triggered every time the channel list was updated. Instead, the getNodeInfo() call is moved into a more specific reaction within LSPStore, ensuring it only runs when there are changes to LSP-related channels. This is a well-reasoned optimization that correctly targets the intended functionality (keeping block height fresh for LSPS7) without the performance overhead of unnecessary API calls. The implementation is clean and aligns with the detailed explanation in the pull request description.
Description
NodeInfoStorehad a MobX reaction that calledgetNodeInfo()wheneverChannelsStore.channelswas written. During startup,fetchData()already callsgetNodeInfo()sequentially beforegetChannels()runs, so whengetChannels()resolves and writes tothis.channels, the reaction fired a second redundantgetNodeInfo().The reaction was introduced in #2720 (commit 9d8b330), probably to keep
nodeInfo.currentBlockHeightfresh for the LSPS7 lease expiry display inChannel.tsx.Fix: remove the broad reaction from
NodeInfoStoreand move the targetedgetNodeInfo()call intoLSPStore's existingchannelsreaction, which is already gated onsupportsLSPScustomMessage() && lspPubkey && channel belongs to LSP. Both reactions watch the same observable, so the LSP reaction fires in every scenario where the original one did, but only for users who actually have an LSP channel configured, and never unconditionally on startup.No LSPS7 behaviour is lost:
currentBlockHeightis read at navigation time, which is after node data has been fetched on startup - and for live channel changes, the LSPStore reaction now refreshes it.This pull request is categorized as a:
Checklist
yarn run tscand made sure my code compiles correctlyyarn run lintand made sure my code didn’t contain any problematic patternsyarn run prettierand made sure my code is formatted correctlyyarn run testand made sure all of the tests passTesting
If you modified or added a utility file, did you add new unit tests?
I have tested this PR on the following platforms (please specify OS version and phone model/VM):
I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
Locales
Third Party Dependencies and Packages
yarnafter this PR is merged inpackage.jsonandyarn.lockhave been properly updatedOther: