diff --git a/.changeset/objectview-named-view-sort-arity.md b/.changeset/objectview-named-view-sort-arity.md new file mode 100644 index 000000000..50ad77f9e --- /dev/null +++ b/.changeset/objectview-named-view-sort-arity.md @@ -0,0 +1,42 @@ +--- +'@object-ui/plugin-view': patch +--- + +`ObjectView` sends a named view's `sort` to the grid slot that can hold it — the declared sort now reaches both the header indicator and `$orderby`. + +A named view's sort is an **array**: `NamedListView.sort` is +`Array< { field, order } >`, and the `views` prop declares an array too. +`ObjectView` forwarded the resolved view sort into `gridSchema.defaultSort`, +which `ObjectGridSchema` declares as a **single** `{ field, order }`. The +arity mismatch had no compile-time witness — `ObjectViewSchema.table` +collapses to a bare index signature — and both of `ObjectGrid`'s readers then +failed, in different ways: + +- **The header drew nothing.** `parseSchemaSort(schemaSort ?? (schema.defaultSort + ? [schema.defaultSort] : undefined))` re-wraps an already-array `defaultSort` + into `[[{ field, order }]]`. Each entry must be a string or an object with a + string `field`; a nested array is neither, so the entry was skipped and the + parse returned `[]`. A view that arrived sorted `name desc` looked unsorted, + and the first click on that column asked for `asc` on a list already `desc`. +- **The fetch sent nonsense.** `` `${(schema.defaultSort as any).field} ${(schema + .defaultSort as any).order}` `` reads two absent keys off an array, so the + request carried the literal string `"undefined undefined"` as `$orderby`. + `serializeOrderBy` passes a non-empty string through untouched, so that + reached the server verbatim. + +The two view precedence segments (`listViews` entry, then the active `views` +entry) now ride the **canonical** `sort` slot, declared `string | SortConfig[]` +— the arity a view actually carries, and the only one of the pair that can +express a multi-key sort at all. The legacy `defaultSort` slot keeps carrying +the `table` segment alone and is read exactly as before. + +**Precedence is unchanged.** `ObjectGrid` resolves `sort ?? defaultSort`, so a +view sort still outranks both `table.sort` and `table.defaultSort`, and a +`table.sort` still outranks a `table.defaultSort` — the same order the non-grid +fetch and the delegated `renderListView` schema already express. A view that +supplies no sort forwards exactly what it forwarded before. + +This is also the shape the shared sort sink accepts (`convertSortToQueryParams` +takes `string | SortConfig[]`), so the fix converges on the normalized dialect +rather than adding another spelling for the sort-sink convergence work to fold +in later. diff --git a/packages/plugin-view/src/ObjectView.tsx b/packages/plugin-view/src/ObjectView.tsx index 0f077bb46..870119efd 100644 --- a/packages/plugin-view/src/ObjectView.tsx +++ b/packages/plugin-view/src/ObjectView.tsx @@ -1038,13 +1038,42 @@ export const ObjectView: React.FC = ({ // free choice here: emitting both slots lets ObjectGrid's existing // canonical-wins rule decide, which is the only answer that keeps the two // layers saying the same thing. + // + // objectui#5270: the two segments AHEAD of `table` had a second, separate + // problem — an ARITY mismatch, not a spelling one. Both of them carry an + // ARRAY of sort keys (`NamedListView.sort` is `Array< { field, order } >`; + // the `views` prop declares an array too) and both were being written into + // `defaultSort`, which is declared a SINGLE `{ field, order }`. Neither of + // ObjectGrid's two readers survives that: + // + // header `parseSchemaSort(schemaSort ?? [schema.defaultSort])` becomes + // `parseSchemaSort([[{ field, order }]])`. The outer array is + // iterated and each entry must be a string or an object with a + // string `field`; a nested ARRAY is neither, so the entry is + // dropped and the result is `[]` — no arrow, the view arrives + // looking unsorted. + // fetch `` `${(schema.defaultSort as any).field} ${….order}` `` reads two + // missing keys off an array and sends the literal string + // `"undefined undefined"` as `$orderby`. + // + // So the view's sort now rides the CANONICAL slot, which is declared + // `string | SortConfig[]` and therefore already holds the arity a view + // carries. That is also the shape the shared sort sink accepts + // (`convertSortToQueryParams`, `string | SortConfig[]` — objectui#4869), so + // this converges on the normalized dialect instead of introducing another. + // Precedence is unchanged: ObjectGrid resolves `sort ?? defaultSort`, so a + // view sort still outranks a `table.defaultSort`, and `table.sort` still + // outranks it too — the same order `mergedSort` and the non-grid fetch use. const gridSchema: ObjectGridSchema = useMemo(() => { - // The two segments ahead of the `table` one, resolved once. They keep - // riding the LEGACY slots they ride today: `filter`/`defaultFilters` are - // not interchangeable downstream — ObjectGrid lowers the canonical slot - // through `toFilterNode` and raw-assigns the legacy one — so moving a - // named-view filter across would change the wire shape of a path this - // card does not own. + // The two segments ahead of the `table` one, resolved once. + // + // `viewFilter` keeps riding the LEGACY slot it rides today: + // `filter`/`defaultFilters` are not interchangeable downstream — + // ObjectGrid lowers the canonical slot through `toFilterNode` and + // raw-assigns the legacy one — so moving a named-view filter across would + // change the wire shape of a path objectui#5270 does not own. The sort + // pair has no such asymmetry: both slots reach `$orderby` unlowered, and + // only the canonical one can hold more than a single key. const viewFilter = currentNamedViewConfig?.filter || activeView?.filter; const viewSort = currentNamedViewConfig?.sort || activeView?.sort; @@ -1060,15 +1089,23 @@ export const ObjectView: React.FC = ({ create: false, // Create is handled by the view's create button }, defaultFilters: viewFilter || schema.table?.defaultFilters, - defaultSort: viewSort || schema.table?.defaultSort, - // Canonical `table` keys, at last forwarded. `filter`/`sort` carry the - // `table` segment ONLY: a view segment resolved above already occupies + // Legacy slot, `table` segment ONLY (objectui#5270). The view segments + // moved to the canonical `sort` below because this one holds a single + // `{ field, order }` and they carry arrays; ObjectGrid resolves + // `sort ?? defaultSort`, so a view sort still outranks this default. + defaultSort: schema.table?.defaultSort, + // Canonical `table` keys, at last forwarded. `filter` carries the + // `table` segment ONLY: the view segment resolved above already occupies // the legacy slot, and ObjectGrid prefers this slot over that one — so // handing it `table.filter` while a named view is active would let the // table default outrank the view, inverting the precedence the two // untouched segments exist to express. filter: viewFilter ? undefined : schema.table?.filter, - sort: viewSort ? undefined : schema.table?.sort, + // `sort` carries the WHOLE chain instead — view segments first, then the + // `table` one. Same precedence as `mergedSort` and the non-grid fetch + // express; what changes is only WHICH slot a view's sort arrives in, and + // this is the one whose declared arity can hold it. + sort: viewSort || schema.table?.sort, pagination: schema.table?.pagination, selection: schema.table?.selection, pageSize: schema.table?.pageSize, diff --git a/packages/plugin-view/src/__tests__/ObjectView.canonicalTableKeys.test.tsx b/packages/plugin-view/src/__tests__/ObjectView.canonicalTableKeys.test.tsx index d19e298a5..6c40b0d38 100644 --- a/packages/plugin-view/src/__tests__/ObjectView.canonicalTableKeys.test.tsx +++ b/packages/plugin-view/src/__tests__/ObjectView.canonicalTableKeys.test.tsx @@ -239,11 +239,16 @@ describe('grid path: both spellings written — each rides its own slot', () => }); describe('grid path: a named view still outranks the table segment', () => { - // The two segments ahead of `table` (`listViews` entry, then `activeView`) - // are untouched by objectui#5102 — they keep riding the legacy slots. The + // The FILTER segments ahead of `table` (`listViews` entry, then `activeView`) + // are untouched by objectui#5102 — they keep riding the legacy slot. The // canonical slot must therefore stay EMPTY while one of them is active: // ObjectGrid prefers the canonical slot, so a `table.filter` forwarded // unconditionally would outrank the view the user is looking at. + // + // The SORT half moved in objectui#5270 — see the sort assertion below for + // why, and `ObjectView.namedViewSortArity.test.tsx` for the two consumers + // that made the old slot unreadable. What is pinned here either way is the + // PRECEDENCE, which the move preserves: the view still outranks `table`. const namedView = { listViews: { won: { @@ -265,12 +270,33 @@ describe('grid path: a named view still outranks the table segment', () => { }); it('keeps the named view sort in force over a table.sort', () => { + // objectui#5102 pinned this as `sort: undefined` + + // `defaultSort: [{ field: 'name', order: 'desc' }]` and said in so many + // words that the pin should be UPDATED, not deleted, by whoever fixed the + // arity. Updated here: the array now rides the canonical slot — the only + // one of the two whose declared type (`string | SortConfig[]`) can hold + // more than one key — and the legacy slot carries the `table` segment + // alone. Precedence is what this block asserts and it is unchanged: + // ObjectGrid resolves `schemaSort ?? defaultSort`, so the named view wins + // over `table.sort` because it is what reaches the canonical slot. const grid = forwardedGridSchema({ ...namedView, table: { sort: 'created asc' } as any, } as any); - expect(grid.sort).toBeUndefined(); - expect(grid.defaultSort).toEqual([{ field: 'name', order: 'desc' }]); + expect(grid.sort).toEqual([{ field: 'name', order: 'desc' }]); + expect(grid.defaultSort).toBeUndefined(); + }); + + it('keeps the named view sort in force over a legacy table.defaultSort too', () => { + // The other side of the same precedence: the `table` segment holds the + // legacy slot alone, and ObjectGrid prefers the canonical one, so the view + // still wins. Without this the move could have inverted the pair silently. + const grid = forwardedGridSchema({ + ...namedView, + table: { defaultSort: { field: 'created', order: 'asc' } } as any, + } as any); + expect(grid.sort).toEqual([{ field: 'name', order: 'desc' }]); + expect(grid.defaultSort).toEqual({ field: 'created', order: 'asc' }); }); }); diff --git a/packages/plugin-view/src/__tests__/ObjectView.namedViewSortArity.test.tsx b/packages/plugin-view/src/__tests__/ObjectView.namedViewSortArity.test.tsx new file mode 100644 index 000000000..0d1bd25b7 --- /dev/null +++ b/packages/plugin-view/src/__tests__/ObjectView.namedViewSortArity.test.tsx @@ -0,0 +1,195 @@ +/** + * 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. + */ + +/** + * objectui#5270 — a named view's `sort` is an ARRAY, and it used to be handed + * to a slot declared to hold ONE key. + * + * `NamedListView.sort` is `Array< { field, order } >`. `ObjectView` forwarded + * the resolved view sort into `gridSchema.defaultSort`, which + * `ObjectGridSchema` declares as a single `{ field: string; order: 'asc' | + * 'desc' }`. Nothing complained — `ObjectViewSchema.table` collapses to a bare + * index signature (objectui#5102), so the arity mismatch had no compile-time + * witness — and BOTH of `ObjectGrid`'s readers then failed, in different ways: + * + * header `parseSchemaSort(schemaSort ?? (schema.defaultSort ? + * [schema.defaultSort] : undefined))` re-wraps an already-array + * `defaultSort` into `[[{ field, order }]]`. `parseSchemaSort` + * accepts a string or an object with a string `field` per entry; a + * nested ARRAY is neither, so the entry is skipped and the parse + * returns `[]`. The user saw NO sort indicator at all. + * fetch `` params.$orderby = `${(schema.defaultSort as any).field} ${… + * .order}` `` reads two absent keys off an array, so the request + * carried the literal string `"undefined undefined"`. + * + * The fix routes the view segments into the CANONICAL `sort` slot, declared + * `string | SortConfig[]` — the arity a view actually carries, and the same + * spelling the shared sort sink `convertSortToQueryParams` accepts + * (objectui#4869), so no fourth dialect is introduced to make this work. + * + * These tests drive the REAL `ObjectGrid` rather than a probe that records the + * forwarded schema: the defect was invisible in the forwarded object (the array + * was right there, in the wrong slot) and only appeared at the two consumers. + * Pinning the forwarded shape alone would have re-pinned the bug. + * + * Resolution note for anyone running an ablation on this file: nothing here + * goes through a build. `../ObjectView` is a relative source import, and + * `@object-ui/plugin-grid` / `@object-ui/components` / `@object-ui/core` are + * aliased to each package's own `src` directory in the root + * `vitest.config.mts`, so a stale `dist` build cannot make a reverted source + * read green. + */ + +import { describe, it, expect, vi, beforeAll } from 'vitest'; +import { render, screen, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import React from 'react'; + +import { ObjectView } from '../ObjectView'; +import { ActionProvider } from '@object-ui/react'; +import type { ObjectViewSchema } from '@object-ui/types'; + +// No `registerAllFields()` here, deliberately: `@object-ui/fields` is not a +// dependency of this package, and nothing asserted below needs a registered +// cell renderer. The header cells and `$orderby` are produced by ObjectGrid +// itself; an unregistered column still renders its label and its sort +// affordance. +beforeAll(() => { + if (!Element.prototype.hasPointerCapture) { + Element.prototype.hasPointerCapture = vi.fn(() => false) as any; + } + if (!Element.prototype.scrollIntoView) { + Element.prototype.scrollIntoView = vi.fn() as any; + } +}); + +function makeDataSource() { + const find = vi.fn(async () => ({ + data: [ + { id: 'a', name: 'Alpha', status: 'open' }, + { id: 'b', name: 'Beta', status: 'open' }, + ], + total: 2, + })); + return { + find, + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn(async (name: string) => ({ + name, + label: 'Task', + fields: { + name: { label: 'Name', type: 'text' }, + status: { label: 'Status', type: 'text' }, + }, + })), + } as any; +} + +/** A view whose sort lives on a named `listViews` entry — the card's subject. */ +const namedViewSchema = (sort: unknown): ObjectViewSchema => + ({ + type: 'object-view', + objectName: 'task', + listViews: { + won: { label: 'Won', columns: ['name', 'status'], sort }, + }, + defaultListView: 'won', + }) as unknown as ObjectViewSchema; + +function renderView(schema: ObjectViewSchema, ds: any) { + return render( + + + , + ); +} + +const lastFindParams = (ds: any) => + ds.find.mock.calls[ds.find.mock.calls.length - 1][1]; + +const headerCell = (container: HTMLElement, label: string) => + Array.from(container.querySelectorAll('thead th')).find((th) => + th.textContent?.includes(label), + ) as HTMLElement; + +describe("objectui#5270 — a named view's sort reaches the grid", () => { + it('draws the declared sort indicator before anyone clicks', async () => { + // Half one. The array used to be re-wrapped to `[[…]]` and parsed to `[]`, + // so the column the view was sorted by showed no arrow — and the first + // click on it then asked for `asc` on a list already ordered `desc`. + const ds = makeDataSource(); + const { container } = renderView( + namedViewSchema([{ field: 'name', order: 'desc' }]), + ds, + ); + await waitFor(() => expect(screen.getByText('Alpha')).toBeInTheDocument()); + + const name = headerCell(container, 'Name'); + expect(name).toBeTruthy(); + expect(name.querySelector('[class*="chevron-down"]')).not.toBeNull(); + }); + + it('sends the declared sort as $orderby, not the string "undefined undefined"', async () => { + // Half two. `${arr.field} ${arr.order}` on an array is two `undefined`s, + // and the resulting `"undefined undefined"` reached the wire verbatim — + // `serializeOrderBy` passes a non-empty string through untouched, so the + // server got an unparseable sort rather than none. + const ds = makeDataSource(); + renderView(namedViewSchema([{ field: 'name', order: 'desc' }]), ds); + + await waitFor(() => expect(ds.find).toHaveBeenCalled()); + await waitFor(() => { + expect(lastFindParams(ds).$orderby).toBe('name desc'); + }); + expect(lastFindParams(ds).$orderby).not.toBe('undefined undefined'); + expect(String(lastFindParams(ds).$orderby)).not.toContain('undefined'); + }); + + it('carries every key of a multi-key sort, which the legacy slot could not hold', async () => { + // The arity is the whole point: `defaultSort` is ONE `{ field, order }`, + // so a two-key view sort had nowhere to land even if the single-key case + // had somehow been made to work. + const ds = makeDataSource(); + const { container } = renderView( + namedViewSchema([ + { field: 'status', order: 'asc' }, + { field: 'name', order: 'desc' }, + ]), + ds, + ); + await waitFor(() => expect(screen.getByText('Alpha')).toBeInTheDocument()); + + await waitFor(() => { + expect(lastFindParams(ds).$orderby).toBe('status asc, name desc'); + }); + expect(headerCell(container, 'Status').querySelector('[class*="chevron-up"]')).not.toBeNull(); + expect(headerCell(container, 'Name').querySelector('[class*="chevron-down"]')).not.toBeNull(); + }); + + it("still honours a table.defaultSort when no view supplies one", async () => { + // The legacy `table` slot keeps working: it is the one shape `defaultSort` + // was always declared to hold, and this fix narrows what is written into + // it rather than retiring it. + const ds = makeDataSource(); + const { container } = renderView( + { + type: 'object-view', + objectName: 'task', + table: { columns: ['name', 'status'], defaultSort: { field: 'name', order: 'desc' } }, + } as unknown as ObjectViewSchema, + ds, + ); + await waitFor(() => expect(screen.getByText('Alpha')).toBeInTheDocument()); + + await waitFor(() => expect(lastFindParams(ds).$orderby).toBe('name desc')); + expect(headerCell(container, 'Name').querySelector('[class*="chevron-down"]')).not.toBeNull(); + }); +});