feat(fcm): integrate Firebase Cloud Messaging for push notifications#29
Open
GRACENOBLE wants to merge 1 commit into
Open
feat(fcm): integrate Firebase Cloud Messaging for push notifications#29GRACENOBLE wants to merge 1 commit into
GRACENOBLE wants to merge 1 commit into
Conversation
Backend: - Refactor pkg/firebase to share a single App instance between auth and messaging clients (NewApp → NewAuthClient + NewMessagingClient) - Add NotificationSender and FCMTokenRepository interfaces in usecase/ - Add FCMToken domain entity and fcm_tokens migration (uuid PK, user_id, token UNIQUE, platform, created_at) with user_id index - Implement FCMTokenRepository in infrastructure/database/postgres with upsert semantics on token conflict - Add POST /api/v1/fcm/register and DELETE /api/v1/fcm/unregister handlers, guarded by FirebaseAuth middleware; wired only when Firebase is configured - Wire FCMSender and FCMTokenRepository through bootstrap.App → server → Handler - Add 5 handler unit tests (mock repo) and 4 repository integration tests (Testcontainers / real PostgreSQL) - Regenerate Swagger docs with FCM endpoint annotations Web: - Install firebase JS SDK - Add lib/fcm.ts: requestPermissionAndGetToken() and onForegroundMessage() - Add lib/useFCM.ts hook: requests permission, registers token with backend - Add public/firebase-messaging-sw.js service worker for background messages - Add 4 Vitest unit tests (vi.hoisted mocks for firebase/app and firebase/messaging) Mobile: - Add Firebase BOM 33.7.0 + firebase-messaging-ktx to version catalog and build.gradle - Add MyFirebaseMessagingService: onNewToken (posts to backend), onMessageReceived (shows NotificationCompat notification, channel creation guarded for API 26+) - Add FcmRegistrationPayload (@serializable data class) - Register service in AndroidManifest with MESSAGING_EVENT intent filter - Add POST_NOTIFICATIONS and INTERNET permissions - Add 4 JUnit unit tests for FcmRegistrationPayload serialisation Docs: - Fix stale backend/docs/auth.md (verifier wiring was incorrect) - Create backend/docs/fcm.md, web/docs/fcm.md, mobile/docs/fcm.md - Update all three _index.md files Closes #19
8 tasks
github-actions
Bot
added
area: backend
📝 WalkthroughWalkthroughAdds end-to-end Firebase Cloud Messaging integration across all three platforms. The backend gains a new ChangesBackend FCM Integration
Web FCM Integration
Mobile (Android) FCM Integration
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over Client,FCMTokenRepository: Token Registration Flow
end
participant Client as Client (Web/Android)
participant useFCM as useFCM / MyFirebaseMessagingService
participant FirebaseSDK as Firebase SDK
participant BackendAPI as POST /api/v1/fcm/register
participant FCMTokenRepository as FCMTokenRepository (Postgres)
Client->>useFCM: mount / onNewToken callback
useFCM->>FirebaseSDK: requestPermission + getToken / FirebaseMessagingService.onNewToken
FirebaseSDK-->>useFCM: FCM registration token
useFCM->>BackendAPI: POST {token, platform} + Bearer idToken
BackendAPI->>BackendAPI: validate FirebaseClaimsKey
BackendAPI->>FCMTokenRepository: SaveToken(userID, token, platform) upsert
FCMTokenRepository-->>BackendAPI: ok
BackendAPI-->>useFCM: 200 {message: "FCM token registered"}
sequenceDiagram
rect rgba(255, 200, 150, 0.5)
Note over bootstrap,NotificationSender: Backend Firebase Initialization
end
participant bootstrap
participant NewApp as firebase.NewApp
participant NewAuthClient as firebase.NewAuthClient
participant NewMessagingClient as firebase.NewMessagingClient
participant server
bootstrap->>NewApp: NewApp(ctx, projectID, credentialsJSON)
NewApp-->>bootstrap: *firebase.App
bootstrap->>NewAuthClient: NewAuthClient(ctx, app)
NewAuthClient-->>bootstrap: FirebaseAdminClient → App.Firebase
bootstrap->>NewMessagingClient: NewMessagingClient(ctx, app)
NewMessagingClient-->>bootstrap: NotificationSender → App.FCMSender
bootstrap->>server: NewServer(app) with FCMSender + FCMTokenRepository
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/docs/auth.md (1)
55-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
NewAuthClientdocs to match current API.Lines 55-63 still describe
NewAuthClient(ctx, projectID, credentialsJSON)and in-function SDK init, but the current implementation accepts a prebuilt Firebase app (NewAuthClient(ctx, app *firebasesdk.App)).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/docs/auth.md` around lines 55 - 63, Update the `NewAuthClient` documentation in the auth.md file to reflect the current API. The function signature should show it accepts a context and a prebuilt Firebase app (firebasesdk.App) instead of projectID and credentialsJSON parameters. Remove the documentation describing SDK initialization with credentialsJSON and Application Default Credentials fallback, as the Firebase app is now passed in as an already-initialized dependency rather than being created within the function.
🧹 Nitpick comments (1)
backend/docs/fcm.md (1)
84-90: ⚡ Quick winAvoid showing swallowed errors in the bootstrap example.
Lines 85-88 ignore errors (
_), which is risky as a copy/paste example for backend code.Suggested doc snippet update
if cfg.FirebaseProjectID != "" { - fbApp, _ := firebase.NewApp(ctx, cfg.FirebaseProjectID, cfg.FirebaseServiceAccountJSON) - authClient, _ := firebase.NewAuthClient(ctx, fbApp) - msgClient, _ := firebase.NewMessagingClient(ctx, fbApp) + fbApp, err := firebase.NewApp(ctx, cfg.FirebaseProjectID, cfg.FirebaseServiceAccountJSON) + if err != nil { return nil, err } + authClient, err := firebase.NewAuthClient(ctx, fbApp) + if err != nil { return nil, err } + msgClient, err := firebase.NewMessagingClient(ctx, fbApp) + if err != nil { return nil, err } app.Firebase = authClient app.FCMSender = msgClient }As per coding guidelines, "Return errors up the stack — never swallow them".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/docs/fcm.md` around lines 84 - 90, The documentation example in the Firebase initialization block is ignoring errors returned from firebase.NewApp(), firebase.NewAuthClient(), and firebase.NewMessagingClient() by using the blank identifier `_`. Replace the underscore error assignments with proper error variable declarations and add error handling checks for each call. After each function invocation, check if the error is not nil and handle it appropriately (either by logging, returning the error up the stack, or exiting gracefully). This ensures the documentation example follows the coding guideline of returning errors up the stack rather than swallowing them, making it a better reference for backend code patterns.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/docs/fcm.md`:
- Line 3: The last_verified field in backend/docs/fcm.md on line 3 contains a
future date of 2026-06-16 instead of the actual verification date. Update the
last_verified value to reflect the actual date when the documentation was last
verified, ensuring it is not a future date relative to today.
In
`@backend/internal/infrastructure/database/migrations/20260615220942_add_fcm_tokens.sql`:
- Around line 2-8: The `platform` column in the fcm_tokens table is defined as
unconstrained TEXT, allowing invalid values to be persisted in the database. Add
a CHECK constraint to the `platform` column definition in the CREATE TABLE
fcm_tokens statement to restrict it to only valid platform enum values (such as
'ios', 'android', and 'web'). This enforces data integrity at the schema layer
and prevents invalid platform values from being stored regardless of
handler-level validation.
In
`@backend/internal/infrastructure/database/postgres/fcm_token_repository_test.go`:
- Around line 60-61: Remove the blank identifier (_) that ignores errors
returned by GetTokensByUserID method calls. At each affected site in the
fcm_token_repository_test.go file—lines 60–61 (anchor), lines 75 (sibling), and
lines 80 (sibling)—capture the error return value and check it explicitly using
the test helper (such as t.Fatalf or require.NoError) to fail the test if an
error occurs. This ensures that integration failures in repository calls are not
masked and the test properly validates the behavior.
In `@backend/internal/server/server.go`:
- Line 37: The FCM route gating mechanism is ineffective because fcmTokenRepo is
unconditionally created at line 37 via postgres.NewFCMTokenRepository regardless
of Firebase configuration, and the nil check at line 55 cannot properly gate the
routes. Move the creation of fcmTokenRepo inside a conditional block that checks
whether Firebase is available, so the repo is only instantiated when Firebase is
actually configured, then pass this conditionally-created fcmTokenRepo to the
route registration at line 55.
In `@backend/internal/transport/handlers/fcm_handler.go`:
- Around line 72-85: The unregisterFCMTokenRequest handler extracts the
authenticated FirebaseToken from the context but fails to use it when deleting
the FCM token, creating a security vulnerability where any exposed token can be
revoked by another user. Extract the user ID from the authenticated token (the
val variable that is cast to usecase.FirebaseToken at line 73), and pass both
the user ID and the token to the h.fcmTokenRepo.DeleteToken method call. Update
the repository's DeleteToken method signature to accept both user_id and token
parameters, and modify the underlying SQL query to enforce deletion only when
both the user_id and token match (DELETE FROM fcm_tokens WHERE user_id = $1 AND
token = $2).
In `@backend/internal/transport/handlers/routes.go`:
- Around line 68-71: The FCM routes (RegisterFCMToken and UnregisterFCMToken)
are currently gated only on the presence of h.fcmTokenRepo, but since this repo
is always created in the server setup regardless of Firebase configuration, the
routes register even when Firebase is disabled. Replace the condition checking
h.fcmTokenRepo != nil with a proper check for Firebase availability or
enablement (likely a configuration flag or boolean field on the handler that
reflects whether Firebase is actually configured and enabled), ensuring the
routes only register when Firebase is actually available.
In `@backend/pkg/firebase/app.go`:
- Around line 14-20: The NewApp function does not validate that the projectID
parameter is non-empty before proceeding with Firebase client initialization,
causing misconfiguration to be discovered downstream rather than at bootstrap.
Add validation at the start of the NewApp function to check if projectID is an
empty string and return an error immediately if it is, ensuring fast failure
during configuration and clearer error messaging for users who have
misconfigured the application.
In `@backend/pkg/firebase/messaging.go`:
- Around line 19-21: The NewMessagingClient function does not validate that the
app parameter is non-nil before calling app.Messaging(ctx), which will cause a
nil pointer dereference panic if a nil Firebase app is passed. Add an explicit
nil check at the beginning of the NewMessagingClient function to guard against
this case, and return an appropriate error if app is nil before attempting to
call app.Messaging(ctx).
In
`@mobile/app/src/main/java/com/company/template/fcm/MyFirebaseMessagingService.kt`:
- Around line 46-49: The FCM registration request built in
MyFirebaseMessagingService using Request.Builder() for the /api/v1/fcm/register
endpoint is missing authorization headers. The backend endpoint is auth-guarded
and expects a bearer token, but the request currently sends none, causing a 401
response. Add an authorization header to the Request.Builder() call by chaining
a .header() method that includes a bearer token (obtained from your
authentication provider or token storage mechanism) to satisfy the backend's
authentication contract.
- Line 47: The backendUrl is being used directly in the .url() method call
without validating that it uses the HTTPS scheme, which violates the requirement
that all network calls must use HTTPS. Add scheme validation logic before the
URL is built to ensure that backendUrl starts with "https://" and reject or
throw an error if it uses HTTP or any other non-HTTPS scheme. Perform this
validation before passing backendUrl to the .url() method call for the FCM
register endpoint.
In `@mobile/docs/_index.md`:
- Line 3: The `last_verified` field in both documentation files is set to a
future date (2026-06-16), which is after the current date (2026-06-15) and
creates inaccurate metadata. Update the `last_verified` field in
mobile/docs/_index.md at line 3 from 2026-06-16 to the actual verification date
(a non-future date). Similarly, update the `last_verified` field in
mobile/docs/fcm.md at line 3 from 2026-06-16 to the actual verification date (a
non-future date). Both updates should use the same date to reflect when these
docs were actually verified.
In `@web/docs/fcm.md`:
- Around line 19-27: Add the language identifier `env` to the opening fence of
the code block containing the Firebase environment variables in web/docs/fcm.md
(lines 19-27). Change the opening ``` to ```env to comply with the markdownlint
MD040 rule that requires fenced code blocks to have a language identifier.
In `@web/lib/fcm.ts`:
- Around line 25-37: The requestPermissionAndGetToken() function does not handle
errors from navigator.serviceWorker.register() and getToken() calls, which can
reject due to unsupported environments, invalid VAPID keys, or permission
failures, resulting in unhandled promise rejections. Wrap the service worker
registration and token generation logic in a try-catch block, catching any
errors from both the register() call and the getToken() call, and return null
when an error occurs to gracefully handle these failure cases.
- Around line 31-37: The service worker at `web/public/firebase-messaging-sw.js`
requires a `FIREBASE_CONFIG` message to initialize Firebase before
`firebase.messaging()` becomes usable, but the token retrieval logic in the
`getToken` call does not send this message. After registering the service worker
and before calling `getToken(messaging, ...)`, send a `FIREBASE_CONFIG` message
to all clients of the registered service worker, passing the Firebase app
configuration as the message payload. This ensures the service worker
initializes Firebase properly and can handle background messaging.
In `@web/lib/useFCM.ts`:
- Around line 14-44: Create a Vitest unit test file for the useFCM hook
(following the naming convention web/lib/useFCM.test.ts or similar) and add test
cases to verify the hook's behavior. The tests should cover: the FCM token
registration flow when requestPermissionAndGetToken succeeds and fails, proper
inclusion of the Authorization header when idToken is provided and omission when
it is not, the cleanup behavior of the returned function (setting cancelled flag
and calling unsub), and the onMessage handler registration when onMessage
callback is provided. Ensure all tests follow Vitest conventions and verify both
the async registration behavior and the effect cleanup mechanism of the useFCM
hook.
- Around line 36-43: The useEffect hook in the onForegroundMessage setup returns
early when onMessage is undefined, but this early return prevents the cleanup
function from running on unmount, leaving the cancelled flag unset and allowing
pending async work to continue after component unmount. Restructure the effect
to always return a cleanup function that sets cancelled = true, regardless of
whether onMessage is defined. The conditional check for onMessage should guard
the subscription setup (the onForegroundMessage call) but not prevent the
cleanup function from being returned.
- Around line 24-34: The fetch call to '/api/v1/fcm/register' in the
requestPermissionAndGetToken().then() callback does not validate the response
status or handle network/HTTP failures, nor does it enforce the auth-guard
requirement when idToken is missing. Add error handling to check the response
status (response.ok or similar) and handle failures appropriately, and only
proceed with the registration request when idToken is present, otherwise skip or
handle the missing token case.
In `@web/public/firebase-messaging-sw.js`:
- Around line 8-10: The firebase.initializeApp() call in the FIREBASE_CONFIG
event handler can throw a duplicate-app error if the message is received
multiple times. Guard the initialization by checking firebase.apps.length before
calling firebase.initializeApp(event.data.config). Only execute the
initializeApp() and firebase.messaging() calls when firebase.apps.length is 0,
ensuring the app is only initialized once.
- Around line 1-2: The importScripts calls in the service worker are loading
Firebase SDK version 11.0.0, but the main app depends on firebase@^12.14.0,
creating a version mismatch that can cause runtime integration failures. Update
both importScripts URLs in the firebase-app-compat.js and
firebase-messaging-compat.js lines to use a version from the 12.x series
(12.14.0 or later) to match your app's Firebase dependency version.
---
Outside diff comments:
In `@backend/docs/auth.md`:
- Around line 55-63: Update the `NewAuthClient` documentation in the auth.md
file to reflect the current API. The function signature should show it accepts a
context and a prebuilt Firebase app (firebasesdk.App) instead of projectID and
credentialsJSON parameters. Remove the documentation describing SDK
initialization with credentialsJSON and Application Default Credentials
fallback, as the Firebase app is now passed in as an already-initialized
dependency rather than being created within the function.
---
Nitpick comments:
In `@backend/docs/fcm.md`:
- Around line 84-90: The documentation example in the Firebase initialization
block is ignoring errors returned from firebase.NewApp(),
firebase.NewAuthClient(), and firebase.NewMessagingClient() by using the blank
identifier `_`. Replace the underscore error assignments with proper error
variable declarations and add error handling checks for each call. After each
function invocation, check if the error is not nil and handle it appropriately
(either by logging, returning the error up the stack, or exiting gracefully).
This ensures the documentation example follows the coding guideline of returning
errors up the stack rather than swallowing them, making it a better reference
for backend code patterns.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42f03e70-fb93-456e-a1ab-0352054bf2f4
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (36)
backend/docs/_index.mdbackend/docs/auth.mdbackend/docs/fcm.mdbackend/docs/swagger/docs.gobackend/docs/swagger/swagger.jsonbackend/docs/swagger/swagger.yamlbackend/internal/bootstrap/bootstrap.gobackend/internal/domain/fcm_token.gobackend/internal/infrastructure/database/migrations/20260615220942_add_fcm_tokens.sqlbackend/internal/infrastructure/database/postgres/fcm_token_repository.gobackend/internal/infrastructure/database/postgres/fcm_token_repository_test.gobackend/internal/server/server.gobackend/internal/transport/handlers/fcm_handler.gobackend/internal/transport/handlers/fcm_handler_test.gobackend/internal/transport/handlers/handler.gobackend/internal/transport/handlers/health_handler_test.gobackend/internal/transport/handlers/routes.gobackend/internal/usecase/notification.gobackend/pkg/firebase/admin.gobackend/pkg/firebase/app.gobackend/pkg/firebase/messaging.gomobile/app/build.gradle.ktsmobile/app/src/main/AndroidManifest.xmlmobile/app/src/main/java/com/company/template/fcm/FcmRegistrationPayload.ktmobile/app/src/main/java/com/company/template/fcm/MyFirebaseMessagingService.ktmobile/app/src/test/java/com/company/template/fcm/FcmRegistrationPayloadTest.ktmobile/docs/_index.mdmobile/docs/fcm.mdmobile/gradle/libs.versions.tomlweb/docs/_index.mdweb/docs/fcm.mdweb/lib/fcm.test.tsweb/lib/fcm.tsweb/lib/useFCM.tsweb/package.jsonweb/public/firebase-messaging-sw.js
| @@ -0,0 +1,174 @@ | |||
| --- | |||
| topic: fcm | |||
| last_verified: 2026-06-16 | |||
There was a problem hiding this comment.
Fix last_verified to a non-future date.
Line 3 is 2026-06-16, which is in the future relative to June 15, 2026; please set it to the actual verification date.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/docs/fcm.md` at line 3, The last_verified field in
backend/docs/fcm.md on line 3 contains a future date of 2026-06-16 instead of
the actual verification date. Update the last_verified value to reflect the
actual date when the documentation was last verified, ensuring it is not a
future date relative to today.
Comment on lines
+2
to
+8
| CREATE TABLE IF NOT EXISTS fcm_tokens ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| user_id TEXT NOT NULL, | ||
| token TEXT NOT NULL UNIQUE, | ||
| platform TEXT NOT NULL, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT now() | ||
| ); |
There was a problem hiding this comment.
Enforce platform enum at the schema layer.
platform is unconstrained TEXT, so invalid values can be persisted outside handler validation. Add a CHECK constraint to protect storage integrity.
Proposed fix
CREATE TABLE IF NOT EXISTS fcm_tokens (
id UUID PRIMARY KEY DEFAULT gen_random_uuid(),
user_id TEXT NOT NULL,
token TEXT NOT NULL UNIQUE,
- platform TEXT NOT NULL,
+ platform TEXT NOT NULL CHECK (platform IN ('android', 'ios', 'web')),
created_at TIMESTAMPTZ NOT NULL DEFAULT now()
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/internal/infrastructure/database/migrations/20260615220942_add_fcm_tokens.sql`
around lines 2 - 8, The `platform` column in the fcm_tokens table is defined as
unconstrained TEXT, allowing invalid values to be persisted in the database. Add
a CHECK constraint to the `platform` column definition in the CREATE TABLE
fcm_tokens statement to restrict it to only valid platform enum values (such as
'ios', 'android', and 'web'). This enforces data integrity at the schema layer
and prevents invalid platform values from being stored regardless of
handler-level validation.
Comment on lines
+60
to
+61
| tokensA, _ := repo.GetTokensByUserID(ctx, "userA") | ||
| tokensB, _ := repo.GetTokensByUserID(ctx, "userB") |
There was a problem hiding this comment.
Stop swallowing repository errors in tests.
Lines 60–61, 75, and 80 ignore returned errors, which can hide actual integration failures and make tests pass incorrectly.
As per coding guidelines, “Return errors up the stack — never swallow them”.
Proposed fix
- tokensA, _ := repo.GetTokensByUserID(ctx, "userA")
- tokensB, _ := repo.GetTokensByUserID(ctx, "userB")
+ tokensA, err := repo.GetTokensByUserID(ctx, "userA")
+ if err != nil {
+ t.Fatalf("GetTokensByUserID userA: %v", err)
+ }
+ tokensB, err := repo.GetTokensByUserID(ctx, "userB")
+ if err != nil {
+ t.Fatalf("GetTokensByUserID userB: %v", err)
+ }
@@
- _ = repo.SaveToken(ctx, "user2", "tok-del", "ios")
+ if err := repo.SaveToken(ctx, "user2", "tok-del", "ios"); err != nil {
+ t.Fatalf("SaveToken: %v", err)
+ }
@@
- tokens, _ := repo.GetTokensByUserID(ctx, "user2")
+ tokens, err := repo.GetTokensByUserID(ctx, "user2")
+ if err != nil {
+ t.Fatalf("GetTokensByUserID: %v", err)
+ }Also applies to: 75-75, 80-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/internal/infrastructure/database/postgres/fcm_token_repository_test.go`
around lines 60 - 61, Remove the blank identifier (_) that ignores errors
returned by GetTokensByUserID method calls. At each affected site in the
fcm_token_repository_test.go file—lines 60–61 (anchor), lines 75 (sibling), and
lines 80 (sibling)—capture the error return value and check it explicitly using
the test helper (such as t.Fatalf or require.NoError) to fail the test if an
error occurs. This ensures that integration failures in repository calls are not
masked and the test properly validates the behavior.
Source: Coding guidelines
|
|
||
| healthRepo := postgres.NewHealthRepository(app.DB) | ||
| healthUC := usecase.NewHealthUseCase(healthRepo) | ||
| fcmTokenRepo := postgres.NewFCMTokenRepository(app.DB) |
There was a problem hiding this comment.
FCM route gating is effectively disabled by unconditional repo wiring.
Line 37 always creates fcmTokenRepo, and route registration is keyed off fcmTokenRepo != nil; this exposes FCM routes even when Firebase isn’t configured. Gate repo creation on Firebase availability before passing it at Line 55.
Suggested patch
- fcmTokenRepo := postgres.NewFCMTokenRepository(app.DB)
+ var fcmTokenRepo usecase.FCMTokenRepository
+ if app.Firebase != nil {
+ fcmTokenRepo = postgres.NewFCMTokenRepository(app.DB)
+ }
h := handlers.NewHandler(healthUC, app.Firebase, hub, app.Enqueuer, queueUI, app.FCMSender, fcmTokenRepo)Also applies to: 55-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/server/server.go` at line 37, The FCM route gating mechanism
is ineffective because fcmTokenRepo is unconditionally created at line 37 via
postgres.NewFCMTokenRepository regardless of Firebase configuration, and the nil
check at line 55 cannot properly gate the routes. Move the creation of
fcmTokenRepo inside a conditional block that checks whether Firebase is
available, so the repo is only instantiated when Firebase is actually
configured, then pass this conditionally-created fcmTokenRepo to the route
registration at line 55.
Comment on lines
+72
to
+85
| val, _ := c.Get(middleware.FirebaseClaimsKey) | ||
| if _, ok := val.(*usecase.FirebaseToken); !ok { | ||
| c.JSON(http.StatusUnauthorized, gin.H{"error": "unauthorized"}) | ||
| return | ||
| } | ||
|
|
||
| var req unregisterFCMTokenRequest | ||
| if err := c.ShouldBindJSON(&req); err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) | ||
| return | ||
| } | ||
|
|
||
| if err := h.fcmTokenRepo.DeleteToken(c.Request.Context(), req.Token); err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to remove token"}) |
There was a problem hiding this comment.
Bind unregister to the authenticated user, not token alone.
On Line 84, deletion is scoped only by token; the authenticated user from Line 73 is never used. That allows cross-user token revocation if a token is exposed. Change the contract and query to delete by both user_id and token.
Suggested fix direction
- if err := h.fcmTokenRepo.DeleteToken(c.Request.Context(), req.Token); err != nil {
+ if err := h.fcmTokenRepo.DeleteTokenForUser(c.Request.Context(), claims.UID, req.Token); err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to remove token"})
return
}And update the repository implementation/query to enforce:
DELETE FROM fcm_tokens WHERE user_id = $1 AND token = $2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/transport/handlers/fcm_handler.go` around lines 72 - 85, The
unregisterFCMTokenRequest handler extracts the authenticated FirebaseToken from
the context but fails to use it when deleting the FCM token, creating a security
vulnerability where any exposed token can be revoked by another user. Extract
the user ID from the authenticated token (the val variable that is cast to
usecase.FirebaseToken at line 73), and pass both the user ID and the token to
the h.fcmTokenRepo.DeleteToken method call. Update the repository's DeleteToken
method signature to accept both user_id and token parameters, and modify the
underlying SQL query to enforce deletion only when both the user_id and token
match (DELETE FROM fcm_tokens WHERE user_id = $1 AND token = $2).
Comment on lines
+14
to
+44
| export function useFCM({ | ||
| idToken, | ||
| onMessage, | ||
| }: { | ||
| idToken?: string | ||
| onMessage?: (payload: MessagePayload) => void | ||
| } = {}): void { | ||
| useEffect(() => { | ||
| let cancelled = false | ||
|
|
||
| requestPermissionAndGetToken().then(async (token) => { | ||
| if (!token || cancelled) return | ||
| await fetch('/api/v1/fcm/register', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| ...(idToken ? { Authorization: `Bearer ${idToken}` } : {}), | ||
| }, | ||
| body: JSON.stringify({ token, platform: 'web' }), | ||
| }) | ||
| }) | ||
|
|
||
| if (!onMessage) return | ||
| const unsub = onForegroundMessage(onMessage) | ||
| return () => { | ||
| cancelled = true | ||
| unsub() | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [idToken]) | ||
| } |
There was a problem hiding this comment.
Add Vitest unit tests for useFCM hook.
This new lib/ utility hook currently has no dedicated unit test file in the provided changes, leaving registration + cleanup behavior unverified.
As per coding guidelines, “web/lib//*.{ts,js}: All functions in lib/ must have Vitest unit tests” and “web/lib//*.{ts,tsx}: New utility functions in Next.js lib/ must have Vitest unit tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/lib/useFCM.ts` around lines 14 - 44, Create a Vitest unit test file for
the useFCM hook (following the naming convention web/lib/useFCM.test.ts or
similar) and add test cases to verify the hook's behavior. The tests should
cover: the FCM token registration flow when requestPermissionAndGetToken
succeeds and fails, proper inclusion of the Authorization header when idToken is
provided and omission when it is not, the cleanup behavior of the returned
function (setting cancelled flag and calling unsub), and the onMessage handler
registration when onMessage callback is provided. Ensure all tests follow Vitest
conventions and verify both the async registration behavior and the effect
cleanup mechanism of the useFCM hook.
Source: Coding guidelines
Comment on lines
+24
to
+34
| requestPermissionAndGetToken().then(async (token) => { | ||
| if (!token || cancelled) return | ||
| await fetch('/api/v1/fcm/register', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| ...(idToken ? { Authorization: `Bearer ${idToken}` } : {}), | ||
| }, | ||
| body: JSON.stringify({ token, platform: 'web' }), | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Handle registration failures and non-2xx responses in the hook path.
This code fires a POST and ignores both network failures and HTTP failures. Also, when idToken is absent, it still sends an auth-guarded registration request in this PR’s backend contract.
Suggested fix
- requestPermissionAndGetToken().then(async (token) => {
- if (!token || cancelled) return
- await fetch('/api/v1/fcm/register', {
- method: 'POST',
- headers: {
- 'Content-Type': 'application/json',
- ...(idToken ? { Authorization: `Bearer ${idToken}` } : {}),
- },
- body: JSON.stringify({ token, platform: 'web' }),
- })
- })
+ requestPermissionAndGetToken().then(async (token) => {
+ if (!token || cancelled || !idToken) return
+ try {
+ const res = await fetch('/api/v1/fcm/register', {
+ method: 'POST',
+ headers: {
+ 'Content-Type': 'application/json',
+ Authorization: `Bearer ${idToken}`,
+ },
+ body: JSON.stringify({ token, platform: 'web' }),
+ })
+ if (!res.ok) {
+ // optional: surface or log failure for observability
+ }
+ } catch {
+ // optional: surface or log failure for observability
+ }
+ })📝 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.
Suggested change
| requestPermissionAndGetToken().then(async (token) => { | |
| if (!token || cancelled) return | |
| await fetch('/api/v1/fcm/register', { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| ...(idToken ? { Authorization: `Bearer ${idToken}` } : {}), | |
| }, | |
| body: JSON.stringify({ token, platform: 'web' }), | |
| }) | |
| }) | |
| requestPermissionAndGetToken().then(async (token) => { | |
| if (!token || cancelled || !idToken) return | |
| try { | |
| const res = await fetch('/api/v1/fcm/register', { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${idToken}`, | |
| }, | |
| body: JSON.stringify({ token, platform: 'web' }), | |
| }) | |
| if (!res.ok) { | |
| // optional: surface or log failure for observability | |
| } | |
| } catch { | |
| // optional: surface or log failure for observability | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/lib/useFCM.ts` around lines 24 - 34, The fetch call to
'/api/v1/fcm/register' in the requestPermissionAndGetToken().then() callback
does not validate the response status or handle network/HTTP failures, nor does
it enforce the auth-guard requirement when idToken is missing. Add error
handling to check the response status (response.ok or similar) and handle
failures appropriately, and only proceed with the registration request when
idToken is present, otherwise skip or handle the missing token case.
Comment on lines
+36
to
+43
| if (!onMessage) return | ||
| const unsub = onForegroundMessage(onMessage) | ||
| return () => { | ||
| cancelled = true | ||
| unsub() | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [idToken]) |
There was a problem hiding this comment.
Always return cleanup so pending async work can be cancelled on unmount.
When onMessage is undefined, the effect returns early and never sets cancelled = true on unmount. The async token flow can continue after unmount.
Suggested fix
- if (!onMessage) return
- const unsub = onForegroundMessage(onMessage)
+ const unsub = onMessage ? onForegroundMessage(onMessage) : undefined
return () => {
cancelled = true
- unsub()
+ unsub?.()
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/lib/useFCM.ts` around lines 36 - 43, The useEffect hook in the
onForegroundMessage setup returns early when onMessage is undefined, but this
early return prevents the cleanup function from running on unmount, leaving the
cancelled flag unset and allowing pending async work to continue after component
unmount. Restructure the effect to always return a cleanup function that sets
cancelled = true, regardless of whether onMessage is defined. The conditional
check for onMessage should guard the subscription setup (the onForegroundMessage
call) but not prevent the cleanup function from being returned.
Comment on lines
+1
to
+2
| importScripts('https://www.gstatic.com/firebasejs/11.0.0/firebase-app-compat.js') | ||
| importScripts('https://www.gstatic.com/firebasejs/11.0.0/firebase-messaging-compat.js') |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Firebase Web SDK 12.x apps, is it officially supported to load service worker compat scripts from 11.0.0 (firebase-app-compat.js and firebase-messaging-compat.js), or should the worker use matching 12.x versions?
💡 Result:
For Firebase Web SDK 12.x applications, you should use the matching 12.x versions of the compatibility (compat) scripts in your service worker, rather than version 11.0.0 [1][2][3]. Official Firebase documentation consistently advises that when using compat scripts (such as firebase-app-compat.js and firebase-messaging-compat.js) in a service worker, you must replace the version number in the CDN URL with the specific version of the Firebase JS SDK you are using in your main application [1]. Using mismatched versions between your main app and your service worker can lead to unexpected behavior or integration failures, as the underlying modular code and compatibility layers are frequently updated in tandem [4][5][6]. To ensure stability and proper functionality, verify that the scripts imported via importScripts in your firebase-messaging-sw.js (or equivalent service worker file) match the version of the Firebase SDK installed in your project [1].
Citations:
- 1: https://firebase.google.com/docs/web/best-practices
- 2: https://hideme.live/proxy/firebase.google.com/docs/web/modular-upgrade
- 3: https://93b2c18dx6669726562617365x676f6f676c65x636f6d.gateway.web.tr/https/docs/web/modular-upgrade
- 4: firebase/firebase-js-sdk@225004b
- 5: firebase/firebase-js-sdk@f6b4095
- 6: firebase/firebase-js-sdk@62a4653
🏁 Script executed:
find . -type f -name "package.json" | head -5Repository: GRACENOBLE/fullstack-template
Length of output: 92
🏁 Script executed:
cat ./web/package.json | jq '.dependencies | select(.firebase != null) | .firebase'Repository: GRACENOBLE/fullstack-template
Length of output: 84
Update service worker Firebase SDK to match app dependency version.
The web app imports firebase@^12.14.0 but the service worker loads compat scripts from 11.0.0. Per Firebase documentation, the compat script versions in the service worker must match the version of the Firebase SDK installed in your main app. Mismatched versions can lead to unexpected behavior or integration failures at runtime.
Update lines 1–2 to use 11.14.0 or later from the 12.x series:
Suggested change
-importScripts('https://www.gstatic.com/firebasejs/11.0.0/firebase-app-compat.js')
-importScripts('https://www.gstatic.com/firebasejs/11.0.0/firebase-messaging-compat.js')
+importScripts('https://www.gstatic.com/firebasejs/12.14.0/firebase-app-compat.js')
+importScripts('https://www.gstatic.com/firebasejs/12.14.0/firebase-messaging-compat.js')📝 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.
Suggested change
| importScripts('https://www.gstatic.com/firebasejs/11.0.0/firebase-app-compat.js') | |
| importScripts('https://www.gstatic.com/firebasejs/11.0.0/firebase-messaging-compat.js') | |
| importScripts('https://www.gstatic.com/firebasejs/12.14.0/firebase-app-compat.js') | |
| importScripts('https://www.gstatic.com/firebasejs/12.14.0/firebase-messaging-compat.js') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/public/firebase-messaging-sw.js` around lines 1 - 2, The importScripts
calls in the service worker are loading Firebase SDK version 11.0.0, but the
main app depends on firebase@^12.14.0, creating a version mismatch that can
cause runtime integration failures. Update both importScripts URLs in the
firebase-app-compat.js and firebase-messaging-compat.js lines to use a version
from the 12.x series (12.14.0 or later) to match your app's Firebase dependency
version.
Comment on lines
+8
to
+10
| if (event.data?.type === 'FIREBASE_CONFIG') { | ||
| firebase.initializeApp(event.data.config) | ||
| firebase.messaging() |
There was a problem hiding this comment.
Prevent duplicate Firebase app initialization in the worker.
If FIREBASE_CONFIG is posted more than once, firebase.initializeApp(...) can throw app/duplicate-app. Guard initialization with firebase.apps.length.
Suggested fix
self.addEventListener('message', (event) => {
if (event.data?.type === 'FIREBASE_CONFIG') {
- firebase.initializeApp(event.data.config)
+ if (!firebase.apps.length) {
+ firebase.initializeApp(event.data.config)
+ }
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.
Suggested change
| if (event.data?.type === 'FIREBASE_CONFIG') { | |
| firebase.initializeApp(event.data.config) | |
| firebase.messaging() | |
| self.addEventListener('message', (event) => { | |
| if (event.data?.type === 'FIREBASE_CONFIG') { | |
| if (!firebase.apps.length) { | |
| firebase.initializeApp(event.data.config) | |
| } | |
| firebase.messaging() | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/public/firebase-messaging-sw.js` around lines 8 - 10, The
firebase.initializeApp() call in the FIREBASE_CONFIG event handler can throw a
duplicate-app error if the message is received multiple times. Guard the
initialization by checking firebase.apps.length before calling
firebase.initializeApp(event.data.config). Only execute the initializeApp() and
firebase.messaging() calls when firebase.apps.length is 0, ensuring the app is
only initialized once.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NotificationSender+FCMTokenRepositoryinterfaces,fcm_tokensmigration, FCM token repository (with upsert),POST /api/v1/fcm/registerandDELETE /api/v1/fcm/unregisterendpoints, FCM sender via Admin SDK HTTP v1. Refactoredpkg/firebaseto share a singlefirebasesdk.Appbetween auth and messaging clients.lib/fcm.ts(permission + token),lib/useFCM.tshook (registers token with backend),public/firebase-messaging-sw.jsservice worker for background messages. Firebase JS SDK installed.MyFirebaseMessagingServicehandlesonNewToken(POSTs to backend) andonMessageReceived(showsNotificationCompatnotification).FcmRegistrationPayloadserialisable data class. Manifest updated withPOST_NOTIFICATIONSpermission and service registration.backend/docs/auth.md, createdfcm.mdin all three layers, updated all three_index.mdfiles.Test plan
cd backend && go vet ./... && make test— all pass including 4 FCM repository integration tests (Testcontainers) and 5 FCM handler unit testscd web && pnpm lint && pnpm build && pnpm test— 4 FCM Vitest unit tests pass; no TypeScript errorscd mobile && ./gradlew lint && ./gradlew test— 4FcmRegistrationPayloadTestJUnit tests pass; lint cleancd backend && make migrate-upFIREBASE_PROJECT_ID+FIREBASE_SERVICE_ACCOUNT_JSONinbackend/.envand setNEXT_PUBLIC_FIREBASE_*vars inweb/.env.localto test end-to-end token registrationNotes
h.fcmTokenRepo != nil(always true after this PR sinceNewFCMTokenRepositoryis always called inserver.go) — auth is enforced viaFirebaseAuthmiddleware inherited from the/api/v1groupgoogle-services.json+com.google.gms.google-servicesGradle plugin must be added for Firebase to initialise at runtime (noted inmobile/docs/fcm.md)postMessage; the caller must send{ type: 'FIREBASE_CONFIG', config }after registering the workerCloses #19
Summary by CodeRabbit
New Features
Documentation