fix(meet): mint unique correlationId per upcoming-meeting join#4349
Merged
graycyrus merged 2 commits intoJul 1, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change replaces the deterministic ChangesPer-join correlation id
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/meetings/__tests__/UpcomingTable.test.tsx`:
- Around line 205-209: The UpcomingTable test is clicking the Join button again
before the first `handleJoin` flow has fully finished. Update
`UpcomingTable.test.tsx` to wait for the first `joinMock` call and for the
button to become enabled again before triggering the second click, using the
existing `joinBtn`/`joinMock` assertions to anchor the fix and avoid
timing-dependent failures.
🪄 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: 9e70d5cc-59f7-4211-a64c-fefe462c7a26
📒 Files selected for processing (2)
app/src/components/meetings/UpcomingTable.tsxapp/src/components/meetings/__tests__/UpcomingTable.test.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/agent_meetings/bus.rs (1)
55-192: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftBusiness logic in bus.rs should delegate to ops.rs.
The new policy-gating, conditional summarization, and Ask-notification steps (lines 69-80, 98-170) are substantial business logic embedded directly in
EventHandler::handle. Per repo conventions,bus.rsshould host the subscriber wiring while domain logic lives inops.rs; consider extracting this into a singleops.rsfunction (e.g.handle_backend_meet_transcript(...)) thathandle()simply awaits and logs.As per coding guidelines: "
src/openhuman/**/bus.rs: Put domain event subscriber implementations (EventHandlerimpls) inbus.rs." and "src/openhuman/**/ops.rs: Put domain business logic andRpcOutcome<T>handlers inops.rswhen a domain has logic."🤖 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/bus.rs` around lines 55 - 192, The EventHandler::handle implementation in bus.rs is carrying too much domain behavior; move the policy gating, summary generation, Ask notification, and memory-ingest flow into a dedicated ops.rs function such as handle_backend_meet_transcript(...). Keep bus.rs limited to subscriber wiring and a simple await/log wrapper that delegates to the ops function, preserving the existing symbols create_meeting_thread_with_transcript, generate_meeting_summary_bounded, and ingest_backend_meeting_transcript behind the ops boundary.Source: Coding guidelines
🤖 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/openhuman/agent_meetings/bus.rs`:
- Around line 159-170: The Ask notification in `generate_summary_on_demand` is
currently published unconditionally when `SummaryAction::Ask` matches, even
though the stored call detail may not exist if `record_backend_call_detail`
failed. Update the flow around the `publish_core_notification` call in `bus.rs`
so the Ask card is only sent after detail persistence is confirmed, using the
existing `request_id`-based call detail record as the gate. If persistence
cannot be verified, skip publishing the Ask card and keep the `tracing::info!`
logging aligned with the new condition.
In `@src/openhuman/agent_meetings/ops.rs`:
- Around line 435-477: generate_summary_on_demand currently saves the generated
summary via recent_calls::record_backend_call_detail and then fails the whole
flow if create_meeting_thread_with_transcript returns an error. Change this path
so thread creation is best-effort: keep the persisted summary, log the failure
with tracing, and still return success using the already-generated state instead
of propagating the error. Use generate_summary_on_demand and
create_meeting_thread_with_transcript as the key places to update, matching the
tolerant behavior used in bus.rs.
---
Outside diff comments:
In `@src/openhuman/agent_meetings/bus.rs`:
- Around line 55-192: The EventHandler::handle implementation in bus.rs is
carrying too much domain behavior; move the policy gating, summary generation,
Ask notification, and memory-ingest flow into a dedicated ops.rs function such
as handle_backend_meet_transcript(...). Keep bus.rs limited to subscriber wiring
and a simple await/log wrapper that delegates to the ops function, preserving
the existing symbols create_meeting_thread_with_transcript,
generate_meeting_summary_bounded, and ingest_backend_meeting_transcript behind
the ops boundary.
🪄 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: a2506591-a9b9-49c7-81ea-34386d93de34
📒 Files selected for processing (5)
app/src/components/meetings/__tests__/UpcomingTable.test.tsxsrc/openhuman/agent_meetings/bus.rssrc/openhuman/agent_meetings/ops.rssrc/openhuman/agent_meetings/schemas.rssrc/openhuman/agent_meetings/summary.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/components/meetings/tests/UpcomingTable.test.tsx
The Upcoming table's Join button passed the deterministic `calendar_event_id` (<event_id>_<start>) as the join correlationId. That correlationId is echoed back at call-end and copied verbatim into `MeetCallRecord.request_id` — which is both the recent-calls list key/React key and the per-call detail filename. Re-joining the same calendar meeting therefore produced two call records sharing one request_id: the history list double-highlighted both rows and the second call's transcript overwrote / was unreachable. Mint a fresh `crypto.randomUUID()` per join (mirroring MeetComposer and the background auto-join in calendar.rs). `calendar_event_id` stays the dedup/per-event-policy key and button-disable state only — never the call identity. Closes tinyhumansai#4338
Addresses CodeRabbit: the regression test clicked Join a second time before handleJoin's finally cleared joiningId, so the (disabled) button could swallow the click and make the two-joins assertion timing-dependent. Wait for the button to re-enable first.
graycyrus
force-pushed
the
fix/meet-autojoin-duplicate-request-id
branch
from
f15b142 to
9a286bb
Compare
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
1 participant
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.
Summary
The Upcoming-meetings Join button passed the deterministic
calendar_event_id(<event_id>_<start>, or<meet_url>@<start_ms>for id-less events) as the joincorrelationId. ThatcorrelationIdis echoed back at call-end (BackendMeetTranscript) and copied verbatim intoMeetCallRecord.request_idbyrecent_calls::build_record— which is simultaneously:call_details/<id>.json) thatgetMeetCallDetail(request_id)reads.So re-joining the same calendar meeting produced two call records with an identical
request_id: the history list highlighted both rows together, and the second call's transcript overwrote / was unreachable. Surfaced by the Meetings redesign (#4275 / #4308), but the collision was pre-existing.MeetComposerand the background auto-join (calendar.rs:404) already mint a freshcrypto.randomUUID()per join —UpcomingTablewas the lone offender.Fix
UpcomingTable.handleJoinnow mints a freshcrypto.randomUUID()per join and passes that ascorrelationId.calendar_event_idstays the dedup / per-event-policy key (handleJoinPolicyChange→setEventPolicy) and the local button-disable state (setJoiningId) only — never the call identity.correlationIdas the call identity; the bug was a frontend misuse.Test plan
correlationIdis a fresh id, notcalendar_event_id.UpcomingTable.test.tsx— 23/23 pass; fullmeetings/suite — 173/173 pass.Closes #4338
Summary by CodeRabbit
Bug Fixes
Tests