Skip to content

finding: AppShellBranding.logo 无任何读取方,注释「passed to sidebar/navbar via context」是假的(AppShell 不提供该 context) #4818

Description

@yinlianghui

发现于 #4793 的实施,回传立单。未认领。observation-class:今天没有用户会撞上,因为真正渲染 logo 的是另一条路径(见下),所以打 finding 标、不挂 pm:queue,严重度交 PM 分诊。

事实(基线 d44279598 实测)

packages/layout/src/AppShell.tsx:12-23 声明:

export interface AppShellBranding {
  primaryColor?: string;
  accentColor?: string;
  favicon?: string;
  /** Logo URL — passed to sidebar/navbar via context */
  logo?: string;
  title?: string;
}

useAppShellBranding(:122-231)是唯一消费 AppShellBranding 的地方,它实际读的只有 primaryColor / accentColor / favicon / title —— 连 effect 的依赖数组(:230)都写成 [branding?.primaryColor, branding?.accentColor, branding?.favicon, title],logo 不在其中。AppShell 本体(:233-265)也没有任何 context provider,所以注释里那句 「passed to sidebar/navbar via context」在代码里不存在。

且已经有调用方在喂它:apps/console/src/hooks/useBranding.ts:27 明确传了 logo: app.branding.logo 进 useAppShellBranding —— 那个值今天被静默丢弃。

为什么说「今天没人撞上」

侧栏里那个 logo 确实会显示,但它走的是另一条完全独立的路:packages/app-shell/src/layout/AppSidebar.tsx:194 直接从 app schema 读 activeApp?.branding?.logo,不经过 AppShellBranding。所以现象上「logo 正常」,坏掉的只是 AppShellBranding.logo 这个声明面和它的注释 —— 典型的 declared-but-unenforced 漂移。

可选修法(不预设,两条都不是本单要做的)

  1. 删(ADR-0049 enforce-or-remove 的 remove 侧):AppShellBranding 去掉 logo,顺带清掉 useBranding.ts:27 那行喂值;真实入口是 app schema 的 branding.logo。
  2. 兑现:给 AppShell 真加一个 branding context,让 sidebar/navbar 插槽能读到 —— 那是产品增强,得单独判。

无论走哪条,那行注释都必须跟着改:它现在描述的是一个不存在的机制,比缺注释更坏。

参考


Generated by Claude Code

Activity

  1. os-support-ai commented on Aug 18, 2026

    @os-support-ai
    Collaborator

    Triage — first-touch grading (triage seat, session session_01KTguWpd7WNGYTGmSNY31RL): escalated to needs-user-decision, type Bug (the declared surface and its comment describe a mechanism that does not exist, and a real feeder's value is silently dropped).

    Why this is the inbox and not the queue: the two fixes genuinely fork — (1) remove logo from AppShellBranding (+ the useBranding.ts:27 feed line, + the false comment) vs (2) actually build the branding context so sidebar/navbar can read it. Option 1 shrinks a published interface; option 2 is a product enhancement — both sides sit on the manual floor, and #4830 (app-shell.mdx prose) is explicitly waiting on this ruling.

    Four-prong analysis:

    • Platform long-term coherence: two independent logo paths (AppShellBranding vs app-schema branding.logo read directly by AppSidebar) is one path too many; removal converges on the single real one.
    • Measured business pull: zero — the sidebar logo renders correctly today via AppSidebar.tsx:194; no user-visible defect, no consumer of the declared key.
    • AI-agent error-resistance: a key that types cleanly, is fed by real code, and silently does nothing is exactly the declare-without-enforce trap; removal makes the false affordance unwritable.
    • Startup scope discipline: remove beats declare-and-maintain; option 2 creates a permanent context-provider obligation with no demand behind it.

    Recommendation: option 1 (remove), all four prongs align — but the published-surface deletion is yours to call. Ruling here also unblocks #4830.


    Generated by Claude Code

  2. os-support-ai commented on Aug 20, 2026

    @os-support-ai
    Collaborator

    Claim: PM loop round 18
    Session: session_01RV6yuVCxymHYE16PL9vQkE
    Branch: claude/issue-4818-appshellbranding-logo
    Worktree: objectui-issue-4818
    Domain: repo:objectui (execution seat)
    File surface: packages/layout/src/AppShell.tsx, packages/layout/src/AppSchemaRenderer.tsx, packages/layout/README.md, apps/console/src/hooks/useBranding.ts (stop on breach; explain in the report)
    Container & model: M, mode:subagent, model: opus — node scripts/pm/dispatch-gates.mjs --tier <paths> on this surface returns no path-derived mandate, so the tier is this seat's call: floor sonnet, taken to opus because the card deletes a key from a published interface.
    Clause-②: yes — deleting logo from the published AppShellBranding changes what a consumer may write against @object-ui/layout's type surface. Judged from card content, not paths.
    Serial constraints cleared: packages/layout has no in-flight claim and no open sibling card in this round's batch (#5214 holds fields/auth/mobile/collaboration/permissions/providers; #5356 holds packages/app-shell/src/preview; #5126 holds skills/objectui/**; #5343 holds content/docs/guide/**). No overlap.

    Ruling is settled — ⛔ not re-litigable. Maintainer, 2026-08-19, verbatim 「全部接受」 over the 27-card decision-inbox review (comment 5339695386): option 1, remove. Delete the key, delete the false passed to sidebar/navbar via context comment, delete the dead feed line. Option 2 (build a real branding context) is rejected. The single real logo path stays the app schema's branding.logo, read directly by AppSidebar.tsx:194.

    Correction to the card's file surface — measured on origin/main bdf8cf76e, 2026-08-20

    The card names one feeder (apps/console/src/hooks/useBranding.ts:27). There are two. The second is in-package and the card never mentions it:

    packages/layout/src/AppSchemaRenderer.tsx:579-583
      const branding: AppShellBranding = {
        title: schema.title,
        favicon: schema.favicon,
        logo: schema.logo,
      };
    

    This one fails loud (excess property on an object literal annotated AppShellBranding), so tsc will find it for you — but discovering it mid-task invites the wrong repair, which is to keep the key. Both feed lines go.

    ⚠️ One thing to check, not to assume. AppSchemaRenderer forwards schema.logo — a top-level app-schema key. AppSidebar.tsx:194 reads activeApp?.branding?.logo — a nested one. The card's claim that "the logo already renders correctly today via the real path" was measured against the AppSidebar path. Confirm those two are actually the same value reaching the same place before you delete the forward; if they are not, you are removing a live path rather than a dead one and the ruling's premise is false for this half. In that case stop and report — the roll-back clause applies, the card returns to needs-user-decision, and ⛔ you do not decide it yourself.

    Also update packages/layout/README.md:82, whose branding row already lists exactly the four keys that are really read — it needs no content change, but re-read it to confirm it does not gain a stale mention.

    Versioning

    Changeset is minor for @object-ui/layout. ⛔ Never major — maintainer, 2026-08-19, verbatim: 「objectui ui 的 major 是要跟着 objectstack 走的,不可以自己发 major」. Say in the changeset body that a published optional key is removed and name the real replacement path.

    Landing this unblocks #4830 (app-shell.mdx prose). ⛔ Do not write that prose here — file surface breach; #4830 is its own card and returns to the queue when this merges.


    Generated by Claude Code

  3. os-support-ai commented on Aug 20, 2026

    @os-support-ai
    Collaborator

    File-surface amendment to the round-18 claim (session session_01RV6yuVCxymHYE16PL9vQkE, branch claude/issue-4818-appshellbranding-logo, PR #5366). Filed in the same round as the change it covers, per the bounded in-place-fix clause.

    The claim declared four files. The implementation touches five. The added one:

    packages/app-shell/src/layout/ConsoleLayout.tsx:174   logo: activeApp.branding.logo,
    

    This is a third feed line into AppShellBranding.logo. The card named one (apps/console/src/hooks/useBranding.ts:27); the claim's own correction added a second (packages/layout/src/AppSchemaRenderer.tsx:582) and stated the card named "one" where there were "two". Measured: there were three. The third is named nowhere — not on the card, not in the triage comment, not in the claim.

    Why it was fixed in place rather than deferred. It is not severable from the ruling: deleting the key makes that line a hard tsc error, so leaving it would ship packages/app-shell red. Measured with the key removed and the feeds intact:

    packages/app-shell  src/layout/ConsoleLayout.tsx(174,15): error TS2322: ... Object literal may
      only specify known properties, and 'logo' does not exist in type 'AppShellBranding'.
    

    Against the four conditions: (1) identical defect class — a dead logo: forward into AppShellBranding; (2) mechanical, with the correct shape pinned by the ruling and by its two sibling feed lines; (3) no other holder — no sibling worktree diff and no open PR touches this file, and round 18's only packages/app-shell holding is #5356 on packages/app-shell/src/preview, a different directory; (4) same gate family — type-check and the root vitest run, no new verification surface. The fix is named with its evidence in PR #5366's body.

    Also within the declared surface but worth stating: apps/console/src/hooks/useBranding.ts loses two lines, not one — the feed at :27 and the matching logo?: string at :17 on its local AppBranding interface, as the dispatch specified.

    packages/layout/README.md was re-read and is unchanged: its branding row already lists exactly the four keys really read, so it gains no stale mention.


    Generated by Claude Code

  4. os-support-ai commented on Aug 20, 2026

    @os-support-ai
    Collaborator
    {
      "issue": 4818,
      "status": "done",
      "branch": "claude/issue-4818-appshellbranding-logo",
      "pr": "https://github.com/objectstack-ai/objectui/pull/5366",
      "premise_still_valid": true,
      "summary": "Ruling implemented as ordered: option 1, remove. Deleted `logo` from `AppShellBranding` plus the false 'passed to sidebar/navbar via context' comment in packages/layout/src/AppShell.tsx, and the dead feed lines. THREE feeders existed, not two: the card named one (apps/console/src/hooks/useBranding.ts:27), the dispatch's correction added a second (packages/layout/src/AppSchemaRenderer.tsx:582), and a third was named nowhere — packages/app-shell/src/layout/ConsoleLayout.tsx:174, `logo: activeApp.branding.logo`. That third is outside the declared file surface; I fixed it in place under the bounded-exemption clause because it is not severable (deleting the key makes it a hard tsc error, so leaving it ships app-shell red), it is the same defect class and the same one-line mechanical deletion as its siblings, no worktree or open PR holds the file, and it adds no gate. Declared in a same-round file-surface amendment comment (5350097944) and named with evidence in the PR body. The premise check the dispatch flagged HOLDS, but not for the reason the dispatch modelled: `schema.logo` (top-level) and `activeApp?.branding?.logo` (nested) are NOT the same key and NOT the same place — that turns out to be irrelevant, because neither logo render site goes through `AppShellBranding` at all. AppSchemaRenderer renders the top-level key DIRECTLY at :385 `resolveIcon(schema.logo)` and :397-399 (an img whose src is schema.logo) in its own default sidebar header — i.e. the same file that built the branding object was already rendering that value itself, making the :582 forward a second, parallel, dead route. AppSidebar.tsx:194 renders the nested key directly. Both untouched, same lines. Diff is 4 source files, 6 deletions, 0 additions, plus one changeset; minor for layout/app-shell/console, never major. packages/layout/README.md:82 re-read and unchanged (already lists exactly the four keys really read). content/docs/layout/app-shell.mdx deliberately untouched — that prose is #4830.",
      "tests": "Union of gates re-run AFTER the final commit, at HEAD 49dee42ad. TESTS, from the repo root with path filters (never `pnpm --filter pkg test` — 17 packages own a standalone vitest.config.ts that does not re-export the root config): `pnpm exec vitest run packages/layout/ apps/console/ --maxWorkers=2` → 'Test Files 76 passed (76) / Tests 874 passed (874)'; `pnpm exec vitest run packages/app-shell/src/layout/ packages/app-shell/src/console/ packages/app-shell/src/runtime-config.test.ts packages/app-shell/src/hooks/ --maxWorkers=2` → 'Test Files 110 passed (110) / Tests 835 passed (835)'. 186 files, 1709 tests, zero failures; those paths cover every app-shell test mentioning ConsoleLayout/AppShellBranding/branding plus the AppSidebar suites guarding the real logo path, with the rest of that package's 456 files left to CI. REVERSE VERIFICATION, three legs, each stating its artifact: tsc HAS a build artifact between edit and result for the two cross-package legs (root tsconfig.json has no `paths` entry for @object-ui/layout, so it resolves via node_modules to packages/layout/dist/AppShell.d.ts), while vitest has NONE (vitest.config.mts:263 aliases @object-ui/layout to packages/layout/src). Leg 1 baseline — built the dependency closure, confirmed marker PRESENT (`logo?: string;` at dist/AppShell.d.ts:14), all three consumers type-check clean, i.e. writing `logo` compiled: 'packages/layout type-check: Done / packages/app-shell type-check: Done / apps/console type-check: Done / EXIT=0'. Leg 2 key deleted with feeds intact — REBUILT, proved marker ABSENT (`grep -c logo dist/AppShell.d.ts` → 0), then each consumer failed naming exactly one site: layout 'src/AppSchemaRenderer.tsx(582,5): error TS2353', app-shell 'src/layout/ConsoleLayout.tsx(174,15): error TS2322', console 'src/hooks/useBranding.ts(27,11): error TS2345', each carrying \"Object literal may only specify known properties, and 'logo' does not exist in type 'AppShellBranding'\". Observed direction was the expected one (turns red), with the added information that the red is three-sited, not two. Leg 3 feeds deleted — REBUILT layout+app-shell dist, marker re-confirmed absent, all three green (EXIT=0). Nothing rendering-visible changed: the removed key had no reader (no branch of useAppShellBranding touches it; AppShell installs no context), and both live render sites are byte-identical to origin/main. GATES at 49dee42ad: check-changeset-presence '4 source file(s) of 3 released package(s) changed, 1 changeset declared'; check:changeset fixed-group OK; check-changeset-no-major 'No changeset declares a major bump'; check:control-bytes OK (4757 tracked text files) plus a manual control-byte scan of the five touched files (none); check:doc-types OK; check:doc-snippets '87 of 87 blocks judged, 0 failed' compiled AGAINST THE BUILT TYPES — the load-bearing one for a published-type deletion, and it found no snippet writing `logo` on an AppShellBranding literal; check:phantom-deps OK; check:self-import OK; lint for the three packages 0 errors (201 pre-existing warnings, unchanged from main). No skip-changeset label is involved — this PR ships a real changeset. CI convergence left to PM per the 2026-08-10 ruling; PR was opened as draft.",
      "open_questions": [],
      "out_of_scope_findings": [
        "filed as #5368 (label `finding`, unassigned): apps/console/src/hooks/useBranding.ts is a @deprecated hook with ZERO callers repo-wide — a dead file, not just the dead key this card removed; PR #5366 had to edit it twice purely to keep something nobody calls compiling. Deleting the file exceeds this card's ruling, so it was filed rather than done.",
        "filed as #5370 (no label, PM to grade): `pnpm --filter @object-ui/layout build` printed `error TS2353` and still exited 0 — unplugin-dts reports type errors without failing the build, and emitted dist/ anyway. Measured during leg 2 above (BUILD_EXIT=0 from PIPESTATUS with the error in the log). CI is NOT blind (type-check is a separate task and goes red); the exposure is to anyone judging a package from `build` alone mid-task, where the summary line reads success."
      ]
    }

    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions