fix(types): ViewNavigationConfig.mode is optional — the second navigation spelling agrees with the spec (#4588) - #4591
Merged
Conversation
…tion spelling agrees with the spec (#4588) `@object-ui/types` published two types for one spec object, and they disagreed about whether `mode` may be omitted. `index.ts` re-exports the spec's `NavigationConfig` unchanged; `objectql.ts` hand-declared a `ViewNavigationConfig` over the same six keys with `mode` required — under a doc comment that itself claimed `@default 'page'`. The spec declares `mode: NavigationModeSchema.default('page')`, and a `.default()` lands on the authoring side as `| undefined`, which is why the spec publishes its own type as the schema's `z.input`. So `{ view: 'summary_view' }` is legal authored metadata that the hand copy refused, at the three interfaces spelling `navigation?: ViewNavigationConfig`. `ViewNavigationConfig` is now that spec type, per this file's own standing rule: "Never Redefine Types. ALWAYS import them." Measured against the published spec build, the copy had drifted on `mode` and nothing else. No runtime behaviour changes — every `.mode` read in the repo is a `=== 'x'` comparison or `?? 'page'`. This is #4550 / PR #4586 one package over. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4588
The mismatch, measured
@object-ui/typespublished two types for one spec object, and they disagreed about whethermodemay be omitted.The doc comment claimed
@default 'page'on the one key it then made required.The spec never asked for that.
NavigationConfigSchemadeclaresmode: NavigationModeSchema.default('page'), and a.default()lands on the authoring side as| undefined— so the spec publishes its own type asz.input< typeof NavigationConfigSchema >.navigation: { view: 'summary_view' }is legal authored metadata that lets the mode default.Measured against the published spec build actually resolved here (
@objectstack/spec@17.0.0-rc.6,dist/view.zod-CN_gQt7k.d.ts:826), the hand copy had drifted onmodeand nothing else — the other five keys carried the spec's exact value domains:.d.ts)modeZodDefault< ZodEnum >→ input-optionalviewZodOptional< ZodString >view?: stringpreventNavigationZodDefault< ZodBoolean >preventNavigation?: booleanopenNewTabZodDefault< ZodBoolean >openNewTab?: booleansizeZodDefault< ZodEnum >, 6 memberswidthZodOptional< ZodUnion >string | numberAlias shape: collapse, not restate — the convention, measured
The ruling preferred collapsing to the spec type if the file's conventions allow it. They do, emphatically:
objectql.ts:28-30carries the rule as a banner: "Spec-Canonical Types — imported from@objectstack/spec/ui. Rule: 'Never Redefine Types. ALWAYS import them.'":51 :57 :68 :74 :80 :86 :134 :143, plus the multi-import at:89-99), and:51is even a renaming one.@object-ui/typeshas zero workspace dependencies (--filter '@object-ui/types^...' buildmatched 0 projects);@objectstack/specis an external dependency the file already imports, andindex.tsalready re-exports this very symbol from the same entry point.One correction along the way, worth recording:
export type { NavigationConfig as ViewNavigationConfig } from '@objectstack/spec/ui're-exports but creates no local binding, so the three in-file uses failed withTS2304: Cannot find name 'ViewNavigationConfig'. The file already has the right idiom for that case — theimport type { … } from '@objectstack/spec/ui'block at:89labelled "Import spec types for local use in interfaces below". So:That is a structural derivation, which is one of the two forms
check:spec-symbolssanctions.Writer / reader census
The three
navigation?: ViewNavigationConfigsites (objectql.ts:852ObjectGridSchema,:1453ObjectViewSchema,:1674NamedListView) now accept a mode-less config. Each is pinned in the new suite.Readers — every
.moderead in the repo, and its disposition:plugin-view/ObjectView.tsx:542-570navigationConfig.mode === 'x', behindif (navigationConfig)plugin-view/ObjectView.tsx:1174-1177navigationConfig?.mode === 'x'app-shell/views/ObjectView.tsx:1421authored.mode === 'page'react/useNavigationOverlay.ts:224navigation?.mode ?? 'page'undefined:536/ calendar:396/ gantt:1148navConfig.mode === 'x'onany-typed valuesfalse(non-overlay), agreeing with the'page'defaultplugin-grid/ObjectGrid.tsx:3309So "fix any reader that inherits the default" is again a measured no-op — no reader of this alias reads
modeunguarded. Reported as measurement, not manufactured into edits.Obsoleted assertions: none, measured. No test in
packages/typesreferencedViewNavigationConfigat all before this PR (grepoversrc/__tests__), andnavigation-spec-parity.test.tsguardsNavigationItemSchema— app navigation, a different object. No assertion existed for this change to obsolete, so none was invented.A convergence worth naming:
app-shell/views/ObjectView.tsx:1414declaresdetailNavigation: ViewNavigationConfigand feeds it straight touseNavigationOverlay({ navigation: detailNavigation })— whose option type #4586 already made mode-optional. The two halves now agree instead of colliding.Red-first
Predictions were written before the edit; all four materialised. Verbatim
tsc -p tsconfig.test.jsonagainst the unchanged alias:6 errors, exit 2 — line 117 is the card's own
{ view: 'summary_view' }. After the change all threetscpasses are clean.Two pins did not go red pre-change, and that is the finding rather than a gap:
_KeysAreExactlyTheSpecSixand_ModeMembershipIsTheSevenSpecModesalready held. The hand copy had the right six keys and the right seven modes —mode's optionality was its only drift. Both pins stay, because they are what catches the next drift.Type-level pins, per the #4586 idiom
_ModeMembershipIsTheSevenSpecModescompares againstNonNullable< ViewNavigationConfig['mode'] >. A bareEqualwould now befalsefor the wrong reason — the| undefinedthe.default()puts there deliberately, not a change in which modes exist — soNonNullablekeeps the assertion pointed at the membership it guards. Same care_ModeIsConfigModeneeded one package over._IsExactlyTheSpecInputpins the collapse itself: the two names this package publishes for the spec's navigation config are now invariantly one type, so a re-grown hand copy fails the build on the day it drifts.Reverse verification — direction stated up front
A type-only change, so the honest red is a tsc red, not a suite red. Both were run:
git diff > fix.patch+git checkout --, nevergit stash):tsc -p tsconfig.test.json→ the same 6 errors, exit 2.vitestoverpackages/types packages/plugin-viewwith the fix removed → 522/522 still green, because the runtime is untouched. Reporting a vitest red here would have been a fabrication.sha256sum -con both files.Record< string, unknown >producedTS2578: Unused '@ts-expect-error' directive.at both refusal sites, plus reds on the collapse, key-set and membership pins. Restored and sha256-verified again.Verification
pnpm --filter '@object-ui/types^...' build— 0 projects: this package has no workspace dependencies (measured, not assumed).pnpm --filter @object-ui/types type-check— green, all three passes (tsc --noEmit,tsconfig.examples.json,tsconfig.test.json).pnpm exec vitest run --maxWorkers=2 packages/types packages/plugin-view— 43 files, 522 tests passed (32 types + 11 plugin-view). The new suite is 5 of them, confirmed running in theunitproject rather than assumed....@object-ui/types= downstream consumers), after a full 47-project workspace build so no stale artifacts: 40 packages green, includingapp-shell,plugin-view,plugin-list,plugin-grid,react,apps/consoleand three examples. The first attempt failedTS2307on@object-ui/collaboration→@object-ui/i18n, which sits outside the filter's scope (42 of 47) — the known stale-artifact trap, fixed by the full build, not by touching anything.origin/mainin a comparison worktree, on the edited source file: 15 → 15, NET ZERO, 0 errors both sides. The new test file adds 7no-unused-varswarnings for its type-level pins — exactly the class the sibling pin fileobjectql.exportOptions.test.tsalready emits (4 of them), which carries noeslint-disable, so this follows the established convention.check:control-bytes,check:phantom-deps,changeset:check,check:spec-symbols— all green.grep -naPcontrol-byte self-scan over both touched files: clean.Changeset grade:
minorfor@object-ui/typesMeasured from the built
.d.ts(cleandist/+tsconfig.tsbuildinfobetween both builds).ViewNavigationConfigoccurs in only three consumer-facing positions — thenavigation?:properties ofObjectGridSchema,ObjectViewSchemaandNamedListView— and in no return position: a regex for): ViewNavigationConfig/=> ViewNavigationConfigover the wholedistmatched nothing, and this is a pure type package that publishes no function to hand one back. Every published position is an input position.So for consumers the change is purely permissive: everything that compiled still compiles, and spec-shaped configs that previously needed an invented
modenow compile without one. That gained input shape is a capability rather than an internal repair, which is more thanpatchdescribes. The reader-side narrowing is real but secondary, and the in-repo census found no reader affected by it. Nevermajor.The
.d.tsdiff is two hunks: the import line, and the interface replaced by the alias.index.d.tsis byte-identical. The only type-level lines in the whole diff are those two.Surface
packages/types/src/objectql.ts, one new test file inpackages/types, one changeset.plugin-viewwas read-only and is byte-unchanged — the census did not demand an edit there. Untouched as instructed:packages/react(#4585),core/i18n(#4576),ObjectGrid,content/docs/releases/.Generated by Claude Code