fix: camera light flashes and turns off when clicking webcam button (…#325
Open
dheerajmr01 wants to merge 5 commits intosiddharthvaddem:mainfrom
Open
fix: camera light flashes and turns off when clicking webcam button (…#325dheerajmr01 wants to merge 5 commits intosiddharthvaddem:mainfrom
dheerajmr01 wants to merge 5 commits intosiddharthvaddem:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDecouples webcam acquisition from recording start by moving getUserMedia into a useEffect that manages readiness, onended and cleanup; startRecording now waits for webcam readiness. Disables the webcam toggle while recording, adds camera-related i18n keys, relaxes Electron camera-access return filtering, and ignores Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant LaunchWindow as LaunchWindow
participant RecorderHook as useScreenRecorder
participant MediaDevices as navigator.mediaDevices
participant Toasts as Toasts
User->>LaunchWindow: click "enable webcam"
LaunchWindow->>RecorderHook: setWebcamEnabled(true)
RecorderHook->>MediaDevices: getUserMedia(constraints)
alt success
MediaDevices-->>RecorderHook: MediaStream
RecorderHook-->>LaunchWindow: store stream, webcamReady=true
else failure
MediaDevices-->>RecorderHook: DOMException
RecorderHook->>Toasts: show cameraBlocked / cameraNotFound
RecorderHook-->>LaunchWindow: setWebcamEnabled(false)
end
User->>LaunchWindow: click "start recording"
LaunchWindow->>RecorderHook: startRecording()
RecorderHook->>RecorderHook: poll webcamReady (~5s)
alt webcamStream available
RecorderHook->>RecorderHook: proceed with recording using webcam stream
else no webcamStream
RecorderHook-->>LaunchWindow: setWebcamEnabled(false)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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 unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20b0899c05
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![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 the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 467-470: The code incorrectly treats webcamStream.current === null
as a denial; change the check in the Record-click handler to wait for the
acquire flow to settle by adding or using an initializing/ready flag (e.g.,
webcamInitializing or webcamReady) instead of assuming null means denied: only
call setWebcamEnabledState(false) and toast.error(t("recording.cameraDenied"))
when acquisition has completed and been rejected (initializing is false and
webcamStream.current is null or an explicit denied status), or alternatively
gate the early return when webcamInitializing is true so the handler waits for
the effect that populates webcamStream.current to finish.
- Around line 220-225: The catch block in useScreenRecorder.ts currently treats
all webcam acquisition failures the same; update the cameraError handling inside
the catch (cameraError) branch to inspect the error type/name (e.g.,
cameraError.name or cameraError.message) and map permission errors
('NotAllowedError'/'PermissionDenied') to
toast.error(t("recording.cameraBlocked")) while mapping hardware/device-loss
errors
('NotFoundError'/'OverconstrainedError'/'NotReadableError'/'DevicesNotFoundError'
or messages indicating missing device) to
toast.error(t("recording.cameraDisconnected")); keep the existing
setWebcamEnabledState(false) behavior and ensure the logic references the
existing symbols cameraError, setWebcamEnabledState, toast.error, and t() so the
correct localized key is shown for each failure reason.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cfe40ec9-97e6-4fd1-a3ce-e6af46933119
📒 Files selected for processing (6)
src/components/launch/LaunchWindow.tsxsrc/hooks/useScreenRecorder.tssrc/i18n/locales/en/editor.jsonsrc/i18n/locales/es/editor.jsonsrc/i18n/locales/zh-CN/editor.jsonsrc/lib/requestCameraAccess.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/hooks/useScreenRecorder.ts (2)
227-232: Error differentiation looks good, but consider addingAbortError.The current error differentiation addresses the past review concern well. However,
AbortErrorcan also indicate hardware issues (e.g., camera in use by another application) and might warrant the same "not found" style message rather than a permission-blocked message.💡 Optional enhancement
const isDeviceError = cameraError instanceof DOMException && - ["NotFoundError", "OverconstrainedError", "NotReadableError"].includes( + ["NotFoundError", "OverconstrainedError", "NotReadableError", "AbortError"].includes( cameraError.name, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 227 - 232, The camera error handling in useScreenRecorder.ts should also treat AbortError as a device/hardware issue; update the isDeviceError check where cameraError is evaluated (the instanceof DOMException && ["NotFoundError", "OverconstrainedError", "NotReadableError"].includes(cameraError.name) logic) to include "AbortError" so toast.error(t(...)) shows "recording.cameraNotFound" for AbortError as well.
478-495: Polling pattern works but has an edge case when webcam is toggled off mid-wait.If the user disables webcam while
startRecordingis waiting in the polling loop, the cleanup setswebcamReady.current = false, causing the loop to wait the full 5 seconds before timing out. This is a minor UX delay, not a bug.Consider tracking the enabled state in the polling condition to exit early:
💡 Optional improvement for faster exit on disable
if (webcamEnabled) { if (!webcamReady.current) { await new Promise<void>((resolve) => { + let resolved = false; const interval = setInterval(() => { - if (webcamReady.current) { + if (webcamReady.current || !webcamEnabled) { clearInterval(interval); - resolve(); + if (!resolved) { + resolved = true; + resolve(); + } } }, 50); setTimeout(() => { clearInterval(interval); - resolve(); + if (!resolved) { + resolved = true; + resolve(); + } }, 5000); }); }Note: This requires passing
webcamEnabledstate into the closure properly or using a ref. The current implementation is acceptable as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 478 - 495, Polling in startRecording can hang up to the 5s timeout if the user disables the webcam mid-wait because the loop only checks webcamReady.current; update the polling to also observe the webcam enabled state so it can exit immediately when disabled. Concretely, add or use an existing ref like webcamEnabledRef (or sync webcamEnabled into a ref) and include a check inside the interval callback (e.g., if (!webcamReady.current || !webcamEnabledRef.current) { clearInterval(interval); resolve(); }) and ensure cleanup sets webcamEnabledRef appropriately; keep the existing fallback timeout logic and the later webcamStream.current check/notification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 492-495: The camera acquisition failure currently may trigger a
duplicate toast: when webcamStream.current is null inside the startRecording
flow we call setWebcamEnabledState(false) and
toast.error(t("recording.cameraDenied")), but an earlier useEffect may already
have shown cameraNotFound or cameraBlocked and set state asynchronously. Update
the logic in useScreenRecorder (the block that checks webcamStream.current and
calls setWebcamEnabledState + toast.error) to avoid duplicating toasts — either
remove the toast.error here and rely on the useEffect messages, or add a guard
(e.g., a boolean flag or ref like hasShownCameraError) that the useEffect sets
when it displays cameraNotFound/cameraBlocked and that startRecording checks
before calling toast.error; ensure the flag is set/cleared appropriately so
setWebcamEnabledState(false) still runs but duplicate toasts are prevented.
---
Nitpick comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 227-232: The camera error handling in useScreenRecorder.ts should
also treat AbortError as a device/hardware issue; update the isDeviceError check
where cameraError is evaluated (the instanceof DOMException && ["NotFoundError",
"OverconstrainedError", "NotReadableError"].includes(cameraError.name) logic) to
include "AbortError" so toast.error(t(...)) shows "recording.cameraNotFound" for
AbortError as well.
- Around line 478-495: Polling in startRecording can hang up to the 5s timeout
if the user disables the webcam mid-wait because the loop only checks
webcamReady.current; update the polling to also observe the webcam enabled state
so it can exit immediately when disabled. Concretely, add or use an existing ref
like webcamEnabledRef (or sync webcamEnabled into a ref) and include a check
inside the interval callback (e.g., if (!webcamReady.current ||
!webcamEnabledRef.current) { clearInterval(interval); resolve(); }) and ensure
cleanup sets webcamEnabledRef appropriately; keep the existing fallback timeout
logic and the later webcamStream.current check/notification.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8919a8dd-76f9-4150-8a2d-29d04aa5c67b
📒 Files selected for processing (5)
.gitignoresrc/hooks/useScreenRecorder.tssrc/i18n/locales/en/editor.jsonsrc/i18n/locales/es/editor.jsonsrc/i18n/locales/zh-CN/editor.json
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- src/i18n/locales/es/editor.json
- src/i18n/locales/en/editor.json
- src/i18n/locales/zh-CN/editor.json
dheerajmr01
force-pushed
the
fix/camera-bugs
branch
from
fdc8833 to
b270aff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/hooks/useScreenRecorder.ts (1)
481-494: Extract the timeout value to a named constant.The 5000ms timeout is a magic number. Consider extracting it for clarity and maintainability.
♻️ Suggested refactor
Add near the other constants at the top of the file:
const WEBCAM_READY_TIMEOUT_MS = 5000; const WEBCAM_READY_POLL_INTERVAL_MS = 50;Then update the polling code:
const interval = setInterval(() => { if (webcamReady.current) { clearInterval(interval); resolve(); } - }, 50); + }, WEBCAM_READY_POLL_INTERVAL_MS); setTimeout(() => { clearInterval(interval); resolve(); - }, 5000); + }, WEBCAM_READY_TIMEOUT_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 481 - 494, Extract the hard-coded timeouts into named constants and use them in the polling logic: add constants (e.g. WEBCAM_READY_TIMEOUT_MS = 5000 and WEBCAM_READY_POLL_INTERVAL_MS = 50) near the top of useScreenRecorder (with other constants), then replace the literal 50 and 5000 in the Promise/polling block that checks webcamReady.current with these constants so the interval and timeout values are clear and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 213-219: The onended handler for the media track (track.onended)
is firing when you explicitly stop the track, causing the spurious "Webcam
disconnected" toast; before you stop or replace tracks (e.g., in the cleanup
path that calls track.stop(), in the webcam toggle logic, or when switching
devices inside useScreenRecorder), clear the handler by setting track.onended =
null (or a no-op) first, then stop the track and update webcamStream.current;
this preserves the existing restarting.current check and
setWebcamEnabledState/toast.error behavior for genuine disconnects while
preventing notifications when you intentionally stop the camera.
---
Nitpick comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 481-494: Extract the hard-coded timeouts into named constants and
use them in the polling logic: add constants (e.g. WEBCAM_READY_TIMEOUT_MS =
5000 and WEBCAM_READY_POLL_INTERVAL_MS = 50) near the top of useScreenRecorder
(with other constants), then replace the literal 50 and 5000 in the
Promise/polling block that checks webcamReady.current with these constants so
the interval and timeout values are clear and maintainable.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32e26240-d5d7-4f5d-8490-56e5edb69b8a
📒 Files selected for processing (5)
.gitignoresrc/hooks/useScreenRecorder.tssrc/i18n/locales/en/editor.jsonsrc/i18n/locales/es/editor.jsonsrc/i18n/locales/zh-CN/editor.json
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- src/i18n/locales/en/editor.json
- src/i18n/locales/zh-CN/editor.json
- src/i18n/locales/es/editor.json
…to prevent disconnect toast
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/hooks/useScreenRecorder.ts (1)
226-239:⚠️ Potential issue | 🟡 MinorUse the disconnect toast for unplugged-before-start failures.
This device-error branch still routes startup hardware-loss cases to
recording.cameraNotFound, so unplugging the webcam beforetrack.onendedis attached will not show the same disconnect toast as the during-recording path. That misses the start-time disconnect case described in the PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 226 - 239, When a device-error occurs at startup (cameraError with isDeviceError true) we should surface the same disconnect toast as the during-recording path rather than the startup "cameraNotFound" message; update the catch in useScreenRecorder (the block handling cameraError, setWebcamEnabledState, webcamReady.current) so that when isDeviceError is true it calls toast.error(t("recording.disconnected")) (or the same key used by the track.onended disconnect path) instead of t("recording.cameraNotFound"), ensuring the unplugged-before-start case shows the disconnect toast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useScreenRecorder.ts`:
- Line 106: The slow getUserMedia() calls need a cancellation/invalidation check
so late acquisitions can't repopulate webcamStream.current after the 5s fallback
toggles webcamEnabled off; add an AbortController or an acquireId flag used by
the webcam acquisition logic (the same place that uses webcamReady,
getUserMedia, and assigns webcamStream.current) and have the effect cleanup
increment/abort that id, and in the getUserMedia success callback verify the
controller is not aborted (or the acquireId still matches) and that
webcamEnabled is still true before assigning webcamStream.current or starting
recorder setup; if the result is stale, immediately stop all tracks on the
returned MediaStream and discard it. Ensure the same invalidation check is
applied to all acquisition sites mentioned (around useRef webcamReady, the
acquisition block that assigns webcamStream.current, and the 5s fallback/cleanup
logic).
---
Duplicate comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 226-239: When a device-error occurs at startup (cameraError with
isDeviceError true) we should surface the same disconnect toast as the
during-recording path rather than the startup "cameraNotFound" message; update
the catch in useScreenRecorder (the block handling cameraError,
setWebcamEnabledState, webcamReady.current) so that when isDeviceError is true
it calls toast.error(t("recording.disconnected")) (or the same key used by the
track.onended disconnect path) instead of t("recording.cameraNotFound"),
ensuring the unplugged-before-start case shows the disconnect toast.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4df8e4b0-0cd3-4ebf-897a-5c1b6f86b168
📒 Files selected for processing (1)
src/hooks/useScreenRecorder.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes these issues:
Type of Change
Related Issue(s)
Fixes #308
Screenshot :
Testing
Checklist
Summary by CodeRabbit
Bug Fixes
Documentation