fix: races in incoming calls#318
Merged
Merged
Conversation
✅ Deploy Preview for hoppdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis PR enhances call handling by propagating creation timestamps from the backend through incoming-call messages, centralizing caller ID state in the Tauri store, filtering stale calls on reception, improving rejection flows, adding auto-timeout for unanswered calls, and strengthening websocket reliability with inactivity deadlines and faster heartbeats. Docker task configuration and app version are also updated. ChangesCall Handling Improvements
Deployment and Versioning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/Taskfile.yml`:
- Around line 54-61: The Taskfile entry for the task fullstack-up has a
misleading desc "Build and run" while the build-image dependency is commented
out; either uncomment the commented task reference "- task: build-image" under
cmds so fullstack-up actually performs the build (restore the dependency in the
cmds list) or change the desc to remove "Build and" so it accurately reads e.g.
"Run the fullstack setup with Redis and backend"; update the single symbol
fullstack-up and the commented line "- task: build-image" accordingly.
In `@tauri/src/services/socket.ts`:
- Line 108: The comment next to the ping interval (the setInterval that sends a
ping every 15_000 ms) is stale: backend uses a 65s read timeout, so keep the 15s
heartbeat but update the comment to reflect the backend wsReadTimeout (≈65s)
instead of "2 heartbeats (30 seconds)"; e.g. note that connections will be
considered dead after the backend's ~65s read timeout (~4 heartbeats), or simply
reference the backend read-deadline, and leave the setInterval/ping logic
(sendPing / setInterval) 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: 30e3f753-dcf1-424a-b98a-638e4554bbb7
📒 Files selected for processing (10)
backend/Taskfile.ymlbackend/internal/handlers/websocketHandlers.gobackend/internal/messages/messages.gotauri/src-tauri/tauri.conf.jsontauri/src/components/ui/call-banner.tsxtauri/src/components/ui/participant-row-wo-livekit.tsxtauri/src/payloads.tstauri/src/services/socket.tstauri/src/store/store.tstauri/src/windows/main-window/app.tsx
Comment on lines
+54
to
+61
| fullstack-up: | ||
| desc: Build and run the fullstack setup with Redis and backend | ||
| cmds: | ||
| # - task: build-image | ||
| - docker compose -f docker-files/fullstack-compose.yml up -d | ||
| env: | ||
| ENV_STACK: local | ||
|
|
There was a problem hiding this comment.
Description doesn't match implementation.
The task description claims "Build and run" but the build-image dependency on line 57 is commented out, so no build step actually executes.
Either uncomment the build task or update the description to remove "Build and".
📝 Proposed fix options
Option 1: Update description to match current behavior
- desc: Build and run the fullstack setup with Redis and backend
+ desc: Run the fullstack setup with Redis and backendOption 2: Uncomment the build step to match description
cmds:
- # - task: build-image
+ - task: build-docker
- docker compose -f docker-files/fullstack-compose.yml up -d📝 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
| fullstack-up: | |
| desc: Build and run the fullstack setup with Redis and backend | |
| cmds: | |
| # - task: build-image | |
| - docker compose -f docker-files/fullstack-compose.yml up -d | |
| env: | |
| ENV_STACK: local | |
| fullstack-up: | |
| desc: Run the fullstack setup with Redis and backend | |
| cmds: | |
| # - task: build-image | |
| - docker compose -f docker-files/fullstack-compose.yml up -d | |
| env: | |
| ENV_STACK: local |
🤖 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/Taskfile.yml` around lines 54 - 61, The Taskfile entry for the task
fullstack-up has a misleading desc "Build and run" while the build-image
dependency is commented out; either uncomment the commented task reference "-
task: build-image" under cmds so fullstack-up actually performs the build
(restore the dependency in the cmds list) or change the desc to remove "Build
and" so it accurately reads e.g. "Run the fullstack setup with Redis and
backend"; update the single symbol fullstack-up and the commented line "- task:
build-image" accordingly.
| }); | ||
| } | ||
| }, 30_000); // Send ping every 30 seconds | ||
| }, 15_000); // Send ping every 15 seconds |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify backend websocket read deadline timeout is compatible with 15s heartbeat.
# Expected: Backend timeout should be at least 30-45 seconds to accommodate 15s heartbeat.
# Search for read deadline configuration in websocket handler
rg -n -C5 'SetReadDeadline|ReadDeadline|pongWait|readWait' backend/internal/handlers/websocketHandlers.go
# Also check for any timeout constants or configurations
rg -n 'time\.(Second|Minute|Duration).*[0-9]+' backend/internal/handlers/websocketHandlers.goRepository: gethopp/hopp
Length of output: 932
Backend timeout already accommodates the 15s heartbeat
- Backend sets
wsReadTimeout = 65 * time.Secondviaws.SetReadDeadline(...)and resets it on each incomingPing, so 15s heartbeats are well within the read-deadline window. - Minor: the comment “Kill the connections after 2 heartbeats (30 seconds for old apps)” no longer matches the 65s timeout; update it.
🤖 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/services/socket.ts` at line 108, The comment next to the ping
interval (the setInterval that sends a ping every 15_000 ms) is stale: backend
uses a 65s read timeout, so keep the 15s heartbeat but update the comment to
reflect the backend wsReadTimeout (≈65s) instead of "2 heartbeats (30 seconds)";
e.g. note that connections will be considered dead after the backend's ~65s read
timeout (~4 heartbeats), or simply reference the backend read-deadline, and
leave the setInterval/ping logic (sendPing / setInterval) unchanged.
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 by CodeRabbit
New Features
Bug Fixes
Improvements
Chores