diff --git a/.changeset/shadcn-close-label-i18n.md b/.changeset/shadcn-close-label-i18n.md new file mode 100644 index 0000000000..faf0fda41d --- /dev/null +++ b/.changeset/shadcn-close-label-i18n.md @@ -0,0 +1,7 @@ +--- +'@object-ui/components': patch +--- + +The close button that the `Sheet` and `Dialog` primitives auto-render now announces itself in the session locale instead of always in English. Both buttons are icon-only (a lucide `X`), so their `sr-only` span is not decoration — it IS the control's accessible name, and upstream Shadcn ships it as a hardcoded English literal. Under zh/ja/es every drawer and modal in the console (~20 `SheetContent` consumers plus every `DialogContent` consumer — ChatDock, ActivityFeed, metadata-admin, AiChatPage, BuildDebugDrawer, PeoplePicker, RecordDetailDrawer, …) announced "Close" to a screen reader. The span now renders ``, which resolves `common.close` (present in all ten locale packs since objectstack#5430) and falls back to English when no `I18nProvider` is mounted, so existing suites and e2e specs that address these controls by their English name are unaffected (objectstack#5505). + +Because `packages/components/src/ui/**` is regenerated from the Shadcn registry, the edit is not a hand patch that the next `pnpm shadcn:update` would silently revert: it is declared as data in `scripts/shadcn-local-patches.mjs`, re-applied automatically on every sync (including `--force`), and enforced in both directions — `pnpm shadcn:check` now exits non-zero if a declared patch is missing from the file on disk or can no longer be re-applied to current upstream, and an offline test gates the same invariant on every PR. diff --git a/packages/components/shadcn-components.json b/packages/components/shadcn-components.json index 2cd0b8ba86..bee38a0de6 100644 --- a/packages/components/shadcn-components.json +++ b/packages/components/shadcn-components.json @@ -139,7 +139,8 @@ "dependencies": [ "radix-ui" ], - "registryDependencies": [] + "registryDependencies": [], + "localEdits": "objectstack#5505 i18n close patch: `DialogContent` auto-renders an icon-only close button whose `sr-only` span IS its accessible name, and upstream hardcodes the English literal — so under zh/ja/es every modal in the console announced \"Close\" in English. The span is replaced by `` (`packages/components/src/lib/close-label.tsx`), which resolves `common.close` for the session locale and falls back to English with no I18nProvider mounted. The edit is DECLARED as data in `scripts/shadcn-local-patches.mjs` and re-applied on every `--update`, and `pnpm shadcn:check` exits non-zero if it is ever missing from the file or can no longer be re-applied to upstream." }, "drawer": { "source": "https://ui.shadcn.com/r/styles/default/drawer.json", @@ -268,7 +269,7 @@ "radix-ui" ], "registryDependencies": [], - "localEdits": "Adds a `hideOverlay` prop that suppresses ``, for sheets that must not dim the page behind them. Upstream renders the overlay unconditionally." + "localEdits": "Adds a `hideOverlay` prop that suppresses ``, for sheets that must not dim the page behind them. Upstream renders the overlay unconditionally. Also carries the objectstack#5505 i18n close patch (see `dialog` for the full rationale): `SheetContent`'s auto-rendered close button renders `` instead of a hardcoded English `sr-only` span. Declared in `scripts/shadcn-local-patches.mjs` and RE-APPLIED automatically by every sync, so unlike the `hideOverlay` edit above it does not depend on anyone remembering it." }, "sidebar": { "source": "https://ui.shadcn.com/r/styles/default/sidebar.json", diff --git a/packages/components/src/__tests__/navigation-overlay-close-i18n.test.tsx b/packages/components/src/__tests__/navigation-overlay-close-i18n.test.tsx index c34f24dcc5..3c1ffa43cb 100644 --- a/packages/components/src/__tests__/navigation-overlay-close-i18n.test.tsx +++ b/packages/components/src/__tests__/navigation-overlay-close-i18n.test.tsx @@ -69,16 +69,21 @@ afterEach(() => cleanup()); /** * Addressing the drawer's close button: by `title`, NOT by role+name. * - * The shadcn `Sheet` primitive auto-renders a close button of its own, whose - * only accessible name is a hardcoded English `sr-only` span - * (`packages/components/src/ui/sheet.tsx:80` — an upstream No-Touch zone, - * AGENTS.md #7). `NavigationOverlay` CSS-hides it with - * `[&>button:last-of-type]:hidden`, so a real browser drops it from the - * accessibility tree — but jsdom does not apply Tailwind, so RTL still sees it - * and `getByRole('button', { name: 'Close' })` matches two elements under `en`. + * The shadcn `Sheet` primitive auto-renders a close button of its own. + * `NavigationOverlay` CSS-hides it with `[&>button:last-of-type]:hidden`, so a + * real browser drops it from the accessibility tree — but jsdom does not apply + * Tailwind, so RTL still sees it and role+name queries match TWO elements. * - * Ours is the only close carrying a `title`, so that is the precise handle. The - * primitive's own untranslated label is a separate, out-of-scope finding. + * Ours is the only close carrying a `title`, so that is the precise handle. + * + * As of objectstack#5505 the primitive's own label is translated too (it + * renders `` rather than a hardcoded English span), so the + * duplicate now appears under EVERY locale rather than only under `en` — under + * `zh` both buttons are named 关闭. The bare `getByRole` assertions that used + * to work here therefore became "found multiple elements" errors, and are now + * written as `getAllByRole(...)` containment checks against our titled button. + * That keeps the original intent — our control is reachable by role + name — + * without asserting anything about how many close buttons jsdom can see. */ describe('NavigationOverlay drawer close — accessible name (objectstack#5430)', () => { it('still reads English under an en session', () => { @@ -90,18 +95,20 @@ describe('NavigationOverlay drawer close — accessible name (objectstack#5430)' it('reads the zh bundle value under a zh session', () => { renderDrawerIn('zh'); - expect(screen.getByTitle('关闭').getAttribute('aria-label')).toBe('关闭'); - expect(screen.getByRole('button', { name: '关闭' })).toBeTruthy(); + const ours = screen.getByTitle('关闭'); + expect(ours.getAttribute('aria-label')).toBe('关闭'); + expect(screen.getAllByRole('button', { name: '关闭' })).toContain(ours); // The literal this replaced, scoped to OUR button via `title` so the - // primitive's hidden English one cannot mask a re-inlined string here. + // primitive's own close cannot mask a re-inlined string here. expect(screen.queryByTitle('Close')).toBeNull(); }); it('reads the de bundle value under a de session', () => { renderDrawerIn('de'); - expect(screen.getByTitle('Schließen').getAttribute('aria-label')).toBe('Schließen'); - expect(screen.getByRole('button', { name: 'Schließen' })).toBeTruthy(); + const ours = screen.getByTitle('Schließen'); + expect(ours.getAttribute('aria-label')).toBe('Schließen'); + expect(screen.getAllByRole('button', { name: 'Schließen' })).toContain(ours); expect(screen.queryByTitle('Close')).toBeNull(); }); }); diff --git a/packages/components/src/__tests__/sheet-dialog-close-i18n.test.tsx b/packages/components/src/__tests__/sheet-dialog-close-i18n.test.tsx new file mode 100644 index 0000000000..03d56d105e --- /dev/null +++ b/packages/components/src/__tests__/sheet-dialog-close-i18n.test.tsx @@ -0,0 +1,123 @@ +/** + * 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. + */ + +/** + * The Shadcn `Sheet`/`Dialog` close buttons speak the session locale — + * objectstack#5505. + * + * Both primitives auto-render a close button that is icon-only (a lucide `X`), + * so its `sr-only` span is not decoration: it IS the control's accessible + * name. Upstream ships that span as a hardcoded English literal, so under + * zh/ja/es every drawer and modal in the console — ~20 `SheetContent` + * consumers plus every `DialogContent` consumer (ChatDock, ActivityFeed, + * metadata-admin, AiChatPage, PeoplePicker, …) — announced "Close" in English. + * + * The span now renders `` (`../lib/close-label`), which + * resolves `common.close`. Because `src/ui/**` is regenerated from the + * registry, that one-line reference is DECLARED in + * `scripts/shadcn-local-patches.mjs` and re-applied by every sync; the patch + * mechanism itself is tested in + * `scripts/__tests__/shadcn-local-patches.test.ts`. + * + * ## Why the no-provider cases matter as much as the translated ones + * + * A large amount of existing coverage addresses these controls by their + * English name with NO `I18nProvider` mounted (`discardGuard.test.tsx`, + * `InlineCreateRelated.closeButtonName.test.tsx`, e2e specs). The safe + * translation's English fallback is what keeps those green, so it is pinned + * here explicitly rather than assumed. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; +import { I18nProvider } from '@object-ui/i18n'; +import { Sheet, SheetContent, SheetTitle, SheetDescription } from '../ui/sheet'; +import { Dialog, DialogContent, DialogTitle, DialogDescription } from '../ui/dialog'; + +afterEach(() => cleanup()); + +/** Sheet/Dialog bodies carry a title + description so Radix has no a11y complaint. */ +function sheetBody() { + return ( + + + Acme Corp + Record drawer + + + ); +} + +function dialogBody() { + return ( + + + Acme Corp + Record modal + + + ); +} + +function inLocale(language: string, body: React.ReactElement) { + return render( + + {body} + , + ); +} + +const primitives: Array<[string, () => React.ReactElement]> = [ + ['SheetContent', sheetBody], + ['DialogContent', dialogBody], +]; + +describe.each(primitives)('%s close button — accessible name (objectstack#5505)', (_name, body) => { + it('reads English under an en session', () => { + inLocale('en', body()); + + expect(screen.getByRole('button', { name: 'Close' })).toBeTruthy(); + }); + + it('reads the zh bundle value under a zh session', () => { + inLocale('zh', body()); + + expect(screen.getByRole('button', { name: '关闭' })).toBeTruthy(); + // The literal this replaced. Before the fix this query MATCHED under zh — + // that is the whole bug. + expect(screen.queryByRole('button', { name: 'Close' })).toBeNull(); + expect(screen.queryByText('Close')).toBeNull(); + }); + + it('reads the ja bundle value under a ja session', () => { + inLocale('ja', body()); + + expect(screen.getByRole('button', { name: '閉じる' })).toBeTruthy(); + expect(screen.queryByRole('button', { name: 'Close' })).toBeNull(); + }); + + it('reads the es bundle value under an es session', () => { + inLocale('es', body()); + + expect(screen.getByRole('button', { name: 'Cerrar' })).toBeTruthy(); + expect(screen.queryByRole('button', { name: 'Close' })).toBeNull(); + }); +}); + +/** + * The English no-provider fallback is pinned in a FILE OF ITS OWN — + * `sheet-dialog-close-no-provider-fallback.test.tsx`. + * + * `createI18n` registers its instance as react-i18next's module-global + * default, and that registration survives unmount and `cleanup()`. So any + * "no provider" render placed in THIS file would silently resolve against + * whichever locale the tests above mounted last. Written here first, the + * fallback case failed with the button named "Cerrar" under a test that + * mounted no provider at all — see the same warning on + * `chrome-i18n-no-provider-fallback.test.tsx` (objectstack#5506). + */ diff --git a/packages/components/src/__tests__/sheet-dialog-close-no-provider-fallback.test.tsx b/packages/components/src/__tests__/sheet-dialog-close-no-provider-fallback.test.tsx new file mode 100644 index 0000000000..b23ea2e915 --- /dev/null +++ b/packages/components/src/__tests__/sheet-dialog-close-no-provider-fallback.test.tsx @@ -0,0 +1,76 @@ +/** + * 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. + */ + +/** + * The `Sheet`/`Dialog` close buttons still resolve to ENGLISH when no + * `I18nProvider` is mounted — objectstack#5505. + * + * This is the load-bearing half of the fix, not a formality. Routing the + * primitives' `sr-only` label through `t()` touches every `SheetContent` and + * `DialogContent` consumer in the repo, and a large amount of existing + * coverage addresses those controls by their ENGLISH accessible name with no + * provider in the tree — `packages/plugin-form/src/discardGuard.test.tsx` + * (`getByRole('button', { name: 'Close' })`) and + * `packages/plugin-detail/src/__tests__/InlineCreateRelated.closeButtonName.test.tsx` + * (`/^Close$/`) among them. A `t()` call without a working default renders the + * raw `common.close` key and breaks all of it — in other packages' suites, not + * this one's. + * + * ── Why this is its own FILE, not a describe block ──────────────────────── + * `createI18n` calls `instance.use(initReactI18next)`, which registers that + * instance as react-i18next's module-global default, and the registration + * survives unmount and `cleanup()`. The moment any test in a file mounts + * ``, every later "no + * provider" render in that same file resolves against the Spanish instance. + * Written as a describe block inside + * `sheet-dialog-close-i18n.test.tsx` this case failed with the button named + * "Cerrar" under a test that mounted no provider at all — the identical trap + * `chrome-i18n-no-provider-fallback.test.tsx` documents for objectstack#5506. + * + * Vitest's `dom` project runs with `isolate: true`, so a file that never + * mounts a provider gets a genuinely clean global. Keep it that way: + * **do not import or mount `I18nProvider` here.** + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; +import { Sheet, SheetContent, SheetTitle, SheetDescription } from '../ui/sheet'; +import { Dialog, DialogContent, DialogTitle, DialogDescription } from '../ui/dialog'; + +afterEach(() => cleanup()); + +describe('Sheet/Dialog close — English fallback with no provider (objectstack#5505)', () => { + it('names SheetContent’s close button "Close"', () => { + render( + + + Acme Corp + Record drawer + + , + ); + + expect(screen.getByRole('button', { name: 'Close' })).toBeTruthy(); + // Never the raw key — that is the shape that would break the consumers above. + expect(screen.queryByText('common.close')).toBeNull(); + }); + + it('names DialogContent’s close button "Close"', () => { + render( + + + Acme Corp + Record modal + + , + ); + + expect(screen.getByRole('button', { name: 'Close' })).toBeTruthy(); + expect(screen.queryByText('common.close')).toBeNull(); + }); +}); diff --git a/packages/components/src/lib/close-label.tsx b/packages/components/src/lib/close-label.tsx new file mode 100644 index 0000000000..f550642a6b --- /dev/null +++ b/packages/components/src/lib/close-label.tsx @@ -0,0 +1,85 @@ +/** + * 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. + */ + +"use client" + +/** + * Translated accessible name for the close button the Shadcn `Sheet` and + * `Dialog` primitives auto-render — objectstack#5505. + * + * Those buttons are icon-only (a lucide `X`), so their `sr-only` span is not + * decoration: it IS the control's accessible name, the thing a screen reader + * announces and the thing `getByRole('button', { name })` matches. Upstream + * ships it as a hardcoded English literal, so under zh/ja/es every drawer and + * modal in the console announced "Close" in English. + * + * ## Why the implementation lives HERE and not in the primitives + * + * `packages/components/src/ui/**` is regenerated from the Shadcn registry + * (AGENTS.md Commandment #7) — anything written there is overwritten by the + * next `pnpm shadcn:update`. `src/lib/` is not part of that regeneration, so + * this file is permanent. The primitives get only a one-line + * `` reference, re-applied on every sync by the declared patch + * in `scripts/shadcn-local-patches.mjs`. Keeping the payload out of the + * regenerated zone is what makes the patch small enough to survive upstream + * churn: two anchored lines rather than an inlined hook. + * + * ## Why `createSafeTranslation` + * + * The no-provider path must stay English. A large number of existing unit + * tests and e2e specs address dialogs and drawers by their English accessible + * name with no `I18nProvider` mounted (e.g. + * `packages/plugin-form/src/discardGuard.test.tsx`, + * `packages/plugin-detail/src/__tests__/InlineCreateRelated.closeButtonName.test.tsx`), + * and a primitive that rendered a raw `common.close` key there would break all + * of them. `createSafeTranslation` probes its test key and falls back to the + * defaults map below when translations are not configured, so "no provider" + * resolves to "Close" rather than to a key. + */ + +import * as React from "react" +import { createSafeTranslation } from "@object-ui/i18n" + +/** + * The English fallback, used when no `I18nProvider` is mounted. + * + * `common.close` is deliberately the key the rest of the console already uses + * for a bare "Close" (present in all ten locale packs since objectstack#5430), + * not a new primitive-private one. + * + * Deliberately NOT exported: this file's only export is a component, which is + * what `react-refresh/only-export-components` wants, and nothing outside needs + * the map — consumers that want the string should read `common.close` from the + * locale packs like everyone else. + */ +const CLOSE_LABEL_DEFAULT_TRANSLATIONS: Record = { + 'common.close': 'Close', +} + +/** + * Probe key is `common.close` itself: it exists in every shipped pack, so a + * mounted provider always resolves it and the real `t` is used; with no + * provider the probe fails and the default above is returned. + */ +const useCloseTranslation = createSafeTranslation( + CLOSE_LABEL_DEFAULT_TRANSLATIONS, + 'common.close', +) + +/** + * `sr-only` accessible name for an icon-only close control. + * + * A component rather than a helper call because it owns a hook — the + * primitives render it as `` inside their close button, which + * keeps the hook call legal in files whose components are expression-bodied + * `forwardRef` arrows with no statement block to put a hook in. + */ +export function CloseSrLabel(): React.ReactElement { + const { t } = useCloseTranslation() + return {t('common.close')} +} diff --git a/packages/components/src/ui/dialog.tsx b/packages/components/src/ui/dialog.tsx index 3b8b6652d5..873cf5b55d 100644 --- a/packages/components/src/ui/dialog.tsx +++ b/packages/components/src/ui/dialog.tsx @@ -13,6 +13,7 @@ import * as DialogPrimitive from "@radix-ui/react-dialog" import { X } from "lucide-react" import { cn } from "../lib/utils" +import { CloseSrLabel } from "../lib/close-label" const Dialog = DialogPrimitive.Root @@ -54,7 +55,7 @@ const DialogContent = React.forwardRef< {children} - Close + diff --git a/packages/components/src/ui/sheet.tsx b/packages/components/src/ui/sheet.tsx index ef40442f58..91fb0e185d 100644 --- a/packages/components/src/ui/sheet.tsx +++ b/packages/components/src/ui/sheet.tsx @@ -14,6 +14,7 @@ import { cva, type VariantProps } from "class-variance-authority" import { X } from "lucide-react" import { cn } from "../lib/utils" +import { CloseSrLabel } from "../lib/close-label" const Sheet = SheetPrimitive.Root @@ -77,7 +78,7 @@ const SheetContent = React.forwardRef< {children} - Close + diff --git a/scripts/__tests__/shadcn-local-patches.test.ts b/scripts/__tests__/shadcn-local-patches.test.ts new file mode 100644 index 0000000000..e4c788ced0 --- /dev/null +++ b/scripts/__tests__/shadcn-local-patches.test.ts @@ -0,0 +1,190 @@ +import { describe, expect, it } from 'vitest'; +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +// @ts-expect-error — plain-JS CI helper, intentionally untyped +import { + LOCAL_PATCHES, + applyLocalPatches, + verifyLocalPatches, + patchedComponents, + describePatchFailure, +} from '../shadcn-local-patches.mjs'; + +/** + * objectstack#5505 — the Shadcn `Sheet`/`Dialog` primitives shipped a hardcoded + * English `Close` sr-only span, which (being icon-only buttons) IS their + * accessible name, so every drawer and modal announced "Close" under zh/ja/es. + * + * `packages/components/src/ui/**` is regenerated from the upstream registry, so + * the fix could not simply be typed into those files — the next + * `pnpm shadcn:update` would revert it, silently and compilably. The fix is + * therefore DECLARED in `scripts/shadcn-local-patches.mjs` and re-applied by + * the sync itself. + * + * This file is the enforcement half of that contract, and it is deliberately + * offline: the registry is not reachable from CI (nor from the sandbox this was + * written in), so a test that needed the network could not gate anything. Every + * assertion below is pure string work over a fixture or over the files on disk. + * + * It covers three separate regressions: + * + * 1. the patch stops being applied to the files we ship (`describe` #3) + * 2. the patch engine stops applying it to fresh upstream (`describe` #1) + * 3. the patch silently no-ops against changed upstream (`describe` #2) + */ + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); +const uiDir = path.join(repoRoot, 'packages/components/src/ui'); + +/** + * A faithful excerpt of what the registry serves for `dialog`, AFTER + * `rewriteRegistryImports` (so `@/lib/utils` already reads `../lib/utils`). + * This is the exact shape the patch engine sees during `--update`. + */ +const UPSTREAM_DIALOG = `"use client" + +import * as React from "react" +import * as DialogPrimitive from "@radix-ui/react-dialog" +import { X } from "lucide-react" + +import { cn } from "../lib/utils" + +const DialogContent = React.forwardRef((props, ref) => ( + + + + {children} + + + Close + + + +)) +`; + +describe('shadcn local patches — application to fresh upstream (objectstack#5505)', () => { + it.each(patchedComponents())('%s declares the i18n close patch', (name: string) => { + const ids = LOCAL_PATCHES[name].map((p: { id: string }) => p.id); + expect(ids).toEqual([`${name}-i18n-close-import`, `${name}-i18n-close-label`]); + }); + + it('turns an unpatched upstream file into the translated form', () => { + const result = applyLocalPatches('dialog', UPSTREAM_DIALOG); + + expect(result.failed).toEqual([]); + expect(result.applied).toHaveLength(2); + // The English literal is GONE — this is the actual user-visible defect. + expect(result.content).not.toContain('Close'); + expect(result.content).toContain(''); + expect(result.content).toContain('import { CloseSrLabel } from "../lib/close-label"'); + // The anchor line itself survives; the import is added beside it. + expect(result.content).toContain('import { cn } from "../lib/utils"'); + }); + + it('is idempotent — re-running over already-patched content changes nothing', () => { + const once = applyLocalPatches('dialog', UPSTREAM_DIALOG); + const twice = applyLocalPatches('dialog', once.content); + + expect(twice.content).toBe(once.content); + expect(twice.applied).toEqual([]); + expect(twice.already).toHaveLength(2); + expect(twice.failed).toEqual([]); + }); + + it('leaves components with no declared patches untouched', () => { + const input = 'const Button = () => null\n'; + const result = applyLocalPatches('button', input); + + expect(result.content).toBe(input); + expect(result.applied).toEqual([]); + expect(result.failed).toEqual([]); + }); +}); + +describe('shadcn local patches — loud failure when upstream moves (objectstack#5505)', () => { + it('refuses (does not silently no-op) when the label anchor is gone', () => { + // A plausible future upstream: the close button keeps its shape but the + // label text changes. The old anchor no longer matches. + const moved = UPSTREAM_DIALOG.replace( + 'Close', + 'Close dialog', + ); + + const result = applyLocalPatches('dialog', moved); + + expect(result.failed.map((p: { id: string }) => p.id)).toEqual(['dialog-i18n-close-label']); + expect(result.failed[0].found).toBe(0); + // Critically: the content is NOT silently returned as "fine". A caller that + // ignored `failed` would ship an untranslated primitive again. + expect(result.content).not.toContain(''); + }); + + it('refuses when the import anchor is gone', () => { + const moved = UPSTREAM_DIALOG.replace( + 'import { cn } from "../lib/utils"', + 'import { cn, cva } from "../lib/utils"', + ); + + const result = applyLocalPatches('dialog', moved); + + expect(result.failed.map((p: { id: string }) => p.id)).toEqual(['dialog-i18n-close-import']); + }); + + it('refuses when the anchor became AMBIGUOUS rather than absent', () => { + // Two close buttons upstream: patching "the" span is no longer well + // defined, so guessing is worse than stopping. `occurrences` pins this. + const doubled = UPSTREAM_DIALOG.replace( + 'Close', + 'Close\n Close', + ); + + const result = applyLocalPatches('dialog', doubled); + + expect(result.failed.map((p: { id: string }) => p.id)).toEqual(['dialog-i18n-close-label']); + expect(result.failed[0].found).toBe(2); + }); + + it('explains a failure with id, issue and reason — not just "changed"', () => { + const moved = UPSTREAM_DIALOG.replace('Close', ''); + const [failure] = applyLocalPatches('dialog', moved).failed; + + const text = describePatchFailure('dialog', failure).join('\n'); + + expect(text).toContain('dialog-i18n-close-label'); + expect(text).toContain('objectstack#5505'); + expect(text).toContain('accessible name'); + }); +}); + +describe('shipped primitives still carry every declared patch (objectstack#5505)', () => { + /** + * The PR gate. If a future forced sync, hand edit or bad merge drops the + * patch from the files we actually ship, this goes red — offline, on every + * PR, without needing anyone to run `pnpm shadcn:check` against the network. + */ + it.each(patchedComponents())('%s.tsx carries its declared patches', (name: string) => { + const content = fs.readFileSync(path.join(uiDir, `${name}.tsx`), 'utf-8'); + + const missing = verifyLocalPatches(name, content); + expect( + missing.map((p: { id: string }) => p.id), + `${name}.tsx lost declared patch(es); re-apply with: node scripts/shadcn-sync.js --update ${name}`, + ).toEqual([]); + }); + + /** + * The negative assertion the issue asked for, at the source level: the + * English literal must not be back in the file under any spelling of the + * span. Rendering-level negatives live in + * `packages/components/src/__tests__/sheet-dialog-close-i18n.test.tsx`. + */ + it.each(patchedComponents())('%s.tsx has no hardcoded English close label', (name: string) => { + const content = fs.readFileSync(path.join(uiDir, `${name}.tsx`), 'utf-8'); + + expect(content).not.toMatch(/\s*Close\s*<\/span>/); + expect(content).toContain(''); + }); +}); diff --git a/scripts/shadcn-local-patches.mjs b/scripts/shadcn-local-patches.mjs new file mode 100644 index 0000000000..2c33a3f183 --- /dev/null +++ b/scripts/shadcn-local-patches.mjs @@ -0,0 +1,212 @@ +/** + * Declarative local patches for the regenerated Shadcn primitives. + * + * ## Why this file exists + * + * `packages/components/src/ui/**` is a No-Touch zone (AGENTS.md Commandment + * #7): every file in it is overwritten by `pnpm shadcn:update` from the + * upstream registry. That is fine for the 99% of those files we take as-is, + * and `shadcn-sync.js` already protects hand edits *defensively* — it counts + * `localOnlyLines` and REFUSES to overwrite a diverged file. + * + * Refusal is not enough for a patch that must never silently disappear: + * + * 1. `--force` bypasses the refusal by design, and takes the edit with it. + * 2. A type-check cannot see the loss. Upstream drops a local addition *and* + * its usages consistently, so the regenerated file still compiles — this + * is the `command.tsx` failure mode documented in `shadcn-sync.js`. + * 3. Nothing declares WHICH lines were load-bearing, so a reviewer staring + * at a post-sync diff has no way to tell a deliberate fix from drift. + * + * So the edits that MUST survive regeneration are declared here as data, and + * the sync script re-applies them on every write. The patch is no longer a + * hand edit someone has to remember to re-do; it is part of what "sync" means. + * + * ## Declared = enforced + * + * Every patch carries `marker` (what the patched file must contain) and + * `find`/`occurrences` (the upstream anchor it needs). That makes three + * different regressions loud instead of silent: + * + * - `verifyLocalPatches()` — the file on disk lost its marker (a forced + * sync, a bad merge, a hand edit). Offline, so it can gate every PR. + * - `applyLocalPatches()` on freshly fetched upstream — the anchor is gone + * or has moved, i.e. the patch will NOT survive the next regeneration. + * `shadcn:check` reports this before anyone runs `--update`. + * - `applyLocalPatches()` during `--update` — same, and the write is + * refused rather than producing a file with the patch quietly missing. + * + * ## Adding a patch + * + * Keep the payload OUT of `src/ui/`. A patch should be a one-line reference to + * code that lives somewhere the sync never touches (here: `src/lib/`), because + * a small anchored reference survives upstream churn far better than an + * inlined implementation, and the implementation itself stays reviewable and + * unit-testable in a normal file. + */ + +/** + * @typedef {object} LocalPatch + * @property {string} id Stable identifier, used in messages. + * @property {string} issue Tracking issue for the divergence. + * @property {string} reason Why this edit exists, in prose. + * @property {string} find Literal upstream anchor (NOT a regex). + * @property {string} replace Literal replacement. + * @property {string} marker Substring that proves the patch is applied. + * @property {number} occurrences Exact number of `find` hits expected. + */ + +/** + * The i18n patch, shared by `sheet` and `dialog`. + * + * Both primitives auto-render an icon-only close button whose ONLY accessible + * name is a hardcoded English `sr-only` span, so under zh/ja/es every drawer + * and modal in the console announced "Close" in English (objectstack#5505). + * + * The two patches per file are deliberately the smallest possible edit: add an + * import, swap one element. All of the actual i18n behaviour — the safe + * translation hook, the English fallback, the defaults map — lives in + * `packages/components/src/lib/close-label.tsx`, which the sync never + * regenerates. + * + * @param {string} primitive Human-readable primitive name, for the reason text. + * @returns {LocalPatch[]} + */ +function i18nCloseLabelPatches(primitive) { + return [ + { + id: `${primitive.toLowerCase()}-i18n-close-import`, + issue: 'objectstack#5505', + reason: + `Imports the shared translated close label used by ${primitive}Content's ` + + 'auto-rendered close button. Anchored on the `cn` import, which every ' + + 'Shadcn component carries.', + find: 'import { cn } from "../lib/utils"', + replace: + 'import { cn } from "../lib/utils"\nimport { CloseSrLabel } from "../lib/close-label"', + marker: 'from "../lib/close-label"', + occurrences: 1, + }, + { + id: `${primitive.toLowerCase()}-i18n-close-label`, + issue: 'objectstack#5505', + reason: + `${primitive}Content's close button is icon-only (lucide X), so this ` + + 'sr-only span IS the control\'s accessible name. Upstream hardcodes the ' + + 'English literal; CloseSrLabel resolves `common.close` for the session ' + + 'locale and falls back to "Close" when no I18nProvider is mounted.', + find: 'Close', + replace: '', + marker: '', + occurrences: 1, + }, + ]; +} + +/** + * Component name (as tracked in `shadcn-components.json`) → patches it needs. + * + * @type {Record} + */ +export const LOCAL_PATCHES = { + sheet: i18nCloseLabelPatches('Sheet'), + dialog: i18nCloseLabelPatches('Dialog'), +}; + +/** Components that carry at least one declared patch. */ +export function patchedComponents() { + return Object.keys(LOCAL_PATCHES); +} + +/** Count non-overlapping literal occurrences of `needle` in `haystack`. */ +function countOccurrences(haystack, needle) { + if (!needle) return 0; + return haystack.split(needle).length - 1; +} + +/** + * Re-apply every declared patch for `name` to `content`. + * + * Idempotent: a patch whose `marker` is already present is reported as + * `already` and the content is left alone, so running this over an + * already-patched local file is a no-op rather than a double application. + * + * A patch whose marker is absent AND whose anchor does not appear exactly + * `occurrences` times is a HARD failure — it means upstream restructured the + * code the patch depends on. Callers must refuse to write in that case: a + * silently-unapplied patch is precisely the regression this module exists to + * prevent. + * + * @param {string} name Component name, e.g. `sheet`. + * @param {string} content File content to patch. + * @returns {{ content: string, applied: LocalPatch[], already: LocalPatch[], + * failed: Array }} + */ +export function applyLocalPatches(name, content) { + const patches = LOCAL_PATCHES[name] || []; + const applied = []; + const already = []; + const failed = []; + let out = content; + + for (const patch of patches) { + if (out.includes(patch.marker)) { + already.push(patch); + continue; + } + + const found = countOccurrences(out, patch.find); + if (found !== patch.occurrences) { + // Anchor gone or duplicated. Either way we must not guess: applying a + // patch to the wrong place is worse than refusing to apply it. + failed.push({ ...patch, found }); + continue; + } + + out = out.split(patch.find).join(patch.replace); + applied.push(patch); + } + + return { content: out, applied, already, failed }; +} + +/** + * Which declared patches are MISSING from `content`. + * + * Pure string containment against `marker`, so this needs no network and no + * registry access — it is the check that can run on every PR and catch a + * patch that was reverted by a forced sync, a bad merge or a hand edit. + * + * @param {string} name + * @param {string} content + * @returns {LocalPatch[]} empty when the file carries every declared patch + */ +export function verifyLocalPatches(name, content) { + return (LOCAL_PATCHES[name] || []).filter((patch) => !content.includes(patch.marker)); +} + +/** + * Human-readable explanation of a violated or unappliable patch. + * + * Shared by the sync script's several failure paths so the operator always + * gets the id, the tracking issue and the reason — not just "something + * changed". + * + * @param {string} name + * @param {LocalPatch & { found?: number }} patch + * @returns {string[]} lines to print + */ +export function describePatchFailure(name, patch) { + const lines = [ + ` [${patch.id}] declared for ${name}.tsx (${patch.issue})`, + ` why: ${patch.reason}`, + ]; + if (typeof patch.found === 'number') { + lines.push( + ` anchor expected ${patch.occurrences}x, found ${patch.found}x: ${JSON.stringify(patch.find)}`, + ); + } else { + lines.push(` missing marker: ${JSON.stringify(patch.marker)}`); + } + return lines; +} diff --git a/scripts/shadcn-sync.js b/scripts/shadcn-sync.js index 4b1d72bff5..4e5b5fd4d2 100755 --- a/scripts/shadcn-sync.js +++ b/scripts/shadcn-sync.js @@ -27,6 +27,12 @@ import { fileURLToPath } from 'url'; import https from 'https'; import { spawnSync } from 'child_process'; import crypto from 'crypto'; +import { + LOCAL_PATCHES, + applyLocalPatches, + verifyLocalPatches, + describePatchFailure, +} from './shadcn-local-patches.mjs'; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); @@ -294,7 +300,35 @@ async function checkComponent(name, manifest, options = {}) { const registryData = await fetchRegistry(componentInfo.source, { allowCache: options.allowCache }); // Rewrite before comparing, so import-path style alone does not read as // a difference — the local file has already been through this transform. - const shadcnContent = rewriteRegistryImports(registryData.files?.[0]?.content || ''); + const rawShadcnContent = rewriteRegistryImports(registryData.files?.[0]?.content || ''); + + // A registry response we could not parse (proxy error page, 403 from an + // egress allowlist, schema change) yields empty content. Treat that as a + // fetch error rather than letting it flow into the comparisons below: + // against empty upstream EVERY local line reads as local-only and EVERY + // declared patch reads as unappliable, which would fire the patch gate + // for a network problem. The gate must only ever accuse real drift. + if (!rawShadcnContent.trim()) { + return { + name, + status: 'error', + missingPatches: verifyLocalPatches(name, localContent), + unappliablePatches: [], + message: 'Registry returned no usable file content (offline, blocked, or schema change)', + }; + } + + // Preflight the declared patches against what upstream serves RIGHT NOW. + // This is the early-warning half of the contract: it answers "would the + // next `--update` still be able to re-apply our required edits?" before + // anyone runs one. A failure here means upstream moved the anchor, and + // the patch must be re-targeted. + const upstreamPatched = applyLocalPatches(name, rawShadcnContent); + // Compare against upstream WITH the declared patches applied, so a + // component whose only divergence is a declared, mechanically-reapplied + // patch reads as `synced` rather than as mystery drift. Undeclared local + // edits still show up exactly as before. + const shadcnContent = upstreamPatched.content; const shadcnLines = shadcnContent.split('\n').length; // `localOnlyLines` is directional — lines in the first argument that the @@ -341,12 +375,20 @@ async function checkComponent(name, manifest, options = {}) { localOnly: localOnly.length, upstreamOnly: upstreamOnly.length, documented: Boolean(componentInfo.localEdits), + // Declared patches missing from the file ON DISK (offline signal). + missingPatches: verifyLocalPatches(name, localContent), + // Declared patches that would NOT re-apply to current upstream. + unappliablePatches: upstreamPatched.failed, message, }; } catch (fetchError) { return { name, status: 'error', + // The local-patch check needs no network, so it still reports even + // when the registry is unreachable (offline CI, egress allowlist). + missingPatches: verifyLocalPatches(name, localContent), + unappliablePatches: [], message: `Error fetching from registry: ${fetchError.message}`, }; } @@ -428,6 +470,41 @@ async function checkAllComponents(options = {}) { } } + // ── Declared local patches ──────────────────────────────────────────────── + // Two independent failures, both fatal. Everything above this point is + // advisory status reporting; this block is a gate, and `main()` turns a + // non-empty result into a non-zero exit. Silent reversion of a required edit + // is the failure mode the patch manifest exists to close, so it must never + // be reported as just another yellow line in a 46-component list. + const all = Object.values(results).flat(); + const reverted = all.filter((r) => r.missingPatches?.length > 0); + const unappliable = all.filter((r) => r.unappliablePatches?.length > 0); + + if (reverted.length > 0 || unappliable.length > 0) { + logSection('DECLARED LOCAL PATCHES — FAILED'); + } + + if (reverted.length > 0) { + log('✗ Required local patches are MISSING from the file on disk:', 'red'); + reverted.forEach((r) => + r.missingPatches.forEach((p) => describePatchFailure(r.name, p).forEach((l) => log(l, 'red'))), + ); + log('\n These edits are declared in scripts/shadcn-local-patches.mjs and are', 'yellow'); + log(' supposed to be re-applied by every sync. Finding them absent means the', 'yellow'); + log(' file was overwritten by `--force`, hand-edited, or mis-merged.', 'yellow'); + log(` Restore with: node scripts/shadcn-sync.js --update ${reverted[0].name}`, 'cyan'); + } + + if (unappliable.length > 0) { + log('\n✗ Required local patches NO LONGER APPLY to current upstream:', 'red'); + unappliable.forEach((r) => + r.unappliablePatches.forEach((p) => describePatchFailure(r.name, p).forEach((l) => log(l, 'red'))), + ); + log('\n The file on disk is still correct — but the next `--update` cannot', 'yellow'); + log(' re-apply these, so it will refuse to write rather than drop them.', 'yellow'); + log(' Re-target the anchors in scripts/shadcn-local-patches.mjs.', 'cyan'); + } + if (results.modified.length > 0) { const undocumented = results.modified.filter((r) => !r.documented); const documented = results.modified.filter((r) => r.documented); @@ -445,6 +522,10 @@ async function checkAllComponents(options = {}) { } } + // Unique components, not the sum: one component can be in BOTH lists (its + // file lost the patch AND upstream moved the anchor), and reporting that as + // "2 components" would be wrong. + results.patchFailures = new Set([...reverted, ...unappliable].map((r) => r.name)).size; return results; } @@ -492,6 +573,31 @@ async function updateComponent(name, manifest, options = {}) { // Transform imports to match ObjectUI structure let content = rewriteRegistryImports(registryData.files[0].content); + // Re-apply the declared local patches (scripts/shadcn-local-patches.mjs). + // + // This is what makes a required edit survive regeneration instead of being + // something a human has to remember to re-do. It runs BEFORE the local-edit + // refusal below on purpose: with the patches re-applied, a component whose + // only divergence is a declared patch compares equal to upstream and syncs + // cleanly, so the refusal keeps signalling only UNDECLARED divergence. + const patched = applyLocalPatches(name, content); + if (patched.failed.length > 0) { + // The anchor a patch needs is gone (or ambiguous) — upstream restructured + // the code it depends on. Writing anyway would produce a file that + // compiles and looks fine while silently missing the fix, which is the + // exact regression the patch manifest exists to prevent. Refuse. + log(`✗ Refusing to write ${name}.tsx — ${patched.failed.length} declared local patch(es) no longer apply:`, 'red'); + patched.failed.forEach((p) => describePatchFailure(name, p).forEach((l) => log(l, 'yellow'))); + log(` Upstream changed the code these patches anchor on. Re-target them in`, 'yellow'); + log(` scripts/shadcn-local-patches.mjs, then re-run. Do NOT --force past this:`, 'yellow'); + log(` --force writes upstream verbatim and the patched behaviour is lost.`, 'yellow'); + return false; + } + content = patched.content; + if (patched.applied.length > 0) { + log(` ↺ Re-applied ${patched.applied.length} declared local patch(es): ${patched.applied.map((p) => p.id).join(', ')}`, 'cyan'); + } + // Fail closed. An unmapped `@/…` specifier does not resolve from src/ui/, // so writing the file would swap working code for code that cannot // compile. Better to refuse and have someone extend IMPORT_REWRITES. @@ -553,7 +659,18 @@ async function updateComponent(name, manifest, options = {}) { // Write updated component const targetPath = path.join(COMPONENTS_DIR, `${name}.tsx`); await fs.writeFile(targetPath, content, 'utf-8'); - + + // Post-write assertion. `applyLocalPatches` already guarantees this, so a + // hit here means the engine itself regressed — still worth saying out loud + // rather than trusting an invariant we never check. + const missingAfterWrite = verifyLocalPatches(name, content); + if (missingAfterWrite.length > 0) { + log(`✗ ${name}.tsx was written WITHOUT ${missingAfterWrite.length} declared patch(es):`, 'red'); + missingAfterWrite.forEach((p) => describePatchFailure(name, p).forEach((l) => log(l, 'red'))); + log(` Roll back with: git checkout -- packages/components/src/ui/${name}.tsx`, 'cyan'); + return false; + } + log(`✓ Updated ${name}.tsx`, 'green'); // Check and log dependencies @@ -682,7 +799,16 @@ async function main() { const allowCache = !args.includes('--no-cache'); if (args.length === 0 || args.includes('--check')) { - await checkAllComponents({ allowCache }); + const results = await checkAllComponents({ allowCache }); + // Exit non-zero on a declared-patch failure ONLY. Ordinary drift + // (outdated/modified/undocumented) stays exit 0: it is status, and making + // it fatal would turn this command into noise nobody runs. A required edit + // that vanished, or one that can no longer be re-applied, is different in + // kind — it is a broken contract, so it breaks the build. + if (results.patchFailures > 0) { + log(`\n✗ ${results.patchFailures} component(s) with declared local patch failures — see above.`, 'red'); + process.exitCode = 1; + } } else if (args.includes('--update-all')) { const backup = args.includes('--backup'); const verify = !args.includes('--no-verify');