Conversation
This comment has been minimized.
This comment has been minimized.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 6 minutes Limit details: You’ve used all 2 included reviews currently available. Your 57 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
| freshRouterPushWithRetry.mockResolvedValue(undefined); | ||
|
|
||
| jest.useFakeTimers(); | ||
| await pushNotificationService.initialize(); |
There was a problem hiding this comment.
Unhandled rejection risk in src/services/tests/push-notification.test.ts at lines 631, 638, and 645: await pushNotificationService.initialize() adds an awaited async operation without local error handling. Guard the call with try/catch so initialization failures are handled explicitly instead of surfacing as unstructured unhandled rejections.
Kody rule violation: Handle async operations with proper error handling
try {
await pushNotificationService.initialize();
} catch (err) {
throw err;
}Prompt for LLM
File src/services/__tests__/push-notification.test.ts:
Line 567:
Unhandled rejection risk in src/services/__tests__/push-notification.test.ts at lines 631, 638, and 645: await pushNotificationService.initialize() adds an awaited async operation without local error handling. Guard the call with try/catch so initialization failures are handled explicitly instead of surfacing as unstructured unhandled rejections.
Suggested Code:
try {
await pushNotificationService.initialize();
} catch (err) {
throw err;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (this.tryDeepLinkForData(data)) { | ||
| return; | ||
| } | ||
| this.showModalForData(data, content.title, content.body); |
There was a problem hiding this comment.
Deep-link success is reported in src/services/push-notification.ts before the async navigation completes, so failed routerPushWithRetry retries or a waitUntil condition that never becomes true suppress the modal fallback and drop tapped call/chat notifications after only logging an error. Make tryDeepLinkForData async and suppress showModalForData only after routerPushWithRetry resolves, or call showModalForData from the catch path when the deep link fails.
void this.tryDeepLinkForData(data)
.then((didNavigate) => {
if (!didNavigate) {
this.showModalForData(data, content.title, content.body);
}
})
.catch(() => {
this.showModalForData(data, content.title, content.body);
});Prompt for LLM
File src/services/push-notification.ts:
Line 363 to 366:
Deep-link success is reported in src/services/push-notification.ts before the async navigation completes, so failed routerPushWithRetry retries or a waitUntil condition that never becomes true suppress the modal fallback and drop tapped call/chat notifications after only logging an error. Make tryDeepLinkForData async and suppress showModalForData only after routerPushWithRetry resolves, or call showModalForData from the catch path when the deep link fails.
Suggested Code:
void this.tryDeepLinkForData(data)
.then((didNavigate) => {
if (!didNavigate) {
this.showModalForData(data, content.title, content.body);
}
})
.catch(() => {
this.showModalForData(data, content.title, content.body);
});
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (parsed.type === 'call' && parsed.id !== '' && !/[/\\?#]/.test(parsed.id)) { | ||
| void routerPushWithRetry( | ||
| { pathname: '/call/[id]', params: { id: parsed.id } }, | ||
| { | ||
| maxAttempts: 40, | ||
| retryDelayMs: 250, | ||
| // On a cold start the session is still hydrating — see handleChatDeepLink. | ||
| waitUntil: () => useAuthStore.getState().status === 'signedIn', | ||
| } | ||
| ).catch((error) => { | ||
| logger.error({ message: 'Failed to deep-link to call from push notification', context: { error, eventCode } }); | ||
| }); | ||
| return true; |
There was a problem hiding this comment.
False success in tryDeepLinkForData causes tap handlers in src/services/push-notification.ts to skip the modal fallback before routerPushWithRetry actually navigates. Return true only after routerPushWithRetry resolves, and return false after logging the error when the retry budget expires or useAuthStore.getState().status === 'signedIn' never becomes true so the caller can fall back to showNotificationModal.
if (parsed.type === 'call' && parsed.id !== '' && !/[/\\?#]/.test(parsed.id)) {
try {
await routerPushWithRetry(
{ pathname: '/call/[id]', params: { id: parsed.id } },
{
maxAttempts: 40,
retryDelayMs: 250,
waitUntil: () => useAuthStore.getState().status === 'signedIn',
}
);
return true;
} catch (error) {
logger.error({ message: 'Failed to deep-link to call from push notification', context: { error, eventCode } });
return false;
}
}Prompt for LLM
File src/services/push-notification.ts:
Line 291 to 303:
False success in tryDeepLinkForData causes tap handlers in src/services/push-notification.ts to skip the modal fallback before routerPushWithRetry actually navigates. Return true only after routerPushWithRetry resolves, and return false after logging the error when the retry budget expires or useAuthStore.getState().status === 'signedIn' never becomes true so the caller can fall back to showNotificationModal.
Suggested Code:
if (parsed.type === 'call' && parsed.id !== '' && !/[/\\?#]/.test(parsed.id)) {
try {
await routerPushWithRetry(
{ pathname: '/call/[id]', params: { id: parsed.id } },
{
maxAttempts: 40,
retryDelayMs: 250,
waitUntil: () => useAuthStore.getState().status === 'signedIn',
}
);
return true;
} catch (error) {
logger.error({ message: 'Failed to deep-link to call from push notification', context: { error, eventCode } });
return false;
}
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }); | ||
|
|
||
| // Delay so the React tree is mounted and the modal store is ready. | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Timer leak risk in src/services/push-notification.ts: the added setTimeout starts asynchronous work without a deterministic cleanup path. Store the timeout handle and clear it during teardown or disposal, or ensure the surrounding lifecycle guarantees the timer cannot outlive the service.
Kody rule violation: Clear timers on teardown/unmount
const timeoutId = setTimeout(() => {
// Deep-link chat and call notifications straight to their screen; anything
// else falls back to the persistent modal.
if (this.tryDeepLinkForData(data)) {
return;
}
this.showModalForData(data, content.title, content.body);
}, 500);
// clearTimeout(timeoutId) in the deterministic teardown/unmount path if this listener/service can be disposed before it fires.Prompt for LLM
File src/services/push-notification.ts:
Line 360:
Timer leak risk in src/services/push-notification.ts: the added setTimeout starts asynchronous work without a deterministic cleanup path. Store the timeout handle and clear it during teardown or disposal, or ensure the surrounding lifecycle guarantees the timer cannot outlive the service.
Suggested Code:
const timeoutId = setTimeout(() => {
// Deep-link chat and call notifications straight to their screen; anything
// else falls back to the persistent modal.
if (this.tryDeepLinkForData(data)) {
return;
}
this.showModalForData(data, content.title, content.body);
}, 500);
// clearTimeout(timeoutId) in the deterministic teardown/unmount path if this listener/service can be disposed before it fires.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
| // microtask after the timer — advanceTimersByTimeAsync flushes both. | ||
| it('falls back to the modal for a message tap', async () => { | ||
| responseHandler(makeResponse('tap-msg', { eventCode: 'M:5' })); | ||
| await jest.advanceTimersByTimeAsync(400); |
There was a problem hiding this comment.
This awaited async operation is not guarded with error handling. Wrap the await in try/catch so timer-flush failures are handled explicitly and do not become unhandled rejections.
Also found in:
src/services/__tests__/push-notification.test.ts:619-619src/services/__tests__/push-notification.test.ts:609-609src/services/__tests__/push-notification.test.ts:629-629
Kody rule violation: Handle async operations with proper error handling
try {
await jest.advanceTimersByTimeAsync(400);
} catch (err) {
throw err;
}Prompt for LLM
File src/services/__tests__/push-notification.test.ts:
Line 601:
This awaited async operation is not guarded with error handling. Wrap the await in try/catch so timer-flush failures are handled explicitly and do not become unhandled rejections.
Suggested Code:
try {
await jest.advanceTimersByTimeAsync(400);
} catch (err) {
throw err;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await routerPushWithRetry(href, DEEP_LINK_RETRY_OPTIONS); | ||
| return true; | ||
| } catch (error) { | ||
| logger.error({ message: failureMessage, context: { error, eventCode } }); |
There was a problem hiding this comment.
Include explicit structured context fields for the operation and identifiers at the top level of the log payload. This makes the error log compliant with structured logging requirements and easier to query by operation/eventCode.
Kody rule violation: Include error context in structured logs
logger.error({ message: failureMessage, op: 'deepLinkWithRetry', eventCode, error });Prompt for LLM
File src/services/push-notification.ts:
Line 107:
Include explicit structured context fields for the operation and identifiers at the top level of the log payload. This makes the error log compliant with structured logging requirements and easier to query by operation/eventCode.
Suggested Code:
logger.error({ message: failureMessage, op: 'deepLinkWithRetry', eventCode, error });
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }); | ||
|
|
||
| // Delay so the React tree is mounted and the modal store is ready. | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Store the timeout handle and provide a deterministic cleanup path that clears it. Starting a timer without retaining and clearing it can leave pending work running after teardown.
Kody rule violation: Clear timers on teardown/unmount
const timeoutId = setTimeout(() => {
// ...
}, delayMs);
// ensure timeoutId is cleared during teardown/unmount or service cleanupPrompt for LLM
File src/services/push-notification.ts:
Line 378:
Store the timeout handle and provide a deterministic cleanup path that clears it. Starting a timer without retaining and clearing it can leave pending work running after teardown.
Suggested Code:
const timeoutId = setTimeout(() => {
// ...
}, delayMs);
// ensure timeoutId is cleared during teardown/unmount or service cleanup
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 participants
Summary
Fixes push notification deep linking so tapped notifications route users to the correct chat or call screen more reliably, including on cold app starts and on iOS payload formats where
eventCodeis not exposed in the standard notification data.What changed
eventCodefrom multiple push payload locations, not justcontent.data.t:/g:prefixes, now handled case-insensitively), orC:and equivalent parsed call event codes)C:1234,t:channel-id,g:group-idC1234Functional impact
eventCodefrom raw push payload fields when needed.Testing
eventCodefrom Android and iOS payload shapes