Skip to content

Commit 24e0e0a

Browse files
os-zhuangclaude
andauthored
feat(components,grid,list): a column-header sort orders the whole list, not the page you can see (#3106) (#3112)
Clicking a column header under server pagination sorted the CURRENT PAGE. The user saw "sorted by this column" and got "these fifty rows are in order; page 2 starts over". The sort was real — its scope was not the one the screen implied — and it had no way out of `data-table` at all: it lived in two `useState`s with no callback, so the layer that issues the request could not see it even in principle. DataTable gains `manualSorting` + a controlled `sort` + `onSortChange`. In that mode it sorts nothing, reports what a header click asks for, and renders `sort` as the indicator — holding NO sort state of its own, because a private copy beside a controlled prop is the shape the defect had. `manualSorting` is independent of `manualPagination`: what matters is whether `data` is a window, not who owns the pager. ObjectGrid turns that into a `$orderby` in both of its server modes (its own fetch and a parent-driven one) and returns to page 1, since a new ordering makes the old page index a different set of rows. ListView lands it in `currentSort` — the same state the toolbar's sort builder writes. One sort with two controls onto it is what makes "does a header sort outrank the saved view's sort?" a non-question rather than a precedence rule someone has to remember. Three decisions worth naming: - A header click REPLACES the order rather than appending, so the column under the cursor is the one the list is sorted by. Multi-key orders still come from the sort builder, and the headers render them numbered. - It cannot ask for "no sort". The third click clears in client mode, which is meaningful there (rows return to the order they arrived in); across a server-paged collection there is no such order (objectstack#4363), so a header offering it would hand the user a worse lie than the one being fixed. - Relational columns render no sort affordance under server sorting: a `lookup` column shows a related record's name while `$orderby` can only order by the stored id (objectstack#4256) — the same reason #3096 removed them from the toolbar's picker. Client-side sorting keys off the rendered label, so those headers stay live there. The header context menu's Sort Ascending/Descending items now route through the same write path; they wrote internal state directly, which under `manualSorting` would have been a menu item that highlights, closes, and changes nothing. Client-side tables are untouched: same three-state cycle, same local sort. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent a17ef09 commit 24e0e0a

9 files changed

Lines changed: 953 additions & 38 deletions

File tree

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
---
2+
"@object-ui/types": minor
3+
"@object-ui/components": minor
4+
"@object-ui/plugin-grid": minor
5+
"@object-ui/plugin-list": minor
6+
---
7+
8+
feat(components,grid,list): a column-header sort orders the whole list, not the page you can see — #3106
9+
10+
Clicking a column header under server pagination sorted **the current page**.
11+
The user saw "sorted by this column" and got "these fifty rows are in order;
12+
page 2 starts over". The sort was real — its scope was not the one the screen
13+
implied — and it had no way out of `data-table` at all: the sort lived in two
14+
`useState`s with no callback, so the layer that issues the request could not
15+
see it even in principle.
16+
17+
`DataTable` gains `manualSorting` + a controlled `sort` + `onSortChange`. In
18+
that mode it sorts nothing, reports what a header click asks for, and renders
19+
`sort` as the indicator — keeping **no** sort state of its own, because a
20+
private copy beside a controlled prop is the shape the defect had.
21+
22+
`ObjectGrid` turns that into a `$orderby` in both of its server modes (its own
23+
fetch, and a parent-driven one), and `ListView` lands it in `currentSort` — the
24+
same state the toolbar's sort builder writes. One sort, two controls: that is
25+
what makes "does a header sort outrank the saved view's sort?" a non-question
26+
rather than a precedence rule someone has to remember.
27+
28+
Three details that are decisions, not incidentals:
29+
30+
- **A header click replaces the order** instead of appending to it, so the
31+
column under the cursor is the one the list is sorted by. Multi-key orders
32+
still come from the sort builder, and the headers render them numbered.
33+
- **It cannot ask for "no sort".** In client mode the third click clears, and
34+
that is meaningful there — the rows return to the order they arrived in.
35+
Across a server-paged collection there is no such order (objectstack#4363), so
36+
a header offering it would hand the user a worse lie than the one being fixed.
37+
Clearing stays with the sort builder, which can restore the view's default.
38+
- **Relational columns render no sort affordance** under server sorting. A
39+
`lookup` column shows a related record's name while `$orderby` can only order
40+
by the stored id (objectstack#4256) — the same reason #3096 removed them from
41+
the toolbar's sort picker. Client-side sorting keys off the rendered label, so
42+
those headers stay live there.
43+
44+
Client-side tables are untouched: same three-state cycle, same local sort.
Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,207 @@
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+
* Server-side ("manual") sorting for DataTable (objectui#3106).
11+
*
12+
* The defect: a column-header click sorted whatever rows the table was holding.
13+
* Under server pagination that is ONE PAGE, so the header said "sorted by this
14+
* column" about fifty rows and nothing about the list they came from — and the
15+
* sort had no way out of the component to reach the layer that fetches.
16+
*
17+
* In manual mode the table sorts nothing, reports the requested sort through
18+
* `onSortChange`, and renders `sort` as the indicator. It keeps **no** sort
19+
* state of its own: a private copy alongside a controlled prop is the shape the
20+
* bug had, so these tests assert the rows are untouched and that the indicator
21+
* follows the prop rather than the click.
22+
*/
23+
import { describe, it, expect, vi } from 'vitest';
24+
import { fireEvent } from '@testing-library/react';
25+
import '@testing-library/jest-dom';
26+
import { renderComponent } from './test-utils';
27+
// Registers the renderers at module scope, NOT inside a `beforeAll` — there the
28+
// cold transform is billed to `hookTimeout` (objectui#3010/#3021).
29+
import '../renderers';
30+
31+
/** Deliberately NOT in `status` order — a local sort would visibly reorder it. */
32+
const pageData = [
33+
{ id: 'r1', name: 'Alpha', status: 'open' },
34+
{ id: 'r2', name: 'Bravo', status: 'done' },
35+
{ id: 'r3', name: 'Charlie', status: 'hold' },
36+
];
37+
38+
const columns = [
39+
{ header: 'Name', accessorKey: 'name' },
40+
{ header: 'Status', accessorKey: 'status' },
41+
];
42+
43+
const baseSchema = {
44+
type: 'data-table' as const,
45+
columns,
46+
data: pageData,
47+
manualSorting: true,
48+
sort: [],
49+
} as any;
50+
51+
const rowOrder = (container: HTMLElement) =>
52+
Array.from(container.querySelectorAll('tbody tr')).map(
53+
(tr) => tr.querySelector('td')?.textContent?.trim(),
54+
);
55+
56+
const headerCell = (container: HTMLElement, label: string) =>
57+
Array.from(container.querySelectorAll('thead th')).find((th) =>
58+
th.textContent?.includes(label),
59+
) as HTMLElement;
60+
61+
describe('data-table — manual (server-side) sorting', () => {
62+
it('does not reorder the rows it was handed', () => {
63+
const { container } = renderComponent({
64+
...baseSchema,
65+
sort: [{ field: 'status', order: 'asc' }],
66+
onSortChange: vi.fn(),
67+
});
68+
// `status` ascending would be done, hold, open. The rows arrive in the
69+
// server's order and stay in it — sorting this window is the whole defect.
70+
expect(rowOrder(container)).toEqual(['Alpha', 'Bravo', 'Charlie']);
71+
});
72+
73+
it('reports a header click instead of sorting locally', () => {
74+
const onSortChange = vi.fn();
75+
const { container } = renderComponent({ ...baseSchema, onSortChange });
76+
77+
fireEvent.click(headerCell(container, 'Status'));
78+
79+
expect(onSortChange).toHaveBeenCalledWith([{ field: 'status', order: 'asc' }]);
80+
expect(rowOrder(container)).toEqual(['Alpha', 'Bravo', 'Charlie']);
81+
});
82+
83+
it('toggles the direction of the column already sorted', () => {
84+
const onSortChange = vi.fn();
85+
const { container } = renderComponent({
86+
...baseSchema,
87+
sort: [{ field: 'status', order: 'asc' }],
88+
onSortChange,
89+
});
90+
91+
fireEvent.click(headerCell(container, 'Status'));
92+
expect(onSortChange).toHaveBeenCalledWith([{ field: 'status', order: 'desc' }]);
93+
});
94+
95+
it('never asks for "no sort" — the third click returns to ascending', () => {
96+
// Client mode cycles asc → desc → none, and none is meaningful there (the
97+
// rows return to the order they arrived in). Across a server-paged
98+
// collection there is no such order: an unsorted paged read is arbitrary
99+
// per page (objectstack#4363), so a header must not be able to request it.
100+
const onSortChange = vi.fn();
101+
const { container } = renderComponent({
102+
...baseSchema,
103+
sort: [{ field: 'status', order: 'desc' }],
104+
onSortChange,
105+
});
106+
107+
fireEvent.click(headerCell(container, 'Status'));
108+
expect(onSortChange).toHaveBeenCalledWith([{ field: 'status', order: 'asc' }]);
109+
// Not `[]`, and not a call with an empty array anywhere.
110+
for (const call of onSortChange.mock.calls) expect(call[0]).not.toHaveLength(0);
111+
});
112+
113+
it('REPLACES the order when a different column is clicked', () => {
114+
// The column under the cursor becomes the one the list is sorted by. A
115+
// header that appended would grow an invisible sort stack — the arrow says
116+
// "Name" while the rows are ordered by Status first.
117+
const onSortChange = vi.fn();
118+
const { container } = renderComponent({
119+
...baseSchema,
120+
sort: [{ field: 'status', order: 'asc' }],
121+
onSortChange,
122+
});
123+
124+
fireEvent.click(headerCell(container, 'Name'));
125+
expect(onSortChange).toHaveBeenCalledWith([{ field: 'name', order: 'asc' }]);
126+
});
127+
128+
it('holds no sort state of its own — the indicator follows the prop, not the click', () => {
129+
// The controlled value never changes, so nothing may appear to have been
130+
// sorted. A table keeping a private copy would light up its own arrow here,
131+
// which is exactly how a sort ends up somewhere the fetching layer cannot
132+
// see it.
133+
const { container } = renderComponent({ ...baseSchema, onSortChange: vi.fn() });
134+
const before = headerCell(container, 'Status').innerHTML;
135+
136+
fireEvent.click(headerCell(container, 'Status'));
137+
138+
expect(headerCell(container, 'Status').innerHTML).toBe(before);
139+
});
140+
141+
it('renders every key of a multi-key sort, numbered', () => {
142+
// Multi-key orders come from a host's sort builder. Marking only the first
143+
// column would say "sorted by Status" about a list ordered by Status then
144+
// Name.
145+
const { container } = renderComponent({
146+
...baseSchema,
147+
sort: [{ field: 'status', order: 'asc' }, { field: 'name', order: 'desc' }],
148+
onSortChange: vi.fn(),
149+
});
150+
151+
expect(headerCell(container, 'Status').textContent).toContain('1');
152+
expect(headerCell(container, 'Name').textContent).toContain('2');
153+
});
154+
155+
it('renders no rank badge for an ordinary single-column sort', () => {
156+
const { container } = renderComponent({
157+
...baseSchema,
158+
sort: [{ field: 'status', order: 'asc' }],
159+
onSortChange: vi.fn(),
160+
});
161+
expect(headerCell(container, 'Status').textContent?.trim()).toBe('Status');
162+
});
163+
164+
it('renders inert headers when no onSortChange was supplied', () => {
165+
// A manual-sorting table with nowhere to report a click must not look
166+
// clickable: a dead affordance is the same class of lie as a sort that
167+
// silently covers one page.
168+
const { container } = renderComponent(baseSchema);
169+
const status = headerCell(container, 'Status');
170+
expect(status.className).not.toContain('cursor-pointer');
171+
// No sort chevron. (Other header chrome — the column drag grip — stays.)
172+
expect(status.querySelector('[class*="chevron"]')).toBeNull();
173+
});
174+
175+
it('honours a column that opts out of sorting', () => {
176+
// How a relational column is withheld: its label is a related record's
177+
// name, but a server `$orderby` can only order by the stored id (#3096).
178+
const onSortChange = vi.fn();
179+
const { container } = renderComponent({
180+
...baseSchema,
181+
columns: [columns[0], { ...columns[1], sortable: false }],
182+
onSortChange,
183+
});
184+
185+
fireEvent.click(headerCell(container, 'Status'));
186+
expect(onSortChange).not.toHaveBeenCalled();
187+
});
188+
189+
it('leaves client-side sorting exactly as it was', () => {
190+
// No `manualSorting`: the table owns the rows, so it sorts them and keeps
191+
// its three-state cycle. This mode is untouched by #3106.
192+
const { container } = renderComponent({
193+
type: 'data-table',
194+
columns,
195+
data: pageData,
196+
} as any);
197+
198+
fireEvent.click(headerCell(container, 'Status'));
199+
expect(rowOrder(container)).toEqual(['Bravo', 'Charlie', 'Alpha']); // done, hold, open
200+
201+
fireEvent.click(headerCell(container, 'Status'));
202+
expect(rowOrder(container)).toEqual(['Alpha', 'Charlie', 'Bravo']); // open, hold, done
203+
204+
fireEvent.click(headerCell(container, 'Status'));
205+
expect(rowOrder(container)).toEqual(['Alpha', 'Bravo', 'Charlie']); // cleared
206+
});
207+
});

0 commit comments

Comments
 (0)