diff --git a/.changeset/7638-navigation-url-follows-record-source.md b/.changeset/7638-navigation-url-follows-record-source.md new file mode 100644 index 0000000000..ad2d5dc14d --- /dev/null +++ b/.changeset/7638-navigation-url-follows-record-source.md @@ -0,0 +1,48 @@ +--- +'@object-ui/plugin-calendar': patch +'@object-ui/plugin-tree': patch +'@object-ui/react': patch +--- + +A record-page URL now names the object the clicked rows actually came from, in +`ObjectTree` and `ObjectCalendar` (objectui#7638). + +`useNavigationOverlay` builds `/{objectName}/record/{id}` out of whatever it is handed, +and both components handed it the bare top-level `schema.objectName` while resolving +their own rows through the objectui#6939 record-source ladder (`data`, then +`staticData`, then `objectName`). objectui#6939 published `objectName` as that ladder's +THIRD RUNG and not as a parallel "page object" concept, so a block has exactly one +record source — and a row fetched through `data.object` whose click built +`/{schema.objectName}/record/{id}` named a record that the URL's own object does not +contain. + +Two shapes change, both toward the object the rows came from: + +- a block carrying **both** bindings navigated to the top-level key and now navigates to + `data.object`; +- a **data-only** block had no name to build a URL from at all, so the hook took its + `/{id}` leg — an unrouted path that paints a blank page — and now builds the routed + record URL. + +`ObjectCalendar` is where the divergence was plainest: on one click it resolved the +detail drawer through the ladder and the navigation URL through the top-level key. The +URL now reuses the very `schemaObjectName` that already keys the calendar's record query +and its `$expand` derivation, so query, drawer and URL agree by construction. + +**Nothing else moves.** Both converted sites keep a site-local `?? schema.objectName` +tail for the off-contract `data: { provider: 'object' }` that carries no `object` +(`ViewDataSchema` declares it required) — the same tail `ObjectTree`'s `headerObjectName` +already carries, and the same conservatism objectui#7627 applied when it published the +shared reader. `useNavigationOverlay`'s own signature is unchanged: it still takes an +`objectName`, and only what callers hand it has changed. + +The hook's `@example` stops prescribing `objectName: schema.objectName`. That prose is +why there were copies to convert at all — component authors copied the divergence out of +the documentation, correctly, as written — so it now points at +`resolveRecordSourceObjectName` and says explicitly that a caller with no data config +has nothing above rung three and should keep passing `schema.objectName`. + +`ObjectKanban` is deliberately **not** converted: it has no data config, no +`getDataConfig`, and its `data` is a raw row array rather than a `ViewData` binding, so +`schema.objectName` already IS its record source and its board, drawer and URL already +agree. diff --git a/packages/plugin-calendar/src/ObjectCalendar.tsx b/packages/plugin-calendar/src/ObjectCalendar.tsx index e776037371..2db6268f89 100644 --- a/packages/plugin-calendar/src/ObjectCalendar.tsx +++ b/packages/plugin-calendar/src/ObjectCalendar.tsx @@ -622,7 +622,21 @@ export const ObjectCalendar: React.FC = ({ const navIsOverlay = navConfig.mode === 'drawer' || navConfig.mode === 'modal' || navConfig.mode === 'split' || navConfig.mode === 'popover'; const navigation = useNavigationOverlay({ navigation: navConfig, - objectName: schema.objectName, + // The record-page URL follows the RECORD SOURCE (objectui#7638): the very + // `schemaObjectName` resolved above, which already keys this calendar's + // record query and which the detail drawer at the bottom of this file + // resolves the same way. Before this it read the bare `schema.objectName`, + // so ONE click resolved the drawer through the objectui#6939 ladder and the + // navigation URL through the top-level key — two receivers, one gesture, + // two different objects. + // + // The `?? schema.objectName` tail is NOT the shared rung repeated: it is + // this site's own coercion of the OFF-CONTRACT `data: { provider: 'object' }` + // that carries no `object` (`ViewDataSchema` declares it required), and it + // is here so this conversion changes nothing this component navigates to + // today EXCEPT the divergence it closes. `ObjectTree`'s converted site and + // `headerObjectName` both keep the same tail for the same reason. + objectName: schemaObjectName ?? schema.objectName, onRowClick: navIsOverlay ? undefined : onRowClick, }); diff --git a/packages/plugin-calendar/src/__tests__/ObjectCalendar.navigationRecordSource-7638.test.tsx b/packages/plugin-calendar/src/__tests__/ObjectCalendar.navigationRecordSource-7638.test.tsx new file mode 100644 index 0000000000..0d238c8f92 --- /dev/null +++ b/packages/plugin-calendar/src/__tests__/ObjectCalendar.navigationRecordSource-7638.test.tsx @@ -0,0 +1,141 @@ +/** + * 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#7638 — `ObjectCalendar`'s record-page URL follows the RECORD SOURCE. + * + * ## The divergence this closes, which THIS component proves was accidental + * + * On a single click this calendar used to resolve two receivers two different + * ways: the detail drawer's `objectName` through the objectui#6939 + * record-source ladder, and the navigation URL through the bare top-level + * `schema.objectName`. Two receivers, one gesture, two different objects — + * which is the evidence that the divergence was a copy, not a design. + * + * The same `schemaObjectName` that keys this calendar's record query and its + * `$expand` derivation now also builds the URL, so all three agree by + * construction rather than by coincidence. + * + * ## What is asserted, and why not a spy on the resolver + * + * The observable is the URL a click actually navigates to, so every case drives + * a real event click through the real `new_window` branch and reads + * `window.open`'s first argument. Asserting `resolveRecordSourceObjectName` was + * CALLED would pass equally well with its result discarded. + * + * `new_window` is the mode under test because it is the branch that builds the + * URL in-process — the overlay modes open the drawer and build none, and the + * `page` branch delegates to an `onNavigate` this call site never passes. + * + * ## The lit control + * + * The first case carries no `data` block, so rung three IS its record source + * and `/appointments/record/e1` is both the old answer and the new one. It is + * the instrument check and it must read NON-ZERO: a `window.open` that never + * fires would make every "did not navigate to the decoy" assertion below + * vacuously true, and this file would be a dark instrument reporting green. + */ + +import React from 'react'; +import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest'; +import { render, screen, cleanup, fireEvent, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { ObjectCalendar } from '../ObjectCalendar'; + +/** The object the rows really came from — what the URL must name. */ +const RECORD_SOURCE = 'clinic_visit'; +/** The top-level key, rung three — the decoy the URL must stop naming. */ +const DECOY = 'appointments'; + +/** + * One event on TODAY, so it lands in the default month view without the test + * having to drive the calendar's navigation controls first. + */ +const TODAY = new Date(); +const ROWS = [{ id: 'e1', name: 'Follow-up', starts_at: TODAY.toISOString() }]; + +function renderCalendar(schema: Record) { + return render( + , + ); +} + +/** Click the event and hand back the URL `window.open` was given. */ +async function clickEventAndReadUrl( + open: ReturnType, +): Promise { + const event = await screen.findByText('Follow-up'); + fireEvent.click(event); + await waitFor(() => expect(open).toHaveBeenCalled()); + return open.mock.calls[0]?.[0] as string | undefined; +} + +let open: ReturnType; + +beforeEach(() => { + open = vi.spyOn(window, 'open').mockImplementation(() => null); +}); + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +describe('ObjectCalendar navigation URL follows the record source (objectui#7638)', () => { + it('LIT CONTROL: with no `data` block, rung three IS the record source and still builds the URL', async () => { + renderCalendar({ objectName: DECOY }); + + // Reads non-zero, or every assertion below is vacuous. + expect(await clickEventAndReadUrl(open)).toBe(`/${DECOY}/record/e1`); + expect(open).toHaveBeenCalledWith(`/${DECOY}/record/e1`, '_blank'); + }); + + it('navigates to the object the ROWS came from, not the top-level key', async () => { + renderCalendar({ + objectName: DECOY, + data: { provider: 'object', object: RECORD_SOURCE }, + }); + + const url = await clickEventAndReadUrl(open); + expect(url).toBe(`/${RECORD_SOURCE}/record/e1`); + // The whole finding in one line: before objectui#7638 this was the answer, + // while the drawer on the very same click resolved `clinic_visit`. + expect(url).not.toBe(`/${DECOY}/record/e1`); + }); + + it('builds a routed URL for a data-only block, which previously had no name to use', async () => { + // No top-level `objectName` at all, so `schema.objectName` was `undefined` + // and the hook took its `/${encodedId}` leg — an unrouted path that paints + // a blank page. + renderCalendar({ data: { provider: 'object', object: RECORD_SOURCE } }); + + const url = await clickEventAndReadUrl(open); + expect(url).toBe(`/${RECORD_SOURCE}/record/e1`); + expect(url).not.toBe('/e1'); + }); + + it('keeps the `?? schema.objectName` tail for the OFF-CONTRACT `{ provider: "object" }`', async () => { + // `ViewDataSchema` declares `object` REQUIRED on the `object` provider, so + // this shape is off-contract and the shared reader returns `undefined` for + // it rather than coercing. The site keeps its own tail, so this conversion + // changes nothing this component navigates to today. + renderCalendar({ objectName: DECOY, data: { provider: 'object' } }); + + expect(await clickEventAndReadUrl(open)).toBe(`/${DECOY}/record/e1`); + }); +}); diff --git a/packages/plugin-tree/src/ObjectTree.navigationRecordSource-7638.test.tsx b/packages/plugin-tree/src/ObjectTree.navigationRecordSource-7638.test.tsx new file mode 100644 index 0000000000..b886806ed4 --- /dev/null +++ b/packages/plugin-tree/src/ObjectTree.navigationRecordSource-7638.test.tsx @@ -0,0 +1,153 @@ +/** + * 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#7638 — `ObjectTree`'s record-page URL follows the RECORD SOURCE. + * + * ## The divergence this closes + * + * `useNavigationOverlay` builds `/{objectName}/record/{id}` out of whatever it + * is handed. This component used to hand it the bare top-level + * `schema.objectName` while resolving its own rows — and its column labels + * (`headerObjectName`) — through the objectui#6939 record-source ladder + * (`data`, then `staticData`, then `objectName`). objectui#6939 published + * `objectName` as that ladder's THIRD RUNG and not as a parallel "page object" + * concept, so a block has exactly ONE record source: a row fetched through + * `data.object` whose click built `/{schema.objectName}/record/{id}` named a + * record that the URL's own object does not contain. + * + * ## Why these assertions and not a spy on the resolver + * + * The observable under test is the URL a user's click actually navigates to, so + * every case here drives a real click through the real `new_window` branch and + * reads `window.open`'s first argument. Asserting that + * `resolveRecordSourceObjectName` was CALLED would pass just as well with its + * result thrown away. + * + * `new_window` is the mode chosen because it is the branch that builds the URL + * in-process; the `page` branch delegates to an `onNavigate` this component + * never passes, and the overlay modes never build a URL at all. + * + * ## The lit control + * + * The first case carries NO `data` block, so rung three is the record source + * and `/business_unit/record/1` is both the old and the new answer. It is here + * as the instrument check: it is the case that must read NON-ZERO — a + * `window.open` that is never called at all would make every "did not navigate + * to the decoy" assertion below vacuously true. If that case ever goes silent, + * the rest of this file is a dark instrument and its greens mean nothing. + */ + +import React from 'react'; +import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest'; +import { render, screen, cleanup, fireEvent, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { ObjectTree } from './ObjectTree'; + +/** + * Rows reach the component through the `data` PROP, so no `dataSource` is + * needed and the fetch effect falls through to them for every schema shape + * below. That keeps the only variable across these cases the thing under test: + * which object the schema NAMES. + */ +const ROWS = [ + { id: '1', name: 'Acme', parent_id: null }, + { id: '2', name: 'Engineering', parent_id: '1' }, +]; + +/** The object the rows really came from — what the URL must name. */ +const RECORD_SOURCE = 'org_chart_node'; +/** The top-level key, rung three — the decoy the URL must stop naming. */ +const DECOY = 'business_unit'; + +function renderTree(schema: Record) { + return render( + , + ); +} + +/** Click the first row and hand back the URL `window.open` was given. */ +async function clickRowAndReadUrl(open: ReturnType): Promise { + const cell = await screen.findByText('Acme'); + fireEvent.click(cell); + await waitFor(() => expect(open).toHaveBeenCalled()); + return open.mock.calls[0]?.[0] as string | undefined; +} + +let open: ReturnType; + +beforeEach(() => { + open = vi.spyOn(window, 'open').mockImplementation(() => null); +}); + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +describe('ObjectTree navigation URL follows the record source (objectui#7638)', () => { + it('LIT CONTROL: with no `data` block, rung three IS the record source and still builds the URL', async () => { + renderTree({ objectName: DECOY }); + + // Reads non-zero, or every assertion below is vacuous. + expect(await clickRowAndReadUrl(open)).toBe(`/${DECOY}/record/1`); + expect(open).toHaveBeenCalledWith(`/${DECOY}/record/1`, '_blank'); + }); + + it('navigates to the object the ROWS came from, not the top-level key', async () => { + renderTree({ + objectName: DECOY, + data: { provider: 'object', object: RECORD_SOURCE }, + }); + + const url = await clickRowAndReadUrl(open); + expect(url).toBe(`/${RECORD_SOURCE}/record/1`); + // The whole finding in one line: before objectui#7638 this was the answer. + expect(url).not.toBe(`/${DECOY}/record/1`); + }); + + it('builds a routed URL for a data-only block, which previously had no name to use', async () => { + // No top-level `objectName` at all. `schema.objectName` was `undefined` + // here, so the hook took its `: `/${encodedId}`` leg and produced `/1` — an + // unrouted path that paints a blank page. + renderTree({ data: { provider: 'object', object: RECORD_SOURCE } }); + + const url = await clickRowAndReadUrl(open); + expect(url).toBe(`/${RECORD_SOURCE}/record/1`); + expect(url).not.toBe('/1'); + }); + + it('keeps the `?? schema.objectName` tail for the OFF-CONTRACT `{ provider: "object" }`', async () => { + // `ViewDataSchema` declares `object` REQUIRED on the `object` provider, so + // this shape is off-contract and the shared reader deliberately returns + // `undefined` for it rather than coercing. This site keeps its own tail — + // exactly as `headerObjectName` above it does — so the conversion changes + // nothing this component resolves today except the divergence it closes. + renderTree({ objectName: DECOY, data: { provider: 'object' } }); + + expect(await clickRowAndReadUrl(open)).toBe(`/${DECOY}/record/1`); + }); + + it('falls back to rung three for a `value`-provider block, which names no object', async () => { + renderTree({ objectName: DECOY, data: { provider: 'value', items: ROWS } }); + + expect(await clickRowAndReadUrl(open)).toBe(`/${DECOY}/record/1`); + }); +}); diff --git a/packages/plugin-tree/src/ObjectTree.tsx b/packages/plugin-tree/src/ObjectTree.tsx index 88ea97119b..021f457d83 100644 --- a/packages/plugin-tree/src/ObjectTree.tsx +++ b/packages/plugin-tree/src/ObjectTree.tsx @@ -595,7 +595,20 @@ export const ObjectTree: React.FC = ({ const navigation = useNavigationOverlay({ navigation: (schema as any).navigation, - objectName: schema.objectName, + // The record-page URL names the object the ROWS came from, not the block's + // bare top-level key (objectui#7638). objectui#6939 published `objectName` + // as the THIRD RUNG of ONE record-source ladder (`data`, then `staticData`, + // then `objectName`) rather than as a parallel "page object" concept, so a + // block has exactly one record source. A row fetched through + // `data.object` whose click built `/{schema.objectName}/record/{id}` named + // a record that the URL's own object does not contain. + // + // The `?? schema.objectName` tail is NOT the shared rung repeated: it is + // this site's own coercion of the OFF-CONTRACT `data: { provider: 'object' }` + // that carries no `object` (`ViewDataSchema` declares it required), kept for + // exactly the reason `headerObjectName` above keeps it — so this conversion + // changes nothing this site resolves today EXCEPT the divergence it closes. + objectName: resolveRecordSourceObjectName(schema, dataConfig) ?? schema.objectName, onRowClick, }); diff --git a/packages/react/src/hooks/__tests__/useNavigationOverlay.docExampleRecordSource-7638.test.ts b/packages/react/src/hooks/__tests__/useNavigationOverlay.docExampleRecordSource-7638.test.ts new file mode 100644 index 0000000000..89285aa33c --- /dev/null +++ b/packages/react/src/hooks/__tests__/useNavigationOverlay.docExampleRecordSource-7638.test.ts @@ -0,0 +1,98 @@ +/** + * 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#7638 — the `useNavigationOverlay` doc comment must not re-seed the + * bare `objectName: schema.objectName` call-site spelling. + * + * ## Why a source-text pin, which is not this repo's usual instrument + * + * The three call sites converted by objectui#7638 are pinned behaviourally, in + * their own packages, by the URL a real click produces. Nothing pins the + * PROSE — and the prose is the reason there were three copies to convert. The + * hook's `@example` block prescribed `objectName: schema.objectName`, so every + * component author who reached for this hook copied the divergence out of the + * documentation, correctly, as written. #7627 hit the identical trap. + * + * A behavioural test cannot reach a doc comment: it is erased before anything + * runs. So the guard has to read the source, and that is a deliberate, narrow + * exception rather than a new habit. + * + * ## Why this is not covered by the objectui#7617 gate, despite the card + * + * Both card #7638 and its dispatch ruling state that the doc comment is + * "the exact defect class the gate landed by PR #7617 exists to catch". It is + * NOT, and this file is where that correction is recorded so the next reader + * does not go looking for enforcement that was never there. + * + * PR #7617 modified `scripts/check-spec-symbol-derivation.mjs`, whose rule 4 + * judges CITATIONS OF `@objectstack/spec` at member granularity — a comment + * that names `SelectOptionSchema.description` when the spec's own shape has no + * `description` key. The prose here cites no spec symbol at all; it prescribes + * a LOCAL call-site spelling. That gate reads this file and finds nothing to + * say about this comment, which is the correct outcome for that gate and the + * reason this pin has to exist separately. + * + * ## What it asserts, kept deliberately loose + * + * Only that the example's `objectName:` argument resolves a record source + * rather than reading a bare `schema.objectName`. It does not pin wording, + * ordering, or the surrounding prose — a doc comment that cannot be edited is + * its own defect. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; + +const HOOK_SOURCE = readFileSync( + fileURLToPath(new URL('../useNavigationOverlay.ts', import.meta.url)), + 'utf8', +); + +/** + * The hook's leading JSDoc block — everything up to its `export function`. + * Scoping to it keeps the pin off the implementation below, where + * `const { navigation, objectName, ... } = options` legitimately spells the + * parameter name on its own. + */ +const DOC_BLOCK = HOOK_SOURCE.slice(0, HOOK_SOURCE.indexOf('export function useNavigationOverlay')); + +describe('useNavigationOverlay doc comment prescribes the record source (objectui#7638)', () => { + it('LIT CONTROL: the doc block was actually read and does document this hook', () => { + // Reads non-zero, or every assertion below is a claim about an empty + // string: a moved/renamed source file, or a changed `export function` + // spelling, would otherwise leave this file silently green. + expect(DOC_BLOCK.length).toBeGreaterThan(200); + expect(DOC_BLOCK).toContain('useNavigationOverlay({'); + expect(DOC_BLOCK).toContain('objectName:'); + }); + + it('does not hand the example a bare `schema.objectName`', () => { + expect( + DOC_BLOCK, + 'The @example is what component authors copy. Prescribing the bare ' + + 'top-level key here is what put the same divergence in three ' + + 'components (objectui#7638); the example must resolve the record ' + + 'source instead.', + ).not.toContain('objectName: schema.objectName'); + }); + + it('points the example at the ONE shared record-source reader', () => { + expect(DOC_BLOCK).toContain('resolveRecordSourceObjectName'); + }); + + it('says what a caller with no data config should pass, so rung three is not read as a bug', () => { + // Not every caller has a data config — `ObjectKanban` has none, and its + // `schema.objectName` IS its record source. Without this sentence the + // guidance above reads as "never pass `schema.objectName`", which would be + // wrong and would send the next author looking for a ladder that is not + // there. + expect(DOC_BLOCK).toMatch(/no data config/i); + }); +}); diff --git a/packages/react/src/hooks/useNavigationOverlay.ts b/packages/react/src/hooks/useNavigationOverlay.ts index 43842694a6..18ef67013d 100644 --- a/packages/react/src/hooks/useNavigationOverlay.ts +++ b/packages/react/src/hooks/useNavigationOverlay.ts @@ -190,12 +190,31 @@ export interface NavigationOverlayState { /** * Hook for NavigationConfig-driven navigation overlay. * + * ## What to hand `objectName` + * + * The block's RECORD SOURCE — the object the clicked rows actually came from — + * and never a bare top-level `schema.objectName` read in its place. objectui#6939 + * published `objectName` as the THIRD RUNG of one record-source ladder (`data`, + * then `staticData`, then `objectName`), not as a parallel "page object" + * concept, so a block has exactly ONE record source. `handleClick` below builds + * the record-page URL `/{objectName}/record/{id}` out of whatever it is handed: + * a caller that hands it the top-level key while its rows came from + * `data.object` navigates to a record that the URL's own object does not + * contain (objectui#7638). + * + * A caller that resolves a data config reads that ladder through the ONE shared + * reader — `resolveRecordSourceObjectName` from `@object-ui/core` + * (objectui#7627) — as the example does. A caller with NO data config has + * nothing above rung three, and its `schema.objectName` already IS its record + * source; that spelling is correct there and needs no conversion. + * * @example * ```tsx + * const dataConfig = useMemo(() => getDataConfig(schema), [schema]); * const { handleClick, isOpen, selectedRecord, mode, close, width } = * useNavigationOverlay({ * navigation: schema.navigation, - * objectName: schema.objectName, + * objectName: resolveRecordSourceObjectName(schema, dataConfig), * onNavigate: schema.onNavigate, * onRowClick: props.onRowClick, * });