Conversation
📝 WalkthroughWalkthroughThe PR introduces a new notification-trigger service for dispatching push notifications via APNS and FCM, adds debug logging to the signature validator, updates admin configuration entries, and applies minor styling improvements to control panel notification buttons. Changes
Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant Server as Notification Server
participant Detector as Platform Detector
participant APNS as APNS Provider
participant FCM as FCM Provider
Client->>Server: POST /api/send<br/>(token, optional platform, payload)
Server->>Server: Validate inputs
alt Platform not provided
Server->>Detector: detectPlatformFromToken(token)
Detector-->>Server: Platform ("ios" or "android")
else Platform provided
Server->>Server: Use provided platform
end
alt Platform is "ios"
Server->>APNS: sendApns(token, payload)
APNS->>APNS: Build notification with<br/>title, body, subtitle, sound, badge
APNS-->>Server: { success: boolean, error?: string }
else Platform is "android"
Server->>FCM: sendFcm(token, payload)
FCM->>FCM: Build message with<br/>notification & platform-specific fields
FCM-->>Server: { success: boolean, error?: string }
end
Server-->>Client: JSON response<br/>(success or error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
infrastructure/control-panel/src/routes/notifications/+page.svelte (1)
329-329: Inconsistent button styling: first button missingw-fit.The
w-fitclass was added to the "Send to eName" and "Send to all" buttons, but the first "Send" button (lines 262-270) doesn't have this class. Consider adding it for visual consistency across all action buttons.🎨 Suggested fix for consistency
<ButtonAction variant="solid" size="sm" + class="w-fit" isLoading={singleSending} blockingClick callback={sendSingle}Also applies to: 389-389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/control-panel/src/routes/notifications/`+page.svelte at line 329, The "Send" action button in +page.svelte is missing the w-fit utility class that the other action buttons ("Send to eName" and "Send to all") use; update the first "Send" button's class attribute to include w-fit so all three action buttons use the same width-fitting styling for visual consistency and predictable layout.notification-trigger/src/index.ts (2)
108-111: Add SIGINT handler for graceful local shutdown.SIGTERM handles container/orchestrator shutdowns, but local dev typically uses Ctrl+C (SIGINT). Consider handling both signals for consistent cleanup.
Suggested fix
-process.on("SIGTERM", () => { +const shutdown = () => { shutdownApns(); server.close(); -}); +}; + +process.on("SIGTERM", shutdown); +process.on("SIGINT", shutdown);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-trigger/src/index.ts` around lines 108 - 111, Add a SIGINT handler to perform the same graceful shutdown as SIGTERM: either register process.on("SIGINT", ...) calling shutdownApns() and server.close(), or factor the logic into a reusable gracefulShutdown() function and call that from both the SIGTERM and SIGINT handlers; reference shutdownApns and server.close (and the process.on registration) so the same cleanup sequence runs on Ctrl+C locally as on container SIGTERM.
98-100: Consider logging when provider initialization fails.The return values from
initApns()andinitFcm()are discarded. While they log internally, capturing and logging a summary here improves operator visibility at startup.Suggested improvement
// Initialize providers -initApns(); -initFcm(); +const apnsOk = initApns(); +const fcmOk = initFcm(); +if (!apnsOk && !fcmOk) { + console.warn("Warning: No notification providers configured"); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-trigger/src/index.ts` around lines 98 - 100, Capture and check the results returned by initApns() and initFcm() during startup and emit a concise startup-level log summarizing success/failure for each provider; update the startup code that currently calls initApns() and initFcm() to assign their return values (e.g., apnsResult, fcmResult), then call the existing logger to record a short status line like "APNS initialized: OK/ERROR" and "FCM initialized: OK/ERROR" including any returned error or summary info from those results so operators can see provider initialization outcomes at launch.notification-trigger/package.json (1)
9-9: Build script is not cross-platform.The
cp -rcommand won't work on Windows. Consider using a cross-platform alternative if Windows support is needed.Cross-platform options
Option 1: Use
copyfilespackage:"build": "tsc && copyfiles -u 1 public/**/* dist/"Option 2: Use
cpy-cli:"build": "tsc && cpy 'public/**/*' dist/"If this is strictly a Unix/container environment, the current script is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-trigger/package.json` at line 9, The "build" npm script in package.json uses the Unix-only "cp -r" command (script name: "build"), which fails on Windows; replace it with a cross-platform copy solution (e.g., install and use "copyfiles" or "cpy-cli") and update the "build" script to invoke that tool to copy the "public" assets into "dist", and add the chosen package as a devDependency so the script works consistently across platforms.notification-trigger/src/senders/index.ts (1)
1-3: Inconsistent import extension.Line 3 uses
.jsextension ("../types.js") while lines 1-2 omit extensions. For consistency with other imports in this file and the codebase, remove the.jssuffix.Suggested fix
import { sendApns } from "./apns"; import { sendFcm } from "./fcm"; -import type { NotificationPayload, Platform } from "../types.js"; +import type { NotificationPayload, Platform } from "../types";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-trigger/src/senders/index.ts` around lines 1 - 3, The import for types is using a .js extension and is inconsistent with the other imports; update the types import in this file by removing the ".js" suffix so it reads the same style as the other imports (change the import that brings in NotificationPayload and Platform from "../types.js" to "../types") and verify there are no other mismatched extensions in this module (references: sendApns, sendFcm, NotificationPayload, Platform).notification-trigger/src/senders/apns.ts (1)
19-29: MakeinitApns()idempotent.The
apndocs recommend keeping oneProviderper process. Right now a secondinitApns()call overwrites the existing provider without shutting it down first, which can leak connections/resources. (github.com)Suggested fix
export function initApns(): boolean { + if (provider) return true; + const keyPath = process.env.APNS_KEY_PATH; const keyId = process.env.APNS_KEY_ID; const teamId = process.env.APNS_TEAM_ID; const bundleId = process.env.APNS_BUNDLE_ID;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-trigger/src/senders/apns.ts` around lines 19 - 29, The initApns function currently overwrites the module-level provider without cleaning it up; make initApns idempotent by first checking the existing provider variable and returning true immediately if it's already initialized, or if you must recreate it call provider.shutdown() (or provider.close() per apn version) to cleanly close connections before assigning a new apn.Provider instance; update the initApns routine to use this check/shutdown around the new apn.Provider creation so only one Provider is active per process.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infrastructure/control-panel/config/admin-enames.json`:
- Around line 4-7: Remove the duplicate admin entry
"@82f7a77a-f03a-52aa-88fc-1b1e488ad498" from the JSON array in admin-enames.json
so each value is unique, and ensure the source matches the AI-generated summary;
the runtime de-duplication in allowlist.ts (which uses a Set) should remain
unchanged but the file itself must no longer contain repeated entries.
In `@infrastructure/signature-validator/src/index.ts`:
- Around line 333-341: Remove the sensitive console.log statements that print
eName, registryBaseUrl, signature, payload, and keyBindingCertificates in
index.ts; instead either delete them or replace with redacted/low-sensitivity
messages (e.g., log only the eName hashed/shortened or count of certificates)
and/or gate any detailed logs behind an explicit debug flag or logger level
check (use an existing logger or process.env.DEBUG/LOG_LEVEL) around the call to
getKeyBindingCertificates so raw payloads, signatures and JWTs are never emitted
in production.
In `@notification-trigger/src/index.ts`:
- Around line 30-36: The health endpoint currently checks only environment
variables (APNS_KEY_PATH, GOOGLE_APPLICATION_CREDENTIALS) in
app.get("/api/health") which can be misleading; change it to report actual
initialization status by introducing and exporting boolean flags (e.g.,
apnsInitialized, fcmInitialized) that are set by initApns() and the FCM init
routine (or have those init functions return success/failure), update initApns()
in apns.ts to set the flag to true only on successful init (false on error),
ensure FCM initialization similarly sets its flag, and modify the /api/health
handler to return apns: apnsInitialized and fcm: fcmInitialized instead of
relying on env vars.
In `@notification-trigger/src/senders/apns.ts`:
- Around line 44-56: The APNS Notification must set pushType for iOS 13+/watchOS
6+; update the construction of the apn.Notification (variable note in this
block) to set note.pushType = "alert" since this is a user-visible alert
notification, ensuring APNs accepts and delivers the message correctly.
In `@notification-trigger/src/senders/fcm.ts`:
- Around line 9-12: Remove the hard guard that checks
process.env.GOOGLE_APPLICATION_CREDENTIALS (the if block currently returning
false) so you don't short-circuit admin.credential.applicationDefault(); instead
call admin.initializeApp({ credential: admin.credential.applicationDefault() })
inside the existing try-catch in this module (senders/fcm.ts) and let the SDK
resolve credentials from ADC/metadata server; on error catch and log the failure
and return false as before.
In `@notification-trigger/src/types.ts`:
- Around line 18-19: The current optional field clickAction in types.ts
conflates iOS category, Android click action, and Android channel ID; split it
into platform-specific fields (e.g., iosCategory?: string, androidClickAction?:
string, androidChannelId?: string) and update the mapping in fcm.ts (where
clickAction is currently mapped to channelId) to use androidClickAction for the
Android notification Intent action and androidChannelId for the channelId
property; ensure any code referencing clickAction (types.ts, fcm.ts) is updated
to the new property names and keep backward-compatibility mapping or validation
if needed.
---
Nitpick comments:
In `@infrastructure/control-panel/src/routes/notifications/`+page.svelte:
- Line 329: The "Send" action button in +page.svelte is missing the w-fit
utility class that the other action buttons ("Send to eName" and "Send to all")
use; update the first "Send" button's class attribute to include w-fit so all
three action buttons use the same width-fitting styling for visual consistency
and predictable layout.
In `@notification-trigger/package.json`:
- Line 9: The "build" npm script in package.json uses the Unix-only "cp -r"
command (script name: "build"), which fails on Windows; replace it with a
cross-platform copy solution (e.g., install and use "copyfiles" or "cpy-cli")
and update the "build" script to invoke that tool to copy the "public" assets
into "dist", and add the chosen package as a devDependency so the script works
consistently across platforms.
In `@notification-trigger/src/index.ts`:
- Around line 108-111: Add a SIGINT handler to perform the same graceful
shutdown as SIGTERM: either register process.on("SIGINT", ...) calling
shutdownApns() and server.close(), or factor the logic into a reusable
gracefulShutdown() function and call that from both the SIGTERM and SIGINT
handlers; reference shutdownApns and server.close (and the process.on
registration) so the same cleanup sequence runs on Ctrl+C locally as on
container SIGTERM.
- Around line 98-100: Capture and check the results returned by initApns() and
initFcm() during startup and emit a concise startup-level log summarizing
success/failure for each provider; update the startup code that currently calls
initApns() and initFcm() to assign their return values (e.g., apnsResult,
fcmResult), then call the existing logger to record a short status line like
"APNS initialized: OK/ERROR" and "FCM initialized: OK/ERROR" including any
returned error or summary info from those results so operators can see provider
initialization outcomes at launch.
In `@notification-trigger/src/senders/apns.ts`:
- Around line 19-29: The initApns function currently overwrites the module-level
provider without cleaning it up; make initApns idempotent by first checking the
existing provider variable and returning true immediately if it's already
initialized, or if you must recreate it call provider.shutdown() (or
provider.close() per apn version) to cleanly close connections before assigning
a new apn.Provider instance; update the initApns routine to use this
check/shutdown around the new apn.Provider creation so only one Provider is
active per process.
In `@notification-trigger/src/senders/index.ts`:
- Around line 1-3: The import for types is using a .js extension and is
inconsistent with the other imports; update the types import in this file by
removing the ".js" suffix so it reads the same style as the other imports
(change the import that brings in NotificationPayload and Platform from
"../types.js" to "../types") and verify there are no other mismatched extensions
in this module (references: sendApns, sendFcm, NotificationPayload, Platform).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f088b8e0-a31f-4c5f-8f65-22b095106161
⛔ Files ignored due to path filters (2)
infrastructure/eid-wallet/src-tauri/gen/android/app/build.gradle.ktsis excluded by!**/gen/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
infrastructure/control-panel/config/admin-enames.jsoninfrastructure/control-panel/src/routes/notifications/+page.svelteinfrastructure/signature-validator/src/index.tsnotification-trigger/.env.examplenotification-trigger/.gitignorenotification-trigger/README.mdnotification-trigger/package.jsonnotification-trigger/public/index.htmlnotification-trigger/src/index.tsnotification-trigger/src/senders/apns.tsnotification-trigger/src/senders/fcm.tsnotification-trigger/src/senders/index.tsnotification-trigger/src/types.tsnotification-trigger/tsconfig.json
| "@82f7a77a-f03a-52aa-88fc-1b1e488ad498", | ||
| "@35a31f0d-dd76-5780-b383-29f219fcae99", | ||
| "@82f7a77a-f03a-52aa-88fc-1b1e488ad498" | ||
| "@82f7a77a-f03a-52aa-88fc-1b1e488ad498", | ||
| "@af7e4f55-ad9d-537c-81ef-4f3a234bdd2c" |
There was a problem hiding this comment.
Duplicate admin entry detected.
Lines 4 and 6 both contain @82f7a77a-f03a-52aa-88fc-1b1e488ad498. While allowlist.ts uses a Set which will deduplicate at runtime, keeping duplicates in the source JSON is confusing and error-prone for future maintenance.
Additionally, this contradicts the AI-generated summary which states the duplicate was removed.
,
🧹 Suggested fix to remove the duplicate
{
"admins": [
"@7218b67d-da21-54d6-9a85-7c4db1d09768",
"@82f7a77a-f03a-52aa-88fc-1b1e488ad498",
"@35a31f0d-dd76-5780-b383-29f219fcae99",
- "@82f7a77a-f03a-52aa-88fc-1b1e488ad498",
"@af7e4f55-ad9d-537c-81ef-4f3a234bdd2c"
]
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/control-panel/config/admin-enames.json` around lines 4 - 7,
Remove the duplicate admin entry "@82f7a77a-f03a-52aa-88fc-1b1e488ad498" from
the JSON array in admin-enames.json so each value is unique, and ensure the
source matches the AI-generated summary; the runtime de-duplication in
allowlist.ts (which uses a Set) should remain unchanged but the file itself must
no longer contain repeated entries.
| console.log("Verifying signature for eName:", eName); | ||
| console.log("Registry base URL:", registryBaseUrl); | ||
| console.log("Signature:", signature); | ||
| console.log("Payload:", payload); | ||
|
|
||
| // Get key binding certificates from eVault | ||
| const keyBindingCertificates = await getKeyBindingCertificates(eName, registryBaseUrl); | ||
|
|
||
| console.log("Key binding certificates:", keyBindingCertificates); |
There was a problem hiding this comment.
Remove or redact these debug logs.
This block writes user identifiers plus raw payload, signature, and key-binding JWTs to logs. That creates a real privacy/security leak in production and makes centralized logs a source of sensitive material. Please either delete these logs or gate them behind debug mode and only emit redacted metadata.
Safer logging shape
- console.log("Verifying signature for eName:", eName);
- console.log("Registry base URL:", registryBaseUrl);
- console.log("Signature:", signature);
- console.log("Payload:", payload);
+ console.debug("Verifying signature", {
+ eName,
+ registryBaseUrl,
+ signatureLength: signature.length,
+ payloadLength: payload.length,
+ });
...
- console.log("Key binding certificates:", keyBindingCertificates);
+ console.debug("Loaded key binding certificates", {
+ count: keyBindingCertificates.length,
+ });📝 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.
| console.log("Verifying signature for eName:", eName); | |
| console.log("Registry base URL:", registryBaseUrl); | |
| console.log("Signature:", signature); | |
| console.log("Payload:", payload); | |
| // Get key binding certificates from eVault | |
| const keyBindingCertificates = await getKeyBindingCertificates(eName, registryBaseUrl); | |
| console.log("Key binding certificates:", keyBindingCertificates); | |
| console.debug("Verifying signature", { | |
| eName, | |
| registryBaseUrl, | |
| signatureLength: signature.length, | |
| payloadLength: payload.length, | |
| }); | |
| // Get key binding certificates from eVault | |
| const keyBindingCertificates = await getKeyBindingCertificates(eName, registryBaseUrl); | |
| console.debug("Loaded key binding certificates", { | |
| count: keyBindingCertificates.length, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/signature-validator/src/index.ts` around lines 333 - 341,
Remove the sensitive console.log statements that print eName, registryBaseUrl,
signature, payload, and keyBindingCertificates in index.ts; instead either
delete them or replace with redacted/low-sensitivity messages (e.g., log only
the eName hashed/shortened or count of certificates) and/or gate any detailed
logs behind an explicit debug flag or logger level check (use an existing logger
or process.env.DEBUG/LOG_LEVEL) around the call to getKeyBindingCertificates so
raw payloads, signatures and JWTs are never emitted in production.
| app.get("/api/health", (_req: Request, res: Response) => { | ||
| res.json({ | ||
| ok: true, | ||
| apns: !!process.env.APNS_KEY_PATH, | ||
| fcm: !!process.env.GOOGLE_APPLICATION_CREDENTIALS, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Health endpoint may report incorrect provider availability.
The health check relies on environment variables (APNS_KEY_PATH, GOOGLE_APPLICATION_CREDENTIALS) but doesn't reflect actual initialization success. Per apns.ts:6-34, initApns() can fail even when env vars are set (e.g., invalid key file). Consider tracking init results for accurate health reporting.
Suggested approach
+let apnsReady = false;
+let fcmReady = false;
+
// Initialize providers
-initApns();
-initFcm();
+apnsReady = initApns();
+fcmReady = initFcm();
app.get("/api/health", (_req: Request, res: Response) => {
res.json({
ok: true,
- apns: !!process.env.APNS_KEY_PATH,
- fcm: !!process.env.GOOGLE_APPLICATION_CREDENTIALS,
+ apns: apnsReady,
+ fcm: fcmReady,
});
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@notification-trigger/src/index.ts` around lines 30 - 36, The health endpoint
currently checks only environment variables (APNS_KEY_PATH,
GOOGLE_APPLICATION_CREDENTIALS) in app.get("/api/health") which can be
misleading; change it to report actual initialization status by introducing and
exporting boolean flags (e.g., apnsInitialized, fcmInitialized) that are set by
initApns() and the FCM init routine (or have those init functions return
success/failure), update initApns() in apns.ts to set the flag to true only on
successful init (false on error), ensure FCM initialization similarly sets its
flag, and modify the /api/health handler to return apns: apnsInitialized and
fcm: fcmInitialized instead of relying on env vars.
| const note = new apn.Notification(); | ||
| note.alert = { | ||
| title: payload.title, | ||
| body: payload.body, | ||
| ...(payload.subtitle && { subtitle: payload.subtitle }), | ||
| }; | ||
| note.topic = process.env.APNS_BUNDLE_ID!; | ||
| note.sound = payload.sound ?? "default"; | ||
| if (payload.badge !== undefined) note.badge = payload.badge; | ||
| if (payload.clickAction) note.aps = { ...note.aps, category: payload.clickAction }; | ||
| if (payload.data && Object.keys(payload.data).length > 0) { | ||
| note.payload = payload.data; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the repository structure and locate the file
find . -type f -name "apns.ts" | head -20Repository: MetaState-Prototype-Project/prototype
Length of output: 123
🏁 Script executed:
# Check if the notification-trigger directory exists
ls -la notification-trigger/ 2>/dev/null || echo "Directory not found at root level"Repository: MetaState-Prototype-Project/prototype
Length of output: 616
🏁 Script executed:
# Let's also search for files containing apn references
fd -t f "apns" .Repository: MetaState-Prototype-Project/prototype
Length of output: 123
🏁 Script executed:
# Read the apns.ts file to see the full context
cat -n ./notification-trigger/src/senders/apns.tsRepository: MetaState-Prototype-Project/prototype
Length of output: 2798
🏁 Script executed:
# Check the package.json to see what version of apn is being used
cat ./notification-trigger/package.jsonRepository: MetaState-Prototype-Project/prototype
Length of output: 747
🌐 Web query:
node-apn pushType iOS 13 watchOS 6 requirement documentation
💡 Result:
In node-apn, the APNs apns-push-type HTTP/2 header is set via notification.pushType.
- What to set:
notification.pushTypemust be either"alert"(user-visible: alert/sound/badge) or"background"(silent/background update). If it doesn’t match the payload, APNs may delay or drop the notification. [1] - When it’s required: Required when delivering notifications to devices running iOS 13+ or watchOS 6+ (ignored on earlier OS versions). [1]
- Docs noting the requirement: Apple announced this as a requirement for pushes to iOS 13 and watchOS 6 devices, and services (e.g., AWS) documented it when updating their APNs integrations. [2] (See also common implementation guidance referencing Apple’s APNs docs.) [3]
node-apn reference: notification.pushType in doc/notification.markdown. [1]
Sources: node-apn documentation [1], AWS Messaging Blog [2], Stack Overflow (implementation guidance quoting Apple APNs requirements) [3].
Set pushType to alert for these APNS messages.
apn.Notification.pushType is required for iOS 13+/watchOS 6+, and missing or mismatched values will cause APNs to delay or drop delivery. This payload constructs a user-visible alert notification, so it must declare alert. (node-apn docs)
Suggested fix
const note = new apn.Notification();
note.alert = {
title: payload.title,
body: payload.body,
...(payload.subtitle && { subtitle: payload.subtitle }),
};
note.topic = process.env.APNS_BUNDLE_ID!;
+ note.pushType = "alert";
note.sound = payload.sound ?? "default";📝 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.
| const note = new apn.Notification(); | |
| note.alert = { | |
| title: payload.title, | |
| body: payload.body, | |
| ...(payload.subtitle && { subtitle: payload.subtitle }), | |
| }; | |
| note.topic = process.env.APNS_BUNDLE_ID!; | |
| note.sound = payload.sound ?? "default"; | |
| if (payload.badge !== undefined) note.badge = payload.badge; | |
| if (payload.clickAction) note.aps = { ...note.aps, category: payload.clickAction }; | |
| if (payload.data && Object.keys(payload.data).length > 0) { | |
| note.payload = payload.data; | |
| } | |
| const note = new apn.Notification(); | |
| note.alert = { | |
| title: payload.title, | |
| body: payload.body, | |
| ...(payload.subtitle && { subtitle: payload.subtitle }), | |
| }; | |
| note.topic = process.env.APNS_BUNDLE_ID!; | |
| note.pushType = "alert"; | |
| note.sound = payload.sound ?? "default"; | |
| if (payload.badge !== undefined) note.badge = payload.badge; | |
| if (payload.clickAction) note.aps = { ...note.aps, category: payload.clickAction }; | |
| if (payload.data && Object.keys(payload.data).length > 0) { | |
| note.payload = payload.data; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@notification-trigger/src/senders/apns.ts` around lines 44 - 56, The APNS
Notification must set pushType for iOS 13+/watchOS 6+; update the construction
of the apn.Notification (variable note in this block) to set note.pushType =
"alert" since this is a user-visible alert notification, ensuring APNs accepts
and delivers the message correctly.
| if (!process.env.GOOGLE_APPLICATION_CREDENTIALS) { | ||
| console.warn("[FCM] Missing GOOGLE_APPLICATION_CREDENTIALS"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the FCM initialization file
find . -type f -name "fcm.ts" 2>/dev/null | head -20Repository: MetaState-Prototype-Project/prototype
Length of output: 122
🏁 Script executed:
# Check the file structure around notification-trigger
git ls-files | grep -E "(notification-trigger|fcm)" | head -20Repository: MetaState-Prototype-Project/prototype
Length of output: 476
🏁 Script executed:
# Look for package.json to verify firebase-admin version
find . -type f -name "package.json" | xargs grep -l "firebase-admin" 2>/dev/nullRepository: MetaState-Prototype-Project/prototype
Length of output: 302
🏁 Script executed:
cat -n notification-trigger/src/senders/fcm.tsRepository: MetaState-Prototype-Project/prototype
Length of output: 2665
🏁 Script executed:
cat notification-trigger/package.jsonRepository: MetaState-Prototype-Project/prototype
Length of output: 747
🌐 Web query:
firebase-admin applicationDefault credentials behavior
💡 Result:
admin.credential.applicationDefault() (Firebase Admin SDK) creates a credential from Google Application Default Credentials (ADC) and uses it to mint OAuth2 access tokens for Firebase/Google APIs. [1]
Where the credentials come from (ADC resolution order)
When Firebase Admin uses ADC, credentials are discovered in this order: [2]
- The file pointed to by
GOOGLE_APPLICATION_CREDENTIALS - The “well-known” local ADC file created by
gcloud auth application-default login - The attached service account from the Google Cloud metadata server (when running on GCP)
What happens if you don’t pass credential at all
If you initialize with no explicit credential (for example, initializeApp() in Node / FirebaseApp.initializeApp() in Java / firebase_admin.initialize_app() in Python), the Admin SDK uses ADC automatically. [3]
Related env var: FIREBASE_CONFIG
If you need extra app options (like projectId, databaseURL, storageBucket, etc.), Admin SDK can load them from the FIREBASE_CONFIG environment variable (either inline JSON or a path to a JSON file). [3]
Common local-dev gotcha (Firebase Auth + gcloud ADC)
When using local ADC from gcloud auth application-default login, Firebase docs note Firebase Authentication needs extra setup because:
- Firebase Auth doesn’t accept gcloud “end user” credentials generated with the gcloud OAuth client ID
- Firebase Auth also requires
projectIdto be provided at initialization for these end-user credentials [3]
🌐 Web query:
Google Application Default Credentials ADC environment variables support
💡 Result:
Environment variables commonly supported by Google Application Default Credentials (ADC) (as implemented by Google auth libraries / Cloud client libraries):
GOOGLE_APPLICATION_CREDENTIALS– Primary ADC override: path to an ADC JSON file (service account key JSON, or workload identity federation / “external account” configuration). If set, ADC loads credentials from this file. [1][2]GOOGLE_CLOUD_PROJECT– Overrides/sets the default project ID used by client libraries’ project auto-detection. [3][4]GCLOUD_PROJECT– Legacy default project ID env var (still used in some environments, e.g., certain App Engine cases). [5]GOOGLE_CLOUD_QUOTA_PROJECT– Overrides the “quota/billing project” used for quota and billing attribution (when supported by the library). [6][7]CLOUDSDK_CONFIG– Points to an alternate Google Cloud SDK config directory (affects where local ADC fromgcloud auth application-default loginis discovered). [5]GCE_METADATA_ROOT/GCE_METADATA_IP– Metadata server override knobs used by the auth library for GCE-style environments (Compute Engine / Cloud Run, etc.). [5]
Sources: Google Cloud ADC docs [1], google-auth library reference [2][5], google-auth user guide [4], Google AIP-4110 (ADC requirements, incl. quota env var) [6], Google Cloud quotas docs [7].
Remove the hard dependency on GOOGLE_APPLICATION_CREDENTIALS.
admin.credential.applicationDefault() resolves credentials in this order: (1) the file at GOOGLE_APPLICATION_CREDENTIALS, (2) local ADC from gcloud auth application-default login, and (3) the metadata server when running on GCP. This guard blocks valid deployments on Google Cloud runtimes that rely on metadata server credentials or local ADC before the SDK even attempts initialization. Let the try-catch handle credential failures instead.
Suggested fix
export function initFcm(): boolean {
if (initialized) return true;
-
- if (!process.env.GOOGLE_APPLICATION_CREDENTIALS) {
- console.warn("[FCM] Missing GOOGLE_APPLICATION_CREDENTIALS");
- return false;
- }
try {
admin.initializeApp({
credential: admin.credential.applicationDefault(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@notification-trigger/src/senders/fcm.ts` around lines 9 - 12, Remove the hard
guard that checks process.env.GOOGLE_APPLICATION_CREDENTIALS (the if block
currently returning false) so you don't short-circuit
admin.credential.applicationDefault(); instead call admin.initializeApp({
credential: admin.credential.applicationDefault() }) inside the existing
try-catch in this module (senders/fcm.ts) and let the SDK resolve credentials
from ADC/metadata server; on error catch and log the failure and return false as
before.
| /** Click action / category (iOS) / channel (Android) */ | ||
| clickAction?: string; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "types.ts" | grep notification-triggerRepository: MetaState-Prototype-Project/prototype
Length of output: 116
🏁 Script executed:
find . -name "fcm.ts" | grep notification-triggerRepository: MetaState-Prototype-Project/prototype
Length of output: 122
🏁 Script executed:
git ls-files | grep notification-triggerRepository: MetaState-Prototype-Project/prototype
Length of output: 476
🏁 Script executed:
cat -n notification-trigger/src/types.tsRepository: MetaState-Prototype-Project/prototype
Length of output: 1571
🏁 Script executed:
cat -n notification-trigger/src/senders/fcm.tsRepository: MetaState-Prototype-Project/prototype
Length of output: 2665
🌐 Web query:
Firebase Admin SDK Android notification clickAction vs channelId properties
💡 Result:
In the Firebase Admin SDK, clickAction and channelId configure different parts of how Android handles an FCM notification:
AndroidNotification.clickAction: Controls what happens when the user taps the notification. If set, Android will try to launch an Activity whose<intent-filter>matches that action. [1][2]AndroidNotification.channelId: Controls which Android notification channel (Android 8.0/O+) the notification is posted to. Your app must have created that channel ID beforehand; if you omit it (or the channel doesn’t exist), FCM falls back to the channel ID declared in your app manifest. [1]
They’re independent: channelId = delivery/appearance settings bucket, clickAction = tap behavior. [1][2]
Sources:
[1] Firebase Admin SDK AndroidNotification reference (properties channelId, clickAction) (firebase.google.com)
[2] Android client SDK RemoteMessage.Notification (getClickAction(), getChannelId()) (firebase.google.com)
Citations:
- 1: https://firebase.google.com/docs/reference/admin/node/firebase-admin.messaging.androidnotification
- 2: https://firebase.google.com/docs/reference/android/com/google/firebase/messaging/RemoteMessage.Notification
🌐 Web query:
Firebase Admin Node.js AndroidNotification API reference channelId clickAction
💡 Result:
In the Firebase Admin SDK for Node.js, both channelId and clickAction are fields on admin.messaging.AndroidNotification:
channelId?: string— Sets the Android notification channel ID (Android 8.0/O+). Your app must create this channel before posting notifications to it. If you omit it (or the channel doesn’t exist), FCM falls back to the channel ID declared as the default in your app manifest. [1]clickAction?: string— Sets the Intent action to fire when the user taps the notification. If set, Android will launch anActivitythat has anintent-filtermatching that action. [1][2]
Example (Admin SDK / Node.js)
import admin from "firebase-admin";
await admin.messaging().send({
token,
notification: { title: "Hello", body: "Tap me" },
android: {
notification: {
channelId: "news", // must exist in the app (NotificationChannel)
clickAction: "OPEN_NEWS_ACTIVITY", // must match an <intent-filter> action
},
},
});Client-side note (Android)
On the receiving device, the notification payload exposes these values via:
RemoteMessage.Notification.getChannelId()RemoteMessage.Notification.getClickAction()[2]
Sources: Firebase Admin SDK Node.js AndroidNotification reference [1], Firebase Android SDK RemoteMessage.Notification reference [2].
References
[1] Firebase Admin SDK (Node.js) — AndroidNotification interface
[2] Firebase Android SDK — RemoteMessage.Notification (getChannelId, getClickAction)
Split this into platform-specific fields.
Firebase Admin exposes Android clickAction and channelId as distinct properties with different behavior: clickAction sets the Intent action that fires when the notification is tapped, while channelId specifies which Android notification channel the notification is posted to (Android 8.0+). The current single clickAction field is mapped to channelId at line 53 of fcm.ts, which means Android's actual click action Intent is never set. The shared field cannot accurately represent iOS category, Android channel ID, and Android click action simultaneously, so the API is forcing the sender to drop the Android click action semantics.
Suggested direction
export interface NotificationPayload {
/** Notification title */
title: string;
/** Notification body text */
body: string;
/** Optional subtitle (iOS) / subtext (Android) */
subtitle?: string;
/** Custom data payload - string values only for FCM compatibility */
data?: Record<string, string>;
/** Sound name - "default" or custom */
sound?: string;
/** Badge count (iOS) */
badge?: number;
- /** Click action / category (iOS) / channel (Android) */
- clickAction?: string;
+ /** APNS category identifier */
+ iosCategory?: string;
+ /** Android notification channel ID */
+ androidChannelId?: string;
+ /** Android click action intent */
+ androidClickAction?: string;
}📝 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.
| /** Click action / category (iOS) / channel (Android) */ | |
| clickAction?: string; | |
| /** APNS category identifier */ | |
| iosCategory?: string; | |
| /** Android notification channel ID */ | |
| androidChannelId?: string; | |
| /** Android click action intent */ | |
| androidClickAction?: string; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@notification-trigger/src/types.ts` around lines 18 - 19, The current optional
field clickAction in types.ts conflates iOS category, Android click action, and
Android channel ID; split it into platform-specific fields (e.g., iosCategory?:
string, androidClickAction?: string, androidChannelId?: string) and update the
mapping in fcm.ts (where clickAction is currently mapped to channelId) to use
androidClickAction for the Android notification Intent action and
androidChannelId for the channelId property; ensure any code referencing
clickAction (types.ts, fcm.ts) is updated to the new property names and keep
backward-compatibility mapping or validation if needed.
1 participant
Description of change
readds missing segregated notification trigger service
Issue Number
n/a
Type of change
How the change has been tested
manual
Change checklist
Summary by CodeRabbit
New Features
Chores