From b8d05b015026ff9dd8ee8db766664264e1285ce6 Mon Sep 17 00:00:00 2001 From: shaun0927 Date: Thu, 16 Apr 2026 10:23:30 +0900 Subject: [PATCH 1/3] fix: guard getNativeAspectRatioValue against zero denominator The helper performed an unguarded division, returning Infinity, NaN, or negative values when videoHeight, the crop region height, or the numerator was 0 / non-finite. The result is consumed by nine call sites in VideoEditor/VideoPlayback that feed canvas sizing and CSS aspect-ratio, producing 0x0 previews before video metadata loads or when the crop height collapses. Fall back to 16/9 (the same default getAspectRatioValue uses for 'native') whenever the computed ratio is not a finite positive number. Covered by a new vitest suite exercising zero, NaN, negative, and Infinity inputs. --- src/utils/aspectRatioUtils.test.ts | 54 ++++++++++++++++++++++++++++++ src/utils/aspectRatioUtils.ts | 10 +++++- 2 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 src/utils/aspectRatioUtils.test.ts diff --git a/src/utils/aspectRatioUtils.test.ts b/src/utils/aspectRatioUtils.test.ts new file mode 100644 index 000000000..453478490 --- /dev/null +++ b/src/utils/aspectRatioUtils.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it } from "vitest"; +import { getNativeAspectRatioValue } from "./aspectRatioUtils"; + +const FALLBACK = 16 / 9; + +describe("getNativeAspectRatioValue", () => { + it("returns the natural video ratio for a standard HD frame", () => { + expect(getNativeAspectRatioValue(1920, 1080)).toBeCloseTo(16 / 9); + }); + + it("applies the crop region when provided", () => { + const crop = { x: 0, y: 0, width: 0.5, height: 0.5 }; + expect(getNativeAspectRatioValue(1920, 1080, crop)).toBeCloseTo(16 / 9); + }); + + it("falls back to 16/9 when video metadata is not yet loaded (height = 0)", () => { + expect(getNativeAspectRatioValue(1920, 0)).toBe(FALLBACK); + }); + + it("falls back to 16/9 when both dimensions are zero", () => { + expect(getNativeAspectRatioValue(0, 0)).toBe(FALLBACK); + }); + + it("falls back to 16/9 when the crop height collapses to zero", () => { + const crop = { x: 0, y: 0, width: 0.5, height: 0 }; + expect(getNativeAspectRatioValue(1920, 1080, crop)).toBe(FALLBACK); + }); + + it("falls back to 16/9 when inputs are NaN", () => { + expect(getNativeAspectRatioValue(Number.NaN, 1080)).toBe(FALLBACK); + expect(getNativeAspectRatioValue(1920, Number.NaN)).toBe(FALLBACK); + }); + + it("falls back to 16/9 for non-positive dimensions", () => { + expect(getNativeAspectRatioValue(-1920, 1080)).toBe(FALLBACK); + expect(getNativeAspectRatioValue(1920, -1080)).toBe(FALLBACK); + }); + + it("never returns Infinity, NaN, or a non-positive ratio", () => { + const pathologicalInputs: Array<[number, number]> = [ + [0, 0], + [1920, 0], + [0, 1080], + [Number.POSITIVE_INFINITY, 1080], + [1920, Number.POSITIVE_INFINITY], + [Number.NaN, Number.NaN], + ]; + for (const [w, h] of pathologicalInputs) { + const ratio = getNativeAspectRatioValue(w, h); + expect(Number.isFinite(ratio)).toBe(true); + expect(ratio).toBeGreaterThan(0); + } + }); +}); diff --git a/src/utils/aspectRatioUtils.ts b/src/utils/aspectRatioUtils.ts index 887b543f7..c7344ed7b 100644 --- a/src/utils/aspectRatioUtils.ts +++ b/src/utils/aspectRatioUtils.ts @@ -41,6 +41,8 @@ export function getAspectRatioValue(aspectRatio: AspectRatio): number { } } +const NATIVE_ASPECT_FALLBACK = 16 / 9; + export function getNativeAspectRatioValue( videoWidth: number, videoHeight: number, @@ -48,7 +50,13 @@ export function getNativeAspectRatioValue( ): number { const cropW = cropRegion?.width ?? 1; const cropH = cropRegion?.height ?? 1; - return (videoWidth * cropW) / (videoHeight * cropH); + const numerator = videoWidth * cropW; + const denominator = videoHeight * cropH; + if (!Number.isFinite(numerator) || !Number.isFinite(denominator) || denominator <= 0) { + return NATIVE_ASPECT_FALLBACK; + } + const ratio = numerator / denominator; + return ratio > 0 && Number.isFinite(ratio) ? ratio : NATIVE_ASPECT_FALLBACK; } export function getAspectRatioDimensions( From 271268e540095928d7d976313fce679042be08de Mon Sep 17 00:00:00 2001 From: JunghwanNA <70629228+shaun0927@users.noreply.github.com> Date: Thu, 16 Apr 2026 11:54:49 +0900 Subject: [PATCH 2/3] refactor: unify 16/9 fallback literals with NATIVE_ASPECT_FALLBACK constant Move the NATIVE_ASPECT_FALLBACK constant above its first usage and replace all remaining 16/9 literals in getAspectRatioValue (native case) and formatAspectRatioForCSS with the shared constant so the fallback value has a single source of truth. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/utils/aspectRatioUtils.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/utils/aspectRatioUtils.ts b/src/utils/aspectRatioUtils.ts index c7344ed7b..dc0e08bbe 100644 --- a/src/utils/aspectRatioUtils.ts +++ b/src/utils/aspectRatioUtils.ts @@ -11,6 +11,8 @@ export const ASPECT_RATIOS = [ export type AspectRatio = (typeof ASPECT_RATIOS)[number]; +const NATIVE_ASPECT_FALLBACK = 16 / 9; + /** * Returns the numeric value of an aspect ratio. * For "native", returns a fallback ratio of 16/9. @@ -33,7 +35,7 @@ export function getAspectRatioValue(aspectRatio: AspectRatio): number { case "10:16": return 10 / 16; case "native": - return 16 / 9; + return NATIVE_ASPECT_FALLBACK; default: { const _exhaustiveCheck: never = aspectRatio; return _exhaustiveCheck; @@ -41,8 +43,6 @@ export function getAspectRatioValue(aspectRatio: AspectRatio): number { } } -const NATIVE_ASPECT_FALLBACK = 16 / 9; - export function getNativeAspectRatioValue( videoWidth: number, videoHeight: number, @@ -80,6 +80,6 @@ export function isPortraitAspectRatio(aspectRatio: AspectRatio): boolean { } export function formatAspectRatioForCSS(aspectRatio: AspectRatio, nativeRatio?: number): string { - if (aspectRatio === "native") return String(nativeRatio ?? 16 / 9); + if (aspectRatio === "native") return String(nativeRatio ?? NATIVE_ASPECT_FALLBACK); return aspectRatio.replace(":", "/"); } From 559765111a4b1383d1326e8e7b6c2075fcb9949a Mon Sep 17 00:00:00 2001 From: shaun0927 <70629228+shaun0927@users.noreply.github.com> Date: Tue, 28 Apr 2026 18:26:32 +0900 Subject: [PATCH 3/3] test(aspectRatioUtils): exercise the crop-region branch with non-canceling values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous "applies the crop region when provided" case used width/height = 0.5/0.5, which cancels out and returns the same ratio as no crop — the test would still pass even if cropRegion were ignored. Switch to non-proportional 0.75 × 0.5 (expected 8/3) so a future regression in the crop branch actually fails the assertion. Also broaden the pathological-input loop to drive the crop-region branch directly: zero / NaN / Infinity / negative crop dimensions, all of which must still resolve to the 16/9 fallback. --- src/utils/aspectRatioUtils.test.ts | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/utils/aspectRatioUtils.test.ts b/src/utils/aspectRatioUtils.test.ts index 453478490..ab7162961 100644 --- a/src/utils/aspectRatioUtils.test.ts +++ b/src/utils/aspectRatioUtils.test.ts @@ -9,8 +9,11 @@ describe("getNativeAspectRatioValue", () => { }); it("applies the crop region when provided", () => { - const crop = { x: 0, y: 0, width: 0.5, height: 0.5 }; - expect(getNativeAspectRatioValue(1920, 1080, crop)).toBeCloseTo(16 / 9); + // Use non-proportional crop dimensions so the ratio actually changes; + // equal width/height would cancel out and silently pass even if the + // crop were ignored. + const crop = { x: 0, y: 0, width: 0.75, height: 0.5 }; + expect(getNativeAspectRatioValue(1920, 1080, crop)).toBeCloseTo((1920 * 0.75) / (1080 * 0.5)); }); it("falls back to 16/9 when video metadata is not yet loaded (height = 0)", () => { @@ -37,16 +40,26 @@ describe("getNativeAspectRatioValue", () => { }); it("never returns Infinity, NaN, or a non-positive ratio", () => { - const pathologicalInputs: Array<[number, number]> = [ + const pathologicalInputs: Array< + [number, number, { x: number; y: number; width: number; height: number }?] + > = [ [0, 0], [1920, 0], [0, 1080], [Number.POSITIVE_INFINITY, 1080], [1920, Number.POSITIVE_INFINITY], [Number.NaN, Number.NaN], + // Same idea, but exercising the crop-region branch so a future + // regression there can't slip past the dimension-only cases above. + [1920, 1080, { x: 0, y: 0, width: 0.5, height: 0 }], + [1920, 1080, { x: 0, y: 0, width: 0, height: 0.5 }], + [1920, 1080, { x: 0, y: 0, width: Number.NaN, height: 0.5 }], + [1920, 1080, { x: 0, y: 0, width: 0.5, height: Number.NaN }], + [1920, 1080, { x: 0, y: 0, width: Number.POSITIVE_INFINITY, height: 0.5 }], + [1920, 1080, { x: 0, y: 0, width: 0.5, height: -1 }], ]; - for (const [w, h] of pathologicalInputs) { - const ratio = getNativeAspectRatioValue(w, h); + for (const [w, h, crop] of pathologicalInputs) { + const ratio = getNativeAspectRatioValue(w, h, crop); expect(Number.isFinite(ratio)).toBe(true); expect(ratio).toBeGreaterThan(0); }