Skip to content

Add MP4 Electron export E2E#537

Open
yusufm wants to merge 2 commits intosiddharthvaddem:mainfrom
yusufm:codex/mp4-electron-e2e
Open

Add MP4 Electron export E2E#537
yusufm wants to merge 2 commits intosiddharthvaddem:mainfrom
yusufm:codex/mp4-electron-e2e

Conversation

@yusufm
Copy link
Copy Markdown
Contributor

@yusufm yusufm commented May 3, 2026

Summary

  • add a Playwright Electron MP4 export test that validates the saved MP4 bytes
  • share Electron E2E helpers for save interception, fixture setup, and deterministic shutdown
  • run Electron E2E serially to avoid shared Electron userData collisions
  • add an MP4 format test id and fix the linted hook dependency used by CI

Testing

  • npm run lint
  • npm run build-vite
  • npm run test:e2e
  • npm run test:browser

Summary by CodeRabbit

  • Tests

    • Added end-to-end MP4 export test with file validation.
    • Refactored GIF export tests to use shared test utilities.
    • Added e2e helpers for app lifecycle and export interception.
  • New Features

    • MP4 export format control now includes a test identifier for UI testing.
  • Chores

    • Ensured test runner uses a single worker.
    • Improved screen-recording cleanup behavior.

@yusufm yusufm requested a review from siddharthvaddem as a code owner May 3, 2026 21:30
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 3, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 135457a1-bf15-426a-92a0-b793e9486c0d

📥 Commits

Reviewing files that changed from the base of the PR and between 15316d9 and 251b29b.

📒 Files selected for processing (2)
  • tests/e2e/helpers.ts
  • tests/e2e/mp4-export.spec.ts
✅ Files skipped from review due to trivial changes (1)
  • tests/e2e/mp4-export.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/e2e/helpers.ts

📝 Walkthrough

Walkthrough

This PR adds shared Playwright/Electron e2e helpers, a new MP4 export test, refactors the GIF export test to use those helpers, memoizes a recorder helper in useScreenRecorder, adds a test-id to the MP4 format button, and forces Playwright to run with a single worker.

Changes

MP4 Export E2E Testing Infrastructure

Layer / File(s) Summary
Test IDs / type
src/utils/getTestId.ts, src/components/video-editor/SettingsPanel.tsx
TestId union reformatted; MP4 export button gains data-testid={getTestId("mp4-format-button")}.
Hook / Cleanup
src/hooks/useScreenRecorder.ts
safeHideCountdownOverlay memoized with useCallback; recording cleanup effect now depends on safeHideCountdownOverlay and teardownMedia.
Test Helpers
tests/e2e/helpers.ts
Adds waitForProcessExit, closeElectronApp, interceptExportSave, copyFixtureToRecordings, and readCapturedExportBuffer to centralize IPC, fixture copying, and process teardown.
Tests (implementation/refactor)
tests/e2e/mp4-export.spec.ts, tests/e2e/gif-export.spec.ts
New MP4 export e2e test validates intercepted export buffer (ftyp signature and size). GIF export test refactored to use shared helpers and readCapturedExportBuffer.

Test Configuration

Layer / File(s) Summary
Playwright config
playwright.config.ts
Sets workers: 1 in Playwright config to force single-worker test execution.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • siddharthvaddem

Poem

night shift tests hum, a tiny electron glow,
helpers stitched up tidy, exports ebb and flow,
mp4 bytes whisper ftyp in the dark,
one worker trudges onward—steady, not stark,
low-key triumphant, we ship another spark.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly summarizes the main change: adding an MP4 Electron export end-to-end test, which is the primary focus of this PR.
Description check ✅ Passed Description covers all key changes with testing instructions, though it omits several template sections like Motivation and Type of Change checkboxes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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.

❤️ Share
Review rate limit: 3/8 reviews remaining, refill in 37 minutes and 29 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/e2e/helpers.ts`:
- Around line 62-66: Add a defensive guard in readCapturedExportBuffer: before
calling Buffer.from, retrieve the captured value from
globalThis["__testExportData"] (as currently done) and verify it is a non-empty
string (or Buffer-encodable value); if missing or invalid, throw a clear Error
mentioning that __testExportData was not set or is invalid so tests fail with a
helpful message. Update the function readCapturedExportBuffer to validate the
retrieved base64 value and only call Buffer.from(base64, "base64") when the
check passes.

In `@tests/e2e/mp4-export.spec.ts`:
- Around line 41-44: The test fires two IPC calls inside hudWindow.evaluate that
can race; change the evaluate callback so it awaits
window.electronAPI.setCurrentVideoPath(videoPath) before calling/awaiting
window.electronAPI.switchToEditor(), ensuring setCurrentVideoPath completes
first (i.e., use await on setCurrentVideoPath and then await switchToEditor
inside the hudWindow.evaluate callback) to eliminate the export-flow race.
🪄 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: c4a3bcbc-624d-4165-beb1-141318472a6c

📥 Commits

Reviewing files that changed from the base of the PR and between 7e00cdb and 15316d9.

📒 Files selected for processing (7)
  • playwright.config.ts
  • src/components/video-editor/SettingsPanel.tsx
  • src/hooks/useScreenRecorder.ts
  • src/utils/getTestId.ts
  • tests/e2e/gif-export.spec.ts
  • tests/e2e/helpers.ts
  • tests/e2e/mp4-export.spec.ts

Comment thread tests/e2e/helpers.ts
Comment thread tests/e2e/mp4-export.spec.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant