-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Fix clip cut deletion and preview playback #1029
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import { describe, expect, it, vi } from "vitest"; | ||
| import type { ClipRegion } from "../types"; | ||
| import { useClipRegionCommands } from "./useClipRegionCommands"; | ||
|
|
||
| vi.mock("react", async (importOriginal) => ({ | ||
| ...(await importOriginal<typeof import("react")>()), | ||
| useCallback: (callback: unknown) => callback, | ||
| })); | ||
|
|
||
| describe("clip split selection", () => { | ||
| it("selects the middle section after two cuts so Delete removes it", () => { | ||
| let clips: ClipRegion[] = [{ id: "original", startMs: 0, endMs: 3_000, speed: 1 }]; | ||
| let selectedClipId: string | null = null; | ||
| const nextClipIdRef = { current: 1 }; | ||
| const setClipRegions = (value: ClipRegion[] | ((current: ClipRegion[]) => ClipRegion[])) => { | ||
| clips = typeof value === "function" ? value(clips) : value; | ||
| }; | ||
| const setSelectedClipId = (value: string | null) => { | ||
| selectedClipId = value; | ||
| }; | ||
| const commands = () => | ||
| useClipRegionCommands({ | ||
| clipRegions: clips, | ||
| selectedClipId, | ||
| sourceDurationMs: 3_000, | ||
| nextClipIdRef, | ||
| setClipRegions, | ||
| setSelectedClipId, | ||
| setZoomRegions: () => {}, | ||
| setAnnotationRegions: () => {}, | ||
| setAudioRegions: () => {}, | ||
| setSelectedZoomId: () => {}, | ||
| setSelectedAnnotationId: () => {}, | ||
| setSelectedAudioId: () => {}, | ||
| setSelectedCaptionId: () => {}, | ||
| setActiveEffectSection: () => {}, | ||
| t: (key) => key, | ||
| }); | ||
|
|
||
| commands().handleClipSplit(1_000); | ||
| expect(selectedClipId).toBe(clips[0].id); | ||
| selectedClipId = null; | ||
| commands().handleClipSplit(2_000); | ||
| expect(selectedClipId).toBe(clips[1].id); | ||
| commands().handleClipDelete(selectedClipId!); | ||
| expect(clips).toMatchObject([ | ||
| { startMs: 0, endMs: 1_000 }, | ||
| { startMs: 1_000, endMs: 2_000, sourceStartMs: 2_000 }, | ||
| ]); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { expect, test } from "@playwright/test"; | ||
| import { installDesktopBridge } from "./bridge"; | ||
|
|
||
| test.use({ channel: process.env.RECORDLY_TEST_BROWSER_CHANNEL === "chrome" ? "chrome" : undefined }); | ||
|
|
||
| test("two cuts select the middle clip for deletion", async ({ page }) => { | ||
| await installDesktopBridge(page, "filmstrip.mp4"); | ||
| await page.goto("/?windowType=editor"); | ||
| const clips = page.locator('[data-variant="clip"]'); | ||
| await expect(clips).toHaveCount(1, { timeout: 20_000 }); | ||
| const firstClip = await clips.first().boundingBox(); | ||
| const timeline = page.locator(".select-none.bg-editor-bg.relative.cursor-pointer.group.flex.flex-col").last(); | ||
| const canvas = await timeline.boundingBox(); | ||
| if (!firstClip || !canvas) throw new Error("Timeline is not visible"); | ||
|
|
||
| await page.mouse.click(firstClip.x + firstClip.width * 0.25, canvas.y + 6); | ||
| await page.getByRole("button", { name: /Split Clip/ }).click(); | ||
| await expect(clips).toHaveCount(2); | ||
| await page.mouse.click(firstClip.x + firstClip.width * 0.5, canvas.y + 6); | ||
| await page.getByRole("button", { name: /Split Clip/ }).click(); | ||
| await expect(clips).toHaveCount(3); | ||
|
|
||
| await page.keyboard.press("Delete"); | ||
| await expect(clips).toHaveCount(2); | ||
| const firstEnd = Number(await clips.nth(0).getAttribute("data-end-ms")); | ||
| const lastStart = Number(await clips.nth(1).getAttribute("data-start-ms")); | ||
| expect(lastStart).toBe(firstEnd); | ||
|
Comment on lines
+25
to
+27
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Assert which clip Delete removes. The count and boundary checks can pass if Delete removes the first clip instead of the selected middle clip. Capture the clips’ identities or source ranges before Delete. Then assert that the original first and last clips remain and the middle clip is gone. 🤖 Prompt for AI Agents |
||
| const lastClip = await clips.nth(1).boundingBox(); | ||
| if (!lastClip) throw new Error("Retained clip is not visible"); | ||
| await page.mouse.click(lastClip.x + Math.min(8, lastClip.width / 4), canvas.y + 6); | ||
| await expect | ||
| .poll(() => | ||
| page.locator('video[src*="filmstrip.mp4"]').evaluate((video: HTMLVideoElement) => video.currentTime), | ||
| ) | ||
| .toBeGreaterThan((firstEnd + 500) / 1_000); | ||
|
Comment on lines
+31
to
+35
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '1,90p' tests/ui/clip-cut-delete.spec.ts
sed -n '935,980p' src/components/video-editor/timeline/components/viewport/TimelineCanvas.tsx
rg -n 'isPlaying|setIsPlaying|onPlay|onPause|handleTimelineSeek|onSeek' src/components/video-editor/VideoPlayback.tsx src/components/video-editor/layout/EditorTimelinePanel.tsx src/components/video-editor/timeline/components/viewport/TimelineCanvas.tsx | head -95Repository: webadderallorg/Recordly Length of output: 8247 🏁 Script executed: sed -n '40,95p' src/components/video-editor/layout/EditorTimelinePanel.tsx
sed -n '1140,1270p' src/components/video-editor/VideoPlayback.tsx
sed -n '1880,1975p' src/components/video-editor/VideoPlayback.tsx
sed -n '1310,1365p' src/components/video-editor/VideoPlayback.tsx
sed -n '1,150p' tests/ui/caption-speed.spec.ts
rg -n 'autoPlay|autoplay|handleTimelineSeek|onPlayStateChange|isPlaying' src/components/video-editor tests/ui --glob '*.ts' --glob '*.tsx' | head -160Repository: webadderallorg/Recordly Length of output: 27437 🏁 Script executed: sed -n '1,110p' src/components/video-editor/hooks/useEditorPlaybackControls.ts
sed -n '1,100p' src/components/video-editor/layout/EditorVideoPreview.tsx
sed -n '1,45p' src/components/video-editor/state/useEditorUiState.ts
sed -n '1,220p' src/components/video-editor/videoPlayback/clipPlayback.ts
sed -n '1,90p' src/components/video-editor/timeline/core/clipPresentation.ts
sed -n '980,1065p' src/components/video-editor/timeline/components/viewport/TimelineCanvas.tsx
rg -n 'pixelsToValue|timelineWidth|pixelsPer|range=' src/components/video-editor/timeline/components/viewport/TimelineCanvas.tsx src/components/video-editor/timeline -g '*.ts' -g '*.tsx' | head -100Repository: webadderallorg/Recordly Length of output: 22885 Start playback before asserting progress. The editor starts paused. Click inside the clip, capture the post-selection time, start playback, and assert that Suggested fix- await page.mouse.click(lastClip.x + Math.min(8, lastClip.width / 4), canvas.y + 6);
+ const video = page.locator('video[src*="filmstrip.mp4"]');
+ await page.mouse.click(
+ lastClip.x + Math.min(8, lastClip.width / 4),
+ lastClip.y + lastClip.height / 2,
+ );
+ const timeAfterSelection = await video.evaluate((element: HTMLVideoElement) => element.currentTime);
+ await page.getByRole("button", { name: "Play", exact: true }).click();
await expect
- .poll(() =>
- page.locator('video[src*="filmstrip.mp4"]').evaluate((video: HTMLVideoElement) => video.currentTime),
- )
- .toBeGreaterThan((firstEnd + 500) / 1_000);
+ .poll(() => video.evaluate((element: HTMLVideoElement) => element.currentTime))
+ .toBeGreaterThan(timeAfterSelection);🤖 Prompt for AI Agents |
||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
Repository: webadderallorg/Recordly
Length of output: 13188
🏁 Script executed:
Repository: webadderallorg/Recordly
Length of output: 42279
🏁 Script executed:
Repository: webadderallorg/Recordly
Length of output: 42593
🏁 Script executed:
Repository: webadderallorg/Recordly
Length of output: 42139
🏁 Script executed:
Repository: webadderallorg/Recordly
Length of output: 41668
🏁 Script executed:
Repository: webadderallorg/Recordly
Length of output: 41681
Resume source audio when its seek completes.
audio.currentTime = targetTimecan start an asynchronous seek after the effect runs. IfensureSourceAudioRunning()resolves first, theaudio.seekingguard skipsplay(). The effect has noseekedlistener.The playback transport can keep reporting the same time while the video is seeking. React then has no new dependency value to rerun this effect. Source audio can remain paused while
isPlayingstays true.Resume from a cleaned-up
seekedlistener when playback is still requested.🤖 Prompt for AI Agents