Skip to content

fix(audit): keep blank rows last when sorting results tables descending - #166

Open
evanclan wants to merge 1 commit into
every-app:mainfrom
evanclan:fix/audit-table-nulls-last-on-desc
Open

fix(audit): keep blank rows last when sorting results tables descending#166
evanclan wants to merge 1 commit into
every-app:mainfrom
evanclan:fix/audit-table-nulls-last-on-desc

Conversation

@evanclan

Copy link
Copy Markdown

Summary

Sorting any nullable column in the Site Audit Pages or Performance table descending moves the blank rows to the top instead of leaving them at the bottom. The two comparators those tables use return ±1 for a blank side, and TanStack negates a comparator's result on a descending column, so the intent is inverted on the second header click.

This reuses the helper the repo already has for exactly this problem — src/client/components/table/nullSafeSort.ts — instead of keeping a second, direction-blind copy. 11 sortable columns are affected.

Problem

getSortedRowModel applies the descending flip after calling the column's sortingFn (table-core@8.21.3):

if (sortInt === 0) {
  sortInt = columnInfo.sortingFn(rowA, rowB, sortEntry.id)
}

// If sorting is non-zero, take care of desc and inversion
if (sortInt !== 0) {
  if (isDesc) {
    sortInt *= -1
  }

So a comparator that hard-codes "blank goes after" gets "blank goes before" on desc:

// src/client/features/audit/results/AuditResultsTableFilterLogic.ts:167
export function nullableNumberSort(left, right, columnId) {
  const a = left.getValue(columnId);
  const b = right.getValue(columnId);
  if (a == null && b == null) return 0;
  if (a == null) return 1;   // becomes -1 when the column is desc
  if (b == null) return -1;
  return a - b;
}

Verified against a real table instance — with rows score: [5, null, 10] and the column sorted desc, the old comparator returns [null, 10, 5].

The user-visible sting is on the Performance tab. isLighthouseFailure is defined as "has no category scores" (AuditResultsTableFilterLogic.ts:60-71), so the null rows are the failed audits. Clicking Perf, A11y or SEO to see your worst pages first fills the top of the table with rows that have no scores at all, pushing the real data below the fold. Same for LCP / CLS / INP / TTFB, for Status and Title on the Pages tab (pages that aren't HTML documents have no title), and for the Performance URL column.

Note that sortUndefined: "last" is not an option here: it returns early before the flip, so it is direction-safe, but it only inspects undefined and these columns carry null straight from the DB.

Solution

nullSafeSort.ts already solves this for the Brand Lookup tables (added in faf7a29) by reading the column's sort direction off the cell and pre-compensating for the flip. This PR:

  • adds stringNullsLast, a sibling of the existing numericNullsLast, for the URL/Title columns — empty strings count as blank, preserving the old !a behaviour;
  • factors the shared "one side is blank" sign into a small blankLast helper used by both;
  • points the 3 Pages columns and 8 Performance columns at the shared helpers;
  • deletes nullableNumberSort / nullableStringSort, so there is one implementation of this rule rather than two.

There is a second working pattern in the repo — RankTrackingColumns.tsx maps accessorFn: (row) => row.cpc ?? undefined and lets sortUndefined: "last" do the work. That would also fix these tables, but it needs an accessorFn on all 11 columns, so reusing the existing comparator seemed like the smaller change. Happy to switch if you'd rather converge on the rank-tracking approach.

Out of scope

  • KeywordSuggestionStep.tsx:70-124 has the same class of issue via sentinels (position ?? 999, searchVolume ?? 0), so unranked suggestions lead on a descending position sort. Different feature and it needs a call on whether 0 should mean "unknown" for volume/traffic, so I left it alone.
  • No change to the audit filter logic, the tables' markup, or any other feature's sorting.

Test plan

  • New src/client/components/table/nullSafeSort.test.ts drives a real headless createTable + getSortedRowModel, so it exercises TanStack's own desc flip rather than asserting my reading of it. Covers nulls-last in both directions, 0 not treated as blank, blanks keeping their relative order, and "" counting as blank.
  • Confirmed the new tests fail against the removed comparators (blank sorts first on desc) and pass with the shared ones.
  • pnpm test — 92 files, 740 tests passing.
  • pnpm ci:check — prettier, knip, tsc --noEmit (both projects) and oxlint --type-aware all clean.

Related

  • src/client/components/table/nullSafeSort.ts — the existing fix for this bug class, used by the Brand Lookup citation tables.
  • src/client/features/rank-tracking/RankTrackingColumns.tsx — the null -> undefined + sortUndefined: "last" variant of the same fix.

Made with Cursor

The Pages and Performance tables sorted with their own nullableNumberSort /
nullableStringSort, which return ±1 when one side is blank. TanStack negates a
comparator's result on a descending column, so those blanks jumped to the top
of the table on the second header click. On the Lighthouse score columns the
blank rows are exactly the failed audits, so sorting Perf/A11y/SEO descending
filled the first screen with rows that have no scores at all.

src/client/components/table/nullSafeSort.ts already solves this for the Brand
Lookup tables by reading the sort direction off the cell and pre-compensating
for the flip. Reuse numericNullsLast here, add a stringNullsLast sibling for
the URL/Title columns, and drop the two direction-blind copies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant