[localserver] add settings import and export#226
Conversation
WalkthroughThis update introduces comprehensive settings management, including new interfaces ( Changes
Sequence Diagram(s)Settings Update Flow (Local Server)sequenceDiagram
participant Client
participant WebService
participant SettingsRoutes
participant SettingsService
participant NotificationsService
Client->>WebService: PATCH /settings (with data)
WebService->>SettingsRoutes: handle PATCH
SettingsRoutes->>SettingsService: update(data)
SettingsService->>SettingsService: Import data into settings modules
SettingsService->>NotificationsService: notify settings changed
SettingsService-->>SettingsRoutes: updated settings
SettingsRoutes-->>Client: 200 OK (updated settings)
Periodic Settings Sync (Cloud Push/Event)sequenceDiagram
participant PushService
participant SettingsUpdateWorker
participant GatewayService
participant SettingsService
PushService->>SettingsUpdateWorker: Start on SettingsUpdated event
SettingsUpdateWorker->>GatewayService: getSettings()
GatewayService-->>SettingsUpdateWorker: settings map
SettingsUpdateWorker->>SettingsService: update(settings)
SettingsService-->>SettingsUpdateWorker: (done)
GET Settings API FlowsequenceDiagram
participant Client
participant WebService
participant SettingsRoutes
participant SettingsService
Client->>WebService: GET /settings
WebService->>SettingsRoutes: handle GET
SettingsRoutes->>SettingsService: getAll()
SettingsService-->>SettingsRoutes: settings map
SettingsRoutes-->>Client: 200 OK (settings map)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
app/src/main/java/me/capcom/smsgateway/modules/encryption/EncryptionSettings.kt (2)
9-9: Consider implementing theExporterinterface for consistencyThe class currently implements only the
Importerinterface while other settings classes implement bothExporterandImporter. For consistency and to support full export/import capabilities, consider implementing theExporterinterface as well.
43-49: Add validation for the passphraseThe import method directly converts any value to a string without validation. Since the passphrase is security-sensitive, consider adding validation before storing it.
override fun import(data: Map<String, *>) { data.forEach { (key, value) -> when (key) { - PASSPHRASE -> storage.set(key, value?.toString()) + PASSPHRASE -> { + val passphraseStr = value?.toString() + // Validate passphrase is not empty if provided + if (passphraseStr != null && passphraseStr.isNotBlank()) { + storage.set(key, passphraseStr) + } + } } } }app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt (2)
42-46: Inconsistency between export and import methodsThe
export()method only includes theCLOUD_URLfield, while theimport()method handles bothCLOUD_URLandPRIVATE_TOKEN. If this is intentional (for security reasons), consider adding a comment explaining whyPRIVATE_TOKENis write-only. Otherwise, consider including it in the export for consistency.override fun export(): Map<String, *> { return mapOf( CLOUD_URL to serverUrl, + // PRIVATE_TOKEN is excluded for security reasons + // Or include it if appropriate: + // PRIVATE_TOKEN to privateToken, ) }
48-55: Add validation for imported valuesThe current implementation converts values to strings without validation. Consider adding validation and error handling for the imported values to ensure they meet your requirements.
override fun import(data: Map<String, *>) { data.forEach { (key, value) -> when (key) { - CLOUD_URL -> storage.set(key, value?.toString()) - PRIVATE_TOKEN -> storage.set(key, value?.toString()) + CLOUD_URL -> { + val url = value?.toString() + if (url != null && url.isNotBlank() && (url.startsWith("http://") || url.startsWith("https://"))) { + storage.set(key, url) + } + } + PRIVATE_TOKEN -> { + val token = value?.toString() + if (token != null && token.isNotBlank()) { + storage.set(key, token) + } + } } } }app/src/main/java/me/capcom/smsgateway/modules/localserver/LocalServerSettings.kt (1)
49-55: Consider adding support for other settings in import methodThe current implementation only handles the PORT setting, but ignores other settings like ENABLED, DEVICE_ID, etc.
override fun import(data: Map<String, *>) { data.forEach { (key, value) -> when (key) { PORT -> storage.set(key, value?.toString()?.toFloat()?.toInt()?.toString()) + ENABLED -> value?.toString()?.toBooleanStrictOrNull()?.let { storage.set(key, it) } + DEVICE_ID -> value?.toString()?.let { storage.set(key, it) } + USERNAME -> value?.toString()?.let { storage.set(key, it) } } } }app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/SettingsRoutes.kt (2)
21-32: Add specific error types in GET responseThe error handling is good, but consider providing more specific error information in the response.
get { try { val settings = settingsService.getAll() call.respond(settings) } catch (e: Exception) { + e.printStackTrace() // Add this for consistency with the PATCH route call.respond( HttpStatusCode.InternalServerError, - mapOf("message" to "Failed to get settings: ${e.message}") + mapOf( + "message" to "Failed to get settings: ${e.message}", + "error" to e.javaClass.simpleName + ) ) } }
33-50: Consider adding input validationThe PATCH route correctly handles the settings update, but should validate the input data structure before attempting to apply changes.
patch { try { val settings = call.receive<Map<String, *>>() + + // Validate that we have valid module names + val invalidModules = settings.keys.filter { !settingsService.hasModule(it) } + if (invalidModules.isNotEmpty()) { + call.respond( + HttpStatusCode.BadRequest, + mapOf("message" to "Unknown module(s): ${invalidModules.joinToString()}") + ) + return@patch + } settingsService.apply(settings) call.respond( HttpStatusCode.OK, settingsService.getAll() ) } catch (e: Exception) { e.printStackTrace() call.respond( HttpStatusCode.BadRequest, mapOf("message" to "Invalid request: ${e.message}") ) } }Note: This requires adding a
hasModule(name: String)method to theSettingsServiceclass to check if a module name is valid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/src/main/java/me/capcom/smsgateway/modules/encryption/EncryptionSettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/localserver/LocalServerSettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/localserver/WebService.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/SettingsRoutes.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/logs/LogsSettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/notifications/NotificationsService.kt(4 hunks)app/src/main/java/me/capcom/smsgateway/modules/ping/PingSettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/settings/Exporter.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/Module.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt(2 hunks)app/src/main/res/drawable/notif_settings.xml(1 hunks)docs/api/swagger.json(26 hunks)
🔇 Additional comments (27)
app/src/main/java/me/capcom/smsgateway/modules/settings/Exporter.kt (1)
1-5: Well-designed interface for settings export.The
Exporterinterface establishes a clean contract for settings serialization with a single method that returns a map of configuration values. This design supports a consistent export mechanism across different settings classes, enabling centralized configuration management.app/src/main/res/drawable/notif_settings.xml (1)
1-12: Appropriate vector drawable for settings notification.The settings icon follows Android's material design guidelines with appropriate sizing (24dp) and styling. The white tint allows the system to apply theme-appropriate coloring in notifications.
app/src/main/java/me/capcom/smsgateway/modules/localserver/WebService.kt (1)
175-178: Good implementation of settings route.The settings route is properly implemented following the same pattern as other authenticated routes. It's appropriately placed behind authentication protection, which is essential for secure settings management.
app/src/main/java/me/capcom/smsgateway/modules/settings/Importer.kt (1)
1-5: Well-designed interface for settings import.The
Importerinterface provides a clean contract for settings deserialization with a single method that accepts a map of configuration values. This complements theExporterinterface to create a complete import/export system for application settings.app/src/main/java/me/capcom/smsgateway/modules/settings/Module.kt (1)
13-14: LGTM: Clean registration of SettingsService singletonThe addition of the SettingsService as a singleton in the Koin module is well-implemented.
Also applies to: 18-18
app/src/main/java/me/capcom/smsgateway/modules/logs/LogsSettings.kt (1)
41-45: LGTM: Export implementation is clean and straightforwardThe export method is well-implemented, providing a clear mapping of the lifetimeDays setting.
app/src/main/java/me/capcom/smsgateway/modules/localserver/LocalServerSettings.kt (2)
4-5: Implementation of Exporter and Importer interfaces looks goodThe class now properly implements the Exporter and Importer interfaces which enables settings management through a unified interface.
Also applies to: 11-11
43-47: Export implementation is clean and straightforwardThe export method correctly maps the PORT setting to its current value.
app/src/main/java/me/capcom/smsgateway/modules/ping/PingSettings.kt (2)
3-4: Implementation of Exporter and Importer interfaces looks goodThe class now properly implements the Exporter and Importer interfaces which enables settings management through a unified interface.
Also applies to: 10-10
22-26: Export implementation is clean and straightforwardThe export method correctly maps the INTERVAL_SECONDS setting to its current value.
app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt (2)
4-5: Implementation of Exporter and Importer interfaces looks goodThe class now properly implements the Exporter and Importer interfaces which enables settings management through a unified interface.
Also applies to: 11-11
32-37: Export implementation is clean and straightforwardThe export method correctly maps the INTERNET_REQUIRED and RETRY_COUNT settings to their current values.
app/src/main/java/me/capcom/smsgateway/modules/notifications/NotificationsService.kt (5)
24-25: New notification type for settings changes added correctlyThe code properly adds a new notification type with corresponding icon for settings changes.
27-31: Good use of builder pattern for notification configurationCreating a map of builder functions allows for clean and extensible configuration of different notification types.
46-48: Helpful convenience method for notificationsThe new notify method simplifies the process of sending notifications by handling the creation and display in one step.
55-63: Well-implemented notification action with PendingIntentThe code properly sets up a PendingIntent to launch the app when the notification is tapped and applies any additional configuration from the builders map.
Consider generating a unique request code for the PendingIntent instead of using 0 to avoid potential intent collisions if multiple notifications are created:
PendingIntent.getActivity( context, - 0, + id, // Use notification ID as request code for uniqueness context.packageManager.getLaunchIntentForPackage(context.packageName), PendingIntent.FLAG_IMMUTABLE )
74-74: Notification ID constant properly definedThe new notification ID constant is appropriately defined and follows the existing pattern.
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (3)
3-4: Good implementation of interfacesAdding the
ExporterandImporterinterfaces to support unified settings management is a good approach for extending functionality.
10-10: Interface implementation looks goodThe class now properly implements the
ExporterandImporterinterfaces.
89-98: Well-structured export implementationThe export method is well-structured and exports all relevant settings with their storage keys.
app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/SettingsRoutes.kt (1)
12-19: Good route registration design patternThe class follows a good design pattern for Ktor route registration with constructor dependency injection.
app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt (1)
24-32: Good organization of settings modulesThe approach of organizing settings by module with a map is clean and maintainable.
docs/api/swagger.json (5)
6-6: Good clarification in API descriptionThe updated description more clearly states that this API is for sending SMS messages.
489-555: Well-structured API documentation for settings endpointsThe settings endpoints are well-documented with proper request/response schemas and security requirements.
568-569: Good addition of Settings tagAdding a dedicated tag for Settings endpoints improves API documentation organization.
627-636: Good response definition for settingsThe response schema for settings is well-defined and reuses the Settings schema.
965-1077: Complete settings schema definitionThe Settings schema is comprehensive, covering all modules with appropriate property types and nullability.
One suggestion for future consideration: It might be helpful to add descriptions for each setting property to explain its purpose and constraints.
| override fun import(data: Map<String, *>) { | ||
| data.forEach { (key, value) -> | ||
| when (key) { | ||
| LIFETIME_DAYS -> storage.set(key, value?.toString()?.toFloat()?.toInt()?.toString()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the lifetimeDays conversion logic
The current conversion chain (toString()?.toFloat()?.toInt()?.toString()) is complex and error-prone. A simpler approach would be to convert directly to an integer and handle potential conversion errors.
override fun import(data: Map<String, *>) {
data.forEach { (key, value) ->
when (key) {
- LIFETIME_DAYS -> storage.set(key, value?.toString()?.toFloat()?.toInt()?.toString())
+ LIFETIME_DAYS -> {
+ try {
+ val days = when (value) {
+ is Int -> value
+ is Double, is Float -> value.toString().toFloat().toInt()
+ is String -> value.toIntOrNull()
+ else -> null
+ }
+
+ if (days != null && days > 0) {
+ storage.set(key, days.toString())
+ }
+ } catch (e: NumberFormatException) {
+ // Handle conversion error - maybe log it
+ }
+ }
}
}
}📝 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.
| override fun import(data: Map<String, *>) { | |
| data.forEach { (key, value) -> | |
| when (key) { | |
| LIFETIME_DAYS -> storage.set(key, value?.toString()?.toFloat()?.toInt()?.toString()) | |
| } | |
| } | |
| } | |
| override fun import(data: Map<String, *>) { | |
| data.forEach { (key, value) -> | |
| when (key) { | |
| LIFETIME_DAYS -> { | |
| try { | |
| val days = when (value) { | |
| is Int -> value | |
| is Double, is Float -> value.toString().toFloat().toInt() | |
| is String -> value.toIntOrNull() | |
| else -> null | |
| } | |
| if (days != null && days > 0) { | |
| storage.set(key, days.toString()) | |
| } | |
| } catch (e: NumberFormatException) { | |
| // Handle conversion error - maybe log it | |
| } | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/me/capcom/smsgateway/modules/logs/LogsSettings.kt around
lines 47 to 53, simplify the conversion of lifetimeDays by directly converting
the value to an integer instead of chaining toString, toFloat, toInt, and back
to string. Implement a safer conversion that attempts to parse the value as an
integer and handles any potential errors or nulls gracefully before storing it.
| override fun import(data: Map<String, *>) { | ||
| data.forEach { (key, value) -> | ||
| when (key) { | ||
| PORT -> storage.set(key, value?.toString()?.toFloat()?.toInt()?.toString()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify type conversion and add error handling in import method
The current type conversion chain is overly complex and lacks error handling for conversion failures.
override fun import(data: Map<String, *>) {
data.forEach { (key, value) ->
when (key) {
- PORT -> storage.set(key, value?.toString()?.toFloat()?.toInt()?.toString())
+ PORT -> try {
+ val portValue = value?.toString()?.toIntOrNull()
+ if (portValue != null && portValue > 0) {
+ storage.set(key, portValue.toString())
+ }
+ } catch (e: Exception) {
+ // Log error or handle invalid port value
+ }
}
}
}📝 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.
| override fun import(data: Map<String, *>) { | |
| data.forEach { (key, value) -> | |
| when (key) { | |
| PORT -> storage.set(key, value?.toString()?.toFloat()?.toInt()?.toString()) | |
| } | |
| } | |
| } | |
| override fun import(data: Map<String, *>) { | |
| data.forEach { (key, value) -> | |
| when (key) { | |
| PORT -> try { | |
| val portValue = value?.toString()?.toIntOrNull() | |
| if (portValue != null && portValue > 0) { | |
| storage.set(key, portValue.toString()) | |
| } | |
| } catch (e: Exception) { | |
| // Log error or handle invalid port value | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
app/src/main/java/me/capcom/smsgateway/modules/localserver/LocalServerSettings.kt
around lines 49 to 55, simplify the type conversion in the import method by
directly converting the value to an integer if possible, avoiding unnecessary
chaining through float and string conversions. Add error handling to catch any
exceptions or invalid values during conversion and handle them gracefully, such
as by logging an error or skipping the invalid entry, to prevent runtime
crashes.
| override fun import(data: Map<String, *>) { | ||
| data.forEach { (key, value) -> | ||
| when (key) { | ||
| INTERVAL_SECONDS -> storage.set( | ||
| key, | ||
| value?.toString()?.toFloat()?.toInt()?.toString() | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify type conversion and add error handling in import method
The current type conversion chain is overly complex and lacks error handling for conversion failures.
override fun import(data: Map<String, *>) {
data.forEach { (key, value) ->
when (key) {
- INTERVAL_SECONDS -> storage.set(
- key,
- value?.toString()?.toFloat()?.toInt()?.toString()
- )
+ INTERVAL_SECONDS -> try {
+ val intervalValue = value?.toString()?.toIntOrNull()
+ if (intervalValue != null && intervalValue >= 0) {
+ storage.set(key, intervalValue.toString())
+ }
+ } catch (e: Exception) {
+ // Log error or handle invalid interval value
+ }
}
}
}📝 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.
| override fun import(data: Map<String, *>) { | |
| data.forEach { (key, value) -> | |
| when (key) { | |
| INTERVAL_SECONDS -> storage.set( | |
| key, | |
| value?.toString()?.toFloat()?.toInt()?.toString() | |
| ) | |
| } | |
| } | |
| } | |
| override fun import(data: Map<String, *>) { | |
| data.forEach { (key, value) -> | |
| when (key) { | |
| INTERVAL_SECONDS -> try { | |
| val intervalValue = value?.toString()?.toIntOrNull() | |
| if (intervalValue != null && intervalValue >= 0) { | |
| storage.set(key, intervalValue.toString()) | |
| } | |
| } catch (e: Exception) { | |
| // Log error or handle invalid interval value | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/me/capcom/smsgateway/modules/ping/PingSettings.kt around
lines 28 to 37, simplify the type conversion in the import method by directly
converting the value to an integer in a safer way. Add error handling to catch
any exceptions or invalid conversions during this process, ensuring the method
does not fail silently or crash. Use a clear and concise approach to convert and
store the value, handling nulls and invalid formats gracefully.
| override fun import(data: Map<String, *>) { | ||
| data.forEach { (key, value) -> | ||
| when (key) { | ||
| INTERNET_REQUIRED -> storage.set(key, value?.toString()?.toBoolean()) | ||
| RETRY_COUNT -> storage.set(key, value?.toString()?.toFloat()?.toInt()?.toString()) | ||
| SIGNING_KEY -> storage.set(key, value?.toString()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type conversion and add error handling in import method
The current implementation has a complex conversion chain for RETRY_COUNT and lacks error handling for conversion failures.
override fun import(data: Map<String, *>) {
data.forEach { (key, value) ->
when (key) {
- INTERNET_REQUIRED -> storage.set(key, value?.toString()?.toBoolean())
- RETRY_COUNT -> storage.set(key, value?.toString()?.toFloat()?.toInt()?.toString())
+ INTERNET_REQUIRED -> value?.toString()?.toBooleanStrictOrNull()?.let { storage.set(key, it) }
+ RETRY_COUNT -> try {
+ val retryValue = value?.toString()?.toIntOrNull()
+ if (retryValue != null && retryValue >= 0) {
+ storage.set(key, retryValue.toString())
+ }
+ } catch (e: Exception) {
+ // Log error or handle invalid retry count
+ }
SIGNING_KEY -> storage.set(key, value?.toString())
}
}
}📝 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.
| override fun import(data: Map<String, *>) { | |
| data.forEach { (key, value) -> | |
| when (key) { | |
| INTERNET_REQUIRED -> storage.set(key, value?.toString()?.toBoolean()) | |
| RETRY_COUNT -> storage.set(key, value?.toString()?.toFloat()?.toInt()?.toString()) | |
| SIGNING_KEY -> storage.set(key, value?.toString()) | |
| } | |
| } | |
| } | |
| override fun import(data: Map<String, *>) { | |
| data.forEach { (key, value) -> | |
| when (key) { | |
| INTERNET_REQUIRED -> value | |
| ?.toString() | |
| ?.toBooleanStrictOrNull() | |
| ?.let { storage.set(key, it) } | |
| RETRY_COUNT -> try { | |
| val retryValue = value | |
| ?.toString() | |
| ?.toIntOrNull() | |
| if (retryValue != null && retryValue >= 0) { | |
| storage.set(key, retryValue.toString()) | |
| } | |
| } catch (e: Exception) { | |
| // Log error or handle invalid retry count | |
| } | |
| SIGNING_KEY -> storage.set(key, value?.toString()) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt
around lines 39 to 47, the import method uses a convoluted conversion chain for
RETRY_COUNT and lacks error handling for conversion failures. Simplify the
conversion by directly converting the value to an integer safely, and add
try-catch or null checks to handle any conversion errors gracefully, ensuring
the method does not throw exceptions on invalid input.
| override fun import(data: Map<String, *>) { | ||
| data.forEach { (key, value) -> | ||
| when (key) { | ||
| SEND_INTERVAL_MIN -> storage.set( | ||
| key, | ||
| value?.toString()?.toFloat()?.toInt()?.toString() | ||
| ) | ||
|
|
||
| SEND_INTERVAL_MAX -> storage.set( | ||
| key, | ||
| value?.toString()?.toFloat()?.toInt()?.toString() | ||
| ) | ||
|
|
||
| LIMIT_PERIOD -> storage.set(key, value?.let { Period.valueOf(it.toString()) }) | ||
| LIMIT_VALUE -> storage.set(key, value?.toString()?.toFloat()?.toInt()?.toString()) | ||
| SIM_SELECTION_MODE -> storage.set( | ||
| key, | ||
| value?.let { SimSelectionMode.valueOf(it.toString()) }) | ||
|
|
||
| LOG_LIFETIME_DAYS -> storage.set( | ||
| key, | ||
| value?.toString()?.toFloat()?.toInt()?.toString() | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for enum parsing
The import method has potential exception scenarios when parsing enum values that should be handled.
override fun import(data: Map<String, *>) {
data.forEach { (key, value) ->
when (key) {
SEND_INTERVAL_MIN -> storage.set(
key,
value?.toString()?.toFloat()?.toInt()?.toString()
)
SEND_INTERVAL_MAX -> storage.set(
key,
value?.toString()?.toFloat()?.toInt()?.toString()
)
- LIMIT_PERIOD -> storage.set(key, value?.let { Period.valueOf(it.toString()) })
+ LIMIT_PERIOD -> storage.set(key, value?.let {
+ try {
+ Period.valueOf(it.toString())
+ } catch (e: IllegalArgumentException) {
+ null // Or provide a default value like Period.Disabled
+ }
+ })
LIMIT_VALUE -> storage.set(key, value?.toString()?.toFloat()?.toInt()?.toString())
- SIM_SELECTION_MODE -> storage.set(
- key,
- value?.let { SimSelectionMode.valueOf(it.toString()) })
+ SIM_SELECTION_MODE -> storage.set(
+ key,
+ value?.let {
+ try {
+ SimSelectionMode.valueOf(it.toString())
+ } catch (e: IllegalArgumentException) {
+ null // Or provide a default value like SimSelectionMode.OSDefault
+ }
+ })
LOG_LIFETIME_DAYS -> storage.set(
key,
value?.toString()?.toFloat()?.toInt()?.toString()
)
}
}
}🤖 Prompt for AI Agents
In app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt
between lines 100 and 125, the import method parses enum values without error
handling, which can cause exceptions if the input is invalid. Wrap the enum
parsing calls for Period.valueOf and SimSelectionMode.valueOf in try-catch
blocks to catch IllegalArgumentException or other relevant exceptions, and
handle them gracefully, such as by logging the error or skipping the invalid
value, to prevent the method from crashing.
| fun apply(data: Map<String, *>) { | ||
| data.forEach { (key, value) -> | ||
| settings[key]?.let { | ||
| (it as? Importer)?.import(value as Map<String, *>) | ||
| } | ||
| } | ||
|
|
||
| notificationsService.notify( | ||
| context, | ||
| NotificationsService.NOTIFICATION_ID_SETTINGS_CHANGED, | ||
| "Settings changed via API. Restart the app to apply changes." | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and better error handling in apply()
The apply() method should validate input data and handle errors more gracefully.
fun apply(data: Map<String, *>) {
+ val errors = mutableListOf<String>()
+
data.forEach { (key, value) ->
settings[key]?.let {
- (it as? Importer)?.import(value as Map<String, *>)
+ if (it !is Importer) {
+ errors.add("Module '$key' does not support importing settings")
+ return@forEach
+ }
+
+ if (value !is Map<*, *>) {
+ errors.add("Settings for module '$key' must be a map")
+ return@forEach
+ }
+
+ try {
+ @Suppress("UNCHECKED_CAST")
+ it.import(value as Map<String, *>)
+ } catch (e: Exception) {
+ errors.add("Failed to import settings for module '$key': ${e.message}")
+ }
}
}
- notificationsService.notify(
- context,
- NotificationsService.NOTIFICATION_ID_SETTINGS_CHANGED,
- "Settings changed via API. Restart the app to apply changes."
- )
+ if (errors.isEmpty()) {
+ notificationsService.notify(
+ context,
+ NotificationsService.NOTIFICATION_ID_SETTINGS_CHANGED,
+ "Settings changed via API. Restart the app to apply changes."
+ )
+ } else {
+ throw IllegalArgumentException(errors.joinToString("\n"))
+ }
}Additionally, consider adding a public method to check if a module exists:
fun hasModule(name: String): Boolean {
return settings.containsKey(name)
}🤖 Prompt for AI Agents
In app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt
between lines 38 and 50, the apply() method lacks input validation and error
handling. Add checks to ensure that the value is a Map before casting and
importing, and handle any exceptions during import gracefully, possibly logging
errors without stopping the entire process. Also, add a public hasModule(name:
String): Boolean method that returns true if the settings map contains the given
module name, to allow external checks for module existence.
| fun getAll(): Map<String, *> { | ||
| return settings.mapValues { (it.value as? Exporter)?.export() } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling null values in getAll()
The getAll() method uses a safe cast but doesn't handle the case where a module isn't an Exporter.
fun getAll(): Map<String, *> {
- return settings.mapValues { (it.value as? Exporter)?.export() }
+ return settings.mapValues { (_, value) ->
+ (value as? Exporter)?.export() ?: emptyMap<String, Any>()
+ }
}📝 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.
| fun getAll(): Map<String, *> { | |
| return settings.mapValues { (it.value as? Exporter)?.export() } | |
| } | |
| fun getAll(): Map<String, *> { | |
| return settings.mapValues { (_, value) -> | |
| (value as? Exporter)?.export() ?: emptyMap<String, Any>() | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt at
lines 34 to 36, the getAll() method uses a safe cast to Exporter but does not
handle null values when the cast fails. Update the mapValues transformation to
provide a default value or handle the null case explicitly for entries where the
value is not an Exporter, ensuring the returned map does not contain nulls or
unexpected values.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt (2)
39-54: Improve type conversion and add error handling in import methodThe current implementation has a complex conversion chain and insufficient error handling for conversion failures. The retry count validation is good, but the code should handle potential conversion exceptions.
override fun import(data: Map<String, *>) { data.forEach { (key, value) -> when (key) { - INTERNET_REQUIRED -> storage.set(key, value?.toString()?.toBoolean()) + INTERNET_REQUIRED -> value?.toString()?.toBooleanStrictOrNull()?.let { storage.set(key, it) } RETRY_COUNT -> { - val retryCount = value?.toString()?.toInt() - if (retryCount != null && retryCount < 1) { - throw IllegalArgumentException("Retry count must be >= 1") - } - storage.set(key, retryCount?.toString()) + try { + val retryCount = value?.toString()?.toIntOrNull() + if (retryCount != null) { + if (retryCount < 1) { + throw IllegalArgumentException("Retry count must be >= 1") + } + storage.set(key, retryCount.toString()) + } + } catch (e: Exception) { + throw IllegalArgumentException("Invalid retry count: ${value}", e) + } } SIGNING_KEY -> storage.set(key, value?.toString()) } } }
43-49: 🛠️ Refactor suggestionConsider using toIntOrNull() instead of toInt() to safely handle conversions
The current implementation could throw NumberFormatException if the retry count is not a valid integer. Using toIntOrNull() would make the code more robust.
RETRY_COUNT -> { - val retryCount = value?.toString()?.toInt() + val retryCount = value?.toString()?.toIntOrNull() if (retryCount != null && retryCount < 1) { throw IllegalArgumentException("Retry count must be >= 1") } storage.set(key, retryCount?.toString()) }
🧹 Nitpick comments (2)
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt (2)
48-62: Good implementation with security check, but consider null handling improvements.The import method has appropriate validation for the HTTPS scheme, which is important for security. However, the current implementation has a few areas for improvement:
- The code passes potentially null URL to storage.set() without explicit handling
- Consider adding more comprehensive URL validation
Consider enhancing the null handling and URL validation:
override fun import(data: Map<String, *>) { data.forEach { (key, value) -> when (key) { CLOUD_URL -> { val url = value?.toString() if (url != null && !url.startsWith("https://")) { throw IllegalArgumentException("url must start with https://") } - storage.set(key, url) + // Only set non-null values or clear the setting if explicitly null + if (value != null) { + storage.set(key, url) + } else { + storage.remove(key) + } } - PRIVATE_TOKEN -> storage.set(key, value?.toString()) + PRIVATE_TOKEN -> { + if (value != null) { + storage.set(key, value.toString()) + } else { + storage.remove(key) + } + } } } }Note: The above suggestion assumes your KeyValueStorage has a remove() method. If not, adjust accordingly.
48-57: Consider adding KDoc comments for better documentation.The implemented methods would benefit from KDoc comments explaining their purpose, parameters, and potential exceptions.
Add documentation for clarity:
/** * Imports settings from a map of key-value pairs. * * @param data Map containing settings to import * @throws IllegalArgumentException if CLOUD_URL doesn't start with "https://" */ override fun import(data: Map<String, *>) { // Implementation... } /** * Exports current settings as a map of key-value pairs. * * @return Map containing exported settings */ override fun export(): Map<String, *> { // Implementation... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/SettingsRoutes.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/SettingsRoutes.kt
- app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt
- app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt
🔇 Additional comments (4)
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt (3)
3-4: Import statements added for interfaces - Good addition.The import statements for Exporter and Importer interfaces align with the PR objective of adding settings import/export functionality.
10-10: Well-implemented interface adoption.The class now implements the Exporter and Importer interfaces as part of the unified settings management system.
42-46: Export implementation only includes serverUrl.The export method correctly serializes the serverUrl under the CLOUD_URL key. However, I notice that privateToken is not included in the export while it's handled in the import method.
Is this asymmetry between export and import intentional? If privateToken is sensitive, consider adding a comment explaining why it's excluded from exports.
app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt (1)
4-5: Implementation of Exporter and Importer interfaces looks good.The WebhooksSettings class now correctly implements the Exporter and Importer interfaces, which aligns with the broader settings management system changes.
Also applies to: 11-11
| override fun export(): Map<String, *> { | ||
| return mapOf( | ||
| INTERNET_REQUIRED to internetRequired, | ||
| RETRY_COUNT to retryCount, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The export method should include the signingKey in returned data.
The export method creates a map of settings but omits the signingKey property. This is inconsistent with the import method which handles the signingKey. For completeness and symmetry between import/export operations, consider adding the signingKey to the exported map.
override fun export(): Map<String, *> {
return mapOf(
INTERNET_REQUIRED to internetRequired,
RETRY_COUNT to retryCount,
+ SIGNING_KEY to signingKey,
)
}📝 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.
| override fun export(): Map<String, *> { | |
| return mapOf( | |
| INTERNET_REQUIRED to internetRequired, | |
| RETRY_COUNT to retryCount, | |
| ) | |
| } | |
| override fun export(): Map<String, *> { | |
| return mapOf( | |
| INTERNET_REQUIRED to internetRequired, | |
| RETRY_COUNT to retryCount, | |
| SIGNING_KEY to signingKey, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt
around lines 32 to 37, the export method returns a map missing the signingKey
property. To fix this, add the signingKey to the map returned by export so it
matches the import method and includes all relevant settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/src/main/java/me/capcom/smsgateway/modules/gateway/workers/SettingsUpdateWorker.kt (3)
21-37: Consider improving error handling and logging.The error handling could be enhanced in a few ways:
- Use a proper logging system instead of
printStackTrace()- Consider implementing a maximum retry count or conditional retry logic
- Catch more specific exceptions rather than the broad
Throwableoverride suspend fun doWork(): Result { val gatewaySvc: GatewayService = get() val settingsSvc: SettingsService = get() return try { val settings = gatewaySvc.getSettings() settings?.let { settingsSvc.update(settings) } Result.success() - } catch (th: Throwable) { - th.printStackTrace() - Result.retry() + } catch (e: Exception) { + android.util.Log.e("SettingsUpdateWorker", "Failed to update settings", e) + // Optional: Check how many times this work has been retried + val runAttemptCount = runAttemptCount + return if (runAttemptCount < 3) { + Result.retry() + } else { + Result.failure() + } } }
42-64: Consider making the sync interval configurable.The 24-hour interval is hard-coded. Consider making it configurable through settings, allowing adjustments based on user needs or app requirements.
Additionally, consider adding battery optimization constraints (e.g., device charging) for better battery life.
fun start(context: Context) { + // Get interval from settings, default to 24 hours + val intervalHours = 24L val work = PeriodicWorkRequestBuilder<SettingsUpdateWorker>( - 24, + intervalHours, TimeUnit.HOURS ) .setBackoffCriteria( BackoffPolicy.EXPONENTIAL, WorkRequest.MIN_BACKOFF_MILLIS, TimeUnit.MILLISECONDS ) .setConstraints( Constraints.Builder() .setRequiredNetworkType(NetworkType.CONNECTED) + .setRequiresBatteryNotLow(true) // Optional: Add battery constraint .build() ) .build() WorkManager.getInstance(context) .enqueueUniquePeriodicWork( NAME, ExistingPeriodicWorkPolicy.REPLACE, work ) }
28-31: Consider handling the null settings case explicitly.When
settingsis null, the code silently continues. Consider adding a log or specific handling for this case to help with debugging.val settings = gatewaySvc.getSettings() settings?.let { settingsSvc.update(settings) + } ?: run { + android.util.Log.d("SettingsUpdateWorker", "Retrieved settings were null") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt(5 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/workers/SettingsUpdateWorker.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/SettingsRoutes.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/SettingsRoutes.kt
- app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt
🔇 Additional comments (2)
app/src/main/java/me/capcom/smsgateway/modules/gateway/workers/SettingsUpdateWorker.kt (2)
19-20: Good use of CoroutineWorker for background processing!Using CoroutineWorker is a great choice for background tasks that involve network operations. The implementation with KoinComponent provides clean dependency injection.
1-71: Overall well-structured worker implementation.The worker is well-structured with appropriate constraints for periodic execution. The use of WorkManager's unique work name ensures only one instance runs at a time, and the proper lifecycle management with start/stop methods provides good control over the worker's execution.
| Constraints.Builder() | ||
| .setRequiredNetworkType(NetworkType.CONNECTED) | ||
| .build() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider network type constraints based on app requirements.
The worker currently runs on any network connection. Depending on your app's requirements and the potential size of settings data, you might want to consider specifying whether it should run on metered connections.
🏁 Script executed:
#!/bin/bash
# Checking if there are any settings for network type preference elsewhere in the code
rg -i "networktype" --type kotlinLength of output: 3546
Review NetworkType Constraints
All gateway and webhook workers—including SettingsUpdateWorker—currently use NetworkType.CONNECTED. If your settings payload is small and occasional metered uploads are acceptable, this is fine. Otherwise, you may want to:
- Use
NetworkType.UNMETEREDto restrict work to unmetered networks, or - Expose a runtime/user preference so the app can choose between CONNECTED and UNMETERED.
Affected workers for consistency review:
• SettingsUpdateWorker.kt
• RegistrationWorker.kt
• PullMessagesWorker.kt
• SendStateWorker.kt
• WebhooksUpdateWorker.kt
• SendWebhookWorker.kt
🤖 Prompt for AI Agents
In
app/src/main/java/me/capcom/smsgateway/modules/gateway/workers/SettingsUpdateWorker.kt
around lines 53 to 55, the network constraint is set to NetworkType.CONNECTED,
allowing the worker to run on any network including metered ones. To align with
app requirements and manage data usage, update the constraint to
NetworkType.UNMETERED if uploads should avoid metered networks, or implement a
runtime/user preference to toggle between CONNECTED and UNMETERED. This change
ensures the worker respects network usage policies and maintains consistency
with other gateway and webhook workers.
capcom6
force-pushed
the
issue/214-settings-export-import
branch
2 times, most recently
from
c81978b to
a9a00cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (1)
38-42: 🛠️ Refactor suggestionPotential type mismatch between persisted data and accessor
sendIntervalMin/Maxare retrieved asInt(storage.get<Int>), but theimport()logic stores them asString(toString()).
IfKeyValueStorageis strict about types, future reads will returnnullor crash withClassCastException.Either:
- Persist them as
Intdirectly, or- Change the getters to read
Stringand convert.
♻️ Duplicate comments (5)
app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt (1)
32-37: The export method should include the signingKey in returned data.The export method creates a map of settings but omits the signingKey property. This is inconsistent with the import method which handles the signingKey. For completeness and symmetry between import/export operations, consider adding the signingKey to the exported map.
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (1)
100-135: Enum parsing still lacks error-handling
Previous review already pointed this out; the risk remains: invalid values in the import payload will crash the whole PATCH request.Wrap
valueOfcalls inrunCatching { … }.getOrNull()(or explicittry/catch) and decide what to do with bad data (ignore, default, log).app/src/main/java/me/capcom/smsgateway/modules/gateway/workers/SettingsUpdateWorker.kt (1)
53-55: Review network constraint consistencyThe worker is allowed on any network (
CONNECTED). Earlier reviews flagged similar workers: if settings payloads are sizeable or sensitive to metered data, switch toUNMETEREDor expose a user preference.app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt (2)
31-33: Handle null values in getAll() methodThe current implementation can return null values when a settings module doesn't implement the
Exporterinterface, which could cause issues for API consumers.fun getAll(): Map<String, *> { - return settings.mapValues { (it.value as? Exporter)?.export() } + return settings.mapValues { (_, value) -> + (value as? Exporter)?.export() ?: emptyMap<String, Any>() + } }
35-51: Add comprehensive input validation and error handlingThe current
update()method lacks proper input validation and could fail unexpectedly with unclear error messages.fun update(data: Map<String, *>) { + val errors = mutableListOf<String>() + data.forEach { (key, value) -> - try { - settings[key]?.let { - (it as? Importer)?.import(value as Map<String, *>) + settings[key]?.let { + if (it !is Importer) { + errors.add("Module '$key' does not support importing settings") + return@forEach } - } catch (e: IllegalArgumentException) { - throw IllegalArgumentException("Failed to import $key: ${e.message}", e) + + if (value !is Map<*, *>) { + errors.add("Settings for module '$key' must be a map") + return@forEach + } + + try { + @Suppress("UNCHECKED_CAST") + it.import(value as Map<String, *>) + } catch (e: Exception) { + errors.add("Failed to import settings for module '$key': ${e.message}") + } } } - notificationsService.notify( - context, - NotificationsService.NOTIFICATION_ID_SETTINGS_CHANGED, - "Settings changed via API. Restart the app to apply changes." - ) + if (errors.isEmpty()) { + notificationsService.notify( + context, + NotificationsService.NOTIFICATION_ID_SETTINGS_CHANGED, + "Settings changed via API. Restart the app to apply changes." + ) + } else { + throw IllegalArgumentException(errors.joinToString("\n")) + } }
🧹 Nitpick comments (4)
app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/SettingsRoutes.kt (1)
26-32: Prefer structured logging overprintStackTrace()Using
printStackTrace()clutters stdout and makes it hard to trace issues in production. Ktor already provides logging facilities (or you can inject an SLF4J logger). Replace with a proper logger at an appropriate level (warn/error).- } catch (e: Exception) { - e.printStackTrace() - call.respond( + } catch (e: Exception) { + logger.error(e) { "Failed to get settings" } + call.respond(Remember to add
private val logger = LoggerFactory.getLogger(SettingsRoutes::class.java)and the corresponding import.
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (1)
103-112: Redundant float→int→string conversion
value?.toString()?.toFloat()?.toInt()?.toString()does two unnecessary hops and can introduce rounding errors.- value?.toString()?.toFloat()?.toInt()?.toString() + value?.toString()?.toIntOrNull()Simpler, safer, and avoids precision loss.
app/src/main/java/me/capcom/smsgateway/modules/gateway/workers/SettingsUpdateWorker.kt (1)
34-35: Use logger instead ofprintStackTrace()Same reasoning as in
SettingsRoutes.app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt (1)
12-29: Consider adding a module existence check methodBased on previous review feedback, adding a public method to check module existence would be beneficial for external validation.
fun hasModule(name: String): Boolean { return settings.containsKey(name) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt(5 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/workers/SettingsUpdateWorker.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/SettingsRoutes.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/push/Event.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/services/PushService.kt(2 hunks)docs/api/swagger.json(26 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/me/capcom/smsgateway/modules/push/Event.kt
- app/src/main/java/me/capcom/smsgateway/services/PushService.kt
🔇 Additional comments (13)
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.kt (1)
119-123: LGTM! Well-implemented API method.The
getSettingsmethod follows the established pattern of other API methods in the class, properly uses bearer authentication, and correctly handles the HTTP GET request to the/settingsendpoint.app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt (2)
42-46: Verify intentional omission of PRIVATE_TOKEN from export.The export method only includes
CLOUD_URLbut the import method can handle bothCLOUD_URLandPRIVATE_TOKEN. This might be intentional for security reasons (not exporting sensitive tokens), but please confirm this design decision.
48-62: Excellent validation and error handling.The import method properly validates that URLs start with "https://" and throws descriptive error messages for invalid inputs. The type conversion handling is also well implemented.
app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt (1)
43-49: Good improvement in type conversion and validation.The retry count handling now uses direct integer conversion instead of the complex string→float→int→string chain mentioned in previous reviews. The validation ensuring retry count >= 1 is also well implemented with a clear error message.
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt (4)
36-59: Excellent code organization with region comments.The addition of region comments and proper grouping of lifecycle methods (
start,stop,isActiveLiveData) significantly improves code readability and maintainability. The integration ofSettingsUpdateWorkerinto the lifecycle is consistent with other workers.
175-179: Well-designed sealed class for registration modes.The
RegistrationModesealed class provides excellent type safety and clarity compared to the previous ad-hoc approach. The three variants (Anonymous,WithCredentials,WithCode) clearly represent all possible registration scenarios.
152-173: Properly implemented device update method.The
updateDevicemethod follows the established patterns in the class, properly handles the registration state check, and emits appropriate events. The implementation is consistent with other device management methods.
223-265: New API methods are well-implemented.The
sendState,getWebHooks, andgetSettingsmethods all follow established patterns in the class:
- Proper state validation with
registrationInfochecks- Consistent error handling
- Appropriate return types and null safety
These methods provide the necessary bridge between the service layer and the new settings management functionality.
docs/api/swagger.json (5)
489-555: Well-structured settings endpoints with comprehensive documentationThe new
/settingsendpoints are well-designed with proper HTTP methods, security, and documentation. The PATCH method correctly uses partial update semantics, and both endpoints support both local and cloud server URLs.
965-1069: Comprehensive settings schema with proper validation attributesThe Settings schema is well-structured with appropriate use of:
nullable: truefor optional fieldswriteOnly: truefor sensitive data like passphrases and tokens- Proper enum definitions for constrained values
- Clear property descriptions
The schema covers all major configuration areas including encryption, gateway, messages, ping, logs, and webhooks.
627-636: Properly defined response componentThe SettingsResponse component correctly references the Settings schema and provides appropriate content-type specification.
567-569: New Settings tag properly integratedThe new "Settings" tag is correctly added to the tags array for proper API organization.
6-6: Improved documentation clarity throughout the APIThe various textual improvements enhance the API documentation:
- More precise descriptions for endpoints and parameters
- Clarification about webhooks in different modes
- Better grammatical consistency
- Enhanced field descriptions
These changes improve the developer experience when working with the API.
Also applies to: 40-40, 131-131, 214-214, 252-252, 424-424, 440-440
| } catch (e: Exception) { | ||
| e.printStackTrace() | ||
| call.respond( | ||
| HttpStatusCode.BadRequest, | ||
| mapOf("message" to "Invalid request: ${e.message}") | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Differentiate client-side errors from server faults
All exceptions in the PATCH handler currently return 400 Bad Request.
• Deserialization problems or validation failures → 400
• Internal errors (e.g., storage failure) → 500
Distinguishing them gives clients clearer feedback and keeps monitoring/alerting meaningful.
Consider catching SerializationException, IllegalArgumentException, etc. separately and mapping them to 400, while treating everything else as 500.
🤖 Prompt for AI Agents
In
app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/SettingsRoutes.kt
between lines 44 and 50, all exceptions are currently caught and responded with
a 400 Bad Request, which does not differentiate between client-side errors and
server faults. To improve this, catch specific exceptions such as
SerializationException or IllegalArgumentException separately and respond with
400 for these, while catching general exceptions and responding with 500
Internal Server Error. This provides clearer feedback to clients and better
monitoring of server issues.
| val settings = gatewaySvc.getSettings() | ||
|
|
||
| settings?.let { | ||
| settingsSvc.update(settings) | ||
| } | ||
|
|
||
| Result.success() | ||
| } catch (th: Throwable) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Null settings still return Result.success()
If the gateway returns null, no update is applied, yet the worker reports success and the work manager won’t retry.
Return Result.retry() (or Result.failure()) when settings == null so the job is re-attempted.
- settings?.let {
- settingsSvc.update(settings)
- }
-
- Result.success()
+ if (settings == null) {
+ return Result.retry()
+ }
+ settingsSvc.update(settings)
+ Result.success()📝 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.
| val settings = gatewaySvc.getSettings() | |
| settings?.let { | |
| settingsSvc.update(settings) | |
| } | |
| Result.success() | |
| } catch (th: Throwable) { | |
| val settings = gatewaySvc.getSettings() | |
| if (settings == null) { | |
| return Result.retry() | |
| } | |
| settingsSvc.update(settings) | |
| Result.success() | |
| } catch (th: Throwable) { |
🤖 Prompt for AI Agents
In
app/src/main/java/me/capcom/smsgateway/modules/gateway/workers/SettingsUpdateWorker.kt
around lines 26 to 33, when the getSettings() method returns null, the code
still returns Result.success(), which causes the worker to report success even
though no update occurs. To fix this, add a check for null settings and return
Result.retry() or Result.failure() if settings is null, ensuring the work is
retried or failed appropriately instead of falsely indicating success.
🤖 Pull request artifacts
|
capcom6
force-pushed
the
issue/214-settings-export-import
branch
from
037844d to
973a056
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt (1)
268-275: Consider using the existing api property for consistency.The method creates a new GatewayApi instance instead of using the existing
apiproperty. This is inconsistent with other methods and potentially inefficient.Consider this refactor to use the existing api property:
- suspend fun getPublicIP(): String { - return GatewayApi( - settings.serverUrl, - settings.privateToken - ) - .getDevice(settings.registrationInfo?.token) - .externalIp - } + suspend fun getPublicIP(): String { + return api.getDevice(settings.registrationInfo?.token).externalIp + }Also consider making this method
internalfor consistency with other similar methods in the class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt(5 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/workers/SettingsUpdateWorker.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/SettingsRoutes.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/push/Event.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/services/PushService.kt(2 hunks)app/src/main/res/values/strings.xml(6 hunks)docs/api/swagger.json(26 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (10)
- app/src/main/java/me/capcom/smsgateway/services/PushService.kt
- app/src/main/java/me/capcom/smsgateway/modules/push/Event.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt
- app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhooksSettings.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/workers/SettingsUpdateWorker.kt
- app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt
- app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/SettingsRoutes.kt
- docs/api/swagger.json
- app/src/main/java/me/capcom/smsgateway/modules/settings/SettingsService.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build / build-apk
🔇 Additional comments (9)
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt (9)
13-13: LGTM!The import is correctly placed and needed for the SettingsUpdateWorker integration.
36-46: Good integration of SettingsUpdateWorker in startup sequence.The placement of SettingsUpdateWorker.start() is appropriate, and the region organization improves code readability.
48-56: Proper worker lifecycle management in stop method.The SettingsUpdateWorker is correctly stopped in reverse order of startup, following good lifecycle management practices.
152-173: Well-implemented device update method.The method follows existing patterns with proper error handling, early returns for invalid states, and consistent event emission.
175-179: Good encapsulation of RegistrationMode within GatewayService.Moving the sealed class inside the service improves organization and encapsulation since it's only used within this class.
223-245: Solid implementation of sendState method.The method properly handles null safety, data transformation, and follows the established patterns in the class.
249-256: Clean implementation of getWebHooks method.The method has proper null safety with a sensible fallback to an empty list when no registration info is available.
260-264: Appropriate implementation of getSettings method.The method correctly handles null safety and uses a flexible return type for the settings data structure.
36-276: Excellent code organization and feature integration.The region-based organization significantly improves code readability and maintainability. The new settings management functionality is well-integrated with proper lifecycle management and consistent patterns throughout.
1 participant
Summary by CodeRabbit
New Features
/settingsAPI endpoint with GET and PATCH methods for retrieving and updating configuration.SettingsUpdateWorker) for periodic settings synchronization.SettingsUpdatedevent.notif_settings.xml).SettingsServiceto manage and notify about settings updates.Improvements
Bug Fixes