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: Provide selector that enables cross-chain polling, regardless of network filter state #28662

Merged
merged 5 commits into from
Nov 26, 2024

Conversation

gambinish
Copy link
Contributor

@gambinish gambinish commented Nov 22, 2024

Description

We cannot rely on the same selector for all cases, as not all UI is tightly coupled to the tokenNetworkFilter, else we will not be able to compute aggregated balances across chains, when filtered by current network. Since polling for balances is UI based, we can use a different selector on the network-filter, which should execute polling loops only when the dropdown is toggled open.

With the current behavior, the aggregated balance will only display when "All Networks" filter is selected, and when the "Current Network" is selected, it will aggregate balances only for that chain.

Open in GitHub Codespaces

Related issues

Fixes: Current chain aggregated balance showing up in cross chain aggregated balance when current network is filterd.

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.

…k to ensure cross-chain balances appear as expected
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@gambinish gambinish changed the title fix: Temporarily disable isTokenNetworkFilterEqualCurrentNetwork chec… fix: Temporarily disable isTokenNetworkFilterEqualCurrentNetwork Nov 22, 2024
@gambinish gambinish marked this pull request as ready for review November 22, 2024 20:20
@gambinish gambinish requested a review from a team as a code owner November 22, 2024 20:20
@metamaskbot
Copy link
Collaborator

Builds ready [77a79e2]
Page Load Metrics (2061 ± 85 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint27023941678710341
domContentLoaded17152366201416780
load17262406206117785
domInteractive257042157
backgroundConnect14124423014
firstReactRender742901345627
getState688302612
initialActions01000
loadScripts12301773147413565
setupStore75814147
uiStartup197328772341247118
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: -2 Bytes (-0.00%)

sahar-fehri
sahar-fehri previously approved these changes Nov 22, 2024
@darkwing darkwing added the DO-NOT-MERGE Pull requests that should not be merged label Nov 22, 2024
@gambinish gambinish added DO-NOT-MERGE Pull requests that should not be merged and removed DO-NOT-MERGE Pull requests that should not be merged labels Nov 25, 2024
@gambinish gambinish added portfolio-view Used for PRs and issues related to Q4 2024 portfolio view and removed DO-NOT-MERGE Pull requests that should not be merged labels Nov 25, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [57c1412]
Page Load Metrics (1755 ± 55 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15461921176011857
domContentLoaded15371903173011053
load15461926175511455
domInteractive228932168
backgroundConnect1079312311
firstReactRender71155108189
getState45614178
initialActions01000
loadScripts10971469127310350
setupStore611711
uiStartup17392128195212660
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 1 Bytes (0.00%)
  • common: 127 Bytes (0.00%)

@gambinish gambinish changed the title fix: Temporarily disable isTokenNetworkFilterEqualCurrentNetwork fix: Provide selector that enables cross-chain polling, regardless of network filter state Nov 25, 2024
ui/selectors/selectors.js Outdated Show resolved Hide resolved
@gambinish gambinish requested a review from salimtb November 26, 2024 17:16
@metamaskbot
Copy link
Collaborator

Builds ready [8d8c222]
Page Load Metrics (2012 ± 156 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint165430012020328157
domContentLoaded163928451971308148
load166029932012325156
domInteractive21207494421
backgroundConnect10149493517
firstReactRender814071609646
getState589192110
initialActions00000
loadScripts118721781465279134
setupStore690172211
uiStartup189335042331405195
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 1 Bytes (0.00%)
  • common: 124 Bytes (0.00%)

@gambinish gambinish added this pull request to the merge queue Nov 26, 2024
Merged via the queue into develop with commit c272b25 Nov 26, 2024
75 checks passed
@gambinish gambinish deleted the fix/current-network-zero-balance-aggregate-bug branch November 26, 2024 18:34
@github-actions github-actions bot locked and limited conversation to collaborators Nov 26, 2024
@metamaskbot metamaskbot added the release-12.9.0 Issue or pull request that will be included in release 12.9.0 label Nov 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
portfolio-view Used for PRs and issues related to Q4 2024 portfolio view release-12.9.0 Issue or pull request that will be included in release 12.9.0 team-assets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants