diff --git a/.changeset/7823-objectview-chart-relay-whole-block.md b/.changeset/7823-objectview-chart-relay-whole-block.md new file mode 100644 index 000000000..028d672f4 --- /dev/null +++ b/.changeset/7823-objectview-chart-relay-whole-block.md @@ -0,0 +1,38 @@ +--- +'@object-ui/app-shell': patch +--- + +Forward the authored `chart:` block WHOLE from the object-view relay to `ListView` +(objectui#7823). + +`ObjectView`'s `renderListView` builds the list schema by spreading the object's list +schema and then relaying the active view's visualization blocks under `options`. Every +sibling block is forwarded whole; `chart` was a hand-listed projection of exactly six +keys — `chartType`, `xAxisField`, `yAxisFields`, `aggregation`, `series`, `config` — the +pre-ADR-0021 key set, frozen. The whole ADR-0021 (objectui#1890) authoring shape +(`dataset` / `dimensions` / `values`) and the legacy `categoryField` / `valueField` +spelling had no rung, so a view that declared them reached `ListView` with its binding +stripped, and `ListView` could not tell that from a view that had declared nothing. + +**What changes for authors.** On the object-view route, a list view that declares an +ADR-0021 chart block and whitelists `chart` in `appearance.allowedVisualizations` is now +offered the Chart toggle and renders from the dataset it named. Before this fix the +capability gate added by objectui#7544 was handed six `undefined` keys, correctly +answered "nothing declared" about a view whose author had declared everything, and +ADR-0047 filtered the author's own whitelist down to `['grid']` — no toggle, no +diagnostic. The legacy `xAxisField` / `yAxisFields` spelling did survive the projection +and did resolve, so the two authoring shapes behaved differently on this route for +reasons that lived entirely in that one object literal. + +**Not a wider whitelist.** The projection is replaced by a pointer, not extended from +six keys to nine: a hand-listed key set is a copy, and copies rot silently — three more +keys would buy ADR-0021's correctness while re-arming the identical trap for the next +block key, and nothing would fire then either, because the relay's view definition is +`Record` and a missing rung is invisible to `tsc` (objectui#7559 owns that +mechanism). Forwarding whole is safe because `ListView` reads this block by name at both +of its readers — the capability gate's `resolveListChartBinding` and the `chart` render +branch — and never spreads it. + +A view that declares no chart block, an empty block, or a block with no binding at all +is still offered no Chart toggle: the relay now forwards `undefined` instead of the old +permanently-truthy husk of six `undefined` keys. diff --git a/packages/app-shell/src/views/ObjectView.chartRelay-7823.test.tsx b/packages/app-shell/src/views/ObjectView.chartRelay-7823.test.tsx new file mode 100644 index 000000000..c06da2c77 --- /dev/null +++ b/packages/app-shell/src/views/ObjectView.chartRelay-7823.test.tsx @@ -0,0 +1,340 @@ +/** + * 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#7823 — the object-view relay handed `ListView` a SIX-KEY PROJECTION + * of the authored `chart:` block. + * + * ## The defect this pins + * + * `renderListView` builds `fullSchema` by spreading the OBJECT's `listSchema` + * and then relaying the active view's visualization blocks under `options`. + * Every sibling block is relayed WHOLE — `gantt: ganttViewOptions(viewDef)`, + * `timeline: timelineViewOptions(viewDef)`, `tree`/`gallery` by spread — but + * `chart` was a hand-listed copy of exactly six keys: + * + * chart: { + * chartType, xAxisField, yAxisFields, aggregation, series, config + * } + * + * That list is the PRE-ADR-0021 key set, frozen. `dataset`, `dimensions` and + * `values` — the whole ADR-0021 (#1890) authoring shape — plus the legacy + * `categoryField` / `valueField` spelling had no rung, so they were dropped + * here and could not reach the renderer at all on this route. + * + * ## Why it became load-bearing + * + * objectui#7544 gave `ListView.availableViews` a chart capability check that + * asks `resolveListChartBinding` — the render branch's own resolver — whether + * the block it was handed binds to names the author wrote. Through this + * projection an ADR-0021 block arrived as six `undefined` keys, so the gate + * correctly answered "nothing declared" about a view whose author declared + * everything: no Chart toggle, no diagnostic. The legacy + * `xAxisField` / `yAxisFields` spelling survived the projection and did + * resolve, so the two authoring shapes behaved differently on this route for + * reasons that lived entirely in this one object literal. + * + * ## The fix is a POINTER, not a wider copy + * + * Widening the list from six keys to nine would buy ADR-0021's correctness and + * re-arm the identical trap for the next block key — and nothing would fire + * then either, because `viewDef` is `Record` and a missing rung is + * invisible to `tsc` (that mechanism is objectui#7559's, and #7559 explicitly + * disclaims the census this card belongs to). A hand-listed key projection is a + * COPY of a schema's key set, and copies rot silently. So the relay forwards + * the authored block itself. + * + * The whole relay is safe to write because `ListView` never SPREADS this block: + * `resolveListChartBinding` and `case 'chart'` both read it BY NAME + * (`dataset` / `dimensions` / `values` / `chartType` / `xAxisField` / + * `yAxisFields` / `categoryField` / `valueField` / `aggregation` / `series`). + * Extra authored keys arrive and are ignored, exactly as they are for `gantt` + * and `tree`, so no key this relay stops dropping can collide downstream. + * + * ## Why the REAL `ListView` renders here + * + * The sibling relay tests in this directory stub `ListView` and inspect the + * captured schema, which answers "what does this file hand down". This card's + * claim is about the CAPABILITY GATE one seam further on, so the real component + * renders and the assertion is the Chart toggle's presence in the DOM — the + * objectui#6318 standard, that it renders differently. The captured schema is + * asserted too, as the more precise statement of the same fact. + * + * ## Direction, written before the run (reverse verification) + * + * The six-key literal was PREDICTED to turn the four `THE FIX` arms RED (no + * Chart tab, and `options.chart` carrying six `undefined` keys) while leaving + * every CONTROL's behaviour assertion GREEN — the legacy `xAxisField` arm, + * whose keys the projection already carried, and the three negative controls, + * which declare no usable binding and must be offered nothing in either world. + * + * MEASURED on the unmodified tree, before the fix was written: 5 failed, 3 + * passed. Four of the five are the `THE FIX` arms. The fifth is the SCHEMA half + * of the first negative control (`options.chart` was the six-`undefined`-key + * husk, not absent) — its DOM half, the toggle, was green there as predicted + * and stayed green after. After the fix: 8 passed. + * + * That asymmetry is the point: the old relay emitted a permanently truthy block + * for EVERY view, so a "fix" that merely added three keys to the husk would + * still hand the gate an object for a view that declared nothing. The negative + * controls are what refuse that shape. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, cleanup, waitFor, screen, fireEvent } from '@testing-library/react'; +import { MemoryRouter, Routes, Route } from 'react-router-dom'; + +vi.mock('@object-ui/permissions', () => ({ + usePermissions: () => ({ + check: () => ({ allowed: true }), + checkField: () => true, + getFieldPermissions: () => [], + getRowFilter: () => undefined, + getObjectApiOperations: () => undefined, + roles: [], + isLoaded: false, + hasCapabilities: () => true, + can: () => true, + cannot: () => false, + }), + useFieldPermissions: () => ({ canRead: () => true, canWrite: () => true, permissions: [] }), +})); + +vi.mock('@object-ui/auth', () => ({ + useAuth: () => ({ user: { id: 'u1', name: 'Ada' }, activeOrganization: null }), + useWorkspaceAdminStatus: () => ({ isAdmin: false, isResolved: true }), + createAuthenticatedFetch: () => vi.fn(), +})); + +vi.mock('@object-ui/collaboration', () => ({ + useRealtimeSubscription: () => ({ lastMessage: null }), + useConflictResolution: () => ({ hasConflicts: false, resolveAllConflicts: () => {} }), +})); + +vi.mock('sonner', () => ({ + toast: Object.assign(vi.fn(), { + success: vi.fn(), error: vi.fn(), info: vi.fn(), + warning: vi.fn(), loading: vi.fn(), dismiss: vi.fn(), + }), +})); + +/** + * The list schema this page hands down — captured on the way INTO the real + * `ListView`, which then renders. Both halves of the evidence come from one + * mount: what the relay emitted, and what the capability gate did with it. + */ +let captured: any = null; +vi.mock('@object-ui/plugin-list', async (importOriginal) => { + const actual = await importOriginal>(); + const { createElement } = await import('react'); + return { + ...actual, + ListView: (props: any) => { + captured = props.schema; + return createElement(actual.ListView as any, props); + }, + }; +}); + +vi.mock('@object-ui/plugin-view', async (importOriginal) => ({ + ...(await importOriginal>()), + ObjectView: (props: any) => + props.renderListView?.({ + schema: { ...(props.schema ?? {}) }, + dataSource: props.dataSource, + onEdit: props.onEdit, + className: '', + refreshKey: 0, + }) ?? null, + ViewTabBar: () => null, + ManageViewsDialog: () => null, +})); + +vi.mock('./MetadataInspector', () => ({ + MetadataPanel: () => null, + useMetadataInspector: () => ({ showDebug: false, toggle: () => {} }), +})); +vi.mock('./RecordDetailView', () => ({ RecordDetailView: () => null })); + +import { ObjectView } from './ObjectView'; +import { ExpressionProvider } from '../providers/ExpressionProvider'; + +const OBJECT_NAME = 'duly_task'; + +/** The ADR-0021 (#1890) authoring shape — a semantic dataset selected BY NAME. */ +const ADR_0021_BLOCK = { + dataset: 'task_throughput', + dimensions: ['status'], + values: ['hours'], + chartType: 'bar', +}; + +function objectsWith(view: Record) { + return [ + { + name: OBJECT_NAME, + label: 'Task', + fields: { + id: { type: 'text', label: 'Id' }, + name: { type: 'text', label: 'Name' }, + status: { type: 'text', label: 'Status' }, + hours: { type: 'number', label: 'Hours' }, + }, + listViews: { + by_unit: { label: 'By business unit', type: 'grid', columns: ['name'], ...view }, + }, + }, + ]; +} + +function makeDataSource() { + return { + find: vi.fn(async () => ({ data: [], total: 0 })), + findOne: vi.fn(async () => null), + create: vi.fn(async () => ({})), + update: vi.fn(async () => ({})), + delete: vi.fn(async () => ({})), + getObjectSchema: vi.fn(async () => ({ name: OBJECT_NAME, fields: {} })), + } as any; +} + +/** Mount the object list for one authored view and wait for the relay to run. */ +async function mountObjectList(view: Record) { + captured = null; + const dataSource = makeDataSource(); + render( + + + + {}} />} + /> + + + , + ); + // `options` is built unconditionally by the same object literal as the rung + // under test, so its arrival is the signal that the relay actually ran — + // waiting on `options.chart` itself would hang rather than fail. + await waitFor(() => { + expect(captured?.options).toBeTruthy(); + }); +} + +/** + * Find a visualization option by accessible name, in either switcher form — + * two-to-four resolvable types render an inline segmented control exposing + * `role="tab"`, five or more collapse into a dropdown of plain buttons. + * Mirrors the helper in `plugin-list`'s `ListView.chart-capability-7544`. + */ +const queryViewOption = (name: string) => + screen.queryByRole('tab', { name }) ?? screen.queryByRole('button', { name }); + +/** + * Is the Chart toggle offered for a view whitelisting exactly + * `['grid', 'chart']`? The whitelist alone never suffices — ADR-0047 + * intersects it with what the capability gate finds RESOLVABLE, which is the + * question this relay's output answers. + */ +async function chartOffered(view: Record): Promise { + await mountObjectList({ appearance: { allowedVisualizations: ['grid', 'chart'] }, ...view }); + const trigger = screen.queryByTestId('view-switcher-dropdown'); + if (trigger) fireEvent.click(trigger); + return Boolean(queryViewOption('Chart')); +} + +beforeEach(() => { + cleanup(); + captured = null; + vi.stubGlobal( + 'fetch', + vi.fn(async () => + new Response(JSON.stringify({ data: [] }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }), + ), + ); +}); + +afterEach(() => { + vi.unstubAllGlobals(); + vi.clearAllMocks(); +}); + +describe('the object-view relay forwards the authored chart block WHOLE (objectui#7823)', () => { + it('THE FIX: an ADR-0021 view is offered the Chart toggle', async () => { + // THE DISCRIMINATING ARM, and the card's whole report in one line. Before + // the fix this read `false`: the six-key projection dropped `dataset` / + // `dimensions` / `values`, the gate found nothing declared, and ADR-0047 + // filtered the author's own whitelist down to `['grid']`. + expect(await chartOffered({ chart: ADR_0021_BLOCK })).toBe(true); + }); + + it('THE FIX: the ADR-0021 keys arrive at the renderer, not six undefined ones', async () => { + // The precise statement of the same fact. `toEqual` against the authored + // object pins BOTH directions: no declared key is dropped, and no key the + // author did not write is invented on the way through. + await mountObjectList({ chart: ADR_0021_BLOCK }); + expect(captured.options.chart).toEqual(ADR_0021_BLOCK); + }); + + it('THE FIX: the legacy `categoryField` / `valueField` spelling survives too', async () => { + // Absent from the six-key list exactly as the ADR-0021 keys were, and read + // by the same resolver. One projection dropped both; one pointer carries + // both. + expect(await chartOffered({ chart: { categoryField: 'status', valueField: 'hours' } })).toBe(true); + await mountObjectList({ chart: { categoryField: 'status', valueField: 'hours' } }); + expect(captured.options.chart).toEqual({ categoryField: 'status', valueField: 'hours' }); + }); + + it('THE FIX: keys beyond the block are relayed verbatim, not whitelisted', async () => { + // The rot this card is about is a key set frozen at a moment in time. A + // relay that carried today's nine keys would fail this the day the block + // grows a tenth, which is precisely the trap being closed. + const block = { ...ADR_0021_BLOCK, filter: [{ field: 'status', op: 'eq', value: 'open' }], config: { stacked: true } }; + await mountObjectList({ chart: block }); + expect(captured.options.chart).toEqual(block); + }); + + it('CONTROL: the legacy `xAxisField` / `yAxisFields` block still resolves', async () => { + // GREEN in either world — these keys the old projection already carried. + // It is here so a regression in the relay's shape cannot hide behind the + // arms above, and to pin that the two authoring shapes now behave the same + // on this route, which is the asymmetry the card reported. + expect(await chartOffered({ chart: { xAxisField: 'status', yAxisFields: ['hours'] } })).toBe(true); + }); + + it('NEGATIVE CONTROL: a view declaring no chart block is offered no toggle', async () => { + // GREEN in either world, and the reason the relay must forward `undefined` + // rather than a permanently truthy husk: the old projection was ALWAYS a + // truthy object, and a fix that kept that shape while adding the three keys + // would still be handing the gate an object for a view that declared + // nothing. `resolveListChartBinding` reads no binding out of either, so the + // observable answer is the same — this pins that it stays the same. + expect(await chartOffered({})).toBe(false); + await mountObjectList({}); + expect(captured.options.chart).toBeUndefined(); + }); + + it('NEGATIVE CONTROL: an EMPTY chart block is offered no toggle', async () => { + // The half-written declaration: `allowedVisualizations: ['grid','chart']` + // with nothing under `chart:`. It must stay half-written all the way down — + // the gate offers only blocks that render from names the author wrote. + expect(await chartOffered({ chart: {} })).toBe(false); + }); + + it('NEGATIVE CONTROL: a block with no binding at all is offered no toggle', async () => { + // `chartType` alone chooses a shape, not a binding. Offering this would + // route into the legacy branch's invented `'name'` / `'value'` floors — + // the objectui#7547 / #7029 / #7070 family, deliberately untouched here. + expect(await chartOffered({ chart: { chartType: 'pie' } })).toBe(false); + }); +}); diff --git a/packages/app-shell/src/views/ObjectView.tsx b/packages/app-shell/src/views/ObjectView.tsx index 12af054ae..6070f3429 100644 --- a/packages/app-shell/src/views/ObjectView.tsx +++ b/packages/app-shell/src/views/ObjectView.tsx @@ -2432,14 +2432,39 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an ...((viewDef as any).tree || {}), labelField: (viewDef as any).tree?.labelField || (viewDef as any).tree?.titleField || 'name', }, - chart: { - chartType: viewDef.chart?.chartType, - xAxisField: viewDef.chart?.xAxisField, - yAxisFields: viewDef.chart?.yAxisFields, - aggregation: viewDef.chart?.aggregation, - series: viewDef.chart?.series, - config: viewDef.chart?.config, - }, + // The chart block the view DECLARED, forwarded WHOLE — a + // pointer, not a copy of its key set (objectui#7823). + // + // This was a hand-listed projection of six keys (`chartType` / + // `xAxisField` / `yAxisFields` / `aggregation` / `series` / + // `config`): the PRE-ADR-0021 set, frozen. The whole ADR-0021 + // (#1890) authoring shape (`dataset` / `dimensions` / `values`) + // and the legacy `categoryField` / `valueField` spelling had no + // rung here, so an author who declared them reached `ListView` + // with the binding stripped. Once objectui#7544 gave + // `ListView.availableViews` a chart capability check, that gate + // was handed six `undefined` keys and correctly answered + // "nothing declared" about a view whose author declared + // everything — the Chart toggle stayed hidden, with no + // diagnostic, while the legacy `xAxisField` spelling (which the + // projection did carry) went on resolving. + // + // ⛔ NOT widened to nine keys. A hand-listed projection is a + // COPY of a schema's key set and copies rot silently: three more + // keys would buy this ADR's correctness and re-arm the identical + // trap for the next block key, with nothing to fire — `viewDef` + // is `Record`, so a missing rung is invisible to + // tsc (objectui#7559 owns that mechanism and disclaims the + // census this belongs to). + // + // Forwarding whole is safe because `ListView` never SPREADS this + // block — `resolveListChartBinding` and `case 'chart'` both read + // it BY NAME — so keys it does not consult are ignored exactly + // as they are for the `gantt` / `timeline` / `tree` blocks above, + // which are already relayed whole. Undeclared stays `undefined` + // rather than the old permanently-truthy husk, which is what + // keeps the gate from offering a chart nobody configured. + chart: viewDef.chart, }, };