Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds a new SettingsGateway type and a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
🧹 Nitpick comments (1)
smsgateway/domain_settings.go (1)
140-151: Consider using*stringfor optional fields to distinguish "not set" from "empty".
CloudURLandNotificationChannelare plainstringwithomitempty, while other optional fields in sibling settings structs (e.g.,Passphrase,SigningKey) use*string. With value-type strings, a missing JSON field and an explicit""are indistinguishable after deserialization, which matters if these settings are applied as partial updates (PATCH semantics) — you can't express "clear this field" vs. "leave unchanged."If these fields are intentionally always-present or never need to be cleared, the current types are fine; otherwise, aligning with the pointer convention used elsewhere would be more consistent.
♻️ Suggested change
type SettingsGateway struct { // CloudURL is the URL of the cloud server. - CloudURL string `json:"cloud_url,omitempty" validate:"omitempty,url"` + CloudURL *string `json:"cloud_url,omitempty" validate:"omitempty,url"` // PrivateToken is the auth token for the private server. PrivateToken *string `json:"private_token,omitempty"` // NotificationChannel is the way device receives notifications. - NotificationChannel string `json:"notification_channel,omitempty" validate:"omitempty,oneof=AUTO SSE_ONLY"` + NotificationChannel *string `json:"notification_channel,omitempty" validate:"omitempty,oneof=AUTO SSE_ONLY"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smsgateway/domain_settings.go` around lines 140 - 151, The SettingsGateway struct currently defines CloudURL and NotificationChannel as plain strings which makes JSON "missing" vs explicit empty indistinguishable; change their types to *string (like PrivateToken) so callers can detect unset vs cleared values. Update the struct fields CloudURL and NotificationChannel to use *string, adjust any code that constructs or reads SettingsGateway (marshal/unmarshal, validation, and usages of CloudURL/NotificationChannel) to handle nil safely, and keep PrivateToken unchanged; ensure validation tags still apply or are adapted for pointer types where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@smsgateway/domain_settings.go`:
- Around line 140-151: The SettingsGateway struct currently defines CloudURL and
NotificationChannel as plain strings which makes JSON "missing" vs explicit
empty indistinguishable; change their types to *string (like PrivateToken) so
callers can detect unset vs cleared values. Update the struct fields CloudURL
and NotificationChannel to use *string, adjust any code that constructs or reads
SettingsGateway (marshal/unmarshal, validation, and usages of
CloudURL/NotificationChannel) to handle nil safely, and keep PrivateToken
unchanged; ensure validation tags still apply or are adapted for pointer types
where needed.
capcom6
force-pushed
the
client/add-gateway-settings
branch
from
83d2765 to
0a3e55a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
smsgateway/domain_settings.go (1)
142-151: Inconsistent use of value types vs pointer types for optional fields.
CloudURLandNotificationChannelarestringwhilePrivateTokenis*string. Every other optional field in sibling settings structs (SettingsEncryption,SettingsMessages,SettingsPing, etc.) uses pointer types to distinguish "not provided" (nil) from "set to zero value". If this is a settings-patch API, a barestringcan't differentiate between "field absent" and "field explicitly cleared," which could lead to unintended overwrites.Consider using
*stringfor consistency unless these fields are intentionally required whenGatewayis present.Suggested diff
type SettingsGateway struct { // CloudURL is the URL of the cloud server. Must not be used with Cloud Server. - CloudURL string `json:"cloud_url,omitempty" validate:"omitempty,url"` + CloudURL *string `json:"cloud_url,omitempty" validate:"omitempty,url"` // PrivateToken is the auth token for the private server. Must not be used with Cloud Server. PrivateToken *string `json:"private_token,omitempty"` // NotificationChannel is the way device receives notifications. - NotificationChannel string `json:"notification_channel,omitempty" validate:"omitempty,oneof=AUTO SSE_ONLY"` + NotificationChannel *string `json:"notification_channel,omitempty" validate:"omitempty,oneof=AUTO SSE_ONLY"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smsgateway/domain_settings.go` around lines 142 - 151, The struct SettingsGateway uses value strings for CloudURL and NotificationChannel but a pointer for PrivateToken, causing inconsistent semantics for "absent" vs "empty"; change CloudURL and NotificationChannel to *string (matching PrivateToken and sibling structs like SettingsEncryption, SettingsMessages, SettingsPing) so callers can distinguish nil (not provided) from empty string (explicitly cleared), and update any validation/usage code that reads SettingsGateway.CloudURL and SettingsGateway.NotificationChannel to handle pointer dereferencing and nil checks accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@smsgateway/domain_settings.go`:
- Around line 142-151: The struct SettingsGateway uses value strings for
CloudURL and NotificationChannel but a pointer for PrivateToken, causing
inconsistent semantics for "absent" vs "empty"; change CloudURL and
NotificationChannel to *string (matching PrivateToken and sibling structs like
SettingsEncryption, SettingsMessages, SettingsPing) so callers can distinguish
nil (not provided) from empty string (explicitly cleared), and update any
validation/usage code that reads SettingsGateway.CloudURL and
SettingsGateway.NotificationChannel to handle pointer dereferencing and nil
checks accordingly.
0a3e55a to
63c1007
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@smsgateway/domain_settings.go`:
- Around line 142-151: The SettingsGateway struct's CloudURL and PrivateToken
fields lack the same Cloud Server restriction tag used on Passphrase and
SigningKey; update the struct tags for CloudURL and PrivateToken (fields named
CloudURL and PrivateToken in SettingsGateway) to include
validate:"omitempty,isdefault" (keeping existing tags such as json and url) so
the "must not be used with Cloud Server" rule is enforced consistently.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
smsgateway/domain_settings.go (1)
149-150:NotificationChannelshould use a custom type instead of*string, consistent withLimitPeriod,SimSelectionMode, andMessagesProcessingOrder.All other enum-like fields in this file use a named string type with exported constants, giving library consumers type safety and IDE discoverability.
NotificationChannelwith*stringis the odd one out.♻️ Proposed refactor
Add the type and constants alongside the other enum types:
+// NotificationChannelMode defines how a device receives notifications. +type NotificationChannelMode string + +const ( + // NotificationChannelAuto uses the default notification channel. + NotificationChannelAuto NotificationChannelMode = "AUTO" + // NotificationChannelSSEOnly uses SSE for notifications. + NotificationChannelSSEOnly NotificationChannelMode = "SSE_ONLY" +)Then update the
SettingsGatewayfield:- // NotificationChannel is the way device receives notifications. - NotificationChannel *string `json:"notification_channel,omitempty" validate:"omitempty,oneof=AUTO SSE_ONLY"` + // NotificationChannel is the way device receives notifications. + NotificationChannel *NotificationChannelMode `json:"notification_channel,omitempty" validate:"omitempty,oneof=AUTO SSE_ONLY"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smsgateway/domain_settings.go` around lines 149 - 150, NotificationChannel is declared as *string but should be a named string type like the other enum-like fields (e.g., LimitPeriod, SimSelectionMode, MessagesProcessingOrder); add a new exported type (e.g., NotificationChannelType) with exported constants for the allowed values (AUTO, SSE_ONLY), replace the NotificationChannel field in the SettingsGateway struct to use *NotificationChannelType (preserve json tag and omitempty), and update any validation tags or switch/compare sites to use the new type and its constants for type-safety and IDE discoverability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@smsgateway/domain_settings.go`:
- Line 147: The linter flags the PrivateToken field for potential secret
exposure; either suppress the finding with an inline nosec if marshaling this
field as-is is intentional (add a `// `#nosec`` on the PrivateToken struct tag) or
implement redaction by adding a MarshalJSON on SettingsGateway that creates an
alias of the struct, replaces PrivateToken with a redacted string (e.g.,
"[REDACTED]") when non-nil, and returns json.Marshal of that masked alias so
normal API requests can still send the real token but any other marshaling
(e.g., logs) yields the redacted value.
---
Duplicate comments:
In `@smsgateway/domain_settings.go`:
- Around line 141-148: CloudURL and PrivateToken now correctly include the
`validate:"omitempty,isdefault"` tags matching the enforcement pattern used by
Passphrase and SigningKey; no code change is required — just keep the `CloudURL`
and `PrivateToken` fields in SettingsGateway with their current tags
(`CloudURL`, `PrivateToken`) and consider removing any duplicate review comments
or stale TODOs referencing this issue.
---
Nitpick comments:
In `@smsgateway/domain_settings.go`:
- Around line 149-150: NotificationChannel is declared as *string but should be
a named string type like the other enum-like fields (e.g., LimitPeriod,
SimSelectionMode, MessagesProcessingOrder); add a new exported type (e.g.,
NotificationChannelType) with exported constants for the allowed values (AUTO,
SSE_ONLY), replace the NotificationChannel field in the SettingsGateway struct
to use *NotificationChannelType (preserve json tag and omitempty), and update
any validation tags or switch/compare sites to use the new type and its
constants for type-safety and IDE discoverability.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Line 267: Remove the duplicate exhaustruct.exclude entries by keeping a single
instance of each regex pattern and deleting the repeated lines; specifically
remove the duplicate patterns "^github.com/gofiber/.+Config$",
"^gopkg.in/telebot.v4.LongPoller$", "^gopkg.in/telebot.v4.ReplyMarkup$", and
"^gopkg.in/telebot.v4.Settings$" so each appears only once in the
exhaustruct.exclude block.
- Line 4: The config header "## Golden config for golangci-lint v2.9.0" is
inconsistent with exclusions for gosec rules G117/G704 (introduced in gosec
2.23.0), so either update the header to indicate the config targets a newer
golangci-lint (e.g., change the header to mention v2.10.0+ or "latest") or
instead pin the CI workflow that runs golangci-lint to the older v2.9.0 to match
the file; locate the header string in .golangci.yml and the CI job that sets
golangci-lint version (e.g., GitHub Actions step using golangci-lint action or
version variable) and make the chosen change so header, config, and CI version
are consistent.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.golangci.yml:
- Around line 342-347: The golangci-lint config lists gosec excludes for rules
G117 and G704 but the config header still advertises v2.9.0, which doesn't
include those rules; update consistency by either changing the config header
version to v2.10.0 (or latest) so gosec includes G117/G704, or else remove those
excludes and pin CI to v2.9.0 to match; edit the .golangci.yml header version
string and/or the CI golangci-lint version to ensure the gosec excludes
(gosec.excludes -> G117, G704) match the actual linter version used.
capcom6
force-pushed
the
client/add-gateway-settings
branch
from
e11699e to
fbd9562
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
New Features
Refactor
Documentation
Chores