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 (cherry-pick): Handle nullish value in alphanumeric sort #30534

Merged
merged 1 commit into from
Feb 24, 2025

Conversation

gambinish
Copy link
Contributor

@gambinish gambinish commented Feb 24, 2025

Cherry-picks 2c72002 to v12.13.0.

Description

Fixes a bug reported where alphaNumeric sort was breaking on .localeCompare method, where it was comparing a null value.

Screenshot 2025-02-21 at 8 36 07 AM

Open in GitHub Codespaces

Related issues

Fixes: #30496

Manual testing steps

To replicate the bug

  1. checkout main
  2. In token-list hard code one of the values in filteredAssets to have a symbol of null. This is the value being passed to the alphaNumeric sort handler, and tokens should be filtered by that value when "Sort by alphanumeric" sort filter is toggled on.
  3. App will break with the error posted above.

To check fix

  1. Checkout this branch
  2. In token-list hard code one of the values in filteredAssets to have a symbol of null. This is the value being passed to the alphaNumeric sort handler, and tokens should be filtered by that value when "Sort by alphanumeric" sort filter is toggled on.
  3. Token list should render, app should not break. The token will appear at the top of the list, as an empty string is considered > than an actual value.

Screenshots/Recordings

After

The duplicated 1Inch token is the value I hardcoded for testing purposes.

Screenshot 2025-02-21 at 8 47 34 AM

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

Open in GitHub Codespaces

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.

## **Description**

Fixes a bug reported where alphaNumeric sort was breaking on
`.localeCompare` method, where it was comparing a `null` value.

<img width="677" alt="Screenshot 2025-02-21 at 8 36 07 AM"
src="https://github.com/user-attachments/assets/fd66c259-a43e-47d6-9854-36d3b456e925"
/>

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30500?quickstart=1)

## **Related issues**

Fixes: #30496

## **Manual testing steps**

To replicate the bug

1. checkout main
2. In `token-list` hard code one of the values in `filteredAssets` to
have a `symbol` of `null`. This is the value being passed to the
alphaNumeric sort handler, and tokens should be filtered by that value
when "Sort by alphanumeric" sort filter is toggled on.
3. App will break with the error posted above.

To check fix

1. Checkout this branch
2. In `token-list` hard code one of the values in `filteredAssets` to
have a `symbol` of `null`. This is the value being passed to the
alphaNumeric sort handler, and tokens should be filtered by that value
when "Sort by alphanumeric" sort filter is toggled on.
3. Token list should render, app should not break. The token will appear
at the top of the list, as an empty string is considered > than an
actual value.

## **Screenshots/Recordings**

### **After**

The duplicated `1Inch` token is the value I hardcoded for testing
purposes.

<img width="359" alt="Screenshot 2025-02-21 at 8 47 34 AM"
src="https://github.com/user-attachments/assets/d6f7f6f6-a807-4a18-8847-295d13b90684"
/>


## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/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

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 requested a review from danjm February 24, 2025 17:28
@gambinish gambinish marked this pull request as ready for review February 24, 2025 17:28
@metamaskbot
Copy link
Collaborator

Builds ready [0992127]
Page Load Metrics (1752 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint39521521684324155
domContentLoaded15332114171712862
load15622147175213464
domInteractive166734136
backgroundConnect974352512
firstReactRender1593372613
getState46819189
initialActions01000
loadScripts11111620125611957
setupStore76017168
uiStartup17712598205420096

@danjm danjm merged commit 26b46df into Version-v12.13.0 Feb 24, 2025
72 checks passed
@danjm danjm deleted the cherry-pick-30500-null-sort-bug branch February 24, 2025 18:02
@github-actions github-actions bot locked and limited conversation to collaborators Feb 24, 2025
@metamaskbot metamaskbot added the release-12.13.0 Issue or pull request that will be included in release 12.13.0 label Feb 24, 2025
@metamaskbot
Copy link
Collaborator

No release label on PR. Adding release label release-12.13.0 on PR, as PR was cherry-picked in branch 12.13.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.13.0 Issue or pull request that will be included in release 12.13.0 team-assets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants