Repository navigation
Let candidates save local interview recordings - #469
Hipsterthomas wants to merge 5 commits into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Add screenshots to the pull request description to illustrate your work, as @ColtenOuO suggests. Example: #349 |
27efc95 to
d326b07
Compare
Screenshots of the local recording controls have been added to the pull request description. |
jserv
left a comment
jserv
left a comment
There was a problem hiding this comment.
Check CONTRIBUTING.md carefully and enforce the rules for consistent commit style.
Never write commit messages such as Update web/interview.js, as Git always tracks changed files.
jserv
left a comment
jserv
left a comment
There was a problem hiding this comment.
Before asking someone to review, you MUST resolve any regressions flagged by GitHub Actions.
| > | ||
| Thinking | ||
| </button> | ||
| <div id="local-recording" class="recording-row" hidden> |
There was a problem hiding this comment.
With this row visible the dock wraps onto another line, which is why local recording controls preserve the original problem reading space fails in CI. At 1280x720 the problem panel is 332.2px with the row hidden and 292.2px with it shown, in both the idle and the recording state. The 40px is one more wrapped row (a 2rem button plus the 0.5rem gap), so Start recording takes the reading space the test says it must keep. Fit the controls on the existing row at that width, or move them out of the wrapping row, and rerun the test. The loop stops at the first width, so the 390px case has not run yet.
| if (nodes.screenRecording.checked) return; | ||
| detachSelfView(); | ||
| if (active && !nodes.meetPresentation.checked) | ||
| detachSelfView = attachSelfView(nodes.selfView, state.localUserStream); |
There was a problem hiding this comment.
Local recording now paints the candidate's self view without the "I am recording this interview with a screen recorder" tick, but the README still says that without the tick the interview page shows no self view. It also never mentions Start recording or Start & record. Update that README paragraph in this PR so it describes both paths.
7b4502a to
c865a55
Compare
jserv
left a comment
jserv
left a comment
There was a problem hiding this comment.
Rebase onto the latest main branch, fix any regressions detected by GitHub Actions, and address the review comments.
Practice interviews need a personal recording without extra recording services. Keep capture and microphone mixing in the browser so candidates can download WebM files while recording failures leave interviews usable. Closes sysprog21#424
Devices can change while the capture picker is open. Recheck setup before joining so capture permission cannot bypass device readiness or a return to the lobby. Co-authored-by: Jim Huang <jserv.tw@gmail.com>
Device changes and recorder state transitions must not leave a local recording silent or its controls unreachable. Preserve interview media ownership and the existing reading space while making recording notices and the documented capture choices available to candidates.
System fonts can make the recording timer wrap the turn control and reduce the problem's reading space. Keep the existing dock height when recording starts, including on narrow screens.
Connection failures still offer offline practice. Keep local recording available there without another microphone request, while leaving camera and room cleanup intact. Release the borrowed microphone when the session really ends.
c865a55 to
1660451
Compare
| state.localUserStream = pool.stream; | ||
| await localRecording.start(); | ||
| if (finished) return; | ||
| if (!sampleReadiness().ready) { |
There was a problem hiding this comment.
If sampleReadiness() turns not-ready while the picker is open, this returns to editable setup with the recorder already running. Picking another microphone then makes the device pool stop the original track, but the recorder keeps its snapshot of that stopped track, while the interview joins with the replacement. A second "Start & record" click is a no-op because recording is active, so the saved file silently has no candidate voice. Stop the recording before returning to setup (and start afresh once readiness passes), or rebind the mic source when the pool replaces the track.
| clearInterval(timer); | ||
| paint(failure); | ||
| }; | ||
| video.addEventListener("ended", stopRecording, { once: true }); |
There was a problem hiding this comment.
The video.readyState check happens before await context.resume(), but the ended listener is only attached here, after it. If the user clicks "Stop sharing" during that await, startup still reports "Recording locally" with an ended video track, and the mixed-audio destination stays live so the recorder never stops itself. The timer, recorder and AudioContext then run until the user stops manually. Attach the ended listener right after the video check, and re-check video.readyState after the await, cancelling startup (release()) if capture already ended.
Candidates can save a practice interview without recording infrastructure. Capture the selected CodeTrial tab and mix its audio with the existing interview microphone into a local WebM, without another microphone request or LiveKit subscription. Recording errors leave the interview running.
An explicit Start & record option starts capture before joining Jim so the opening is included. End recording, browser Stop sharing, and End interview finalize and automatically download one file. Reuse the existing self-view while recording and keep the result entirely in the browser until downloaded.
Validation:
./scripts/test.shpassed on macOS after rebasing onto upstream main b0059b2: 1,940 Rust tests, 1,753 browser tests, and three server/browser tests passed. The 113 focused recording and related checks also passed with no skips.make buildand local HTTP smoke checks passed. The Linux-only Clang check and cargo-audit were not run locally, and 23 existing Rust tests remained ignored.Closes #424