Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSwaps Tauri notification plugins, renames mobile permission, adds client push-token retrieval and onboarding UI, registers devices on app startup, introduces notification-trigger service (APNs/FCM) plus sender modules, adds DeviceToken entity/service and migration, and implements control-panel notification UI and server endpoints. New env vars and docs added. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Onboarding as Onboarding Page
participant NotifService as NotificationService (client)
participant PluginAPI as Tauri Plugin API
participant Backend as Evault Backend
participant Trigger as Notification Trigger (APNs/FCM)
User->>Onboarding: Open onboarding page
Onboarding->>NotifService: getPushToken()
NotifService->>PluginAPI: request notification permission & registerForPushNotifications()
PluginAPI-->>NotifService: FCM/APNs token
NotifService->>Onboarding: return token
Onboarding->>Backend: POST registerDevice(token, ename, deviceId)
Backend->>Backend: DeviceTokenService.register(...)
Backend->>Trigger: (later) POST /api/send with token & payload
Trigger->>Trigger: initFcm/initApns -> sendFcm/sendApns
Trigger-->>Backend: send result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrastructure/eid-wallet/src/lib/services/NotificationService.ts (1)
107-107:⚠️ Potential issue | 🟠 MajorPII leakage via
console.log: fullDeviceRegistrationobject logged.Line 107 logs the complete registration object, which contains
eNameanddeviceId— persistent user identifiers. This should be removed or replaced with a non-identifying log (e.g., log only success status). The same concern applies to line 213, which logsvault.enamedirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/eid-wallet/src/lib/services/NotificationService.ts` at line 107, Remove the PII-bearing console.log that prints the full DeviceRegistration object and the direct logging of vault.ename in NotificationService; specifically, replace the console.log("Device registered successfully:", registration) and any console.log(vault.ename) calls with non-identifying messages (e.g., "Device registered successfully" or include only a masked/hashed device id or a boolean success flag). Locate these in the NotificationService class (search for the variable registration and for vault.ename) and ensure logs never emit eName or deviceId; if you need traceability, log a generated request id or the outcome status instead.
🧹 Nitpick comments (2)
infrastructure/eid-wallet/src/lib/services/NotificationService.ts (2)
360-366:getPushToken()has no error boundary of its own.
requestPermissions(),getPlatform(), andgetFCMToken()each suppress errors internally and return safe fallback values. However, if any of those internal contracts ever changes, exceptions fromgetPushToken()will propagate uncaught to callers. Adding a top-leveltry/catch(asgetFCMTokenitself does) is cheap insurance.🛡️ Proposed fix
async getPushToken(): Promise<string | undefined> { - const hasPermission = await this.requestPermissions(); - if (!hasPermission) return undefined; - const platform = await this.getPlatform(); - if (platform !== "android" && platform !== "ios") return undefined; - return this.getFCMToken(); + try { + const hasPermission = await this.requestPermissions(); + if (!hasPermission) return undefined; + const platform = await this.getPlatform(); + if (platform !== "android" && platform !== "ios") return undefined; + return await this.getFCMToken(); + } catch (error) { + console.error("Failed to get push token:", error); + return undefined; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/eid-wallet/src/lib/services/NotificationService.ts` around lines 360 - 366, Add a top-level try/catch to getPushToken to prevent any unexpected exceptions from bubbling to callers: wrap the existing calls to requestPermissions(), getPlatform(), and getFCMToken() in a try block and in the catch return undefined (and optionally log the error using the service logger consistent with how getFCMToken handles errors). This ensures getPushToken remains a safe, exception-free API even if the internal contracts of requestPermissions, getPlatform, or getFCMToken change.
344-354:getFCMTokennaming is misleading — it now returns an APNs token on iOS too.The method name implies FCM-only, but since it calls
registerForPushNotifications()which returns either an FCM token (Android) or an APNs device token (iOS), it is platform-agnostic. Consider renaming togetPushRegistrationTokenor similar to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/eid-wallet/src/lib/services/NotificationService.ts` around lines 344 - 354, The method getFCMToken is misnamed because registerForPushNotifications() returns platform-agnostic tokens (FCM on Android or APNs on iOS); rename getFCMToken to getPushRegistrationToken (and update its Promise<string | undefined> signature and JSDoc) and replace all internal and external references/calls to getFCMToken with getPushRegistrationToken (including any tests, imports, and usages in NotificationService and consumers) so the name accurately reflects it returns a generic push registration token.
🤖 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/eid-wallet/package.json`:
- Line 32: The package dependency "@choochmeque/tauri-plugin-notifications-api"
poses supply-chain risk; audit its source, change the version specifier to an
exact pinned version ("@choochmeque/tauri-plugin-notifications-api": "0.4.3") to
avoid caret-based upgrades, and either replace it with the official
"@tauri-apps/plugin-notification" if it meets requirements or add a documented
justification and security review for continuing to use the community plugin;
update package.json accordingly and add a short note in your PR explaining the
audit result and the decision.
- Line 32: The CI is failing because package.json was changed but pnpm-lock.yaml
wasn't updated; run pnpm install in the repository root to regenerate
pnpm-lock.yaml and commit the updated pnpm-lock.yaml alongside the package.json
change so the lockfile and dependency tree are in sync with the new
"@choochmeque/tauri-plugin-notifications-api" entry.
In `@infrastructure/eid-wallet/PUSH_NOTIFICATIONS_SETUP.md`:
- Around line 12-14: The fenced code block containing the single line
"src-tauri/gen/android/app/google-services.json" is missing a language tag
(MD040); update the opening fence from ``` to a language-tagged fence such as
```text (or ```sh) so the block reads e.g. ```text and keeps the existing
content unchanged to satisfy markdownlint.
In `@infrastructure/eid-wallet/src/routes/`(auth)/onboarding/+page.svelte:
- Around line 20-22: The onboarding flow obtains a push token (pushToken via
getPushToken()) but never sends it to the backend; call
NotificationService.registerDevice(eName) once the envoy name (eName) is
available (i.e., immediately after the provision step that sets eName) to POST
the token; update the onboarding page to check pushToken and
pushTokenLoading/pushTokenError and invoke
NotificationService.registerDevice(eName) with the retrieved token (handle and
surface errors via pushTokenError) so device registration occurs as part of the
provisioning sequence.
- Around line 226-235: Remove the debug UI that displays raw push tokens from
the onboarding page: delete or conditionally hide the entire <section> block
that renders pushToken, pushTokenLoading, and pushTokenError in +page.svelte so
push tokens are never shown in production; if you need a developer-only view,
wrap that block behind a strict dev-only flag (e.g., import a isDev boolean from
config) and ensure it is false in production builds.
---
Outside diff comments:
In `@infrastructure/eid-wallet/src/lib/services/NotificationService.ts`:
- Line 107: Remove the PII-bearing console.log that prints the full
DeviceRegistration object and the direct logging of vault.ename in
NotificationService; specifically, replace the console.log("Device registered
successfully:", registration) and any console.log(vault.ename) calls with
non-identifying messages (e.g., "Device registered successfully" or include only
a masked/hashed device id or a boolean success flag). Locate these in the
NotificationService class (search for the variable registration and for
vault.ename) and ensure logs never emit eName or deviceId; if you need
traceability, log a generated request id or the outcome status instead.
---
Nitpick comments:
In `@infrastructure/eid-wallet/src/lib/services/NotificationService.ts`:
- Around line 360-366: Add a top-level try/catch to getPushToken to prevent any
unexpected exceptions from bubbling to callers: wrap the existing calls to
requestPermissions(), getPlatform(), and getFCMToken() in a try block and in the
catch return undefined (and optionally log the error using the service logger
consistent with how getFCMToken handles errors). This ensures getPushToken
remains a safe, exception-free API even if the internal contracts of
requestPermissions, getPlatform, or getFCMToken change.
- Around line 344-354: The method getFCMToken is misnamed because
registerForPushNotifications() returns platform-agnostic tokens (FCM on Android
or APNs on iOS); rename getFCMToken to getPushRegistrationToken (and update its
Promise<string | undefined> signature and JSDoc) and replace all internal and
external references/calls to getFCMToken with getPushRegistrationToken
(including any tests, imports, and usages in NotificationService and consumers)
so the name accurately reflects it returns a generic push registration token.
Comment on lines
+20
to
+22
| let pushToken = $state<string | undefined>(undefined); | ||
| let pushTokenError = $state<string | undefined>(undefined); | ||
| let pushTokenLoading = $state(true); |
Contributor
There was a problem hiding this comment.
Push token is fetched but never transmitted to the backend in this flow.
getPushToken() retrieves the FCM/APNs token, but neither the onboarding page nor any function it calls sends the token to the provisioner. NotificationService.registerDevice() is the method that POSTs the token, but it is not invoked here. If the intent is for onboarding to register the device, registerDevice(eName) should be called once eName is available (e.g., after the provision step at line 172). Leaving the token un-transmitted makes the push notification flow inoperative end-to-end.
Also applies to: 132-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/eid-wallet/src/routes/`(auth)/onboarding/+page.svelte around
lines 20 - 22, The onboarding flow obtains a push token (pushToken via
getPushToken()) but never sends it to the backend; call
NotificationService.registerDevice(eName) once the envoy name (eName) is
available (i.e., immediately after the provision step that sets eName) to POST
the token; update the onboarding page to check pushToken and
pushTokenLoading/pushTokenError and invoke
NotificationService.registerDevice(eName) with the retrieved token (handle and
surface errors via pushTokenError) so device registration occurs as part of the
provisioning sequence.
Comment on lines
+226
to
+235
| <section class="mb-4 p-2 rounded bg-gray-100 text-xs"> | ||
| <p class="font-medium text-gray-600 mb-1">Push token (FCM/APNs):</p> | ||
| {#if pushTokenLoading} | ||
| <span class="text-gray-500">Loading...</span> | ||
| {:else if pushToken} | ||
| <code class="block break-all text-gray-800">{pushToken}</code> | ||
| {:else} | ||
| <span class="text-amber-600">{pushTokenError ?? "—"}</span> | ||
| {/if} | ||
| </section> |
Contributor
There was a problem hiding this comment.
Debug push-token UI must not ship to production.
The entire <section> block renders the raw FCM/APNs token — a sensitive credential — directly to the user's screen, along with the debug string "No token (desktop or permission denied)". Push tokens should never be displayed in the production UI; they are meant for server-side consumption only. This block appears to be a development/verification artefact and should be removed before merge.
🐛 Proposed fix — remove debug section
- <section class="mb-4 p-2 rounded bg-gray-100 text-xs">
- <p class="font-medium text-gray-600 mb-1">Push token (FCM/APNs):</p>
- {`#if` pushTokenLoading}
- <span class="text-gray-500">Loading...</span>
- {:else if pushToken}
- <code class="block break-all text-gray-800">{pushToken}</code>
- {:else}
- <span class="text-amber-600">{pushTokenError ?? "—"}</span>
- {/if}
- </section>📝 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
| <section class="mb-4 p-2 rounded bg-gray-100 text-xs"> | |
| <p class="font-medium text-gray-600 mb-1">Push token (FCM/APNs):</p> | |
| {#if pushTokenLoading} | |
| <span class="text-gray-500">Loading...</span> | |
| {:else if pushToken} | |
| <code class="block break-all text-gray-800">{pushToken}</code> | |
| {:else} | |
| <span class="text-amber-600">{pushTokenError ?? "—"}</span> | |
| {/if} | |
| </section> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/eid-wallet/src/routes/`(auth)/onboarding/+page.svelte around
lines 226 - 235, Remove the debug UI that displays raw push tokens from the
onboarding page: delete or conditionally hide the entire <section> block that
renders pushToken, pushTokenLoading, and pushTokenError in +page.svelte so push
tokens are never shown in production; if you need a developer-only view, wrap
that block behind a strict dev-only flag (e.g., import a isDev boolean from
config) and ensure it is false in production builds.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
infrastructure/eid-wallet/src/lib/services/NotificationService.ts (1)
344-354: Optional: renamegetFCMTokento reflect it also handles APNs.The method now retrieves both FCM (Android) and APNs (iOS) tokens, but the name only references FCM. Since it's
privatethe blast radius is contained, but a more accurate name avoids future confusion.♻️ Suggested rename
- /** - * Get push notification token (FCM on Android, APNs on iOS) - */ - private async getFCMToken(): Promise<string | undefined> { + /** + * Get push notification token (FCM on Android, APNs on iOS) + */ + private async getPushNotificationToken(): Promise<string | undefined> {Update call sites accordingly:
- fcmToken = await this.getFCMToken(); + fcmToken = await this.getPushNotificationToken();- return this.getFCMToken(); + return this.getPushNotificationToken();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/eid-wallet/src/lib/services/NotificationService.ts` around lines 344 - 354, Rename the private method getFCMToken to a clearer name like getPushToken or getPushNotificationToken (it wraps registerForPushNotifications and returns APNs on iOS as well), update its JSDoc/comment to reflect it returns platform-agnostic push tokens, and update all internal call sites that reference getFCMToken to the new name (the method is private so only update usages within the class/module).
🤖 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/eid-wallet/src/lib/services/NotificationService.ts`:
- Around line 360-366: In getPushToken, avoid calling requestPermissions() until
we've confirmed the runtime is a supported mobile platform: call
this.getPlatform() first, return undefined if platform is not "android" or
"ios", and only then call requestPermissions(); if permissions are granted call
this.getFCMToken() as before (references: getPushToken, getPlatform,
requestPermissions, getFCMToken).
---
Nitpick comments:
In `@infrastructure/eid-wallet/src/lib/services/NotificationService.ts`:
- Around line 344-354: Rename the private method getFCMToken to a clearer name
like getPushToken or getPushNotificationToken (it wraps
registerForPushNotifications and returns APNs on iOS as well), update its
JSDoc/comment to reflect it returns platform-agnostic push tokens, and update
all internal call sites that reference getFCMToken to the new name (the method
is private so only update usages within the class/module).
Comment on lines
+360
to
+366
| async getPushToken(): Promise<string | undefined> { | ||
| const hasPermission = await this.requestPermissions(); | ||
| if (!hasPermission) return undefined; | ||
| const platform = await this.getPlatform(); | ||
| if (platform !== "android" && platform !== "ios") return undefined; | ||
| return this.getFCMToken(); | ||
| } |
Contributor
There was a problem hiding this comment.
Permission is requested before the mobile platform gate — desktop users get a spurious permission dialog.
requestPermissions() (which can show a native OS prompt) is called before the android/ios check. On desktop, the method unconditionally requests notification permission and then returns undefined anyway. The platform check should be moved ahead of the permission request.
Push notifications are currently supported on mobile platforms (iOS and Android) only; macOS and Windows support is not yet available. Requesting permissions before knowing we're on a supported platform is both wasteful and surprising to the user.
🐛 Proposed fix — gate on platform first
async getPushToken(): Promise<string | undefined> {
- const hasPermission = await this.requestPermissions();
- if (!hasPermission) return undefined;
const platform = await this.getPlatform();
if (platform !== "android" && platform !== "ios") return undefined;
+ const hasPermission = await this.requestPermissions();
+ if (!hasPermission) return undefined;
return this.getFCMToken();
}📝 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
| async getPushToken(): Promise<string | undefined> { | |
| const hasPermission = await this.requestPermissions(); | |
| if (!hasPermission) return undefined; | |
| const platform = await this.getPlatform(); | |
| if (platform !== "android" && platform !== "ios") return undefined; | |
| return this.getFCMToken(); | |
| } | |
| async getPushToken(): Promise<string | undefined> { | |
| const platform = await this.getPlatform(); | |
| if (platform !== "android" && platform !== "ios") return undefined; | |
| const hasPermission = await this.requestPermissions(); | |
| if (!hasPermission) return undefined; | |
| return this.getFCMToken(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/eid-wallet/src/lib/services/NotificationService.ts` around
lines 360 - 366, In getPushToken, avoid calling requestPermissions() until we've
confirmed the runtime is a supported mobile platform: call this.getPlatform()
first, return undefined if platform is not "android" or "ios", and only then
call requestPermissions(); if permissions are granted call this.getFCMToken() as
before (references: getPushToken, getPlatform, requestPermissions, getFCMToken).
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
infrastructure/eid-wallet/package.json (1)
33-33: Prefer exact pin for the community notification plugin.Line 33 still uses a caret range (
^0.4.3), which can drift when lockfiles are refreshed. For this security-sensitive path, consider pinning to0.4.3and upgrading via explicit PRs.#!/bin/bash set -euo pipefail # 1) Verify declared spec vs locked resolution rg -n '"@choochmeque/tauri-plugin-notifications-api"' infrastructure/eid-wallet/package.json pnpm-lock.yaml # 2) Check whether CI enforces frozen lockfiles rg -n 'frozen-lockfile|--frozen-lockfile|prefer-frozen-lockfile' .github/workflows package.json .npmrc pnpm-workspace.yaml # 3) Check for pnpm overrides that already pin this dependency rg -n '"overrides"|"@choochmeque/tauri-plugin-notifications-api"' package.json pnpm-workspace.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/eid-wallet/package.json` at line 33, Update the dependency spec for "@choochmeque/tauri-plugin-notifications-api" in infrastructure/eid-wallet/package.json from the caret range "^0.4.3" to the exact pin "0.4.3"; after changing the spec run your package manager (pnpm install) to regenerate pnpm-lock.yaml, verify the lockfile contains the exact 0.4.3 resolution, and ensure CI settings still enforce frozen lockfiles; also search pnpm-workspace.yaml/package.json for any "overrides" entries that might conflict and update them to the exact version if present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@infrastructure/eid-wallet/package.json`:
- Line 33: Update the dependency spec for
"@choochmeque/tauri-plugin-notifications-api" in
infrastructure/eid-wallet/package.json from the caret range "^0.4.3" to the
exact pin "0.4.3"; after changing the spec run your package manager (pnpm
install) to regenerate pnpm-lock.yaml, verify the lockfile contains the exact
0.4.3 resolution, and ensure CI settings still enforce frozen lockfiles; also
search pnpm-workspace.yaml/package.json for any "overrides" entries that might
conflict and update them to the exact version if present.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
infrastructure/eid-wallet/src-tauri/Cargo.lockis excluded by!**/*.lockinfrastructure/eid-wallet/src-tauri/gen/android/app/build.gradle.ktsis excluded by!**/gen/**infrastructure/eid-wallet/src-tauri/gen/android/build.gradle.ktsis excluded by!**/gen/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
infrastructure/eid-wallet/package.jsoninfrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (15)
infrastructure/notification-trigger/src/senders/apns.ts (1)
6-34: Consider adding an idempotent guard to prevent accidental reinitializations.
initApns()is currently called once at startup (line 95 insrc/index.ts) andshutdownApns()is called on SIGTERM. However, if the initialization were called again before shutdown, it would create a new provider without disposing the previous one. Adding a simple guard likeif (provider) return true;at the start of the function would prevent this scenario and make the code more defensive:export function initApns(): boolean { + if (provider) return true; + const keyPath = process.env.APNS_KEY_PATH;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/notification-trigger/src/senders/apns.ts` around lines 6 - 34, The initApns function can reinitialize provider and leak resources; add an idempotent guard at the start of initApns that checks the module-level provider variable (e.g., if (provider) return true;) so you never create a new apn.Provider when one already exists, and optionally log a no-op message; ensure this interacts correctly with existing shutdownApns so shutdownApns still disposes provider when called.infrastructure/evault-core/src/services/NotificationService.ts (1)
127-130: Move push-token filtering to the query layer.The
getDevicesWithPushTokens()method loads all active devices into memory and then filters, which is inefficient at scale. Push devices with empty or null tokens should be filtered at the database level using QueryBuilder.Note: This method currently has no call sites in the codebase—verify if it's intended for future use or can be removed entirely.
♻️ Proposed refactor
async getDevicesWithPushTokens(): Promise<Verification[]> { - const all = await this.getAllDevices(); - return all.filter((v) => v.fcmToken && v.fcmToken.trim().length > 0); + return await this.verificationRepository + .createQueryBuilder("v") + .where("v.deviceActive = :active", { active: true }) + .andWhere("v.fcmToken IS NOT NULL") + .andWhere("TRIM(v.fcmToken) <> ''") + .getMany(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/services/NotificationService.ts` around lines 127 - 130, Replace the in-memory filtering in getDevicesWithPushTokens() by moving the fcmToken null/empty check into the DB query: modify the repository/query used by getDevicesWithPushTokens (or replace its call to getAllDevices()) to use QueryBuilder and add a where clause that fcmToken IS NOT NULL AND TRIM(fcmToken) <> '' so only devices with push tokens are returned; if getDevicesWithPushTokens() has no callers, confirm whether to delete it instead of refactoring. Ensure you update references to getDevicesWithPushTokens() and any repository methods it uses (e.g., getAllDevices) so the filtering is applied at the query layer.infrastructure/notification-trigger/src/index.ts (3)
19-19: Wide-open CORS with no origin restriction.
cors()with no options allows any origin. Since this is an internal notification-trigger service, consider restricting to known origins (e.g., the control panel host) or binding to127.0.0.1only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/notification-trigger/src/index.ts` at line 19, The app currently enables wide-open CORS via app.use(cors()), which allows any origin; change this to supply explicit options to cors() restricting allowed origins (e.g., a whitelist array or a single control-panel origin) or remove external CORS and ensure the server binds to localhost (127.0.0.1) in the startup (where app.listen is called) so only internal callers can reach it; update the app.use(cors()) invocation to pass the chosen origin/whitelist validator and add tests or configuration values for the allowed origin(s).
45-48:platformParamis assigned toPlatformwithout an explicit type cast — verify TypeScript accepts this.
platformParamisunknown/anyfromreq.body, and the ternary at Line 47 returns it directly as typePlatform. Depending on strictness settings, TypeScript may infer this asany, masking a type gap.♻️ Proposed fix
- const platform: Platform = - platformParam && ["ios", "android"].includes(platformParam) - ? platformParam - : detectPlatformFromToken(token); + const platform: Platform = + platformParam && (["ios", "android"] as string[]).includes(platformParam) + ? (platformParam as Platform) + : detectPlatformFromToken(token);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/notification-trigger/src/index.ts` around lines 45 - 48, platformParam comes from req.body (unknown/any) but is being assigned to Platform without validation; update the ternary to first ensure platformParam is a string and one of the allowed values before treating it as Platform (e.g., check typeof platformParam === 'string' && ["ios","android"].includes(platformParam)), otherwise call detectPlatformFromToken(token); use an explicit cast (platformParam as Platform) only after the runtime check to satisfy TypeScript and avoid widening to any.
104-107: SIGTERM handler does not drain in-flight requests before exiting.
server.close()stops accepting new connections but the process exits immediately when the handler returns. In-flight/api/sendcalls can be killed mid-flight. Additionally,shutdownApns()likely performs async teardown but its return value is not awaited.♻️ Proposed fix
-process.on("SIGTERM", () => { - shutdownApns(); - server.close(); -}); +process.on("SIGTERM", () => { + server.close(async () => { + await shutdownApns(); + process.exit(0); + }); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/notification-trigger/src/index.ts` around lines 104 - 107, The SIGTERM handler currently calls shutdownApns() and server.close() without awaiting their completion, so in-flight requests can be terminated; change the handler registered with process.on("SIGTERM") to perform an async graceful shutdown: stop accepting new connections, await the server.close() callback or promise (ensuring pending /api/send handlers finish or a short timeout elapses), await shutdownApns() to finish its async teardown, and only then call process.exit with an appropriate code; reference shutdownApns, server.close, and the SIGTERM process.on handler when making the change and add a bounded timeout fallback to force exit if shutdown hangs.infrastructure/eid-wallet/src/routes/(app)/+layout.svelte (2)
38-38:vaulttruthiness check is redundant after the guard at Line 27.
vaultis guaranteed non-null at this point (checked at lines 27–31). The expressionvault && "ename" in vaultre-checks vault's truthiness unnecessarily.♻️ Proposed fix
- const ename = vault && "ename" in vault ? String(vault.ename) : undefined; + const ename = "ename" in vault ? String(vault.ename) : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/eid-wallet/src/routes/`(app)/+layout.svelte at line 38, Remove the redundant truthiness check on vault when computing ename: since vault is already validated as non-null by the earlier guard (lines 27–31), replace the expression that uses "vault && 'ename' in vault" with a direct property-existence test on vault (e.g., use "'ename' in vault ? String(vault.ename) : undefined") so ename derives from the confirmed non-null vault without re-checking its truthiness; update the const ename assignment accordingly.
36-44:notificationServiceis accessed twice in separate scopes — consider hoisting it.
notificationServiceis read fromglobalStateat Line 37 (inside the registration block) and again at Line 48 (inside the notification-check block). Both reads occur afterglobalStateandvaultare confirmed non-null. Hoisting a singleconst notificationService = globalState.notificationServicebefore both inner try blocks removes duplication and makes the null-safety clearer.♻️ Proposed refactor
console.log("User authenticated, allowing access to app routes"); + const notificationService = globalState.notificationService; + // Register device for push notifications (eName + token to provisioner) try { - const notificationService = globalState.notificationService; const ename = vault && "ename" in vault ? String(vault.ename) : undefined; if (ename) { await notificationService.registerDevice(ename); } } catch (error) { console.error("Failed to register device for notifications:", error); } // Check for notifications after successful authentication try { - const notificationService = globalState.notificationService; await notificationService.checkAndShowNotifications(); } catch (error) { console.error("Failed to check notifications:", error); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/eid-wallet/src/routes/`(app)/+layout.svelte around lines 36 - 44, Hoist the repeated access to globalState.notificationService into a single const before the two try blocks so you only read it once; e.g., after confirming globalState and vault are non-null, create const notificationService = globalState.notificationService and then use that variable in both the await notificationService.registerDevice(ename) call and in the later notification-check logic, removing the duplicate reads to improve null-safety and clarity around notificationService usage.infrastructure/evault-core/src/entities/DeviceToken.ts (1)
22-23:platformstored as unconstrainedvarchar— consider an enum or check constraint.Any arbitrary string can be persisted as
platform. A typo (e.g.,"IOS") would causesendNotificationto route to FCM silently. Using a PostgreSQLenumor an explicit check constraint ensures only"ios"/"android"is stored.♻️ Proposed fix
- `@Column`({ type: "varchar" }) - platform!: string; + `@Column`({ type: "enum", enum: ["ios", "android"] }) + platform!: "ios" | "android";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/entities/DeviceToken.ts` around lines 22 - 23, The DeviceToken entity's platform property is currently an unconstrained varchar allowing typos; update DeviceToken.platform to use a strict enum or DB check constraint so only the allowed values ("ios" and "android") are persisted. Define a TypeScript enum (e.g., DevicePlatform) and change the `@Column` on platform to use type: "enum" with enum: DevicePlatform (or add a Postgres CHECK constraint via TypeORM `@Check`) and add a migration to convert/validate existing values (normalise to lowercase or reject invalid rows) so sendNotification's routing logic receives only "ios" or "android".infrastructure/control-panel/src/routes/api/notifications/devices-count/+server.ts (1)
5-11:catchblock is unreachable —getDevicesWithTokens()swallows all errors internally.
getDevicesWithTokens()(ininfrastructure/control-panel/src/lib/services/notificationService.ts, lines 52–69) wraps its body in atry/catchand returns[]on failure — it never throws. Thecatchat Line 9 will therefore never execute, giving a false sense of resilience.Either let
getDevicesWithTokenspropagate errors and rely on this handler's catch, or remove the outer try/catch here since it adds no value.♻️ Proposed fix (simplify)
export const GET: RequestHandler = async () => { - try { - const devices = await getDevicesWithTokens(); - return json({ count: devices.length }); - } catch { - return json({ count: 0 }); - } + const devices = await getDevicesWithTokens(); + return json({ count: devices.length }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/control-panel/src/routes/api/notifications/devices-count/`+server.ts around lines 5 - 11, The GET handler wraps getDevicesWithTokens() in a try/catch but that function already swallows errors and returns [] (so the catch is unreachable); either remove the outer try/catch in the GET handler (in +server.ts) and return json({ count: devices.length }) directly, or modify getDevicesWithTokens (in notificationService.ts) to rethrow errors instead of returning [] so the handler's catch can handle failures — pick one approach and update the corresponding function (GET RequestHandler or getDevicesWithTokens) so error handling is single-responsibility and not duplicated.infrastructure/control-panel/src/routes/api/notifications/health/+server.ts (1)
5-12: Redundant outertry/catch— the service never throws.
checkNotificationTriggerHealth(seenotificationService.tslines 116–131) already has its owntry/catchand always returns a plain value — it never propagates an exception. The wrappercatchin this route is therefore dead code.♻️ Simplified handler
-export const GET: RequestHandler = async () => { - try { - const health = await checkNotificationTriggerHealth(); - return json(health); - } catch { - return json({ ok: false, apns: false, fcm: false }); - } -}; +export const GET: RequestHandler = async () => { + const health = await checkNotificationTriggerHealth(); + return json(health); +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/control-panel/src/routes/api/notifications/health/`+server.ts around lines 5 - 12, The outer try/catch in the GET route is redundant because checkNotificationTriggerHealth always returns a value and never throws; remove the try/catch and simplify the handler (export const GET: RequestHandler) to directly await checkNotificationTriggerHealth and return json(...) of that result, deleting the dead catch branch that returns the static { ok: false, apns: false, fcm: false } object.infrastructure/control-panel/src/lib/services/notificationService.ts (3)
42-44: Checkresponse.okbefore calling.json()to surface real HTTP error codes.If the trigger service returns a 4xx/5xx with a non-JSON body, the
catchswallows the actual status and returns a generic'Request failed'string, making it hard to diagnose errors.♻️ Proposed fix
const response = await fetch(`${baseUrl}/api/send`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(request), signal: AbortSignal.timeout(15000) }); + if (!response.ok) { + return { success: false, error: `HTTP ${response.status}` }; + } const data = await response.json();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/control-panel/src/lib/services/notificationService.ts` around lines 42 - 44, Before calling response.json() in the fetch handling code (the block that awaits response and currently uses the local variable response), check response.ok and if false throw or return an error that includes response.status and response.statusText; if the body is JSON include parsed error details, otherwise include the raw text (await response.text()) so non-JSON 4xx/5xx bodies are surfaced; update the function that performs the fetch (the handler using the response variable in notificationService.ts) to return { success: false, error: `HTTP ${response.status} ${response.statusText}: ${bodyOrMessage}` } when !response.ok instead of blindly parsing or returning a generic message.
3-17:NotificationPayloadandSendNotificationRequestare duplicated across at least three files.Identical (or near-identical) types already exist in
infrastructure/notification-trigger/src/types.ts. Maintaining three separate definitions risks silent drift when fields change. Consider extracting these to a shared package (e.g., a@proto/notification-typesworkspace package) or re-exporting from the trigger module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/control-panel/src/lib/services/notificationService.ts` around lines 3 - 17, The types NotificationPayload and SendNotificationRequest are duplicated; remove the local declarations and import/re-export the canonical definitions instead (either from the existing centralized types module or by extracting them into a shared workspace package like `@proto/notification-types`), then update any code that referenced the local interfaces to use the shared types to avoid drift; target the exported symbols NotificationPayload and SendNotificationRequest when replacing the local definitions and ensure any optional fields (subtitle, data, sound, badge, clickAction, platform) match the canonical shape.
124-128: Same missingresponse.okguard before.json()as insendNotification.A non-JSON error response from the health endpoint will be caught and produce the silent
{ ok: false, apns: false, fcm: false }fallback, hiding the real HTTP status code. The sameif (!response.ok)guard suggested forsendNotificationapplies here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/control-panel/src/lib/services/notificationService.ts` around lines 124 - 128, The health-check fetch currently calls response.json() without checking response.ok, which can swallow non-JSON HTTP errors; update the health endpoint block (the fetch that sets `response` and `data`) to first check `if (!response.ok)` and handle/report the error (e.g., throw or return a structured error including `response.status`/statusText) before calling `response.json()`, mirroring the `if (!response.ok)` guard you added in `sendNotification`.infrastructure/evault-core/src/services/DeviceTokenService.ts (2)
50-61:getDevicesWithTokens()performs an unbounded full-table scan.As device registrations grow, loading every row without pagination will cause memory pressure and slow responses. Consider adding
take/skippagination or streaming, especially since this is likely called by a control-panel list endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/services/DeviceTokenService.ts` around lines 50 - 61, getDevicesWithTokens is doing an unbounded full-table scan and should be changed to support pagination or streaming to avoid memory pressure; update DeviceTokenService.getDevicesWithTokens to accept pagination params (e.g., limit/take and offset/skip or a cursor) and use this.deviceTokenRepository.find({ order: { updatedAt: "DESC" }, take, skip }) or switch to a query builder/stream to iterate results in batches, ensuring the returned items still map to { token, platform, eName } and update any callers to pass pagination/cursor values.
7-7:platformshould be a union type rather than a rawstring.The control-panel service's
SendNotificationRequestnarrowsplatformto'ios' | 'android', butDeviceTokenRegistration.platformaccepts any string, allowing invalid values to be persisted silently.♻️ Proposed fix
- platform: string; + platform: 'ios' | 'android';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/services/DeviceTokenService.ts` at line 7, Change the DeviceTokenRegistration.platform from a plain string to a union type limiting values to 'ios' | 'android' so invalid platforms cannot be persisted; update the type declaration in DeviceTokenRegistration (and any related type or interface in DeviceTokenService) to use the union, and adjust any call sites that construct DeviceTokenRegistration or assign its platform to ensure they produce one of the allowed literal values (or perform validation/coercion), keeping SendNotificationRequest's existing 'ios' | 'android' compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 40-53: Replace machine-specific absolute paths and account
identifiers in the template with neutral placeholders: change
GOOGLE_APPLICATION_CREDENTIALS and APNS_KEY_PATH from local user paths to
generic placeholders like /path/to/credentials.json or
./secrets/credentials.json, and replace APNS_KEY_ID, APNS_TEAM_ID,
APNS_BUNDLE_ID and APNS_BROADCAST_CHANNEL_ID with clearly marked placeholder
tokens (e.g. <APNS_KEY_ID>, <APNS_TEAM_ID>, <APNS_BUNDLE_ID>,
<APNS_BROADCAST_CHANNEL_ID>); also ensure NOTIFICATION_TRIGGER_URL remains a
generic default (e.g. http://localhost:${NOTIFICATION_TRIGGER_PORT}) rather than
embedding any machine-specific values so the .env.example is portable and
non-sensitive.
In `@infrastructure/control-panel/src/lib/services/notificationService.ts`:
- Around line 56-58: Remove the redundant dynamic re-import of env and stop
reading PUBLIC_PROVISIONER_URL from the private env binding: use the
module-level env already imported at the top to read PROVISIONER_URL when
computing provisionerUrl (i.e., replace the await import('$env/dynamic/private')
usage and the env.PUBLIC_PROVISIONER_URL fallback with env.PROVISIONER_URL ||
'http://localhost:3001'); if you need to support PUBLIC_PROVISIONER_URL, import
it explicitly from '$env/dynamic/public' and prefer that value. Apply the same
change to the getDevicesByEName() code path where env is re-imported and
PUBLIC_PROVISIONER_URL is referenced.
- Around line 92-115: The sendBulkNotifications function currently fires
sendNotification for every token via Promise.all, which can overwhelm resources;
modify sendBulkNotifications to process tokens in bounded concurrent batches
(e.g., chunks of 50 or 100) instead of all at once by creating a small chunk
helper or loop that slices tokens into batches, awaits Promise.all for each
batch of sendNotification calls, collects and aggregates results exactly as
before (use token.trim(), preserve returned fields success/error), and return
the same { sent, failed, errors } shape; reference sendBulkNotifications and
sendNotification when locating the change.
In
`@infrastructure/control-panel/src/routes/api/notifications/send-bulk/`+server.ts:
- Around line 42-47: The success JSON response is missing a total field, causing
inconsistency with other bulk endpoints; update the response returned by the
handler that builds the json({ success, sent: result.sent, failed:
result.failed, errors: result.errors }) to include total — either use total:
result.sent + result.failed or, if the tokens array is in scope, total:
tokens.length — so the returned object contains total, sent, failed, and errors;
change the return to json({... , total: <choose one of the two expressions
above> }).
In `@infrastructure/control-panel/src/routes/api/notifications/send/`+server.ts:
- Around line 10-12: The current token check allows whitespace-only strings;
update the validation in the handler around the token variable so you trim it
and reject empty results before calling sendNotification: e.g., compute const
trimmedToken = typeof token === 'string' ? token.trim() : ''; then if
(!trimmedToken) return json({ success: false, error: 'Missing or invalid token'
}, { status: 400 }); and pass trimmedToken to sendNotification so
whitespace-only tokens produce a clean 400 instead of downstream failures.
- Around line 20-32: The code forwards the incoming platform value directly into
sendNotification, allowing arbitrary strings to reach the notification trigger;
mirror the send-bulk guard by validating platform before calling
sendNotification. Locate the platform variable in the send handler and replace
the raw platform with a validated value such as: use
(['ios','android'].includes(platform) ? platform : undefined) when constructing
the sendNotification call so only 'ios' or 'android' are forwarded to
sendNotification.
In `@infrastructure/control-panel/src/routes/notifications/`+page.svelte:
- Around line 34-40: The component sets a 4s timeout via toastTimeout in
showToast but never clears it on unmount; add a cleanup to avoid the callback
writing to a destroyed component. Import and use Svelte's onDestroy and in the
onDestroy callback clearTimeout(toastTimeout) and set toastTimeout = null (and
optionally check for existence) so the pending timeout is cancelled when the
component unmounts; reference the existing toastTimeout variable and showToast
function to implement this.
- Around line 163-175: The onMount error handler resets health but not
deviceCount, leaving deviceCount null on fetch failure; update the catch block
inside the onMount async function to also set deviceCount to a safe fallback
(e.g., 0 or a sentinel like -1) so the UI shows a clear state when
fetch('/api/notifications/devices-count') fails; locate the onMount handler and
add a line setting deviceCount = 0 (or your chosen fallback) alongside health =
{ ok: false, apns: false, fcm: false } to ensure the bulk-send section displays
an explicit count state.
In `@infrastructure/evault-core/src/controllers/NotificationController.ts`:
- Around line 86-92: The call to fcmToken.trim() in NotificationController when
calling notificationService.registerDevice causes a TypeError if fcmToken is
undefined; update the code in the handler so you compute a safe trimmed token
first (e.g., let trimmedFcm = typeof fcmToken === 'string' ? fcmToken.trim() :
undefined) and pass trimmedFcm to notificationService.registerDevice (and keep
the existing if (fcmToken && ...) guard for deviceTokenService.register). This
ensures notificationService.registerDevice receives either a trimmed string or
undefined and avoids calling .trim() on non-strings.
In `@infrastructure/evault-core/src/entities/DeviceToken.ts`:
- Around line 19-20: The DeviceToken entity's token property currently uses an
unqualified varchar (token in class DeviceToken) which maps to varchar(255) and
risks silently truncating FCM registration tokens; change the Column decorator
on token to use a non-length-limited type (e.g., type: "text") or an explicitly
large varchar length to avoid truncation, update any related TypeORM
migrations/schema generation for the DeviceToken.token column, and ensure
tests/fixtures that insert tokens still pass after the schema change.
In `@infrastructure/evault-core/src/migrations/1771880000000-DeviceToken.ts`:
- Around line 6-25: Add a DB-level unique constraint and index on the "token"
column in the migration: modify the up() in this migration to create a unique
index/constraint (e.g., CREATE UNIQUE INDEX "UQ_device_token_token" ON
"device_token" ("token")) and add the corresponding drop statement in down();
also update DeviceTokenService.register to stop using read-then-write and
instead perform an INSERT ... ON CONFLICT (token) DO UPDATE (or appropriate
upsert) so the DB enforces uniqueness and eliminates the TOCTOU race and
expensive sequential scans.
In `@infrastructure/evault-core/src/services/DeviceTokenService.ts`:
- Around line 14-48: DeviceTokenService.register has a TOCTOU race: sequential
findOne calls then create/save can trigger unique-constraint errors under
concurrency; wrap the register logic in a DB transaction (using your ORM's
transactional API) and either perform SELECT ... FOR UPDATE semantics or attempt
an upsert/ON CONFLICT (e.g., queryBuilder.orUpdate()/upsert) inside the
transaction, and catch unique-constraint DB errors on save to reload the
existing row and return it (or retry the lookup/update once) rather than letting
the constraint propagate; reference the register method,
deviceTokenRepository.findOne, deviceTokenRepository.create,
deviceTokenRepository.save, and use a transactional wrapper around that whole
flow.
In `@infrastructure/notification-trigger/public/index.html`:
- Around line 165-170: The catch branch that updates the status element
currently calls status.classList.remove("hidden", "success") but omits removing
"error", leaving a stale class; update the catch block handling for the status
element (the code manipulating status.textContent and status.classList) to
remove all three state classes ("hidden", "success", "error") before adding
"error", mirroring the success path so the status element's classes are always
reset (refer to the status variable and the catch branch in the event handler).
In `@infrastructure/notification-trigger/src/index.ts`:
- Line 67: The current spread uses Number(payload.badge) which can produce NaN;
update the logic in index.ts around the notification payload construction to
validate the badge before including it: compute a numeric value from
payload.badge (e.g., const badge = Number(payload.badge)), then only spread {
badge: badge } when payload.badge !== undefined AND Number.isFinite(badge) (and
optionally badge >= 0 / Number.isInteger(badge) if you require integers);
otherwise omit the badge field or reject the request. Ensure you reference
payload.badge and the payload construction site so NaN is never forwarded to
APNs/FCM.
In `@infrastructure/notification-trigger/src/senders/fcm.ts`:
- Around line 14-24: The initialize block currently calls admin.initializeApp()
unconditionally and treats any thrown error as a real failure; change it to
first check if admin.apps && admin.apps.length > 0 and, if so, set initialized =
true and return true (so existing initialization isn't treated as a failure),
otherwise call admin.initializeApp() inside the try; update the catch to only
log and return false for genuine errors while not masking the "app already
exists" case; reference the existing admin.initializeApp() call and the
initialized variable in this file (senders/fcm.ts) when making the change.
---
Nitpick comments:
In `@infrastructure/control-panel/src/lib/services/notificationService.ts`:
- Around line 42-44: Before calling response.json() in the fetch handling code
(the block that awaits response and currently uses the local variable response),
check response.ok and if false throw or return an error that includes
response.status and response.statusText; if the body is JSON include parsed
error details, otherwise include the raw text (await response.text()) so
non-JSON 4xx/5xx bodies are surfaced; update the function that performs the
fetch (the handler using the response variable in notificationService.ts) to
return { success: false, error: `HTTP ${response.status} ${response.statusText}:
${bodyOrMessage}` } when !response.ok instead of blindly parsing or returning a
generic message.
- Around line 3-17: The types NotificationPayload and SendNotificationRequest
are duplicated; remove the local declarations and import/re-export the canonical
definitions instead (either from the existing centralized types module or by
extracting them into a shared workspace package like `@proto/notification-types`),
then update any code that referenced the local interfaces to use the shared
types to avoid drift; target the exported symbols NotificationPayload and
SendNotificationRequest when replacing the local definitions and ensure any
optional fields (subtitle, data, sound, badge, clickAction, platform) match the
canonical shape.
- Around line 124-128: The health-check fetch currently calls response.json()
without checking response.ok, which can swallow non-JSON HTTP errors; update the
health endpoint block (the fetch that sets `response` and `data`) to first check
`if (!response.ok)` and handle/report the error (e.g., throw or return a
structured error including `response.status`/statusText) before calling
`response.json()`, mirroring the `if (!response.ok)` guard you added in
`sendNotification`.
In
`@infrastructure/control-panel/src/routes/api/notifications/devices-count/`+server.ts:
- Around line 5-11: The GET handler wraps getDevicesWithTokens() in a try/catch
but that function already swallows errors and returns [] (so the catch is
unreachable); either remove the outer try/catch in the GET handler (in
+server.ts) and return json({ count: devices.length }) directly, or modify
getDevicesWithTokens (in notificationService.ts) to rethrow errors instead of
returning [] so the handler's catch can handle failures — pick one approach and
update the corresponding function (GET RequestHandler or getDevicesWithTokens)
so error handling is single-responsibility and not duplicated.
In `@infrastructure/control-panel/src/routes/api/notifications/health/`+server.ts:
- Around line 5-12: The outer try/catch in the GET route is redundant because
checkNotificationTriggerHealth always returns a value and never throws; remove
the try/catch and simplify the handler (export const GET: RequestHandler) to
directly await checkNotificationTriggerHealth and return json(...) of that
result, deleting the dead catch branch that returns the static { ok: false,
apns: false, fcm: false } object.
In `@infrastructure/eid-wallet/src/routes/`(app)/+layout.svelte:
- Line 38: Remove the redundant truthiness check on vault when computing ename:
since vault is already validated as non-null by the earlier guard (lines 27–31),
replace the expression that uses "vault && 'ename' in vault" with a direct
property-existence test on vault (e.g., use "'ename' in vault ?
String(vault.ename) : undefined") so ename derives from the confirmed non-null
vault without re-checking its truthiness; update the const ename assignment
accordingly.
- Around line 36-44: Hoist the repeated access to
globalState.notificationService into a single const before the two try blocks so
you only read it once; e.g., after confirming globalState and vault are
non-null, create const notificationService = globalState.notificationService and
then use that variable in both the await
notificationService.registerDevice(ename) call and in the later
notification-check logic, removing the duplicate reads to improve null-safety
and clarity around notificationService usage.
In `@infrastructure/evault-core/src/entities/DeviceToken.ts`:
- Around line 22-23: The DeviceToken entity's platform property is currently an
unconstrained varchar allowing typos; update DeviceToken.platform to use a
strict enum or DB check constraint so only the allowed values ("ios" and
"android") are persisted. Define a TypeScript enum (e.g., DevicePlatform) and
change the `@Column` on platform to use type: "enum" with enum: DevicePlatform (or
add a Postgres CHECK constraint via TypeORM `@Check`) and add a migration to
convert/validate existing values (normalise to lowercase or reject invalid rows)
so sendNotification's routing logic receives only "ios" or "android".
In `@infrastructure/evault-core/src/services/DeviceTokenService.ts`:
- Around line 50-61: getDevicesWithTokens is doing an unbounded full-table scan
and should be changed to support pagination or streaming to avoid memory
pressure; update DeviceTokenService.getDevicesWithTokens to accept pagination
params (e.g., limit/take and offset/skip or a cursor) and use
this.deviceTokenRepository.find({ order: { updatedAt: "DESC" }, take, skip }) or
switch to a query builder/stream to iterate results in batches, ensuring the
returned items still map to { token, platform, eName } and update any callers to
pass pagination/cursor values.
- Line 7: Change the DeviceTokenRegistration.platform from a plain string to a
union type limiting values to 'ios' | 'android' so invalid platforms cannot be
persisted; update the type declaration in DeviceTokenRegistration (and any
related type or interface in DeviceTokenService) to use the union, and adjust
any call sites that construct DeviceTokenRegistration or assign its platform to
ensure they produce one of the allowed literal values (or perform
validation/coercion), keeping SendNotificationRequest's existing 'ios' |
'android' compatibility.
In `@infrastructure/evault-core/src/services/NotificationService.ts`:
- Around line 127-130: Replace the in-memory filtering in
getDevicesWithPushTokens() by moving the fcmToken null/empty check into the DB
query: modify the repository/query used by getDevicesWithPushTokens (or replace
its call to getAllDevices()) to use QueryBuilder and add a where clause that
fcmToken IS NOT NULL AND TRIM(fcmToken) <> '' so only devices with push tokens
are returned; if getDevicesWithPushTokens() has no callers, confirm whether to
delete it instead of refactoring. Ensure you update references to
getDevicesWithPushTokens() and any repository methods it uses (e.g.,
getAllDevices) so the filtering is applied at the query layer.
In `@infrastructure/notification-trigger/src/index.ts`:
- Line 19: The app currently enables wide-open CORS via app.use(cors()), which
allows any origin; change this to supply explicit options to cors() restricting
allowed origins (e.g., a whitelist array or a single control-panel origin) or
remove external CORS and ensure the server binds to localhost (127.0.0.1) in the
startup (where app.listen is called) so only internal callers can reach it;
update the app.use(cors()) invocation to pass the chosen origin/whitelist
validator and add tests or configuration values for the allowed origin(s).
- Around line 45-48: platformParam comes from req.body (unknown/any) but is
being assigned to Platform without validation; update the ternary to first
ensure platformParam is a string and one of the allowed values before treating
it as Platform (e.g., check typeof platformParam === 'string' &&
["ios","android"].includes(platformParam)), otherwise call
detectPlatformFromToken(token); use an explicit cast (platformParam as Platform)
only after the runtime check to satisfy TypeScript and avoid widening to any.
- Around line 104-107: The SIGTERM handler currently calls shutdownApns() and
server.close() without awaiting their completion, so in-flight requests can be
terminated; change the handler registered with process.on("SIGTERM") to perform
an async graceful shutdown: stop accepting new connections, await the
server.close() callback or promise (ensuring pending /api/send handlers finish
or a short timeout elapses), await shutdownApns() to finish its async teardown,
and only then call process.exit with an appropriate code; reference
shutdownApns, server.close, and the SIGTERM process.on handler when making the
change and add a bounded timeout fallback to force exit if shutdown hangs.
In `@infrastructure/notification-trigger/src/senders/apns.ts`:
- Around line 6-34: The initApns function can reinitialize provider and leak
resources; add an idempotent guard at the start of initApns that checks the
module-level provider variable (e.g., if (provider) return true;) so you never
create a new apn.Provider when one already exists, and optionally log a no-op
message; ensure this interacts correctly with existing shutdownApns so
shutdownApns still disposes provider when called.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
infrastructure/eid-wallet/src-tauri/gen/android/app/.gitignoreis excluded by!**/gen/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.env.exampleinfrastructure/control-panel/.env.exampleinfrastructure/control-panel/src/lib/services/notificationService.tsinfrastructure/control-panel/src/routes/+layout.svelteinfrastructure/control-panel/src/routes/api/notifications/devices-count/+server.tsinfrastructure/control-panel/src/routes/api/notifications/health/+server.tsinfrastructure/control-panel/src/routes/api/notifications/send-bulk-all/+server.tsinfrastructure/control-panel/src/routes/api/notifications/send-bulk/+server.tsinfrastructure/control-panel/src/routes/api/notifications/send-by-ename/+server.tsinfrastructure/control-panel/src/routes/api/notifications/send/+server.tsinfrastructure/control-panel/src/routes/notifications/+page.svelteinfrastructure/eid-wallet/src/routes/(app)/+layout.svelteinfrastructure/evault-core/src/config/database.tsinfrastructure/evault-core/src/controllers/NotificationController.tsinfrastructure/evault-core/src/entities/DeviceToken.tsinfrastructure/evault-core/src/migrations/1771880000000-DeviceToken.tsinfrastructure/evault-core/src/services/DeviceTokenService.tsinfrastructure/evault-core/src/services/NotificationService.tsinfrastructure/notification-trigger/README.mdinfrastructure/notification-trigger/package.jsoninfrastructure/notification-trigger/public/index.htmlinfrastructure/notification-trigger/src/index.tsinfrastructure/notification-trigger/src/senders/apns.tsinfrastructure/notification-trigger/src/senders/fcm.tsinfrastructure/notification-trigger/src/senders/index.tsinfrastructure/notification-trigger/src/types.tsinfrastructure/notification-trigger/tsconfig.json
✅ Files skipped from review due to trivial changes (1)
- infrastructure/notification-trigger/README.md
Comment on lines
+40
to
+53
| GOOGLE_APPLICATION_CREDENTIALS="/Users/sosweetham/projs/metastate/prototype/secrets/eid-w-firebase-adminsdk.json" | ||
|
|
||
| # Notification Trigger (APNS/FCM toy platform) | ||
| NOTIFICATION_TRIGGER_PORT=3998 | ||
| # Full URL for control panel proxy (optional; defaults to http://localhost:NOTIFICATION_TRIGGER_PORT) | ||
| NOTIFICATION_TRIGGER_URL=http://localhost:3998 | ||
| # APNS (iOS) - from Apple Developer | ||
| APNS_KEY_PATH="/Users/sosweetham/projs/metastate/prototype/secrets/AuthKey_A3BBXD9YR3.p8" | ||
| APNS_KEY_ID="A3BBXD9YR3" | ||
| APNS_TEAM_ID="M49C8XS835" | ||
| APNS_BUNDLE_ID="com.example.app" | ||
| APNS_PRODUCTION=false | ||
| # Broadcast push (Live Activities) - base64 channel ID | ||
| APNS_BROADCAST_CHANNEL_ID=znbhuBJCEfEAAMIJbS9xUw== |
Contributor
There was a problem hiding this comment.
Replace machine-specific values in .env.example with placeholders.
Line 40 and Line 47 currently expose local absolute paths (including a username), and the APNS block uses concrete account identifiers. This is a portability and privacy/compliance risk for a template file.
🔧 Proposed fix
-GOOGLE_APPLICATION_CREDENTIALS="/Users/sosweetham/projs/metastate/prototype/secrets/eid-w-firebase-adminsdk.json"
+GOOGLE_APPLICATION_CREDENTIALS=/path/to/firebase-service-account.json
# Notification Trigger (APNS/FCM toy platform)
NOTIFICATION_TRIGGER_PORT=3998
# Full URL for control panel proxy (optional; defaults to http://localhost:NOTIFICATION_TRIGGER_PORT)
NOTIFICATION_TRIGGER_URL=http://localhost:3998
# APNS (iOS) - from Apple Developer
-APNS_KEY_PATH="/Users/sosweetham/projs/metastate/prototype/secrets/AuthKey_A3BBXD9YR3.p8"
-APNS_KEY_ID="A3BBXD9YR3"
-APNS_TEAM_ID="M49C8XS835"
-APNS_BUNDLE_ID="com.example.app"
+APNS_KEY_PATH=/path/to/AuthKey_XXXXXXXXXX.p8
+APNS_KEY_ID=YOUR_APNS_KEY_ID
+APNS_TEAM_ID=YOUR_APNS_TEAM_ID
+APNS_BUNDLE_ID=com.example.app
APNS_PRODUCTION=false
# Broadcast push (Live Activities) - base64 channel ID
-APNS_BROADCAST_CHANNEL_ID=znbhuBJCEfEAAMIJbS9xUw==
+APNS_BROADCAST_CHANNEL_ID=YOUR_BASE64_CHANNEL_ID📝 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
| GOOGLE_APPLICATION_CREDENTIALS="/Users/sosweetham/projs/metastate/prototype/secrets/eid-w-firebase-adminsdk.json" | |
| # Notification Trigger (APNS/FCM toy platform) | |
| NOTIFICATION_TRIGGER_PORT=3998 | |
| # Full URL for control panel proxy (optional; defaults to http://localhost:NOTIFICATION_TRIGGER_PORT) | |
| NOTIFICATION_TRIGGER_URL=http://localhost:3998 | |
| # APNS (iOS) - from Apple Developer | |
| APNS_KEY_PATH="/Users/sosweetham/projs/metastate/prototype/secrets/AuthKey_A3BBXD9YR3.p8" | |
| APNS_KEY_ID="A3BBXD9YR3" | |
| APNS_TEAM_ID="M49C8XS835" | |
| APNS_BUNDLE_ID="com.example.app" | |
| APNS_PRODUCTION=false | |
| # Broadcast push (Live Activities) - base64 channel ID | |
| APNS_BROADCAST_CHANNEL_ID=znbhuBJCEfEAAMIJbS9xUw== | |
| GOOGLE_APPLICATION_CREDENTIALS=/path/to/firebase-service-account.json | |
| # Notification Trigger (APNS/FCM toy platform) | |
| NOTIFICATION_TRIGGER_PORT=3998 | |
| # Full URL for control panel proxy (optional; defaults to http://localhost:NOTIFICATION_TRIGGER_PORT) | |
| NOTIFICATION_TRIGGER_URL=http://localhost:3998 | |
| # APNS (iOS) - from Apple Developer | |
| APNS_KEY_PATH=/path/to/AuthKey_XXXXXXXXXX.p8 | |
| APNS_KEY_ID=YOUR_APNS_KEY_ID | |
| APNS_TEAM_ID=YOUR_APNS_TEAM_ID | |
| APNS_BUNDLE_ID=com.example.app | |
| APNS_PRODUCTION=false | |
| # Broadcast push (Live Activities) - base64 channel ID | |
| APNS_BROADCAST_CHANNEL_ID=YOUR_BASE64_CHANNEL_ID |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 40-40: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 47-47: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 47-47: [UnorderedKey] The APNS_KEY_PATH key should go before the NOTIFICATION_TRIGGER_PORT key
(UnorderedKey)
[warning] 48-48: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 48-48: [UnorderedKey] The APNS_KEY_ID key should go before the APNS_KEY_PATH key
(UnorderedKey)
[warning] 49-49: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 49-49: [UnorderedKey] The APNS_TEAM_ID key should go before the NOTIFICATION_TRIGGER_PORT key
(UnorderedKey)
[warning] 50-50: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 50-50: [UnorderedKey] The APNS_BUNDLE_ID key should go before the APNS_KEY_ID key
(UnorderedKey)
[warning] 51-51: [UnorderedKey] The APNS_PRODUCTION key should go before the APNS_TEAM_ID key
(UnorderedKey)
[warning] 53-53: [UnorderedKey] The APNS_BROADCAST_CHANNEL_ID key should go before the APNS_BUNDLE_ID key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 40 - 53, Replace machine-specific absolute paths
and account identifiers in the template with neutral placeholders: change
GOOGLE_APPLICATION_CREDENTIALS and APNS_KEY_PATH from local user paths to
generic placeholders like /path/to/credentials.json or
./secrets/credentials.json, and replace APNS_KEY_ID, APNS_TEAM_ID,
APNS_BUNDLE_ID and APNS_BROADCAST_CHANNEL_ID with clearly marked placeholder
tokens (e.g. <APNS_KEY_ID>, <APNS_TEAM_ID>, <APNS_BUNDLE_ID>,
<APNS_BROADCAST_CHANNEL_ID>); also ensure NOTIFICATION_TRIGGER_URL remains a
generic default (e.g. http://localhost:${NOTIFICATION_TRIGGER_PORT}) rather than
embedding any machine-specific values so the .env.example is portable and
non-sensitive.
Comment on lines
+56
to
+58
| const { env } = await import('$env/dynamic/private'); | ||
| const provisionerUrl = | ||
| env.PUBLIC_PROVISIONER_URL || env.PROVISIONER_URL || 'http://localhost:3001'; |
Contributor
There was a problem hiding this comment.
PUBLIC_PROVISIONER_URL is never accessible from $env/dynamic/private, and the dynamic re-import is redundant.
$env/dynamic/private only includes variables that do not begin with config.kit.env.publicPrefix (which defaults to PUBLIC_). Accessing env.PUBLIC_PROVISIONER_URL from this module will always return undefined, silently falling through to PROVISIONER_URL. Additionally, env was already imported at the module level on Line 1, making the await import(...) on Line 56 a redundant re-import that just shadows the outer binding.
Remove the dynamic re-import and read PROVISIONER_URL directly from the module-level env; if PUBLIC_PROVISIONER_URL must be supported, import it separately from $env/dynamic/public.
🐛 Proposed fix
export async function getDevicesWithTokens(): Promise<
{ token: string; platform: string; eName: string }[]
> {
- const { env } = await import('$env/dynamic/private');
- const provisionerUrl =
- env.PUBLIC_PROVISIONER_URL || env.PROVISIONER_URL || 'http://localhost:3001';
+ const provisionerUrl = env.PROVISIONER_URL || 'http://localhost:3001';
try {Apply the same fix to getDevicesByEName() (Lines 75-77).
📝 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
| const { env } = await import('$env/dynamic/private'); | |
| const provisionerUrl = | |
| env.PUBLIC_PROVISIONER_URL || env.PROVISIONER_URL || 'http://localhost:3001'; | |
| const provisionerUrl = env.PROVISIONER_URL || 'http://localhost:3001'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/control-panel/src/lib/services/notificationService.ts` around
lines 56 - 58, Remove the redundant dynamic re-import of env and stop reading
PUBLIC_PROVISIONER_URL from the private env binding: use the module-level env
already imported at the top to read PROVISIONER_URL when computing
provisionerUrl (i.e., replace the await import('$env/dynamic/private') usage and
the env.PUBLIC_PROVISIONER_URL fallback with env.PROVISIONER_URL ||
'http://localhost:3001'); if you need to support PUBLIC_PROVISIONER_URL, import
it explicitly from '$env/dynamic/public' and prefer that value. Apply the same
change to the getDevicesByEName() code path where env is re-imported and
PUBLIC_PROVISIONER_URL is referenced.
Comment on lines
+92
to
+115
| export async function sendBulkNotifications( | ||
| tokens: string[], | ||
| payload: NotificationPayload, | ||
| platform?: 'ios' | 'android' | ||
| ): Promise<{ sent: number; failed: number; errors: { token: string; error: string }[] }> { | ||
| const results = await Promise.all( | ||
| tokens.map(async (token) => { | ||
| const result = await sendNotification({ | ||
| token: token.trim(), | ||
| platform, | ||
| payload | ||
| }); | ||
| return { token: token.trim(), ...result }; | ||
| }) | ||
| ); | ||
|
|
||
| const sent = results.filter((r) => r.success).length; | ||
| const failed = results.filter((r) => !r.success); | ||
| return { | ||
| sent, | ||
| failed: failed.length, | ||
| errors: failed.map((r) => ({ token: r.token.slice(0, 20) + '...', error: r.error ?? 'Unknown' })) | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Promise.all over an unbounded token list with no concurrency cap.
All sendNotification requests are fired simultaneously. For large token arrays this can saturate the notification trigger's connection pool or hit OS socket limits. Consider limiting concurrency (e.g., process in chunks of 50–100).
♻️ Proposed fix using a simple chunk helper
+async function* chunks<T>(arr: T[], size: number) {
+ for (let i = 0; i < arr.length; i += size) yield arr.slice(i, i + size);
+}
export async function sendBulkNotifications(
tokens: string[],
payload: NotificationPayload,
platform?: 'ios' | 'android'
): Promise<{ sent: number; failed: number; errors: { token: string; error: string }[] }> {
- const results = await Promise.all(
- tokens.map(async (token) => {
- const result = await sendNotification({ token: token.trim(), platform, payload });
- return { token: token.trim(), ...result };
- })
- );
+ const results: ({ token: string } & SendResult)[] = [];
+ for await (const batch of chunks(tokens, 50)) {
+ const batchResults = await Promise.all(
+ batch.map(async (token) => {
+ const result = await sendNotification({ token: token.trim(), platform, payload });
+ return { token: token.trim(), ...result };
+ })
+ );
+ results.push(...batchResults);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/control-panel/src/lib/services/notificationService.ts` around
lines 92 - 115, The sendBulkNotifications function currently fires
sendNotification for every token via Promise.all, which can overwhelm resources;
modify sendBulkNotifications to process tokens in bounded concurrent batches
(e.g., chunks of 50 or 100) instead of all at once by creating a small chunk
helper or loop that slices tokens into batches, awaits Promise.all for each
batch of sendNotification calls, collects and aggregates results exactly as
before (use token.trim(), preserve returned fields success/error), and return
the same { sent, failed, errors } shape; reference sendBulkNotifications and
sendNotification when locating the change.
Comment on lines
+42
to
+47
| return json({ | ||
| success: true, | ||
| sent: result.sent, | ||
| failed: result.failed, | ||
| errors: result.errors | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing total field in success response — inconsistent with other bulk endpoints.
send-bulk-all and send-by-ename both return total: tokens.length. send-bulk omits it, making responses inconsistent for any consumer that inspects all three.
🔧 Proposed fix
return json({
success: true,
+ total: validTokens.length,
sent: result.sent,
failed: result.failed,
errors: result.errors
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@infrastructure/control-panel/src/routes/api/notifications/send-bulk/`+server.ts
around lines 42 - 47, The success JSON response is missing a total field,
causing inconsistency with other bulk endpoints; update the response returned by
the handler that builds the json({ success, sent: result.sent, failed:
result.failed, errors: result.errors }) to include total — either use total:
result.sent + result.failed or, if the tokens array is in scope, total:
tokens.length — so the returned object contains total, sent, failed, and errors;
change the return to json({... , total: <choose one of the two expressions
above> }).
Comment on lines
+10
to
+12
| if (!token || typeof token !== 'string') { | ||
| return json({ success: false, error: 'Missing or invalid token' }, { status: 400 }); | ||
| } |
Contributor
There was a problem hiding this comment.
Whitespace-only token passes validation.
!token is truthy only for ''/null/undefined. A token like ' ' (spaces only) passes, gets trimmed to '', and is forwarded to sendNotification as an empty string, causing a confusing downstream failure instead of a clean 400.
🔧 Proposed fix
- if (!token || typeof token !== 'string') {
+ if (!token || typeof token !== 'string' || !token.trim()) {📝 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 (!token || typeof token !== 'string') { | |
| return json({ success: false, error: 'Missing or invalid token' }, { status: 400 }); | |
| } | |
| if (!token || typeof token !== 'string' || !token.trim()) { | |
| return json({ success: false, error: 'Missing or invalid token' }, { status: 400 }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/control-panel/src/routes/api/notifications/send/`+server.ts
around lines 10 - 12, The current token check allows whitespace-only strings;
update the validation in the handler around the token variable so you trim it
and reject empty results before calling sendNotification: e.g., compute const
trimmedToken = typeof token === 'string' ? token.trim() : ''; then if
(!trimmedToken) return json({ success: false, error: 'Missing or invalid token'
}, { status: 400 }); and pass trimmedToken to sendNotification so
whitespace-only tokens produce a clean 400 instead of downstream failures.
Comment on lines
+6
to
+25
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query(` | ||
| CREATE TABLE "device_token" ( | ||
| "id" uuid NOT NULL DEFAULT uuid_generate_v4(), | ||
| "eName" character varying NOT NULL, | ||
| "token" character varying NOT NULL, | ||
| "platform" character varying NOT NULL, | ||
| "deviceId" character varying NOT NULL, | ||
| "createdAt" TIMESTAMP NOT NULL DEFAULT now(), | ||
| "updatedAt" TIMESTAMP NOT NULL DEFAULT now(), | ||
| CONSTRAINT "PK_device_token" PRIMARY KEY ("id") | ||
| ) | ||
| `); | ||
| await queryRunner.query( | ||
| `CREATE UNIQUE INDEX "UQ_device_token_ename_deviceid" ON "device_token" ("eName", "deviceId")`, | ||
| ); | ||
| await queryRunner.query( | ||
| `CREATE INDEX "IDX_device_token_ename" ON "device_token" ("eName")`, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing unique constraint and index on the token column.
DeviceTokenService.register (lines 22–25 of the service snippet) does a findOne({ where: { token } }) on every registration to enforce "one row per physical device token". Without a DB-level unique constraint there are two problems:
- TOCTOU race: two concurrent registrations with the same token both pass the
findOnecheck and then both insert, silently creating duplicate token rows that will cause duplicate notifications. - Performance: the lookup is a sequential scan on every registration; as the table grows this becomes a hot-path bottleneck.
🔧 Proposed fix
await queryRunner.query(
`CREATE UNIQUE INDEX "UQ_device_token_ename_deviceid" ON "device_token" ("eName", "deviceId")`,
);
await queryRunner.query(
`CREATE INDEX "IDX_device_token_ename" ON "device_token" ("eName")`,
);
+ await queryRunner.query(
+ `CREATE UNIQUE INDEX "UQ_device_token_token" ON "device_token" ("token")`,
+ );And the corresponding down:
+ await queryRunner.query(`DROP INDEX "UQ_device_token_token"`);
await queryRunner.query(`DROP INDEX "UQ_device_token_ename_deviceid"`);
await queryRunner.query(`DROP INDEX "IDX_device_token_ename"`);
await queryRunner.query(`DROP TABLE "device_token"`);With the unique index in place, register should use an ON CONFLICT upsert instead of the read-then-write pattern to eliminate the race entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/evault-core/src/migrations/1771880000000-DeviceToken.ts`
around lines 6 - 25, Add a DB-level unique constraint and index on the "token"
column in the migration: modify the up() in this migration to create a unique
index/constraint (e.g., CREATE UNIQUE INDEX "UQ_device_token_token" ON
"device_token" ("token")) and add the corresponding drop statement in down();
also update DeviceTokenService.register to stop using read-then-write and
instead perform an INSERT ... ON CONFLICT (token) DO UPDATE (or appropriate
upsert) so the DB enforces uniqueness and eliminates the TOCTOU race and
expensive sequential scans.
Comment on lines
+14
to
+48
| async register(registration: DeviceTokenRegistration): Promise<DeviceToken> { | ||
| const { eName, deviceId, platform, token } = registration; | ||
|
|
||
| // 1. Exact match: same eName + deviceId → update token/platform | ||
| const byEnameAndDevice = await this.deviceTokenRepository.findOne({ | ||
| where: { eName, deviceId }, | ||
| }); | ||
| if (byEnameAndDevice) { | ||
| byEnameAndDevice.token = token; | ||
| byEnameAndDevice.platform = platform; | ||
| byEnameAndDevice.updatedAt = new Date(); | ||
| return this.deviceTokenRepository.save(byEnameAndDevice); | ||
| } | ||
|
|
||
| // 2. Same token (same physical device) but different eName or deviceId → update eName/deviceId | ||
| const byToken = await this.deviceTokenRepository.findOne({ | ||
| where: { token }, | ||
| }); | ||
| if (byToken) { | ||
| byToken.eName = eName; | ||
| byToken.deviceId = deviceId; | ||
| byToken.platform = platform; | ||
| byToken.updatedAt = new Date(); | ||
| return this.deviceTokenRepository.save(byToken); | ||
| } | ||
|
|
||
| // 3. Both eName and token are new → create | ||
| const deviceToken = this.deviceTokenRepository.create({ | ||
| eName, | ||
| deviceId, | ||
| platform, | ||
| token, | ||
| }); | ||
| return this.deviceTokenRepository.save(deviceToken); | ||
| } |
Contributor
There was a problem hiding this comment.
TOCTOU race condition in register() can cause an unhandled unique-constraint error.
The three-path logic performs two sequential findOne queries followed by an un-guarded create/save. Under concurrent registrations (a real scenario for mobile apps), two calls sharing the same (eName, deviceId) can both observe null in the first lookup, skip the second, and both reach the create path — the second one will throw a DB unique-constraint violation that propagates to the caller as an unhandled exception.
The fix is to wrap the entire block in a transaction and catch the constraint error on save, or retry with a read after a conflict. A lighter-weight alternative on PostgreSQL/SQLite is to use the query-builder's .orUpdate() for each conflict path.
🔒 Proposed fix: wrap in a serialisable transaction
async register(registration: DeviceTokenRegistration): Promise<DeviceToken> {
const { eName, deviceId, platform, token } = registration;
+ return this.deviceTokenRepository.manager.transaction(async (manager) => {
+ const repo = manager.getRepository(DeviceToken);
- const byEnameAndDevice = await this.deviceTokenRepository.findOne({
+ const byEnameAndDevice = await repo.findOne({
where: { eName, deviceId },
});
if (byEnameAndDevice) {
byEnameAndDevice.token = token;
byEnameAndDevice.platform = platform;
byEnameAndDevice.updatedAt = new Date();
- return this.deviceTokenRepository.save(byEnameAndDevice);
+ return repo.save(byEnameAndDevice);
}
- const byToken = await this.deviceTokenRepository.findOne({
+ const byToken = await repo.findOne({
where: { token },
});
if (byToken) {
byToken.eName = eName;
byToken.deviceId = deviceId;
byToken.platform = platform;
byToken.updatedAt = new Date();
- return this.deviceTokenRepository.save(byToken);
+ return repo.save(byToken);
}
- const deviceToken = this.deviceTokenRepository.create({
+ const deviceToken = repo.create({
eName,
deviceId,
platform,
token,
});
- return this.deviceTokenRepository.save(deviceToken);
+ return repo.save(deviceToken);
+ });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/evault-core/src/services/DeviceTokenService.ts` around lines
14 - 48, DeviceTokenService.register has a TOCTOU race: sequential findOne calls
then create/save can trigger unique-constraint errors under concurrency; wrap
the register logic in a DB transaction (using your ORM's transactional API) and
either perform SELECT ... FOR UPDATE semantics or attempt an upsert/ON CONFLICT
(e.g., queryBuilder.orUpdate()/upsert) inside the transaction, and catch
unique-constraint DB errors on save to reload the existing row and return it (or
retry the lookup/update once) rather than letting the constraint propagate;
reference the register method, deviceTokenRepository.findOne,
deviceTokenRepository.create, deviceTokenRepository.save, and use a
transactional wrapper around that whole flow.
Comment on lines
165
to
170
| status.classList.remove("hidden", "success", "error"); | ||
| status.classList.add(data.success ? "success" : "error"); | ||
| } catch (err) { | ||
| status.textContent = err.message || "Network error"; | ||
| status.classList.remove("hidden", "success"); | ||
| status.classList.add("error"); |
Contributor
There was a problem hiding this comment.
classList.remove in the catch branch does not clear the "error" class, leaving it stale.
Line 165 correctly removes all three state classes ("hidden", "success", "error") before applying the new one. Line 169 (catch branch) only removes "hidden" and "success", so a residual "error" class from a previous failure is never explicitly cleared (it just gets re-added). This is harmless today but is semantically inconsistent and fragile if class logic changes.
🐛 Proposed fix
- status.classList.remove("hidden", "success");
+ status.classList.remove("hidden", "success", "error");
status.classList.add("error");📝 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
| status.classList.remove("hidden", "success", "error"); | |
| status.classList.add(data.success ? "success" : "error"); | |
| } catch (err) { | |
| status.textContent = err.message || "Network error"; | |
| status.classList.remove("hidden", "success"); | |
| status.classList.add("error"); | |
| status.classList.remove("hidden", "success", "error"); | |
| status.classList.add(data.success ? "success" : "error"); | |
| } catch (err) { | |
| status.textContent = err.message || "Network error"; | |
| status.classList.remove("hidden", "success", "error"); | |
| status.classList.add("error"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/notification-trigger/public/index.html` around lines 165 -
170, The catch branch that updates the status element currently calls
status.classList.remove("hidden", "success") but omits removing "error", leaving
a stale class; update the catch block handling for the status element (the code
manipulating status.textContent and status.classList) to remove all three state
classes ("hidden", "success", "error") before adding "error", mirroring the
success path so the status element's classes are always reset (refer to the
status variable and the catch branch in the event handler).
| ), | ||
| }), | ||
| ...(payload.sound && { sound: String(payload.sound) }), | ||
| ...(payload.badge !== undefined && { badge: Number(payload.badge) }), |
Contributor
There was a problem hiding this comment.
Number(payload.badge) can produce NaN and corrupt the push notification.
If payload.badge is a non-numeric string (e.g. "abc"), Number("abc") returns NaN. NaN will then be forwarded to APNs/FCM as the badge count, which can cause the notification to be rejected or silently dropped.
🐛 Proposed fix
- ...(payload.badge !== undefined && { badge: Number(payload.badge) }),
+ ...(payload.badge !== undefined && {
+ badge: (() => {
+ const n = Number(payload.badge);
+ return Number.isFinite(n) && n >= 0 ? Math.floor(n) : undefined;
+ })(),
+ }),Or simpler — validate and reject early:
+ if (payload.badge !== undefined && !Number.isFinite(Number(payload.badge))) {
+ return res.status(400).json({ success: false, error: "'badge' must be a non-negative integer" });
+ }📝 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
| ...(payload.badge !== undefined && { badge: Number(payload.badge) }), | |
| ...(payload.badge !== undefined && { | |
| badge: (() => { | |
| const n = Number(payload.badge); | |
| return Number.isFinite(n) && n >= 0 ? Math.floor(n) : undefined; | |
| })(), | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/notification-trigger/src/index.ts` at line 67, The current
spread uses Number(payload.badge) which can produce NaN; update the logic in
index.ts around the notification payload construction to validate the badge
before including it: compute a numeric value from payload.badge (e.g., const
badge = Number(payload.badge)), then only spread { badge: badge } when
payload.badge !== undefined AND Number.isFinite(badge) (and optionally badge >=
0 / Number.isInteger(badge) if you require integers); otherwise omit the badge
field or reject the request. Ensure you reference payload.badge and the payload
construction site so NaN is never forwarded to APNs/FCM.
Comment on lines
14
to
24
| try { | ||
| admin.initializeApp({ | ||
| credential: admin.credential.applicationDefault(), | ||
| }); | ||
| initialized = true; | ||
| console.log("[FCM] Initialized"); | ||
| return true; | ||
| } catch (err) { | ||
| console.error("[FCM] Failed to initialize:", err); | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for initializeApp calls
rg -nP --type=ts -C3 'initializeApp\s*\('Repository: MetaState-Prototype-Project/prototype
Length of output: 6034
🏁 Script executed:
#!/bin/bash
# Search for firebase-admin imports
rg -nP --type=ts -C3 'from\s+["'"'"']firebase-admin["'"'"']'Repository: MetaState-Prototype-Project/prototype
Length of output: 1775
🏁 Script executed:
#!/bin/bash
# Check the full context of fcm.ts to understand the initialization flow
cat -n infrastructure/notification-trigger/src/senders/fcm.tsRepository: MetaState-Prototype-Project/prototype
Length of output: 2665
🏁 Script executed:
#!/bin/bash
# Check if fcm.ts is imported anywhere that might also initialize admin
rg -nP --type=ts "from.*fcm|import.*fcm"Repository: MetaState-Prototype-Project/prototype
Length of output: 264
🏁 Script executed:
#!/bin/bash
# Verify firebase-admin apps property is documented/used in the codebase
rg -nP --type=ts "admin\.apps|\.apps\.length"Repository: MetaState-Prototype-Project/prototype
Length of output: 63
Check if Firebase Admin is already initialized before calling initializeApp() to prevent false initialization failures.
If firebase-admin was initialized by another module in the same process, admin.initializeApp() on line 15 throws "app already exists". The catch block masks this as a generic initialization failure, returning false even though messaging is usable. This causes all sendFcm() calls to incorrectly fail with "FCM not configured".
Use admin.apps.length > 0 to detect existing initialization and return true instead of attempting re-initialization.
Proposed fix
export function initFcm(): boolean {
if (initialized) return true;
+ if (admin.apps.length > 0) {
+ initialized = true;
+ 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 `@infrastructure/notification-trigger/src/senders/fcm.ts` around lines 14 - 24,
The initialize block currently calls admin.initializeApp() unconditionally and
treats any thrown error as a real failure; change it to first check if
admin.apps && admin.apps.length > 0 and, if so, set initialized = true and
return true (so existing initialization isn't treated as a failure), otherwise
call admin.initializeApp() inside the try; update the catch to only log and
return false for genuine errors while not masking the "app already exists" case;
reference the existing admin.initializeApp() call and the initialized variable
in this file (senders/fcm.ts) when making the change.
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.
Description of change
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores