Add subtitle export IPC and hidden-cursor click effects#631
Add subtitle export IPC and hidden-cursor click effects#631webadderall wants to merge 3 commits into
Conversation
Addresses CodeRabbit pre-merge docstring coverage warning.
|
@coderabbitai review @copilot review |
📝 WalkthroughWalkthroughThis PR introduces end-to-end subtitle export functionality (SRT/VTT formats) via Electron file dialogs and VideoEditor UI controls, alongside a cursor visibility tracking refinement that gates sprite rendering independently and updates export validation logic for cursor click effects. ChangesSubtitle Export Feature
Cursor Visibility Refinement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch PR diff and files changed
gh pr view 631 --repo webadderallorg/Recordly --json title,body,files,additions,deletions
echo "---FILES CHANGED---"
gh pr diff 631 --repo webadderallorg/RecordlyLength of output: 28908 🏁 Script executed: #!/bin/bash
# Check what approveUserPath does and how it's used in other handlers
rg -n "approveUserPath" --type ts -A 5 -B 2Length of output: 9235
Review of PR
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Line 365: The regex used to build safeBaseName unnecessarily escapes the
forward slash inside the character class; update the assignment that constructs
safeBaseName (the line using sourceName.replace(...).trim()) to remove the
backslash before the "/" in the character class so the pattern reads the same
but without the unnecessary escape, leaving the control-range \x00-\x1f and
other forbidden chars intact and preserving .trim().
In `@src/components/video-editor/videoPlayback/cursorRenderer.ts`:
- Around line 1314-1339: When the cursor is hidden the persistent
SmoothedCursorState (this.state) is left unchanged so when the cursor reappears
within CURSOR_TIME_DISCONTINUITY_MS it updates from stale coordinates; fix by
resetting or reinitializing this.state to a fresh SmoothedCursorState (or call
its reset/initialize method) whenever you take the hide paths (the branch that
returns when samples empty/viewport invalid and the branch that returns when
interpolateCursorPosition returns falsy). Ensure the reset happens before the
early return so subsequent frames start from the current target rather than old
coordinates.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e72e2593-072d-4ca7-ac5e-96f52d32b819
📒 Files selected for processing (10)
electron/electron-env.d.tselectron/ipc/captions/exportSubtitleFile.test.tselectron/ipc/captions/exportSubtitleFile.tselectron/ipc/register/captions.tselectron/preload.tssrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/videoPlayback/cursorRenderer.tssrc/lib/exporter/modernVideoExporter.nativeStaticLayout.test.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/types.ts
💤 Files with no reviewable changes (1)
- src/lib/exporter/modernVideoExporter.ts
| ?.split(/[\\/]/) | ||
| .pop() | ||
| ?.replace(/\.[^.]+$/, "") ?? "captions"; | ||
| const safeBaseName = sourceName.replace(/[\x00-\x1f<>:"\/\\|?*]+/g, "-").trim(); |
There was a problem hiding this comment.
Remove unnecessary escape in regex character class.
The forward slash doesn't need escaping inside a character class. The control character range (\x00-\x1f) is intentional for filename sanitization and can be ignored, but the / escape should be removed.
🔧 Proposed fix
- const safeBaseName = sourceName.replace(/[\x00-\x1f<>:"\/\\|?*]+/g, "-").trim();
+ const safeBaseName = sourceName.replace(/[\x00-\x1f<>:"/\\|?*]+/g, "-").trim();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const safeBaseName = sourceName.replace(/[\x00-\x1f<>:"\/\\|?*]+/g, "-").trim(); | |
| const safeBaseName = sourceName.replace(/[\x00-\x1f<>:"/\\|?*]+/g, "-").trim(); |
🧰 Tools
🪛 Biome (2.4.15)
[error] 365-365: Unexpected control character in a regular expression.
(lint/suspicious/noControlCharactersInRegex)
[error] 365-365: Unexpected control character in a regular expression.
(lint/suspicious/noControlCharactersInRegex)
[error] 365-365: The character doesn't need to be escaped.
(lint/complexity/noUselessEscapeInRegex)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/video-editor/VideoEditor.tsx` at line 365, The regex used to
build safeBaseName unnecessarily escapes the forward slash inside the character
class; update the assignment that constructs safeBaseName (the line using
sourceName.replace(...).trim()) to remove the backslash before the "/" in the
character class so the pattern reads the same but without the unnecessary
escape, leaving the control-range \x00-\x1f and other forbidden chars intact and
preserving .trim().
| if (samples.length === 0 || viewport.width <= 0 || viewport.height <= 0) { | ||
| this.container.visible = false; | ||
| this.cursorVisible = false; | ||
| this.clickRingGraphics.clear(); | ||
| this.lastRenderedPoint = null; | ||
| this.lastRenderedTimeMs = null; | ||
| this.swayRotation = 0; | ||
| resetSpringState(this.swaySpring, 0); | ||
| this.cursorMotionBlurFilter.velocity = { x: 0, y: 0 }; | ||
| this.customCursorShadowSprite.visible = false; | ||
| this.customCursorSprite.visible = false; | ||
| for (const shadowSprite of Object.values(this.cursorShadowSprites)) { | ||
| shadowSprite.visible = false; | ||
| } | ||
| for (const sprite of Object.values(this.cursorSprites)) { | ||
| sprite.visible = false; | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| const target = interpolateCursorPosition(samples, timeMs); | ||
| if (!target) { | ||
| this.container.visible = false; | ||
| this.cursorVisible = false; | ||
| this.clickRingGraphics.clear(); | ||
| return; |
There was a problem hiding this comment.
Reset SmoothedCursorState when the sprite is hidden.
After the hide paths on Lines 1314-1339 and Lines 1369-1423, this.state stays initialized. If the cursor becomes visible again within CURSOR_TIME_DISCONTINUITY_MS, the next frame runs this.state.update(...) from stale coordinates, so the cursor can slide in from the old position instead of snapping to the current target.
Suggested fix
if (samples.length === 0 || viewport.width <= 0 || viewport.height <= 0) {
+ this.state.reset();
this.container.visible = false;
this.cursorVisible = false;
this.clickRingGraphics.clear();
@@
const target = interpolateCursorPosition(samples, timeMs);
if (!target) {
+ this.state.reset();
this.container.visible = false;
this.cursorVisible = false;
this.clickRingGraphics.clear();
return;
}
@@
if (!shouldShowCursorSprite && !shouldDrawClickEffect) {
+ this.state.reset();
this.container.visible = false;
this.cursorVisible = false;
this.clickRingGraphics.clear();
@@
} else {
+ this.state.reset();
this.customCursorShadowSprite.visible = false;
this.customCursorSprite.visible = false;
for (const shadowSprite of Object.values(this.cursorShadowSprites)) {Also applies to: 1369-1423
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/video-editor/videoPlayback/cursorRenderer.ts` around lines
1314 - 1339, When the cursor is hidden the persistent SmoothedCursorState
(this.state) is left unchanged so when the cursor reappears within
CURSOR_TIME_DISCONTINUITY_MS it updates from stale coordinates; fix by resetting
or reinitializing this.state to a fresh SmoothedCursorState (or call its
reset/initialize method) whenever you take the hide paths (the branch that
returns when samples empty/viewport invalid and the branch that returns when
interpolateCursorPosition returns falsy). Ensure the reset happens before the
early return so subsequent frames start from the current target rather than old
coordinates.
2 participants
Summary
Validation
Summary by CodeRabbit
New Features
Improvements
Tests