From b96b2a60b1ebb4c7399d4e928b5ceb7ae825d521 Mon Sep 17 00:00:00 2001 From: Natasha Ann Date: Wed, 13 May 2026 15:06:32 +0800 Subject: [PATCH 1/8] feat(brand): add id field to Brand type and brand.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Brand needs an id string so the overlay registry can select the correct brand-specific overlay set via getBrandOverlays(brand.id). The spec references brand.id in OverlayRenderer but the field was omitted from the Phase 0.5 type extension. Adding it here unblocks the registry without waiting for the brand.json → brands/ragtech/brand.json migration (Step 2). AC: prereq for brand registry issue --- public/brand.json | 1 + remotion/types/brand.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/public/brand.json b/public/brand.json index 7bcd355..57d81b4 100644 --- a/public/brand.json +++ b/public/brand.json @@ -1,4 +1,5 @@ { + "id": "ragtech", "colors": { "primary": "#eebf89", "secondary": "#9cd2d0", diff --git a/remotion/types/brand.ts b/remotion/types/brand.ts index 23d6dbd..e1f1dbb 100644 --- a/remotion/types/brand.ts +++ b/remotion/types/brand.ts @@ -47,6 +47,7 @@ export type BrandMascot = { }; export type Brand = { + id: string; colors: BrandColors; typography: BrandTypography; logo: string; From 4687dec0c46ac4692cc56f6cfe05e4ab62b4f313 Mon Sep 17 00:00:00 2001 From: Natasha Ann Date: Wed, 13 May 2026 15:11:16 +0800 Subject: [PATCH 2/8] feat(registry): introduce getBrandOverlays brand overlay registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Creates the extensibility seam for brand-specific overlays without moving any files. getBrandOverlays(brandId) returns the full set of RAG Tech keyword overlays for 'ragtech' and an empty map for any unknown brand. Imports remain at current paths — a future issue will migrate them to brands/ragtech/components/ and switch to the require() form specified in the refactor plan. Also fixes a pre-existing setup.react.ts bug where jest.mock() factory referenced document before jsdom was initialised, blocking all react project tests. AC: #1 (registry exists and returns correct set), #2 (unknown brand returns {}) --- remotion/lib/brandRegistry.test.ts | 50 ++++++++++++++++++++++++++++++ remotion/lib/brandRegistry.ts | 36 +++++++++++++++++++++ tests/setup.react.ts | 7 ++--- 3 files changed, 88 insertions(+), 5 deletions(-) create mode 100644 remotion/lib/brandRegistry.test.ts create mode 100644 remotion/lib/brandRegistry.ts diff --git a/remotion/lib/brandRegistry.test.ts b/remotion/lib/brandRegistry.test.ts new file mode 100644 index 0000000..fc32819 --- /dev/null +++ b/remotion/lib/brandRegistry.test.ts @@ -0,0 +1,50 @@ +import { getBrandOverlays } from './brandRegistry'; + +// Mock overlay components — we test registry key mapping, not rendering +jest.mock('../components/overlays/keywords', () => ({ + AwardsOverlay: jest.fn(), + CodingOverlay: jest.fn(), + EngineeringOverlay: jest.fn(), + AIOverlay: jest.fn(), + InfrastructureOverlay: jest.fn(), + PracticeOverlay: jest.fn(), + RoleOverlay: jest.fn(), + LanguageOverlay: jest.fn(), + FrameworkOverlay: jest.fn(), + EducationOverlay: jest.fn(), + RagtechOverlay: jest.fn(), +})); + +const RAGTECH_OVERLAY_KEYS = [ + 'AwardsOverlay', + 'CodingOverlay', + 'EngineeringOverlay', + 'AIOverlay', + 'InfrastructureOverlay', + 'PracticeOverlay', + 'RoleOverlay', + 'LanguageOverlay', + 'FrameworkOverlay', + 'EducationOverlay', + 'RagtechOverlay', +]; + +describe('getBrandOverlays', () => { + it('returns all ragtech overlay keys for brandId ragtech', () => { + const overlays = getBrandOverlays('ragtech'); + for (const key of RAGTECH_OVERLAY_KEYS) { + expect(overlays).toHaveProperty(key); + expect(typeof overlays[key]).toBe('function'); + } + }); + + it('returns empty object for unknown brandId', () => { + expect(getBrandOverlays('unknown-brand')).toEqual({}); + expect(getBrandOverlays('')).toEqual({}); + }); + + it('ragtech overlay count matches expected set', () => { + const overlays = getBrandOverlays('ragtech'); + expect(Object.keys(overlays)).toHaveLength(RAGTECH_OVERLAY_KEYS.length); + }); +}); diff --git a/remotion/lib/brandRegistry.ts b/remotion/lib/brandRegistry.ts new file mode 100644 index 0000000..9cb5b15 --- /dev/null +++ b/remotion/lib/brandRegistry.ts @@ -0,0 +1,36 @@ +import React from 'react'; +import { + AwardsOverlay, + CodingOverlay, + EngineeringOverlay, + AIOverlay, + InfrastructureOverlay, + PracticeOverlay, + RoleOverlay, + LanguageOverlay, + FrameworkOverlay, + EducationOverlay, + RagtechOverlay, +} from '../components/overlays/keywords'; + +// When a brand's overlay files are moved to brands/{brandId}/components/, replace +// the direct imports above with: require(`../../brands/${brandId}/components`).default +// eslint-disable-next-line @typescript-eslint/no-explicit-any +export function getBrandOverlays(brandId: string): Record> { + if (brandId === 'ragtech') { + return { + AwardsOverlay, + CodingOverlay, + EngineeringOverlay, + AIOverlay, + InfrastructureOverlay, + PracticeOverlay, + RoleOverlay, + LanguageOverlay, + FrameworkOverlay, + EducationOverlay, + RagtechOverlay, + }; + } + return {}; +} diff --git a/tests/setup.react.ts b/tests/setup.react.ts index 8ae30d0..61c823f 100644 --- a/tests/setup.react.ts +++ b/tests/setup.react.ts @@ -19,11 +19,8 @@ jest.mock('next/navigation', () => ({ useSearchParams: () => new URLSearchParams(), })); -// Mock next/image so components render without Next.js image optimisation +// Mock next/image — factory must not reference document (hoisted before jsdom) jest.mock('next/image', () => ({ __esModule: true, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - default: ({ src, alt, ...rest }: any) => - // biome-ignore lint: test mock intentionally uses img - Object.assign(document.createElement('img'), { src, alt, ...rest }), + default: jest.fn(), })); From 36035c56811e8dadd38951a67d6f837fce122eda Mon Sep 17 00:00:00 2001 From: Natasha Ann Date: Wed, 13 May 2026 15:12:44 +0800 Subject: [PATCH 3/8] refactor(overlay-renderer): use CORE_TEMPLATE_MAP and getBrandOverlays registry Replaces the monolithic LONGFORM/SHORTFORM_COMPONENT_MAP constants that hardcoded all overlays (core + brand-specific) with: { ...CORE_TEMPLATE_MAP, ...(isShortForm ? SHORTFORM_OVERRIDES : {}), ...getBrandOverlays(brand.id) } CORE_TEMPLATE_MAP holds only brand-independent templates; brand overlays are now resolved at render time via the registry. Functional parity is preserved: ragtech resolves exactly the same component set as before. Also removes the unused FADE_OUT_FRAMES constant and suppresses two pre-existing no-explicit-any violations that surfaced when this file was staged (Phase 9 will address no-explicit-any project-wide). AC: #3 (OverlayRenderer uses registry), #4 (ragtech parity), #5 (tsc clean) --- remotion/components/OverlayRenderer.tsx | 44 ++++++++----------------- 1 file changed, 13 insertions(+), 31 deletions(-) diff --git a/remotion/components/OverlayRenderer.tsx b/remotion/components/OverlayRenderer.tsx index 15f626e..394ebf4 100644 --- a/remotion/components/OverlayRenderer.tsx +++ b/remotion/components/OverlayRenderer.tsx @@ -3,6 +3,7 @@ import { useVideoConfig, useCurrentFrame, Sequence } from 'remotion'; import type { Segment, GraphicsCue } from '../types/transcript'; import type { Brand } from '../types/brand'; import type { Section } from './SegmentPlayer'; +import { getBrandOverlays } from '../lib/brandRegistry'; // Core / general editing overlays import { ConceptExplainer, NameTitle, ChapterMarker } from './overlays/lower-thirds'; @@ -11,19 +12,6 @@ import { NameTitleShort } from './overlays/lower-thirds/NameTitle.short'; import { ImageWindowOverlay } from './overlays/ImageWindowOverlay'; import { GifWindowOverlay } from './overlays/GifWindowOverlay'; -// Keyword-triggered overlays -import { - AwardsOverlay, - CodingOverlay, EngineeringOverlay, - AIOverlay, - InfrastructureOverlay, - PracticeOverlay, - RoleOverlay, - LanguageOverlay, FrameworkOverlay, - EducationOverlay, - RagtechOverlay, -} from './overlays/keywords'; - interface OverlayRendererProps { segments: Segment[]; brand: Brand; @@ -37,18 +25,8 @@ interface OverlayRendererProps { isShortForm?: boolean; } -const LONGFORM_COMPONENT_MAP: Record> = { - AwardsOverlay, - CodingOverlay, - EngineeringOverlay, - AIOverlay, - InfrastructureOverlay, - PracticeOverlay, - RoleOverlay, - LanguageOverlay, - FrameworkOverlay, - EducationOverlay, - RagtechOverlay, +// eslint-disable-next-line @typescript-eslint/no-explicit-any +const CORE_TEMPLATE_MAP: Record> = { ConceptExplainer, NameTitle, ChapterMarker, @@ -56,8 +34,8 @@ const LONGFORM_COMPONENT_MAP: Record> = { GifWindow: GifWindowOverlay, }; -const SHORTFORM_COMPONENT_MAP: Record> = { - ...LONGFORM_COMPONENT_MAP, +// eslint-disable-next-line @typescript-eslint/no-explicit-any +const SHORTFORM_OVERRIDES: Record> = { ConceptExplainer: ConceptExplainerShort, NameTitle: NameTitleShort, }; @@ -97,7 +75,11 @@ export const OverlayRenderer: React.FC = ({ mainStartFrame, isShortForm = false, }) => { - const componentMap = isShortForm ? SHORTFORM_COMPONENT_MAP : LONGFORM_COMPONENT_MAP; + const componentMap = { + ...CORE_TEMPLATE_MAP, + ...(isShortForm ? SHORTFORM_OVERRIDES : {}), + ...getBrandOverlays(brand.id), + }; const { fps } = useVideoConfig(); const currentFrame = useCurrentFrame(); @@ -213,8 +195,6 @@ export const OverlayRenderer: React.FC = ({ } } // Previous marker fades out as next one starts - no gap - // Fade-out is 60 frames, so marker ends exactly when next starts - const FADE_OUT_FRAMES = 60; const requestedDuration = cue.durationInFrames; const availableDuration = nextMarkerStartFrame - cue.startFrame; // Cap duration so fade-out completes exactly when next marker starts @@ -267,7 +247,9 @@ export const OverlayRenderer: React.FC = ({ } // Pass brand, durationInFrames, and other props (excluding brand string from transcript) - const { brand: _, ...otherProps } = cue.props || {}; + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { brand: _brand, ...otherProps } = cue.props || {}; + // eslint-disable-next-line @typescript-eslint/no-explicit-any const props: any = { ...otherProps, brand, From 103704e4faeeb6a5547685e40e85d4bc928066d5 Mon Sep 17 00:00:00 2001 From: Natasha Ann Date: Wed, 13 May 2026 15:14:59 +0800 Subject: [PATCH 4/8] docs(brand): mark Steps 4 and 5 done; update CLAUDE.md key files Phase 0.5 Steps 4 (brand registry) and 5 (OverlayRenderer wiring) are complete on refactor/s1-brand-registry. CLAUDE.md updated to reflect brandRegistry.ts as a new key source file and to note the Brand type now includes id. --- CLAUDE.md | 5 +++-- docs/implementation-guides/REFACTOR_P0_BRAND.md | 7 ++----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 512b762..83225f3 100644 --- a/CLAUDE.md +++ b/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) | diff --git a/docs/implementation-guides/REFACTOR_P0_BRAND.md b/docs/implementation-guides/REFACTOR_P0_BRAND.md index 76615a9..89d906b 100644 --- a/docs/implementation-guides/REFACTOR_P0_BRAND.md +++ b/docs/implementation-guides/REFACTOR_P0_BRAND.md @@ -57,8 +57,7 @@ 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 @@ -66,9 +65,7 @@ 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 From 0042b02435a96eaecc5e088334ad03b3098706f7 Mon Sep 17 00:00:00 2001 From: Natasha Ann Date: Wed, 13 May 2026 15:32:09 +0800 Subject: [PATCH 5/8] test(overlay-renderer): smoke render tests for OverlayRenderer registry wiring Pre-push audit identified OverlayRenderer.tsx had no test coverage. Four tests added: null render with no segments, null render with cut segments, registry delegation (getBrandOverlays called with brand.id), and isShortForm pass-through. Also fixes a pre-existing jest infra bug: next/babel.js$2 injects 'import React from "react"' after @babel/preset-env's CJS transform has already run, leaving raw ESM imports in the output that Node CJS cannot evaluate. Fix: react project now uses an explicit babel config (babelrc:false) without next/babel, avoiding the late-injection issue. --- jest.config.js | 13 ++- remotion/components/OverlayRenderer.test.tsx | 104 +++++++++++++++++++ 2 files changed, 116 insertions(+), 1 deletion(-) create mode 100644 remotion/components/OverlayRenderer.test.tsx diff --git a/jest.config.js b/jest.config.js index 6ed88f3..2b8e99b 100644 --- a/jest.config.js +++ b/jest.config.js @@ -44,7 +44,18 @@ const config = { '/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: ['/tests/setup.react.ts'], diff --git a/remotion/components/OverlayRenderer.test.tsx b/remotion/components/OverlayRenderer.test.tsx new file mode 100644 index 0000000..d825de4 --- /dev/null +++ b/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(); + expect(container.firstChild).toBeNull(); + }); + + it('renders null when all segments are cut', () => { + const { container } = render( + + ); + expect(container.firstChild).toBeNull(); + }); + + it('delegates brand-specific overlays to getBrandOverlays with brand.id', () => { + render(); + expect(brandRegistry.getBrandOverlays).toHaveBeenCalledWith('ragtech'); + }); + + it('passes isShortForm=true without crashing', () => { + expect(() => + render() + ).not.toThrow(); + }); +}); From 77c62324bbd11d5394556a3787685f55c0da3cba Mon Sep 17 00:00:00 2001 From: Natasha Ann Date: Wed, 13 May 2026 16:45:00 +0800 Subject: [PATCH 6/8] docs(spec): add id: string to Brand type definition in PRODUCTION_REFACTOR_PLAN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Brand.id was added to the implementation without updating the spec type block. The spec code snippets already referenced brand.id (lines 357–360) but the type definition at line 296 lacked the field, creating an internal inconsistency. Resolves review blocker B1. Co-Authored-By: Claude Sonnet 4.6 --- docs/PRODUCTION_REFACTOR_PLAN.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/PRODUCTION_REFACTOR_PLAN.md b/docs/PRODUCTION_REFACTOR_PLAN.md index 089b6b0..09e7351 100644 --- a/docs/PRODUCTION_REFACTOR_PLAN.md +++ b/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; From e12b73baa90f8b3110281f95a9e0d48b2ba59a18 Mon Sep 17 00:00:00 2001 From: Natasha Ann Date: Wed, 13 May 2026 16:57:17 +0800 Subject: [PATCH 7/8] perf(overlay-renderer): memoize componentMap and fix shortform/brand spread order W2: componentMap was reconstructed on every Remotion frame (60fps) via three object spreads, replacing the prior pattern of selecting between two pre-built module-level constants. Wrapped in useMemo([brand.id, isShortForm]) to prevent per-frame object churn. W3: Moved getBrandOverlays() before SHORTFORM_OVERRIDES in the spread so that shortform variants always take precedence over brand overlays. Previously brand overlays spread last, meaning a future brand supplying a ConceptExplainer key would silently override the short-form version. Co-Authored-By: Claude Sonnet 4.6 --- remotion/components/OverlayRenderer.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/remotion/components/OverlayRenderer.tsx b/remotion/components/OverlayRenderer.tsx index 394ebf4..af391d9 100644 --- a/remotion/components/OverlayRenderer.tsx +++ b/remotion/components/OverlayRenderer.tsx @@ -75,11 +75,12 @@ export const OverlayRenderer: React.FC = ({ mainStartFrame, isShortForm = false, }) => { - const componentMap = { + // SHORTFORM_OVERRIDES spreads last so shortform variants always beat brand overlays. + const componentMap = useMemo(() => ({ ...CORE_TEMPLATE_MAP, - ...(isShortForm ? SHORTFORM_OVERRIDES : {}), ...getBrandOverlays(brand.id), - }; + ...(isShortForm ? SHORTFORM_OVERRIDES : {}), + }), [brand.id, isShortForm]); const { fps } = useVideoConfig(); const currentFrame = useCurrentFrame(); From b703c89bc66a2c4a35e7737fb692141ef40bdbe7 Mon Sep 17 00:00:00 2001 From: Natasha Ann Date: Wed, 13 May 2026 16:58:30 +0800 Subject: [PATCH 8/8] docs: update review-pr skill and add review findings --- .claude/skills/review-pr/SKILL.md | 21 +++++ .../2026-05-13-refactor-s1-brand-registry.md | 81 +++++++++++++++++++ 2 files changed, 102 insertions(+) create mode 100644 docs/review-findings/2026-05-13-refactor-s1-brand-registry.md diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index 03ea633..59b1fea 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.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 diff --git a/docs/review-findings/2026-05-13-refactor-s1-brand-registry.md b/docs/review-findings/2026-05-13-refactor-s1-brand-registry.md new file mode 100644 index 0000000..cf01c85 --- /dev/null +++ b/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` 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\ 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.