Uh oh!
There was an error while loading. Please reload this page.
refactor(components,plugin-list): draw the list load failure with DataErrorState, which now takes DataEmptyState's icon props - #7430
Merged
os-project-manager merged 1 commit intoSep 3, 2026
Conversation
…ataErrorState, which now takes the icon props DataEmptyState had `ListView` rendered its load FAILURE through `DataEmptyState` — the component named for the *empty* case — passing it a destructive icon, error copy and a retry control, while `DataErrorState` sat in the same file with the same layout, `role="alert"` already declared, and no consumer anywhere in the repo. It was not a drop-in replacement: it hardcoded its glyph, so the panel that must draw a network outage differently from a permission denial could only get an icon from the wrong component. `DataErrorState` gains three additive optional props — `icon`, `showIcon`, `iconWrapperClassName` — mirrored from `DataEmptyState` in the same file: same names, same types, same default semantics, including `iconWrapperClassName` REPLACING the wrapper's default class rather than merging with it. The only intended difference is the class the `??` falls back to, which stays this component's own destructive square. `illustration` and `action` are deliberately not mirrored. The migration moves no pixels. The call site passes the same glyph through the new `icon`, the same `iconWrapperClassName="mb-3"`, the same title, and the same copy through `message`; its retry `<Button>` moves from `action` to `children`, which renders at the identical position and keeps both its `data-testid` and its RotateCw glyph. The whole rendered delta is two `data-slot` renames — `data-empty-state` → `data-error-state` on the panel root, and the icon wrapper's to match. `role="alert"` (objectui#7132) is untouched. Pins: component identity and every surviving affordance are pinned in `ListView.errorStateComponent-7143`, and the mirrored prop semantics in `data-error-state-icon-props-7143`, which asserts each arm twice — once per component — so "mirrored" is measured rather than restated. Three stale comments that described the borrow as current are corrected in the same commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMrWaQw3XS5DxTHxp4yRyC
os-project-manager
commented
Sep 3, 2026
Collaborator
Handoff to the project director seat — ⛔ NOT reviewed or accepted by this seatPM note from the Clause ② is engaged (public props surface of a published component). Per item 3 of the ruling on #7143 (comment 5494762960, 2026-09-01, batch #27, maintainer verbatim 「同意」), the
|
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
|
This was referenced Sep 3, 2026
os-project-manager
marked this pull request as ready for review
September 3, 2026 04:46
Uh oh!
There was an error while loading. Please reload this page.
os-project-manager
deleted the
claude/issue-7143-dataerrorstate-migration
branch
September 3, 2026 05:03
os-sales pushed a commit
that referenced
this pull request
Sep 3, 2026
…sed-import-gate Resolves one content conflict in packages/plugin-list/src/ListView.tsx, on the `@object-ui/components` import line. Both sides rewrote that single line: - main (#7430) added `DataErrorState`, so the list load failure is drawn by the component named for it. - this branch removed `Select, SelectContent, SelectItem, SelectTrigger, SelectValue` as unused. The resolution is the union: main's `DataErrorState` kept, this branch's five removals kept. This branch's other two edits to the file (`Ruler`, `AlignJustify` from lucide-react; `useObjectTranslation` from `@object-ui/i18n`) were untouched by main and merged cleanly. Re-measured on the MERGED content, since main added new code to this file and new code can create a new use: all eight removed names remain unused. Seven have zero word-boundary occurrences of any kind; `useObjectTranslation` has exactly one, at line 706, inside the JSDoc block spanning lines 705-712 -- prose, not a call site. The removal population is unchanged by the merge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aCUUSwWefnbCJ4Xk1vqQW
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#7143
ListViewrendered its load failure throughDataEmptyState— the component namedfor the empty case — while
DataErrorStatesat in the same file, with the same layoutand
role="alert"already declared, and no consumer anywhere in the repo. This migratesthe panel onto the component named for what it is, and gives that component the three
icon props it was missing.
Implemented against the ruling in comment 5494762960 (director seat, 2026-09-01,
maintainer verbatim 「同意」, decision batch #27) — not against the issue body, which
ends "Suggested shape, not a decision" and is superseded. Item 1 of that ruling, quoted
verbatim and untranslated:
The documentation-note alternative is excluded by item 2 and was not considered.
Clause 2 is engaged, and this PR is NOT self-reviewed
This widens the public props surface of a published component. Item 3 of the ruling moves
the
CONTRACT_REVIEW_TIERrequirement onto the project director seat: the shape ispinned by item 1, this lane implements it mechanically, and the PR carries
needs:contract-review. It is a draft, auto-merge is not enabled and it has notbeen enqueued. It lands after the director seat reviews — same path as objectstack#13897.
What was mirrored, and from where
All three props are copied from
DataEmptyStatein the same file(
packages/components/src/custom/view-states.tsx) — same names, same types, same defaultsemantics. Line references are against the file as it stands in this PR:
DataErrorStateDataEmptyStateicon(L182)React.ReactNode, optional; falls back to the component's own glyphshowIcon(L188, default at L235)boolean, defaults totrueiconWrapperClassName(L195, resolved at L258)string, optionalThe semantics that could have diverged silently is
iconWrapperClassName.DataEmptyStateresolves it with??(L157), so it REPLACES the wrapper's defaultclass rather than merging with it — which makes
""a meaningful value that strips thestyling.
DataErrorStatedoes the same at L258. Acn(default, override)reading wouldhave type-checked, looked right, and quietly kept
bg-destructive/10underneath everyoverride — including
plugin-list'smb-3, which exists precisely to remove that box.The only intended difference between the two resolutions is the class the
??falls backto: the empty state's
bg-mutedsquare, and the error state's ownbg-destructive/10square that it has always drawn.
DataEmptyState's other two props were deliberately not mirrored.illustration—an empty state's product-feel hero image has no load-failure analogue.
action—DataErrorStatealready spells its affordanceonRetry/retryLabel, andchildrencovers a call site that needs to render its own control, so an
actionprop would be asecond spelling of something the component has.
One non-prop addition, called out rather than folded in
The icon wrapper now carries
data-slot="data-error-state-icon", mirroring the emptystate's
data-empty-state-icon(L155). It is not one of the three props the rulingnames, so it is flagged here for the reviewer rather than buried:
iconWrapperClassNamenow governs has no name — it cannotbe selected by a test or by a host stylesheet, only by DOM position;
DataEmptyStatewould drop an identifierrather than rename it.
It is one line and strikes cleanly if the director seat would rather not have it.
The visual delta — what the rendered output becomes
DataErrorStatehardcoded its icon, which is why #7132 fenced this swap out of its ownscope: it is a visual change, not a rename. Measured, the visual change is nothing.
Both primitives already carried the identical root class string
(
flex flex-col items-center justify-center gap-3 p-6 text-center), the identical titleh3and the identical bodyp; the call site passes the same glyph through the newicon, the sameiconWrapperClassName="mb-3", the same title, and the same copy throughmessage— which is the error state's spelling of the empty state'sdescriptionandrenders the identical
pelement with the identical classes. The retryButtonmovesfrom
actiontochildren, which renders at the same position and keeps both itsdata-testid="list-error-retry"and itsRotateCwglyph.The entire rendered delta is two attribute renames:
data-slot="data-empty-state"data-slot="data-error-state"data-slot="data-empty-state-icon"data-slot="data-error-state-icon"Every class on every node, and the glyphs themselves, are unchanged.
role="alert"isuntouched, as the ruling requires — it stays spelled out at the call site even though it
is now the primitive's own default, because that property is pinned against this call
site and must hold whichever component draws it.
Nothing in this repo styles or selects on either slot value — no CSS rule and no test read
them, verified by grep across
*.css,*.ts,*.tsx,*.mdand*.mdx. A hostapplication targeting
[data-slot="data-empty-state"]to reach this panel is the onlyway to observe the change, and it should be reading
data-error-statenow.So the visual review the ruling asks for has a small and specific question: is the panel
that a 403 or an outage produces still right, given that it is now identified as an error
state and nothing else about it moved.
Eager-closure budget — measured, both sides
packages/componentslands in theui-componentseager chunk, the tightest of the threebudgeted chunks. Both readings come from a real console build
(
pnpm turbo run build --filter='./packages/*'thenpnpm --filter @object-ui/console build)followed by
node scripts/check-eager-closure-budget.mjs, exit code captured before anypipe.
42c129b64)ui-componentsExact bytes for
ui-components, read out ofeager-closure.jsonrather than from therounded table: 396,598 before, 396,762 after — +164 bytes gzipped, against a
ceiling of 399,000. Headroom goes from 2,402 bytes to 2,238.
frameworkandvendor-objectstackare byte-identical on both sides (523,823 and 948,329), and theaggregate moves +149 bytes. The gate exits 0 on all four ceilings. No ceiling was
raised, no baseline re-pinned, no gate weakened.
Worth stating plainly rather than assuming: a props addition is not free, but it is 164
bytes — 0.02x of the 89 KB regression this gate exists to catch — in the tightest of the
three budgeted chunks.
Pins
The card named two pin files; the tree was re-derived rather than trusted, and the
data-testid="list-error-state"selector turns out to be carried through the migrationunchanged — it is passed to
DataErrorStateexactly as it was toDataEmptyState, soall four existing selector pins keep passing untouched:
packages/plugin-list/src/__tests__/ListView.loadErrorKind.test.tsx(17 arms)packages/plugin-list/src/__tests__/ListView.elementDataSource.test.tsxpackages/plugin-list/src/__tests__/ListView.emptyVsErrorRole-7132.test.tsxpackages/app-shell/src/views/objectListApiDisabled-4408.test.tsx— not named on thecard, found by re-deriving; it selects the same test id from a different package.
What did need updating was prose, in three places that described the borrow as a
current fact. Left alone, the next reader checks the claim, finds it false, and distrusts
the pin around it:
ListView.emptyVsErrorRole-7132.test.tsx— its header said the error branch "borrowsDataEmptyStatepurely for its layout". Neither arm changed; the suite still pinsroleat the call site, which is what makes it survive the swap.view-states.tsx—DataEmptyState'sroledocblock cited the borrow as the livereason the default must stay overridable.
data-empty-state-role-7132.test.tsx— one arm title named "the load-error borrow".Two new pin files carry the migration itself:
packages/plugin-list/src/__tests__/ListView.errorStateComponent-7143.test.tsx—component identity, plus every affordance that had to survive the swap (role, test id,
error kind, the per-kind glyph in its stripped wrapper, the retry button, the
enable-block denial's absence of one) and a control arm holding the genuine empty
branch at
DataEmptyState/role="status", so a change that swapped the wrong panelcannot read as a pass.
packages/components/src/__tests__/data-error-state-icon-props-7143.test.tsx— themirrored semantics, asserted twice per behaviour, once per component, so "mirrored"
is a measurement rather than a restatement of the new code. The
iconWrapperClassNamearms assert the resolved class by exact value, because a merging implementation
would also satisfy "contains
mb-3".Reverse verification
Both new pin files were run against the base commit by reverting the two source files to
42c129b64(the mutation confirmed on disk by grep count, not by an editor exit code;restore by
git checkout HEAD -- ..., proved byte-identical viagit hash-objectagainstthe HEAD blobs with
git diff HEADempty). No rebuild is involved: Vitest aliases@object-ui/componentstopackages/components/src, and the component suite imports thefile by relative path, so nothing here resolves through
dist/.Both new pin files run RED there and GREEN here — measured, not asserted:
Test Files 7 passed (7)·Tests 57 passed (57)42c129b64, the 2 new pin filesTests 8 failed, 7 passed (15)The 8 red arms are exactly the ones that read the migration: panel identity (twice), the
icon wrapper, the three prop behaviours, and the DEFAULTS arm. The 7 green ones are the
deliberate controls — the three
DataEmptyStatemirror halves, the role/test-id arm, theretry arm, the untouched-surface arm and the empty-branch control — which is the shape a
migration pin should have: identity assertions that fail, wrapped in affordance assertions
that do not.
That measurement found and fixed a real defect in this PR's own test: the
showIcon={false}arm originally asserted only that the named wrapper was absent, andit passed on the base — where the selector does not exist either, so "no wrapper" and "no
such name" were the same reading and the arm could not fail. It now names the glyph too.
Gates
Exit codes captured before any pipe; verdicts quoted from what each gate printed.
Test Files 7 passed (7)/Tests 57 passed (57)node scripts/check-eager-closure-budget.mjsnode scripts/check-control-bytes.mjscheck-control-bytes: OK (scanned 6115 tracked text file(s); skipped 85 binary).node scripts/check-changeset-presence.mjs4 source file(s) of 2 released package(s) changed, and this change declares 1 changeset(s)node scripts/check-changeset-no-major.mjsNo changeset declares a major bump.node scripts/check-changeset-fixed.mjsAll workspace packages are in the changeset fixed group.pnpm exec eslint . --no-inline-configmain(the root scan is stricter than CI's per-packageturbo run lint)pnpm --filter @object-ui/components --filter @object-ui/plugin-list run type-checkA control-byte self-scan over the touched files
(
grep -naP '[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]') also returned no matches.Type-check needs the dependency closure built first, and this is worth recording
because the failure looks exactly like a real bug. Run before
@object-ui/componentswasrebuilt,
plugin-list's type-check reported— which reads as "the new props are wrong" and is in fact "
plugin-listresolves@object-ui/componentsthrough itsdist/*.d.ts, and thatdistwas still the baselinebuild". After
pnpm turbo run build --filter=@object-ui/components --filter=@object-ui/plugin-listthe same command exits 0. CI builds packages before type-checking, so it never sees this;
a local run that skips the rebuild will.
The new test files are genuinely type-checked, not silently excluded: both packages chain
tsc -p tsconfig.test.jsonfrom theirtype-checkscript, and--listFilesconfirmseach new file is in its project's file list.
Changeset
.changeset/7143-data-error-state-migration.md, minor for both packages.@object-ui/components— minor, the repo's standing level for an additive optionalprops widening on a published component. Same call as
6158-radio-group-orientation(a prop the renderer began honouring) and
7188-components-data-table-pending-row(anew prop handed to a host editor);
patchis used here for fixes that change nosurface.
majoris never declared in this repo — the 39-packagefixedgroup wouldcarry the whole release with it.
@object-ui/plugin-list— minor as well, and not merely by inheritance: the twodata-slotrenames are a change to rendered output that a host stylesheet can observe,and the repo's convention is that its own contract changes are declared
minorwith thesemantics spelled out in the body.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EMrWaQw3XS5DxTHxp4yRyC
Generated by Claude Code