feat: handle onesignal push notification initialization #1247
feat: handle onesignal push notification initialization #1247kushagrasarathe merged 39 commits intopeanut-wallet-devfrom
Conversation
kushagrasarathe
commented
Sep 23, 2025
kushagrasarathe
commented
- contributes to TASK-14356 : integrated onesignal web sdk for notifications initialization and subscription handling
- also handles notification banners/modal for subscription
WalkthroughAdds OneSignal push-notification support: env vars and dependency, a OneSignal service worker, a notifications hook and UI (permission modal, banner, bell nav, notifications page), notifications API client/types, bell icon, Home wiring, banner click fix, and a Tailwind color token. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/(mobile-ui)/home/page.tsx (2)
168-188: Remove duplicate balance warning modal effectLines 168-188 are an exact duplicate of the effect on lines 144-165. This appears to be a merge conflict resolution error that left duplicate code.
Apply this diff to remove the duplicate effect:
-// effect for showing balance warning modal -useEffect(() => { - if (isFetchingBalance || balance === undefined) return - - if (typeof window !== 'undefined') { - const hasSeenBalanceWarning = getFromLocalStorage(`${user!.user.userId}-hasSeenBalanceWarning`) - const balanceInUsd = Number(formatUnits(balance, PEANUT_WALLET_TOKEN_DECIMALS)) - - // show if: - // 1. balance is above the threshold - // 2. user hasn't seen this warning in the current session - // 3. no other modals are currently active - if ( - balanceInUsd > BALANCE_WARNING_THRESHOLD && - !hasSeenBalanceWarning && - !showIOSPWAInstallModal && - !showAddMoneyPromptModal - ) { - setShowBalanceWarningModal(true) - } - } -}, [balance, isFetchingBalance, showIOSPWAInstallModal, showAddMoneyPromptModal])
144-165: Add notification modal state to balance warning dependenciesThe first balance warning modal effect should include
showPermissionModalandisPostSignupActionModalVisiblein its dependencies to ensure proper modal priority and prevent multiple modals from appearing simultaneously.Apply this diff to fix the dependencies and conditions:
// effect for showing balance warning modal useEffect(() => { if (isFetchingBalance || balance === undefined) return if (typeof window !== 'undefined') { const hasSeenBalanceWarning = getFromLocalStorage(`${user!.user.userId}-hasSeenBalanceWarning`) const balanceInUsd = Number(formatUnits(balance, PEANUT_WALLET_TOKEN_DECIMALS)) // show if: // 1. balance is above the threshold // 2. user hasn't seen this warning in the current session // 3. no other modals are currently active if ( balanceInUsd > BALANCE_WARNING_THRESHOLD && !hasSeenBalanceWarning && + !showPermissionModal && !showIOSPWAInstallModal && !showAddMoneyPromptModal && !isPostSignupActionModalVisible ) { setShowBalanceWarningModal(true) } } -}, [balance, isFetchingBalance, showIOSPWAInstallModal, showAddMoneyPromptModal]) +}, [balance, isFetchingBalance, showPermissionModal, showIOSPWAInstallModal, showAddMoneyPromptModal, isPostSignupActionModalVisible])
🧹 Nitpick comments (4)
src/hooks/useNotifications.ts (1)
74-118: Optimize timer management for banner schedulingThe
evaluateVisibilityfunction recursively calls itself viasetTimeout, which could lead to multiple timers if called rapidly. Consider canceling existing timers before scheduling new ones.Add timer cleanup before scheduling:
// handle scheduled banner display const now = Date.now() const bannerShowAt = typeof bannerShowAtVal === 'number' ? bannerShowAtVal : parseInt(bannerShowAtVal || '0', 10) if (bannerShowAt > 0) { if (now >= bannerShowAt) { setShowReminderBanner(true) } else { setShowReminderBanner(false) // schedule banner to show at the right time if (bannerTimerRef.current) clearTimeout(bannerTimerRef.current) const delay = Math.max(0, bannerShowAt - now) + // Prevent scheduling if delay is too large (>30 days) + if (delay > 30 * 24 * 60 * 60 * 1000) { + console.warn('Banner scheduled too far in future, skipping timer') + return + } bannerTimerRef.current = setTimeout(() => { evaluateVisibility() }, delay) } } else { setShowReminderBanner(false) }src/components/Notifications/SetupNotifcationsModal.tsx (1)
22-22: Minor: Consider making the modal dismissibleSetting
preventClose={true}forces users to make a decision, which could be frustrating. Consider allowing dismissal via backdrop click while still hiding the close button.- preventClose={true} + preventClose={false}src/components/Notifications/NotificationBanner.tsx (1)
71-76: Consider improving the permission-denied messageThe current message about "re-installing the PWA" might be confusing for non-technical users. Consider providing clearer instructions or a link to documentation.
- <span>Please enable notifications to receive updates by re-installing the PWA.</span> + <span>Notifications are currently blocked by your browser settings.</span> <br /> <span> - Unfortunately browser limitations prevent us from re-enabling notifications until you - manually re-install the PWA. + To enable notifications, you'll need to update your browser's permission settings + for this site or reinstall the app. </span>src/app/(mobile-ui)/home/page.tsx (1)
267-270: Consider error handling for permission requestThe async permission request could fail, but there's no error handling in the onClick handler. Consider wrapping it in a try-catch to handle potential errors gracefully.
Apply this diff to add error handling:
onClick={async () => { - await requestPermission() - await afterPermissionAttempt() + try { + await requestPermission() + await afterPermissionAttempt() + } catch (error) { + console.error('Failed to request notification permission:', error) + // Optionally show user-friendly error message + } }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
.env.example(1 hunks)package.json(1 hunks)public/OneSignalSDKUpdaterWorker.js(1 hunks)public/OneSignalSDKWorker.js(1 hunks)public/onesignal/OneSignalSDKUpdaterWorker.js(1 hunks)public/onesignal/OneSignalSDKWorker.js(1 hunks)src/app/(mobile-ui)/history/page.tsx(0 hunks)src/app/(mobile-ui)/home/page.tsx(4 hunks)src/components/Global/Icons/Icon.tsx(3 hunks)src/components/Global/Icons/bell.tsx(1 hunks)src/components/Notifications/NotificationBanner.tsx(1 hunks)src/components/Notifications/SetupNotifcationsModal.tsx(1 hunks)src/components/Setup/Setup.types.ts(1 hunks)src/components/Setup/components/SetupWrapper.tsx(3 hunks)src/hooks/useNotifications.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/app/(mobile-ui)/history/page.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
src/components/Notifications/NotificationBanner.tsx (1)
src/components/Global/Icons/Icon.tsx (1)
Icon(199-208)
src/hooks/useNotifications.ts (2)
src/redux/hooks.ts (1)
useUserStore(13-13)src/utils/general.utils.ts (2)
getFromLocalStorage(117-139)saveToLocalStorage(99-115)
src/components/Notifications/SetupNotifcationsModal.tsx (3)
src/hooks/useNotifications.ts (1)
useNotifications(8-373)src/components/Setup/components/SetupWrapper.tsx (1)
SetupImageSection(92-166)src/components/0_Bruddle/Button.tsx (1)
Button(76-267)
src/app/(mobile-ui)/home/page.tsx (2)
src/hooks/useNotifications.ts (1)
useNotifications(8-373)src/components/Notifications/SetupNotifcationsModal.tsx (1)
SetupNotifcationsModal(8-65)
src/components/Global/Icons/Icon.tsx (1)
src/components/Global/Icons/bell.tsx (1)
BellIcon(3-10)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 61-61: [UnorderedKey] The NEXT_PUBLIC_ONEIGNAL_WEBHOOK key should go before the NEXT_PUBLIC_ONESIGNAL_APP_ID key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (12)
src/components/Setup/Setup.types.ts (1)
14-14: LGTM! Type definition correctly extended.The
LayoutTypeunion type properly includes the new'notification-permission'variant, maintaining consistency with the existing pattern and aligning with theScreenIdtype.src/components/Global/Icons/bell.tsx (1)
3-10: Clean SVG icon implementation.The BellIcon component follows good React patterns:
- Proper TypeScript typing with
FC<SVGProps<SVGSVGElement>>- Uses
currentColorfor theme compatibility- Spreads props for flexibility
- Clean, accessible SVG markup
public/onesignal/OneSignalSDKUpdaterWorker.js (1)
1-1: Same CDN security concerns and potential duplication.This file has identical security risks as
public/OneSignalSDKWorker.js(external CDN loading) and appears to be a duplicate worker with the same functionality.public/onesignal/OneSignalSDKWorker.js (1)
1-1: Third duplicate of the same OneSignal worker.This is yet another file with identical content to the other OneSignal workers, raising the same security concerns and creating maintenance overhead.
public/OneSignalSDKUpdaterWorker.js (1)
1-1: Fourth duplicate worker file - consolidation needed.This creates the fourth identical OneSignal worker file across the codebase. Consider consolidating to a single worker file to reduce maintenance burden and potential inconsistencies.
Based on the PR summary mentioning multiple worker variants, verify if OneSignal actually requires multiple worker files or if this is over-engineering:
#!/bin/bash # Description: Check OneSignal documentation requirements for multiple worker files # Expected: Understand if multiple worker files are actually needed # Find all OneSignal worker files to assess duplication fd -t f "OneSignal.*Worker\.js" . --exec echo "File: {}" \; --exec cat {} \; --exec echo "---" \; # Search for any references to these specific worker file paths in the codebase rg -i "onesignal.*worker" --type=js --type=ts -C2package.json (1)
72-72: react-onesignal v3.2.3 — verified; no known runtime vulnerabilitiesnpm shows 3.2.3 as the latest; no direct vulnerabilities reported (CVE-2023-28430 affected OneSignal GitHub Actions workflows, not runtime). ^3.2.3 allows future minor/patch updates (semver); pin to 3.2.3 or use ~3.2.3 if you want stricter update control.
src/components/Setup/components/SetupWrapper.tsx (2)
42-42: Good alignment with existing layout patternsThe new
notification-permissionlayout type follows the established pattern and maintains consistency with other layout types.
92-100: LGTM - Well-structured component exportThe
SetupImageSectioncomponent is properly exported with clear prop types and the newcustomContainerClassprop provides good flexibility for layout customization.src/components/Global/Icons/Icon.tsx (1)
61-61: LGTM - Clean icon integrationThe bell icon is properly integrated into the icon system following the established pattern.
Also applies to: 72-72, 141-141
src/components/Notifications/NotificationBanner.tsx (1)
24-28: Good practice: Proper event propagation handlingThe
stopPropagationcall correctly prevents the click event from bubbling up to the parent Card component.src/app/(mobile-ui)/home/page.tsx (2)
263-275: Good implementation of notification UI components!The integration of the notification modal and banner is well-structured:
- Modal is conditionally rendered based on
showPermissionModal- Banner correctly handles both permission requests and closure actions
- The async flow with
afterPermissionAttemptensures proper state updates
49-56: Typo in imported component name:SetupNotifcationsModalshould beSetupNotificationsModalThe import statement has a typo in the component name. This inconsistency could lead to confusion and should be corrected to maintain naming consistency throughout the codebase.
Apply this diff to fix the typo:
-import SetupNotifcationsModal from '@/components/Notifications/SetupNotifcationsModal' +import SetupNotificationsModal from '@/components/Notifications/SetupNotificationsModal'Also update the usage on line 263:
-{showPermissionModal && <SetupNotifcationsModal />} +{showPermissionModal && <SetupNotificationsModal />}⛔ Skipped due to learnings
Learnt from: Zishan-7 PR: peanutprotocol/peanut-ui#1072 File: src/app/(setup)/setup/page.tsx:173-175 Timestamp: 2025-08-07T12:53:50.946Z Learning: In the peanut-ui setup flow at `src/app/(setup)/setup/page.tsx`, when handling unsupported scenarios, both device not supported and browser not supported cases should show the same "Unsupported browser" message using the `UnsupportedBrowserModal` component, rather than having distinct messaging for each scenario.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.env.example (1)
58-61: Fix dotenv-linter ordering warning.Reorder keys to satisfy dotenv-linter.
-# one signal -export NEXT_PUBLIC_ONESIGNAL_APP_ID= -export NEXT_PUBLIC_SAFARI_WEB_ID= -export NEXT_PUBLIC_ONESIGNAL_WEBHOOK= +# one signal +export NEXT_PUBLIC_ONESIGNAL_APP_ID= +export NEXT_PUBLIC_ONESIGNAL_WEBHOOK= +export NEXT_PUBLIC_SAFARI_WEB_ID=
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.env.example(1 hunks)src/components/Notifications/NotificationBanner.tsx(1 hunks)src/hooks/useNotifications.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Notifications/NotificationBanner.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-07T20:22:11.092Z
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#958
File: src/app/actions/tokens.ts:266-266
Timestamp: 2025-07-07T20:22:11.092Z
Learning: In `src/app/actions/tokens.ts`, within the `fetchWalletBalances` function, using the non-null assertion operator `!` on `process.env.MOBULA_API_KEY!` is intentional and correct, and should not be flagged for replacement with explicit validation.
Applied to files:
src/hooks/useNotifications.ts
📚 Learning: 2025-06-22T16:10:53.167Z
Learnt from: kushagrasarathe
PR: peanutprotocol/peanut-ui#915
File: src/hooks/useKycFlow.ts:96-124
Timestamp: 2025-06-22T16:10:53.167Z
Learning: The `initiateKyc` function in `src/app/actions/users.ts` already includes comprehensive error handling with try-catch blocks and returns structured responses with either `{ data }` or `{ error }` fields, so additional try-catch blocks around its usage are not needed.
Applied to files:
src/hooks/useNotifications.ts
📚 Learning: 2025-09-11T17:46:12.507Z
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#1200
File: src/app/(mobile-ui)/recover-funds/page.tsx:9-9
Timestamp: 2025-09-11T17:46:12.507Z
Learning: Functions in Next.js that are not marked with "use server" and contain secrets are unsafe to import in client components, as they get bundled into the client JavaScript and can leak environment variables to the browser.
Applied to files:
.env.example
🧬 Code graph analysis (1)
src/hooks/useNotifications.ts (2)
src/redux/hooks.ts (1)
useUserStore(13-13)src/utils/general.utils.ts (2)
getFromLocalStorage(117-139)saveToLocalStorage(99-115)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 61-61: [UnorderedKey] The NEXT_PUBLIC_ONESIGNAL_WEBHOOK key should go before the NEXT_PUBLIC_SAFARI_WEB_ID key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (6)
.env.example (1)
61-61: Verify webhook value is safe to expose publicly.This is a NEXT_PUBLIC_ value and will be bundled to the client. Ensure it contains no secrets/tokens. If the endpoint needs credentials, move that handling server‑side and expose a safe public endpoint.
Would you confirm that NEXT_PUBLIC_ONESIGNAL_WEBHOOK points to a tokenless, public ingestion URL? If not, I can propose a server‑side proxy pattern.
src/hooks/useNotifications.ts (5)
236-257: Avoid empty catch blocks; log at least debug-level context.Silent failures hinder debugging. Log and continue.
try { await OneSignal.User.PushSubscription.optIn() - } catch (_) {} + } catch (error) { + console.debug('OneSignal optIn failed (may be normal if already opted in):', error) + } @@ } catch (error) { console.warn('Error with slidedown prompt, trying direct permission request:', error) try { await OneSignal.Notifications.requestPermission() - } catch (_) {} + } catch (err2) { + console.debug('Direct permission request failed:', err2) + } }const logoutUser = async () => { try { await OneSignal.logout() - } catch (_) {} + } catch (error) { + console.debug('OneSignal.logout failed:', error) + } }Also applies to: 312-314
126-184: Solid singleton bootstrap to prevent duplicate SDK inits.The in-flight promise and idempotent flags are correct and avoid race conditions and double listeners.
186-221: Event listener setup looks correct.Listeners are added once and tied to state refresh; good use of a gate to prevent duplicates.
55-56: OneSignal.Notifications.permission is boolean — current check is correct.It returns true/false (true when granted), so
resolve(perm === true)is valid; you can optionally simplify toresolve(perm).
142-173: Remove non-null assertion and make webhooks optional.If NEXT_PUBLIC_ONESIGNAL_WEBHOOK is unset, the current code passes undefined strings into OneSignal.init and may cause init failures.
- const webhookUrl = process.env.NEXT_PUBLIC_ONESIGNAL_WEBHOOK! + const webhookUrl = process.env.NEXT_PUBLIC_ONESIGNAL_WEBHOOK @@ - await OneSignal.init({ + const webhooks = + webhookUrl + ? { + cors: true, + 'notification.willDisplay': webhookUrl, + 'notification.clicked': webhookUrl, + } + : undefined + + await OneSignal.init({ appId: appId, safari_web_id: safariWebId, @@ - webhooks: { - cors: true, - 'notification.willDisplay': webhookUrl, - 'notification.clicked': webhookUrl, - }, + webhooks,
Hugo0
left a comment
Hugo0
left a comment
There was a problem hiding this comment.
Havent finished review (no time for useNotifs hook)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/(mobile-ui)/home/page.tsx (1)
138-182: Remove duplicate balance warning modal effect.Lines 138-159 and lines 162-182 contain identical logic for showing the balance warning modal. This duplication will cause the effect to run twice and may lead to inconsistent state or double-renders.
Apply this diff to remove the duplicate:
}, [balance, isFetchingBalance, showIOSPWAInstallModal, showAddMoneyPromptModal, user]) - // effect for showing balance warning modal - useEffect(() => { - if (isFetchingBalance || balance === undefined || !user) return - - if (typeof window !== 'undefined') { - const hasSeenBalanceWarning = getFromLocalStorage(`${user!.user.userId}-hasSeenBalanceWarning`) - const balanceInUsd = Number(formatUnits(balance, PEANUT_WALLET_TOKEN_DECIMALS)) - - // show if: - // 1. balance is above the threshold - // 2. user hasn't seen this warning in the current session - // 3. no other modals are currently active - if ( - balanceInUsd > BALANCE_WARNING_THRESHOLD && - !hasSeenBalanceWarning && - !showIOSPWAInstallModal && - !showAddMoneyPromptModal - ) { - setShowBalanceWarningModal(true) - } - } - }, [balance, isFetchingBalance, showIOSPWAInstallModal, showAddMoneyPromptModal, user]) - // effect for showing add money prompt modal useEffect(() => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/(mobile-ui)/home/page.tsx(5 hunks)src/components/Home/HomeBanners/BannerCard.tsx(1 hunks)src/components/Home/HomeBanners/index.tsx(1 hunks)src/components/Notifications/NotificationBanner.tsx(1 hunks)src/hooks/useBanners.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/components/Home/HomeBanners/index.tsx (2)
src/hooks/useBanners.tsx (1)
useBanners(19-69)src/components/Global/Icons/Icon.tsx (1)
IconName(65-127)
src/hooks/useBanners.tsx (2)
src/components/Global/Icons/Icon.tsx (1)
IconName(65-127)src/hooks/useNotifications.ts (1)
useNotifications(11-390)
src/components/Home/HomeBanners/BannerCard.tsx (1)
src/components/Global/Icons/Icon.tsx (1)
Icon(199-208)
src/app/(mobile-ui)/home/page.tsx (3)
src/hooks/useNotifications.ts (1)
useNotifications(11-390)src/components/Notifications/NotificationNavigation.tsx (1)
NotificationNavigation(9-47)src/components/Notifications/SetupNotificationsModal.tsx (1)
SetupNotificationsModal(5-50)
src/components/Notifications/NotificationBanner.tsx (1)
src/components/Global/Icons/Icon.tsx (1)
Icon(199-208)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (8)
src/components/Home/HomeBanners/index.tsx (2)
12-15: LGTM!The guard clause prevents rendering an empty carousel when there are no banners, improving performance and avoiding unnecessary DOM elements.
33-44: LGTM!The BannerCard rendering logic correctly preserves the existing close behavior by filtering out the banner from the state when closed.
src/app/(mobile-ui)/home/page.tsx (6)
41-43: LGTM!The new imports for notification components and hook are correctly added.
50-50: LGTM!Extracting
showPermissionModalfromuseNotificationscorrectly integrates the notification permission flow.
230-233: LGTM!The NotificationNavigation is correctly placed alongside SearchUsers in the header, wrapped in a flex container for proper alignment.
261-261: LGTM!The conditional rendering of
SetupNotificationsModalbased onshowPermissionModalcorrectly integrates the notification permission flow. The modal is placed afterHomeBannersand beforeHomeHistory, ensuring proper visual hierarchy.
192-196: LGTM!The updated comments and condition correctly prioritize the setup notifications modal over the add money prompt, ensuring users see the notification permission flow first.
Also applies to: 199-199
208-215: LGTM!Adding
showPermissionModalandisPostSignupActionModalVisibleto the effect dependencies correctly ensures the add money prompt logic re-evaluates when notification or post-signup modal state changes.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)src/app/(mobile-ui)/home/page.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/(mobile-ui)/home/page.tsx (3)
src/hooks/useNotifications.ts (1)
useNotifications(11-390)src/components/Notifications/NotificationNavigation.tsx (1)
NotificationNavigation(9-47)src/components/Notifications/SetupNotificationsModal.tsx (1)
SetupNotificationsModal(5-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/hooks/useNotifications.ts (4)
167-172: Require Safari Web ID only on SafariBlocking init on all browsers if safari_web_id is missing is unnecessary. Gate it to Safari.
- if (!appId || !safariWebId) { - console.error( - 'OneSignal configuration missing: NEXT_PUBLIC_ONESIGNAL_APP_ID and NEXT_PUBLIC_SAFARI_WEB_ID are required' - ) - return - } + const isSafari = + typeof navigator !== 'undefined' && + /^((?!chrome|android).)*safari/i.test(navigator.userAgent) + if (!appId || (isSafari && !safariWebId)) { + console.error( + 'OneSignal config missing: NEXT_PUBLIC_ONESIGNAL_APP_ID is required; NEXT_PUBLIC_SAFARI_WEB_ID is required on Safari' + ) + return + }As noted by a prior reviewer.
221-239: DRY up identity-verification error handlingSame error parsing/flagging is duplicated; extract a helper and reuse.
- } catch (err: any) { - const msg = (err && (err.message || err.toString())) || '' - // disable login on identity verification errors - if ( - msg.toLowerCase().includes('identity') || - msg.toLowerCase().includes('verify') - ) { - disableExternalIdLoginRef.current = true - console.warn( - 'OneSignal external_id login disabled due to identity verification error' - ) - } - } + } catch (err: any) { + handleIdentityError(err) + }- } catch (err: any) { - const msg = (err && (err.message || err.toString())) || '' - // disable on identity errors - if (msg.toLowerCase().includes('identity') || msg.toLowerCase().includes('verify')) { - disableExternalIdLoginRef.current = true - console.warn('OneSignal external_id login disabled due to identity verification error') - } - } + } catch (err: any) { + handleIdentityError(err) + }Add this helper inside the hook:
const handleIdentityError = useCallback((err: any) => { const msg = (err && (err.message || err.toString())) || '' if (msg.toLowerCase().includes('identity') || msg.toLowerCase().includes('verify')) { disableExternalIdLoginRef.current = true console.warn('OneSignal external_id login disabled due to identity verification error') return true } return false }, [])Also applies to: 306-318
165-166: Remove non-null assertion and make webhooks optionalwebhookUrl uses a non-null assertion and webhooks are always set. If undefined, OneSignal.init can misbehave.
- const webhookUrl = process.env.NEXT_PUBLIC_ONESIGNAL_WEBHOOK! + const webhookUrl = process.env.NEXT_PUBLIC_ONESIGNAL_WEBHOOK ... - webhooks: { - cors: true, - // note: webhook endpoint is protected server-side via a shared token in additionaldata.authtoken - // this prevents random generated webhook calls, onesignal dashboard sends pass through additionaldata - 'notification.willDisplay': webhookUrl, - 'notification.clicked': webhookUrl, - }, + webhooks: webhookUrl + ? { + cors: true, + // server validates shared token from additionalData + 'notification.willDisplay': webhookUrl, + 'notification.clicked': webhookUrl, + } + : undefined,Also applies to: 189-195
329-331: Avoid empty catch; log at debug levelSilent failures hurt diagnosability.
- } catch (_) {} + } catch (error) { + console.debug('OneSignal logout failed (likely benign):', error) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Notifications/SetupNotificationsModal.tsx(1 hunks)src/hooks/useNotifications.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Notifications/SetupNotificationsModal.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T07:58:17.593Z
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#1247
File: src/hooks/useNotifications.ts:324-329
Timestamp: 2025-09-24T07:58:17.593Z
Learning: When implementing environment-based behavior in production code, always default to production behavior and only use development/testing behavior when explicitly in a development environment. Never rely on positive checks for production (like `=== 'production'`) as missing environment variables could accidentally enable development behavior in production.
Applied to files:
src/hooks/useNotifications.ts
🧬 Code graph analysis (1)
src/hooks/useNotifications.ts (2)
src/redux/hooks.ts (1)
useUserStore(13-13)src/utils/general.utils.ts (2)
getFromLocalStorage(112-134)saveToLocalStorage(94-110)
🔇 Additional comments (2)
src/hooks/useNotifications.ts (2)
100-106: Prod-default timing: LGTMUsing NODE_ENV and defaulting to 7 days except in explicit dev/test matches the “default-to-prod” guidance. Nice.
Based on learnings
Also applies to: 341-344, 366-381
54-57: OneSignal.Notifications.permission returns boolean—no change required
SDK docs confirm it returns a boolean (trueif granted,falseotherwise), so the existingperm === truecheck is valid.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/components/Notifications/NotificationNavigation.tsx (3)
31-37: Remove redundant window checks in useEffect.The
typeof window !== 'undefined'checks are unnecessary insideuseEffect. In Next.js,useEffectonly runs on the client side, sowindowis always defined.Apply this diff to simplify:
const onUpdated = () => { void fetchNotificationCount() } - if (typeof window !== 'undefined') { - window.addEventListener('notifications:updated', onUpdated) - } + window.addEventListener('notifications:updated', onUpdated) return () => { - if (typeof window !== 'undefined') { - window.removeEventListener('notifications:updated', onUpdated) - } + window.removeEventListener('notifications:updated', onUpdated) }
29-29: Avoid usingvoidoperator for async functions.Using
voidto suppress the promise warning is a code smell. Either handle the promise properly or make the handler non-async.Apply this diff:
- const onUpdated = () => { - void fetchNotificationCount() - } + const onUpdated = () => { + fetchNotificationCount().catch(console.error) + }This makes error handling explicit rather than silently discarding the promise.
42-42: Remove unnecessarytwMergefor static class.Using
twMergefor a single static class name is unnecessary overhead.Apply this diff:
- <Link href={'/notifications'} className={twMerge('relative')}> + <Link href={'/notifications'} className="relative">
twMergeis useful when merging multiple class strings or conditional classes, but not for a single static value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/(mobile-ui)/home/page.tsx(5 hunks)src/app/(mobile-ui)/notifications/page.tsx(1 hunks)src/components/Global/Icons/Icon.tsx(3 hunks)src/components/Notifications/NotificationNavigation.tsx(1 hunks)src/components/SearchUsers/index.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- src/components/SearchUsers/index.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/(mobile-ui)/notifications/page.tsx
- src/app/(mobile-ui)/home/page.tsx
- src/components/Global/Icons/Icon.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Notifications/NotificationNavigation.tsx (2)
src/services/notifications.ts (1)
notificationsApi(20-69)src/components/Global/Icons/Icon.tsx (1)
Icon(204-213)
🪛 GitHub Actions: Tests
src/components/Notifications/NotificationNavigation.tsx
[warning] 1-1: Code style issues found by Prettier. Run 'pnpm prettier --write' to fix.
🔇 Additional comments (1)
src/components/Notifications/NotificationNavigation.tsx (1)
44-44: Verify badge design: dot indicator vs. count number.The badge currently shows a small dot (1.5 x 1.5) rather than displaying the notification count number. Is this intentional?
Typical notification badges show the actual count (e.g., "3", "12+"). If showing the count is desired, consider this implementation:
- {notificationCount > 0 && <div className="bg-orange-2 absolute -right-1 -top-1 h-1.5 w-1.5 rounded-full" />} + {notificationCount > 0 && ( + <div className="bg-orange-2 absolute -right-1 -top-1 flex h-4 w-4 items-center justify-center rounded-full text-[10px] font-semibold text-white"> + {notificationCount > 9 ? '9+' : notificationCount} + </div> + )}If the dot indicator is the intended design, this comment can be ignored.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/(mobile-ui)/home/page.tsx (1)
146-191: Remove duplicate balance warning effect.Lines 146-168 and 170-191 contain identical logic for showing the balance warning modal. Remove one of these duplicate effects.
Apply this diff to remove the duplicate:
}, [balance, isFetchingBalance, showIOSPWAInstallModal, showAddMoneyPromptModal, user]) - // effect for showing balance warning modal - useEffect(() => { - if (isFetchingBalance || balance === undefined || !user) return - - if (typeof window !== 'undefined') { - const hasSeenBalanceWarning = getFromLocalStorage(`${user!.user.userId}-hasSeenBalanceWarning`) - const balanceInUsd = Number(formatUnits(balance, PEANUT_WALLET_TOKEN_DECIMALS)) - - // show if: - // 1. balance is above the threshold - // 2. user hasn't seen this warning in the current session - // 3. no other modals are currently active - if ( - balanceInUsd > BALANCE_WARNING_THRESHOLD && - !hasSeenBalanceWarning && - !showIOSPWAInstallModal && - !showAddMoneyPromptModal - ) { - setShowBalanceWarningModal(true) - } - } - }, [balance, isFetchingBalance, showIOSPWAInstallModal, showAddMoneyPromptModal, user]) - // effect for showing add money prompt modal
♻️ Duplicate comments (7)
src/components/Notifications/NotificationNavigation.tsx (4)
11-11: Remove unusedisLoadingstate.The
isLoadingstate is set but never read. Either remove it or display a loading indicator in the UI.Apply this diff to remove the unused state:
const [notificationCount, setNotificationCount] = useState<number>(0) - const [, setIsLoading] = useState<boolean>(false) useEffect(() => { const fetchNotificationCount = async () => { - setIsLoading(true) try { const { count } = await notificationsApi.unreadCount() setNotificationCount(count) } catch (error) { console.error(error) - } finally { - setIsLoading(false) } }Also applies to: 15-15, 22-22
19-20: Improve error handling with user feedback.Errors are only logged to console. Consider showing an error state in the UI or using a toast notification.
Example approach:
const [notificationCount, setNotificationCount] = useState<number>(0) + const [hasError, setHasError] = useState<boolean>(false) const fetchNotificationCount = async () => { try { const { count } = await notificationsApi.unreadCount() setNotificationCount(count) + setHasError(false) } catch (error) { console.error(error) + setHasError(true) } }Then indicate error state in the render with a different icon color or tooltip.
13-39: Add cleanup to prevent setState on unmounted component.The async fetch can call
setNotificationCountafter unmount, causing React warnings. Add an abort controller or mounted flag for cleanup.Apply this diff:
useEffect(() => { + let mounted = true + const fetchNotificationCount = async () => { try { const { count } = await notificationsApi.unreadCount() - setNotificationCount(count) + if (mounted) { + setNotificationCount(count) + } } catch (error) { - console.error(error) + if (mounted) { + console.error(error) + } } } fetchNotificationCount() const onUpdated = () => { void fetchNotificationCount() } if (typeof window !== 'undefined') { window.addEventListener('notifications:updated', onUpdated) } return () => { + mounted = false if (typeof window !== 'undefined') { window.removeEventListener('notifications:updated', onUpdated) } } }, [])
41-46: Add accessibility attributes for screen readers.The notification bell lacks accessible labels, making it difficult for screen reader users to understand the notification state.
Apply this diff:
<Link href={'/notifications'} - className={twMerge('relative')} + className={twMerge('relative')} + aria-label={`Notifications${notificationCount > 0 ? ` (${notificationCount} unread)` : ''}`} > <Icon name="bell" size={20} /> - {notificationCount > 0 && <div className="absolute -right-1 -top-1 h-1.5 w-1.5 rounded-full bg-orange-2" />} + {notificationCount > 0 && ( + <div + className="absolute -right-1 -top-1 h-1.5 w-1.5 rounded-full bg-orange-2" + aria-hidden="true" + /> + )} </Link>src/app/(mobile-ui)/notifications/page.tsx (1)
167-193: Avoid invalid Next.js Link href when ctaDeeplink is missing.Passing an empty string (
href ?? '') toLinkat line 168 can cause navigation errors or unexpected behavior whenctaDeeplinkis null. Instead, conditionally render the Link wrapper only whenhrefexists.Apply this diff to fix the issue:
const href = notif.ctaDeeplink return ( <Card key={notif.id} position={position} className="relative flex min-h-16 w-full items-center justify-center px-5 py-2" data-notification-id={notif.id} > - <Link - href={href ?? ''} - className="flex w-full items-center gap-3" - data-notification-id={notif.id} - onClick={() => handleNotificationClick(notif.id)} - > + {href ? ( + <Link + href={href} + className="flex w-full items-center gap-3" + data-notification-id={notif.id} + onClick={() => handleNotificationClick(notif.id)} + > + <Image + src={notif.iconUrl ?? PEANUTMAN_LOGO} + alt="icon" + width={32} + height={32} + className="size-8 min-w-8 self-center" + /> + + <div className="flex min-w-0 flex-col"> + <div className="flex items-center gap-2"> + <div className="line-clamp-2 font-semibold"> + {notif.title} + </div> + </div> + {notif.body ? ( + <div className="line-clamp-2 text-sm text-gray-600"> + {notif.body} + </div> + ) : null} + </div> + </Link> + ) : ( + <button + className="flex w-full items-center gap-3 text-left" + onClick={() => handleNotificationClick(notif.id)} + > <Image src={notif.iconUrl ?? PEANUTMAN_LOGO} alt="icon" width={32} height={32} className="size-8 min-w-8 self-center" /> <div className="flex min-w-0 flex-col"> <div className="flex items-center gap-2"> <div className="line-clamp-2 font-semibold"> {notif.title} </div> </div> {notif.body ? ( <div className="line-clamp-2 text-sm text-gray-600"> {notif.body} </div> ) : null} </div> - </Link> + </button> + )} {!notif.state.readAt ? ( <span className="absolute right-2 top-2 size-1.5 rounded-full bg-orange-2" /> ) : null}src/app/(mobile-ui)/home/page.tsx (1)
53-53: CRITICAL: Avoid double-instantiatinguseNotifications; modal state won't synchronize.
HomecallsuseNotifications()at line 53 andSetupNotificationsModalcalls it again internally. This creates two independent state instances. When the modal updates its local state (e.g., user clicks "Not now"),Home'sshowPermissionModalremainstrue, leaving the modal mounted (just hidden) and permanently suppressing the add-money prompt at line 210.Apply this diff to lift the hook and pass it as props:
- const { showPermissionModal } = useNotifications() + const notifications = useNotifications() + const { showPermissionModal } = notifications // ... rest of component - {showPermissionModal && <SetupNotificationsModal />} + {showPermissionModal && <SetupNotificationsModal notifications={notifications} />}Then update
SetupNotificationsModalto accept and destructure thenotificationsprop instead of callinguseNotifications()internally:// In SetupNotificationsModal.tsx export default function SetupNotificationsModal({ notifications }: { notifications: ReturnType<typeof useNotifications> }) { const { showPermissionModal, requestPermission, closePermissionModal, afterPermissionAttempt, hidePermissionModalImmediate, } = notifications // ... rest of component }Also applies to: 281-281
.env.example (1)
59-62: Public webhook var: confirm name alignment and no secret exposure
- Ensure the code uses the same key name as here (avoid mismatches with variants like NEXT_PUBLIC_WEBHOOKS_ONESIGNAL_URL or the earlier typo ONEIGNAL).
- If the webhook URL embeds any secrets/tokens, remove NEXT_PUBLIC_ and source it server-side.
Run to verify usage and naming:
#!/bin/bash set -euo pipefail echo "Searching for env reads of OneSignal webhook across the repo..." rg -n -S -C2 --no-ignore --hidden \ 'process\.env\.(NEXT_PUBLIC_ONESIGNAL_WEBHOOK|NEXT_PUBLIC_WEBHOOKS_ONESIGNAL_URL|NEXT_PUBLIC_ONEIGNAL_WEBHOOK)|NEXT_PUBLIC_(ONESIGNAL_WEBHOOK|WEBHOOKS_ONESIGNAL_URL|ONEIGNAL_WEBHOOK)' \ -g '!.git' -g '!node_modules' echo echo "Also show all mentions of 'onesignal' envs for context..." rg -n -S -i -C2 --no-ignore --hidden 'onesignal|oneignal|WEBHOOKS_ONESIGNAL' -g '!.git' -g '!node_modules' || true
🧹 Nitpick comments (3)
tailwind.config.js (1)
91-91:orange.2usage verified; consider consolidation
orange.2: '#FF5656'is used in your notification components (bg-orange-2in page.tsx and Navigation). It’s very similar toerror.3: '#FF4A4A'; consider reusing or consolidating these shades for consistency..env.example (2)
60-62: Reorder keys to satisfy dotenv-linter (alphabetical within block)Place NEXT_PUBLIC_ONESIGNAL_WEBHOOK before NEXT_PUBLIC_SAFARI_WEB_ID.
export NEXT_PUBLIC_ONESIGNAL_APP_ID= -export NEXT_PUBLIC_SAFARI_WEB_ID= -export NEXT_PUBLIC_ONESIGNAL_WEBHOOK= +export NEXT_PUBLIC_ONESIGNAL_WEBHOOK= +export NEXT_PUBLIC_SAFARI_WEB_ID=
59-59: Nit: normalize header textCapitalize and remove trailing space.
-# one signal +# OneSignal
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.env.example(1 hunks)src/app/(mobile-ui)/home/page.tsx(10 hunks)src/app/(mobile-ui)/notifications/page.tsx(1 hunks)src/components/Global/Icons/Icon.tsx(3 hunks)src/components/Notifications/NotificationNavigation.tsx(1 hunks)tailwind.config.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/components/Global/Icons/Icon.tsx (1)
src/components/Global/Icons/bell.tsx (1)
BellIcon(3-10)
src/app/(mobile-ui)/home/page.tsx (4)
src/hooks/useNotifications.ts (1)
useNotifications(22-416)src/components/Notifications/NotificationNavigation.tsx (1)
NotificationNavigation(9-47)src/components/Notifications/SetupNotificationsModal.tsx (1)
SetupNotificationsModal(5-71)src/components/Global/PostSignupActionManager/index.tsx (1)
PostSignupActionManager(11-76)
src/components/Notifications/NotificationNavigation.tsx (2)
src/services/notifications.ts (1)
notificationsApi(20-69)src/components/Global/Icons/Icon.tsx (1)
Icon(210-219)
src/app/(mobile-ui)/notifications/page.tsx (6)
src/services/notifications.ts (2)
InAppItem(4-16)notificationsApi(20-69)src/utils/dateGrouping.utils.ts (3)
getDateGroup(69-93)getDateGroupKey(128-148)formatGroupHeaderDate(102-120)src/components/Global/PeanutLoading/index.tsx (1)
PeanutLoading(4-19)src/components/Global/EmptyStates/EmptyState.tsx (1)
EmptyState(13-28)src/components/0_Bruddle/Button.tsx (1)
Button(76-267)src/components/Global/Card/index.tsx (1)
CardPosition(4-4)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 62-62: [UnorderedKey] The NEXT_PUBLIC_ONESIGNAL_WEBHOOK key should go before the NEXT_PUBLIC_SAFARI_WEB_ID key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (4)
src/components/Global/Icons/Icon.tsx (1)
61-61: LGTM! Icon integration follows established patterns.The BellIcon is correctly imported, added to the IconName type union, and registered in the iconComponents mapping, consistent with how other icons are integrated in this file.
Also applies to: 76-76, 148-148
src/app/(mobile-ui)/notifications/page.tsx (1)
26-42: Well-structured pagination and error handling.The implementation provides solid user experience with:
- Clear loading states for initial and subsequent loads
- Retry controls for both initial load failures and pagination errors
- Empty state handling
- Optimistic UI updates for mark-as-read
Also applies to: 126-237
src/app/(mobile-ui)/home/page.tsx (2)
194-234: Complex modal prioritization logic; verify interaction paths.The add-money prompt effect implements thorough modal prioritization (notification setup > iOS PWA > balance warning > add money), including race-condition handling at lines 220-223. The logic appears sound, but the extensive conditions make it prone to edge cases.
Please verify the following scenarios through manual testing:
- Notification modal appears → user dismisses it → add-money modal should show (if balance == 0)
- Multiple modals try to show simultaneously → correct priority order is maintained
- User navigates away and back → modal state resets appropriately
After fixing the critical double-instantiation issue, re-test these flows to ensure proper synchronization.
40-42: Notification UI integration looks good.The notification system is cleanly integrated:
NotificationNavigationin header alongside existing iconsSetupNotificationsModalconditionally rendered- Modal prioritization logic properly gates other modals
Once the critical hook duplication is fixed, this integration will work as intended.
Also applies to: 245-252
2 participants
