Repository navigation
Production audit – Batch 7 (Message Idempotency, Call State Consistency & Input Reliability) - #8
Merged
Merged
Conversation
…d nothing, a second finger that hung up on the first, and fixtures that failed silently Batch 7. The first item closes the trade the previous batch had to accept, and the next two correct defects that batch introduced. A RETRY AFTER AN UNCERTAIN SEND STORED THE MESSAGE TWICE. `send_message` commits before the sender learns anything, so a lost ack, a request timeout or a 502 in front of the app all leave the client holding a message it cannot classify. Batch 6 gave those a deadline and a retry control, which was the right call — the alternative is deleting a message that may never have been sent — but the retry minted a fresh temp id, so precisely the case where the message HAD been stored is the case that stored it a second time. That was named as a known trade in the previous commit; this removes it. `temp_id` was already on the wire for both transports and was only echoed back. It is now persisted as `client_msg_id` and enforced by a partial unique index on (conversation_id, sender_id, client_msg_id). Partial because system messages and every row predating the migration carry no client id, and a plain unique constraint over a nullable column would be satisfied by those rows under Postgres NULL semantics rather than stating the intent. Scoped to the sender and conversation rather than the id alone: client ids are uuid4 in practice but they are client-supplied, so a key unique only per sender cannot be used by one account to block a send in another. The service checks for an existing row and replays it, and because that check is check-then-act, the `IntegrityError` path catches the two concurrent copies of one send that can both pass it. The idempotency lookup sits AFTER the access gate, so a repeat send is never a way to read a message back out of a conversation the caller has been removed from. No backfill: existing rows keep a NULL key and fall outside the index, which is correct — they predate the guarantee. The client half is two changes. `handleRetry` sends the ORIGINAL key back instead of minting a new one, so the server answers with the row it already has. And every serialized message now carries `client_msg_id`, so the refetch carry-over can drop a failed bubble whose message is present in the fetched history — otherwise the user is shown their delivered message twice, with an invitation to send it again. A MUTE TAP INSIDE THE SAME FRAME AS ANOTHER DID NOTHING. `setMicEnabled` wrote the store only once the SDK settled, and both callers computed their target from the flag — so two taps in one animation frame read the same stale value, asked for the same state, and the second silently did nothing. It is now optimistic and serialised through its own queue, exactly as `setCameraEnabled` already was and for the same reasons, and rolled back if the mic refuses. Both callers now read the target from the store rather than from their render's prop. Note the asymmetry the camera does not have: `previous` is a MUTE flag while the return is an ENABLED flag, so the failure path returns its negation. A SECOND FINGER HUNG UP ON THE FIRST. The previous batch stopped a second pointer STARTING a recording, with a boolean. But its release still ran the owner's `onPointerUp` — `held` is true regardless of which finger lifted — so a non-owner lifting off sent or discarded the recording the first finger was still holding. The gesture now records the owning pointer's ID and scopes move, up and cancel to it. A stale owner cannot lock the button out either: the composer swaps the mic button for the recorder bar as soon as recording starts, so the release often reaches an element that no longer exists and `reset` never runs. `hasPointerCapture` separates a finger genuinely still down from a leftover, and missing capture support degrades to the old free-for-all rather than to a control that cannot be pressed. E2E FIXTURE WRITES FAILED SILENTLY. Every seed write was unchecked, so a 4xx returned an error body instead of a record, `_id` came back undefined, and the next write was built on it — an org with no id, a department under `org_id: undefined`, users under both, and not one of them throwing. The run then died much later on "conversation-search never appeared", which reads as a broken frontend rather than a stack that never seeded. Now each write asserts its status AND that a record came back, and names the endpoint and the API's own words. This paid for itself immediately: a rate-limited run during verification reported `429` at the seed instead of a mystery timeout twelve steps downstream. VERIFIED 184 backend tests, ruff clean, alembic upgrade applied and at head. Frontend: lint 0 problems, build green, check_contracts.py no drift, and the full Playwright suite — 56 passed, 0 failed, 0 skipped. The two new frontend specs produce the uncertain send honestly rather than mocking it: `routeWebSocket` forwards the chat frame to the real server, so the message really is stored, and swallows only the `message_ack` coming back. Both fail against the previous client — the retry one with the server holding two copies of the message, the carry-over one with a delivered message still offering a retry that would duplicate it. Worth recording for whoever runs this stack locally: the API must be restarted after backend changes (it runs without --reload) and started with RXHIVE_RATE_LIMIT_LOGIN raised, as CI does. A stale process is invisible — the suite goes green against seventeen-hour-old code — and the default 10 logins/minute cannot seat a full suite run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe backend adds durable, sender-scoped client message IDs with replay and concurrent-send handling. The frontend preserves IDs during retries, reconciles failed messages, and tests message recovery. Call controls serialize microphone changes and track pointer ownership. ChangesMessage idempotency
Call interaction state
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatPanel
participant send_message
participant messages
Client->>ChatPanel: send message with temp_id
ChatPanel->>send_message: submit client_msg_id
send_message->>messages: lookup or create message
messages-->>send_message: existing or unique message
send_message-->>ChatPanel: serialized message
ChatPanel-->>Client: reconcile local bubble
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@backend/alembic/versions/c5d81e37a204_message_client_idempotency_key.py`:
- Around line 41-52: Update the upgrade() index creation to use CREATE UNIQUE
INDEX CONCURRENTLY IF NOT EXISTS and execute it with the migration bind in
autocommit mode, since concurrent index creation cannot run inside the
transaction. Preserve the existing nullable client_msg_id column addition and
downgrade() behavior.
In `@backend/app/services/enrich.py`:
- Around line 401-406: The serialize_message response currently exposes
client_msg_id for every participant; restrict it to the requesting user's own
messages. Update serialize_message to return msg.client_msg_id only when
for_user is set and matches msg.sender_id, otherwise return None, and verify all
history/list call sites provide for_user while leaving the send path's
doc["temp_id"] behavior unchanged.
In `@backend/app/services/messaging.py`:
- Around line 155-175: Update the replay handler around the existing
message-loading and serialization flow to ensure recipients receive new_message
when the original request committed before fan-out. Prefer re-publishing the
message on replay, preserving client-side _id deduplication; if the
implementation uses delivery tracking instead, persist a post-fan-out marker and
replay only when it is absent. Revise the surrounding docstring to reflect the
actual replay behavior.
In `@frontend/src/hooks/useHoldToTalk.js`:
- Around line 84-96: Update the onStart flow in the hold-to-talk hook to capture
the initiating e.pointerId before awaiting onStart?.(). After the await, verify
it still matches ownerRef.current and return immediately if ownership changed,
before setting armed.current or invoking reset() based on res.
In `@frontend/src/services/livekitClient.js`:
- Around line 811-841: The queued operation in setMicEnabled must capture the
current room and call identity when created, then verify both still match before
invoking LiveKit or updating useCallStore; discard stale operations without
rolling back state for the newer call. Update ActiveCallView’s signaling logic
to ensure it does not emit the captured old callId after the active call
changes.
In `@frontend/tests/message-idempotency.spec.js`:
- Around line 120-122: Update the sent-status assertion in both tests to locate
the message row containing the retried message body before selecting its
message-status element, replacing the unscoped .first() locator. Keep the
existing visibility timeout and retry-count assertion unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dbc33229-5d85-4649-86d4-bf37dc55d1ea
📒 Files selected for processing (12)
backend/alembic/versions/c5d81e37a204_message_client_idempotency_key.pybackend/app/db/models.pybackend/app/services/enrich.pybackend/app/services/messaging.pybackend/tests/test_message_idempotency.pyfrontend/src/components/calls/ActiveCallView.jsxfrontend/src/components/calls/MinimizedCallBanner.jsxfrontend/src/components/chat/ChatPanel.jsxfrontend/src/hooks/useHoldToTalk.jsfrontend/src/services/livekitClient.jsfrontend/tests/helpers.jsfrontend/tests/message-idempotency.spec.js
Comment on lines
+41
to
+52
| def upgrade() -> None: | ||
| op.add_column("messages", sa.Column("client_msg_id", sa.String(length=64), nullable=True)) | ||
| op.execute( | ||
| """CREATE UNIQUE INDEX uq_messages_client_msg_id | ||
| ON messages (conversation_id, sender_id, client_msg_id) | ||
| WHERE client_msg_id IS NOT NULL""" | ||
| ) | ||
|
|
||
|
|
||
| def downgrade() -> None: | ||
| op.execute("DROP INDEX IF EXISTS uq_messages_client_msg_id") | ||
| op.drop_column("messages", "client_msg_id") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Build the unique index concurrently — this locks messages for the whole build.
CREATE UNIQUE INDEX without CONCURRENTLY takes a SHARE lock on messages. That blocks every insert and update to the table until the index is built. messages is the highest-volume table in this schema, so the deploy stalls all sends.
Use CONCURRENTLY. It cannot run inside a transaction, so switch the bind to autocommit. Add IF NOT EXISTS because a failed concurrent build leaves an invalid index behind.
The nullable column add and the downgrade() are correct and reversible.
🔒️ Proposed fix for online index creation
def upgrade() -> None:
op.add_column("messages", sa.Column("client_msg_id", sa.String(length=64), nullable=True))
- op.execute(
- """CREATE UNIQUE INDEX uq_messages_client_msg_id
- ON messages (conversation_id, sender_id, client_msg_id)
- WHERE client_msg_id IS NOT NULL"""
- )
+ # CONCURRENTLY cannot run inside a transaction block.
+ with op.get_context().autocommit_block():
+ op.execute(
+ """CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS uq_messages_client_msg_id
+ ON messages (conversation_id, sender_id, client_msg_id)
+ WHERE client_msg_id IS NOT NULL"""
+ )
def downgrade() -> None:
- op.execute("DROP INDEX IF EXISTS uq_messages_client_msg_id")
+ with op.get_context().autocommit_block():
+ op.execute("DROP INDEX CONCURRENTLY IF EXISTS uq_messages_client_msg_id")
op.drop_column("messages", "client_msg_id")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def upgrade() -> None: | |
| op.add_column("messages", sa.Column("client_msg_id", sa.String(length=64), nullable=True)) | |
| op.execute( | |
| """CREATE UNIQUE INDEX uq_messages_client_msg_id | |
| ON messages (conversation_id, sender_id, client_msg_id) | |
| WHERE client_msg_id IS NOT NULL""" | |
| ) | |
| def downgrade() -> None: | |
| op.execute("DROP INDEX IF EXISTS uq_messages_client_msg_id") | |
| op.drop_column("messages", "client_msg_id") | |
| def upgrade() -> None: | |
| op.add_column("messages", sa.Column("client_msg_id", sa.String(length=64), nullable=True)) | |
| # CONCURRENTLY cannot run inside a transaction block. | |
| with op.get_context().autocommit_block(): | |
| op.execute( | |
| """CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS uq_messages_client_msg_id | |
| ON messages (conversation_id, sender_id, client_msg_id) | |
| WHERE client_msg_id IS NOT NULL""" | |
| ) | |
| def downgrade() -> None: | |
| with op.get_context().autocommit_block(): | |
| op.execute("DROP INDEX CONCURRENTLY IF EXISTS uq_messages_client_msg_id") | |
| op.drop_column("messages", "client_msg_id") |
🤖 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 `@backend/alembic/versions/c5d81e37a204_message_client_idempotency_key.py`
around lines 41 - 52, Update the upgrade() index creation to use CREATE UNIQUE
INDEX CONCURRENTLY IF NOT EXISTS and execute it with the migration bind in
autocommit mode, since concurrent index creation cannot run inside the
transaction. Preserve the existing nullable client_msg_id column addition and
downgrade() behavior.
Source: Path instructions
Comment on lines
+401
to
+406
| # The sender's own id for this message, on EVERY serialization including | ||
| # history. Without it a client refetching a conversation cannot tell which | ||
| # fetched rows its own unresolved bubbles became, so a send whose outcome | ||
| # was never learned showed twice: once as the stored message and once as | ||
| # the local bubble still waiting on it. | ||
| "client_msg_id": msg.client_msg_id, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Consider limiting client_msg_id to the requesting user's own messages.
serialize_message now returns client_msg_id on every message to every participant. Only the sender needs it: reconciliation matches a fetched row against that client's own unresolved bubbles. Recipients gain a stable, sender-supplied identifier for someone else's device state, and the value is client-controlled text that is not passed through sanitize_text the way content is.
for_user is already a parameter here. Gate the field on sender ownership:
"client_msg_id": msg.client_msg_id if for_user and msg.sender_id == for_user else None,Confirm first that every history and list endpoint passes for_user; the send path relies on doc["temp_id"] instead, so it is unaffected.
#!/bin/bash
# Which serialize_message call sites pass for_user, and which do not.
rg -nP -C6 'serialize_message\s*\(' --type=py backend/🤖 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 `@backend/app/services/enrich.py` around lines 401 - 406, The serialize_message
response currently exposes client_msg_id for every participant; restrict it to
the requesting user's own messages. Update serialize_message to return
msg.client_msg_id only when for_user is set and matches msg.sender_id, otherwise
return None, and verify all history/list call sites provide for_user while
leaving the send path's doc["temp_id"] behavior unchanged.
Comment on lines
+84
to
+96
| // | ||
| // A stale owner must never lock the button out, and it can go stale easily: the | ||
| // composer swaps this button for the recorder bar the moment recording starts, | ||
| // so the release often reaches a button that no longer exists and `reset` never | ||
| // runs. `hasPointerCapture` tells the two cases apart — a finger genuinely still | ||
| // down is still captured, while an owner left over from a previous gesture is | ||
| // not. Missing capture support reads as "not held", which gives back the old | ||
| // free-for-all rather than a control that cannot be pressed at all. | ||
| const owner = ownerRef.current; | ||
| if (owner !== null && owner !== e.pointerId | ||
| && e.currentTarget.hasPointerCapture?.(owner)) return; | ||
|
|
||
| ownerRef.current = e.pointerId; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guard the onStart continuation with the owner pointer ID.
onStart can resolve after the owner releases and another pointer starts a new gesture. The old continuation can then set armed.current = true, or call reset() on its failure path, for the newer gesture. Capture e.pointerId before await onStart?.() and return unless it still equals ownerRef.current before processing res.
Proposed fix
const onPointerDown = useCallback(async (e) => {
if (!enabled || !coarse || e.button !== 0) return;
e.preventDefault();
+ const pointerId = e.pointerId;
// ...
const owner = ownerRef.current;
- if (owner !== null && owner !== e.pointerId
+ if (owner !== null && owner !== pointerId
&& e.currentTarget.hasPointerCapture?.(owner)) return;
- ownerRef.current = e.pointerId;
- e.currentTarget.setPointerCapture?.(e.pointerId);
+ ownerRef.current = pointerId;
+ e.currentTarget.setPointerCapture?.(pointerId);
// ...
const res = await onStart?.();
+ if (ownerRef.current !== pointerId) return;
if (res && res.ok === false) { reset(); return; }
armed.current = true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // | |
| // A stale owner must never lock the button out, and it can go stale easily: the | |
| // composer swaps this button for the recorder bar the moment recording starts, | |
| // so the release often reaches a button that no longer exists and `reset` never | |
| // runs. `hasPointerCapture` tells the two cases apart — a finger genuinely still | |
| // down is still captured, while an owner left over from a previous gesture is | |
| // not. Missing capture support reads as "not held", which gives back the old | |
| // free-for-all rather than a control that cannot be pressed at all. | |
| const owner = ownerRef.current; | |
| if (owner !== null && owner !== e.pointerId | |
| && e.currentTarget.hasPointerCapture?.(owner)) return; | |
| ownerRef.current = e.pointerId; | |
| const pointerId = e.pointerId; | |
| // | |
| // A stale owner must never lock the button out, and it can go stale easily: the | |
| // composer swaps this button for the recorder bar the moment recording starts, | |
| // so the release often reaches a button that no longer exists and `reset` never | |
| // runs. `hasPointerCapture` tells the two cases apart — a finger genuinely still | |
| // down is still captured, while an owner left over from a previous gesture is | |
| // not. Missing capture support reads as "not held", which gives back the old | |
| // free-for-all rather than a control that cannot be pressed at all. | |
| const owner = ownerRef.current; | |
| if (owner !== null && owner !== pointerId | |
| && e.currentTarget.hasPointerCapture?.(owner)) return; | |
| ownerRef.current = pointerId; | |
| e.currentTarget.setPointerCapture?.(pointerId); | |
| const res = await onStart?.(); | |
| if (ownerRef.current !== pointerId) return; | |
| if (res && res.ok === false) { reset(); return; } | |
| armed.current = true; |
🤖 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 `@frontend/src/hooks/useHoldToTalk.js` around lines 84 - 96, Update the onStart
flow in the hold-to-talk hook to capture the initiating e.pointerId before
awaiting onStart?.(). After the await, verify it still matches ownerRef.current
and return immediately if ownership changed, before setting armed.current or
invoking reset() based on res.
Comment on lines
+811
to
+841
| const run = async () => { | ||
| // The mute button is live throughout "Connecting", when there is no room to | ||
| // mute. Reporting the state we are still in — rather than the one that was | ||
| // asked for — is the same rule as the failure path below: `joinCall` brings | ||
| // the microphone up LIVE moments later, so answering `enabled` here would | ||
| // put exactly the lie this method exists to stop back on the wire. | ||
| if (!this.room) { | ||
| useCallStore.setState({ isMuted: previous }); | ||
| return !previous; | ||
| } | ||
| try { | ||
| await this.room.localParticipant.setMicrophoneEnabled(enabled); | ||
| useCallStore.setState({ isMuted: !enabled }); | ||
| return enabled; | ||
| } catch (err) { | ||
| console.error('[LiveKit] mic toggle failed:', err); | ||
| // Put the button back where it was rather than leaving it claiming a mic | ||
| // state the hardware never reached. | ||
| useCallStore.setState({ isMuted: previous }); | ||
| notifyMicToggleFailed(enabled, mediaErrorReason(err)); | ||
| // `previous` is a MUTE flag and the return is an ENABLED flag — the two are | ||
| // opposite, unlike the camera's, where both sides of this are `isCameraOn`. | ||
| return !previous; | ||
| } | ||
| }; | ||
|
|
||
| // Chained, so N taps produce N operations in tap order and the LAST one owns the | ||
| // final state. Errors are swallowed into the chain so one failure cannot poison | ||
| // every later toggle. | ||
| this._micQueue = this._micQueue.then(run, run); | ||
| return this._micQueue; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope queued microphone operations to the call that created them.
run dereferences this.room only when the queued task starts. If a user queues two toggles in call A, leaves, and joins call B before the second task starts, that task can change call B's microphone state. An in-flight task from call A can also overwrite call B's isMuted state after it resolves.
Capture the room and call identity when setMicEnabled is called. If either no longer matches before an SDK call or store update, discard the operation without rolling back state for the new call. Also prevent ActiveCallView from signaling the old callId after the call changes.
🤖 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 `@frontend/src/services/livekitClient.js` around lines 811 - 841, The queued
operation in setMicEnabled must capture the current room and call identity when
created, then verify both still match before invoking LiveKit or updating
useCallStore; discard stale operations without rolling back state for the newer
call. Update ActiveCallView’s signaling logic to ensure it does not emit the
captured old callId after the active call changes.
Comment on lines
+120
to
+122
| await expect(page.locator('[data-testid="message-status"][data-status="sent"]').first()) | ||
| .toBeVisible({ timeout: 15000 }); | ||
| await expect(page.getByTestId('message-retry')).toHaveCount(0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope the sent assertion to the retried message.
.first() selects the first message-status element in the thread, not the row for body. Both tests in this file reuse the same seeded conversation, so an earlier own-message can already carry data-status="sent". The assertion then passes without observing the retry.
Anchor the locator on the message row that contains body.
💚 Proposed fix to scope the locator
socket.swallowAcks = false;
await page.getByTestId('message-retry').click();
- await expect(page.locator('[data-testid="message-status"][data-status="sent"]').first())
- .toBeVisible({ timeout: 15000 });
+ const retriedRow = page.locator('[data-testid="message-bubble"]').filter({ hasText: body });
+ await expect(retriedRow.locator('[data-testid="message-status"][data-status="sent"]'))
+ .toBeVisible({ timeout: 15000 });
await expect(page.getByTestId('message-retry')).toHaveCount(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await expect(page.locator('[data-testid="message-status"][data-status="sent"]').first()) | |
| .toBeVisible({ timeout: 15000 }); | |
| await expect(page.getByTestId('message-retry')).toHaveCount(0); | |
| const retriedRow = page.locator('[data-testid="message-bubble"]').filter({ hasText: body }); | |
| await expect(retriedRow.locator('[data-testid="message-status"][data-status="sent"]')) | |
| .toBeVisible({ timeout: 15000 }); | |
| await expect(page.getByTestId('message-retry')).toHaveCount(0); |
🤖 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 `@frontend/tests/message-idempotency.spec.js` around lines 120 - 122, Update
the sent-status assertion in both tests to locate the message row containing the
retried message body before selecting its message-status element, replacing the
unscoped .first() locator. Keep the existing visibility timeout and retry-count
assertion unchanged.
`ruff check` passed and `ruff format --check` did not, which is the half of the backend lint step I never ran locally — so the commit that added these claimed "ruff clean" on the strength of pytest alone. Both gates now pass. Pure formatting: ruff joins three call sites back onto single lines that fit the project's 110-column limit. No behaviour, no semantics, 184 tests still passing.
arxdeployments
force-pushed
the
production-audit-batch-7
branch
from
August 6, 2026 09:26
ff8d663 to
b2f340b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 `@backend/tests/test_message_idempotency.py`:
- Around line 128-139: Extend test_one_users_key_does_not_block_another with an
authenticated user from the other organization sending to the existing conv, and
assert the request returns 403. Also verify the conversation history remains
unchanged so the cross-tenant request neither creates nor replays a message,
using the existing login, _client_for, _send, and _history helpers.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f7bd0de8-d95f-4531-9406-4866f9f734be
📒 Files selected for processing (12)
backend/alembic/versions/c5d81e37a204_message_client_idempotency_key.pybackend/app/db/models.pybackend/app/services/enrich.pybackend/app/services/messaging.pybackend/tests/test_message_idempotency.pyfrontend/src/components/calls/ActiveCallView.jsxfrontend/src/components/calls/MinimizedCallBanner.jsxfrontend/src/components/chat/ChatPanel.jsxfrontend/src/hooks/useHoldToTalk.jsfrontend/src/services/livekitClient.jsfrontend/tests/helpers.jsfrontend/tests/message-idempotency.spec.js
🚧 Files skipped from review as they are similar to previous changes (10)
- frontend/src/components/calls/ActiveCallView.jsx
- backend/app/services/enrich.py
- backend/app/services/messaging.py
- frontend/tests/helpers.js
- frontend/src/hooks/useHoldToTalk.js
- frontend/src/services/livekitClient.js
- frontend/tests/message-idempotency.spec.js
- backend/app/db/models.py
- frontend/src/components/calls/MinimizedCallBanner.jsx
- backend/alembic/versions/c5d81e37a204_message_client_idempotency_key.py
Comment on lines
+128
to
+139
| async def test_one_users_key_does_not_block_another(client, two_orgs_with_users): | ||
| """The key is scoped per sender, so it cannot be used to suppress a peer.""" | ||
| users = two_orgs_with_users | ||
| await login(client, "alice@a.com") | ||
| conv = await _direct(client, users["bob"].id) | ||
| mine = await _send(client, conv, "from alice", temp_id="shared-key") | ||
|
|
||
| async with _client_for("bob@a.com") as bob: | ||
| theirs = await _send(bob, conv, "from bob", temp_id="shared-key") | ||
|
|
||
| assert theirs["_id"] != mine["_id"], "bob's send must not resolve to alice's message" | ||
| assert len(await _history(client, conv)) == 2 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add a cross-organization idempotency test.
This test uses alice@a.com and bob@a.com. It verifies sender scoping, but not the organization boundary.
Add a request from a user in the other organization against conv. Assert 403. Assert that the request does not create or replay a message. This prevents a known temp_id or conversation ID from becoming a cross-tenant access path.
As per coding guidelines, “Maintain tests proving cross-tenant isolation ... including ... correct 403 responses.”
🤖 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 `@backend/tests/test_message_idempotency.py` around lines 128 - 139, Extend
test_one_users_key_does_not_block_another with an authenticated user from the
other organization sending to the existing conv, and assert the request returns
403. Also verify the conversation history remains unchanged so the cross-tenant
request neither creates nor replays a message, using the existing login,
_client_for, _send, and _history helpers.
Source: Coding guidelines
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
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.
…d nothing, a second finger that hung up on the first, and fixtures that failed silently
Batch 7. The first item closes the trade the previous batch had to accept, and the next two correct defects that batch introduced.
A RETRY AFTER AN UNCERTAIN SEND STORED THE MESSAGE TWICE.
send_messagecommits before the sender learns anything, so a lost ack, a request timeout or a 502 in front of the app all leave the client holding a message it cannot classify. Batch 6 gave those a deadline and a retry control, which was the right call — the alternative is deleting a message that may never have been sent — but the retry minted a fresh temp id, so precisely the case where the message HAD been stored is the case that stored it a second time. That was named as a known trade in the previous commit; this removes it.temp_idwas already on the wire for both transports and was only echoed back. It is now persisted asclient_msg_idand enforced by a partial unique index on (conversation_id, sender_id, client_msg_id). Partial because system messages and every row predating the migration carry no client id, and a plain unique constraint over a nullable column would be satisfied by those rows under Postgres NULL semantics rather than stating the intent. Scoped to the sender and conversation rather than the id alone: client ids are uuid4 in practice but they are client-supplied, so a key unique only per sender cannot be used by one account to block a send in another. The service checks for an existing row and replays it, and because that check is check-then-act, theIntegrityErrorpath catches the two concurrent copies of one send that can both pass it. The idempotency lookup sits AFTER the access gate, so a repeat send is never a way to read a message back out of a conversation the caller has been removed from. No backfill: existing rows keep a NULL key and fall outside the index, which is correct — they predate the guarantee.The client half is two changes.
handleRetrysends the ORIGINAL key back instead of minting a new one, so the server answers with the row it already has. And every serialized message now carriesclient_msg_id, so the refetch carry-over can drop a failed bubble whose message is present in the fetched history — otherwise the user is shown their delivered message twice, with an invitation to send it again.A MUTE TAP INSIDE THE SAME FRAME AS ANOTHER DID NOTHING.
setMicEnabledwrote the store only once the SDK settled, and both callers computed their target from the flag — so two taps in one animation frame read the same stale value, asked for the same state, and the second silently did nothing. It is now optimistic and serialised through its own queue, exactly assetCameraEnabledalready was and for the same reasons, and rolled back if the mic refuses. Both callers now read the target from the store rather than from their render's prop. Note the asymmetry the camera does not have:previousis a MUTE flag while the return is an ENABLED flag, so the failure path returns its negation.A SECOND FINGER HUNG UP ON THE FIRST.
The previous batch stopped a second pointer STARTING a recording, with a boolean. But its release still ran the owner's
onPointerUp—heldis true regardless of which finger lifted — so a non-owner lifting off sent or discarded the recording the first finger was still holding. The gesture now records the owning pointer's ID and scopes move, up and cancel to it. A stale owner cannot lock the button out either: the composer swaps the mic button for the recorder bar as soon as recording starts, so the release often reaches an element that no longer exists andresetnever runs.hasPointerCaptureseparates a finger genuinely still down from a leftover, and missing capture support degrades to the old free-for-all rather than to a control that cannot be pressed.E2E FIXTURE WRITES FAILED SILENTLY.
Every seed write was unchecked, so a 4xx returned an error body instead of a record,
_idcame back undefined, and the next write was built on it — an org with no id, a department underorg_id: undefined, users under both, and not one of them throwing. The run then died much later on "conversation-search never appeared", which reads as a broken frontend rather than a stack that never seeded. Now each write asserts its status AND that a record came back, and names the endpoint and the API's own words. This paid for itself immediately: a rate-limited run during verification reported429at the seed instead of a mystery timeout twelve steps downstream.VERIFIED
184 backend tests, ruff clean, alembic upgrade applied and at head. Frontend: lint 0 problems, build green, check_contracts.py no drift, and the full Playwright suite — 56 passed, 0 failed, 0 skipped.
The two new frontend specs produce the uncertain send honestly rather than mocking it:
routeWebSocketforwards the chat frame to the real server, so the message really is stored, and swallows only themessage_ackcoming back. Both fail against the previous client — the retry one with the server holding two copies of the message, the carry-over one with a delivered message still offering a retry that would duplicate it.Worth recording for whoever runs this stack locally: the API must be restarted after backend changes (it runs without --reload) and started with RXHIVE_RATE_LIMIT_LOGIN raised, as CI does. A stale process is invisible — the suite goes green against seventeen-hour-old code — and the default 10 logins/minute cannot seat a full suite run.
Summary by CodeRabbit
New Features
Bug Fixes
Tests