Add SSE support #154
Add SSE support #154
Conversation
WalkthroughThis change introduces a new Server-Sent Events (SSE) module and service for real-time event streaming to mobile devices. The service is integrated into the application's dependency injection and HTTP routing, with configuration, lifecycle management, and API documentation updates. Supporting controllers and handlers are added and wired into the existing module structure. Additionally, the push notification mechanism is refactored to use a more generic events notification system, replacing direct push token usage and related domain event types with a unified events service. Changes
Sequence Diagram(s)sequenceDiagram
participant Client (Mobile)
participant Fiber Router
participant MobileController
participant SSE Service
Client->>Fiber Router: GET /events (with device auth)
Fiber Router->>MobileController: get(device, ctx)
MobileController->>SSE Service: Handler(deviceId, ctx)
SSE Service->>Client: SSE stream (events/keepalive)
SSE Service-->>Client: (on event) send event data
Client-->>SSE Service: (disconnect or close)
SSE Service->>SSE Service: Cleanup connection
Estimated code review effort4 (60–120 minutes) Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.mod(1 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/events/mobile.go(1 hunks)internal/sms-gateway/handlers/mobile.go(5 hunks)internal/sms-gateway/handlers/module.go(2 hunks)internal/sms-gateway/modules/sse/config.go(1 hunks)internal/sms-gateway/modules/sse/module.go(1 hunks)internal/sms-gateway/modules/sse/service.go(1 hunks)pkg/swagger/docs/mobile.http(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test
- GitHub Check: E2E
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
go.mod (1)
12-12: LGTM! Standard dependency update to support SSE functionality.The minor version bump from v0.2.1 to v0.2.2 aligns with the SSE module integration requirements.
internal/sms-gateway/app.go (2)
18-18: LGTM! Proper import placement.The SSE module import follows alphabetical ordering and integrates cleanly with existing imports.
49-49: LGTM! Consistent module integration.The SSE module is properly registered in the fx.Module list, following the established dependency injection pattern.
pkg/swagger/docs/mobile.http (1)
79-81: LGTM! Well-documented SSE endpoint.The new
/eventsendpoint documentation follows the established format with proper authorization headers and is consistent with other API endpoints in this file.internal/sms-gateway/modules/sse/config.go (1)
1-15: LGTM! Well-structured SSE configuration.The configuration implementation follows Go best practices:
- Appropriate 15-second keep-alive period for SSE connections
- Clean zero-value handling in SetDefaults()
- Simple, focused structure with clear naming
internal/config/module.go (1)
11-11: LGTM! Proper import addition.The SSE module import is correctly placed and follows the existing import structure.
internal/sms-gateway/handlers/module.go (1)
5-5: LGTM!The integration of the events controller follows the established pattern for adding new controllers to the module.
Also applies to: 35-35
internal/sms-gateway/handlers/mobile.go (1)
11-11: LGTM!The events controller integration is consistent with the existing webhook and settings controller patterns.
Also applies to: 39-39, 314-314, 329-329, 342-342
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/sms-gateway/modules/events/service.go (1)
65-65: Consider queue size for high-traffic scenarios.The buffered queue size of 128 may be insufficient for high-traffic scenarios. Consider making this configurable or increasing the default size based on expected load patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/upstream.go(1 hunks)internal/sms-gateway/modules/events/events.go(1 hunks)internal/sms-gateway/modules/events/module.go(1 hunks)internal/sms-gateway/modules/events/service.go(1 hunks)internal/sms-gateway/modules/events/types.go(1 hunks)internal/sms-gateway/modules/messages/errors.go(1 hunks)internal/sms-gateway/modules/messages/service.go(6 hunks)internal/sms-gateway/modules/push/domain/events.go(0 hunks)internal/sms-gateway/modules/push/fcm/client.go(2 hunks)internal/sms-gateway/modules/push/fcm/utils.go(1 hunks)internal/sms-gateway/modules/push/service.go(4 hunks)internal/sms-gateway/modules/push/types.go(1 hunks)internal/sms-gateway/modules/push/types/types.go(1 hunks)internal/sms-gateway/modules/push/upstream/client.go(3 hunks)internal/sms-gateway/modules/settings/service.go(3 hunks)internal/sms-gateway/modules/sse/service.go(1 hunks)internal/sms-gateway/modules/sse/types.go(1 hunks)internal/sms-gateway/modules/webhooks/service.go(4 hunks)pkg/swagger/docs/mobile.http(2 hunks)
🧠 Learnings (7)
internal/sms-gateway/modules/push/fcm/client.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
internal/sms-gateway/modules/push/upstream/client.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
internal/sms-gateway/modules/events/service.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
internal/sms-gateway/modules/settings/service.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
internal/sms-gateway/modules/push/service.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
internal/sms-gateway/modules/push/types.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
internal/sms-gateway/modules/messages/service.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
🧬 Code Graph Analysis (8)
internal/sms-gateway/handlers/upstream.go (4)
internal/sms-gateway/modules/events/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/sse/types.go (1)
Event(5-8)
internal/sms-gateway/modules/push/fcm/utils.go (1)
internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
internal/sms-gateway/modules/webhooks/service.go (2)
internal/sms-gateway/modules/events/service.go (2)
Service(23-36)NewService(38-73)internal/sms-gateway/modules/events/events.go (1)
NewWebhooksUpdatedEvent(13-15)
internal/sms-gateway/modules/push/fcm/client.go (2)
internal/sms-gateway/modules/push/types.go (1)
Event(16-16)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
internal/sms-gateway/modules/push/upstream/client.go (2)
internal/sms-gateway/modules/events/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
internal/sms-gateway/modules/settings/service.go (3)
internal/sms-gateway/modules/events/service.go (2)
Service(23-36)NewService(38-73)internal/sms-gateway/modules/push/service.go (1)
Service(37-50)internal/sms-gateway/modules/events/events.go (1)
NewSettingsUpdatedEvent(27-29)
internal/sms-gateway/modules/push/types.go (1)
internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
internal/sms-gateway/modules/messages/service.go (3)
internal/sms-gateway/modules/events/service.go (1)
Service(23-36)internal/sms-gateway/modules/push/service.go (1)
Service(37-50)internal/sms-gateway/modules/events/events.go (2)
NewMessageEnqueuedEvent(9-11)NewMessagesExportRequestedEvent(17-25)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/push/domain/events.go
✅ Files skipped from review due to trivial changes (6)
- internal/sms-gateway/app.go
- internal/sms-gateway/modules/messages/errors.go
- internal/sms-gateway/modules/sse/types.go
- internal/sms-gateway/modules/push/types/types.go
- internal/sms-gateway/modules/events/events.go
- internal/sms-gateway/modules/events/types.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/swagger/docs/mobile.http
- internal/sms-gateway/modules/sse/service.go
🧰 Additional context used
🧠 Learnings (7)
internal/sms-gateway/modules/push/fcm/client.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
internal/sms-gateway/modules/push/upstream/client.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
internal/sms-gateway/modules/events/service.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
internal/sms-gateway/modules/settings/service.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
internal/sms-gateway/modules/push/service.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
internal/sms-gateway/modules/push/types.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
internal/sms-gateway/modules/messages/service.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
🧬 Code Graph Analysis (8)
internal/sms-gateway/handlers/upstream.go (4)
internal/sms-gateway/modules/events/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/sse/types.go (1)
Event(5-8)
internal/sms-gateway/modules/push/fcm/utils.go (1)
internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
internal/sms-gateway/modules/webhooks/service.go (2)
internal/sms-gateway/modules/events/service.go (2)
Service(23-36)NewService(38-73)internal/sms-gateway/modules/events/events.go (1)
NewWebhooksUpdatedEvent(13-15)
internal/sms-gateway/modules/push/fcm/client.go (2)
internal/sms-gateway/modules/push/types.go (1)
Event(16-16)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
internal/sms-gateway/modules/push/upstream/client.go (2)
internal/sms-gateway/modules/events/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
internal/sms-gateway/modules/settings/service.go (3)
internal/sms-gateway/modules/events/service.go (2)
Service(23-36)NewService(38-73)internal/sms-gateway/modules/push/service.go (1)
Service(37-50)internal/sms-gateway/modules/events/events.go (1)
NewSettingsUpdatedEvent(27-29)
internal/sms-gateway/modules/push/types.go (1)
internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
internal/sms-gateway/modules/messages/service.go (3)
internal/sms-gateway/modules/events/service.go (1)
Service(23-36)internal/sms-gateway/modules/push/service.go (1)
Service(37-50)internal/sms-gateway/modules/events/events.go (2)
NewMessageEnqueuedEvent(9-11)NewMessagesExportRequestedEvent(17-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (28)
internal/sms-gateway/handlers/upstream.go (1)
71-74: LGTM! Clean refactoring to use struct literal initialization.The change from constructor function to direct struct initialization aligns well with the new simplified event type structure. The use of
anys.ZeroDefaultmaintains the same defaulting behavior for the event type.internal/sms-gateway/modules/push/fcm/utils.go (1)
10-20: LGTM! Well-implemented utility function.The function correctly converts the event to FCM-compatible format by marshaling the data to JSON and structuring it appropriately. Error handling is proper with descriptive error wrapping.
internal/sms-gateway/modules/events/module.go (1)
10-29: LGTM! Proper Fx module setup with clean lifecycle management.The module correctly implements the Fx pattern with appropriate logger decoration and lifecycle hooks. The use of context cancellation for clean shutdown is well-implemented.
internal/sms-gateway/modules/webhooks/service.go (1)
9-9: LGTM! Clean migration to the events notification system.The changes consistently replace the push service dependency with the events service throughout the struct definitions, dependency injection, and method calls. The refactoring maintains the same notification behavior while integrating with the new unified events system.
Also applies to: 23-23, 34-34, 46-46, 125-125
internal/sms-gateway/modules/push/fcm/client.go (1)
10-10: LGTM! Good refactoring with improved error handling.The changes correctly update to use the new event type and implement better error handling. The use of
eventToMaphelper function and individual error collection for marshaling failures makes the service more resilient - it continues processing other messages even if one fails.Also applies to: 55-55, 58-66
internal/sms-gateway/modules/push/upstream/client.go (3)
13-13: LGTM! Clean import path update.The import path change from
domaintotypesaligns with the architectural refactoring to use the new event type system.
45-45: LGTM! Consistent type signature and field access updates.The method signature update and the change from method calls (
data.Event(),data.Data()) to direct field access (data.Type,data.Data) correctly reflects the newtypes.Eventstruct with exported fields.Also applies to: 51-52
87-88: LGTM! mapErrors method signature updated consistently.The method signature and parameter type changes are consistent with the overall refactoring to use
types.Event.internal/sms-gateway/modules/push/service.go (4)
8-8: LGTM! Import path updated consistently.The import change to use the
typespackage aligns with the overall refactoring.
114-114: LGTM! Clean refactoring to use types.Event.The method signature change from
*domain.Eventtotypes.Event(value instead of pointer) is more efficient for the simple struct. The internal storage as a pointer and direct field access for metrics are handled correctly.Also applies to: 123-123, 131-131
189-191: LGTM! sendAll method updated consistently.The
sendAllmethod correctly uses the newtypes.Eventtype in the map conversion.
136-180: Notify fully migrated to events serviceVerified that the old
push.Service.Notifyimplementation is commented out and all callers have been updated to useevents.Service.Notify.
- No remaining references to
pushSvc.Notifyin the codebase.internal/sms-gateway/modules/events/service.godefinesfunc (s *Service) Notify(userID string, deviceID *string, event *Event) error.- Webhooks, Settings and Messages modules now call
s.eventsSvc.Notify(...)instead of the push service.All Notify functionality has been successfully moved—no further action required.
internal/sms-gateway/modules/settings/service.go (4)
4-4: LGTM! Clean migration to events service.The import change from
pushtoeventsmodule reflects the architectural shift to a unified events notification system.
14-14: LGTM! Service dependencies updated consistently.The field changes from
PushSvctoEventsSvcin bothServiceParamsandServicestructs are consistent with the migration to the events service.Also applies to: 22-22
27-35: LGTM! Constructor updated correctly.The constructor properly initializes the events service dependency and maintains the same structure as before.
91-91: LGTM! Notification method updated to use events service.The call to
eventsSvc.Notifywithevents.NewSettingsUpdatedEvent()properly replaces the previous push service call while maintaining the same notification functionality.internal/sms-gateway/modules/events/service.go (4)
75-92: LGTM! Well-designed async notification interface.The
Notifymethod provides a clean interface with proper error handling for queue overflow. The non-blocking send with metrics tracking is appropriate for maintaining system responsiveness.
94-104: LGTM! Clean event processing loop.The
Runmethod implements a standard worker pattern with proper context cancellation handling.
106-150: LGTM! Comprehensive event processing with good error handling.The
processEventmethod handles device selection, dual delivery channels (push/SSE), and comprehensive error logging with metrics. The fallback from push to SSE when no push token is available is a solid design choice.
126-148: Event type compatibility confirmedAll three
Eventstructs use the same underlyingsmsgateway.PushEventTypefor the type field andmap[string]stringfor the data field. Converting fromevents.Eventto bothpush.Eventandsse.Eventpreserves all information without any type mismatches.internal/sms-gateway/modules/push/types.go (3)
6-6: LGTM! Import updated consistently.The import change to use the
typespackage aligns with the overall refactoring to the new event type system.
16-16: LGTM! Clean type alias for backward compatibility.The type alias
Event = types.Eventprovides a clean way to maintain compatibility while using the new event type system.
20-20: LGTM! Interface and struct updated consistently.Both the
clientinterface andeventWrapperstruct are updated consistently to use the newtypes.Eventtype.Also applies to: 26-26
internal/sms-gateway/modules/messages/service.go (5)
14-14: LGTM: Import updated for events module.The import change from push to events module aligns with the architectural shift to a more generic notification system.
43-45: LGTM: ServiceParams updated for events service.The replacement of
PushSvcwithEventsSvcin the dependency injection parameters is consistent with the architectural changes.
54-56: LGTM: Service struct updated for events service.The field replacement maintains the same dependency pattern while switching to the new events-based notification system.
77-79: LGTM: Constructor updated for events service.The constructor properly initializes the new
eventsSvcfield, maintaining consistency with the parameter changes.
220-226: Review the notification approach and error handling.The notification logic has been updated to use the events service, which is good. However, there are a few considerations:
Unconditional counter increment: The message counter is now incremented regardless of push token availability (which was previously checked). This appears intentional for the new architecture.
Notification parameters: The call now uses
userIDanddeviceIDinstead of push tokens, which aligns with the events service design.The error handling in the goroutine properly logs failures without blocking the main flow.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/config/module.go(3 hunks)internal/sms-gateway/modules/events/service.go(1 hunks)internal/sms-gateway/modules/sse/types.go(1 hunks)
🧠 Learnings (2)
internal/sms-gateway/modules/events/service.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
internal/config/module.go (2)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:89-92
Timestamp: 2025-07-22T00:34:31.871Z
Learning: In Go SSE implementations, when cleaning up connections with buffered channels, avoid closing the data channels directly as this can cause panics if concurrent goroutines attempt to write to them. Instead, use a separate close signal channel to notify writers that the connection should be terminated, allowing the data channels to be garbage collected naturally when the connection is removed.
🧬 Code Graph Analysis (1)
internal/config/module.go (2)
internal/sms-gateway/modules/sse/config.go (2)
Config(7-9)NewConfig(11-15)internal/config/config.go (2)
Config(10-16)HTTP(23-26)
🪛 GitHub Check: Test
internal/config/module.go
[failure] 36-36:
unknown field WriteTimeout in struct literal of type "github.com/capcom6/go-infra-fx/http".Config
🪛 GitHub Actions: docker-publish
internal/config/module.go
[error] 36-36: Build failed: unknown field WriteTimeout in struct literal of type "github.com/capcom6/go-infra-fx/http".Config
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sms-gateway/modules/sse/types.go
🧰 Additional context used
🧠 Learnings (2)
internal/sms-gateway/modules/events/service.go (1)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
internal/config/module.go (2)
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.742Z
Learning: In the SSE service Send method (internal/sms-gateway/modules/sse/service.go), blocking on full channels is intentional and acceptable. Timeout control is handled at the calling code level through context, not within the Send method itself. The user prefers this approach over adding a default case that would drop messages when channels are full.
Learnt from: capcom6
PR: #154
File: internal/sms-gateway/modules/sse/service.go:89-92
Timestamp: 2025-07-22T00:34:31.871Z
Learning: In Go SSE implementations, when cleaning up connections with buffered channels, avoid closing the data channels directly as this can cause panics if concurrent goroutines attempt to write to them. Instead, use a separate close signal channel to notify writers that the connection should be terminated, allowing the data channels to be garbage collected naturally when the connection is removed.
🧬 Code Graph Analysis (1)
internal/config/module.go (2)
internal/sms-gateway/modules/sse/config.go (2)
Config(7-9)NewConfig(11-15)internal/config/config.go (2)
Config(10-16)HTTP(23-26)
🪛 GitHub Check: Test
internal/config/module.go
[failure] 36-36:
unknown field WriteTimeout in struct literal of type "github.com/capcom6/go-infra-fx/http".Config
🪛 GitHub Actions: docker-publish
internal/config/module.go
[error] 36-36: Build failed: unknown field WriteTimeout in struct literal of type "github.com/capcom6/go-infra-fx/http".Config
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
internal/config/module.go (4)
11-11: LGTM: SSE module import added correctly.The import of the SSE module is properly added to support the new Server-Sent Events functionality.
29-29: Good practice: Making main config private.Adding
fx.Privateto the main config provider is a good practice to encapsulate the configuration within this module while still providing specific config types to other modules.
95-97: Excellent improvement: Using NewConfig() for proper initialization.Using
sse.NewConfig()instead of an empty struct ensures the SSE configuration is properly initialized with default values, addressing the previous review feedback.This approach is much better than the previous empty struct initialization as it ensures
keepAlivePeriodand other fields get their proper default values.
35-36: Verifyhttp.ConfigforWriteTimeout
TheWriteTimeoutsetting isn’t defined on thehttp.Configstruct in thego-infra-fxlibrary, which is why the build is failing. Please confirm by inspecting your version of the library (for example viago doc github.com/your-org/go-infra-fx/http.Configor by viewing the source in your module cache). IfWriteTimeouttruly isn’t supported, you’ll need to choose one of the following:
- Upgrade to a version of
go-infra-fxthat addsWriteTimeout.- Remove the
WriteTimeoutfield from your configuration.- Use an alternative mechanism provided by
go-infra-fx(e.g.ReadTimeout, custom middleware/transport, or context-based deadlines) to achieve the longer SSE write timeout.internal/sms-gateway/modules/events/service.go (6)
1-13: Clean package structure and imports.The imports are well-organized and all necessary dependencies are included for the service implementation.
15-21: Good use of constants for delivery types and failure reasons.The constants provide clear abstraction and help maintain consistency across the service implementation.
23-36: Well-structured service with proper dependency injection.The service struct properly encapsulates its dependencies and includes necessary fields for queue management and observability.
38-73: Robust constructor with proper metrics initialization.The constructor properly sets up Prometheus metrics with clear naming and initializes all service dependencies correctly.
75-92: Efficient non-blocking event notification with proper error handling.The method correctly handles queue full scenarios and maintains appropriate metrics tracking for observability.
94-104: Proper service worker implementation with context cancellation.The Run method correctly implements the event processing loop with graceful shutdown support and appropriate logging.
| func (s *Service) processEvent(wrapper eventWrapper) { | ||
| // Load devices from database | ||
| filters := []devices.SelectFilter{} | ||
| if wrapper.DeviceID != nil { | ||
| filters = append(filters, devices.WithID(*wrapper.DeviceID)) | ||
| } | ||
|
|
||
| devices, err := s.deviceSvc.Select(wrapper.UserID, filters...) | ||
| if err != nil { | ||
| s.logger.Error("Failed to select devices", zap.String("user_id", wrapper.UserID), zap.Error(err)) | ||
| return | ||
| } | ||
|
|
||
| if len(devices) == 0 { | ||
| s.logger.Info("No devices found for user", zap.String("user_id", wrapper.UserID)) | ||
| return | ||
| } | ||
|
|
||
| // Process each device | ||
| for _, device := range devices { | ||
| if device.PushToken != nil && *device.PushToken != "" { | ||
| // Device has push token, use push service | ||
| if err := s.pushSvc.Enqueue(*device.PushToken, push.Event{ | ||
| Type: wrapper.Event.eventType, | ||
| Data: wrapper.Event.data, | ||
| }); err != nil { | ||
| s.logger.Error("Failed to enqueue push notification", zap.String("user_id", wrapper.UserID), zap.String("device_id", device.ID), zap.Error(err)) | ||
| s.failedCounter.WithLabelValues(string(wrapper.Event.eventType), DeliveryTypePush, FailureReasonProviderFailed).Inc() | ||
| } else { | ||
| s.sentCounter.WithLabelValues(string(wrapper.Event.eventType), DeliveryTypePush).Inc() | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // No push token, use SSE service | ||
| if err := s.sseSvc.Send(device.ID, sse.Event{ | ||
| Type: wrapper.Event.eventType, | ||
| Data: wrapper.Event.data, | ||
| }); err != nil { | ||
| s.logger.Error("Failed to send SSE notification", zap.String("user_id", wrapper.UserID), zap.String("device_id", device.ID), zap.Error(err)) | ||
| s.failedCounter.WithLabelValues(string(wrapper.Event.eventType), DeliveryTypeSSE, FailureReasonProviderFailed).Inc() | ||
| } else { | ||
| s.sentCounter.WithLabelValues(string(wrapper.Event.eventType), DeliveryTypeSSE).Inc() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical bug: Early return prevents notification to all devices.
There's a return statement at line 137 that exits the method after processing the first device with a push token. This prevents other devices from receiving notifications.
Remove the return statement to ensure all devices receive notifications:
// Device has push token, use push service
if err := s.pushSvc.Enqueue(*device.PushToken, push.Event{
Type: wrapper.Event.eventType,
Data: wrapper.Event.data,
}); err != nil {
s.logger.Error("Failed to enqueue push notification", zap.String("user_id", wrapper.UserID), zap.String("device_id", device.ID), zap.Error(err))
s.failedCounter.WithLabelValues(string(wrapper.Event.eventType), DeliveryTypePush, FailureReasonProviderFailed).Inc()
} else {
s.sentCounter.WithLabelValues(string(wrapper.Event.eventType), DeliveryTypePush).Inc()
}
- returnThis ensures that events are delivered to all devices associated with a user, not just the first one with a push token.
📝 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.
| func (s *Service) processEvent(wrapper eventWrapper) { | |
| // Load devices from database | |
| filters := []devices.SelectFilter{} | |
| if wrapper.DeviceID != nil { | |
| filters = append(filters, devices.WithID(*wrapper.DeviceID)) | |
| } | |
| devices, err := s.deviceSvc.Select(wrapper.UserID, filters...) | |
| if err != nil { | |
| s.logger.Error("Failed to select devices", zap.String("user_id", wrapper.UserID), zap.Error(err)) | |
| return | |
| } | |
| if len(devices) == 0 { | |
| s.logger.Info("No devices found for user", zap.String("user_id", wrapper.UserID)) | |
| return | |
| } | |
| // Process each device | |
| for _, device := range devices { | |
| if device.PushToken != nil && *device.PushToken != "" { | |
| // Device has push token, use push service | |
| if err := s.pushSvc.Enqueue(*device.PushToken, push.Event{ | |
| Type: wrapper.Event.eventType, | |
| Data: wrapper.Event.data, | |
| }); err != nil { | |
| s.logger.Error("Failed to enqueue push notification", zap.String("user_id", wrapper.UserID), zap.String("device_id", device.ID), zap.Error(err)) | |
| s.failedCounter.WithLabelValues(string(wrapper.Event.eventType), DeliveryTypePush, FailureReasonProviderFailed).Inc() | |
| } else { | |
| s.sentCounter.WithLabelValues(string(wrapper.Event.eventType), DeliveryTypePush).Inc() | |
| } | |
| return | |
| } | |
| // No push token, use SSE service | |
| if err := s.sseSvc.Send(device.ID, sse.Event{ | |
| Type: wrapper.Event.eventType, | |
| Data: wrapper.Event.data, | |
| }); err != nil { | |
| s.logger.Error("Failed to send SSE notification", zap.String("user_id", wrapper.UserID), zap.String("device_id", device.ID), zap.Error(err)) | |
| s.failedCounter.WithLabelValues(string(wrapper.Event.eventType), DeliveryTypeSSE, FailureReasonProviderFailed).Inc() | |
| } else { | |
| s.sentCounter.WithLabelValues(string(wrapper.Event.eventType), DeliveryTypeSSE).Inc() | |
| } | |
| } | |
| } | |
| func (s *Service) processEvent(wrapper eventWrapper) { | |
| // Load devices from database | |
| filters := []devices.SelectFilter{} | |
| if wrapper.DeviceID != nil { | |
| filters = append(filters, devices.WithID(*wrapper.DeviceID)) | |
| } | |
| devices, err := s.deviceSvc.Select(wrapper.UserID, filters...) | |
| if err != nil { | |
| s.logger.Error("Failed to select devices", zap.String("user_id", wrapper.UserID), zap.Error(err)) | |
| return | |
| } | |
| if len(devices) == 0 { | |
| s.logger.Info("No devices found for user", zap.String("user_id", wrapper.UserID)) | |
| return | |
| } | |
| // Process each device | |
| for _, device := range devices { | |
| if device.PushToken != nil && *device.PushToken != "" { | |
| // Device has push token, use push service | |
| if err := s.pushSvc.Enqueue(*device.PushToken, push.Event{ | |
| Type: wrapper.Event.eventType, | |
| Data: wrapper.Event.data, | |
| }); err != nil { | |
| s.logger.Error("Failed to enqueue push notification", zap.String("user_id", wrapper.UserID), zap.String("device_id", device.ID), zap.Error(err)) | |
| s.failedCounter.WithLabelValues(string(wrapper.Event.eventType), DeliveryTypePush, FailureReasonProviderFailed).Inc() | |
| } else { | |
| s.sentCounter.WithLabelValues(string(wrapper.Event.eventType), DeliveryTypePush).Inc() | |
| } | |
| } | |
| // No push token, use SSE service | |
| if err := s.sseSvc.Send(device.ID, sse.Event{ | |
| Type: wrapper.Event.eventType, | |
| Data: wrapper.Event.data, | |
| }); err != nil { | |
| s.logger.Error("Failed to send SSE notification", zap.String("user_id", wrapper.UserID), zap.String("device_id", device.ID), zap.Error(err)) | |
| s.failedCounter.WithLabelValues(string(wrapper.Event.eventType), DeliveryTypeSSE, FailureReasonProviderFailed).Inc() | |
| } else { | |
| s.sentCounter.WithLabelValues(string(wrapper.Event.eventType), DeliveryTypeSSE).Inc() | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/sms-gateway/modules/events/service.go between lines 106 and 151,
there is a return statement at line 137 inside the loop that processes devices
which causes the function to exit after sending a notification to the first
device with a push token. To fix this, remove the return statement so the loop
continues and notifications are sent to all devices associated with the user.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/swagger/docs/swagger.json (1)
1309-1315: Changes mirror those inswagger.yaml; comments already provided there.
🧹 Nitpick comments (3)
pkg/swagger/docs/swagger.yaml (3)
432-459: NewMobilePatchMessageItemduplicates existing structuresThe new schema mostly repeats the shape of
GetMessageResponse(minus some fields).
To avoid drift, consider embedding / referencing an existing DTO or usingallOfto extend a base “patchable” message schema.+smsgateway.MobilePatchMessageItem: + allOf: + - $ref: '#/definitions/smsgateway.GetMessageResponse' + required: + - id + - recipients + - stateReduces duplication and keeps the spec DRY.
1290-1313: Document SSE-specific response headers for/mobile/v1/eventsSSE clients usually rely on:
Cache-Control: no-cacheConnection: keep-aliveContent-Type: text/event-streamAdding them under
responses.200.headersimproves generated client code and clarifies behaviour."200": description: Event stream headers: + Cache-Control: + type: string + description: should be set to `no-cache` + Connection: + type: string + description: should be `keep-alive` schema: type: object
1348-1350: Parameter description still singular
description: New message stateis misleading now that the body is an array of states.- description: New message state + description: List of message state updates
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.mod(1 hunks)internal/sms-gateway/handlers/events/mobile.go(1 hunks)internal/sms-gateway/handlers/messages/3rdparty.go(3 hunks)pkg/swagger/docs/swagger.json(4 hunks)pkg/swagger/docs/swagger.yaml(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- internal/sms-gateway/handlers/events/mobile.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build / Docker image (linux/arm64)
🔇 Additional comments (4)
pkg/swagger/docs/swagger.yaml (1)
119-123: Backend must now returnid— check serializers / DB queries
idwas added to therequiredlist ofsmsgateway.GetMessageResponse.
If any handler still omits this field (e.g. pre-existing JSON marshalling withomitempty), clients will now see schema/validation errors.Consider grepping the handlers for
GetMessageResponseand ensuring the struct tag does not includeomitemptyforID, and that the DB query populates it.internal/sms-gateway/handlers/messages/3rdparty.go (3)
48-57: LGTM! Improved Swagger annotation formatting.The alignment and spacing improvements make the API documentation more readable while preserving all functional aspects of the annotations.
182-182: LGTM! Consistent formatting improvement.The @router annotation formatting aligns with the improvements made to other Swagger annotations in this file.
219-219: LGTM! Consistent Swagger annotation formatting.The formatting improvement maintains consistency with other annotation updates in this file while preserving the correct route specification.
Summary by CodeRabbit
New Features
/eventsendpoint./eventsendpoint.Chores