Skip to content

Commit

Permalink
fix (cherry-pick): Handle nullish value in alphanumeric sort (#30534)
Browse files Browse the repository at this point in the history
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.

<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.


<!--
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?
-->

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

## **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
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/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-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.
  • Loading branch information
gambinish authored Feb 24, 2025
1 parent 0ffc724 commit 26b46df
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 4 deletions.
23 changes: 20 additions & 3 deletions ui/components/app/assets/util/sort.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,31 @@ const mockAssets: MockAsset[] = [
// Define the sorting tests
describe('sortAssets function - nested value handling with dates and numeric sorting', () => {
test('sorts by name in ascending order', () => {
const sortedById = sortAssets(mockAssets, {
const sortedByName = sortAssets(mockAssets, {
key: 'name',
sortCallback: 'alphaNumeric',
order: 'asc',
});

expect(sortedByName[0].name).toBe('Asset A');
expect(sortedByName[sortedByName.length - 1].name).toBe('Asset Z');
});

test('should handle null values in alphanumeric sorting gracefully', () => {
const badAsset = {
name: null,
balance: '400',
createdAt: new Date('2021-07-20'),
profile: { id: '2', info: { category: 'bronze' } },
};
const sortedByName = sortAssets([...mockAssets, badAsset], {
key: 'name',
sortCallback: 'alphaNumeric',
order: 'asc',
});

expect(sortedById[0].name).toBe('Asset A');
expect(sortedById[sortedById.length - 1].name).toBe('Asset Z');
expect(sortedByName[0].name).toBe(null);
expect(sortedByName[sortedByName.length - 1].name).toBe('Asset Z');
});

test('sorts by balance in ascending order (stringNumeric)', () => {
Expand Down
6 changes: 5 additions & 1 deletion ui/components/app/assets/util/sort.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ const sortingCallbacks: SortingCallbacksT = {
const numB = b ? parseFloat(parseFloat(b).toFixed(5)) : 0;
return numA - numB;
},
alphaNumeric: (a: string, b: string) => a.localeCompare(b),
alphaNumeric: (a: string | null, b: string | null) => {
const valueA = a ?? '';
const valueB = b ?? '';
return valueA.localeCompare(valueB);
},
date: (a: Date, b: Date) => a.getTime() - b.getTime(),
};

Expand Down

0 comments on commit 26b46df

Please sign in to comment.