Skip to content

fix(common): return 0 for equal values in useSort comparator - #897

Open
zunixport wants to merge 1 commit into
coinbase:masterfrom
zunixport:fix/use-sort-stable-tie-ordering
Open

zunixport wants to merge 1 commit into
coinbase:masterfrom
zunixport:fix/use-sort-stable-tie-ordering

Conversation

@zunixport

Copy link
Copy Markdown

Problem

The comparator passed to Array.prototype.sort in
packages/common/src/hooks/useSort.ts never returned 0:

return [...data].sort((a, b) => {
  if (sortDirection === 'descending') {
    return aVal > bVal ? -1 : 1;   // aVal === bVal -> 1
  }
  return bVal > aVal ? -1 : 1;     // aVal === bVal -> 1
});

Array.prototype.sort requires 0 when both values are equal. Returning 1
in both directions means the comparator claims a > b and b > a at the
same time — an inconsistent comparator, for which the resulting order is
implementation-defined.

This triggers whenever two or more rows share the same value for the sort key,
which is very common in real tables: the same appStatus, the same balance,
the same price or rating. In that case the engine (V8 uses TimSort) can
reorder the tied rows arbitrarily.

Impact

  • Stability is lost. The ES2019 spec guarantees Array.prototype.sort is
    stable, and stability is only observable for equal elements — exactly what
    this comparator gets wrong.
  • Visibly shifting UI. Rows with duplicate keys can move around between
    renders for no user-visible reason.
  • Engine-dependent results. V8 (Chrome/Node), JavaScriptCore
    (Safari/iOS) and Hermes (React Native) can each produce a different order
    for the same data. CDS ships across web, mobile and native, so the same
    table can render differently per platform.
  • Public API. useSort is exported as
    @coinbase/cds-common/hooks/useSort, is documented on the docs site, and is
    used in the Table stories and docs examples.

Fix

Resolve the sort values once and return 0 when they are equal:

-      if (sortDirection === 'descending') {
-        return (sortBy ? get(a, sortBy) : a) > (sortBy ? get(b, sortBy) : b) ? -1 : 1;
-      }
-
-      return (sortBy ? get(b, sortBy) : b) > (sortBy ? get(a, sortBy) : a) ? -1 : 1;
+      const aValue = sortBy ? get(a, sortBy) : a;
+      const bValue = sortBy ? get(b, sortBy) : b;
+
+      if (aValue === bValue) return 0;
+
+      if (sortDirection === 'descending') {
+        return aValue > bValue ? -1 : 1;
+      }
+
+      return aValue < bValue ? -1 : 1;

Both sort directions are otherwise unchanged — ascending previously read
b > a ? -1 : 1, which is identical to a < b ? -1 : 1.

Tests

useSort already had tests, so a regression test was added to
packages/common/src/hooks/__tests__/useSort.test.ts:

  • Preserves the original order of tied values — asserts that rows sharing the
    same floor value keep their input order, for both ascending and descending.

The pre-existing tests (ascending, descending, nested, deeply nested,
no-mutation) are unaffected: for unequal values the behavior is unchanged, and
the fix only affects pairs with equal values.

@cb-heimdall

Copy link
Copy Markdown
Collaborator

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/2
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 1
Global minimum 0
Max 1
1
1 if commit is unverified 1
Sum 2
CODEOWNERS 🟡 See below

🟡 CODEOWNERS

Code Owner Status Calculation
ui-systems-eng-team 🟡 0/1
Denominator calculation
Additional CODEOWNERS Requirement
Show calculation
Sum 0
0
From CODEOWNERS 1
Sum 1

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants