feature: show call info about other users#324
Open
iparaskev wants to merge 14 commits into
Open
Conversation
1:1 calls: user:call:{userID} → JSON {peer, room} — set on accept,
deleted on end or WS disconnect (peer notified via call_end).
Rooms: user:room:{userID} → roomID — set on join, deleted on leave
or WS disconnect.
✅ Deploy Preview for hoppdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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:
📝 WalkthroughWalkthroughBackend: adds a Redis-backed call Tracker (Join/Leave, locks, CleanupUser, GetCallStates) and a GET /api/auth/calls/presence endpoint. Handlers and websocket publish/consume presence_changed. Frontend: adds callsPresence store, polls presence, and renders peer badges and in-call labels. ChangesCall Presence Tracking
Sequence DiagramsequenceDiagram
participant App as FrontendApp
participant API as Auth API (/api/auth/calls/presence)
participant AuthHandler as AuthHandler
participant WS as WebSocketHandler
participant Tracker as callstate.Tracker
participant Redis as Redis
App->>API: GET /api/auth/calls/presence
API->>AuthHandler: GetCallsPresence
AuthHandler->>Tracker: GetCallStates(userIDs)
Tracker->>Redis: Batch GET user:call/<id>, user:room/<id>
Redis-->>Tracker: call/room values
Tracker-->>AuthHandler: presence map
App->>WS: accept/join/end events
WS->>Tracker: JoinCall / SetCallActive / LeaveCall / CleanupUser
Tracker->>Redis: acquire lock, update/delete per-user entries
WS->>API: publish presence_changed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/internal/handlers/slackHandlers.go (1)
766-804:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the same team check before mutating leave state.
LeaveRoomloads the room and then updates Slack membership, Redis presence, andLastParticipantLeftAtwithout verifying that the room belongs to the authenticated user's team. That lets any authenticated user hit this endpoint for another team's room ID and mutate its state. Mirror theroom.TeamIDvsuser.TeamIDguard already used inGetSessionTokensbefore the Slack/presence updates.Suggested fix
room, err := models.GetRoomByID(h.DB, roomID) if err != nil { c.Logger().Errorf("Failed to get room: %v", err) return echo.NewHTTPError(http.StatusInternalServerError, "Failed to load room") } if room == nil { return echo.NewHTTPError(http.StatusNotFound, "Room not found") } + if room.TeamID == nil || user.TeamID == nil || *room.TeamID != *user.TeamID { + return echo.NewHTTPError(http.StatusForbidden, "You don't have access to this room") + } // For Slack rooms, remove the user from the Slack call if room.Type == models.RoomTypeSlack {🤖 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/internal/handlers/slackHandlers.go` around lines 766 - 804, After loading the room, ensure the authenticated user belongs to the same team before making any external or state mutations (Slack removal, Redis presence, LastParticipantLeftAt, CallState). Add a check comparing room.TeamID and user.TeamID (same guard used in GetSessionTokens) immediately after the room nil check and return the same NOT FOUND HTTP error if they differ; only proceed with getBotTokenForSlackTeam/removeParticipantFromSlackCall and h.CallState.RemoveRoomParticipant when the team IDs match.
🧹 Nitpick comments (1)
tauri/src/components/ui/participant-row-wo-livekit.tsx (1)
66-75: ⚡ Quick winConsider using an explicit type guard for better type safety.
The
.filter(Boolean)on line 74 correctly removes null values but doesn't narrow the TypeScript type, which is why optional chaining is needed later (lines 252-256). An explicit type guard would improve type safety.Optional refactor for better type inference
const callPeers = useMemo(() => { const ids = userPresence?.peerIds; if (!ids || ids.length === 0) return []; return ids .map((id) => { if (currentUser?.id === id) return currentUser; return teammates?.find((t) => t.id === id) ?? null; }) - .filter(Boolean); + .filter((p): p is NonNullable<typeof p> => p !== null); }, [userPresence?.peerIds, teammates, currentUser]);This allows you to remove the optional chaining on lines 252-256:
callPeers={callPeers.map((p) => ({ - avatarUrl: p?.avatar_url || undefined, - firstName: p?.first_name ?? "", - lastName: p?.last_name ?? "", + avatarUrl: p.avatar_url || undefined, + firstName: p.first_name, + lastName: p.last_name, }))}🤖 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 `@tauri/src/components/ui/participant-row-wo-livekit.tsx` around lines 66 - 75, The callPeers useMemo currently filters nulls with .filter(Boolean) which won't narrow the TypeScript type; replace that with an explicit type guard so the resulting array is typed as non-null participant objects. Update the .map/.filter chain in the useMemo for callPeers to use a predicate like "isParticipant" (or an inline type guard: (p): p is Participant => p !== null && p !== undefined) so TypeScript knows the array contains only Participant objects and you can remove later optional chaining when accessing callPeers elements; keep references to userPresence?.peerIds, teammates and currentUser unchanged.
🤖 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/internal/callstate/callstate.go`:
- Around line 20-24: The CleanupUser logic currently treats callEntry as
single-peer and deletes every peer's presence key when one user disconnects;
update CleanupUser to load each peer's callEntry (struct callEntry with Peers
[]string and legacy Peer string), remove only the departing user's ID from that
peer's Peers slice (or clear legacy Peer if it matches), and write the modified
callEntry back to Redis; only delete the peer's user:call:* key if the resulting
Peers slice is empty and no legacy Peer remains. Ensure you update any code
paths that iterate peers (the block around CleanupUser and the code referenced
at lines ~128-139) to perform an in-place removal and conditional delete rather
than unconditionally deleting all peer keys.
- Around line 72-73: RemoveRoomParticipant currently ignores roomID and
unconditionally deletes the user's room key; change it to an atomic
compare-and-delete so we only remove the key if its stored value equals the
provided roomID. In the Tracker.RemoveRoomParticipant implementation use an
atomic Redis operation (e.g. EVAL/Lua or a transactional WATCH/MULTI) to read
userRoomKey(userID) and delete it only when its value matches the roomID
argument, otherwise leave it untouched; ensure you still return any Redis error
from the operation.
In `@tauri/src/windows/main-window/app.tsx`:
- Around line 96-99: The useEffect depends on authToken but only lists inACall,
which can cause stale closures; update the dependency array for the effect that
calls refetchCallsPresence so it includes authToken (e.g., useEffect(...,
[inACall, authToken])). Keep refetchCallsPresence out if it is stable, and
reference the existing symbols inACall, callTokens, authToken,
refetchCallsPresence, and the useEffect wrapper when making the change.
---
Outside diff comments:
In `@backend/internal/handlers/slackHandlers.go`:
- Around line 766-804: After loading the room, ensure the authenticated user
belongs to the same team before making any external or state mutations (Slack
removal, Redis presence, LastParticipantLeftAt, CallState). Add a check
comparing room.TeamID and user.TeamID (same guard used in GetSessionTokens)
immediately after the room nil check and return the same NOT FOUND HTTP error if
they differ; only proceed with
getBotTokenForSlackTeam/removeParticipantFromSlackCall and
h.CallState.RemoveRoomParticipant when the team IDs match.
---
Nitpick comments:
In `@tauri/src/components/ui/participant-row-wo-livekit.tsx`:
- Around line 66-75: The callPeers useMemo currently filters nulls with
.filter(Boolean) which won't narrow the TypeScript type; replace that with an
explicit type guard so the resulting array is typed as non-null participant
objects. Update the .map/.filter chain in the useMemo for callPeers to use a
predicate like "isParticipant" (or an inline type guard: (p): p is Participant
=> p !== null && p !== undefined) so TypeScript knows the array contains only
Participant objects and you can remove later optional chaining when accessing
callPeers elements; keep references to userPresence?.peerIds, teammates and
currentUser unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7eb407b-58bf-4ade-ac10-e63bb6332c9f
📒 Files selected for processing (14)
backend/api-files/openapi.yamlbackend/internal/callstate/callstate.gobackend/internal/common/common.gobackend/internal/handlers/handlers.gobackend/internal/handlers/slackHandlers.gobackend/internal/handlers/websocketHandlers.gobackend/internal/server/server.gotauri/src/components/ui/call-center.tsxtauri/src/components/ui/hopp-avatar.tsxtauri/src/components/ui/participant-row-wo-livekit.tsxtauri/src/openapi.d.tstauri/src/store/store.tstauri/src/windows/main-window/app.tsxtauri/src/windows/main-window/tabs/Rooms.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/internal/callstate/callstate.go`:
- Around line 30-48: The current lock release uses bare t.redis.Del(ctx,
lockKey) which can delete a new holder's lock if the original holder's TTL
expires; add a method on Tracker named releaseLock(ctx context.Context, lockKey,
holderID string) that runs an EVAL scripted compare-and-delete (check GET ==
holderID then DEL) and return/discard errors, and replace all deferred or direct
calls to t.redis.Del(ctx, lockKey) with t.releaseLock(ctx, lockKey, holderID)
(ensure the holderID used matches the value stored when acquiring the lock in
acquireLock); update usages where Del appears (the other Del occurrences in this
file) to call the new releaseLock helper.
- Around line 256-263: In JoinCall, treat a missing peer entry as non-fatal
instead of aborting: when calling t.redis.Get(ctx, callKey(peerID)).Result(), if
the error is redis.Nil (or equivalent “not found”), skip that peer and continue
iterating rather than returning an error; only return on other Redis errors.
After retrieving peerRaw, only attempt json.Unmarshal into callEntry if peerRaw
exists; if the key was missing skip the unmarshal and continue. Update the logic
around t.redis.Get, peerRaw, callEntry, and json.Unmarshal in the JoinCall loop
to mirror the tolerant behavior used in LeaveCall.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b995f31-2cd9-402d-b509-4ec6d33f4704
📒 Files selected for processing (3)
backend/internal/callstate/callstate.gobackend/internal/handlers/websocketHandlers.gobackend/internal/messages/messages.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tauri/src/components/ui/participant-row-wo-livekit.tsx (1)
67-76: ⚡ Quick winConsider using a type predicate for better type narrowing.
The
.filter(Boolean)on line 75 correctly removesnullvalues at runtime, but TypeScript doesn't automatically narrow the type from(BaseUser | null)[]toBaseUser[]. Using a type predicate would eliminate the need for optional chaining on lines 305-307 and improve type safety.♻️ Proposed fix with type predicate
return ids .map((id) => { if (currentUser?.id === id) return currentUser; return teammates?.find((t) => t.id === id) ?? null; }) - .filter(Boolean); + .filter((p): p is BaseUser => p !== null); }, [userPresence?.peerIds, teammates, currentUser]);Then on lines 304-308, the optional chaining can be simplified:
callPeers={callPeers.map((p) => ({ - avatarUrl: p?.avatar_url || undefined, - firstName: p?.first_name ?? "", - lastName: p?.last_name ?? "", + avatarUrl: p.avatar_url || undefined, + firstName: p.first_name ?? "", + lastName: p.last_name ?? "", }))}🤖 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 `@tauri/src/components/ui/participant-row-wo-livekit.tsx` around lines 67 - 76, callPeers currently returns (BaseUser | null)[] because .filter(Boolean) doesn't let TypeScript narrow types; define a type predicate function (e.g., isBaseUser(item: BaseUser | null): item is BaseUser) and replace .filter(Boolean) with .filter(isBaseUser) inside the useMemo so callPeers is typed as BaseUser[] (update references to callPeers / usages like currentUser and teammates without optional chaining accordingly).
🤖 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 `@tauri/src/components/ui/participant-row-wo-livekit.tsx`:
- Around line 67-76: callPeers currently returns (BaseUser | null)[] because
.filter(Boolean) doesn't let TypeScript narrow types; define a type predicate
function (e.g., isBaseUser(item: BaseUser | null): item is BaseUser) and replace
.filter(Boolean) with .filter(isBaseUser) inside the useMemo so callPeers is
typed as BaseUser[] (update references to callPeers / usages like currentUser
and teammates without optional chaining accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c545740-d649-4cbd-81ee-71b6cf7453e9
📒 Files selected for processing (3)
backend/internal/callstate/callstate.gobackend/internal/server/server.gotauri/src/components/ui/participant-row-wo-livekit.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/internal/server/server.go
- backend/internal/callstate/callstate.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/internal/callstate/callstate.go (2)
347-351:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly skip
redis.NilinLeaveCall; surface real Redis errors.Continuing on all errors can commit partial updates and leave stale peer references.
Suggested fix
peerRaw, err := t.redis.Get(ctx, callKey(peerID)).Result() if err != nil { - // Peer already left, skip - continue + if err == redis.Nil { + // Peer already left, skip + continue + } + return "", nil, fmt.Errorf("failed to read peer %s entry: %w", peerID, err) }🤖 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/internal/callstate/callstate.go` around lines 347 - 351, In LeaveCall, do not treat all Redis errors as "peer already left": when calling t.redis.Get(ctx, callKey(peerID)) check for redis.Nil and continue only in that case; for any other non-nil err from t.redis.Get return or propagate the error so the caller can handle it and avoid partial updates. Update the error handling around the peerRaw/err from t.redis.Get in the LeaveCall function to explicitly compare against redis.Nil and surface other errors.
141-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t drop non-
redis.Nilpipeline failures inGetCallStates.Ignoring
Execerrors can silently return empty/incomplete presence during Redis failures.Suggested fix
- pipe.Exec(ctx) //nolint:errcheck — individual cmd errors checked below + if _, err := pipe.Exec(ctx); err != nil && !errors.Is(err, redis.Nil) { + return nil, fmt.Errorf("get call states pipeline failed: %w", err) + }🤖 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/internal/callstate/callstate.go` at line 141, GetCallStates currently ignores the error returned by the Redis pipeline Exec call (pipe.Exec(ctx)), which can hide real failures; change GetCallStates to capture the error from pipe.Exec(ctx), treat redis.Nil results specially but return or log any non-redis.Nil errors (or wrap and return them) instead of dropping them, so callers get an error when the pipeline fails; update the code around pipe.Exec(ctx) in GetCallStates to check err := pipe.Exec(ctx) and handle err != nil && err != redis.Nil appropriately.
♻️ Duplicate comments (3)
backend/internal/callstate/callstate.go (3)
114-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake room removal conditional on the stored room value.
RemoveRoomParticipantignoresroomID, so a stale leave for room A can delete a newer room B membership. Use atomic compare-and-delete.Suggested fix
func (t *Tracker) RemoveRoomParticipant(ctx context.Context, roomID, userID string) error { - return t.redis.Del(ctx, userRoomKey(userID)).Err() + const script = ` + if redis.call("GET", KEYS[1]) == ARGV[1] then + return redis.call("DEL", KEYS[1]) + end + return 0 + ` + return t.redis.Eval(ctx, script, []string{userRoomKey(userID)}, roomID).Err() }🤖 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/internal/callstate/callstate.go` around lines 114 - 116, RemoveRoomParticipant currently ignores the roomID and unconditionally deletes the userRoomKey, which can remove a newer room membership; change it to perform an atomic compare-and-delete: read and compare the key's value to the provided roomID and only delete if they match using an atomic Redis operation (e.g. EVAL Lua script or WATCH/MULTI) inside Tracker.RemoveRoomParticipant so the key is removed only when its stored value equals roomID and return appropriate errors via the existing t.redis API.
287-290:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat missing peer entries as a valid race in
JoinCall.If a peer leaves mid-join,
redis.Nilshould be skipped instead of failing the whole join flow.Suggested fix
peerRaw, err := t.redis.Get(ctx, callKey(peerID)).Result() if err != nil { - return "", nil, fmt.Errorf("failed to read peer %s entry: %w", peerID, err) + if err == redis.Nil { + continue + } + return "", nil, fmt.Errorf("failed to read peer %s entry: %w", peerID, err) }🤖 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/internal/callstate/callstate.go` around lines 287 - 290, The call to t.redis.Get(...).Result() in JoinCall currently treats any error (including redis.Nil) as fatal; change the logic so that when err == redis.Nil you treat the peer as having left during join and skip/ignore that peer instead of returning an error. Locate the call using symbols t.redis.Get, callKey(peerID), peerRaw and adjust the error handling so redis.Nil is handled as a non-fatal race (continue/omit the peer) while other errors still return fmt.Errorf(...) as before.
56-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelease room locks with ownership check, not plain
DEL.If the TTL expires and another goroutine acquires the lock, the deferred
DELcan remove the new holder’s lock and break mutual exclusion.Suggested fix
+func (t *Tracker) releaseLock(ctx context.Context, lockKey, holderID string) { + const script = ` + if redis.call("GET", KEYS[1]) == ARGV[1] then + return redis.call("DEL", KEYS[1]) + end + return 0 + ` + _ = t.redis.Eval(ctx, script, []string{lockKey}, holderID).Err() +} ... - defer t.redis.Del(ctx, lockKey) + defer t.releaseLock(ctx, lockKey, userID) ... - defer t.redis.Del(ctx, lockKey) + defer t.releaseLock(ctx, lockKey, joinerID) ... - defer t.redis.Del(ctx, lockKey) + defer t.releaseLock(ctx, lockKey, userID)Also applies to: 191-191, 255-255, 339-339
🤖 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/internal/callstate/callstate.go` around lines 56 - 57, The deferred plain DEL used to release room locks can delete a lock acquired by another goroutine if TTL expired and was re-acquired; update the lock release logic to perform an ownership-checked delete (compare-and-delete) instead of unconditional DEL: when creating a lock (the code that uses lockTTL/lockMaxRetries), generate and store a unique owner token/value per lock, set that value in Redis, and replace every plain DEL call with a Redis EVAL/LUA script (or Redis DEL-if-value-matches helper) that atomically checks the stored value equals the owner token and only then deletes the key; apply this change to all release sites referenced around callstate.go (the deferred release calls at the locations mentioned) so locks are only released by their original owner.
🧹 Nitpick comments (1)
backend/internal/callstate/callstate_test.go (1)
128-138: ⚡ Quick winAdd a stale-room regression case for
RemoveRoomParticipant.Current tests don’t cover the race where a delayed leave from room A must not remove a newer room B membership.
Suggested test case
func TestRemoveRoomParticipant(t *testing.T) { tr, mr := setupTracker(t) ctx := context.Background() @@ // No error when key missing require.NoError(t, tr.RemoveRoomParticipant(ctx, "room1", "u1")) + + t.Run("stale leave does not remove current room", func(t *testing.T) { + require.NoError(t, tr.AddRoomParticipant(ctx, "room2", "u1")) + require.NoError(t, tr.RemoveRoomParticipant(ctx, "room1", "u1")) // stale + assert.Equal(t, "room2", getRoomEntry(t, mr, "u1")) + }) }Also applies to: 140-149
🤖 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/internal/callstate/callstate_test.go` around lines 128 - 138, Extend the TestRemoveRoomParticipant test to cover the stale-room race: after adding "u1" to "room1" and then updating their entry to "room2" (use tr.AddRoomParticipant(ctx, "room2", "u1") or directly mutate the store via the same helpers), call tr.RemoveRoomParticipant(ctx, "room1", "u1") to simulate a delayed leave for the old room and assert the store still shows "room2" for "u1" (use getRoomEntry and require/assert helpers). Do the same addition/removal sequence for the other similar test case referenced (the block around RemoveRoomParticipant usages) to ensure a RemoveRoomParticipant for a stale room never clears a newer room membership. Ensure you reference the tracker methods AddRoomParticipant and RemoveRoomParticipant and the helper getRoomEntry in the new assertions.
🤖 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.
Outside diff comments:
In `@backend/internal/callstate/callstate.go`:
- Around line 347-351: In LeaveCall, do not treat all Redis errors as "peer
already left": when calling t.redis.Get(ctx, callKey(peerID)) check for
redis.Nil and continue only in that case; for any other non-nil err from
t.redis.Get return or propagate the error so the caller can handle it and avoid
partial updates. Update the error handling around the peerRaw/err from
t.redis.Get in the LeaveCall function to explicitly compare against redis.Nil
and surface other errors.
- Line 141: GetCallStates currently ignores the error returned by the Redis
pipeline Exec call (pipe.Exec(ctx)), which can hide real failures; change
GetCallStates to capture the error from pipe.Exec(ctx), treat redis.Nil results
specially but return or log any non-redis.Nil errors (or wrap and return them)
instead of dropping them, so callers get an error when the pipeline fails;
update the code around pipe.Exec(ctx) in GetCallStates to check err :=
pipe.Exec(ctx) and handle err != nil && err != redis.Nil appropriately.
---
Duplicate comments:
In `@backend/internal/callstate/callstate.go`:
- Around line 114-116: RemoveRoomParticipant currently ignores the roomID and
unconditionally deletes the userRoomKey, which can remove a newer room
membership; change it to perform an atomic compare-and-delete: read and compare
the key's value to the provided roomID and only delete if they match using an
atomic Redis operation (e.g. EVAL Lua script or WATCH/MULTI) inside
Tracker.RemoveRoomParticipant so the key is removed only when its stored value
equals roomID and return appropriate errors via the existing t.redis API.
- Around line 287-290: The call to t.redis.Get(...).Result() in JoinCall
currently treats any error (including redis.Nil) as fatal; change the logic so
that when err == redis.Nil you treat the peer as having left during join and
skip/ignore that peer instead of returning an error. Locate the call using
symbols t.redis.Get, callKey(peerID), peerRaw and adjust the error handling so
redis.Nil is handled as a non-fatal race (continue/omit the peer) while other
errors still return fmt.Errorf(...) as before.
- Around line 56-57: The deferred plain DEL used to release room locks can
delete a lock acquired by another goroutine if TTL expired and was re-acquired;
update the lock release logic to perform an ownership-checked delete
(compare-and-delete) instead of unconditional DEL: when creating a lock (the
code that uses lockTTL/lockMaxRetries), generate and store a unique owner
token/value per lock, set that value in Redis, and replace every plain DEL call
with a Redis EVAL/LUA script (or Redis DEL-if-value-matches helper) that
atomically checks the stored value equals the owner token and only then deletes
the key; apply this change to all release sites referenced around callstate.go
(the deferred release calls at the locations mentioned) so locks are only
released by their original owner.
---
Nitpick comments:
In `@backend/internal/callstate/callstate_test.go`:
- Around line 128-138: Extend the TestRemoveRoomParticipant test to cover the
stale-room race: after adding "u1" to "room1" and then updating their entry to
"room2" (use tr.AddRoomParticipant(ctx, "room2", "u1") or directly mutate the
store via the same helpers), call tr.RemoveRoomParticipant(ctx, "room1", "u1")
to simulate a delayed leave for the old room and assert the store still shows
"room2" for "u1" (use getRoomEntry and require/assert helpers). Do the same
addition/removal sequence for the other similar test case referenced (the block
around RemoveRoomParticipant usages) to ensure a RemoveRoomParticipant for a
stale room never clears a newer room membership. Ensure you reference the
tracker methods AddRoomParticipant and RemoveRoomParticipant and the helper
getRoomEntry in the new assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4c2b8ef-afd0-4a8a-84c0-65dc4756ff07
⛔ Files ignored due to path filters (1)
backend/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
backend/go.modbackend/internal/callstate/callstate.gobackend/internal/callstate/callstate_test.go
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.
Closes #317.
Only thing missing is allowing to join an in progress call.
Summary by CodeRabbit
New Features
Refactor
Tests