Skip to content
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

fix: separate global hooks from app #1487

Merged
merged 3 commits into from
Feb 7, 2025

Conversation

mmrrnn
Copy link
Collaborator

@mmrrnn mmrrnn commented Feb 6, 2025

Description

Most of the hooks were place inside AppWrapper which caused constant rerender of the whole app when any of these hooks had side effects.

Case I also fixed that visualize the problem:
There was a hook

    const { setReleaseNotes, setIsAppUpdateAvailable } = useAppStateStore((state) => ({
        setReleaseNotes: state.setReleaseNotes,
        setIsAppUpdateAvailable: state.setIsAppUpdateAvailable,
    }));

Whenever any property of the AppStateStore changed, there was a check if the value had actually changed and the component needs to be rerendered. In this case, even though the setReleaseNotes and setIsAppUpdateAvailable references remained unchanged, we returned them together as an object, which reference actually is always different. As a result, the component was rerendered, even though no actual changes occurred.

Simple solution:

    const setReleaseNotes = useAppStateStore((state) => state.setReleaseNotes);
    const setIsAppUpdateAvailable = useAppStateStore((state) => state.setIsAppUpdateAvailable);
  • The issue was that this hook was positioned at the top of the tree, causing a RERENDER OF THE ENTIRE APP.

These seemingly minor details can significantly impact overall performance. To mitigate this, I decided to encapsulate all of these hooks within separate component that don't render anything, ensuring that this issue does not happen again.

@mmrrnn mmrrnn marked this pull request as ready for review February 6, 2025 15:58
@mmrrnn mmrrnn requested review from shanimal08 and brianp February 6, 2025 15:58
Copy link
Collaborator

@shanimal08 shanimal08 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice:D

@brianp
Copy link
Collaborator

brianp commented Feb 7, 2025

Fantastic 👏🏻

@brianp brianp merged commit ff59462 into tari-project:main Feb 7, 2025
8 checks passed
brianp pushed a commit that referenced this pull request Feb 11, 2025
Description
---

Motivation and Context
---
- memoised our app & setup components
- removed unnecessary `useEffects` where possible
- split out the setters for `setupProgress` data so we can check if the
data from the BE has in fact changed before setting the item (this
caused a ton of re-renders in the `SetupProgress` components (and
subsequently all it's children and parents before memo-ising)

How Has This Been Tested?
---

locally with `react-devtools` profiling, and mac activity monitor. bot
on esme and nextnet.



**profile from `main` as of when #1487 was merged**:
  

[main_1487_profiling-data.07-02-2025.13-52-24.json](https://github.com/user-attachments/files/18706584/main_1487_profiling-data.07-02-2025.13-52-24.json)

**profile from this branch, up to date with `main` (at the same point as
above with #1487):**


[startup_mem-uptodate-profiling-data.07-02-2025.14-00-56.json](https://github.com/user-attachments/files/18706590/startup_mem-uptodate-profiling-data.07-02-2025.14-00-56.json)


---


**all the `SetupProgress` renders on `main`**:



https://github.com/user-attachments/assets/74cd9008-138f-4b32-a00e-a803a98e1539



**all the `SetupProgress` renders on here after :D**:



https://github.com/user-attachments/assets/93618d0e-acbe-42e3-b63e-ee3b73a3d1cc


---



| before (on main) | after |
| :---: | :---: |
| <img width="1840" alt="frame 1 render"
src="https://github.com/user-attachments/assets/1ed60d8c-98e3-4bc7-9542-a0e21eb1a7e5"
/> | <img width="1840" alt="frame 1 renders"
src="https://github.com/user-attachments/assets/130cfc0f-3473-4fdc-b29d-519ed6879fd3"
/> |
| <img width="1840" alt="frame 1 render2"
src="https://github.com/user-attachments/assets/88ca3d2b-9db5-4d3f-9e16-1e3d7c79b78b"
/> | <img width="1840" alt="frame1 renders2"
src="https://github.com/user-attachments/assets/a72c36c4-fe01-42cf-8b24-3db0a489f62e"
/> |
| <img width="1728" alt="infonav renders "
src="https://github.com/user-attachments/assets/1cc8520f-9e38-4909-9a7b-c002f7297d69"
/> | <img width="1728" alt="infonav renders"
src="https://github.com/user-attachments/assets/16992644-60c2-406f-bc00-2ad1bc2ffe41"
/> |
| <img width="1728" alt="mainview renders"
src="https://github.com/user-attachments/assets/9ed214f5-78ff-4349-8790-d2f6c3452681"
/> | <img width="1728" alt="mainview renders"
src="https://github.com/user-attachments/assets/61fb299a-d122-45ed-aa3c-a561a72dbd72"
/> |



<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Introduced a configuration variable to reliably display the
application version.

- **Style**
- Updated canvas opacity and refined the progress indicator for a
smoother visual experience.

- **Refactor**
- Optimized numerous UI components and dialogs with memoization and
consolidated state logic for enhanced performance and clarity.

- **Chores**
  - Improved logging and timeout management to boost overall stability.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants