Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions .changeset/9874-navigation-view-retired-consumer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
---
'@object-ui/react': minor
---

`useNavigationOverlay` stops reading the retired `navigation.view` key, and stops substituting an authored name for the navigation-MODE token

`view.list.navigation.view` was removed in `@objectstack/spec` 17.5.0 under ADR-0049
(enforce-or-remove). `useNavigationOverlay` was its one shipped consumer, and the spec's
tombstone describes that hook by name: the authored value "was passed straight into the
navigation-MODE argument of the console's `onNavigate`, where anything other than `edit`
or `view` matched no branch, so the key selected nothing and could silently deaden the
row click."

The second argument of `onNavigate` is the navigation MODE token, not a view name. The
hook read `const view = navigation?.view` and spent it at two call sites as
`view ?? 'view'` — the no-config early return and the `page` branch — so an authored
`{ mode: 'page', view: 'summary_view' }` dispatched `'summary_view'` into a slot hosts
read against a closed `edit`/`view` vocabulary. An authored name did not SELECT a view,
it SUBSTITUTED for the mode.

Both call sites now pass the literal `'view'`, which is what every config WITHOUT the key
already dispatched. No replacement route is wired into this hook, and no fallback belongs
here: per the same tombstone, choosing what opens for a record is page assignment — assign
a `record` page to the object and let `isDefault` pick the one that opens — while a list
view's `navigation` block only decides HOW the detail is surfaced (`mode`, `size`).

**Breaking for anyone who authored the key, and for anyone reading the hook's `view`
member.** `NavigationOverlayState` no longer publishes `view`. It could only ever carry
the retired key, so keeping it would publish a field that is permanently `undefined` —
the same "declared, consumed, and wrong" state the retirement exists to end. Measured
before removing it: zero readers of that member anywhere in this repository, against a
lit control on its sibling members in the same command. Hosts outside this repository
were not measured. Per this repo's version policy a breaking change ships as `minor`; the
semantics are stated here rather than in the bump.

Behaviour on the Console is unchanged, and that is the point rather than a caveat:
`ObjectView`'s `onNavigate` already routed every non-`new_window` action to the same
record-detail route, so the substitution was inaudible exactly where it was loudest in
the metadata. Its comment claimed a custom name was "resolved by RecordDetailView from
its own config"; no layer ever resolved a view by that name, and the comment is corrected
with the read it described.

Two existing pins asserted the substitution as intended behaviour and are INVERTED rather
than deleted — `useNavigationOverlay.modeDefault` and `gridNavigationMembers-8071`, both
keeping their authored `view` fixture because it is the one input the old and new
implementations disagree about.
17 changes: 13 additions & 4 deletions packages/app-shell/src/views/ObjectView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2170,10 +2170,19 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an
return;
}
// Default: navigate to record detail page.
// `action` may be 'view' / 'page' / undefined, OR a custom view name
// forwarded from `navigation.view` (e.g. 'detail_form'). The view
// variant is resolved by RecordDetailView from its own config, so
// any non-`new_window` action lands on the record detail route.
// `action` is a navigation-MODE token — 'view' / 'page' / undefined
// — and every non-`new_window` value lands on the record detail
// route.
//
// ⛔ It is NOT a view name, and this comment used to say a custom
// one could arrive here "forwarded from `navigation.view` (e.g.
// 'detail_form')", resolved "by RecordDetailView from its own
// config". No layer ever resolved a view by that name — which is
// why `@objectstack/spec` 17.5.0 retired the key under ADR-0049,
// and why `useNavigationOverlay` stopped forwarding it
// (objectui#9874). Nothing here changes behaviour: this branch
// already treated the name and the token identically, which is
// exactly what made the substitution inaudible on this surface.
const originState = {
from: {
pathname: location.pathname + (location.search || ''),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,23 @@
*
* ⛔ What a declaration can never publish:
*
* - **`view` is not a route — it is the second ARGUMENT `onNavigate`
* receives**, and the literal `'view'` is what stands in when the member is
* absent. The two are one character apart in the source and mean entirely
* different things.
* - **`openNewTab` OUTRANKS `mode` and DISCARDS `view` while doing it.** An
* authored `{ mode: 'page', view: 'summary', openNewTab: true }` dispatches
* `'new_window'`, not `'summary'`: the member the author wrote to choose a
* destination is dropped by the member they wrote to choose a window.
* - **`view` was not a route — it landed in the second ARGUMENT `onNavigate`
* receives**, which carries the navigation MODE token, and the literal
* `'view'` is what stands there. The two are one character apart in the
* source and mean entirely different things. ⭐ **objectui#9874 REVERSED
* the row that pinned this**: `@objectstack/spec` 17.5.0 retired
* `view.list.navigation.view` under ADR-0049 precisely because an authored
* name SUBSTITUTED for the mode and matched no branch, so the member no
* longer reaches `onNavigate` at all and every config dispatches `'view'`.
* The row below is kept, inverted, rather than deleted: the authored member
* is the input the two implementations disagree about, so it is the only
* input that can witness the change.
* - **`openNewTab` OUTRANKS `mode`.** An authored
* `{ mode: 'page', openNewTab: true }` dispatches `'new_window'`. Before
* objectui#9874 this row also read as "and DISCARDS `view` while doing it",
* because a sibling `view: 'summary'` was dropped here while riding through
* the `page` branch; now nothing rides through either branch and the
* precedence is the whole of what this row pins.
* - **`preventNavigation` OUTRANKS every mode, including the overlay ones.**
* `{ mode: 'drawer', preventNavigation: true }` draws no drawer and throws
* nothing — a grid that looks clickable and is not.
Expand Down Expand Up @@ -146,18 +155,27 @@ describe('object-grid `navigation` members decide the row click (objectui#8071)'
expect(onNavigate).toHaveBeenCalledWith('7', 'view');
});

it('`mode` defaults to `page`, and `view` is the action it dispatches', async () => {
it('`mode` defaults to `page`, and a RETIRED `view` member does not become the action (objectui#9874)', async () => {
// No `mode` member at all — legal authored metadata, because the spec
// defaults it. `view` rides through as the second argument.
// defaults it. This row used to assert `('7', 'summary_view')` under the
// comment "`view` rides through as the second argument"; it did, into the
// slot that carries the navigation MODE token, where a host reading it
// against `edit`/`view` matched no branch and the row click went quiet.
// `@objectstack/spec` 17.5.0 retired the key for that (ADR-0049), so the
// authored name is now inert here and the dispatched action is the literal.
const { onNavigate } = await clickRow({ view: 'summary_view' });
await waitFor(() => expect(onNavigate).toHaveBeenCalledTimes(1));
expect(onNavigate).toHaveBeenCalledWith('7', 'summary_view');
expect(onNavigate).toHaveBeenCalledWith('7', 'view');
expect(onNavigate).not.toHaveBeenCalledWith('7', 'summary_view');
});

it('an omitted `view` falls back to the literal `view`, not to undefined', async () => {
// The pair for the row above: same mode, member removed. A renderer passing
// `navigation.view` straight through would send `undefined` here and the
// host would open its default form — which looks identical until it is not.
it('an omitted `view` dispatches the literal `view`, not undefined', async () => {
// The pair for the row above: same mode, member removed. Since objectui#9874
// the two rows agree by construction — which is the POINT of the retirement
// and not a reason to drop either. ⛔ Keeping both is what makes "the member
// is inert" a measurement rather than an assertion about one input: a
// renderer that resumed forwarding the member would turn the row above red
// and leave this one green.
const { onNavigate } = await clickRow({ mode: 'page' });
await waitFor(() => expect(onNavigate).toHaveBeenCalledTimes(1));
expect(onNavigate).toHaveBeenCalledWith('7', 'view');
Expand Down Expand Up @@ -186,10 +204,11 @@ describe('object-grid `navigation` members decide the row click (objectui#8071)'
await waitFor(() => expect(document.querySelector('[role="dialog"]')).not.toBeNull());
});

it('`openNewTab` OUTRANKS `mode: "page"` and DISCARDS `view`', async () => {
// The author wrote a destination and a window preference. Only the window
// preference survives: the dispatched action is `new_window`, and
// `summary_view` is nowhere in the call.
it('`openNewTab` OUTRANKS `mode: "page"`, with a retired `view` still inert', async () => {
// The window preference decides: the dispatched action is `new_window`, and
// `summary_view` is nowhere in the call. Before objectui#9874 this row was
// the ONLY place the authored member was dropped; it is now dropped on
// every path, and the row survives as the precedence pin it also always was.
const { onNavigate } = await clickRow({
mode: 'page',
view: 'summary_view',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@
* makes a missing `mode` mean something other than `'page'`, this suite goes
* red and the parity file stays green — which is exactly the split of duties
* the two files are for.
*
* ⚠️ objectui#9874 rewrote one row here and added one. `AS_ALIAS` authors
* `view: 'summary_view'`, which used to ride through into `onNavigate`'s
* navigation-MODE argument; `@objectstack/spec` 17.5.0 retired that key under
* ADR-0049 and the read is gone, so the row now pins that the authored name
* does NOT reach the mode slot. ⛔ The fixture keeps its `view` member on
* purpose — it is the one input the old and new implementations disagree
* about, and it is also still what objectui#4550 reported the alias rejecting.
*/

import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
Expand Down Expand Up @@ -96,7 +104,7 @@ describe('useNavigationOverlay: a config without `mode` defaults to `page` (obje
expect(result.current.isOpen).toBe(false);
});

it('routes a click through the `page` branch, carrying the declared view', () => {
it('routes a click through the `page` branch WITHOUT the authored `view` reaching the mode slot (objectui#9874)', () => {
const onNavigate = vi.fn();
const { result } = renderHook(() =>
useNavigationOverlay({ navigation: AS_ALIAS, objectName: 'contacts', onNavigate }),
Expand All @@ -106,11 +114,35 @@ describe('useNavigationOverlay: a config without `mode` defaults to `page` (obje
result.current.handleClick(RECORD);
});

// `view ?? 'view'` — the authored `view` survives the default-mode path.
expect(onNavigate).toHaveBeenCalledWith('r1', 'summary_view');
// ⭐ THE DISAGREEMENT ROW. `AS_ALIAS` authors `view: 'summary_view'`, and
// this line used to read `toHaveBeenCalledWith('r1', 'summary_view')` under
// the comment "`view ?? 'view'` — the authored `view` survives the
// default-mode path". It did survive, into the wrong slot: the second
// argument is the navigation MODE token, so the authored name SUBSTITUTED
// for the mode and matched no branch in a host reading it against
// `edit`/`view` — the silence `@objectstack/spec` 17.5.0 retired the key to
// end (ADR-0049). The old implementation and this one disagree on exactly
// this input, which is why the fixture keeps its `view` member.
expect(onNavigate).toHaveBeenCalledWith('r1', 'view');
expect(onNavigate).not.toHaveBeenCalledWith('r1', 'summary_view');
expect(result.current.isOpen).toBe(false);
});

it('publishes no `view` member on its state — the key it mirrored is retired (objectui#9874)', () => {
const { result } = renderHook(() =>
useNavigationOverlay({ navigation: AS_ALIAS, objectName: 'contacts' }),
);

// Asserted on the KEY, not on the value: `toBeUndefined()` passes just as
// happily against a member that is present and empty, which is the state
// this card removed. The lit control is the sibling members in the same
// expectation — without them an empty `state` object would pass row one.
expect(Object.keys(result.current)).not.toContain('view');
expect(Object.keys(result.current)).toEqual(
expect.arrayContaining(['mode', 'isOverlay', 'width', 'handleClick']),
);
});

it('falls back to the `view` action when the config declares neither key', () => {
const onNavigate = vi.fn();
// A present-but-empty config is a different input from NO config: it takes
Expand Down
50 changes: 40 additions & 10 deletions packages/react/src/hooks/useNavigationOverlay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,38 @@
*
* Used by plugin-grid, plugin-list, plugin-detail and any component that needs
* NavigationConfig support.
*
* ## The RETIRED `view` key (objectui#9874)
*
* `view.list.navigation.view` was removed in `@objectstack/spec` 17.5.0 under
* ADR-0049 (enforce-or-remove). This hook was its one shipped consumer, and the
* spec's own tombstone describes THIS file: the authored value "was passed
* straight into the navigation-MODE argument of the console's `onNavigate`,
* where anything other than `edit` or `view` matched no branch, so the key
* selected nothing and could silently deaden the row click."
*
* ⭐ The point that is one character wide in the source and total in meaning:
* `onNavigate`'s second argument is the navigation MODE token, not a view name.
* An authored `view` did not SELECT a view, it SUBSTITUTED for the mode — so
* `{ mode: 'page', view: 'summary_view' }` dispatched `'summary_view'` into the
* slot a host reads against its closed `edit`/`view` vocabulary, matching
* neither branch. Two reads (`view ?? 'view'`, in the no-config and the `page`
* branch) are now the literal `'view'`, which is what every config without the
* key already dispatched and what every config dispatches now.
*
* The replacement route is NOT in this hook and no fallback belongs here. Per
* the same tombstone: to choose what opens for a record, assign a `record` page
* to the object and let `isDefault` pick the one that opens — page assignment
* is the machinery that resolves a detail layout, and a list view's
* `navigation` block only decides HOW the detail is surfaced (`mode`, `size`).
*
* ⚠️ `NavigationOverlayState` lost its `view` member with the read. It could
* only ever carry the retired key, so keeping it would publish a field that is
* permanently `undefined` — the same "declared, consumed, and wrong" state the
* retirement exists to end. Measured before removing it: zero readers of that
* member in this repository (`(navOverlay|nav|state).view` → no lines, against
* a lit control on its sibling members in the same command). Hosts outside this
* repository were NOT measured.
*/

import { useState, useCallback, useMemo } from 'react';
Expand All @@ -37,8 +69,10 @@ import type { SpecAuthoredInput } from '../spec-input.js';
* `mode` is OPTIONAL, because the spec says so and this hook agrees.
* `packages/spec/src/ui/view.zod.ts` declares
* `mode: NavigationModeSchema.default('page')`, and a `.default()` lands on the
* AUTHORING side as `| undefined` — so `navigation: { view: 'summary_view' }`
* is legal authored metadata that lets the mode default.
* AUTHORING side as `| undefined` — so `navigation: { size: 'lg' }` is legal
* authored metadata that lets the mode default. ⛔ This sentence used to make
* the same point with a `view` member; see "The RETIRED `view` key" below for
* why no example in this file may spell that key again.
*
* Until objectui#4550 this alias `Omit`ted `mode` and re-added it as
* `NonNullable<…>`, on the stated reasoning that "this hook dispatches on
Expand Down Expand Up @@ -208,8 +242,6 @@ export interface NavigationOverlayState {
width: string | number | undefined;
/** Whether navigation is an overlay mode (drawer/modal/split/popover) */
isOverlay: boolean;
/** The target view/form name from NavigationConfig */
view: string | undefined;
}

/**
Expand Down Expand Up @@ -272,7 +304,6 @@ export function useNavigationOverlay(

const mode: NavigationMode = navigation?.mode ?? 'page';
const width = resolveOverlayWidth(navigation);
const view = navigation?.view;
const isOverlay = mode === 'drawer' || mode === 'modal' || mode === 'split' || mode === 'popover';

const close = useCallback(() => {
Expand Down Expand Up @@ -320,7 +351,7 @@ export function useNavigationOverlay(
if (!navigation) {
const recordId = record.id || record._id;
if (onNavigate && recordId != null) {
onNavigate(recordId as string | number, view ?? 'view');
onNavigate(recordId as string | number, 'view');
}
return;
}
Expand Down Expand Up @@ -353,7 +384,7 @@ export function useNavigationOverlay(
if (mode === 'page') {
const recordId = record.id || record._id;
if (onNavigate && recordId != null) {
onNavigate(recordId as string | number, view ?? 'view');
onNavigate(recordId as string | number, 'view');
}
return;
}
Expand All @@ -365,7 +396,7 @@ export function useNavigationOverlay(
return;
}
},
[onRowClick, navigation, mode, objectName, onNavigate, isOverlay, view]
[onRowClick, navigation, mode, objectName, onNavigate, isOverlay]
);

return useMemo(
Expand All @@ -378,9 +409,8 @@ export function useNavigationOverlay(
setIsOpen,
handleClick,
width,
view,
isOverlay,
}),
[isOpen, selectedRecord, mode, close, open, handleClick, width, view, isOverlay]
[isOpen, selectedRecord, mode, close, open, handleClick, width, isOverlay]
);
}
8 changes: 8 additions & 0 deletions scripts/check-installed-spec-pin-claims.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -750,6 +750,14 @@ export const LEDGER = [
class: "historical",
why: "\"`publicPicker` arrives in @objectstack/spec 17.0.0 GA\" \u2014 the arrival release. \u26a0\ufe0f The stale claim in the SAME docblock (\"this repo is pinned to `^17.0.0-rc.6`\") is a RANGE and therefore outside this gate's predicate by construction; it is recorded in objectui#8924 rather than silently covered here.",
},
{
file: "packages/plugin-grid/src/__tests__/gridNavigationMembers-8071.test.tsx",
package: "@objectstack/spec",
version: "17.5.0",
sites: 1,
class: "historical",
why: "\"`@objectstack/spec` 17.5.0 retired `view.list.navigation.view` under ADR-0049\" names the RELEASE that removed the key, not what is installed -- the same shape as the `offline-nav-performance-spec-parity` entry below (\"retired that name in 17.0.0-rc.1\"). The marker that puts it in the population is the word `pinned` in \"the row that pinned this\", where it means a TEST ROW pinning a behaviour rather than a version pin; the `pinned-recording-sense` cue does not reach that phrasing. This version is AHEAD of the resolved pin rather than behind it, which is why restamping is not merely unnecessary but false: 17.4.0 is exactly the version where the key still EXISTS, so stamping the sentence at the pin would assert that 17.4.0 retired it. That is the fresh false premise this gate\u0027s docblock warns restamping plants (objectui#9874).",
},
{
file: "packages/react/src/hooks/__tests__/offline-nav-performance-spec-parity.test.ts",
package: "@objectstack/spec",
Expand Down
Loading