Skip to content

Commit 2d5d594

Browse files
os-zhuangclaude
andauthored
fix(list,detail): sorting a lookup column stops ordering by an invisible key (#3096) (#3102)
A relational column (lookup / master_detail / user / tree) never holds the string its cell shows: it holds the $expand-ed record, or a raw foreign-key id whose label was resolved separately. Every sort path took that raw value as its key, so a column of names came back in an order with no relation to the names — sorting looked broken, and nothing said the key was something else. The two halves can order by different things, so they are fixed differently. Client-side sorts (grid column headers, any data-table, a non-windowed related list) now key off the label the cell renders, via getSortValue / compareSortValues in @object-ui/core — which resolves an expanded record through getRecordDisplayName (ADR-0079), so the sort key and the lookup cell agree on which field names a record. That replaces two broken comparators: `a[col] < b[col]` is always false between two objects (the comparator collapsed to a constant and permuted the rows), and `String(a[col])` is "[object Object]" (every row compared equal, so the sort silently did nothing). Server $orderby sorts cannot be fixed here — the key is the stored id by construction, and objectstack#4256 settled that no relation join is coming. So those entry points stop offering the illusion: the ListView toolbar sort picker withholds relational fields and explains why (pointing at a formula field as the supported way to sort by a related name), and a windowed related list renders no sort button for them. A relational field the view's CURRENT sort already uses stays listed, labelled "(by ID)", so view metadata authored or saved with such a sort round-trips instead of rendering a blank row and losing the sort on the next edit. Each regression test was run against the unfixed code first: the data-table one showed the constant comparator leaving rows untouched, the RelatedList one showed the old raw-id order, the ListView one showed all four relational fields in the picker. Verified live in the console against a CRM backend: crm_lead's two lookups are gone from the sort picker with the hint rendered beneath it, and the contacts grid's account column now sorts Acme/Globex/Initech ↔ reverse by the shown company name. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent c7c5294 commit 2d5d594

22 files changed

Lines changed: 815 additions & 21 deletions

File tree

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
---
2+
"@object-ui/core": patch
3+
"@object-ui/components": patch
4+
"@object-ui/plugin-list": patch
5+
"@object-ui/plugin-detail": patch
6+
"@object-ui/i18n": patch
7+
---
8+
9+
fix(list,detail): sorting a lookup column no longer orders by an invisible key — #3096
10+
11+
A relational column (`lookup` / `master_detail` / `user` / `tree`) never holds
12+
the string its cell shows: it holds the `$expand`-ed record, or a raw foreign-key
13+
id whose label was resolved separately. Every sort path took that raw value as
14+
its key, so the column of names came back in an order with no relation to the
15+
names — sorting looked broken, with nothing saying the key was something else.
16+
17+
The two halves are fixed differently, because they can order by different things:
18+
19+
- **Client-side sorts** (grid column headers, any `data-table`, a non-windowed
20+
related list) now key off the label the cell renders, via the new
21+
`getSortValue` / `compareSortValues` in `@object-ui/core` — which resolves an
22+
expanded record through `getRecordDisplayName` (ADR-0079), so the sort key and
23+
the lookup cell agree on which field names a record. This replaces two broken
24+
comparators: `a[col] < b[col]` is always false between two objects (the
25+
comparator collapsed to a constant and permuted the rows), and
26+
`String(a[col])` is `"[object Object]"` (every row compared equal, so the sort
27+
silently did nothing).
28+
- **Server `$orderby` sorts** cannot be fixed here — the key is the stored id by
29+
construction, and `objectstack#4256` settled that no relation join is coming.
30+
So those entry points stop offering the illusion: the ListView toolbar sort
31+
picker withholds relational fields and explains why (pointing at a formula
32+
field as the supported way to sort by a related name), and a windowed related
33+
list renders no sort button for them.
34+
35+
A relational field the view's CURRENT sort already uses stays listed, labelled
36+
`(by ID)`, so view metadata authored or saved with such a sort round-trips
37+
instead of rendering a blank row and losing the sort on the next edit.
Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* Regression (objectui#3096): clicking a lookup column header sorted by the
11+
* `$expand`-ed RECORD OBJECT, not by the name in the cell.
12+
*
13+
* `aValue < bValue` is always false for two objects, so the comparator returned
14+
* the constant `1` for every pair — the rows moved (the arrow turned, the order
15+
* changed) but the resulting order had no relation to the names on screen. The
16+
* header sort now keys off the same display name the cell renders.
17+
*
18+
* The fixture is chosen so the three candidate orders are all distinct: the
19+
* incoming order and the foreign-key-id order are Mallory/Zoe/Alice, while the
20+
* displayed-name order is Alice/Mallory/Zoe. Only a sort that reads the label
21+
* can produce the assertions below.
22+
*/
23+
import { describe, it, expect, beforeAll } from 'vitest';
24+
import { render, screen, fireEvent, within } from '@testing-library/react';
25+
import '@testing-library/jest-dom';
26+
import React from 'react';
27+
import { ComponentRegistry } from '@object-ui/core';
28+
import '../data-table';
29+
30+
const rows = [
31+
{ id: '1', code: 'C-1', owner: { id: 'rec_1a', name: 'Mallory' } },
32+
{ id: '2', code: 'C-2', owner: { id: 'rec_4m', name: 'Zoe' } },
33+
{ id: '3', code: 'C-3', owner: { id: 'rec_9z', name: 'Alice' } },
34+
];
35+
36+
const schema = {
37+
data: rows,
38+
pagination: false,
39+
searchable: false,
40+
columns: [
41+
{ header: 'Code', accessorKey: 'code' },
42+
{
43+
header: 'Owner',
44+
accessorKey: 'owner',
45+
// Mirrors the lookup cell renderer: the cell shows the related record's
46+
// name while the row value stays the expanded object.
47+
cell: (value: any) => <span>{value?.name ?? '—'}</span>,
48+
},
49+
],
50+
};
51+
52+
function renderTable() {
53+
const DataTable = ComponentRegistry.get('data-table') as any;
54+
if (!DataTable) throw new Error('data-table not registered');
55+
return render(<DataTable schema={schema} />);
56+
}
57+
58+
/** Owner names in DOM row order. */
59+
function ownerColumn(): string[] {
60+
return screen
61+
.getAllByRole('row')
62+
.slice(1) // drop the header row
63+
.map((row) => within(row).getAllByRole('cell')[1]?.textContent?.trim() ?? '');
64+
}
65+
66+
describe('data-table sorting a relational column', () => {
67+
beforeAll(() => {
68+
expect(ComponentRegistry.has('data-table')).toBe(true);
69+
});
70+
71+
it('orders by the displayed name, ascending then descending', () => {
72+
renderTable();
73+
// Unsorted — and this is also the order the raw foreign-key ids produce,
74+
// so neither "no sort" nor "sorted by id" can satisfy the next assertion.
75+
expect(ownerColumn()).toEqual(['Mallory', 'Zoe', 'Alice']);
76+
expect([...rows].sort((a, b) => a.owner.id.localeCompare(b.owner.id)).map((r) => r.owner.name))
77+
.toEqual(['Mallory', 'Zoe', 'Alice']);
78+
79+
fireEvent.click(screen.getByText('Owner'));
80+
expect(ownerColumn()).toEqual(['Alice', 'Mallory', 'Zoe']);
81+
82+
fireEvent.click(screen.getByText('Owner'));
83+
expect(ownerColumn()).toEqual(['Zoe', 'Mallory', 'Alice']);
84+
});
85+
86+
it('leaves a third click unsorted, back to the incoming row order', () => {
87+
renderTable();
88+
fireEvent.click(screen.getByText('Owner'));
89+
fireEvent.click(screen.getByText('Owner'));
90+
fireEvent.click(screen.getByText('Owner'));
91+
expect(ownerColumn()).toEqual(['Mallory', 'Zoe', 'Alice']);
92+
});
93+
94+
it('still sorts a plain text column', () => {
95+
renderTable();
96+
fireEvent.click(screen.getByText('Code'));
97+
expect(ownerColumn()).toEqual(['Mallory', 'Zoe', 'Alice']);
98+
fireEvent.click(screen.getByText('Code'));
99+
expect(ownerColumn()).toEqual(['Alice', 'Zoe', 'Mallory']);
100+
});
101+
});

packages/components/src/renderers/complex/data-table.tsx

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import React, { useState, useMemo, useRef, useEffect, useLayoutEffect } from 're
1111
import { cn } from '../../lib/utils';
1212
import { resolveIcon } from '../action/resolve-icon';
1313
import { useGridFieldAuthoring } from '../../context/gridFieldAuthoring';
14-
import { ComponentRegistry } from '@object-ui/core';
14+
import { ComponentRegistry, compareSortValues, getSortValue } from '@object-ui/core';
1515
import type { DataTableSchema } from '@object-ui/types';
1616
import { useRowPredicate } from '@object-ui/react';
1717
import { createSafeTranslation } from '@object-ui/i18n';
@@ -599,19 +599,26 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => {
599599
);
600600
}, [data, searchQuery, columns]);
601601

602-
// Sorting
602+
// Sorting — client-side, over the rows this table was handed.
603+
//
604+
// Sort keys go through `getSortValue` so a relational column orders by the
605+
// label its cell shows, not by the `$expand`-ed record object (objectui#3096).
606+
// The old `aValue < bValue` was always false for two objects, so a lookup
607+
// column's comparator collapsed to the constant `1` and the sort produced an
608+
// order unrelated to anything on screen.
609+
//
610+
// Decorate → sort → undecorate: the key is resolved ONCE per row rather than
611+
// on every one of the O(n log n) comparisons.
603612
const sortedData = useMemo(() => {
604613
if (!sortColumn || !sortDirection) return filteredData;
605-
606-
return [...filteredData].sort((a, b) => {
607-
const aValue = a[sortColumn];
608-
const bValue = b[sortColumn];
609-
610-
if (aValue === bValue) return 0;
611-
612-
const comparison = aValue < bValue ? -1 : 1;
614+
615+
const keyed = filteredData.map((row) => ({ row, key: getSortValue(row[sortColumn]) }));
616+
// Array#sort is stable, so rows with equal keys keep their incoming order.
617+
keyed.sort((a, b) => {
618+
const comparison = compareSortValues(a.key, b.key);
613619
return sortDirection === 'asc' ? comparison : -comparison;
614620
});
621+
return keyed.map((entry) => entry.row);
615622
}, [filteredData, sortColumn, sortDirection]);
616623

617624
// Pagination. Under manual (server-side) pagination the parent controls the

packages/core/src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ export * from './utils/filter-converter.js';
1818
export * from './utils/managedBy.js';
1919
export * from './utils/extract-records.js';
2020
export * from './utils/expand-fields.js';
21+
export * from './utils/sort-values.js';
2122
export * from './evaluator/index.js';
2223
export * from './actions/index.js';
2324
export * from './query/index.js';
Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* objectui#3096 — a client-side sort must order by the string the cell shows.
11+
*
12+
* The comparators these helpers replace failed in two distinct ways on a
13+
* relational column: `a < b` between two `$expand`-ed record objects is always
14+
* false (so the comparator returned a constant and the rows were permuted into
15+
* a meaningless order), and `String(record)` is `"[object Object]"` (so every
16+
* row compared equal and the sort silently did nothing). Both are pinned below.
17+
*/
18+
import { describe, it, expect } from 'vitest';
19+
import { compareCellValues, compareSortValues, getSortValue } from '../sort-values.js';
20+
21+
const sortBy = <T,>(rows: T[], read: (row: T) => unknown, labels?: Record<string, string>): T[] =>
22+
[...rows].sort((a, b) => compareCellValues(read(a), read(b), { labels }));
23+
24+
describe('getSortValue', () => {
25+
it('reduces an expanded reference record to its display name', () => {
26+
expect(getSortValue({ id: 'rec_7f3', name: 'Alice' })).toBe('Alice');
27+
expect(getSortValue({ id: 'rec_001', full_name: 'Bob Lin' })).toBe('Bob Lin');
28+
expect(getSortValue({ id: 'rec_002', title: 'Q3 Renewal' })).toBe('Q3 Renewal');
29+
});
30+
31+
it('maps a raw foreign-key id through the client-resolved label map', () => {
32+
const labels = { u1: 'Carol', u2: 'Alice' };
33+
expect(getSortValue('u1', { labels })).toBe('Carol');
34+
expect(getSortValue(7, { labels: { '7': 'Dave' } })).toBe('Dave');
35+
// An expanded record still honours a label resolved for the same id.
36+
expect(getSortValue({ id: 'u2', name: 'stale' }, { labels })).toBe('Alice');
37+
// No entry → the raw value, never a crash.
38+
expect(getSortValue('u9', { labels })).toBe('u9');
39+
});
40+
41+
it('keeps primitives in their own domain so numbers stay numeric', () => {
42+
expect(getSortValue(42)).toBe(42);
43+
expect(getSortValue(true)).toBe(true);
44+
expect(getSortValue('Alice')).toBe('Alice');
45+
expect(getSortValue(new Date('2024-03-01T00:00:00Z'))).toBe(Date.parse('2024-03-01T00:00:00Z'));
46+
});
47+
48+
it('treats every flavour of empty as one blank value', () => {
49+
expect(getSortValue(null)).toBeNull();
50+
expect(getSortValue(undefined)).toBeNull();
51+
expect(getSortValue('')).toBeNull();
52+
expect(getSortValue(Number.NaN)).toBeNull();
53+
// A record with nothing name-like must NOT become the literal "Untitled" —
54+
// that would bunch unrelated empty records together under "U".
55+
expect(getSortValue({})).toBeNull();
56+
});
57+
58+
it('joins multi-value cells the way they read', () => {
59+
expect(getSortValue([{ id: 'a', name: 'Alice' }, { id: 'b', name: 'Bob' }])).toBe('Alice, Bob');
60+
expect(getSortValue([])).toBeNull();
61+
});
62+
});
63+
64+
describe('compareSortValues', () => {
65+
it('sorts blanks last ascending (so callers that negate get them first)', () => {
66+
expect(compareSortValues(null, 'Alice')).toBe(1);
67+
expect(compareSortValues('Alice', null)).toBe(-1);
68+
expect(compareSortValues(null, null)).toBe(0);
69+
});
70+
71+
it('compares numbers numerically, not lexicographically', () => {
72+
expect(compareSortValues(9, 10)).toBeLessThan(0);
73+
// `numeric: true` extends that to numbers embedded in strings.
74+
expect(compareSortValues('Item 9', 'Item 10')).toBeLessThan(0);
75+
});
76+
77+
it('orders booleans false before true', () => {
78+
expect(compareSortValues(false, true)).toBeLessThan(0);
79+
expect(compareSortValues(true, true)).toBe(0);
80+
});
81+
});
82+
83+
describe('sorting a relational column (the objectui#3096 regression)', () => {
84+
const rows = [
85+
{ id: '1', owner: { id: 'rec_ccc', name: 'Carol' } },
86+
{ id: '2', owner: { id: 'rec_aaa', name: 'Alice' } },
87+
{ id: '3', owner: { id: 'rec_bbb', name: 'Bob' } },
88+
];
89+
90+
it('orders expanded lookup cells by the label', () => {
91+
expect(sortBy(rows, (r) => r.owner).map((r) => r.owner.name)).toEqual(['Alice', 'Bob', 'Carol']);
92+
});
93+
94+
it('disagrees with id order whenever the ids are opaque', () => {
95+
const opaque = [
96+
{ owner: { id: 'rec_9z', name: 'Alice' } },
97+
{ owner: { id: 'rec_1a', name: 'Carol' } },
98+
{ owner: { id: 'rec_4m', name: 'Bob' } },
99+
];
100+
expect(sortBy(opaque, (r) => r.owner).map((r) => r.owner.name)).toEqual([
101+
'Alice',
102+
'Bob',
103+
'Carol',
104+
]);
105+
// The old server-shaped key (bare id) produces a different, meaningless order.
106+
expect([...opaque].sort((a, b) => a.owner.id.localeCompare(b.owner.id)).map((r) => r.owner.name))
107+
.toEqual(['Carol', 'Bob', 'Alice']);
108+
});
109+
110+
it('does not collapse to a no-op the way String(record) did', () => {
111+
// Every record stringifies to "[object Object]" — the old RelatedList
112+
// comparator therefore returned 0 for every pair and left rows untouched.
113+
const stringified = rows.map((r) => String(r.owner));
114+
expect(new Set(stringified).size).toBe(1);
115+
expect(sortBy(rows, (r) => r.owner).map((r) => r.id)).not.toEqual(rows.map((r) => r.id));
116+
});
117+
118+
it('orders raw-id cells by the client-resolved labels', () => {
119+
const idRows = [{ owner: 'u3' }, { owner: 'u1' }, { owner: 'u2' }];
120+
const labels = { u1: 'Zoe', u2: 'Alice', u3: 'Mallory' };
121+
expect(sortBy(idRows, (r) => r.owner, labels).map((r) => labels[r.owner as keyof typeof labels]))
122+
.toEqual(['Alice', 'Mallory', 'Zoe']);
123+
});
124+
});

0 commit comments

Comments
 (0)