Repository navigation
feat(mobile): add push notifications support for iOS Capacitor app - #300
Merged
Merged
Conversation
- Add @capacitor/push-notifications plugin to iOS app - Configure iOS entitlements for APNs (development environment) - Update AppDelegate.swift to handle push notification registration - Create push_notification_tokens database table with Drizzle migration - Add APNs push notification service with JWT authentication - Create API routes for device token registration/unregistration - Add usePushNotifications hook for frontend push management - Integrate push notifications with existing notification system - Notifications now trigger push notifications to registered devices The implementation uses Apple Push Notification service (APNs) with JWT authentication. Push notifications are sent alongside existing Socket.IO broadcasts and email notifications when createNotification is called. Environment variables required for production: - APNS_TEAM_ID: Apple Developer Team ID - APNS_KEY_ID: APNs auth key ID - APNS_PRIVATE_KEY: APNs auth private key (PEM format) - APNS_BUNDLE_ID: iOS app bundle ID (defaults to ai.pagespace.ios) https://claude.ai/code/session_014bS6ABJHDSt7QWcXhrXv55
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR implements end-to-end push notification support including iOS/Capacitor configuration, a Next.js API for push token management, a React hook for client-side handling, a database schema for token persistence, and server-side APNs integration with token lifecycle management. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as iOS/Web Client
participant AppDelegate as AppDelegate
participant APNs as Apple Push Notification Service
participant API as Web API
participant DB as Database
participant Server as Server Push Logic
Client->>AppDelegate: requestPermission()
AppDelegate->>APNs: register()
APNs-->>AppDelegate: deviceToken
AppDelegate->>Client: Post notification (token received)
Client->>API: POST /notifications/push-tokens
API->>DB: registerPushToken(userId, token, platform)
DB-->>API: tokenId
API-->>Client: {tokenId}
sequenceDiagram
participant App as Notification Creator
participant PushLib as Push Library
participant Server as APNs Server
participant DB as Database
participant iOS as iOS Device
App->>PushLib: sendPushNotification(userId, payload)
PushLib->>DB: getUserPushTokens(userId)
DB-->>PushLib: [tokens]
PushLib->>PushLib: getApnsJwtToken()
loop For each token
PushLib->>Server: Send APNs request (JWT auth)
Server-->>PushLib: Success/Error
alt Token invalid
PushLib->>DB: Mark token inactive
else Send success
PushLib->>DB: Update lastUsedAt
end
end
PushLib-->>App: {sent, failed, errors}
Server->>iOS: Push notification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@apps/ios/ios/App/App/App.entitlements`:
- Around line 14-15: The entitlements file currently sets the APNs environment
key <key>aps-environment</key> to "development", which will route pushes to the
sandbox for all builds; create separate entitlements for Release or a
Release-specific entitlements file that sets aps-environment to "production" (or
programmatically swap entitlements per build configuration), update your Xcode
build settings to use the new Release entitlements for the Release
configuration, and ensure the development entitlements remain for Debug so
production pushes use the production APNs environment.
In `@apps/web/src/hooks/usePushNotifications.ts`:
- Around line 97-167: The effect that sets up push notification listeners
(useEffect containing setupListeners) captures registerTokenWithServer but only
lists state.isSupported in its dependency array, risking a stale closure; fix by
stabilizing or including registerTokenWithServer in the dependencies — either
wrap registerTokenWithServer in useCallback (so its identity is stable) or add
registerTokenWithServer to the useEffect dependency array (and ensure
pushNotificationsRef and state.isSupported remain included), or alternatively
stop calling registerTokenWithServer from the registration listener and instead
store tokenRef.current and let the existing auto-register effect handle server
registration; update the dependency list on the effect that defines
setupListeners accordingly so the listener uses the current
registerTokenWithServer.
In `@packages/db/drizzle/0054_funny_mentor.sql`:
- Around line 7-20: The migration defines the
push_notification_tokens.failedAttempts column as text DEFAULT '0' but the
schema should use an integer; update the schema declaration to
integer('failedAttempts').default(0) (replacing the current text(...)
definition) and then regenerate the migration so the SQL uses "failedAttempts"
integer DEFAULT 0 in the CREATE TABLE; finally run pnpm db:generate to produce
the corrected migration file.
In `@packages/db/src/schema/push-notifications.ts`:
- Line 35: The column definition for failedAttempts currently uses
text('failedAttempts').default('0') but it should be an integer counter; change
the schema to use an integer type (e.g., integer('failedAttempts') or
smallint('failedAttempts')) with a numeric default 0 (default(0)) instead of the
string '0', and update any related migrations or schema exports that reference
failedAttempts to reflect the integer type.
In `@packages/lib/src/notifications/notifications.ts`:
- Around line 60-69: The push payload in sendPushNotification currently spreads
...params.metadata last which allows metadata to overwrite reserved keys (e.g.,
notificationId, type, pageId, driveId); fix by sanitizing metadata before
building the data object: create a filteredMetadata from params.metadata that
removes any reserved keys (notificationId, type, pageId, driveId, title, body)
or alternatively spread filteredMetadata first and then explicitly set
notificationId: notification[0].id, type: params.type, ...(params.pageId && {
pageId: params.pageId }), ...(params.driveId && { driveId: params.driveId }) so
reserved values in the payload cannot be overridden.
In `@packages/lib/src/notifications/push-notifications.ts`:
- Around line 72-76: The PEM normalization currently builds formattedKey from
privateKey but doesn't unescape literal "\n" sequences, which can produce
invalid PEMs; before checking for '-----BEGIN' and constructing formattedKey,
replace escaped newlines in the APNS secret (privateKey) with real newlines
(e.g., transform "\\n" to "\n"), trim surrounding whitespace, then apply the
existing logic that checks privateKey.includes('-----BEGIN') to decide whether
to wrap with BEGIN/END markers; update the variable used by sign.sign
(formattedKey) so the signing call uses the normalized PEM.
- Around line 149-162: The APNs fetch call can hang; wrap it with an
AbortController timeout: create an AbortController before calling fetch in the
send path that uses apnsHost/deviceToken (the block that assigns const response
= await fetch(...)), start a timer (e.g. setTimeout) that calls
controller.abort() after a configured timeout, pass controller.signal into the
fetch options, and ensure you clear the timer and perform any needed cleanup in
a finally block so the timer is always cleared and no leaked timers remain.
🧹 Nitpick comments (2)
apps/web/src/hooks/usePushNotifications.ts (1)
164-166: Optional: Suppress implicit return in forEach callback.Biome flags this because the arrow function implicitly returns the result of
remove(). While functionally harmless, you can make the intent clearer.♻️ Minor fix to address linter warning
return () => { - listeners.forEach(remove => remove()); + listeners.forEach(remove => { remove(); }); };packages/lib/src/notifications/push-notifications.ts (1)
203-257: Consider idempotent registration to prevent duplicate token rowsConcurrent registrations can race past
findFirstand insert duplicates, leading to duplicate pushes. Consider a unique constraint on(userId, token)plus an upsert to make this idempotent.
Comment on lines
+14
to
+15
| <key>aps-environment</key> | ||
| <string>development</string> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all entitlements files in the repository
find . -type f -name "*.entitlements" | head -20Repository: 2witstudios/PageSpace
Length of output: 104
🏁 Script executed:
# Check the directory structure of the iOS app
ls -la apps/ios/ios/App/App/Repository: 2witstudios/PageSpace
Length of output: 694
🏁 Script executed:
# Get more context about the entitlements file
cat -n apps/ios/ios/App/App/App.entitlementsRepository: 2witstudios/PageSpace
Length of output: 677
🏁 Script executed:
# Search for Release-related entitlements configuration
find apps/ios -type f \( -name "*.entitlements" -o -name "*.pbxproj" \) | head -20Repository: 2witstudios/PageSpace
Length of output: 149
🏁 Script executed:
# Extract entitlements configuration from the pbxproj file
grep -A 5 -B 5 "entitlements" apps/ios/ios/App/App.xcodeproj/project.pbxproj | head -50Repository: 2witstudios/PageSpace
Length of output: 3041
🏁 Script executed:
# Check for different build configurations
grep -E "buildConfigurations|Release|Debug" apps/ios/ios/App/App.xcodeproj/project.pbxproj | head -30Repository: 2witstudios/PageSpace
Length of output: 616
Avoid shipping APNs dev entitlement in release builds.
aps-environment set to development will route pushes to the APNs sandbox; production pushes will fail since the Release build configuration uses this same entitlements file. Separate the entitlements by build configuration or create a Release-specific entitlements file with aps-environment=production.
🤖 Prompt for AI Agents
In `@apps/ios/ios/App/App/App.entitlements` around lines 14 - 15, The entitlements
file currently sets the APNs environment key <key>aps-environment</key> to
"development", which will route pushes to the sandbox for all builds; create
separate entitlements for Release or a Release-specific entitlements file that
sets aps-environment to "production" (or programmatically swap entitlements per
build configuration), update your Xcode build settings to use the new Release
entitlements for the Release configuration, and ensure the development
entitlements remain for Debug so production pushes use the production APNs
environment.
Comment on lines
+97
to
+167
| // Set up listeners for push notification events | ||
| useEffect(() => { | ||
| if (!state.isSupported || !pushNotificationsRef.current) return; | ||
|
|
||
| const PushNotifications = pushNotificationsRef.current; | ||
| const listeners: (() => void)[] = []; | ||
|
|
||
| const setupListeners = async () => { | ||
| // Registration success | ||
| const registrationListener = await PushNotifications.addListener( | ||
| 'registration', | ||
| (token: { value: string }) => { | ||
| console.log('[PushNotifications] Registered with token:', token.value.substring(0, 20) + '...'); | ||
| tokenRef.current = token.value; | ||
| registerTokenWithServer(token.value); | ||
| } | ||
| ); | ||
| listeners.push(() => registrationListener.remove()); | ||
|
|
||
| // Registration error | ||
| const registrationErrorListener = await PushNotifications.addListener( | ||
| 'registrationError', | ||
| (error: { error: string }) => { | ||
| console.error('[PushNotifications] Registration error:', error); | ||
| setState(prev => ({ | ||
| ...prev, | ||
| error: error.error, | ||
| isLoading: false, | ||
| })); | ||
| } | ||
| ); | ||
| listeners.push(() => registrationErrorListener.remove()); | ||
|
|
||
| // Notification received while app is in foreground | ||
| const receivedListener = await PushNotifications.addListener( | ||
| 'pushNotificationReceived', | ||
| (notification: PushNotificationSchema) => { | ||
| console.log('[PushNotifications] Received:', notification); | ||
| // Handle foreground notification | ||
| // Could dispatch an event or update state here | ||
| if (typeof window !== 'undefined') { | ||
| window.dispatchEvent(new CustomEvent('push:received', { | ||
| detail: notification, | ||
| })); | ||
| } | ||
| } | ||
| ); | ||
| listeners.push(() => receivedListener.remove()); | ||
|
|
||
| // Notification tapped | ||
| const actionListener = await PushNotifications.addListener( | ||
| 'pushNotificationActionPerformed', | ||
| (action: ActionPerformed) => { | ||
| console.log('[PushNotifications] Action performed:', action); | ||
| // Handle notification tap | ||
| if (typeof window !== 'undefined') { | ||
| window.dispatchEvent(new CustomEvent('push:action', { | ||
| detail: action, | ||
| })); | ||
| } | ||
| } | ||
| ); | ||
| listeners.push(() => actionListener.remove()); | ||
| }; | ||
|
|
||
| setupListeners(); | ||
|
|
||
| return () => { | ||
| listeners.forEach(remove => remove()); | ||
| }; | ||
| }, [state.isSupported]); |
Contributor
There was a problem hiding this comment.
Potential stale closure: registerTokenWithServer captured without dependencies.
The setupListeners effect (Line 98) captures registerTokenWithServer in the registration listener (Line 111), but the effect's dependency array only includes state.isSupported. Since registerTokenWithServer depends on isAuthenticated and platform, this creates a stale closure risk—the callback may use outdated values.
🔧 Proposed fix: Add registerTokenWithServer to dependencies
return () => {
listeners.forEach(remove => remove());
};
- }, [state.isSupported]);
+ }, [state.isSupported, registerTokenWithServer]);Note: This may cause listeners to be re-registered when registerTokenWithServer changes. Consider whether to store the token and register it via the auto-register effect (Lines 284-295) instead, which already handles authentication state changes.
📝 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
| // Set up listeners for push notification events | |
| useEffect(() => { | |
| if (!state.isSupported || !pushNotificationsRef.current) return; | |
| const PushNotifications = pushNotificationsRef.current; | |
| const listeners: (() => void)[] = []; | |
| const setupListeners = async () => { | |
| // Registration success | |
| const registrationListener = await PushNotifications.addListener( | |
| 'registration', | |
| (token: { value: string }) => { | |
| console.log('[PushNotifications] Registered with token:', token.value.substring(0, 20) + '...'); | |
| tokenRef.current = token.value; | |
| registerTokenWithServer(token.value); | |
| } | |
| ); | |
| listeners.push(() => registrationListener.remove()); | |
| // Registration error | |
| const registrationErrorListener = await PushNotifications.addListener( | |
| 'registrationError', | |
| (error: { error: string }) => { | |
| console.error('[PushNotifications] Registration error:', error); | |
| setState(prev => ({ | |
| ...prev, | |
| error: error.error, | |
| isLoading: false, | |
| })); | |
| } | |
| ); | |
| listeners.push(() => registrationErrorListener.remove()); | |
| // Notification received while app is in foreground | |
| const receivedListener = await PushNotifications.addListener( | |
| 'pushNotificationReceived', | |
| (notification: PushNotificationSchema) => { | |
| console.log('[PushNotifications] Received:', notification); | |
| // Handle foreground notification | |
| // Could dispatch an event or update state here | |
| if (typeof window !== 'undefined') { | |
| window.dispatchEvent(new CustomEvent('push:received', { | |
| detail: notification, | |
| })); | |
| } | |
| } | |
| ); | |
| listeners.push(() => receivedListener.remove()); | |
| // Notification tapped | |
| const actionListener = await PushNotifications.addListener( | |
| 'pushNotificationActionPerformed', | |
| (action: ActionPerformed) => { | |
| console.log('[PushNotifications] Action performed:', action); | |
| // Handle notification tap | |
| if (typeof window !== 'undefined') { | |
| window.dispatchEvent(new CustomEvent('push:action', { | |
| detail: action, | |
| })); | |
| } | |
| } | |
| ); | |
| listeners.push(() => actionListener.remove()); | |
| }; | |
| setupListeners(); | |
| return () => { | |
| listeners.forEach(remove => remove()); | |
| }; | |
| }, [state.isSupported]); | |
| // Set up listeners for push notification events | |
| useEffect(() => { | |
| if (!state.isSupported || !pushNotificationsRef.current) return; | |
| const PushNotifications = pushNotificationsRef.current; | |
| const listeners: (() => void)[] = []; | |
| const setupListeners = async () => { | |
| // Registration success | |
| const registrationListener = await PushNotifications.addListener( | |
| 'registration', | |
| (token: { value: string }) => { | |
| console.log('[PushNotifications] Registered with token:', token.value.substring(0, 20) + '...'); | |
| tokenRef.current = token.value; | |
| registerTokenWithServer(token.value); | |
| } | |
| ); | |
| listeners.push(() => registrationListener.remove()); | |
| // Registration error | |
| const registrationErrorListener = await PushNotifications.addListener( | |
| 'registrationError', | |
| (error: { error: string }) => { | |
| console.error('[PushNotifications] Registration error:', error); | |
| setState(prev => ({ | |
| ...prev, | |
| error: error.error, | |
| isLoading: false, | |
| })); | |
| } | |
| ); | |
| listeners.push(() => registrationErrorListener.remove()); | |
| // Notification received while app is in foreground | |
| const receivedListener = await PushNotifications.addListener( | |
| 'pushNotificationReceived', | |
| (notification: PushNotificationSchema) => { | |
| console.log('[PushNotifications] Received:', notification); | |
| // Handle foreground notification | |
| // Could dispatch an event or update state here | |
| if (typeof window !== 'undefined') { | |
| window.dispatchEvent(new CustomEvent('push:received', { | |
| detail: notification, | |
| })); | |
| } | |
| } | |
| ); | |
| listeners.push(() => receivedListener.remove()); | |
| // Notification tapped | |
| const actionListener = await PushNotifications.addListener( | |
| 'pushNotificationActionPerformed', | |
| (action: ActionPerformed) => { | |
| console.log('[PushNotifications] Action performed:', action); | |
| // Handle notification tap | |
| if (typeof window !== 'undefined') { | |
| window.dispatchEvent(new CustomEvent('push:action', { | |
| detail: action, | |
| })); | |
| } | |
| } | |
| ); | |
| listeners.push(() => actionListener.remove()); | |
| }; | |
| setupListeners(); | |
| return () => { | |
| listeners.forEach(remove => remove()); | |
| }; | |
| }, [state.isSupported, registerTokenWithServer]); |
🧰 Tools
🪛 Biome (2.3.13)
[error] 165-165: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@apps/web/src/hooks/usePushNotifications.ts` around lines 97 - 167, The effect
that sets up push notification listeners (useEffect containing setupListeners)
captures registerTokenWithServer but only lists state.isSupported in its
dependency array, risking a stale closure; fix by stabilizing or including
registerTokenWithServer in the dependencies — either wrap
registerTokenWithServer in useCallback (so its identity is stable) or add
registerTokenWithServer to the useEffect dependency array (and ensure
pushNotificationsRef and state.isSupported remain included), or alternatively
stop calling registerTokenWithServer from the registration listener and instead
store tokenRef.current and let the existing auto-register effect handle server
registration; update the dependency list on the effect that defines
setupListeners accordingly so the listener uses the current
registerTokenWithServer.
Comment on lines
+7
to
+20
| CREATE TABLE IF NOT EXISTS "push_notification_tokens" ( | ||
| "id" text PRIMARY KEY NOT NULL, | ||
| "userId" text NOT NULL, | ||
| "token" text NOT NULL, | ||
| "platform" "PushPlatformType" NOT NULL, | ||
| "deviceId" text, | ||
| "deviceName" text, | ||
| "isActive" boolean DEFAULT true NOT NULL, | ||
| "webPushSubscription" text, | ||
| "createdAt" timestamp DEFAULT now() NOT NULL, | ||
| "updatedAt" timestamp DEFAULT now() NOT NULL, | ||
| "lastUsedAt" timestamp, | ||
| "failedAttempts" text DEFAULT '0', | ||
| "lastFailedAt" timestamp |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for failedAttempts in schema files
find packages/db/src/schema -type f -name "*.ts" 2>/dev/null | head -20Repository: 2witstudios/PageSpace
Length of output: 786
🏁 Script executed:
# Check if schema directory exists and list its contents
ls -la packages/db/src/ 2>/dev/null || echo "No schema dir found"Repository: 2witstudios/PageSpace
Length of output: 709
🏁 Script executed:
# Search for failedAttempts in all TypeScript files
rg -n 'failedAttempts' packages/db/ --type=ts -B2 -A2Repository: 2witstudios/PageSpace
Length of output: 485
🏁 Script executed:
# Check the context around failedAttempts to understand its usage
rg -n 'failedAttempts' packages/db/src/schema/push-notifications.ts -B5 -A5Repository: 2witstudios/PageSpace
Length of output: 682
🏁 Script executed:
# Also check if there's any index or other constraints
cat packages/db/src/schema/push-notifications.tsRepository: 2witstudios/PageSpace
Length of output: 2339
Change failedAttempts to integer type in the schema and regenerate the migration.
The column is defined as text with DEFAULT '0' in packages/db/src/schema/push-notifications.ts (line 35), which will break numeric comparisons and increment operations. Change it to an integer type (integer('failedAttempts').default(0)) and run pnpm db:generate to regenerate the migration file.
🤖 Prompt for AI Agents
In `@packages/db/drizzle/0054_funny_mentor.sql` around lines 7 - 20, The migration
defines the push_notification_tokens.failedAttempts column as text DEFAULT '0'
but the schema should use an integer; update the schema declaration to
integer('failedAttempts').default(0) (replacing the current text(...)
definition) and then regenerate the migration so the SQL uses "failedAttempts"
integer DEFAULT 0 in the CREATE TABLE; finally run pnpm db:generate to produce
the corrected migration file.
| lastUsedAt: timestamp('lastUsedAt', { mode: 'date' }), | ||
|
|
||
| // Track failed delivery attempts for cleanup | ||
| failedAttempts: text('failedAttempts').default('0'), |
Contributor
There was a problem hiding this comment.
failedAttempts should be an integer, not text.
The failedAttempts column is used as a counter but is defined as text with a string default '0'. This will require type coercion when incrementing and could lead to unexpected behavior.
🔧 Proposed fix
+import { pgTable, text, timestamp, boolean, index, pgEnum, integer } from 'drizzle-orm/pg-core';
-import { pgTable, text, timestamp, boolean, index, pgEnum } from 'drizzle-orm/pg-core';
...
- failedAttempts: text('failedAttempts').default('0'),
+ failedAttempts: integer('failedAttempts').default(0).notNull(),📝 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
| failedAttempts: text('failedAttempts').default('0'), | |
| import { pgTable, text, timestamp, boolean, index, pgEnum, integer } from 'drizzle-orm/pg-core'; | |
| // ... other code ... | |
| failedAttempts: integer('failedAttempts').default(0).notNull(), |
🤖 Prompt for AI Agents
In `@packages/db/src/schema/push-notifications.ts` at line 35, The column
definition for failedAttempts currently uses text('failedAttempts').default('0')
but it should be an integer counter; change the schema to use an integer type
(e.g., integer('failedAttempts') or smallint('failedAttempts')) with a numeric
default 0 (default(0)) instead of the string '0', and update any related
migrations or schema exports that reference failedAttempts to reflect the
integer type.
Comment on lines
+60
to
+69
| void sendPushNotification(params.userId, { | ||
| title: params.title, | ||
| body: params.message, | ||
| data: { | ||
| notificationId: notification[0].id, | ||
| type: params.type, | ||
| ...(params.pageId && { pageId: params.pageId }), | ||
| ...(params.driveId && { driveId: params.driveId }), | ||
| ...params.metadata, | ||
| }, |
Contributor
There was a problem hiding this comment.
Prevent metadata from overriding reserved push payload keys.
...params.metadata is spread last, so a metadata key like type or notificationId can overwrite reserved values and corrupt the payload. Consider placing metadata first or filtering reserved keys.
Suggested fix
- data: {
- notificationId: notification[0].id,
- type: params.type,
- ...(params.pageId && { pageId: params.pageId }),
- ...(params.driveId && { driveId: params.driveId }),
- ...params.metadata,
- },
+ data: {
+ ...params.metadata,
+ notificationId: notification[0].id,
+ type: params.type,
+ ...(params.pageId && { pageId: params.pageId }),
+ ...(params.driveId && { driveId: params.driveId }),
+ },📝 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
| void sendPushNotification(params.userId, { | |
| title: params.title, | |
| body: params.message, | |
| data: { | |
| notificationId: notification[0].id, | |
| type: params.type, | |
| ...(params.pageId && { pageId: params.pageId }), | |
| ...(params.driveId && { driveId: params.driveId }), | |
| ...params.metadata, | |
| }, | |
| void sendPushNotification(params.userId, { | |
| title: params.title, | |
| body: params.message, | |
| data: { | |
| ...params.metadata, | |
| notificationId: notification[0].id, | |
| type: params.type, | |
| ...(params.pageId && { pageId: params.pageId }), | |
| ...(params.driveId && { driveId: params.driveId }), | |
| }, |
🤖 Prompt for AI Agents
In `@packages/lib/src/notifications/notifications.ts` around lines 60 - 69, The
push payload in sendPushNotification currently spreads ...params.metadata last
which allows metadata to overwrite reserved keys (e.g., notificationId, type,
pageId, driveId); fix by sanitizing metadata before building the data object:
create a filteredMetadata from params.metadata that removes any reserved keys
(notificationId, type, pageId, driveId, title, body) or alternatively spread
filteredMetadata first and then explicitly set notificationId:
notification[0].id, type: params.type, ...(params.pageId && { pageId:
params.pageId }), ...(params.driveId && { driveId: params.driveId }) so reserved
values in the payload cannot be overridden.
Comment on lines
+72
to
+76
| // The private key should be in PEM format | ||
| const formattedKey = privateKey.includes('-----BEGIN') | ||
| ? privateKey | ||
| : `-----BEGIN PRIVATE KEY-----\n${privateKey}\n-----END PRIVATE KEY-----`; | ||
|
|
Contributor
There was a problem hiding this comment.
Normalize escaped newlines in APNS_PRIVATE_KEY
Env secrets often store PEMs with \n escapes. Wrapping without unescaping can produce an invalid key and cause sign.sign failures in production.
🔧 Suggested fix
- const formattedKey = privateKey.includes('-----BEGIN')
- ? privateKey
- : `-----BEGIN PRIVATE KEY-----\n${privateKey}\n-----END PRIVATE KEY-----`;
+ const normalizedKey = privateKey.replace(/\\n/g, '\n');
+ const formattedKey = normalizedKey.includes('-----BEGIN')
+ ? normalizedKey
+ : `-----BEGIN PRIVATE KEY-----\n${normalizedKey}\n-----END PRIVATE KEY-----`;📝 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
| // The private key should be in PEM format | |
| const formattedKey = privateKey.includes('-----BEGIN') | |
| ? privateKey | |
| : `-----BEGIN PRIVATE KEY-----\n${privateKey}\n-----END PRIVATE KEY-----`; | |
| // The private key should be in PEM format | |
| const normalizedKey = privateKey.replace(/\\n/g, '\n'); | |
| const formattedKey = normalizedKey.includes('-----BEGIN') | |
| ? normalizedKey | |
| : `-----BEGIN PRIVATE KEY-----\n${normalizedKey}\n-----END PRIVATE KEY-----`; |
🤖 Prompt for AI Agents
In `@packages/lib/src/notifications/push-notifications.ts` around lines 72 - 76,
The PEM normalization currently builds formattedKey from privateKey but doesn't
unescape literal "\n" sequences, which can produce invalid PEMs; before checking
for '-----BEGIN' and constructing formattedKey, replace escaped newlines in the
APNS secret (privateKey) with real newlines (e.g., transform "\\n" to "\n"),
trim surrounding whitespace, then apply the existing logic that checks
privateKey.includes('-----BEGIN') to decide whether to wrap with BEGIN/END
markers; update the variable used by sign.sign (formattedKey) so the signing
call uses the normalized PEM.
Comment on lines
+149
to
+162
| const response = await fetch( | ||
| `https://${apnsHost}/3/device/${deviceToken}`, | ||
| { | ||
| method: 'POST', | ||
| headers: { | ||
| 'authorization': `bearer ${jwtToken}`, | ||
| 'apns-topic': bundleId, | ||
| 'apns-push-type': 'alert', | ||
| 'apns-priority': '10', | ||
| 'content-type': 'application/json', | ||
| }, | ||
| body: JSON.stringify(apnsPayload), | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Apple APNs Provider API HTTP/2 request timeout recommended
💡 Result:
Apple doesn’t publish a specific “recommended HTTP/2 request timeout” for APNs Provider API requests. What Apple does document is that APNs is a long-lived HTTP/2 connection you should keep open, monitor, and be prepared to recreate if the connection becomes unhealthy or APNs closes it. [1]
Practical guidance used by many providers:
- Set a per-request deadline (time waiting for the HTTP response from APNs) of about 5–10 seconds and treat longer hangs as a network/transport problem; cancel the request and retry (ideally on a fresh connection if you suspect the existing one is stuck). (Example: a common Go APNs client shows a 10s per-push timeout via context.) [2]
- Keep the HTTP/2 connection open and use HTTP/2 PING to check connection health; handle GOAWAY by reconnecting and retrying in-flight notifications as needed. [1]
- Avoid rapid connect/disconnect patterns; Apple recommends keeping connections open across multiple notifications. [3]
Sources: Apple APNs connection management/GOAWAY/PING guidance [1], common client practice example (10s) [2], Apple best practice to keep connections open [3].
References:
[1] (developer.apple.com)
[2] (github.com)
[3] (developer.apple.com)
Citations:
- 1: https://developer.apple.com/library/archive/documentation/NetworkingInternet/Conceptual/RemoteNotificationsPG/CommunicatingwithAPNs.html?utm_source=openai
- 2: https://github.com/sideshow/apns2?utm_source=openai
- 3: https://developer.apple.com/library/archive/documentation/NetworkingInternet/Conceptual/RemoteNotificationsPG/CommunicatingwithAPNs.html?utm_source=openai
🏁 Script executed:
find . -type f -name "push-notifications.ts" | head -5Repository: 2witstudios/PageSpace
Length of output: 166
🏁 Script executed:
cat -n packages/lib/src/notifications/push-notifications.ts | head -200Repository: 2witstudios/PageSpace
Length of output: 6699
🏁 Script executed:
cat -n packages/lib/src/notifications/push-notifications.ts | sed -n '180,195p'Repository: 2witstudios/PageSpace
Length of output: 546
Add a timeout/abort for the APNs request
The APNs call can hang indefinitely and block request threads. Add an AbortController timeout to prevent requests from timing out and ensure cleanup in finally.
⏱️ Suggested timeout guard
- try {
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 10_000);
+ try {
const jwtToken = getApnsJwtToken();
...
const response = await fetch(
`https://${apnsHost}/3/device/${deviceToken}`,
{
method: 'POST',
headers: {
'authorization': `bearer ${jwtToken}`,
'apns-topic': bundleId,
'apns-push-type': 'alert',
'apns-priority': '10',
'content-type': 'application/json',
},
- body: JSON.stringify(apnsPayload),
+ body: JSON.stringify(apnsPayload),
+ signal: controller.signal,
}
);
...
- } catch (error) {
+ } catch (error) {
console.error('APNs send error:', error);
return {
success: false,
tokenId,
error: error instanceof Error ? error.message : 'Unknown error',
};
+ } finally {
+ clearTimeout(timeoutId);
}🤖 Prompt for AI Agents
In `@packages/lib/src/notifications/push-notifications.ts` around lines 149 - 162,
The APNs fetch call can hang; wrap it with an AbortController timeout: create an
AbortController before calling fetch in the send path that uses
apnsHost/deviceToken (the block that assigns const response = await fetch(...)),
start a timer (e.g. setTimeout) that calls controller.abort() after a configured
timeout, pass controller.signal into the fetch options, and ensure you clear the
timer and perform any needed cleanup in a finally block so the timer is always
cleared and no leaked timers remain.
Resolved migration conflicts: - Renumbered push-notifications migration from 0054 to 0055 - Kept page-views migration as 0054 (from master) - Combined both schema exports Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Resolved migration conflicts: - Kept hotkeys migration as 0055 (from master) - Renumbered push-notifications migration to 0056 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2witstudios
added a commit
that referenced
this pull request
Feb 2, 2026
This dependency was missing from the push notifications feature (#300). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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.
The implementation uses Apple Push Notification service (APNs) with JWT
authentication. Push notifications are sent alongside existing Socket.IO
broadcasts and email notifications when createNotification is called.
Environment variables required for production:
https://claude.ai/code/session_014bS6ABJHDSt7QWcXhrXv55
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.