diff --git a/.changeset/8637-tab-title-one-writer.md b/.changeset/8637-tab-title-one-writer.md new file mode 100644 index 0000000000..cc9c5882fa --- /dev/null +++ b/.changeset/8637-tab-title-one-writer.md @@ -0,0 +1,32 @@ +--- +'@object-ui/layout': patch +'@object-ui/console': patch +--- + +The console tab title no longer reverts to the bare product name after an in-app +navigation (objectui#8637). + +Two effects wrote `document.title` on different keys. The console's `BrandingSync` +was keyed on `useLocation()` and assigned the bare product name on **every route +change**; `useAppShellBranding` assigns the composed `"App label — Product name"` +from an effect keyed on that string, so it fires when the title changes and not on +navigation. Both run on the commit that mounts the shell, and the composed title +wins — which is why the tab looked right and the defect stayed hidden. Navigating +between two pages of the same app moved `location` and not the composed title, so +only the route-keyed writer ran and the tab fell back to the bare product name. +Measured in a real browser, not inferred. + +The repair is one writer rather than two careful ones. `useAppShellBranding` now +owns `document.title` for as long as a shell is mounted: it captures whatever the +tab already said, writes `title` over it, and puts the captured string back when the +shell unmounts or `title` changes. That is what let the console's route-keyed writer +drop its title assignment entirely — it had doubled as the reset that took the app +label back off the tab on the way out — and it is now `FaviconSync`, which syncs only +the favicon. + +For hosts of `@object-ui/layout`: the forward assignment is unchanged, and a shell +with no `title` still leaves the tab untouched in both directions. What is new is the +restore, so a shell mounted over part of a route tree hands the title back on exit +instead of stranding it. The restore replays the captured string unconditionally, so +a surface that writes the tab title from **inside** a mounted shell has its value +overwritten on unmount; keep such surfaces outside the shell. diff --git a/apps/console/src/App.tsx b/apps/console/src/App.tsx index a1634a870f..a626a8f7d0 100644 --- a/apps/console/src/App.tsx +++ b/apps/console/src/App.tsx @@ -12,8 +12,8 @@ * with extra `` children. */ -import { lazy, Suspense, useEffect } from 'react'; -import { BrowserRouter, Routes, Route, Navigate, useLocation } from 'react-router-dom'; +import { lazy, Suspense } from 'react'; +import { BrowserRouter, Routes, Route, Navigate } from 'react-router-dom'; import { AuthProvider, useAuth } from '@object-ui/auth'; import { DevMasterDetail } from './dev/DevMasterDetail'; import { DevLists } from './dev/DevLists'; @@ -36,12 +36,11 @@ import { DefaultSettingsPage, DefaultAcceptInvitationPage, DefaultAiChatPage, - getProductName, - getFaviconUrl, RedirectWithSplash, } from '@object-ui/app-shell'; import { AppContent } from './AppContent'; +import { FaviconSync } from './components/FaviconSync'; import { RootLandingRedirect } from './components/RootLandingRedirect'; import { ProtectedRoute } from './components/ProtectedRoute'; import { studioRoutes } from './components/StudioRoute'; @@ -128,23 +127,6 @@ function HomeRoute() { ); } -/** Syncs document title + favicon with runtime branding on every route change. */ -function BrandingSync() { - const location = useLocation(); - useEffect(() => { - document.title = getProductName(); - const faviconUrl = getFaviconUrl(); - if (faviconUrl) { - const link = document.getElementById('favicon') as HTMLLinkElement | null; - if (link) { - link.href = faviconUrl; - link.type = faviconUrl.endsWith('.svg') ? 'image/svg+xml' : 'image/png'; - } - } - }, [location]); - return null; -} - export function App() { return ( @@ -163,7 +145,7 @@ export function App() { - + {/* diff --git a/apps/console/src/__tests__/tabTitleAfterNavigation.test.tsx b/apps/console/src/__tests__/tabTitleAfterNavigation.test.tsx new file mode 100644 index 0000000000..16292e2e91 --- /dev/null +++ b/apps/console/src/__tests__/tabTitleAfterNavigation.test.tsx @@ -0,0 +1,178 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The tab title survives an in-app navigation (objectui#8637). + * + * ## The defect this pins shut + * + * Two independent effects wrote `document.title`, keyed on different inputs. + * `apps/console/src/App.tsx` rendered `BrandingSync` — a sibling mounted BEFORE + * the shell, inside `BrowserRouter` — whose effect was keyed on `useLocation()` + * and assigned the BARE product name on every route change. `AppShell`'s + * `useAppShellBranding` assigns the composed `"App label — Product name"` from + * an effect whose dependency list ends in that string, so it fires when the + * title changes and NOT on navigation. + * + * On the commit that first mounts the shell both fire, in tree order, and the + * composed title wins — which is why the tab looks right and the defect hides. + * Navigating between two pages of the SAME app changes `location` and not the + * composed title: only the route-keyed writer runs, and the tab falls back to + * the bare product name. + * + * ## Why this file exists at all + * + * From the card: "the reason this survived is that no test asserts what the tab + * title is after a navigation." Every earlier pin asserted a single write in + * isolation — `app-shell-branding-title-assignment.test.tsx` pins that + * `AppShell` assigns its `title` argument wholesale, and passes identically on + * the defect and on the fix, because it never navigates. So the pin has to + * render BOTH writers together and move the router, which is what the + * `navigate` step below does. + * + * ## The real subjects, not replicas + * + * `FaviconSync` is imported from the console module it actually ships in, and + * `AppShell` from `@object-ui/layout` — so re-adding a `document.title` + * assignment to either one turns this file red. A hand-written replica of the + * route-keyed effect would pin the replica instead, and the defect would walk + * straight back in through the real component. + * + * ⚠️ happy-dom is not where this behaviour was MEASURED — effect ordering + * against real navigation is not something a jsdom-style harness reproduces + * faithfully, and the card's PM note ruled reading the code insufficient too. + * The measurement is a real-Chromium before/after on objectui#8637's pull + * request, driving the same two components under a real `BrowserRouter`. This + * file is the cheap regression guard that runs in CI afterwards; the first test + * below is an environment control so a vacuous green is distinguishable from a + * real one. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { render, cleanup, screen, fireEvent } from '@testing-library/react'; +import { MemoryRouter, Routes, Route, Link } from 'react-router-dom'; +import { AppShell } from '@object-ui/layout'; +import { FaviconSync } from '../components/FaviconSync'; + +/** What `main.tsx` writes before React mounts: the bare product name. */ +const PRODUCT = 'ObjectOS'; +/** What `ConsoleLayout` composes and hands to `AppShell.branding.title`. */ +const COMPOSED = 'Sales CRM — ObjectOS'; + +/** + * Navigation is driven by clicking a real ``, the way the console's own + * sidebar and breadcrumbs move between pages of an app — not by calling the + * router imperatively from outside a component. + */ +function goTo(label: string) { + fireEvent.click(screen.getByText(label)); +} + +/** Mirrors `App.tsx`: the route-keyed sync is a sibling rendered BEFORE the shell. */ +function ConsoleTree({ inApp }: { inApp: boolean }) { + return ( + <> + + + {inApp ? ( + + + page a} /> + page b} /> + + + ) : ( +
no shell
+ )} + + ); +} + +beforeEach(() => { + document.title = PRODUCT; +}); + +afterEach(() => { + cleanup(); +}); + +describe('environment control', () => { + it('happy-dom lets `document.title` be written and read back', () => { + // Without this every assertion below could be vacuous in a DOM whose + // `title` setter is a no-op. Measured, not assumed. + document.title = 'probe'; + expect(document.title).toBe('probe'); + document.title = PRODUCT; + expect(document.title).toBe(PRODUCT); + }); +}); + +describe('the tab title after an in-app navigation (objectui#8637)', () => { + it('mounting the shell puts the composed title up — the state the defect started from', () => { + render( + + + , + ); + expect(document.title).toBe(COMPOSED); + }); + + it('navigating to another page of the SAME app leaves the composed title alone', () => { + render( + + + , + ); + expect(document.title).toBe(COMPOSED); + + goTo('go to page b'); + + expect( + document.title, + [ + 'The tab title reverted after an in-app navigation. A second writer keyed on the', + 'ROUTE is assigning `document.title` again — that is objectui#8637. `AppShell`', + '(`useAppShellBranding`) owns the title while a shell is mounted; a route-keyed', + 'writer beside it cannot know the app label, so it can only write the bare product', + 'name over the specific one. Remove the assignment, do not try to order the two.', + ].join('\n'), + ).toBe(COMPOSED); + }); + + it('navigating a third time still leaves it alone — the effect is not merely deferred', () => { + render( + + + , + ); + goTo('go to page b'); + goTo('go to page a'); + expect(document.title).toBe(COMPOSED); + }); +}); + +describe('leaving the app hands the tab back (objectui#8637)', () => { + it('unmounting the shell restores the title it found, without a route-keyed reset', () => { + // The route-keyed writer used to double as the reset that took the app + // label back off the tab when the shell went away. Deleting it without a + // replacement would strand `"Sales CRM — ObjectOS"` on `/home`; the + // replacement is `useAppShellBranding`'s own restore-on-unmount, and this + // is the assertion that keeps the two halves of the change together. + const view = render( + + + , + ); + expect(document.title).toBe(COMPOSED); + + view.rerender( + + + , + ); + + expect(document.title).toBe(PRODUCT); + }); +}); diff --git a/apps/console/src/components/FaviconSync.tsx b/apps/console/src/components/FaviconSync.tsx new file mode 100644 index 0000000000..298b1fa4af --- /dev/null +++ b/apps/console/src/components/FaviconSync.tsx @@ -0,0 +1,46 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { useEffect } from 'react'; +import { useLocation } from 'react-router-dom'; +import { getFaviconUrl } from '@object-ui/app-shell'; + +/** + * Re-applies the runtime-branded favicon on every route change. + * + * ⛔ **It deliberately does not write `document.title`** (objectui#8637). It + * used to — as `BrandingSync`, it assigned the BARE product name on every + * `useLocation()` change, while `AppShell`'s `useAppShellBranding` assigns the + * composed `"App label — Product name"` from an effect keyed on that string. + * Two writers on one global, keyed on different inputs: navigating between two + * pages of the same app changed `location` but not the composed title, so only + * this one fired and the tab reverted to the bare product name until something + * else changed the composed title. Measured in a real browser, not inferred — + * the reading is on objectui#8637's pull request. + * + * The repair is one writer, not two careful ones: `useAppShellBranding` owns + * `document.title` while a shell is mounted and restores the previous title + * when it unmounts, so leaving an app no longer needs a route-keyed reset here. + * ⛔ Do not re-add a title assignment to this component — that re-creates the + * race, and `apps/console/src/__tests__/tabTitleAfterNavigation.test.tsx` is + * the pin that goes red when it comes back. + * + * Boot-time title writers are a different lifecycle and are left alone: the + * inline script in `apps/console/index.html` and `main.tsx` both set the bare + * product name before React mounts, which is the correct title until a shell + * with an app label is on screen — and now the string `useAppShellBranding` + * captures and restores. + */ +export function FaviconSync() { + const location = useLocation(); + useEffect(() => { + const faviconUrl = getFaviconUrl(); + if (faviconUrl) { + const link = document.getElementById('favicon') as HTMLLinkElement | null; + if (link) { + link.href = faviconUrl; + link.type = faviconUrl.endsWith('.svg') ? 'image/svg+xml' : 'image/png'; + } + } + }, [location]); + return null; +} diff --git a/packages/layout/src/AppShell.tsx b/packages/layout/src/AppShell.tsx index 440e263b46..5b930413b2 100644 --- a/packages/layout/src/AppShell.tsx +++ b/packages/layout/src/AppShell.tsx @@ -120,6 +120,22 @@ function foregroundForHex(hex: string): string { /** * Apply branding CSS custom properties to the document root. * This is extracted as a standalone hook so it can be re-used independently. + * + * It is also the ONE writer of `document.title` for as long as a shell is + * mounted (objectui#8637). Ownership is scoped, not permanent: the hook + * captures whatever the tab already said, writes `title` over it, and puts the + * captured string back when the shell unmounts or `title` changes. So a host + * that mounts a shell for part of its route tree gets the specific title while + * it is there and its previous title back when it leaves, without a second + * writer keyed on navigation — which is what the console used to do, and what + * reverted the composed title to the bare product name on every in-app + * navigation. + * + * ⚠️ The capture is a read of the live `document.title`, so a host that lets + * something else write the tab title WHILE a shell is mounted hands this hook a + * value it did not put there, and that value is what comes back on unmount. + * Nesting a second title-writing surface inside a mounted shell is the shape to + * avoid; the console's own auth surfaces sit outside the shell for this reason. */ export function useAppShellBranding(branding?: AppShellBranding, title?: string) { useEffect(() => { @@ -210,13 +226,20 @@ export function useAppShellBranding(branding?: AppShellBranding, title?: string) } } - // Page title + // Page title. `previousTitle` stays `null` when this hook writes nothing, + // so the no-`title` case restores nothing either — a shell without a title + // leaves the tab entirely alone in both directions. + let previousTitle: string | null = null; if (title) { + previousTitle = document.title; document.title = title; } return () => { observer.disconnect(); + if (previousTitle !== null) { + document.title = previousTitle; + } root.style.removeProperty('--brand-primary'); root.style.removeProperty('--brand-primary-hsl'); root.style.removeProperty('--brand-accent'); diff --git a/packages/layout/src/__tests__/app-shell-branding-title-restore.test.tsx b/packages/layout/src/__tests__/app-shell-branding-title-restore.test.tsx new file mode 100644 index 0000000000..e5ca0b0e57 --- /dev/null +++ b/packages/layout/src/__tests__/app-shell-branding-title-restore.test.tsx @@ -0,0 +1,125 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * `useAppShellBranding` gives `document.title` back when the shell goes away + * (objectui#8637). + * + * ## Why the restore is part of the contract, not a nicety + * + * The hook assigns `title` wholesale while the shell is mounted — that half is + * pinned next door in `app-shell-branding-title-assignment.test.tsx`, and this + * file deliberately does not restate it. What was missing is the other end of + * the lifecycle. A host mounts a shell for part of its route tree (the console + * mounts `ConsoleLayout` under `/apps/*` and nothing outside it), so a title + * this hook writes has to come back off the tab when the shell unmounts. Before + * this, it did not, and the console compensated with a SECOND writer keyed on + * the route — which reset the tab correctly on the way out and also clobbered + * the composed title on every in-app navigation. Removing that writer is only + * safe because the restore below exists, so the two are pinned as one change. + * + * ## Shape + * + * Every test starts from a known sentinel, mounts the hook alone (it is the + * writer; the shell chrome is irrelevant), and reads `document.title` back + * after an unmount or a `title` change. The first test is an environment + * control — without it a DOM with a no-op `title` setter would make the rest + * vacuously green. + * + * ⚠️ The restore is a REPLAY of whatever the tab said at write time, not a + * memory of who owned it. That is exactly the guarantee a host needs to reason + * about, and its cost is the case the last test pins: a surface that writes the + * title from INSIDE a mounted shell has its value overwritten on unmount. The + * console keeps its auth surfaces outside the shell for this reason. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import React from 'react'; +import { render, cleanup } from '@testing-library/react'; +import { useAppShellBranding, type AppShellBranding } from '../AppShell'; + +const SENTINEL = 'sentinel — document.title before the shell mounted'; + +/** Mounts only the hook — the actual `document.title` writer — with no shell chrome. */ +function HookOnly({ branding, title }: { branding?: AppShellBranding; title?: string }) { + useAppShellBranding(branding, title); + return null; +} + +beforeEach(() => { + document.title = SENTINEL; +}); + +afterEach(() => { + cleanup(); +}); + +describe('environment control', () => { + it('happy-dom lets `document.title` be written and read back', () => { + document.title = 'probe'; + expect(document.title).toBe('probe'); + document.title = SENTINEL; + expect(document.title).toBe(SENTINEL); + }); +}); + +describe('`useAppShellBranding` restores the title it replaced', () => { + it('unmounting puts back the title that was there before the shell mounted', () => { + const view = render(); + expect(document.title).toBe('Sales CRM — ObjectStack'); + + view.unmount(); + + expect( + document.title, + [ + 'The shell unmounted and left its own title on the tab. A host that mounts a shell', + 'for part of its route tree now has to reset the title itself on the way out — and', + 'the only writer positioned to do that is one keyed on the ROUTE, which is the', + 'second writer objectui#8637 removed. The restore here is what makes one writer', + 'enough.', + ].join('\n'), + ).toBe(SENTINEL); + }); + + it('changing `title` restores once and writes once — no accumulation', () => { + const view = render(); + view.rerender(); + expect(document.title).toBe('Support Desk — ObjectStack'); + + view.unmount(); + + // The restore replays the string captured before the FIRST write, not the + // one the shell itself put up a moment ago. + expect(document.title).toBe(SENTINEL); + }); + + it('a shell with no `title` restores nothing, because it wrote nothing', () => { + // The mirror of "no `title` leaves `document.title` untouched": an absent + // title must not turn the cleanup into a writer either. Something else + // changes the tab while this shell is mounted, and the shell leaves it be. + const view = render(); + expect(document.title).toBe(SENTINEL); + + document.title = 'written by someone else'; + view.unmount(); + + expect(document.title).toBe('written by someone else'); + }); + + it('a title written from inside a mounted shell is overwritten on unmount', () => { + // Documented cost, pinned so it is a known property rather than a surprise: + // the restore replays the captured string unconditionally. + const view = render(); + document.title = 'written while the shell was mounted'; + + view.unmount(); + + expect(document.title).toBe(SENTINEL); + }); +}); diff --git a/packages/layout/src/__tests__/app-shell-branding-title-surfaces.test.ts b/packages/layout/src/__tests__/app-shell-branding-title-surfaces.test.ts index c8fb7156b2..abd7dbc539 100644 --- a/packages/layout/src/__tests__/app-shell-branding-title-surfaces.test.ts +++ b/packages/layout/src/__tests__/app-shell-branding-title-surfaces.test.ts @@ -64,13 +64,25 @@ * interface declares (read off the source, not hand-listed) must each carry * a doc comment and those comments must be pairwise distinct. * - * 6. **The code half, by source.** `document.title` is written exactly once in - * `AppShell.tsx`, and the right-hand side is the bare `title` — no template, - * no `+`, no `+=`. The behavioural version of this pin (render, then read - * `document.title`) lives in `app-shell-branding-title-assignment.test.tsx`; - * this half exists so a source diff that adds a SECOND writer is caught by - * the same file that pins the wording, since a second writer would change - * what the correct wording is. + * 6. **The code half, by source.** `AppShell.tsx` writes `document.title` in + * exactly TWO places, one per role, and each right-hand side is a bare + * identifier — no template, no `+`, no `+=`. The forward write assigns + * `title`; the cleanup write restores the captured `previousTitle` + * (objectui#8637). The behavioural version of this pin (render, then read + * `document.title`) lives in `app-shell-branding-title-assignment.test.tsx` + * and `app-shell-branding-title-restore.test.tsx`; this half exists so a + * source diff that adds a writer beyond those two roles is caught by the + * same file that pins the wording, since another writer would change what + * the correct wording is. + * + * ⚠️ This started life as "exactly one writer" and it FAILED on + * objectui#8637's restore, which is the pin doing its job rather than a + * reason to relax it. What was kept is every property that made it worth + * having: the assigning writer is still exactly one, its operator is still + * `=`, its right-hand side is still the bare `title`, and the total is still + * an exact count — so a third writer is still a failure. What was added is + * strictly more pinning, not less: the restore's own right-hand side is now + * named too, so the cleanup cannot quietly start composing a title either. * * ## Scan surface * @@ -308,14 +320,30 @@ describe('`AppShellBranding` fields are each described, and each differently (ob // The code half, by source // --------------------------------------------------------------------------- -describe('`AppShell.tsx` writes `document.title` once, wholesale (objectui#6872)', () => { - it('exactly one writer, and its right-hand side is the bare `title`', () => { - const writers = [...APP_SHELL_SRC.matchAll(/document\.title\s*(\+?=)\s*([^;\n]+);/g)]; +describe('`AppShell.tsx` writes `document.title` wholesale, in two roles (objectui#6872, objectui#8637)', () => { + /** Every `document.title = …` / `+= …` in the source, as `[whole, operator, rhs]`. */ + const writers = [...APP_SHELL_SRC.matchAll(/document\.title\s*(\+?=)\s*([^;\n]+);/g)]; + /** The forward write; the restore is the one whose right-hand side is the captured title. */ + const assigning = writers.filter((match) => match[2].trim() !== 'previousTitle'); + const restoring = writers.filter((match) => match[2].trim() === 'previousTitle'); + + it('exactly two writers, one per role — nothing else touches the tab title', () => { expect( writers.map((match) => match[0]), - 'a second `document.title` writer appeared — the wording every surface carries would need to change with it', - ).toHaveLength(1); - const [, operator, rhs] = writers[0]; + [ + 'A `document.title` writer appeared or disappeared in AppShell.tsx. This hook is the ONE', + 'writer while a shell is mounted (objectui#8637), and that is exactly two source writes:', + 'the forward assignment of `title`, and the cleanup restore of `previousTitle`. A third', + 'would change the wording every surface carries; a missing restore strands the shell', + 'title on the tab and pushes the host back to a route-keyed reset, which is the defect.', + ].join('\n'), + ).toHaveLength(2); + expect(assigning.map((match) => match[0]), 'the forward writer').toHaveLength(1); + expect(restoring.map((match) => match[0]), 'the restore writer').toHaveLength(1); + }); + + it('the forward writer assigns the bare `title`', () => { + const [, operator, rhs] = assigning[0]; expect(operator).toBe('='); expect( rhs.trim(), @@ -326,4 +354,13 @@ describe('`AppShell.tsx` writes `document.title` once, wholesale (objectui#6872) ].join('\n'), ).toBe('title'); }); + + it('the restore writer replays the captured title, and composes nothing', () => { + const [, operator, rhs] = restoring[0]; + expect(operator).toBe('='); + expect( + rhs.trim(), + 'the cleanup must put back what it captured, not build a new title out of it', + ).toBe('previousTitle'); + }); });