From b1f1e16af1994084e8597930540318aee480da9b Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 17:26:29 +0800 Subject: [PATCH 1/2] feat(components,grid,list): a column-header sort orders the whole list, not the page you can see (#3106) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: Claude Fable 5 --- .changeset/server-side-column-header-sort.md | 44 ++++ .../data-table-manual-sorting.test.tsx | 207 +++++++++++++++++ .../src/renderers/complex/data-table.tsx | 103 +++++++-- packages/plugin-grid/src/ObjectGrid.tsx | 144 ++++++++++-- .../src/__tests__/serverSorting.test.tsx | 212 ++++++++++++++++++ packages/plugin-list/src/ListView.tsx | 34 +++ .../__tests__/ListView.headerSort.test.tsx | 188 ++++++++++++++++ packages/types/src/data-display.ts | 58 +++++ packages/types/src/index.ts | 1 + 9 files changed, 953 insertions(+), 38 deletions(-) create mode 100644 .changeset/server-side-column-header-sort.md create mode 100644 packages/components/src/__tests__/data-table-manual-sorting.test.tsx create mode 100644 packages/plugin-grid/src/__tests__/serverSorting.test.tsx create mode 100644 packages/plugin-list/src/__tests__/ListView.headerSort.test.tsx diff --git a/.changeset/server-side-column-header-sort.md b/.changeset/server-side-column-header-sort.md new file mode 100644 index 000000000..e8e82222f --- /dev/null +++ b/.changeset/server-side-column-header-sort.md @@ -0,0 +1,44 @@ +--- +"@object-ui/types": minor +"@object-ui/components": minor +"@object-ui/plugin-grid": minor +"@object-ui/plugin-list": minor +--- + +feat(components,grid,list): a column-header sort orders the whole list, not the page you can see — #3106 + +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: the sort 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 — keeping **no** sort state of its own, because a +private copy beside a controlled prop is the shape the defect had. + +`ObjectGrid` turns that into a `$orderby` in both of its server modes (its own +fetch, and a parent-driven one), and `ListView` lands it in `currentSort` — the +same state the toolbar's sort builder writes. One sort, two controls: that 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 details that are decisions, not incidentals: + +- **A header click replaces the order** instead of appending to it, 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".** In client mode the third click clears, and + that is meaningful there — the 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. + Clearing stays with the sort builder, which can restore the view's default. +- **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 sort picker. Client-side sorting keys off the rendered label, so + those headers stay live there. + +Client-side tables are untouched: same three-state cycle, same local sort. diff --git a/packages/components/src/__tests__/data-table-manual-sorting.test.tsx b/packages/components/src/__tests__/data-table-manual-sorting.test.tsx new file mode 100644 index 000000000..9e45bfc27 --- /dev/null +++ b/packages/components/src/__tests__/data-table-manual-sorting.test.tsx @@ -0,0 +1,207 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * Server-side ("manual") sorting for DataTable (objectui#3106). + * + * The defect: a column-header click sorted whatever rows the table was holding. + * Under server pagination that is ONE PAGE, so the header said "sorted by this + * column" about fifty rows and nothing about the list they came from — and the + * sort had no way out of the component to reach the layer that fetches. + * + * In manual mode the table sorts nothing, reports the requested sort through + * `onSortChange`, and renders `sort` as the indicator. It keeps **no** sort + * state of its own: a private copy alongside a controlled prop is the shape the + * bug had, so these tests assert the rows are untouched and that the indicator + * follows the prop rather than the click. + */ +import { describe, it, expect, vi } from 'vitest'; +import { fireEvent } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { renderComponent } from './test-utils'; +// Registers the renderers at module scope, NOT inside a `beforeAll` — there the +// cold transform is billed to `hookTimeout` (objectui#3010/#3021). +import '../renderers'; + +/** Deliberately NOT in `status` order — a local sort would visibly reorder it. */ +const pageData = [ + { id: 'r1', name: 'Alpha', status: 'open' }, + { id: 'r2', name: 'Bravo', status: 'done' }, + { id: 'r3', name: 'Charlie', status: 'hold' }, +]; + +const columns = [ + { header: 'Name', accessorKey: 'name' }, + { header: 'Status', accessorKey: 'status' }, +]; + +const baseSchema = { + type: 'data-table' as const, + columns, + data: pageData, + manualSorting: true, + sort: [], +} as any; + +const rowOrder = (container: HTMLElement) => + Array.from(container.querySelectorAll('tbody tr')).map( + (tr) => tr.querySelector('td')?.textContent?.trim(), + ); + +const headerCell = (container: HTMLElement, label: string) => + Array.from(container.querySelectorAll('thead th')).find((th) => + th.textContent?.includes(label), + ) as HTMLElement; + +describe('data-table — manual (server-side) sorting', () => { + it('does not reorder the rows it was handed', () => { + const { container } = renderComponent({ + ...baseSchema, + sort: [{ field: 'status', order: 'asc' }], + onSortChange: vi.fn(), + }); + // `status` ascending would be done, hold, open. The rows arrive in the + // server's order and stay in it — sorting this window is the whole defect. + expect(rowOrder(container)).toEqual(['Alpha', 'Bravo', 'Charlie']); + }); + + it('reports a header click instead of sorting locally', () => { + const onSortChange = vi.fn(); + const { container } = renderComponent({ ...baseSchema, onSortChange }); + + fireEvent.click(headerCell(container, 'Status')); + + expect(onSortChange).toHaveBeenCalledWith([{ field: 'status', order: 'asc' }]); + expect(rowOrder(container)).toEqual(['Alpha', 'Bravo', 'Charlie']); + }); + + it('toggles the direction of the column already sorted', () => { + const onSortChange = vi.fn(); + const { container } = renderComponent({ + ...baseSchema, + sort: [{ field: 'status', order: 'asc' }], + onSortChange, + }); + + fireEvent.click(headerCell(container, 'Status')); + expect(onSortChange).toHaveBeenCalledWith([{ field: 'status', order: 'desc' }]); + }); + + it('never asks for "no sort" — the third click returns to ascending', () => { + // Client mode cycles asc → desc → none, and none is meaningful there (the + // rows return to the order they arrived in). Across a server-paged + // collection there is no such order: an unsorted paged read is arbitrary + // per page (objectstack#4363), so a header must not be able to request it. + const onSortChange = vi.fn(); + const { container } = renderComponent({ + ...baseSchema, + sort: [{ field: 'status', order: 'desc' }], + onSortChange, + }); + + fireEvent.click(headerCell(container, 'Status')); + expect(onSortChange).toHaveBeenCalledWith([{ field: 'status', order: 'asc' }]); + // Not `[]`, and not a call with an empty array anywhere. + for (const call of onSortChange.mock.calls) expect(call[0]).not.toHaveLength(0); + }); + + it('REPLACES the order when a different column is clicked', () => { + // The column under the cursor becomes the one the list is sorted by. A + // header that appended would grow an invisible sort stack — the arrow says + // "Name" while the rows are ordered by Status first. + const onSortChange = vi.fn(); + const { container } = renderComponent({ + ...baseSchema, + sort: [{ field: 'status', order: 'asc' }], + onSortChange, + }); + + fireEvent.click(headerCell(container, 'Name')); + expect(onSortChange).toHaveBeenCalledWith([{ field: 'name', order: 'asc' }]); + }); + + it('holds no sort state of its own — the indicator follows the prop, not the click', () => { + // The controlled value never changes, so nothing may appear to have been + // sorted. A table keeping a private copy would light up its own arrow here, + // which is exactly how a sort ends up somewhere the fetching layer cannot + // see it. + const { container } = renderComponent({ ...baseSchema, onSortChange: vi.fn() }); + const before = headerCell(container, 'Status').innerHTML; + + fireEvent.click(headerCell(container, 'Status')); + + expect(headerCell(container, 'Status').innerHTML).toBe(before); + }); + + it('renders every key of a multi-key sort, numbered', () => { + // Multi-key orders come from a host's sort builder. Marking only the first + // column would say "sorted by Status" about a list ordered by Status then + // Name. + const { container } = renderComponent({ + ...baseSchema, + sort: [{ field: 'status', order: 'asc' }, { field: 'name', order: 'desc' }], + onSortChange: vi.fn(), + }); + + expect(headerCell(container, 'Status').textContent).toContain('1'); + expect(headerCell(container, 'Name').textContent).toContain('2'); + }); + + it('renders no rank badge for an ordinary single-column sort', () => { + const { container } = renderComponent({ + ...baseSchema, + sort: [{ field: 'status', order: 'asc' }], + onSortChange: vi.fn(), + }); + expect(headerCell(container, 'Status').textContent?.trim()).toBe('Status'); + }); + + it('renders inert headers when no onSortChange was supplied', () => { + // A manual-sorting table with nowhere to report a click must not look + // clickable: a dead affordance is the same class of lie as a sort that + // silently covers one page. + const { container } = renderComponent(baseSchema); + const status = headerCell(container, 'Status'); + expect(status.className).not.toContain('cursor-pointer'); + // No sort chevron. (Other header chrome — the column drag grip — stays.) + expect(status.querySelector('[class*="chevron"]')).toBeNull(); + }); + + it('honours a column that opts out of sorting', () => { + // How a relational column is withheld: its label is a related record's + // name, but a server `$orderby` can only order by the stored id (#3096). + const onSortChange = vi.fn(); + const { container } = renderComponent({ + ...baseSchema, + columns: [columns[0], { ...columns[1], sortable: false }], + onSortChange, + }); + + fireEvent.click(headerCell(container, 'Status')); + expect(onSortChange).not.toHaveBeenCalled(); + }); + + it('leaves client-side sorting exactly as it was', () => { + // No `manualSorting`: the table owns the rows, so it sorts them and keeps + // its three-state cycle. This mode is untouched by #3106. + const { container } = renderComponent({ + type: 'data-table', + columns, + data: pageData, + } as any); + + fireEvent.click(headerCell(container, 'Status')); + expect(rowOrder(container)).toEqual(['Bravo', 'Charlie', 'Alpha']); // done, hold, open + + fireEvent.click(headerCell(container, 'Status')); + expect(rowOrder(container)).toEqual(['Alpha', 'Charlie', 'Bravo']); // open, hold, done + + fireEvent.click(headerCell(container, 'Status')); + expect(rowOrder(container)).toEqual(['Alpha', 'Bravo', 'Charlie']); // cleared + }); +}); diff --git a/packages/components/src/renderers/complex/data-table.tsx b/packages/components/src/renderers/complex/data-table.tsx index 3a48786e6..6d58566cc 100644 --- a/packages/components/src/renderers/complex/data-table.tsx +++ b/packages/components/src/renderers/complex/data-table.tsx @@ -12,7 +12,7 @@ import { cn } from '../../lib/utils'; import { resolveIcon } from '../action/resolve-icon'; import { useGridFieldAuthoring } from '../../context/gridFieldAuthoring'; import { ComponentRegistry, compareSortValues, getSortValue } from '@object-ui/core'; -import type { DataTableSchema } from '@object-ui/types'; +import type { DataTableSchema, TableSortItem } from '@object-ui/types'; import { useRowPredicate } from '@object-ui/react'; import { createSafeTranslation } from '@object-ui/i18n'; import { @@ -361,6 +361,9 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { page: controlledPage, onPageChange, onPageSizeChange, + manualSorting = false, + sort: controlledSort, + onSortChange, searchable = true, selectable: selectableProp = false, showSelectionCount = true, @@ -601,6 +604,12 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { // Sorting — client-side, over the rows this table was handed. // + // Under `manualSorting` there is nothing to do: `data` arrived in the order + // the server produced, for the whole collection rather than this window. + // Re-sorting it here would order the CURRENT PAGE — the defect objectui#3106 + // reports, where "sorted by this column" is true of fifty rows and false of + // the list they came from. + // // Sort keys go through `getSortValue` so a relational column orders by the // label its cell shows, not by the `$expand`-ed record object (objectui#3096). // The old `aValue < bValue` was always false for two objects, so a lookup @@ -610,6 +619,7 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { // Decorate → sort → undecorate: the key is resolved ONCE per row rather than // on every one of the O(n log n) comparisons. const sortedData = useMemo(() => { + if (manualSorting) return filteredData; if (!sortColumn || !sortDirection) return filteredData; const keyed = filteredData.map((row) => ({ row, key: getSortValue(row[sortColumn]) })); @@ -619,7 +629,7 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { return sortDirection === 'asc' ? comparison : -comparison; }); return keyed.map((entry) => entry.row); - }, [filteredData, sortColumn, sortDirection]); + }, [filteredData, sortColumn, sortDirection, manualSorting]); // Pagination. Under manual (server-side) pagination the parent controls the // page and supplies the grand total via `rowCount`; `data` already IS the @@ -678,10 +688,54 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { return row.id !== undefined ? row.id : `row-${index}`; }; + // The sort the headers display and cycle. + // + // Under `manualSorting` this is the caller's prop and nothing else — the two + // `useState`s below are not read, not written, and not mirrored. Keeping a + // private copy in sync with a controlled prop is precisely how objectui#3106 + // happened: the table held the user's sort somewhere the layer that fetches + // the rows could not see it. + const activeSort: TableSortItem[] = manualSorting + ? (controlledSort ?? []) + : (sortColumn && sortDirection ? [{ field: sortColumn, order: sortDirection }] : []); + + // A manual-sorting table with nowhere to report a click renders inert headers + // rather than clickable ones that do nothing — a dead affordance is the same + // class of lie as a sort that only covers the current page. + const sortingEnabled = sortable && (!manualSorting || !!onSortChange); + + /** + * Sort by one column in a given direction — the single write path, so the + * column-header cycle and the header context menu cannot diverge about where + * a sort goes. The menu used to call `setSortColumn`/`setSortDirection` + * directly, which under `manualSorting` would have written to state nothing + * reads: a menu item that highlights, closes, and changes nothing. + */ + const applySort = (columnKey: string, order: 'asc' | 'desc') => { + if (manualSorting) { + onSortChange?.([{ field: columnKey, order }]); + return; + } + setSortColumn(columnKey); + setSortDirection(order); + }; + // Handlers const handleSort = (columnKey: string) => { - if (!sortable) return; - + if (!sortingEnabled) return; + + if (manualSorting) { + // Two states, not three. A header click REPLACES the order — the column + // under the cursor becomes the one the list is sorted by — and it never + // produces "no sort": across a server-paged collection that is an + // arbitrary order per page (objectstack#4363), so it is not a state a + // header should be able to ask for. Clearing a sort belongs to the host's + // sort builder, which can also restore the view's configured default. + const current = activeSort.find((s) => s.field === columnKey); + applySort(columnKey, current?.order === 'asc' ? 'desc' : 'asc'); + return; + } + if (sortColumn === columnKey) { if (sortDirection === 'asc') { setSortDirection('desc'); @@ -773,14 +827,31 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { window.URL.revokeObjectURL(url); }; + /** + * The indicator for one column, read from {@link activeSort}. + * + * Every participating key is marked, not just the first: a multi-key sort + * arrives from the host's sort builder, and showing only its primary column + * would say "sorted by Status" about a list actually ordered by Status then + * Rank. The rank badge appears only when there is more than one key, so the + * ordinary single-column case renders exactly as it always has. + */ const getSortIcon = (columnKey: string) => { - if (sortColumn !== columnKey) { + const index = activeSort.findIndex((s) => s.field === columnKey); + if (index === -1) { return ; } - if (sortDirection === 'asc') { - return ; - } - return ; + const Arrow = activeSort[index].order === 'asc' ? ChevronUp : ChevronDown; + return ( + + + {activeSort.length > 1 && ( + + {index + 1} + + )} + + ); }; // Column resizing handlers @@ -1374,7 +1445,7 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { key={col.accessorKey} className={cn( col.className, - sortable && col.sortable !== false && 'cursor-pointer select-none', + sortingEnabled && col.sortable !== false && 'cursor-pointer select-none', isDragging && 'opacity-50', isDragOver && 'border-l-2 border-primary', col.align === 'right' && 'text-right', @@ -1401,7 +1472,7 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { onDragOver={(e) => handleColumnDragOver(e, index)} onDrop={(e) => handleColumnDrop(e, index)} onDragEnd={handleColumnDragEnd} - onClick={() => sortable && col.sortable !== false && handleSort(col.accessorKey)} + onClick={() => sortingEnabled && col.sortable !== false && handleSort(col.accessorKey)} onContextMenu={(e) => handleColumnContextMenu(e, col.accessorKey)} >
{ {col.headerIcon} )} {col.header} - {sortable && col.sortable !== false && getSortIcon(col.accessorKey)} + {sortingEnabled && col.sortable !== false && getSortIcon(col.accessorKey)} {editColumnEnabled && (