feat: add session_id to webhook payloads and jid to API responses#579
feat: add session_id to webhook payloads and jid to API responses#579occult wants to merge 3 commits intoaldinokemal:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdded optional Changes
Sequence Diagram(s)sequenceDiagram
participant WhatsApp as "WhatsApp Client"
participant Router as "Event Handler"
participant Creator as "Payload Creator"
participant Webhook as "External Webhook"
WhatsApp->>Router: emit event (Message/Receipt/Delete/Group/Call/Newsletter)
Router->>Router: read instance.ID() as sessionID\nand instance.JID() as deviceJID
Router->>Creator: create payload (event, device_id=deviceJID, session_id=sessionID, payload...)
Creator->>Webhook: POST webhook body (includes device_id and optional session_id)
Webhook-->>Router: 200 / ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
Summary of ChangesHello @occult, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical need for multi-tenant integrations by enhancing data correlation capabilities. It introduces Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces the session_id field to webhook payloads and adds the jid field to API responses, which is a valuable addition for multi-tenant integrations to correlate webhook events and registered sessions. Static analysis (SAST Recon) found no vulnerabilities within the audited scope. However, some areas for improvement were noted in goroutine closure handling and variable naming to enhance safety and maintainability.
|
sgtm |
|
LGMT, go ahead |
aldinokemal
left a comment
aldinokemal
left a comment
There was a problem hiding this comment.
Code Review Summary
Overall the PR is well-structured and backward-compatible. The session_id is additive with proper omitempty/conditional checks, and device_id (JID) remains unchanged. All webhook event types are covered consistently.
Key findings:
-
Message webhook derives
device_iddifferently than all other events — The message path usesNormalizeJIDFromLID()onclient.Store.IDinsidecreateWebhookEvent, while all other events useinstance.JID()directly. For LID-based accounts, this could produce differentdevice_idvalues between message webhooks and other webhook types. -
Goroutine closure capture inconsistency — The message handler correctly passes
sessionIDas a goroutine function argument, but other handlers (delete, group, receipt, newsletter) capturesessionID/deviceIDfrom the outer scope. While safe for immutable strings, it's inconsistent. -
MCP layer not updated — REST endpoints now include
jid, but MCP equivalents do not. Clarify if intentional. -
WebhookEventstruct JSON tags are decorative — The struct is manually converted tomap[string]anybefore serialization, so JSON tags have no effect.
See inline comments for details.
| handleStreamReplaced(ctx) | ||
| case *events.Message: | ||
| handleMessage(ctx, evt, chatStorageRepo, client) | ||
| handleMessage(ctx, evt, chatStorageRepo, sessionID, client) |
There was a problem hiding this comment.
Inconsistency: handleMessage receives sessionID but not deviceJID, unlike every other event handler in this switch. The message webhook path re-derives device_id from client.Store.ID using NormalizeJIDFromLID() inside createWebhookEvent (event_message.go), while all other events use the pre-resolved deviceJID (from instance.JID()).
For LID-based WhatsApp accounts, NormalizeJIDFromLID() may resolve to a different value than instance.JID(), causing device_id to differ between message webhooks and all other webhook types.
Suggestion: Pass deviceJID to handleMessage as well, and have createWebhookEvent accept the pre-resolved JID instead of re-deriving it from the client store. This makes all event handlers use the same pattern.
| webhookCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| defer cancel() | ||
| if err := forwardDeleteToWebhook(webhookCtx, evt, message, deviceID, c); err != nil { | ||
| if err := forwardDeleteToWebhook(webhookCtx, evt, message, sessionID, deviceID, c); err != nil { |
There was a problem hiding this comment.
Inconsistency: sessionID, deviceID, evt, and message are captured from the outer scope rather than passed as goroutine function arguments. Compare with the message handler in event_message_handler.go which correctly passes all values as arguments:
go func(e *events.Message, c *whatsmeow.Client, sid string) {While safe here (strings are immutable, pointers are not mutated after launch), the inconsistent pattern is fragile. Consider passing all referenced variables as goroutine arguments for uniformity.
| Payload map[string]any `json:"payload"` | ||
| Event string `json:"event"` | ||
| DeviceID string `json:"device_id"` | ||
| SessionID string `json:"session_id,omitempty"` |
There was a problem hiding this comment.
Note: These JSON struct tags are decorative — WebhookEvent is never marshaled directly to JSON. Instead, forwardMessageToWebhook manually constructs a map[string]any from the struct fields. If someone later marshals the struct directly (relying on these tags), the behavior would differ from the current map-based approach.
Consider either marshaling the struct directly, or removing the JSON tags to avoid the false impression that they control serialization.
| if deviceID != "" { | ||
| body["device_id"] = deviceID | ||
| } | ||
| if sessionID != "" { |
There was a problem hiding this comment.
Suggestion: This if sessionID != "" { body["session_id"] = sessionID } pattern is repeated across 9+ webhook functions (call, delete, group, joined-group, newsletter join/leave/update/mute, receipt). Consider extracting a helper:
func buildWebhookEnvelope(sessionID, deviceID string) map[string]any {
body := make(map[string]any)
if deviceID != "" {
body["device_id"] = deviceID
}
if sessionID != "" {
body["session_id"] = sessionID
}
return body
}This reduces duplication and ensures any future envelope changes are applied uniformly.
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge Review NotesThis PR adds Key Changes:
Existing Comments: Files Reviewed (11 files)
|
|
please update this PR and we can merge it |
Webhook payloads only contained the WhatsApp JID as device_id, while API responses (/app/status, /app/devices) only returned the user-assigned session ID. This made it impossible for multi-tenant integrations to correlate webhook events back to the session they registered. Add session_id field to all webhook payloads (message, receipt, delete, group, call, newsletter) alongside the existing device_id (JID) for backward compatibility. Add jid field to /app/status and /app/devices responses. Closes aldinokemal#578 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
aldinokemal
force-pushed
the
feat/webhook-session-id
branch
from
366d02b to
1741d28
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/infrastructure/whatsapp/event_handler.go (1)
234-240: Same goroutine pattern inconsistency.
sessionIDanddeviceIDare captured from outer scope rather than passed as arguments. For uniformity withhandleWebhookForwardinevent_message_handler.go, consider:♻️ Suggested refactor
- go func(e *events.Receipt, c *whatsmeow.Client) { + go func(e *events.Receipt, c *whatsmeow.Client, sid, did string) { webhookCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - if err := forwardReceiptToWebhook(webhookCtx, e, sessionID, deviceID, c); err != nil { + if err := forwardReceiptToWebhook(webhookCtx, e, sid, did, c); err != nil { logrus.Errorf("Failed to forward ack event to webhook: %v", err) } - }(evt, client) + }(evt, client, sessionID, deviceID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/whatsapp/event_handler.go` around lines 234 - 240, The goroutine in event_handler.go captures sessionID and deviceID from the outer scope; change it to accept them as explicit parameters like the pattern used in handleWebhookForward to avoid closure capture. Update the anonymous function signature for forwardReceiptToWebhook to include sessionID and deviceID (e.g., func(e *events.Receipt, c *whatsmeow.Client, sessionID string, deviceID string) {...}) and pass sessionID and deviceID in the invocation (e.g., ...(evt, client, sessionID, deviceID)). Keep the existing context.WithTimeout/ cancel and the forwardReceiptToWebhook call unchanged except for using the passed-in sessionID and deviceID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/infrastructure/whatsapp/event_handler.go`:
- Around line 234-240: The goroutine in event_handler.go captures sessionID and
deviceID from the outer scope; change it to accept them as explicit parameters
like the pattern used in handleWebhookForward to avoid closure capture. Update
the anonymous function signature for forwardReceiptToWebhook to include
sessionID and deviceID (e.g., func(e *events.Receipt, c *whatsmeow.Client,
sessionID string, deviceID string) {...}) and pass sessionID and deviceID in the
invocation (e.g., ...(evt, client, sessionID, deviceID)). Keep the existing
context.WithTimeout/ cancel and the forwardReceiptToWebhook call unchanged
except for using the passed-in sessionID and deviceID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 89eb8fc8-dfb0-4243-8e5c-45901584b086
📒 Files selected for processing (11)
src/domains/app/app.gosrc/infrastructure/whatsapp/event_call.gosrc/infrastructure/whatsapp/event_delete.gosrc/infrastructure/whatsapp/event_group.gosrc/infrastructure/whatsapp/event_handler.gosrc/infrastructure/whatsapp/event_message.gosrc/infrastructure/whatsapp/event_message_handler.gosrc/infrastructure/whatsapp/event_newsletter.gosrc/infrastructure/whatsapp/event_receipt.gosrc/ui/rest/app.gosrc/usecase/app.go
✅ Files skipped from review due to trivial changes (3)
- src/domains/app/app.go
- src/ui/rest/app.go
- src/infrastructure/whatsapp/event_newsletter.go
🚧 Files skipped from review as they are similar to previous changes (5)
- src/infrastructure/whatsapp/event_receipt.go
- src/infrastructure/whatsapp/event_delete.go
- src/infrastructure/whatsapp/event_message.go
- src/infrastructure/whatsapp/event_call.go
- src/infrastructure/whatsapp/event_group.go
8 participants
Context
device_id, while API responses (/app/status,/app/devices) only returned the user-assigned session ID. This made it impossible for multi-tenant integrations to correlate webhook events back to the session they registered withPOST /devices.session_idfield to all webhook payloads (message, receipt, delete, group, call, newsletter) alongside the existingdevice_id(JID) for backward compatibility.jidfield to/app/statusand/app/devicesAPI responses.Test Results
go build ./...compiles cleanly with no errorsgo test ./...— all tests pass:go vet ./...— no issuesdevice_id(JID) field is unchanged;session_idandjidare additive fields