feat(components): honour the declared breadcrumb-family keys — separator, maxItems, and header-bar crumb icons - #6721
Merged
Conversation
…er-bar crumb icons
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 29, 2026 01:37
This was referenced Aug 29, 2026
This was referenced Aug 29, 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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 #6646
Fixes #6645
Family branch: two cards, one branch, because both route through the shared
resolveIconand the sameBreadcrumbItemdeclaration. Verified ate1d51d4da.What landed
#6646 —
ui:breadcrumbnow readsseparatorandmaxItemsBreadcrumbSchemahas declared both since it shipped (packages/types/src/navigation.ts, mirrored inzod/navigation.zod.ts), andseparatoris additionally advertised on the component's own docs page. The renderer contained zero occurrences of either name.separatoris read, with the declared default honoured.??, not||: an authored empty string means "no visible separator" and must not be promoted to the default.maxItemsbounds the count of RENDERED crumbs. Over-long trails keep the first crumb and the lastmaxItems - 1, with shadcn'sBreadcrumbEllipsisbetween, so the current page is never the crumb that gets dropped; atmaxItems: 1there is no room for both ends and the current page is what stays. A value that cannot mean a count (absent, non-finite, below 1) is declined rather than coerced.The default-alignment direction, stated explicitly because it is the one judgement call here.
separatorcarries@default '/'in the declaration while the renderer fell through to shadcn'sChevronRight— declared default and actual render disagreed, and honouring only the authored value would have left the docs lying about the unauthored one. Two ways to align:schema.separator ?? '/') — chosen; or@defaultto describe the chevron — rejected:separatoris typedstring, so no honest@defaultcan name a component, which means that route deletes a documented default. That is a published-contract change, which the triage ruling routes to a maintainer, and the arm dispatched here is "implement the declaration".Consequence, and it is user-visible: every existing
ui:breadcrumbthat authors noseparatornow renders/where it rendered a chevron. That is what moved the changeset frompatch(the sibling repair, PR #6644) tominor.#6645 —
ui:header-barnow resolvescrumbs[].iconHeaderBarSchema.crumbsis typedBreadcrumbItem[], the same declarationBreadcrumbSchema.itemsuses, and the zod mirror does not merely declareicon— it describes it (.describe('Breadcrumb icon')), so any authoring surface reading zoddescribecan already offer the key. The renderer contained zero occurrences of the substringicon.resolveIcon, never a local normaliser (objectui#5993's lesson). Pinned from the outside by a row asserting thathomedraws lucide'sHouseglyph — that rename lives only in the shared resolver's map, so a localtoPascalCasecopy is red.BreadcrumbLabel, so all three of that helper's arms — the siblings quick-switch dropdown, the last crumb's page arm, and every earlier link arm — carry the glyph by construction. The dropdown arm is the shape this card could not inherit from PR fix(components): resolve ui:breadcrumb and ui:command child item icons through the record resolver #6644, becauseui:breadcrumbhas no such arm; it gets its own rows.LazyIconwould satisfy the first and diverge on exactly the names that matter.Anti-vacuity: red before, green after
Both suites were written and run against the merge-base renderers first. Full run recorded:
19 failed | 16 passed (35), the 16 being the harness controls and the rows that are green in both worlds by design.98188c284separator— declared@default '/'honouredrenders the declared @default,draws NO chevron glyph)separator— authored value readmaxItems— collapsemaxItems: 1, separators through the collapse, icons through the collapse)crumbs[].icon— link armcrumbs[].icon— page armcrumbs[].icon— siblings-dropdown armhometolucide-house), glyph-not-wordThe
separatordefault has its own assertion pair rather than one row: a repair that appended/while still drawing the chevron would pass "renders the declared default" and still show an author something the declaration never promised.Two negative rows were green in both worlds and are labelled as such rather than counted: "both consumers agree on a RETIRED spelling" (a uniform zero before the fix — the card's own framing) and "the dropdown's own chevron survives".
One instrument bug found and fixed mid-run, disclosed rather than quietly corrected: the first post-fix run was
3 failed, all three counting the elision's own list item as a crumb. The renderer was right; the test helper's "not a separator" filter was too loose. It now excludes both decorative kinds by the one attribute shadcn marks them with.An existing test's control had to move, and why
breadcrumb-item-icon.test.tsx(PR #6644) used the renderer's own separator chevron as the positive control on itssvg.lucide-*selector. Aligning the default separator removes that chevron, so the control would have gone red for a reason with nothing to do with icons. It now renders the shadcn primitive directly — still a positive control on the selector, and now green in both worlds by construction instead of by a renderer default that was free to change. Its prose says so.The duplicate
BreadcrumbSchema— verified, documented, NOT deletedThe triage asked the taker to handle this rather than treat it as trivia, with the fence "do not delete the un-exported one; verify first". Both halves done:
renderers/data-display/breadcrumb.tsxnavigation.ts:205— the oneindex.ts:272re-exports, and the only one carryingmaxItems✅packages/types/src/registry.ts:87,176navigation.ts(importedfrom './navigation.js') ✅zod/navigation.zod.ts:97— the only zod breadcrumb in the packagenavigation.ts✅data-display.ts:1623DataDisplaySchemauniondata-display.ts:1578, the narrow one — so it is still referenced and was not touched, exactly as fencedpackages/types/__tests__/zod-mirror-parity.test.ts:136,606,607data-display.ts— the wrong oneThe last row is a real find and is now #6720, filed as a sub-issue of #6349 (the 46-name collision card that already lists this row). The parity file's job is "the mirror accepts everything the TS declaration declares", and for both Breadcrumb pairs it compares the
navigation.zodmirror against thedata-displaydeclaration — somaxItems,icon,onClickandsiblingsare mirror-only keys that fall out of the comparison entirely and the pair reads clean about a declaration no consumer resolves.Not repaired here on purpose. Re-pointing those two import specifiers runs the comparison against four more declared keys, and whatever that reveals may need new entries in
KnownDrift/UnmirroredDeclared/RuntimeOnlyDeclared— writing new debt into governed ratchet ledgers is a ruling, not an execution step.renderers/data-display/breadcrumb.tsxgained a docblock naming which declaration it consumes, so the next reader does not have to re-derive it.One thing to look at: the icon-record gate gains a
header-barcensus entryFlagged prominently because #6645's triage carries a fence near it. The fence is "⛔ do not FIX this by teaching
scripts/check-lucide-icon-record-names.mjsto guess", with the stated ground that the gate "correctly does not censusheader-bar(its crumb names reach no resolver)". That ground expires with this PR — the crumb names now reachresolveIcon— and the gate's own header says a row's verdict "is a fact about a renderer, so it expires when that renderer is repaired".So this is not guessing and not the fix: the fix is the renderer. It is the same lawful move
context-menumade in objectui#6278 (whose entry says in so many words that it could not have been added before its repair landed), and the same move PR #6644 made forbreadcrumb. Without it the three icon names the new fixture authors would be unjudged, which is the hazard the gate exists for. It is explicitly NOT objectui#5992's blind spot, which is the gate censusing containers nobody read off a renderer.The header's part-2 table was re-measured rather than adjusted by arithmetic: an independent walk over
examples/schema-catalog/reproduces the seven existing rows identically (tree-view 30, command 9, button-group 8, context-menu 4, timeline 4, breadcrumb 3, dropdown-menu 3 = 61) and finds header-bar 3, for 64 across eight containers.Reversing it is one hunk (the entry plus its header paragraph) if a maintainer reads the fence more strictly; nothing else in this PR depends on it.
Fixtures and docs, so neither key gets re-filed as "declared but unenforced"
components-data-display-breadcrumb/custom-separatorand.../collapsed-trailauthor the two keys;components-navigation-header-bar/crumbs-with-iconsauthors three crumb icons.catalog-meta.jsoncarries curated titles/descriptions for all three;index.tsregenerated by the script that owns it, additions only (regenerate:checkclean).maxItemsrow its interface block never had.BreadcrumbItem, notBreadcrumb) withiconandsiblingsadded.packages/typesis untouched by this PR: every key here was already declared, and the only thing missing was renderers that read them.Verification
All heavy work ran under the shared verify lock; verdicts quoted from what each command printed, not from a shell status.
pnpm exec vitest run packages/components/src/__tests__/{breadcrumb-separator-max-items,header-bar-crumb-icon,breadcrumb-item-icon}.test.tsxon the merge-base:Tests 19 failed | 16 passed (35).e1d51d4da(working tree clean at commit, so the tested tree is HEAD's tree),VERDICT command-exit 0over an&&chain of:pnpm --filter @object-ui/components type-check,pnpm check:icon-record-names, andpnpm exec vitest runover the gate's own suite, all ofexamples/schema-catalog/test/, the three component suites andsnapshot-critical—Test Files 19 passed (19) · Tests 1952 passed (1952).OK lucide icon names: 182 authored/declared names reaching 7 record-reading resolvers are live icons keys.VERDICT command-exit 0: dependency-closure build,pnpm --filter @object-ui/example-schema-catalog type-check,pnpm type-check:scripts,regenerate:check("index.ts is up to date (431 entries)"),check:control-bytes("scanned 5577 tracked text file(s)"),check:doc-types("Every documented component type is registered"),check:doc-fences.packages/components/tsconfig.test.jsonincludessrc/**/*.test.tsx, and both new files sit insrc/__tests__/.Two declared narrowings, so they are not read as coverage:
pnpm lintwas run over the changed files, not the repo.eslint --no-inline-config --format jsonover the 7 lintable changed paths: 7 files, 0 errors, 11 warnings, every warning on a pre-existing file being a construct that is verbatim present at98188c284(the unusedrenderChildrenimport,[key: string]: any,React.ComponentType(any)in the sibling suite,ExampleMetain the generated index). The new suites' warnings are the same already-accepted construct. The narrowing is sound becauseeslint.config.jsenables no type-aware linting (noparserOptions.project, noprojectService), so this diff cannot move the verdict on any file it does not touch. One real ERROR was found and fixed this way:react-hooks/static-componentson a first-draftCrumbIconhelper, which is why the resolution is inline in the map callback — the same shapebreadcrumb.tsxuses.pnpm check:doc-snippetswas NOT run. It exits 2 withPRECONDITION NOT METuntil 21 package closures are built, which is not a red and not a green — it is NOT MEASURED. It compiles onlyts/tsx/typescriptfences; every fence this PR touches isplaintext, andcheck:doc-fencespassing over all 223 documents is the independent confirmation that the classification is right. CI runs it regardless.Out of scope, filed not fixed
zod-mirror-paritypairsnavigation.zod's two Breadcrumb mirrors with thedata-displaydeclaration, so their green says nothing about the mirror it names #6720 —zod-mirror-paritypairs bothnavigation.zodBreadcrumb mirrors against thedata-displaydeclaration. Sub-issue of 46 exported type names carry a second authority — 42 of them are named by no family card #6349, unassigned.Generated by Claude Code