feat: implement push notifications and update dependencies - #54
Conversation
- Added support for push notifications using @convex-dev/expo-push-notifications. - Updated the convex configuration to include push notification handling in message actions. - Enhanced tests to verify push notification functionality upon message creation. - Updated dependencies, including upgrading convex to version 1.32.0 and related Expo packages. - Modified configuration files to reflect changes in app identifiers and notification settings.
📝 WalkthroughWalkthroughAdds Expo + Convex push notification support: backend mutations for token management and notification dispatch, Convex middleware config, frontend hook and layout integration for token registration and navigation, tests/mocks and dependency/config updates across frontend and backend. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Frontend as Frontend App
participant Convex as Convex Backend
participant ExpoService as Expo Push Service
participant Device as Device OS
rect rgba(100,150,200,0.5)
Note over Frontend,Convex: Token registration
Frontend->>Convex: recordPushToken(token)
Convex->>Convex: authenticate & store token
Convex-->>Frontend: { ok: true }
end
rect rgba(150,100,200,0.5)
Note over User,ExpoService: Message send -> notify
User->>Frontend: Send message
Frontend->>Convex: sendMessage(...)
Convex->>Convex: persist message
Convex->>Convex: sendNewMessageNotification(...)
Convex->>ExpoService: sendPushNotification(token, {title,body,data})
ExpoService->>Device: deliver push
Convex-->>Frontend: { messageId }
end
rect rgba(200,150,100,0.5)
Note over Device,Frontend: Notification interaction
Device->>Frontend: user taps notification
Frontend->>Frontend: parse data -> conversationId
Frontend->>Frontend: navigate to /messages/[id]
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
frontend/app.json (1)
52-55: Remove the unusedextra.router.originconfig.
frontend/app/listings/[id].tsx, Lines 29-38 still resolves the runtime origin fromEXPO_PUBLIC_APP_ORIGIN, andfrontend/hooks/usePushNotifications.ts, Lines 19-24 only consumesextra.eas.projectId. That makesextra.router.origina dead third source of truth that can drift without changing behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app.json` around lines 52 - 55, Remove the dead configuration key extra.router.origin from app.json: delete the "router": { "origin": "https://polybuys.com" } block so the app only relies on the runtime EXPO_PUBLIC_APP_ORIGIN and extra.eas.projectId currently used by listings/[id].tsx (origin resolution) and hooks/usePushNotifications.ts (EAS projectId). Verify no other code references extra.router.origin and run a quick grep to ensure there are no lingering uses before committing.frontend/hooks/usePushNotifications.ts (1)
115-128: Verify notification taps from a terminated app.This listener covers responses after JS is already running. Please verify on-device that tapping a notification from a fully terminated app still lands on
/messages/[id]; if it doesn't, bootstrap from the last notification response during startup as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/hooks/usePushNotifications.ts` around lines 115 - 128, The notification response listener (responseListener via Notifications.addNotificationResponseReceivedListener) only handles responses once JS is running; to handle taps that launch the app from a terminated state call Notifications.getLastNotificationResponseAsync() (or equivalent) during hook initialization, extract the same data.conversationId and, if present, invoke router.push({ pathname: '/messages/[id]', params: { id: conversationId } }) so the app bootstraps into the correct conversation; keep the existing responseListener logic for in-session responses.backend/convex/__tests__/messages.test.ts (1)
146-176: Add the push-failure case too.
backend/convex/messages.tsnow treats notification delivery as best-effort, but this test only proves the happy path. Please add a case wheresendPushNotificationMockrejects and assert the message is still persisted and returned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/__tests__/messages.test.ts` around lines 146 - 176, Add a new test that simulates push delivery failure by making sendPushNotificationMock reject, then call api.messages.sendMessage (using createConvexTest, createTestUser, createTestListing, createTestConversation and t.withIdentity as in the existing test) and assert that despite the rejection the message is persisted and returned: verify sendPushNotificationMock was called and rejected, then read the conversation/messages (or assert the API response) to confirm the message body, senderId and conversationId were saved and returned. Ensure the test restores/clears the mock rejection after running so other tests are unaffected.backend/convex/pushNotifications.ts (1)
42-50: Make token removal device-scoped.This mutation only lets the client unregister by user, not by the specific push token that is signing out. As soon as the same account is registered on multiple devices, a single-device sign-out has no way to preserve the others.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/pushNotifications.ts` around lines 42 - 50, The removePushToken mutation currently removes all tokens for a user; change it to require and accept a specific push token (or deviceId) in its args, validate presence after calling ctx.auth.getUserIdentity(), and pass both userId (identity.subject) and that token/deviceId to pushNotifications.removeToken so only that device's token is deleted; update the mutation signature (removePushToken), handler argument handling, and the call to pushNotifications.removeToken accordingly and ensure you throw ConvexError('Unauthorized') if identity is missing and a validation error if the token/deviceId arg is absent.frontend/app/_layout.tsx (1)
2-3: Consolidate imports from the same module.Lines 2 and 3 both import from
'convex/react'. Consider combining them into a single import statement for cleaner code organization.♻️ Suggested consolidation
-import { ConvexReactClient } from 'convex/react'; -import { useConvexAuth } from 'convex/react'; +import { ConvexReactClient, useConvexAuth } from 'convex/react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/_layout.tsx` around lines 2 - 3, Two separate imports from the same module should be consolidated: replace the two import statements that import ConvexReactClient and useConvexAuth from 'convex/react' with a single combined import. Locate the import lines that reference ConvexReactClient and useConvexAuth and merge them into one statement that imports both symbols from 'convex/react' to improve clarity and reduce redundancy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/convex/messages.ts`:
- Around line 113-125: The push notification call in sendMessage is currently
awaited and blocks message delivery; change the call to run out-of-band by
removing the await and making it fire-and-forget (e.g., void
ctx.runMutation(internal.pushNotifications.sendNewMessageNotification,
{...}).catch(err => console.error(...))). Keep the same payload (recipientId,
senderId: userId, conversationId: args.conversationId, listingId:
convo.listingId, messageId: result.messageId, body: args.body) and log errors in
the catch so failures remain non-fatal and do not delay the sendMessage
response.
In `@frontend/hooks/usePushNotifications.ts`:
- Around line 77-88: The cleanup currently runs after isAuthenticated becomes
false (in syncPushToken) so the backend rejects the removal; instead ensure the
device token is removed while the user is still authenticated by calling
removePushToken before auth is cleared. Concretely: invoke removePushToken from
the sign-out path (or add an effect that detects the upcoming transition and
runs removal while previousIsAuthenticated.current === true and isAuthenticated
is still true) so removePushToken executes with valid credentials; keep
previousIsAuthenticated updates only after successful removal and swallow errors
only if removal truly fails after an authenticated attempt.
In `@frontend/public/.well-known/assetlinks.json`:
- Line 7: Replace the placeholder value in assetlinks.json so Android App Links
can verify: open the public/.well-known/assetlinks.json and replace the
"YOUR_SHA256_CERT_FINGERPRINT" entry inside the "sha256_cert_fingerprints" array
with the actual app signing certificate fingerprint (SHA-256) used to sign the
Android app; verify the fingerprint format (uppercase hex with colons if
required by your verifier) and update any CI/secret management or release notes
if the signing key differs between debug/release builds so MainActivity/domain
verification uses the correct fingerprint.
---
Nitpick comments:
In `@backend/convex/__tests__/messages.test.ts`:
- Around line 146-176: Add a new test that simulates push delivery failure by
making sendPushNotificationMock reject, then call api.messages.sendMessage
(using createConvexTest, createTestUser, createTestListing,
createTestConversation and t.withIdentity as in the existing test) and assert
that despite the rejection the message is persisted and returned: verify
sendPushNotificationMock was called and rejected, then read the
conversation/messages (or assert the API response) to confirm the message body,
senderId and conversationId were saved and returned. Ensure the test
restores/clears the mock rejection after running so other tests are unaffected.
In `@backend/convex/pushNotifications.ts`:
- Around line 42-50: The removePushToken mutation currently removes all tokens
for a user; change it to require and accept a specific push token (or deviceId)
in its args, validate presence after calling ctx.auth.getUserIdentity(), and
pass both userId (identity.subject) and that token/deviceId to
pushNotifications.removeToken so only that device's token is deleted; update the
mutation signature (removePushToken), handler argument handling, and the call to
pushNotifications.removeToken accordingly and ensure you throw
ConvexError('Unauthorized') if identity is missing and a validation error if the
token/deviceId arg is absent.
In `@frontend/app.json`:
- Around line 52-55: Remove the dead configuration key extra.router.origin from
app.json: delete the "router": { "origin": "https://polybuys.com" } block so the
app only relies on the runtime EXPO_PUBLIC_APP_ORIGIN and extra.eas.projectId
currently used by listings/[id].tsx (origin resolution) and
hooks/usePushNotifications.ts (EAS projectId). Verify no other code references
extra.router.origin and run a quick grep to ensure there are no lingering uses
before committing.
In `@frontend/app/_layout.tsx`:
- Around line 2-3: Two separate imports from the same module should be
consolidated: replace the two import statements that import ConvexReactClient
and useConvexAuth from 'convex/react' with a single combined import. Locate the
import lines that reference ConvexReactClient and useConvexAuth and merge them
into one statement that imports both symbols from 'convex/react' to improve
clarity and reduce redundancy.
In `@frontend/hooks/usePushNotifications.ts`:
- Around line 115-128: The notification response listener (responseListener via
Notifications.addNotificationResponseReceivedListener) only handles responses
once JS is running; to handle taps that launch the app from a terminated state
call Notifications.getLastNotificationResponseAsync() (or equivalent) during
hook initialization, extract the same data.conversationId and, if present,
invoke router.push({ pathname: '/messages/[id]', params: { id: conversationId }
}) so the app bootstraps into the correct conversation; keep the existing
responseListener logic for in-session responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b4469ac-536f-4715-9daa-3e982a733fa4
⛔ Files ignored due to path filters (2)
backend/convex/_generated/api.d.tsis excluded by!**/_generated/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
backend/convex/__tests__/messages.test.tsbackend/convex/__tests__/testUtils.tsbackend/convex/convex.config.tsbackend/convex/messages.tsbackend/convex/pushNotifications.tsbackend/package.jsondocs/contributing.mdfrontend/app.jsonfrontend/app/_layout.tsxfrontend/eas.jsonfrontend/google-services.jsonfrontend/hooks/usePushNotifications.tsfrontend/package.jsonfrontend/public/.well-known/apple-app-site-associationfrontend/public/.well-known/assetlinks.jsonjest.config.jspackage.json
- Updated the sendMessage action to use a more structured approach for push notifications based on the environment. - Enhanced test setup for push notifications by refactoring the mock implementation for better clarity and maintainability. - Added error handling for push notification failures during message delivery. - Updated the useAuth hook to ensure push tokens are removed on sign-out, improving token management.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
frontend/hooks/usePushNotifications.ts (3)
52-55: Thrown error in async function could cause unhandled rejection.If
getExpoProjectId()returnsnull, the thrown error insideregisterForPushNotificationsAsyncwill propagate to the caller. ThesyncPushTokenfunction catches this (line 89-91), so it won't crash, but the error message "Expo project ID not found" might be confusing in logs since it's logged under "Failed to register push notifications".This is acceptable as-is, but consider returning
nullwith a warning for consistency with other early-return patterns in this function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/hooks/usePushNotifications.ts` around lines 52 - 55, In registerForPushNotificationsAsync, avoid throwing when getExpoProjectId() returns null; instead log a warning and return null for consistency with other early returns (e.g., the existing permission and token checks), so callers like syncPushToken still handle the null result; update the logic around getExpoProjectId() to use a warning (console.warn or the project logger) and return null rather than throwing an Error.
106-113: Consider defensive validation for notification payload data.The type assertion assumes a specific shape, but notification payloads from external sources could be malformed. A safer approach would validate at runtime:
🛡️ Safer payload extraction
const responseListener = Notifications.addNotificationResponseReceivedListener((response) => { - const data = response.notification.request.content.data as - | { conversationId?: string; type?: string } - | undefined; - const conversationId = data?.conversationId; + const data = response.notification.request.content.data; + const conversationId = + typeof data === 'object' && data !== null && 'conversationId' in data + ? String(data.conversationId) + : undefined; if (!conversationId) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/hooks/usePushNotifications.ts` around lines 106 - 113, The response listener in usePushNotifications.ts (Notifications.addNotificationResponseReceivedListener / responseListener) currently asserts the payload shape into "data" and reads conversationId directly; change this to defensive runtime validation: verify response.notification?.request?.content?.data exists and is an object, check typeof data.conversationId === 'string' (or valid ID format) before using it, and guard against other types by early-returning or logging the unexpected payload instead of assuming the asserted shape.
64-64:previousIsAuthenticatedref is written but never read.The
previousIsAuthenticatedref is assigned values on lines 78 and 88 but is never consumed anywhere in the hook. This appears to be dead code, possibly leftover from a previous iteration.♻️ Remove unused ref
export function usePushNotifications(isAuthenticated: boolean, isAuthLoading: boolean) { const router = useRouter(); const recordPushToken = useMutation(api.pushNotifications.recordPushToken); - const previousIsAuthenticated = useRef<boolean | null>(null); useEffect(() => { // ... const syncPushToken = async () => { if (!isAuthenticated) { - previousIsAuthenticated.current = false; return; } try { const token = await registerForPushNotificationsAsync(); if (!isMounted || !token) { return; } await recordPushToken({ token }); - previousIsAuthenticated.current = true; } catch (error) { console.error('Failed to register push notifications', error); } };Also applies to: 78-78, 88-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/hooks/usePushNotifications.ts` at line 64, The ref previousIsAuthenticated in the usePushNotifications hook is written to but never read, making it dead code; remove the unused ref and any assignments to it (references around where previousIsAuthenticated is set) and ensure no other logic depends on it—clean up the declaration const previousIsAuthenticated = useRef<boolean | null>(null) and the assignments at those locations so the hook contains only used state/refs.frontend/app.json (1)
37-44: Duplicaterouter.originconfiguration.The
originvalue"https://polybuys.com"is specified in both theexpo-routerplugin configuration (line 39) and in theextra.router.originblock (line 54). Consider removing one to avoid potential inconsistencies during future updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app.json` around lines 37 - 44, There is a duplicate router origin configured: "https://polybuys.com" appears both in the expo-router plugin options and in extra.router.origin; remove one to avoid divergence — e.g., delete the "origin" object from the expo-router plugin configuration (the entry alongside "expo-router") so the single source of truth is extra.router.origin, or conversely remove extra.router.origin and keep the plugin option; update only one of the two (expo-router plugin options or extra.router.origin) and ensure any runtime code that reads router origin uses that single location.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/convex/messages.ts`:
- Around line 126-141: The NODE_ENV check in the message enqueue logic
(process.env.NODE_ENV === 'test') never becomes true during Jest runs, so update
your Jest global setup (jest.setup.js) to set process.env.NODE_ENV = 'test'
before any mock definitions; this ensures the synchronous branch using
ctx.runMutation (calling internal.pushNotifications.sendNewMessageNotification)
is executed in tests instead of the ctx.scheduler.runAfter path and avoids race
conditions.
In `@frontend/app.json`:
- Line 19: The committed google-services.json referenced by the
"googleServicesFile" key in frontend/app.json must be removed from source
control and ignored: add google-services.json to .gitignore, remove it from git
cache with git rm --cached frontend/google-services.json and commit that change,
and update project docs (README.md or CONTRIBUTING.md) to explain how developers
obtain the file (Firebase Console or EAS Secrets); also change CI/CD build
instructions to inject the file via EAS Secrets rather than committing it.
---
Nitpick comments:
In `@frontend/app.json`:
- Around line 37-44: There is a duplicate router origin configured:
"https://polybuys.com" appears both in the expo-router plugin options and in
extra.router.origin; remove one to avoid divergence — e.g., delete the "origin"
object from the expo-router plugin configuration (the entry alongside
"expo-router") so the single source of truth is extra.router.origin, or
conversely remove extra.router.origin and keep the plugin option; update only
one of the two (expo-router plugin options or extra.router.origin) and ensure
any runtime code that reads router origin uses that single location.
In `@frontend/hooks/usePushNotifications.ts`:
- Around line 52-55: In registerForPushNotificationsAsync, avoid throwing when
getExpoProjectId() returns null; instead log a warning and return null for
consistency with other early returns (e.g., the existing permission and token
checks), so callers like syncPushToken still handle the null result; update the
logic around getExpoProjectId() to use a warning (console.warn or the project
logger) and return null rather than throwing an Error.
- Around line 106-113: The response listener in usePushNotifications.ts
(Notifications.addNotificationResponseReceivedListener / responseListener)
currently asserts the payload shape into "data" and reads conversationId
directly; change this to defensive runtime validation: verify
response.notification?.request?.content?.data exists and is an object, check
typeof data.conversationId === 'string' (or valid ID format) before using it,
and guard against other types by early-returning or logging the unexpected
payload instead of assuming the asserted shape.
- Line 64: The ref previousIsAuthenticated in the usePushNotifications hook is
written to but never read, making it dead code; remove the unused ref and any
assignments to it (references around where previousIsAuthenticated is set) and
ensure no other logic depends on it—clean up the declaration const
previousIsAuthenticated = useRef<boolean | null>(null) and the assignments at
those locations so the hook contains only used state/refs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 733251ba-bcd1-4ff4-bd0c-c30eaad46619
📒 Files selected for processing (6)
backend/convex/__tests__/messages.test.tsbackend/convex/messages.tsfrontend/app.jsonfrontend/app/_layout.tsxfrontend/hooks/useAuth.tsfrontend/hooks/usePushNotifications.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/convex/tests/messages.test.ts
- frontend/app/_layout.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/app.json (2)
3-4: Consider aligning the slug with the new branding.The app name is updated to
"PolyBuys"but the slug remains"polybuy". While the slug is typically used for Expo URLs and may have existing dependencies, this inconsistency could cause confusion during development and deployment.Additionally, the identifiers
com.polybuys.polybuysrepeat "polybuys" twice. A more conventional pattern would becom.polybuys.appor similar, though changing this after release requires migration. Verify this is intentional before the first production release.Also applies to: 11-11, 18-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app.json` around lines 3 - 4, Update the inconsistent identifiers so branding matches: change the "slug" value from "polybuy" to "polybuys" to align with the "name" field ("PolyBuys"), and update the bundle identifier entries (e.g., currently `com.polybuys.polybuys`) to a conventional pattern such as `com.polybuys.app` or another agreed identifier; locate and update the "slug" and bundleIdentifier keys in app.json (references: "name", "slug", bundleIdentifier) and confirm these changes are intentional before finalizing release.
52-60: Theeas.projectIdandownerare correctly configured for push notifications.The
extra.eas.projectIdis properly set and will be read byusePushNotifications.tsviaConstants.expoConfig?.extra?.eas?.projectIdfor push token registration.However, note that
router.originis defined in two places:
- Line 39: within the
expo-routerplugin configuration- Line 54: within the
extrablockConsider documenting why both are needed, or consolidating if one is sufficient, to prevent them from drifting out of sync during future updates.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app.json` around lines 52 - 60, The file defines router.origin in two places (the expo-router plugin config and extra.router.origin) which can drift out of sync; consolidate or document the reason for both by choosing a single source of truth (either remove the duplicate in the extra block or remove it from the plugin config) and update references accordingly (e.g., ensure usePushNotifications.ts reads Constants.expoConfig?.extra?.eas?.projectId still works and any code that reads router.origin uses the chosen location), or add a clear comment in app.json explaining why both "router.origin" (in the expo-router plugin config) and "extra.router.origin" (in the extra block) must coexist to prevent future divergence, while keeping "extra.eas.projectId" and "owner" unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/app.json`:
- Around line 3-4: Update the inconsistent identifiers so branding matches:
change the "slug" value from "polybuy" to "polybuys" to align with the "name"
field ("PolyBuys"), and update the bundle identifier entries (e.g., currently
`com.polybuys.polybuys`) to a conventional pattern such as `com.polybuys.app` or
another agreed identifier; locate and update the "slug" and bundleIdentifier
keys in app.json (references: "name", "slug", bundleIdentifier) and confirm
these changes are intentional before finalizing release.
- Around line 52-60: The file defines router.origin in two places (the
expo-router plugin config and extra.router.origin) which can drift out of sync;
consolidate or document the reason for both by choosing a single source of truth
(either remove the duplicate in the extra block or remove it from the plugin
config) and update references accordingly (e.g., ensure usePushNotifications.ts
reads Constants.expoConfig?.extra?.eas?.projectId still works and any code that
reads router.origin uses the chosen location), or add a clear comment in
app.json explaining why both "router.origin" (in the expo-router plugin config)
and "extra.router.origin" (in the extra block) must coexist to prevent future
divergence, while keeping "extra.eas.projectId" and "owner" unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e538881e-3cc3-43e0-95c2-41890136e04a
📒 Files selected for processing (1)
frontend/app.json
Linked Issues
Closes POLY-50
Summary
implemented push notifs. Tested on iOS - I don't have a way to test on android.
Checklist
npm run lint)devScreenshots / Demos
Summary by CodeRabbit
New Features
Documentation
App Config
Tests