diff --git a/.changeset/olive-donkeys-invent.md b/.changeset/olive-donkeys-invent.md new file mode 100644 index 000000000..b3139d133 --- /dev/null +++ b/.changeset/olive-donkeys-invent.md @@ -0,0 +1,17 @@ +--- +"@object-ui/app-shell": patch +--- + +ObjectView no longer forwards `chart.config` onto the `object-chart` node it builds. + +`@objectstack/spec`'s `ListChartConfigSchema` is a `strictObject` declaring exactly +`chartType` / `dataset` / `dimensions` / `values`, and objectui binds it by reference, so +`config` is refused by name (`unrecognized_keys ["config"]`) — by the client schema and by +the same schema the platform's metadata write door parses every save through. The rung was +therefore a channel no conforming author could feed, on both the dataset and the legacy +branch, and it landed on a node that does not declare `config` either. + +For every spec-conforming view this is a no-op. A non-conforming row that somehow carried +the key degrades rather than breaks: `ChartRenderer` generates a container config from +`series` plus a positional palette when none is present, so the chart still renders, with +series-derived labels and default colours. diff --git a/packages/app-shell/src/views/ObjectView.chartConfigForward-7891.test.tsx b/packages/app-shell/src/views/ObjectView.chartConfigForward-7891.test.tsx new file mode 100644 index 000000000..44b32ce7a --- /dev/null +++ b/packages/app-shell/src/views/ObjectView.chartConfigForward-7891.test.tsx @@ -0,0 +1,294 @@ +/** + * 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#7891 — `ObjectView`'s dedicated chart-view route forwarded + * `chart.config` onto the `object-chart` node it builds, on BOTH branches: + * + * schema={{ type: 'object-chart', …, config: chartConfig.config, … } as any} + * + * ## Why that rung was a channel nobody could feed + * + * `@objectstack/spec`'s `ListChartConfigSchema` is a `strictObject` whose + * declared keys are exactly `chartType` / `dataset` / `dimensions` / `values`, + * and objectui binds that schema BY REFERENCE — `chart` is absent from + * `LIST_VIEW_LOCAL_OVERRIDES` (`packages/types/src/zod/objectql.zod.ts`), so + * `ListViewSchema.chart` IS the spec's strict schema. Measured against the + * published `@objectstack/spec@17.2.0` this repo resolves, a whole view body + * carrying the key is refused by name: + * + * unrecognized_keys keys=["config"] path=["config","chart"] + * + * and that is not a client-side opinion — it is the SAME schema the platform's + * metadata write door parses every save through (`saveMetaItem` -> + * `resolveOverlaySchema` -> `getMetadataTypeSchema('view')`, answering + * `422 INVALID_METADATA`, on draft and publish alike, `force` or not). So no + * conforming author could write the key and no write path could store it: the + * forward was not one rendering of a legal declaration, it was a second + * UNDECLARED channel — and `ObjectChartSchema` does not declare `config` + * either, so it landed on a node that had no name for it. + * + * ## Why nothing caught it, and why the casts went with it + * + * `ObjectChart` is published as `(props: any)`, so the literal was type-checked + * against nothing at all. The `as any` on both literals was therefore INERT, + * not load-bearing — measured: `tsc --noEmit` over a program that provably + * contains `ObjectView.tsx` is green with both casts removed. Leaving an inert + * cast sitting exactly where the defect lived is what invites the next reader + * to assume the shape was checked. + * + * ## What this file pins, and what it deliberately does not + * + * It reads the schema `ObjectView` HANDS DOWN, on each of the two branches — + * not what `ChartRenderer` does with it. `'config' in schema` is the assertion + * rather than `toBeUndefined()`, because the removed line emitted a PRESENT + * `config: undefined` key for every chart view, authored or not; `undefined` + * would have passed against the very defect. + * + * The positive controls are not decoration: without them a pin asserting only + * an ABSENCE goes green when nothing renders at all. + * + * ## Direction, written before the run (reverse verification) + * + * Re-injecting `config: chartConfig.config,` into either literal was PREDICTED + * to turn that branch's absence arm RED while leaving every positive control + * GREEN. Measured — see the PR body for the run. + * + * ## Not a break, a degrade + * + * `ChartRenderer` generates a container config from `series` plus a positional + * palette when none is present, so a non-conforming row that somehow existed + * still renders — with series-derived labels and default colours. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, cleanup, waitFor } 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 `object-chart` node this file builds — captured on the way INTO + * `ObjectChart`, which is where the removed rung wrote. `ObjectView` reaches it + * through `lazy(() => import('@object-ui/plugin-charts'))`, so the module mock + * is what the dynamic import resolves to. + */ +let capturedChartSchema: any = null; +vi.mock('@object-ui/plugin-charts', () => ({ + ObjectChart: (props: any) => { + capturedChartSchema = props.schema; + return null; + }, + ChartRenderer: () => null, +})); + +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 off-spec value an author would have had to write for the removed rung to + * carry anything. Kept here so the arms below read as one question: does this + * key reach the node? It must not — the protocol refuses it by name. + */ +const OFF_SPEC_CONFIG = { hours: { label: 'Hours logged', color: '#8884d8' } }; + +/** ADR-0021 (#1890) dataset binding — `ObjectView`'s FIRST chart branch. */ +const DATASET_BLOCK = { + dataset: 'task_throughput', + dimensions: ['status'], + values: ['hours'], + chartType: 'bar', +}; + +/** The pre-ADR-0021 inline spelling — `ObjectView`'s SECOND (legacy) branch. */ +const LEGACY_BLOCK = { + chartType: 'line', + xAxisField: 'status', + yAxisFields: ['hours'], + aggregation: 'sum', +}; + +function objectsWith(chart: 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: 'chart', columns: ['name'], chart }, + }, + }, + ]; +} + +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 dedicated chart view for one authored block and capture the node. */ +async function mountChartView(chart: Record) { + capturedChartSchema = null; + const dataSource = makeDataSource(); + render( + + + + {}} />} + /> + + + , + ); + // `type` is set by the same object literal the rung sat in, so its arrival is + // the signal that the branch actually ran. + await waitFor(() => { + expect(capturedChartSchema?.type).toBe('object-chart'); + }); + return capturedChartSchema; +} + +beforeEach(() => { + cleanup(); + capturedChartSchema = 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('ObjectView builds no `config` rung on the object-chart node (objectui#7891)', () => { + it('DATASET BRANCH: an authored `chart.config` does not reach the node', async () => { + // THE DISCRIMINATING ARM for the first branch. `in` rather than + // `toBeUndefined()` — the removed line emitted the key PRESENT-and- + // undefined whenever the author wrote none, so `toBeUndefined()` passed + // against the defect itself. + const schema = await mountChartView({ ...DATASET_BLOCK, config: OFF_SPEC_CONFIG }); + expect('config' in schema).toBe(false); + }); + + it('DATASET BRANCH: no `config` key even when the author wrote none', async () => { + // The everyday population. Under the old literal this key was present on + // EVERY chart view ever rendered, carrying `undefined`. + const schema = await mountChartView(DATASET_BLOCK); + expect('config' in schema).toBe(false); + }); + + it('DATASET BRANCH POSITIVE CONTROL: the declared keys still arrive', async () => { + // Without this, an absence assertion goes green when nothing renders. It + // also pins that removing the rung removed ONLY the rung. + const schema = await mountChartView({ ...DATASET_BLOCK, config: OFF_SPEC_CONFIG }); + expect(schema.dataset).toBe('task_throughput'); + expect(schema.dimensions).toEqual(['status']); + expect(schema.values).toEqual(['hours']); + expect(schema.chartType).toBe('bar'); + expect(schema.xAxisKey).toBe('status'); + expect(schema.series).toEqual([{ dataKey: 'hours', label: 'hours' }]); + }); + + it('LEGACY BRANCH: an authored `chart.config` does not reach the node', async () => { + // THE DISCRIMINATING ARM for the second branch. The two literals are + // separate code — one arm each, or half the defect stays pinnable. + const schema = await mountChartView({ ...LEGACY_BLOCK, config: OFF_SPEC_CONFIG }); + expect(schema.dataset).toBeUndefined(); // proves the legacy branch ran + expect('config' in schema).toBe(false); + }); + + it('LEGACY BRANCH: no `config` key even when the author wrote none', async () => { + const schema = await mountChartView(LEGACY_BLOCK); + expect(schema.dataset).toBeUndefined(); + expect('config' in schema).toBe(false); + }); + + it('LEGACY BRANCH POSITIVE CONTROL: the translated keys still arrive', async () => { + const schema = await mountChartView({ ...LEGACY_BLOCK, config: OFF_SPEC_CONFIG }); + expect(schema.objectName).toBe(OBJECT_NAME); + expect(schema.chartType).toBe('line'); + expect(schema.aggregate).toEqual({ field: 'hours', function: 'sum', groupBy: 'status' }); + expect(schema.xAxisKey).toBe('status'); + expect(schema.series).toEqual([{ dataKey: 'hours', label: 'hours' }]); + }); +}); diff --git a/packages/app-shell/src/views/ObjectView.tsx b/packages/app-shell/src/views/ObjectView.tsx index 6070f3429..ef1b7e366 100644 --- a/packages/app-shell/src/views/ObjectView.tsx +++ b/packages/app-shell/src/views/ObjectView.tsx @@ -2095,6 +2095,33 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an } } + /** + * objectui#7891 — this branch forwards the AUTHORED `chart:` block, and + * `config` is NOT part of it. Do not add a `config:` rung back. + * + * `@objectstack/spec`'s `ListChartConfigSchema` is a `strictObject` + * declaring exactly `chartType` / `dataset` / `dimensions` / `values`, + * and objectui binds it BY REFERENCE (`chart` is absent from + * `LIST_VIEW_LOCAL_OVERRIDES`), so a `config` written on this block is + * refused BY NAME — `unrecognized_keys ["config"]` — by the same schema + * the platform's metadata write door parses every save through + * (`saveMetaItem` -> `getMetadataTypeSchema('view')` -> 422 + * INVALID_METADATA, draft and publish alike). No conforming author can + * write the key, and no write path can store it. + * + * Why nothing ever caught the forward: `ObjectChart` is published as + * `(props: any)`, so this literal is checked against nothing whatever. + * The `as any` casts that used to sit on both literals were measured + * INERT — `tsc --noEmit` is green without them — so they are gone too; + * they read as load-bearing and were not. + * + * Losing the key is a DEGRADE, not a break: `ChartRenderer` generates a + * container config from `series` plus a positional palette when none is + * present, so any non-conforming row still renders, with series-derived + * labels and default colours. + * + * Pinned by `ObjectView.chartConfigForward-7891.test.tsx`. + */ if (viewDef.type === 'chart') { const chartConfig = viewDef.chart || {}; // ADR-0021 (#1890): dataset-bound chart — the single author-facing @@ -2115,9 +2142,9 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an chartType: chartConfig.chartType || 'bar', xAxisKey: dims[0], series: vals.map((v: string) => ({ dataKey: v, label: v })), - config: chartConfig.config, + // no `config` rung — see the block above (#7891) className: 'h-[400px] w-full', - } as any} + }} /> ); @@ -2149,10 +2176,10 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an }, xAxisKey: categoryField, series, - config: chartConfig.config, + // no `config` rung — see the block above (#7891) filter: chartConfig.filter, className: 'h-[400px] w-full', - } as any} + }} /> );