Uh oh!
There was an error while loading. Please reload this page.
fix(fields): render inline lookup dropdown columns through the cell renderer - #5598
Merged
Merged
Conversation
…enderer (#5492) A lookup's inline dropdown and its "browse all records" picker read one `lookup_columns` declaration and answered differently: the picker resolved each cell through the type-aware cell renderer, while the dropdown printed the raw stored value into the option subtitle and concatenated `label: String(rawValue)` into the row's `title`. So one declaration produced a bare foreign-key id beside a resolved name, a raw ISO timestamp beside a formatted date, and an enum code beside an option label. Both surfaces now call one shared module, `widgets/lookupColumnDisplay.tsx`, which owns column normalisation, field-descriptor enrichment from the referenced object's schema, and the render itself. No query changed and no contract widened: `lookupColumns` entries stay bare field names, and an unresolved foreign key is resolved client-side by the lookup cell renderer exactly as the picker has always done. 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
commented
Aug 21, 2026
CollaboratorAuthor
PM review — ACCEPT (card #5492)Gates. 22 named check runs read individually for Merits. The load-bearing check for this card was whether the popover path already depends on relation expansion, because if it did, the spec-widening ask in the card body would be a prerequisite and this change could not land alone. It does not: the popover query at Diff stays inside the fenced surface; no public surface widened, so clause ② is not engaged. Generated by Claude Code |
os-sales
marked this pull request as ready for review
August 21, 2026 16:47
Uh oh!
There was an error while loading. Please reload this page.
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#5492
The disagreement
A lookup field offers two ways to pick a related record, and both read one
lookup_columnsdeclaration:RecordPickerDialog's table behind "show all results"Both code paths, re-derived on
origin/mainatcad512fe1and named as thecard asks:
packages/fields/src/widgets/LookupField.tsx:recordToOption, which setdescription = record[descriptionField]and printed it verbatim;previewOf, which built`${lbl}: ${text}`per column,textbeing the raw value, and droppedthe result into the row's
titleattribute.RecordPickerDialog.renderCellContent, which looked each column up incolumnFieldDescriptors(enriched from the referenced object's schema) andrendered it through the resolver
LookupFieldhands it ascellRenderer={getCellRendererResolver()}.Measured on the branch, driving the same declaration through both surfaces
(this is the ablation output, quoted verbatim below):
The fix
A new shared module,
packages/fields/src/widgets/lookupColumnDisplay.tsx,becomes the one place that answers "what does column C of this candidate
record display?". It owns column normalisation, the field-descriptor
enrichment from the referenced object's schema, and the render itself.
RecordPickerDialognow buildscolumnFieldDescriptorswithbuildLookupColumnDescriptorsand implementsrenderCellContentas a callto
renderLookupColumnValue. Its behaviour is unchanged — the logic moved,it was not rewritten.
LookupFieldbuilds the same descriptors from the samerefObjectSchema.fieldsand renders its option preview through the samerenderLookupColumnValue. The subtitle and the old tooltip merge into onerendered preview line in the option row; the row's
titlenow carries thefull option label, which is what a truncated label actually needs, instead of
a raw-value dump.
There is one renderer left, so the two surfaces cannot drift apart again.
The cost this accepts, stated plainly
Resolving a foreign key to a name is a client-side fetch, so a dropdown page
that previously rendered ids now mounts one lookup cell renderer per lookup
column per option, up to the popover's page size of 50. Those resolutions are
deduplicated by the module-level name cache the picker already fills, and only
lookup-typed columns fetch at all — a date or a select column is pureformatting. It is the same work the picker has always done over its (smaller,
10-row) page, and it is the work the reporter is asking for: the alternative to
the fetch is the bare id. Flagging it because it is a real change in request
volume on popover open, not because it is believed to be a problem.
What is deliberately NOT here
No query changed and no contract widened.
lookupColumnsentries stay barefield names — no dot-path, no populate/expand semantics — and this fix does not
need them: neither surface's request carries populate. Re-derived on the
branch,
LookupFieldandRecordPickerDialogboth calluseRecordQuerywithout an
expandargument. The picker resolved foreign-key ids to namespurely client-side all along, in
LookupCellRenderervia its fetch-on-demanduseLookupNameplus a module-level name cache. The dropdown now inheritsexactly that resolution. So the spec-widening ask on the card is not a
prerequisite for the primary defect, and is not attempted here.
The unresolved-id decision
Because no populate is involved, a lookup column's value can legitimately be an
id that resolves to nothing. The decision: the dropdown adopts whatever the
picker shows, and the column is never dropped.
decided on the raw value, never on what the renderer makes of it — so an
unresolved id keeps its slot.
present-but-unnamed reference (a muted en dash, still wrapped in the
referenced-record link so the reference stays openable), not the shared
empty-value slot that means "this record has nothing here". The test asserts
that distinction structurally, not by glyph.
reports for a dot-path attempt is the case where the column disappears; that
is what the raw-value presence rule rules out.
Tests
packages/fields/src/widgets/LookupField.pickerAgreement.test.tsxpins thedisagreement, not either surface alone: it drives one
lookup_columnsdeclaration through the inline dropdown and through the browse-all picker and
compares them column for column, for a lookup, a date and a select field, in
two states — a reference that resolves and one that does not.
Reverse-verification, both mutations proved on disk with anchored
grep -cinboth directions plus
git diff --stat, restored by atrap ... EXIT INT TERM,tree confirmed byte-identical afterwards (
git status --porcelainempty, markersweep 0/0). No rebuild leg is needed and that is a property of the resolution
path, not an assumption: the test reaches every mutated file through a
relative specifier inside
packages/fields/src, so nothing resolves througha sibling package's
exportsto adisttree.raw value again (the pre-fix behaviour). Anchor count 1 to 0, marker 0 to 1,
1 insertion(+), 5 deletions(-). Predicted red, observed red: 2 of 2 testsfailed, and the failure diff reproduces the card's table exactly (the block
quoted above is that diff). The fallback assertions behaved as predicted —
present,emptySlot === falseand non-empty still passed under theablation, because the bare id is also non-empty. They are a floor the fix must
not fall below, not a detector for this mutation; the agreement assertion is
what turns red.
Anchor count 1 to 0, marker 0 to 1,
1 insertion(+), 1 deletion(-). Predictedand observed: both surfaces degrade together, so the agreement assertion
keeps passing while the concrete renderings go red
(
expected 'T5MsMCuwP4t_yUHq' to be 'Fitting, Line A',expected 'pending' to be 'Pending approval'). That is the proof that theshared module is the live renderer for both surfaces and not just for one.
Gates
Exit codes captured before any pipe; each verdict is the gate's own line.
pnpm --filter @object-ui/fields type-checkos-verify-lock: VERDICT command-exit 0(tsc --noEmit && tsc -p tsconfig.test.json, no diagnostics)pnpm --filter @object-ui/fields lintos-verify-lock: VERDICT command-exit 0—✖ 869 problems (0 errors, 869 warnings)over 190 filespnpm exec vitest run packages/fields/(repo root)Test Files 111 passed (111)/Tests 1839 passed (1839)node scripts/check-control-bytes.mjs✅ check-control-bytes: OK (scanned 4645 tracked text file(s))node scripts/check-changeset-presence.mjs✅ 4 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)node scripts/check-changeset-no-major.mjs✅ No changeset declares a major bump.node scripts/check-changeset-fixed.mjs✅ All workspace packages are in the changeset fixed group.pnpm check:i18n-keys✅ every in-scope call-site key resolves against the en pack(not()call site changed)pnpm check:phantom-deps✅ Every in-scope import is declared by the package that publishes it.pnpm check:self-import✅ No package names itself inside its own src/.pnpm check:esm-specifiersSpecifier leg: no un-ledgered package emits an extensionless relative specifier.pnpm check:spec-symbols✅ spec symbol derivation: 1289 files scannedpnpm check:action-forward-paritypnpm check:i18n-driftNo en value changed in this range.node scripts/check-type-check-coverage.mjs✅ type-check coverage: 45/46 ... ✅ test type-check coverage: 41/41node scripts/check-lint-coverage.mjs✅ lint coverage: 46/46 packages linted, 0 with outstanding errorsAll of the above ran on the final commit's tree,
6582f0d2b, withgit status --porcelainempty.type-checkandlintwere re-run on thatexact tree after the last edit, so no verdict here reports a tree that is no
longer HEAD.
Declared narrowing. Lint ran over
packages/fields(190 files, fromeslint's
--format jsonoutput) rather than the repo-wideeslint .. It cannothide a failure elsewhere:
eslint.config.jsdeclares noproject/projectService, so type-aware linting is off and every file's verdict is afunction of its own text — this diff touches five files, four of them under
packages/fields/srcand one changeset, so no untouched file's verdict canmove. The population is read from eslint's own configuration by
check-lint-coverage.mjs, which reports 46/46 packages in scope at 0 outstandingerrors. Type-check was likewise narrowed to
@object-ui/fields: the onlypublicly visible type change is
CellRendererResolver, which becomes an aliasof a structurally identical type, and the new module is not re-exported from the
package barrel.
pnpm check:eager-closurewas not run — it readsapps/console/dist/eager-closure.jsonand reports itself a broken gauge withouta console build; that build is CI's, and this diff does not touch the console.
Generated by Claude Code