refactor(overlay-renderer): introduce brand overlay registry

.claude/skills/review-pr/SKILL.md

@@ -678,3 +678,24 @@ _This section is populated automatically by Step 8c as patterns are observed in
 **Check:** For each new or changed type in the diff, search `docs/PRODUCTION_REFACTOR_PLAN.md` for the type name or relevant phase section. Compare field names, types, and required/optional status between the spec and the implementation. Pay special attention to fields that downstream phase steps reference by name (grep the phase steps for `.fieldName` usage).
 **Verdict:** BLOCKER
 **First seen:** refactor/s1-brand-types — 2026-05-10
+
+### Underscore placeholder renamed to descriptive name, introducing a vacuous lint-disable
+**Category:** QUALITY
+**Trigger:** A PR renames a `_` (or `__`) destructuring placeholder to a more descriptive `_foo` name and adds an `eslint-disable-next-line @typescript-eslint/no-unused-vars` comment.
+**Check:** `git diff origin/main...HEAD | grep '_brand\|_result\|_value' | grep 'eslint-disable'` — look for any renamed placeholder that now has a suppress comment. Verify whether the original `_` idiom was already suppression-free.
+**Verdict:** BLOCKER — revert to `_` (no comment needed); if a descriptive name is wanted, use a type-only assertion or a comment instead.
+**First seen:** refactor/s1-brand-registry — 2026-05-13
+
+### eslint-disable comments added for rules not active in the project ESLint config
+**Category:** QUALITY
+**Trigger:** A PR adds `eslint-disable` / `eslint-disable-next-line` comments citing a rule (e.g. `@typescript-eslint/no-explicit-any`) that is not present in `.eslintrc`, `eslint.config.*`, or any extended preset in the project config.
+**Check:** After collecting `eslint-disable` findings in Step 4a, read the project `.eslintrc` (or `eslint.config.*`) and verify the cited rule is actually enabled. If the rule is not in the config, the comment is vacuous noise.
+**Verdict:** WARNING — vacuous suppress comments add clutter without benefit; remove them or add the rule to the config if it should be enforced.
+**First seen:** refactor/s1-brand-registry — 2026-05-13
+
+### Module-level constant references replaced by per-render object construction in a Remotion component
+**Category:** QUALITY
+**Trigger:** A Remotion component's render function (or function body) constructs a new object via spread (`{ ...A, ...B, ...fn() }`) in place of a previously-used module-level constant reference or ternary between two pre-built constants.
+**Check:** Search the diff for `const [a-zA-Z]+ = {` lines inside component function bodies in `remotion/components/*.tsx`. Verify whether the original code used a pre-built constant (no per-frame allocation) and whether the replacement creates a new object on every frame.
+**Verdict:** WARNING — wrap in `useMemo` with appropriate dependencies to avoid per-frame object churn at 60 fps.
+**First seen:** refactor/s1-brand-registry — 2026-05-13

CLAUDE.md

@@ -192,10 +192,11 @@ ty    = (0.5 - vp.cy) × 100%
 | `remotion/components/SegmentPlayer.tsx` | Jump-cut player, section builders | Extract hookTiming, captions (Phase 5) |
 | `remotion/components/CameraPlayer.tsx` | Camera shots, multi-angle viewport (779 lines) | Extract cameraShots lib → <350 lines (Phase 6) |
 | `remotion/components/HookOverlay.tsx` | Hook captions, Techybara (518 lines) | Extract captions.ts (Phase 5) |
-| `remotion/components/OverlayRenderer.tsx` | Graphics cue dispatcher | Remove brand hardcoding (Phase 0.5+5) |
+| `remotion/components/OverlayRenderer.tsx` | Graphics cue dispatcher; uses `CORE_TEMPLATE_MAP` + `getBrandOverlays(brand.id)` | Remove remaining brand hardcoding (Phase 0.5 Steps 6–7) |
+| `remotion/lib/brandRegistry.ts` | `getBrandOverlays(brandId)` — brand overlay registry; ragtech imports from current paths | Switch to `require()` form after overlays move to `brands/ragtech/components/` (Phase 0.5 Step 3) |
 | `remotion/types/transcript.ts` | `Segment`, `Token`, `TimeCut`, `Transcript` | Will import from `scripts/types/` (Phase 6) |
 | `remotion/types/camera.ts` | `CameraProfiles`, `CameraShot`, `CropViewport` | Will import from `scripts/types/` (Phase 6) |
-| `remotion/types/brand.ts` | Brand design tokens only | Extend with identity/hosts/mascot/audio (Phase 0.5) |
+| `remotion/types/brand.ts` | Brand design tokens + extended identity/hosts/mascot/audio; `id: string` field required by registry | Move overlays + parameterize (Phase 0.5 Steps 3, 6–7) |
 | `scripts/config/project.ts` | `ProjectFile` type, `readProject`/`writeProject`, `ProjectNotFoundError` | Sprint 1 Issue #1 |
 | `scripts/edit-transcript.js` | Sentence merging, `deriveCuts`, doc generation | Migrate to .ts (Phase 3) |
 | `scripts/sync/AudioSyncer.js` | FFT sync, `syncMultiple` | Add FFT tie-breaking (Phase 0) |

docs/PRODUCTION_REFACTOR_PLAN.md

@@ -294,6 +294,8 @@ export type BrandMascot = {
 };
 
 export type Brand = {
+  id: string;          // brand registry key (e.g. 'ragtech')
+
   // Existing (keep)
   colors: BrandColors;
   typography: BrandTypography;

docs/implementation-guides/REFACTOR_P0_BRAND.md

@@ -57,18 +57,15 @@ ls -la brands/ragtech/components/ | wc -l  # Should show 11 overlay files + inde
 ```
 
 ### Step 4: Create brand registry
-**Status:** ⏳ PENDING
-- [ ] Create `remotion/lib/brandRegistry.ts` with `getBrandOverlays(brandId)` static switch
+**Status:** ✅ DONE — `refactor/s1-brand-registry` — `getBrandOverlays(brandId)` static switch; ragtech overlays imported from current paths pending file migration (Step 3)
 
 **Status check:**
 ```bash
 ls remotion/lib/brandRegistry.ts
 ```
 
 ### Step 5: Update OverlayRenderer
-**Status:** ⏳ PENDING
-- [ ] Update `OverlayRenderer`: remove hardcoded keyword imports
-- [ ] Use `{ ...CORE_TEMPLATE_MAP, ...getBrandOverlays(brand.id) }`
+**Status:** ✅ DONE — `refactor/s1-brand-registry` — `CORE_TEMPLATE_MAP` + `SHORTFORM_OVERRIDES` + `getBrandOverlays(brand.id)` replace monolithic maps; `Brand.id` field added
 
 **Status check:**
 ```bash

docs/review-findings/2026-05-13-refactor-s1-brand-registry.md

@@ -0,0 +1,81 @@
+# Review: refactor/s1-brand-registry
+Date: 2026-05-13
+Reviewer: AI (review-pr skill) — session bias: CLEAN
+PR: NONE (description provided inline)
+
+## Verdict
+CHANGES REQUESTED
+
+## Summary
+This PR introduces the brand overlay registry (`getBrandOverlays`), refactors `OverlayRenderer` to dispatch brand overlays through the registry instead of hardcoded imports, and adds `id: string` to the `Brand` type to enable the registry pattern. It also fixes a `next/babel` → Jest CJS/ESM ordering bug. Two blockers were found: the `Brand` type spec in `PRODUCTION_REFACTOR_PLAN.md` was not updated to include `id`, and a destructuring rename (`_` → `_brand`) introduced a needless `eslint-disable` comment that is likely suppressing a rule not in the project's ESLint config.
+
+## Blockers (must fix before merge)
+
+### B1 — PRODUCTION_REFACTOR_PLAN.md: Brand type spec missing `id: string`
+- **Type:** QUALITY (spec / convention)
+- **File:** `docs/PRODUCTION_REFACTOR_PLAN.md` line 296
+- **Finding:** The `Brand` type definition in the spec block does not include `id: string`, but the implementation adds it to `remotion/types/brand.ts`. The spec code snippets at lines 357–360 already reference `brand.id`, making the type block internally inconsistent. Per the project convention ("Type shapes match spec — if a type is defined in docs/PRODUCTION_REFACTOR_PLAN.md, the implementation must use the exact field names…"), the spec must be updated when the implementation diverges from it.
+- **Fix:** Add `id: string` as the first field in the `Brand` type block at line 296 of `PRODUCTION_REFACTOR_PLAN.md`:
+  ```typescript
+  export type Brand = {
+    id: string;          // brand registry key (e.g. 'ragtech')
+    // Existing (keep)
+    colors: BrandColors;
+    ...
+  ```
+  This is a one-line spec update, not a code change.
+
+### B2 — OverlayRenderer.tsx:250: `_brand` rename introduces vacuous `eslint-disable` comment
+- **Type:** QUALITY
+- **File:** `remotion/components/OverlayRenderer.tsx` line 250
+- **Finding:** The original code used `const { brand: _, ...otherProps }` — the `_` prefix is the idiomatic TypeScript/ESLint convention for intentionally unused destructured bindings and requires no suppression. The PR renames it to `_brand` and adds `// eslint-disable-next-line @typescript-eslint/no-unused-vars`. The project's `.eslintrc` only configures `@remotion/recommended` — the `@typescript-eslint/no-unused-vars` rule does not appear to be active in this config, making the disable comment vacuous cargo-cult noise. The rename + comment is strictly worse than the original.
+- **Fix:** Revert to `const { brand: _, ...otherProps }` and remove the disable comment entirely.
+
+## Warnings (should address)
+
+### W1 — Four `@typescript-eslint/no-explicit-any` disable comments suppress a rule not in the ESLint config
+- **Type:** QUALITY
+- **File:** `remotion/components/OverlayRenderer.tsx` lines 28, 37, 252; `remotion/lib/brandRegistry.ts` line 17
+- **Finding:** Four `// eslint-disable-next-line @typescript-eslint/no-explicit-any` comments were added. The project's `.eslintrc` only enables `plugin:@remotion/recommended` — there is no `@typescript-eslint/recommended` or explicit `@typescript-eslint/no-explicit-any` rule configured. These suppress comments may be vacuous. The `React.FC<any>` pattern is pre-existing tracked technical debt (Phase 5 target: `remotion/types/overlayProps.ts` discriminated union per CLAUDE.md).
+- **Suggestion:** If `@typescript-eslint/no-explicit-any` is not active in this project's ESLint config, remove the suppress comments — they add noise without benefit. If the rule IS active (e.g. via the remotion plugin transitively), add a one-line justification to the PR description: "React.FC\<any\> suppress: blocked on Phase 5 overlayProps discriminated union."
+
+### W2 — `componentMap` reconstructed on every Remotion frame (performance regression)
+- **Type:** QUALITY
+- **File:** `remotion/components/OverlayRenderer.tsx` lines 78–82
+- **Finding:** The original code assigned `componentMap` via a ternary that selected between two pre-built module-level constants — zero per-frame allocation. The new code creates a new object every render via three spreads plus a `getBrandOverlays()` call. In Remotion at 60 fps, this produces 60 new objects/second with ~16 property assignments each. The `getBrandOverlays()` function itself is cheap, but the churn is unnecessary.
+- **Suggestion:** Memoize the map:
+  ```typescript
+  const componentMap = useMemo(() => ({
+    ...CORE_TEMPLATE_MAP,
+    ...(isShortForm ? SHORTFORM_OVERRIDES : {}),
+    ...getBrandOverlays(brand.id),
+  }), [brand.id, isShortForm]);
+  ```
+
+### W3 — Spread order gives brand overlays higher precedence than shortform overrides
+- **Type:** QUALITY
+- **File:** `remotion/components/OverlayRenderer.tsx` lines 78–82
+- **Finding:** The spread order is `[CORE_TEMPLATE_MAP, SHORTFORM_OVERRIDES, getBrandOverlays(brand.id)]`. This means brand overlay keys can silently override shortform variants (e.g. if a future brand supplies a `ConceptExplainer`, it overrides `ConceptExplainerShort` in short-form). For ragtech this is harmless (all 11 brand overlay keys are unique vs core/shortform keys), but the ordering is a footgun for future brands.
+- **Suggestion:** Either document the intentional precedence in a comment, or reverse brand and shortform: `[CORE_TEMPLATE_MAP, getBrandOverlays(brand.id), SHORTFORM_OVERRIDES]` to guarantee shortform variants always win.
+
+## Suggestions (optional improvements)
+
+- `remotion/lib/brandRegistry.ts`: Consider extracting the `RAGTECH_OVERLAYS` object as a named module-level constant. Currently the object literal is constructed inside the `if` branch on every call. Minor; the function is pure and cheap.
+- `remotion/components/OverlayRenderer.test.tsx`: The `passes isShortForm=true without crashing` test (smoke only — `.not.toThrow()`) is acceptable as a guard, but a follow-up test asserting that `SHORTFORM_OVERRIDES` entries are present in the resolved map would give stronger coverage of the shortform path.
+
+## Test plan verification
+
+| Item | Status | Notes |
+|------|--------|-------|
+| `npm test` passes | PASS | 253 tests, 11 suites, 0 failures; 2 skipped (pre-existing) |
+| `npm run test:react` passes | PASS | 7 tests, 2 suites |
+| `tsc --noEmit` | PASS | No errors |
+| `npm run test:e2e` | SKIPPED | No e2e-applicable changes in this PR |
+| [REMOTION-VISUAL] Remotion Studio scrub | NOT RUN | Human verification required (marked ✓ by author) |
+
+## Patterns observed
+
+- **B1** matches known pattern: "Implementation diverges from documented spec without updating the spec" (first seen refactor/s1-brand-types, 2026-05-10). The `id: string` field was added to the implementation without updating the `Brand` type block in `PRODUCTION_REFACTOR_PLAN.md`.
+- **B2** is a new pattern: renaming a `_` placeholder to a more descriptive `_foo` name breaks the idiomatic ESLint-ignore convention and introduces a vacuous disable comment. Recorded in SKILL.md.
+- **W1** is a new pattern: `eslint-disable` comments added for rules not present in the project ESLint config. Recorded in SKILL.md.
+- **W2** is a new pattern: per-render object spread replacing module-level constant references in a Remotion component body. Recorded in SKILL.md.

jest.config.js

@@ -44,7 +44,18 @@ const config = {
         '<rootDir>/tests/react/**/*.test.{ts,tsx}',
       ],
       transform: {
-        '^.+\\.[jt]sx?$': 'babel-jest',
+        // next/babel injects `import React` after the CJS transform; bypass it
+        // entirely for jest by disabling .babelrc and using an explicit config.
+        '^.+\\.[jt]sx?$': ['babel-jest', {
+          babelrc: false,
+          configFile: false,
+          presets: [
+            ['@babel/preset-env', { targets: { node: 'current' }, modules: 'commonjs' }],
+            ['@babel/preset-react', { runtime: 'automatic' }],
+            '@babel/preset-typescript',
+          ],
+          plugins: ['babel-plugin-transform-import-meta'],
+        }],
       },
       transformIgnorePatterns: ['node_modules/'],
       setupFilesAfterEnv: ['<rootDir>/tests/setup.react.ts'],

public/brand.json

@@ -1,4 +1,5 @@
 {
+  "id": "ragtech",
   "colors": {
     "primary":    "#eebf89",
     "secondary":  "#9cd2d0",

remotion/components/OverlayRenderer.test.tsx

@@ -0,0 +1,104 @@
+import React from 'react';
+import { render } from '@testing-library/react';
+import { OverlayRenderer } from './OverlayRenderer';
+import type { Brand } from '../types/brand';
+import * as brandRegistry from '../lib/brandRegistry';
+
+// Mock remotion hooks used by OverlayRenderer
+jest.mock('remotion', () => ({
+  useVideoConfig: () => ({ fps: 60, width: 1920, height: 1080, durationInFrames: 3600 }),
+  useCurrentFrame: () => 0,
+  Sequence: ({ children }: { children: React.ReactNode }) => <>{children}</>,
+}));
+
+// Mock registry so the test doesn't cascade into overlay component imports
+jest.mock('../lib/brandRegistry', () => ({
+  getBrandOverlays: jest.fn(() => ({})),
+}));
+
+// Mock all overlay components imported directly by OverlayRenderer
+jest.mock('./overlays/lower-thirds', () => ({
+  ConceptExplainer: jest.fn(() => null),
+  NameTitle: jest.fn(() => null),
+  ChapterMarker: jest.fn(() => null),
+}));
+jest.mock('./overlays/lower-thirds/ConceptExplainer.short', () => ({
+  ConceptExplainerShort: jest.fn(() => null),
+}));
+jest.mock('./overlays/lower-thirds/NameTitle.short', () => ({
+  NameTitleShort: jest.fn(() => null),
+}));
+jest.mock('./overlays/ImageWindowOverlay', () => ({
+  ImageWindowOverlay: jest.fn(() => null),
+}));
+jest.mock('./overlays/GifWindowOverlay', () => ({
+  GifWindowOverlay: jest.fn(() => null),
+}));
+
+const mockBrand: Brand = {
+  id: 'ragtech',
+  colors: {
+    primary: '#eebf89',
+    secondary: '#9cd2d0',
+    accent: '#ffa3a6',
+    background: '#fff3c2',
+    surface: '#1c1006',
+    text: { primary: '#fff', secondary: '#b0b0cc', onPrimary: '#0f0f1a' },
+    palette: [],
+  },
+  typography: {
+    fontFamily: 'Nunito',
+    fontSrc: '/fonts/Nunito.ttf',
+    weights: { regular: 400, semiBold: 600, bold: 700, extraBold: 800, black: 900 },
+  },
+  logo: '/assets/logo.png',
+  shape: { borderRadius: 12, borderRadiusSmall: 6 },
+  identity: { name: 'RAG Tech', terminalPath: '~/ragtech', socialHandle: '@ragtechdev' },
+  hosts: [],
+  mascot: { enabled: false, name: 'Techybara', assets: {} },
+  audio: { introOutroMusic: '/sounds/intro.mp3', backgroundMusic: '/sounds/bg.mp3' },
+  background: { episodeGridAssets: [] },
+};
+
+const baseProps = {
+  segments: [],
+  brand: mockBrand,
+  mainSections: [],
+  hookSections: [],
+  mainStartFrame: 0,
+};
+
+describe('OverlayRenderer', () => {
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
+  it('renders null when there are no segments', () => {
+    const { container } = render(<OverlayRenderer {...baseProps} />);
+    expect(container.firstChild).toBeNull();
+  });
+
+  it('renders null when all segments are cut', () => {
+    const { container } = render(
+      <OverlayRenderer
+        {...baseProps}
+        segments={[{
+          id: 1, start: 0, end: 5, speaker: 'Natasha',
+          text: 'hello', cut: true, tokens: [], cuts: [], graphics: [],
+        }]}
+      />
+    );
+    expect(container.firstChild).toBeNull();
+  });
+
+  it('delegates brand-specific overlays to getBrandOverlays with brand.id', () => {
+    render(<OverlayRenderer {...baseProps} />);
+    expect(brandRegistry.getBrandOverlays).toHaveBeenCalledWith('ragtech');
+  });
+
+  it('passes isShortForm=true without crashing', () => {
+    expect(() =>
+      render(<OverlayRenderer {...baseProps} isShortForm />)
+    ).not.toThrow();
+  });
+});