Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell): resolve the Home item-type label through one shared helper - #6334
Merged
Merged
Conversation
…elper The rail (`HomeRail.HomeContinue`), `RecentApps` and `StarredApps` all render a label for the same item kind through the `home.recentApps.itemType.*` key namespace, and all three spelled the lookup themselves. They drifted: the two card surfaces fell back to `capitalizeFirst(type)` while the rail fell back to the bare `type`, so a kind with no key rendered `Report` on the cards and `report` in the rail — on the same screen. Converge on the capitalising fallback the two card surfaces already used, and remove the duplication that allowed the drift: one `recentItemTypeLabel` helper now owns the key spelling and the fallback for all three call sites. The pin asserts AGREEMENT between the three surfaces for a kind with no translation key, because the defect is disagreement between consumers rather than any one component's output. It uses a synthetic kind deliberately: every member of the union today has a key, so a fixture built on a real member takes the keyed path and never reaches the fallback at all.
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 Aug 25, 2026
os-support-ai
marked this pull request as ready for review
August 25, 2026 13:45
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#6165
Three Home surfaces render a label for the same item kind through the
home.recentApps.itemType.*key namespace, and each spelled the lookup itself. They had drifted: the two card surfaces fell back tocapitalizeFirst(type)while the rail fell back to the baretype, so a kind with no key renderedReporton the cards andreportin the rail — on the same screen.All three now resolve through one
recentItemTypeLabelhelper.Premise re-derivation (every PM assumption checked against
main)HomeRail.tsx:251,RecentApps.tsx:55,StarredApps.tsx:61)mainadvanced several times, but none of it touched these files.grepforhome.recentApps.itemTypefinds exactly these three call sites.RecentItem['type']unioncapitalizeFirstis one shared helper, not two copies../../utils/index.js. (A second, unrelated copy exists atapps/console/src/utils.ts:32; neither sibling uses it.)report+metadata, so the fallback is dormantmain— all six members are keyed inen.ts.The two-union finding matters for the fix's shape. The rail and
RecentAppstakeRecentItem['type'](object|dashboard|page|report|record|**metadata**);StarredAppstakesFavoriteItem['type'](object|dashboard|page|report|record|**nav**). They share the key namespace, not the type —StarredAppsreuses therecentApps.itemType.*keys deliberately, and says so in a comment. So the helper's parameter isstring, not a union: narrowing it to either one would reject a legitimate caller. That is documented at the helper.A side-effect worth recording:
navhas nohome.recentApps.itemType.navkey, so on the favorite side the fallback is not fully dormant today.navitems are documented as excluded from Starred, andStarredAppsis a public export whose caller supplies the items, so whether the path is reachable depends on the host. Not fixed here — out of this card's scope.Fourth-consumer census:
views/metadata-admin/StudioHomePage.tsxalso types onRecentItem['type'], but only forRECENT_ICONS; it renders no type label and usestranslateMetadataTypefor a different concern. So the count really is three.Why a helper rather than a one-line change
The ruling permitted either, conditional on the helper being the honest shape. It is: three identical expressions, one key namespace, one directory, and the card exists because they drifted. Re-spelling the odd one out fixes this instance; removing the duplication removes the recurrence. The two card surfaces' rendered output is unchanged — they keep
capitalizeFirst, which is what the ruling protects; only the expression's home moves. No fourth spelling, and nothing outsideconsole/home/is touched. The helper is deliberately not added toconsole/home/index.ts— it is internal, and there is no call for widening the package's public surface.The test is a parity pin, not a ghost assertion
Every union member shipping today has a key, so a fixture built on a real member takes the keyed path, never reaches the fallback, and passes identically before and after — the easy and worthless test here. The pin therefore uses a synthetic unkeyed kind (
playbook), and asserts agreement between consumers rather than one component's output:playbookrenders one identical string across the rail,RecentAppsandStarredApps(all three pinned, not the two the ruling required), and that string is the capitalising form, pinning the direction of the convergence.报表, whichcapitalizeFirstcould never produce — so a green control means the keyed path genuinely ran, rather than the fallback quietly answering.All three surfaces are driven by one translator: the rail takes
tas a prop while the cards pull it from the hook, so the test reads it from that same hook and passes it down. Otherwise it would be comparing two unrelated renders.Ablation — proven red before
Committed first, then reverted only
HomeRail's fallback expression, restore undertrap … EXIT INT TERMwith absolute paths (REPO_ROOTviagit rev-parse --show-toplevel).Mutation proven on disk in both directions (this is a replacement, so both genuinely exist — before: fixed-form 1 / reverted-form 0; after: fixed-form 0 / reverted-form 1), plus a blob-hash change (
18c1e559…→e44b3acb…).Exactly the predicted direction and count: the rail read
playbookwhile the cards readPlaybook. Restore verified —git diff HEAD --statempty and the blob hash back to18c1e559….No rebuild was needed for either leg, and that is measured rather than assumed:
vitest.config.mtsaliases every@object-ui/*topackages/*/src, and the test imports its subjects relatively, so nodist/sits on the path.Verification — all at
2510207aa(final commit, tree clean)Test Files 1 passed (1)·Tests 3 passed (3)Test Files 34 passed (34)·Tests 429 passed (429)pnpm type-check(@object-ui/app-shell)pnpm lint(@object-ui/app-shell)✖ 2679 problems (0 errors, 2679 warnings)— exit 0check-changeset-presence✅ … declares 1 changeset(s)check-changeset-no-major✅ No changeset declares a major bump.check-control-bytes✅ OK (scanned 5208 tracked text file(s))check-vi-mock-specifiers✅ OK (… 0 non-static …)check:i18n-keyscheck:i18n-driftNo en value changed in this range.check:i18n-dead-keys,check:self-import,check-shell-escape-residueGate set derived by enumerating each CI job's own step list under
.github/workflows/, not from memory.Declared narrowing (the whole-package
app-shellsuite is ~784 s and would be killed by the container's ~10 min foreground cap):grep -rlnfor tests importing or mentioning the changed modules, not my guess about what is affected. It is a superset: it matches any test namingHomePage/console/homeat all.eslint.config.jssets noparserOptions.project, so linting is not type-aware and this diff cannot move the verdict on any untouched file. The full farm runs in CI regardless.Two coverage facts measured with
--listFilesrather than assumed:app-shell's basetsconfig.jsonexcludes the new test (0 hits), whiletsconfig.test.json— the second program intype-check— includes it. The shipped helper is in both. So "type-check is clean" genuinely covers the new test.recentItemTypeLabel.tsadds one@typescript-eslint/no-explicit-anywarning, from theTFnalias it carries over verbatim fromHomeRail.tsx:26. Kept identical to the line it replaces rather than diverging; the gate is green (warnings, not errors).Generated by Claude Code