fix: address timeline editor review feedback

This commit is contained in:
webadderall
2026-04-16 19:34:02 +10:00
parent 8cdf21e125
commit 1dc758e535
11 changed files with 141 additions and 71 deletions
+20 -6
View File
@@ -1,11 +1,11 @@
import {
ChatCircle as MessageSquare,
FilmSlate as Film,
Gauge,
MagnifyingGlassPlus as ZoomIn,
MouseLeftClickIcon as PhMouseLeftClick,
ChatCircle as MessageSquare,
MusicNotes as Music,
MouseLeftClickIcon as PhMouseLeftClick,
Scissors,
MagnifyingGlassPlus as ZoomIn,
} from "@phosphor-icons/react";
import type { Span } from "dnd-timeline";
import { useItem } from "dnd-timeline";
@@ -88,7 +88,12 @@ export default function Item({
);
const MIN_ITEM_PX = 6;
const safeItemStyle = { ...itemStyle, minWidth: MIN_ITEM_PX, height: "100%" };
const safeItemStyle = {
...itemStyle,
minWidth: MIN_ITEM_PX,
height: "100%",
overflow: "hidden",
};
return (
<div
@@ -99,7 +104,16 @@ export default function Item({
onPointerDownCapture={() => onSelect?.()}
className="group h-full"
>
<div className="h-full" style={{ ...itemContentStyle, minWidth: 24, height: "100%", display: "flex", alignItems: "center" }}>
<div
className="h-full"
style={{
...itemContentStyle,
minWidth: MIN_ITEM_PX,
height: "100%",
display: "flex",
alignItems: "center",
}}
>
<div
className={cn(
glassClass,
@@ -110,7 +124,7 @@ export default function Item({
height: "85%",
minHeight: 22,
color: "#fff",
minWidth: 24,
minWidth: MIN_ITEM_PX,
backgroundImage:
"linear-gradient(180deg, transparent 20%, rgba(255,255,255,0.08) 100%)",
}}
@@ -3,7 +3,6 @@
border-radius: 8px;
background: rgba(37, 99, 235, 0.38);
border: 1px solid rgba(37, 99, 235, 0.5);
transition:
background 0.15s ease,
border-color 0.15s ease;
@@ -26,7 +25,6 @@
border-radius: 8px;
background: rgba(239, 68, 68, 0.38);
border: 1px solid rgba(239, 68, 68, 0.5);
transition:
background 0.15s ease,
border-color 0.15s ease;
@@ -49,7 +47,6 @@
border-radius: 8px;
background: rgba(180, 160, 70, 0.38);
border: 1px solid rgba(180, 160, 70, 0.5);
transition:
background 0.15s ease,
border-color 0.15s ease;
@@ -72,7 +69,6 @@
border-radius: 8px;
background: rgba(245, 158, 11, 0.38);
border: 1px solid rgba(245, 158, 11, 0.5);
transition:
background 0.15s ease,
border-color 0.15s ease;
@@ -95,7 +91,6 @@
border-radius: 8px;
background: rgba(168, 85, 247, 0.38);
border: 1px solid rgba(168, 85, 247, 0.5);
transition:
background 0.15s ease,
border-color 0.15s ease;
@@ -118,7 +113,6 @@
border-radius: 8px;
background: rgba(37, 99, 235, 0.38);
border: 1px solid rgba(37, 99, 235, 0.5);
transition:
background 0.15s ease,
border-color 0.15s ease;
@@ -141,7 +135,6 @@
border-radius: 8px;
background: rgba(34, 197, 94, 0.35);
border: 1px solid rgba(34, 197, 94, 0.5);
transition:
background 0.15s ease,
border-color 0.15s ease;
@@ -84,6 +84,9 @@ const KeyframeMarkers: React.FC<KeyframeMarkersProps> = ({
onMouseDown={(e) => {
e.stopPropagation();
setSelectedKeyframeId(kf.id);
if (e.button !== 0) {
return;
}
setDraggingKeyframeId(kf.id);
}}
onContextMenu={(e) => {
@@ -258,12 +258,12 @@ export default function TimelineWrapper({
// Drag/resize tooltip (direct DOM updates, no re-renders)
const tooltipRef = useRef<HTMLDivElement>(null);
const formatTooltipMs = (ms: number) => {
const formatTooltipMs = useCallback((ms: number) => {
const s = ms / 1000;
const min = Math.floor(s / 60);
const sec = s % 60;
return min > 0 ? `${min}:${sec.toFixed(1).padStart(4, "0")}` : `${sec.toFixed(1)}s`;
};
}, []);
const showTooltip = useCallback(
(span: { start: number; end: number } | null, screenX?: number) => {
@@ -284,7 +284,7 @@ export default function TimelineWrapper({
}
}
},
[],
[formatTooltipMs],
);
const onDragStart = useCallback(
@@ -344,17 +344,7 @@ export default function TimelineWrapper({
const desired = updater(normalized);
if (totalMs > 0) {
const clamped = clampRange(desired);
if (clamped.end > totalMs) {
const span = Math.min(clamped.end - clamped.start, totalMs);
return {
start: Math.max(0, totalMs - span),
end: totalMs,
};
}
return clamped;
return clampRange(desired);
}
return desired;
@@ -54,6 +54,18 @@ export function createVideoEventHandlers(params: VideoEventHandlersParams) {
);
};
const skipPastTrimRegion = (trimRegion: TrimRegion) => {
const skipToTime = trimRegion.endMs / 1000;
const clampedSkipToTime = Math.min(skipToTime, video.duration);
video.currentTime = clampedSkipToTime;
emitTime(clampedSkipToTime);
if (clampedSkipToTime >= video.duration) {
video.pause();
}
};
function updateTime() {
if (!video) return;
@@ -62,15 +74,7 @@ export function createVideoEventHandlers(params: VideoEventHandlersParams) {
// If we're in a trim region during playback, skip to the end of it
if (activeTrimRegion && !video.paused && !video.ended) {
const skipToTime = activeTrimRegion.endMs / 1000;
// If the skip would take us past the video duration, pause instead
if (skipToTime >= video.duration) {
video.pause();
} else {
video.currentTime = skipToTime;
emitTime(skipToTime);
}
skipPastTrimRegion(activeTrimRegion);
} else {
// Apply playback speed from active speed region
const activeSpeedRegion = findActiveSpeedRegion(currentTimeMs);
@@ -115,14 +119,7 @@ export function createVideoEventHandlers(params: VideoEventHandlersParams) {
// If we seeked into a trim region while playing, skip to the end
if (activeTrimRegion && isPlayingRef.current && !video.paused) {
const skipToTime = activeTrimRegion.endMs / 1000;
if (skipToTime >= video.duration) {
video.pause();
} else {
video.currentTime = skipToTime;
emitTime(skipToTime);
}
skipPastTrimRegion(activeTrimRegion);
} else {
emitTime(video.currentTime);
}
@@ -366,6 +366,30 @@ describe("findDominantRegion", () => {
}
});
it("keeps the outgoing region active until the connected transition begins", () => {
const regions: ZoomRegion[] = [
{ id: "a", startMs: 1000, endMs: 3000, depth: 2, focus: { cx: 0.2, cy: 0.2 } },
{ id: "b", startMs: 3500, endMs: 6000, depth: 3, focus: { cx: 0.8, cy: 0.8 } },
];
const result = findDominantRegion(regions, 3100, { connectZooms: true });
expect(result.transition).toBeNull();
expect(result.region?.id).toBe("a");
expect(result.strength).toBeGreaterThan(0);
});
it("keeps the incoming region at full strength after a connected handoff", () => {
const regions: ZoomRegion[] = [
{ id: "a", startMs: 1000, endMs: 3000, depth: 2, focus: { cx: 0.2, cy: 0.2 } },
{ id: "b", startMs: 3500, endMs: 6000, depth: 3, focus: { cx: 0.8, cy: 0.8 } },
];
const result = findDominantRegion(regions, 4300, { connectZooms: true });
expect(result.transition).toBeNull();
expect(result.region?.id).toBe("b");
expect(result.strength).toBe(1);
});
it("does NOT connect zooms with a large gap", () => {
const regions: ZoomRegion[] = [
{ id: "a", startMs: 1000, endMs: 3000, depth: 2, focus: { cx: 0.2, cy: 0.2 } },
@@ -108,13 +108,23 @@ function getActiveRegion(
const activeRegions = regions
.map((region) => {
const outgoingPair = connectedPairs.find((pair) => pair.currentRegion.id === region.id);
if (outgoingPair && timeMs > outgoingPair.currentRegion.endMs) {
if (outgoingPair && timeMs >= outgoingPair.transitionStart) {
return { region, strength: 0 };
}
const incomingPair = connectedPairs.find((pair) => pair.nextRegion.id === region.id);
if (incomingPair && timeMs < incomingPair.transitionEnd) {
return { region, strength: 0 };
if (incomingPair) {
if (timeMs < incomingPair.transitionStart) {
return { region, strength: 0 };
}
const nextRegionZoomOutStart =
incomingPair.nextRegion.endMs -
ZOOM_OUT_EARLY_START_MS +
ZOOM_ANIMATION_LEAD_MS;
if (timeMs < nextRegionZoomOutStart) {
return { region, strength: 1 };
}
}
return { region, strength: computeRegionStrength(region, timeMs) };
@@ -65,6 +65,26 @@ interface ZoomTransformGeometry {
focusY: number;
}
function resetMotionEffects(
blurFilter: BlurFilter | null,
motionBlurFilter?: MotionBlurFilter | null,
motionBlurState?: MotionBlurState,
) {
if (motionBlurFilter) {
motionBlurFilter.velocity = { x: 0, y: 0 };
motionBlurFilter.kernelSize = 5;
motionBlurFilter.offset = 0;
}
if (blurFilter) {
blurFilter.blur = 0;
}
if (motionBlurState) {
motionBlurState.initialized = false;
}
}
export function computeZoomTransform({
stageSize,
baseMask,
@@ -150,6 +170,9 @@ export function applyZoomTransform({
baseMask.width <= 0 ||
baseMask.height <= 0
) {
cameraContainer.scale.set(1);
cameraContainer.position.set(0, 0);
resetMotionEffects(blurFilter, motionBlurFilter, motionBlurState);
return { scale: 1, x: 0, y: 0 };
}
@@ -225,17 +248,7 @@ export function applyZoomTransform({
}
}
} else {
if (motionBlurFilter) {
motionBlurFilter.velocity = { x: 0, y: 0 };
motionBlurFilter.kernelSize = 5;
motionBlurFilter.offset = 0;
}
if (blurFilter) {
blurFilter.blur = 0;
}
if (motionBlurState) {
motionBlurState.initialized = false;
}
resetMotionEffects(blurFilter, motionBlurFilter, motionBlurState);
}
return {
+18 -1
View File
@@ -38,7 +38,11 @@ function stopRecording(
const recorderState = recorder.state;
if (recorderState === "recording" || recorderState === "paused") {
if (recorderState === "paused") {
recorder.resume();
try {
recorder.resume();
} catch {
// Stopping a paused recorder is still valid; mirror the hook's fallback path.
}
}
if (webcamRecorder && webcamRecorder.state !== "inactive") {
webcamRecorder.stop();
@@ -194,6 +198,19 @@ describe("useScreenRecorder state machine", () => {
expect(callOrder).toEqual(["resume", "stop"]);
});
it("still stops when resume throws from paused state", () => {
recorder.pause();
recorder.resume.mockImplementation(() => {
throw new Error("resume failed");
});
const result = stopRecording(recorder, false);
expect(result.stopped).toBe(true);
expect(recorder.stop).toHaveBeenCalled();
expect(recorder.state).toBe("inactive");
});
it("does nothing when already inactive", () => {
const inactiveRecorder = createMockMediaRecorder("inactive");
+9 -5
View File
@@ -261,7 +261,7 @@ export function useScreenRecorder(): UseScreenRecorderReturn {
return false;
}, []);
const selectMimeType = () => {
const selectMimeType = useCallback(() => {
const preferred = [
"video/webm;codecs=av1",
"video/webm;codecs=h264",
@@ -271,7 +271,7 @@ export function useScreenRecorder(): UseScreenRecorderReturn {
];
return preferred.find((type) => MediaRecorder.isTypeSupported(type)) ?? "video/webm";
};
}, []);
const computeBitrate = (width: number, height: number) => {
const pixels = width * height;
@@ -537,7 +537,7 @@ export function useScreenRecorder(): UseScreenRecorderReturn {
webcamStream.current = null;
}
}
}, [getRecordingDurationMs, webcamDeviceId, webcamEnabled]);
}, [getRecordingDurationMs, selectMimeType, webcamDeviceId, webcamEnabled]);
/** Start the prepared webcam MediaRecorder. Call after main recording begins. */
const beginWebcamCapture = useCallback(() => {
@@ -622,8 +622,12 @@ export function useScreenRecorder(): UseScreenRecorderReturn {
const recorderState = recorder?.state;
if (recorder && (recorderState === "recording" || recorderState === "paused")) {
if (recorderState === "paused") {
markRecordingResumed(Date.now());
recorder.resume();
try {
recorder.resume();
markRecordingResumed(Date.now());
} catch (error) {
console.warn("Failed to resume recorder before stopping:", error);
}
}
pendingWebcamPathPromise.current = stopWebcamRecorder();
cleanupCapturedMedia();
+12 -7
View File
@@ -20,13 +20,17 @@ export function useVideoDevices(enabled: boolean = true) {
}
let mounted = true;
let activeLoadId = 0;
const loadDevices = async () => {
const loadId = ++activeLoadId;
let permissionStream: MediaStream | null = null;
try {
setIsLoading(true);
setError(null);
if (mounted && loadId === activeLoadId) {
setIsLoading(true);
setError(null);
}
let allDevices = await navigator.mediaDevices.enumerateDevices();
let videoInputs = allDevices
@@ -41,7 +45,6 @@ export function useVideoDevices(enabled: boolean = true) {
videoInputs.length > 0 && videoInputs.every((device) => !device.label.trim());
if (needsLabelPermission && !hasRequestedVideoLabels) {
hasRequestedVideoLabels = true;
permissionStream = await navigator.mediaDevices.getUserMedia({
video: true,
audio: false,
@@ -54,9 +57,10 @@ export function useVideoDevices(enabled: boolean = true) {
label: device.label || `Camera ${index + 1}`,
groupId: device.groupId,
}));
hasRequestedVideoLabels = true;
}
if (mounted) {
if (mounted && loadId === activeLoadId) {
setDevices(videoInputs);
setSelectedDeviceId((currentDeviceId) => {
if (currentDeviceId === "default" && videoInputs.length > 0) {
@@ -72,20 +76,21 @@ export function useVideoDevices(enabled: boolean = true) {
return videoInputs[0]?.deviceId ?? "default";
});
setIsLoading(false);
}
} catch (error) {
if (mounted) {
if (mounted && loadId === activeLoadId) {
const message =
error instanceof Error
? error.message
: "Failed to enumerate video devices";
setError(message);
setIsLoading(false);
console.error("Error loading video devices:", error);
}
} finally {
permissionStream?.getTracks().forEach((track) => track.stop());
if (mounted && loadId === activeLoadId) {
setIsLoading(false);
}
}
};