Skip to content

chore: Batch bg state updates in Engine service (#15189) #15416

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Cal-L
Copy link
Contributor

@Cal-L Cal-L commented May 16, 2025

Description

Changes are from Stas from Margelo from the original PR here - #15189

Some information describing the changes #15189 (comment)

  • usage of unstable_batchedUpdates - state updates triggered outside of React event handlers - such as within a setTimeout callback (debounce uses setTimeout under the hood) - are not automatically batched, so each dispatch would normally trigger its own rerender, so usage of unstable_batchedUpdates ensures that multiple redux actions flush result in a single render pass
  • setTimeout vs debounce - it was already mentioned in perf: Debounce and group state updates being flushed to Redux #14927 (comment) that if events keep coming in faster than the delay (constant background updates), a debounced flush may never occur, and that results in no UI updates. The setTimeout approach ensured a flush at least once per interval. Additionally, the setTimeout approach avoids pulling in an external dependency.
    also this pr introduces fewer changes, and the Batcher class might be reused in other places to batch updates

Related issues

Fixes: #15028 #15560

Manual testing steps

  1. Go to this page... 2.

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Description

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
Copy link
Contributor

github-actions bot commented May 16, 2025

CLA Signature Action:

Thank you for your submission, we really appreciate it. We ask that you all read and sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just by adding a comment to this pull request with this exact sentence:

I have read the CLA Document and I hereby sign the CLA

By commenting with the above message you are agreeing to the terms of the CLA. Your account will be recorded as agreeing to our CLA so you don't need to sign it again for future contributions to this repository.

1 out of 2 committers have signed the CLA.
@Cal-L
@stas-kravchuk

@metamaskbot metamaskbot added the team-mobile-platform Mobile Platform team label May 16, 2025
@Cal-L Cal-L self-assigned this May 16, 2025
@Cal-L Cal-L added Run Smoke E2E Requires smoke E2E testing No QA Needed Apply this label when your PR does not need any QA effort. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels May 16, 2025
@github-project-automation github-project-automation bot moved this to Needs dev review in PR review queue May 16, 2025
@Cal-L Cal-L changed the title [Proposal] Batch bg state updates in Engine service (#15189) Batch bg state updates in Engine service (#15189) May 16, 2025
Copy link
Contributor

github-actions bot commented May 16, 2025

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 78b9a94
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/fa63ff3b-e78a-4a1b-bcdf-dd375430013e

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 26.08696% with 17 lines in your changes missing coverage. Please review.

Project coverage is 68.89%. Comparing base (c62b042) to head (98342cd).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
app/core/EngineService/EngineService.ts 26.08% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15416      +/-   ##
==========================================
- Coverage   68.92%   68.89%   -0.03%     
==========================================
  Files        2394     2394              
  Lines       51403    51432      +29     
  Branches     7720     7727       +7     
==========================================
+ Hits        35430    35435       +5     
- Misses      13702    13726      +24     
  Partials     2271     2271              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
20.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@Cal-L Cal-L added Run Smoke E2E Requires smoke E2E testing and removed Run Smoke E2E Requires smoke E2E testing labels May 21, 2025
Copy link
Contributor

github-actions bot commented May 21, 2025

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 98342cd
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/0b7079a6-6166-469d-9724-fb2bd63150b9

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@Cal-L
Copy link
Contributor Author

Cal-L commented May 21, 2025

@Cal-L Cal-L changed the title Batch bg state updates in Engine service (#15189) chore: Batch bg state updates in Engine service (#15189) May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) No QA Needed Apply this label when your PR does not need any QA effort. Run Smoke E2E Requires smoke E2E testing team-mobile-platform Mobile Platform team
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

Measure performance implications for Redux dispatches
4 participants