Add FCM push notification support#122
Merged
malkoG merged 10 commits intohackers-pub:mainfrom Apr 25, 2026
Merged
Conversation
- Add firebase-messaging dependency - Implement HackersPubMessagingService for receiving FCM data messages - Implement FcmTokenManager for token registration/unregistration via registerFcmDeviceToken/unregisterFcmDeviceToken GraphQL mutations - Register FCM token on login, unregister on logout - Request notification permission on login (not just app startup) - Remove foreground polling (FCM replaces it; 15-min WorkManager polling remains as fallback) - Handle GraphQL union error types in mutation responses - Add FCM mutation types to local GraphQL schema Requires server-side FCM support: hackers-pub/hackerspub#251 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve conflict in app/src/main/graphql/pub/hackers/android/operations.graphql by keeping both sides' new operations: main's EditAccount/UpdateAccount/ MarkNotificationsAsRead and this branch's RegisterFcmDeviceToken/ UnregisterFcmDeviceToken. main also moved app/google-services.json out of the repo into a base64 GitHub Actions secret (645a38f); the merge takes that change as-is. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The merge commit silently dropped two manifest changes from main: - c38339e (Instantiatable suppression on MainActivity) - e8ef8ec (removed /notifications deep link intent-filter; the router side of that change did land in the merge) Reapply both, plus add the same Instantiatable suppression to the new HackersPubMessagingService (FCM) since it triggers the same Hilt @androidentrypoint false positive that c38339e narrowed for MainActivity. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 29 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis pull request adds Firebase Cloud Messaging (FCM) support to the app. It introduces a new FCM service to handle incoming messages and token updates, defines GraphQL mutations for registering and unregistering device tokens, adds a token manager for handling the registration flow, updates authentication logic to trigger token registration/unregistration at login/logout, and includes comprehensive test coverage for token operations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as HackersPubApp
participant TokenMgr as FcmTokenManager
participant Firebase as Firebase Messaging
participant GraphQL as GraphQL Server
User->>App: Login
App->>TokenMgr: registerFcmToken()
TokenMgr->>Firebase: Fetch device token
Firebase-->>TokenMgr: Token string
TokenMgr->>GraphQL: RegisterFcmDeviceTokenMutation
GraphQL-->>TokenMgr: Success/Error response
TokenMgr->>App: Logged
User->>App: Logout
App->>TokenMgr: unregisterFcmToken()
TokenMgr->>GraphQL: UnregisterFcmDeviceTokenMutation
GraphQL-->>TokenMgr: Success/Error response
TokenMgr->>App: Complete sign-out
sequenceDiagram
participant Firebase as Firebase Cloud<br/>Messaging
participant Service as HackersPubMessaging<br/>Service
participant NotifMgr as Notification State<br/>Manager
participant OS as Android OS
participant User
Firebase->>Service: onMessageReceived(RemoteMessage)
Service->>NotifMgr: Update notification state
Service->>OS: Check POST_NOTIFICATIONS permission
OS-->>Service: Permission granted/denied
alt Permission granted
Service->>OS: Post NotificationCompat
OS->>User: Display notification
User->>OS: Tap notification
OS->>Service: Open MainActivity with nav extra
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Member
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Adds Firebase Cloud Messaging (FCM) support to replace foreground notification polling, including token lifecycle management via GraphQL mutations and a FirebaseMessagingService to handle incoming data messages.
Changes:
- Add
firebase-messagingdependency and register anFirebaseMessagingServicein the manifest. - Implement
FcmTokenManagerto register/unregister device tokens via GraphQL mutations and hook registration into login flow. - Remove foreground polling, keeping periodic WorkManager polling as a fallback.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
gradle/libs.versions.toml |
Adds the Firebase Messaging library coordinate. |
app/build.gradle.kts |
Includes firebase-messaging dependency under the Firebase BoM. |
app/src/main/AndroidManifest.xml |
Registers HackersPubMessagingService for com.google.firebase.MESSAGING_EVENT. |
app/src/main/java/pub/hackers/android/data/messaging/HackersPubMessagingService.kt |
Receives FCM messages, updates unread state, and posts notifications. |
app/src/main/java/pub/hackers/android/data/messaging/FcmTokenManager.kt |
Handles FCM token registration/unregistration via Apollo GraphQL mutations. |
app/src/main/java/pub/hackers/android/ui/HackersPubApp.kt |
Triggers WorkManager polling fallback + FCM token registration on login. |
app/src/main/java/pub/hackers/android/ui/AppViewModel.kt |
Removes foreground polling and adds a method to register the FCM token. |
app/src/main/java/pub/hackers/android/ui/screens/settings/SettingsViewModel.kt |
Unregisters the current FCM token during sign-out. |
app/src/main/java/pub/hackers/android/MainActivity.kt |
Requests POST_NOTIFICATIONS permission based on login state changes. |
app/src/main/graphql/pub/hackers/android/schema.graphqls |
Adds schema definitions for FCM register/unregister mutations and result unions. |
app/src/main/graphql/pub/hackers/android/operations.graphql |
Adds GraphQL operations for FCM token register/unregister. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- HackersPubMessagingService.onMessageReceived: replace runBlocking with serviceScope.launch so the FCM callback thread is not blocked on DataStore writes (avoids ANR risk under load) - HackersPubMessagingService.showNotification: derive notification id from server notificationId (hashCode) instead of currentTimeMillis, so re-deliveries update an existing notification and the int doesn't overflow - MainActivity.requestNotificationPermissionIfNeeded: gate the isLoggedIn collect with repeatOnLifecycle(STARTED) and react only on the false→true transition (distinctUntilChanged + filter), so the permission dialog can't be launched while the activity isn't visible - HackersPubApp: unregister the FCM token on the true→false logged-in transition so logout paths beyond Settings sign-out also have a chance to clear the token. Track the previous value in remember to avoid firing the unauthenticated mutation on cold start while logged out - FcmTokenManager: split unregisterCurrentToken into a token-fetch wrapper plus unregisterToken(token), mirroring the register path, and log the token-fetch failure that was previously swallowed Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cover the testable surface of register/unregister flows without having to mockkStatic FirebaseMessaging: - registerToken returns early without calling Apollo when logged out - registerToken sends the mutation with the captured token when logged in - registerToken survives hasErrors responses, every union variant (success payload, FailedError, InvalidInput, NotAuthenticated, plus an unknown __typename), and Apollo execute() throwing - unregisterToken counterpart tests for the same shape (no FailedError variant since the schema doesn't have one) ApolloResponse exposes data/errors/requestUuid as @JvmField, which mockk cannot intercept, so the helpers build real ApolloResponse instances via ApolloResponse.Builder + uuid4 instead of mocking. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/pub/hackers/android/MainActivity.kt (1)
146-164:⚠️ Potential issue | 🟡 MinorPermission prompt may re-launch on every foreground transition while denied.
Because
repeatOnLifecycle(STARTED)re-subscribes on each entry to STARTED,distinctUntilChanged()resets per-collection. For a logged-in user who has denied (but not permanently denied)POST_NOTIFICATIONS, the launcher will fire on every return to foreground. Consider tracking a "we already asked this session" flag, or moving this to a one-shot per-process flag, so the prompt isn't re-shown until the user explicitly re-enables it from settings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/pub/hackers/android/MainActivity.kt` around lines 146 - 164, The current requestNotificationPermissionIfNeeded() flow will re-show the POST_NOTIFICATIONS prompt on every STARTED lifecycle re-entry because distinctUntilChanged() is scoped to each collection; to fix, add and check a one-shot flag (e.g., hasAskedNotificationPermissionThisSession or a process-level boolean) before calling requestPermissionLauncher.launch and set it true immediately after launching so the prompt is only fired once per session/process; update the logic around sessionManager.isLoggedIn.collect in requestNotificationPermissionIfNeeded() to consult this flag (and reset it only on explicit logout or app restart) rather than relying on the repeatOnLifecycle subscription to suppress repeated prompts.
🧹 Nitpick comments (6)
app/src/main/java/pub/hackers/android/ui/HackersPubApp.kt (1)
209-221: Transition tracking lives in Composable state and is lost across configuration changes / process recreation.
prevLoggedInis held inremember { mutableStateOf<Boolean?>(null) }, so on every Activity recreation (rotation, theme change, dynamic-color recreate viawallpaperColorsListener, etc.) it resets tonull. Consequences:
- A logged-in user rotating the device will trigger another
registerFcmToken()(extra Apollo mutation on every config change).- The
prevLoggedIn == trueguard correctly prevents the cold-start logout case, but it cannot distinguish "process was recreated while logged out" from "user just logged out within the same process" once the user logs back in and out within a recreation boundary.Consider lifting the transition logic into
AppViewModel(which survives configuration changes), e.g. observesessionManager.isLoggedIn.distinctUntilChanged()with arunningFold/scanto detect edges, and let the ViewModel be the single owner of register/unregister +enqueueNotificationPolling. That also collapses the duplicate unregister path that happens today (Settings callsunregisterCurrentToken()and then thisLaunchedEffectfiresunregisterFcmToken()again on the resultingisLoggedInflip).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/pub/hackers/android/ui/HackersPubApp.kt` around lines 209 - 221, The transition tracking stored in the Composable via prevLoggedIn resets across configuration changes causing duplicate register/unregister calls; move the edge-detection and token lifecycle into AppViewModel: observe sessionManager.isLoggedIn.distinctUntilChanged() (or Flow/LiveData) and use a runningFold/scan to detect true→false and false→true edges, then invoke enqueueNotificationPolling(), registerFcmToken(), and unregisterFcmToken() from the ViewModel (remove prevLoggedIn and the LaunchedEffect logic in HackersPubApp.kt) so the ViewModel becomes the single owner of register/unregister behavior and survives configuration changes.app/src/main/java/pub/hackers/android/data/messaging/HackersPubMessagingService.kt (2)
34-34: CancelserviceScopeinonDestroy()to avoid leaking coroutines.
serviceScopeis created with aSupervisorJobbut never cancelled when the service is destroyed. Coroutines launched fromonNewToken/onMessageReceived(e.g. theupdateLastPolledIdwrite) can outlive the service and continue running against a destroyedServicecontext. OverrideonDestroy()to cancel it.♻️ Proposed fix
import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel import kotlinx.coroutines.launch @@ private val serviceScope = CoroutineScope(SupervisorJob() + Dispatchers.IO) + + override fun onDestroy() { + serviceScope.cancel() + super.onDestroy() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/pub/hackers/android/data/messaging/HackersPubMessagingService.kt` at line 34, The serviceScope CoroutineScope (created with SupervisorJob + Dispatchers.IO) is never cancelled, risking coroutine leaks from onNewToken/onMessageReceived (e.g., updateLastPolledId); override onDestroy() in HackersPubMessagingService and call serviceScope.cancel() (and then super.onDestroy()) to cancel any running coroutines when the service is destroyed.
70-77:PendingIntentrequest code is constant — taps on different notifications will reuse the same intent.
PendingIntent.getActivity(this, 0, intent, FLAG_UPDATE_CURRENT | FLAG_IMMUTABLE)uses request code0for every notification. Combined withnotify(notificationId.hashCode(), ...)allowing multiple stacked notifications, all of them currently route to the same"navigate_to=notifications"extra (so this works today), but if you ever add per-notification deep links the shared request code will silently overwrite extras across pending intents. Consider usingnotificationId.hashCode()as the request code for forward compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/pub/hackers/android/data/messaging/HackersPubMessagingService.kt` around lines 70 - 77, The PendingIntent in HackersPubMessagingService (the block creating intent and pendingIntent) uses a fixed requestCode of 0 which causes different notifications to share/overwrite the same PendingIntent; change the requestCode argument passed to PendingIntent.getActivity from 0 to the notification identifier (use notificationId.hashCode()) so each notification gets a unique PendingIntent while keeping the same intent extras and flags (PendingIntent.FLAG_UPDATE_CURRENT or FLAG_IMMUTABLE).app/src/test/java/pub/hackers/android/data/messaging/FcmTokenManagerTest.kt (1)
146-179: Loop tests rely oneveryre-stubbing without resetting state between iterations.Both
registerToken handles all union variants without throwingand the unregister equivalent loop over cases and callstubRegisterMutation/stubUnregisterMutationin each iteration. SinceapolloClientandsessionManagerare class-levelmockks, recorded calls and slot history accumulate across iterations. Today this is fine because no test asserts on call counts within these loops, but it's a fragile pattern. ConsiderclearMocks(apolloClient, sessionManager)(or constructing fresh mocks per iteration) so future verification additions don't silently observe stale state.Also applies to: 219-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/pub/hackers/android/data/messaging/FcmTokenManagerTest.kt` around lines 146 - 179, The looped tests like `registerToken handles all union variants without throwing` call `stubRegisterMutation` (and the similar unregister test that calls `stubUnregisterMutation`) repeatedly while using class-level mockk instances `apolloClient` and `sessionManager`, which lets recorded calls/slots accumulate across iterations; fix by clearing mock state before each iteration (e.g., call clearMocks(apolloClient, sessionManager) at the top of the loop) or by constructing fresh mocks per iteration so each case starts with a clean mock state and future call-count assertions won’t observe stale interactions.app/src/main/java/pub/hackers/android/data/messaging/FcmTokenManager.kt (2)
50-50: Avoid logging the fullresponse.errorspayload.
response.errorsmay include server-sideextensions/pathdata that, depending on the server implementation, could echo back portions of the request (including the device token inpath/locationsfor input-validation errors). Prefer logging justerrors.sizeorerrors.joinToString { it.message }to keep the device-token value out of logs.♻️ Proposed fix
- Log.w(TAG, "FCM token registration returned errors: ${response.errors}") + Log.w(TAG, "FCM token registration returned ${response.errors?.size ?: 0} error(s): " + + response.errors?.joinToString { it.message }.orEmpty()) @@ - Log.w(TAG, "FCM token unregistration returned errors: ${response.errors}") + Log.w(TAG, "FCM token unregistration returned ${response.errors?.size ?: 0} error(s): " + + response.errors?.joinToString { it.message }.orEmpty())Also applies to: 91-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/pub/hackers/android/data/messaging/FcmTokenManager.kt` at line 50, The current Log.w calls in FcmTokenManager.kt that log "response.errors" should be changed to avoid emitting the full errors payload; update the Log.w invocations (the ones that currently reference response.errors) to log only a safe summary such as the count and/or concatenated messages (e.g., response.errors.size and response.errors.joinToString { it.message }) instead of the full object; apply this change to both occurrences in the FcmTokenManager class so the device token or other sensitive data in extensions/path is not written to logs.
25-36: RedundantisLoggedIncheck causes a duplicate DataStore read.
registerCurrentToken()checksisLoggedInat line 26 and then callsregisterToken(token), which checks it again at line 39. Both go throughdataStore.data.map { ... }.first()(seeSessionManager.isLoggedInflow atapp/src/main/java/pub/hackers/android/data/local/SessionManager.kt:32-43). Either drop the upfront check inregisterCurrentToken()(the inner method is the source of truth) or skip the inner check when called from this path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/pub/hackers/android/data/messaging/FcmTokenManager.kt` around lines 25 - 36, The upfront isLoggedIn check in registerCurrentToken() is redundant because registerToken(token) already reads SessionManager.isLoggedIn (causing a second DataStore read); remove the initial isLoggedIn read and early return from registerCurrentToken() so it simply fetches the FCM token and calls registerToken(token), letting registerToken() remain the single source of truth for login state (identify registerCurrentToken and registerToken methods and remove the isLoggedIn .first() check and conditional return in registerCurrentToken()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/pub/hackers/android/data/messaging/FcmTokenManager.kt`:
- Around line 38-70: registerToken currently reads
sessionManager.isLoggedIn.first() then awaits the GraphQL mutation which can
race with logout; after executing the mutation in registerToken, re-check
sessionManager.isLoggedIn.first() and if it is now false immediately call
unregisterToken(token) to undo the registration, or alternatively protect
registerToken/unregisterToken and SettingsViewModel.signOut/clearSession with a
shared Mutex to serialize those flows so they cannot interleave; update
registerToken to perform the post-mutation check and/or use a shared Mutex
(referencing registerToken, unregisterToken, sessionManager.isLoggedIn and
SettingsViewModel.signOut/clearSession) so the device token is not left
registered for a logged-out user.
In
`@app/src/main/java/pub/hackers/android/data/messaging/HackersPubMessagingService.kt`:
- Around line 79-85: Replace the adaptive launcher foreground and hardcoded
title in the NotificationCompat.Builder in HackersPubMessagingService (where the
variable notification is built): change setSmallIcon(...) to use a monochrome
notification asset (e.g., R.drawable.ic_notification) instead of
R.drawable.ic_launcher_foreground, and change setContentTitle("Hackers' Pub") to
use a string resource (e.g., getString(R.string.notification_title) or
R.string.app_name) after adding the corresponding entry in strings.xml; ensure
the new ic_notification drawable is an alpha-only asset suitable for
notification small icons.
---
Outside diff comments:
In `@app/src/main/java/pub/hackers/android/MainActivity.kt`:
- Around line 146-164: The current requestNotificationPermissionIfNeeded() flow
will re-show the POST_NOTIFICATIONS prompt on every STARTED lifecycle re-entry
because distinctUntilChanged() is scoped to each collection; to fix, add and
check a one-shot flag (e.g., hasAskedNotificationPermissionThisSession or a
process-level boolean) before calling requestPermissionLauncher.launch and set
it true immediately after launching so the prompt is only fired once per
session/process; update the logic around sessionManager.isLoggedIn.collect in
requestNotificationPermissionIfNeeded() to consult this flag (and reset it only
on explicit logout or app restart) rather than relying on the repeatOnLifecycle
subscription to suppress repeated prompts.
---
Nitpick comments:
In `@app/src/main/java/pub/hackers/android/data/messaging/FcmTokenManager.kt`:
- Line 50: The current Log.w calls in FcmTokenManager.kt that log
"response.errors" should be changed to avoid emitting the full errors payload;
update the Log.w invocations (the ones that currently reference response.errors)
to log only a safe summary such as the count and/or concatenated messages (e.g.,
response.errors.size and response.errors.joinToString { it.message }) instead of
the full object; apply this change to both occurrences in the FcmTokenManager
class so the device token or other sensitive data in extensions/path is not
written to logs.
- Around line 25-36: The upfront isLoggedIn check in registerCurrentToken() is
redundant because registerToken(token) already reads SessionManager.isLoggedIn
(causing a second DataStore read); remove the initial isLoggedIn read and early
return from registerCurrentToken() so it simply fetches the FCM token and calls
registerToken(token), letting registerToken() remain the single source of truth
for login state (identify registerCurrentToken and registerToken methods and
remove the isLoggedIn .first() check and conditional return in
registerCurrentToken()).
In
`@app/src/main/java/pub/hackers/android/data/messaging/HackersPubMessagingService.kt`:
- Line 34: The serviceScope CoroutineScope (created with SupervisorJob +
Dispatchers.IO) is never cancelled, risking coroutine leaks from
onNewToken/onMessageReceived (e.g., updateLastPolledId); override onDestroy() in
HackersPubMessagingService and call serviceScope.cancel() (and then
super.onDestroy()) to cancel any running coroutines when the service is
destroyed.
- Around line 70-77: The PendingIntent in HackersPubMessagingService (the block
creating intent and pendingIntent) uses a fixed requestCode of 0 which causes
different notifications to share/overwrite the same PendingIntent; change the
requestCode argument passed to PendingIntent.getActivity from 0 to the
notification identifier (use notificationId.hashCode()) so each notification
gets a unique PendingIntent while keeping the same intent extras and flags
(PendingIntent.FLAG_UPDATE_CURRENT or FLAG_IMMUTABLE).
In `@app/src/main/java/pub/hackers/android/ui/HackersPubApp.kt`:
- Around line 209-221: The transition tracking stored in the Composable via
prevLoggedIn resets across configuration changes causing duplicate
register/unregister calls; move the edge-detection and token lifecycle into
AppViewModel: observe sessionManager.isLoggedIn.distinctUntilChanged() (or
Flow/LiveData) and use a runningFold/scan to detect true→false and false→true
edges, then invoke enqueueNotificationPolling(), registerFcmToken(), and
unregisterFcmToken() from the ViewModel (remove prevLoggedIn and the
LaunchedEffect logic in HackersPubApp.kt) so the ViewModel becomes the single
owner of register/unregister behavior and survives configuration changes.
In `@app/src/test/java/pub/hackers/android/data/messaging/FcmTokenManagerTest.kt`:
- Around line 146-179: The looped tests like `registerToken handles all union
variants without throwing` call `stubRegisterMutation` (and the similar
unregister test that calls `stubUnregisterMutation`) repeatedly while using
class-level mockk instances `apolloClient` and `sessionManager`, which lets
recorded calls/slots accumulate across iterations; fix by clearing mock state
before each iteration (e.g., call clearMocks(apolloClient, sessionManager) at
the top of the loop) or by constructing fresh mocks per iteration so each case
starts with a clean mock state and future call-count assertions won’t observe
stale interactions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47d8fe08-f7b2-409c-8b51-1c5598ef6d3e
📒 Files selected for processing (12)
app/build.gradle.ktsapp/src/main/AndroidManifest.xmlapp/src/main/graphql/pub/hackers/android/operations.graphqlapp/src/main/graphql/pub/hackers/android/schema.graphqlsapp/src/main/java/pub/hackers/android/MainActivity.ktapp/src/main/java/pub/hackers/android/data/messaging/FcmTokenManager.ktapp/src/main/java/pub/hackers/android/data/messaging/HackersPubMessagingService.ktapp/src/main/java/pub/hackers/android/ui/AppViewModel.ktapp/src/main/java/pub/hackers/android/ui/HackersPubApp.ktapp/src/main/java/pub/hackers/android/ui/screens/settings/SettingsViewModel.ktapp/src/test/java/pub/hackers/android/data/messaging/FcmTokenManagerTest.ktgradle/libs.versions.toml
The CoroutineScope(SupervisorJob() + Dispatchers.IO) was never cancelled. FirebaseMessagingService instances are destroyed and recreated by the system, so without an onDestroy() hook the launched jobs (token registration, DataStore writes) would outlive their service instance and pile up across recreations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous setSmallIcon target ic_launcher_foreground is a colored adaptive vector (#1A1A2E / #E94560). On API 21+ the system tints the notification small icon based on alpha only, so a colored asset renders as a white square on many devices. Add a dedicated alpha-only ic_notification.xml and switch the FCM service to it. Also replace the hardcoded "Hackers' Pub" title with getString(R.string.app_name) — this string is already provided per buildType via build.gradle.kts resValue (debug shows "Hackers' Pub Dev", release shows "Hackers' Pub"), matching the manifest's android:label source. Scope is limited to HackersPubMessagingService; the same pattern in NotificationWorker is pre-existing tech debt outside this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
registerToken reads sessionManager.isLoggedIn before awaiting the GraphQL mutation. SettingsViewModel.signOut() unregisters the current token then calls clearSession(), but a register call already in flight when sign-out begins can complete after clearSession() runs, leaving the device token registered on the server for a now logged-out user. After the success arm of the mutation response, re-check sessionManager.isLoggedIn.first(); if it has flipped to false, call unregisterToken(token) to roll back. Only the success arm runs the check — error responses (FailedError, InvalidInputError, NotAuthenticatedError, hasErrors) never committed a registration on the server, so a rollback there would just be a guaranteed-fail extra call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add §9.4 covering the gotcha discovered while writing FcmTokenManagerTest: ApolloResponse exposes data/errors/requestUuid as @JvmField, so mockk can't intercept the field reads. The fix is to construct real instances via the public ApolloResponse.Builder (plus uuid4() from com.benasher44.uuid). Reference: app/src/test/java/pub/hackers/android/data/messaging/FcmTokenManagerTest.kt Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two functional pitfalls bit this exact PR while resolving conflicts against main: 1. Merging a stale local `main` (one commit behind `origin/main`) produced a merge that silently lacked the latest commit's changes; recovering required `git merge --abort` and remerging against the synced main. 2. Even after the synced merge succeeded with no conflict markers, AndroidManifest.xml lost two hunks that main had added (the MainActivity Instantiatable suppression and the /notifications intent-filter removal). git's 3-way auto-merge picked one side silently for those regions. Add a one-line expectation so contributors know to sync local main first and to verify intersection files against `main` after the merge instead of trusting the "Auto-merging" output. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
firebase-messagingdependency andHackersPubMessagingServicefor receiving FCM data messagesFcmTokenManagerfor token registration/unregistration via GraphQL mutationsLaunchedEffect(isLoggedIn)), unregister on logoutPOST_NOTIFICATIONSpermission on login transition (not just app startup)RegisterFcmDeviceTokenFailedError,InvalidInputError,NotAuthenticatedError) in mutation responsesTest plan
./gradlew assembleDebugbuild passesFcmTokenManagerlogs)🤖 Generated with Claude Code