Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds notification channel resources, UI and icon; introduces NotificationChannel enum and storage/export/import; makes DevicePatchRequest.pushToken nullable and always sent; PushService skips FCM registration when channel is SSE_ONLY; minor string formatting and preference whitespace tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Push as PushService
participant Settings as GatewaySettings
participant FCM as FcmRegistrar
participant Worker as RegistrationWorker
participant API as GatewayApi
Push->>Settings: read notificationChannel
alt notificationChannel == SSE_ONLY
Push->>Worker: start(token=null, persist=false)
Worker-->>Push: started
Push-->>Push: return (skip FCM)
else
Push->>FCM: perform FCM registration
FCM-->>Push: token
Push->>Worker: start(token, persist=true)
Worker->>API: devicePatch(pushToken=token or null)
API-->>Worker: response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/src/main/res/values/strings.xml`:
- Around line 70-79: The three description resources
(notification_mode_auto_description, notification_mode_fcm_only_description,
notification_mode_sse_only_description) are unused because the ListPreference in
notification_preferences.xml uses app:useSimpleSummaryProvider="true" which only
shows the selected title from notification_modes_titles; either remove the
unused strings or wire them into the UI: to wire them, replace
useSimpleSummaryProvider with an entrySummaries array (or set android:summary to
reference entrySummaries) that maps to these three description strings, or
implement a custom SummaryProvider in your PreferenceFragmentCompat (in the
fragment class that loads notification_preferences.xml) to set the
ListPreference summary based on the selected value and the corresponding
description resource; update the ListPreference declaration (same preference
key/name as the existing ListPreference) or the fragment's onCreatePreferences
logic to point to notification_mode_auto_description,
notification_mode_fcm_only_description, and
notification_mode_sse_only_description so they are actually displayed.
In `@app/src/main/res/xml/root_preferences.xml`:
- Around line 26-29: The Preference element for NotificationSettingsFragment
uses a hardcoded summary and lacks an icon; add a new string resource named
notification_delivery_mode_selection in strings.xml with the English text, then
update the Preference (the element with
app:fragment="me.capcom.smsgateway.ui.settings.NotificationSettingsFragment") to
use android:summary="@string/notification_delivery_mode_selection" and add the
same android:icon attribute pattern used by the other preferences (match their
drawable resource) so the preference is localizable and visually consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/res/xml/cloud_server_preferences.xml`:
- Around line 15-22: The ListPreference element using android:icon should be
changed to use the AndroidX attribute app:icon for consistency with other
preferences; locate the ListPreference with key "gateway.notification_mode" and
replace the android:icon attribute with app:icon="@drawable/ic_notifications" so
the Preference (ListPreference) uses the androidx preference attribute namespace
like the EditTextPreference entries.
capcom6
force-pushed
the
issue/304-sorce-sse
branch
from
de5bcec to
6e0d6b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt`:
- Around line 89-97: The import path for NOTIFICATION_CHANNEL uses
NotificationChannel.valueOf which will throw IllegalArgumentException on invalid
strings and abort the import; update the NOTIFICATION_CHANNEL branch to parse
safely (e.g., wrap NotificationChannel.valueOf in runCatching/try-catch or
lookup via enum values) to fall back to NotificationChannel.AUTO on failure,
keep the existing changed calculation (compare notificationChannel to the
resolved newValue) and still call storage.set(it.key, newValue.name) so invalid
inputs become AUTO instead of crashing the import.
- Around line 42-43: The stored enums were saved as raw names (e.g., "AUTO") but
the getters call storage.get<T>(...) which triggers Gson deserialization and
fails on unquoted strings; update each enum getter (e.g.,
GatewaySettings.notificationChannel and MessagesSettings.limitPeriod,
simSelectionMode, processingOrder) to read the stored value as a plain String
via storage.get<String>(KEY) (or storage.getFallback path) and then convert to
the enum using a safe mapping (e.g., EnumName.values().find { it.name == stored
} ?: DEFAULT) so invalid/missing values fall back to the existing defaults;
ensure the import() behavior (which uses storage.set(key, newValue.name))
remains unchanged and add this manual String-to-enum conversion wherever enums
are read from storage.
🧹 Nitpick comments (1)
app/src/main/java/me/capcom/smsgateway/services/PushService.kt (1)
59-62: Consider adding a log entry for the SSE_ONLY early-exit path.The normal FCM registration path logs
"FCM registration started"(Line 68), but the SSE_ONLY short-circuit is silent. A brief log entry here would help with debugging notification channel issues.Proposed change
if (settings.notificationChannel == GatewaySettings.NotificationChannel.SSE_ONLY) { + logger.insert( + priority = LogEntry.Priority.INFO, + module = PushService::class.java.simpleName, + message = "FCM registration skipped: SSE_ONLY channel configured" + ) RegistrationWorker.start(context, null, false) return }
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt
Show resolved
Hide resolved
capcom6
force-pushed
the
issue/304-sorce-sse
branch
from
69aead2 to
28f9ec6
Compare
capcom6
force-pushed
the
issue/304-sorce-sse
branch
from
6360438 to
df340e9
Compare
Contributor
🤖 Pull request artifacts
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/me/capcom/smsgateway/modules/orchestrator/OrchestratorService.kt (1)
34-54:⚠️ Potential issue | 🔴 CriticalBug:
receiverService.start(context)is called twice.
receiverService.start(context)appears at line 37 (inside the foreground-services try block) and again at line 47 (in its own try block). Based on the summary, the intent was to decouple receiver registration into a separate try/catch — line 37 should have been removed.Additionally, both catch blocks swallow the exception entirely (
eis never referenced). Including the exception message in the log entries would aid debugging. This aligns with the detektSwallowedExceptionwarning.Proposed fix
try { localServerSvc.start(context) pingSvc.start(context) - receiverService.start(context) } catch (e: Throwable) { logsSvc.insert( LogEntry.Priority.WARN, MODULE_NAME, - "Can't start foreground services while the app is running in the background" + "Can't start foreground services while the app is running in the background: ${e.message}" ) } try { receiverService.start(context) } catch (e: Throwable) { logsSvc.insert( LogEntry.Priority.WARN, MODULE_NAME, - "Can't register receiver" + "Can't register receiver: ${e.message}" ) }
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
New Resources
Behavior Change
Style