Uh oh!
There was an error while loading. Please reload this page.
refactor(layout): remove the unread logo key from AppShellBranding - #5366
Merged
Conversation
…4818) `AppShellBranding.logo` was declared but never read. `useAppShellBranding` applies only `primaryColor` / `accentColor` / `favicon` / `title` — its effect dependency array does not even list `logo` — and `AppShell` installs no context provider, so the key's doc comment ("Logo URL — passed to sidebar/navbar via context") named a mechanism that does not exist in the code. Three call sites were feeding the key a value that was silently discarded; all three go with it: - packages/layout/src/AppSchemaRenderer.tsx logo: schema.logo - packages/app-shell/src/layout/ConsoleLayout.tsx logo: activeApp.branding.logo - apps/console/src/hooks/useBranding.ts logo: app.branding.logo The card named only the third; the first fails loud under tsc, and the second was unnamed anywhere and is the reason this diff reaches packages/app-shell. Nothing rendering-visible changes. The two real logo paths never went through AppShellBranding and are untouched: AppSchemaRenderer renders the app schema's top-level `logo` directly in its default sidebar header, and AppSidebar renders the app schema's nested `branding.logo` directly. Ruling: maintainer, 2026-08-19, over the objectui decision-inbox review — option 1, remove. Building a real branding context (option 2) was rejected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE
This was referenced Aug 20, 2026
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
os-support-ai
marked this pull request as ready for review
August 20, 2026 01:53
Uh oh!
There was an error while loading. Please reload this page.
os-sales pushed a commit
that referenced
this pull request
Aug 22, 2026
`apps/console/src/hooks/useBranding.ts` was a `@deprecated` thin wrapper around `useAppShellBranding`, with no caller anywhere in the repo. The migration its own deprecation note asked for had already happened: `ConsoleLayout` passes `branding` straight to `AppShell`, and nothing was left behind to call the wrapper. It was not free to keep. PR #5366 had to edit this file twice — the feed line and its local `AppBranding` interface — purely to keep a file nobody calls compiling. The hook was the only file in the console's hooks directory, so the directory goes with it. `skills/objectui/guides/console-development.md` stated both coordinates in backtick code spans, which `scripts/check-skills-paths.mjs` resolves against disk; both are updated, and the caveat about the first row's differing prefix is dropped because every remaining hook now lives in `@object-ui/app-shell`. Fixes#5368
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
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#4818
Maintainer ruling, 2026-08-19, over the 27-card objectui decision-inbox review (comment 5339695386), verbatim: 「全部接受」 — option 1, remove. Option 2 (build a real branding context) was rejected as a permanent provider obligation with zero demand.
What was removed
AppShellBranding.logowas declared but never read.useAppShellBranding(packages/layout/src/AppShell.tsx) applies onlyprimaryColor,accentColor,faviconandtitle; its effect dependency array does not listlogoat all.AppShellinstalls no context provider, so the key's doc comment — "Logo URL — passed to sidebar/navbar via context" — named a mechanism that does not exist in the code. That comment is deleted with the key.Six deleted lines, no additions, across four files plus a changeset.
Three feed lines, not one
The card named one feeder. There are three, and each one was handing the key a real value that was silently discarded:
apps/console/src/hooks/useBranding.ts:27—logo: app.branding.logopackages/layout/src/AppSchemaRenderer.tsx:582—logo: schema.logopackages/app-shell/src/layout/ConsoleLayout.tsx:174—logo: activeApp.branding.logoThe third is the only reason this diff reaches
packages/app-shell, which is outside the file surface the card and the dispatch drew. It is not optional: deleting the key makes that line a hardtscerror (evidence below), so leaving it would ship a red typecheck. It is the same defect class, the same mechanical one-line deletion as its two siblings, in a file no in-flight worktree or open PR touches, and it adds no new gate.apps/console/src/hooks/useBranding.tsalso loses the matchinglogo?: stringfrom its localAppBrandinginterface — that hook has zero callers today.The premise check the dispatch asked for — measured, and it holds
The dispatch flagged one thing to verify rather than assume:
AppSchemaRendererforwardedschema.logo, a top-level app-schema key, whileAppSidebar.tsx:194readsactiveApp?.branding?.logo, a nested one — so was the forward actually dead?Measured on
origin/mainbdf8cf76e: they are not the same key and not the same place — but that turns out not to matter, because neither logo render site goes throughAppShellBrandingat all. Both are untouched by this PR:packages/layout/src/AppSchemaRenderer.tsx:385—resolveIcon(schema.logo)— and:397-399, animgwhosesrcisschema.logo, in the default sidebar header. This renders the top-level key directly, in the very same file that built the branding object. The forward at:582was a second, parallel, dead route for a value the file was already rendering itself.packages/app-shell/src/layout/AppSidebar.tsx:194—activeApp?.branding?.logo, read directly off the app schema.So the ruling's premise — "the logo already renders correctly today via the real path" — is true for both halves. The roll-back clause does not trigger.
Reverse verification
Stated per leg, because a deletion PR is exactly the shape that passes vacuously.
tschas a build artifact between the edit and the result for the two cross-package legs: the roottsconfig.jsonhas nopathsentry for@object-ui/layout, so consumers resolve it throughnode_modulestopackages/layout/dist/AppShell.d.ts. Every leg below rebuilt that artifact first and proved the marker's presence or absence in it. Vitest is the opposite:vitest.config.mts:263aliases@object-ui/layouttopackages/layout/src, so no artifact sits between the edit and the tests.Leg 1 — baseline,
logopresent in the built.d.ts. After building the dependency closure,packages/layout/dist/AppShell.d.ts:14carriedlogo?: string;. All three consumer packages type-checked clean, i.e. writinglogocompiled:Leg 2 — key deleted, feed lines still in place; rebuild proved the marker gone (
grep -c logo packages/layout/dist/AppShell.d.ts→0). Each of the three consumers then failed, naming exactly one site:That is the answer to "show that a consumer writing
logois now a compile error" — demonstrated by three real consumers, two of them resolving through the published.d.tsexactly as an external consumer would.Leg 3 — feed lines deleted, artifacts rebuilt, marker re-confirmed absent. All three green again (
EXIT=0).Nothing rendering-visible changed. The removed key had no reader: no branch of
useAppShellBrandingtouched it, andAppShellexposes no context for anything downstream to read it from. The two live render sites listed above still sit at the same lines.Tests
Run from the repo root with path filters, never
pnpm --filter pkg test— 17 packages here own a standalonevitest.config.tsthat does not re-export the root config, so a package-scoped run executes a different config than CI.186 files, 1709 tests, zero failures. The second command's paths cover every
packages/app-shelltest that mentionsConsoleLayout,AppShellBrandingorbranding, plus theAppSidebarsuites that guard the real logo path. The remainder of that package's 456 files is left to CI, which runs the whole farm once anyway.Gates
Union re-run after the final commit, at
49dee42ad:check:doc-snippetsis the load-bearing one for a published-type deletion: it compiles documentation snippets against the built types and found no snippet writinglogoon anAppShellBrandingliteral.Docs
packages/layout/README.md:82— re-read, unchanged. Itsbrandingrow already lists exactly the four keys really read, so it gains no stale mention.content/docs/guide/layout.md:82— same row, same four keys, likewise needs nothing.content/docs/layout/app-shell.mdx— deliberately not touched. Its onlyAppShellBrandingmention is a "see AppShellBranding" pointer, and its logo prose is about thenavbarslot, an unrelatedReactNode. The app-shell.mdx prose is finding: app-shell.mdx 的 Features/Styling 散文完全不提 branding 与 rightRail —— 键表已补齐(#4808/PR4829),散文半截待 #4818 裁定 logo 后写 #4830's job and remains open; writing it here would be a file-surface breach.Versioning
minorfor@object-ui/layout,@object-ui/app-shelland@object-ui/console— nevermajor. Maintainer, 2026-08-19, verbatim: 「objectui ui 的 major 是要跟着 objectstack 走的,不可以自己发 major」. The changeset body names the removed published optional key and the real replacement path.Generated by Claude Code