WIP feat: cross-platform notification system (Android FCM/Native + macOS + Linux)#1296
WIP feat: cross-platform notification system (Android FCM/Native + macOS + Linux)#1296alltheseas wants to merge 23 commits intodamus-io:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds a cross-platform notification subsystem: new notification types, extraction, image caching, desktop (Linux/macOS) and Android backends (FCM/native), a NotificationManager/service/worker, JNI bridges and Android services, settings UI and persistence, and related build/manifest updates. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ 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 |
alltheseas
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Cargo.toml (1)
4-15:⚠️ Potential issue | 🟡 MinorLine 14 contains duplicate workspace members and incorrect formatting.
notedeck_dave,notedeck_ui, andnotedeck_clndashare already listed on lines 8, 11, and 12 respectively. These three should not be repeated on line 14. However,enostrandtokenatorappear only on line 14 and must remain as workspace members.Remove the duplicate crate entries from line 14, leaving only the new members:
Proposed fix
"crates/notedeck_clndash", - "crates/enostr", "crates/tokenator", "crates/notedeck_dave", "crates/notedeck_ui", "crates/notedeck_clndash", + "crates/enostr", "crates/tokenator", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 4 - 15, The workspace members list in the members = [...] array contains duplicate entries; remove the repeated crate paths "crates/notedeck_dave", "crates/notedeck_ui", and "crates/notedeck_clndash" from the trailing line while keeping "crates/enostr" and "crates/tokenator"; also normalize formatting so each member is a separate quoted entry (e.g., "crates/enostr", "crates/tokenator") consistent with the existing list and ensure the members array contains unique crate paths only.
🟡 Minor comments (15)
crates/notedeck_chrome/Cargo.toml-85-86 (1)
85-86:⚠️ Potential issue | 🟡 MinorStale copyright year and potential version drift in bundle metadata.
Two concerns:
copyright = "Copyright © 2024 Damus, Nostr Inc."— the year is stale (current year: 2026).version = "0.7.1"is hardcoded here while[package].versionis managed by the workspace. These can silently diverge on version bumps.🛠️ Suggested fixes
-version = "0.7.1" -copyright = "Copyright © 2024 Damus, Nostr Inc." +copyright = "Copyright © 2026 Damus, Nostr Inc."For the bundle
version, consider documenting that it must be kept in sync with the workspace version, or derive it from the workspace if the bundling tool supports it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/Cargo.toml` around lines 85 - 86, Update the bundle metadata in Cargo.toml: replace the hardcoded copyright value and the hardcoded bundle version to avoid drift — change the copyright = "Copyright © 2024 Damus, Nostr Inc." to the current year (2026) and either remove or document the bundle `version = "0.7.1"` so it is kept in sync with the workspace-managed [package].version (or configure the bundling tool to derive the bundle version from the workspace package version); ensure references to the `version` and `copyright` keys in Cargo.toml are updated accordingly.crates/notedeck_chrome/android/build.gradle-5-6 (1)
5-6:⚠️ Potential issue | 🟡 MinorUpdate Kotlin and Google Services plugin versions to latest stable.
org.jetbrains.kotlin.android:1.9.22is outdated; latest stable is2.3.10(Feb 2026).com.google.gms.google-services:4.4.0has a newer patch; latest is4.4.4.Both plugins are declared with
apply falseand are functionally compatible at their current versions. Thekotlin-stdlib:1.9.22inapp/build.gradlecorrectly matches the Kotlin plugin version, so there are no version mismatches. However, updating to the latest versions is recommended to stay current with security updates and compatibility improvements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/build.gradle` around lines 5 - 6, The Gradle plugin versions for org.jetbrains.kotlin.android and com.google.gms.google-services are outdated; update the plugin declarations for id 'org.jetbrains.kotlin.android' to version '2.3.10' and id 'com.google.gms.google-services' to version '4.4.4' (keeping apply false), and also update the Kotlin runtime dependency (kotlin-stdlib) in app/build.gradle to match the new Kotlin plugin version (2.3.10) so plugin and stdlib versions remain aligned.crates/notedeck_chrome/android/app/build.gradle-38-39 (1)
38-39:⚠️ Potential issue | 🟡 MinorFirebase BOM
32.7.0is significantly outdated, andfirebase-messaging-ktxis end-of-maintenance.The Firebase Android BoM has reached v34.9.0 (released Feb 2026), while this PR pins
32.7.0— two major versions behind. More critically: in July 2025, Firebase stopped releasing new versions of KTX modules and removed them from the BoM starting with v34.0.0. Thefirebase-messaging-ktxartifact is now end-of-maintenance.This means:
- Security and bug fixes for FCM are being missed.
- Upgrading the BOM to ≥ 34.0.0 in the future will require changing
firebase-messaging-ktx→firebase-messaging(KTX APIs were merged into the main module).It's worth upgrading now rather than deferring the migration.
🔧 Suggested update
- implementation platform("com.google.firebase:firebase-bom:32.7.0") - implementation "com.google.firebase:firebase-messaging-ktx" + implementation platform("com.google.firebase:firebase-bom:34.9.0") + implementation "com.google.firebase:firebase-messaging"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/app/build.gradle` around lines 38 - 39, The Gradle deps pin the Firebase BoM to 32.7.0 and use the removed KTX artifact (implementation platform("com.google.firebase:firebase-bom:32.7.0") and implementation "com.google.firebase:firebase-messaging-ktx"), which is end-of-maintenance; update the BOM to a 34.x release (e.g., 34.9.0) and replace firebase-messaging-ktx with the main artifact implementation "com.google.firebase:firebase-messaging", then audit any code that relied on KTX-only APIs and switch to the merged APIs/imports as needed, run a Gradle sync/build and fix any API or import changes.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotepushClient.kt-72-76 (1)
72-76:⚠️ Potential issue | 🟡 MinorEmpty string body with
application/jsoncontent type.
"".toRequestBody(JSON_MEDIA_TYPE)sends an empty string with a JSON content type header. A strict server or middleware might reject this as invalid JSON. If an empty body is intended, consider omitting the content type, or use"{}".toRequestBody(JSON_MEDIA_TYPE)for a valid empty JSON object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotepushClient.kt` around lines 72 - 76, The PUT request currently uses "".toRequestBody(JSON_MEDIA_TYPE) which sends an empty string with application/json; update the Request.Builder put call in NotepushClient.kt to send valid JSON (e.g. "{}" as the body created with JSON_MEDIA_TYPE) or, if truly no body is required, use the parameterless put() overload to omit the content type; adjust the code around Request.Builder().url(url).put(...) and JSON_MEDIA_TYPE accordingly.crates/notedeck/src/notifications/extraction.rs-54-69 (1)
54-69:⚠️ Potential issue | 🟡 Minor
kinddefaults to0silently when missing — potentially misleading.If the
"kind"field is absent, the event getskind = 0(Nostr metadata). While it won't cause a crash (the id/pubkey validation below will catch truly malformed events), a missing kind on an otherwise valid event could be misrouted. Consider returningNonewhenkindis missing, or at least logging a warning.Suggested approach
- let kind = obj.get("kind").and_then(|v| v.as_i64()).unwrap_or(0) as i32; + let kind = match obj.get("kind").and_then(|v| v.as_i64()) { + Some(k) => k as i32, + None => { + warn!("Event missing 'kind' field, dropping"); + return None; + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/notifications/extraction.rs` around lines 54 - 69, The code silently defaults `kind` to 0 which can misclassify events; update the extraction in this function so that when `obj.get("kind")` is missing or not an integer you do not default to 0 but instead treat the event as invalid (return None) or at minimum log a warning and skip it; change the `kind` extraction logic (the `kind` binding near `id`, `pubkey`, `content`) to return early/None on missing/invalid `kind` rather than using `unwrap_or(0)`, ensuring downstream validation uses a real kind value.crates/notedeck/src/notifications/macos.rs-34-39 (1)
34-39:⚠️ Potential issue | 🟡 MinorUnsafe
Send/Syncimpl onMacOSDelegatewrapper in manager.rs relies on a non-trivial invariant.The
SAFETYcomment at line 34–35 states the delegate is "never accessed from Rust after initialization," butMacOSDelegateis a field ofNotificationManager(inmanager.rs) which callsis_initialized()on it. The true safety argument is thatRetained<AnyObject>is only kept alive (preventing deallocation) and the Objective-C runtime handles all callback dispatch on the main thread. The safety comment should be updated to reflect this more accurately.Note: This comment references the
MacOSDelegatewrapper inmanager.rs(lines 34–39) which wraps theRetained<AnyObject>returned byinitialize_on_main_thread()from this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/notifications/macos.rs` around lines 34 - 39, Update the SAFETY comment on the MacOSDelegate wrapper to reflect the actual invariant: explain that the wrapper holds a Retained<AnyObject> returned from initialize_on_main_thread(), which keeps the Objective‑C delegate object alive (prevents deallocation) and that the Objective‑C runtime guarantees callbacks are dispatched on the main thread, so Send/Sync is safe because Rust code will not race with Objective‑C callbacks and the object is only retained for the lifetime managed by NotificationManager (which calls is_initialized()). Reference MacOSDelegate, initialize_on_main_thread, Retained<AnyObject>, and NotificationManager in the comment to make the invariant explicit.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotificationHelper.kt-234-283 (1)
234-283:⚠️ Potential issue | 🟡 MinorImage loading has no HTTPS validation and relies on external
robohash.org.
URL(pictureUrl)at line 246 will follow any URL, including non-HTTPS. For user-providedpictureUrlvalues from Nostr profiles, this could be an SSRF vector if the Android app is running in a privileged network context. Consider validating the URL scheme is HTTPS before loading.Additionally, the
robohash.orgfallback (lines 249, 267) is an external dependency — if the service is unavailable, every notification for an author without a profile picture will incur a 5-second timeout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotificationHelper.kt` around lines 234 - 283, The loadProfileImage function accepts unvalidated pictureUrl and constructs URL(pictureUrl), which allows non-HTTPS schemes and risks SSRF; change loadProfileImage to validate the pictureUrl scheme before using it (only accept https schemes and ignore or log+fallback for others), and avoid calling external robohash.org on every miss — replace the robohash fallback with a local bundled default avatar or a configurable trusted CDN and/or make the robohash call optional and rate-limited with a shorter timeout; update uses of pictureUrl, URL(...) construction, and the fallback logic around profileImageCache and BitmapFactory.decodeStream to reflect these checks and safe defaults.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotificationHelper.kt-52-53 (1)
52-53:⚠️ Potential issue | 🟡 MinorEvent IDs and pubkeys logged at INFO level in production.
Lines 52–53 log
eventIdandauthorPubkeyprefixes atLog.iin production builds. While these are truncated, they can still correlate user activity. Consider gating these behindBuildConfig.DEBUGfor consistency with the FCM token redaction approach used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotificationHelper.kt` around lines 52 - 53, The info-level logging in showNotification that prints eventId and authorPubkey (Log.i calls) should be gated behind BuildConfig.DEBUG like the FCM token redaction elsewhere; update the Log.i statements in NotificationHelper.kt (the showNotification function) to only execute when BuildConfig.DEBUG is true (or demote to Log.d within that conditional) so event IDs and pubkeys are not emitted at INFO level in production.crates/notedeck/src/notifications/manager.rs-178-182 (1)
178-182:⚠️ Potential issue | 🟡 MinorHot-path allocation: monitored pubkeys Vec rebuilt on every relay message.
process_relay_messageis called for every relay message. Lines 178–182 allocate aVec<String>of all account pubkeys each time. Consider caching the monitored pubkey set inNotificationManagerand refreshing it only when accounts change, rather than rebuilding on every message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/notifications/manager.rs` around lines 178 - 182, process_relay_message currently rebuilds the monitored Vec<String> every call (via accounts.cache.accounts().map(...).collect()), causing hot-path allocations; instead add a cached monitored set on NotificationManager (e.g., monitored_pubkeys: HashSet<String> or HashSet<Pubkey>) and use that in process_relay_message, and update/refresh this cache only when accounts change (hook the account-change path that mutates accounts.cache or expose an on_accounts_updated method to recompute monitored_pubkeys from accounts.cache.accounts()); ensure process_relay_message reads from the cached field rather than recreating the Vec on each message.crates/notedeck/src/notifications/manager.rs-34-39 (1)
34-39:⚠️ Potential issue | 🟡 MinorUnsafe
Send + Syncimpls forMacOSDelegate— safety comment is inaccurate.The comment says the delegate is "never accessed from Rust after initialization," but
is_initialized()(line 29) readsself.0viais_some(). The actual safety guarantee is that theRetained<AnyObject>only serves as a prevent-deallocation handle — Rust never sends messages to the object or reads its fields, and the Objective-C runtime owns all callback dispatch. Update the comment to reflect this.📝 Suggested comment update
-// SAFETY: The delegate is only kept alive for macOS callbacks, -// never accessed from Rust after initialization. +// SAFETY: The Retained<AnyObject> is only held to prevent deallocation. +// Rust never sends Objective-C messages to the delegate after installation; +// `is_initialized()` only checks the Option wrapper, not the ObjC object. +// All delegate callbacks are dispatched by the Objective-C runtime on the main thread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/notifications/manager.rs` around lines 34 - 39, The safety comment above the unsafe impls for MacOSDelegate is misleading because is_initialized() does read self.0; change the comment to state that the Retained<AnyObject> is only used to keep the Objective‑C delegate alive (prevent deallocation), Rust code never sends messages to the Objective‑C object nor reads its fields, and all callback dispatching is performed by the Objective‑C runtime — therefore it is safe to implement Send and Sync for MacOSDelegate; update the comment near MacOSDelegate, reference is_initialized() and the Retained<AnyObject> behavior, and ensure the explanation clearly limits Rust access to no message sends or field reads.crates/notedeck_columns/src/ui/settings.rs-824-830 (1)
824-830:⚠️ Potential issue | 🟡 MinorBadge text strings are not localized.
"Granted","Pending", and"Required"at lines 825, 827, 829 are user-facing but not wrapped intr!(). As per coding guidelines, user-facing strings should usetr!ortr_plural!macros for localization via Fluent.🌐 Proposed localization fix
let (badge_text, badge_color) = if permission_granted { - ("Granted", Color32::from_rgb(34, 197, 94)) // Green + (tr!(self.note_context.i18n, "Granted", "Notification 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", "Notification permission pending badge"), Color32::from_rgb(234, 179, 8)) } else { - ("Required", Color32::from_rgb(239, 68, 68)) // Red + (tr!(self.note_context.i18n, "Required", "Notification permission required badge"), Color32::from_rgb(239, 68, 68)) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_columns/src/ui/settings.rs` around lines 824 - 830, The badge text strings inside the let binding for (badge_text, badge_color) (the branch that sets "Granted", "Pending", "Required") are not localized; wrap each user-facing string with the tr!() macro (e.g., tr!("Granted")) so they go through the Fluent localization system, keeping the same variable name (badge_text) and preserving the Color32 values in the branches inside the function/block in settings.rs where that let binding occurs.crates/notedeck_columns/src/ui/settings.rs-104-116 (1)
104-116:⚠️ Potential issue | 🟡 MinorThe localization issue is the primary concern; the panic risk is low due to the fallback account guarantee.
The
selected_account_pubkey()method is safe becauseAccounts::new()always initializes a fallback account, guaranteeing thatcache.selected()will always find a valid entry. The.expect()is backed by a solid invariant.However, the badge status strings at lines 824–829 ("Granted", "Pending", "Required") must be wrapped with the
tr!()macro for localization per the coding guidelines. These are user-facing strings that translators need to handle.Note: The proposed defensive guard using
get_selected_account_pubkey()won't work—this method doesn't exist in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_columns/src/ui/settings.rs` around lines 104 - 116, Wrap the user-facing badge status strings "Granted", "Pending", and "Required" with the tr!() localization macro where they are rendered (lines referenced around 824–829) so translators can localize them; leave the existing selected_account_pubkey() usage (and its .expect-backed invariant) as-is and do not replace it with a non-existent get_selected_account_pubkey() method. Ensure each literal is replaced by tr!("Granted"), tr!("Pending"), and tr!("Required") respectively in the badge rendering code.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt-300-334 (1)
300-334:⚠️ Potential issue | 🟡 Minor
enableFcmNotificationsreturnstruebefore async registration completes.The method returns
truesynchronously (Line 333) after launching registration in a background coroutine (Line 315). The Rust caller via JNI may treattrueas "registration succeeded" when it only means "FCM token was present and registration was initiated." If the network call fails, the rollback at Lines 322-329 happens asynchronously and the Rust side won't know.Consider either making the return value represent only "token available" (and documenting that clearly), or using a callback to report the final registration result back to Rust.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt` around lines 300 - 334, enableFcmNotifications currently returns true immediately after launching an async coroutine, which misleads JNI/Rust callers; change the method so its return reflects the final registration outcome: either (A) make enableFcmNotifications only indicate token presence by returning a distinct value/documentation (e.g., rename/return "tokenAvailable") and do not imply success, or (B) report the async result back to Rust by invoking a callback/JNI method once notepushClient.registerDevice(pubkeyHex, fcmToken) completes—on success log and return success via the callback, on failure perform the existing rollback using setNotificationMode(KEY_MODE/ PREFS_NAME) and invoke the failure callback; ensure the chosen approach updates callers and any JNI signatures accordingly so Rust receives the true registration result.crates/notedeck_chrome/src/notifications.rs-637-651 (1)
637-651:⚠️ Potential issue | 🟡 MinorDedup eviction clears the entire set — may cause duplicate notifications.
When
processed_eventsexceeds 10,000 entries, the wholeHashSetis cleared (Line 647). Immediately after clearing, every previously-seen event ID becomes "new" again. If a relay replays recent events, they'll trigger duplicate notifications. Consider evicting just the oldest entries (e.g., using aVecDequealongside theHashSet, as done in the desktopWorkerStateintypes.rs).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/src/notifications.rs` around lines 637 - 651, The dedup eviction currently clears the entire HashSet in record_event_if_new, causing recent IDs to be treated as new after a flush; instead add an ordered ring (e.g., VecDeque<String>) to WorkerState (like desktop WorkerState in types.rs) alongside processed_events: HashSet<String>, and in record_event_if_new when inserting a new event push_back the ID into the VecDeque and, if the deque.len() > 10_000, pop_front an ID and remove that ID from processed_events; update imports to use std::collections::VecDeque and ensure all places constructing WorkerState initialize the deque.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt-131-151 (1)
131-151:⚠️ Potential issue | 🟡 MinorLoop always exits on first iteration — use
if (moveToFirst())instead.The
while (it.moveToNext())at Line 139 unconditionally returns on the first row (Line 147), making the loop misleading. Replace withif (it.moveToFirst())to express the intent of reading exactly one row.Proposed fix
cursor?.use { - while (it.moveToNext()) { + if (it.moveToFirst()) { 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 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt` around lines 131 - 151, In getUriInfo, the cursor loop uses while(it.moveToNext()) but immediately returns on the first row, which is misleading; change the cursor iteration to use if (it.moveToFirst()) to explicitly read a single row, keep the same logic that fills the info array (OpenableColumns.DISPLAY_NAME, OpenableColumns.SIZE, "mime_type") and return that info, and preserve the cursor.use block and the final return of emptyArray() if no row is present.
...ome/android/app/src/main/java/com/damus/notedeck/service/NotedeckFirebaseMessagingService.kt
Outdated
Show resolved
Hide resolved
...ome/android/app/src/main/java/com/damus/notedeck/service/NotedeckFirebaseMessagingService.kt
Show resolved
Hide resolved
...ome/android/app/src/main/java/com/damus/notedeck/service/NotedeckFirebaseMessagingService.kt
Show resolved
Hide resolved
| // Cache for profile images - thread-safe for concurrent access | ||
| private val profileImageCache = ConcurrentHashMap<String, Bitmap>() | ||
|
|
There was a problem hiding this comment.
Unbounded ConcurrentHashMap<String, Bitmap> will leak memory.
profileImageCache has no eviction policy. Each 128×128 ARGB bitmap is ~64 KB. Over time, with many unique notification authors, this will grow without bound. Consider using an LruCache (from android.util) with a size limit, or at minimum call clearCache() periodically.
🛠️ Suggested fix using LruCache
- private val profileImageCache = ConcurrentHashMap<String, Bitmap>()
+ // Bounded LRU cache: ~100 entries × ~64KB ≈ 6.4MB max
+ private val profileImageCache = object : android.util.LruCache<String, Bitmap>(100) {
+ override fun sizeOf(key: String, value: Bitmap): Int = 1
+ }Note: LruCache is not thread-safe by default, so access would need synchronization, or you could use a synchronized wrapper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotificationHelper.kt`
around lines 27 - 29, The unbounded ConcurrentHashMap profileImageCache in
NotificationHelper.kt will leak memory; replace it with an
android.util.LruCache<String, Bitmap> sized to a reasonable byte or entry limit
(override sizeOf to count bitmap bytes, e.g., width*height*4) and use that as
the cache backend, or keep a ConcurrentHashMap but add an eviction
strategy/periodic clearCache() method; ensure thread-safety by synchronizing all
accesses to the LruCache (or wrapping in a synchronized block or using
Collections.synchronizedMap if you choose a map), and add a clearCache() helper
on NotificationHelper to explicitly evict entries when appropriate.
| fn handle_pending_deep_link(damus: &mut Damus, app_ctx: &mut AppContext<'_>) { | ||
| // Check columns first — don't consume the deep link if we can't route yet | ||
| let columns = get_active_columns_mut(app_ctx.i18n, app_ctx.accounts, &mut damus.decks_cache); | ||
| if columns.columns().is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| let Some(deep_link) = notedeck::platform::take_pending_deep_link() else { | ||
| return; | ||
| }; | ||
|
|
||
| info!( | ||
| "Processing deep link: event_id={}, kind={}", | ||
| &deep_link.event_id[..8.min(deep_link.event_id.len())], | ||
| deep_link.event_kind | ||
| ); | ||
|
|
||
| // Convert hex event_id to NoteId | ||
| let note_id = match enostr::NoteId::from_hex(&deep_link.event_id) { | ||
| Ok(id) => id, | ||
| Err(e) => { | ||
| error!("Invalid event_id in deep link: {}", e); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| // Create a thread selection with proper root note resolution | ||
| // This validates the note exists and finds the actual root if this is a reply | ||
| let txn = match Transaction::new(app_ctx.ndb) { | ||
| Ok(t) => t, | ||
| Err(e) => { | ||
| error!("Failed to create transaction for deep link: {}", e); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| let thread_selection = match timeline::ThreadSelection::from_note_id( | ||
| app_ctx.ndb, | ||
| app_ctx.note_cache, | ||
| &txn, | ||
| note_id, | ||
| ) { | ||
| Ok(selection) => selection, | ||
| Err(e) => { | ||
| // Note doesn't exist in DB yet - use the note_id as root (will be fetched) | ||
| warn!("Deep link note not in DB, using as root: {:?}", e); | ||
| timeline::ThreadSelection::from_root_id(notedeck::RootNoteIdBuf::new_unsafe( | ||
| *note_id.bytes(), | ||
| )) | ||
| } | ||
| }; | ||
|
|
||
| let route = Route::Thread(thread_selection); | ||
|
|
||
| // Navigate to the thread in the currently selected column | ||
| columns.get_selected_router().route_to(route); | ||
|
|
||
| info!("Deep link navigation complete"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the definition of take_pending_deep_link
rg -n 'fn take_pending_deep_link' --type=rust -A15Repository: damus-io/notedeck
Length of output: 2882
🏁 Script executed:
#!/bin/bash
# Also search for any platform module files
find . -path '*/notedeck*/platform*' -type f -name '*.rs' | head -20Repository: damus-io/notedeck
Length of output: 235
🏁 Script executed:
#!/bin/bash
# Look for static or thread_local declarations in platform code
rg -n 'static|thread_local' --type=rust | grep -i platform | head -20Repository: damus-io/notedeck
Length of output: 547
take_pending_deep_link() uses forbidden global state.
The coding guidelines explicitly state: "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." However, the Android implementation of notedeck::platform::take_pending_deep_link() (crates/notedeck/src/platform/android.rs:840-846) accesses a global static NOTIFICATION_BRIDGE to retrieve the pending deep link. Thread the deep-link state through AppContext or Notedeck instead of relying on platform globals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/notedeck_columns/src/app.rs` around lines 239 - 297, The function
handle_pending_deep_link currently calls
notedeck::platform::take_pending_deep_link(), which relies on forbidden global
state; instead, add and use a pending deep-link field on an existing context
struct (e.g., AppContext or Damus) and consume it there. Change
handle_pending_deep_link to read and take the Option<PendingDeepLink> from that
field (e.g., app_ctx.pending_deep_link.take() or damus.pending_deep_link.take())
rather than calling notedeck::platform::take_pending_deep_link(), and update the
platform integration to set that field via a callback/initializer when a deep
link arrives (removing use of NOTIFICATION_BRIDGE/global). Ensure symbols to
update: handle_pending_deep_link, AppContext (or Damus) pending_deep_link field,
and any platform-to-app initialization that currently writes
NOTIFICATION_BRIDGE.
| pub fn stop(&self) { | ||
| let handle = { | ||
| if let Ok(mut guard) = self.worker.write() { | ||
| guard.take() | ||
| } else { | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| if let Some(mut handle) = handle { | ||
| handle.running.store(false, Ordering::SeqCst); | ||
| info!("Signaled notification worker to stop"); | ||
|
|
||
| if let Some(thread) = handle.thread.take() { | ||
| let start = std::time::Instant::now(); | ||
| while !thread.is_finished() && start.elapsed().as_secs() < 2 { | ||
| std::thread::sleep(std::time::Duration::from_millis(50)); | ||
| } | ||
|
|
||
| if thread.is_finished() { | ||
| let _ = thread.join(); | ||
| info!("Notification worker stopped"); | ||
| } else { | ||
| info!("Notification worker didn't stop in time, detaching"); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
stop() busy-waits up to 2 seconds, risking render-loop stalls.
The Drop impl at Line 231 calls stop(), which polls is_finished() in a 50 ms sleep loop for up to 2 s. If the NotificationService is dropped while the render loop is running (e.g., during account switch or shutdown), this will block the UI thread.
Consider signalling the worker and joining in a background task, or simply dropping the JoinHandle (detaching) since the AtomicBool already tells the worker to exit.
Proposed simplification
pub fn stop(&self) {
let handle = {
if let Ok(mut guard) = self.worker.write() {
guard.take()
} else {
return;
}
};
if let Some(mut handle) = handle {
handle.running.store(false, Ordering::SeqCst);
info!("Signaled notification worker to stop");
-
- if let Some(thread) = handle.thread.take() {
- let start = std::time::Instant::now();
- while !thread.is_finished() && start.elapsed().as_secs() < 2 {
- std::thread::sleep(std::time::Duration::from_millis(50));
- }
-
- if thread.is_finished() {
- let _ = thread.join();
- info!("Notification worker stopped");
- } else {
- info!("Notification worker didn't stop in time, detaching");
- }
- }
+ // Drop the sender so the worker's recv unblocks immediately
+ drop(handle.sender);
+ // Detach the thread — the worker will exit on its own
+ // once it observes `running == false` or channel disconnect.
+ info!("Notification worker signaled to stop (detached)");
}
}As per coding guidelines: "Never block the render loop".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/notedeck/src/notifications/mod.rs` around lines 185 - 212, The stop()
method currently busy-waits up to 2s polling handle.thread.is_finished(), which
can block the render loop when Drop calls stop(); instead, signal the worker by
setting handle.running.store(false, Ordering::SeqCst) and avoid polling on the
caller thread: either immediately detach by dropping the JoinHandle (just take()
and drop it) or offload the join to a background thread (spawn a short-lived
thread that calls join on handle.thread.take()) so the render/drop path never
sleeps; update the stop() implementation to remove the 50ms loop and use
handle.running and handle.thread (the worker field and the thread JoinHandle)
accordingly.
4e3abf8 to
c382b61
Compare
alltheseas
force-pushed
the
notedeck-notifications
branch
from
c382b61 to
b647072
Compare
Add Firebase SDK (BOM, messaging), Kotlin coroutines, OkHttp, and AndroidX dependencies for both FCM push and native WebSocket notification services. Raise minSdk to 26 for foreground service type. Co-Authored-By: Claude Opus 4.6 <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>
Add JNI bridge functions for Firebase Cloud Messaging support: - nativeOnFcmTokenRefreshed: stores FCM token from Kotlin - nativeProcessNostrEvent: parses Nostr events for notification display - nativeSignNip98Auth: signs NIP-98 HTTP auth events for notepush API Includes helper functions for event parsing, NIP-98 signing, and thread-safe FCM token and signing keypair storage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
HTTP client for the notepush server API with NIP-98 authentication. Supports device registration/unregistration and preference management for FCM-based push notifications. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
NotedeckFirebaseMessagingService handles FCM token refresh and incoming push notifications. Delegates event parsing to native Rust code via JNI and displays notifications with deep-link support for tapping to open the relevant event. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add notification permission handling, FCM/native mode toggling, and deep-link support to MainActivity. Updates accounts.rs to propagate signing keypair changes to the JNI bridge when the active account changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion API Add NotificationMode (Fcm/Native/Disabled) enum with index conversion for UI radio buttons. Define cross-platform notification API with cfg-gated function signatures for enable/disable, permission handling, service status, and deep linking. Desktop stubs return sensible defaults; Android implementation follows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Android-specific implementations of the notification platform API: enable/disable FCM and native notifications via JNI calls to Kotlin, notification permission request/result flow, service status checks, and notification mode persistence via SharedPreferences. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Core data types for the notification system: NotificationAccount, CachedProfile, ExtractedEvent, and WorkerState. Includes BOLT11 invoice parsing for zap amount extraction from lightning invoices. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Platform abstraction trait for notification delivery with DesktopBackend implementation using notify-rust. Includes App Nap prevention on macOS and NoopBackend for testing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Robust Nostr event extraction from relay messages with proper JSON parsing. Extracts p-tags for event attribution, delegates to BOLT11 parser for zap amount extraction, and produces structured events. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Profile cache management with LRU-style pruning, profile request deduplication, and npub mention resolution. Decodes bech32 npub references to hex pubkeys and resolves @mentions from cached data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Core notification_worker event loop managing relay connections, event subscriptions, deduplication, and notification delivery via the NotificationBackend trait. Adds NotificationService<B> generic wrapper for lifecycle management and start_desktop_notifications convenience function. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
NotificationImageCache provides async profile picture downloading with SHA256-based disk caching, bounded LRU eviction, and platform-appropriate cache directory detection. macOS UNNotificationAttachment requires local file URLs with correct extensions, so this cache preserves the original image format rather than re-encoding to WebP like the main MediaCache. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
macOS notification backend using UNUserNotificationCenter with objc2 bindings. Features: foreground notification presentation delegate, bundle ID validation, and profile picture attachments via the NotificationImageCache. Adds objc2 workspace dependencies and macOS bundle metadata. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
NotificationManager wraps the notification service with platform- specific initialization (macOS delegate, App Nap prevention). Platform glue layer provides cross-platform enable/disable/status API for notification mode management via desktop_notifications module. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wire NotificationManager into AppContext for lifecycle management. Add notification settings panel with enable/disable controls and status display to the settings UI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rust implementation for Android native (non-FCM) notifications. Manages direct WebSocket relay connections, event subscriptions, deduplication, and profile enrichment via JNI callbacks to Kotlin. Includes BOLT11 zap amount parsing, bech32 npub mention resolution, and comprehensive unit tests. Communicates with NotificationsService via JNI callbacks (onNostrEvent, onRelayStatusChanged). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
NotificationHelper builds rich Android notifications with profile images, group summaries, and deep-link intents. NotificationActionReceiver handles notification button actions (mark read, mute user/thread). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
NotificationsService manages a foreground service with WebSocket relay connections via JNI to Rust. Handles notification channels, wake lock, event deduplication, and broadcasts for other Nostr apps (non-DM only). BootReceiver auto-restarts the service after device boot or app update. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Group all notification-related JNI statics (FCM token, signing keypair, permission flags, deep link) into a single NotificationJniBridge struct to minimize global surface area. Also: structured FcmNotificationData for event parsing, enableFcm returns boolean for error feedback, and get_jvm docstring. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implement deep link handling so tapping a notification navigates to the relevant note thread. Uses thread-safe RwLock storage for pending deep links from JNI callbacks, with guard against consuming links before columns are initialized. Also integrates notification signing keypair updates with account switching to support multi-account setups. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add unified notification settings with FCM/Native/Disabled radio group, permission status badges, and shadcn-inspired UI components. Persist notification preference across app restarts via SettingsHandler. Includes: notification_badge, notification_radio_option, and notification_privacy_info UI components showing FCM vs Native tradeoffs. Auto-enables notifications on startup if previously enabled. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
alltheseas
force-pushed
the
notedeck-notifications
branch
from
b647072 to
ff51aa3
Compare
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (23)
crates/notedeck_chrome/android/app/build.gradle (1)
63-63: The explicitkotlin-stdlib:1.9.22is redundant but not problematic.The root project's Kotlin plugin (declared in
android/build.gradle) is version1.9.22, which matches the explicit stdlib declaration here. The Kotlin Gradle plugin already includeskotlin-stdlibautomatically at the same version, so this explicit dependency is unnecessary but causes no harm since the versions are aligned. Remove this line to let the Kotlin plugin manage the stdlib version automatically, or keep it if you prefer explicit dependency tracking.🔧 Suggested fix
- // Kotlin - implementation "org.jetbrains.kotlin:kotlin-stdlib:1.9.22"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/app/build.gradle` at line 63, Remove the redundant explicit Kotlin stdlib declaration by deleting the implementation "org.jetbrains.kotlin:kotlin-stdlib:1.9.22" line so the root project's Kotlin Gradle plugin (version 1.9.22) will provide the stdlib automatically; locate the module build.gradle that contains the implementation "org.jetbrains.kotlin:kotlin-stdlib:1.9.22" entry and remove that single dependency line.crates/notedeck/src/notifications/types.rs (2)
104-120: Inlineuse tracing::infoinside function body.The
tracing::infoimport on line 108 is inside the function body. While valid Rust, the module already depends ontracingelsewhere in the crate, and the convention in this codebase is module-level imports.♻️ Move import to module level
use enostr::Pubkey; use std::collections::{HashMap, HashSet, VecDeque}; use std::sync::mpsc; +use tracing::info;Then remove line 108.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/notifications/types.rs` around lines 104 - 120, Move the inline import out of the function: add a module-level `use tracing::info;` near the top of the `crates/notedeck/src/notifications/types.rs` file (so the module imports tracing consistently), then remove the `use tracing::info;` statement from inside the `pub fn new(...) -> Self` function (the constructor that logs "WorkerState created with {} accounts"). This keeps imports consistent with the crate convention and leaves the existing `info!(...)` call intact.
85-97: Unboundedprocessed_eventsHashSet — eviction must be enforced by the consumer.
processed_eventsandprocessed_events_orderprovide the data structures for bounded LRU deduplication, but no capacity limit is encoded here. If the worker fails to evict entries, this will grow without bound. Consider adding amax_capacityfield or a helper method likerecord_event(&mut self, id: String)that encapsulates the insert + eviction logic, keeping the invariant in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/notifications/types.rs` around lines 85 - 97, WorkerState currently exposes processed_events and processed_events_order without any capacity enforcement, risking unbounded growth; add a max_capacity: usize field to WorkerState and implement a helper method like record_event(&mut self, id: String) (or ensure existing constructors set max_capacity) that encapsulates insertion and eviction: push the id to processed_events_order and insert into processed_events, then while processed_events_order.len() > self.max_capacity pop_front() and remove that id from processed_events to maintain the invariant; update any place that mutates processed_events/processed_events_order to use record_event to centralize logic (ensure initialization of max_capacity in WorkerState creation).crates/notedeck/src/platform/mod.rs (2)
211-227: Two identicalDeepLinkInfostructs — one inplatform::modand one inplatform::android.
take_pending_deep_linkmaps field-by-field between two structurally identical types. Consider re-exporting the Android type or defining it only inplatform::modand importing it in the Android module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/platform/mod.rs` around lines 211 - 227, There are two identical DeepLinkInfo definitions causing redundant mapping in take_pending_deep_link; instead, consolidate by exporting a single type: either remove the DeepLinkInfo in platform::android and have android::take_pending_deep_link return platform::DeepLinkInfo (use crate::platform::DeepLinkInfo inside the android module), or keep the android::DeepLinkInfo and re-export it from platform::mod (pub use android::DeepLinkInfo;) and change take_pending_deep_link to return android::DeepLinkInfo directly (call android::take_pending_deep_link without field-by-field mapping). Update the signature/returns of android::take_pending_deep_link and any uses of DeepLinkInfo accordingly.
175-205: API signature asymmetry between Android and desktop forare_notifications_enabledandis_notification_service_running.The Android variants take no parameters while the desktop variants require
&Option<NotificationManager>. This forces all callers to use#[cfg]blocks, leaking platform details through the abstraction boundary. Consider either:
- Having the
NotificationManagerstored in a place accessible without passing it through every call, or- Providing a unified signature (e.g., accepting
Option<&NotificationManager>on both platforms, with Android ignoring it).This is a design consideration for a follow-up — noting it here since the current shape adds
#[cfg]burden to every call site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/platform/mod.rs` around lines 175 - 205, The public APIs are asymmetric: are_notifications_enabled and is_notification_service_running have Android variants with no params and desktop variants taking &Option<NotificationManager>, forcing callers to #[cfg] their call sites; unify the signatures by choosing one approach—either (A) make both platform functions accept Option<&NotificationManager> (Android implementations ignore the param) so callers can call a single function without cfgs, or (B) move/store NotificationManager in a global/accessible location (e.g., module-level singleton or injected during init) so both Android and desktop APIs remain parameterless; update the functions are_notifications_enabled and is_notification_service_running and their android::/desktop_notifications:: implementations accordingly and update callers to use the unified signature.crates/notedeck_columns/src/ui/settings.rs (2)
104-116: Usepool.urls()instead of directly accessingpool.relays.
RelayPoolexposes aurls()method (seecrates/enostr/src/relay/pool.rs:172-177) that returns aBTreeSet<String>. Accessing.relaysdirectly couples to the internal representation.♻️ Suggested fix
- let relay_urls: Vec<String> = - pool.relays.iter().map(|r| r.url().to_string()).collect(); + let relay_urls: Vec<String> = pool.urls().into_iter().collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_columns/src/ui/settings.rs` around lines 104 - 116, Replace the direct access to pool.relays with the public API pool.urls() to avoid coupling to RelayPool internals: in the Self::SetNotificationMode match arm, build relay_urls from pool.urls() (e.g., pool.urls().iter().cloned().collect::<Vec<String>>() or similar) and pass that Vec<String> into notedeck::platform::set_notification_mode along with pubkey_hex; keep the existing error handling and settings.set_notifications_enabled(!mode.is_disabled()) logic intact.
208-289: Hardcoded colors bypass theme system.
notification_radio_optionuses hardcoded color values (e.g.,Color32::from_rgb(30, 41, 59)for slate-800,Color32::from_rgb(139, 92, 246)for violet-500) rather than pulling fromui.visuals()orctx.style(). While it switches between dark/light variants, custom user themes won't affect these elements.Consider using
ui.visuals().widgets.active.bg_fill,ui.visuals().selection.bg_fill, etc., for better theme integration. This can be deferred since it's a settings panel, not a high-traffic view.As per coding guidelines: "Respect
ctx.style()for colors/fonts and support narrow layouts for responsive UI design."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_columns/src/ui/settings.rs` around lines 208 - 289, notification_radio_option currently uses hardcoded Color32 values (see bg_color and border_color) which bypass user themes; update notification_radio_option to derive colors from ui.visuals()/ui.style() instead of literal Color32::from_rgb(...)—for example use ui.visuals().widgets.active.bg_fill or ui.visuals().selection.bg_fill when is_selected, use ui.visuals().widgets.noninteractive.bg_fill or ui.visuals().widgets.inactive.bg_fill for default, and use ui.visuals().selection.stroke or ui.visuals().widgets.inactive.bg_stroke (or ui.visuals().text_color() / ui.visuals().gray_out(...) as already used) for border_color and disabled states; keep the same control geometry (radio_size, radio_center, response.rect) and only replace the color expressions for bg_color and border_color so custom themes propagate.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotificationActionReceiver.kt (1)
50-63: TODO stubs: notification actions won't take effect until JNI integration is implemented.All three action handlers (
markAsRead,muteUser,muteThread) are currently no-ops. Additionally, the receiver doesn't dismiss the notification after handling an action — users will likely expect the notification to disappear after tapping "Mark as Read" or "Mute".Would you like me to open an issue to track the JNI integration and notification dismissal for these action handlers?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotificationActionReceiver.kt` around lines 50 - 63, markAsRead, muteUser, and muteThread in NotificationActionReceiver are currently TODO stubs and do not perform any action or dismiss the notification; replace the no-op bodies to (1) invoke the corresponding JNI/FFI hooks (e.g., call Rust-exposed functions like mark_event_read(eventId), add_pubkey_to_mute(pubkey), add_thread_to_mute(threadId)) via your existing JNI bridge, and (2) dismiss the notification after handling by obtaining NotificationManager/NotificationManagerCompat from the provided Context and calling cancel(notificationId) or cancel(tag, id) (you may need to extract the notification id/tag from the Intent extras). If JNI functions are not yet implemented, at minimum dismiss the notification and log a clear TODO that the real JNI call must replace the placeholder; ensure the methods (markAsRead, muteUser, muteThread) live inside NotificationActionReceiver and accept the Intent extras used to locate eventId/pubkey/threadId and notification identifiers.crates/notedeck/src/account/accounts.rs (1)
234-259: Duplicate relay URLs possible when local and advertised overlap.
localandadvertisedrelay lists may contain the same URL. Depending on how the caller uses this (e.g., passing to Android native notifications which opens WebSocket connections per URL), duplicates could cause redundant connections.Consider deduplicating:
♻️ Suggested deduplication
- let mut urls: Vec<String> = relay_data - .local - .iter() - .chain(relay_data.advertised.iter()) - .map(|spec| spec.url.clone()) - .collect(); + let mut seen = std::collections::HashSet::new(); + let mut urls: Vec<String> = relay_data + .local + .iter() + .chain(relay_data.advertised.iter()) + .filter_map(|spec| { + if seen.insert(spec.url.clone()) { + Some(spec.url.clone()) + } else { + None + } + }) + .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/account/accounts.rs` around lines 234 - 259, get_selected_account_relay_urls may return duplicate URLs because relay_data.local and relay_data.advertised can overlap; update the function to deduplicate the collected URLs (including when falling back to self.relay_defaults.bootstrap_relays) before returning. Implement deduplication by tracking seen URLs (e.g., a HashSet) and only pushing new URLs to the result to preserve first-seen order; refer to get_selected_account_relay_urls, relay_data.local, relay_data.advertised, and relay_defaults.bootstrap_relays when locating the code to change.crates/notedeck/src/platform/desktop_notifications.rs (1)
39-50:are_notifications_enabledandis_notification_service_runningare identical.Both functions delegate to
manager.as_ref().map(|m| m.is_running()).unwrap_or(false). If they're meant to represent different concepts (e.g., user preference vs. runtime state), consider adding a comment explaining the intended semantic difference, or consolidate into one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/platform/desktop_notifications.rs` around lines 39 - 50, The two functions are duplicates: are_notifications_enabled and is_notification_service_running both return manager.as_ref().map(|m| m.is_running()).unwrap_or(false); either consolidate them into one exported helper (e.g., keep are_notifications_enabled and remove is_notification_service_running or have one call the other) or implement the intended distinct semantics (e.g., make are_notifications_enabled check a user preference method like m.is_enabled() if such exists, otherwise add a clarifying doc comment above both functions explaining why both exist and how they differ). Update callers to use the single function or the newly implemented semantic method to avoid redundancy.crates/notedeck/src/notifications/extraction.rs (1)
138-166:extract_p_tagsvalidates length but not hex content, unlikeid/pubkeyvalidation above.Lines 161–162 check
pubkey.len() == 64but don't verify the string is valid hex, whereasextract_eventvalidates both length andhex::decodeforidandpubkey(lines 76–107). If downstream code assumes p_tags are valid hex, a non-hex 64-char string would slip through.Optional: add hex validation for consistency
// Validate pubkey is 64-char hex - if pubkey.len() == 64 { + if pubkey.len() == 64 && hex::decode(pubkey).is_ok() { p_tags.push(pubkey.to_string()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/notifications/extraction.rs` around lines 138 - 166, The extract_p_tags function currently only checks that the second element string has length 64 but doesn't verify it's valid hex; update extract_p_tags to mirror extract_event's validation by attempting hex::decode on the pubkey (from tag_arr[1]) and only push the pubkey string to p_tags if hex::decode succeeds, keeping the existing length check as a quick filter; reference the extract_p_tags function and follow the same hex validation pattern used in extract_event for id/pubkey to ensure consistency.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/BootReceiver.kt (1)
42-53: Extract"active_pubkey"to a companion constant for consistency.
PREF_NOTIFICATIONS_ENABLEDis properly defined as a constant (line 28), but"active_pubkey"on line 45 is an inline string. Consider extracting it to keep the preference keys maintainable in one place.Proposed fix
companion object { private const val TAG = "BootReceiver" private const val PREF_NOTIFICATIONS_ENABLED = "notifications_enabled" + private const val PREF_ACTIVE_PUBKEY = "active_pubkey" }- val hasPubkey = !prefs.getString("active_pubkey", null).isNullOrEmpty() + val hasPubkey = !prefs.getString(PREF_ACTIVE_PUBKEY, null).isNullOrEmpty()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/BootReceiver.kt` around lines 42 - 53, The inline preference key "active_pubkey" used in startServiceIfEnabled should be extracted to a companion constant for consistency with PREF_NOTIFICATIONS_ENABLED; add a companion object in BootReceiver (or the existing one) with a val PREF_ACTIVE_PUBKEY = "active_pubkey" and replace the literal in getString(...) with PREF_ACTIVE_PUBKEY so all preference keys are centralized and consistent.crates/notedeck/src/notifications/mod.rs (1)
89-94:std::sync::RwLockused for worker handle.The coding guidelines advise against Mutexes and prefer
tokio::sync::RwLockwhen cross-thread sharing is truly necessary. Thestd::sync::RwLockhere is accessed infrequently (start/stop are rare events), and thesend()path only takes a read lock, so contention isn't a practical concern. However, if this is ever called from an async context, the blocking nature ofstd::sync::RwLockcould be problematic. Worth noting for future reference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/notifications/mod.rs` around lines 89 - 94, The NotificationService currently uses std::sync::RwLock for the worker field; replace it with tokio::sync::RwLock to avoid blocking in async contexts: change the worker field type in NotificationService<B> from RwLock<Option<WorkerHandle>> to tokio::sync::RwLock<Option<WorkerHandle>>, update the imports accordingly, and audit all usages (start/stop and the send() read-lock path) to use .read().await / .write().await as needed (or explicitly block_in_place only where truly synchronous behavior is required) while keeping WorkerHandle and the generic B unchanged.crates/notedeck_chrome/src/notifications.rs (1)
226-251: 1-second idle sleep means notification delivery latency up to 1 second.When no events are pending, the worker sleeps for a full second (Line 249) before polling again. This means after a burst of events, the next event can take up to 1 second to be processed. For a notification system, this may be acceptable, but consider using a shorter sleep (e.g., 100-200ms) or using a blocking
recvwith timeout on a channel if the relay pool supports it, to improve responsiveness while still being battery-friendly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/src/notifications.rs` around lines 226 - 251, The loop in notifications.rs uses a 1s idle sleep (thread::sleep(Duration::from_secs(1))) which can add up to 1s notification latency; change this to a shorter sleep (e.g., 100–200ms) or, if the relay pool supports blocking waits, replace the try_recv() + sleep pattern with a blocking recv with timeout on state.pool (use a timeout ~100–200ms) so keepalive_ping(&state.pool) and connected_count updates still run each iteration; update references to state.pool.try_recv(), state.pool.keepalive_ping(), shared.running and shared.generation accordingly.crates/notedeck/src/notifications/macos.rs (2)
396-444: Temp file not cleaned up after successful attachment creation.
create_attachmentcopies the cached image to a temp file, then passes it toUNNotificationAttachment. The comment on Line 394 says macOS MOVES the file — if that's the case, the temp file is gone after success (good). But if macOS copies instead of moves, temp files accumulate in the temp directory. The error path correctly cleans up (Line 440). Consider verifying macOS's actual behavior, or adding cleanup on success as a safety measure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/notifications/macos.rs` around lines 396 - 444, The create_attachment function may leave temp files behind on success because it copies the image to temp_path and only removes it on the Err branch; ensure temp_path is removed after successful creation as a safety measure (in case macOS copies rather than moves). After UNNotificationAttachment::attachmentWithIdentifier_URL_options_error returns Ok(attachment), schedule or perform cleanup of temp_path (e.g., call std::fs::remove_file(&temp_path) or equivalent) while still returning the created attachment; keep the existing error-branch cleanup via remove_file for failures. This change should reference create_attachment, temp_path, UNNotificationAttachment::attachmentWithIdentifier_URL_options_error, and std::fs::remove_file.
285-288: Use named constants fromobjc2_user_notificationsinstead of magic numbers at lines 285-288.
UNAuthorizationOptions(1 | 2 | 4)should use the available associated constantsUNAuthorizationOptions::Alert,UNAuthorizationOptions::Sound, andUNAuthorizationOptions::Badge, combined with|operator:let options = UNAuthorizationOptions::Alert | UNAuthorizationOptions::Sound | UNAuthorizationOptions::Badge;This improves clarity and prevents errors from incorrect bit values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/notifications/macos.rs` around lines 285 - 288, Replace the magic numeric bitmask UNAuthorizationOptions(1 | 2 | 4) with the named associated constants on UNAuthorizationOptions: combine UNAuthorizationOptions::Alert, UNAuthorizationOptions::Sound and UNAuthorizationOptions::Badge using the bitwise OR operator; update the declaration where UNAuthorizationOptions is constructed so it reads the combined named constants instead of raw numbers to improve clarity and correctness.crates/notedeck/src/notifications/worker.rs (2)
131-162: Duplicate deduplication logic — extract to shared utility.
record_event_if_newandMAX_PROCESSED_EVENTSare duplicated verbatim between this file andcrates/notedeck_chrome/src/notifications.rs(Lines 577-602). If the eviction logic or capacity changes in one place, the other will silently diverge.Consider extracting this into a shared
EventDeduplicatorstruct intypes.rsor a shared utility module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/notifications/worker.rs` around lines 131 - 162, The deduplication logic duplicated by MAX_PROCESSED_EVENTS and record_event_if_new should be extracted into a shared EventDeduplicator struct (e.g., in types.rs or a new util module) that owns a HashSet and VecDeque and enforces capacity with LRU eviction; implement a method like EventDeduplicator::record_if_new(&mut self, event_id: &str) -> bool that encapsulates the current behavior and constant, replace the local MAX_PROCESSED_EVENTS and record_event_if_new usages in WorkerState (and the duplicate in crates/notedeck_chrome/src/notifications.rs) with a field of type EventDeduplicator or calls to the shared utility, and update initialization and any tests accordingly so both crates use the single implementation.
103-116:fetch_and_cache_blockingblocks the worker thread.This is the notification display worker thread (not the render loop), so blocking here is acceptable per the coding guidelines. However, a slow or unresponsive image server could delay subsequent notifications. Consider adding a timeout to the fetch, or falling back to no-image after a brief deadline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/notifications/worker.rs` around lines 103 - 116, The current macOS-only block calls cache.fetch_and_cache_blocking(url) which can stall the notification worker; replace the direct blocking call with a timed fetch and fallback to None on deadline so slow image servers don't delay later notifications. Specifically, where picture_path is built (the macOS cfg block referencing data.author_picture_url and state.image_cache), change the fetch to a bounded operation: start the cache fetch in a short-lived thread or use a cache API with a timeout, wait up to the configured deadline, and if it returns an image path use that value, otherwise log a warning and return None; leave behavior unchanged when no url or no cache. Ensure the new logic still produces Option<String> for picture_path and references the same symbols (picture_path, data.author_picture_url, state.image_cache, fetch_and_cache_blocking) so callers need no change.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotepushClient.kt (2)
61-91: No retry logic for device registration.If
registerDevicefails due to a transient network error, the device won't receive FCM push notifications until the next registration attempt. Consider adding a simple retry with exponential backoff for this critical operation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotepushClient.kt` around lines 61 - 91, registerDevice currently makes a single network call and returns false on failure; implement a simple retry with exponential backoff (e.g., 3 attempts) around the HTTP PUT performed by client.newCall(request).execute() so transient network errors are retried: on exceptions or non-successful responses from the call triggered after building the url via buildUrl and signing with nativeSignNip98Auth, delay using increasing intervals (e.g., baseDelay * 2^attempt) with kotlinx.coroutines.delay on Dispatchers.IO and reattempt creating/sending the request; return true immediately on a successful response and false after exhausting retries, and only treat non-transient errors (e.g., authHeader null) as immediate failure.
37-52: FCM token in URL path — verify server compatibility with long tokens.FCM device tokens can be up to ~256 characters. While
HttpUrl.Builder.addPathSegmentcorrectly URL-encodes them, the resulting URL could be quite long. Verify the notepush server and any intermediary proxies/CDNs don't truncate URLs. This is standard practice for push APIs, but worth confirming.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotepushClient.kt` around lines 37 - 52, The buildUrl function in NotepushClient (buildUrl, baseUrl, addPathSegment usage) currently places the potentially long fcmToken in the URL path which can create very long URLs; change this to either place fcmToken in a query parameter (use addQueryParameter("fcmToken", fcmToken)) or send it in the request body instead of addPathSegment, and add a validation guard in buildUrl to check token length (e.g. fail/throw or log and truncate if fcmToken.length exceeds a safe limit like 200–2000 characters) and ensure the final URL length stays below common proxy limits (e.g. 2048 chars) before returning. Ensure any callers of buildUrl are updated to expect the token in the query/body as needed and keep backend query parameter handling (addQueryParameter("backend", ...)) unchanged.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt (3)
220-226: EmptyonResume/onPauseoverrides add noise without behaviour.Both overrides only call
super. Unless GameActivity requires the overrides to be present for reflection or future additions are planned, they can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt` around lines 220 - 226, The onResume and onPause overrides in MainActivity (methods onResume() and onPause()) are empty and only call super, so remove these two method overrides to reduce noise; if they must remain for a framework/reflection requirement, replace them with a brief comment explaining why they are intentionally empty (e.g., "kept for reflection/framework hooks") to avoid confusion.
74-81:startActivityForResultis deprecated — preferActivityResultLauncher.
startActivityForResult/onActivityResulthave been deprecated since API 30 in favour of theActivityResultContractsAPI, which is lifecycle-safe and avoids leaking Activity results.♻️ Sketch of the modern approach
// In class body private val pickFileLauncher = registerForActivityResult( ActivityResultContracts.OpenMultipleDocuments() ) { uris -> uris.forEach { processSelectedFile(it) } } // Replace openFilePicker body fun openFilePicker() { pickFileLauncher.launch(arrayOf("*/*")) }Then
onActivityResultforREQUEST_CODE_PICK_FILEcan be removed entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt` around lines 74 - 81, Replace the deprecated startActivityForResult usage in openFilePicker with an ActivityResultLauncher: add a private val pickFileLauncher = registerForActivityResult(ActivityResultContracts.OpenMultipleDocuments()) { uris -> uris?.forEach { processSelectedFile(it) } } in the Activity class, change openFilePicker to call pickFileLauncher.launch(arrayOf("*/*")) instead of startActivityForResult, and remove the old onActivityResult handling for REQUEST_CODE_PICK_FILE; ensure processSelectedFile is invoked for each returned Uri.
245-258: Remove the unusedsetupFullscreen()andfocus()methods — they have no call sites.Both private methods are fully implemented but never invoked anywhere in the codebase, including via JNI reflection or external callers. Since they serve no purpose, they should be removed to reduce code clutter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt` around lines 245 - 258, The two private, unused methods setupFullscreen() and focus(View) in MainActivity.kt are dead code; remove both method definitions (setupFullscreen and focus) from the MainActivity class to eliminate unused private functions and related imports if they become unused (e.g., WindowCompat, WindowInsetsControllerCompat, WindowInsetsCompat, and View references) and run a build/IDE cleanup to ensure no remaining references or unused-import warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/notedeck_chrome/android/app/build.gradle`:
- Line 18: The minSdk increase from 19 to 26 is a breaking change; instead of
raising minSdk, revert minSdk back to 19 and make the code call
startForegroundService() only on API 26+ (or use
ContextCompat.startForegroundService) with appropriate runtime checks (e.g., if
(Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
context.startForegroundService(...) } else { context.startService(...) }) and
ensure any service uses startForeground with a notification to satisfy newer
APIs; update any related manifest/service code that assumed API26 behavior so
the project remains compatible with older Android versions while still using
foreground services when available.
- Around line 59-60: Update the Firebase BOM and dependency declarations: change
the platform version from "com.google.firebase:firebase-bom:32.7.0" to
"com.google.firebase:firebase-bom:34.9.0" and replace the end-of-maintenance KTX
artifact "com.google.firebase:firebase-messaging-ktx" with the main artifact
"com.google.firebase:firebase-messaging" so the build uses the current BOM and
the supported messaging module.
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt`:
- Around line 192-196: Move the call to setupInsets() so it runs after the
Activity superclass initialization: in onCreate(), call
super.onCreate(savedInstanceState) first, then invoke setupInsets(), then
proceed to handleDeepLinkIntent(intent); this ensures
getContent()/window.decorView.findViewById(android.R.id.content) inside
setupInsets() observes the fully-initialized view hierarchy (important for
GameActivity which may replace/configure android.R.id.content during its
onCreate()).
- Around line 143-147: The code is querying a non-standard "mime_type" column
from the cursor (in MainActivity.kt where you populate info[] using colIdx and
OpenableColumns), which is not guaranteed and yields nulls; replace that logic
by calling contentResolver.getType(uri) to obtain the MIME type and assign the
result to info[2] (falling back to null or a default if getType returns null),
and remove the cursor colIdx lookup for "mime_type". Ensure you use the same uri
variable and contentResolver instance and handle nulls safely.
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotificationsService.kt`:
- Around line 170-217: The createNotificationChannels function uses hardcoded
English labels/descriptions for channels and other UI text (e.g., "Background
Service", "Shows when Notedeck is listening for notifications", "Notifications",
"Direct Messages", "Zaps", etc.); move these literals into strings.xml and
replace the hardcoded values with context.getString(R.string.<name>) (or
getString(...) inside the service) when constructing NotificationChannel
instances and other UI text (references: createNotificationChannels,
CHANNEL_SERVICE, CHANNEL_NOTIFICATIONS, CHANNEL_DMS, CHANNEL_ZAPS, and the
deleteNotificationChannel id); add corresponding entries in strings.xml with
clear keys (e.g., notification_channel_service_title,
notification_channel_service_desc, notification_channel_notifications_title,
notification_channel_dms_title, notification_channel_zaps_title,
listening_for_nostr_events, stop_button) and use R.string.<key> in the Kotlin
code.
- Around line 104-106: connectedRelays is mutated from the JNI callback
onRelayStatusChanged and read on the main thread in
startForegroundWithNotification/createServiceNotification, so make its updates
visible across threads by marking the field `@Volatile` or converting it to an
AtomicInteger; update the declaration of connectedRelays accordingly and change
any increments/decrements or reads in onRelayStatusChanged and
createServiceNotification/startForegroundWithNotification to use the chosen
concurrency primitive (volatile read/writes or
AtomicInteger.get()/incrementAndGet()/decrementAndGet()) so there is no data
race.
- Around line 281-289: The info log in NotificationsService (inside the
serviceScope.launch block that calls nativeStartSubscriptions) prints the full
pubkeyHex and relayUrlsJson; change the Log.i call to redact the pubkey by
truncating it (use pubkeyHex.take(8) as elsewhere) and stop printing the full
relayUrlsJson (either omit it or log a hashed/summary representation), leaving
nativeStartSubscriptions(pubkeyHex, relayUrlsJson) unchanged; update the Log.i
message to include only the truncated key and a non-sensitive relay indicator.
- Around line 292-303: The wake lock is only acquired in acquireWakeLock()
(called from onCreate()) with a 10-minute timeout and never renewed, which lets
it expire and drops persistent connections; update onStartCommand() to
re-acquire or renew the wake lock each time the service is started: call
acquireWakeLock() (or check wakeLock?.isHeld and re-acquire with the same
timeout) from onStartCommand(), ensuring you avoid double-acquiring (release if
held or guard with isHeld) so the PowerManager.PARTIAL_WAKE_LOCK named
"notedeck:NotificationsService" is periodically renewed while the foreground
service runs.
In `@crates/notedeck_chrome/Cargo.toml`:
- Around line 87-88: Replace the hardcoded version and stale copyright in
Cargo.toml: change version = "0.7.1" to inherit the workspace version (e.g.,
version = { workspace = true }) so it cannot drift, and update the copyright
value from "Copyright © 2025 Damus, Nostr Inc." to the current year "Copyright ©
2026 Damus, Nostr Inc." (make these edits for the existing version and copyright
entries).
- Around line 69-73: Update the jni dependency entry in Cargo.toml from "0.21"
to the newer "0.21.1": locate the jni = "0.21" line in the Cargo.toml entries
(near other workspace deps like enostr, ewebsock, bech32, hex) and change the
version string to jni = "0.21.1", then run cargo update or cargo build to ensure
the lockfile is updated and the project compiles with the new jni version.
In `@crates/notedeck_chrome/src/notifications.rs`:
- Around line 78-86: Replace the process-wide static SHARED_STATE/OnceLock
pattern with explicit state passing: remove the SHARED_STATE static and change
get_shared_state() into a function that accepts a &SharedState (or an opaque
pointer/jlong for Android JNI entry points) and return/clones as needed; create
an initialization path that allocates the SharedState (e.g., Box::into_raw or
Arc created by the caller) and pass that handle into JNI-exported functions or
store it in a per-JNI-object field so callers of functions that currently call
get_shared_state (referencing get_shared_state, SHARED_STATE, SharedState,
OnceLock) receive the state via parameter instead of a global; alternatively, if
JNI requires, document and justify using an opaque jlong handle passed from Java
to native and update all call sites to accept that handle.
- Around line 488-491: The debug logging slices strings by bytes (e.g., the
variables content, resolved_content, and picture_url) using expressions like
&content[..content.len().min(200)], which can panic on multi-byte UTF-8
boundaries; create a small helper truncate_str(s: &str, max_bytes: usize) ->
&str that returns a valid &str truncated at the nearest char boundary (iterate
chars or use s.char_indices to find the largest byte index <= max_bytes) and
replace the unsafe byte-slices in the parsing/logging calls (where content,
resolved_content, and picture_url are logged) with truncate_str(...) to ensure
UTF-8-safe truncation.
- Around line 548-574: In request_profile_if_needed, avoid slicing pubkey with
[&pubkey[..8]] and [&pubkey[..16]] which can panic for short strings; instead
use a safe prefix helper or pubkey.get(..8).unwrap_or(pubkey) and
pubkey.get(..16).unwrap_or(pubkey) when building log messages and the sub_id
(referencing SUB_PROFILES and the sub_id construction) and ensure
state.requested_profiles.insert(pubkey.to_string()) and the
profile_filter/subscribe logic remain unchanged.
In `@crates/notedeck/src/notifications/bolt11.rs`:
- Around line 47-53: Update the misleading inline comment in the test function
test_extract_zap_amount_real_invoice: the invoice string in variable bolt11
represents 330 nano-BTC which equals 33 sats, so change the comment that
currently reads "(330 sats)" to the correct value (e.g., "(33 sats)" or "330
nano-BTC = 33 sats") so it matches the assertion and the explanatory comment
already present; you can locate this in the test using the symbols
test_extract_zap_amount_real_invoice, bolt11, make_zap_event, and
extract_zap_amount.
In `@crates/notedeck/src/notifications/macos.rs`:
- Around line 190-231: The manual BlockLiteral/((*block).invoke)(...) usage in
will_present_notification should be replaced with block2 to safely call the
Objective-C completion block: cast completion_handler to a
block2::Block<(usize,), ()> (or create a reference from the raw pointer) and
invoke it via .call((options,)); remove the hand-rolled BlockLiteral struct and
unsafe invoke. Also replace the raw numeric presentation and authorization
bitmasks (4 | 2 | 16 | 8 and 1 | 2 | 4) with the named flags from
UNNotificationPresentationOptions (e.g. ::Alert | ::Sound | ::Banner | ::List)
and objc2_user_notifications::UNAuthorizationOptions (e.g. ::Badge | ::Sound |
::Alert) so will_present_notification and the authorization call use the library
constants for clarity and safety.
In `@crates/notedeck/src/notifications/mod.rs`:
- Around line 136-142: The start() function currently blocks by calling
thread.join() on the old worker handle; instead, set the stop signal
(old_handle.running.store(false, Ordering::SeqCst)) and do NOT call join() so
the caller won't block the render loop—take the thread handle
(old_handle.thread.take()) and simply drop it (detaching) so the worker can exit
asynchronously; ensure the worker loop checks running and exits cleanly, and
remove or replace the synchronous join call in the block that handles
guard.take().
In `@crates/notedeck/src/platform/android.rs`:
- Around line 291-331: Add an explicit match arm for kind 1059 in
parse_nostr_event_for_notification so gift-wrapped DMs are treated like
encrypted DMs (do not reveal decrypted content): in the match over kind inside
parse_nostr_event_for_notification add a 1059 => ("New direct
message".to_string(), "Contents are encrypted".to_string()) branch (mirroring
kind 4 behavior), ensuring FcmNotificationData still receives title, body,
event_id, event_kind and author_pubkey; reference NOTIFICATION_KINDS in types.rs
to keep parity with declared notification kinds.
- Around line 804-813: The code handling author_pubkey should explicitly check
the incoming JString for null (like the jbody null check) instead of relying on
env.get_string() to error; update the author_pubkey block to first test
author_pubkey.is_null() and return None if null, otherwise call
env.get_string(&author_pubkey) and convert to Option<String> (treating empty
string as None) — use the same pattern as the existing jbody handling and keep
references to author_pubkey and env.get_string in the updated logic.
In `@crates/notedeck/src/platform/desktop_notifications.rs`:
- Around line 52-56: The function is_notification_permission_granted() currently
always returns true which masks macOS permission denial; change it so macOS
returns the actual authorization state by reading a shared/observable value set
by macos.rs::initialize_on_main_thread() (or by directly querying the macOS
notification API), and return Err on query failure; specifically, update
is_notification_permission_granted() to consult the permission flag populated by
initialize_on_main_thread() (or call the platform check implemented for macOS)
instead of hardcoding Ok(true), and ensure the macOS initializer stores the
granted/denied state in a thread-safe/global place (e.g., static/atomic or a
Result-returning getter) that is used by is_notification_permission_granted().
In `@crates/notedeck/src/platform/mod.rs`:
- Around line 99-106: The desktop stub for set_notification_mode currently
no-ops; replace it so it forwards the requested NotificationMode to the desktop
notification implementation: inside pub fn set_notification_mode(...), match on
NotificationMode and call
desktop_notifications::enable_notifications(_pubkey_hex, _relay_urls) when
enabling and desktop_notifications::disable_notifications() when disabling,
propagating any error as Result<(), Box<dyn std::error::Error>> (use ? to return
errors). Keep the #[cfg(not(target_os = "android"))] guard and ensure signatures
(set_notification_mode, NotificationMode,
desktop_notifications::enable_notifications/disable_notifications) match
existing APIs.
---
Duplicate comments:
In `@crates/notedeck_columns/src/app.rs`:
- Around line 233-297: The code calls the forbidden global
notedeck::platform::take_pending_deep_link() inside handle_pending_deep_link;
instead add a pending deep-link slot to AppContext (e.g., pub pending_deep_link:
Option<DeepLink> or similar), have platform/Android code populate that field
during startup/notification handling, and change handle_pending_deep_link to
consume app_ctx.pending_deep_link.take() (or borrow) rather than calling
take_pending_deep_link(); update any callers/tests that relied on the global to
use the AppContext field so deep-link state is threaded through AppContext
rather than global NOTIFICATION_BRIDGE.
In `@crates/notedeck/src/app.rs`:
- Around line 486-500: process_relay_message is doing synchronous DB work on the
main render thread inside the RelayEvent::Message handling (next to
process_message_core), so move that call off the render loop by spawning a
background task; capture/clones of app_ctx.notification_manager, app_ctx.ndb,
app_ctx.accounts and app_ctx.i18n and then call mgr.process_relay_message from a
JobPool worker or tokio::task::spawn_blocking/spawn so the DB lookups run off
the main thread (keep the RelayMessage::Event match and only offload the inner
call). Ensure you clone or Arc the needed references before spawning and handle
errors/logging inside the spawned task.
In `@crates/notedeck/src/notifications/mod.rs`:
- Around line 184-212: stop() currently busy-waits up to 2s by polling
thread.is_finished() which can block the render loop on Drop; change stop() (the
worker.write() / handle.running.store(false, Ordering::SeqCst) path) to signal
the worker and immediately detach the thread handle instead of looping—set
handle.running to false, log the signal, take handle.thread and drop it (no
sleep-loop or join), and keep the channel/atomic to let the worker exit
gracefully so Drop won't block.
---
Nitpick comments:
In `@crates/notedeck_chrome/android/app/build.gradle`:
- Line 63: Remove the redundant explicit Kotlin stdlib declaration by deleting
the implementation "org.jetbrains.kotlin:kotlin-stdlib:1.9.22" line so the root
project's Kotlin Gradle plugin (version 1.9.22) will provide the stdlib
automatically; locate the module build.gradle that contains the implementation
"org.jetbrains.kotlin:kotlin-stdlib:1.9.22" entry and remove that single
dependency line.
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt`:
- Around line 220-226: The onResume and onPause overrides in MainActivity
(methods onResume() and onPause()) are empty and only call super, so remove
these two method overrides to reduce noise; if they must remain for a
framework/reflection requirement, replace them with a brief comment explaining
why they are intentionally empty (e.g., "kept for reflection/framework hooks")
to avoid confusion.
- Around line 74-81: Replace the deprecated startActivityForResult usage in
openFilePicker with an ActivityResultLauncher: add a private val
pickFileLauncher =
registerForActivityResult(ActivityResultContracts.OpenMultipleDocuments()) {
uris -> uris?.forEach { processSelectedFile(it) } } in the Activity class,
change openFilePicker to call pickFileLauncher.launch(arrayOf("*/*")) instead of
startActivityForResult, and remove the old onActivityResult handling for
REQUEST_CODE_PICK_FILE; ensure processSelectedFile is invoked for each returned
Uri.
- Around line 245-258: The two private, unused methods setupFullscreen() and
focus(View) in MainActivity.kt are dead code; remove both method definitions
(setupFullscreen and focus) from the MainActivity class to eliminate unused
private functions and related imports if they become unused (e.g., WindowCompat,
WindowInsetsControllerCompat, WindowInsetsCompat, and View references) and run a
build/IDE cleanup to ensure no remaining references or unused-import warnings.
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/BootReceiver.kt`:
- Around line 42-53: The inline preference key "active_pubkey" used in
startServiceIfEnabled should be extracted to a companion constant for
consistency with PREF_NOTIFICATIONS_ENABLED; add a companion object in
BootReceiver (or the existing one) with a val PREF_ACTIVE_PUBKEY =
"active_pubkey" and replace the literal in getString(...) with
PREF_ACTIVE_PUBKEY so all preference keys are centralized and consistent.
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotepushClient.kt`:
- Around line 61-91: registerDevice currently makes a single network call and
returns false on failure; implement a simple retry with exponential backoff
(e.g., 3 attempts) around the HTTP PUT performed by
client.newCall(request).execute() so transient network errors are retried: on
exceptions or non-successful responses from the call triggered after building
the url via buildUrl and signing with nativeSignNip98Auth, delay using
increasing intervals (e.g., baseDelay * 2^attempt) with kotlinx.coroutines.delay
on Dispatchers.IO and reattempt creating/sending the request; return true
immediately on a successful response and false after exhausting retries, and
only treat non-transient errors (e.g., authHeader null) as immediate failure.
- Around line 37-52: The buildUrl function in NotepushClient (buildUrl, baseUrl,
addPathSegment usage) currently places the potentially long fcmToken in the URL
path which can create very long URLs; change this to either place fcmToken in a
query parameter (use addQueryParameter("fcmToken", fcmToken)) or send it in the
request body instead of addPathSegment, and add a validation guard in buildUrl
to check token length (e.g. fail/throw or log and truncate if fcmToken.length
exceeds a safe limit like 200–2000 characters) and ensure the final URL length
stays below common proxy limits (e.g. 2048 chars) before returning. Ensure any
callers of buildUrl are updated to expect the token in the query/body as needed
and keep backend query parameter handling (addQueryParameter("backend", ...))
unchanged.
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotificationActionReceiver.kt`:
- Around line 50-63: markAsRead, muteUser, and muteThread in
NotificationActionReceiver are currently TODO stubs and do not perform any
action or dismiss the notification; replace the no-op bodies to (1) invoke the
corresponding JNI/FFI hooks (e.g., call Rust-exposed functions like
mark_event_read(eventId), add_pubkey_to_mute(pubkey),
add_thread_to_mute(threadId)) via your existing JNI bridge, and (2) dismiss the
notification after handling by obtaining
NotificationManager/NotificationManagerCompat from the provided Context and
calling cancel(notificationId) or cancel(tag, id) (you may need to extract the
notification id/tag from the Intent extras). If JNI functions are not yet
implemented, at minimum dismiss the notification and log a clear TODO that the
real JNI call must replace the placeholder; ensure the methods (markAsRead,
muteUser, muteThread) live inside NotificationActionReceiver and accept the
Intent extras used to locate eventId/pubkey/threadId and notification
identifiers.
In `@crates/notedeck_chrome/src/notifications.rs`:
- Around line 226-251: The loop in notifications.rs uses a 1s idle sleep
(thread::sleep(Duration::from_secs(1))) which can add up to 1s notification
latency; change this to a shorter sleep (e.g., 100–200ms) or, if the relay pool
supports blocking waits, replace the try_recv() + sleep pattern with a blocking
recv with timeout on state.pool (use a timeout ~100–200ms) so
keepalive_ping(&state.pool) and connected_count updates still run each
iteration; update references to state.pool.try_recv(),
state.pool.keepalive_ping(), shared.running and shared.generation accordingly.
In `@crates/notedeck_columns/src/ui/settings.rs`:
- Around line 104-116: Replace the direct access to pool.relays with the public
API pool.urls() to avoid coupling to RelayPool internals: in the
Self::SetNotificationMode match arm, build relay_urls from pool.urls() (e.g.,
pool.urls().iter().cloned().collect::<Vec<String>>() or similar) and pass that
Vec<String> into notedeck::platform::set_notification_mode along with
pubkey_hex; keep the existing error handling and
settings.set_notifications_enabled(!mode.is_disabled()) logic intact.
- Around line 208-289: notification_radio_option currently uses hardcoded
Color32 values (see bg_color and border_color) which bypass user themes; update
notification_radio_option to derive colors from ui.visuals()/ui.style() instead
of literal Color32::from_rgb(...)—for example use
ui.visuals().widgets.active.bg_fill or ui.visuals().selection.bg_fill when
is_selected, use ui.visuals().widgets.noninteractive.bg_fill or
ui.visuals().widgets.inactive.bg_fill for default, and use
ui.visuals().selection.stroke or ui.visuals().widgets.inactive.bg_stroke (or
ui.visuals().text_color() / ui.visuals().gray_out(...) as already used) for
border_color and disabled states; keep the same control geometry (radio_size,
radio_center, response.rect) and only replace the color expressions for bg_color
and border_color so custom themes propagate.
In `@crates/notedeck/src/account/accounts.rs`:
- Around line 234-259: get_selected_account_relay_urls may return duplicate URLs
because relay_data.local and relay_data.advertised can overlap; update the
function to deduplicate the collected URLs (including when falling back to
self.relay_defaults.bootstrap_relays) before returning. Implement deduplication
by tracking seen URLs (e.g., a HashSet) and only pushing new URLs to the result
to preserve first-seen order; refer to get_selected_account_relay_urls,
relay_data.local, relay_data.advertised, and relay_defaults.bootstrap_relays
when locating the code to change.
In `@crates/notedeck/src/notifications/extraction.rs`:
- Around line 138-166: The extract_p_tags function currently only checks that
the second element string has length 64 but doesn't verify it's valid hex;
update extract_p_tags to mirror extract_event's validation by attempting
hex::decode on the pubkey (from tag_arr[1]) and only push the pubkey string to
p_tags if hex::decode succeeds, keeping the existing length check as a quick
filter; reference the extract_p_tags function and follow the same hex validation
pattern used in extract_event for id/pubkey to ensure consistency.
In `@crates/notedeck/src/notifications/macos.rs`:
- Around line 396-444: The create_attachment function may leave temp files
behind on success because it copies the image to temp_path and only removes it
on the Err branch; ensure temp_path is removed after successful creation as a
safety measure (in case macOS copies rather than moves). After
UNNotificationAttachment::attachmentWithIdentifier_URL_options_error returns
Ok(attachment), schedule or perform cleanup of temp_path (e.g., call
std::fs::remove_file(&temp_path) or equivalent) while still returning the
created attachment; keep the existing error-branch cleanup via remove_file for
failures. This change should reference create_attachment, temp_path,
UNNotificationAttachment::attachmentWithIdentifier_URL_options_error, and
std::fs::remove_file.
- Around line 285-288: Replace the magic numeric bitmask
UNAuthorizationOptions(1 | 2 | 4) with the named associated constants on
UNAuthorizationOptions: combine UNAuthorizationOptions::Alert,
UNAuthorizationOptions::Sound and UNAuthorizationOptions::Badge using the
bitwise OR operator; update the declaration where UNAuthorizationOptions is
constructed so it reads the combined named constants instead of raw numbers to
improve clarity and correctness.
In `@crates/notedeck/src/notifications/mod.rs`:
- Around line 89-94: The NotificationService currently uses std::sync::RwLock
for the worker field; replace it with tokio::sync::RwLock to avoid blocking in
async contexts: change the worker field type in NotificationService<B> from
RwLock<Option<WorkerHandle>> to tokio::sync::RwLock<Option<WorkerHandle>>,
update the imports accordingly, and audit all usages (start/stop and the send()
read-lock path) to use .read().await / .write().await as needed (or explicitly
block_in_place only where truly synchronous behavior is required) while keeping
WorkerHandle and the generic B unchanged.
In `@crates/notedeck/src/notifications/types.rs`:
- Around line 104-120: Move the inline import out of the function: add a
module-level `use tracing::info;` near the top of the
`crates/notedeck/src/notifications/types.rs` file (so the module imports tracing
consistently), then remove the `use tracing::info;` statement from inside the
`pub fn new(...) -> Self` function (the constructor that logs "WorkerState
created with {} accounts"). This keeps imports consistent with the crate
convention and leaves the existing `info!(...)` call intact.
- Around line 85-97: WorkerState currently exposes processed_events and
processed_events_order without any capacity enforcement, risking unbounded
growth; add a max_capacity: usize field to WorkerState and implement a helper
method like record_event(&mut self, id: String) (or ensure existing constructors
set max_capacity) that encapsulates insertion and eviction: push the id to
processed_events_order and insert into processed_events, then while
processed_events_order.len() > self.max_capacity pop_front() and remove that id
from processed_events to maintain the invariant; update any place that mutates
processed_events/processed_events_order to use record_event to centralize logic
(ensure initialization of max_capacity in WorkerState creation).
In `@crates/notedeck/src/notifications/worker.rs`:
- Around line 131-162: The deduplication logic duplicated by
MAX_PROCESSED_EVENTS and record_event_if_new should be extracted into a shared
EventDeduplicator struct (e.g., in types.rs or a new util module) that owns a
HashSet and VecDeque and enforces capacity with LRU eviction; implement a method
like EventDeduplicator::record_if_new(&mut self, event_id: &str) -> bool that
encapsulates the current behavior and constant, replace the local
MAX_PROCESSED_EVENTS and record_event_if_new usages in WorkerState (and the
duplicate in crates/notedeck_chrome/src/notifications.rs) with a field of type
EventDeduplicator or calls to the shared utility, and update initialization and
any tests accordingly so both crates use the single implementation.
- Around line 103-116: The current macOS-only block calls
cache.fetch_and_cache_blocking(url) which can stall the notification worker;
replace the direct blocking call with a timed fetch and fallback to None on
deadline so slow image servers don't delay later notifications. Specifically,
where picture_path is built (the macOS cfg block referencing
data.author_picture_url and state.image_cache), change the fetch to a bounded
operation: start the cache fetch in a short-lived thread or use a cache API with
a timeout, wait up to the configured deadline, and if it returns an image path
use that value, otherwise log a warning and return None; leave behavior
unchanged when no url or no cache. Ensure the new logic still produces
Option<String> for picture_path and references the same symbols (picture_path,
data.author_picture_url, state.image_cache, fetch_and_cache_blocking) so callers
need no change.
In `@crates/notedeck/src/platform/desktop_notifications.rs`:
- Around line 39-50: The two functions are duplicates: are_notifications_enabled
and is_notification_service_running both return manager.as_ref().map(|m|
m.is_running()).unwrap_or(false); either consolidate them into one exported
helper (e.g., keep are_notifications_enabled and remove
is_notification_service_running or have one call the other) or implement the
intended distinct semantics (e.g., make are_notifications_enabled check a user
preference method like m.is_enabled() if such exists, otherwise add a clarifying
doc comment above both functions explaining why both exist and how they differ).
Update callers to use the single function or the newly implemented semantic
method to avoid redundancy.
In `@crates/notedeck/src/platform/mod.rs`:
- Around line 211-227: There are two identical DeepLinkInfo definitions causing
redundant mapping in take_pending_deep_link; instead, consolidate by exporting a
single type: either remove the DeepLinkInfo in platform::android and have
android::take_pending_deep_link return platform::DeepLinkInfo (use
crate::platform::DeepLinkInfo inside the android module), or keep the
android::DeepLinkInfo and re-export it from platform::mod (pub use
android::DeepLinkInfo;) and change take_pending_deep_link to return
android::DeepLinkInfo directly (call android::take_pending_deep_link without
field-by-field mapping). Update the signature/returns of
android::take_pending_deep_link and any uses of DeepLinkInfo accordingly.
- Around line 175-205: The public APIs are asymmetric: are_notifications_enabled
and is_notification_service_running have Android variants with no params and
desktop variants taking &Option<NotificationManager>, forcing callers to #[cfg]
their call sites; unify the signatures by choosing one approach—either (A) make
both platform functions accept Option<&NotificationManager> (Android
implementations ignore the param) so callers can call a single function without
cfgs, or (B) move/store NotificationManager in a global/accessible location
(e.g., module-level singleton or injected during init) so both Android and
desktop APIs remain parameterless; update the functions
are_notifications_enabled and is_notification_service_running and their
android::/desktop_notifications:: implementations accordingly and update callers
to use the unified signature.
|
|
||
| defaultConfig { | ||
| minSdk 19 | ||
| minSdk 26 // Raised for foreground service type |
There was a problem hiding this comment.
minSdk bump from 19 → 26 is a hard breaking change for existing users.
startForegroundService() (API 26) does justify this floor, but raising from 19 to 26 silently drops support for Android 5.0–7.1 (Nougat and below). Confirm the trade-off has been evaluated against the project's current install base before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/notedeck_chrome/android/app/build.gradle` at line 18, The minSdk
increase from 19 to 26 is a breaking change; instead of raising minSdk, revert
minSdk back to 19 and make the code call startForegroundService() only on API
26+ (or use ContextCompat.startForegroundService) with appropriate runtime
checks (e.g., if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
context.startForegroundService(...) } else { context.startService(...) }) and
ensure any service uses startForeground with a notification to satisfy newer
APIs; update any related manifest/service code that assumed API26 behavior so
the project remains compatible with older Android versions while still using
foreground services when available.
| implementation platform("com.google.firebase:firebase-bom:32.7.0") | ||
| implementation "com.google.firebase:firebase-messaging-ktx" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Firebase BOM version 34.9.0 release date 2026
💡 Result:
Firebase Android BoM 34.9.0 was released on February 5, 2026. [1][2]
🌐 Web query:
Firebase KTX modules deprecated July 2025 end of maintenance
💡 Result:
Firebase’s standalone Android KTX (*-ktx) modules were deprecated after their APIs were moved into the corresponding “main” Firebase Android artifacts (starting around October 2023, usable via Firebase BoM v32.5.0+). Then, in July 2025, Firebase stopped releasing new versions of the KTX modules and removed all *-ktx libraries from the Firebase Android BoM starting with firebase-bom:34.0.0. Previously released *-ktx versions will still work, but they’re end-of-maintenance (no new bug fixes, features, or updates). [1][2]
What to do: remove *-ktx dependencies and depend on the main modules (for example, use com.google.firebase:firebase-firestore instead of com.google.firebase:firebase-firestore-ktx) and update imports to the new packages per Firebase’s migration guide. [1]
Sources: [1] [2]
🌐 Web query:
firebase-bom latest version February 2026
💡 Result:
As of February 18, 2026, the latest Firebase Android BoM version (com.google.firebase:firebase-bom) is 34.9.0 (released Feb 5, 2026). [1]
Gradle:
dependencies {
implementation platform("com.google.firebase:firebase-bom:34.9.0")
}Sources: [1]
🌐 Web query:
firebase-messaging-ktx end of life deprecated 2025
💡 Result:
com.google.firebase:firebase-messaging-ktx (and the other Firebase Android *-ktx artifacts) was deprecated when Firebase moved the KTX APIs into the main modules (Messaging note shown in the Android release notes), and then effectively reached end-of-maintenance in July 2025 when Firebase stopped releasing new versions of KTX modules and removed all KTX artifacts from the Firebase Android BoM starting with BoM 34.0.0. Existing previously released KTX versions will still work, but they won’t receive fixes or new features. [1], [2]
What to do now (Messaging):
- Replace
implementation("com.google.firebase:firebase-messaging-ktx")withimplementation("com.google.firebase:firebase-messaging") - Continue using the KTX-style APIs, now provided from the main module under
com.google.firebase.messaging. [1], [2]
Update Firebase BOM to the latest version (34.9.0) and migrate from the end-of-maintenance KTX module.
Firebase BOM v32.7.0 is significantly out of date. The current version is 34.9.0 (released February 5, 2026), which includes two years of security patches and bug fixes. Additionally, firebase-messaging-ktx reached end-of-maintenance status in July 2025 when Firebase stopped releasing new versions of KTX modules and removed them from the BoM starting with v34.0.0. While previously released KTX versions remain functional, they will not receive further updates or fixes.
Upgrade to the latest BOM and switch to the main firebase-messaging module, which now provides the same KTX-style APIs:
🔧 Suggested dependency update
- implementation platform("com.google.firebase:firebase-bom:32.7.0")
- implementation "com.google.firebase:firebase-messaging-ktx"
+ implementation platform("com.google.firebase:firebase-bom:34.9.0")
+ implementation "com.google.firebase:firebase-messaging"📝 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.
| implementation platform("com.google.firebase:firebase-bom:32.7.0") | |
| implementation "com.google.firebase:firebase-messaging-ktx" | |
| implementation platform("com.google.firebase:firebase-bom:34.9.0") | |
| implementation "com.google.firebase:firebase-messaging" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/notedeck_chrome/android/app/build.gradle` around lines 59 - 60, Update
the Firebase BOM and dependency declarations: change the platform version from
"com.google.firebase:firebase-bom:32.7.0" to
"com.google.firebase:firebase-bom:34.9.0" and replace the end-of-maintenance KTX
artifact "com.google.firebase:firebase-messaging-ktx" with the main artifact
"com.google.firebase:firebase-messaging" so the build uses the current BOM and
the supported messaging module.
| 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 |
There was a problem hiding this comment.
"mime_type" is not a standard OpenableColumns column — use contentResolver.getType(uri) instead.
OpenableColumns only guarantees DISPLAY_NAME and SIZE. Querying "mime_type" from the cursor will return getColumnIndex == -1 on most content providers, silently leaving info[2] as null. The correct API for MIME type is contentResolver.getType(uri).
🐛 Proposed fix
- colIdx = it.getColumnIndex("mime_type")
- info[2] = if (colIdx >= 0) it.getString(colIdx) else null
+ // MIME type is not a standard OpenableColumns column; use the dedicated API
+ info[2] = contentResolver.getType(uri)📝 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.
| 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 | |
| colIdx = it.getColumnIndex(OpenableColumns.SIZE) | |
| info[1] = if (colIdx >= 0) it.getLong(colIdx) else 0L | |
| // MIME type is not a standard OpenableColumns column; use the dedicated API | |
| info[2] = contentResolver.getType(uri) | |
| return info |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt`
around lines 143 - 147, The code is querying a non-standard "mime_type" column
from the cursor (in MainActivity.kt where you populate info[] using colIdx and
OpenableColumns), which is not guaranteed and yields nulls; replace that logic
by calling contentResolver.getType(uri) to obtain the MIME type and assign the
result to info[2] (falling back to null or a default if getType returns null),
and remove the cursor colIdx lookup for "mime_type". Ensure you use the same uri
variable and contentResolver instance and handle nulls safely.
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| setupInsets() | ||
| super.onCreate(savedInstanceState) | ||
| handleDeepLinkIntent(intent) | ||
| } |
There was a problem hiding this comment.
setupInsets() is called before super.onCreate() — unconventional and fragile with GameActivity.
setupInsets() calls getContent() → window.decorView.findViewById(android.R.id.content) before GameActivity.onCreate() has had a chance to initialize its native surface and content hierarchy. While window is technically available from Activity.attach(), GameActivity may populate android.R.id.content or configure the window during its super.onCreate(). Setting the WindowInsetsCompat listener on a potentially pre-initialized view can silently no-op if the view is later replaced. Move setupInsets() after the super call.
📐 Proposed fix
override fun onCreate(savedInstanceState: Bundle?) {
- setupInsets()
super.onCreate(savedInstanceState)
+ setupInsets()
handleDeepLinkIntent(intent)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.kt`
around lines 192 - 196, Move the call to setupInsets() so it runs after the
Activity superclass initialization: in onCreate(), call
super.onCreate(savedInstanceState) first, then invoke setupInsets(), then
proceed to handleDeepLinkIntent(intent); this ensures
getContent()/window.decorView.findViewById(android.R.id.content) inside
setupInsets() observes the fully-initialized view hierarchy (important for
GameActivity which may replace/configure android.R.id.content during its
onCreate()).
| // Connected relay count for status | ||
| private var connectedRelays = 0 | ||
|
|
There was a problem hiding this comment.
Data race on connectedRelays — needs @Volatile or AtomicInteger.
connectedRelays is a plain var Int. It is written from a JNI callback thread (onRelayStatusChanged) and read from the main thread inside startForegroundWithNotification → createServiceNotification. Without a memory visibility guarantee, the main thread may observe a stale value or (on JVM) the compiler may cache the field in a register. Mark it @Volatile (or switch to AtomicInteger).
🔒 Proposed fix
- private var connectedRelays = 0
+ `@Volatile` private var connectedRelays = 0📝 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.
| // Connected relay count for status | |
| private var connectedRelays = 0 | |
| // Connected relay count for status | |
| `@Volatile` private var connectedRelays = 0 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/service/NotificationsService.kt`
around lines 104 - 106, connectedRelays is mutated from the JNI callback
onRelayStatusChanged and read on the main thread in
startForegroundWithNotification/createServiceNotification, so make its updates
visible across threads by marking the field `@Volatile` or converting it to an
AtomicInteger; update the declaration of connectedRelays accordingly and change
any increments/decrements or reads in onRelayStatusChanged and
createServiceNotification/startForegroundWithNotification to use the chosen
concurrency primitive (volatile read/writes or
AtomicInteger.get()/incrementAndGet()/decrementAndGet()) so there is no data
race.
| // Stop any previous worker | ||
| if let Some(mut old_handle) = guard.take() { | ||
| old_handle.running.store(false, Ordering::SeqCst); | ||
| if let Some(thread) = old_handle.thread.take() { | ||
| let _ = thread.join(); | ||
| } | ||
| } |
There was a problem hiding this comment.
start() blocks on joining old worker thread — can stall render loop.
Line 140 calls thread.join() synchronously, which blocks until the old worker thread exits. The worker uses a 1-second recv_timeout, so this can block the caller for up to ~1 second (plus any in-flight fetch_and_cache_blocking on macOS). If start() is called from the render loop path, this violates the "never block the render loop" guideline.
Proposed fix — signal and detach
// Stop any previous worker
if let Some(mut old_handle) = guard.take() {
old_handle.running.store(false, Ordering::SeqCst);
- if let Some(thread) = old_handle.thread.take() {
- let _ = thread.join();
- }
+ // Drop sender to unblock recv immediately, then detach thread
+ drop(old_handle.sender);
+ // Thread will exit on its own; don't block here
}As per coding guidelines, "Never block the render loop."
📝 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.
| // Stop any previous worker | |
| if let Some(mut old_handle) = guard.take() { | |
| old_handle.running.store(false, Ordering::SeqCst); | |
| if let Some(thread) = old_handle.thread.take() { | |
| let _ = thread.join(); | |
| } | |
| } | |
| // Stop any previous worker | |
| if let Some(mut old_handle) = guard.take() { | |
| old_handle.running.store(false, Ordering::SeqCst); | |
| // Drop sender to unblock recv immediately, then detach thread | |
| drop(old_handle.sender); | |
| // Thread will exit on its own; don't block here | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/notedeck/src/notifications/mod.rs` around lines 136 - 142, The start()
function currently blocks by calling thread.join() on the old worker handle;
instead, set the stop signal (old_handle.running.store(false, Ordering::SeqCst))
and do NOT call join() so the caller won't block the render loop—take the thread
handle (old_handle.thread.take()) and simply drop it (detaching) so the worker
can exit asynchronously; ensure the worker loop checks running and exits
cleanly, and remove or replace the synchronous join call in the block that
handles guard.take().
| fn parse_nostr_event_for_notification(event_json: &str) -> Option<FcmNotificationData> { | ||
| let value: serde_json::Value = serde_json::from_str(event_json).ok()?; | ||
|
|
||
| let kind = value.get("kind")?.as_u64()?; | ||
| let content = value.get("content")?.as_str().unwrap_or(""); | ||
| let event_id = value.get("id")?.as_str().map(|s| s.to_string()); | ||
| let author_pubkey = value | ||
| .get("pubkey") | ||
| .and_then(|v| v.as_str()) | ||
| .map(|s| s.to_string()); | ||
|
|
||
| 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)), | ||
| }; | ||
|
|
||
| Some(FcmNotificationData { | ||
| title, | ||
| body, | ||
| event_id, | ||
| event_kind: kind as i32, | ||
| author_pubkey, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Kind 1059 (gift-wrapped DM) falls through to generic "New activity" in FCM parsing.
parse_nostr_event_for_notification handles kinds 1, 4, 6, 7, and 9735 explicitly but doesn't have a case for kind 1059 (gift-wrapped DM), which is listed in NOTIFICATION_KINDS in types.rs. The catch-all produces a generic "New activity" title with the (encrypted) content shown in the body.
For gift-wrapped DMs, the content is encrypted and shouldn't be displayed. Consider adding an explicit case:
🐛 Proposed fix
9735 => ("Someone zapped you".to_string(), "".to_string()),
+ 1059 => (
+ "New direct message".to_string(),
+ "Contents are encrypted".to_string(),
+ ),
_ => ("New activity".to_string(), truncate_content(content, 100)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/notedeck/src/platform/android.rs` around lines 291 - 331, Add an
explicit match arm for kind 1059 in parse_nostr_event_for_notification so
gift-wrapped DMs are treated like encrypted DMs (do not reveal decrypted
content): in the match over kind inside parse_nostr_event_for_notification add a
1059 => ("New direct message".to_string(), "Contents are encrypted".to_string())
branch (mirroring kind 4 behavior), ensuring FcmNotificationData still receives
title, body, event_id, event_kind and author_pubkey; reference
NOTIFICATION_KINDS in types.rs to keep parity with declared notification kinds.
| let author_pubkey: Option<String> = { | ||
| let s: String = env | ||
| .get_string(&author_pubkey) | ||
| .map(|s| s.into()) | ||
| .unwrap_or_default(); | ||
| if s.is_empty() { | ||
| None | ||
| } else { | ||
| Some(s) | ||
| } |
There was a problem hiding this comment.
Null JString not explicitly checked for author_pubkey in deep link handler.
Unlike the NIP-98 body handling (line 406 checks jbody.is_null()), author_pubkey on line 806 relies on get_string returning an Err for null, which is then silently converted to an empty string via unwrap_or_default(). This works but is fragile and inconsistent with the pattern used elsewhere in this file.
♻️ Suggested fix for consistency
let author_pubkey: Option<String> = {
- let s: String = env
- .get_string(&author_pubkey)
- .map(|s| s.into())
- .unwrap_or_default();
- if s.is_empty() {
- None
+ if author_pubkey.is_null() {
+ None
} else {
- Some(s)
+ match env.get_string(&author_pubkey) {
+ Ok(s) => {
+ let s: String = s.into();
+ if s.is_empty() { None } else { Some(s) }
+ }
+ Err(_) => None,
+ }
}
};📝 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.
| let author_pubkey: Option<String> = { | |
| let s: String = env | |
| .get_string(&author_pubkey) | |
| .map(|s| s.into()) | |
| .unwrap_or_default(); | |
| if s.is_empty() { | |
| None | |
| } else { | |
| Some(s) | |
| } | |
| let author_pubkey: Option<String> = { | |
| if author_pubkey.is_null() { | |
| None | |
| } else { | |
| match env.get_string(&author_pubkey) { | |
| Ok(s) => { | |
| let s: String = s.into(); | |
| if s.is_empty() { None } else { Some(s) } | |
| } | |
| Err(_) => None, | |
| } | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/notedeck/src/platform/android.rs` around lines 804 - 813, The code
handling author_pubkey should explicitly check the incoming JString for null
(like the jbody null check) instead of relying on env.get_string() to error;
update the author_pubkey block to first test author_pubkey.is_null() and return
None if null, otherwise call env.get_string(&author_pubkey) and convert to
Option<String> (treating empty string as None) — use the same pattern as the
existing jbody handling and keep references to author_pubkey and env.get_string
in the updated logic.
| /// Check if notification permission is granted. | ||
| /// On desktop, always returns true (no permission system like Android). | ||
| pub fn is_notification_permission_granted() -> Result<bool, Box<dyn std::error::Error>> { | ||
| Ok(true) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*notification*" | head -20Repository: damus-io/notedeck
Length of output: 282
🏁 Script executed:
git ls-files | grep -i notification | head -20Repository: damus-io/notedeck
Length of output: 1069
🏁 Script executed:
cat -n crates/notedeck/src/platform/desktop_notifications.rsRepository: damus-io/notedeck
Length of output: 2862
🏁 Script executed:
cat -n crates/notedeck/src/notifications/macos.rsRepository: damus-io/notedeck
Length of output: 21344
🏁 Script executed:
head -50 crates/notedeck/src/notifications/mod.rsRepository: damus-io/notedeck
Length of output: 1765
🏁 Script executed:
grep -r "is_notification_permission_granted" --include="*.rs" | head -20Repository: damus-io/notedeck
Length of output: 906
🏁 Script executed:
grep -r "cfg.*macos\|cfg.*target_os" crates/notedeck/src/notifications/ crates/notedeck/src/platform/ | head -20Repository: damus-io/notedeck
Length of output: 1725
🏁 Script executed:
cat -n crates/notedeck/src/platform/mod.rsRepository: damus-io/notedeck
Length of output: 10717
🏁 Script executed:
grep -r "request_notification_permission\|is_notification_permission_granted" --include="*.rs" -B 2 -A 2 | head -60Repository: damus-io/notedeck
Length of output: 4422
🏁 Script executed:
cat -n crates/notedeck/src/platform/android.rs | head -100Repository: damus-io/notedeck
Length of output: 4484
🏁 Script executed:
grep -n "permission" crates/notedeck/src/notifications/manager.rs | head -30Repository: damus-io/notedeck
Length of output: 240
🏁 Script executed:
cat -n crates/notedeck/src/notifications/manager.rs | head -150Repository: damus-io/notedeck
Length of output: 6128
🏁 Script executed:
grep -A 20 "request_permission\|permission_granted\|permission_pending" crates/notedeck/src/notifications/manager.rs | head -40Repository: damus-io/notedeck
Length of output: 43
🏁 Script executed:
rg "permission.*result\|permission.*granted\|permission.*state" crates/notedeck/src/notifications/ -A 2 -B 2 | head -80Repository: damus-io/notedeck
Length of output: 43
macOS hardcodes is_notification_permission_granted() to true, masking actual permission denial.
On macOS, initialize_on_main_thread() requests notification permission and logs the result (lines 289–303 in macos.rs), but this state is never stored or exposed. When users deny permissions, the stub function still returns true, leaving callers unaware that notifications will fail. The comment incorrectly generalizes "desktop" as having no permission system—only Linux lacks one; macOS requires explicit authorization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/notedeck/src/platform/desktop_notifications.rs` around lines 52 - 56,
The function is_notification_permission_granted() currently always returns true
which masks macOS permission denial; change it so macOS returns the actual
authorization state by reading a shared/observable value set by
macos.rs::initialize_on_main_thread() (or by directly querying the macOS
notification API), and return Err on query failure; specifically, update
is_notification_permission_granted() to consult the permission flag populated by
initialize_on_main_thread() (or call the platform check implemented for macOS)
instead of hardcoding Ok(true), and ensure the macOS initializer stores the
granted/denied state in a thread-safe/global place (e.g., static/atomic or a
Result-returning getter) that is used by is_notification_permission_granted().
| #[cfg(not(target_os = "android"))] | ||
| pub fn set_notification_mode( | ||
| _mode: NotificationMode, | ||
| _pubkey_hex: &str, | ||
| _relay_urls: &[String], | ||
| ) -> Result<(), Box<dyn std::error::Error>> { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Desktop set_notification_mode is a no-op, but desktop supports notifications.
supports_notifications() returns true on macOS and Linux, and the settings UI calls set_notification_mode. However, this non-Android stub silently succeeds without actually enabling/disabling the NotificationManager. This means the radio buttons in settings will appear to work but won't change notification behavior on desktop.
The desktop path should delegate to desktop_notifications::enable_notifications / disable_notifications based on the mode, similar to how the Android path does.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/notedeck/src/platform/mod.rs` around lines 99 - 106, The desktop stub
for set_notification_mode currently no-ops; replace it so it forwards the
requested NotificationMode to the desktop notification implementation: inside
pub fn set_notification_mode(...), match on NotificationMode and call
desktop_notifications::enable_notifications(_pubkey_hex, _relay_urls) when
enabling and desktop_notifications::disable_notifications() when disabling,
propagating any error as Result<(), Box<dyn std::error::Error>> (use ? to return
errors). Keep the #[cfg(not(target_os = "android"))] guard and ensure signatures
(set_notification_mode, NotificationMode,
desktop_notifications::enable_notifications/disable_notifications) match
existing APIs.
1 participant
Summary
Adds a full push notification system to Notedeck with two delivery modes and three platform backends:
A settings UI lets users choose between FCM, Native, or Disabled modes. Notifications support mentions, replies, reactions, reposts, DMs, and zaps with localized titles/bodies.
Architecture
Commits (23)
0788bafa96ec45e5371f6fd4c3433b9c1c7b835028aaffae9fe50fc7e9e7b0d115c4cdc80288f2ddc7c716972a61c1e042590e5d3b779abab81a3a45fee4c9079d2e0616df4261f6a6d5d98bce6eb74133843c978dc15206ff51aa39Review Feedback Status
All feedback from PRs #1261, #1265, #1266 has been addressed:
parse_bolt11_msats()intozaps/zap.rs;notifications/bolt11.rsdelegates to it instead of importinglightning_invoicedirectlyMediaCachecan't be reused: it re-encodes all images to WebP, but macOSUNNotificationAttachmentrequires original format local filesmpsc::channelto sendNotificationDatato worker thread; event extraction/profile lookup happens in main loop, display in workercolumns/app.rsintotry_process_events_core()innotedeck/src/app.rs;process_relay_message()now accepts&Accountsand extracts pubkeys internally#[cfg(target_os = "android")]gated; Android and desktop paths are mutually exclusiveArc<AtomicBool>for worker stop signal (standard pattern),RwLock<Option<WorkerHandle>>for service lifecycle,Arc<RwLock<HashMap>>in image_cache (tokio async).beads/directory is fully untrackedMainActivity.kt(lines 274-450)CodeRabbit Feedback Status
Automated review findings addressed:
MODE_FCM,MODE_NATIVE,MODE_DISABLEDconstants to companion objectsnativeProcessNostrEventcan throwUnsatisfiedLinkErrorNotedeckFirebaseMessagingService.ktnativeOnFcmTokenRefreshedcan throwUnsatisfiedLinkErrorNotedeckFirebaseMessagingService.ktConcurrentHashMapfor bitmap cacheLruCache<String, Bitmap>(100)acquire(10 * 60 * 1000L)with comment explaining 10-minute window""in NotepushClient"{}"kinddefaults to 0 silentlyNonewhen kind field missing, with warning logstartsWith("https://")checkBuildConfig.DEBUGtr!()macroenableFcmNotificationsreturn semantics unclearVecDeque+HashSetbounded LRU evictionwhile (it.moveToNext())cursor loopif (it.moveToFirst())for single-row queryBug Fixes (squashed into parent commits)
set_signing_keypair()inAccounts::new()aftercache.select()set_notification_modenow re-triggersenable_fcm_notificationson same-mode restoreonMessageReceivedgates onnotification_mode == FCM; drops messages when mode is disabledNew Files
notifications/mod.rsnotifications/manager.rsnotifications/types.rsnotifications/extraction.rsnotifications/worker.rsnotifications/backend.rsnotifications/desktop.rsnotifications/macos.rsnotifications/image_cache.rsnotifications/profiles.rsnotifications/bolt11.rsplatform/mod.rsplatform/desktop_notifications.rschrome/notifications.rsMainActivity.ktNotificationsService.ktNotificationHelper.ktNotedeckFirebaseMessagingService.ktNotepushClient.ktNotificationActionReceiver.ktBootReceiver.ktTest plan
cargo checkpasses (host target)cargo test -p notedeck -p notedeck_chromepasses (bolt11 parsing, event extraction)cargo fmt -p notedeck -p notedeck_chrome -p notedeck_columns --checkclean🤖 Generated with Claude Code