diff --git a/.changeset/6677-grid-host-fed-default-columns.md b/.changeset/6677-grid-host-fed-default-columns.md new file mode 100644 index 0000000000..60e44f46be --- /dev/null +++ b/.changeset/6677-grid-host-fed-default-columns.md @@ -0,0 +1,60 @@ +--- +'@object-ui/plugin-grid': patch +--- + +An object-bound grid whose rows arrive from a **host** now renders the object +schema's default-columns policy instead of the row payload's keys +(objectui#6677). + +`ObjectGrid.generateColumns()` checks three default paths in order: authored +`columns` → the inline-data path → the object-schema path. The inline-data path +is gated on `hasInlineData` (`dataConfig.provider === 'value'`), and +`dataConfig` is built as `provider: 'value'` from the `data` **prop** before +anything else — so it is taken by every grid whose rows were handed down rather +than fetched. It returned unconditionally whenever rows were present, and its +projection is `schemaFields || Object.keys(inlineData[0])`, the first row's +keys. That made the object-schema path — the one carrying the documented policy +(`highlightFields` first; else drop `hidden`, drop readonly system-managed, push +the remaining system/ownership columns to the end) — **unreachable for every +object-bound grid reached through a fetching host** (`ListView`, `ObjectView`, +…). The branch that knows the object was the one that never ran. + +Measured on the same page, source and object with one variable — who fetches: +`` rendered the policy's **5** columns +(Opportunity Name / Stage / Amount / Close Date / Owner); the same object behind +`` rendered **10**, adding `Id` (`hidden: true`) and the four audit +columns (`system`). Those are exactly what the policy exists to keep off a +default list, and the extra key set was whatever the query happened to return. + +**The yield is as narrow as the defect, and the two boundaries are the change.** +Only the row-key *fallback* is wrong for an object-bound grid, so only that is +given up, and only once there is a policy to give it up to +(`!schemaFields && !!objectName && !!objectSchema`): + +- **An authored `fields` projection still wins.** The schema path drops a name + the object does not declare (`if (!field) return;`), and a host may + legitimately join or derive keys, so an explicit projection is not overridden + — including when it names an audit column on purpose. `!schemaFields` is + exactly the condition under which the `||` reaches for the row keys, so the + gate cannot drift from the fallback it guards. +- **Gating on `objectName` alone would have been a worse defect.** The schema + arrives from an async fetch, so `objectSchema` is `null` on first paint; that + gate falls through to `if (!objectSchema) return []` and paints an empty + header row before flipping. Requiring the *loaded* schema keeps the row-key + columns on screen until the object is actually known, and is also the + graceful fallback when the schema fetch fails or the data source has no + `getObjectSchema` — the grid degrades to heuristic columns rather than going + blank. + +Inline data with no object behind it is untouched: the "Legacy support" path is +reordered, never deleted, and is still the right answer there. + +Scored **patch**, deliberately. No public API moves — no prop, type, export or +signature changes — and this restores the default-columns policy the component +already documents and already applied whenever the grid fetched its own rows; +the host-fed divergence was the defect, not a contract. `minor` was considered, +because the visible column set changes on existing screens, and rejected: the +lost columns were never *declared* by any author, only leaked by the branch +order, and this repo scores behaviour-correcting fixes as patch and reserves +`minor` for new capability (a `major` is never authored here — the fixed group +tracks `@objectstack`). diff --git a/packages/plugin-grid/src/ObjectGrid.tsx b/packages/plugin-grid/src/ObjectGrid.tsx index edde494e5d..0e314c331c 100644 --- a/packages/plugin-grid/src/ObjectGrid.tsx +++ b/packages/plugin-grid/src/ObjectGrid.tsx @@ -2256,8 +2256,47 @@ export const ObjectGrid: React.FC = ({ }); } - // Legacy support: use 'fields' if columns not provided - if (hasInlineData) { + // Legacy support: use 'fields' if columns not provided. + // + // ⭐ THE ORDER OF THIS PATH AND THE OBJECT-SCHEMA PATH BELOW IS + // LOAD-BEARING (objectui#6677). + // + // `hasInlineData` is `dataConfig.provider === 'value'`, and `dataConfig` is + // built as `provider: 'value'` from the `data` PROP before anything else. + // So this path is taken by EVERY grid whose rows were handed down instead + // of fetched — which is every object-bound grid reached through a fetching + // host (`ListView`, `ObjectView`, …). It used to return unconditionally + // whenever rows were present, which made the object-schema path below + // unreachable for all of them: the branch that knows the object was the one + // that never ran. Measured, same page / source / object, one variable: + // grid-fetches rendered the policy's 5 columns, host-fetches rendered 10 — + // the payload's keys, including `id` (`hidden: true`) and the four audit + // columns (`system`), exactly what the policy exists to exclude. + // + // The yield is as NARROW as the defect. Only the ROW-KEY FALLBACK + // (`Object.keys(inlineData[0])`) is wrong for an object-bound grid, so only + // that is given up, and only once there is a policy to give it up TO: + // + // - `schemaFields` present ⇒ this path keeps it. An authored projection + // is the author's contract, and the schema path would silently drop a + // name the object does not declare (`if (!field) return;`) — a host may + // legitimately join or derive keys. `!schemaFields` is exactly the + // condition under which the `||` below reaches for the row keys, so the + // gate and the fallback cannot drift apart. + // - `objectSchema` still `null` ⇒ this path keeps it. ⚠️ Gating on + // `objectName` ALONE is the trap: the schema arrives from an async + // fetch, so `objectSchema` is null on first paint and the grid would + // fall straight through to `if (!objectSchema) return []` and render an + // empty header row before flipping — a worse defect than this one. It + // is also the graceful fallback when the schema fetch fails or the data + // source has no `getObjectSchema`: the row keys stay the answer instead + // of the grid going blank. + // + // Both are pinned in `hostFetchedDefaultColumns-6677.test.tsx`, together + // with the case this file's own comment calls the right one for this path: + // inline data with no object behind it at all. + const rowKeysWouldOutrankSchemaPolicy = !schemaFields && !!objectName && !!objectSchema; + if (hasInlineData && !rowKeysWouldOutrankSchemaPolicy) { const inlineData = dataConfig?.provider === 'value' ? dataConfig.items as any[] : []; if (inlineData.length > 0) { const fieldsToShow = schemaFields || Object.keys(inlineData[0]); @@ -2381,7 +2420,7 @@ export const ObjectGrid: React.FC = ({ }); return generatedColumns; - }, [objectSchema, schemaFields, schemaColumns, dataConfig, hasInlineData, navigation.handleClick, executeAction, data, resolveFieldLabel, translateOptions, schema.objectName, perms]); + }, [objectSchema, schemaFields, schemaColumns, dataConfig, hasInlineData, objectName, navigation.handleClick, executeAction, data, resolveFieldLabel, translateOptions, schema.objectName, perms]); // Formats this grid can actually deliver (objectui#2942): the server stream // handles csv/xlsx/json, the client fallback only csv/json. Declared-but-dead diff --git a/packages/plugin-grid/src/__tests__/hostFetchedDefaultColumns-6677.test.tsx b/packages/plugin-grid/src/__tests__/hostFetchedDefaultColumns-6677.test.tsx new file mode 100644 index 0000000000..b734bcdcf8 --- /dev/null +++ b/packages/plugin-grid/src/__tests__/hostFetchedDefaultColumns-6677.test.tsx @@ -0,0 +1,311 @@ +/** + * 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#6677 — an object-bound grid whose rows arrive from a HOST must still + * render the object-schema default-columns policy, not the row payload's keys. + * + * ## The defect + * + * `generateColumns()` has three default paths and used to check them in this + * order: authored `columns` → the inline-data path → the object-schema path. + * The inline-data path is gated on `hasInlineData` (`dataConfig.provider === + * 'value'`), and `dataConfig` is built as `provider: 'value'` from the `data` + * PROP before anything else. Its projection is `schemaFields || + * Object.keys(inlineData[0])` — the FIRST ROW'S KEYS. + * + * A fetching host always passes `data`. `ListView` with no authored columns + * sends `{ objectName, fields: undefined, columns: undefined }` alongside the + * rows it fetched, so `schemaFields` is absent and the projection fell all the + * way through to the payload keys. The object-schema path — the one carrying + * the documented policy (`highlightFields` first; else drop `hidden`, drop + * readonly system-managed, push the remaining system/ownership columns to the + * end) — was unreachable for EVERY object-bound grid reached through a host. + * The branch that knows the object was the one that never ran. + * + * Measured on the same page / source / object with one variable — who fetches: + * + * grid fetches → 5 columns: Opportunity Name / Stage / Amount / Close Date / Owner + * host fetches → 10 columns: Id, …, Created At, Created By, Updated At, Updated By + * + * `id` was `hidden: true` and the bookkeeping fields `system` — exactly what + * the policy exists to keep off a default list. + * + * ## What is asserted, and why the absence half is load-bearing + * + * The extras are APPENDED to a superset, not substituted for the five. A test + * that only checked "the five are present" passes on the broken build too, so + * the `id`/audit ABSENCE assertions are what make this pin discriminate. + * + * ## RED-FIRST — measured on the merge-base (98188c284), this file only + * + * 3 red / 4 green. The reds: + * + * ✗ renders the schema policy's five columns, not the payload's ten + * → AssertionError: expected [ 'Id', 'Opportunity Name', …(8) ] to + * deeply equal [ 'Opportunity Name', 'Stage', …(3) ] + * ✗ never appends the payload-only keys once the schema has loaded + * → AssertionError: expected [ 'Id', 'Opportunity Name', …(8) ] to have + * a length of 5 but got 10 + * ✗ TRANSITION: the policy takes over once the schema resolves + * → AssertionError: expected [ 'Id', 'Opportunity Name', …(8) ] to + * deeply equal [ 'Opportunity Name', 'Stage', …(3) ] + * + * The elided members are the ten of `PAYLOAD_COLUMNS_LABELLED` below. After the + * fix, 7 green in the same file. + * + * The four greens are boundaries the reorder must not cross — controls, not a + * restatement of the fix: + * + * - inline data with NO object behind it still derives from the row keys + * (the "Legacy support" path is reordered, never deleted); + * - an authored `fields` projection is still honoured verbatim when the rows + * come from a host — including a key the object schema does not declare. + * It is the row-key FALLBACK that yields to the policy, not the whole + * path, so an author who asks for an audit column or a host-joined key by + * name still gets it; + * - first paint with the schema still IN FLIGHT renders the row-key columns + * rather than an empty header row. This is the case the naive reorder + * (gate the legacy path on `objectName` alone) gets wrong: `objectSchema` + * is `null` until an async fetch lands, so that gate falls through to + * `if (!objectSchema) return []` and paints ZERO data columns first — a + * worse defect than the one being fixed. Pinned in both directions: the + * control above holds the first paint, the third red holds the flip. + */ +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 { ObjectGrid } from '../ObjectGrid'; +import { registerAllFields } from '@object-ui/fields'; +import { ActionProvider } from '@object-ui/react'; + +registerAllFields(); + +beforeAll(() => { + if (!Element.prototype.hasPointerCapture) { + Element.prototype.hasPointerCapture = vi.fn(() => false) as any; + } + if (!Element.prototype.scrollIntoView) { + Element.prototype.scrollIntoView = vi.fn() as any; + } +}); + +/** + * The object from the measurement. Declaration order is the POLICY's output + * order (`Object.keys(fields)`), which is deliberately NOT the payload's key + * order below — that difference is what lets the two column sets be told apart + * by content rather than by luck. + * + * `owner_id` is the interesting one: framework-injected and `system`, but NOT + * readonly (ownership is reassignable), so the policy KEEPS it and pushes it to + * the end rather than dropping it. That is why the expected set is five, not + * four. + */ +const OPPORTUNITY_SCHEMA = { + name: 'opportunity', + label: 'Opportunity', + fields: { + name: { type: 'text', label: 'Opportunity Name' }, + stage: { + type: 'select', + label: 'Stage', + options: [ + { value: 'proposal', label: 'Proposal' }, + { value: 'won', label: 'Won' }, + ], + }, + amount: { type: 'currency', label: 'Amount', currency: 'USD' }, + close_date: { type: 'date', label: 'Close Date' }, + owner_id: { type: 'lookup', label: 'Owner', reference_to: 'user', system: true }, + id: { type: 'text', label: 'Id', hidden: true, system: true, readonly: true }, + created_at: { type: 'datetime', label: 'Created At', system: true, readonly: true }, + created_by: { type: 'lookup', label: 'Created By', reference_to: 'user', system: true, readonly: true }, + updated_at: { type: 'datetime', label: 'Updated At', system: true, readonly: true }, + updated_by: { type: 'lookup', label: 'Updated By', reference_to: 'user', system: true, readonly: true }, + }, +}; + +/** The policy's answer for `OPPORTUNITY_SCHEMA` — business fields, ownership last. */ +const POLICY_COLUMNS = ['Opportunity Name', 'Stage', 'Amount', 'Close Date', 'Owner']; + +/** + * What the payload's keys derive to once the schema HAS loaded — the legacy + * path labels each key from `objectSchema.fields[key].label`, so this is the + * ten-column set the issue measured on screen. + */ +const PAYLOAD_COLUMNS_LABELLED = [ + 'Id', 'Opportunity Name', 'Amount', 'Stage', 'Close Date', 'Owner', + 'Created At', 'Created By', 'Updated At', 'Updated By', +]; + +/** + * The same keys with NO schema to label them — the legacy path's own + * humanisation (`charAt(0).toUpperCase() + slice(1).replace(/_/g, ' ')`). + * This is what "no object behind the data" and "schema still in flight" look + * like, and it is how those two cases are told apart from the labelled one. + */ +const PAYLOAD_COLUMNS_HUMANISED = [ + 'Id', 'Name', 'Amount', 'Stage', 'Close date', 'Owner id', + 'Created at', 'Created by', 'Updated at', 'Updated by', +]; + +/** The five the policy exists to keep off a default list. */ +const EXCLUDED_BY_POLICY = ['Id', 'Created At', 'Created By', 'Updated At', 'Updated By']; + +/** Rows exactly as a fetching host hands them down — every stored key present. */ +const HOST_ROWS = [ + { + id: 'opp-1', + name: 'Acme expansion', + amount: 42000, + stage: 'proposal', + close_date: '2026-09-30', + owner_id: 'u-1', + created_at: '2026-08-01T10:00:00Z', + created_by: 'u-9', + updated_at: '2026-08-20T10:00:00Z', + updated_by: 'u-9', + }, +]; + +function makeDataSource(overrides: Record = {}) { + return { + // A host owns the fetch, so the grid must never call this. Asserted below. + find: vi.fn(async () => ({ data: [], total: 0 })), + getObjectSchema: vi.fn(async () => OPPORTUNITY_SCHEMA), + ...overrides, + } as any; +} + +/** + * The DATA columns' header labels, in render order. + * + * Two kinds of furniture are dropped: cells with no header text (selection + * checkbox, row-action kebab) and the row-index column, whose header is a + * literal `#`. What is left is exactly the derived data columns. + */ +function dataHeaders(container: HTMLElement): string[] { + return Array.from(container.querySelectorAll('thead th')) + .map((th) => (th.textContent ?? '').trim()) + .filter((text) => text.length > 0 && text !== '#'); +} + +function renderHostFedGrid(schemaOverrides: Record = {}, dataSource?: any) { + const ds = dataSource ?? makeDataSource(); + const schema: any = { + type: 'object-grid', + objectName: 'opportunity', + ...schemaOverrides, + }; + const utils = render( + + + , + ); + return { ...utils, ds }; +} + +describe('ObjectGrid — host-fetched rows still get the object-schema policy (#6677)', () => { + it('renders the schema policy\'s five columns, not the payload\'s ten', async () => { + const { container, ds } = renderHostFedGrid(); + + await waitFor(() => expect(screen.getByText('Acme expansion')).toBeInTheDocument()); + await waitFor(() => expect(dataHeaders(container)).toEqual(POLICY_COLUMNS)); + + // The absence half. The broken build APPENDS the excluded five to a + // superset, so a presence-only check passes there too. + const headers = dataHeaders(container); + for (const excluded of EXCLUDED_BY_POLICY) { + expect(headers).not.toContain(excluded); + } + + // The host owns the fetch; the grid must not have gone looking for rows. + expect(ds.find).not.toHaveBeenCalled(); + }); + + it('never appends the payload-only keys once the schema has loaded', async () => { + const { container, ds } = renderHostFedGrid(); + + await waitFor(() => expect(ds.getObjectSchema).toHaveBeenCalledWith('opportunity')); + await waitFor(() => expect(dataHeaders(container)).toHaveLength(POLICY_COLUMNS.length)); + expect(dataHeaders(container)).not.toEqual(expect.arrayContaining(EXCLUDED_BY_POLICY)); + expect(dataHeaders(container)).not.toEqual(PAYLOAD_COLUMNS_LABELLED); + }); + + it('TRANSITION: the policy takes over once the schema resolves', async () => { + let releaseSchema: ((schema: unknown) => void) | undefined; + const pending = new Promise((resolve) => { releaseSchema = resolve; }); + const ds = makeDataSource({ getObjectSchema: vi.fn(() => pending) }); + + const { container } = renderHostFedGrid({}, ds); + await waitFor(() => expect(screen.getByText('Acme expansion')).toBeInTheDocument()); + + releaseSchema!(OPPORTUNITY_SCHEMA); + + await waitFor(() => expect(dataHeaders(container)).toEqual(POLICY_COLUMNS)); + }); + + /* ---------------------------------------------------------------- * + * Boundaries — green in BOTH worlds. Controls, not restatements. * + * ---------------------------------------------------------------- */ + + it('CONTROL: inline data with no object behind it still derives from the row keys', async () => { + const { container } = render( + + + , + ); + + await waitFor(() => expect(screen.getByText('Acme expansion')).toBeInTheDocument()); + // No `objectName`, so no schema can outrank anything: the "Legacy support" + // path is still the right answer and still runs. + expect(dataHeaders(container)).toEqual(PAYLOAD_COLUMNS_HUMANISED); + }); + + it('CONTROL: an authored `fields` projection is honoured verbatim over host rows', async () => { + const { container, ds } = renderHostFedGrid({ fields: ['name', 'created_at'] }); + + await waitFor(() => expect(ds.getObjectSchema).toHaveBeenCalled()); + await waitFor(() => expect(screen.getByText('Acme expansion')).toBeInTheDocument()); + + // The author asked for an audit column BY NAME. The policy drops that field + // from its DEFAULTS; it must not veto an explicit request. + await waitFor(() => + expect(dataHeaders(container)).toEqual(['Opportunity Name', 'Created At'])); + }); + + it('CONTROL: an authored `fields` key the object schema does not declare survives', async () => { + const { container, ds } = renderHostFedGrid({ fields: ['name', 'computed_score'] }); + + await waitFor(() => expect(ds.getObjectSchema).toHaveBeenCalled()); + await waitFor(() => expect(screen.getByText('Acme expansion')).toBeInTheDocument()); + + // A host may join or derive keys that are not object fields. The row-key + // FALLBACK is what yields to the policy — an authored projection does not, + // and the schema path would silently drop `computed_score` (`if (!field) + // return;`). That is why the reorder is gated on `!schemaFields`. + await waitFor(() => + expect(dataHeaders(container)).toEqual(['Opportunity Name', 'Computed score'])); + }); + + it('CONTROL: first paint with the schema in flight shows the row keys, never zero columns', async () => { + const pending = new Promise(() => { /* never resolves */ }); + const ds = makeDataSource({ getObjectSchema: vi.fn(() => pending) }); + + const { container } = renderHostFedGrid({}, ds); + await waitFor(() => expect(screen.getByText('Acme expansion')).toBeInTheDocument()); + + // `objectSchema` is still `null`. Gating the legacy path on `objectName` + // alone would fall through to `if (!objectSchema) return []` and paint an + // empty header row here. + expect(dataHeaders(container).length).toBeGreaterThan(0); + expect(dataHeaders(container)).toEqual(PAYLOAD_COLUMNS_HUMANISED); + }); +});