Conversation
HTML cleanup and speaker observation can hang after a successful join if the Teams page becomes unresponsive. Keep them off the setup critical path so recording can start and RecordingState owns leave decisions.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR migrates browser launching fully to CloakBrowser, adds external-navigation blocking and click-handling changes for Teams meetings, injects a cleanup stylesheet in Teams HTML cleaning, reworks recording sync/offset timing calculations, and adjusts non-blocking sequencing across several state-machine states. ChangesBrowser Launch & CloakBrowser Migration
Teams Meeting Page Navigation Blocking & Click Handling
Teams HTML Cleanup Stylesheet
Recording Sync Timing & Video Offset Calculation
State Machine Flow Adjustments
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Tip For best results, initiate chat on the files or code changes.
Just let me know! (￣ω￣;) hmm, what did you have in mind? |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/recording/ScreenRecorder.ts (1)
1245-1266: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAnchor the final trim to the sync flash, not
recordingStartTime.
(meetingStartTime - recordingStartTime)can over-trim by the FFmpeg capture-init gap and clip the beginning of the meeting. Use the detected flash in video time, e.g.syncResult.videoTimestamp + (meetingStartTime - syncSignalTimestamp) / 1000, or another capture-start adjustment for the final cut.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/recording/ScreenRecorder.ts` around lines 1245 - 1266, The final trim in ScreenRecorder should not be anchored to recordingStartTime because it can over-trim by the capture-init gap; instead, compute the cut from the detected sync flash using syncResult.videoTimestamp plus the meetingStartTime-to-sync-signal offset (or an equivalent capture-start adjustment). Update the offset calculation and the related debug/warn logic in the trimming block so the final cut is based on the flash timing rather than the raw recording start.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/browser/browser.ts`:
- Around line 39-47: The Chrome launch args in browser.ts are using repeated
--disable-features switches, which causes earlier disabled features to be
overridden. Update the browser launch options so the feature list in the
relevant args array is merged into a single --disable-features entry, and make
sure the Browser/browser launch setup still includes SigninInterception,
IdentityConsistency, ChromeBrowserCloudManagement, SignInPromo,
ChromeWhatsNewUI, and AccountConsistency together.
In `@src/state-machine/states/waiting-room-state.ts`:
- Around line 189-208: Extract the inline async redirect-check callback passed
to handleTimingControl in waiting-room-state’s waiting-room flow into a small
private method such as checkMeetRedirectAway on the same class. Keep the
existing behavior exactly the same, including the url() check, logging,
GLOBAL.setShouldRetry, and GLOBAL.setError with MeetingEndReason.BotNotAccepted,
and pass the new named method into handleTimingControl instead of the nested
ternary callback.
---
Outside diff comments:
In `@src/recording/ScreenRecorder.ts`:
- Around line 1245-1266: The final trim in ScreenRecorder should not be anchored
to recordingStartTime because it can over-trim by the capture-init gap; instead,
compute the cut from the detected sync flash using syncResult.videoTimestamp
plus the meetingStartTime-to-sync-signal offset (or an equivalent capture-start
adjustment). Update the offset calculation and the related debug/warn logic in
the trimming block so the final cut is based on the flash timing rather than the
raw recording start.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 6a9378fe-c386-4f17-94d4-91e8653116e1
📒 Files selected for processing (10)
Dockerfilesrc/browser/browser.tssrc/meeting/teams.tssrc/meeting/teams/htmlCleaner.tssrc/recording/ScreenRecorder.tssrc/state-machine/states/in-call-state.tssrc/state-machine/states/recording-state.tssrc/state-machine/states/waiting-room-state.tssrc/utils/CalculVideoOffset.tssrc/utils/SyncSignal.ts
| // Suppress Chrome's "Sign in to Chrome?" / "Turn on sync" dialogs that | ||
| // can appear when an authenticated browser session is used. | ||
| '--no-first-run', | ||
| '--no-default-browser-check', | ||
| '--disable-sync', | ||
| '--disable-component-update', | ||
| '--disable-signin', | ||
| '--disable-features=SigninInterception,IdentityConsistency,ChromeBrowserCloudManagement,SignInPromo,ChromeWhatsNewUI,AccountConsistency', | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Chromium multiple --disable-features command line only last one used
💡 Result:
In Chromium, the --disable-features and --enable-features command-line flags are not cumulative [1][2]. When these flags are specified multiple times on the same command line, only the value associated with the final occurrence is typically respected by the browser [2][3]. This behavior occurs because Chromium's command-line parsing logic [3][4] stores switches in a map structure where duplicate keys result in the previous value being overwritten by the last one encountered [3]. Consequently, if you provide --disable-features=FeatureA followed by --disable-features=FeatureB, Chromium will only disable FeatureB [2]. To effectively disable or enable multiple features, you must aggregate them into a single, comma-separated string within one instance of the flag (e.g., --disable-features=FeatureA,FeatureB) [5][1]. If you need to manage complex configurations across different sources (like environment variables or configuration files), you must programmatically combine the feature lists into a single command-line argument before launching the browser [1].
Citations:
- 1: Disable and enable flags ungoogled-software/ungoogled-chromium#3126
- 2: chromium / chrome fractional scaling hyprwm/Hyprland#11627
- 3: https://chromium.googlesource.com/chromium/src/+/master/base/command_line.h
- 4: https://chromium.googlesource.com/chromium/+/refs/heads/trunk/base/command_line.h
- 5: https://chromium.googlesource.com/chromium/src/+/da186a79eef96fac1a12df0719569b75f16e68ea/base/feature_list.h
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file around the cited lines.
git ls-files src/browser/browser.ts
wc -l src/browser/browser.ts
cat -n src/browser/browser.ts | sed -n '1,140p'Repository: Meeting-BaaS/meet-teams-bot
Length of output: 7529
Merge the repeated --disable-features flags
Chromium treats repeated --disable-features switches as non-cumulative, so the sign-in suppression list here is overridden by the later --disable-features entries. Fold all disabled features into a single flag so SigninInterception,IdentityConsistency,ChromeBrowserCloudManagement,SignInPromo,ChromeWhatsNewUI,AccountConsistency actually take effect.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/browser/browser.ts` around lines 39 - 47, The Chrome launch args in
browser.ts are using repeated --disable-features switches, which causes earlier
disabled features to be overridden. Update the browser launch options so the
feature list in the relevant args array is merged into a single
--disable-features entry, and make sure the Browser/browser launch setup still
includes SigninInterception, IdentityConsistency, ChromeBrowserCloudManagement,
SignInPromo, ChromeWhatsNewUI, and AccountConsistency together.
| const startTime = await handleTimingControl( | ||
| GLOBAL.get().start_time, | ||
| isMeet | ||
| ? async () => { | ||
| const url = this.context.playwrightPage?.url() ?? '' | ||
| if (url && !url.includes('meet.google.com')) { | ||
| console.log( | ||
| `Page navigated away from Meet during timing wait: ${url}`, | ||
| ) | ||
| GLOBAL.setShouldRetry(true) | ||
| GLOBAL.setError( | ||
| MeetingEndReason.BotNotAccepted, | ||
| 'Google Meet denied entry - page redirected during scheduled wait', | ||
| ) | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
| : undefined, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider extracting the inline redirect-check callback into a named method.
The anonymous async arrow function nested inside a ternary argument adds cognitive overhead. Extracting it to a small private method (e.g. checkMeetRedirectAway) would improve readability without behavior change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/state-machine/states/waiting-room-state.ts` around lines 189 - 208,
Extract the inline async redirect-check callback passed to handleTimingControl
in waiting-room-state’s waiting-room flow into a small private method such as
checkMeetRedirectAway on the same class. Keep the existing behavior exactly the
same, including the url() check, logging, GLOBAL.setShouldRetry, and
GLOBAL.setError with MeetingEndReason.BotNotAccepted, and pass the new named
method into handleTimingControl instead of the nested ternary callback.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n "handleTimingControl|meet.google.com|BotNotAccepted|shouldRetry|start_time" srcRepository: Meeting-BaaS/meet-teams-bot
Length of output: 6875
🏁 Script executed:
ast-grep outline src/utils/timing-control.ts --view expanded
ast-grep outline src/meeting/meet.ts --view expanded
sed -n '1,220p' src/utils/timing-control.ts
sed -n '1,120p' src/meeting/meet.ts
sed -n '400,470p' src/meeting/meet.tsRepository: Meeting-BaaS/meet-teams-bot
Length of output: 12099
🏁 Script executed:
sed -n '180,220p' src/state-machine/states/waiting-room-state.tsRepository: Meeting-BaaS/meet-teams-bot
Length of output: 2059
Compare the hostname here instead of using includes. page.url() can include meet.google.com in a path or redirect param on a non-Meet page, so this check can miss a real redirect away from Meet. new URL(url).hostname === 'meet.google.com' makes the denial check exact.
Chromium treats repeated feature-list switches as single-valued. Keep disabled feature lists in one flag so later entries do not overwrite earlier launch options.
1 participant
Summary
Validation
NPM_CONFIG_CACHE=/data/lazrossi/.npm npm run build