Uh oh!
There was an error while loading. Please reload this page.
refactor(app-shell): paramToField's reference-bearing rule derives from core, not the last private copy - #5655
Merged
Conversation
…om core, not the last private copy `packages/app-shell/src/utils/paramToField.ts` restated the rule inline as `LOOKUP_WIDGET_TYPES.has(type) || type === 'user'` — the fourth and last hand-maintained answer to "does this widget resolve a foreign key, so hand it the reference config". The other three converged on `@object-ui/core`'s `EXPANDABLE_FIELD_TYPES` in objectui#4770 / #4790 / #4815; this face now does too. No reachable behaviour change: the shared set is one member wider (`tree`), and that member can never be a widget key here — absent from `fields`' widget map, and `mapFieldTypeToFormType` sends it to `field:lookup` — so every key the rule tests arrives as `lookup`. Both halves pinned. The module's second rule (which keys degrade to text for want of a declared `referenceTo`) is a different set over overlapping types and stays un-merged: `user` defaults its target to `sys_user` and must never degrade. Same split the plugin-grid twin keeps. Retires the comment claiming the disjunction "moves in lockstep with plugin-grid's `bulkParamToField` twin — the two param faces are never split". Measured false in both senses before this change: the twin had read core's Set since objectui#4815 while this line read a private literal, and the two member sets already differed by `tree`. Lockstep now holds mechanically — the pin is on object identity (a spy on core's `has`), so a member-identical private copy fails where a value check would pass. Fixes#5312 Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-sales
marked this pull request as ready for review
August 22, 2026 01:14
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 22, 2026
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 freeto 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#5312
packages/app-shell/src/utils/paramToField.tsrestated the reference-bearing rule inline asLOOKUP_WIDGET_TYPES.has(type) || type === 'user'— the fourth and last hand-maintained answer to one question: does this widget resolve a foreign key, so hand itreference_to/display_field/ the rest of the picker config? The other three converged on@object-ui/core'sEXPANDABLE_FIELD_TYPESin #4770 / #4790 / #4815. This face now does too.The comment was a claim to test, not a fact to inherit — and it was false
The disjunction carried this in-source comment:
Measured on the tip (
aa3b81062) before touching anything. It was false in both senses:paramToField(this file, before)bulkParamToField(the twin)new Set(['lookup','master_detail'])||type === 'user'EXPANDABLE_FIELD_TYPES.has(t)— core's exported object, since #4815{lookup, master_detail, user}{lookup, master_detail, tree, user}bulkParamToField.test.ts:137)Setsince finding(plugin-grid):DATA_SOURCE_WIDGET_TYPES是「哪些 widget 要 DataSource」的第四份私有副本 —— 与表单规则不同集、零 gate #4815; this line read a private literal. The two faces shared no object, so "lockstep" was hand-kept — and nothing anywhere could report a divergence.tree.So the comment described a guarantee that had stopped existing. It is replaced with what was measured, and lockstep now holds mechanically rather than by assertion.
What changed
EXPANDABLE_FIELD_TYPES.has(type)— core's object, read, never copied.new Set([...EXPANDABLE_FIELD_TYPES, …])" extension note the twin carries.LOOKUP_WIDGET_TYPESstill answers a different question — which keys degrade to a text input for want of a declaredreferenceTo— over an overlapping but different set:useris reference-bearing yet defaults its target tosys_user, so it must never degrade. Its docblock now says so explicitly. Same split the twin keeps between its ownLOOKUP_WIDGET_TYPESandwidgetNeedsDataSource.No reachable behaviour change
The derived set is one member wider (
tree), and that member can never be a widget key here: it is absent fromfields' widget map andmapFieldTypeToFormTypesends it tofield:lookup, so every key the rule tests — alwaysresolveParamWidgetTypeoutput — arrives aslookup. Both halves are pinned, so registering a realtreewidget surfaces the change instead of shipping it silently.Pins, and the ablation legs that prove each can fail
Pinned by identity, not membership: every membership assertion in the file is satisfied by a member-identical private copy — i.e. by the exact state this PR removes — so a value check would have passed on the defect.
Each control got its own mutation leg. Every leg asserted its anchor count non-zero before mutating, then proved the mutation reached disk in both directions (removed-text count and injected-text count and the file's
git hash-object), and restored under anEXIT/INT/TERMtrap. No rebuild was needed and none was skipped: the test imports./paramToFieldrelatively and@object-ui/corethrough the repo-root vitest alias topackages/core/src, so both legs read source, never adist/artefact. (The green identity pin is itself the proof that test and module resolve to one@object-ui/coreinstance — a split resolution would leave the spy empty.)new Set(['lookup','master_detail','tree','user'])EXPANDABLE_FIELD_TYPES.has(type) && type !== 'user'tree: 'user', soresolveParamWidgetType('tree') !== 'lookup'tree-unreachability probe red:51assertion of the same factEXPANDABLE_FIELD_TYPES)sys_usertest, and the identity pin (merging degradesuserto text before the branch is reached, so the spy never sees'user')Leg A is the load-bearing one: it is the defect this PR removes, and only the identity pins notice it.
Leg C mutated by insertion, not replacement, so its removed-text anchor stayed at 1 by construction; the injected-text count went 1 → 2 and the file hash changed. Recorded as measured rather than smoothed into the table's shape.
All four legs restored to the pristine blob
b3e8a51a, verified by hash after each leg and once more by the trap.Verification — all at
7784e137eTest scope is a derived superset, not a sample.
packages/app-shellhas ~488 test files and its full suite exceeds the container's foreground cap. Both changed files are side-effect-free ES modules and vitest isolates the module graph per test file, so a test whose static import graph never reaches a changed file cannot observe this change — there is no other channel (no global written, no registry mutated, no generated artefact). The reverse import closure of the two changed files is therefore a superset of "tests this change can affect". It was over-approximated three further ways so the answer only ever grows: specifiers resolved extension- and index-agnostically, every existing candidate path counted as an edge, and bare@object-ui/*specifiers resolved through the vitest alias table (which is what pulls the wholeapps/consoleproject in, via theapp-shellbarrel).105 test files — 67 in
packages/app-shell, 38 inapps/console. 1127 tests, 0 failures. Run from the repo root (package-cwd vitest is refused by the repo's own guard), paced into nine batches because a single 67-file invocation was SIGTERM'd at the cap with a buffered reporter that had written nothing — the batches use a streaming reporter so a cap kill can never again cost the findings. Each superset file was confirmed to have actually executed by diffing the TAP file names against the derived list (theapps/consoleproject reports paths relative to its own root, so the two lists only reconcile after normalising that prefix — the first comparison "found" 38 missing files that had all run).Tests 1127 passed, 0 failedpnpm --filter @object-ui/app-shell type-checktsc --noEmit && tsc -p tsconfig.test.json, both echoed — not a zero-match no-op)pnpm --filter @object-ui/app-shell lintcheck-changeset-presence✅ 2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)check-changeset-fixed/check-changeset-no-major✅ All workspace packages are in the changeset fixed group./✅ No changeset declares a major bump.check-control-bytes✅ OK (scanned 4695 tracked text file(s); skipped 85 binary)check-package-self-import✅ No package names itself inside its own src/.check-phantom-dependencies✅ Every in-scope import is declared by the package that publishes it.check-spec-symbol-derivation✅ 1290 files scanned against 4912 spec export namescheck-lint-coverage✅ 46/46 packages linted, 0 with outstanding errors (0 total)check-type-check-coverage✅ 45/46 via type-check … ✅ 41/41 packages compile their testsRecorded because it looks like a discrepancy and is not: an ad-hoc
eslint packages/app-shell --no-inline-configfrom the repo root reports 15 errors across 920 files, while the gate (turbo run lint→ per-packageeslint .) reports 0. The difference is--no-inline-configstripping pre-existingeslint-disablecomments in 13 files this PR does not touch (12 ×react-hooks/static-components, plus oneno-unused-expressionsand oneno-console). None are in the changed files, andcheck-lint-coverageindependently reports 0 outstanding errors repo-wide.The two
@typescript-eslint/no-explicit-anywarnings on the changed file are pre-existing: linting the base blob through stdin reproduces both at the pre-shift line numbers (63/76→73/86).Scope
The root barrel was not needed and is untouched (
packages/app-shell/src/index.tsdoes not re-export this module, the rule is read inline rather than through a new export, and the pin test imports./paramToFieldrelatively) — so no serialisation against #5596 is required.packages/app-shell/src/views/metadata-admin/**is untouched; its tests were only run, as part of the derived superset.One out-of-scope finding filed, not fixed here: #5654 —
ActionParamDialog'sisLookupParamrestates the picker family over raw param spellings ({lookup, reference}) while the module that performs the degradation tests resolved widget keys ({lookup, master_detail}). The two sets are not in a subset relation either way, and the measured consequence is that a targetlessmaster_detailparam degrades to a text input but gets neither the placeholder nor the help text #3405 added for that state. It is a different rule with no authority to derive from, and repairing it changes behaviour — so it is a separate card, not a rider.Generated by Claude Code