Open
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces the notification plugin (JS + Rust), adjusts mobile permission name, adds push-token retrieval via a new API (NotificationService.getPushToken), surfaces token in onboarding UI, and adds push notification setup documentation for Android/iOS. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Onboarding as Onboarding Page
participant NotifService as NotificationService
participant PluginAPI as Plugin API
participant Backend
User->>Onboarding: Open onboarding page
Onboarding->>NotifService: getPushToken()
NotifService->>NotifService: Request/verify notification permission
alt permission granted
NotifService->>NotifService: Check platform (mobile vs desktop)
alt mobile
NotifService->>PluginAPI: registerForPushNotifications()
PluginAPI-->>NotifService: return FCM/APNs token
NotifService-->>Onboarding: token
Onboarding->>User: show token
Onboarding->>Backend: (optional) send token for registration
else desktop
NotifService-->>Onboarding: undefined
end
else permission denied
NotifService-->>Onboarding: error
Onboarding->>User: show error message
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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).
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