[settings] notify user on real changes only #287
Conversation
🤖 Pull request artifacts
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughImporter.import now returns a Boolean; each module's import was updated to return true when any handled key actually changed. SettingsService collects those Booleans and only emits notifications when at least one module reported a change. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SettingsService
participant ModuleA
participant ModuleB
Client->>SettingsService: update(importedData)
rect rgb(230,245,255)
Note over SettingsService: call each Module.import(data) and collect Boolean results
SettingsService->>ModuleA: import(data)
ModuleA-->>SettingsService: true / false
SettingsService->>ModuleB: import(data)
ModuleB-->>SettingsService: true / false
end
rect rgb(235,255,230)
Note over SettingsService: aggregate results (.any { it })
alt any true
SettingsService->>SettingsService: emit change notification
else all false
SettingsService-->>Client: return early (no notification)
end
end
SettingsService-->>Client: Done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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
🧹 Nitpick comments (4)
app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt (1)
51-53: Simplify the change detection condition.The condition
!changed.any { it == true }works correctly but can be simplified.Apply this diff:
- if (!changed.any { it == true }) { + if (changed.none { it == true }) { return }Or, if you adopt
mapNotNullfrom the previous suggestion:- if (!changed.any { it == true }) { + if (changed.none { it }) { return }app/src/main/java/me/capcom/smsgateway/modules/encryption/EncryptionSettings.kt (1)
43-55: Improve type safety in the comparison.Line 47 compares
passphrase(String?) withit.value(Any?) without explicit type conversion. While this may work due to Kotlin's equality semantics, it's clearer and safer to explicitly convert the value to String.Apply this diff:
when (it.key) { PASSPHRASE -> { - val changed = passphrase != it.value - storage.set(it.key, it.value?.toString()) + val newValue = it.value?.toString() + val changed = passphrase != newValue + storage.set(it.key, newValue) changed }This makes the type conversion explicit and avoids potential confusion.
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt (1)
69-75: Improve type safety in the comparison.Line 70 compares
privateToken(String?) withit.value(Any?) without explicit type conversion, similar to the issue in EncryptionSettings.Apply this diff:
PRIVATE_TOKEN -> { - val changed = privateToken != it.value + val newValue = it.value?.toString() + val changed = privateToken != newValue - storage.set(it.key, it.value?.toString()) + storage.set(it.key, newValue) changed }app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt (1)
63-69: Minor type safety improvement.Line 64 compares
this.signingKey(String) withvalue?.toString()(String?) directly. While this works, extracting the new value makes the comparison more explicit.Apply this diff:
SIGNING_KEY -> { - val changed = this.signingKey != value?.toString() + val newValue = value?.toString() + val changed = newValue != null && this.signingKey != newValue - storage.set(key, value?.toString()) + if (newValue != null) { + storage.set(key, newValue) + } changed }Note: This also adds a null check to avoid storing null signing keys, maintaining the invariant that
signingKeyalways has a value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/src/main/java/me/capcom/smsgateway/modules/encryption/EncryptionSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/logs/LogsSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/ping/PingSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/settings/Importer.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt(1 hunks)
🔇 Additional comments (6)
app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt (1)
41-49: Verify nullable handling in change detection.The
mapoperation producesList<Boolean?>because(it as? Importer)?.import(...)returnsnullwhen the cast fails or the interface is not implemented. The subsequent check on line 51 treatsnullas "no change," which is correct but implicit.Consider making the null handling explicit for clarity:
- val changed = data.map { (key, value) -> + val changed = data.mapNotNull { (key, value) -> try { settings[key]?.let { (it as? Importer)?.import(value as Map<String, *>) } } catch (e: IllegalArgumentException) { throw IllegalArgumentException("Failed to import $key: ${e.message}", e) } }Using
mapNotNullmakes it clear that null results (from settings that don't implement Importer) are filtered out before checking for changes.app/src/main/java/me/capcom/smsgateway/modules/settings/Importer.kt (1)
4-4: LGTM!The interface change to return
Booleanis straightforward and enables change detection across all implementations.app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt (1)
56-67: LGTM!The URL validation and change detection for
CLOUD_URLare correctly implemented.app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (1)
138-144: LGTM!The change detection logic for
LIMIT_PERIOD,LIMIT_VALUE,SIM_SELECTION_MODE,PROCESSING_ORDER, andLOG_LIFETIME_DAYSis correctly implemented with proper type conversions and validation.Also applies to: 146-157, 159-168, 170-179, 181-191
app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt (2)
42-48: LGTM!The change detection for
INTERNET_REQUIREDcorrectly converts and compares boolean values.
50-61: LGTM!The change detection for
RETRY_COUNTcorrectly validates the value, handles the comparison, and updates storage.
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/src/main/java/me/capcom/smsgateway/modules/ping/PingSettings.kt (1)
31-41: Unnecessary float conversion for integer values.Line 32 uses
value?.toString()?.toFloat()?.toInt()which unnecessarily converts through Float and can introduce precision issues for large integers. This issue was previously flagged in past reviews.app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (2)
116-126: Unnecessary float conversion for integer values.Line 117 uses the unnecessary float conversion pattern
value?.toString()?.toFloat()?.toInt()forSEND_INTERVAL_MIN. This was previously flagged in past reviews.
128-138: Unnecessary float conversion for integer values.Line 129 uses the unnecessary float conversion pattern
value?.toString()?.toFloat()?.toInt()forSEND_INTERVAL_MAX. This was previously flagged in past reviews.
🧹 Nitpick comments (4)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (3)
140-146: Eliminate duplicate valueOf call.
Period.valueOf(it.toString())is evaluated twice (lines 141 and 143). Compute it once and reuse to improve readability and avoid redundant work.Apply this diff:
LIMIT_PERIOD -> { - val changed = this.limitPeriod != value?.let { Period.valueOf(it.toString()) } - - storage.set(key, value?.let { Period.valueOf(it.toString()) }) + val newValue = value?.let { Period.valueOf(it.toString()) } + val changed = this.limitPeriod != newValue + + storage.set(key, newValue) changed }
161-170: Eliminate duplicate valueOf call.
SimSelectionMode.valueOf(it.toString())is evaluated twice (lines 162-163 and 165-167). Compute it once and reuse.Apply this diff:
SIM_SELECTION_MODE -> { - val changed = - this.simSelectionMode != value?.let { SimSelectionMode.valueOf(it.toString()) } - - storage.set( - key, - value?.let { SimSelectionMode.valueOf(it.toString()) }) + val newValue = value?.let { SimSelectionMode.valueOf(it.toString()) } + val changed = this.simSelectionMode != newValue + + storage.set(key, newValue) changed }
172-181: Eliminate duplicate valueOf call.
ProcessingOrder.valueOf(it.toString())is evaluated twice (lines 173-174 and 176-178). Compute it once and reuse.Apply this diff:
PROCESSING_ORDER -> { - val changed = - this.processingOrder != value?.let { ProcessingOrder.valueOf(it.toString()) } - - storage.set( - key, - value?.let { ProcessingOrder.valueOf(it.toString()) }) + val newValue = value?.let { ProcessingOrder.valueOf(it.toString()) } + val changed = this.processingOrder != newValue + + storage.set(key, newValue) changed }app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt (1)
42-48: Eliminate duplicate toBoolean call.
value?.toString()?.toBoolean()is evaluated twice (lines 43 and 45). Compute it once and reuse for consistency with theSIGNING_KEYcase (lines 63-70).Apply this diff:
INTERNET_REQUIRED -> { - val changed = this.internetRequired != value?.toString()?.toBoolean() - - storage.set(key, value?.toString()?.toBoolean()) + val newValue = value?.toString()?.toBoolean() + val changed = this.internetRequired != newValue + + storage.set(key, newValue) changed }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/main/java/me/capcom/smsgateway/modules/encryption/EncryptionSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/logs/LogsSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/ping/PingSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/me/capcom/smsgateway/modules/logs/LogsSettings.kt
- app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build insecure / build-apk
- GitHub Check: Build release / build-apk
🔇 Additional comments (3)
app/src/main/java/me/capcom/smsgateway/modules/encryption/EncryptionSettings.kt (1)
43-56: Clean implementation of change detection.The implementation correctly computes the new value once, compares it with the current value, stores it, and returns the change flag. This is the correct pattern.
app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt (2)
50-61: Good implementation pattern.The
RETRY_COUNTcase correctly computesretryCountonce, validates it, compares it, and stores it. This is the recommended pattern.
63-70: Good implementation pattern.The
SIGNING_KEYcase correctly computesnewValueonce, compares it, and stores it. This is the recommended pattern that should be followed by the other cases.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (2)
128-138: Fix the float conversion for integer values.The
toString()?.toFloat()?.toInt()pattern is unnecessary and lossy. UsetoIntOrNull()for direct integer parsing.Apply this diff:
SEND_INTERVAL_MAX -> { - val newValue = value?.toString()?.toFloat()?.toInt() + val newValue = value?.toString()?.toIntOrNull() val changed = this.sendIntervalMax != newValue storage.set( key, newValue?.toString() ) changed }Note: This issue was also flagged in previous review comments.
116-126: Fix float conversion and false positive change detection.Two issues:
Float conversion (previously flagged): The
toString()?.toFloat()?.toInt()pattern is unnecessary and lossy for integer values.False positive detection: When storage is null and importing null, this reports a change because
this.sendIntervalMin(returns 0 via default) is compared withnull, evaluating to0 != null→true. However, the storage remains null → null (no actual change).Apply this diff to fix both issues:
SEND_INTERVAL_MIN -> { - val newValue = value?.toString()?.toFloat()?.toInt() + val newValue = value?.toString()?.toIntOrNull() ?: 0 val changed = this.sendIntervalMin != newValue storage.set( key, - newValue?.toString() + newValue.toString() ) changed }This applies the same default (0) to imported null values as the getter does, ensuring consistent effective-value comparison.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build release / build-apk
- GitHub Check: Build insecure / build-apk
🔇 Additional comments (3)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (3)
110-110: Method signature change looks good.The addition of
Booleanreturn type correctly implements the PR objective to detect and report actual changes.
180-190: LGTM!The change detection logic is correct. The nullable property with validation is properly handled, and the comparison accurately detects effective value changes.
192-194: LGTM!The aggregation logic correctly returns
trueonly when at least one key reports a change. Theelse -> falsefor unrecognized keys is appropriate.
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt
Show resolved
Hide resolved
capcom6
force-pushed
the
settings/fix-changes-notification
branch
from
a43bfea to
a233f85
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
app/src/main/java/me/capcom/smsgateway/modules/ping/PingSettings.kt (1)
32-38: Avoid float round-trips when parsing integers.Line 32 currently funnels integer parsing through
toFloat()?.toInt(). That round-trip can silently round large values (e.g., 16,777,217 becomes 16,777,216) and breaks the PR goal of only flagging real changes. Parse the integer directly instead.- val newValue = value?.toString()?.toFloat()?.toInt() + val newValue = value?.toString()?.toIntOrNull()app/src/main/java/me/capcom/smsgateway/modules/logs/LogsSettings.kt (1)
51-55: Parse integers without float conversion.Line 51 repeats the
toFloat()?.toInt()round-trip, introducing the same precision-loss risk noted previously. Switch to direct integer parsing so unchanged large values aren’t misdetected as changes.- val newValue = it.value?.toString()?.toFloat()?.toInt() + val newValue = it.value?.toString()?.toIntOrNull()app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (2)
116-126: Fix unnecessary float conversion and false positive for null values.Line 117 uses the lossy pattern
?.toString()?.toFloat()?.toInt()for an integer setting, and Line 118 compares the getter (which defaults to 0) with a nullablenewValue. When both storage and import are null, this reports0 != nullas a change even though the effective value remains 0.Apply this diff to parse directly to Int and apply the default before comparison:
SEND_INTERVAL_MIN -> { - val newValue = value?.toString()?.toFloat()?.toInt() + val newValue = value?.toString()?.toIntOrNull() ?: 0 val changed = this.sendIntervalMin != newValue storage.set( key, - newValue?.toString() + newValue.toString() ) changed }
128-138: Remove unnecessary float conversion.Line 129 uses the lossy pattern
?.toString()?.toFloat()?.toInt()for an integer setting. Parse directly to Int.Apply this diff:
SEND_INTERVAL_MAX -> { - val newValue = value?.toString()?.toFloat()?.toInt() + val newValue = value?.toString()?.toIntOrNull() val changed = this.sendIntervalMax != newValue storage.set( key, newValue?.toString() ) changed }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/src/main/java/me/capcom/smsgateway/modules/encryption/EncryptionSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/logs/LogsSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/ping/PingSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/settings/Importer.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt
🔇 Additional comments (7)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (7)
110-111: LGTM! Change tracking approach is sound.The function signature now returns Boolean, and the
map { ... }.any { it }pattern correctly aggregates per-entry change flags to report whether any setting changed.
140-147: Good fix! Default is now applied before comparison.Line 141 correctly applies
?: Period.Disabledto match the getter's default (Line 50), eliminating the false positive when importing null with null storage.
149-160: Good fix! Default is now applied in comparison.Line 155 correctly uses
(limitValue ?: 0)to match the getter's default (Line 52), eliminating the false positive when importing null with null storage.
162-170: Good fix! Default is now applied before comparison.Lines 163-164 correctly apply
?: SimSelectionMode.OSDefaultto match the getter's default (Line 55), eliminating the false positive when importing null with null storage.
172-180: Good fix! Default is now applied before comparison.Lines 173-174 correctly apply
?: ProcessingOrder.LIFOto match the getter's default (Line 61), eliminating the false positive when importing null with null storage.
194-194: LGTM! Unhandled keys correctly return false.The else branch ensures that unrecognized keys don't contribute to the change flag.
196-196: LGTM! Aggregation correctly tracks if any entry changed.The
.any { it }aggregation returns true if any setting entry reported a change.
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build insecure / build-apk
- GitHub Check: Build release / build-apk
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (3)
182-192: False positive due to getter's filter and unnecessary float conversion.Line 188 compares the getter's filtered value (which returns null for values ≤ 0 via
takeIf { it > 0 }at line 58) against the raw imported value. This causes false positives when importing non-positive values if such values exist in storage.Example: If storage contains
-5, the getter returnsnull. Importing-5again would hit the validation error. But importingnullcomparesnull != nullwhich works correctly only by coincidence.Additionally, line 183 uses an unnecessary float conversion for an integer value.
Apply this diff to fix both issues:
LOG_LIFETIME_DAYS -> { - val logLifetimeDays = value?.toString()?.toFloat()?.toInt() + val logLifetimeDays = value?.toString()?.toIntOrNull() if (logLifetimeDays != null && logLifetimeDays < 1) { throw IllegalArgumentException("Log lifetime days must be >= 1") } - val changed = this.logLifetimeDays != logLifetimeDays + val effectiveNew = logLifetimeDays?.takeIf { it > 0 } + val changed = this.logLifetimeDays != effectiveNew storage.set(key, logLifetimeDays?.toString()) changed }This applies the same filter as the getter before comparison and removes the float conversion.
128-138: Remove unnecessary float conversion for integer value.Line 129 unnecessarily converts through float for an integer setting.
Apply this diff:
SEND_INTERVAL_MAX -> { - val newValue = value?.toString()?.toFloat()?.toInt() + val newValue = value?.toString()?.toIntOrNull() val changed = this.sendIntervalMax != newValue storage.set( key, newValue?.toString() ) changed }
116-126: False positive when importing null for SEND_INTERVAL_MIN.Line 118 compares the getter's default value (0) against a nullable
newValue. When storage is empty and you import null, this reports a change (0 != null= true) even though the effective value remains 0 before and after import.Additionally, line 117 uses an unnecessary float conversion for an integer value.
Apply this diff to fix both issues:
SEND_INTERVAL_MIN -> { - val newValue = value?.toString()?.toFloat()?.toInt() - val changed = this.sendIntervalMin != newValue + val newValue = value?.toString()?.toIntOrNull() ?: 0 + val changed = this.sendIntervalMin != newValue storage.set( key, - newValue?.toString() + newValue.toString() ) changed }This ensures null imports are treated as 0 (consistent with the getter's default) and removes the unnecessary float conversion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build release / build-apk
- GitHub Check: Build insecure / build-apk
🔇 Additional comments (4)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (4)
140-147: LGTM! Default handling fixed.The default
Period.Disabledis now correctly applied tonewValuebefore comparison (line 141), eliminating the false positive flagged in previous reviews.
162-170: LGTM! Default handling fixed.The default
SimSelectionMode.OSDefaultis correctly applied tonewValuebefore comparison (lines 163-164), eliminating the false positive flagged in previous reviews.
172-180: LGTM! Default handling fixed.The default
ProcessingOrder.LIFOis correctly applied tonewValuebefore comparison (lines 173-174), eliminating the false positive flagged in previous reviews.
194-196: LGTM! Aggregation logic is correct.The
elsebranch correctly returnsfalsefor unknown keys, and the.any { it }aggregation (line 196) properly returnstrueonly when at least one setting has actually changed.
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt
Show resolved
Hide resolved
capcom6
force-pushed
the
settings/fix-changes-notification
branch
from
d8523f4 to
69fc3f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (2)
150-158: Please drop the float round-trip for limit_value.
Line 151 still parses withtoFloat()?.toInt(), solimit_value = 16777217would be stored as16777216. This was already flagged earlier; we should parse without losing precision.- val limitValue = value?.toString()?.toFloat()?.toInt() + val limitValue = value?.toString()?.toDoubleOrNull()?.toInt()
117-126: Stop flagging null → default transitions on send_interval_min.
Line 118 compares the rawnewValuetosendIntervalMin, which falls back to0whenever nothing is stored. If the import sendsnull(or the backup omitted the key) and we were already at the default,sendIntervalMinis0whilenewValueisnull, sochangedbecomestrueeven though we persistnulland remain at the default. Normalise the incoming value to the same effective representation and avoid the float round-trip.- val newValue = value?.toString()?.toFloat()?.toInt() - val changed = this.sendIntervalMin != newValue - - storage.set( - key, - newValue?.toString() - ) + val rawValue = value?.toString()?.toDoubleOrNull()?.toInt() + val normalisedNew = rawValue ?: 0 + val changed = sendIntervalMin != normalisedNew + + storage.set(key, rawValue?.toString())
🧹 Nitpick comments (1)
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt (1)
56-67: Consider validating empty strings for CLOUD_URL.The current validation (lines 58-60) only checks non-null URLs for the
https://prefix. Empty strings would pass through but are unlikely to be valid URLs. Consider whether empty strings should be rejected or treated as null.Example enhancement:
CLOUD_URL -> { - val url = it.value?.toString() + val url = it.value?.toString()?.takeIf { it.isNotEmpty() } if (url != null && !url.startsWith("https://")) { throw IllegalArgumentException("url must start with https://") } val changed = storage.get<String?>(CLOUD_URL) != url storage.set(it.key, url) changed }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/src/main/java/me/capcom/smsgateway/modules/encryption/EncryptionSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/logs/LogsSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/ping/PingSettings.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/settings/Importer.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/me/capcom/smsgateway/modules/settings/Importer.kt
- app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt
🔇 Additional comments (2)
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt (2)
69-76: LGTM: PRIVATE_TOKEN change detection is correct.The comparison correctly checks the actual stored value (which can be null) against the new value.
53-55: LGTM: Overall approach aligns with PR objective.The refactored implementation using
map+anycorrectly aggregates per-entry change flags and returnstrueonly when at least one setting actually changed, which fulfills the PR objective to "notify user on real changes only."Also applies to: 78-80
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/logs/LogsSettings.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/ping/PingSettings.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt
Show resolved
Hide resolved
capcom6
force-pushed
the
settings/fix-changes-notification
branch
from
df69365 to
d456d3f
Compare
Summary by CodeRabbit