fix(components,plugin-list): the Sort picker lists a field it keeps only for the current sort as removable only (objectui#11943)

.changeset/11943-sort-in-use-removable-only.md

@@ -0,0 +1,10 @@
+---
+'@object-ui/components': minor
+'@object-ui/plugin-list': patch
+---
+
+The list's Sort picker lists a field it keeps only for the current sort as removable, never as a new choice (objectui#11943).
+
+`SortBuilder`'s `fields` entries take an optional `disabled`. A disabled entry is drawn as an unavailable option in every row's dropdown and cannot be chosen by click or keyboard. A row whose field it already is still shows its label, and can be changed to another field or removed. "Add sort" seeds the first entry that is not disabled, and is disabled when every entry is. An entry without the flag behaves as before.
+
+`ListView` sets the flag on each field its Sort picker keeps only because the current sort names it: a field the user may not read, a field the platform refuses to order by, and a relational field listed as ordering by ID. Before this, a stored or URL sort on such a field left it choosable in the picker's other rows, and "Add sort" seeded it when it came first.

packages/components/src/__tests__/sort-builder-disabled-entry-11943.test.tsx

@@ -0,0 +1,199 @@
+/**
+ * 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#11943 — `SortBuilder` honours `disabled` on a `fields` entry.
+ *
+ * A host sometimes has to list a field it must not offer: the field a row
+ * already sorts by, kept so that row is not blank and can be removed, while no
+ * other row may pick it (the ListView Sort picker's in-use exception). Before
+ * this the entries were `{ value, label }` only, and one shared list fed every
+ * row's dropdown and "Add sort", so a kept field was choosable everywhere.
+ *
+ * Every case reads the real `SortBuilder` and its real Radix `Select`, and sits
+ * beside a control that runs the same interaction on the same entry WITHOUT the
+ * flag: a negative such as "onChange was not called" is otherwise satisfied by a
+ * dropdown that never opened or an option that was never there.
+ */
+import { describe, it, expect, vi, afterEach } from 'vitest';
+import React from 'react';
+import { cleanup, render, screen, fireEvent, within } from '@testing-library/react';
+import '@testing-library/jest-dom';
+import { SortBuilder, type SortBuilderProps, type SortItem } from '../custom/sort-builder';
+
+type Fields = NonNullable<SortBuilderProps['fields']>;
+
+/** `secret` is the kept entry: flagged here, and listed FIRST so "Add sort" would seed it. */
+const FLAGGED: Fields = [
+  { value: 'secret', label: 'Secret', disabled: true },
+  { value: 'title', label: 'Title' },
+  { value: 'status', label: 'Status' },
+];
+
+/** The control: the same entries, in the same order, with no flag. */
+const PLAIN: Fields = FLAGGED.map(({ value, label }) => ({ value, label }));
+
+type Row = Pick<SortItem, 'field' | 'order'>;
+
+/** Holds the value the way a host does, so a change is rendered back. */
+function Harness({ fields, initial, onChange }: { fields: Fields; initial: Row[]; onChange: (rows: Row[]) => void }) {
+  const [value, setValue] = React.useState<SortItem[]>(() =>
+    initial.map((row, i) => ({ id: `r${i}`, ...row })),
+  );
+  return (
+    <SortBuilder
+      fields={fields}
+      value={value}
+      onChange={(next) => {
+        onChange(next.map(({ field, order }) => ({ field, order })));
+        setValue(next);
+      }}
+    />
+  );
+}
+
+function mount(fields: Fields, initial: Row[] = []) {
+  const onChange = vi.fn();
+  render(<Harness fields={fields} initial={initial} onChange={onChange} />);
+  return onChange;
+}
+
+/** Each row is `[field select, direction select]`; these are the field selects. */
+function fieldTriggers(): HTMLElement[] {
+  return screen.getAllByRole('combobox').filter((_, i) => i % 2 === 0);
+}
+
+/** Open a row's field dropdown and return its option elements. */
+async function open(trigger: HTMLElement): Promise<HTMLElement[]> {
+  fireEvent.click(trigger);
+  const listbox = await screen.findByRole('listbox');
+  return within(listbox).getAllByRole('option');
+}
+
+function option(options: HTMLElement[], label: string): HTMLElement {
+  const found = options.find((o) => o.textContent?.trim() === label);
+  if (!found) throw new Error(`no option "${label}"`);
+  return found;
+}
+
+const asc = (field: string): Row => ({ field, order: 'asc' });
+
+afterEach(() => {
+  cleanup();
+});
+
+describe('SortBuilder honours `disabled` on a fields entry (objectui#11943)', () => {
+  it('renders a flagged entry as an unavailable option, and the same entry unflagged as an ordinary one', async () => {
+    mount(FLAGGED, [asc('title')]);
+    let options = await open(fieldTriggers()[0]);
+    expect(options.map((o) => o.textContent?.trim())).toEqual(['Secret', 'Title', 'Status']);
+    expect(option(options, 'Secret')).toHaveAttribute('aria-disabled', 'true');
+    expect(option(options, 'Secret')).toHaveAttribute('data-disabled');
+    expect(option(options, 'Title')).not.toHaveAttribute('aria-disabled');
+    expect(option(options, 'Title')).not.toHaveAttribute('data-disabled');
+
+    cleanup();
+    mount(PLAIN, [asc('title')]);
+    options = await open(fieldTriggers()[0]);
+    expect(option(options, 'Secret')).not.toHaveAttribute('aria-disabled');
+    expect(option(options, 'Secret')).not.toHaveAttribute('data-disabled');
+  });
+
+  it('a flagged entry cannot be chosen by click; unflagged, the same click chooses it', async () => {
+    let onChange = mount(FLAGGED, [asc('title')]);
+    let secret = option(await open(fieldTriggers()[0]), 'Secret');
+    fireEvent.click(secret);
+    fireEvent.pointerDown(secret, { pointerType: 'mouse' });
+    fireEvent.pointerUp(secret, { pointerType: 'mouse' });
+    expect(onChange).not.toHaveBeenCalled();
+    // Nothing was chosen, so the list is still open; close it to read the row.
+    fireEvent.keyDown(screen.getByRole('listbox'), { key: 'Escape' });
+    expect(fieldTriggers()[0]).toHaveTextContent('Title');
+
+    cleanup();
+    onChange = mount(PLAIN, [asc('title')]);
+    secret = option(await open(fieldTriggers()[0]), 'Secret');
+    fireEvent.click(secret);
+    expect(onChange).toHaveBeenLastCalledWith([asc('secret')]);
+    expect(fieldTriggers()[0]).toHaveTextContent('Secret');
+  });
+
+  it('a flagged entry cannot be chosen from the keyboard; unflagged, the same keys choose it', async () => {
+    // Enter on the option itself.
+    let onChange = mount(FLAGGED, [asc('title')]);
+    fireEvent.keyDown(option(await open(fieldTriggers()[0]), 'Secret'), { key: 'Enter' });
+    expect(onChange).not.toHaveBeenCalled();
+
+    cleanup();
+    onChange = mount(PLAIN, [asc('title')]);
+    fireEvent.keyDown(option(await open(fieldTriggers()[0]), 'Secret'), { key: 'Enter' });
+    expect(onChange).toHaveBeenLastCalledWith([asc('secret')]);
+
+    // Typeahead on the closed trigger, which picks the next entry after the
+    // current one whose label starts with the typed key. From `Status` the
+    // only other "s" entry is `Secret`.
+    cleanup();
+    onChange = mount(FLAGGED, [asc('status')]);
+    fireEvent.keyDown(fieldTriggers()[0], { key: 's' });
+    expect(onChange).not.toHaveBeenCalled();
+    expect(fieldTriggers()[0]).toHaveTextContent('Status');
+
+    cleanup();
+    onChange = mount(PLAIN, [asc('status')]);
+    fireEvent.keyDown(fieldTriggers()[0], { key: 's' });
+    expect(onChange).toHaveBeenLastCalledWith([asc('secret')]);
+  });
+
+  it('a row whose field is a flagged entry shows its label, and can be changed and removed', async () => {
+    let onChange = mount(FLAGGED, [asc('secret'), asc('status')]);
+    // Not blank: the label of a value with no entry at all renders nothing,
+    // which is what this assertion would read if the flag dropped the entry.
+    expect(fieldTriggers()[0]).toHaveTextContent('Secret');
+
+    // Changed to another field.
+    fireEvent.click(option(await open(fieldTriggers()[0]), 'Title'));
+    expect(onChange).toHaveBeenLastCalledWith([asc('title'), asc('status')]);
+    expect(fieldTriggers()[0]).toHaveTextContent('Title');
+
+    // Removed: the row's only button is its remove control.
+    cleanup();
+    onChange = mount(FLAGGED, [asc('secret'), asc('status')]);
+    const row = screen.getByText('Sort by').parentElement as HTMLElement;
+    fireEvent.click(within(row).getByRole('button'));
+    expect(onChange).toHaveBeenLastCalledWith([asc('status')]);
+  });
+
+  it('control: a value with no entry renders a blank row, which the flagged row above is not', () => {
+    mount(FLAGGED, [asc('nowhere')]);
+    expect(fieldTriggers()[0].textContent?.trim()).toBe('');
+  });
+
+  it('"Add sort" seeds the first entry that is not flagged; unflagged, it seeds the first entry', () => {
+    let onChange = mount(FLAGGED);
+    fireEvent.click(screen.getByRole('button', { name: /add sort/i }));
+    expect(onChange).toHaveBeenLastCalledWith([asc('title')]);
+
+    cleanup();
+    onChange = mount(PLAIN);
+    fireEvent.click(screen.getByRole('button', { name: /add sort/i }));
+    expect(onChange).toHaveBeenLastCalledWith([asc('secret')]);
+  });
+
+  it('"Add sort" is disabled when no entry can be chosen, and enabled when one can', () => {
+    mount(FLAGGED.map((f) => ({ ...f, disabled: true })), [asc('secret')]);
+    expect(screen.getByRole('button', { name: /add sort/i })).toBeDisabled();
+
+    cleanup();
+    mount(FLAGGED, [asc('secret')]);
+    expect(screen.getByRole('button', { name: /add sort/i })).toBeEnabled();
+
+    cleanup();
+    mount([]);
+    expect(screen.getByRole('button', { name: /add sort/i })).toBeDisabled();
+  });
+});

packages/components/src/custom/sort-builder.tsx

@@ -45,9 +45,17 @@ export interface SortItem extends SpecSortItem {
 }
 
 export interface SortBuilderProps {
-  fields?: Array<{ 
+  fields?: Array<{
     value: string
     label: string
+    /**
+     * Listed but not choosable (objectui#11943). A disabled entry still names
+     * a row whose current field it is, so that row shows its label and can be
+     * changed or removed, but no row's dropdown lets the user pick it and
+     * "Add sort" never seeds it. "Add sort" seeds the first entry that is not
+     * disabled, and is itself disabled when every entry is.
+     */
+    disabled?: boolean
   }>;
   value?: SortItem[];
   onChange?: (value: SortItem[]) => void;
@@ -89,10 +97,14 @@ export function SortBuilder({
     onChange?.(newItems);
   };
 
+  // A new row starts on the first field the user may choose: a disabled
+  // entry is only there for the row that already names it.
+  const firstChoosable = fields.find((f) => !f.disabled);
+
   const addItem = () => {
     const newItem: SortItem = {
       id: crypto.randomUUID(),
-      field: fields[0]?.value || "",
+      field: firstChoosable?.value || "",
       order: 'asc',
     };
     handleChange([...items, newItem]);
@@ -124,7 +136,7 @@ export function SortBuilder({
                   </SelectTrigger>
                   <SelectContent>
                     {fields.map(f => (
-                      <SelectItem key={f.value} value={f.value}>{f.label}</SelectItem>
+                      <SelectItem key={f.value} value={f.value} disabled={f.disabled}>{f.label}</SelectItem>
                     ))}
                   </SelectContent>
                 </Select>
@@ -159,7 +171,7 @@ export function SortBuilder({
         size="sm"
         onClick={addItem}
         className="h-8"
-        disabled={fields.length === 0}
+        disabled={!firstChoosable}
       >
         <Plus className="h-3 w-3 mr-2" />
         {t('sortBuilder.addSort')}

packages/plugin-list/src/ListView.tsx

@@ -3188,8 +3188,8 @@ export const ListView = React.forwardRef<ListViewHandle, ListViewProps>(({
   // it can. The Filter panel's list and the Sort picker's list ask through
   // `canReadField` (objectui#11925, objectui#11943). One exception: the Sort
   // picker keeps a field the current sort already names, so its row can be
-  // removed. Until objectui#11943's follow-up that field is also still
-  // choosable in the picker's other rows.
+  // removed. It lists that field disabled, so no other row and no "Add sort"
+  // can choose it.
   const effectiveFields = React.useMemo(() => {
     // Defensive: `columns` is `string[] | ListColumn[]`, but metadata is
     // user-authored — anything non-array degrades to "no declared columns".
@@ -4123,12 +4123,18 @@ export const ListView = React.forwardRef<ListViewHandle, ListViewProps>(({
   // user may not read is not offered: the server answers a sort on it with a
   // 403, and the list blanks to its no-access state. A dropped field does not
   // raise the relational hint either; the hint explains a missing relation the
-  // user could otherwise read. The in-use exception covers this rule too. A
+  // user could otherwise read. The in-use exception covers this rule too: a
   // field the current sort already names (a stored or URL sort) stays listed,
-  // exactly as the two rules above would list it and with no mark, so its row
-  // is not blank and can be removed. Until objectui#11943's follow-up it is
-  // also still choosable in every other row, and by "Add sort" when it comes
-  // first: listing it as removable only needs a `SortBuilder` change.
+  // so its row is not blank and can be removed.
+  //
+  // An entry the exception alone keeps is listed REMOVABLE ONLY
+  // (objectui#11943): it carries `disabled`, which `SortBuilder` renders as an
+  // unavailable option. Its own row still shows its label and can be changed
+  // to another field or removed, but no row's dropdown offers it as a choice
+  // and "Add sort" never seeds it. That holds for each reason the exception
+  // keeps a field: unreadable, relational, or refused by the platform (or by
+  // the type read when no projection is served). A field the two rules list
+  // anyway carries no flag, whether or not the sort names it.
   //
   // 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,
@@ -4143,21 +4149,25 @@ export const ListView = React.forwardRef<ListViewHandle, ListViewProps>(({
   const { sortFields, sortHasRelationalField } = React.useMemo(() => {
     const inUse = new Set(currentSort.map((item) => item.field).filter(Boolean));
     let excluded = false;
-    const fields: Array<{ value: string; label: string }> = [];
+    const fields: Array<{ value: string; label: string; disabled?: boolean }> = [];
     for (const field of candidateFields) {
-      if (!inUse.has(field.value) && !canReadField(perms, schema.objectName, field.value)) continue;
+      const readable = canReadField(perms, schema.objectName, field.value);
+      if (!readable && !inUse.has(field.value)) continue;
       const relational = EXPANDABLE_FIELD_TYPES.has(field.type);
       const platformSortable = platformSortability
         ? isPlatformSortableField(platformSortability, field.value)
         : !UNMATERIALIZED_FIELD_TYPES.has(field.type);
-      if (!relational && platformSortable) {
+      if (readable && !relational && platformSortable) {
         fields.push({ value: field.value, label: field.label });
         continue;
       }
       if (inUse.has(field.value)) {
+        // Listed only because the current sort names it: `disabled`, so its
+        // own row shows it and can drop it, and nothing can choose it anew.
         fields.push({
           value: field.value,
           label: relational ? `${field.label} ${t('list.sortByIdSuffix')}` : field.label,
+          disabled: true,
         });
         continue;
       }

packages/plugin-list/src/__tests__/ListView.sortFieldRead-11943.test.tsx

@@ -197,13 +197,12 @@ describe('the Sort picker field list asks the column read check (objectui#11943)
     });
   });
 
-  it('a field the current sort already names stays listed (the known half-state)', async () => {
+  it('a field the current sort already names stays listed, so its row is named', async () => {
     // objectui#11943's ruling keeps such a field listed so its row is not
-    // blank and can be removed. This pins the half shipped here: it stays
-    // listed exactly as before, with no mark, and it is still choosable in the
-    // picker's other rows. Listing it as removable only (marked unavailable,
-    // never offered as a new choice) needs a `SortBuilder` change and is
-    // objectui#11943's follow-up, which will change this pin.
+    // blank and can be removed. It is listed as removable only: disabled, so
+    // no other row and no "Add sort" can choose it. That half is pinned, for
+    // each reason the picker keeps a field, in
+    // `ListView.sortRemovableOnly-11943.test.tsx`.
     mount({ perms: RESTRICTED, sort: [{ field: 'secret_note', order: 'asc' }] });
     const labels = await openSortOptions();
     expect(labels).toContain('Secret Note');