feat(sip): add granular call and transfer lifecycle webhook events#126
Open
nilay-automatesmb wants to merge 2 commits into
Open
feat(sip): add granular call and transfer lifecycle webhook events#126nilay-automatesmb wants to merge 2 commits into
nilay-automatesmb wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR extends the SIP pipeline with granular call and transfer lifecycle webhook events, enabling CRM systems to track call session identity, transfer attempts, and normalized outcomes through existing webhook infrastructure. ChangesSIP Call and Transfer Lifecycle Webhook Events
Sequence Diagram(s)The changes do not warrant a sequence diagram. The lifecycle webhook feature is integrated through existing infrastructure (dispatcher → webhook executor → assistant webhooks) without introducing new multi-component interaction flows; webhook emission is a series of handler invocations that directly call standardized payload builders and the lifecycle webhook callback. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
api/assistant-api/sip/pipeline/transfer.go (2)
217-246: ⚡ Quick winTrack the connected target by loop index to handle duplicate targets correctly.
indexOfTargetreturns the first occurrence. If a target value repeats, the connectedAttemptand the cancellation skip boundary (i < connectedIndex) are computed against the wrong index — reporting a wrong attempt number and emitting spuriousTransferCancelledevents for earlier targets that already failed. Reuse the index from the dial loop; this also removes the redundant rescans and makesindexOfTarget(Lines 296-303) dead code.♻️ Use the loop index instead of rescanning
var outboundSession *sip_infra.Session var connectedTarget string + connectedIndex := -1 for i, target := range targets {if err == nil { outboundSession = session connectedTarget = target + connectedIndex = i d.OnPipeline(ctx, sip_infra.TransferAttemptEndedPipeline{TargetURI: connectedTarget, - Attempt: indexOfTarget(targets, connectedTarget) + 1, + Attempt: connectedIndex + 1, TotalAttempts: len(targets), TransferID: v.TransferID, RoutingMode: v.RoutingMode, }) - connectedIndex := indexOfTarget(targets, connectedTarget) for i, target := range targets { - if target == connectedTarget { + if i == connectedIndex { continue }Then drop the now-unused
indexOfTargethelper.🤖 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 `@api/assistant-api/sip/pipeline/transfer.go` around lines 217 - 246, The loop currently rescans targets with indexOfTarget(…) to compute connectedIndex and Attempt, which misattributes attempt numbers when targets repeat; change the logic in the dial loop to capture the connected target's loop index (i) when you issue the TransferConnectedPipeline and use that i for Attempt and for the cancellation boundary instead of calling indexOfTarget; update the TransferConnectedPipeline.Attempt to use the loop index+1, use that same index to skip earlier cancellations (preserve the v.RoutingMode check), and then remove the now-unused indexOfTarget helper and its calls so duplicate target values are handled correctly and no spurious TransferCancelledPipeline events are emitted.
189-196: ⚡ Quick winPreserve the last dial error in the exhausted-targets failure.
The synthesized message drops the actual SIP failure from the final attempt, so the
TransferFailedPipeline.Errorcarries no diagnostic detail for downstream consumers/logs. Capture the last attempt error and wrap it.♻️ Preserve and wrap the last attempt error
var outboundSession *sip_infra.Session var connectedTarget string + var lastErr error for i, target := range targets {d.logger.Warnw("Pipeline: transfer_target_failed", "call_id", v.ID, "target", target, "attempt", attempt, "error", err) + lastErr = err- Error: fmt.Errorf("all %d transfer targets failed", len(targets)), + Error: fmt.Errorf("all %d transfer targets failed: %w", len(targets), lastErr),If
targetscan be empty here, guard the wrap so anillastErrisn't formatted with%w.As per coding guidelines, "Return errors with context wrapping using fmt.Errorf with %w verb for error chain preservation".
🤖 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 `@api/assistant-api/sip/pipeline/transfer.go` around lines 189 - 196, The TransferFailedPipeline currently emits a generic error; change it to preserve and wrap the last dial error from the final attempt: obtain the lastErr (the error returned by the last target dial attempt), and when building TransferFailedPipeline.Error use fmt.Errorf("all %d transfer targets failed: %w", len(targets), lastErr) to wrap it so the error chain is preserved; if targets is empty or lastErr is nil, fall back to the existing fmt.Errorf("all %d transfer targets failed", len(targets)) to avoid formatting a nil error. Ensure this change is applied where OnPipeline is called for TransferFailedPipeline (using v, targets, lastErr).api/assistant-api/sip/pipeline/signal.go (1)
222-222: 💤 Low valueConsider extracting extension length limit as a named constant.
The magic number
6(maximum extension length) would be clearer as a named constant, e.g.,maxExtensionLength = 6.🤖 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 `@api/assistant-api/sip/pipeline/signal.go` at line 222, Extract the magic number 6 used in the extension-length check into a named constant (e.g., maxExtensionLength = 6) and replace the literal in the condition "if len(user) > 0 && len(user) <= 6" with that constant; define the constant near other file-level constants (or at top of api/assistant-api/sip/pipeline/signal.go) and add a short comment indicating it represents the maximum extension length so callers reading the "len(user) <= maxExtensionLength" check understand its meaning.api/assistant-api/sip/webhook_lifecycle.go (1)
15-24: ⚡ Quick winImport group is not goimports-sorted; likely fails golangci-lint.
Within the
github.com/rapidaaigroup, goimports orders by import path.internal_services(.../internal/services) is placed afterinternal_webhook(.../internal/webhook), so the block is not sorted and the lint check will fail.♻️ Proposed reorder
internal_condition "github.com/rapidaai/api/assistant-api/internal/condition" internal_assistant_entity "github.com/rapidaai/api/assistant-api/internal/entity/assistants" + internal_services "github.com/rapidaai/api/assistant-api/internal/services" internal_type "github.com/rapidaai/api/assistant-api/internal/type" internal_webhook "github.com/rapidaai/api/assistant-api/internal/webhook" - internal_services "github.com/rapidaai/api/assistant-api/internal/services" sip_infra "github.com/rapidaai/api/assistant-api/sip/infra"As per coding guidelines: "Go import groups must follow order: stdlib, external, then github.com/rapidaai (enforced by goimports in golangci-lint)".
🤖 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 `@api/assistant-api/sip/webhook_lifecycle.go` around lines 15 - 24, Reorder the imports in the import block so the github.com/rapidaai group is sorted by path (e.g., place internal_services (.../internal/services) before internal_webhook (.../internal/webhook)); specifically adjust the import block containing internal_condition, internal_assistant_entity, internal_type, internal_webhook, internal_services, sip_infra, gorm_generator, types, type_enums, utils to be goimports-sorted (alphabetical within the rapidaai group) and then run goimports/golangci-lint to verify.
🤖 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 `@api/assistant-api/sip/webhook_lifecycle.go`:
- Around line 59-61: The CreateLog call currently returns its error directly,
losing context; update the return to wrap the error with fmt.Errorf using %w
(e.g. return fmt.Errorf("persisting SIP HTTP log in CreateLog: %w", err)) so the
call site and cause are preserved, and add/import fmt if not already imported;
ensure this change is applied at the return after the CreateLog invocation in
webhook_lifecycle.go (the block that currently does "if err != nil { return err
}").
---
Nitpick comments:
In `@api/assistant-api/sip/pipeline/signal.go`:
- Line 222: Extract the magic number 6 used in the extension-length check into a
named constant (e.g., maxExtensionLength = 6) and replace the literal in the
condition "if len(user) > 0 && len(user) <= 6" with that constant; define the
constant near other file-level constants (or at top of
api/assistant-api/sip/pipeline/signal.go) and add a short comment indicating it
represents the maximum extension length so callers reading the "len(user) <=
maxExtensionLength" check understand its meaning.
In `@api/assistant-api/sip/pipeline/transfer.go`:
- Around line 217-246: The loop currently rescans targets with indexOfTarget(…)
to compute connectedIndex and Attempt, which misattributes attempt numbers when
targets repeat; change the logic in the dial loop to capture the connected
target's loop index (i) when you issue the TransferConnectedPipeline and use
that i for Attempt and for the cancellation boundary instead of calling
indexOfTarget; update the TransferConnectedPipeline.Attempt to use the loop
index+1, use that same index to skip earlier cancellations (preserve the
v.RoutingMode check), and then remove the now-unused indexOfTarget helper and
its calls so duplicate target values are handled correctly and no spurious
TransferCancelledPipeline events are emitted.
- Around line 189-196: The TransferFailedPipeline currently emits a generic
error; change it to preserve and wrap the last dial error from the final
attempt: obtain the lastErr (the error returned by the last target dial
attempt), and when building TransferFailedPipeline.Error use fmt.Errorf("all %d
transfer targets failed: %w", len(targets), lastErr) to wrap it so the error
chain is preserved; if targets is empty or lastErr is nil, fall back to the
existing fmt.Errorf("all %d transfer targets failed", len(targets)) to avoid
formatting a nil error. Ensure this change is applied where OnPipeline is called
for TransferFailedPipeline (using v, targets, lastErr).
In `@api/assistant-api/sip/webhook_lifecycle.go`:
- Around line 15-24: Reorder the imports in the import block so the
github.com/rapidaai group is sorted by path (e.g., place internal_services
(.../internal/services) before internal_webhook (.../internal/webhook));
specifically adjust the import block containing internal_condition,
internal_assistant_entity, internal_type, internal_webhook, internal_services,
sip_infra, gorm_generator, types, type_enums, utils to be goimports-sorted
(alphabetical within the rapidaai group) and then run goimports/golangci-lint to
verify.
🪄 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: 7df713d5-ebcb-4da4-8e12-2b1315dd3de6
📒 Files selected for processing (13)
api/assistant-api/sip/infra/pipeline.goapi/assistant-api/sip/infra/server.goapi/assistant-api/sip/infra/server_inbound.goapi/assistant-api/sip/infra/types.goapi/assistant-api/sip/pipeline/dispatcher.goapi/assistant-api/sip/pipeline/signal.goapi/assistant-api/sip/pipeline/signal_lifecycle_test.goapi/assistant-api/sip/pipeline/transfer.goapi/assistant-api/sip/sip.goapi/assistant-api/sip/webhook_lifecycle.goapi/assistant-api/sip/webhook_lifecycle_test.gopkg/utils/rapida_event.gopkg/utils/rapida_event_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.
Description
Adds granular SIP call and transfer lifecycle webhook events for CRM workflows, extending the existing SIP webhook pipeline without adding websockets or a new realtime server.
Highlights:
call.created,call.no_answer,call.busy,call.rejected(and preserves existingcall.ringing,call.answered,call.cancelled,call.ended,call.failed)transfer.requested,transfer.attempt.ended,transfer.cancelled(and preserves existingtransfer.attempt.started,transfer.connected,transfer.failed)call.media.startedandtransfer.target.ringingevent types with wiring, but does not emit them yet without reliable low-level hooksType of Change
Related Issues
Fixes #125
Checklist
General
Testing
Documentation
Security
Screenshots (if applicable)
N/A
Additional Notes
Go toolchain was not available in my execution environment, so I could not run
go testlocally in this session (gocommand not found).I added/updated tests in:
api/assistant-api/sip/pipeline/signal_lifecycle_test.goapi/assistant-api/sip/webhook_lifecycle_test.gopkg/utils/rapida_event_test.goChanged files:
api/assistant-api/sip/infra/pipeline.goapi/assistant-api/sip/infra/server.goapi/assistant-api/sip/infra/server_inbound.goapi/assistant-api/sip/infra/types.goapi/assistant-api/sip/pipeline/dispatcher.goapi/assistant-api/sip/pipeline/signal.goapi/assistant-api/sip/pipeline/transfer.goapi/assistant-api/sip/sip.goapi/assistant-api/sip/webhook_lifecycle.goapi/assistant-api/sip/pipeline/signal_lifecycle_test.goapi/assistant-api/sip/webhook_lifecycle_test.gopkg/utils/rapida_event.gopkg/utils/rapida_event_test.goSummary by CodeRabbit
New Features
Improvements
Tests