feat(android): Add Firebase Cloud Messaging push notifications#1265
feat(android): Add Firebase Cloud Messaging push notifications#1265alltheseas wants to merge 8 commits intodamus-io:masterfrom
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:
📝 WalkthroughWalkthroughAdds Android FCM + Notepush integration: JNI shims and globals for FCM token and NIP‑98 signing keypair in Rust, Kotlin FirebaseMessagingService and Notepush HTTP client, Android build/manifest updates, notification mode API and UI wiring, and account-to-Android signing keypair synchronization. Changes
Sequence Diagram(s)sequenceDiagram
participant FCM as Firebase Cloud Messaging
participant App as Android App<br/>NotedeckFirebaseMessagingService
participant Rust as Rust Layer<br/>(android.rs)
participant KotlinClient as NotepushClient (Kotlin)
participant Notepush as Notepush Server
participant System as Android System<br/>(Notifications)
FCM->>App: onMessageReceived(RemoteMessage)
Note over App: extract `nostr_event` or notification payload
App->>Rust: nativeProcessNostrEvent(eventJson)
Rust->>Rust: parse_nostr_event_for_notification()
Rust-->>App: NotificationResult {title, body, eventId}
App->>System: showNotification(title, body, eventId)
rect rgba(100,150,200,0.5)
FCM->>App: onNewToken(token)
App->>App: save token (SharedPreferences)
App->>Rust: nativeOnFcmTokenRefreshed(token)
Rust->>Rust: update FCM_TOKEN
end
rect rgba(150,200,120,0.5)
App->>KotlinClient: registerDevice(pubkey, fcmToken)
KotlinClient->>Rust: nativeSignNip98Auth(url, method, body)
Rust->>Rust: sign_nip98_event -> signature
Rust-->>KotlinClient: base64 signature
KotlinClient->>Notepush: HTTP request w/ Authorization: Nostr ...
Notepush-->>KotlinClient: response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 7
🤖 Fix all issues with AI agents
In `@crates/notedeck_chrome/android/app/src/main/AndroidManifest.xml`:
- Around line 55-56: The manifest adds POST_NOTIFICATIONS but you must request
it at runtime on Android 13+ to avoid silent failures when calling
notificationManager.notify() (see NotedeckFirebaseMessagingService.kt:127); add
a runtime check using ContextCompat.checkSelfPermission() for
Manifest.permission.POST_NOTIFICATIONS and, if not granted, trigger a permission
request flow from MainActivity (either ActivityCompat.requestPermissions() in
onCreate or preferably registerForActivityResult() with
ActivityResultContracts.RequestPermission) and gate notification posting until
permission is granted so notificationManager.notify() is only called when the
permission is granted.
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotedeckFirebaseMessagingService.kt`:
- Around line 39-41: The onNewToken override currently logs the full FCM token
(Log.d(TAG, "New FCM token: $token")), which exposes sensitive device
credentials; update onNewToken to avoid printing the full token by conditionally
logging only in debug builds (use BuildConfig.DEBUG) and redact the token (e.g.,
log length and last 4 chars or a short hash) instead of the full string, and
ensure release builds either omit the log or have it stripped via R8/ProGuard;
reference the onNewToken method and TAG constant when making this change.
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotepushClient.kt`:
- Around line 56-65: The Response from client.newCall(request).execute() (stored
in response) must be closed on all paths to avoid leaked connections; wrap the
execute() result in a Kotlin use { } block so the Response is automatically
closed whether successful or not and move the success check and logging inside
that block for the register call (and apply the same pattern to the other places
noted where response is assigned at lines matching the other execute() calls).
Ensure you still read the body string only inside the use block when logging
error details and return the success boolean from within or after the use block
as appropriate.
- Line 42: The URL construction is currently done via string interpolation
(e.g., val url = "$baseUrl/user-info/$pubkeyHex/$fcmToken?backend=fcm") which
doesn't URL-encode path segments; update all four methods in NotepushClient.kt
that build URLs (the methods that assemble the
user-info/user-delete/subscribe/unsubscribe endpoints) to use
okhttp3.HttpUrl.Builder and addPathSegment(...) for the pubkeyHex and fcmToken
segments and set the "backend" query parameter with addQueryParameter(...) so
path segments (FCM token and public key) are properly encoded and queries are
built safely before converting to a string for signing and requests.
In `@crates/notedeck_chrome/android/build.gradle`:
- Around line 2-6: The plugins block declares AGP 8.4.1, Kotlin plugin
'org.jetbrains.kotlin.android' v1.9.22, and Google Services
'com.google.gms.google-services' v4.4.0 which are incompatible; fix by either
downgrading the AGP plugin ids ('com.android.application' and
'com.android.library') to an AGP 8.2.x version to match Kotlin 1.9.x, or upgrade
the Kotlin plugin ('org.jetbrains.kotlin.android') to a compatible 2.x release
and bump 'com.google.gms.google-services' to 4.4.1 or newer; update the versions
in the plugins block accordingly and keep the three identifiers
(com.android.application, com.android.library, org.jetbrains.kotlin.android,
com.google.gms.google-services) consistent across the build configuration.
In `@crates/notedeck/src/platform/android.rs`:
- Around line 241-255: The notification title/body string literals in the match
over kind (the tuple assignment to (title, body) in android.rs) must be
localized: replace hard-coded titles like "New mention", "New direct message",
"Someone reposted", "New reaction", "Someone zapped you", "New activity" and
body texts such as "Contents are encrypted" and truncated results from
truncate_content(content, 100) with tr! (or tr_plural! where pluralization is
needed) calls; ensure the reaction branch still uses localized text for the "New
reaction" title and localize any non-emoji fallback content, and export all new
source strings for translation after updating the code.
- Around line 201-205: Three places currently slice strings by bytes and can
panic on non‑ASCII UTF‑8: the logging call around the FCM token (info!("FCM
token refreshed: {}", ...)), the debug log that prints event_json
(debug!("Processing Nostr event from FCM: {}...",
&event_json[..event_json.len().min(100)])), and the truncate_content function
itself. Change truncate_content to operate on characters (e.g., iterate chars
and collect up to N, appending "…" when truncated) so it never slices bytes,
then replace the unsafe byte slices in the two log calls to use
truncate_content(&token, 20) and truncate_content(&event_json, 100) and ensure
parse_nostr_event_for_notification usages that previously relied on byte slices
now pass full strings or use the new safe truncation helper. Ensure function
names referenced: truncate_content and parse_nostr_event_for_notification.
| <!-- Required for Android 13+ to show notifications --> | ||
| <uses-permission android:name="android.permission.POST_NOTIFICATIONS" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "POST_NOTIFICATIONS|Manifest\.permission\.POST_NOTIFICATIONS|requestPermissions|ActivityCompat\.requestPermissions|registerForActivityResult" -C3Repository: damus-io/notedeck
Length of output: 703
🏁 Script executed:
find crates/notedeck_chrome/android -name "*.kt" -o -name "*.java" | head -20Repository: damus-io/notedeck
Length of output: 352
🏁 Script executed:
cat -n crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.javaRepository: damus-io/notedeck
Length of output: 9637
🏁 Script executed:
cat -n crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotedeckFirebaseMessagingService.ktRepository: damus-io/notedeck
Length of output: 7255
🏁 Script executed:
cat -n crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotepushClient.ktRepository: damus-io/notedeck
Length of output: 8959
Add runtime POST_NOTIFICATIONS permission request before notifying on Android 13+.
Manifest declaration alone is insufficient on API 33+. The notificationManager.notify() call at NotedeckFirebaseMessagingService.kt:127 will silently fail without a runtime permission grant. Add a permission check and request flow (e.g., using ContextCompat.checkSelfPermission() and ActivityCompat.requestPermissions() in MainActivity's onCreate or via registerForActivityResult()) to ensure notifications display.
🤖 Prompt for AI Agents
In `@crates/notedeck_chrome/android/app/src/main/AndroidManifest.xml` around lines
55 - 56, The manifest adds POST_NOTIFICATIONS but you must request it at runtime
on Android 13+ to avoid silent failures when calling
notificationManager.notify() (see NotedeckFirebaseMessagingService.kt:127); add
a runtime check using ContextCompat.checkSelfPermission() for
Manifest.permission.POST_NOTIFICATIONS and, if not granted, trigger a permission
request flow from MainActivity (either ActivityCompat.requestPermissions() in
onCreate or preferably registerForActivityResult() with
ActivityResultContracts.RequestPermission) and gate notification posting until
permission is granted so notificationManager.notify() is only called when the
permission is granted.
...ome/android/app/src/main/java/com/damus/notedeck/service/NotedeckFirebaseMessagingService.kt
Outdated
Show resolved
Hide resolved
crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotepushClient.kt
Outdated
Show resolved
Hide resolved
crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotepushClient.kt
Outdated
Show resolved
Hide resolved
| plugins { | ||
| id 'com.android.application' version '8.4.1' apply false | ||
| id 'com.android.library' version '8.4.1' apply false | ||
| id 'org.jetbrains.kotlin.android' version '1.9.22' apply false | ||
| id 'com.google.gms.google-services' version '4.4.0' apply false |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Kotlin Android plugin 1.9.22 compatibility with Android Gradle Plugin 8.4.1
💡 Result:
Short answer: Kotlin Gradle plugin 1.9.22 is not listed as officially compatible with AGP 8.4.1 — the Kotlin compatibility tables document 1.9.x (e.g. 1.9.20/1.9.x line) as supporting AGP up to ~8.2, so AGP 8.4.1 is outside the officially supported range. [1][2]
Recommendation: either keep AGP ≤ 8.2 when staying on KGP 1.9.22, or upgrade Kotlin (use a newer 2.x KGP that explicitly lists support for later AGP versions) before moving to AGP 8.4.1. [1][3]
Sources:
- Kotlin compatibility tables (shows 1.9.20/1.9.x → AGP 7.4.2–8.2). [1]
- Android Developer page on AGP/Kotlin requirements. [2]
- Kotlin plugin listing for 1.9.22 (plugin page). [3]
🌐 Web query:
Google Services plugin 4.4.0 compatibility with Android Gradle Plugin 8.4.1
💡 Result:
Short answer: Possibly, but not recommended — com.google.gms:google-services:4.4.0 reports only “improved compatibility with newer AGP versions” and several downstream issues were reported with 4.4.0. Google/Firebase later published fixes (4.4.1+). Upgrade to the newer google‑services plugin (4.4.1 or the latest) when using AGP 8.4.x. [1][2][3]
Sources:
- Firebase Android release notes (google‑services 4.4.0 notes and later mentions). [1]
- Firebase GitHub issue reports about crashes/bugs after updating to 4.4.0. [2]
- Android Gradle Plugin 8.4 release notes (AGP 8.4 compatibility context). [3]
🏁 Script executed:
find . -path "*notedeck_chrome/android/build.gradle" -type f | head -5Repository: damus-io/notedeck
Length of output: 106
🏁 Script executed:
cat -n ./crates/notedeck_chrome/android/build.gradle | head -20Repository: damus-io/notedeck
Length of output: 564
Kotlin 1.9.22 and Google Services 4.4.0 are incompatible with AGP 8.4.1.
Kotlin Android plugin 1.9.22 is not officially supported with AGP 8.4.1—Kotlin 1.9.x is documented to support only up to AGP 8.2. Additionally, Google Services plugin 4.4.0 has known issues with AGP 8.4.1 and was followed by fixes in 4.4.1+.
Either downgrade AGP to 8.2 or earlier, or upgrade: Kotlin to 2.x (which supports AGP 8.4.1) and Google Services to 4.4.1+.
🤖 Prompt for AI Agents
In `@crates/notedeck_chrome/android/build.gradle` around lines 2 - 6, The plugins
block declares AGP 8.4.1, Kotlin plugin 'org.jetbrains.kotlin.android' v1.9.22,
and Google Services 'com.google.gms.google-services' v4.4.0 which are
incompatible; fix by either downgrading the AGP plugin ids
('com.android.application' and 'com.android.library') to an AGP 8.2.x version to
match Kotlin 1.9.x, or upgrade the Kotlin plugin
('org.jetbrains.kotlin.android') to a compatible 2.x release and bump
'com.google.gms.google-services' to 4.4.1 or newer; update the versions in the
plugins block accordingly and keep the three identifiers
(com.android.application, com.android.library, org.jetbrains.kotlin.android,
com.google.gms.google-services) consistent across the build configuration.
| let (title, body) = match kind { | ||
| 1 => ("New mention".to_string(), truncate_content(content, 100)), | ||
| 4 => ("New direct message".to_string(), "Contents are encrypted".to_string()), | ||
| 6 => ("Someone reposted".to_string(), truncate_content(content, 100)), | ||
| 7 => { | ||
| let reaction = match content { | ||
| "" | "+" => "❤️", | ||
| "-" => "👎", | ||
| _ => content, | ||
| }; | ||
| ("New reaction".to_string(), reaction.to_string()) | ||
| } | ||
| 9735 => ("Someone zapped you".to_string(), "".to_string()), | ||
| _ => ("New activity".to_string(), truncate_content(content, 100)), | ||
| }; |
There was a problem hiding this comment.
Localize notification title/body strings.
These are user-facing; wrap with tr!/tr_plural! and export source strings after updates. As per coding guidelines.
🤖 Prompt for AI Agents
In `@crates/notedeck/src/platform/android.rs` around lines 241 - 255, The
notification title/body string literals in the match over kind (the tuple
assignment to (title, body) in android.rs) must be localized: replace hard-coded
titles like "New mention", "New direct message", "Someone reposted", "New
reaction", "Someone zapped you", "New activity" and body texts such as "Contents
are encrypted" and truncated results from truncate_content(content, 100) with
tr! (or tr_plural! where pluralization is needed) calls; ensure the reaction
branch still uses localized text for the "New reaction" title and localize any
non-emoji fallback content, and export all new source strings for translation
after updating the code.
alltheseas
force-pushed
the
feat/android-fcm-notifications
branch
from
2fef516 to
3b0f9ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/notedeck/src/platform/android.rs`:
- Around line 12-31: Replace the module-level statics FCM_TOKEN and
SIGNING_KEYPAIR with fields on the shared AppContext (e.g., add a
notifications/android state struct inside AppContext that holds Option<String>
and Option<FullKeypair>), remove the RwLock statics, and update any JNI
entrypoints (the JNI callbacks that currently read/write FCM_TOKEN and the
set_signing_keypair function) to call AppContext setters/getters or send
messages through the existing app-managed channel to mutate those fields; ensure
the AppContext accessors use the same sync primitive you use across the codebase
and preserve thread-safety when called from JNI.
♻️ Duplicate comments (2)
crates/notedeck/src/platform/android.rs (2)
175-204: Avoid UTF‑8 byte slicing and logging sensitive token/event data.
Byte slicing can panic on non‑ASCII, and logging token/event content risks leaking sensitive data. Consider logging only metadata (length/hash) and make truncation UTF‑8 safe for user-facing content.🛠️ Proposed fix
- info!("FCM token refreshed: {}...", &token[..token.len().min(20)]); + info!("FCM token refreshed (len={} chars)", token.chars().count()); - debug!( - "Processing Nostr event from FCM: {}...", - &event_json[..event_json.len().min(100)] - ); + debug!( + "Processing Nostr event from FCM (len={} chars)", + event_json.chars().count() + ); -fn truncate_content(content: &str, max_len: usize) -> String { - if content.len() <= max_len { - content.to_string() - } else { - format!("{}...", &content[..max_len]) - } -} +/// Truncate a UTF-8 string to at most `max_len` characters, appending `...`. +fn truncate_content(content: &str, max_len: usize) -> String { + let truncated: String = content.chars().take(max_len).collect(); + if truncated.len() == content.len() { + truncated + } else { + format!("{truncated}...") + } +}Also applies to: 271-276
246-265: Localize notification titles/bodies.
These are user-facing strings; wrap withtr!/tr_plural!and export source strings after updates. Based on learnings, ...
| /// Stores the current FCM token for access from Rust code. | ||
| /// | ||
| /// # Why a static global? | ||
| /// JNI callbacks are invoked directly by the JVM and cannot receive Rust struct | ||
| /// references. This follows the same pattern as `KEYBOARD_HEIGHT` and | ||
| /// `SELECTED_MEDIA_CHANNEL` in the codebase. | ||
| static FCM_TOKEN: RwLock<Option<String>> = RwLock::new(None); | ||
|
|
||
| /// Stores the active account's keypair for NIP-98 HTTP authentication signing. | ||
| /// | ||
| /// # Why a static global? | ||
| /// The `nativeSignNip98Auth` JNI function is called synchronously from Kotlin | ||
| /// and cannot receive Rust struct references. The keypair must be pre-set via | ||
| /// [`set_signing_keypair`] when the active account changes. | ||
| /// | ||
| /// # Why std::sync::RwLock? | ||
| /// JNI callbacks are synchronous; tokio::sync::RwLock requires an async runtime | ||
| /// which is unavailable in JNI context. | ||
| static SIGNING_KEYPAIR: RwLock<Option<FullKeypair>> = RwLock::new(None); | ||
|
|
There was a problem hiding this comment.
Avoid new static globals; move FCM token/signing keypair into AppContext-managed state.
Guidelines prohibit global state in crates/notedeck*/src/**. Please store this in an AppContext-owned struct (e.g., an Android/notifications state) and update it via app-managed channels or setters invoked from JNI. As per coding guidelines, ...
🤖 Prompt for AI Agents
In `@crates/notedeck/src/platform/android.rs` around lines 12 - 31, Replace the
module-level statics FCM_TOKEN and SIGNING_KEYPAIR with fields on the shared
AppContext (e.g., add a notifications/android state struct inside AppContext
that holds Option<String> and Option<FullKeypair>), remove the RwLock statics,
and update any JNI entrypoints (the JNI callbacks that currently read/write
FCM_TOKEN and the set_signing_keypair function) to call AppContext
setters/getters or send messages through the existing app-managed channel to
mutate those fields; ensure the AppContext accessors use the same sync primitive
you use across the codebase and preserve thread-safety when called from JNI.
3b0f9ec to
19186df
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/notedeck/src/platform/android.rs`:
- Around line 336-343: The conversion of jbody to body silently swallows errors
(match on env.get_string(&jbody) maps Err(_) => None); change the Err arm to
capture the error (e.g., Err(e)) and log it before returning None so JNI
conversion failures are visible; update the match in the block where body,
jbody, and env.get_string are used (the body variable initialization) to log the
error with a descriptive message and the error value.
- Around line 148-153: The RwLock operations currently ignore Err (poisoned)
cases silently; update set_signing_keypair (and similarly get_fcm_token and
get_signing_keypair) to log the lock error when the write/read fails instead of
silently returning—e.g., match on SIGNING_KEYPAIR.write() / .read(), on Ok
proceed as before, and on Err call error! with context and the error value (for
getters return None after logging). Reference the SIGNING_KEYPAIR symbol and the
functions set_signing_keypair, get_fcm_token, and get_signing_keypair when
making these changes.
♻️ Duplicate comments (5)
crates/notedeck/src/platform/android.rs (5)
12-30: Global state usage violates coding guidelines.The static
FCM_TOKENandSIGNING_KEYPAIRvariables have already been flagged in a prior review. The coding guidelines explicitly prohibit global variables, requiring state to be managed in structs passed by reference (e.g., viaAppContext).The documented justification cites JNI callback constraints, but this should be addressed architecturally—perhaps via a channel-based approach or an
AppContext-owned state struct that JNI callbacks signal into.
175-175: UTF-8 byte slicing can panic on non-ASCII tokens.This has been flagged in a prior review. Use character-based truncation via the
truncate_contenthelper (once fixed) instead of byte slicing.
201-204: UTF-8 byte slicing in debug log can panic.Already flagged in prior review. Replace with character-safe truncation.
246-266: User-facing notification strings need localization.Already flagged in prior review. Wrap strings like
"New mention","New direct message","Contents are encrypted", etc., withtr!macros. As per coding guidelines.
271-277: UTF-8 unsafe truncation.Already flagged in prior review. The function should iterate characters rather than slice bytes:
+/// Truncates content to at most `max_len` characters, appending "..." if truncated. fn truncate_content(content: &str, max_len: usize) -> String { - if content.len() <= max_len { - content.to_string() - } else { - format!("{}...", &content[..max_len]) + let truncated: String = content.chars().take(max_len).collect(); + if truncated.len() == content.len() { + truncated + } else { + format!("{}...", truncated) } }
🧹 Nitpick comments (1)
crates/notedeck/src/platform/android.rs (1)
182-182: TODO: Consider implementing re-registration trigger.The comment indicates missing functionality for triggering re-registration with the notepush server when the FCM token refreshes and notifications are enabled.
Would you like me to help implement this re-registration flow or open an issue to track this task?
alltheseas
force-pushed
the
feat/android-fcm-notifications
branch
from
19186df to
e46088a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotedeckFirebaseMessagingService.kt`:
- Around line 62-70: The code currently logs raw Nostr event JSON in
onMessageReceived which can leak PII; update the logging to avoid printing the
full nostrEventJson in production by either removing the Log.d that includes the
event or wrapping it in a debug-only check (e.g. BuildConfig.DEBUG) so only
debug builds log the first 100 chars, while release builds log a generic
message; keep the rest of the flow (call to handleNostrEvent) unchanged and
reference TAG, onMessageReceived, nostrEventJson, and handleNostrEvent when
making the change.
♻️ Duplicate comments (4)
crates/notedeck/src/platform/android.rs (4)
12-31: Avoid new global state; move FCM token and signing keypair into AppContext-managed state.Global statics are disallowed in this codebase and make state ownership/ordering hard to reason about. Please migrate these into an AppContext-owned struct and update the JNI entry points to read/write through that shared state.
As per coding guidelines, global variables are not allowed; state should be managed via AppContext.
141-158: Log lock-poisoning failures instead of silently dropping updates.Read/write failures are currently ignored, which hides real bugs and makes JNI failures harder to diagnose.
🔧 Suggested change
pub fn get_fcm_token() -> Option<String> { - FCM_TOKEN.read().ok().and_then(|guard| guard.clone()) + match FCM_TOKEN.read() { + Ok(guard) => guard.clone(), + Err(e) => { + error!("Failed to read FCM token: lock poisoned: {}", e); + None + } + } } pub fn set_signing_keypair(keypair: Option<FullKeypair>) { - if let Ok(mut guard) = SIGNING_KEYPAIR.write() { - *guard = keypair; - info!("Signing keypair updated for FCM registration"); - } + match SIGNING_KEYPAIR.write() { + Ok(mut guard) => { + *guard = keypair; + info!("Signing keypair updated for FCM registration"); + } + Err(e) => error!("Failed to update signing keypair: lock poisoned: {}", e), + } } - if let Ok(mut guard) = FCM_TOKEN.write() { - *guard = Some(token); - } + match FCM_TOKEN.write() { + Ok(mut guard) => *guard = Some(token), + Err(e) => error!("Failed to store FCM token: lock poisoned: {}", e), + }Also applies to: 175-180
246-265: Localize user-facing notification strings.These strings surface directly to users and should be wrapped with
tr!/tr_plural!and exported for localization.As per coding guidelines, user-facing strings must be localized.
340-347: Log JNI body conversion failures instead of silently ignoring them.Swallowing conversion errors makes JNI issues hard to diagnose in the field.
🔧 Suggested change
- match env.get_string(&jbody) { - Ok(s) => Some(s.into()), - Err(_) => None, - } + match env.get_string(&jbody) { + Ok(s) => Some(s.into()), + Err(e) => { + warn!("Failed to get body string, treating as empty: {}", e); + None + } + }
...ome/android/app/src/main/java/com/damus/notedeck/service/NotedeckFirebaseMessagingService.kt
Show resolved
Hide resolved
alltheseas
force-pushed
the
feat/android-fcm-notifications
branch
from
d7bed34 to
04eead1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.gitignore:
- Line 25: The .gitignore entry was changed to "local.propertiesFCM/" which
likely removed the original "local.properties" ignore and risks committing
sensitive local SDK paths; restore an explicit "local.properties" ignore and
keep the new "local.propertiesFCM/" pattern so both files remain ignored (update
the .gitignore to include both "local.properties" and "local.propertiesFCM/");
ensure the change references the .gitignore file and these exact patterns so the
original local.properties rule is preserved.
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotedeckFirebaseMessagingService.kt`:
- Around line 118-136: The current code in NotedeckFirebaseMessagingService uses
System.currentTimeMillis().toInt() as both the PendingIntent requestCode and the
notification ID (seen in PendingIntent.getActivity and
notificationManager.notify), which can collide; replace these runtime
millisecond-based ints with a monotonically-incrementing, thread-safe counter
(e.g., an AtomicInteger in the class companion object like
notificationIdCounter) and use its getAndIncrement() value for both the
PendingIntent requestCode and the notificationManager.notify call so each
notification and intent has a unique ID.
In `@crates/notedeck_columns/src/ui/settings.rs`:
- Around line 103-113: The handler for Self::SetNotificationMode calls
accounts.selected_account_pubkey() which can panic if no account is selected;
update the branch in settings.rs (the SetNotificationMode arm) to first check
for a selected account (use the safe accessor such as
accounts.selected_account() or have selected_account_pubkey() return an
Option/Result) and early-return or log and skip the operation when none exists,
only deriving pubkey_hex and calling notedeck::platform::set_notification_mode
when a valid pubkey is present; keep the existing error logging for
set_notification_mode failures.
In `@crates/notedeck/src/platform/android.rs`:
- Around line 408-449: Add a Rust docstring to the sign_nip98_event function
that briefly describes its purpose (creates and signs a NIP-98 HTTP Auth event /
kind 27235), documents each parameter (keypair: &FullKeypair, url: &str, method:
&str, body: Option<&str>), explains the produced tags (u URL tag, method tag,
optional payload tag containing SHA-256 hex when body is present), and states
the return type (Result<String, Box<dyn std::error::Error>> containing the
base64-encoded JSON event) and possible errors (signing/serialization errors
from EventBuilder::new, sign_with_keys, or serde_json::to_string). Ensure the
docstring appears immediately above the sign_nip98_event declaration and
references the relevant symbols (FullKeypair, EventBuilder, TagKind, Sha256).
♻️ Duplicate comments (1)
crates/notedeck/src/platform/android.rs (1)
263-283: Notification strings not localized.The notification title/body strings ("New mention", "New direct message", etc.) are hardcoded English. Per coding guidelines, user-facing strings should use
tr!macro. However, since this runs in a JNI callback context, theLocalizationinstance fromAppContextmay not be accessible.Consider either:
- Passing localized strings from Kotlin side
- Storing a static reference to localization data
- Using Android string resources on the Kotlin side instead
🧹 Nitpick comments (2)
crates/notedeck_columns/src/ui/settings.rs (1)
821-829: Localize permission status badge text.The badge text strings ("Granted", "Pending", "Required") should be wrapped with
tr!macro for localization consistency with the rest of the UI. As per coding guidelines.♻️ Suggested fix
let (badge_text, badge_color) = if permission_granted { - ("Granted", Color32::from_rgb(34, 197, 94)) // Green + (tr!(self.note_context.i18n, "Granted", "Permission granted badge"), Color32::from_rgb(34, 197, 94)) } else if permission_pending { - ("Pending", Color32::from_rgb(234, 179, 8)) // Yellow + (tr!(self.note_context.i18n, "Pending", "Permission pending badge"), Color32::from_rgb(234, 179, 8)) } else { - ("Required", Color32::from_rgb(239, 68, 68)) // Red + (tr!(self.note_context.i18n, "Required", "Permission required badge"), Color32::from_rgb(239, 68, 68)) };crates/notedeck/src/platform/android.rs (1)
12-30: Static globals justified for JNI constraint, but consider future refactoring.The comment block (lines 12-18) explains why statics are necessary here - JNI callbacks have fixed signatures that cannot receive custom Rust state. This is a valid technical constraint. However, this does violate the coding guidelines that prohibit global variables.
For future consideration: investigate if these could be minimized by having JNI callbacks dispatch to a channel that the main app loop processes, reducing the number of globals.
.gitignore
Outdated
| .lsp | ||
| .idea | ||
| local.properties No newline at end of file | ||
| local.propertiesFCM/ |
There was a problem hiding this comment.
Keep ignoring local.properties to avoid accidental leakage.
If this replaced the original local.properties ignore, the file may now be committed. That file often contains local SDK paths (and sometimes secrets). Consider keeping both patterns.
🔧 Suggested fix
+local.properties
local.propertiesFCM/📝 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.
| local.propertiesFCM/ | |
| local.properties | |
| local.propertiesFCM/ |
🤖 Prompt for AI Agents
In @.gitignore at line 25, The .gitignore entry was changed to
"local.propertiesFCM/" which likely removed the original "local.properties"
ignore and risks committing sensitive local SDK paths; restore an explicit
"local.properties" ignore and keep the new "local.propertiesFCM/" pattern so
both files remain ignored (update the .gitignore to include both
"local.properties" and "local.propertiesFCM/"); ensure the change references the
.gitignore file and these exact patterns so the original local.properties rule
is preserved.
| val pendingIntent = PendingIntent.getActivity( | ||
| this, | ||
| System.currentTimeMillis().toInt(), | ||
| intent, | ||
| PendingIntent.FLAG_ONE_SHOT or PendingIntent.FLAG_IMMUTABLE | ||
| ) | ||
|
|
||
| val notification = NotificationCompat.Builder(this, CHANNEL_ID) | ||
| .setSmallIcon(R.mipmap.ic_launcher) | ||
| .setContentTitle(title) | ||
| .setContentText(body) | ||
| .setAutoCancel(true) | ||
| .setPriority(NotificationCompat.PRIORITY_HIGH) | ||
| .setContentIntent(pendingIntent) | ||
| .build() | ||
|
|
||
| val notificationManager = getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager | ||
| notificationManager.notify(System.currentTimeMillis().toInt(), notification) | ||
| } |
There was a problem hiding this comment.
Potential notification ID collision with rapid notifications.
Using System.currentTimeMillis().toInt() for both the PendingIntent request code and the notification ID can cause collisions if multiple notifications arrive within the same millisecond, as the Long to Int conversion may produce identical values. Consider using an atomic counter or including additional entropy.
🔧 Suggested improvement
companion object {
private const val TAG = "NotedeckFCM"
private const val CHANNEL_ID = "notedeck_notifications"
private const val CHANNEL_NAME = "Notedeck Notifications"
private var notificationIdCounter = AtomicInteger(0)
}
// In showNotification:
val notificationId = notificationIdCounter.getAndIncrement()
val pendingIntent = PendingIntent.getActivity(
this,
notificationId,
intent,
PendingIntent.FLAG_ONE_SHOT or PendingIntent.FLAG_IMMUTABLE
)
// ...
notificationManager.notify(notificationId, notification)🤖 Prompt for AI Agents
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotedeckFirebaseMessagingService.kt`
around lines 118 - 136, The current code in NotedeckFirebaseMessagingService
uses System.currentTimeMillis().toInt() as both the PendingIntent requestCode
and the notification ID (seen in PendingIntent.getActivity and
notificationManager.notify), which can collide; replace these runtime
millisecond-based ints with a monotonically-incrementing, thread-safe counter
(e.g., an AtomicInteger in the class companion object like
notificationIdCounter) and use its getAndIncrement() value for both the
PendingIntent requestCode and the notificationManager.notify call so each
notification and intent has a unique ID.
| Self::SetNotificationMode(mode) => { | ||
| let pubkey_hex = accounts.selected_account_pubkey().hex(); | ||
| let relay_urls: Vec<String> = | ||
| pool.relays.iter().map(|r| r.url().to_string()).collect(); | ||
|
|
||
| if let Err(e) = | ||
| notedeck::platform::set_notification_mode(mode, &pubkey_hex, &relay_urls) | ||
| { | ||
| tracing::error!("Failed to set notification mode: {}", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential panic if no account is selected.
selected_account_pubkey() may panic or return unexpected results if called when no account is selected. Consider adding a guard clause.
🔧 Suggested fix
Self::SetNotificationMode(mode) => {
+ let Some(pubkey) = accounts.get_selected_account().map(|a| a.key.pubkey()) else {
+ tracing::warn!("Cannot set notification mode: no account selected");
+ return route_action;
+ };
+ let pubkey_hex = pubkey.hex();
- let pubkey_hex = accounts.selected_account_pubkey().hex();
let relay_urls: Vec<String> =
pool.relays.iter().map(|r| r.url().to_string()).collect();🤖 Prompt for AI Agents
In `@crates/notedeck_columns/src/ui/settings.rs` around lines 103 - 113, The
handler for Self::SetNotificationMode calls accounts.selected_account_pubkey()
which can panic if no account is selected; update the branch in settings.rs (the
SetNotificationMode arm) to first check for a selected account (use the safe
accessor such as accounts.selected_account() or have selected_account_pubkey()
return an Option/Result) and early-return or log and skip the operation when
none exists, only deriving pubkey_hex and calling
notedeck::platform::set_notification_mode when a valid pubkey is present; keep
the existing error logging for set_notification_mode failures.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/notedeck_columns/src/ui/settings.rs`:
- Around line 821-829: The permission-status badge strings are user-facing but
not localized: wrap the three literals ("Granted", "Pending", "Required") with
the localization macro tr! (e.g., set badge_text = tr!("Granted") etc.) where
the badge_text is computed before calling notification_badge, and then run the
i18n extraction/export script to re-export source strings; update any nearby
uses in the same block (the if/else computing badge_text and badge_color, and
the call to notification_badge) so the localized values are passed through.
| let (badge_text, badge_color) = if permission_granted { | ||
| ("Granted", Color32::from_rgb(34, 197, 94)) // Green | ||
| } else if permission_pending { | ||
| ("Pending", Color32::from_rgb(234, 179, 8)) // Yellow | ||
| } else { | ||
| ("Required", Color32::from_rgb(239, 68, 68)) // Red | ||
| }; | ||
|
|
||
| notification_badge(ui, badge_text, badge_color); |
There was a problem hiding this comment.
Localize the permission status badge labels.
The badge text (“Granted/Pending/Required”) is user-facing but not wrapped in tr!, so it won’t be localized. Consider localizing these strings and then re-exporting source strings. As per coding guidelines, please wrap user-facing strings with tr! and run the export script after updates.
♻️ Suggested localization tweak
- let (badge_text, badge_color) = if permission_granted {
- ("Granted", Color32::from_rgb(34, 197, 94)) // Green
- } else if permission_pending {
- ("Pending", Color32::from_rgb(234, 179, 8)) // Yellow
- } else {
- ("Required", Color32::from_rgb(239, 68, 68)) // Red
- };
-
- notification_badge(ui, badge_text, badge_color);
+ let (badge_text, badge_color) = if permission_granted {
+ (
+ tr!(
+ self.note_context.i18n,
+ "Granted",
+ "Notification permission status: granted"
+ ),
+ Color32::from_rgb(34, 197, 94), // Green
+ )
+ } else if permission_pending {
+ (
+ tr!(
+ self.note_context.i18n,
+ "Pending",
+ "Notification permission status: pending"
+ ),
+ Color32::from_rgb(234, 179, 8), // Yellow
+ )
+ } else {
+ (
+ tr!(
+ self.note_context.i18n,
+ "Required",
+ "Notification permission status: required"
+ ),
+ Color32::from_rgb(239, 68, 68), // Red
+ )
+ };
+
+ notification_badge(ui, &badge_text, badge_color);🤖 Prompt for AI Agents
In `@crates/notedeck_columns/src/ui/settings.rs` around lines 821 - 829, The
permission-status badge strings are user-facing but not localized: wrap the
three literals ("Granted", "Pending", "Required") with the localization macro
tr! (e.g., set badge_text = tr!("Granted") etc.) where the badge_text is
computed before calling notification_badge, and then run the i18n
extraction/export script to re-export source strings; update any nearby uses in
the same block (the if/else computing badge_text and badge_color, and the call
to notification_badge) so the localized values are passed through.
Dependency NoteThis PR (#1265 - FCM notifications) should be merged before PR #1261 (Native WebSocket notifications). PR #1261 builds on the notification infrastructure established here. The two PRs implement different notification backends:
After both are merged, users can choose between FCM and Native notification modes in settings. Merge conflicts resolved - rebased on current master and CI should now pass. |
|
Where are getNotificationMode, setNotificationMode, enableFcmNotifications, disableFcmNotifications, enableNativeNotifications, disableNativeNotifications, isNotificationPermissionGranted, and requestNotificationPermission implemented on Android? I don't see them |
alltheseas
force-pushed
the
feat/android-fcm-notifications
branch
from
5928935 to
864d139
Compare
- Add Kotlin and google-services plugins to root build.gradle - Add Firebase BOM, firebase-messaging-ktx, coroutines, OkHttp deps - Register FCM service in AndroidManifest - Add POST_NOTIFICATIONS permission for Android 13+ - Add default notification channel metadata Note: Requires google-services.json from Firebase Console Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt`:
- Around line 306-324: The code in disableFcmNotifications clears the cached FCM
token which prevents re-registration later; instead either keep the stored token
and only call notepushClient.unregisterDevice(pubkeyHex, fcmToken) (remove the
prefs.edit().remove("fcm_token").apply() line) or, if you must clear it, replace
that removal with an explicit new token fetch using
FirebaseMessaging.getInstance().token and persist the new token so
enableFcmNotifications can succeed; update the disableFcmNotifications function
(and related prefs handling) accordingly and ensure
notepushClient.unregisterDevice is still called with the valid token.
- Around line 97-104: The processSelectedFile function force-unwraps
readUriContent(uri) with !! which can throw a KotlinNullPointerException and
produce a misleading stack trace; replace the force-unwrap with a null check:
call readUriContent(uri) into a local val (e.g., content), if content is null
log an explicit error and call nativeOnFilePickedFailed(uri.toString(), "Failed
to read content: <reason or generic message>") (optionally include exception
message from readUriContent if available) and return, otherwise call
nativeOnFilePickedWithContent(getUriInfo(uri), content); keep
nativeOnFilePickedWithContent, nativeOnFilePickedFailed, getUriInfo, and
processSelectedFile names to locate the code.
- Around line 119-133: The cursor column index may be -1 so calls to
getString/getLong can throw; in the method using contentResolver.query and the
cursor loop (where you call getColumnIndex for OpenableColumns.DISPLAY_NAME,
OpenableColumns.SIZE and "mime_type"), check each colIdx != -1 before calling
it.getString(colIdx)/it.getLong(colIdx) and provide safe fallbacks (e.g., null
or 0L) when the column is missing; update the logic around
getColumnIndex/getString/getLong to only read when index >= 0 and return the
info array with those safe values.
In `@crates/notedeck/src/platform/android.rs`:
- Around line 484-508: The current flow disables the old mode then calls the
relevant enable_* function and persists the new mode, which can leave runtime
and persisted state inconsistent if enabling fails; update set_notification_mode
to handle enable failures by wrapping the enable step in error handling: after
calling disable_fcm_notifications()/disable_native_notifications(), attempt the
new enable (enable_fcm_notifications or enable_native_notifications) and if it
returns Err, either (a) try to roll back by re-enabling the previous mode via
the appropriate enable_* for get_notification_mode() (and return the original
error if rollback fails or after rollback), or (b) persist
NotificationMode::Disabled with save_notification_mode_to_prefs to keep prefs
consistent, then return the error; only call
save_notification_mode_to_prefs(mode) after a successful enable (or after
explicitly saving Disabled on failure).
- Around line 193-197: The FCM_TOKEN write currently ignores a poisoned lock
while other places (get_fcm_token and set_signing_keypair) log such errors;
change the FCM_TOKEN.write() usage so it matches those functions: attempt the
write, and if it returns Err(poisoned), log the error via the same logger used
in get_fcm_token/set_signing_keypair and avoid silently dropping the token
(e.g., return or handle error path consistently). Reference the FCM_TOKEN static
and the token-storage block that currently uses if let Ok(mut guard) =
FCM_TOKEN.write().
- Around line 654-663: The JNI callback
Java_com_damus_notedeck_MainActivity_nativeOnNotificationPermissionResult writes
NOTIFICATION_PERMISSION_GRANTED (and sets NOTIFICATION_PERMISSION_PENDING) but
that atomic is never read and globals are forbidden; either remove
NOTIFICATION_PERMISSION_GRANTED and the stores in
Java_com_damus_notedeck_MainActivity_nativeOnNotificationPermissionResult and
rely solely on the existing is_notification_permission_granted() JNI call, or
refactor to move the cached state into the platform instance (e.g., add a field
like notification_permission_cache and pending flag on your Android platform
struct) so is_notification_permission_granted() first checks that instance cache
as a fast-path then falls back to the JNI query, and update
Java_com_damus_notedeck_MainActivity_nativeOnNotificationPermissionResult to
update the instance cache (not a global) via the same platform reference.
In `@crates/notedeck/src/platform/mod.rs`:
- Around line 14-18: The Default derive currently marks the enum variant Fcm as
the default, which conflicts with runtime calls to get_notification_mode() and
load_notification_mode_from_prefs().unwrap_or(Disabled) that expect Disabled to
be the default; change the enum's default to Disabled by moving the #[default]
attribute to the Disabled variant in the NotificationMode enum so
NotificationMode::default() returns Disabled and aligns with
get_notification_mode() and load_notification_mode_from_prefs() behavior.
🧹 Nitpick comments (4)
crates/notedeck_columns/src/ui/settings.rs (3)
149-172: Notification badge text contrast check uses a simplistic luminance formula.The
r + g + b > 382check is a rough approximation. For accessibility, consider using relative luminance (ITU-R BT.709 weights:0.2126*R + 0.7152*G + 0.0722*B). The current formula may pick the wrong text color for some badge backgrounds (e.g., the yellowColor32::from_rgb(234, 179, 8)sums to 421 > 382, giving black text, which is correct — but edge cases could fail).
176-286: Custom radio option widget uses hardcoded pixel offsets for text positioning.The label at
top + 10.0and description attop + 30.0assume specific font sizes. If font size settings change (the user can adjust note body font size), text may overlap or be clipped within the fixed 52px height. Consider usingui.put_back_response(response)with a layout-based approach, or at minimum derive offsets from font metrics.
806-811: Cache notification state values in UI struct to reduce JNI calls every frame.Two of the three platform calls here involve JNI thread attachment and Java method invocation:
get_notification_mode()callsload_notification_mode_from_prefs()(JNIgetNotificationMode()), andis_notification_permission_granted()directly invokes Java code. Called every frame in the settings view, this adds unnecessary overhead. Cache these values in the settings UI state and refresh only when the section becomes visible or after an action is taken.Note:
is_notification_permission_pending()is not affected—it only performs an atomic load.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt (1)
195-201: Remove empty lifecycle overrides.
onResume()andonPause()only callsuperand add no behavior. They can be removed to reduce noise.
| private fun processSelectedFile(uri: Uri) { | ||
| try { | ||
| nativeOnFilePickedWithContent(getUriInfo(uri), readUriContent(uri)!!) | ||
| } catch (e: Exception) { | ||
| Log.e(TAG, "Error processing file: $uri", e) | ||
| nativeOnFilePickedFailed(uri.toString(), e.toString()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Force-unwrap readUriContent(uri)!! can crash.
If readUriContent returns null (e.g., on I/O or security error), the !! operator throws a KotlinNullPointerException. While the catch block handles it, this produces a misleading stack trace. Use a null check with an explicit error message instead.
🔧 Suggested fix
private fun processSelectedFile(uri: Uri) {
try {
- nativeOnFilePickedWithContent(getUriInfo(uri), readUriContent(uri)!!)
+ val content = readUriContent(uri)
+ ?: throw IOException("Failed to read content from URI: $uri")
+ nativeOnFilePickedWithContent(getUriInfo(uri), content)
} catch (e: Exception) {
Log.e(TAG, "Error processing file: $uri", e)
nativeOnFilePickedFailed(uri.toString(), e.toString())
}
}🤖 Prompt for AI Agents
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt`
around lines 97 - 104, The processSelectedFile function force-unwraps
readUriContent(uri) with !! which can throw a KotlinNullPointerException and
produce a misleading stack trace; replace the force-unwrap with a null check:
call readUriContent(uri) into a local val (e.g., content), if content is null
log an explicit error and call nativeOnFilePickedFailed(uri.toString(), "Failed
to read content: <reason or generic message>") (optionally include exception
message from readUriContent if available) and return, otherwise call
nativeOnFilePickedWithContent(getUriInfo(uri), content); keep
nativeOnFilePickedWithContent, nativeOnFilePickedFailed, getUriInfo, and
processSelectedFile names to locate the code.
| val cursor: Cursor? = contentResolver.query(uri, null, null, null, null) | ||
| cursor?.use { | ||
| while (it.moveToNext()) { | ||
| val info = arrayOfNulls<Any>(3) | ||
| var colIdx = it.getColumnIndex(OpenableColumns.DISPLAY_NAME) | ||
| info[0] = it.getString(colIdx) | ||
| colIdx = it.getColumnIndex(OpenableColumns.SIZE) | ||
| info[1] = it.getLong(colIdx) | ||
| colIdx = it.getColumnIndex("mime_type") | ||
| info[2] = it.getString(colIdx) | ||
| return info | ||
| } | ||
| } | ||
| return emptyArray() | ||
| } |
There was a problem hiding this comment.
getColumnIndex can return -1, causing getString(-1) / getLong(-1) to throw.
If the cursor doesn't contain the expected column (e.g., mime_type which isn't a standard OpenableColumns constant), getColumnIndex returns -1 and the subsequent getString/getLong call throws an IllegalStateException. Add bounds checks.
🔧 Suggested fix
cursor?.use {
while (it.moveToNext()) {
val info = arrayOfNulls<Any>(3)
- var colIdx = it.getColumnIndex(OpenableColumns.DISPLAY_NAME)
- info[0] = it.getString(colIdx)
- colIdx = it.getColumnIndex(OpenableColumns.SIZE)
- info[1] = it.getLong(colIdx)
- colIdx = it.getColumnIndex("mime_type")
- info[2] = it.getString(colIdx)
+ var colIdx = it.getColumnIndex(OpenableColumns.DISPLAY_NAME)
+ info[0] = if (colIdx >= 0) it.getString(colIdx) else null
+ colIdx = it.getColumnIndex(OpenableColumns.SIZE)
+ info[1] = if (colIdx >= 0) it.getLong(colIdx) else 0L
+ colIdx = it.getColumnIndex("mime_type")
+ info[2] = if (colIdx >= 0) it.getString(colIdx) else null
return info
}
}📝 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 cursor: Cursor? = contentResolver.query(uri, null, null, null, null) | |
| cursor?.use { | |
| while (it.moveToNext()) { | |
| val info = arrayOfNulls<Any>(3) | |
| var colIdx = it.getColumnIndex(OpenableColumns.DISPLAY_NAME) | |
| info[0] = it.getString(colIdx) | |
| colIdx = it.getColumnIndex(OpenableColumns.SIZE) | |
| info[1] = it.getLong(colIdx) | |
| colIdx = it.getColumnIndex("mime_type") | |
| info[2] = it.getString(colIdx) | |
| return info | |
| } | |
| } | |
| return emptyArray() | |
| } | |
| val cursor: Cursor? = contentResolver.query(uri, null, null, null, null) | |
| cursor?.use { | |
| while (it.moveToNext()) { | |
| val info = arrayOfNulls<Any>(3) | |
| var colIdx = it.getColumnIndex(OpenableColumns.DISPLAY_NAME) | |
| info[0] = if (colIdx >= 0) it.getString(colIdx) else null | |
| colIdx = it.getColumnIndex(OpenableColumns.SIZE) | |
| info[1] = if (colIdx >= 0) it.getLong(colIdx) else 0L | |
| colIdx = it.getColumnIndex("mime_type") | |
| info[2] = if (colIdx >= 0) it.getString(colIdx) else null | |
| return info | |
| } | |
| } | |
| return emptyArray() | |
| } |
🤖 Prompt for AI Agents
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt`
around lines 119 - 133, The cursor column index may be -1 so calls to
getString/getLong can throw; in the method using contentResolver.query and the
cursor loop (where you call getColumnIndex for OpenableColumns.DISPLAY_NAME,
OpenableColumns.SIZE and "mime_type"), check each colIdx != -1 before calling
it.getString(colIdx)/it.getLong(colIdx) and provide safe fallbacks (e.g., null
or 0L) when the column is missing; update the logic around
getColumnIndex/getString/getLong to only read when index >= 0 and return the
info array with those safe values.
| fun disableFcmNotifications() { | ||
| val prefs = getSharedPreferences("notedeck_fcm", Context.MODE_PRIVATE) | ||
| val fcmToken = prefs.getString("fcm_token", null) | ||
| val pubkeyHex = getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) | ||
| .getString("registered_pubkey", null) | ||
|
|
||
| if (fcmToken != null && pubkeyHex != null) { | ||
| notificationScope.launch { | ||
| val success = notepushClient.unregisterDevice(pubkeyHex, fcmToken) | ||
| if (success) { | ||
| Log.d(TAG, "FCM unregistration succeeded") | ||
| } else { | ||
| Log.e(TAG, "FCM unregistration failed") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| prefs.edit().remove("fcm_token").apply() | ||
| } |
There was a problem hiding this comment.
Clearing cached FCM token on disable may prevent re-registration.
Line 323 removes fcm_token from SharedPreferences. If the user re-enables FCM notifications later, enableFcmNotifications will fail because no token is available (line 279). Firebase won't trigger onNewToken again if the token hasn't actually changed. Consider keeping the cached token and only unregistering from the notepush server, or explicitly requesting a new token via FirebaseMessaging.getInstance().token.
🤖 Prompt for AI Agents
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt`
around lines 306 - 324, The code in disableFcmNotifications clears the cached
FCM token which prevents re-registration later; instead either keep the stored
token and only call notepushClient.unregisterDevice(pubkeyHex, fcmToken) (remove
the prefs.edit().remove("fcm_token").apply() line) or, if you must clear it,
replace that removal with an explicit new token fetch using
FirebaseMessaging.getInstance().token and persist the new token so
enableFcmNotifications can succeed; update the disableFcmNotifications function
(and related prefs handling) accordingly and ensure
notepushClient.unregisterDevice is still called with the valid token.
|
|
||
| // Store token for later use | ||
| if let Ok(mut guard) = FCM_TOKEN.write() { | ||
| *guard = Some(token); | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling: poisoned lock silently ignored here but logged elsewhere.
get_fcm_token (line 143) and set_signing_keypair (line 155) properly log poisoned lock errors, but this FCM_TOKEN.write() silently ignores the error. Apply the same pattern.
🔧 Suggested fix
// Store token for later use
- if let Ok(mut guard) = FCM_TOKEN.write() {
- *guard = Some(token);
+ match FCM_TOKEN.write() {
+ Ok(mut guard) => {
+ *guard = Some(token);
+ }
+ Err(e) => {
+ error!("Failed to store FCM token: lock poisoned: {}", e);
+ }
}🤖 Prompt for AI Agents
In `@crates/notedeck/src/platform/android.rs` around lines 193 - 197, The
FCM_TOKEN write currently ignores a poisoned lock while other places
(get_fcm_token and set_signing_keypair) log such errors; change the
FCM_TOKEN.write() usage so it matches those functions: attempt the write, and if
it returns Err(poisoned), log the error via the same logger used in
get_fcm_token/set_signing_keypair and avoid silently dropping the token (e.g.,
return or handle error path consistently). Reference the FCM_TOKEN static and
the token-storage block that currently uses if let Ok(mut guard) =
FCM_TOKEN.write().
| let current = get_notification_mode(); | ||
|
|
||
| if current == mode { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // Disable current mode first (mutual exclusivity) | ||
| match current { | ||
| NotificationMode::Fcm => disable_fcm_notifications()?, | ||
| NotificationMode::Native => disable_native_notifications()?, | ||
| NotificationMode::Disabled => {} | ||
| } | ||
|
|
||
| // Enable new mode | ||
| match mode { | ||
| NotificationMode::Fcm => enable_fcm_notifications(pubkey_hex)?, | ||
| NotificationMode::Native => enable_native_notifications(pubkey_hex, relay_urls)?, | ||
| NotificationMode::Disabled => {} | ||
| } | ||
|
|
||
| // Persist to SharedPreferences | ||
| save_notification_mode_to_prefs(mode)?; | ||
|
|
||
| info!("Notification mode changed from {:?} to {:?}", current, mode); | ||
| Ok(()) |
There was a problem hiding this comment.
set_notification_mode may leave an inconsistent state if enabling the new mode fails.
If disable_fcm_notifications() succeeds at line 492 but enable_native_notifications() fails at line 500, the user ends up with both modes disabled and the old mode's preference cleared, but save_notification_mode_to_prefs at line 505 is never reached. The notification mode in SharedPreferences still reflects the old (now-disabled) mode, creating a mismatch between persisted state and actual state.
Consider saving Disabled as a fallback if enabling the new mode fails, or rolling back by re-enabling the old mode.
🤖 Prompt for AI Agents
In `@crates/notedeck/src/platform/android.rs` around lines 484 - 508, The current
flow disables the old mode then calls the relevant enable_* function and
persists the new mode, which can leave runtime and persisted state inconsistent
if enabling fails; update set_notification_mode to handle enable failures by
wrapping the enable step in error handling: after calling
disable_fcm_notifications()/disable_native_notifications(), attempt the new
enable (enable_fcm_notifications or enable_native_notifications) and if it
returns Err, either (a) try to roll back by re-enabling the previous mode via
the appropriate enable_* for get_notification_mode() (and return the original
error if rollback fails or after rollback), or (b) persist
NotificationMode::Disabled with save_notification_mode_to_prefs to keep prefs
consistent, then return the error; only call
save_notification_mode_to_prefs(mode) after a successful enable (or after
explicitly saving Disabled on failure).
| pub extern "C" fn Java_com_damus_notedeck_MainActivity_nativeOnNotificationPermissionResult( | ||
| _env: JNIEnv, | ||
| _class: JClass, | ||
| granted: jni::sys::jboolean, | ||
| ) { | ||
| let granted = granted != 0; | ||
| debug!("Notification permission result: {}", granted); | ||
| NOTIFICATION_PERMISSION_GRANTED.store(granted, Ordering::SeqCst); | ||
| NOTIFICATION_PERMISSION_PENDING.store(false, Ordering::SeqCst); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all occurrences of NOTIFICATION_PERMISSION_GRANTED in the file
rg -n "NOTIFICATION_PERMISSION_GRANTED" crates/notedeck/src/platform/android.rsRepository: damus-io/notedeck
Length of output: 214
🏁 Script executed:
#!/bin/bash
# Get the implementation of is_notification_permission_granted() function
rg -n -A 10 "fn is_notification_permission_granted" crates/notedeck/src/platform/android.rsRepository: damus-io/notedeck
Length of output: 612
🏁 Script executed:
#!/bin/bash
# Search for NOTIFICATION_PERMISSION_GRANTED across the entire codebase
rg -n "NOTIFICATION_PERMISSION_GRANTED" --type rustRepository: damus-io/notedeck
Length of output: 294
Remove NOTIFICATION_PERMISSION_GRANTED static or implement it as a fast-path cache.
The atomic at line 27 is written in the JNI callback (line 661) but never read anywhere in the codebase. Meanwhile, is_notification_permission_granted() at line 624 always performs a JNI call instead of checking this cached value. Additionally, this global variable violates the coding guideline: "Global variables are not allowed in this codebase, even if they are thread local; state should be managed in a struct that is passed in as reference." Either remove the dead atomic entirely or refactor to use it as a cache to avoid redundant JNI calls, while addressing the architectural constraint against global state.
🤖 Prompt for AI Agents
In `@crates/notedeck/src/platform/android.rs` around lines 654 - 663, The JNI
callback
Java_com_damus_notedeck_MainActivity_nativeOnNotificationPermissionResult writes
NOTIFICATION_PERMISSION_GRANTED (and sets NOTIFICATION_PERMISSION_PENDING) but
that atomic is never read and globals are forbidden; either remove
NOTIFICATION_PERMISSION_GRANTED and the stores in
Java_com_damus_notedeck_MainActivity_nativeOnNotificationPermissionResult and
rely solely on the existing is_notification_permission_granted() JNI call, or
refactor to move the cached state into the platform instance (e.g., add a field
like notification_permission_cache and pending flag on your Android platform
struct) so is_notification_permission_granted() first checks that instance cache
as a fast-path then falls back to the JNI query, and update
Java_com_damus_notedeck_MainActivity_nativeOnNotificationPermissionResult to
update the instance cache (not a global) via the same platform reference.
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] | ||
| pub enum NotificationMode { | ||
| /// Firebase Cloud Messaging - battery efficient, requires Google services | ||
| #[default] | ||
| Fcm, |
There was a problem hiding this comment.
#[default] on Fcm conflicts with the runtime default of Disabled.
The Default derive marks Fcm as the default variant, but get_notification_mode() returns NotificationMode::Disabled when no preference exists (both on Android via load_notification_mode_from_prefs().unwrap_or(Disabled) and on non-Android platforms). If anyone calls NotificationMode::default(), they'll get Fcm, which is inconsistent. Consider making Disabled the #[default] variant instead.
🔧 Suggested fix
pub enum NotificationMode {
/// Firebase Cloud Messaging - battery efficient, requires Google services
- #[default]
Fcm,
/// Direct relay connection - maximum privacy, higher battery usage
Native,
/// Notifications disabled
+ #[default]
Disabled,
}📝 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.
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] | |
| pub enum NotificationMode { | |
| /// Firebase Cloud Messaging - battery efficient, requires Google services | |
| #[default] | |
| Fcm, | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] | |
| pub enum NotificationMode { | |
| /// Firebase Cloud Messaging - battery efficient, requires Google services | |
| Fcm, | |
| /// Direct relay connection - maximum privacy, higher battery usage | |
| Native, | |
| /// Notifications disabled | |
| #[default] | |
| Disabled, | |
| } |
🤖 Prompt for AI Agents
In `@crates/notedeck/src/platform/mod.rs` around lines 14 - 18, The Default derive
currently marks the enum variant Fcm as the default, which conflicts with
runtime calls to get_notification_mode() and
load_notification_mode_from_prefs().unwrap_or(Disabled) that expect Disabled to
be the default; change the enum's default to Disabled by moving the #[default]
attribute to the Disabled variant in the NotificationMode enum so
NotificationMode::default() returns Disabled and aligns with
get_notification_mode() and load_notification_mode_from_prefs() behavior.
864d139 to
c7f89a3
Compare
- NotedeckFirebaseMessagingService: handles FCM messages, token refresh - NotepushClient: HTTP client for notepush server with NIP-98 auth - Supports register/unregister device and preferences management - Parses Nostr events from FCM data payload for notifications Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add FCM token storage and retrieval via static RwLock - Add signing keypair management for NIP-98 auth - Implement sign_nip98_event() with proper kind 27235 format - Add JNI functions: nativeOnFcmTokenRefreshed, nativeProcessNostrEvent, nativeSignNip98Auth - Hook set_signing_keypair into account selection flow Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add NotificationMode enum with Fcm (default), Native, Disabled variants - Add cross-platform notification API with Android implementations - Implement mutual exclusivity between FCM and Native modes - Add JNI functions for notification mode persistence Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…Disabled RadioGroup - Add shadcn-inspired RadioGroup for notification mode selection - FCM (Google Push) as default, Native (Direct Relay), or Disabled - Add permission status badge and request button - Add collapsible privacy information panel showing tradeoffs - Use NotificationMode enum from platform module - 44px touch targets for mobile accessibility Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Mechanical conversion of MainActivity from Java to Kotlin with no behavior changes. Prepares for Kotlin interop with the existing FCM service classes (NotepushClient, NotedeckFirebaseMessagingService) which use suspend functions. JNI bindings are unaffected — JNI function names are derived from package + class name, not file extension. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implement the 8 methods that the Rust JNI bridge (android.rs) calls on the activity context for notification management: - getNotificationMode / setNotificationMode: persist mode index (0=FCM, 1=Native, 2=Disabled) in SharedPreferences - enableFcmNotifications: read cached FCM token and register with notepush server via NotepushClient on a background coroutine - disableFcmNotifications: unregister from notepush and clear token - enableNativeNotifications / disableNativeNotifications: store or clear relay config for a future native WebSocket service - isNotificationPermissionGranted: check POST_NOTIFICATIONS on API 33+ - requestNotificationPermission: launch runtime permission dialog, forward result to Rust via nativeOnNotificationPermissionResult Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
alltheseas
force-pushed
the
feat/android-fcm-notifications
branch
from
c7f89a3 to
560efaa
Compare
2 participants
Summary
Adds Firebase Cloud Messaging (FCM) push notification support to the Android app, enabling notifications from the notepush server when the app is in the background or killed.
NotedeckFirebaseMessagingServicefor handling FCM messages and token refreshNotepushClientfor registering devices with the notepush serverMainActivity.java→MainActivity.ktfor Kotlin interop with FCM servicesCloses #1264
Server dependency: Requires damus-io/notepush#19 to be merged and deployed for FCM backend support.
Related PRs - Merge Order
platform/andsettings.rscodeTwo notification backends:
After both merge, users choose their preferred mode in the unified Settings UI added by this PR.
Implementation Details
Kotlin Components:
MainActivity.kt: Converted from Java — implements 8 notification methods called from Rust via JNI (get/set mode, enable/disable FCM, enable/disable native, permission check/request)NotedeckFirebaseMessagingService: Receives FCM messages, parses Nostr events from data payload, displays notificationsNotepushClient: HTTP client for notepush API with NIP-98 authenticationRust Components:
NotificationModeenum:Fcm,Native,DisabledSettings UI:
Test Plan
Summary by CodeRabbit
Release Notes
New Features
Chores