fix: ios videoconf push notification deduplication#7050
fix: ios videoconf push notification deduplication#7050deepak0x wants to merge 1 commit intoRocketChat:developfrom
Conversation
WalkthroughAdds persistent and in-memory deduplication for video conference push notifications across notification entry points and the deep-linking saga; handlers short-circuit when an ID was already processed, otherwise the ID is recorded and processing continues. Also updates notification callback types to accept async handlers and guards promise errors. Changes
Sequence Diagram(s)sequenceDiagram
participant System as Notifications
participant App as onNotification
participant Storage as AsyncStorage
participant Dispatcher as deepLinkingClickCallPush
System->>App: deliver push (videoconf)
App->>Storage: isPushVideoConfAlreadyProcessed(id)?
alt already processed
App-->>System: return (short-circuit)
else not processed
App->>Storage: persist id
App->>Dispatcher: dispatch deepLinkingClickCallPush(event)
end
sequenceDiagram
participant Dispatcher as UI / NotificationResponse
participant Saga as handleClickCallPush
participant Storage as AsyncStorage
participant Service as HostNormalization / JoinFlow
Dispatcher->>Saga: invoke handleClickCallPush(params with notId)
Saga->>Storage: isPushVideoConfAlreadyProcessed(notId)?
alt already processed
Saga-->>Dispatcher: return (handled)
else not processed
Saga->>Storage: persist notId
Saga->>Service: continue processing (normalize host, join call)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/notifications/index.ts (1)
142-142:⚠️ Potential issue | 🟡 MinorAwait the async
onNotificationcall to ensure dedup completes.
onNotificationis now async, but this call site doesn't await it. The deduplication logic (AsyncStorage read/write) may not complete before subsequent notification handling runs, potentially causing race conditions.Suggested fix
- onNotification(notification); + await onNotification(notification);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/notifications/index.ts` at line 142, The call to onNotification(notification) must be awaited so its async dedup logic completes before proceeding; update the surrounding handler (the function containing this call) to be async if it isn't, replace the bare call with await onNotification(notification), and wrap it in try/catch to log or handle errors from onNotification to avoid unhandled promise rejections (referencing the onNotification symbol and the enclosing handler function where the call occurs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/notifications/index.ts`:
- Line 20: The onNotification signature was changed to async (Promise<void>) but
pushNotificationConfigure expects a synchronous callback; update the push
listener contract and callers: change the callback type used by
pushNotificationConfigure to (notification: INotification) => Promise<void>,
keep onNotification as async, and then await its invocation wherever listeners
are invoked (the push.ts listener invocation, the caller in app/index.tsx, and
the local checkPendingNotification call) so any AsyncStorage rejections are
handled instead of becoming unhandled promise rejections.
In `@app/sagas/deepLinking.js`:
- Around line 249-261: deepLinkingClickCallPush's dedup logic uses params.notId
but callers sometimes send the ID under other keys (e.g., push.identifier or
push.payload.notId), so update the saga to normalize the ID before dedup: derive
notId by checking params.notId || params.identifier || params.payload?.notId ||
params.push?.identifier || params.push?.payload?.notId (or similar fallbacks) at
the top of deepLinkingClickCallPush, then use that normalized notId for the
AsyncStorage checks; alternatively ensure all callers (the code that dispatches
deepLinkingClickCallPush) pass a notId property, but keep the normalization in
deepLinkingClickCallPush to handle Android/native paths (functions referenced:
deepLinkingClickCallPush, getInitialNotification, index dispatchers).
---
Outside diff comments:
In `@app/lib/notifications/index.ts`:
- Line 142: The call to onNotification(notification) must be awaited so its
async dedup logic completes before proceeding; update the surrounding handler
(the function containing this call) to be async if it isn't, replace the bare
call with await onNotification(notification), and wrap it in try/catch to log or
handle errors from onNotification to avoid unhandled promise rejections
(referencing the onNotification symbol and the enclosing handler function where
the call occurs).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 099cb964-acc5-4350-802a-14ce64198839
📒 Files selected for processing (3)
app/lib/notifications/index.tsapp/lib/notifications/videoConf/getInitialNotification.tsapp/sagas/deepLinking.js
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-10T15:21:45.098Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7046
File: app/containers/InAppNotification/NotifierComponent.stories.tsx:46-75
Timestamp: 2026-03-10T15:21:45.098Z
Learning: In `app/containers/InAppNotification/NotifierComponent.tsx` (React Native, Rocket.Chat), `NotifierComponent` is exported as a Redux-connected component via `connect(mapStateToProps)`. The `isMasterDetail` prop is automatically injected from `state.app.isMasterDetail` and does not need to be passed explicitly at call sites or in Storybook stories that use the default (connected) export.
Applied to files:
app/lib/notifications/videoConf/getInitialNotification.tsapp/sagas/deepLinking.jsapp/lib/notifications/index.ts
🔇 Additional comments (2)
app/lib/notifications/videoConf/getInitialNotification.ts (1)
70-77: Deduplication logic looks correct for iOS cold boot path.The implementation properly:
- Reads the stored ID before checking
- Returns early on duplicate
- Stores the new ID before dispatching
One consideration: ensure
notification.request.identifierused here matches the value stored by other paths (push.identifier || push.payload?.notIdinindex.ts). Based on context snippet frompush.ts(line 79), both should resolve to the samenotification.request.identifiervalue for iOS, so this should be consistent.app/lib/notifications/index.ts (1)
28-35: Deduplication logic is correct and well-structured.Both code paths properly:
- Read the stored ID first
- Compare and early-return on match
- Store the new ID before dispatching
The use of
push.identifier || push.payload?.notIdas fallback ensures coverage across different notification sources.Also applies to: 52-59
deepak0x
force-pushed
the
fix/ios-videoconf-notification-dedup
branch
from
dba46f0 to
120ce2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/lib/notifications/index.ts (1)
28-35: Extract duplicated deduplication logic into a helper function.The deduplication logic at lines 28-35 and lines 52-59 is nearly identical. Extract it to reduce duplication and ensure consistent behavior:
♻️ Suggested helper extraction
+const VIDEOCONF_DEDUP_KEY = 'lastProcessedVideoConfNotificationId'; + +const isVideoConfDuplicate = async (currentId: string | undefined): Promise<boolean> => { + if (!currentId) return false; + const lastId = await AsyncStorage.getItem(VIDEOCONF_DEDUP_KEY); + if (lastId === currentId) return true; + await AsyncStorage.setItem(VIDEOCONF_DEDUP_KEY, currentId); + return false; +}; + export const onNotification = async (push: INotification): Promise<void> => { // ... if (identifier === 'ACCEPT_ACTION' || identifier === 'DECLINE_ACTION') { if (push?.payload?.ejson) { try { const notification = EJSON.parse(push.payload.ejson); - const lastId = await AsyncStorage.getItem('lastProcessedVideoConfNotificationId'); const currentId = push.identifier || push.payload?.notId; - if (currentId && lastId === currentId) { - return; - } - if (currentId) { - await AsyncStorage.setItem('lastProcessedVideoConfNotificationId', currentId); - } + if (await isVideoConfDuplicate(currentId)) return; store.dispatch(/* ... */);Also applies to: 52-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/notifications/index.ts` around lines 28 - 35, Extract the duplicated deduplication block into a helper called shouldSkipVideoConfNotification(push) that reads AsyncStorage.getItem('lastProcessedVideoConfNotificationId'), computes currentId from push.identifier || push.payload?.notId, returns true if currentId exists and equals lastId, otherwise stores currentId in AsyncStorage when present and returns false; replace both original blocks (the one using lastProcessedVideoConfNotificationId and push.identifier/push.payload?.notId at lines shown) with a single await shouldSkipVideoConfNotification(push) check and early return when true to keep behavior identical.app/index.tsx (1)
142-145: Consider simplifying the Promise handling.Since
onNotificationis now consistentlyasyncand always returnsPromise<void>, the duck-typing check for.catchis unnecessary. You can simplify to:-const result = onNotification(notification); -if (result?.catch) { - result.catch(e => console.warn('app/index.tsx: onNotification error', e)); -} +onNotification(notification).catch(e => console.warn('app/index.tsx: onNotification error', e));The current pattern works, but adds indirection that isn't needed given the guaranteed
Promise<void>return type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/index.tsx` around lines 142 - 145, The duck-typing check for result?.catch is unnecessary because onNotification is always async; replace the two-step pattern with a single invocation that directly attaches the catcher: call onNotification(notification) and attach a .catch handler to log errors (use the existing log message 'app/index.tsx: onNotification error'). Remove the const result and the conditional branch around it so only the direct promise call with .catch remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/notifications/index.ts`:
- Around line 28-35: The AsyncStorage read-then-write in
app/lib/notifications/index.ts can race with other handlers (and with the dedupe
in app/sagas/deepLinking.js), so replace the raw getItem/setItem pattern with a
serialized access pattern: introduce a shared in-memory lock/processing marker
(e.g., module-level pendingNotificationId or a simple Promise queue) used by the
notification handler in app/lib/notifications/index.ts and by
deepLinkingClickCallPush flow in app/sagas/deepLinking.js so reads/writes are
sequenced; on receipt, check the in-memory marker first, then read AsyncStorage
only when not already processing, write the new id via AsyncStorage.setItem and
clear the marker after completion — this ensures compare-and-set semantics
across concurrent handlers without relying solely on AsyncStorage.
---
Nitpick comments:
In `@app/index.tsx`:
- Around line 142-145: The duck-typing check for result?.catch is unnecessary
because onNotification is always async; replace the two-step pattern with a
single invocation that directly attaches the catcher: call
onNotification(notification) and attach a .catch handler to log errors (use the
existing log message 'app/index.tsx: onNotification error'). Remove the const
result and the conditional branch around it so only the direct promise call with
.catch remains.
In `@app/lib/notifications/index.ts`:
- Around line 28-35: Extract the duplicated deduplication block into a helper
called shouldSkipVideoConfNotification(push) that reads
AsyncStorage.getItem('lastProcessedVideoConfNotificationId'), computes currentId
from push.identifier || push.payload?.notId, returns true if currentId exists
and equals lastId, otherwise stores currentId in AsyncStorage when present and
returns false; replace both original blocks (the one using
lastProcessedVideoConfNotificationId and push.identifier/push.payload?.notId at
lines shown) with a single await shouldSkipVideoConfNotification(push) check and
early return when true to keep behavior identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac0ef3a2-1cfb-42ad-9ee6-91f9c0df1083
📒 Files selected for processing (5)
app/index.tsxapp/lib/notifications/index.tsapp/lib/notifications/push.tsapp/lib/notifications/videoConf/getInitialNotification.tsapp/sagas/deepLinking.js
🚧 Files skipped from review as they are similar to previous changes (2)
- app/sagas/deepLinking.js
- app/lib/notifications/videoConf/getInitialNotification.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7046
File: app/containers/InAppNotification/NotifierComponent.stories.tsx:46-75
Timestamp: 2026-03-10T15:21:45.098Z
Learning: In `app/containers/InAppNotification/NotifierComponent.tsx` (React Native, Rocket.Chat), `NotifierComponent` is exported as a Redux-connected component via `connect(mapStateToProps)`. The `isMasterDetail` prop is automatically injected from `state.app.isMasterDetail` and does not need to be passed explicitly at call sites or in Storybook stories that use the default (connected) export.
📚 Learning: 2026-03-10T15:21:45.098Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7046
File: app/containers/InAppNotification/NotifierComponent.stories.tsx:46-75
Timestamp: 2026-03-10T15:21:45.098Z
Learning: In `app/containers/InAppNotification/NotifierComponent.tsx` (React Native, Rocket.Chat), `NotifierComponent` is exported as a Redux-connected component via `connect(mapStateToProps)`. The `isMasterDetail` prop is automatically injected from `state.app.isMasterDetail` and does not need to be passed explicitly at call sites or in Storybook stories that use the default (connected) export.
Applied to files:
app/lib/notifications/push.tsapp/lib/notifications/index.ts
🔇 Additional comments (4)
app/lib/notifications/push.ts (2)
162-164: Type signature update looks good.The updated callback type
Promise<void> | voidproperly reflects that the handler may be async, addressing the type contract issue. This maintains backwards compatibility while supporting the new async deduplication logic.
212-221: Proper async error handling for both platforms.Both iOS background and non-iOS paths now correctly handle potential Promise rejections from the async
onNotificationcallback. The same simplification mentioned forapp/index.tsxcould apply here if you prefer consistency:Optional: Simplified async handling
if (isIOS) { const { background } = reduxStore.getState().app; if (background) { - const result = onNotification(notification); - if (result?.catch) { - result.catch((e: any) => console.warn('[push.ts] Notification handler error:', e)); - } + Promise.resolve(onNotification(notification)).catch((e: any) => + console.warn('[push.ts] Notification handler error:', e) + ); } } else { - const result = onNotification(notification); - if (result?.catch) { - result.catch((e: any) => console.warn('[push.ts] Notification handler error:', e)); - } + Promise.resolve(onNotification(notification)).catch((e: any) => + console.warn('[push.ts] Notification handler error:', e) + ); }app/lib/notifications/index.ts (2)
20-20: Async signature change is appropriate.The function now correctly declares its async nature with
Promise<void>return type, which aligns with the internalawaitcalls for AsyncStorage operations and the updated type contract inpush.ts.
142-145: Consistent async error handling for Android pending notifications.The error handling pattern matches the other call sites. Same optional simplification applies here for consistency with the guaranteed
Promise<void>return type.
Fixes stale duplicate notification handling on cold boot causing users to blindly navigate into ended video conference sessions infinitely. Closes RocketChat#7015
deepak0x
force-pushed
the
fix/ios-videoconf-notification-dedup
branch
from
120ce2f to
d3fc951
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/notifications/videoConf/deduplication.ts (1)
3-14: In-memory Set grows unbounded during app lifetime.The
processingIdsSet accumulates entries without cleanup. While video conference notifications are infrequent enough that this is unlikely to cause memory issues in practice, consider clearing old entries on app state changes or limiting the Set size.♻️ Optional: Add size limit to prevent unbounded growth
const processingIds = new Set<string>(); +const MAX_PROCESSING_IDS = 100; export const isPushVideoConfAlreadyProcessed = async (notificationId?: string | null): Promise<boolean> => { if (!notificationId) { return false; // Can't dedup without an ID } // 1. Synchronously check and lock in-memory to prevent TOCTOU race condition if (processingIds.has(notificationId)) { return true; } processingIds.add(notificationId); + // Prevent unbounded growth + if (processingIds.size > MAX_PROCESSING_IDS) { + const firstId = processingIds.values().next().value; + processingIds.delete(firstId); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/notifications/videoConf/deduplication.ts` around lines 3 - 14, The in-memory Set processingIds used by isPushVideoConfAlreadyProcessed grows without bounds; change the approach so entries are removed after processing or expired: either delete the notificationId from processingIds once processing completes (in the same code paths that call isPushVideoConfAlreadyProcessed) or switch processingIds to a Map<string, number> (or a small LRU) that stores insertion timestamps and add a small periodic cleanup/TTL routine to evict entries older than X minutes, or enforce a fixed max-size and evict oldest keys when exceeded; update references to processingIds and isPushVideoConfAlreadyProcessed accordingly so the dedupe lock is released/expired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/lib/notifications/videoConf/deduplication.ts`:
- Around line 3-14: The in-memory Set processingIds used by
isPushVideoConfAlreadyProcessed grows without bounds; change the approach so
entries are removed after processing or expired: either delete the
notificationId from processingIds once processing completes (in the same code
paths that call isPushVideoConfAlreadyProcessed) or switch processingIds to a
Map<string, number> (or a small LRU) that stores insertion timestamps and add a
small periodic cleanup/TTL routine to evict entries older than X minutes, or
enforce a fixed max-size and evict oldest keys when exceeded; update references
to processingIds and isPushVideoConfAlreadyProcessed accordingly so the dedupe
lock is released/expired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bfa50122-925d-4683-95c3-18de3b5b644a
📒 Files selected for processing (6)
app/index.tsxapp/lib/notifications/index.tsapp/lib/notifications/push.tsapp/lib/notifications/videoConf/deduplication.tsapp/lib/notifications/videoConf/getInitialNotification.tsapp/sagas/deepLinking.js
🚧 Files skipped from review as they are similar to previous changes (2)
- app/sagas/deepLinking.js
- app/index.tsx
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-10T15:21:45.098Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7046
File: app/containers/InAppNotification/NotifierComponent.stories.tsx:46-75
Timestamp: 2026-03-10T15:21:45.098Z
Learning: In `app/containers/InAppNotification/NotifierComponent.tsx` (React Native, Rocket.Chat), `NotifierComponent` is exported as a Redux-connected component via `connect(mapStateToProps)`. The `isMasterDetail` prop is automatically injected from `state.app.isMasterDetail` and does not need to be passed explicitly at call sites or in Storybook stories that use the default (connected) export.
Applied to files:
app/lib/notifications/index.tsapp/lib/notifications/push.ts
🔇 Additional comments (7)
app/lib/notifications/videoConf/deduplication.ts (1)
5-31: LGTM - Well-designed deduplication logic.The three-layer approach effectively handles the cold boot deduplication:
- Early return for missing IDs prevents false negatives
- In-memory Set prevents TOCTOU races within the same process
- AsyncStorage persistence handles cross-session (cold boot) scenarios
The error handling appropriately logs warnings while falling back to the in-memory state.
app/lib/notifications/videoConf/getInitialNotification.ts (1)
70-73: LGTM - Correct deduplication integration.The dedup check is properly placed before the dispatch, uses the correct identifier from
notification.request.identifier, and returnsfalseto allow the app initialization flow to continue when a duplicate is detected. This aligns with the caller inapp/index.tsxwhich treatsfalseas "not handled, continue".app/lib/notifications/push.ts (2)
162-164: LGTM - Type signature correctly updated for async handlers.The signature now properly accepts both synchronous and asynchronous notification handlers with
Promise<void> | void, addressing the type contract concern from previous reviews.
212-221: LGTM - Promise handling correctly implemented.The
result?.catchpattern effectively handles both sync (void) and async (Promise) returns. WhenonNotificationreturns a Promise, the catch handler logs errors; when it returns void/undefined, the optional chaining safely skips the error handling.app/lib/notifications/index.ts (3)
20-20: LGTM - Async signature correctly aligns with push.ts contract.The
asyncfunction returningPromise<void>matches the updated callback type inpushNotificationConfigure, resolving the type contract issue flagged in previous reviews.
28-31: LGTM - Deduplication correctly integrated in both video conference paths.Both the explicit action (ACCEPT/DECLINE) and default tap paths:
- Extract
currentIdconsistently usingpush.identifier || push.payload?.notId- Call
isPushVideoConfAlreadyProcessedbefore dispatching- Return early when duplicate detected
This ensures all iOS video conference notification entry points are protected against stale notification re-processing.
Also applies to: 48-51
134-137: LGTM - Promise handling consistent with push.ts pattern.The
result?.catchpattern matches the approach used inpush.ts, providing consistent error handling for async notification callbacks across the codebase.
1 participant
Proposed changes
Fixes an issue where stale video conference notifications on iOS are re-dispatched upon cold boot. This happens because
expo-notificationsdoes not clear the internally cached read notification response, causing the app to repeatedly jump into an expired or handled video call indefinitely upon app restart.Fixed by implementing a simple deduplication mechanism using
AsyncStorage. We store thelastProcessedVideoConfNotificationIdand cross-reference it natively across event handlers before proceeding to the deep linking dispatcher.Issue(s)
Fixes #7015
How to test or reproduce
Screenshots
N/A
Types of changes
Checklist
Further comments
N/A
Summary by CodeRabbit