fix: request screen permissions before countdown

[uriva]

Summary

• Moves the permission request (preparePermissions) to execute before starting the countdown in toggleRecording.
• This ensures the recording flow is not blocked by a permission dialog while the countdown is ticking, and avoids starting a countdown if permissions are denied.
• I confirm this logic does not affect Linux Wayland portal sentinel source persistence.

Summary by CodeRabbit

• Bug Fixes
  • Improved permission request timing for screen recording to prevent the countdown from initiating when permissions are denied or unavailable.

[coderabbitai]

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

💤 Files selected but had no reviewable changes (1)

• src/hooks/useScreenRecorder.ts

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 59299f3e-b700-46fa-9de6-789fd536d009

📥 Commits

Reviewing files that changed from the base of the PR and between a7595cf and 55cc5d0.

📒 Files selected for processing (1)

• src/hooks/useScreenRecorder.ts

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Refactored permission validation in the screen recorder hook by relocating the preparePermissions() call from startRecording to toggleRecording. This ensures permission checks occur before countdown activation, allowing startRecording to proceed without redundant permission validation.

Changes

Cohort / File(s)	Summary
Permission Check Relocation src/hooks/useScreenRecorder.ts	Moved preparePermissions() guard from startRecording to the beginning of toggleRecording. Permission denial now prevents countdown initiation, while the recording start flow no longer includes a separate permission check gate.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 In the hook where permissions dance,
We shifted checks along the path,
Before the countdown takes its chance,
Permission guards protect our craft!
**whisker twitch**

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description lacks several required sections from the template: no 'Type of Change' checkbox selection, no 'Related Issue(s)' link, no 'Screenshots/Video', and no 'Testing Guide'. Only a brief summary is provided.	Add missing required sections: select the 'Bug Fix' type, link related issues, provide testing instructions, and add any relevant screenshots or videos demonstrating the permission timing change.
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 (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix: request screen permissions before countdown' accurately and clearly describes the main change—moving permission requests before countdown activation to prevent blocking delays.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/hooks/useScreenRecorder.ts (1)

1415-1432: ⚠️ Potential issue | 🟡 Minor

Re-entrancy window widened by async permission check.

toggleRecording guards against concurrent invocation via starting || countdownActive, but neither flag is set before await preparePermissions(). On macOS that await can span several event-loop ticks (IPC + permission dialog), during which a second trigger (hotkey/tray/button) will pass the guard and race into the countdown/startRecording path. startInFlight protects the actual recorder, but the countdown itself can still be initiated twice (setCountdownActive(true) → startCountdown invoked twice).

Consider setting a guard before the await, e.g.:

🔒 Proposed fix

-		if (starting || countdownActive) {
+		if (starting || countdownActive) {
 			return;
 		}
 
 		if (recording) {
 			stopRecording.current();
 			return;
 		}
 
-		const permissionsReady = await preparePermissions();
-		if (!permissionsReady) {
-			return;
-		}
+		setStarting(true);
+		try {
+			const permissionsReady = await preparePermissions();
+			if (!permissionsReady) {
+				return;
+			}
+		} finally {
+			setStarting(false);
+		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useScreenRecorder.ts` around lines 1415 - 1432, toggleRecording
currently awaits preparePermissions without setting any guard, allowing a second
invocation to pass the checks and start a second countdown; to fix, set the
re-entrancy guard (reuse the existing starting flag or a dedicated
startInProgress flag) immediately before calling await preparePermissions()
inside toggleRecording, and ensure you clear that flag on all early exits
(permissions false, errors) and after countdown/start completes; reference
toggleRecording, preparePermissions, starting, countdownActive,
setCountdownActive, startCountdown and startInFlight to place the guard and
clear paths so the countdown cannot be initiated twice.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 1415-1432: toggleRecording currently awaits preparePermissions
without setting any guard, allowing a second invocation to pass the checks and
start a second countdown; to fix, set the re-entrancy guard (reuse the existing
starting flag or a dedicated startInProgress flag) immediately before calling
await preparePermissions() inside toggleRecording, and ensure you clear that
flag on all early exits (permissions false, errors) and after countdown/start
completes; reference toggleRecording, preparePermissions, starting,
countdownActive, setCountdownActive, startCountdown and startInFlight to place
the guard and clear paths so the countdown cannot be initiated twice.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8ed87c0e-80aa-4c06-81ca-a1799b56f617

📥 Commits

Reviewing files that changed from the base of the PR and between 7494c90 and a7595cf.

📒 Files selected for processing (1)

• src/hooks/useScreenRecorder.ts

[uriva]

Note: this is still a draft and not yet ready for review. Working on ironing out some timing edge cases with the Wayland portal dialog.

[uriva]

Update: The Wayland timing issue has been resolved. The PR is now ready for review!

[uriva]

Addressed @coderabbitai's feedback regarding re-entrancy in toggleRecording. Wrapped the async permissions check and stream acquisition in a try/finally block that properly guards using setStarting(true/false) to prevent duplicate clicks from double-triggering the countdown while the native permission dialogs are open.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me take a look at the current implementation to verify the changes.

---

🏁 Script executed:

#!/bin/bash
fd "useScreenRecorder.ts" --exec cat {}

Length of output: 56975

---

@uriva: The implementation looks solid. Here's a quick breakdown of why the fix holds up across all paths:

Guard at entry:

if (starting || countdownActive) return;

Prevents any duplicate invocation while permission dialogs, the Wayland portal dialog, or the countdown are in flight.

setStarting(true) before the first await:
This is the key — it's set synchronously before preparePermissions() suspends, so any subsequent click sees starting === true immediately.

try/finally handoff:
The finally block uses startInFlight.current as a coordination flag to decide who clears starting:

• If an early-return path is taken (permissions denied, Linux portal cancelled, countdown cancelled) → startInFlight.current is still false → toggleRecording's finally resets setStarting(false). ✅
• If startRecording() is reached → it sets startInFlight.current = true immediately and owns its own setStarting(false) in its finally. ✅

The redundant setStarting(true) inside startRecording (which was there before this PR) is now effectively a no-op since toggleRecording has already set it — harmless but could be cleaned up later if desired.

Overall the re-entrancy fix is correct and covers all abort paths cleanly.

[meiiie]

Quick triage pass: I don’t see any active review threads here right now. This looks mainly blocked by the merge conflict, so if the author still wants to land it, rebasing onto current main seems like the next step before another review pass.