fix: finalize android screenrecord mp4 on ctrl-c by signaling on-device process

[gmegidish]

No description provided.

[coderabbitai]

Walkthrough

This PR modifies screen recording behavior on Android devices to properly finalize video files. The ScreenRecord method now signals the remote screenrecord process by PID using adb shell and pgrep/kill instead of sending SIGINT directly to the local process. A new helper function signalRemoteScreenRecord performs the PID lookup and signaling with logging. Test coverage validates both time-limited and Ctrl+C-interrupted recordings produce playable MP4 files through ffprobe dimension verification.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	No pull request description was provided by the author, so this check cannot evaluate whether the description relates to the changeset.	Add a pull request description explaining the problem being solved, the solution implemented, and why signaling the remote process is necessary for proper MP4 finalization.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title clearly and specifically summarizes the main change: fixing Android screenrecord to finalize the MP4 file on Ctrl-C by signaling the on-device process.
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.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/android-screenrecord-ctrlc-finalize

---

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

[coderabbitai]

🧹 Nitpick comments (1)

test/emulator.spec.ts (1)

249-260: 💤 Low value

Consider clearing the timeout on process close.

If the spawned process exits early (e.g., due to an error), the setTimeout callback still fires and attempts to signal a dead process. While harmless, clearing it is cleaner.

♻️ Suggested improvement

 function recordThenInterruptWithCtrlC(deviceId: string, outputPath: string, recordSeconds: number): Promise<void> {
 	return new Promise((resolve, reject) => {
 		const child = spawn(mobilecliBinary, ['screenrecord', '--device', deviceId, '--output', outputPath], {
 			stdio: ['pipe', 'pipe', 'pipe'],
 		});

 		child.on('error', reject);
-		child.on('close', () => resolve());
+		child.on('close', () => {
+			clearTimeout(timeout);
+			resolve();
+		});

-		setTimeout(() => child.kill('SIGINT'), recordSeconds * 1000);
+		const timeout = setTimeout(() => child.kill('SIGINT'), recordSeconds * 1000);
 	});
 }

🤖 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 `@test/emulator.spec.ts` around lines 249 - 260, The timeout created in
recordThenInterruptWithCtrlC can fire after the child process already closed;
update the function to store the timer (e.g., const timer = setTimeout(...)) and
clear it when the child emits 'close' or 'error' (clearTimeout(timer)) so the
callback won't attempt to kill an already-exited child; keep the existing
child.on('error', reject) and child.on('close', () => { clearTimeout(timer);
resolve(); }) behavior and also clear the timer in the error handler.

🤖 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.

Nitpick comments:
In `@test/emulator.spec.ts`:
- Around line 249-260: The timeout created in recordThenInterruptWithCtrlC can
fire after the child process already closed; update the function to store the
timer (e.g., const timer = setTimeout(...)) and clear it when the child emits
'close' or 'error' (clearTimeout(timer)) so the callback won't attempt to kill
an already-exited child; keep the existing child.on('error', reject) and
child.on('close', () => { clearTimeout(timer); resolve(); }) behavior and also
clear the timer in the error handler.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8ca27d40-34a9-4fde-884a-a424dcb7d028

📥 Commits

Reviewing files that changed from the base of the PR and between 9fd9f9c and b576222.

📒 Files selected for processing (2)

• devices/android.go
• test/emulator.spec.ts

[gmegidish]

mobile-next/mobile-mcp#321