Draft
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (36)
📝 WalkthroughWalkthroughThe PR refactors device management by introducing a new ChangesDevice Module Migration & Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 7
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/handlers/messages/3rdparty.go (1)
253-253:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale OpenAPI annotation — body type mismatch.
The
@Paramannotation still referencessmsgateway.MessagesExportRequest, but the handler now parsessmsgateway.InboxRefreshRequest(line 263). The generated OpenAPI spec will document the wrong request schema.📝 Proposed fix
-// `@Param` request body smsgateway.MessagesExportRequest true "Export inbox request" +// `@Param` request body smsgateway.InboxRefreshRequest true "Export inbox request"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sms-gateway/handlers/messages/3rdparty.go` at line 253, The OpenAPI `@Param` annotation is stale: it declares the request body as smsgateway.MessagesExportRequest while the handler actually parses smsgateway.InboxRefreshRequest; update the annotation line that currently contains "@Param request body smsgateway.MessagesExportRequest" to reference "smsgateway.InboxRefreshRequest" (or remove the `@Param` if the handler no longer accepts a body) so the generated OpenAPI schema matches the handler's parsed type (see the handler parsing of smsgateway.InboxRefreshRequest).
🧹 Nitpick comments (6)
internal/sms-gateway/modules/devices/models.go (1)
24-24: 💤 Low valueConsider specifying an explicit JSON column type for
SimCardsWithout a
typetag, GORM's JSON serializer falls back tolongtexton MySQL instead of a properjsoncolumn. If structured queries or JSON-specific indexing are ever needed, the column type should be specified explicitly.♻️ Proposed improvement
- SimCards []simCardModel `gorm:"serializer:json"` + SimCards []simCardModel `gorm:"serializer:json;type:json"`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sms-gateway/modules/devices/models.go` at line 24, The SimCards field on the device model is using GORM's JSON serializer but lacks an explicit column type, causing MySQL to default to longtext; update the struct tag on SimCards (the SimCards []simCardModel field in models.go) to include a type declaration for JSON (e.g., add type:json in the gorm tag) so the column is created as a native JSON type and supports JSON queries/indexing.internal/sms-gateway/handlers/converters/devices_test.go (1)
14-74: ⚡ Quick winMissing test coverage for SIM card conversion — the primary feature of this PR
All three test cases use an empty
DeviceUpdate{}(noSimCards), so thedevices.SimCard → smsgateway.SimCardconversion path in the converter is completely untested. At minimum, add a case with one or more populated SIM cards and verify the output DTO matches.♻️ Suggested additional test case
+ { + name: "device with sim cards", + device: devices.Device{ + DeviceInput: devices.DeviceInput{ + DeviceInfo: devices.DeviceInfo{ + DeviceUpdate: devices.DeviceUpdate{ + SimCards: []devices.SimCard{ + { + SlotIndex: 0, + SimNumber: 1, + PhoneNumber: lo.ToPtr("+79990001234"), + CarrierName: lo.ToPtr("Carrier"), + ICCID: lo.ToPtr("8901260000000000000"), + }, + }, + }, + }, + ID: "test-id", + }, + }, + expected: smsgateway.Device{ + ID: "test-id", + SimCards: []smsgateway.SimCard{ + { + SlotIndex: 0, + SimNumber: 1, + PhoneNumber: lo.ToPtr("+79990001234"), + CarrierName: lo.ToPtr("Carrier"), + ICCID: lo.ToPtr("8901260000000000000"), + }, + }, + }, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sms-gateway/handlers/converters/devices_test.go` around lines 14 - 74, TestDeviceToDTO currently never exercises the devices.SimCard → smsgateway.SimCard path; add a new case in the tests slice inside TestDeviceToDTO that constructs a devices.Device whose DeviceUpdate contains one or more populated devices.SimCard entries (with fields like ICCID, MSISDN, Provider, Status, CreatedAt/UpdatedAt) and set the expected smsgateway.Device to include the corresponding smsgateway.SimCard entries, then call converters.DeviceToDTO and assert equality; ensure you reference the existing TestDeviceToDTO test, the converters.DeviceToDTO function, and the devices.SimCard / smsgateway.SimCard types so the mapping of each field (including nil/empty edge cases) is validated.internal/sms-gateway/handlers/settings/mobile.go (1)
16-17: ⚡ Quick winRemove unused
devicesSvcdependency fromMobileController
devicesSvcis injected and stored but never referenced in theget()method. Since theDeviceobject is passed directly to the handler, the service dependency is redundant and can be removed from the struct field, constructor parameter, and assignment.Constructor refactor
func NewMobileController( settingsSvc *settings.Service, logger *zap.Logger, validator *validator.Validate, ) *MobileController { return &MobileController{ Handler: base.Handler{ Logger: logger, Validator: validator, }, settingsSvc: settingsSvc, } }Also applies to: 20-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sms-gateway/handlers/settings/mobile.go` around lines 16 - 17, The MobileController currently stores an unused devicesSvc dependency; remove the devicesSvc field from the MobileController struct, remove the devicesSvc parameter from NewMobileController, and delete any assignment of devicesSvc in the constructor so only settingsSvc, Logger and Validator remain; ensure the get() handler signature and implementation (get()) continue to accept the Device object directly and compile after removing references to devicesSvc.internal/sms-gateway/modules/events/service.go (1)
120-120: ⚡ Quick winLocal variable
devicesshadows the imported packagedevices.While harmless now (all package-level references appear before the assignment), this is a refactoring hazard — any future code added after line 120 that tries to reference the package will fail to compile or silently refer to the wrong thing.
♻️ Proposed fix
- devices, err := s.deviceSvc.Select(ctx, wrapper.UserID, filters...) + devList, err := s.deviceSvc.Select(ctx, wrapper.UserID, filters...) if err != nil { ... } - if len(devices) == 0 { + if len(devList) == 0 { ... } - for _, device := range devices { + for _, device := range devList {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sms-gateway/modules/events/service.go` at line 120, The local variable name "devices" in the assignment from s.deviceSvc.Select(ctx, wrapper.UserID, filters...) shadows the imported package "devices"; rename the local variable to a non-conflicting identifier (e.g., deviceList or fetchedDevices) wherever s.deviceSvc.Select is called to avoid shadowing the package and update any subsequent references to that variable accordingly so future code can still reference the devices package without ambiguity.internal/sms-gateway/modules/devices/domain.go (2)
11-11: ⚡ Quick win
AuthTokeninDeviceInputis a sensitive credential — guard against accidental serialization.
DeviceInput(and by embedding,Device) carriesAuthToken string. If aDeviceorDeviceInputvalue is ever accidentally passed to a JSON encoder, the token will leak. The DTO converter today manually picks fields, but there's no structural guard.Consider annotating
AuthTokenwithjson:"-"to make accidental serialization safe by default.🛡️ Proposed fix
type DeviceInput struct { DeviceInfo ID string UserID string - AuthToken string + AuthToken string `json:"-"` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sms-gateway/modules/devices/domain.go` at line 11, DeviceInput (and by embedding Device) contains the sensitive AuthToken string which can leak if the struct is ever JSON-encoded; update the AuthToken field definition to forbid JSON serialization (e.g., add the struct tag json:"-") so accidental encoding won't include the token, and apply the same tag to any embedded Device AuthToken field if present to ensure the credential is protected by default.
38-44: ⚡ Quick win
SlotIndexandSimNumberhave ambiguous semantics.
SimCardhas bothSlotIndex intandSimNumber int. It's unclear what distinguishes them — isSlotIndexzero-based andSimNumberone-based? Is one the physical slot and the other a logical SIM index? Without documentation or at minimum distinct naming, this will be confusing to both implementors and API consumers.Consider adding Go doc comments clarifying the meaning and range of each field.
📝 Example doc addition
type SimCard struct { + // SlotIndex is the zero-based index of the physical SIM slot (0, 1, ...). SlotIndex int + // SimNumber is the one-based subscription number used internally by the device. SimNumber int PhoneNumber *string CarrierName *string ICCID *string }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sms-gateway/modules/devices/domain.go` around lines 38 - 44, The SlotIndex and SimNumber fields in the SimCard struct are ambiguous; add Go doc comments above the SimCard type and above each field (SlotIndex and SimNumber) that clearly states their semantics and valid ranges (e.g., whether SlotIndex is zero-based physical slot, SimNumber is one-based logical SIM id, or vice versa), what each represents (physical slot vs logical SIM index), and any constraints (min/max or nil handling for PhoneNumber/ICCID), using the exact symbol names SimCard, SlotIndex, and SimNumber so readers and API consumers understand the distinction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sms-gateway/handlers/mobile.go`:
- Around line 198-209: The current call to h.devicesSvc.Update builds
devices.DeviceUpdate using lo.EmptyableToPtr(req.PushToken) and
lo.Map(req.SimCards, ...), which drops whether the client actually sent those
fields and causes unintended destructive updates; change the construction so you
only set PushToken and SimCards on devices.DeviceUpdate when the corresponding
request fields were present: check req.PushToken presence (preserve nil vs empty
vs value) and only assign PushToken when present, and likewise only assign
SimCards when req.SimCards is present (map to []devices.SimCard only in that
branch), leaving those fields unset on DeviceUpdate otherwise so
devicesSvc.Update can decide not to modify them. Reference symbols:
h.devicesSvc.Update, devices.DeviceUpdate, req.PushToken, req.SimCards,
lo.EmptyableToPtr, lo.Map.
In `@internal/sms-gateway/modules/auth/service.go`:
- Around line 99-103: The background goroutine that calls s.onlineSvc.SetOnline
is incorrectly deriving a timeout context from the handler-scoped ctx (which may
be a fasthttp RequestCtx) causing unsafe retention of request-scoped state;
change the context creation inside the goroutine from context.WithTimeout(ctx,
timeout) to use a non-request context (e.g.,
context.WithTimeout(context.Background(), timeout) or a server-level context
like s.ctx/s.shutdownCtx) so the goroutine does not capture the handler
RequestCtx, keep the same timeout/cancel pattern and still call
s.onlineSvc.SetOnline(subCtx, id).
In `@internal/sms-gateway/modules/devices/domain.go`:
- Around line 34-36: The IsEmpty method currently uses a pointer receiver and
checks for nil, which breaks calls on value-typed devices; change the receiver
to a value receiver on Device (i.e., func (d Device) IsEmpty() bool) and
simplify the body to only check d.ID == "" (remove the nil check), so existing
handlers that accept devices.Device by value can call device.IsEmpty() without
compilation errors.
In `@internal/sms-gateway/modules/devices/models.go`:
- Line 102: The error string currently uses "models migration failed" which is
misleading for this module; update the error message to use the module name
"devices" (e.g., change the fmt.Errorf call in the migration function in
internal/sms-gateway/modules/devices/models.go so it returns "devices migration
failed: %w" instead of "models migration failed: %w") so logs consistently
identify the devices module (look for the migration function that returns
fmt.Errorf in this file).
In `@internal/sms-gateway/modules/devices/service.go`:
- Around line 133-135: The Update method currently writes through to the
repository but does not clear the in-memory cache used by GetByToken, so callers
may see stale device data; modify Service.Update to, after calling
s.devices.Update(ctx, id, device), invalidate the cache entries that GetByToken
uses (e.g., delete the cache key(s) for the device token and/or device id), by
either calling the existing cache delete method (the same cache used in
GetByToken) or by adding a small helper like invalidateDeviceCache(id) that
removes those keys; ensure the invalidation occurs only after a successful
update and covers all keys GetByToken may have stored.
In `@internal/sms-gateway/modules/webhooks/models.go`:
- Around line 25-26: The DeviceID struct tag uses type:varchar(21) which
conflicts with the migration that defines the column as char(21); update the
DeviceID field's gorm tag to use type:char(21) (retain the json tag and index)
so the struct matches the actual DB column, e.g. modify the DeviceID field
definition and keep the existing Device relationship (Device
*devices.DeviceModel and its gorm
foreignKey:DeviceID;constraint:OnDelete:CASCADE) unchanged.
In `@internal/sms-gateway/openapi/docs.go`:
- Around line 2271-2295: Update the SimCard struct in
internal/sms-gateway/modules/devices/domain.go by adding validation tags to the
non-pointer integer fields so OpenAPI marks them required and constrained: add
`validate:"required"` to SlotIndex and to SimNumber, and add bounds to SimNumber
as `validate:"required,min=1,max=3"` (or equivalent combined tag) so the
generated schema reflects mandatory presence and the 1–3 range.
---
Outside diff comments:
In `@internal/sms-gateway/handlers/messages/3rdparty.go`:
- Line 253: The OpenAPI `@Param` annotation is stale: it declares the request body
as smsgateway.MessagesExportRequest while the handler actually parses
smsgateway.InboxRefreshRequest; update the annotation line that currently
contains "@Param request body smsgateway.MessagesExportRequest" to reference
"smsgateway.InboxRefreshRequest" (or remove the `@Param` if the handler no longer
accepts a body) so the generated OpenAPI schema matches the handler's parsed
type (see the handler parsing of smsgateway.InboxRefreshRequest).
---
Nitpick comments:
In `@internal/sms-gateway/handlers/converters/devices_test.go`:
- Around line 14-74: TestDeviceToDTO currently never exercises the
devices.SimCard → smsgateway.SimCard path; add a new case in the tests slice
inside TestDeviceToDTO that constructs a devices.Device whose DeviceUpdate
contains one or more populated devices.SimCard entries (with fields like ICCID,
MSISDN, Provider, Status, CreatedAt/UpdatedAt) and set the expected
smsgateway.Device to include the corresponding smsgateway.SimCard entries, then
call converters.DeviceToDTO and assert equality; ensure you reference the
existing TestDeviceToDTO test, the converters.DeviceToDTO function, and the
devices.SimCard / smsgateway.SimCard types so the mapping of each field
(including nil/empty edge cases) is validated.
In `@internal/sms-gateway/handlers/settings/mobile.go`:
- Around line 16-17: The MobileController currently stores an unused devicesSvc
dependency; remove the devicesSvc field from the MobileController struct, remove
the devicesSvc parameter from NewMobileController, and delete any assignment of
devicesSvc in the constructor so only settingsSvc, Logger and Validator remain;
ensure the get() handler signature and implementation (get()) continue to accept
the Device object directly and compile after removing references to devicesSvc.
In `@internal/sms-gateway/modules/devices/domain.go`:
- Line 11: DeviceInput (and by embedding Device) contains the sensitive
AuthToken string which can leak if the struct is ever JSON-encoded; update the
AuthToken field definition to forbid JSON serialization (e.g., add the struct
tag json:"-") so accidental encoding won't include the token, and apply the same
tag to any embedded Device AuthToken field if present to ensure the credential
is protected by default.
- Around line 38-44: The SlotIndex and SimNumber fields in the SimCard struct
are ambiguous; add Go doc comments above the SimCard type and above each field
(SlotIndex and SimNumber) that clearly states their semantics and valid ranges
(e.g., whether SlotIndex is zero-based physical slot, SimNumber is one-based
logical SIM id, or vice versa), what each represents (physical slot vs logical
SIM index), and any constraints (min/max or nil handling for PhoneNumber/ICCID),
using the exact symbol names SimCard, SlotIndex, and SimNumber so readers and
API consumers understand the distinction.
In `@internal/sms-gateway/modules/devices/models.go`:
- Line 24: The SimCards field on the device model is using GORM's JSON
serializer but lacks an explicit column type, causing MySQL to default to
longtext; update the struct tag on SimCards (the SimCards []simCardModel field
in models.go) to include a type declaration for JSON (e.g., add type:json in the
gorm tag) so the column is created as a native JSON type and supports JSON
queries/indexing.
In `@internal/sms-gateway/modules/events/service.go`:
- Line 120: The local variable name "devices" in the assignment from
s.deviceSvc.Select(ctx, wrapper.UserID, filters...) shadows the imported package
"devices"; rename the local variable to a non-conflicting identifier (e.g.,
deviceList or fetchedDevices) wherever s.deviceSvc.Select is called to avoid
shadowing the package and update any subsequent references to that variable
accordingly so future code can still reference the devices package without
ambiguity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75cc8804-2390-438f-b267-1f233554478c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
go.modinternal/config/config.gointernal/sms-gateway/handlers/converters/devices.gointernal/sms-gateway/handlers/converters/devices_test.gointernal/sms-gateway/handlers/devices/3rdparty.gointernal/sms-gateway/handlers/events/mobile.gointernal/sms-gateway/handlers/messages/3rdparty.gointernal/sms-gateway/handlers/messages/mobile.gointernal/sms-gateway/handlers/middlewares/deviceauth/deviceauth.gointernal/sms-gateway/handlers/mobile.gointernal/sms-gateway/handlers/settings/mobile.gointernal/sms-gateway/handlers/webhooks/3rdparty.gointernal/sms-gateway/handlers/webhooks/mobile.gointernal/sms-gateway/jwt/metrics.gointernal/sms-gateway/models/migration.gointernal/sms-gateway/models/models.gointernal/sms-gateway/models/models_test.gointernal/sms-gateway/models/module.gointernal/sms-gateway/modules/auth/service.gointernal/sms-gateway/modules/devices/cache.gointernal/sms-gateway/modules/devices/domain.gointernal/sms-gateway/modules/devices/models.gointernal/sms-gateway/modules/devices/module.gointernal/sms-gateway/modules/devices/repository.gointernal/sms-gateway/modules/devices/service.gointernal/sms-gateway/modules/events/metrics.gointernal/sms-gateway/modules/events/service.gointernal/sms-gateway/modules/messages/models.gointernal/sms-gateway/modules/messages/service.gointernal/sms-gateway/modules/push/metrics.gointernal/sms-gateway/modules/sse/metrics.gointernal/sms-gateway/modules/webhooks/models.gointernal/sms-gateway/modules/webhooks/service.gointernal/sms-gateway/online/metrics.gointernal/sms-gateway/openapi/docs.gointernal/worker/config/config.gointernal/worker/executor/metrics.go
💤 Files with no reviewable changes (4)
- internal/sms-gateway/models/models.go
- internal/sms-gateway/models/migration.go
- internal/sms-gateway/models/models_test.go
- internal/sms-gateway/models/module.go
capcom6
force-pushed
the
devices/sim-cards-info
branch
from
0955da9 to
15ad054
Compare
There was a problem hiding this comment.
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/handlers/messages/3rdparty.go (1)
264-274:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
messageTypesandtriggerWebhooksfromInboxRefreshRequestare silently ignored.The OpenAPI schema documents both
messageTypesandtriggerWebhooksas valid request fields, but the handler parses them intoInboxRefreshRequestwithout forwarding them toinboxSvc.Refresh(). TheRefresh()method signature only acceptsuserID,deviceID,since, anduntil—the two new fields are never passed. Clients relying ontriggerWebhooks: falseto suppress webhook delivery ormessageTypesto filter re-exported messages will experience unexpected behavior with no error surfaced.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sms-gateway/handlers/messages/3rdparty.go` around lines 264 - 274, postInboxExport currently parses messageTypes and triggerWebhooks from InboxRefreshRequest but does not forward them to inboxSvc.Refresh; update the handler and service call so those fields are honored: modify the call in postInboxExport to pass req.MessageTypes and req.TriggerWebhooks (e.g., extend inboxSvc.Refresh signature to Refresh(userID string, deviceID *string, since, until time.Time, messageTypes []smsgateway.MessageType, triggerWebhooks *bool) or create a single options/params struct and pass req into it), then update the inboxSvc.Refresh implementation signature (and all callers) to accept and apply those new parameters so webhook suppression and message type filtering work as documented.
🧹 Nitpick comments (1)
internal/sms-gateway/handlers/converters/devices_test.go (1)
66-98: ⚡ Quick win
SlotIndex: 0is a zero-value and may mask a mapping omission.Both the input and expected fixture use
SlotIndex: 0, which is the Go zero value forint. If the converter accidentally omits this field, the test still passes because the uninitialized field would also be0. Use a non-zero value (e.g.,1) to confirm the field is actually being mapped.🛠️ Proposed fix
SimCards: []devices.SimCard{ { - SlotIndex: 0, + SlotIndex: 1, SimNumber: 1,SimCards: []smsgateway.SimCard{ { - SlotIndex: 0, + SlotIndex: 1, SimNumber: 1,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sms-gateway/handlers/converters/devices_test.go` around lines 66 - 98, The test uses SlotIndex: 0 which can hide mapping bugs; update the table case named "device with sim cards" so the input devices.SimCard and the expected smsgateway.SimCard both use a non-zero SlotIndex (e.g., 1) instead of 0 to ensure the converter actually maps SlotIndex in the conversion logic (look for the test case in devices_test.go and the structs devices.Device/devices.SimCard and smsgateway.SimCard to update).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/sms-gateway/handlers/messages/3rdparty.go`:
- Around line 264-274: postInboxExport currently parses messageTypes and
triggerWebhooks from InboxRefreshRequest but does not forward them to
inboxSvc.Refresh; update the handler and service call so those fields are
honored: modify the call in postInboxExport to pass req.MessageTypes and
req.TriggerWebhooks (e.g., extend inboxSvc.Refresh signature to Refresh(userID
string, deviceID *string, since, until time.Time, messageTypes
[]smsgateway.MessageType, triggerWebhooks *bool) or create a single
options/params struct and pass req into it), then update the inboxSvc.Refresh
implementation signature (and all callers) to accept and apply those new
parameters so webhook suppression and message type filtering work as documented.
---
Nitpick comments:
In `@internal/sms-gateway/handlers/converters/devices_test.go`:
- Around line 66-98: The test uses SlotIndex: 0 which can hide mapping bugs;
update the table case named "device with sim cards" so the input devices.SimCard
and the expected smsgateway.SimCard both use a non-zero SlotIndex (e.g., 1)
instead of 0 to ensure the converter actually maps SlotIndex in the conversion
logic (look for the test case in devices_test.go and the structs
devices.Device/devices.SimCard and smsgateway.SimCard to update).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccaebd13-02a5-4cc8-b538-13fd922c85dd
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
go.modinternal/sms-gateway/handlers/converters/devices.gointernal/sms-gateway/handlers/converters/devices_test.gointernal/sms-gateway/handlers/devices/3rdparty.gointernal/sms-gateway/handlers/events/mobile.gointernal/sms-gateway/handlers/messages/3rdparty.gointernal/sms-gateway/handlers/messages/mobile.gointernal/sms-gateway/handlers/middlewares/deviceauth/deviceauth.gointernal/sms-gateway/handlers/mobile.gointernal/sms-gateway/handlers/settings/mobile.gointernal/sms-gateway/handlers/webhooks/3rdparty.gointernal/sms-gateway/handlers/webhooks/mobile.gointernal/sms-gateway/jwt/metrics.gointernal/sms-gateway/models/migration.gointernal/sms-gateway/models/migrations/mysql/20260507035019_device_sim_cards.sqlinternal/sms-gateway/models/models.gointernal/sms-gateway/models/models_test.gointernal/sms-gateway/models/module.gointernal/sms-gateway/modules/auth/service.gointernal/sms-gateway/modules/devices/cache.gointernal/sms-gateway/modules/devices/domain.gointernal/sms-gateway/modules/devices/models.gointernal/sms-gateway/modules/devices/module.gointernal/sms-gateway/modules/devices/repository.gointernal/sms-gateway/modules/devices/service.gointernal/sms-gateway/modules/events/metrics.gointernal/sms-gateway/modules/events/service.gointernal/sms-gateway/modules/messages/models.gointernal/sms-gateway/modules/messages/service.gointernal/sms-gateway/modules/push/metrics.gointernal/sms-gateway/modules/sse/metrics.gointernal/sms-gateway/modules/webhooks/models.gointernal/sms-gateway/modules/webhooks/service.gointernal/sms-gateway/online/metrics.gointernal/sms-gateway/openapi/docs.gointernal/worker/executor/metrics.go
💤 Files with no reviewable changes (4)
- internal/sms-gateway/models/models_test.go
- internal/sms-gateway/models/module.go
- internal/sms-gateway/models/migration.go
- internal/sms-gateway/models/models.go
✅ Files skipped from review due to trivial changes (4)
- go.mod
- internal/sms-gateway/jwt/metrics.go
- internal/sms-gateway/models/migrations/mysql/20260507035019_device_sim_cards.sql
- internal/worker/executor/metrics.go
🚧 Files skipped from review as they are similar to previous changes (19)
- internal/sms-gateway/handlers/events/mobile.go
- internal/sms-gateway/modules/devices/module.go
- internal/sms-gateway/modules/sse/metrics.go
- internal/sms-gateway/handlers/webhooks/3rdparty.go
- internal/sms-gateway/handlers/webhooks/mobile.go
- internal/sms-gateway/handlers/devices/3rdparty.go
- internal/sms-gateway/modules/webhooks/models.go
- internal/sms-gateway/modules/events/metrics.go
- internal/sms-gateway/handlers/mobile.go
- internal/sms-gateway/handlers/messages/mobile.go
- internal/sms-gateway/handlers/converters/devices.go
- internal/sms-gateway/modules/devices/domain.go
- internal/sms-gateway/modules/devices/models.go
- internal/sms-gateway/modules/push/metrics.go
- internal/sms-gateway/modules/auth/service.go
- internal/sms-gateway/modules/messages/service.go
- internal/sms-gateway/online/metrics.go
- internal/sms-gateway/modules/devices/cache.go
- internal/sms-gateway/modules/devices/repository.go
🤖 Pull request artifacts
|
capcom6
force-pushed
the
devices/sim-cards-info
branch
from
15ad054 to
7d176aa
Compare
capcom6
force-pushed
the
devices/sim-cards-info
branch
from
86a1c57 to
8971974
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Chores