[build] migrate to GoReleaser #189
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
WalkthroughRemoved legacy Docker build/publish workflows; added PR/release CI using GoReleaser; introduced golangci-lint and GoReleaser configs and Dockerfile.goreleaser; large refactor across modules (module factories, push client interface, messages/auth/services, cache/pubsub, handlers) with many signature and error-surface changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant GH as "GitHub Actions"
participant GR as "GoReleaser"
participant GHCR as "ghcr.io"
participant S3 as "S3"
Dev->>GH: push tag v*
GH->>GR: run goreleaser (release)
GR->>GR: build binaries & docker images (multi-arch)
GR->>GHCR: push images & manifests
GR->>S3: upload artifacts
sequenceDiagram
autonumber
participant HTTP as "Client"
participant Handler as "Auth Handler"
participant AuthSvc as "Auth Service"
participant Repo as "Auth Repo"
participant Cache as "Auth Cache"
HTTP->>Handler: Login(credentials)
Handler->>AuthSvc: AuthorizeUser(...)
AuthSvc->>Cache: Get(userID)
alt cache hit
Cache-->>AuthSvc: *User
else cache miss
AuthSvc->>Repo: GetByLogin(...)
Repo-->>AuthSvc: *User
AuthSvc->>Cache: Set(*User)
end
AuthSvc-->>Handler: *User
Handler-->>HTTP: token
sequenceDiagram
autonumber
participant PushSvc as "Push Service"
participant Factory as "newClient(config)"
participant Client as "client.Client (FCM/Upstream)"
participant Provider as "External Provider"
PushSvc->>Factory: request client
Factory-->>PushSvc: client.Client
PushSvc->>Client: Open(ctx)
Client->>Provider: init
PushSvc->>Client: Send(ctx, []client.Message)
Client->>Provider: deliver messages
Provider-->>Client: responses/errors
Client-->>PushSvc: per-message errors
PushSvc->>Client: Close(ctx)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/sms-gateway/modules/push/upstream/client.go (1)
68-91: Restore upstream options when building the request.
c.optionsis now completely unused, so any upstream configuration (API token, base URL override, etc.) injected through the options map never makes it into the outgoing request. Deployments that relied on those values to authenticate requests (e.g., Authorization header or custom endpoint) will start getting 401/404 responses even though their config hasn’t changed. Please reinstate the options handling (e.g., set headers/query params or derive the URL from the configured values) so existing upstream integrations keep working.internal/sms-gateway/modules/settings/repository.go (1)
33-48: Restore theWHEREplaceholder to avoid broken queries.
Where("user_id", settings.UserID)drops the= ?placeholder, so GORM buildsWHERE user_id, which either errors (PostgreSQL) or matches every non-empty row (MySQL). That breaks updates and defeats the per-user lock.Apply this fix:
- if err := tx.Clauses(clause.Locking{Strength: clause.LockingStrengthUpdate}).Where("user_id", settings.UserID).Limit(1).Find(source).Error; err != nil { + if err := tx.Clauses(clause.Locking{Strength: clause.LockingStrengthUpdate}).Where("user_id = ?", settings.UserID).Limit(1).Find(source).Error; err != nil {
🧹 Nitpick comments (6)
internal/config/config.go (1)
28-28: Cosmetic whitespace changes for alignment.These spacing adjustments in the struct tags have no functional impact. If vertical alignment is desired, consider applying a consistent formatting style across all struct fields in this file (or using an automated formatter like
gofmtorgofumpt).Also applies to: 77-77
internal/sms-gateway/modules/messages/cache.go (1)
63-63: Consider clarifying the nolint comment.The comment could be more explicit about what the "special meaning" is (e.g., "cache miss is not an error") to help future maintainers understand the intent.
- return nil, nil //nolint:nilnil //special meaning + return nil, nil //nolint:nilnil // cache miss is not an errorinternal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
94-100: Consider storing pointers in Locals to reduce allocations.The current pattern stores
models.Uservalues in Locals (lines 53, 80) then converts to*models.Userby taking the address of a local copy. This causes the value to escape to the heap.For better performance, consider storing
*models.Userdirectly in Locals throughout the flow, eliminating unnecessary allocations.Example refactor for NewBasic (line 53):
- c.Locals(localsUser, user) + c.Locals(localsUser, &user)And simplify GetUser:
func GetUser(c *fiber.Ctx) *models.User { - if user, ok := c.Locals(localsUser).(models.User); ok { - return &user + if user, ok := c.Locals(localsUser).(*models.User); ok { + return user } - return nil }internal/sms-gateway/pubsub/pubsub.go (1)
29-47: LGTM!The refactoring to an end-return pattern improves consistency with:
- Single return point
- Uniform error wrapping with "can't create pubsub: %w"
- Cleaner control flow
For slightly better clarity on line 32, you could use:
- pubSub, err = pubsub.NewMemory(opts...), nil + pubSub = pubsub.NewMemory(opts...) + err = nilThough the current form is valid and commonly used in Go.
.github/workflows/pr.yml (1)
75-76: Consider documenting the RELEASE_ID epoch date.The RELEASE_ID calculation uses
2022-06-15as an epoch date. Consider adding a comment explaining the significance of this date or linking to documentation, as this calculation will be used for versioning.Apply this diff:
+ # RELEASE_ID: Days since project inception (2022-06-15) - name: Set RELEASE_ID env run: echo RELEASE_ID=$(( ($(date +%s) - $(date -d "2022-06-15" +%s)) / 86400 )) >> ${GITHUB_ENV}internal/sms-gateway/handlers/messages/mobile.go (1)
35-43: Consider aligning constructor patterns across mobile controllers.This constructor uses fx.In-based dependency injection, while
internal/sms-gateway/handlers/settings/mobile.go(line 23-37) uses explicit parameters. The PR summary mentions "converting module variables to factory functions," suggesting a move toward explicit dependencies. Consider standardizing the constructor pattern across all mobile controllers for consistency.
📜 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 (74)
.github/workflows/docker-build.yml(0 hunks).github/workflows/docker-publish.yml(0 hunks).github/workflows/go.yml(3 hunks).github/workflows/pr.yml(1 hunks).github/workflows/release.yml(2 hunks).gitignore(1 hunks).golangci.yml(1 hunks).goreleaser.yaml(1 hunks)Dockerfile.goreleaser(1 hunks)Makefile(2 hunks)internal/config/config.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/cache/errors.go(1 hunks)internal/sms-gateway/cache/factory.go(2 hunks)internal/sms-gateway/handlers/base/handler_test.go(5 hunks)internal/sms-gateway/handlers/converters/messages.go(1 hunks)internal/sms-gateway/handlers/devices/3rdparty.go(3 hunks)internal/sms-gateway/handlers/events/mobile.go(1 hunks)internal/sms-gateway/handlers/health.go(2 hunks)internal/sms-gateway/handlers/logs/3rdparty.go(2 hunks)internal/sms-gateway/handlers/messages/3rdparty.go(8 hunks)internal/sms-gateway/handlers/messages/mobile.go(2 hunks)internal/sms-gateway/handlers/messages/params.go(4 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(2 hunks)internal/sms-gateway/handlers/mobile.go(7 hunks)internal/sms-gateway/handlers/root.go(1 hunks)internal/sms-gateway/handlers/settings/3rdparty.go(4 hunks)internal/sms-gateway/handlers/settings/mobile.go(2 hunks)internal/sms-gateway/handlers/upstream.go(2 hunks)internal/sms-gateway/handlers/webhooks/3rdparty.go(4 hunks)internal/sms-gateway/handlers/webhooks/mobile.go(2 hunks)internal/sms-gateway/models/models.go(2 hunks)internal/sms-gateway/modules/auth/errors.go(1 hunks)internal/sms-gateway/modules/auth/module.go(1 hunks)internal/sms-gateway/modules/auth/repository.go(1 hunks)internal/sms-gateway/modules/auth/service.go(6 hunks)internal/sms-gateway/modules/auth/types.go(1 hunks)internal/sms-gateway/modules/devices/service.go(2 hunks)internal/sms-gateway/modules/events/errors.go(1 hunks)internal/sms-gateway/modules/events/metrics.go(3 hunks)internal/sms-gateway/modules/events/module.go(1 hunks)internal/sms-gateway/modules/events/service.go(6 hunks)internal/sms-gateway/modules/events/types.go(2 hunks)internal/sms-gateway/modules/health/service.go(1 hunks)internal/sms-gateway/modules/messages/cache.go(2 hunks)internal/sms-gateway/modules/messages/converters.go(1 hunks)internal/sms-gateway/modules/messages/errors.go(1 hunks)internal/sms-gateway/modules/messages/models.go(6 hunks)internal/sms-gateway/modules/messages/module.go(1 hunks)internal/sms-gateway/modules/messages/repository.go(5 hunks)internal/sms-gateway/modules/messages/repository_filter.go(2 hunks)internal/sms-gateway/modules/messages/service.go(10 hunks)internal/sms-gateway/modules/push/client.go(1 hunks)internal/sms-gateway/modules/push/client/types.go(1 hunks)internal/sms-gateway/modules/push/fcm/client.go(4 hunks)internal/sms-gateway/modules/push/fcm/errors.go(1 hunks)internal/sms-gateway/modules/push/fcm/utils.go(1 hunks)internal/sms-gateway/modules/push/module.go(2 hunks)internal/sms-gateway/modules/push/service.go(11 hunks)internal/sms-gateway/modules/push/types.go(3 hunks)internal/sms-gateway/modules/push/upstream/client.go(5 hunks)internal/sms-gateway/modules/settings/models.go(1 hunks)internal/sms-gateway/modules/settings/module.go(1 hunks)internal/sms-gateway/modules/settings/repository.go(5 hunks)internal/sms-gateway/modules/sse/service.go(3 hunks)internal/sms-gateway/modules/webhooks/models.go(1 hunks)internal/sms-gateway/openapi/docs.go(5 hunks)internal/sms-gateway/pubsub/module.go(2 hunks)internal/sms-gateway/pubsub/pubsub.go(1 hunks)pkg/cache/memory_edge_test.go(1 hunks)pkg/cache/redis.go(2 hunks)pkg/pubsub/redis.go(2 hunks)test/e2e/mobile_test.go(1 hunks)test/e2e/utils_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/docker-publish.yml
- .github/workflows/docker-build.yml
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-10T23:45:45.502Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: internal/sms-gateway/online/module.go:14-16
Timestamp: 2025-09-10T23:45:45.502Z
Learning: fx.Decorate in uber-go/fx creates module-scoped decorated dependencies, not global modifications. Using fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("module_name") }) is a correct pattern for creating module-specific loggers that are scoped only to that module's dependency graph.
Applied to files:
internal/sms-gateway/modules/auth/module.gointernal/sms-gateway/modules/settings/module.gointernal/sms-gateway/pubsub/module.gointernal/sms-gateway/modules/events/module.go
📚 Learning: 2025-07-22T00:31:28.796Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.796Z
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.
Applied to files:
internal/sms-gateway/modules/events/service.gointernal/sms-gateway/modules/sse/service.go
📚 Learning: 2025-09-05T08:37:28.077Z
Learnt from: eznix86
Repo: android-sms-gateway/server PR: 172
File: deployments/kubernetes/sms-gateway-server/Chart.yaml:9-10
Timestamp: 2025-09-05T08:37:28.077Z
Learning: In the android-sms-gateway/server project, the Helm chart's appVersion in Chart.yaml is set to "latest" as a placeholder because CI automatically updates it with the actual version from git tags during the release process via the bump-chart-app-version.yaml workflow.
Applied to files:
.goreleaser.yaml
📚 Learning: 2025-09-10T23:53:42.006Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Applied to files:
pkg/cache/redis.go
📚 Learning: 2025-07-22T00:34:31.896Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 154
File: internal/sms-gateway/modules/sse/service.go:89-92
Timestamp: 2025-07-22T00:34:31.896Z
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.
Applied to files:
internal/sms-gateway/modules/sse/service.go
🧬 Code graph analysis (42)
internal/sms-gateway/cache/errors.go (1)
internal/sms-gateway/modules/push/service.go (1)
New(43-73)
internal/sms-gateway/modules/health/service.go (1)
internal/sms-gateway/modules/health/types.go (3)
HealthProvider(66-72)Checks(64-64)CheckResult(27-30)
internal/sms-gateway/modules/auth/module.go (2)
internal/sms-gateway/app.go (2)
Module(36-62)Run(64-75)internal/sms-gateway/modules/events/module.go (1)
Module(10-39)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
internal/sms-gateway/models/models.go (1)
User(17-23)
internal/sms-gateway/handlers/events/mobile.go (2)
internal/sms-gateway/handlers/messages/mobile.go (1)
MobileController(29-33)internal/sms-gateway/models/models.go (1)
Device(32-43)
internal/sms-gateway/modules/push/client.go (5)
internal/sms-gateway/modules/push/service.go (2)
Config(23-30)New(43-73)internal/sms-gateway/modules/push/client/types.go (1)
Client(9-13)internal/sms-gateway/modules/push/fcm/client.go (2)
Client(14-19)New(21-27)internal/sms-gateway/modules/push/upstream/client.go (2)
Client(22-27)New(29-35)internal/sms-gateway/modules/push/types.go (3)
Mode(10-10)ModeFCM(13-13)ModeUpstream(14-14)
internal/sms-gateway/modules/auth/repository.go (1)
internal/sms-gateway/models/models.go (1)
User(17-23)
internal/sms-gateway/modules/messages/models.go (1)
internal/sms-gateway/models/models.go (1)
SoftDeletableModel(12-15)
internal/sms-gateway/modules/devices/service.go (3)
internal/sms-gateway/models/models.go (1)
Device(32-43)internal/sms-gateway/modules/devices/repository_filter.go (4)
SelectFilter(9-9)WithUserID(23-27)WithID(11-15)ActiveWithin(29-33)internal/sms-gateway/modules/devices/repository.go (1)
ErrNotFound(13-13)
internal/sms-gateway/modules/settings/models.go (3)
internal/sms-gateway/models/models.go (1)
TimedModel(7-10)internal/sms-gateway/modules/messages/models.go (1)
Migrate(136-141)internal/sms-gateway/modules/webhooks/models.go (1)
Migrate(25-27)
internal/sms-gateway/modules/events/service.go (7)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)internal/sms-gateway/modules/events/errors.go (1)
ErrValidationFailed(6-6)internal/sms-gateway/modules/events/metrics.go (6)
DeliveryTypeUnknown(20-20)FailureReasonPublishError(23-23)EventTypeUnknown(26-26)FailureReasonSerializationError(22-22)DeliveryTypePush(18-18)FailureReasonProviderFailed(24-24)internal/sms-gateway/modules/events/types.go (1)
Event(10-13)internal/sms-gateway/modules/push/client/types.go (1)
Event(20-23)internal/sms-gateway/modules/push/types.go (1)
Event(17-17)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)
internal/sms-gateway/modules/settings/repository.go (1)
internal/sms-gateway/modules/settings/models.go (1)
DeviceSettings(10-17)
internal/sms-gateway/modules/push/module.go (4)
internal/sms-gateway/modules/push/fcm/client.go (2)
New(21-27)Client(14-19)internal/sms-gateway/modules/push/service.go (1)
New(43-73)internal/sms-gateway/modules/push/upstream/client.go (2)
New(29-35)Client(22-27)internal/sms-gateway/modules/push/client/types.go (1)
Client(9-13)
internal/sms-gateway/modules/settings/module.go (3)
internal/sms-gateway/app.go (1)
Module(36-62)internal/sms-gateway/modules/messages/module.go (1)
Module(11-35)internal/sms-gateway/handlers/module.go (1)
Module(15-39)
internal/sms-gateway/handlers/upstream.go (1)
internal/sms-gateway/handlers/config.go (1)
Config(3-11)
internal/sms-gateway/modules/messages/cache.go (2)
internal/sms-gateway/modules/messages/domain.go (1)
MessageStateOut(38-44)pkg/cache/options.go (2)
WithTTL(22-30)AndSetTTL(64-68)
internal/sms-gateway/handlers/settings/mobile.go (3)
internal/sms-gateway/handlers/events/mobile.go (2)
NewMobileController(19-27)MobileController(13-17)internal/sms-gateway/handlers/messages/mobile.go (2)
NewMobileController(35-43)MobileController(29-33)internal/sms-gateway/handlers/webhooks/mobile.go (2)
NewMobileController(21-33)MobileController(15-19)
internal/sms-gateway/modules/push/fcm/errors.go (2)
internal/sms-gateway/modules/push/fcm/client.go (1)
New(21-27)internal/sms-gateway/modules/push/upstream/client.go (1)
New(29-35)
internal/sms-gateway/modules/push/fcm/utils.go (2)
internal/sms-gateway/modules/push/client/types.go (1)
Event(20-23)internal/sms-gateway/modules/push/types.go (1)
Event(17-17)
internal/sms-gateway/pubsub/pubsub.go (2)
pkg/pubsub/memory.go (1)
NewMemory(24-36)pkg/pubsub/redis.go (2)
NewRedis(42-76)RedisConfig(15-27)
internal/sms-gateway/handlers/devices/3rdparty.go (5)
internal/sms-gateway/modules/devices/service.go (1)
Service(31-40)internal/sms-gateway/handlers/logs/3rdparty.go (2)
NewThirdPartyController(24-31)ThirdPartyController(20-22)internal/sms-gateway/handlers/messages/3rdparty.go (2)
NewThirdPartyController(44-53)ThirdPartyController(37-42)internal/sms-gateway/handlers/settings/3rdparty.go (2)
NewThirdPartyController(35-44)ThirdPartyController(28-33)internal/sms-gateway/handlers/webhooks/3rdparty.go (2)
NewThirdPartyController(32-40)ThirdPartyController(26-30)
internal/sms-gateway/modules/sse/service.go (1)
internal/sms-gateway/modules/sse/metrics.go (1)
ErrorTypeWriteFailure(21-21)
internal/sms-gateway/modules/messages/repository.go (3)
internal/sms-gateway/modules/messages/repository_filter.go (3)
SelectFilter(16-23)SelectOptions(51-62)Order(7-7)internal/sms-gateway/modules/messages/models.go (2)
Message(35-55)ProcessingStatePending(16-16)internal/sms-gateway/modules/messages/errors.go (1)
ErrLockFailed(6-6)
internal/sms-gateway/handlers/mobile.go (1)
internal/sms-gateway/models/models.go (2)
User(17-23)Device(32-43)
internal/sms-gateway/app.go (6)
internal/config/module.go (1)
Module(22-132)internal/sms-gateway/handlers/module.go (1)
Module(15-39)internal/sms-gateway/modules/sse/module.go (1)
Module(10-29)internal/sms-gateway/modules/devices/module.go (1)
Module(16-32)internal/sms-gateway/modules/cleaner/module.go (1)
Module(28-36)internal/sms-gateway/modules/metrics/module.go (1)
Module(9-17)
internal/sms-gateway/handlers/settings/3rdparty.go (5)
internal/sms-gateway/handlers/devices/3rdparty.go (2)
NewThirdPartyController(24-36)ThirdPartyController(18-22)internal/sms-gateway/handlers/logs/3rdparty.go (2)
NewThirdPartyController(24-31)ThirdPartyController(20-22)internal/sms-gateway/handlers/messages/3rdparty.go (2)
NewThirdPartyController(44-53)ThirdPartyController(37-42)internal/sms-gateway/handlers/webhooks/3rdparty.go (2)
NewThirdPartyController(32-40)ThirdPartyController(26-30)internal/sms-gateway/modules/settings/models.go (1)
DeviceSettings(10-17)
internal/sms-gateway/modules/push/fcm/client.go (3)
internal/sms-gateway/modules/push/fcm/errors.go (1)
ErrInitializationFailed(6-6)internal/sms-gateway/modules/push/client/types.go (2)
Client(9-13)Message(15-18)internal/sms-gateway/modules/push/upstream/client.go (1)
Client(22-27)
internal/sms-gateway/handlers/webhooks/3rdparty.go (4)
internal/sms-gateway/handlers/logs/3rdparty.go (2)
NewThirdPartyController(24-31)ThirdPartyController(20-22)internal/sms-gateway/handlers/messages/3rdparty.go (2)
NewThirdPartyController(44-53)ThirdPartyController(37-42)internal/sms-gateway/handlers/settings/3rdparty.go (2)
NewThirdPartyController(35-44)ThirdPartyController(28-33)internal/sms-gateway/modules/webhooks/models.go (1)
Webhook(9-23)
internal/sms-gateway/handlers/messages/3rdparty.go (8)
internal/sms-gateway/handlers/devices/3rdparty.go (2)
NewThirdPartyController(24-36)ThirdPartyController(18-22)internal/sms-gateway/handlers/logs/3rdparty.go (2)
NewThirdPartyController(24-31)ThirdPartyController(20-22)internal/sms-gateway/handlers/settings/3rdparty.go (2)
NewThirdPartyController(35-44)ThirdPartyController(28-33)internal/sms-gateway/handlers/webhooks/3rdparty.go (2)
NewThirdPartyController(32-40)ThirdPartyController(26-30)internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/modules/devices/repository.go (1)
ErrNotFound(13-13)internal/sms-gateway/modules/messages/service.go (1)
EnqueueOptions(27-29)internal/sms-gateway/modules/messages/errors.go (1)
ValidationError(9-9)
internal/sms-gateway/modules/push/types.go (1)
internal/sms-gateway/modules/push/client/types.go (1)
Event(20-23)
internal/sms-gateway/modules/push/service.go (2)
internal/sms-gateway/modules/push/client/types.go (3)
Client(9-13)Event(20-23)Message(15-18)internal/sms-gateway/modules/push/types.go (1)
Event(17-17)
internal/sms-gateway/cache/factory.go (2)
internal/sms-gateway/cache/errors.go (1)
ErrInvalidConfig(6-6)internal/config/config.go (1)
Cache(81-83)
internal/sms-gateway/handlers/health.go (1)
internal/sms-gateway/modules/health/types.go (1)
CheckDetail(51-61)
internal/sms-gateway/handlers/messages/params.go (1)
internal/sms-gateway/modules/messages/repository_filter.go (3)
SelectFilter(16-23)SelectOptions(51-62)Order(7-7)
internal/sms-gateway/handlers/webhooks/mobile.go (4)
internal/sms-gateway/modules/webhooks/service.go (1)
Service(28-37)internal/sms-gateway/handlers/events/mobile.go (2)
NewMobileController(19-27)MobileController(13-17)internal/sms-gateway/handlers/messages/mobile.go (2)
NewMobileController(35-43)MobileController(29-33)internal/sms-gateway/handlers/settings/mobile.go (2)
NewMobileController(23-37)MobileController(16-21)
internal/sms-gateway/modules/push/upstream/client.go (3)
internal/sms-gateway/modules/push/fcm/client.go (2)
New(21-27)Client(14-19)internal/sms-gateway/modules/push/service.go (1)
New(43-73)internal/sms-gateway/modules/push/client/types.go (2)
Client(9-13)Message(15-18)
internal/sms-gateway/handlers/logs/3rdparty.go (4)
internal/sms-gateway/handlers/devices/3rdparty.go (2)
NewThirdPartyController(24-36)ThirdPartyController(18-22)internal/sms-gateway/handlers/messages/3rdparty.go (2)
NewThirdPartyController(44-53)ThirdPartyController(37-42)internal/sms-gateway/handlers/settings/3rdparty.go (2)
NewThirdPartyController(35-44)ThirdPartyController(28-33)internal/sms-gateway/handlers/webhooks/3rdparty.go (2)
NewThirdPartyController(32-40)ThirdPartyController(26-30)
internal/sms-gateway/handlers/messages/mobile.go (5)
internal/sms-gateway/handlers/events/mobile.go (2)
NewMobileController(19-27)MobileController(13-17)internal/sms-gateway/handlers/settings/mobile.go (2)
NewMobileController(23-37)MobileController(16-21)internal/sms-gateway/handlers/webhooks/mobile.go (2)
NewMobileController(21-33)MobileController(15-19)internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/models/models.go (1)
Device(32-43)
internal/sms-gateway/modules/messages/repository_filter.go (2)
internal/sms-gateway/modules/webhooks/repository_filter.go (2)
WithExtID(7-11)WithDeviceID(41-46)internal/sms-gateway/modules/messages/models.go (1)
ProcessingState(12-12)
internal/sms-gateway/modules/events/module.go (2)
internal/sms-gateway/app.go (2)
Module(36-62)Run(64-75)internal/sms-gateway/modules/auth/module.go (1)
Module(10-32)
internal/sms-gateway/modules/auth/service.go (6)
internal/sms-gateway/online/service.go (2)
New(28-37)Service(14-17)internal/sms-gateway/models/models.go (4)
User(17-23)NewUser(25-30)Device(32-43)NewDevice(45-50)internal/sms-gateway/modules/devices/service.go (1)
Service(31-40)internal/sms-gateway/modules/auth/types.go (1)
OneTimeCode(15-18)pkg/crypto/passwords.go (2)
MakeBCryptHash(14-20)CompareBCryptHash(22-27)internal/sms-gateway/modules/auth/errors.go (1)
ErrAuthorizationFailed(6-6)
internal/sms-gateway/modules/messages/service.go (3)
internal/sms-gateway/modules/messages/repository_filter.go (3)
Order(7-7)SelectFilter(16-23)SelectOptions(51-62)internal/sms-gateway/modules/messages/models.go (2)
MessageRecipient(111-117)ProcessingState(12-12)internal/sms-gateway/modules/messages/errors.go (1)
ValidationError(9-9)
🪛 GitHub Check: CodeQL
.github/workflows/pr.yml
[warning] 13-42: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
⏰ 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). (5)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (79)
pkg/cache/memory_edge_test.go (1)
252-257: Approved: Multi-line formatting improves readability.The reformatted
t.Logf()call follows idiomatic Go conventions for long function invocations. Breaking arguments across separate lines enhances readability without altering behavior or logic. This aligns well with the broader formatting and linting improvements in the PR.internal/sms-gateway/handlers/upstream.go (1)
97-103: LGTM! Good refactor.Extracting rate limiting values into named constants improves code clarity and maintainability.
internal/sms-gateway/modules/messages/cache.go (3)
30-30: LGTM: Idiomatic parameter naming.The change from
IDtoidfollows Go conventions for unexported parameters.
46-50: LGTM: Improved error wrapping.The explicit error wrapping with context and explicit nil return improve debuggability and code clarity.
53-53: LGTM: Consistent naming and error handling.The parameter naming aligns with the
Setmethod, and the error wrapping provides helpful context. UsingjsonErrappropriately avoids variable shadowing.Also applies to: 57-59, 67-68
internal/sms-gateway/cache/errors.go (1)
5-6: LGTM: Sentinel error for configuration validation.The
ErrInvalidConfigsentinel error follows Go best practices and enables callers to useerrors.Is()for robust error checking.internal/sms-gateway/cache/factory.go (3)
31-31: LGTM: Proper error wrapping with sentinel error.Wrapping URL parse errors with
ErrInvalidConfigenables callers to useerrors.Is()for configuration error checking.
53-53: LGTM: Proper error wrapping with sentinel error.Wrapping invalid scheme errors with
ErrInvalidConfigmaintains consistency with error handling at Line 31.
37-39: No action required. Review comment is incorrect.The memory backend actually creates separate cache instances for each
factory.New()call. Sincecache.NewMemory(0)returns a new*memoryCachewith its ownitemsmap, theeventsandblacklistcaches are completely isolated—no collisions occur.The
nameparameter being ignored is inconsequential here: the Redis backend uses it for prefixing because Redis is a shared singleton service requiring namespace separation. The memory backend achieves the same isolation naturally through separate instances. The suggestedcache.NewMemoryWithPrefixconstructor doesn't exist in the codebase and isn't needed.Likely an incorrect or invalid review comment.
test/e2e/utils_test.go (1)
21-25: LGTM!The multi-line formatting improves readability for this function signature with multiple parameters.
internal/sms-gateway/handlers/root.go (1)
28-28: LGTM!The lint directive is appropriately applied for middleware that propagates errors wrapped elsewhere in the chain.
.gitignore (1)
53-53: LGTM!This correctly implements the macOS Icon file pattern, which requires carriage return characters per the comment above.
internal/sms-gateway/modules/events/metrics.go (1)
8-8: LGTM!The trailing periods improve comment consistency and professionalism throughout the file.
Also applies to: 29-29, 36-36, 60-60, 65-65, 70-70
internal/sms-gateway/modules/webhooks/models.go (1)
10-12: LGTM!The normalized struct tag spacing improves consistency across the model definition.
Also applies to: 16-17
test/e2e/mobile_test.go (1)
193-195: LGTM!The multi-line formatting improves readability of the base64 encoding expression.
internal/sms-gateway/modules/messages/module.go (1)
37-40: Clarify the need forinit()with migration registration.The addition of an
init()function with a side effect (registering a migration) is generally discouraged in Go. Given that this PR is refactoring modules to useModule() fx.Optionpatterns (as seen in other modules), consider registering the migration within theModule()function usingfx.Invokeinstead.Could you clarify:
- What "backward compatibility" this
init()maintains?- Why migration registration cannot be done within the existing
Module()function?internal/sms-gateway/pubsub/module.go (1)
5-5: LGTM!The error wrapping with context follows Go best practices and improves error diagnostics while preserving the error chain.
Also applies to: 23-23
pkg/pubsub/redis.go (2)
89-93: LGTM!The error wrapping improves observability by providing clear context when Redis publish operations fail. The implementation follows Go best practices.
178-180: LGTM!Consistent error wrapping for the Close operation. The conditional wrapping based on
ownedClientis appropriate since we only close the client if we own it.internal/sms-gateway/modules/push/client/types.go (1)
9-13: LGTM!The Client interface provides a clean abstraction with:
- Context-aware methods for proper cancellation and timeout handling
- Granular error handling in Send (per-message errors plus operation-level error)
- Clear lifecycle management with Open/Send/Close
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
118-123: LGTM!The nil check prevents potential nil pointer dereference and provides appropriate error handling when no user is present.
pkg/cache/redis.go (2)
189-190: LGTM!Minor formatting adjustment with no behavioral impact.
257-259: LGTM!Error wrapping is consistent with the pattern in
pkg/pubsub/redis.goand improves observability when Redis client close operations fail.internal/sms-gateway/modules/events/errors.go (1)
1-7: LGTM!Good use of a sentinel error that enables callers to check for validation failures using
errors.Is(). The generic message is appropriate, with specific context added via error wrapping.internal/sms-gateway/modules/settings/models.go (2)
11-12: LGTM!Moving the embedded
TimedModelto the top follows Go conventions and improves readability by separating embedded fields from regular fields.
20-20: LGTM!The change from
&DeviceSettings{}tonew(DeviceSettings)is functionally equivalent and aligns with the pattern ininternal/sms-gateway/modules/messages/models.go.Note: There's inconsistency in migration patterns across modules. For example,
internal/sms-gateway/modules/webhooks/models.go:26uses&Webhook{}. Consider standardizing on one pattern across all modules for consistency.internal/sms-gateway/modules/push/client.go (1)
11-31: LGTM!Clean factory pattern implementation with:
- Proper mode-based client construction
- Clear error messages with context wrapping
- Interface return type for flexibility
- Appropriate handling of invalid modes
internal/sms-gateway/modules/sse/service.go (1)
170-181: LGTM! Improved error handling with explicit flush error capture.The explicit error handling for both write and flush operations, along with proper error wrapping and metrics, is a solid improvement over the previous implementation.
internal/sms-gateway/handlers/base/handler_test.go (1)
6-6: LGTM! Good idiomatic improvements.Using
errors.Newfor simple error strings andhttp.Method*constants instead of string literals improves code clarity and maintainability.Also applies to: 39-39, 46-46, 53-53, 120-120, 123-123, 186-186, 243-243
internal/sms-gateway/handlers/logs/3rdparty.go (1)
24-31: LGTM! Constructor pattern consistent with other handlers.The constructor relocation and explicit dependency injection align well with the broader refactoring pattern seen across other 3rd-party handlers in this PR.
Dockerfile.goreleaser (1)
1-27: LGTM! Well-structured multi-arch Dockerfile.Good security practices with non-root user, proper healthcheck configuration, and correct multi-architecture build support via TARGETPLATFORM.
internal/sms-gateway/handlers/settings/3rdparty.go (1)
35-44: LGTM! Constructor pattern consistent with PR-wide refactoring.The constructor relocation and explicit dependency injection align with the broader pattern applied across other handlers.
internal/sms-gateway/modules/auth/repository.go (1)
19-23: LGTM! Efficient pointer-based API.Returning pointers instead of values is more efficient for structs, and using
new()is idiomatic. The GORM query updates correctly use the pointer without re-referencing.Also applies to: 25-29
.goreleaser.yaml (1)
44-48: No action required—the "latest" tag does not interfere with PR builds.In snapshot mode, GoReleaser creates local images only and appends the platform architecture to tags (e.g.,
latest-arm64,latest-amd64). The PR workflow then manually retags these images with PR-specific identifiers (pr-{PR_NUMBER}-arm64,pr-{PR_NUMBER}-amd64) and pushes only those tags to the registry. The "latest" tag is never pushed during PR builds, so production images are not at risk.internal/sms-gateway/modules/messages/converters.go (1)
13-17: Zero-value TTL init keeps behavior stableDeclaring the pointer nil upfront and only assigning when needed tightens the flow without changing semantics. All good here.
internal/sms-gateway/handlers/events/mobile.go (1)
44-46: Wrapcheck suppression matches handler contractDocumenting that the SSE service wraps its own errors justifies the wrapcheck exemption and keeps lint noise down. Looks tidy.
internal/sms-gateway/modules/events/types.go (1)
29-42: Helpful error context on JSON failuresAdding explicit context to marshal/unmarshal failures will make downstream diagnostics easier with zero behavior change. Nice improvement.
internal/sms-gateway/handlers/devices/3rdparty.go (1)
24-35: Constructor refactor aligns with direct dependency wiringSwitching to explicit parameters while still seeding
base.Handlerwith logger/validator keeps behavior intact and matches the emerging pattern. Looks solid.internal/sms-gateway/models/models.go (1)
25-50: Model helpers make intent explicitThese constructors neatly capture the minimal fields needed when creating users/devices while leaving the rest to be filled by services. No concerns here.
internal/sms-gateway/modules/push/module.go (2)
19-21: LGTM: Provider correctly configured.The
newClientprovider is appropriately scoped as private and follows the standard fx.Provide pattern.
25-34: LGTM: Lifecycle management correctly implemented.The lifecycle hook properly wires the client's Open and Close methods to the application lifecycle using the standard Uber FX pattern.
internal/sms-gateway/handlers/health.go (1)
89-89: LGTM: Unused parameter made explicit.The underscore correctly indicates that the map key is intentionally unused, which is a Go best practice and improves code clarity.
internal/sms-gateway/modules/push/fcm/errors.go (1)
5-7: LGTM: Sentinel error correctly defined.The error follows the standard Go pattern for sentinel errors and is appropriately exported for use with
errors.Is().internal/sms-gateway/modules/auth/errors.go (1)
5-7: LGTM: Sentinel error correctly defined.The error follows the standard Go pattern for sentinel errors and is appropriately exported for use with
errors.Is().internal/sms-gateway/modules/push/fcm/utils.go (1)
7-10: LGTM: Type migration correctly implemented.The change from
types.Eventtoclient.Eventis consistent with the broader refactoring to consolidate types in the client package. The function logic remains correct.internal/sms-gateway/modules/auth/types.go (1)
14-18: LGTM: Type rename improves clarity.The rename from
AuthCodetoOneTimeCodebetter reflects the type's purpose as a one-time authorization code..github/workflows/go.yml (2)
67-67: LGTM: Test caching disabled for coverage.Adding
-count=1ensures fresh test runs for accurate coverage reporting, which is the correct practice for CI coverage collection.
96-129: Review cache key strategy for benchmark history.The restore key (line 102) uses
${{ runner.os }}-benchmark-${{ github.ref_name }}while the save key (line 129) includes an additional${{ github.run_id }}. This creates a unique cache entry for each run, which means:
- The restore step will use the
restore-keysfallback since exact matches won't exist- Multiple cache entries will accumulate per branch
- GitHub Actions will eventually evict older entries due to cache size limits
If the intent is to maintain a single benchmark history per branch, consider removing
github.run_idfrom the save key:- key: ${{ runner.os }}-benchmark-${{ github.ref_name }}-${{ github.run_id }} + key: ${{ runner.os }}-benchmark-${{ github.ref_name }}If accumulating multiple benchmark snapshots per branch is intentional for history tracking, the current approach is acceptable but may lead to cache churn.
internal/sms-gateway/modules/devices/service.go (1)
78-105: LGTM! Well-structured random device selection.The GetAny method implements a clean conditional filtering pattern and handles multiple results appropriately with random selection. The use of
math/rand/v2with thegosecnolint directive is justified for this non-cryptographic device selection use case.internal/sms-gateway/modules/messages/errors.go (1)
3-13: LGTM! Clean error type refactoring.The addition of
ErrLockFailedsentinel error and theValidationErrortype rename maintain clear error semantics. The implementations are correct and align with Go error handling best practices.internal/sms-gateway/modules/auth/module.go (1)
10-32: LGTM! Consistent module factory pattern.The conversion from a module variable to a
Module()factory function is a clean refactor that aligns with the PR objective. The internal wiring remains unchanged, and the pattern is consistently applied across multiple modules.internal/sms-gateway/app.go (3)
36-62: LGTM! Module wiring properly updated.The conversion to
Module()factory function is clean and consistent. The updated call sites correctly useModule()for modules that have been converted to functions, while maintaining direct references for modules that remain as variables.
65-65: LGTM! Appropriate nolint directive.The
reassignnolint is justified here ascli.DefaultCommandis a framework-specific configuration that needs to be set at startup.
136-136: LGTM! Justified gochecknoinits directive.The nolint directive with "backward compatibility" reason is appropriate for this
init()function that registers the CLI command.internal/sms-gateway/modules/messages/models.go (3)
36-37: LGTM! Soft delete capability added.The embedding of
SoftDeletableModeladds soft deletion tracking to messages, which is a useful feature for maintaining historical data.
71-71: LGTM! Appropriate nilnil directives.The
nilnilnolint directives are justified here. Returning(nil, nil)is intentional to distinguish "wrong message type" from error conditions, which is a valid pattern for type-specific getters.Also applies to: 98-98
137-141: LGTM! Improved error handling.The migration function now uses
fmt.Errorfwith%wfor proper error wrapping, which improves error context and enables error unwrapping.internal/sms-gateway/modules/settings/module.go (2)
9-23: LGTM! Consistent module factory pattern.The conversion to
Module()factory function follows the same pattern applied across the codebase, maintaining consistency in module wiring.
25-25: LGTM! Justified gochecknoinits directive.The nolint directive for the migration registration init function is appropriate and consistent with similar patterns in the codebase.
internal/sms-gateway/modules/events/module.go (1)
10-39: LGTM! Improved error handling in shutdown path.The conversion to
Module()factory function is consistent with the broader refactor. Additionally, the use ofshErrto capture and log shutdown errors (line 25-26) is an improvement over direct error checking, providing better observability when graceful shutdown fails.internal/sms-gateway/handlers/converters/messages.go (1)
27-28: DeviceID initialization is correct.The
DeviceIDfield is intentionally set to""because the source structmessages.MessageOutdoes not have aDeviceIDfield. The domain layer separates message content (MessageOut) from device state information (MessageStateOut, which includes DeviceID). This is an architectural choice where the message converter receives only message data without device context. No action needed.internal/sms-gateway/modules/events/service.go (1)
55-57: Nice validation guard.Wrapping the empty event-type check with
ErrValidationFailedkeeps the contract explicit and prevents meaningless messages from hitting downstream transports.internal/sms-gateway/openapi/docs.go (1)
789-865: Health probe docs synced with implementation.The new liveness/readiness/startup endpoints and 503 semantics make the spec match Kubernetes expectations, which should cut down on operator confusion. Nicely done.
internal/sms-gateway/modules/push/types.go (1)
21-44: Serialization error wrapping is on point.Routing errors through
fmt.Errorf(... %w ...)gives callers stackable context while keeping the payload format untouched. Good adjustment alongside the new client.Event alias.internal/sms-gateway/modules/push/fcm/client.go (1)
37-86: Great consistency on initialization and message typing.The unified
ErrInitializationFailedwrapping and the move toclient.Messagekeep the FCM client aligned with the new push abstraction while preserving caller ergonomics..github/workflows/release.yml (4)
1-10: LGTM! Workflow trigger and permissions correctly configured for GoReleaser.The migration from release event to tag-based trigger (v*) and the addition of
packages: writepermission are appropriate for GoReleaser's workflow, which creates releases on tag push and can publish Docker images to GHCR.
56-88: LGTM! Deploy job correctly wired to goreleaser.The dependency change from
buildtogoreleaserand the APP_VERSION extraction logic are correct. The version is properly extracted from the git ref and passed to Terraform for deployment.
99-99: LGTM! Secondary deployment correctly depends on goreleaser.The dependency update is consistent with the primary deployment job.
35-36: Clarify the purpose of the hardcoded date "2022-06-15" in the RELEASE_ID calculation.The date appears nowhere else in the codebase and lacks documentation. Confirm whether this represents a meaningful project milestone (e.g., inception date) or if it's legacy code that should be updated. This ensures the versioning scheme remains intentional and maintainable.
internal/sms-gateway/handlers/messages/params.go (1)
32-86: LGTM! Type renames are consistent and align with module refactoring.The removal of the "Messages" prefix from type names (e.g.,
MessagesSelectFilter→SelectFilter) improves code clarity since these types are already in themessagespackage namespace. All usages have been updated consistently throughout the file.internal/sms-gateway/handlers/messages/3rdparty.go (5)
44-53: LGTM! Constructor follows consistent dependency injection pattern.The new constructor properly initializes the base Handler and wires dependencies, consistent with other controllers in the codebase.
84-101: LGTM! Device selection logic simplified with better error handling.The migration to
GetAnymethod improves code clarity and the error handling properly distinguishes between user errors (no device found) and system errors, with appropriate logging for troubleshooting.
189-204: LGTM! Parameter parsing improved with consistent error handling.The switch to pointer-based parameter parsing and the addition of error logging improve code maintainability and debugging capabilities.
250-269: LGTM! Consistent error handling improvements throughout.The error handling enhancements with logging and standardized HTTP error responses improve both user experience and system observability.
133-137: No changes required.The device parameter is correctly passed by value. The
Enqueuemethod signature expectsdevice models.Device(value type), and the call site correctly dereferences the pointer to pass the value (*device).internal/sms-gateway/handlers/messages/mobile.go (2)
58-63: LGTM! Parameter parsing improved with consistent error handling.The pointer-based parsing and error wrapping align with the error handling improvements throughout the PR.
93-97: LGTM! Consistent error handling.The error wrapping with
fiber.NewErrorstandardizes HTTP error responses.internal/sms-gateway/handlers/settings/mobile.go (2)
23-37: LGTM! Constructor refactored for explicit dependency injection.The migration from fx.In-based parameters to explicit constructor arguments improves testability and reduces framework coupling, aligning with the pattern used in
handlers/events/mobile.goandhandlers/webhooks/mobile.go.
50-54: LGTM! Enhanced error context for debugging.The addition of device ID and user ID to the error message improves troubleshooting capabilities.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sms-gateway/modules/settings/repository.go (1)
47-48: Preservecreated_atwhen upserting.DeviceSettings embeds
models.TimedModel, so callingCreatewithclause.OnConflict{UpdateAll: true}rewrites every column on conflict — includingcreated_at. On everyUpdateSettingsrun for an existing row we now blow away the original creation timestamp with the freshly generated value (or whatever the caller sent), erasing history. Copy the original timestamps fromsourcebefore the upsert, or narrow the conflict handler to only mutate the intended fields (e.g.,settings,updated_at). For example:- if err := tx.Clauses(clause.OnConflict{UpdateAll: true}).Create(settings).Error; err != nil { + if !source.CreatedAt.IsZero() { + settings.CreatedAt = source.CreatedAt + } + + if err := tx.Clauses(clause.OnConflict{UpdateAll: true}).Create(settings).Error; err != nil { return err }Either approach prevents clobbering immutable timestamps.
🧹 Nitpick comments (3)
.github/workflows/pr.yml (2)
24-24: Inconsistent GitHub Actions version pinning across jobs.The e2e job uses
actions/setup-go@v5(line 24) while the goreleaser job usesactions/setup-go@v6(line 73). For consistency and predictability, use the same major version across both jobs unless there's a specific reason requiring different versions.Consider standardizing on
@v6across both jobs:- name: Set up Go uses: actions/setup-go@v5 + uses: actions/setup-go@v6Also applies to: 73-73
142-149: Hardcoded artifact domain limits flexibility.The PR comment table hardcodes
https://github.capcom.me/for artifact URLs (lines 142-149), but the S3 endpoint is already captured in${{ secrets.AWS_ENDPOINT }}(line 99). Using a hardcoded domain reduces flexibility and makes the workflow less portable if the domain or endpoint changes.Parameterize the artifact base URL using a GitHub variable or derive it from the endpoint. For example:
- | 🍎 Darwin arm64 | [${{ env.PROJECT_NAME }}_Darwin_arm64.tar.gz](https://github.capcom.me/${{ github.repository }}/${{ github.event.pull_request.head.sha }}/${{ env.PROJECT_NAME }}_Darwin_arm64.tar.gz) | + | 🍎 Darwin arm64 | [${{ env.PROJECT_NAME }}_Darwin_arm64.tar.gz](${{ vars.ARTIFACT_BASE_URL }}/${{ github.repository }}/${{ github.event.pull_request.head.sha }}/${{ env.PROJECT_NAME }}_Darwin_arm64.tar.gz) |Define
ARTIFACT_BASE_URLas a GitHub Actions repository variable pointing to your CDN or S3 web endpoint.internal/sms-gateway/handlers/upstream.go (1)
97-103: Consider making rate limits configurable.The constants improve readability, but rate limits are operational parameters that may need different values across environments (dev/staging/production). Since the
Configstruct already controls upstream behavior (line 91), consider addingUpstreamRateLimitandUpstreamRateWindowfields there.Example addition to
Config:type Config struct { UpstreamEnabled bool UpstreamRateLimit int // defaults to 5 UpstreamRateWindow time.Duration // defaults to 60s // ... other fields }Then use them here:
- const ( - rateLimit = 5 - rateTime = 60 * time.Second - ) router.Post("/push", limiter.New(limiter.Config{ - Max: rateLimit, - Expiration: rateTime, + Max: h.config.UpstreamRateLimit, + Expiration: h.config.UpstreamRateWindow, LimiterMiddleware: limiter.SlidingWindow{}, }), h.postPush)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/pr.yml(1 hunks)internal/sms-gateway/handlers/messages/params.go(4 hunks)internal/sms-gateway/handlers/middlewares/userauth/userauth.go(2 hunks)internal/sms-gateway/handlers/mobile.go(7 hunks)internal/sms-gateway/handlers/upstream.go(2 hunks)internal/sms-gateway/modules/messages/cache.go(2 hunks)internal/sms-gateway/modules/settings/repository.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sms-gateway/handlers/mobile.go
🧰 Additional context used
🧬 Code graph analysis (4)
internal/sms-gateway/modules/settings/repository.go (1)
internal/sms-gateway/modules/settings/models.go (1)
DeviceSettings(10-17)
internal/sms-gateway/modules/messages/cache.go (2)
internal/sms-gateway/modules/messages/domain.go (1)
MessageStateOut(38-44)pkg/cache/options.go (2)
WithTTL(22-30)AndSetTTL(64-68)
internal/sms-gateway/handlers/messages/params.go (1)
internal/sms-gateway/modules/messages/repository_filter.go (3)
SelectFilter(16-23)SelectOptions(51-62)Order(7-7)
internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
internal/sms-gateway/models/models.go (1)
User(17-23)
⏰ 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: E2E
- GitHub Check: Benchmark
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
.github/workflows/pr.yml (2)
15-16: E2E job permissions now properly scoped.The explicit
permissions: contents: readblock correctly addresses the security concern raised in previous reviews. This minimal permission set is appropriate for the checkout and Go setup operations performed in the e2e job.
47-50: GoReleaser job permissions are properly scoped.The permissions block correctly grants minimal access:
contents: readfor checkout,pull-requests: writefor commenting, andpackages: writefor pushing to GitHub Container Registry. This aligns with security best practices.internal/sms-gateway/modules/messages/cache.go (2)
30-51: LGTM! Improved error handling.The explicit error wrapping with descriptive messages and the clear nil return enhance code maintainability and debugging.
53-72: LGTM! Appropriate nil handling and error wrapping.The improved error messages provide better debugging context. The
//nolint:nilnilannotation on line 63 is correctly applied—returning(nil, nil)is intentional here to distinguish between a cache miss and an error, with the empty value representing a cached "Not Found" state.internal/sms-gateway/handlers/messages/params.go (6)
10-13: Verify the type change from uint to int for DeviceActiveWithin.The
DeviceActiveWithinfield was changed fromuinttoint. While the validationmin=1prevents negative values,uintis semantically more appropriate for duration/count values. Please confirm this change is intentional and doesn't introduce any compatibility issues with existing consumers.
15-22: LGTM!The formatting alignment of validation tags improves readability without changing functionality.
32-56: LGTM!The return type update from
MessagesSelectFiltertoSelectFilteraligns with the broader type renaming refactoring. The method logic remains correct.
58-76: LGTM!The return type update to
SelectOptionsis consistent with the refactoring. The introduction of themaxLimitconstant provides defensive programming, even though Line 20's validation already enforcesmax=100. This redundancy is acceptable as a safety measure.
78-80: LGTM!The
Orderfield type update fromMessagesOrdertoOrderis consistent with the type renaming refactoring.
82-87: LGTM!The return type update to
messages.Orderis correct and maintains the proper default behavior.internal/sms-gateway/handlers/upstream.go (1)
67-69: LGTM! Validation error status code corrected.The change from
fiber.StatusBadGateway(502) tofiber.StatusBadRequest(400) correctly maps validation failures to client errors. This addresses the previous review feedback.internal/sms-gateway/handlers/middlewares/userauth/userauth.go (1)
94-100: LGTM: Safe pointer-based user retrieval.The refactored
GetUsercorrectly performs a safe type assertion and returns nil when the user is not present or has an unexpected type. This avoids panics and aligns with the broader shift to pointer-based user handling.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/pr.yml (1)
92-92: Pin the custom S3 upload action to a specific version.Line 92 references
capcom6/upload-s3-action@master, which uses the unpinned master branch. This introduces supply-chain risk and could break if the action changes unexpectedly. Pin to a specific release tag or commit SHA to eliminate this risk.Apply this diff:
- uses: capcom6/upload-s3-action@master + uses: capcom6/upload-s3-action@v1(Replace
v1with the appropriate stable version tag you intend to use.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pr.yml(1 hunks)
🔇 Additional comments (2)
.github/workflows/pr.yml (2)
15-16: Explicit permissions block added to e2e job.The e2e job now correctly specifies minimal required permissions. This addresses the prior security finding and aligns with GitHub's least-privilege best practices.
44-50: Goreleaser job permissions and dependencies configured appropriately.The goreleaser job correctly specifies permissions (contents: read, pull-requests: write, packages: write), depends on e2e job completion, and uses conditional steps to skip operations for dependabot. The setup aligns with the GoReleaser snapshot distribution workflow.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
pkg/cache/options.go (1)
20-28: Fix logic bug: zero-time assignment is immediately overwritten.Lines 22-24 set a zero time when
ttl <= 0, but line 26 unconditionally overwrites it withtime.Now().Add(ttl). The early-return or else-block is missing.Note: This is a pre-existing bug not introduced by this PR, but it should be fixed to prevent incorrect expiry behavior.
Apply this diff to fix the logic:
func WithTTL(ttl time.Duration) Option { return func(o *options) { if ttl <= 0 { o.validUntil = time.Time{} + return } o.validUntil = time.Now().Add(ttl) } }pkg/cache/memory_profile_test.go (1)
123-143: Restore standard counting loop for integer constants
for i := range numCachesdoes not compile becauserangeexpects an array, slice, map, string, or channel. WithnumCachesdefined as an int constant, this turns the whole test file into a build breaker. Please revert to the classic counting loop so the code builds.- for i := range numCaches { + for i := 0; i < numCaches; i++ {pkg/cache/redis.go (1)
43-46: Fix HTTL update when the field has no TTL
HTTLreturns-1when the hash field has no TTL. AddingttlDeltaon top of-1means a 1-second extension turns into0, expiring the item immediately. That regression breaksGet(..., cache.WithTTLUpdate(...))for keys that previously had no TTL. Clamp negative TTLs to zero before adding the delta so the extension behaves correctly.- local ttl = redis.call('HTTL', KEYS[1], field) - local newTtl = ttl + ttlDelta + local ttl = redis.call('HTTL', KEYS[1], field) + if ttl < 0 then + ttl = 0 + end + local newTtl = ttl + ttlDeltapkg/cache/memory_bench_test.go (3)
286-290: Revert to counting loop for benchmark prefill
for i := range bm.operationsPerGoroutine * bm.goroutineswill not compile becauserangecannot iterate over an integer expression. Please switch back to the explicit counter so the benchmark builds.- for i := range bm.operationsPerGoroutine * bm.goroutines { + for i := 0; i < bm.operationsPerGoroutine*bm.goroutines; i++ {
394-398: Fix invalid range over scalarsizeHere too,
rangeon anintis invalid Go. Use the classicfor j := 0; j < size; j++form so the benchmark compiles.- for j := range size { + for j := 0; j < size; j++ {
453-457: Restore integer-count loop for cold-key prepopulation
for i := range numKeysis a compile-time error. Please reinstate the counted loop.- for i := range numKeys { + for i := 0; i < numKeys; i++ {
🧹 Nitpick comments (3)
.golangci.yml (3)
159-166: Clarify cyclop complexity thresholds.
max-complexity: 30is quite permissive for a "very strict" config (line 7), especially paired withpackage-average: 10.0. This creates tension between per-function and package-level enforcement. Consider whether both thresholds align with your actual code quality goals, or ifmax-complexityshould be lowered to catch outliers more aggressively.
234-282: Review exhaustruct exclusion list for project specificity.The
exhaustruct.excludelist contains many third-party library patterns (Firebase, telebot, S3, Prometheus, etc.) that suggest this config was borrowed from another project. If this PR introduces stricter linting project-wide, verify that:
- These exclusions actually correspond to types used in this codebase.
- The exclusions don't inadvertently suppress legitimate struct initialization bugs.
- Project maintainers understand why each exclusion exists.
Consider adding a brief comment in the config (or a separate ADR) explaining the rationale for the specific exclusion patterns.
372-375: Reconsidernakedretmax-func-lines: 0.Setting
max-func-lines: 0means all naked returns are flagged, regardless of function length. This is extremely strict and will likely generate many issues across the codebase. Standard practice is to enforce naked returns only in shorter functions (typical defaults are 15–30 lines).Verify that this setting aligns with the team's code review standards, or consider raising it to a reasonable threshold.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.golangci.yml(1 hunks)pkg/cache/errors.go(1 hunks)pkg/cache/memory.go(12 hunks)pkg/cache/memory_bench_test.go(19 hunks)pkg/cache/memory_concurrency_test.go(9 hunks)pkg/cache/memory_edge_test.go(8 hunks)pkg/cache/memory_profile_test.go(2 hunks)pkg/cache/memory_test.go(8 hunks)pkg/cache/options.go(1 hunks)pkg/cache/redis.go(14 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-10T11:11:43.462Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/memory.go:59-74
Timestamp: 2025-09-10T11:11:43.462Z
Learning: In pkg/cache/memory.go, the cleanup method removes expired items first, then executes the provided callback function. This means methods like Drain that use cleanup don't need additional expiration filtering since expired items are already removed before the callback runs.
Applied to files:
pkg/cache/memory_concurrency_test.gopkg/cache/memory_test.gopkg/cache/memory_edge_test.gopkg/cache/memory_profile_test.gopkg/cache/redis.gopkg/cache/memory.gopkg/cache/memory_bench_test.go
📚 Learning: 2025-09-10T23:53:42.006Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Applied to files:
pkg/cache/memory_edge_test.gopkg/cache/redis.gopkg/cache/memory.go
🧬 Code graph analysis (6)
pkg/cache/memory_concurrency_test.go (1)
pkg/cache/errors.go (2)
ErrKeyNotFound(9-9)ErrKeyExists(13-13)
pkg/cache/memory_test.go (1)
pkg/cache/errors.go (2)
ErrKeyNotFound(9-9)ErrKeyExists(13-13)
pkg/cache/memory_edge_test.go (1)
pkg/cache/errors.go (2)
ErrKeyExpired(11-11)ErrKeyNotFound(9-9)
pkg/cache/redis.go (4)
pkg/pubsub/redis.go (2)
NewRedis(42-76)RedisConfig(15-27)pkg/cache/errors.go (1)
ErrInvalidConfig(7-7)pkg/cache/options.go (3)
GetOption(46-46)AndDelete(86-90)Option(6-6)pkg/cache/cache.go (1)
Cache(5-40)
pkg/cache/memory.go (2)
pkg/cache/options.go (3)
GetOption(46-46)AndDelete(86-90)Option(6-6)pkg/cache/cache.go (1)
Cache(5-40)
pkg/cache/memory_bench_test.go (1)
pkg/cache/memory.go (1)
NewMemory(16-23)
⏰ 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). (5)
- GitHub Check: Lint
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
.golangci.yml (2)
309-321: Verify gocritic settings impact on code patterns.Line 317:
paramsOnly: falseforcaptLocaland line 321:skipRecvDeref: falseforunderefare non-default and may flag many existing patterns. Ensure the team has reviewed the warnings these generate before merging, or plan remediation ahead of time.
4-4: I need more specific information about the linters mentioned in the review. Let me search for details on those specific linters in v2.5.0.Need to verify the actual
.golangci.ymlconfiguration file content to assess the full scope of the review comment. Let me search for information about it.Based on my verification, I need to provide the rewritten review comment:
Review comment is incorrect—all mentioned linters are available in golangci-lint v2.5.0.
The linters
copyloopvar(updated to 1.1.0) andtestifylint(updated to 1.3.0) are confirmed in v2.5.0's changelog, and bothtestableexamplesandtestifylintare listed among golangci-lint's supported linters.While
copyloopvarrequires Go 1.22+, golangci-lint automatically disables it if the project's Go version is lower—this is expected behavior, not a compatibility error. The configuration is valid as-is.Likely an incorrect or invalid review comment.
pkg/cache/options.go (1)
12-16: I need to verify that the chaining calls are on differentapply()methods (on filter types, not the options type from the cache package).I need to verify that the chaining calls are on different types. Let me check the repository structure:
Based on my analysis of the script output and web search results, I can now confirm that the chaining calls are on different types (filter objects for GORM queries) and have different signatures than the
options.apply()method inpkg/cache/options.go.All call sites in the cache package (
pkg/cache/memory.go,pkg/cache/redis.go) useapply()without chaining, which is correct for the new void signature.No breaking changes detected.
LGTM!
The removal of the return value is correct since the receiver is a pointer and modifications are applied in-place. All cache package call sites correctly use
apply()as a void call (lines 92, 155 inpkg/cache/memory.goand lines 147, 216, 247 inpkg/cache/redis.go). Chaining calls found in other modules use differentapply()methods on filter types, not on the options type.pkg/cache/memory_concurrency_test.go (3)
5-5: Excellent error handling modernization.The switch to
errors.Is()for sentinel error checks (instead of direct equality) follows Go best practices and makes the code robust against future error wrapping. The consistent application across all tests is well done.Also applies to: 118-118, 224-224, 298-298, 345-345, 380-380, 393-393
32-32: Good code quality improvements.The modern
for range numGoroutinessyntax is cleaner, and renaming togetErrproperly avoids variable shadowing in the closure.Also applies to: 37-39
284-284: Proper unused parameter handling.Using blank identifiers for genuinely unused parameters follows Go conventions and improves code clarity.
Also applies to: 399-399
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cache/redis.go (1)
168-187: Critical: Delete-only operations broken by early return in default case.The switch statement's default case (lines 177-187) returns early, preventing the Lua script from executing when
AndDelete()is used without TTL options. This breaks delete functionality for such calls.Example failure scenario:
// This will NOT delete the key! value, err := cache.Get(ctx, key, AndDelete())The code flow:
o.delete = trueis set byAndDelete()o.isEmpty()returns false (line 152), skipping simple get- No TTL cases match in the switch
- Default case executes simple
HGetand returns (line 186)- Lua script with delete logic (lines 195-196) is never reached
Apply this diff to fix the logic:
var ttlTimestamp, ttlDelta int64 switch { case o.validUntil != nil: ttlTimestamp = o.validUntil.Unix() case o.setTTL != nil: ttlTimestamp = time.Now().Add(*o.setTTL).Unix() case o.updateTTL != nil: ttlDelta = int64(o.updateTTL.Seconds()) case o.defaultTTL: ttlTimestamp = time.Now().Add(r.ttl).Unix() - default: - // No TTL options, fallback to simple get - val, err := r.client.HGet(ctx, r.key, key).Result() - if err != nil { - if errors.Is(err, redis.Nil) { - return nil, ErrKeyNotFound - } - return nil, fmt.Errorf("can't get cache item: %w", err) - } - return []byte(val), nil } delArg := "0"This allows the code to fall through to the Lua script invocation, which correctly handles delete-only operations when
ttlTimestampandttlDeltaare both zero.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/cache/options.go(2 hunks)pkg/cache/redis.go(15 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-10T23:53:42.006Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Applied to files:
pkg/cache/redis.go
📚 Learning: 2025-09-10T23:54:16.828Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:118-121
Timestamp: 2025-09-10T23:54:16.828Z
Learning: Redis 7.4.0 introduced HEXPIRE, HEXPIREAT, and related commands that support per-field TTL on hash data structures. go-redis v9.6.0+ includes support for these commands with methods like HExpireAt.
Applied to files:
pkg/cache/redis.go
📚 Learning: 2025-09-10T11:11:43.462Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/memory.go:59-74
Timestamp: 2025-09-10T11:11:43.462Z
Learning: In pkg/cache/memory.go, the cleanup method removes expired items first, then executes the provided callback function. This means methods like Drain that use cleanup don't need additional expiration filtering since expired items are already removed before the callback runs.
Applied to files:
pkg/cache/redis.go
🧬 Code graph analysis (1)
pkg/cache/redis.go (3)
pkg/pubsub/redis.go (2)
NewRedis(42-76)RedisConfig(15-27)pkg/cache/errors.go (1)
ErrInvalidConfig(7-7)pkg/cache/cache.go (1)
Cache(5-40)
⏰ 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: Benchmark
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
pkg/cache/redis.go (7)
5-5: LGTM: Proper error handling dependency added.The
errorsimport is needed for theerrors.Is()calls used to detectredis.Nilerrors.
44-46: LGTM: Correct handling of negative TTL values.Setting
ttl = 0whenHTTLreturns a negative value (key doesn't exist or has no expiry) prevents issues when adding the delta.
72-72: LGTM: Type properly exported.The
RedisCachetype is now exported, making it accessible to external consumers. All related signatures and type assertions are correctly updated.Also applies to: 81-81, 100-100, 271-271
87-87: LGTM: Proper sentinel error wrapping.Using
ErrInvalidConfigallows callers to programmatically detect configuration errors.
136-137: LGTM: Self-documenting constant improves readability.The
itemsPerKeyconstant clarifies that HGETALL returns key-value pairs in a flat array.
156-157: LGTM: Proper sentinel error checking.Using
errors.Is()to check forredis.Nilis the correct approach and handles wrapped errors properly.Also applies to: 181-182
263-265: LGTM: Proper error propagation in Close.The error from
client.Close()is now properly wrapped with a descriptive message.
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 (1)
pkg/cache/options.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T23:53:42.006Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Applied to files:
pkg/cache/options.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). (3)
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
pkg/cache/options.go (1)
12-16: LGTM! Simplified apply signature.The removal of the return value from
applysimplifies the API. Since this is an internal method and the PR compiles, all call sites have been appropriately updated.
pkg/cache/options.go
Outdated
| if ttl < 0 { | ||
| ttl = 0 | ||
| } | ||
|
|
||
| o.validUntil = time.Now().Add(ttl) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that validUntil = time.Now() is treated as expired.
The TTL clamping addresses the past review concern about non-positive TTLs causing "no expiry." However, when ttl == 0, validUntil is set to time.Now(), which might not be treated as expired if the cache's expiry check uses time.Now().After(validUntil) (since now.After(now) is false). Please verify that the cache implementations use >= or AfterOrEqual comparison, or handle this edge case appropriately.
Run the following script to check how the cache implementations handle expiry:
🏁 Script executed:
#!/bin/bash
# Description: Verify expiry logic in cache implementations
# Search for expiry checks in cache implementations
rg -n -A 3 -B 3 'validUntil' --type go -g 'pkg/cache/*.go' | rg -P '(After|Before|Compare|IsZero)'Length of output: 197
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for WithTTL function definition and usage patterns
echo "=== WithTTL Function Definition ==="
ast-grep --pattern 'func WithTTL($_) {
$$$
}'
echo -e "\n=== WithTTL Usage Patterns ==="
rg -n 'WithTTL\(' --type go -B 2 -A 2
Length of output: 4836
🏁 Script executed:
#!/bin/bash
# Check how ttl=0 is handled in tests or real usage
echo "=== Search for test cases with ttl values ==="
rg -n 'ttl.*=.*0' --type go -g 'pkg/cache/*.go' -B 1 -A 1
echo -e "\n=== Check memory.go Expired function full context ==="
rg -n 'func.*Expired' --type go -g 'pkg/cache/memory.go' -A 5Length of output: 981
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full WithTTL function with docstring and surrounding context
rg -n -B 5 -A 10 'func WithTTL' --type go pkg/cache/options.go
Length of output: 427
🏁 Script executed:
#!/bin/bash
# Check if there's any explicit rejection or validation of ttl <= 0
rg -n 'ttl.*<=.*0|ttl.*>=.*0' --type go pkg/cache/ -B 2 -A 2Length of output: 0
Fix the expiry check to handle validUntil = time.Now() as expired.
The TTL clamping prevents the "no expiry" issue from the previous review, but it introduces a subtle bug. When ttl <= 0, the code sets validUntil = time.Now(), which will not be treated as expired because isExpired() in memory.go:44 uses strict After() comparison (now.After(validUntil) returns false when both times are equal).
Fix: Update isExpired() to use now.After(i.validUntil) || now.Equal(i.validUntil) (or equivalently !now.Before(i.validUntil)), so items with validUntil = time.Now() are immediately considered expired. Alternatively, handle ttl <= 0 by using the zero value to represent "never expires" if that's the intended semantics.
🤖 Prompt for AI Agents
In pkg/cache/options.go lines 22-26 and pkg/cache/memory.go around line 44, the
TTL clamp sets validUntil = time.Now() for ttl <= 0 which is not considered
expired because isExpired() uses now.After(validUntil); update isExpired() to
treat equality as expired by using the equivalent of !now.Before(i.validUntil)
(or now.After(...) || now.Equal(...)), so items with validUntil == time.Now()
are immediately expired; alternatively, if desired semantics are "never expires"
for ttl <= 0, change the options logic to leave validUntil as the zero time to
represent no expiry and adjust isExpired() accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/pubsub/redis.go (1)
169-186: GuardCloseagainst concurrent double-close panics.Two goroutines calling
Closeat once can both fall through the non-blockingselectand executeclose(r.closeCh), triggering a panic. Use the existing mutex to serialize the close operation before releasing it for the wait and client teardown.Apply this diff:
func (r *RedisPubSub) Close() error { - select { - case <-r.closeCh: - return nil - default: - close(r.closeCh) - } + r.mu.Lock() + select { + case <-r.closeCh: + r.mu.Unlock() + return nil + default: + close(r.closeCh) + } + r.mu.Unlock() r.wg.Wait() if r.ownedClient { if err := r.client.Close(); err != nil {pkg/pubsub/memory.go (1)
147-158: SynchronizeCloseto avoid double closingcloseCh.Concurrent callers can both take the
defaultbranch and closem.closeCh, resulting in a panic. Grab the mutex before the check/close, release it before waiting on the subscriber goroutines.Apply this diff:
func (m *MemoryPubSub) Close() error { - select { - case <-m.closeCh: - return nil - default: - } - close(m.closeCh) + m.mu.Lock() + select { + case <-m.closeCh: + m.mu.Unlock() + return nil + default: + close(m.closeCh) + } + m.mu.Unlock() m.wg.Wait() return nil
♻️ Duplicate comments (1)
pkg/cache/options.go (1)
22-29: Clarify the intended semantics for non-positive TTL values.The current implementation has ambiguous semantics:
ttl == 0: setsvalidUntilto the zero valuetime.Time{}, which typically means "no expiry"ttl < 0: setsvalidUntiltotime.Now(), intended to expire immediately but won't work correctly due to comparison logic (as flagged in past reviews)Please clarify the intended behavior:
- Should
ttl == 0mean "no expiry" or "expire immediately"?- Should
ttl < 0be rejected, or should it mean something specific?The comparison bug for
ttl < 0has been extensively documented in previous review comments, which provide detailed analysis and fixes.
🧹 Nitpick comments (3)
internal/sms-gateway/handlers/messages/params.go (2)
12-12: Consider usinguintfor semantic correctness.Since the validation ensures
min=1, usinguintwould be more semantically appropriate to express that only non-negative values are valid. However, if downstream dependencies requireint, this change is acceptable.
26-26: Fix grammatical error in comment.The comment has a grammatical error. It should read "won't be used directly" instead of "won't used directly".
Apply this diff:
- return errors.New("`from` date must be before `to` date") //nolint:err113 // won't used directly + return errors.New("`from` date must be before `to` date") //nolint:err113 // won't be used directlyinternal/sms-gateway/modules/health/types.go (1)
41-51: Consider removing or documenting the unreachable default case.The switch covers all possible
statusLevelvalues (levelPass, levelWarn, levelFail), making the default case at line 50 unreachable. You could either remove it or add a comment explaining it serves as a defensive fallback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/messages/params.go(3 hunks)internal/sms-gateway/modules/db/health.go(1 hunks)internal/sms-gateway/modules/health/health.go(2 hunks)internal/sms-gateway/modules/health/module.go(2 hunks)internal/sms-gateway/modules/health/service.go(3 hunks)internal/sms-gateway/modules/health/types.go(3 hunks)internal/sms-gateway/modules/messages/service_test.go(0 hunks)internal/sms-gateway/modules/webhooks/errors.go(3 hunks)internal/sms-gateway/modules/webhooks/models.go(1 hunks)internal/sms-gateway/modules/webhooks/module.go(1 hunks)internal/sms-gateway/modules/webhooks/repository.go(2 hunks)internal/sms-gateway/modules/webhooks/repository_filter.go(1 hunks)internal/sms-gateway/modules/webhooks/service.go(2 hunks)pkg/cache/options.go(1 hunks)pkg/pubsub/memory.go(8 hunks)pkg/pubsub/options.go(1 hunks)pkg/pubsub/pubsub.go(1 hunks)pkg/pubsub/redis.go(6 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/messages/service_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-10T23:45:45.502Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: internal/sms-gateway/online/module.go:14-16
Timestamp: 2025-09-10T23:45:45.502Z
Learning: fx.Decorate in uber-go/fx creates module-scoped decorated dependencies, not global modifications. Using fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("module_name") }) is a correct pattern for creating module-specific loggers that are scoped only to that module's dependency graph.
Applied to files:
internal/sms-gateway/modules/webhooks/module.go
📚 Learning: 2025-09-10T23:53:42.006Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Applied to files:
pkg/cache/options.go
🧬 Code graph analysis (12)
internal/sms-gateway/modules/webhooks/module.go (3)
internal/sms-gateway/app.go (1)
Module(36-62)internal/sms-gateway/modules/health/module.go (1)
Module(8-22)internal/sms-gateway/modules/db/module.go (1)
Module(12-22)
internal/sms-gateway/modules/webhooks/repository.go (1)
internal/sms-gateway/modules/webhooks/models.go (1)
Webhook(11-25)
internal/sms-gateway/modules/health/service.go (1)
internal/sms-gateway/modules/health/types.go (3)
Provider(69-75)Checks(67-67)CheckResult(21-24)
pkg/pubsub/memory.go (2)
pkg/pubsub/options.go (1)
Option(3-3)pkg/pubsub/pubsub.go (2)
Subscription(19-24)PubSub(46-51)
pkg/pubsub/redis.go (2)
pkg/cache/redis.go (2)
NewRedis(81-108)RedisConfig(56-70)pkg/pubsub/pubsub.go (3)
ErrInvalidConfig(9-9)Subscription(19-24)PubSub(46-51)
internal/sms-gateway/modules/webhooks/models.go (3)
internal/sms-gateway/models/models.go (3)
SoftDeletableModel(12-15)User(17-23)Device(32-43)internal/sms-gateway/modules/messages/models.go (1)
Migrate(136-141)internal/sms-gateway/modules/settings/models.go (1)
Migrate(19-24)
internal/sms-gateway/handlers/messages/params.go (1)
internal/sms-gateway/modules/messages/repository_filter.go (3)
SelectFilter(16-23)SelectOptions(51-62)Order(7-7)
internal/sms-gateway/modules/db/health.go (1)
internal/sms-gateway/modules/health/types.go (1)
Provider(69-75)
internal/sms-gateway/modules/health/module.go (1)
internal/sms-gateway/modules/health/types.go (1)
Provider(69-75)
internal/sms-gateway/modules/webhooks/service.go (1)
internal/sms-gateway/modules/webhooks/errors.go (1)
ErrInvalidEvent(9-9)
internal/sms-gateway/modules/health/health.go (1)
internal/sms-gateway/modules/health/types.go (6)
Checks(67-67)CheckDetail(54-64)Status(7-7)StatusPass(11-11)StatusWarn(12-12)Provider(69-75)
internal/sms-gateway/app.go (2)
internal/sms-gateway/handlers/module.go (1)
Module(15-39)internal/sms-gateway/modules/devices/module.go (1)
Module(16-32)
⏰ 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: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (27)
internal/sms-gateway/handlers/messages/params.go (4)
4-4: LGTM: Import addition is appropriate.The
errorspackage import is correctly added for theerrors.New()usage on line 26.
32-56: LGTM: Type refactoring is correctly applied.The return type change from
messages.MessagesSelectFiltertomessages.SelectFilteraligns with the repository layer refactoring. The conversion logic remains intact and correct.
58-76: LGTM: Type refactoring correctly applied.The return type change to
messages.SelectOptionsis consistent with the repository layer refactoring. The logic correctly sets default values and applies limits.Optional note: The
min(p.Limit, maxLimit)on line 66 is technically redundant since the validation on line 20 already enforcesmax=100. However, this defensive approach is acceptable and doesn't cause issues.
78-87: No issues found. Code is correct.The
MessagesOrderLIFOconstant is properly defined as typeOrderininternal/sms-gateway/modules/messages/repository_filter.go(line 11:MessagesOrderLIFO Order = "lifo"), which is fully compatible with the refactored type signature inparams.go. The type change frommessages.MessagesOrdertomessages.Orderhas been consistently applied throughout the codebase.pkg/cache/options.go (1)
12-16: No breaking change—all call sites useapply()as a side-effect only.The verification shows that all five call sites in the codebase (
pkg/cache/redis.go:219,pkg/cache/redis.go:250,pkg/cache/memory.go:155,pkg/cache/redis.go:150,pkg/cache/memory.go:92) invokeapply()as standalone statements without chaining or consuming the return value. The removal of the return value causes no impact.Likely an incorrect or invalid review comment.
pkg/pubsub/options.go (1)
9-13: Non-fluentapplystill covers current usage.
options.applyno longer returns the receiver, but none of the constructors rely on chaining the result, so this keeps behavior intact and simplifies the API surface.pkg/pubsub/pubsub.go (1)
9-11: Good call addingErrInvalidConfig.Exposing a shared sentinel lets the constructors wrap config issues cleanly and makes upstream handling consistent.
internal/sms-gateway/modules/webhooks/repository_filter.go (1)
26-30: LGTM - Idiomatic pointer allocation.The change from composite literal to
new(selectFilter)is functionally identical and represents a minor style improvement for zero-value pointer allocation.internal/sms-gateway/modules/health/types.go (3)
20-24: LGTM - Public health check exposure.The addition of the
Checksfield with JSON tag exposes health check details in API responses, which aligns with standard health check endpoint patterns.
53-64: LGTM - Enhanced observability.The addition of
ObservedUnitandObservedValuefields follows standard health check patterns for exposing metrics.
69-75: LGTM - Simplified naming.Renaming
HealthProvidertoProvideris appropriate since the interface already resides in the health package. The method signatures remain unchanged.internal/sms-gateway/modules/health/module.go (2)
14-17: LGTM - Appropriate visibility restriction.Changing
NewHealthtonewHealth(unexported) is correct for fx-based DI where the constructor is only used within module wiring.
24-30: LGTM - Consistent with interface rename.Updated
fx.Asannotation correctly reflects theHealthProvider→Providerinterface rename.internal/sms-gateway/modules/db/health.go (1)
60-60: LGTM - Updated interface assertion.The compile-time assertion correctly reflects the
HealthProvider→Providerinterface rename.internal/sms-gateway/modules/health/service.go (3)
10-22: LGTM - Consistent type updates.Field types correctly updated from
HealthProvidertoProviderto reflect the interface rename.
32-35: LGTM - Improved formatting and type consistency.The multi-line parameter formatting with trailing comma follows Go conventions, and the
Providertype update aligns with the interface rename.
71-87: LGTM - Consistent closure type updates.All probe closures correctly updated to use the
Providertype while preserving the logic.internal/sms-gateway/modules/webhooks/module.go (2)
9-20: LGTM - Consistent module pattern.The conversion from exported variable to
Module()function aligns with the refactoring pattern applied consistently across other modules in this PR. Based on learnings.
22-25: LGTM - Appropriate linter directive.The
gochecknoinitssuppression is correctly applied with a clear explanation, as theinit()function is required for framework-specific migration registration.internal/sms-gateway/modules/webhooks/errors.go (2)
8-10: LGTM - Sentinel error pattern.The addition of
ErrInvalidEventfollows Go's sentinel error pattern, allowing callers to useerrors.Is()for error checking.
34-36: LGTM - Improved error checking.Using
errors.Asinstead of type assertion correctly handles wrapped errors and is more robust.internal/sms-gateway/modules/webhooks/repository.go (2)
12-16: LGTM - Standard DI constructor.The
NewRepositoryconstructor follows standard dependency injection patterns and aligns with the module wiring updates.
33-35: LGTM - Idiomatic nil pointer for type information.Using
(*Webhook)(nil)instead of&Webhook{}is a more idiomatic way to provide type information to GORM's Delete operation, making it clear that only the type is needed, not an actual instance.internal/sms-gateway/app.go (1)
36-62: Module factory conversion looks solid.Returning an
fx.Optionkeeps the consumer API consistent while giving us an immutable module wiring; the call sites invokingModule()now read clearly. Nice cleanup.internal/sms-gateway/modules/webhooks/service.go (1)
92-100: Constructor reuse keeps Replace concise.Switching to
newWebhook(...)centralizes the persistence shape and the move to the sharedErrInvalidEventkeeps validation errors uniform with the rest of the package.internal/sms-gateway/modules/webhooks/models.go (1)
38-41: Migration error wrapping appreciated.Using
new(Webhook)with wrapped errors aligns this migration with the other modules and should make diagnosing schema issues noticeably easier.internal/sms-gateway/modules/health/health.go (1)
22-51: Please double-check the tighter thresholds.Dropping the memory budget from roughly 1 GiB to 128 MiB and the goroutine cap to 100 is a big jump; if current workloads already hover above those numbers we’ll end up reporting WARN continuously. Can we confirm recent production metrics or make these probes configurable before shipping?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/sms-gateway/modules/db/health.go (1)
17-23: Optional: Remove redundant initialization.The explicit initialization of
failedPings: atomic.Int64{}is redundant sinceatomic.Int64is a struct type that will be zero-initialized automatically. You can simplify this to:func newHealth(db *sql.DB) *health { return &health{ db: db, - - failedPings: atomic.Int64{}, } }internal/sms-gateway/handlers/mobile.go (1)
69-69: Optional: Remove redundant nil assignment.In Go, struct fields are zero-initialized, so explicitly setting
Device: nilis unnecessary.- Device: nil, + Device: nil, // or simply omit if you prefer implicit zero-valueOr omit it entirely if the struct field should remain at its zero value.
internal/sms-gateway/modules/sse/service.go (2)
46-46: Redundant explicit initialization.The zero value of
sync.RWMutexis valid and ready to use in Go. The explicit initialization here is redundant, though harmless.Apply this diff if you prefer the more idiomatic approach:
- mu: sync.RWMutex{}, + mu: sync.RWMutex{}, // or simply omit this lineAlternatively, you can simply omit the line entirely since Go's zero value initialization handles it.
157-163: Simplify ticker handling pattern.The anonymous function pattern for conditional ticker handling is overly complex and creates a new channel on each iteration when the ticker is disabled. A simpler and more idiomatic approach is to use a nil channel, which blocks forever in a select statement.
Apply this diff to simplify the ticker handling:
+ var tickerChan <-chan time.Time + if ticker != nil { + tickerChan = ticker.C + } + for { select { case event := <-conn.channel: s.metrics.ObserveEventDeliveryLatency(func() { if err := s.writeToStream(w, fmt.Sprintf("event: %s\ndata: %s", event.name, utils.UnsafeString(event.data))); err != nil { s.logger.Warn("Failed to write event data", zap.String("device_id", deviceID), zap.String("connection_id", conn.id), zap.Error(err)) return } }) - // Conditionally handle ticker events - case <-func() <-chan time.Time { - if ticker != nil { - return ticker.C - } - // Return nil channel that never fires when disabled - return make(chan time.Time) - }(): + case <-tickerChan: if err := s.writeToStream(w, ":keepalive"); err != nil {When
tickerChanis nil, that case will never be selected by the select statement, which is the desired behavior for disabled keepalives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
internal/config/config.go(3 hunks)internal/config/module.go(1 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/mobile.go(8 hunks)internal/sms-gateway/handlers/module.go(1 hunks)internal/sms-gateway/handlers/upstream.go(3 hunks)internal/sms-gateway/models/migration.go(2 hunks)internal/sms-gateway/models/models.go(2 hunks)internal/sms-gateway/models/module.go(1 hunks)internal/sms-gateway/modules/cleaner/module.go(1 hunks)internal/sms-gateway/modules/cleaner/service.go(2 hunks)internal/sms-gateway/modules/db/health.go(3 hunks)internal/sms-gateway/modules/db/module.go(2 hunks)internal/sms-gateway/modules/devices/module.go(1 hunks)internal/sms-gateway/modules/devices/repository.go(3 hunks)internal/sms-gateway/modules/devices/repository_filter.go(1 hunks)internal/sms-gateway/modules/devices/service.go(6 hunks)internal/sms-gateway/modules/messages/service.go(11 hunks)internal/sms-gateway/modules/metrics/handler.go(1 hunks)internal/sms-gateway/modules/metrics/module.go(1 hunks)internal/sms-gateway/modules/sse/config.go(2 hunks)internal/sms-gateway/modules/sse/errors.go(1 hunks)internal/sms-gateway/modules/sse/metrics.go(3 hunks)internal/sms-gateway/modules/sse/module.go(1 hunks)internal/sms-gateway/modules/sse/service.go(8 hunks)
✅ Files skipped from review due to trivial changes (3)
- internal/sms-gateway/modules/sse/metrics.go
- internal/sms-gateway/models/module.go
- internal/sms-gateway/modules/devices/repository_filter.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sms-gateway/app.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-10T23:45:45.502Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: internal/sms-gateway/online/module.go:14-16
Timestamp: 2025-09-10T23:45:45.502Z
Learning: fx.Decorate in uber-go/fx creates module-scoped decorated dependencies, not global modifications. Using fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("module_name") }) is a correct pattern for creating module-specific loggers that are scoped only to that module's dependency graph.
Applied to files:
internal/sms-gateway/handlers/module.gointernal/sms-gateway/modules/cleaner/module.gointernal/sms-gateway/modules/devices/module.gointernal/sms-gateway/modules/metrics/module.go
📚 Learning: 2025-07-22T00:34:31.896Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 154
File: internal/sms-gateway/modules/sse/service.go:89-92
Timestamp: 2025-07-22T00:34:31.896Z
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.
Applied to files:
internal/sms-gateway/modules/sse/errors.gointernal/sms-gateway/modules/sse/service.go
📚 Learning: 2025-07-22T00:31:28.796Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 154
File: internal/sms-gateway/modules/sse/service.go:67-79
Timestamp: 2025-07-22T00:31:28.796Z
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.
Applied to files:
internal/sms-gateway/modules/sse/service.go
🧬 Code graph analysis (15)
internal/sms-gateway/modules/sse/module.go (2)
internal/sms-gateway/app.go (1)
Module(36-62)internal/sms-gateway/modules/sse/service.go (2)
NewService(42-52)Service(21-29)
internal/sms-gateway/modules/sse/config.go (1)
internal/config/config.go (1)
Config(10-20)
internal/sms-gateway/handlers/module.go (1)
internal/sms-gateway/app.go (1)
Module(36-62)
internal/sms-gateway/modules/cleaner/module.go (1)
internal/sms-gateway/app.go (1)
Module(36-62)
internal/sms-gateway/modules/messages/service.go (6)
internal/sms-gateway/modules/messages/repository_filter.go (3)
Order(7-7)SelectFilter(16-23)SelectOptions(51-62)internal/sms-gateway/modules/devices/repository_filter.go (2)
SelectFilter(9-9)WithUserID(23-27)internal/sms-gateway/modules/webhooks/repository_filter.go (4)
SelectFilter(5-5)WithExtID(7-11)WithDeviceID(41-46)WithUserID(13-17)internal/sms-gateway/modules/messages/models.go (3)
MessageState(129-134)ProcessingState(12-12)MessageRecipient(111-117)internal/sms-gateway/modules/messages/repository.go (1)
ErrMessageNotFound(18-18)internal/sms-gateway/modules/messages/errors.go (1)
ValidationError(9-9)
internal/sms-gateway/models/migration.go (1)
internal/sms-gateway/models/models.go (2)
User(18-24)Device(34-45)
internal/sms-gateway/modules/db/health.go (1)
internal/sms-gateway/modules/health/types.go (2)
Checks(67-67)Provider(69-75)
internal/sms-gateway/modules/devices/repository.go (1)
internal/sms-gateway/models/models.go (1)
Device(34-45)
internal/config/module.go (9)
internal/sms-gateway/app.go (1)
Module(36-62)internal/sms-gateway/modules/sse/config.go (4)
Option(5-5)Config(7-9)NewConfig(19-27)WithKeepAlivePeriod(33-41)internal/config/config.go (12)
Config(10-20)Default(89-127)HTTP(27-33)Database(44-56)Gateway(22-25)Tasks(64-66)API(35-38)OpenAPI(40-42)Messages(76-79)SSE(72-74)Cache(81-83)PubSub(85-87)internal/sms-gateway/modules/auth/service.go (1)
Config(22-25)internal/sms-gateway/modules/push/service.go (1)
Config(23-30)internal/sms-gateway/modules/messages/config.go (1)
Config(5-8)internal/sms-gateway/pubsub/config.go (1)
Config(4-7)internal/sms-gateway/handlers/config.go (1)
Config(3-11)internal/sms-gateway/modules/devices/config.go (1)
Config(5-7)
internal/sms-gateway/handlers/mobile.go (1)
internal/sms-gateway/models/models.go (2)
Device(34-45)User(18-24)
internal/sms-gateway/modules/devices/service.go (5)
internal/sms-gateway/modules/devices/config.go (1)
Config(5-7)internal/sms-gateway/models/models.go (1)
Device(34-45)internal/sms-gateway/modules/db/module.go (1)
IDGen(14-14)internal/sms-gateway/modules/devices/repository_filter.go (4)
SelectFilter(9-9)WithUserID(23-27)WithID(11-15)ActiveWithin(29-33)internal/sms-gateway/modules/devices/repository.go (1)
ErrNotFound(13-13)
internal/sms-gateway/modules/sse/service.go (2)
internal/sms-gateway/modules/sse/errors.go (1)
ErrNoConnection(6-6)internal/sms-gateway/modules/sse/metrics.go (1)
ErrorTypeWriteFailure(21-21)
internal/config/config.go (3)
internal/sms-gateway/modules/messages/config.go (1)
Config(5-8)internal/sms-gateway/pubsub/config.go (1)
Config(4-7)internal/sms-gateway/cache/config.go (1)
Config(4-6)
internal/sms-gateway/modules/devices/module.go (3)
internal/sms-gateway/app.go (1)
Module(36-62)internal/sms-gateway/modules/sse/module.go (1)
Module(10-32)internal/sms-gateway/modules/messages/module.go (1)
Module(11-35)
internal/sms-gateway/modules/metrics/module.go (7)
internal/config/module.go (1)
Module(23-143)internal/sms-gateway/app.go (1)
Module(36-62)internal/sms-gateway/handlers/module.go (1)
Module(15-41)internal/sms-gateway/modules/cleaner/module.go (1)
Module(28-38)internal/sms-gateway/modules/db/module.go (1)
Module(16-26)internal/sms-gateway/modules/devices/module.go (1)
Module(16-35)internal/sms-gateway/modules/sse/module.go (1)
Module(10-32)
⏰ 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: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (39)
internal/sms-gateway/handlers/upstream.go (2)
68-68: Validation error fix looks good!This change correctly addresses the previous critical issue by returning HTTP 400 Bad Request for validation errors instead of 502 Bad Gateway.
97-103: Good refactoring with named constants.Extracting the rate limiting values into named constants improves code readability and makes future adjustments easier.
internal/sms-gateway/modules/db/health.go (2)
31-33: LGTM! Probe implementations are correct.The use of unnamed context parameters (
_) for unused contexts is idiomatic Go. Returningnil, nilfor probes that don't report checks is reasonable, and thenolint:nilnilcomments appropriately document this intentional pattern for empty results.Also applies to: 58-60
62-62: LGTM! Interface assertion updated correctly.The change from
healthmod.HealthProvidertohealthmod.Providercorrectly reflects the interface rename in the health module.internal/sms-gateway/handlers/mobile.go (6)
99-101: Body parsing error handling fixed correctly.The previous issue where
fiber.StatusBadGateway(502) was returned for body parsing errors has been resolved. The handler now correctly returnsfiber.StatusBadRequest(400) for client-side validation failures.
150-168: LGTM: Improved error handling.The function correctly returns
400 Bad Requestfor body parsing errors and implements proper error logging with generic client-facing error messages, which is a good security practice.
180-192: LGTM: Consistent error handling.The error handling follows the same pattern as other handlers, logging detailed errors while returning generic messages to clients.
207-221: Previous issue resolved.The body parsing error handling now correctly returns
fiber.StatusBadRequest(400) instead offiber.StatusBadGateway(502), addressing the previous review feedback.
236-243: LGTM: Validator function improvements.The validator function correctly uses an underscore for the unused context parameter and provides better error context through error wrapping.
124-124: No issues found; dereferencing is correct.The
RegisterDevicemethod signature expectsuser models.User(a value type), not a pointer. The code correctly dereferences*userto match this parameter type.internal/sms-gateway/modules/devices/service.go (1)
9-10: math/rand/v2 is supported.The module's go directive is set to
go 1.24.1, which exceeds the minimum Go 1.22 requirement formath/rand/v2. No toolchain compatibility issues exist.internal/sms-gateway/models/migration.go (1)
14-17: Nice addition—migration errors now have context.Wrapping the
AutoMigratefailure with module-specific context tightens observability without altering behavior. 👍internal/sms-gateway/modules/sse/errors.go (1)
5-7: Good call introducing a sentinel.
ErrNoConnectiongives callers a stable error to match, which will simplify handling across the SSE surface.internal/sms-gateway/modules/sse/module.go (1)
10-31: Module() conversion looks solid.Function form keeps the same wiring while aligning with the new fx.Module pattern; lifecycle cleanup remains intact.
internal/sms-gateway/modules/metrics/module.go (1)
9-18: Metrics module refactor matches the new pattern.Switching to
Module()keeps DI wiring unchanged and updates the handler constructor naming to the newnewHTTPHandler.internal/sms-gateway/modules/devices/module.go (1)
16-33: Devices module wiring preserved.The
Module()helper mirrors existing behavior, and the explicitfx.Outkeeps the FxResult intent clear.internal/config/config.go (1)
89-126: Centralizing defaults pays off.The new
Default()helper gives a single source of truth for baseline config before env/YAML overrides, which modernizes the initialization flow.internal/sms-gateway/modules/cleaner/module.go (1)
28-37: Cleaner module factory pattern confirmedSwitching to a Module() factory keeps the scoped zap logger decoration and NewFx wiring intact, so downstream callers just need to invoke the factory. Based on learnings.
internal/sms-gateway/handlers/module.go (1)
15-40: Handlers wiring now exported as factoryThe new Module() factory keeps the handlers logger decoration and grouped provides exactly as before, matching the updated Fx integration style. Based on learnings.
internal/sms-gateway/modules/metrics/handler.go (1)
8-19: HTTPHandler rename improves export clarityThe all-caps acronym keeps naming idiomatic while preserving Register semantics; constructor stays minimal and focused.
internal/sms-gateway/modules/devices/repository.go (1)
39-104: Nil model usage avoids per-call allocationsUsing typed nil pointers with Model/Delete keeps GORM targeting the devices table while side-stepping needless struct allocations; Exists still short-circuits cleanly.
internal/config/module.go (1)
22-142: App config module factory keeps providers intactThe new Module() factory still loads defaults, scopes Config privately, and exposes the same downstream provider set while documenting constants like the SSE write timeout.
internal/sms-gateway/modules/sse/service.go (5)
17-19: LGTM!Good practice extracting the buffer size into a named constant for improved maintainability.
62-62: LGTM!The error wrapping with device ID context is a good improvement that aligns with Go error handling best practices and enables better error tracing.
Also applies to: 98-98
79-83: Also applies to: 85-89, 227-231
127-131: LGTM!Good refactoring that separates HTTP setup concerns from streaming logic, improving testability and code organization.
180-190: LGTM!The error wrapping with descriptive messages improves error tracing and makes debugging easier.
internal/sms-gateway/modules/messages/service.go (12)
18-18: LGTM: Adding functional programming utilities.The
lolibrary is well-established for functional transformations and is used appropriately throughout this file.
77-88: LGTM: Method signature updated consistently.The signature change from
MessagesOrdertoOrderaligns with the broader type refactoring, and the default value handling is appropriate.
104-114: LGTM: Clean functional transformation.The use of
lo.MapToSliceappropriately converts the states map to a slice ofMessageStatestructs. SettingID: 0is correct for auto-generated GORM IDs.
117-123: LGTM: Clear error handling with appropriate failure semantics.The renamed error variables (
updErr,cacheErr) improve readability, and treating cache errors as non-critical (warn-only) is appropriate for this operation.
130-143: LGTM: Signature refactored consistently.The method signature change from
MessagesSelectFilter/MessagesSelectOptionstoSelectFilter/SelectOptionsaligns with the repository layer refactoring.
145-178: LGTM: Improved error handling and caching strategy.The caching strategy correctly handles "not found" cases by storing nil entries (lines 151-153), preventing repeated database lookups. The consistent error handling with
cacheErris appropriate.
255-261: LGTM: Improved log readability.The multi-line format for structured logging improves readability when multiple context fields are present.
270-275: LGTM: Enhanced error wrapping.The error wrapping with
%wprovides better error context and maintains the error chain forerrors.Ischecks.
288-292: LGTM: Cleaner functional implementation.The refactor to
lo.Mapwith thenewMessageRecipienthelper is more concise and maintains the same logic.
312-317: LGTM: Consistent helper usage.The use of
newMessageRecipienthelper maintains consistency with the refactoredrecipientsToModelmethod.
352-361: LGTM: Custom error type for validation.The use of
ValidationErroras a custom string type for phone validation errors is a valid Go pattern, assumingValidationErrorimplements theError()method (standard for typed error patterns).
91-94: The*new(SelectFilter)pattern is valid but unconventional.The builder methods (
WithExtID,WithDeviceID) onSelectFilteruse pointer receivers and return*SelectFilter. The code creates a pointer withnew(), chains methods on it, then dereferences with*to pass a value toGet().This works correctly but sacrifices readability. More idiomatic approaches:
- Assign to a variable:
sf := new(SelectFilter); sf.WithExtID(...).WithDeviceID(...)- Or redesign builders to return values instead of pointers for fluent chaining
The current pattern is not incorrect, but consider simplifying for maintainability.
| const idGenSize = 21 | ||
| idGen, _ := nanoid.Standard(idGenSize) |
There was a problem hiding this comment.
Handle or document the ignored error.
The error return from nanoid.Standard(idGenSize) is silently discarded. While 21 is a valid size and the call is unlikely to fail, ignoring errors during initialization can hide unexpected failures and make debugging difficult.
Consider handling the error:
- const idGenSize = 21
- idGen, _ := nanoid.Standard(idGenSize)
+ const idGenSize = 21
+ idGen, err := nanoid.Standard(idGenSize)
+ if err != nil {
+ params.Logger.Fatal("failed to initialize ID generator", zap.Error(err))
+ }Alternatively, if you're confident the error can never occur with this constant size, add a comment explaining why it's safe to ignore:
const idGenSize = 21
- idGen, _ := nanoid.Standard(idGenSize)
+ // nanoid.Standard with size 21 never returns an error
+ idGen, _ := nanoid.Standard(idGenSize)📝 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.
| const idGenSize = 21 | |
| idGen, _ := nanoid.Standard(idGenSize) | |
| const idGenSize = 21 | |
| // nanoid.Standard with size 21 never returns an error | |
| idGen, _ := nanoid.Standard(idGenSize) |
🤖 Prompt for AI Agents
In internal/sms-gateway/handlers/mobile.go around lines 275 to 276, the error
returned by nanoid.Standard(idGenSize) is being ignored; either capture and
handle the error (e.g., check the returned error and fail fast with a clear
log/panic if it occurs, or propagate it) or add a concise comment explaining why
the call cannot fail for the constant size (and thus why ignoring the error is
safe). Ensure the chosen approach is applied consistently and includes a short
message describing the failure if you opt to handle the error.
Summary by CodeRabbit
New Features
Bug Fixes
Chores