Open
Conversation
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughstartRecording moved from a synchronous path to an asynchronous Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Driver as WebDriver
participant Browser as Browser Context
participant API as getDisplayMedia API
participant Recorder as MediaRecorder
participant Callback as Async Callback
rect rgba(100,150,200,0.5)
Note over Test,Callback: startRecording (driver.executeAsync)
Test->>Driver: executeAsync(start script)
Driver->>Browser: run async IIFE
Browser->>API: request getDisplayMedia()
API-->>Browser: return MediaStream / error
alt stream acquired
Browser->>Recorder: new MediaRecorder(stream)
Recorder->>Recorder: collect data, setup onstop/ondataavailable
Recorder->>Callback: done(true)
else failure
Browser->>Callback: done(false)
Browser->>Browser: cleanup (close tab, restore title)
end
Callback-->>Driver: resolve boolean
Driver-->>Test: result
end
rect rgba(150,100,200,0.5)
Note over Test,Browser: stopRecording (validation)
Test->>Driver: switch to recorder tab
Driver->>Browser: evaluate window.recorder ?
alt recorder exists
Browser->>Recorder: call stop()
Recorder-->>Browser: onstop -> finalize/download
Browser-->>Test: success
else missing
Browser-->>Test: mark FAIL, clear config, close tab
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tests/startRecording.js (1)
182-187:⚠️ Potential issue | 🔴 CriticalCritical: Recording config not being set correctly.
This sets
result.recordinginstead ofconfig.recording. ThestopRecording.jshandler relies onconfig.recording.type,config.recording.downloadPath, andconfig.recording.targetPath(see lines 34, 56, 63-65 in stopRecording.js), but these values won't be available because they're onresultinstead ofconfig.This will cause
stopRecordingto fail with undefined property errors or incorrect behavior sinceconfig.recordingwill only contain{ tab: ... }from line 100.🐛 Proposed fix
// Set recorder - result.recording = { + config.recording = { + ...config.recording, type: "MediaRecorder", tab: recorderTab.handle, downloadPath: path.join(os.tmpdir(), `${baseName}.webm`), // Where the recording will be downloaded. targetPath: filePath, // Where the recording will be saved. };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/startRecording.js` around lines 182 - 187, The code incorrectly assigns the recording metadata to result.recording instead of config.recording so stopRecording.js (which reads config.recording.type, config.recording.downloadPath, config.recording.targetPath) can't find those fields; update the assignment so that config.recording is set to an object containing type: "MediaRecorder", tab: recorderTab.handle, downloadPath: path.join(os.tmpdir(), `${baseName}.webm`), and targetPath: filePath (remove or stop using result.recording) so stopRecording's references to config.recording.* resolve correctly.
🧹 Nitpick comments (2)
src/tests/startRecording.js (1)
191-314: Remove unreachable dead code.The
return result;at line 193 makes all code from line 195 onwards unreachable. This appears to be leftover implementation code for non-Chrome contexts. Consider removing this dead code to improve maintainability, or if this functionality is planned for future implementation, move it to a separate commented block or issue tracker.♻️ Proposed cleanup
// Other context result.status = "SKIPPED"; result.description = `Recording is not supported for this context.`; return result; - - const dimensions = await driver.execute(() => { - return { - outerHeight: window.outerHeight, - outerWidth: window.outerWidth, ... - result.recording = ffmpegProcess; - } catch (error) { - // Couldn't save screenshot - result.status = "FAIL"; - result.description = `Couldn't start recording. ${error}`; - return result; - } } // PASS return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/startRecording.js` around lines 191 - 314, The early "return result;" makes everything after it unreachable; remove the dead block starting immediately after that return (the dimensions calculation, recordingSettings, ffmpeg args, instantiateCursor/ffmpeg spawn logic, and the try/catch around them) or move it into a clearly marked alternative branch (e.g., a separate function like startRecordingForNonChrome) if you intend to implement non-Chrome recording later; update references to symbols used in that removed block (dimensions, recordingSettings, ffmpegPath, spawn, instantiateCursor, ffmpegProcess, driver, step.record) so no unused imports/variables remain.src/tests/stopRecording.js (1)
56-58: Consider adding a timeout to prevent infinite waiting.This pre-existing loop could hang indefinitely if the file download fails or takes too long. While not part of this PR's changes, consider adding a timeout with a maximum retry count to improve reliability.
💡 Optional improvement
+ const maxWaitTime = 30000; // 30 seconds + const startTime = Date.now(); // Wait for file to be in download path - while (!fs.existsSync(config.recording.downloadPath)) { + while (!fs.existsSync(config.recording.downloadPath)) { + if (Date.now() - startTime > maxWaitTime) { + result.status = "FAIL"; + result.description = "Recording file download timed out."; + await driver.closeWindow(); + config.recording = null; + return result; + } await new Promise((r) => setTimeout(r, 1000)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/stopRecording.js` around lines 56 - 58, The waiting loop in src/tests/stopRecording.js that repeatedly checks fs.existsSync(config.recording.downloadPath) can hang forever; replace it with a bounded wait using either a maxRetries counter or a maxTimeout (e.g., track elapsed time) and a small sleep between attempts, and throw or return an error when the limit is reached; update the code that currently awaits new Promise((r) => setTimeout(r, 1000)) to use a reusable sleep/delay helper and enforce the timeout so callers of the stopRecording logic get a deterministic failure instead of waiting infinitely.
🤖 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/tests/startRecording.js`:
- Around line 182-187: The code incorrectly assigns the recording metadata to
result.recording instead of config.recording so stopRecording.js (which reads
config.recording.type, config.recording.downloadPath,
config.recording.targetPath) can't find those fields; update the assignment so
that config.recording is set to an object containing type: "MediaRecorder", tab:
recorderTab.handle, downloadPath: path.join(os.tmpdir(), `${baseName}.webm`),
and targetPath: filePath (remove or stop using result.recording) so
stopRecording's references to config.recording.* resolve correctly.
---
Nitpick comments:
In `@src/tests/startRecording.js`:
- Around line 191-314: The early "return result;" makes everything after it
unreachable; remove the dead block starting immediately after that return (the
dimensions calculation, recordingSettings, ffmpeg args, instantiateCursor/ffmpeg
spawn logic, and the try/catch around them) or move it into a clearly marked
alternative branch (e.g., a separate function like startRecordingForNonChrome)
if you intend to implement non-Chrome recording later; update references to
symbols used in that removed block (dimensions, recordingSettings, ffmpegPath,
spawn, instantiateCursor, ffmpegProcess, driver, step.record) so no unused
imports/variables remain.
In `@src/tests/stopRecording.js`:
- Around line 56-58: The waiting loop in src/tests/stopRecording.js that
repeatedly checks fs.existsSync(config.recording.downloadPath) can hang forever;
replace it with a bounded wait using either a maxRetries counter or a maxTimeout
(e.g., track elapsed time) and a small sleep between attempts, and throw or
return an error when the limit is reached; update the code that currently awaits
new Promise((r) => setTimeout(r, 1000)) to use a reusable sleep/delay helper and
enforce the timeout so callers of the stopRecording logic get a deterministic
failure instead of waiting infinitely.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tests/startRecording.js (1)
102-173:⚠️ Potential issue | 🟠 MajorUse
try/finallyfor tab/title cleanup across all failure modes.Between Line 102 and Line 173, multiple awaited driver calls can throw outside the explicit
!recorderStartedpath. That can strand the"RECORDER"tab or leave the original title unrecovered. Wrap this section intry/finallyand centralize restoration there.Suggested structure
- const documentTitle = await driver.getTitle(); - const originalTab = await driver.getWindowHandle(); - await driver.execute(() => (document.title = "RECORD_ME")); - await instantiateCursor(driver, { position: "center" }); - const recorderTab = await driver.createWindow("tab"); - await driver.switchToWindow(recorderTab.handle); - ... - if (!recorderStarted) { - ... - await driver.closeWindow(); - await driver.switchToWindow(originalTab); - await driver.execute((documentTitle) => { - document.title = documentTitle; - }, documentTitle); - return result; - } - await driver.switchToWindow(originalTab); - await driver.execute((documentTitle) => { - document.title = documentTitle; - }, documentTitle); + const documentTitle = await driver.getTitle(); + const originalTab = await driver.getWindowHandle(); + let recorderTab; + let openedRecorderTab = false; + try { + await driver.execute(() => (document.title = "RECORD_ME")); + await instantiateCursor(driver, { position: "center" }); + recorderTab = await driver.createWindow("tab"); + openedRecorderTab = true; + await driver.switchToWindow(recorderTab.handle); + ... + if (!recorderStarted) { + config.recording = null; + result.status = "FAIL"; + ... + return result; + } + } finally { + if (result.status === "FAIL" && openedRecorderTab) { + await driver.closeWindow(); + } + await driver.switchToWindow(originalTab); + await driver.execute((title) => { + document.title = title; + }, documentTitle); + }As per coding guidelines, "Always handle driver cleanup in try/finally blocks in browser automation code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/startRecording.js` around lines 102 - 173, The code that starts recording (the driver.executeAsync call that sets window.recorder) and the subsequent recorderStarted handling can throw between awaits and currently only cleans up when !recorderStarted; wrap the entire sequence from the executeAsync call through the post-start logic in a try/finally so tab/title cleanup always runs; move the calls that close the recorder tab and restore the original tab/title (the driver.closeWindow, driver.switchToWindow(originalTab), and driver.execute restoring documentTitle) into the finally block and ensure config.recording/result status handling remains correct (e.g., set config.recording = null and result when recorder fails) while leaving the navigator/getDisplayMedia error path behavior intact.
🤖 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/tests/startRecording.js`:
- Around line 153-154: Replace the direct console.error call in the
startRecording flow with the project logger and propagate the error via the done
callback: when executeAsync resolves with an error (the err variable used on
line 153), call log(config, "error", `Error starting recording: ${err}`) and
then invoke done(err) (or done(false) only if the codebase expects a boolean
failure indicator—prefer passing the error object). Update the block that
currently does console.error(`Error starting recording: ${err}`); done(false);
to use log(config, "error", ...) and to return the error through done(...) so
failures are both logged via log(config, ...) and propagated by the
done/executeAsync handling.
---
Outside diff comments:
In `@src/tests/startRecording.js`:
- Around line 102-173: The code that starts recording (the driver.executeAsync
call that sets window.recorder) and the subsequent recorderStarted handling can
throw between awaits and currently only cleans up when !recorderStarted; wrap
the entire sequence from the executeAsync call through the post-start logic in a
try/finally so tab/title cleanup always runs; move the calls that close the
recorder tab and restore the original tab/title (the driver.closeWindow,
driver.switchToWindow(originalTab), and driver.execute restoring documentTitle)
into the finally block and ensure config.recording/result status handling
remains correct (e.g., set config.recording = null and result when recorder
fails) while leaving the navigator/getDisplayMedia error path behavior intact.
src/tests/startRecording.js
Outdated
Comment on lines
153
to
154
| console.error(`Error starting recording: ${err}`); | ||
| done(false); |
Contributor
There was a problem hiding this comment.
Avoid console.error; route failures through project logger.
Line 153 uses direct console logging. Return the error details through done(...) and log in Node with log(config, "error", ...) after executeAsync resolves.
As per coding guidelines, "Use log(config, level, message) for all logging, where level = "debug"|"info"|"warning"|"error"".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tests/startRecording.js` around lines 153 - 154, Replace the direct
console.error call in the startRecording flow with the project logger and
propagate the error via the done callback: when executeAsync resolves with an
error (the err variable used on line 153), call log(config, "error", `Error
starting recording: ${err}`) and then invoke done(err) (or done(false) only if
the codebase expects a boolean failure indicator—prefer passing the error
object). Update the block that currently does console.error(`Error starting
recording: ${err}`); done(false); to use log(config, "error", ...) and to return
the error through done(...) so failures are both logged via log(config, ...) and
propagated by the done/executeAsync handling.
Contributor
|
Hey @InlinePizza! Thanks for the PR! Unfortunately, we're going through a rather large refactor at the moment, and |
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
3 participants
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.
I ran into #273, ran into the undefined error in stop step, and saw a blank tab with the title RECORD_ME next to the other tab.
driver.execute() sends a script to the browser and waits for it to return synchronously. But getDisplayMedia() returns a Promise [1] . So when the original code called captureAndDownload() without await on line 164, here's what happened:
That tab switch is the killer on macOS. The --auto-select-desktop-capture-source=RECORD_ME flag tells Chrome to auto-pick the tab titled "RECORD_ME," but the code has already moved focus away. On macOS specifically, this race seems to consistently cause getDisplayMedia() to reject or return nothing, so window.recorder never gets created.
driver.executeAsync() fixes this because it doesn't resolve on the Node.js side until the browser code explicitly calls the done callback. So the sequence becomes:
Checking recorderStarted before populating config.recording follows directly from this. If getDisplayMedia() rejects (permissions not granted, API failure, etc.), the done(false) path fires, and startRecording returns FAIL immediately instead of pretending everything worked and letting stopRecording crash later.
Summary by CodeRabbit