feat:add countdown before record start#460
feat:add countdown before record start#460siddharthvaddem merged 10 commits intosiddharthvaddem:mainfrom
Conversation
|
Caution Review failedPull request was closed or merged during review 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:
📝 WalkthroughWalkthroughAdds a countdown-overlay feature across Electron and renderer: new preload/type defs, IPC handlers and overlay window, renderer CountdownOverlay component, and countdown orchestration integrated into the screen-recording hook with run-id based cancellation and safe IPC wrappers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ 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. 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: 6526f82776
ℹ️ 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/ipc/handlers.ts`:
- Around line 362-397: Maintain a small main-process state object (e.g., {
visible, value }) for the countdown overlay and update it from the ipc handlers:
when handling "countdown-overlay-show" (in the ipcMain.handle block that uses
getCountdownOverlayWindow/createCountdownOverlayWindow) set state.visible = true
and state.value = value, create the window if needed, and if
win.webContents.isLoading() attach a did-finish-load listener that, when fired
and win still exists, sends only the latest state via
win.webContents.send("countdown-overlay-value", state.value) and calls
win.showInactive() only if state.visible is still true; similarly, make
"countdown-overlay-set-value" update state.value and send immediately only if
the webContents is not loading, and make "countdown-overlay-hide" set
state.visible = false and either hide immediately (if not loading) or prevent
the did-finish-load handler from showing the window by relying on the updated
state.
In `@electron/main.ts`:
- Around line 331-341: The hidden countdown overlay is keeping getAllWindows() >
0 and breaking app activation; update the app.on("activate") logic to ignore the
overlay window instead of counting it: when deciding to recreate windows, filter
BrowserWindow.getAllWindows() to only visible/non-destroyed windows and exclude
the countdownOverlayWindow (refer to createCountdownOverlayWindowWrapper and the
countdownOverlayWindow variable) — if that filtered list is empty then recreate
the HUD/editor as before. Ensure you check w.isVisible() and !w.isDestroyed()
(or w !== countdownOverlayWindow) so the overlay doesn't prevent activation.
In `@electron/windows.ts`:
- Around line 190-223: The overlay BrowserWindow is created with show: !HEADLESS
which lets it paint before the renderer/first IPC value arrives; change the
BrowserWindow creation in windows.ts (the win = new BrowserWindow({...}) block)
to create the window hidden (show: false) and remove the HEADLESS-based
immediate show, then call win.show() from the existing IPC-ready path in
electron/ipc/handlers.ts (the handler flow around lines 362-379 that sends the
first value) once the renderer signals it's fully loaded/ready — ensure you
expose the BrowserWindow instance or send an IPC "ready-to-show" message from
the renderer and invoke win.show() only after that handshake to prevent the
blank flash.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 404-451: The cancel path can race with the async setup in
startRecordCountdown/startRecording: thread the current run id into
startRecording (pass runId from startRecordCountdown into startRecording) and
inside startRecording re-check countdownRunId.current === runId after every
await (e.g., after getSelectedSource, getUserMedia, any async device/track
setup) and abort if it changed; when aborting, ensure you stop and release any
acquired MediaStreamTracks and hide overlays/cleanup before returning. Update
cancelCountdown to continue bumping countdownRunId but rely on the guarded
checks in startRecording (which now accepts runId) to prevent recording from
starting after cancel.
🪄 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: 3808dbb7-f9f0-4ced-8eb2-d4596b91caab
📒 Files selected for processing (9)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/main.tselectron/preload.tselectron/windows.tssrc/App.tsxsrc/components/launch/CountdownOverlay.tsxsrc/hooks/useScreenRecorder.tssrc/main.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/hooks/useScreenRecorder.ts (2)
340-341: nit: inconsistent use of safe wrapperthe cleanup effect calls
window.electronAPI.hideCountdownOverlay()directly, while line 452 usessafeHideCountdownOverlay(). if the IPC call throws during unmount, it could cause issues. might be cleaner to use the safe wrapper here too.♻️ Suggested change
countdownRunId.current += 1; - void window.electronAPI.hideCountdownOverlay(); + void safeHideCountdownOverlay();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 340 - 341, In the cleanup effect, replace the direct IPC call window.electronAPI.hideCountdownOverlay() with the safe wrapper safeHideCountdownOverlay() (same one used at line 452) to avoid throwing during unmount; update the cleanup to call safeHideCountdownOverlay() and keep incrementing countdownRunId.current as-is so the unmount path is protected by the existing safe IPC wrapper.
419-422: kinda old-school with the alert() but it worksthis matches the existing pattern at line 461. might be worth converting both to toast notifications eventually for UX consistency, but not a blocker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 419 - 422, In useScreenRecorder, avoid using alert(...) for the selectedSource check; replace the alert(t("recording.selectSource")) call with the same toast/notification mechanism used elsewhere in this hook (the other validation branch) so the UX is consistent, keep the translation key t("recording.selectSource") and the early return behavior tied to selectedSource.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/ipc/handlers.ts`:
- Around line 417-429: The hide handler for the countdown overlay
(ipcMain.handle "countdown-overlay-hide") only sets
countdownOverlayState.visible = false and clears countdownOverlayState.value but
never actually hides the BrowserWindow; update the handler that uses
getCountdownOverlayWindow() and win.webContents.send("countdown-overlay-value",
...) to call win.hide() after sending the null value (mirroring the show
handler's use of win.showInactive()) so the overlay window is actually hidden
rather than just emptied.
---
Nitpick comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 340-341: In the cleanup effect, replace the direct IPC call
window.electronAPI.hideCountdownOverlay() with the safe wrapper
safeHideCountdownOverlay() (same one used at line 452) to avoid throwing during
unmount; update the cleanup to call safeHideCountdownOverlay() and keep
incrementing countdownRunId.current as-is so the unmount path is protected by
the existing safe IPC wrapper.
- Around line 419-422: In useScreenRecorder, avoid using alert(...) for the
selectedSource check; replace the alert(t("recording.selectSource")) call with
the same toast/notification mechanism used elsewhere in this hook (the other
validation branch) so the UX is consistent, keep the translation key
t("recording.selectSource") and the early return behavior tied to
selectedSource.
🪄 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: ad152987-5c97-4cdc-94d5-039085c5ccd6
📒 Files selected for processing (7)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/main.tselectron/preload.tselectron/windows.tssrc/components/launch/CountdownOverlay.tsxsrc/hooks/useScreenRecorder.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/launch/CountdownOverlay.tsx
- electron/electron-env.d.ts
- electron/windows.ts
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/hooks/useScreenRecorder.ts (1)
604-639:⚠️ Potential issue | 🟠 MajorThere’s still one async cancel gap in
startRecording().
applyConstraints()at Line 605 is awaited, but the next run-id check does not happen until Line 636. If the user cancels during that constraint negotiation, this path still falls through and starts the recorder anyway. same race, just one await later.small guard, big difference
try { await videoTrack.applyConstraints({ frameRate: { ideal: TARGET_FRAME_RATE, max: TARGET_FRAME_RATE }, width: { ideal: TARGET_WIDTH, max: TARGET_WIDTH }, height: { ideal: TARGET_HEIGHT, max: TARGET_HEIGHT }, }); } catch (constraintError) { console.warn( "Unable to lock 4K/60fps constraints, using best available track settings.", constraintError, ); } + + if (!isCountdownRunActive(countdownRunToken)) { + teardownMedia(); + return; + } let {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 604 - 639, startRecording() can race: after awaiting videoTrack.applyConstraints(...) the code may proceed even if the run was cancelled; add an immediate cancellation guard right after the try/catch (and before calling videoTrack.getSettings()/computeBitrate/selectMimeType) by calling isCountdownRunActive(countdownRunToken) and, if false, call teardownMedia() and return; reference applyConstraints, isCountdownRunActive, countdownRunToken, teardownMedia, and startRecording so you place the early-return check in the correct spot.
🧹 Nitpick comments (1)
electron/ipc/handlers.ts (1)
382-400: looks good, small nit on potential listener stackingIf
countdown-overlay-showis called multiple times while the window is still loading, you'll stack up multipledid-finish-loadlisteners (each using.once()). When load completes, all of them fire andflushCountdownOverlayStateruns multiple times. It's harmless - just redundant sends of the same value - but if you want to be tidy:// track pending listener let pendingFlush = false; // in handler: if (win.webContents.isLoading()) { if (!pendingFlush) { pendingFlush = true; win.webContents.once("did-finish-load", () => { pendingFlush = false; if (!win.isDestroyed()) { flushCountdownOverlayState(win); } }); } } else { flushCountdownOverlayState(win); }lowkey optional though - the current approach works fine, just chatty.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 382 - 400, Multiple calls to the ipcMain handler "countdown-overlay-show" can register multiple win.webContents.once("did-finish-load", ...) listeners causing redundant flushCountdownOverlayState calls; fix it by adding a module-scoped boolean flag (e.g., pendingCountdownFlush) and use it inside the handler that references getCountdownOverlayWindow/createCountdownOverlayWindow: when win.webContents.isLoading() only attach the once listener if pendingCountdownFlush is false, set pendingCountdownFlush = true when attaching, and clear it inside the once callback before calling flushCountdownOverlayState (also guard with win.isDestroyed()), so repeated calls while loading will no-op until the load finishes.
🤖 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/components/launch/CountdownOverlay.tsx`:
- Around line 15-19: The countdown digits in CountdownOverlay (the div with
className "text-white/90 text-[120px] font-bold leading-none tabular-nums") need
better contrast; add a subtle contrast treatment such as a small text-shadow (or
a semi-transparent dark outline/background) to the digits so they remain
readable on bright/translucent backgrounds—update the className or style for
that element (or its wrapper) to include the chosen shadow/outline/backdrop
utility (e.g., add a faint text-shadow or bg-black/20 with rounded padding)
while preserving sizing and pointer-events-none on the outer container.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 428-453: The countdown overlay paint race occurs because IPC
awaits (safeShowCountdownOverlay / safeSetCountdownOverlayValue) can resolve
after the run was cancelled and repaint; to fix it, after every await of
safeShowCountdownOverlay and safeSetCountdownOverlayValue check that
countdownRunId.current === runId before performing any UI updates (i.e., only
call safeSetCountdownOverlayValue or proceed to next steps when still the
current run), and change the finally block to always call
safeHideCountdownOverlay (remove the conditional guard around
safeHideCountdownOverlay) while still only calling setCountdownActive(false)
when countdownRunId.current === runId if you need that guard; reference
functions/variables: safeShowCountdownOverlay, safeSetCountdownOverlayValue,
safeHideCountdownOverlay, countdownRunId, runId, startRecording,
setCountdownActive.
---
Duplicate comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 604-639: startRecording() can race: after awaiting
videoTrack.applyConstraints(...) the code may proceed even if the run was
cancelled; add an immediate cancellation guard right after the try/catch (and
before calling videoTrack.getSettings()/computeBitrate/selectMimeType) by
calling isCountdownRunActive(countdownRunToken) and, if false, call
teardownMedia() and return; reference applyConstraints, isCountdownRunActive,
countdownRunToken, teardownMedia, and startRecording so you place the
early-return check in the correct spot.
---
Nitpick comments:
In `@electron/ipc/handlers.ts`:
- Around line 382-400: Multiple calls to the ipcMain handler
"countdown-overlay-show" can register multiple
win.webContents.once("did-finish-load", ...) listeners causing redundant
flushCountdownOverlayState calls; fix it by adding a module-scoped boolean flag
(e.g., pendingCountdownFlush) and use it inside the handler that references
getCountdownOverlayWindow/createCountdownOverlayWindow: when
win.webContents.isLoading() only attach the once listener if
pendingCountdownFlush is false, set pendingCountdownFlush = true when attaching,
and clear it inside the once callback before calling flushCountdownOverlayState
(also guard with win.isDestroyed()), so repeated calls while loading will no-op
until the load finishes.
🪄 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: cb995814-d481-4616-85cc-62294934d399
📒 Files selected for processing (9)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/main.tselectron/preload.tselectron/windows.tssrc/App.tsxsrc/components/launch/CountdownOverlay.tsxsrc/hooks/useScreenRecorder.tssrc/main.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 761-767: The current toggleRecording logic checks countdownActive
before recording, which can let a click intended to stop an already-started
recording cancel the countdown instead; change the branching to prioritize
active recording first: if recording then call stopRecording.current(), else if
countdownActive then call cancelCountdown(), else call startRecordCountdown();
apply the same reorder to the duplicate handler that uses recording,
countdownActive, cancelCountdown and startRecordCountdown (the block around
lines 829-833) so both handlers consistently prioritize stopping an active
recording.
🪄 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: 25789ab7-9c5e-43af-819d-63ee8cc5a984
📒 Files selected for processing (5)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/preload.tssrc/components/launch/CountdownOverlay.tsxsrc/hooks/useScreenRecorder.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/launch/CountdownOverlay.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/electron-env.d.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 339-343: The direct calls to
window.electronAPI.hideCountdownOverlay in the cleanup/cancel paths (around
countdownRunId.current and allowAutoFinalize.current) can reject and are
currently uncaught; replace these direct awaits/calls with the existing safe
wrapper used elsewhere in this module (the module-level safe invoke used for
electronAPI calls) so rejections are swallowed/handled, and apply the same
change to the other occurrence around lines 373–378; ensure you pass the
activeRunId (countdownRunId.current) into the safe wrapper instead of calling
hideCountdownOverlay directly.
🪄 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: 219635e7-5ff2-4cb0-92fc-4ef7074a3c39
📒 Files selected for processing (1)
src/hooks/useScreenRecorder.ts
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 418-432: The issue is that the code only reserves a countdownRunId
after awaiting getSelectedSource(), leaving a race where another invocation can
claim the run; to fix, claim and store the runId before any await by
incrementing countdownRunId.current and assigning runId at the top of
startRecordCountdown (before calling window.electronAPI.getSelectedSource()),
then proceed to await getSelectedSource() and only call setCountdownActive(true)
after you verify selectedSource is non-null; keep the early-return behavior if
no source is selected (no need to call setCountdownActive in that case).
🪄 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: e9e36a68-9d5d-4855-958f-7a61f0ace72a
📒 Files selected for processing (9)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/main.tselectron/preload.tselectron/windows.tssrc/App.tsxsrc/components/launch/CountdownOverlay.tsxsrc/hooks/useScreenRecorder.tssrc/main.tsx
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/App.tsx (1)
17-33: nit: dependency array might be over-specifiedThe
[windowType]dependency causes the effect to re-run wheneverwindowTypechanges, but sincewindowTypeis derived from the URL (which doesn't change for a given window), the effect only really needs to run once on mount. This also meansloadAllCustomFonts()gets called again if state somehow changes.lowkey harmless since the window type won't change during the overlay's lifetime, but
[]would be cleaner if the intent is mount-only initialization.nit: cleaner dependency array
- }, [windowType]); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 17 - 33, The effect in useEffect currently depends on [windowType], causing it to re-run when windowType changes even though windowType is derived once from the URL; change the dependency array to [] so the effect runs only on mount, keep the same logic that reads const type = new URLSearchParams(window.location.search).get("windowType") || "" and calls setWindowType(type) if different, and still call loadAllCustomFonts().catch(...); if your linter complains about exhaustive-deps, add an inline eslint-disable-next-line comment for that rule scoped to this useEffect.electron/ipc/handlers.ts (1)
362-439: Would add one regression test for the loading path.lowkey this block is where the race fix lives now. a focused test for
show(3)→setValue(2/1)andhide()beforedid-finish-loadwould make the cancel/restart behavior a lot less likely to get kinda cursed again later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 362 - 439, Add a regression test that exercises the loading path for the countdown overlay: simulate calling the IPC handler "countdown-overlay-show" with value 3, then call "countdown-overlay-set-value" with 2 and 1 and finally "countdown-overlay-hide" all before the window emits "did-finish-load"; verify that when the window finishes loading (did-finish-load) the flushCountdownOverlayState/flush behavior uses the latest state (visible false/value null if hidden, or the last set value if still visible) and that no stale or duplicate sends occur. Target the handlers "countdown-overlay-show", "countdown-overlay-set-value", and "countdown-overlay-hide" and exercise the win.webContents.isLoading branch by creating a mocked countdown window via getCountdownOverlayWindow/createCountdownOverlayWindow and controlling did-finish-load to assert correct sends to "countdown-overlay-value".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@electron/ipc/handlers.ts`:
- Around line 362-439: Add a regression test that exercises the loading path for
the countdown overlay: simulate calling the IPC handler "countdown-overlay-show"
with value 3, then call "countdown-overlay-set-value" with 2 and 1 and finally
"countdown-overlay-hide" all before the window emits "did-finish-load"; verify
that when the window finishes loading (did-finish-load) the
flushCountdownOverlayState/flush behavior uses the latest state (visible
false/value null if hidden, or the last set value if still visible) and that no
stale or duplicate sends occur. Target the handlers "countdown-overlay-show",
"countdown-overlay-set-value", and "countdown-overlay-hide" and exercise the
win.webContents.isLoading branch by creating a mocked countdown window via
getCountdownOverlayWindow/createCountdownOverlayWindow and controlling
did-finish-load to assert correct sends to "countdown-overlay-value".
In `@src/App.tsx`:
- Around line 17-33: The effect in useEffect currently depends on [windowType],
causing it to re-run when windowType changes even though windowType is derived
once from the URL; change the dependency array to [] so the effect runs only on
mount, keep the same logic that reads const type = new
URLSearchParams(window.location.search).get("windowType") || "" and calls
setWindowType(type) if different, and still call
loadAllCustomFonts().catch(...); if your linter complains about exhaustive-deps,
add an inline eslint-disable-next-line comment for that rule scoped to this
useEffect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0c04f99-3cea-4b4d-82ac-f06c9e445502
📒 Files selected for processing (9)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/main.tselectron/preload.tselectron/windows.tssrc/App.tsxsrc/components/launch/CountdownOverlay.tsxsrc/hooks/useScreenRecorder.tssrc/main.tsx
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
imAaryash
left a comment
imAaryash
left a comment
There was a problem hiding this comment.
Hii @Galactic99 , its a great addon, could you make the countdown's UI a bit better as its getting mixed with the screen maybe add something like a low opacity circle BG to the countdown :)
|
@imAaryash Thank you for the suggestion. Will implement it right away. |
|
GitHub Dark Mode White Background Dark Background Black Background @imAaryash Is this alright? Can I proceed with the changes? |
looks good |
|
is this ready for another review? @Galactic99 |
|
No @siddharthvaddem, marking this PR as draft for now. |
Galactic99
force-pushed
the
feat/countdown-before-record-start
branch
from
b9e1c16 to
7e02856
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/hooks/useScreenRecorder.ts (1)
373-380: nit: use thesafeHideCountdownOverlaywrapper for consistencyevery other call site (cleanup at L342, the countdown loop finally at L471) goes through the
safeHideCountdownOverlaywrapper. this one opens its own inline.catch(...)which works identically but makes the codebase read a bit inconsistent. kinda cursed to have two patterns for the exact same thing.♻️ tiny cleanup
const cancelCountdown = () => { const activeRunId = countdownRunId.current; countdownRunId.current += 1; setCountdownActive(false); - void window.electronAPI.hideCountdownOverlay(activeRunId).catch((error) => { - console.warn("Failed to hide countdown overlay during cancel:", error); - }); + void safeHideCountdownOverlay(activeRunId); };(note: you'll need to hoist
safeHideCountdownOverlayabovecancelCountdown, or forward-reference it via a ref/useCallback, since it's currently declared a few lines below. easier fix is to just reorder the declarations.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 373 - 380, The cancelCountdown function currently calls window.electronAPI.hideCountdownOverlay with an inline .catch handler; replace that inline call with the existing safeHideCountdownOverlay wrapper for consistency (pass the same activeRunId), and ensure safeHideCountdownOverlay is declared before cancelCountdown (or forward-reference it via a ref/useCallback) so cancelCountdown can call safeHideCountdownOverlay(activeRunId); keep the rest of cancelCountdown logic (incrementing countdownRunId.current and setCountdownActive(false)) unchanged.src/App.tsx (1)
17-33: nit: font loader is now roped into the windowType depchanging the dep from
[]to[windowType]meansloadAllCustomFonts()also re-fires wheneverwindowTypeflips. in practice this only runs twice (initial""→ parsed value), so it's lowkey harmless, but it's kinda a side effect you probably didn't mean to re-trigger. worth splitting into two effects so the transparent-background/query-sync logic is dep-driven while font loading stays mount-only.♻️ suggested split
useEffect(() => { const type = new URLSearchParams(window.location.search).get("windowType") || ""; if (type !== windowType) { setWindowType(type); } if (type === "hud-overlay" || type === "source-selector" || type === "countdown-overlay") { document.body.style.background = "transparent"; document.documentElement.style.background = "transparent"; document.getElementById("root")?.style.setProperty("background", "transparent"); } - - // Load custom fonts on app initialization - loadAllCustomFonts().catch((error) => { - console.error("Failed to load custom fonts:", error); - }); }, [windowType]); + + useEffect(() => { + // Load custom fonts on app initialization + loadAllCustomFonts().catch((error) => { + console.error("Failed to load custom fonts:", error); + }); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 17 - 33, The current useEffect mixes windowType-driven behavior with a mount-only side effect (loadAllCustomFonts), causing fonts to reload whenever windowType changes; split into two effects: keep the existing effect that reads URL, compares and calls setWindowType and updates transparent backgrounds inside a useEffect dependent on [windowType], and create a separate useEffect with an empty dependency array that calls loadAllCustomFonts().catch(...) so fonts load only once on mount; reference functions/hooks: useEffect, setWindowType, windowType, and loadAllCustomFonts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/App.tsx`:
- Around line 17-33: The current useEffect mixes windowType-driven behavior with
a mount-only side effect (loadAllCustomFonts), causing fonts to reload whenever
windowType changes; split into two effects: keep the existing effect that reads
URL, compares and calls setWindowType and updates transparent backgrounds inside
a useEffect dependent on [windowType], and create a separate useEffect with an
empty dependency array that calls loadAllCustomFonts().catch(...) so fonts load
only once on mount; reference functions/hooks: useEffect, setWindowType,
windowType, and loadAllCustomFonts.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 373-380: The cancelCountdown function currently calls
window.electronAPI.hideCountdownOverlay with an inline .catch handler; replace
that inline call with the existing safeHideCountdownOverlay wrapper for
consistency (pass the same activeRunId), and ensure safeHideCountdownOverlay is
declared before cancelCountdown (or forward-reference it via a ref/useCallback)
so cancelCountdown can call safeHideCountdownOverlay(activeRunId); keep the rest
of cancelCountdown logic (incrementing countdownRunId.current and
setCountdownActive(false)) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fccc6eda-a599-4b32-ba3a-c344c1e07bf8
📒 Files selected for processing (9)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/main.tselectron/preload.tselectron/windows.tssrc/App.tsxsrc/components/launch/CountdownOverlay.tsxsrc/hooks/useScreenRecorder.tssrc/main.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/launch/CountdownOverlay.tsx
- src/main.tsx
- electron/main.ts
- electron/preload.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e02856836
ℹ️ 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".
…ing recording setup.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/hooks/useScreenRecorder.ts (1)
409-481: countdown orchestration looks solid.race handling in
startRecordCountdownis kinda thorough now — runId is claimed before any await, every post-await resume point re-checkscountdownRunId, and thefinallyblock cleans up when we bail beforeoverlayHiddenBeforeStartflips. paired with the per-stageisCountdownRunActivegates instartRecording, cancel/unmount during any phase should land gracefully. nice work untangling this.one tiny optional nit: the 1s
window.setTimeouton Line 459 isn't cancellable, so a cancel during the tick still burns up to a full second before the loop notices and returns. not user-visible (overlay is already hidden bycancelCountdown), but if you ever want snappier teardown, anAbortController-backed sleep would do it. totally fine to punt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 409 - 481, The one-second window.setTimeout inside startRecordCountdown can delay teardown; make the per-tick sleep cancellable by introducing an AbortController tied to the current runId (e.g., countdownAbortController.current) and replace the hard setTimeout with an AbortSignal-aware sleep that resolves immediately when aborted; ensure cancelCountdown/cleanup sets countdownRunId appropriately and aborts the controller so safeShowCountdownOverlay, safeSetCountdownOverlayValue, safeHideCountdownOverlay, and the loop in startRecordCountdown detect cancellation without waiting the full second.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 409-481: The one-second window.setTimeout inside
startRecordCountdown can delay teardown; make the per-tick sleep cancellable by
introducing an AbortController tied to the current runId (e.g.,
countdownAbortController.current) and replace the hard setTimeout with an
AbortSignal-aware sleep that resolves immediately when aborted; ensure
cancelCountdown/cleanup sets countdownRunId appropriately and aborts the
controller so safeShowCountdownOverlay, safeSetCountdownOverlayValue,
safeHideCountdownOverlay, and the loop in startRecordCountdown detect
cancellation without waiting the full second.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e0e0b3f-9a50-4ac1-86ab-0fba8aa99e51
📒 Files selected for processing (2)
src/App.tsxsrc/hooks/useScreenRecorder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/App.tsx
|
hey, @Galactic99 have you thought of any solution for the flickering? |
|
@imAaryash yes
|
|
ohhh great, could you drop the final test video |
CountDownBeforeStartFinal.mp4At first I am showing the non flickering state by rapid start/stop. It is not a bug. |
|
@Galactic99 please resolve the conflicts before this can be merged |
|
done i have resolved it |
3 participants
Description
Implemented a dedicated centered countdown overlay before recording start while keeping the HUD visible and stationary at the bottom.
Key implementation details:
Motivation
Helps the user to prepare themselves before recording actually begins.
Type of Change
Related Issue(s)
#458 #453
Screenshots / Video
Video :
After-Countdown-Timer.mp4
Testing
Checklist
Thank you for contributing!
Summary by CodeRabbit
New Features
Bug Fixes / Reliability