From 8761cfe7e0734684ffe3961fafb38a2878a721e8 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 14:38:36 +0000 Subject: [PATCH 1/2] fix(plugin-list): separate what the sort picker lists from what it persists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The toolbar sort picker keeps a platform-refused field listed while the current sort names it — the only way a user can remove a sort the server refuses outright (#6108). But it rendered and emitted from the same array, so any other edit in the popover re-emitted the refused entry and the host turned it into `persistViewPatch({ sort })`: a personalization PUT storing a column the platform answers `400 INVALID_SORT` for, written by a user who never touched that row. Route every `onSortChange` through one emit boundary that drops what the served projection refuses, and leave `currentSort` whole so the entry stays listed and removable. Same separation #5729 made at the grid seam. --- packages/plugin-list/src/ListView.tsx | 66 ++++- .../ListView.sortPersistLeak.test.tsx | 258 ++++++++++++++++++ 2 files changed, 316 insertions(+), 8 deletions(-) create mode 100644 packages/plugin-list/src/__tests__/ListView.sortPersistLeak.test.tsx diff --git a/packages/plugin-list/src/ListView.tsx b/packages/plugin-list/src/ListView.tsx index a4ff10e1e..f52456576 100644 --- a/packages/plugin-list/src/ListView.tsx +++ b/packages/plugin-list/src/ListView.tsx @@ -21,7 +21,7 @@ import { useDensityMode } from '@object-ui/react'; import type { ListViewSchema, ObjectMapConfig } from '@object-ui/types'; import { detectStatusField } from '@object-ui/types'; import { usePullToRefresh } from '@object-ui/mobile'; -import { resolveConditionalFormatting, buildExpandFields, buildExportFileName, resolveEffectiveCrudAffordances, isObjectInlineEditable, partitionRowsByPredicate, normalizeListViewSchema, rowHeightToDensityMode, mergeFilterNodes, columnIdentity, collectPredicateFieldRefs, listViewPredicates, PLATFORM_RECORD_COLUMNS, EXPANDABLE_FIELD_TYPES, UNMATERIALIZED_FIELD_TYPES, readObjectSortability, isPlatformSortableField } from '@object-ui/core'; +import { resolveConditionalFormatting, buildExpandFields, buildExportFileName, resolveEffectiveCrudAffordances, isObjectInlineEditable, partitionRowsByPredicate, normalizeListViewSchema, rowHeightToDensityMode, mergeFilterNodes, columnIdentity, collectPredicateFieldRefs, listViewPredicates, PLATFORM_RECORD_COLUMNS, EXPANDABLE_FIELD_TYPES, UNMATERIALIZED_FIELD_TYPES, readObjectSortability, isPlatformSortableField, filterPlatformSortableSort } from '@object-ui/core'; import { useObjectTranslation, useObjectLabel, useSafeFieldLabel, createSafeTranslation, useDisplayLocale } from '@object-ui/i18n'; // Two resolvers, two vocabularies — the repo spells the distinction into the // NAMES (objectui#4167). `resolveInlineI18nLabel` is the spec's own @@ -2464,8 +2464,18 @@ export const ListView = React.forwardRef(({ // neither renders a blank row nor silently drops that sort on the next edit. // For a platform-refused field that exception is the only way to REMOVE the // offending row, since the sort it names is one the server refuses outright. + // + // ONE read of the served projection, for BOTH legs below — the list this + // picker renders, and the sort it emits for a host to persist. Read twice, + // the two copies could answer differently about the same field on the same + // render, which is the drift `isPlatformSortableField` was consolidated to + // end. `undefined` stays "no signal served", never "nothing is sortable". + const platformSortability = React.useMemo( + () => readObjectSortability(objectDef), + [objectDef], + ); + const { sortFields, sortHasRelationalField } = React.useMemo(() => { - const platformSortability = readObjectSortability(objectDef); const inUse = new Set(currentSort.map((item) => item.field).filter(Boolean)); let excluded = false; const fields: Array<{ value: string; label: string }> = []; @@ -2488,7 +2498,44 @@ export const ListView = React.forwardRef(({ if (relational) excluded = true; } return { sortFields: fields, sortHasRelationalField: excluded }; - }, [candidateFields, currentSort, t, objectDef]); + }, [candidateFields, currentSort, t, platformSortability]); + + /** + * [#6455] THE persist boundary: what this picker LISTS is not what it + * PERSISTS. + * + * The exception just above deliberately keeps a platform-refused field + * listed while the CURRENT sort names it — it is the only way a user can + * REMOVE a sort the server refuses outright. But the picker used to render + * and emit from the SAME array, so any OTHER edit in the popover — adding a + * second key, flipping a direction — re-emitted the refused entry, and the + * host's `onSortChange` turned it into `persistViewPatch({ sort })`: a + * personalization PUT storing a column the platform answers + * `400 INVALID_SORT` for, written by a user who never touched that row. + * + * So the two legs part company HERE, at the one boundary every emit crosses, + * exactly as #5729 parted them at the grid seam (`ObjectGrid`'s + * `manualSort` / `manualOnSortChange` pair). The refused entry stays in + * `currentSort` — listed, removable, and still the order this list asks the + * server for — while no write ever carries it. Removing it persists the + * removal; a sort with nothing refused in it is emitted unchanged. + * + * Every `onSortChange` in this component goes through here rather than each + * call site filtering for itself: a builder edit, a header click and a + * "reset to default" are three doors onto ONE stored `sort`, and a filter + * spelled three times is a filter one new door can be added without. + * + * Only under a served projection: with no signal there is no verdict to + * filter by, and the pre-objectstack#10235 behaviour stands unchanged. + */ + const emitSortChange = React.useCallback((next: SortItem[]) => { + if (!onSortChange) return; + onSortChange( + platformSortability + ? filterPlatformSortableSort(next, platformSortability) + : next, + ); + }, [onSortChange, platformSortability]); /** * A column-header sort from the child grid (#3106). @@ -2513,8 +2560,8 @@ export const ListView = React.forwardRef(({ })); setCurrentSort(items); setServerPage(1); - onSortChange?.(items); - }, [onSortChange]); + emitSortChange(items); + }, [emitSortChange]); /** * "Reset to the view's default sort" (objectui#4243) — the way back the @@ -2535,9 +2582,9 @@ export const ListView = React.forwardRef(({ const restored = parseSortConfig(schema.sort); setCurrentSort(restored); setServerPage(1); - onSortChange?.(restored); + emitSortChange(restored); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [schemaSortKey, onSortChange]); + }, [schemaSortKey, emitSortChange]); // Export handler const handleExport = React.useCallback((format: 'csv' | 'xlsx' | 'json' | 'pdf') => { @@ -3033,8 +3080,11 @@ export const ListView = React.forwardRef(({ fields={sortFields} value={currentSort} onChange={(newSort) => { + // `setCurrentSort` takes the array WHOLE (the in-use + // exception depends on it); `emitSortChange` is what the + // host persists. See the boundary's docblock above. setCurrentSort(newSort); - if (onSortChange) onSortChange(newSort); + emitSortChange(newSort); }} /> {sortHasRelationalField && ( diff --git a/packages/plugin-list/src/__tests__/ListView.sortPersistLeak.test.tsx b/packages/plugin-list/src/__tests__/ListView.sortPersistLeak.test.tsx new file mode 100644 index 000000000..de9693e3b --- /dev/null +++ b/packages/plugin-list/src/__tests__/ListView.sortPersistLeak.test.tsx @@ -0,0 +1,258 @@ +/** + * 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. + */ + +/** + * [#6455] The toolbar sort picker LISTS a platform-refused field it must never + * PERSIST. + * + * #6108 gave the picker the served sortability signal (objectstack#10235 + * ruling A) for the RENDER leg — "should this control be offered?" — and kept + * one deliberate exception: a field the CURRENT sort already names stays + * listed, because that is the only way a user can REMOVE a sort the server + * refuses outright. Withholding it unconditionally renders a blank row nobody + * can delete. + * + * The picker then rendered and emitted from the SAME array. So an edit to a + * DIFFERENT part of the sort — a second key, a direction, a reset — re-emitted + * the refused entry, and the host turned that into `persistViewPatch({ sort })`: + * a personalization PUT storing a column the platform answers + * `400 INVALID_SORT` for, written by a user who never touched that row. + * + * ## What each test is holding down + * + * The two halves pull in opposite directions on one array, so neither half is + * assertable alone: "the leak is closed" is trivially satisfied by a picker + * that lists nothing, and "the entry is still listed" is trivially satisfied + * by the leak. Every fix-direction test below therefore sits beside a control + * that the naive fix (filtering the array the picker renders from) would fail: + * + * - LEAK (fix direction) — an unrelated edit, and a reset, write a payload + * with the refused entry gone. + * - LISTED + REMOVABLE (control) — the refused entry is still offered while + * in use, and removing it persists the removal. + * - UNCHANGED (control) — a sort with nothing refused in it persists exactly + * as before, and a deployment that serves NO projection is untouched. + * + * The host is modelled as `ObjectView` writes it — `onSortChange` → a + * `persistViewPatch({ sort })` spy — so what these tests read is the payload + * that reaches stored view state, not an intermediate array. + */ +import { describe, it, expect, vi } from 'vitest'; +import { render, screen, fireEvent, waitFor, within } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import React from 'react'; +import { resolveObjectSortability } from '@objectstack/spec/api'; +import { attachObjectSortability } from '@object-ui/core'; +import { ListView } from '../ListView'; +import type { ListViewSchema } from '@object-ui/types'; +import { SchemaRendererProvider } from '@object-ui/react'; + +const objectDef = { + name: 'crm_opportunity', + label: 'Opportunity', + fields: { + name: { type: 'text', label: 'Name' }, + amount: { type: 'currency', label: 'Amount' }, + // The refused column. A plain stored `text` field, so no type read + // anywhere refuses it — the refusal is the platform's and only the + // platform's, which is what makes it the right cell for this card. + remote_status: { type: 'text', label: 'Remote Status' }, + }, +}; + +/** + * The served projection: the platform's own resolver (`@objectstack/spec/api` + * — the one the REST layer serves from), with `remote_status` set to the + * refusal a deployment past objectstack#10235 answers with. AGREEMENT over + * hardcoding for every other cell. + */ +function servedProjection() { + const resolved = resolveObjectSortability(objectDef) as { fields: Record }; + const fields: Record = { ...resolved.fields }; + // Sanity on the base the refusal is measured against: if the resolver ever + // stopped offering these two, the controls below would be measuring nothing. + expect(fields.name).toEqual({ sortable: true }); + expect(fields.amount).toEqual({ sortable: true }); + fields.remote_status = { sortable: false }; + return { fields }; +} + +const makeDataSource = (opts: { servesSignal?: boolean } = {}) => ({ + find: vi.fn().mockResolvedValue({ data: [], total: 0 }), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn(async () => { + const schema = JSON.parse(JSON.stringify(objectDef)); + if (opts.servesSignal !== false) attachObjectSortability(schema, servedProjection()); + return schema; + }), +}); + +const baseSchema: ListViewSchema = { + type: 'list-view', + objectName: 'crm_opportunity', + viewType: 'grid', + columns: ['name', 'amount', 'remote_status'] as any, +}; + +/** + * Render, open the sort popover, and hand back the host's write spy. + * + * The spy is cleared once the popover is open, so every assertion below reads + * the write caused by THE interaction it performs and not by anything the + * mount happened to emit. + */ +async function openSortPopover( + sort: Array<{ field: string; order: 'asc' | 'desc' }>, + dsOpts: { servesSignal?: boolean } = {}, +) { + const dataSource = makeDataSource(dsOpts); + // The host, spelled as `ObjectView` spells it: + // `onSortChange: (sort) => persistViewPatch(viewDef.id, viewDef, { sort })`. + const persistViewPatch = vi.fn(); + render( + + persistViewPatch({ sort: next })} + /> + , + ); + await waitFor(() => expect(dataSource.getObjectSchema).toHaveBeenCalled()); + fireEvent.click(screen.getByRole('button', { name: /^sort/i })); + await screen.findByText('Sort Records'); + persistViewPatch.mockClear(); + return { persistViewPatch }; +} + +/** + * The sort payload that reached stored view state, minus `SortBuilder`'s + * synthetic row `id` — which is a React key, never part of the contract. + */ +function persistedSort(spy: ReturnType) { + expect(spy).toHaveBeenCalled(); + const last = spy.mock.calls[spy.mock.calls.length - 1][0] as { sort: any[] }; + return last.sort.map((item: any) => ({ field: item.field, order: item.order })); +} + +/** Labels offered by the sort field s in the row are + * `role="combobox"`, which leaves exactly one button inside it. + */ +function removeSortRow(label: 'Sort by' | 'Then by') { + const row = screen.getByText(label).parentElement as HTMLElement; + fireEvent.click(within(row).getByRole('button')); +} + +/** "Add sort" — an edit to a DIFFERENT part of the sort than the refused row. */ +function addSortRow() { + fireEvent.click(screen.getByRole('button', { name: /add sort/i })); +} + +describe('ListView sort picker — what it lists is not what it persists (#6455)', () => { + it('drops the platform-refused entry from the write an UNRELATED edit causes', async () => { + // A view stored before the signal existed: its sort names a column the + // platform now answers `400 INVALID_SORT` for. + const { persistViewPatch } = await openSortPopover([ + { field: 'remote_status', order: 'asc' }, + ]); + + // The user adds a second sort key. They never touched the refused row. + addSortRow(); + + // The write carries the edit the user made — and nothing else. Before this + // card it carried `remote_status` too, straight into `persistViewPatch`. + expect(persistedSort(persistViewPatch)).toEqual([{ field: 'name', order: 'asc' }]); + }); + + it('keeps the refused entry LISTED while it is the current sort', async () => { + // The control the naive fix — filtering the array the picker renders from + // — fails. Without this the refused row renders blank and unremovable, + // which is a worse defect than the leak. + await openSortPopover([{ field: 'remote_status', order: 'asc' }]); + + expect(await sortFieldOptions()).toEqual(['Name', 'Amount', 'Remote Status']); + }); + + it('persists the REMOVAL when the user deletes the refused row', async () => { + const { persistViewPatch } = await openSortPopover([ + { field: 'remote_status', order: 'asc' }, + ]); + + removeSortRow('Sort by'); + + // Listed, removable, and the removal reaches stored view state — the whole + // point of keeping the entry visible. + expect(persistedSort(persistViewPatch)).toEqual([]); + }); + + it('leaves a sort with nothing refused in it exactly as it was', async () => { + const { persistViewPatch } = await openSortPopover([ + { field: 'amount', order: 'asc' }, + ]); + + addSortRow(); + + // Both entries survive, in order: the filter is identity over a sort the + // platform will order by. + expect(persistedSort(persistViewPatch)).toEqual([ + { field: 'amount', order: 'asc' }, + { field: 'name', order: 'asc' }, + ]); + }); + + it('drops the refused entry from a "reset to default" write too', async () => { + // The third door onto the same stored `sort`. Reset restores the view's + // DECLARED array whole — so a view whose declared sort names a refused + // column would re-persist it on every reset. + const { persistViewPatch } = await openSortPopover([ + { field: 'remote_status', order: 'asc' }, + ]); + + // Diverge from the declared sort first: the reset control is deliberately + // disabled while the active sort already equals the default. + addSortRow(); + fireEvent.click(screen.getByTestId('sort-reset-default')); + + // The declared sort, minus what the platform refuses. + expect(persistedSort(persistViewPatch)).toEqual([]); + }); + + it('changes nothing where the deployment serves NO projection', async () => { + // `undefined` is "no signal served", not "nothing is sortable": a + // deployment older than objectstack#10235, or an inline/mock data source. + // There is no verdict to filter by, so the pre-#10235 behaviour stands — + // this is the pre-#6455 payload, byte for byte. + const { persistViewPatch } = await openSortPopover( + [{ field: 'remote_status', order: 'asc' }], + { servesSignal: false }, + ); + + addSortRow(); + + expect(persistedSort(persistViewPatch)).toEqual([ + { field: 'remote_status', order: 'asc' }, + { field: 'name', order: 'asc' }, + ]); + }); +}); From 2914bd51673236d246e07adc1f5c3990e75e37ae Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 14:46:12 +0000 Subject: [PATCH 2/2] chore(changeset): declare the ListView sort-persist fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A published behaviour fix in @object-ui/plugin-list, scored patch — `scripts/check-changeset-presence.mjs` demanded it and now passes. --- .changeset/6455-listview-sort-persist-leak.md | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 .changeset/6455-listview-sort-persist-leak.md diff --git a/.changeset/6455-listview-sort-persist-leak.md b/.changeset/6455-listview-sort-persist-leak.md new file mode 100644 index 000000000..c082fcf6a --- /dev/null +++ b/.changeset/6455-listview-sort-persist-leak.md @@ -0,0 +1,32 @@ +--- +'@object-ui/plugin-list': patch +--- + +`ListView`'s toolbar sort picker no longer persists a sort the platform refuses to order by. + +The picker keeps a platform-refused field listed while the CURRENT sort names it +(#6108). That exception is deliberate and stays: it is the only way a user can +REMOVE a sort the server answers `400 INVALID_SORT` for — withholding the option +unconditionally renders a blank row nobody can delete, and drops the sort silently +on the next edit. + +What was wrong is that the picker rendered and emitted from the same array. Editing +anything ELSE in that popover — adding a second sort key, resetting to the view's +default — re-emitted the whole array with the refused entry still in it, and the +host's `onSortChange` turned that into `persistViewPatch({ sort })`: a +personalization PUT storing a refused column, written by a user who never touched +that row. A view stored before the sortability signal existed therefore kept +re-persisting its refused `$orderby` indefinitely. + +Every `onSortChange` this component emits — the builder, the column-header sort and +"reset to default" — now crosses one boundary that drops what the served projection +refuses, while `currentSort` keeps the array whole. So what the picker LISTS and +what it PERSISTS are separate: the refused entry stays visible and removable, +removing it persists the removal, and no write carries it. This is the separation +#5729 already made at the grid seam (`ObjectGrid`'s `manualSort` / +`manualOnSortChange` pair); the picker was the second door onto the same stored +view state. + +Only under a served sortability projection (objectstack#10235 ruling A). `undefined` +means NO SIGNAL SERVED — an older deployment, an inline/mock data source — not +"nothing is sortable", and that branch is byte-identical in behaviour to before.