feat: rectangle and ellipse shape annotations (#275)#499
Draft
Enriquefft wants to merge 13 commits intosiddharthvaddem:mainfrom
Draft
feat: rectangle and ellipse shape annotations (#275)#499Enriquefft wants to merge 13 commits intosiddharthvaddem:mainfrom
Enriquefft wants to merge 13 commits intosiddharthvaddem:mainfrom
Conversation
Introduce a discriminated-union field on FigureData to distinguish the upcoming rectangle and ellipse annotation kinds from the existing arrow. Adds an optional fill color used by closed shapes. Legacy projects load with kind defaulted to "arrow" at the single normalization site in projectPersistence.normalizeProjectEditor; no schema bump required. Pure data-model groundwork — no rendering, toolbar, or inspector changes in this commit.
Render the rectangle annotation kind in the in-editor SVG overlay and the export canvas pipeline. Both renderers now switch on FigureData.kind with an exhaustive default arm; the existing arrow path is preserved unchanged. The ellipse arm is a no-op stub here; it will be filled in by the next commit. Rectangles use FigureData.color for stroke and FigureData.fill (when set) for fill, matching the export pipeline pixel-for-pixel.
Fill in the ellipse arm of the renderFigure switch in both the SVG overlay and the export canvas pipeline. Mirrors the structure used for rectangle: an inscribed ellipse with non-scaling stroke, FigureData.color for stroke and FigureData.fill (when set) for fill. The default _exhaustiveCheck arm now passes for all three kinds; no placeholder branches remain.
Add two new buttons to the timeline annotation toolbar that create shape annotations: rectangle (Square icon) and ellipse (Circle icon). Each new annotation is born with kind set to its shape, the existing default color, and a translucent fill matching the border color (alpha 0x33). The annotation-added handler is widened to accept an optional FigureData. When provided, the region is constructed directly as type "figure" with that figureData; the legacy text-annotation path is preserved unchanged for the existing arrow button. Tooltip and aria-label use placeholder i18n keys that the next commit (i18n) will translate across all six locales.
Add a Fill section to the annotation inspector that lets users toggle and edit the fill color of rectangle and ellipse annotations. The section is hidden when the selected annotation is an arrow. Toggling fill on derives a translucent color from the current border color (alpha 0x33), matching the default produced by the toolbar buttons. Toggling off sets fill to undefined so the shape renders as outline-only. Purely additive below the existing color block; no refactor of the existing inspector controls. Labels use placeholder i18n keys that the next commit will translate across all six locales.
Adds the 4 new translation keys used by Commits 4 (toolbar) and 5 (inspector fill section) to every locale already shipped in the repo, keeping locale parity intact: - timeline.buttons.addRectangle - timeline.buttons.addEllipse - settings.annotation.fill.label - settings.annotation.fill.toggle Locales: en, zh-CN, es, fr, tr, ko-KR (the 6 listed in the spec) plus zh-TW (which exists in src/i18n/locales/ and is enforced by i18n:check). Spec §5.5 originally listed 9 keys; only 4 ended up referenced by the implementation, so only those are added — no dead strings.
Spec §3 declares kind as optional. The previous commits implemented it as required, which was a type-contract regression. All runtime consumers now read figureData.kind ?? "arrow" so a fresh FigureData without an explicit kind is treated as the legacy arrow shape. Load-path normalization in projectPersistence.ts continues to fill in the default for stored projects, so persisted state is unchanged. Exhaustive switches in AnnotationOverlay and annotationRenderer are preserved by switching on the narrowed local rather than figureData.kind directly; the never-typed default arm still proves exhaustiveness.
Introduce figureFill.ts as the single source of truth for hex-color handling on figure annotations: typed parser/formatter for #RRGGBB and #RRGGBBAA, alpha extraction, and percent<->alpha conversions. Malformed input throws; no silent fallbacks. The inspector's Fill section now exposes an integer opacity slider (0-100) per spec §5.3. The slider drives the alpha byte of figureData.fill while figureData.color remains the RGB source. When the user changes the border color, the fill adopts the new RGB while preserving its current alpha; when they pick a new fill color, the same invariant holds. The previously hardcoded 0x33 alpha string is gone from both the inspector and the timeline toolbar buttons; both call sites now compose fills via withAlpha(color, percentToAlpha(20)).
Adds annotation.fill.opacity to every settings namespace so the new inspector slider label localizes correctly. Translations chosen for visual parity with surrounding annotation.fill keys.
The slider's onChange wrote withAlpha(figureData.color, …) which silently overwrote the fill's RGB with the border color whenever a user had diverged the two via the fill color picker. Read RGB from the current fillValue instead so the slider only changes the alpha byte.
Contributor
📝 WalkthroughWalkthroughAdds rectangle and ellipse figure kinds with optional fill/alpha; updates rendering (overlay + exporter), settings UI, types, persistence, timeline tools, and i18n; preserves arrow behavior and validates/normalizes figure data. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/video-editor/projectPersistence.ts (1)
374-380:⚠️ Potential issue | 🟠 Majorunknown
kindvalues aren't normalized — escapes to renderers and trips the exhaustive check
region.figureData.kind ?? "arrow"only handles null/undefined. a persisted project withkind: "triangle"(corrupt file, future-version-downgrade, hand-edited json, etc.) flows straight through and hits_exhaustiveCheck: neverinAnnotationOverlay.renderFigureand the same dispatch in the exporter. lowkey risky for backward/forward compatibility.similar concern for
fill: it's not validated here, andfigureFillparsers throw on malformed input — a bad string in a loaded project would crash the inspector/renderer at first interaction rather than degrading gracefully.proposed fix
+const VALID_FIGURE_KINDS = new Set(["arrow", "rectangle", "ellipse"] as const); +const HEX_FILL_RE = /^#[0-9a-f]{6}([0-9a-f]{2})?$/i; + ... figureData: region.figureData ? { ...DEFAULT_FIGURE_DATA, ...region.figureData, - kind: region.figureData.kind ?? "arrow", + kind: + region.figureData.kind && + VALID_FIGURE_KINDS.has(region.figureData.kind) + ? region.figureData.kind + : "arrow", + fill: + typeof region.figureData.fill === "string" && + HEX_FILL_RE.test(region.figureData.fill) + ? region.figureData.fill + : undefined, } : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/projectPersistence.ts` around lines 374 - 380, Persisted figure kinds and fills aren't being validated: region.figureData.kind uses nullish coalescing so unknown strings (e.g., "triangle") pass through and later hit the exhaustive failure in AnnotationOverlay.renderFigure, and malformed fills propagate to figureFill parsers; update the normalization in the place building figureData (where region.figureData is merged with DEFAULT_FIGURE_DATA) to validate and map kind against the known enum/union (fall back to "arrow" for any unrecognized value) and sanitize/validate fill values before assigning (use the same validator/parsers used by figureFill or a safe mapper), ensuring DEFAULT_FIGURE_DATA, region.figureData, and the exported payload only ever contain normalized kinds/fills to prevent runtime _exhaustiveCheck errors and parser throws.
🧹 Nitpick comments (9)
src/i18n/locales/zh-TW/settings.json (1)
143-147: LGTM (with a tiny zh-TW nit)structure matches en. minor regional flavor: zh-TW often prefers "填滿" over "填充" (which is more zh-CN flavored), but "填充" is widely understood. totally optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/locales/zh-TW/settings.json` around lines 143 - 147, Change the zh-TW wording for the "fill" entries to the more regionally preferred term: update the JSON object with key "fill" so that "label": "填充" -> "填滿" and "toggle": "啟用填充" -> "啟用填滿" (keep "opacity": "不透明度" unchanged); target the "fill" object in src/i18n/locales/zh-TW/settings.json (keys: "fill", "label", "toggle") and apply the replacements.src/components/video-editor/figureFill.ts (2)
84-96: NaN slips through the clamp
Math.max(0, NaN)returnsNaN, andMath.round(NaN)isNaN. so a non-finite input here produces a NaN alpha that will later blow up informatHexColor'sNumber.isIntegercheck with a confusing error far from the source. probably never happens in practice (slider values are finite), but a defensiveNumber.isFiniteguard would make the failure mode obvious.proposed tweak
export function alphaToPercent(alpha: number): number { + if (!Number.isFinite(alpha)) { + throw new Error(`Invalid alpha (expected finite number): ${alpha}`); + } const clamped = Math.min(255, Math.max(0, alpha)); return Math.round((clamped / 255) * 100); } export function percentToAlpha(percent: number): number { + if (!Number.isFinite(percent)) { + throw new Error(`Invalid percent (expected finite number): ${percent}`); + } const clamped = Math.min(100, Math.max(0, percent)); return Math.round((clamped / 100) * 255); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/figureFill.ts` around lines 84 - 96, alphaToPercent and percentToAlpha currently let NaN/non-finite values pass through the clamp (Math.max/min) and produce NaN results; add an explicit Number.isFinite check at the start of each function (alphaToPercent and percentToAlpha) and if the input is not finite, throw a clear TypeError (or range error) that includes the function name and the invalid value so the failure is immediate and obvious rather than propagating NaN into formatHexColor.
9-11: doc says "throws on malformed input" but the conversion fns silently clamp
alphaToPercent/percentToAlphaclamp out-of-range values rather than throwing, which contradicts the module-level "All functions throw on malformed input" claim. either tighten the doc to scope the throw promise to the parse/format/withAlpha helpers, or make the conversions throw on out-of-range too. nit-tier, but future-you will thank you.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/figureFill.ts` around lines 9 - 11, The module-level doc promises that "All functions throw on malformed input" but alphaToPercent and percentToAlpha currently clamp out-of-range values; change them to validate inputs and throw on invalid ranges instead of clamping to satisfy the doc. Specifically, in alphaToPercent and percentToAlpha add explicit range checks (e.g., reject alpha <0 or >1 and percent <0 or >100) and throw a clear Error mentioning the function name and the invalid value; update any tests or callers that assumed clamping accordingly. If you prefer to keep clamping, instead update the module comment to limit the "throws" guarantee to parse/format/withAlpha helpers and document that the conversion helpers clamp — but pick one approach and apply it consistently across alphaToPercent, percentToAlpha, and any related utilities.src/i18n/locales/es/settings.json (1)
152-152: nit: toggle wording reads as a state, not a toggle label
"toggle": "Relleno activado"translates to "Fill activated/enabled" (a status), while other locales use imperative forms (zh-CN启用填充= "Enable fill", ko-KR채우기 사용= "Use fill"). assuming the en source is something like "Enable fill", consider:proposed wording
- "toggle": "Relleno activado", + "toggle": "Activar relleno",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/locales/es/settings.json` at line 152, The Spanish locale string for the settings key "toggle" is phrased as a state ("Relleno activado") but should use an imperative/label form to match other locales; update the "toggle" value in settings.json (the "toggle" key) to an imperative like "Activar relleno" or "Habilitar relleno" so it reads as a toggle label rather than a status.src/components/video-editor/types.ts (1)
64-70: recommended: lean into a proper discriminated unionshape works fine as-is and the
kind ?? "arrow"fallback keeps legacy data happy, butarrowDirectionbeing required onrectangle/ellipseis kinda cursed — those shapes don't have a direction. same forfillonly being meaningful on closed shapes. consumers currently switch onkindand just ignore the irrelevant fields.if you want better compile-time guarantees later, a discriminated union narrows the renderer/inspector branches naturally:
sketch (optional, not blocking)
interface ArrowFigureData { kind?: "arrow"; // optional for legacy load-path arrowDirection: ArrowDirection; color: string; strokeWidth: number; } interface ClosedShapeFigureData { kind: "rectangle" | "ellipse"; color: string; strokeWidth: number; fill?: string; } export type FigureData = ArrowFigureData | ClosedShapeFigureData;then
renderFigure's switch narrows fields per branch and you can drop the?? "none"/?? "arrow"defensive bits inside arrow branches.happy to defer this — sticking with the flat shape is fine for v1.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/types.ts` around lines 64 - 70, FigureData currently forces arrowDirection on all shapes and mixes fields; change it to a discriminated union so each kind only has relevant properties: create ArrowFigureData (kind?: "arrow" for legacy, arrowDirection: ArrowDirection, color, strokeWidth) and ClosedShapeFigureData (kind: "rectangle" | "ellipse", color, strokeWidth, fill?: string), then export type FigureData = ArrowFigureData | ClosedShapeFigureData and update consumers like renderFigure to rely on narrowing instead of optional fallbacks.src/i18n/locales/tr/timeline.json (1)
7-8: nit: capitalization is a bit off vs. siblingsother Turkish button labels in this file use Title Case ("Yakınlaştırma Ekle", "Kırpma Ekle", "Açıklama Ekle"), but these two go sentence case ("Dikdörtgen ekle", "Elips ekle"). kinda sticks out. align with the pattern for consistency:
proposed tweak
- "addRectangle": "Dikdörtgen ekle", - "addEllipse": "Elips ekle", + "addRectangle": "Dikdörtgen Ekle", + "addEllipse": "Elips Ekle",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/locales/tr/timeline.json` around lines 7 - 8, The translation entries for addRectangle and addEllipse use sentence case; update their values to Title Case to match sibling labels (change "Dikdörtgen ekle" → "Dikdörtgen Ekle" and "Elips ekle" → "Elips Ekle") so the keys "addRectangle" and "addEllipse" are consistent with other Turkish button labels in this locale file.src/components/video-editor/AnnotationOverlay.tsx (1)
188-244: rendering looks right; tiny nit on the exhaustive default.shapes look good —
vectorEffect="non-scaling-stroke"+overflow:visiblekeeps stroke width in screen pixels and consistent with the canvas exporter'sstrokeRect/strokeso preview ↔ export shouldn't drift.one nit: the
defaultbranch returns thenever-typed_exhaustiveCheck. compile-time it's an exhaustiveness assertion, but at runtime if some legacy/forward-compat data ever sneaks past normalization, you'd render the literal string ("circle", "polygon", whatever) into the dom as text. cheap to harden:♻️ safer default
default: { const _exhaustiveCheck: never = kind; - return _exhaustiveCheck; + void _exhaustiveCheck; + return null; }also worth a sanity check: arrow renders inside
p-2(so it shrinks within the box) while rect/ellipse fill the full container. probably intentional, but means same Rnd dimensions = visually different shape sizes. confirm that's the design.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/AnnotationOverlay.tsx` around lines 188 - 244, renderFigure's default currently returns the never-typed _exhaustiveCheck which is fine for compile-time exhaustiveness but will render the unexpected string at runtime; update the default branch in renderFigure (the _exhaustiveCheck handling for kind) to instead log or warn about the unexpected kind (include the value) and return a safe fallback such as null (or a visually-neutral placeholder) so no unexpected text is injected into the DOM at runtime.src/components/video-editor/timeline/TimelineEditor.tsx (1)
1224-1270: dedupe these two handlers — they're 95% the same function.both handlers do identical timing/clamping work and only differ by
kind. easy win to factor out:♻️ proposed factory
- const handleAddRectangleAnnotation = useCallback(() => { - if (!videoDuration || videoDuration === 0 || totalMs === 0 || !onAnnotationAdded) { - return; - } - - const defaultDuration = Math.min(defaultRegionDurationMs, totalMs); - if (defaultDuration <= 0) { - return; - } - - const startPos = Math.max(0, Math.min(currentTimeMs, totalMs)); - const endPos = Math.min(startPos + defaultDuration, totalMs); - - const figureData: FigureData = { - kind: "rectangle", - arrowDirection: "right", - color: "#34B27B", - strokeWidth: 4, - fill: withAlpha("#34B27B", percentToAlpha(20)), - }; - - onAnnotationAdded({ start: startPos, end: endPos }, figureData); - }, [videoDuration, totalMs, currentTimeMs, onAnnotationAdded, defaultRegionDurationMs]); - - const handleAddEllipseAnnotation = useCallback(() => { - // ...same body, kind: "ellipse" - }, [...]); + const addShapeAnnotation = useCallback( + (kind: "rectangle" | "ellipse") => { + if (!videoDuration || videoDuration === 0 || totalMs === 0 || !onAnnotationAdded) return; + const defaultDuration = Math.min(defaultRegionDurationMs, totalMs); + if (defaultDuration <= 0) return; + const startPos = Math.max(0, Math.min(currentTimeMs, totalMs)); + const endPos = Math.min(startPos + defaultDuration, totalMs); + const figureData: FigureData = { + kind, + arrowDirection: "right", + color: "#34B27B", + strokeWidth: 4, + fill: withAlpha("#34B27B", percentToAlpha(20)), + }; + onAnnotationAdded({ start: startPos, end: endPos }, figureData); + }, + [videoDuration, totalMs, currentTimeMs, onAnnotationAdded, defaultRegionDurationMs], + ); + const handleAddRectangleAnnotation = useCallback(() => addShapeAnnotation("rectangle"), [addShapeAnnotation]); + const handleAddEllipseAnnotation = useCallback(() => addShapeAnnotation("ellipse"), [addShapeAnnotation]);side note: the
arrowDirection: "right"placeholder is a low-key smell —FigureData.arrowDirectionis required even for closed shapes, so every rect/ellipse drags a meaningless field around. probably worth a follow-up to make it optional or move to a discriminated union, but out of scope here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 1224 - 1270, The two handlers handleAddRectangleAnnotation and handleAddEllipseAnnotation duplicate the same timing/clamping logic and only differ by FigureData.kind; refactor by extracting a shared factory/helper (e.g., createAddAnnotationHandler or addAnnotationWithKind) that computes defaultDuration, startPos and endPos, builds the common FigureData fields and accepts the differing kind value, then return/use two callbacks produced from that factory (or a single parameterized handler) wired into the UI; ensure you keep the same dependencies (videoDuration, totalMs, currentTimeMs, onAnnotationAdded, defaultRegionDurationMs) in the resulting useCallback(s) and remove the duplicate code blocks for rectangle/ellipse.src/components/video-editor/AnnotationSettingsPanel.tsx (1)
615-714: nit: extract the fill block into its own component.the inline IIFE returning ~100 lines of jsx is kinda cursed to read inside an already-deep
TabsContent. pulling this out into a<FigureFillControls figureData={...} onChange={onFigureDataChange} />would shrink this file, make the closed-shape gating explicit, and let you unit-test the percent↔alpha plumbing in isolation. purely optional, but your future self will thank you.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/AnnotationSettingsPanel.tsx` around lines 615 - 714, The large inline IIFE rendering the fill UI should be extracted into a new presentational component (e.g. FigureFillControls) to improve readability and testability: create a FigureFillControls React component that accepts props { figureData, onFigureDataChange, colorPalette, t } (or whatever context you already use), move the JSX that references figureData, fillEnabled logic, defaultFillFromColor, fillValue, getAlpha/alphaToPercent/percentToAlpha, withAlpha, Popover/PopoverTrigger/PopoverContent, Block, Slider and Switch into that component, keep the closed-shape gating (the isClosedShape check) in the parent or make the new component return null when figureData is missing/not a closed shape, and replace the IIFE with <FigureFillControls figureData={annotation.figureData} onFigureDataChange={onFigureDataChange} /> so the percent↔alpha plumbing (getAlpha, percentToAlpha, alphaToPercent, withAlpha) is isolated and easier to unit test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/AnnotationSettingsPanel.tsx`:
- Around line 626-647: The Switch (component instance using
checked={fillEnabled} and onCheckedChange) lacks an accessible name; add an
accessible label by giving the visible label or the span a unique id (e.g.,
"annotation-fill-label" or "annotation-fill-toggle") and add
aria-labelledby="<that-id>" to the Switch (or add aria-label with
t("annotation.fill.label") directly on the Switch) so screen readers announce
the purpose of the control; update the JSX around the label/span and the Switch
(referencing the Switch, fillEnabled, and onCheckedChange) to include this
association.
In `@src/i18n/locales/es/timeline.json`:
- Around line 7-8: The keys addRectangle and addEllipse use "Añadir" but other
toolbar keys (e.g., addZoom, addTrim, addAnnotation, addSpeed, addBlur) use
"Agregar"; update the translations for addRectangle and addEllipse to use
"Agregar" (i.e., change their values to "Agregar rectángulo" and "Agregar
elipse") to keep verb usage consistent across the timeline toolbar.
---
Outside diff comments:
In `@src/components/video-editor/projectPersistence.ts`:
- Around line 374-380: Persisted figure kinds and fills aren't being validated:
region.figureData.kind uses nullish coalescing so unknown strings (e.g.,
"triangle") pass through and later hit the exhaustive failure in
AnnotationOverlay.renderFigure, and malformed fills propagate to figureFill
parsers; update the normalization in the place building figureData (where
region.figureData is merged with DEFAULT_FIGURE_DATA) to validate and map kind
against the known enum/union (fall back to "arrow" for any unrecognized value)
and sanitize/validate fill values before assigning (use the same
validator/parsers used by figureFill or a safe mapper), ensuring
DEFAULT_FIGURE_DATA, region.figureData, and the exported payload only ever
contain normalized kinds/fills to prevent runtime _exhaustiveCheck errors and
parser throws.
---
Nitpick comments:
In `@src/components/video-editor/AnnotationOverlay.tsx`:
- Around line 188-244: renderFigure's default currently returns the never-typed
_exhaustiveCheck which is fine for compile-time exhaustiveness but will render
the unexpected string at runtime; update the default branch in renderFigure (the
_exhaustiveCheck handling for kind) to instead log or warn about the unexpected
kind (include the value) and return a safe fallback such as null (or a
visually-neutral placeholder) so no unexpected text is injected into the DOM at
runtime.
In `@src/components/video-editor/AnnotationSettingsPanel.tsx`:
- Around line 615-714: The large inline IIFE rendering the fill UI should be
extracted into a new presentational component (e.g. FigureFillControls) to
improve readability and testability: create a FigureFillControls React component
that accepts props { figureData, onFigureDataChange, colorPalette, t } (or
whatever context you already use), move the JSX that references figureData,
fillEnabled logic, defaultFillFromColor, fillValue,
getAlpha/alphaToPercent/percentToAlpha, withAlpha,
Popover/PopoverTrigger/PopoverContent, Block, Slider and Switch into that
component, keep the closed-shape gating (the isClosedShape check) in the parent
or make the new component return null when figureData is missing/not a closed
shape, and replace the IIFE with <FigureFillControls
figureData={annotation.figureData} onFigureDataChange={onFigureDataChange} /> so
the percent↔alpha plumbing (getAlpha, percentToAlpha, alphaToPercent, withAlpha)
is isolated and easier to unit test.
In `@src/components/video-editor/figureFill.ts`:
- Around line 84-96: alphaToPercent and percentToAlpha currently let
NaN/non-finite values pass through the clamp (Math.max/min) and produce NaN
results; add an explicit Number.isFinite check at the start of each function
(alphaToPercent and percentToAlpha) and if the input is not finite, throw a
clear TypeError (or range error) that includes the function name and the invalid
value so the failure is immediate and obvious rather than propagating NaN into
formatHexColor.
- Around line 9-11: The module-level doc promises that "All functions throw on
malformed input" but alphaToPercent and percentToAlpha currently clamp
out-of-range values; change them to validate inputs and throw on invalid ranges
instead of clamping to satisfy the doc. Specifically, in alphaToPercent and
percentToAlpha add explicit range checks (e.g., reject alpha <0 or >1 and
percent <0 or >100) and throw a clear Error mentioning the function name and the
invalid value; update any tests or callers that assumed clamping accordingly. If
you prefer to keep clamping, instead update the module comment to limit the
"throws" guarantee to parse/format/withAlpha helpers and document that the
conversion helpers clamp — but pick one approach and apply it consistently
across alphaToPercent, percentToAlpha, and any related utilities.
In `@src/components/video-editor/timeline/TimelineEditor.tsx`:
- Around line 1224-1270: The two handlers handleAddRectangleAnnotation and
handleAddEllipseAnnotation duplicate the same timing/clamping logic and only
differ by FigureData.kind; refactor by extracting a shared factory/helper (e.g.,
createAddAnnotationHandler or addAnnotationWithKind) that computes
defaultDuration, startPos and endPos, builds the common FigureData fields and
accepts the differing kind value, then return/use two callbacks produced from
that factory (or a single parameterized handler) wired into the UI; ensure you
keep the same dependencies (videoDuration, totalMs, currentTimeMs,
onAnnotationAdded, defaultRegionDurationMs) in the resulting useCallback(s) and
remove the duplicate code blocks for rectangle/ellipse.
In `@src/components/video-editor/types.ts`:
- Around line 64-70: FigureData currently forces arrowDirection on all shapes
and mixes fields; change it to a discriminated union so each kind only has
relevant properties: create ArrowFigureData (kind?: "arrow" for legacy,
arrowDirection: ArrowDirection, color, strokeWidth) and ClosedShapeFigureData
(kind: "rectangle" | "ellipse", color, strokeWidth, fill?: string), then export
type FigureData = ArrowFigureData | ClosedShapeFigureData and update consumers
like renderFigure to rely on narrowing instead of optional fallbacks.
In `@src/i18n/locales/es/settings.json`:
- Line 152: The Spanish locale string for the settings key "toggle" is phrased
as a state ("Relleno activado") but should use an imperative/label form to match
other locales; update the "toggle" value in settings.json (the "toggle" key) to
an imperative like "Activar relleno" or "Habilitar relleno" so it reads as a
toggle label rather than a status.
In `@src/i18n/locales/tr/timeline.json`:
- Around line 7-8: The translation entries for addRectangle and addEllipse use
sentence case; update their values to Title Case to match sibling labels (change
"Dikdörtgen ekle" → "Dikdörtgen Ekle" and "Elips ekle" → "Elips Ekle") so the
keys "addRectangle" and "addEllipse" are consistent with other Turkish button
labels in this locale file.
In `@src/i18n/locales/zh-TW/settings.json`:
- Around line 143-147: Change the zh-TW wording for the "fill" entries to the
more regionally preferred term: update the JSON object with key "fill" so that
"label": "填充" -> "填滿" and "toggle": "啟用填充" -> "啟用填滿" (keep "opacity": "不透明度"
unchanged); target the "fill" object in src/i18n/locales/zh-TW/settings.json
(keys: "fill", "label", "toggle") and apply the replacements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb46bf6f-1f25-4891-ab5f-9b4138281e00
📒 Files selected for processing (22)
src/components/video-editor/AnnotationOverlay.tsxsrc/components/video-editor/AnnotationSettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/figureFill.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/types.tssrc/i18n/locales/en/settings.jsonsrc/i18n/locales/en/timeline.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/es/timeline.jsonsrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/fr/timeline.jsonsrc/i18n/locales/ko-KR/settings.jsonsrc/i18n/locales/ko-KR/timeline.jsonsrc/i18n/locales/tr/settings.jsonsrc/i18n/locales/tr/timeline.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/i18n/locales/zh-CN/timeline.jsonsrc/i18n/locales/zh-TW/settings.jsonsrc/i18n/locales/zh-TW/timeline.jsonsrc/lib/exporter/annotationRenderer.ts
- es: addRectangle/addEllipse "Añadir" → "Agregar" (sibling consistency) - tr: addRectangle/addEllipse sentence → Title Case - zh-TW: fill.label/toggle 填充 → 填滿 (regional preference)
- figureFill: alphaToPercent/percentToAlpha throw on non-finite or out-of-range input, matching the module's documented contract; add isValidHexColor guard for untrusted input. - types: derive FigureKind from a single FIGURE_KINDS const so the union and runtime validator share one source of truth. - projectPersistence: normalize unknown kind values from loaded JSON back to "arrow" and drop malformed fill strings before they reach the renderer or hex parsers. - AnnotationOverlay/annotationRenderer: exhaustive-default branches warn and return null/void instead of returning the never-typed kind string (which would have been injected into the DOM if a bad payload ever slipped past validation).
The Switch in the figure tab had no accessible name; pair its label with an id and reference it via aria-labelledby so screen readers announce the control's purpose.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/video-editor/types.ts (1)
64-72: Makekindrequired in the runtime model.Now that legacy data is normalized on load and
DEFAULT_FIGURE_DATAalways sets a kind, keepingFigureData.kindoptional here just leaks persistence concerns into app state. every consumer still needs?? "arrow", and the new switches aren't really exhaustive. lowkey cleaner to keepkind?only in the raw/persisted shape and make runtimeFigureData.kindrequired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/types.ts` around lines 64 - 72, FigureData currently has kind optional which forces callers to guard with defaults; update the runtime model by making kind required (change FigureData.kind from optional to required using the existing FigureKind type), keep legacy/raw persisted shapes as the only place where kind remains optional, and ensure DEFAULT_FIGURE_DATA continues to provide a kind so switches over FigureData are exhaustive; look for symbols FIGURE_KINDS, FigureKind, FigureData, and DEFAULT_FIGURE_DATA to apply this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/AnnotationSettingsPanel.tsx`:
- Around line 615-619: The UI shows arrow-specific controls for closed shapes;
use the existing figureData.kind check (the isClosedShape boolean computed from
annotation.figureData in AnnotationSettingsPanel) to conditionally hide the
"Arrow Direction" grid and the "Arrow Color" label when kind !== "arrow", and
change the shared color control label to a generic term like "Stroke" or
"Border" so it is correct for both arrows and closed shapes; update the
conditional rendering around the Arrow Direction/Arrow Color UI blocks to rely
on isClosedShape (or its negation) and rename the color label text used by the
shared color control.
---
Nitpick comments:
In `@src/components/video-editor/types.ts`:
- Around line 64-72: FigureData currently has kind optional which forces callers
to guard with defaults; update the runtime model by making kind required (change
FigureData.kind from optional to required using the existing FigureKind type),
keep legacy/raw persisted shapes as the only place where kind remains optional,
and ensure DEFAULT_FIGURE_DATA continues to provide a kind so switches over
FigureData are exhaustive; look for symbols FIGURE_KINDS, FigureKind,
FigureData, and DEFAULT_FIGURE_DATA to apply this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94b3681d-155a-4a5b-8ef3-037935280bc9
📒 Files selected for processing (9)
src/components/video-editor/AnnotationOverlay.tsxsrc/components/video-editor/AnnotationSettingsPanel.tsxsrc/components/video-editor/figureFill.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/types.tssrc/i18n/locales/es/timeline.jsonsrc/i18n/locales/tr/timeline.jsonsrc/i18n/locales/zh-TW/settings.jsonsrc/lib/exporter/annotationRenderer.ts
✅ Files skipped from review due to trivial changes (3)
- src/i18n/locales/tr/timeline.json
- src/i18n/locales/zh-TW/settings.json
- src/i18n/locales/es/timeline.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/exporter/annotationRenderer.ts
- src/components/video-editor/projectPersistence.ts
- src/components/video-editor/figureFill.ts
Comment on lines
+615
to
+619
| {(() => { | ||
| const figureData = annotation.figureData; | ||
| if (!figureData) return null; | ||
| const isClosedShape = (figureData.kind ?? "arrow") !== "arrow"; | ||
| if (!isClosedShape) return null; |
Contributor
There was a problem hiding this comment.
Closed shapes still inherit arrow-only controls.
This branch already knows when kind !== "arrow", but rectangles/ellipses still show the Arrow Direction grid and Arrow Color label above it. that’s lowkey confusing: direction becomes a no-op, and the color label is wrong. Reuse the same kind check to hide arrow-only controls and rename the shared color control to something generic like Stroke/Border.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/AnnotationSettingsPanel.tsx` around lines 615 -
619, The UI shows arrow-specific controls for closed shapes; use the existing
figureData.kind check (the isClosedShape boolean computed from
annotation.figureData in AnnotationSettingsPanel) to conditionally hide the
"Arrow Direction" grid and the "Arrow Color" label when kind !== "arrow", and
change the shared color control label to a generic term like "Stroke" or
"Border" so it is correct for both arrows and closed shapes; update the
conditional rendering around the Arrow Direction/Arrow Color UI blocks to rely
on isClosedShape (or its negation) and rename the color label text used by the
shared color control.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #275.
Adds rectangle and ellipse shape annotations alongside the existing arrow. Discriminator field
kindonFigureData; render dispatch in overlay and export. Optional with default"arrow"on load — legacy projects unchanged.What changed
FigureData.kind?: "arrow" | "rectangle" | "ellipse"discriminator (optional; defaults to"arrow"via load-path normalization inprojectPersistence.ts).AnnotationOverlay.tsx) and exporter (annotationRenderer.ts) dispatch onkind ?? "arrow"; exhaustive: neverdefaults.figureData.fill.figureFill.tsutility — single source of truth for#RRGGBB[AA]parsing/formatting; throws on malformed input. All previously hardcoded${color}33/"#34B27B33"literals routed through it.Files
src/components/video-editor/types.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/AnnotationOverlay.tsxsrc/components/video-editor/AnnotationSettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/figureFill.ts(new)src/components/video-editor/timeline/TimelineEditor.tsxsrc/lib/exporter/annotationRenderer.tssrc/i18n/locales/{en,zh-CN,zh-TW,es,fr,tr,ko-KR}/{timeline,settings}.json(4 keys × 7 locales)~280 LOC source + 28 i18n entries.
Conflict-free against open PRs
#350, #489, #370, #240 — see commit 1 description for the conflict-avoidance map.
CI
npm run lint✓ (touched files clean; pre-existing warnings onuseScreenRecorder.tsand untracked.mcp.jsonpredate this PR)npx tsc --noEmit✓npx vite build✓npm run test:browser✓ (6/6 pass)npm run i18n:check— added keys present in all 7 locales; pre-existing parity violations onblur*,zoom.speed.*,audio.*are out of scope.Notes
timelineandsettingsnamespaces (buttons.addRectangle,buttons.addEllipse,annotation.fill.label,annotation.fill.toggle,annotation.fill.opacity) for consistency with surrounding code, rather than a neweditornamespace. Spec §5.5 sketched aneditornamespace placement; the chosen namespaces match where these keys actually fire from (TimelineEditor,AnnotationSettingsPanel).zh-TWcovered alongside the 6 locales the spec listed, matching the rest of the repo's locale roster.Test plan
kindpopulated as"arrow"by load-path normalization).Summary by CodeRabbit