From 26b46dfacc417c5bb494dbf0bbf0c2df132e845b Mon Sep 17 00:00:00 2001
From: Nick Gambino <35090461+gambinish@users.noreply.github.com>
Date: Mon, 24 Feb 2025 10:02:38 -0800
Subject: [PATCH] fix (cherry-pick): Handle nullish value in alphanumeric sort
(#30534)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Cherry-picks
https://github.com/MetaMask/metamask-extension/commit/2c720029beaed60ae31a5f2772b96470c8fc0d68
to v12.13.0.
## **Description**
Fixes a bug reported where alphaNumeric sort was breaking on
`.localeCompare` method, where it was comparing a `null` value.
[](https://codespaces.new/MetaMask/metamask-extension/pull/30500?quickstart=1)
## **Related issues**
Fixes: https://github.com/MetaMask/metamask-extension/issues/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.
## **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.
## **Description**
[](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**
### **Before**
### **After**
## **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.
---
ui/components/app/assets/util/sort.test.ts | 23 +++++++++++++++++++---
ui/components/app/assets/util/sort.ts | 6 +++++-
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/ui/components/app/assets/util/sort.test.ts b/ui/components/app/assets/util/sort.test.ts
index f4a99e31b641..821998ccd937 100644
--- a/ui/components/app/assets/util/sort.test.ts
+++ b/ui/components/app/assets/util/sort.test.ts
@@ -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)', () => {
diff --git a/ui/components/app/assets/util/sort.ts b/ui/components/app/assets/util/sort.ts
index e6348cedf79e..0241a95577f6 100644
--- a/ui/components/app/assets/util/sort.ts
+++ b/ui/components/app/assets/util/sort.ts
@@ -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(),
};