Uh oh!
There was an error while loading. Please reload this page.
fix(plugin-kanban): delete resolveDisplay's unreachable isLookup guard, pin the surviving value-shaped rule - #6268
Merged
Conversation
…d and pin the surviving rule The guard sat directly above a line that returned on the same condition unconditionally, over the same `raw` and the same pure predicate, so it was computed, branched on and discarded. The card named two readings and this takes the first: the opaque-id suppression is a rule about the VALUE, not about the field's declared type. Gating it on `isExpandableFieldType` would make the heuristic fail open on every board whose object schema is thin — `objectDef` is optional at that read. Deleting the dead branch also deleted this path's read of core's EXPANDABLE_FIELD_TYPES, so objectui#5874's identity pin is re-anchored onto the face's live read (`buildExpandFields`), where the membership delta is also observable on the wire — the behavioural counter-probe that pin had to record as missing.
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
|
yinlianghui
marked this pull request as ready for review
August 25, 2026 06:24
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#6063
Which arm — A (the guard is redundant), and why it is not "deletion because it is smaller"
The card named two readings and said the choice is behavioural. It is arm A: the
opaque-id suppression in
resolveDisplayis a rule about the value, not about thefield's declared type. Four pieces of evidence, none of them "the code is shorter this way":
IDs, and skips lookup-typed fields whose value didn't get expanded". The
unconditional line implements the first clause. Under reading B that clause would be
deleted — i.e. B contradicts the stated intent rather than restoring it.
incoming
descriptionis run throughisOpaqueIdwith noobjectDefconsulted at all(
descMissing). Under B the board would suppress an id-shapeddescriptionbut printan id-shaped
company— two answers to one question in one function.objectDefis optional at the read site (objectDef?.fields?.[key]). When theobject schema is thin or has not resolved,
isExpandableFieldType(def)isfalsebyconstruction, so B would suppress nothing exactly on the boards that cannot supply
the type information B depends on. A gibberish-suppression heuristic that fails open
when it cannot see field types is not the intent it would be restoring.
the same commit (
9986f9114, the commit that added the file). There is no earlierstate in which lookup-only suppression existed and was later broadened, which is the
shape B's premise ("the unconditional line is the mistake") would need.
The unreachability argument, re-derived on today's
mainMerge base
ef2a3bd8d,packages/plugin-kanban/src/ObjectKanban.tsxlines 366-369:The two conditions are genuinely identical, not merely similar:
rawis aconst, unassigned between the two lines;not:
OPAQUE_ID_REis/^[A-Za-z0-9_-]{12,32}$/with nogand noyflag, so.test()keeps nolastIndexand the second call cannot disagree with the first. Asticky or global regex here would have made the guard load-bearing.
isExpandableFieldTypereads onlyfieldDef.typeand has no side effect that couldmove the answer.
So whenever line 367 returned, line 368 would have returned too. Nothing else reaches the
guarded arm:
resolveDisplayis local to the record mapper and called from exactly twosites (the org-key loop and the owner-key loop, both inside the legacy semantic heuristic).
All of them. Revert the source change, restore the dead guard, and every assertion in
this PR stays green. That is what "unreachable" means, and it is why the passing suite is
not the argument here. The argument is the subsumption above; the tests exist for the
other half — so that the deletion cannot be quietly upgraded into a behaviour change.
What the new pins actually discriminate is A vs B, and that was measured, not asserted:
Tests 2 failed | 2 passed (4)— exactly those twobuildExpandFieldswith a member-identical private table)$expandcounter-probe GREENTests 3 failed | 6 passed (9)— exactly those threeBoth ablations restored under
trap … EXIT INT TERMwith a cwd-independent command, bothmutations proved on disk by grepping the injected and the separately removed text with
the landing site printed and anchor uniqueness asserted, and both restores confirmed
byte-identical afterwards.
What changed
packages/plugin-kanban/src/ObjectKanban.tsx— the dead branch, the now-unusedconst def, and the now-unusedisExpandableFieldTypeimport are gone; the comment blockthat explained the removed read is replaced by one that explains why the surviving
suppression is untyped and names the pin that holds it.
packages/plugin-kanban/src/__tests__/resolveDisplay.opaqueId-6063.test.tsx(new) —four cases: an id-shaped value on a
textcolumn is suppressed; suppressed too when theobject declares no fields at all; suppressed on a
lookupcolumn (the outcome the deletedguard claimed, unchanged); and a positive control so the three absence assertions cannot be
satisfied by a board that renders no description at all.
packages/plugin-kanban/src/__tests__/expandableFamily.identity-5874.test.tsx— thisis the required consequence, not scope creep: #5874's identity pin was attributed to
resolveDisplay's read, and deleting the dead branch deletes that read, so two of itsassertions go red on this diff. They are re-anchored onto the face's live read,
buildExpandFields, and made per-field discriminating (each case types the other field outof the family, so neither spelling can be supplied by the other). Two things improve in the
process:
executably, that
buildExpandFieldsis the only reader in this render, which is thecondition under which the filter is equivalent to no filter (measured: every recorded
EXPANDABLE_FIELD_TYPES.hascall comes throughisExpandableFieldType ← buildExpandFields);user/treeand each carrying the undeclarablereference#5874 had to record as absent now exists. Themembership delta is not observable in this face's rendered output, but it is observable
on the wire: a
user-typed field reaches$expand, the same field typedtextdoes not.(Measured while writing it: the board fetches twice — once before the object schema
resolves, with no
$expandcomputable — so both cases wait for the post-schema fetch,otherwise the negative case would be green against a query that had not been built yet.)
Verification — all runs at
f7a86bc1e(final commit, clean tree)npx vitest run packages/plugin-kanban --maxWorkers=2(from the repo root, perobjectui#3378) →
Test Files 16 passed (16),Tests 92 passed (92). Count sanity-checkedagainst the target: 15 files / 86 tests on the merge base, +1 file and +6 tests here — not
apps/console's 22.pnpm --filter '@object-ui/plugin-kanban^...' build→ exit 0 (the closure built first, sothe type-check below reads rebuilt
dist/*.d.tsrather than stale ones).pnpm --filter @object-ui/plugin-kanban type-check→ exit 0, both configs(
tsc --noEmit && tsc -p tsconfig.test.json, script name echoed — not a zero-matchsilent pass). No errors to classify.
node scripts/check-changeset-presence.mjsverdict line, quoted:✅ 3 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s): .changeset/6063-kanban-resolvedisplay-dead-guard.md.Scored
patch— shipped source changed; behaviour did not. Nevermajor.node scripts/check-changeset-no-major.mjs,check-control-bytes.mjs,check-vi-mock-specifiers.mjs,pnpm check:phantom-deps,check:self-import,check:spec-symbols→ all exit 0 (each verdict line read from a redirected file, notthrough a pipe).
pnpm lintisturbo run lint= per-packageeslint ., so for the only package this diff touches the package's own run is thegate:
npx eslint .inpackages/plugin-kanban→ 31 files linted, 0 errors, 107warnings (all pre-existing
no-explicit-any/unused-var style warnings; the 4 added bythe new tests match the file they sit next to). Invariance: the repo has one root
eslint.config.jswith noproject:/projectService, i.e. no type-aware linting, sothis diff cannot move a verdict on any file it does not touch.
scripts/pm/check-half-states.mjsexits 3 in this container — "PREREQUISITE NOT MET …no reading at all" (no usable GitHub credential). Environmental, unrelated to this diff,
and reported as no reading rather than as green.
Left draft deliberately: not marked ready, not enqueued, no auto-merge.
Generated by Claude Code