feat: add redirection for notifications + API integrations#4079
Open
bistaastha wants to merge 58 commits intofyle-fix-setup-keysfrom
Open
feat: add redirection for notifications + API integrations#4079bistaastha wants to merge 58 commits intofyle-fix-setup-keysfrom
bistaastha wants to merge 58 commits intofyle-fix-setup-keysfrom
Conversation
…yle-mobile-app into fyle-api-integration-for-notifs
WalkthroughThis magnificent pull request orchestrates a complete push notification architecture overhaul! The system now harnesses PushNotificationService as the supreme controller, integrating device token registration, lifecycle management, permission handling, and deep-link navigation through strategically refactored components and services across mobile and web platforms. Changes
Sequence Diagram(s)sequenceDiagram
participant App as AppComponent
participant PNS as PushNotificationService
participant CapPN as Capacitor PushNotifications
participant OUS as OrgUserService
participant DLS as DeepLinkService
participant TS as TrackingService
rect rgba(100, 150, 255, 0.5)
Note over App,TS: Initialization Flow
App->>PNS: initializeNotificationClickListener()
PNS->>CapPN: addListener('pushNotificationActionPerformed')
CapPN-->>PNS: listener registered
end
rect rgba(150, 200, 100, 0.5)
Note over App,TS: Permission & Registration Flow
App->>PNS: initializePushNotifications()
PNS->>CapPN: checkPermissions()
CapPN-->>PNS: PermissionStatus { receive: 'granted' }
PNS->>CapPN: addListener('registration')
PNS->>CapPN: register()
CapPN-->>PNS: token event
PNS->>OUS: sendDeviceToken(token)
OUS-->>PNS: success
PNS->>TS: track registration event
end
rect rgba(255, 150, 100, 0.5)
Note over App,TS: Notification Click Handler
CapPN-->>PNS: pushNotificationActionPerformed event
PNS->>DLS: getJsonFromUrl(clickUrl)
DLS-->>PNS: parsed deep link
PNS->>TS: track notification click
PNS->>DLS: redirect(deepLink)
end
rect rgba(200, 100, 200, 0.5)
Note over App,TS: Logout Cleanup
App->>PNS: unregister()
PNS->>CapPN: unregister()
CapPN-->>PNS: unregistered
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The pull request introduces a sophisticated, multi-layered architectural change with significant refactoring across 15+ files. The heterogeneity spans service refactoring (PushNotificationService), model expansions, component integration, test infrastructure setup with mocks, and lifecycle management across multiple pages. While changes follow consistent patterns, each affected component requires individual reasoning regarding integration points, permission flows, and lifecycle hook implementations. The comprehensive test suite adds verification complexity, and the introduction of a new mock layer demands careful review of test environment configuration. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
bistaastha
commented
Jan 23, 2026
| this.userEventService.onLogout(() => { | ||
| this.trackingService.onSignOut(); | ||
| this.freshChatService.destroy(); | ||
| this.pushNotificationService.unregister(); |
Contributor
Author
There was a problem hiding this comment.
Unregisters a token on log out
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/app/app.component.ts`:
- Around line 296-300: The logout handler registered with
userEventService.onLogout calls pushNotificationService.unregister() but doesn't
handle its returned Promise and may double-unregister; update the onLogout
callback (in the block where trackingService.onSignOut(),
freshChatService.destroy(), pushNotificationService.unregister(),
isSwitchedToDelegator = false are called) to guard and handle the promise: first
check a registration state (e.g. pushNotificationService.isRegistered or an
internal flag) before calling unregister, and if keeping the call always, ensure
you handle the Promise by awaiting it or attaching .catch(...) to swallow or log
harmless "already unregistered" errors to avoid unhandled rejections and
duplicate unregister attempts.
In `@src/app/core/services/push-notification.service.ts`:
- Around line 45-49: The registration listener creates an unused local variable
tokenValue which fails lint; update the
PushNotifications.addListener('registration', ...) handler to either remove the
unused assignment or use the token value intentionally (e.g., pass token.value
into this.trackingService.eventTrack or include it in a payload) — modify the
listener surrounding Token/tokenValue and trackingService.eventTrack so there
are no unused variables.
In
`@src/app/fyle/notifications/email-notifications/email-notifications.component.html`:
- Around line 62-69: The template is passing the string 'push' to
toggleAllNotifications but the component API expects 'email' | 'mobile' and
reads notification.mobile, causing state sync/type errors; update the template
instances (both the header checkbox at lines shown and the individual row
checkboxes referenced around 85-87) to pass 'mobile' instead of 'push' and
ensure the bound property selectAllMobile and any
(ngModelChange)="toggleAllNotifications($event, 'mobile')" usages align with the
component's toggleAllNotifications method and notification.mobile property so
state persists and strict typing passes.
In
`@src/app/fyle/notifications/email-notifications/email-notifications.component.ts`:
- Line 87: Wrap calls to PushNotifications.checkPermissions() used in the
forkJoin so a rejected promise doesn’t bubble up (use from(...) and
pipe(catchError(...)) or otherwise convert to an observable that returns a safe
default); update the forkJoin that reads PushNotifications.checkPermissions()
(and the similar usage around the block for lines ~273-285) to catch errors and
return a sensible fallback permissions object so the UI logic (e.g.,
showMobilePushColumn) continues to work on web/tests/non-native platforms.
Comment on lines
296
to
300
| this.userEventService.onLogout(() => { | ||
| this.trackingService.onSignOut(); | ||
| this.freshChatService.destroy(); | ||
| this.pushNotificationService.unregister(); | ||
| this.isSwitchedToDelegator = false; |
There was a problem hiding this comment.
Handle unregister promise (and avoid double-unregister).
unregister() returns a Promise; unhandled rejection can surface, and the service already auto-unregisters on logout. Pick one place, and if you keep this call, guard it.
🐛 Suggested fix (keep call, guard it)
- this.pushNotificationService.unregister();
+ void this.pushNotificationService.unregister().catch(noop);🤖 Prompt for AI Agents
In `@src/app/app.component.ts` around lines 296 - 300, The logout handler
registered with userEventService.onLogout calls
pushNotificationService.unregister() but doesn't handle its returned Promise and
may double-unregister; update the onLogout callback (in the block where
trackingService.onSignOut(), freshChatService.destroy(),
pushNotificationService.unregister(), isSwitchedToDelegator = false are called)
to guard and handle the promise: first check a registration state (e.g.
pushNotificationService.isRegistered or an internal flag) before calling
unregister, and if keeping the call always, ensure you handle the Promise by
awaiting it or attaching .catch(...) to swallow or log harmless "already
unregistered" errors to avoid unhandled rejections and duplicate unregister
attempts.
src/app/fyle/notifications/email-notifications/email-notifications.component.html
Show resolved
Hide resolved
| saveText: '' | 'Saved' | 'Saving...' = ''; | ||
|
|
||
| showMobilePushColumn = true; | ||
| showMobilePushColumn = false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant sections
find . -type f -name "email-notifications.component.ts" | head -5Repository: fylein/fyle-mobile-app
Length of output: 147
🏁 Script executed:
# Look at the file to understand the context around lines 87 and 273-285
if [ -f "src/app/fyle/notifications/email-notifications/email-notifications.component.ts" ]; then
wc -l "src/app/fyle/notifications/email-notifications/email-notifications.component.ts"
fiRepository: fylein/fyle-mobile-app
Length of output: 149
🏁 Script executed:
# Read the relevant sections of the file
if [ -f "src/app/fyle/notifications/email-notifications/email-notifications.component.ts" ]; then
cat -n "src/app/fyle/notifications/email-notifications/email-notifications.component.ts" | sed -n '80,95p'
echo "---"
cat -n "src/app/fyle/notifications/email-notifications/email-notifications.component.ts" | sed -n '270,290p'
fiRepository: fylein/fyle-mobile-app
Length of output: 1294
🏁 Script executed:
# Check imports to see if error handling operators are already imported
if [ -f "src/app/fyle/notifications/email-notifications/email-notifications.component.ts" ]; then
cat -n "src/app/fyle/notifications/email-notifications/email-notifications.component.ts" | sed -n '1,30p'
fiRepository: fylein/fyle-mobile-app
Length of output: 1876
🌐 Web query:
Capacitor PushNotifications checkPermissions behavior error handling web plugin unavailable
💡 Result:
PushNotifications.checkPermissions() is not available on the Web platform because the Capacitor Push Notifications plugin is for native push (iOS/Android), not browser push. If you call it in a browser (e.g., during ionic serve), Capacitor will reject the promise with an error like “PushNotifications does not have web implementation” / “Not implemented on web.” [2], [5]
How to handle it
- Guard by platform (recommended)
import { Capacitor } from '@capacitor/core'; import { PushNotifications } from '@capacitor/push-notifications'; if (Capacitor.isNativePlatform()) { const perm = await PushNotifications.checkPermissions(); // ... }
- Or catch and ignore “unimplemented/unavailable”
Capacitor’s recommended web error handling is to throw/receive Unimplemented (not implemented on this platform) or Unavailable (API/prereq missing) and handle accordingly. [1], [3]
If you actually need Web Push
Use a Service Worker + Web Push implementation (separate from @capacitor/push-notifications), since that plugin is specifically for native push. [5]
🏁 Script executed:
# Check if the component has platform guards for native-only code
if [ -f "src/app/fyle/notifications/email-notifications/email-notifications.component.ts" ]; then
rg -n "Capacitor|isNativePlatform|platform" "src/app/fyle/notifications/email-notifications/email-notifications.component.ts"
fiRepository: fylein/fyle-mobile-app
Length of output: 493
🏁 Script executed:
# Check the full context around the forkJoin to see if there are any guards
if [ -f "src/app/fyle/notifications/email-notifications/email-notifications.component.ts" ]; then
cat -n "src/app/fyle/notifications/email-notifications/email-notifications.component.ts" | sed -n '265,287p'
fiRepository: fylein/fyle-mobile-app
Length of output: 1022
🏁 Script executed:
# Search for test files related to this component
find . -type f -name "*email-notifications*.spec.ts" | head -3Repository: fylein/fyle-mobile-app
Length of output: 152
🏁 Script executed:
# Check if this component's ngOnInit or similar lifecycle methods have platform checks
ast-grep --pattern $'ngOnInit() {
$$$
}'Repository: fylein/fyle-mobile-app
Length of output: 48
🏁 Script executed:
# Check the test file to see how PushNotifications is handled
if [ -f "src/app/fyle/notifications/email-notifications/email-notifications.component.spec.ts" ]; then
wc -l "src/app/fyle/notifications/email-notifications/email-notifications.component.spec.ts"
fiRepository: fylein/fyle-mobile-app
Length of output: 154
🏁 Script executed:
# Read the test file
if [ -f "src/app/fyle/notifications/email-notifications/email-notifications.component.spec.ts" ]; then
cat -n "src/app/fyle/notifications/email-notifications/email-notifications.component.spec.ts" | head -100
fiRepository: fylein/fyle-mobile-app
Length of output: 5228
🏁 Script executed:
# Search for PushNotifications mocking in test files
rg -n "PushNotifications|checkPermissions" "src/app/fyle/notifications/email-notifications/email-notifications.component.spec.ts" -A 3Repository: fylein/fyle-mobile-app
Length of output: 490
Guard PushNotifications.checkPermissions errors like a warrior, sir.
The forkJoin will crash and burn if the Capacitor plugin is unavailable (web, tests, or non-native platforms). checkPermissions() rejects with "not implemented" errors, and there's no error handling. Add a safe fallback with catchError to handle this gracefully.
The victory code
-import { finalize, forkJoin, from, tap } from 'rxjs';
+import { catchError, finalize, forkJoin, from, of, tap } from 'rxjs';
- forkJoin({
- isPushNotifUiEnabled: this.launchDarklyService.getVariation('show_push_notif_ui', false),
- permissionStatus: from(PushNotifications.checkPermissions()),
- }).subscribe(({ isPushNotifUiEnabled, permissionStatus }) => {
+ const permissionStatus$ = from(PushNotifications.checkPermissions()).pipe(
+ catchError(() => of({ receive: 'denied' as const })),
+ );
+
+ forkJoin({
+ isPushNotifUiEnabled: this.launchDarklyService.getVariation('show_push_notif_ui', false),
+ permissionStatus: permissionStatus$,
+ }).subscribe(({ isPushNotifUiEnabled, permissionStatus }) => {
this.showMobilePushColumn = isPushNotifUiEnabled && isPushColumnSupportedForTitle;
this.isPushPermissionDenied =
isPushNotifUiEnabled &&
isPushColumnSupportedForTitle &&
permissionStatus.receive !== 'granted';
});Also applies to: 273-285
🤖 Prompt for AI Agents
In
`@src/app/fyle/notifications/email-notifications/email-notifications.component.ts`
at line 87, Wrap calls to PushNotifications.checkPermissions() used in the
forkJoin so a rejected promise doesn’t bubble up (use from(...) and
pipe(catchError(...)) or otherwise convert to an observable that returns a safe
default); update the forkJoin that reads PushNotifications.checkPermissions()
(and the similar usage around the block for lines ~273-285) to catch errors and
return a sensible fallback permissions object so the UI logic (e.g.,
showMobilePushColumn) continues to work on web/tests/non-native platforms.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/app/core/services/push-notification.service.ts`:
- Around line 53-61: The fire-and-forget subscribe in addRegistrationListener
ignores errors from orgUserService.sendDeviceToken; update
addRegistrationListener to pass error and complete callbacks to the subscribe
call (or use pipe(tap(), catchError()) and subscribe) so failures are
logged/handled—e.g., call a logging/tracking method (trackingService.eventTrack
or a logger) with the error and optionally retry/backoff behavior; ensure you
reference sendDeviceToken on orgUserService and still call
trackingService.eventTrack('Push Notification Registered') after successful
registration handling.
- Around line 63-65: The current addRegistrationErrorListener swallows errors;
update addRegistrationErrorListener so the
PushNotifications.addListener('registrationError', ...) callback accepts the
error payload and forwards it to your app's telemetry/logging (e.g., call
this.logger.error(error) and/or this.telemetryService.captureException(error)),
falling back to console.error if no logger is available; ensure you reference
PushNotifications.addListener and addRegistrationErrorListener when making the
change so the registrationError handler records the error instead of returning
undefined.
Comment on lines
53
to
61
| private addRegistrationListener(): void { | ||
| PushNotifications.addListener('registration', (token: Token) => { | ||
| const tokenValue = token?.value; | ||
| if (tokenValue) { | ||
| this.orgUserService.sendDeviceToken(tokenValue).subscribe(); | ||
| } | ||
| this.trackingService.eventTrack('Push Notification Registered'); | ||
| }); | ||
| } |
There was a problem hiding this comment.
One small punch, boss! Fire-and-forget subscription needs error handling.
When sendDeviceToken fails silently, the device won't receive push notifications and nobody will know why. Rajini says: "If you don't handle errors, errors will handle you!"
🐛 Proposed fix to handle subscription errors
private addRegistrationListener(): void {
PushNotifications.addListener('registration', (token: Token) => {
const tokenValue = token?.value;
if (tokenValue) {
- this.orgUserService.sendDeviceToken(tokenValue).subscribe();
+ this.orgUserService.sendDeviceToken(tokenValue).subscribe({
+ error: () => {
+ // Token registration failed - user won't receive push notifications
+ },
+ });
}
this.trackingService.eventTrack('Push Notification Registered');
});
}🤖 Prompt for AI Agents
In `@src/app/core/services/push-notification.service.ts` around lines 53 - 61, The
fire-and-forget subscribe in addRegistrationListener ignores errors from
orgUserService.sendDeviceToken; update addRegistrationListener to pass error and
complete callbacks to the subscribe call (or use pipe(tap(), catchError()) and
subscribe) so failures are logged/handled—e.g., call a logging/tracking method
(trackingService.eventTrack or a logger) with the error and optionally
retry/backoff behavior; ensure you reference sendDeviceToken on orgUserService
and still call trackingService.eventTrack('Push Notification Registered') after
successful registration handling.
Comment on lines
63
to
65
| private addRegistrationErrorListener(): void { | ||
| PushNotifications.addListener('registrationError', () => undefined); | ||
| PushNotifications.addListener('pushNotificationActionPerformed', () => undefined); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Enna boss, registration error is going silently into the void!
When push registration fails, this handler does nothing. That's like Rajini walking away without a punchline! At minimum, track the error for debugging production issues.
♻️ Proposed fix to track registration errors
private addRegistrationErrorListener(): void {
- PushNotifications.addListener('registrationError', () => undefined);
+ PushNotifications.addListener('registrationError', (error) => {
+ this.trackingService.eventTrack('Push Notification Registration Failed', { error: error?.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
| private addRegistrationErrorListener(): void { | |
| PushNotifications.addListener('registrationError', () => undefined); | |
| PushNotifications.addListener('pushNotificationActionPerformed', () => undefined); | |
| } | |
| private addRegistrationErrorListener(): void { | |
| PushNotifications.addListener('registrationError', (error) => { | |
| this.trackingService.eventTrack('Push Notification Registration Failed', { error: error?.error }); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@src/app/core/services/push-notification.service.ts` around lines 63 - 65, The
current addRegistrationErrorListener swallows errors; update
addRegistrationErrorListener so the
PushNotifications.addListener('registrationError', ...) callback accepts the
error payload and forwards it to your app's telemetry/logging (e.g., call
this.logger.error(error) and/or this.telemetryService.captureException(error)),
falling back to console.error if no logger is available; ensure you reference
PushNotifications.addListener and addRegistrationErrorListener when making the
change so the registrationError handler records the error instead of returning
undefined.
bistaastha
changed the base branch from
fyle-api-integration-for-notifs
to
fyle-fix-setup-keys
bistaastha
changed the title
…dd-redirection-for-notifications
bistaastha
commented
Jan 30, 2026
__mocks__/@capacitor/app.ts
Outdated
| @@ -0,0 +1,33 @@ | |||
| /* eslint-disable custom-rules/one-interface-per-file */ | |||
Contributor
Author
There was a problem hiding this comment.
Added this as per https://github.com/ionic-team/cap-plugin-mock-jasmine/tree/main
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/app/core/services/push-notification.service.spec.ts`:
- Around line 39-43: The test currently mutates the PushNotifications module by
setting Object.defineProperty(PushNotifications, 'unregister', ...) using
unregisterSpy which can leak across tests; instead use the existing mock module
or a spy-based approach: replace the direct defineProperty with either
importing/using the mock from __mocks__/@capacitor/push-notifications.ts to
provide a mocked unregister implementation or use spyOn(PushNotifications,
'unregister').and.resolveTo() (or spyOnPrototype if needed) to avoid permanently
mutating the module; ensure any spy is restored after the test (e.g.,
jasmine.restoreAllMocks/afterEach) so unregisterSpy/PushNotifications are not
left altered.
- Around line 95-113: The two tests calling service.checkPermissions() are
duplicates; keep one and either remove the other or change it to cover a
different scenario. Remove the redundant it block (the second or first) that
spies on PushNotifications.checkPermissions and asserts the same expectations,
or alternatively modify the duplicate to test an alternate outcome (e.g.,
resolved value of { receive: 'granted' } or an error path) so that the spec for
service.checkPermissions() and the spy on PushNotifications.checkPermissions()
covers distinct behavior.
In
`@src/app/fyle/notifications/notifications-beta/notifications-beta.page.spec.ts`:
- Around line 80-83: The tests currently spy on the global PushNotifications
methods (PushNotifications.checkPermissions and PushNotifications.register)
while the component under test uses PushNotificationService
(pushNotificationServiceSpy); remove the direct spies on PushNotifications to
avoid confusion and rely solely on pushNotificationServiceSpy; specifically
delete the spyOn calls referencing PushNotifications and ensure
pushNotificationServiceSpy implements/mock the same behaviors for
checkPermissions and register so the component's uses of
PushNotificationService.checkPermissions and PushNotificationService.register
are the only mocks exercised.
Comment on lines
+39
to
+43
| unregisterSpy = jasmine.createSpy('unregister').and.resolveTo(); | ||
| Object.defineProperty(PushNotifications, 'unregister', { | ||
| value: unregisterSpy, | ||
| writable: true, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mind it! This mocking style is a bit risky, macha.
Using Object.defineProperty to mock Capacitor methods directly mutates the module. This can leak across tests if not properly cleaned up. Consider using the mock module approach consistently (as shown in __mocks__/@capacitor/push-notifications.ts) or spying on the prototype.
🤖 Prompt for AI Agents
In `@src/app/core/services/push-notification.service.spec.ts` around lines 39 -
43, The test currently mutates the PushNotifications module by setting
Object.defineProperty(PushNotifications, 'unregister', ...) using unregisterSpy
which can leak across tests; instead use the existing mock module or a spy-based
approach: replace the direct defineProperty with either importing/using the mock
from __mocks__/@capacitor/push-notifications.ts to provide a mocked unregister
implementation or use spyOn(PushNotifications, 'unregister').and.resolveTo() (or
spyOnPrototype if needed) to avoid permanently mutating the module; ensure any
spy is restored after the test (e.g., jasmine.restoreAllMocks/afterEach) so
unregisterSpy/PushNotifications are not left altered.
Comment on lines
+95
to
+113
| it('should return the permission status', async () => { | ||
| const permission = { receive: 'denied' } as any; | ||
| const checkPermissionsSpy = spyOn(PushNotifications, 'checkPermissions').and.resolveTo(permission); | ||
|
|
||
| const result = await service.checkPermissions(); | ||
|
|
||
| expect(checkPermissionsSpy).toHaveBeenCalledTimes(1); | ||
| expect(result).toBe(permission); | ||
| }); | ||
|
|
||
| it('should delegate to PushNotifications.checkPermissions', async () => { | ||
| const permission = { receive: 'denied' } as any; | ||
| const checkPermissionsSpy = spyOn(PushNotifications, 'checkPermissions').and.resolveTo(permission); | ||
|
|
||
| const result = await service.checkPermissions(); | ||
|
|
||
| expect(checkPermissionsSpy).toHaveBeenCalledTimes(1); | ||
| expect(result).toBe(permission); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Superstar says: Why write the same scene twice, macha?
Lines 95-103 and 105-113 are identical tests with the same assertions. One of them should be removed or differentiated to test a distinct scenario.
♻️ Proposed fix - remove duplicate test
describe('checkPermissions():', () => {
it('should return the permission status', async () => {
const permission = { receive: 'denied' } as any;
const checkPermissionsSpy = spyOn(PushNotifications, 'checkPermissions').and.resolveTo(permission);
const result = await service.checkPermissions();
expect(checkPermissionsSpy).toHaveBeenCalledTimes(1);
expect(result).toBe(permission);
});
- it('should delegate to PushNotifications.checkPermissions', async () => {
- const permission = { receive: 'denied' } as any;
- const checkPermissionsSpy = spyOn(PushNotifications, 'checkPermissions').and.resolveTo(permission);
-
- const result = await service.checkPermissions();
-
- expect(checkPermissionsSpy).toHaveBeenCalledTimes(1);
- expect(result).toBe(permission);
- });
-
it('should init listeners and register when permission is granted', async () => {📝 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
| it('should return the permission status', async () => { | |
| const permission = { receive: 'denied' } as any; | |
| const checkPermissionsSpy = spyOn(PushNotifications, 'checkPermissions').and.resolveTo(permission); | |
| const result = await service.checkPermissions(); | |
| expect(checkPermissionsSpy).toHaveBeenCalledTimes(1); | |
| expect(result).toBe(permission); | |
| }); | |
| it('should delegate to PushNotifications.checkPermissions', async () => { | |
| const permission = { receive: 'denied' } as any; | |
| const checkPermissionsSpy = spyOn(PushNotifications, 'checkPermissions').and.resolveTo(permission); | |
| const result = await service.checkPermissions(); | |
| expect(checkPermissionsSpy).toHaveBeenCalledTimes(1); | |
| expect(result).toBe(permission); | |
| }); | |
| it('should return the permission status', async () => { | |
| const permission = { receive: 'denied' } as any; | |
| const checkPermissionsSpy = spyOn(PushNotifications, 'checkPermissions').and.resolveTo(permission); | |
| const result = await service.checkPermissions(); | |
| expect(checkPermissionsSpy).toHaveBeenCalledTimes(1); | |
| expect(result).toBe(permission); | |
| }); |
🤖 Prompt for AI Agents
In `@src/app/core/services/push-notification.service.spec.ts` around lines 95 -
113, The two tests calling service.checkPermissions() are duplicates; keep one
and either remove the other or change it to cover a different scenario. Remove
the redundant it block (the second or first) that spies on
PushNotifications.checkPermissions and asserts the same expectations, or
alternatively modify the duplicate to test an alternate outcome (e.g., resolved
value of { receive: 'granted' } or an error path) so that the spec for
service.checkPermissions() and the spy on PushNotifications.checkPermissions()
covers distinct behavior.
Comment on lines
80
to
+83
| spyOn(PushNotifications as any, 'checkPermissions').and.resolveTo({ receive: 'granted' } as any); | ||
| spyOn(PushNotifications as any, 'register').and.resolveTo(); | ||
| spyOn(App as any, 'addListener').and.returnValue( | ||
| Promise.resolve({ | ||
| remove: jasmine.createSpy('remove'), | ||
| } as any), | ||
| ); | ||
| launchDarklyServiceSpy.getVariation.and.returnValue(of(true)); | ||
| pushNotificationServiceSpy.checkPermissions.and.resolveTo({ receive: 'granted' } as any); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM, but one small confusion, boss!
Lines 80-81 spy on PushNotifications directly while also providing a pushNotificationServiceSpy. Since the component uses PushNotificationService (not PushNotifications directly), the direct spies on lines 80-81 may be unnecessary. Consider removing them to reduce confusion about which mock is actually in use.
♻️ Proposed cleanup
- spyOn(PushNotifications as any, 'checkPermissions').and.resolveTo({ receive: 'granted' } as any);
- spyOn(PushNotifications as any, 'register').and.resolveTo();
launchDarklyServiceSpy.getVariation.and.returnValue(of(true));
pushNotificationServiceSpy.checkPermissions.and.resolveTo({ receive: 'granted' } as any);📝 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
| spyOn(PushNotifications as any, 'checkPermissions').and.resolveTo({ receive: 'granted' } as any); | |
| spyOn(PushNotifications as any, 'register').and.resolveTo(); | |
| spyOn(App as any, 'addListener').and.returnValue( | |
| Promise.resolve({ | |
| remove: jasmine.createSpy('remove'), | |
| } as any), | |
| ); | |
| launchDarklyServiceSpy.getVariation.and.returnValue(of(true)); | |
| pushNotificationServiceSpy.checkPermissions.and.resolveTo({ receive: 'granted' } as any); | |
| launchDarklyServiceSpy.getVariation.and.returnValue(of(true)); | |
| pushNotificationServiceSpy.checkPermissions.and.resolveTo({ receive: 'granted' } as any); |
🤖 Prompt for AI Agents
In
`@src/app/fyle/notifications/notifications-beta/notifications-beta.page.spec.ts`
around lines 80 - 83, The tests currently spy on the global PushNotifications
methods (PushNotifications.checkPermissions and PushNotifications.register)
while the component under test uses PushNotificationService
(pushNotificationServiceSpy); remove the direct spies on PushNotifications to
avoid confusion and rely solely on pushNotificationServiceSpy; specifically
delete the spyOn calls referencing PushNotifications and ensure
pushNotificationServiceSpy implements/mock the same behaviors for
checkPermissions and register so the component's uses of
PushNotificationService.checkPermissions and PushNotificationService.register
are the only mocks exercised.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/app/fyle/notifications/notifications-beta/notifications-beta.page.spec.ts`:
- Around line 496-551: The describe block is misnamed: tests call
handleAppStateChange but the suite is named startAppStateListener(); rename the
describe from "startAppStateListener():" to "handleAppStateChange():" so the
suite name matches the tested method (or alternatively, change tests to call
startAppStateListener and trigger its callback), updating references in this
spec where startAppStateListener appears and keeping the existing test bodies
that call (component as any).handleAppStateChange.
Comment on lines
+496
to
+551
| describe('startAppStateListener():', () => { | ||
| it('should exit early when app is inactive', async () => { | ||
| component.showMobilePushColumn = true; | ||
|
|
||
| await (component as any).handleAppStateChange({ isActive: false }); | ||
|
|
||
| expect(pushNotificationService.checkPermissions).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should exit early when push column is hidden', async () => { | ||
| component.showMobilePushColumn = false; | ||
|
|
||
| await (component as any).handleAppStateChange({ isActive: true }); | ||
|
|
||
| expect(pushNotificationService.checkPermissions).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should clear isPushPermissionDenied and register for push notifications when permission becomes granted', async () => { | ||
| component.showMobilePushColumn = true; | ||
| component.isPushPermissionDenied = true; | ||
|
|
||
| pushNotificationService.checkPermissions.and.resolveTo({ receive: 'granted' } as any); | ||
|
|
||
| await (component as any).handleAppStateChange({ isActive: true }); | ||
|
|
||
| expect(component.isPushPermissionDenied).toBeFalse(); | ||
| expect(pushNotificationService.addRegistrationListener).toHaveBeenCalledTimes(1); | ||
| expect(pushNotificationService.register).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('should keep isPushPermissionDenied false when permission remains granted', async () => { | ||
| component.showMobilePushColumn = true; | ||
| component.isPushPermissionDenied = false; | ||
|
|
||
| pushNotificationService.checkPermissions.and.resolveTo({ receive: 'granted' } as any); | ||
|
|
||
| await (component as any).handleAppStateChange({ isActive: true }); | ||
|
|
||
| expect(component.isPushPermissionDenied).toBeFalse(); | ||
| expect(pushNotificationService.addRegistrationListener).not.toHaveBeenCalled(); | ||
| expect(pushNotificationService.register).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should set isPushPermissionDenied to true when permission is denied on app resume', async () => { | ||
| component.showMobilePushColumn = true; | ||
| component.isPushPermissionDenied = false; | ||
|
|
||
| pushNotificationService.checkPermissions.and.resolveTo({ receive: 'denied' } as any); | ||
|
|
||
| await (component as any).handleAppStateChange({ isActive: true }); | ||
|
|
||
| expect(component.isPushPermissionDenied).toBeTrue(); | ||
| expect(pushNotificationService.addRegistrationListener).not.toHaveBeenCalled(); | ||
| expect(pushNotificationService.register).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Style ah? Test suite name says one thing, but tests do another, boss!
The test suite is named startAppStateListener(): but every test inside calls handleAppStateChange method. This is like announcing a Rajinikanth movie but showing something else entirely! Either rename the suite to handleAppStateChange(): to match what's actually being tested, or restructure the tests to call startAppStateListener first and then trigger the callback.
♻️ Proposed fix - rename the suite
- describe('startAppStateListener():', () => {
+ describe('handleAppStateChange():', () => {
it('should exit early when app is inactive', async () => {📝 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
| describe('startAppStateListener():', () => { | |
| it('should exit early when app is inactive', async () => { | |
| component.showMobilePushColumn = true; | |
| await (component as any).handleAppStateChange({ isActive: false }); | |
| expect(pushNotificationService.checkPermissions).not.toHaveBeenCalled(); | |
| }); | |
| it('should exit early when push column is hidden', async () => { | |
| component.showMobilePushColumn = false; | |
| await (component as any).handleAppStateChange({ isActive: true }); | |
| expect(pushNotificationService.checkPermissions).not.toHaveBeenCalled(); | |
| }); | |
| it('should clear isPushPermissionDenied and register for push notifications when permission becomes granted', async () => { | |
| component.showMobilePushColumn = true; | |
| component.isPushPermissionDenied = true; | |
| pushNotificationService.checkPermissions.and.resolveTo({ receive: 'granted' } as any); | |
| await (component as any).handleAppStateChange({ isActive: true }); | |
| expect(component.isPushPermissionDenied).toBeFalse(); | |
| expect(pushNotificationService.addRegistrationListener).toHaveBeenCalledTimes(1); | |
| expect(pushNotificationService.register).toHaveBeenCalledTimes(1); | |
| }); | |
| it('should keep isPushPermissionDenied false when permission remains granted', async () => { | |
| component.showMobilePushColumn = true; | |
| component.isPushPermissionDenied = false; | |
| pushNotificationService.checkPermissions.and.resolveTo({ receive: 'granted' } as any); | |
| await (component as any).handleAppStateChange({ isActive: true }); | |
| expect(component.isPushPermissionDenied).toBeFalse(); | |
| expect(pushNotificationService.addRegistrationListener).not.toHaveBeenCalled(); | |
| expect(pushNotificationService.register).not.toHaveBeenCalled(); | |
| }); | |
| it('should set isPushPermissionDenied to true when permission is denied on app resume', async () => { | |
| component.showMobilePushColumn = true; | |
| component.isPushPermissionDenied = false; | |
| pushNotificationService.checkPermissions.and.resolveTo({ receive: 'denied' } as any); | |
| await (component as any).handleAppStateChange({ isActive: true }); | |
| expect(component.isPushPermissionDenied).toBeTrue(); | |
| expect(pushNotificationService.addRegistrationListener).not.toHaveBeenCalled(); | |
| expect(pushNotificationService.register).not.toHaveBeenCalled(); | |
| }); | |
| }); | |
| describe('handleAppStateChange():', () => { | |
| it('should exit early when app is inactive', async () => { | |
| component.showMobilePushColumn = true; | |
| await (component as any).handleAppStateChange({ isActive: false }); | |
| expect(pushNotificationService.checkPermissions).not.toHaveBeenCalled(); | |
| }); | |
| it('should exit early when push column is hidden', async () => { | |
| component.showMobilePushColumn = false; | |
| await (component as any).handleAppStateChange({ isActive: true }); | |
| expect(pushNotificationService.checkPermissions).not.toHaveBeenCalled(); | |
| }); | |
| it('should clear isPushPermissionDenied and register for push notifications when permission becomes granted', async () => { | |
| component.showMobilePushColumn = true; | |
| component.isPushPermissionDenied = true; | |
| pushNotificationService.checkPermissions.and.resolveTo({ receive: 'granted' } as any); | |
| await (component as any).handleAppStateChange({ isActive: true }); | |
| expect(component.isPushPermissionDenied).toBeFalse(); | |
| expect(pushNotificationService.addRegistrationListener).toHaveBeenCalledTimes(1); | |
| expect(pushNotificationService.register).toHaveBeenCalledTimes(1); | |
| }); | |
| it('should keep isPushPermissionDenied false when permission remains granted', async () => { | |
| component.showMobilePushColumn = true; | |
| component.isPushPermissionDenied = false; | |
| pushNotificationService.checkPermissions.and.resolveTo({ receive: 'granted' } as any); | |
| await (component as any).handleAppStateChange({ isActive: true }); | |
| expect(component.isPushPermissionDenied).toBeFalse(); | |
| expect(pushNotificationService.addRegistrationListener).not.toHaveBeenCalled(); | |
| expect(pushNotificationService.register).not.toHaveBeenCalled(); | |
| }); | |
| it('should set isPushPermissionDenied to true when permission is denied on app resume', async () => { | |
| component.showMobilePushColumn = true; | |
| component.isPushPermissionDenied = false; | |
| pushNotificationService.checkPermissions.and.resolveTo({ receive: 'denied' } as any); | |
| await (component as any).handleAppStateChange({ isActive: true }); | |
| expect(component.isPushPermissionDenied).toBeTrue(); | |
| expect(pushNotificationService.addRegistrationListener).not.toHaveBeenCalled(); | |
| expect(pushNotificationService.register).not.toHaveBeenCalled(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In
`@src/app/fyle/notifications/notifications-beta/notifications-beta.page.spec.ts`
around lines 496 - 551, The describe block is misnamed: tests call
handleAppStateChange but the suite is named startAppStateListener(); rename the
describe from "startAppStateListener():" to "handleAppStateChange():" so the
suite name matches the tested method (or alternatively, change tests to call
startAppStateListener and trigger its callback), updating references in this
spec where startAppStateListener appears and keeping the existing test bodies
that call (component as any).handleAppStateChange.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@__mocks__/`@capacitor/push-notifications.ts:
- Line 1: The file begins with a global "/* eslint-disable */" which already
suppresses all linting for the mock; remove the redundant per-line/section
disable further down (the second eslint-disable directive) so there is only the
single top-level "/* eslint-disable */" left; locate the duplicates in
__mocks__/@capacitor/push-notifications.ts (search for "eslint-disable") and
delete the later directive while keeping the initial one intact.
In `@src/app/fyle/notifications/notifications-beta/notifications-beta.page.ts`:
- Around line 141-147: The eslint-disable comment is misplaced on
App.addListener and doesn't cover the any in the handleAppStateChange parameter;
update handleAppStateChange to use a stricter type (e.g., change the parameter
from { [key: string]: any } to Record<string, unknown>) or move the
eslint-disable to immediately precede the parameter declaration; adjust usages
in handleAppStateChange accordingly and remove the unnecessary top-level
eslint-disable near App.addListener.
| @@ -0,0 +1,38 @@ | |||
| /* eslint-disable */ | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Blanket eslint-disable - use with caution, boss!
The /* eslint-disable */ at the top disables all rules for the entire file. This is acceptable for mock files, but it makes the specific disable on line 11 redundant.
🧹 Cleanup suggestion
Since line 1 already disables all rules, remove the redundant directive on line 11:
export interface PushNotificationEvent {
- // eslint-disable-next-line `@typescript-eslint/no-explicit-any`
[key: string]: any;
}🤖 Prompt for AI Agents
In `@__mocks__/`@capacitor/push-notifications.ts at line 1, The file begins with a
global "/* eslint-disable */" which already suppresses all linting for the mock;
remove the redundant per-line/section disable further down (the second
eslint-disable directive) so there is only the single top-level "/*
eslint-disable */" left; locate the duplicates in
__mocks__/@capacitor/push-notifications.ts (search for "eslint-disable") and
delete the later directive while keeping the initial one intact.
src/app/fyle/notifications/notifications-beta/notifications-beta.page.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/app/fyle/notifications/email-notifications/email-notifications.component.ts`:
- Around line 297-299: The App.addListener race causes a leak because its
.then() may assign this.appStateChangeListener after ngOnDestroy runs; add a
destruction guard (e.g., set this._destroyed = true in ngOnDestroy) and in the
App.addListener().then(...) handler check that guard before assigning
this.appStateChangeListener, and if the component is already destroyed and a
listener instance is returned, immediately call remove() on that listener;
ensure ngOnDestroy still calls this.appStateChangeListener?.remove() and set the
guard so late-resolved promises do not leave active listeners (reference:
App.addListener, appStateChangeListener, ngOnDestroy).
Comment on lines
+297
to
+299
| ngOnDestroy(): void { | ||
| this.appStateChangeListener?.remove(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/app/fyle/notifications/email-notifications/email-notifications.component.ts | head -330 | tail -50Repository: fylein/fyle-mobile-app
Length of output: 2153
🏁 Script executed:
rg -n "App\.addListener|\.then.*listener" src/app/fyle/notifications/email-notifications/email-notifications.component.tsRepository: fylein/fyle-mobile-app
Length of output: 159
🏁 Script executed:
cat -n src/app/fyle/notifications/email-notifications/email-notifications.component.ts | grep -E "(addListener|subscribe|\.then)" | head -20Repository: fylein/fyle-mobile-app
Length of output: 1435
🏁 Script executed:
wc -l src/app/fyle/notifications/email-notifications/email-notifications.component.tsRepository: fylein/fyle-mobile-app
Length of output: 149
🏁 Script executed:
cat -n src/app/fyle/notifications/email-notifications/email-notifications.component.ts | sed -n '1,50p'Repository: fylein/fyle-mobile-app
Length of output: 2690
🏁 Script executed:
cat -n src/app/fyle/notifications/email-notifications/email-notifications.component.ts | sed -n '50,120p'Repository: fylein/fyle-mobile-app
Length of output: 2578
Seal the listener leak before it devours memory, padawan.
Race condition awaits: if App.addListener resolves after destruction, the listener leaks. The promise's .then() handler executes after ngOnDestroy, assigning to a dead component. The Capacitor listener fires eternally in the shadows.
🛠️ Proposed fix
+ private isDestroyed = false;
@@
ngOnDestroy(): void {
+ this.isDestroyed = true;
this.appStateChangeListener?.remove();
+ this.appStateChangeListener = null;
}
@@
- App.addListener('appStateChange', ({ isActive }) => {
+ App.addListener('appStateChange', ({ isActive }) => {
@@
- }).then((listener) => {
- this.appStateChangeListener = listener;
+ }).then((listener) => {
+ if (this.isDestroyed) {
+ listener.remove();
+ return;
+ }
+ this.appStateChangeListener = listener;
});🤖 Prompt for AI Agents
In
`@src/app/fyle/notifications/email-notifications/email-notifications.component.ts`
around lines 297 - 299, The App.addListener race causes a leak because its
.then() may assign this.appStateChangeListener after ngOnDestroy runs; add a
destruction guard (e.g., set this._destroyed = true in ngOnDestroy) and in the
App.addListener().then(...) handler check that guard before assigning
this.appStateChangeListener, and if the component is already destroyed and a
listener instance is returned, immediately call remove() on that listener;
ensure ngOnDestroy still calls this.appStateChangeListener?.remove() and set the
guard so late-resolved promises do not leave active listeners (reference:
App.addListener, appStateChangeListener, ngOnDestroy).
…dd-redirection-for-notifications
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app/core/services/platform/v1/spender/org-settings.service.ts`:
- Around line 424-428: The current mapping always creates a truthy
mobile_notification_settings object with undefined fields from incoming, which
breaks downstream checks; change the assignment so mobile_notification_settings
is either undefined when incoming.mobile_notification_settings is falsy or a
fully-defaulted object when present: i.e., use
incoming.mobile_notification_settings ? a safe/defaulted object (populate
allowed, enabled with booleans and unsubscribed_events with an empty array) :
undefined; update the mapping that constructs mobile_notification_settings to
use this conditional and reference incoming and mobile_notification_settings
accordingly.
Comment on lines
+424
to
+428
| mobile_notification_settings: { | ||
| allowed: incoming.mobile_notification_settings && incoming.mobile_notification_settings.allowed, | ||
| enabled: incoming.mobile_notification_settings && incoming.mobile_notification_settings.enabled, | ||
| unsubscribed_events: incoming.mobile_notification_settings && incoming.mobile_notification_settings.unsubscribed_events, | ||
| }, |
There was a problem hiding this comment.
Boss, avoid truthy-but-empty mobile_notification_settings.
When incoming is missing, you still create an object with undefined fields; downstream if (mobile_notification_settings) becomes true and array ops can crash. Guard and default safely.
🔥 Suggested fix
- mobile_notification_settings: {
- allowed: incoming.mobile_notification_settings && incoming.mobile_notification_settings.allowed,
- enabled: incoming.mobile_notification_settings && incoming.mobile_notification_settings.enabled,
- unsubscribed_events: incoming.mobile_notification_settings && incoming.mobile_notification_settings.unsubscribed_events,
- },
+ mobile_notification_settings: incoming.mobile_notification_settings
+ ? {
+ allowed: incoming.mobile_notification_settings.allowed ?? false,
+ enabled: incoming.mobile_notification_settings.enabled ?? false,
+ unsubscribed_events: incoming.mobile_notification_settings.unsubscribed_events ?? [],
+ }
+ : undefined,📝 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
| mobile_notification_settings: { | |
| allowed: incoming.mobile_notification_settings && incoming.mobile_notification_settings.allowed, | |
| enabled: incoming.mobile_notification_settings && incoming.mobile_notification_settings.enabled, | |
| unsubscribed_events: incoming.mobile_notification_settings && incoming.mobile_notification_settings.unsubscribed_events, | |
| }, | |
| mobile_notification_settings: incoming.mobile_notification_settings | |
| ? { | |
| allowed: incoming.mobile_notification_settings.allowed ?? false, | |
| enabled: incoming.mobile_notification_settings.enabled ?? false, | |
| unsubscribed_events: incoming.mobile_notification_settings.unsubscribed_events ?? [], | |
| } | |
| : undefined, |
🤖 Prompt for AI Agents
In `@src/app/core/services/platform/v1/spender/org-settings.service.ts` around
lines 424 - 428, The current mapping always creates a truthy
mobile_notification_settings object with undefined fields from incoming, which
breaks downstream checks; change the assignment so mobile_notification_settings
is either undefined when incoming.mobile_notification_settings is falsy or a
fully-defaulted object when present: i.e., use
incoming.mobile_notification_settings ? a safe/defaulted object (populate
allowed, enabled with booleans and unsubscribed_events with an empty array) :
undefined; update the mapping that constructs mobile_notification_settings to
use this conditional and reference incoming and mobile_notification_settings
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app/core/services/push-notification.service.ts`:
- Around line 93-100: In addNotificationClickListener replace the two
console.log calls inside
PushNotifications.addListener('pushNotificationActionPerformed', ...) with the
approved logging mechanism (e.g., use the service's logger instance or an
injected Logger) or remove them entirely; locate the anonymous callback in
addNotificationClickListener and change console.log('url', url) and
console.log('actionType', actionType) to an appropriate this.logger.debug/info
call or drop the logs to avoid leaking payloads and lint failures.
Comment on lines
93
to
100
| private addNotificationClickListener(): void { | ||
| PushNotifications.addListener('pushNotificationActionPerformed', (event) => { | ||
| const data = (event?.notification?.data as { cta_url?: string; notification_type?: string }) ?? {}; | ||
| const url = data.cta_url; | ||
| const actionType = data.notification_type; | ||
| console.log('url', url); | ||
| console.log('actionType', actionType); | ||
|
|
There was a problem hiding this comment.
Drop the console logs, boss.
console.log can fail lint and may leak notification payloads. Use approved logging or remove.
🔥 Suggested fix
- console.log('url', url);
- console.log('actionType', actionType);📝 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
| private addNotificationClickListener(): void { | |
| PushNotifications.addListener('pushNotificationActionPerformed', (event) => { | |
| const data = (event?.notification?.data as { cta_url?: string; notification_type?: string }) ?? {}; | |
| const url = data.cta_url; | |
| const actionType = data.notification_type; | |
| console.log('url', url); | |
| console.log('actionType', actionType); | |
| private addNotificationClickListener(): void { | |
| PushNotifications.addListener('pushNotificationActionPerformed', (event) => { | |
| const data = (event?.notification?.data as { cta_url?: string; notification_type?: string }) ?? {}; | |
| const url = data.cta_url; | |
| const actionType = data.notification_type; |
🤖 Prompt for AI Agents
In `@src/app/core/services/push-notification.service.ts` around lines 93 - 100, In
addNotificationClickListener replace the two console.log calls inside
PushNotifications.addListener('pushNotificationActionPerformed', ...) with the
approved logging mechanism (e.g., use the service's logger instance or an
injected Logger) or remove them entirely; locate the anonymous callback in
addNotificationClickListener and change console.log('url', url) and
console.log('actionType', actionType) to an appropriate this.logger.debug/info
call or drop the logs to avoid leaking payloads and lint failures.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app/core/services/deep-link.service.ts`:
- Line 16: Remove the stray console.log calls in
src/app/core/services/deep-link.service.ts (the "console.log('url', url)"
occurrences) to satisfy the no-console lint rule; replace them with the approved
application logger (e.g., use the injected Logger/Logging service or
this.logger.debug/info) or remove the lines entirely if logging is unnecessary,
updating any methods in DeepLinkService that contain those console.log
statements.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app/core/services/push-notification.service.ts`:
- Around line 120-121: Remove the two debug console.log statements shown (the
lines calling console.log('event', event) and console.log('data', data')) from
the PushNotificationService implementation; either delete them or replace with
the project logger (e.g., use processLogger.error/info or the service's logger
method) if structured runtime logging is required, ensuring no raw console.log
remains to satisfy lint/CI checks and preserving any needed variable usage so
there are no unused-variable errors.
Comment on lines
+120
to
+121
| console.log('event', event); | ||
| console.log('data', data); |
There was a problem hiding this comment.
Enna koduma sir ithu! Console logs are breaking the pipeline!
Lines 120-121 have console.log statements that are failing lint checks. Remove them immediately - CI is crying, boss!
🔥 Proposed fix - Remove debug logs
const data = (event?.notification?.data as { cta_url?: string; notification_type?: string }) ?? {};
- console.log('event', event);
- console.log('data', data);
const url = data.cta_url;🧰 Tools
🪛 GitHub Actions: Lint
[error] 120-120: Unexpected console statement. Only these console methods are allowed: . no-console
🪛 GitHub Check: Run linters
[failure] 121-121:
Unexpected console statement. Only these console methods are allowed: .
[failure] 120-120:
Unexpected console statement. Only these console methods are allowed: .
🤖 Prompt for AI Agents
In `@src/app/core/services/push-notification.service.ts` around lines 120 - 121,
Remove the two debug console.log statements shown (the lines calling
console.log('event', event) and console.log('data', data')) from the
PushNotificationService implementation; either delete them or replace with the
project logger (e.g., use processLogger.error/info or the service's logger
method) if structured runtime logging is required, ensuring no raw console.log
remains to satisfy lint/CI checks and preserving any needed variable usage so
there are no unused-variable errors.
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes