From 823f5ae426f95169aeaa18590fca0b038f116b70 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 14:07:59 +0000 Subject: [PATCH] =?UTF-8?q?fix(react):=20NavigationConfig.mode=20is=20opti?= =?UTF-8?q?onal=20=E2=80=94=20the=20type=20says=20what=20the=20hook=20does?= =?UTF-8?q?=20(#4550)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@object-ui/react` published a `NavigationConfig` that required `mode`, in front of a `useNavigationOverlay` that has always defaulted it. The alias `Omit`ted `mode` from the spec's authored config and re-added it as `NonNullable< … >`; ~140 lines below, the hook read `navigation?.mode ?? 'page'`. The type was strictly stricter than the implementation it fronts, and 'page' is meaningful behaviour, not a placeholder. The spec never asked for that: `NavigationConfigSchema` declares `mode: NavigationModeSchema.default('page')` (packages/spec/src/ui/view.zod.ts), and a `.default()` lands on the authoring side as `| undefined`. `@object-ui/types` already re-exported the spec's own `NavigationConfig` unchanged — so one monorepo shipped two published types of the same name that disagreed about whether `mode` could be omitted. The alias is now the spec's authored config verbatim, with no divergence of its own. `ListView` carried `schema.navigation as NavigationConfig | undefined` purely to get a valid spec-shaped value past the old declaration; that assertion is deleted rather than replaced. Nothing changes at runtime: `navigation?.mode ?? 'page'` is untouched. The default is now pinned as observable behaviour alongside every explicit mode, the `none` / `preventNavigation` short-circuits, the `onRowClick` priority and the Cmd/Ctrl/middle-click and `new_window` branches. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .../navigation-config-mode-optional-4550.md | 22 ++ packages/plugin-list/src/ListView.tsx | 23 +- ...ffline-nav-performance-spec-parity.test.ts | 61 +++- .../useNavigationOverlay.modeDefault.test.tsx | 327 ++++++++++++++++++ .../react/src/hooks/useNavigationOverlay.ts | 57 ++- 5 files changed, 450 insertions(+), 40 deletions(-) create mode 100644 .changeset/navigation-config-mode-optional-4550.md create mode 100644 packages/react/src/hooks/__tests__/useNavigationOverlay.modeDefault.test.tsx diff --git a/.changeset/navigation-config-mode-optional-4550.md b/.changeset/navigation-config-mode-optional-4550.md new file mode 100644 index 000000000..8c022606b --- /dev/null +++ b/.changeset/navigation-config-mode-optional-4550.md @@ -0,0 +1,22 @@ +--- +'@object-ui/react': minor +'@object-ui/plugin-list': patch +--- + +`NavigationConfig.mode` is optional — the type now says what the hook does + +`@object-ui/react` published a `NavigationConfig` that required `mode`, in front of a `useNavigationOverlay` that has always defaulted it. The declaration took the spec's authored config, `Omit`ted `mode`, and re-added it as `NonNullable< … >`; 140 lines below, the hook read `navigation?.mode ?? 'page'`. The type was strictly stricter than the implementation it fronted, and `'page'` is meaningful behaviour rather than a placeholder. + +The spec never asked for that. `NavigationConfigSchema` 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. `@object-ui/types` already re-exported the spec's own `NavigationConfig` unchanged, which meant one monorepo shipped two published types of the same name that disagreed about whether `mode` could be omitted. + +The alias is now the spec's authored config verbatim, with no divergence of its own: + +```ts +export type NavigationConfig = SpecAuthoredInput< typeof NavigationConfigSchema >; +``` + +The cost of the old spelling was paid by callers. `ListView` carried `schema.navigation as NavigationConfig | undefined` for no reason except to get a valid spec-shaped value past the declaration; that assertion is deleted here, not replaced. A type in front of an implementation must not be stricter than the implementation — when it is, every caller pays in casts, and a cast is exactly the renderer-side workaround that belongs back at the producer. + +**Nothing changes at runtime.** `navigation?.mode ?? 'page'` is untouched, and the default is now pinned as observable behaviour (`useNavigationOverlay.modeDefault.test.tsx`) rather than only as a comment — the explicit modes, the `preventNavigation` and `none` short-circuits, the `onRowClick` priority, and the Cmd/Ctrl/middle-click and `new_window` branches are all pinned alongside it. + +**Why minor rather than patch**, from the measured `.d.ts`. Optional-izing a property is looser for writers and narrower for readers, so the grade turns on which role the published surface actually plays. In this package `NavigationConfig` occurs only in input positions — `useNavigationOverlay`'s `navigation?:` option and `resolveOverlayWidth`'s parameter — and never in a return type; the package consumes these values and never hands one back. For consumers the change is therefore purely permissive: every call that compiled before still compiles, and spec-shaped configs that previously needed an assertion now compile without one. That gained input shape is a real capability rather than an internal repair, which is more than a patch describes. The reader-side narrowing is real but secondary: code that imports the bare type, annotates its own value with it and reads `.mode` now sees `NavigationMode | undefined`. The in-repo census found exactly one such importer — `ListView` — and it imported the type only to write the assertion this change removes. diff --git a/packages/plugin-list/src/ListView.tsx b/packages/plugin-list/src/ListView.tsx index 2f0c718e8..97bbd0b98 100644 --- a/packages/plugin-list/src/ListView.tsx +++ b/packages/plugin-list/src/ListView.tsx @@ -15,7 +15,6 @@ import { ViewSwitcherDropdown, ViewType } from './ViewSwitcher'; import { ViewSettingsPopover } from './components/ViewSettingsPopover'; import { UserFilters } from './UserFilters'; import { SchemaRenderer, useNavigationOverlay } from '@object-ui/react'; -import type { NavigationConfig } from '@object-ui/react'; import { useDensityMode } from '@object-ui/react'; import type { ListViewSchema } from '@object-ui/types'; import { detectStatusField } from '@object-ui/types'; @@ -1727,15 +1726,21 @@ export const ListView = React.forwardRef(({ }, [onSearchChange]); // --- NavigationConfig support --- - // The assertion bridges two spellings of ONE spec object and changes no - // value: `@object-ui/react`'s `NavigationConfig` alias re-declares `mode` as - // NON-optional, while the spec-derived `ListViewSchema['navigation']` leaves - // it optional. The hook's own body defaults it (`navigation?.mode ?? 'page'`), - // so a spec-shaped value is valid input and only the alias is tighter than - // its implementation. Surfaced by objectui#4528: this call used to type-check - // for the wrong reason, because the erased props type made `schema` `any`. + // No assertion, deliberately. `schema.navigation` is the spec-derived + // `ListViewSchema['navigation']` and the hook's `NavigationConfig` is now the + // spec's authored config verbatim — `mode` optional and all (objectui#4550). + // Two spellings of one spec object, so they simply agree. + // + // This call carried `as NavigationConfig | undefined` from objectui#4528 + // until then. That cast bridged nothing real: the alias re-declared `mode` as + // required while the hook it fronts defaults it (`navigation?.mode ?? 'page'`), + // so the cast's only job was to get a valid value past an over-tight type. + // objectui#4550 fixed that at the producer, which deleted the reason for the + // cast — and a cast kept past its reason is how the next reader learns the + // wrong thing about the contract. Neither the cast nor its removal touches + // the runtime value. const navigation = useNavigationOverlay({ - navigation: schema.navigation as NavigationConfig | undefined, + navigation: schema.navigation, objectName: schema.objectName, onNavigate: schema.onNavigate, onRowClick, diff --git a/packages/react/src/hooks/__tests__/offline-nav-performance-spec-parity.test.ts b/packages/react/src/hooks/__tests__/offline-nav-performance-spec-parity.test.ts index 243e8c76d..01adf6ff1 100644 --- a/packages/react/src/hooks/__tests__/offline-nav-performance-spec-parity.test.ts +++ b/packages/react/src/hooks/__tests__/offline-nav-performance-spec-parity.test.ts @@ -233,7 +233,7 @@ describe('OfflineCacheConfig keeps its defaulted keys authorable', () => { }); }); -describe('NavigationConfig derives from the spec, requiring only `mode`', () => { +describe('NavigationConfig IS the spec\'s authored navigation config', () => { it('is pinned at compile time', () => { type SpecNavigationInput = SpecAuthoredInput; type _SpecNotAny = Assert, false>>; @@ -247,14 +247,35 @@ describe('NavigationConfig derives from the spec, requiring only `mode`', () => type _NoLocalOnlyKeys = Assert, never>>; type _NoMissingKeys = Assert, never>>; - // `mode` is the ONE narrowing, and it is required here. If the spec ever - // makes `mode` required itself, `_StillNarrowed` fails and this alias - // should collapse to `SpecAuthoredInput`. - type _ModeIsRequired = Assert>; - type _StillNarrowed = Assert, false>>; - type _EverythingElseMatches = Assert< - Extends, Omit> - >; + // objectui#4550 — the narrowing is GONE, and these three pins are the + // inverted descendants of the ones that described it. + // + // The alias used to `Omit` `mode` and re-add it as `NonNullable<…>`, which + // made it strictly tighter than BOTH the spec that produces the value and + // the hook that consumes it (`navigation?.mode ?? 'page'`, 140 lines below + // the declaration). A required key in front of an implementation that + // defaults it is not a contract, and the cost was paid at every spec-typed + // caller: `ListView` had to write `schema.navigation as NavigationConfig` + // to hand the hook a value the hook was always happy to take. + // + // The previous spelling of this block predicted its own end — "if the spec + // ever makes `mode` required itself, `_StillNarrowed` fails and this alias + // should collapse to `SpecAuthoredInput`." + // The collapse arrived from the other direction (the spec never moved; the + // alias was wrong all along), but it is the same collapse. + type _IsExactlyTheSpecInput = Assert>; + + // `mode` is OPTIONAL to author, because the spec defaults it + // (`packages/spec/src/ui/view.zod.ts` → `mode: NavigationModeSchema.default('page')`) + // and a `.default()` lands on the AUTHORING side as `| undefined`. This is + // the assertion that was false before objectui#4550. + type _ModeIsOptional = Assert>; + + // Assignability now runs BOTH ways. `_LocalIsASpecConfig` above is the one + // direction that always held; this is the one the narrowing blocked, and + // it is the direction every caller actually needs — spec produces, alias + // consumes. + type _SpecShapedValueFits = Assert>; // The overlay buckets the hook maps to pixel widths are the spec's, and // `size` is the key #2578 added — the deprecated `width` is still here too. @@ -265,7 +286,7 @@ describe('NavigationConfig derives from the spec, requiring only `mode`', () => }); it('still carries every overlay mode the hook switches on', () => { - const all: NavigationConfig['mode'][] = [ + const all: NonNullable[] = [ 'page', 'drawer', 'modal', @@ -284,19 +305,25 @@ describe('NavigationConfig derives from the spec, requiring only `mode`', () => // spec's `NavigationMode` directly, and this is the pin the declaration // promises: the two spellings must stay the SAME type. // - // They can come apart in a way nothing else would catch. The equality holds - // today only because `NavigationConfig` above strips the `undefined` that - // `NavigationConfigSchema`'s `.default('page')` puts on `mode`'s authoring - // side. If the spec ever stops defaulting `mode`, or defaults it on a - // narrower union, `NavigationConfig['mode']` moves and the exported alias - // does not — and every call site keeps compiling, because the hook's + // They can come apart in a way nothing else would catch. If the spec ever + // stops defaulting `mode`, or defaults it on a narrower union, + // `NavigationConfig['mode']` moves and the exported alias does not — and + // every call site keeps compiling, because the hook's // `navigation?.mode ?? 'page'` would still be assignable either way. // // Both directions, deliberately: a one-way `extends` is satisfied by a // narrowing as well as by equality, and a narrowing is exactly the drift // that would delete a mode the hook switches on. + // + // `NonNullable` is objectui#4550's mark on this pin, and it is load-bearing + // rather than cosmetic. The alias no longer strips the `undefined` that + // `.default('page')` puts on the authoring side, so `NavigationConfig['mode']` + // is now `NavigationMode | undefined` — a bare `Equal` here would fail for + // a reason that has nothing to do with the drift this test exists to catch. + // What must stay pinned is the MEMBERSHIP: the seven modes the hook + // switches on are exactly the seven the exported union publishes. type _ModeIsSpecMode = Assert>; - type _ModeIsConfigMode = Assert>; + type _ModeIsConfigMode = Assert>>; // Runtime half — the type assertions above are erased, so this is what // fails visibly if the union ever loses a member. diff --git a/packages/react/src/hooks/__tests__/useNavigationOverlay.modeDefault.test.tsx b/packages/react/src/hooks/__tests__/useNavigationOverlay.modeDefault.test.tsx new file mode 100644 index 000000000..3d9760b7c --- /dev/null +++ b/packages/react/src/hooks/__tests__/useNavigationOverlay.modeDefault.test.tsx @@ -0,0 +1,327 @@ +/** + * 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. + */ + +/** + * `useNavigationOverlay` — the RUNTIME half of objectui#4550. + * + * The type half lives in `offline-nav-performance-spec-parity.test.ts`, which + * pins that `NavigationConfig` IS the spec's authored config (`mode` optional). + * That file is compile-time only: every `Assert<…>` in it is erased before a + * single line runs, so it can prove the alias accepts a config without `mode` + * and prove nothing whatever about what the hook then DOES with it. + * + * This file is the other half. It exists because objectui#4550 relaxed a + * published type over an implementation it never described, and the whole case + * for that relaxation is that the implementation already behaved this way — + * `navigation?.mode ?? 'page'`, 140 lines below the declaration. A claim of + * "behaviour is unchanged" that nothing executes is not a claim, so the default + * is pinned here as an observable outcome, alongside every explicit mode the + * hook switches on. If a later change moves the default, deletes a branch, or + * 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. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { renderHook, act } from '@testing-library/react'; + +import type { NavigationConfigSchema } from '@objectstack/spec/ui'; +import type { SpecAuthoredInput } from '../../spec-input'; +import { useNavigationOverlay } from '../useNavigationOverlay'; +import type { NavigationConfig, NavigationMode } from '../useNavigationOverlay'; + +/** + * The exact value objectui#4550 reported the alias rejecting: a spec-authored + * navigation config that omits `mode`. The spec permits the omission on + * purpose — `packages/spec/src/ui/view.zod.ts` declares + * `mode: NavigationModeSchema.default('page')`, and a `.default()` lands on the + * authoring side as `| undefined`. + * + * The second declaration is the pin. Before objectui#4550 it was the bug + * itself, and `tsc` said so in as many words (verbatim, trimmed only where the + * compiler itself elided the union with "5 more ..."): + * + * error TS2322: Type '{ mode?: "none" | "split" | "page" | ... | undefined; + * view?: string | undefined; ... }' is not assignable to type + * 'NavigationConfig'. + * Types of property 'mode' are incompatible. + * Type '"none" | "split" | "page" | ... | undefined' is not assignable to + * type 'NonNullable< "none" | "split" | "page" | ... | undefined >'. + * Type 'undefined' is not assignable to type 'NonNullable< ... >'. + * + * and, for a bare literal, the shorter form: + * + * error TS2322: Type '{}' is not assignable to type 'NavigationConfig'. + * Property 'mode' is missing in type '{}' but required in type + * '{ mode: NonNullable< ... >; }'. + * + * A spec-shaped value that the spec-derived alias will not accept is the whole + * finding in two lines, which is why it is pinned as a declaration rather than + * described in a comment. + */ +const AUTHORED_WITHOUT_MODE: SpecAuthoredInput = { + view: 'summary_view', +}; +const AS_ALIAS: NavigationConfig = AUTHORED_WITHOUT_MODE; + +/** Every mode the hook's `handleClick` switches on, for the exhaustiveness pin. */ +const EVERY_MODE: NavigationMode[] = [ + 'page', + 'drawer', + 'modal', + 'split', + 'popover', + 'new_window', + 'none', +]; + +const OVERLAY_MODES: NavigationMode[] = ['drawer', 'modal', 'split', 'popover']; + +const RECORD = { id: 'r1', name: 'Ada' }; + +describe('useNavigationOverlay: a config without `mode` defaults to `page` (objectui#4550)', () => { + it('resolves `mode` to `page` and reports a non-overlay surface', () => { + const onNavigate = vi.fn(); + const { result } = renderHook(() => + useNavigationOverlay({ navigation: AS_ALIAS, objectName: 'contacts', onNavigate }), + ); + + expect(result.current.mode).toBe('page'); + expect(result.current.isOverlay).toBe(false); + expect(result.current.isOpen).toBe(false); + }); + + it('routes a click through the `page` branch, carrying the declared view', () => { + const onNavigate = vi.fn(); + const { result } = renderHook(() => + useNavigationOverlay({ navigation: AS_ALIAS, objectName: 'contacts', onNavigate }), + ); + + act(() => { + result.current.handleClick(RECORD); + }); + + // `view ?? 'view'` — the authored `view` survives the default-mode path. + expect(onNavigate).toHaveBeenCalledWith('r1', 'summary_view'); + expect(result.current.isOpen).toBe(false); + }); + + 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 + // the `mode === 'page'` branch rather than the `!navigation` early return. + // Both end at `onNavigate(id, 'view')`, and that agreement is the point — + // omitting `mode` must not become a third behaviour. + const { result } = renderHook(() => + useNavigationOverlay({ navigation: {}, objectName: 'contacts', onNavigate }), + ); + + expect(result.current.mode).toBe('page'); + act(() => { + result.current.handleClick(RECORD); + }); + expect(onNavigate).toHaveBeenCalledWith('r1', 'view'); + }); + + it('agrees with the no-config path it has to be indistinguishable from', () => { + const withEmpty = vi.fn(); + const withNothing = vi.fn(); + + const a = renderHook(() => + useNavigationOverlay({ navigation: {}, objectName: 'contacts', onNavigate: withEmpty }), + ); + const b = renderHook(() => + useNavigationOverlay({ objectName: 'contacts', onNavigate: withNothing }), + ); + + act(() => { + a.result.current.handleClick(RECORD); + b.result.current.handleClick(RECORD); + }); + + expect(withEmpty.mock.calls).toEqual(withNothing.mock.calls); + expect(a.result.current.mode).toBe(b.result.current.mode); + }); +}); + +describe('useNavigationOverlay: explicit modes behave exactly as before', () => { + it('reports `isOverlay` for the four overlay modes and no others', () => { + for (const mode of EVERY_MODE) { + const { result } = renderHook(() => useNavigationOverlay({ navigation: { mode } })); + expect(result.current.mode).toBe(mode); + expect(result.current.isOverlay).toBe(OVERLAY_MODES.includes(mode)); + } + }); + + it('opens the overlay and captures the record for drawer/modal/split/popover', () => { + for (const mode of OVERLAY_MODES) { + const onNavigate = vi.fn(); + const { result } = renderHook(() => + useNavigationOverlay({ navigation: { mode }, objectName: 'contacts', onNavigate }), + ); + + act(() => { + result.current.handleClick(RECORD); + }); + + expect(result.current.isOpen).toBe(true); + expect(result.current.selectedRecord).toEqual(RECORD); + expect(onNavigate).not.toHaveBeenCalled(); + } + }); + + it('delegates `page` to onNavigate and leaves the overlay shut', () => { + const onNavigate = vi.fn(); + const { result } = renderHook(() => + useNavigationOverlay({ navigation: { mode: 'page' }, objectName: 'contacts', onNavigate }), + ); + + act(() => { + result.current.handleClick(RECORD); + }); + + expect(onNavigate).toHaveBeenCalledWith('r1', 'view'); + expect(result.current.isOpen).toBe(false); + }); + + it('does nothing at all for `none`, and nothing for `preventNavigation`', () => { + const onNavigate = vi.fn(); + const none = renderHook(() => + useNavigationOverlay({ navigation: { mode: 'none' }, objectName: 'contacts', onNavigate }), + ); + const prevented = renderHook(() => + useNavigationOverlay({ + navigation: { mode: 'drawer', preventNavigation: true }, + objectName: 'contacts', + onNavigate, + }), + ); + + act(() => { + none.result.current.handleClick(RECORD); + prevented.result.current.handleClick(RECORD); + }); + + expect(onNavigate).not.toHaveBeenCalled(); + expect(none.result.current.isOpen).toBe(false); + expect(prevented.result.current.isOpen).toBe(false); + }); + + it('hands an external onRowClick full priority, whatever the mode says', () => { + const onRowClick = vi.fn(); + const onNavigate = vi.fn(); + const { result } = renderHook(() => + useNavigationOverlay({ + navigation: { mode: 'drawer' }, + objectName: 'contacts', + onNavigate, + onRowClick, + }), + ); + + act(() => { + result.current.handleClick(RECORD); + }); + + expect(onRowClick).toHaveBeenCalledTimes(1); + expect(onNavigate).not.toHaveBeenCalled(); + expect(result.current.isOpen).toBe(false); + }); +}); + +describe('useNavigationOverlay: the modifier-key and new_window branches', () => { + let openSpy: ReturnType; + + beforeEach(() => { + openSpy = vi.spyOn(window, 'open').mockImplementation(() => null); + }); + + afterEach(() => { + openSpy.mockRestore(); + }); + + it.each([ + ['metaKey', { metaKey: true }], + ['ctrlKey', { ctrlKey: true }], + ['middle click', { button: 1 }], + ])('%s opens a new window regardless of the configured mode', (_label, event) => { + const onNavigate = vi.fn(); + const { result } = renderHook(() => + useNavigationOverlay({ navigation: { mode: 'drawer' }, objectName: 'contacts', onNavigate }), + ); + + act(() => { + result.current.handleClick(RECORD, event); + }); + + expect(onNavigate).toHaveBeenCalledWith('r1', 'new_window'); + expect(result.current.isOpen).toBe(false); + }); + + it('applies the same modifier override when `mode` is absent entirely', () => { + // The objectui#4550 shape: the modifier branch runs BEFORE the mode is + // consulted, so a defaulted mode must not change it. + const onNavigate = vi.fn(); + const { result } = renderHook(() => + useNavigationOverlay({ navigation: AS_ALIAS, objectName: 'contacts', onNavigate }), + ); + + act(() => { + result.current.handleClick(RECORD, { metaKey: true }); + }); + + expect(onNavigate).toHaveBeenCalledWith('r1', 'new_window'); + }); + + it('delegates `new_window` to onNavigate when one is supplied', () => { + const onNavigate = vi.fn(); + const { result } = renderHook(() => + useNavigationOverlay({ + navigation: { mode: 'new_window' }, + objectName: 'contacts', + onNavigate, + }), + ); + + act(() => { + result.current.handleClick(RECORD); + }); + + expect(onNavigate).toHaveBeenCalledWith('r1', 'new_window'); + expect(openSpy).not.toHaveBeenCalled(); + }); + + it('opens the routed record URL itself when no onNavigate is supplied', () => { + const { result } = renderHook(() => + useNavigationOverlay({ navigation: { mode: 'new_window' }, objectName: 'contacts' }), + ); + + act(() => { + result.current.handleClick(RECORD); + }); + + expect(openSpy).toHaveBeenCalledWith('/contacts/record/r1', '_blank'); + }); +}); + +describe('useNavigationOverlay: overlay width still resolves off the spec keys', () => { + it('maps a `size` bucket and lets an explicit `width` win, with `mode` omitted', () => { + // `size`/`width` are read off the same config whose `mode` is now optional; + // this pins that relaxing `mode` did not disturb the #2578 size path. + const bucketed = renderHook(() => useNavigationOverlay({ navigation: { size: 'lg' } })); + expect(bucketed.result.current.width).toBe('min(92vw, 960px)'); + + const explicit = renderHook(() => + useNavigationOverlay({ navigation: { size: 'lg', width: '640px' } }), + ); + expect(explicit.result.current.width).toBe('640px'); + + const auto = renderHook(() => useNavigationOverlay({ navigation: { size: 'auto' } })); + expect(auto.result.current.width).toBeUndefined(); + }); +}); diff --git a/packages/react/src/hooks/useNavigationOverlay.ts b/packages/react/src/hooks/useNavigationOverlay.ts index 3a03ff4f4..a5c742e99 100644 --- a/packages/react/src/hooks/useNavigationOverlay.ts +++ b/packages/react/src/hooks/useNavigationOverlay.ts @@ -23,7 +23,8 @@ import type { NavigationConfigSchema, NavigationMode as SpecNavigationMode } fro import type { SpecAuthoredInput } from '../spec-input'; /** - * The spec's `NavigationConfigSchema`, authoring side, with `mode` required. + * The spec's `NavigationConfigSchema`, authoring side — by reference, with no + * divergence of its own. * * This was a hand copy carrying the note "inline … to avoid importing from * @object-ui/types (which may not be a direct dependency of @object-ui/react)". @@ -33,10 +34,35 @@ import type { SpecAuthoredInput } from '../spec-input'; * `@object-ui/app-shell`, where the dependency had likewise been there all * along. Check `package.json` before believing such a note (objectstack#4115). * - * The ONE divergence: the spec defaults `mode`, so its authoring side makes it - * optional; this hook dispatches on `mode` and its callers always supply one, - * so it is required here. Every other key is the spec's, by reference. Pinned - * by `__tests__/offline-nav-performance-spec-parity.test.ts`. + * `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. + * + * Until objectui#4550 this alias `Omit`ted `mode` and re-added it as + * `NonNullable<…>`, on the stated reasoning that "this hook dispatches on + * `mode` and its callers always supply one". Both halves were false. + * `useNavigationOverlay` does not require `mode` — it DEFAULTS it + * (`navigation?.mode ?? 'page'`, ~140 lines below), and `'page'` is meaningful + * behaviour rather than a placeholder. And callers did not always supply one: + * `ListView` carried `schema.navigation as NavigationConfig | undefined` purely + * to get a spec-shaped value past this declaration — a value the hook had + * always been willing to take. + * + * The rule that makes this a producer-side fix rather than a caller-side one: + * a type in front of an implementation must not be stricter than the + * implementation. When it is, every caller pays in assertions, and an assertion + * is exactly the renderer-side workaround AGENTS.md #0.1 sends back to the + * producer. Here the producer was this line. + * + * `@object-ui/types` re-exports the spec's own `NavigationConfig` unchanged, so + * that published name and this one now agree — they did not before, which is + * how one monorepo shipped two `NavigationConfig`s that disagreed about whether + * `mode` could be omitted. Pinned by + * `__tests__/offline-nav-performance-spec-parity.test.ts` (the type) and + * `__tests__/useNavigationOverlay.modeDefault.test.tsx` (the default behaviour + * the relaxation rests on). * * Two per-key notes the hand copy carried, kept here because a derived alias * has no members to hang them on: @@ -45,12 +71,7 @@ import type { SpecAuthoredInput } from '../spec-input'; * - `width` is DEPRECATED by #2578 in favour of `size`. It still wins when * present, because app-shell pre-resolves `size` into it. */ -export type NavigationConfig = Omit< - SpecAuthoredInput, - 'mode' -> & { - mode: NonNullable['mode']>; -}; +export type NavigationConfig = SpecAuthoredInput; /** * Pixel cap per overlay `size` bucket, clamped to the viewport at render — @@ -89,7 +110,7 @@ export function resolveOverlayWidth(navigation: NavigationConfig | undefined): s * The overlay modes — the spec's own union, DERIVED since objectui#4167. * * rc.6 publishes `NavigationMode` (`z.input`), and - * this alias resolved to exactly it already: `NavigationConfig['mode']` above is + * this alias resolved to exactly it already: `NavigationConfig['mode']` was then * `NonNullable<…['mode']>`, and stripping the `undefined` that the schema's * `.default()` puts on the authoring side leaves the seven-member enum itself. * So the spec reference was one hop away rather than absent — the alias just @@ -98,8 +119,16 @@ export function resolveOverlayWidth(navigation: NavigationConfig | undefined): s * * Bound to the spec directly instead: the seven members now arrive from the * schema that validates them. `__tests__/offline-nav-performance-spec-parity.test.ts` - * pins that this stays the same type as `NavigationConfig['mode']`, so the two - * spellings cannot silently come apart if the spec ever stops defaulting `mode`. + * pins that this stays the same type as `NonNullable`, + * so the two spellings cannot silently come apart if the spec ever stops + * defaulting `mode` or defaults it on a narrower union. + * + * The `NonNullable` in that pin is objectui#4550's mark and is load-bearing: + * `NavigationConfig` no longer strips the authoring-side `undefined`, so + * `NavigationConfig['mode']` is `NavigationMode | undefined` and a bare + * equality would now fail for a reason unrelated to the drift being guarded. + * What stays pinned is the MEMBERSHIP — the seven modes this hook switches on + * are exactly the seven the exported union publishes. */ export type NavigationMode = SpecNavigationMode;