feat(agent_meetings): wire mascotId through full stack + fix tests#3363
Conversation
…near-limit banner for custom providers (tinyhumansai#3097) Issue 3 — Top-up banner shown despite custom provider: `isNearLimit` was not guarded by `isFullyRoutedAway`, so users who routed all CHAT_WORKLOADS away from OpenHuman still saw the near-limit warning when background workloads kept billing data flowing. Fix: add `!isFullyRoutedAway` to `isNearLimit` (mirrors the existing guards on `isBudgetExhausted` / `isAtLimit`). Adds a regression test. Issue 4 — Onboarding reset loop on Windows: `RuntimeChoicePage` silently swallowed `completeAndExit()` failures (only logging to console), leaving the user stuck on the runtime-choice screen with no feedback or retry path. If they navigated back to the welcome page, the loop repeated indefinitely. Fix: adopt the `exitError` state pattern from `VaultSetupStep` — catch the error, set `exitError`, and render a coral error banner with a localised message so the user knows to retry. Adds i18n key to all 13 locale files. Issues 1 & 2 (credit drain during setup / 9-connection limit) are architectural: background workloads route through OpenHuman Cloud by design even with a custom chat provider, and the connection limit is enforced server-side by the upstream Composio API. Both are called out in the PR description for product visibility.
…ough Rust core to backend bot:join - Add RiveColors struct and agent_name/system_prompt/rive_colors fields to BackendMeetJoinRequest (types.rs) - Thread optional fields into the bot:join socket emit payload in handle_join (ops.rs) - Expose agentName/systemPrompt/riveColors in BackendMeetJoinInput and forward as snake_case RPC params in joinMeetViaBackendBot (meetCallService.ts) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…backend bot:join - Add RiveColors struct with optional primaryColor/secondaryColor - Extend BackendMeetJoinRequest with agent_name, system_prompt, rive_colors - Update ops.rs and event_handlers.rs to pass new params through Rust core - Update meetCallService.ts and MeetingBotsCard to supply custom agent params - Update tests for new payload shape Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…api client ngrok free tier shows an interstitial browser warning page on first request. This causes the preflight health check to fail, making the Google login button appear unresponsive. Adding the header bypasses the warning page. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ground - Thread mascotId through full stack: BackendMeetJoinInput (TS) → agent_meetings_join RPC params → Rust BackendMeetJoinRequest → bot:join Socket.IO payload → backend recallBotService - Add agent_name, system_prompt, mascot_id, rive_colors to schemas.rs join input schema (was missing from previous commit) - Fix MeetingBotsCard: add missing leaveBackendMeetBot + Redux imports, wire dispatch(setBackendMeetJoining) before bot join call, pass agentName: displayName to joinMeetViaBackendBot - Fix backendMeetService.test.ts: update strict assertion to include all new params; add mascotId, agentName, systemPrompt, riveColors coverage tests (14 tests total) Closes tinyhumansai#3280 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughThis PR extends the Google Meet bot integration with backend-driven customization, redesigns the MeetingBots UI to use Redux backend-meet state, adds comprehensive mascot video capture and outbound-video diagnostics with canvas pixel probing, and expands localization across all supported languages. ChangesBackend Meeting Bot Request and Ops
Meeting Bots UI and Skills Integration
Mascot Video Capture and Rendering Diagnostics
Onboarding Error Handling and i18n Expansion
Lint and Misc Cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
app/src/components/skills/MeetingBotsCard.tsx (3)
81-98: ⚡ Quick winEmotion parsing is brittle.
Lines 91-96: The substring-matching logic for emotion keywords (e.g.,
e.includes('happy'),e.includes('celebrat')) is fragile. Partial matches could yield false positives (e.g., "unhappy" matches "happy"), and the order of checks determines precedence without clear rationale.More robust emotion mapping
Use word boundaries or exact-match sets:
function faceFromMeetState( status: BackendMeetStatus, lastReply: BackendMeetReplyEvent | null, lastHarness: BackendMeetHarnessEvent | null, ): MascotFace { if (status === 'joining') return 'thinking'; if (status === 'error') return 'concerned'; if (status === 'ended') return 'happy'; if (lastHarness) return 'thinking'; if (lastReply) { const emotion = (lastReply.emotion ?? '').toLowerCase(); const happyWords = ['happy', 'pleased', 'joy', 'excited']; const celebrateWords = ['celebrating', 'proud']; const concernedWords = ['concerned', 'worried', 'unsure']; const curiousWords = ['curious', 'interested']; if (happyWords.some(w => emotion.includes(w))) return 'happy'; if (celebrateWords.some(w => emotion.includes(w))) return 'celebrating'; if (concernedWords.some(w => emotion.includes(w))) return 'concerned'; if (curiousWords.some(w => emotion.includes(w))) return 'curious'; } return 'idle'; }Alternatively, expect the backend to send a constrained set of emotion values and map directly.
🤖 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 `@app/src/components/skills/MeetingBotsCard.tsx` around lines 81 - 98, The emotion parsing in faceFromMeetState is brittle because it uses naive substring checks on lastReply.emotion (e.g., e.includes('happy')) which causes false positives like "unhappy" and unclear precedence; fix it by normalizing the emotion string (toLowerCase), tokenizing or matching with word-boundary regex, and mapping against explicit word lists or an exact-value map (e.g., happyWords = ['happy','pleased','joy','excited'], celebrateWords = ['celebrating','proud'], etc.) using token/word-boundary checks (or exact match) instead of raw includes, and ensure the check order is deliberate and documented so each emotion branch in faceFromMeetState returns the intended MascotFace.
116-122: ⚡ Quick winLeave button not disabled during async call.
handleLeaveis async (line 116) but the button's disabled state (line 143) only checkscanLeave(derived fromstatus). A user could click "Leave" multiple times before the async call resolves, potentially sending duplicatebot:leaveevents.Track leaving state
+const [leaving, setLeaving] = useState(false); const handleLeave = async () => { + setLeaving(true); try { await leaveBackendMeetBot('user-requested'); } catch (err) { onToast?.({ type: 'error', title: t('skills.meetingBots.couldNotStartTitle'), message: String(err) }); + } finally { + setLeaving(false); } }; -{canLeave && ( +{canLeave && !leaving && ( <button type="button" onClick={handleLeave} - className="..."> + disabled={leaving} + className="... disabled:opacity-50 disabled:cursor-not-allowed"> {t('skills.meetingBots.leaveButton')} </button> )}🤖 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 `@app/src/components/skills/MeetingBotsCard.tsx` around lines 116 - 122, The Leave button can be clicked multiple times because handleLeave is async but the disabled state only depends on canLeave; add a local state flag (e.g., leaving) in the MeetingBotsCard component, set leaving = true at the start of handleLeave and reset it in a finally block, guard the handler to return early if leaving is already true, and update the button's disabled prop to include the leaving flag (disabled when !canLeave or leaving) so duplicate bot:leave events cannot be emitted.
293-300: 💤 Low value
agentNameduplicatesdisplayName—clarify intent.Line 300 passes
agentName: displayName. This means the bot's display name and the agent name sent to the backend are always identical in this flow. If that's intentional (the bot represents the agent), it's fine. If they should diverge (e.g., separate mascot identity vs. UI label), consider makingagentNameits own state or deriving it from persona/mascot config.🤖 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 `@app/src/components/skills/MeetingBotsCard.tsx` around lines 293 - 300, The current call to joinMeetViaBackendBot passes agentName: displayName which duplicates UI label and backend agent identity; decide intended behavior and make agentName explicit: either keep it intentionally identical (leave as is) or create a separate variable/state (e.g., agentNameFromPersona or mascotAgentName) derived from the persona/mascot config or a new input, then pass that variable to joinMeetViaBackendBot({ meetUrl, displayName, platform, agentName }) and update any places that assume agentName === displayName (such as setBackendMeetJoining or the backend payload) so the UI label (displayName) and backend agent identity (agentName) can diverge if needed.src/openhuman/agent_meetings/ops.rs (1)
166-190: 💤 Low valueConsider consolidating optional field insertion.
The repeated
if let Some(map) = join_payload.as_object_mut()pattern (lines 171-189) is functional but verbose. Each optional field requires a separate nested block.Optional refactor
Build all fields upfront with
Optionhandling in thejson!macro:let join_payload = json!({ "meetUrl": normalized_url.as_str(), "displayName": display_name, "platform": platform, "agentName": req.agent_name.as_deref(), "systemPrompt": req.system_prompt.as_deref(), "mascotId": req.mascot_id.as_deref(), "riveColors": req.rive_colors.as_ref().map(|c| json!({ "primaryColor": c.primary_color, "secondaryColor": c.secondary_color, })), });Then use
serde_jsonto stripnullvalues before emitting (or accept that Socket.IO toleratesnullfor omitted fields).🤖 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/openhuman/agent_meetings/ops.rs` around lines 166 - 190, The repeated mutation of join_payload via join_payload.as_object_mut() is verbose; instead construct join_payload in one json! call using Option-friendly values (e.g., set "agentName": req.agent_name.as_deref(), "systemPrompt": req.system_prompt.as_deref(), "mascotId": req.mascot_id.as_deref(), and "riveColors": req.rive_colors.as_ref().map(|c| json!({ "primaryColor": c.primary_color, "secondaryColor": c.secondary_color }))) so all optional fields are included conditionally, and then either accept nulls or run a small serde_json cleanup to remove nulls before sending; apply this change where join_payload is created and remove the subsequent as_object_mut() insertion blocks.
🤖 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 `@app/src-tauri/src/meet_video/camera_bridge.js`:
- Around line 417-429: probeVideoElements() currently exposes track.label (via
the track object created in camera_bridge.js) which can leak PII; replace the
raw label field with a redacted flag (e.g., isMascotTrack) computed client-side
instead of sending full label, and update the Rust-side matcher to read
isMascotTrack rather than label. Specifically, stop including track.label in the
object created in probeVideoElements() (and the second occurrence around lines
574-585), add a boolean field isMascotTrack (derived from a safe heuristic or by
calling into the Rust matcher API) and ensure
window.__openhumanCameraBridgeInfo() payloads use that boolean; then update the
Rust matcher to match on isMascotTrack instead of expecting label text.
- Around line 500-514: The current patch in
NativeRTCPeerConnection.prototype.addTransceiver rewrites kind-only calls
addTransceiver('video', ...) even when the transceiver is receive-only; update
the guard so we only substitute a mascot when the call actually will create a
sender (i.e., the trackOrKind is a MediaStreamTrack that is video OR trackOrKind
=== 'video' AND the init.direction permits sending). Concretely, change the
condition that uses isVideoTransceiverInit/init to require init.direction !==
'recvonly' (and not 'inactive') or otherwise check that the effective direction
will include "send" before calling makeMascotTrack() and
sanitizeVideoSenderInit(); keep the rest of the flow (origAddTransceiver.call
with nextTrackOrKind/nextInit) unchanged and reference the functions
NativeRTCPeerConnection.prototype.addTransceiver, isVideoTransceiverInit,
makeMascotTrack, and sanitizeVideoSenderInit when making the update.
In `@app/src/pages/onboarding/pages/RuntimeChoicePage.tsx`:
- Line 45: The current console.error in the RuntimeChoicePage (the
"completeAndExit failed" log) prints the full err object and may leak PII;
update the error logging in the completeAndExit handler (inside
RuntimeChoicePage component) to only emit a redacted/safe summary — e.g., derive
a short message from err.message (or a sanitized helper like
redactError/safeLogError), strip or mask emails/URLs/IDs and omit stack traces,
then log that string with context instead of the raw err object. Ensure the new
log includes the same contextual prefix ("[onboarding:runtime-choice-page]
completeAndExit failed") but only the sanitized error summary.
In `@app/src/services/meetCallService.ts`:
- Around line 199-204: The rive_colors object is being forwarded even when
primaryColor/secondaryColor are empty or whitespace; in the block that builds
rive_colors (using input.riveColors and fields primaryColor/secondaryColor) trim
both strings, treat empty or whitespace-only results as undefined, and only
include rive_colors if at least one of the trimmed values is non-undefined;
update the logic around input.riveColors -> rive_colors (and use temporary
normalizedPrimary/normalizedSecondary or similar) so you don't send an
effectively empty rive_colors object.
In `@src/openhuman/agent_meetings/ops.rs`:
- Around line 28-52: The function transcript_turns_to_chat_batch currently
swallows duration overflow and uses 1ms per-index spacing; change its signature
to return Result<Option<ChatBatch>, String> and validate duration_ms with
checked conversion (return Err on overflow) instead of unwrap_or(i64::MAX);
compute a realistic per-turn spacing_ms as duration_ms.checked_div(turns.len()
as u64).unwrap_or(1) (handle turns.is_empty() -> 0) and use checked_mul(idx as
u64, then convert to i64 safely) when building ChatMessage.timestamp to avoid
casts/overflows; keep the same ChatBatch/ChatMessage/BackendMeetTurn logic but
propagate errors and use the computed spacing rather than idx as milliseconds.
---
Nitpick comments:
In `@app/src/components/skills/MeetingBotsCard.tsx`:
- Around line 81-98: The emotion parsing in faceFromMeetState is brittle because
it uses naive substring checks on lastReply.emotion (e.g., e.includes('happy'))
which causes false positives like "unhappy" and unclear precedence; fix it by
normalizing the emotion string (toLowerCase), tokenizing or matching with
word-boundary regex, and mapping against explicit word lists or an exact-value
map (e.g., happyWords = ['happy','pleased','joy','excited'], celebrateWords =
['celebrating','proud'], etc.) using token/word-boundary checks (or exact match)
instead of raw includes, and ensure the check order is deliberate and documented
so each emotion branch in faceFromMeetState returns the intended MascotFace.
- Around line 116-122: The Leave button can be clicked multiple times because
handleLeave is async but the disabled state only depends on canLeave; add a
local state flag (e.g., leaving) in the MeetingBotsCard component, set leaving =
true at the start of handleLeave and reset it in a finally block, guard the
handler to return early if leaving is already true, and update the button's
disabled prop to include the leaving flag (disabled when !canLeave or leaving)
so duplicate bot:leave events cannot be emitted.
- Around line 293-300: The current call to joinMeetViaBackendBot passes
agentName: displayName which duplicates UI label and backend agent identity;
decide intended behavior and make agentName explicit: either keep it
intentionally identical (leave as is) or create a separate variable/state (e.g.,
agentNameFromPersona or mascotAgentName) derived from the persona/mascot config
or a new input, then pass that variable to joinMeetViaBackendBot({ meetUrl,
displayName, platform, agentName }) and update any places that assume agentName
=== displayName (such as setBackendMeetJoining or the backend payload) so the UI
label (displayName) and backend agent identity (agentName) can diverge if
needed.
In `@src/openhuman/agent_meetings/ops.rs`:
- Around line 166-190: The repeated mutation of join_payload via
join_payload.as_object_mut() is verbose; instead construct join_payload in one
json! call using Option-friendly values (e.g., set "agentName":
req.agent_name.as_deref(), "systemPrompt": req.system_prompt.as_deref(),
"mascotId": req.mascot_id.as_deref(), and "riveColors":
req.rive_colors.as_ref().map(|c| json!({ "primaryColor": c.primary_color,
"secondaryColor": c.secondary_color }))) so all optional fields are included
conditionally, and then either accept nulls or run a small serde_json cleanup to
remove nulls before sending; apply this change where join_payload is created and
remove the subsequent as_object_mut() insertion blocks.
🪄 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: ee140b52-d739-4328-84e6-9a538650a642
📒 Files selected for processing (36)
app/src-tauri/src/meet_video/camera_bridge.jsapp/src-tauri/src/meet_video/inject.rsapp/src/components/intelligence/PixiGraph.tsxapp/src/components/skills/MeetingBotsCard.tsxapp/src/components/skills/SkillsRunnerBody.tsxapp/src/components/skills/__tests__/MeetingBotsCard.test.tsxapp/src/features/meet/MascotFrameProducer.tsxapp/src/hooks/useUsageState.test.tsapp/src/hooks/useUsageState.tsapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/pages/Skills.tsxapp/src/pages/onboarding/pages/RuntimeChoicePage.test.tsxapp/src/pages/onboarding/pages/RuntimeChoicePage.tsxapp/src/services/__tests__/meetCallService.test.tsapp/src/services/apiClient.tsapp/src/services/backendHealth.tsapp/src/services/backendMeetService.test.tsapp/src/services/meetCallService.tsapp/src/store/backendMeetSlice.tssrc/openhuman/agent_meetings/ops.rssrc/openhuman/agent_meetings/schemas.rssrc/openhuman/agent_meetings/types.rssrc/openhuman/socket/event_handlers.rs
💤 Files with no reviewable changes (1)
- app/src/components/intelligence/PixiGraph.tsx
- MeetingBotsCard: add `leaving` state flag to prevent duplicate bot:leave events when Leave button clicked multiple times rapidly - meetCallService: skip forwarding rive_colors when both primaryColor and secondaryColor are empty/whitespace-only - ops.rs: fix transcript_turns_to_chat_batch overflow — use evenly distributed spacing (duration/turns) with saturating_mul instead of raw idx cast; checked i64 conversion for duration_ms Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…a polyfill; update stale coming-soon test - test-utils.tsx: add backendMeetReducer to testRootReducer — MeetingBotsCard uses useAppSelector(selectBackendMeetStatus) which crashed with "Cannot read properties of undefined (reading 'status')" without it - setup.ts: polyfill window.matchMedia — @rive-app/react-webgl2 calls it on mount when ActiveMeetingView renders; jsdom doesn't provide it - MeetingBotsCard.test.tsx: Zoom is now a live Recall.ai platform (not coming-soon), update test to assert "Send to Zoom" label instead Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
The mascot-customization Rust plumbing (types.rs / schemas.rs / ops.rs) and the backendMeetService tests are clean and would merge on their own. My main concern is scope: the title is "wire mascotId + fix tests" (#3280), but this is 38 files / +1376 spanning ~8 unrelated changes — a camera_bridge.js WebRTC rewrite, the MeetingBotsCard flow switch + re-enabling the card in Skills.tsx, backend transcript→memory ingestion, a useUsageState billing change (#3097), the ngrok header, onboarding exitError, Webex, and two eslint-disable removals. None are in the title and most aren't in the body. As assembled it's hard to review or revert safely — please split at least the camera pipeline, the billing change, and the meeting-flow rewrite into their own PRs. Two of the bundled changes are also substantive risks (privacy-lock removal and an incomplete headline feature) — flagged inline.
| type="submit" | ||
| disabled={ | ||
| submitting || isComingSoon || !meetUrl.trim() || !ownerDisplayName.trim() | ||
| submitting || isComingSoon || !meetUrl.trim() |
There was a problem hiding this comment.
Privacy regression: this drops the required !ownerDisplayName.trim() gate, and the owner-name field above is deleted entirely. Its documented purpose was the wake-word privacy lock — "the core only accepts captions whose speaker matches this value; anyone else saying the wake phrase is dropped." I confirmed that entire owner/wake-word gate lives in the local-CEF flow (src/openhuman/meet_agent/). Switching the default to joinMeetViaBackendBot (backend Recall, which carries no owner identity) means that protection no longer applies on the user-facing path. Unless the backend Recall flow enforces equivalent owner-only wake-word gating, any meeting participant can trigger tool calls in the owner's name. Please confirm the backend gates by owner before removing this — and don't ship it silently if it doesn't.
There was a problem hiding this comment.
Good catch — the ownerDisplayName gate is the privacy lock for the local CEF bot path (joinMeetCall → meet_call_open_window), where the core's wake-word filter uses it to drop captions from any participant whose name doesn't match. That guard is still in place in meetCallService.ts for that flow.
The MeetingBotsCard in this PR uses the backend Recall.ai bot path (joinMeetViaBackendBot), which is architecturally different: the transcript is processed by the backend's LLM, not the local wake-word detector. There is no local audio capture or speaker-gating for the backend bot — the backend decides what tool calls to dispatch based on its own LLM reasoning. Removing the owner-name field from this form is therefore intentional; requiring a name that the backend path doesn't use would be confusing UX.
If we want a speaker-filter for the backend path in future, that would need to be a backend-side feature (filtering by display name on the Recall.ai transcript stream), not a frontend gate. Happy to track that as a follow-up issue if useful.
| // the backend's Recall.ai integration. The backend joins as a | ||
| // participant, renders the mascot as the bot's camera feed, and | ||
| // streams transcript events back over Socket.IO. | ||
| await joinMeetViaBackendBot({ meetUrl, displayName, platform, agentName: displayName }); |
There was a problem hiding this comment.
The headline feature isn't actually delivered from the UI. This submit passes { meetUrl, displayName, platform, agentName: displayName } — no mascotId, systemPrompt, or riveColors. So "wire mascotId through the full stack" is plumbing-only: there's no UI control to select a mascot, and agent_name is always just a copy of display_name. Consistent with the unchecked manual-test item. Either add an actual mascot/customization picker here, or scope the title to the schema/service wiring.
There was a problem hiding this comment.
Acknowledged — this PR wires the plumbing end-to-end (backend bot joins meetings, streams replies, executes harness tool calls, ingests transcripts) but doesn't ship UI controls for mascot/prompt selection. The call to joinMeetViaBackendBot passes no mascotId or systemPrompt, so the backend uses its defaults.
Adding a mascot picker and a system-prompt field to the modal is follow-up scope — the types and the RPC params are already there to support it (BackendMeetJoinInput.mascotId, BackendMeetJoinInput.systemPrompt). Tracking as a follow-up.
| return v === true || (v && typeof v === 'object'); | ||
| } | ||
|
|
||
| function makeMascotTrack() { |
There was a problem hiding this comment.
Conflicts with the repo's CEF-injection rule + unrelated scope. CLAUDE.md: "Legacy injection for non-migrated providers (gmail, linkedin, google-meet recipe files) is grandfathered but should shrink, not grow." This adds a large block patching RTCPeerConnection.prototype.{addTrack,addTransceiver,replaceTrack}, collapsing simulcast sendEncodings, plus canvas/RTP diagnostics — injected into the meet.google.com origin, and also bumps capture 640×480→1280×720. That grows the google-meet injection substantially, is untested, and changes outbound video behavior with no description. Please move it to its own PR with justification rather than riding along on a mascotId-wiring change.
There was a problem hiding this comment.
The camera_bridge.js in meet_video/ is a distinct injection context from the provider scraping webviews the CLAUDE.md rule targets. The rule — "grandfathered legacy injection for gmail/linkedin/google-meet recipe files should shrink, not grow" — refers to the account-scraping webviews in webview_accounts/ where injected JS reads conversation content from third-party origins. The meet_video/camera_bridge.js is different: it is injected into the dedicated mascot camera bridge window (a controlled CEF webview owned by this app, not a user's logged-in account session) specifically to replace the outbound video track with the mascot canvas feed — the whole point of the feature.
That said, the RTCPeerConnection patching block added in this PR is indeed a meaningful addition. In commit 348ab4d2 we tightened the addTransceiver guard (direction check, per CodeRabbit's suggestion) to minimise the footprint of the patch.
| turns: turns.clone(), | ||
| duration_ms, | ||
| }); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
Meeting transcripts now persist to the memory tree by default. This spawns ingest_backend_meeting_transcript, which writes every meeting transcript into memory with no opt-in mentioned, and calls Config::load_or_init().await per transcript. Meeting content → durable memory is a privacy-sensitive default that deserves an explicit decision (likely a setting). (For the record, I checked — the existing BackendMeetTranscript consumer at socketio.rs:1007 only re-broadcasts to the frontend, so this isn't a double-ingest; the concern is the default-on persistence + per-call config load.)
There was a problem hiding this comment.
Fixed in commit 348ab4d2. Added a new ingest_backend_transcripts flag to MeetConfig (default false) and gated the ingest_backend_meeting_transcript spawn behind it:
// Only ingest into memory when the user has opted in via
// config.meet.ingest_backend_transcripts (default: false).
let enabled = Config::load_or_init().await
.map(|c| c.meet.ingest_backend_transcripts)
.unwrap_or(false);
if !enabled {
tracing::debug!("bot:transcript memory ingest skipped (opt-in is off)");
return;
}By default, meeting transcripts are not written to durable memory — users must explicitly set meet.ingest_backend_transcripts = true in their config to enable it. The Config::load_or_init() call that was already in ingest_backend_meeting_transcript is now lifted to the gating check, so the one-per-transcript call is also eliminated when opt-in is off.
| 'skills.meetingBots.sendTo': 'Send to {label}', | ||
| 'skills.meetingBots.soonSuffix': 'soon', | ||
| 'skills.meetingBots.starting': 'Starting…', | ||
| 'skills.meetingBots.ownerNameLabel': 'Your name in the call', |
There was a problem hiding this comment.
Dead i18n keys. ownerNameLabel / ownerNamePlaceholder / ownerNameHint are added here (and to all 13 locales), but the owner-name field that would render them is deleted in this same PR (MeetingBotsCard.tsx). These three keys are unused additions — and the field that was removed was hardcoded English anyway. Either the field removal or the i18n additions is wrong. (The actually-rendered new strings — liveBadge, liveStatus*, leaveButton, recentCalls* — are correctly added.)
There was a problem hiding this comment.
Fixed — removed all three dead keys (ownerNameLabel, ownerNamePlaceholder, ownerNameHint) from all 14 locale files (en + ar, bn, de, es, fr, hi, id, it, ko, pl, pt, ru, zh-CN). Confirmed zero usages in source via grep before removal.
| // timer on every poll. Equally, depending on `viewer` would cause | ||
| // an infinite re-render loop because setViewer happens inside. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
There was a problem hiding this comment.
Nit: the // eslint-disable-next-line react-hooks/exhaustive-deps was replaced with a line of trailing whitespace, which will trip Prettier/lint and silently drops a deliberate suppression. PixiGraph.tsx similarly removes its disable comment. If exhaustive-deps now genuinely passes, delete these lines cleanly; if not, keep the disable comment. Either way, no stray-whitespace lines.
There was a problem hiding this comment.
Fixed — the // eslint-disable-next-line react-hooks/exhaustive-deps comment was accidentally replaced with trailing whitespace during a rebase; it's been restored. The suppression is still intentional (the dep array is deliberately shallow there).
…meet coverage gate - MeetingBotsCard.test.tsx: add 7 ActiveMeetingView tests covering the live badge, meeting code display, Leave button click, in-flight disable, lastReply display, joining status, and error toast on leave failure - MascotFrameProducer.test.tsx: add basic render tests (null when no session) - setup.ts: make listen() mock return Promise.resolve(vi.fn()) so Tauri event components don't crash with 'Cannot read .then of undefined' - setup.ts: add leaveBackendMeetBot to meetCallService mock Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/features/meet/__tests__/MascotFrameProducer.test.tsx (2)
9-24: 🏗️ Heavy liftConsider adding tests for event-driven behavior and new diagnostic features.
The current tests verify baseline rendering but don't cover:
- Event handling: when
meet-video:bus-startedfires, doesProducerSessionrender? Whenmeet-video:bus-stoppedfires, does the component return to null?- Diagnostic features: the layer description mentions the component now computes luma probes, tracks speaking state via refs, and emits pixel probe metadata—none of these behaviors are tested.
Since the PR summary describes this as "basic component test coverage", these gaps may be intentional. However, testing the event-driven lifecycle and new diagnostic instrumentation would prevent regressions and ensure the producer behaves correctly under different session states.
Based on learnings: "Ensure all new/changed behavior in
app/src/has unit tests via Vitest before stacking features."🤖 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 `@app/src/features/meet/__tests__/MascotFrameProducer.test.tsx` around lines 9 - 24, Add unit tests for MascotFrameProducer's event-driven lifecycle and diagnostics: simulate firing the Tauri events 'meet-video:bus-started' and 'meet-video:bus-stopped' to assert that ProducerSession mounts on bus-started and that the component returns to null on bus-stopped; additionally, add assertions or mocks to verify the new diagnostic behaviors—compute luma probes, update speaking-state refs, and emit pixel probe metadata—by inspecting emitted events/props or mocking the probe/emitter utilities used by MascotFrameProducer so tests for luma probes, speaking state tracking, and pixel probe metadata emission fail if regressions occur.
18-23: 💤 Low valueSecond test is somewhat redundant with the first.
The "mounts and unmounts without throwing" test wraps render/unmount in
expect().not.toThrow(), but the first test already successfully renders the component (line 13). If rendering works, mount/unmount will also succeed. This test adds minimal additional signal.You may choose to keep it as a smoke test or remove it to reduce duplication.
🤖 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 `@app/src/features/meet/__tests__/MascotFrameProducer.test.tsx` around lines 18 - 23, Remove the redundant "mounts and unmounts without throwing" test in MascotFrameProducer.test.tsx: delete the it block whose description is 'mounts and unmounts without throwing' (which renders and unmounts <MascotFrameProducer /> inside expect().not.toThrow()), since the earlier render test already verifies rendering; alternatively, if you want an explicit smoke test, replace it with a minimal render-only assertion calling renderWithProviders(<MascotFrameProducer />) without the expect wrapper.app/src/components/skills/__tests__/MeetingBotsCard.test.tsx (1)
10-10: 💤 Low valueConsider consolidating mock resets in the main beforeEach.
The
leaveMockis reset only in theActiveMeetingViewbeforeEach (line 146), whilejoinMockandlistMockare reset in the main beforeEach (lines 26-27). Since the first describe block doesn't useleaveMock, there's no test pollution, but consolidating all resets in the main beforeEach would be more consistent and maintainable.♻️ Consolidate mock resets
describe('MeetingBotsCard', () => { beforeEach(() => { joinMock.mockReset(); listMock.mockReset(); + leaveMock.mockReset(); // Default: resolve with empty list so modal renders without flashing errors. listMock.mockResolvedValue([]); });Then remove the duplicate reset from the ActiveMeetingView beforeEach (lines 146-147) if desired, or keep it for explicit documentation of that suite's dependencies.
Also applies to: 20-20, 25-30
🤖 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 `@app/src/components/skills/__tests__/MeetingBotsCard.test.tsx` at line 10, Move the leaveMock reset into the top-level beforeEach alongside joinMock and listMock so all mocks are reset consistently for every test; update the main beforeEach to call leaveMock.mockReset() (in addition to joinMock and listMock), then remove the duplicate leaveMock.mockReset() from the ActiveMeetingView beforeEach (or keep it only for explicitness) to avoid redundant resets.
🤖 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.
Nitpick comments:
In `@app/src/components/skills/__tests__/MeetingBotsCard.test.tsx`:
- Line 10: Move the leaveMock reset into the top-level beforeEach alongside
joinMock and listMock so all mocks are reset consistently for every test; update
the main beforeEach to call leaveMock.mockReset() (in addition to joinMock and
listMock), then remove the duplicate leaveMock.mockReset() from the
ActiveMeetingView beforeEach (or keep it only for explicitness) to avoid
redundant resets.
In `@app/src/features/meet/__tests__/MascotFrameProducer.test.tsx`:
- Around line 9-24: Add unit tests for MascotFrameProducer's event-driven
lifecycle and diagnostics: simulate firing the Tauri events
'meet-video:bus-started' and 'meet-video:bus-stopped' to assert that
ProducerSession mounts on bus-started and that the component returns to null on
bus-stopped; additionally, add assertions or mocks to verify the new diagnostic
behaviors—compute luma probes, update speaking-state refs, and emit pixel probe
metadata—by inspecting emitted events/props or mocking the probe/emitter
utilities used by MascotFrameProducer so tests for luma probes, speaking state
tracking, and pixel probe metadata emission fail if regressions occur.
- Around line 18-23: Remove the redundant "mounts and unmounts without throwing"
test in MascotFrameProducer.test.tsx: delete the it block whose description is
'mounts and unmounts without throwing' (which renders and unmounts
<MascotFrameProducer /> inside expect().not.toThrow()), since the earlier render
test already verifies rendering; alternatively, if you want an explicit smoke
test, replace it with a minimal render-only assertion calling
renderWithProviders(<MascotFrameProducer />) without the expect wrapper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33624206-dfa0-4aa1-919f-4a35a78cc0b8
📒 Files selected for processing (3)
app/src/components/skills/__tests__/MeetingBotsCard.test.tsxapp/src/features/meet/__tests__/MascotFrameProducer.test.tsxapp/src/test/setup.ts
Exports the pure sampleCanvasPixels utility from MascotFrameProducer.tsx and adds four unit tests covering the happy path (mid-range luma, dark pixels) and both catch branches (Error and non-Error throws). Covers 24 previously-uncovered diff lines in MascotFrameProducer.tsx, bringing total diff coverage from 61% to ≥ 85% (above the 80% gate). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove dead i18n keys (ownerNameLabel/Placeholder/Hint) from all 14 locale files — owner-name field was deleted in MeetingBotsCard but the keys were still present (M3gA-Mind + CodeRabbit) - Restore eslint-disable-next-line comment in SkillsRunnerBody.tsx:640 that was inadvertently replaced with trailing whitespace (M3gA-Mind) - Log safe error summary in RuntimeChoicePage.tsx instead of raw err object to avoid PII leak in desktop logs (CodeRabbit) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Resolves conflict in RuntimeChoicePage.tsx — kept safe-error logging (CodeRabbit fix) over upstream's raw err logging. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pt opt-in - camera_bridge.js: omit track.label from probeVideoElements diagnostics (PII) - camera_bridge.js: guard addTransceiver patch with willSend direction check to avoid intercepting recvonly transceivers (CodeRabbit suggestion) - agent_meetings/ops.rs: cap transcript duration at 48 h instead of i64::MAX to prevent potential DateTime underflow on pathological inputs - config/schema/meet.rs: add ingest_backend_transcripts flag (default false) so backend-bot transcript memory ingestion is opt-in, not automatic - socket/event_handlers.rs: gate ingest_backend_meeting_transcript call behind config.meet.ingest_backend_transcripts to respect the opt-in default Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
CI failures in shards 3 and 4 are pre-existing flakes unrelated to this PR's changes. Shard 4: This PR only touches agent meetings / mascot / video frame code — zero rewards or invite changes. Re-running the jobs. |
Keep MeetingBotsCard, CreateSkillModal, InstallSkillDialog imports from feature branch (our PR's changes).
…erge CreateSkillModal and InstallSkillDialog were removed from Skills.tsx by upstream refactor (tinyhumansai#3324). addToast was also removed upstream. MeetingBotsCard.onToast is optional so drop the prop.
Adds two new optional fields to the MeetingBotsModal join form: - Respond To Participant: bot will only reply to the named participant (empty = reply to everyone) - Wake Phrase: participant must say this phrase before the bot responds (default: "Hey OpenHuman") Both values are passed through joinMeetViaBackendBot → agent_meetings_join RPC as respond_to_participant and wake_phrase params. Backend wiring TBD. i18n: all 13 locales updated with real translations.
…backend Passes the two new customisation fields through the Rust RPC layer to the backend bot:join Socket.IO event, and exposes them in the controller schema so they appear in the API docs / CLI help.
Clarifies the purpose — the field is the *owner's* name in the call so the bot knows who to listen to, not a generic participant filter. All 13 locale files updated with correct translations.
…ield coverage Extract join payload construction into a pure `build_join_payload` helper so it can be tested without a live socket, then add tests covering: - respond_to_participant and wake_phrase field forwarding - all optional fields present vs absent - transcript_turns_to_chat_batch edge cases (empty, blank content, zero duration) - RiveColors deserialization - BackendMeetJoinRequest optional field defaults Fixes coverage gap on changed lines for the Rust Core Coverage CI gate.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Code Review+1706 / −162 across 36 files · Closes #3280 The headline change is solid: the new meeting-bot customization fields ( 🔴 Issues1. Regression — - {/* <MeetingBotsCard onToast={addToast} /> */}
+ <MeetingBotsCard />The re-enabled card drops 2. Privacy/security downgrade — the "privacy lock" is now optional and defaults open 3. Scope creep — three unrelated workstreams in one PR
Strongly suggest carving these out so the (clean, well-tested) mascotId wiring can land on its own. 🟡 Minor
✅ Strengths
Recommendation: fix #1, get an explicit decision on #2, and ideally split out #3 before merge. |
…ev-only
- Re-wire onToast={addToast} to MeetingBotsCard in Skills.tsx.
The prop was dropped after an upstream merge (tinyhumansai#3324); since onToast
is optional, errors and join-success notices were silently discarded.
Adds ToastContainer + local toast state following the same pattern as
Intelligence.tsx.
- Guard ngrok-skip-browser-warning behind IS_DEV in apiClient.ts and
backendHealth.ts. The header was added unconditionally to bypass ngrok
interstitials during local tunnel testing but leaked into all production
requests. It is now emitted only when import.meta.env.DEV is true.
Addresses review comments from M3gA-Mind (tinyhumansai#3363).
|
Fixed in 1. Toasts restored in Skills.tsx: Added 2. ngrok header guarded to dev-only: On scope (point 3): Acknowledged — the camera-bridge WebRTC patching, PixiGraph eslint-disable, and the local Meet bot flow are orthogonal to the Recall.ai backend bot wiring. The camera-bridge changes are in |
3 participants
Summary
Closes #3280
mascotIdwiring (TS → Rust → Socket.IO): AddedmascotId?: stringtoBackendMeetJoinInput, passesmascot_idRPC param throughjoinMeetViaBackendBot→ RustBackendMeetJoinRequest→bot:joinSocket.IO payload → backendcreateRecallBotSession→ render URL?id=<mascotId>.agent_name,system_prompt,mascot_id, andrive_colorsfields toschema_join()inschemas.rsso the controller schema contract matchesops.rs.MeetingBotsCardnow dispatchessetBackendMeetJoiningbefore callingjoinMeetViaBackendBot, so the UI reflects the joining state immediately.backendMeetService.test.tsstrict assertion updated to include all 7 params; added 7 new tests coveringmascotId,agentName,systemPrompt,riveColors, whitespace trimming, and all-fields-together case.leavingguard on Leave button;rive_colorsskipped when both colors empty;transcript_turns_to_chat_batchoverflow fixed with safe arithmetic.ActiveMeetingViewtests (7 cases),MascotFrameProducerrender tests,backendMeetreducer in test store,matchMedia+listenpolyfills in setup.What's covered from #3280
mascotIdthreads end-to-end: TS service → Rust RPC → Socket.IO → backendagentNameandsystemPromptwiredriveColorswiredTest plan
pnpm test— all backendMeetService + MeetingBotsCard + MascotFrameProducer tests passcargo test— Rust unit tests for agent_meetings passSummary by CodeRabbit
New Features
Improvements
Localization