Repository navigation
fix(console): FaviconSync stops writing the favicon, so an app keeps its own icon across in-app navigation (objectui#10379) - #10429
Merged
Conversation
…its own icon across in-app navigation (objectui#10379) The favicon twin of the objectui#8637 title race. FaviconSync re-applied the operator favicon from an effect keyed on useLocation(), while useAppShellBranding writes the app's branding.favicon from an effect keyed on that URL. Moving between two pages of one app ran only the route-keyed writer, and the operator icon replaced the app's. One writer: FaviconSync now writes nothing. The boot (index.html's pre-React script and main.tsx) puts the operator favicon up before React mounts, from a runtime config main.tsx awaits before the first render, and the shell captures and restores that icon (objectui#10040). A mount-only write would only repeat the boot and would be right only while FaviconSync renders before the shell, so it is not kept. The pin renders the real FaviconSync and the real AppShell under a MemoryRouter, boots the favicon through the shipped index.html script against a stubbed runtime/config fetch, and covers in-app navigation, leaving the app, an app without its own favicon, both render orders and StrictMode. Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BP8CMtACxTdLjqR6rhd33C
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
|
…tes the favicon (objectui#10379) Two pending changesets name FaviconSync in the present tense, and this branch made both statements false: FaviconSync now writes nothing. - 8637-tab-title-one-writer: "it is now FaviconSync, which syncs only the favicon" now reads that it became FaviconSync, which kept only the favicon write until objectui#10379 removed that too. - 10040-app-shell-favicon-restore: "that something is FaviconSync, which only writes when an operator favicon is configured" moves to the past tense and names objectui#10379 as the change that removed the write. Body only. Both frontmatters are byte-identical to HEAD (sha256 of the block between the two --- lines compared before commit). Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BP8CMtACxTdLjqR6rhd33C
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
|
Contributor
Author
Contract reviewServed-tier: Rendered by an isolated review subagent spawned by the ① Derived judgments
② Semver level
③ Boundary flags
Implemented-by: VERDICT: PASS Generated by Claude Code |
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
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 #10379
Clause-②: no
What was wrong
FaviconSync(apps/console/src/components/FaviconSync.tsx) re-applied the operator favicon,getFaviconUrl(), from an effect keyed onuseLocation().useAppShellBrandingwrites the active app'sbranding.faviconfrom an effect keyed on that URL. With an operator favicon configured, moving between two pages of the same app ran only the route-keyed writer. The operator favicon then replaced the app's for the rest of the visit. It is the favicon twin of the title race objectui#8637 removed.The change
FaviconSyncno longer writes the favicon, and it now writes nothing at all. Its docblock records both removals (the title, then the favicon) and explains why a mount-only write is not kept either. No other runtime file changes.Why removing the write is enough, measured on the branch point
74fcda829:getFaviconUrl()is a synchronous read of the@object-ui/app-shellruntime-config singleton.initRuntimeConfig()fills that singleton, andmain.tsxawaits it in thePromise.allgate that runs beforecreateRoot().render(). At74fcda829,git grep -n "initRuntimeConfig(" -- apps packagesfound that call and no other, apart from tests, comments, docs and the definition itself. The runtime config has no subscription API. So the value is settled before the first render and does not change after it, and there is no late-arrival case to pin. That caller reading is not re-derived by any gate.#favicon: the pre-React inline script inapps/console/index.html, andmain.tsxjust beforerender().sharedGetJsonjoins the inline script's in-flight request. The script registers its continuation first, so its write lands beforemain.tsxresumes.cd7b728b, an ancestor of the branch point), the shell captures thehrefattribute it finds on mount and restores it on unmount. With no write left inFaviconSync, the shell is the only favicon writer while React runs. What it captures is therefore the operator favicon, whichever order the components mount in. Under StrictMode, whichmain.tsxuses, the shell restores and then captures again, and the second capture still reads the operator favicon.FaviconSyncrenders before the shell. Ablation leg 2 below shows that order dependence as a red test.Evidence (code head
6e529c3eb)The new pin is
apps/console/src/__tests__/faviconAfterNavigation.test.tsx. It renders the realFaviconSyncand the realAppShellfrom@object-ui/layout(which callsuseAppShellBranding) under aMemoryRouter. The icon link is copied from the shippedindex.html. The shipped pre-React script runs against a stubbedGET /api/v1/runtime/config, andinitRuntimeConfig()joins its request, sogetFaviconUrl()answers from the real singleton.fetchis the only stub. The file has 13 tests:environment controls;
navigating inside the app, and navigating a third time;
switching apps;
leaving the app, with and without an operator favicon;
an app without its own favicon (the control);
both render orders, and StrictMode.
Branch point
74fcda829, before the fix:Tests 4 failed | 9 passed (13). The in-app navigation case readsExpected: "https://cdn.example.test/app-a.svg",Received: "/operator-O.png".Head
6e529c3eb: the pin passes, and so do the neighbouring suites (tabTitleAfterNavigation,App.uploadAltitude-10131,App.docsPortalLazy,internalFormShell,runtimeConfigBootDedup, and thepackages/layoutsuiteapp-shell-branding-favicon-restore):Test Files 7 passed (7),Tests 44 passed (44).Ablation on the committed fix, through
ablation-replace. Each leg's anchor hit once and the blob changed. Each restore was proven by blob == HEAD and an emptygit diff HEAD.[location]-keyed write back:Tests 4 failed | 9 passed (13), including the in-app navigation case.[]deps):Tests 1 failed | 12 passed (13). Only the order with FaviconSync rendered AFTER the shell fails.Real Chromium 141 (
/opt/pw-browsers/chromium) against the console Vite dev server. A throwaway page rendered the same tree underBrowserRouterandStrictMode, with a cold deep link to/apps/a. The operator favicon was written the waymain.tsxwrites it.74fcda829component)/operator-O.pngapp-a.svg/operator-O.png/operator-O.png/operator-O.pngapp-a.svgapp-a.svg/operator-O.pngAn app without its own favicon read
/operator-O.pngat every step in both variants. The probe files were deleted after the run, and none of them is committed.Gates (local, code head
6e529c3eb)node scripts/check-changeset-presence.mjs: exit 0. It printed "2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)".pnpm check:new-line-citations: exit 0,VERDICT new-cross-file-line-citations: 0 new citation(s).pnpm check:control-bytes,pnpm check:changeset-claims,pnpm check:test-path-roots,pnpm check:pending-changeset-literals: exit 0 each.eslint --format jsonran fromapps/console, whose ownlintscript iseslint .under the rooteslint.config.js. The JSON reports 2 files, 0 errors, 0 warnings.parserOptions.projectorprojectService, so linting is not type-aware. No custom rule ineslint-rules/reads the filesystem. So this diff cannot change a verdict on any untouched file.type-check: NOT MEASURED as the gate. It waits on^buildof the 35-package closure, which is too long for the foreground cap on this shared box. CIType Checkowns it. A narrowed run was done instead:tsc -pused a throwaway tsconfig that extendsapps/console/tsconfig.json. Itsfileswere the two touched files plusvite-env.d.ts, and@object-ui/*was mapped to package sources.--listFilesOnlyshows both files in the program, and neither has a diagnostic.noUnusedLocals.The changeset is
.changeset/10379-favicon-one-writer.md, declaring'@object-ui/console': patch. The dispatch said the console is not a released package, but the population ofcheck-changeset-presencesays it is.@object-ui/consoleis in thefixedgroup of.changeset/config.json, so itsapps/console/src/is guarded. The objectui#8637 changeset (.changeset/8637-tab-title-one-writer.md) also declares'@object-ui/console': patch.Patch round 1 (head
6da5b45ee): two pending changesets correctedThe seat answered the report's open question with A in comment 5824829480: the inert
FaviconSyncand itsApp.tsxmount stay. The same comment amended the claim's file surface to add.changeset/8637-tab-title-one-writer.md, body only. The coordinator's patch-round message also asked for every other pending changeset that namesFaviconSync,BrandingSyncor the favicon to be re-read, by symbol as well as by path, and corrected where this PR makes it false.Commit
6da5b45eecorrects two sentences:.changeset/8637-tab-title-one-writer.mdsaid the console's route-keyed writer "is nowFaviconSync, which syncs only the favicon". It now says the writer "becameFaviconSync, which kept only the favicon write until objectui#10379 removed that too"..changeset/10040-app-shell-favicon-restore.mdsaid "In the console that something isFaviconSync, which only writes when an operator favicon is configured". It is now in the past tense:FaviconSync"wrote the icon only when an operator favicon was configured (until objectui#10379 removed that write)".Both frontmatters are byte-identical. The sha256 of the block between the two
---lines is the same at74fcda829and at6da5b45ee:63f43504…for 8637 andee79b57f…for 10040.check-changeset-overwrite(report-only) lists both as modified, with "declared at base" equal to "declares now".What was read. Pending changesets at HEAD were searched with
git grep -l -i -E 'FaviconSync|BrandingSync|favicon'and'getFaviconUrl|icon link|faviconUrl|route-keyed|route keyed'over.changeset/*.md. The control:BrandingSyncmust hit 8637, and it does. Six files matched:8637-tab-title-one-writer: one sentence corrected. The other sentences were re-read against head and are true. "Both run on the commit that mounts the shell" narrates the defect, and the sentences around it put it in the past.10040-app-shell-favicon-restore: one sentence corrected. The others are true at head.10379-favicon-one-writer: this PR's own changeset, true.4830-app-shell-branding-rightrail-prose,console-boot-request-dedup-5544,console-preboot-branding-origin-5660: they mention the favicon but notFaviconSync, and nothing in them is made false by this PR.Gates on
6da5b45ee, each exit 0:node scripts/check-changeset-presence.mjs,pnpm check:changeset-claims,node scripts/check-changeset-no-major.mjs,pnpm check:control-bytes,node scripts/check-changeset-overwrite.mjs,pnpm check:new-line-citations,pnpm check:pending-changeset-literals.6da5b45eediffers from6e529c3ebonly in those two changeset bodies.git grepfinds no test or script that names either file. So the test, ablation, browser, lint and type-check readings above, taken on6e529c3eb, still describe this head.Premise check
FaviconSynchad the location-keyed effect, the shell captures and restores the icon (objectui#10372), and the defect reproduces (red above, in the DOM and in the browser).useAppShellBrandinglives in@object-ui/layout(packages/layout/src/AppShell.tsx), not in@object-ui/app-shell. It was read, not edited.Acceptance notes
FaviconSyncnow renders nothing and runs no effect, andApp.tsxstill mounts it. The seat ruled to keep it (comment 5824829480, answer A). Its docblock records both removed writes, and both pins render the real component. Deleting it is separate dead-code removal. Three other leftovers are now inert:App.tsx;vi.mock('../components/FaviconSync')inApp.uploadAltitude-10131.test.tsx;getFaviconUrlentries (returning an empty string) in three console test mocks of@object-ui/app-shell.Carrier: none.
useAppShellBrandingwrites the iconhrefbut never itstype. With an operator.pngfavicon and an app.svgfavicon, the link readstype="image/png"with the app's.svgURL while the app is shown. That pairing already appeared on every shell mount. This PR makes it last for the whole visit instead of only until the first in-app navigation. Whether a browser renders that icon was not measured. Carrier: none.The Branding section of the layout docs (
content/docs/layout/app-shell.mdx) says the colour properties are removed on unmount. It does not mention the title and favicon restores, so it is incomplete but not false. It was not touched.This draft was written by the
domain:ui#4seat's dispatched developer, sessionhttps://claude.ai/code/session_01BP8CMtACxTdLjqR6rhd33C. It stays a draft on purpose: marking it ready and queueing it are the seat's steps.Generated by Claude Code