feat: Account Specific Notification Badges #1464
Conversation
|
Warning Rate limit exceeded@alexrisch has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 47 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (22)
WalkthroughThis pull request introduces comprehensive changes to the notification handling system across multiple files. The modifications focus on improving notification badge management, refining notification display logic, and ensuring account-specific notification resets. Key changes include updating notification badge tracking, enhancing background and foreground notification processing, and implementing more granular control over notification interactions for different account contexts. Changes
Sequence DiagramsequenceDiagram
participant App
participant NotificationHandler
participant AccountStore
participant BadgeManager
participant MMKVStorage
App->>NotificationHandler: Receive Notification
NotificationHandler->>AccountStore: Get Current Account
AccountStore-->>NotificationHandler: Return Current Account
NotificationHandler->>BadgeManager: Check Badge Count
BadgeManager->>MMKVStorage: Retrieve Account Badge
MMKVStorage-->>BadgeManager: Return Badge Count
BadgeManager->>NotificationHandler: Update Notification
NotificationHandler->>App: Display Notification
Possibly related PRs
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| circularLargeIcon: true, | ||
| } | ||
| : {}; | ||
| const displayedNotificaitons = await notifee.getDisplayedNotifications(); |
There was a problem hiding this comment.
The variable displayedNotificaitons contains a typo and should be renamed to displayedNotifications to maintain consistent spelling throughout the codebase.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
|
LGTM! Just noticed two type-checking errors: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
features/notifications/utils/background/groupMessageNotification.ts (1)
202-205: Error handling is sufficient.
Catching all errors here and logging them is appropriate for background handling. Consider whether rethrowing or additional fallback logic is needed if potential data corruption can occur.features/notifications/utils/resetNotifications.ts (1)
1-11: Consider renaming for clarity.
This function now only resets for a specificaccountif present, yet its name suggests a global reset. ConsiderresetAccountNotificationsor a note clarifying that no action is taken ifaccountis falsy.features/notifications/utils/background/handleBackgroundNotification.ts (1)
17-17: Informative logging statement.
Consider including additional contextual details (e.g., partialrawBody) to help debug issues quickly.features/notifications/utils/index.ts (1)
Line range hint
27-49: Consider adding stricter type checks fordata.
Here,datais cast toany, which can mask potential type mismatches or undefined properties at runtime. To improve reliability, add a well-defined type or perform additional runtime checks to ensure the fields you access (notification.data.body, etc.) exist and are of the expected type.features/notifications/utils/handleForegroundNotification.ts (2)
15-19: Unify string conversion methods.
The code usestoLowerCase()andtoLocaleLowerCase(). For consistency and predictable results, use the same method throughout. Typically,toLowerCase()suffices unless your locale requires special handling.
61-61: Handle potential nullaccountforresetNotifications.
Ifaccountfails the check above, you might passundefinedtoresetNotifications(account). Consider a fallback or an early return to skip silent resets.ios/ConverseNotificationExtension/MMKV.swift (1)
127-129: Consider centralizing storage keys.
getBadgeStorageKey(account:)is good for consistent key naming, but storing all keys in a single place, e.g., an enum or a constants file, can improve maintainability. It also simplifies refactoring if the prefix changes in the future.ios/ConverseNotificationExtension/Xmtp/Messages.swift (1)
100-103: Consider fallback or error-handling for the sender name.
While retrieving and displaying the sender’s preferred name is a good enhancement, you may want to provide a fallback ifgetPreferredNamereturns an empty or default string to ensure the notification title remains meaningful.features/notifications/utils/accountTopicSubscription.ts (1)
4-4: Validate the account format.
When comparing or using account strings, ensure consistent casing or normalizing since account identifiers might differ in uppercase/lowercase. You could do.toLowerCase()when storing or comparing to avoid subtle bugs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
App.tsx(2 hunks)features/notifications/notifications-constants.ts(1 hunks)features/notifications/utils/accountTopicSubscription.ts(2 hunks)features/notifications/utils/background/groupMessageNotification.ts(2 hunks)features/notifications/utils/background/handleBackgroundNotification.ts(1 hunks)features/notifications/utils/background/protocolNotification.ts(1 hunks)features/notifications/utils/handleForegroundNotification.ts(4 hunks)features/notifications/utils/index.ts(1 hunks)features/notifications/utils/notifications-badge.ts(1 hunks)features/notifications/utils/onInteractWithNotification.ts(1 hunks)features/notifications/utils/resetNotifications.ts(1 hunks)ios/ConverseNotificationExtension/MMKV.swift(3 hunks)ios/ConverseNotificationExtension/NotificationService.swift(1 hunks)ios/ConverseNotificationExtension/NotificationUtils.swift(1 hunks)ios/ConverseNotificationExtension/Profile.swift(3 hunks)ios/ConverseNotificationExtension/Xmtp/Messages.swift(1 hunks)utils/logout/index.tsx(2 hunks)utils/profile/getProfile.ts(1 hunks)utils/sharedData.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- features/notifications/notifications-constants.ts
🔇 Additional comments (27)
utils/sharedData.tsx (1)
6-9: Consider handling undefined config.apiURI.
Ifconfig.apiURIis undefined or empty, setting it as"api-uri"inmmkvmight cause problems downstream. Consider validatingconfig.apiURIor adding fallback logic.App.tsx (2)
46-46: Good import practice.
ImportingsaveApiURIhere clearly indicates its usage within this module, ensuring the function remains discoverable and explicit in intent.
62-63: Confirm initialization timing.
saveApiURI()is invoked right afterinitSentry(), which is likely safe. However, ifconfigor mmkv initialization might fail under certain conditions—such as incomplete environment data—consider error handling or logging to detect potential issues.features/notifications/utils/background/groupMessageNotification.ts (7)
26-26: Logger import looks good.
No concerns here. Great addition for better error tracking.
36-46: Effective fallback logic for syncing conversations.
This ensures you properly handle an unsynced conversation before throwing an error. Nicely done.
50-62: Spam score check is well-structured.
Early return safeguards from unnecessary notification spam. Logical approach to reduce overhead.
64-74: Efficient profile lookup.
The combined use ofgetProfileplus local conversation members ensures accurate sender info.
76-80: Notification content retrieval is concise.
Clean approach to skip if content is null or undefined.
82-113: Group notification block looks consistent.
Using a messaging style and re-constructing messages for grouped notifications is great for user clarity.
144-200: Direct/1-1 notification flow.
Your approach to show separate messaging style for individual notifications is well-structured.features/notifications/utils/notifications-badge.ts (1)
15-21: Potential mismatch betweenincrementBadgeCountand the explicit badge update.
notifee.incrementBadgeCount()increments the existing system-level count by 1, but then you storebadgein MMKV. Ifbadgeis not exactly currentCount + 1, the two may diverge. Verify that the intended behavior is simply to increment by one each time.utils/profile/getProfile.ts (3)
35-36: Returning the first array entry is valid.
This handles multiple stored entries properly. If empty, it yieldsundefined, which aligns with the declared return signature.
41-42: Directly returning fetched profile is concise.
This approach avoids confusion by not returning a Promise inside a Promise.
49-49: Consistent usage of index 0.
Same approach as above, ensuring a single object is returned. If you intend to handle multiple profiles differently, consider an explicit fallback. Otherwise, this is fine.features/notifications/utils/onInteractWithNotification.ts (1)
65-65: Validate thataccountis properly resolved.
Ensure thataccount(derived from eithernotificationData["account"]or the store) is actually defined when callingresetNotifications(account). Otherwise, a null or undefined value might cause unexpected behavior.features/notifications/utils/handleForegroundNotification.ts (2)
52-52: Confirmaccountpresence before showing the alert.
Whenaccountis returned fromnotification.request.content.data, verify that the property actually exists; otherwise, your check (account && account !== currentAccount()) might fail quietly ifaccountisundefined.
73-77: Log relevant environment details.
This additional logging for the foregrounded notification onPlatform.OSis helpful. If you need further debugging, consider also logging the app version or additional context, but keep it concise to avoid clutter.ios/ConverseNotificationExtension/Profile.swift (1)
Line range hint
48-71: Avoid ignoring response errors inrefreshProfileFromBackendandrefreshInboxProfileFromBackend.
While the logic re-throws in some cases, if the response is empty (response.datais null) or if decoding fails, consider logging or handling these errors more explicitly to help diagnose issues when network or backend problems occur.ios/ConverseNotificationExtension/NotificationUtils.swift (1)
14-17: Ensure concurrency safety for badge increments.This code correctly handles per-account and total badge updates, but consider concurrency if multiple notifications for the same account may be processed in parallel by the extension. In such a case, performing a simple read–increment–write sequence might result in lost increments unless protected by a concurrency mechanism.
ios/ConverseNotificationExtension/MMKV.swift (3)
68-69: Check for potential nil or partial data in decoded Profile.The code decodes a
Profileobject and then immediately returnsdecoded.socials. Verify thatsocialsis mandatory in theProfilestructure. Ifsocialscan be missing or optional in the stored JSON, consider graceful handling or default values.
131-135: Validate possible double/Int mismatch.
mmkv?.double(forKey: key)is converted to anInt. If any unexpected floating-point value (e.g., partial) is stored, it might lead to a truncated integer. Consider storing and retrieving an integer type directly to avoid confusion.
146-153: Guard for large account arrays.
getAllBadgeCountsloops through all returned accounts, possibly leading to performance impact if many accounts exist. Consider caching or limiting the account list size if needed. Also confirm concurrency safety if invoked in parallel.utils/logout/index.tsx (2)
26-26: Good modular design forresetNotifications.Importing
resetNotificationshere streamlines the logout process. It helps ensure that relevant notifications and badge counts for the user are reset consistently upon logout.
110-110: Verify sync between resetting notifications and unsubscribing.Calling
resetNotifications(account)is appropriate here. Nevertheless, confirm that unsubscribing from notifications or clearing secure storage doesn't undo the reset notifications or lead to inconsistent states. If needed, handle any concurrency or ordering issues between these calls.ios/ConverseNotificationExtension/NotificationService.swift (1)
67-67: Badge increment logic is aligned with per-account flows.Passing the
accountintoincrementBadgeensures that only the relevant badge is incremented, which is consistent with the new per-account design for badge counts. Confirm thathandleNotificationAsynconly runs once per notification to prevent multiple increments on the same message.features/notifications/utils/accountTopicSubscription.ts (1)
31-33: Ensure logic works for multiple concurrent accounts.
Checkingif (account === currentAccount())before resetting notifications is a great approach to avoid unnecessary resets. However, consider any edge cases ifcurrentAccountmight change between subscription and execution. If that’s unlikely or well-handled, this logic should suffice.features/notifications/utils/background/protocolNotification.ts (1)
26-52: Good use of try/catch blocks for improved error handling.
Your newly introduced try/catch effectively logs errors without disrupting the entire process. Consider adding more granular logging for each step (e.g., an info log whengetXmtpClientis first invoked) to aid debugging.
| (n) => n.notification.android?.groupId === conversation?.topic | ||
| ); |
There was a problem hiding this comment.
Fix type-check error by ensuring conversation is recognized as non-null.
Because you throw an error if conversation is missing at line 45, it’s guaranteed non-null here. TypeScript may still complain, so add a non-null assertion:
- displayedNotifications.find(
- (n) => n.notification.android?.groupId === conversation?.topic
- );
+ displayedNotifications.find(
+ (n) => n.notification.android?.groupId === conversation!.topic
+ );📝 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.
| (n) => n.notification.android?.groupId === conversation?.topic | |
| ); | |
| (n) => n.notification.android?.groupId === conversation!.topic | |
| ); |
| displayedNotifications.find( | ||
| (n) => n.notification.android?.groupId === conversation?.topic |
There was a problem hiding this comment.
Replicate the same fix for type-check error here.
As with line 102, TypeScript requires a non-null assertion or additional check:
- displayedNotifications.find(
- (n) => n.notification.android?.groupId === conversation?.topic
- );
+ displayedNotifications.find(
+ (n) => n.notification.android?.groupId === conversation!.topic
+ );Committable suggestion skipped: line range outside the PR's diff.
| @@ -17,7 +17,7 @@ func getProfile(account: String, address: String) async -> ProfileSocials? { | |||
|
|
|||
| // If profile is nil, let's refresh it | |||
| try? await refreshProfileFromBackend(account: account, address: formattedAddress) | |||
|
|
|||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle errors surfaced by try? await refreshProfileFromBackend(...).
try? discards errors, which can mask network or parsing problems. For better observability, consider catching or logging these errors explicitly.
| func setBadgeByAccount(_ badge: Int, account: String) { | ||
| let mmkv = getMmkv() | ||
| mmkv?.set(Int32(badge), forKey: "notifications-badge") | ||
| let key = getBadgeStorageKey(account: account) | ||
| mmkv?.set(Double(badge), forKey: key) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent data type usage for badges.
setBadgeByAccount sets the badge as a Double, while the code interprets the value as an Int in getBadgeByAccount. For clarity, store it as an integer to ensure straightforward usage and avoid type-casting pitfalls.
- func setBadgeByAccount(_ badge: Int, account: String) {
- mmkv?.set(Double(badge), forKey: key)
+ func setBadgeByAccount(_ badge: Int, account: String) {
+ mmkv?.set(badge, forKey: key)Committable suggestion skipped: line range outside the PR's diff.
| let decoded = try decoder.decode([ProfileSocials].self, from: profilesString!.data(using: .utf8)!) | ||
| return decoded[0] |
There was a problem hiding this comment.
Handle empty arrays to avoid index out-of-bounds.
When decoding [ProfileSocials], you return decoded[0] directly. If the array is empty, this will cause a crash. Consider adding a safety check before returning decoded[0].
let decoded = try decoder.decode([ProfileSocials].self, from: profilesString!.data(using: .utf8)!)
- return decoded[0]
+ guard !decoded.isEmpty else {
+ return nil
+ }
+ return decoded[0]Committable suggestion skipped: line range outside the PR's diff.
features/notifications/utils/background/groupMessageNotification.ts
Outdated
Show resolved
Hide resolved
features/notifications/utils/background/groupMessageNotification.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
features/notifications/utils/background/protocolNotification.ts (1)
24-41: Enhance error handling with more specific error types.The error handling has been improved with try-catch and detailed logging. However, consider enhancing the error handling further by:
- Using more specific error types for better error tracking
- Adding structured logging with error codes
try { logger.debug( `[ProtocolNotification] Received a notification for account ${notification.account}` ); const accounts = getAccountsList(); if ( !accounts.find( (a) => a.toLowerCase() === notification.account.toLowerCase() ) ) { - logger.error( + const error = new Error( + `Account ${notification.account} not found` + ); + logger.error( + '[ProtocolNotification] Account not found', + { + error, + errorCode: 'ACCOUNT_NOT_FOUND', + account: notification.account + } + ); - `[ProtocolNotification] Account ${notification.account} not found, skipping` - ); return; }features/notifications/utils/background/groupMessageNotification.ts (2)
27-53: Consider handling edge cases in getSenderProfileInfo.The helper function looks good but could be improved to handle edge cases better:
- The empty string return for missing senderAddress could be more informative
- Consider caching profile info to improve performance
if (!senderAddress) return { - senderName: "", + senderName: "Unknown Sender", + senderImage: undefined, };
268-271: Enhance error logging with structured data.The error logging could be improved with structured data and error categorization.
- logger.error( - `[GroupMessageNotification] Error handling group message notification: ${e}` - ); + logger.error('[GroupMessageNotification] Message notification error', { + error: e, + errorCode: 'MESSAGE_NOTIFICATION_ERROR', + contentTopic: notification.contentTopic, + account: xmtpClient.address + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/ConversationFlashList.tsx(2 hunks)components/PinnedConversations/PinnedV3Conversation.tsx(2 hunks)features/notifications/utils/background/groupMessageNotification.ts(1 hunks)features/notifications/utils/background/protocolNotification.ts(2 hunks)utils/groupUtils/conversationContainerHelpers.ts(0 hunks)
💤 Files with no reviewable changes (1)
- utils/groupUtils/conversationContainerHelpers.ts
🔇 Additional comments (9)
components/PinnedConversations/PinnedV3Conversation.tsx (3)
6-6: Import statement looks good.Since
isConversationGroupis used consistently, this import ensures a more concise way to branch between group vs direct message conversations.
17-18: Clear conditional to handle group conversations.This branch logically returns the
PinnedV3GroupConversationcomponent when the conversation is identified as a group.
20-20: Adequate fallback for non-group conversations.Returning
PinnedV3DMConversationis straightforward and aligns with the conversation type check, ensuring a clean separation of group vs. DM rendering logic.components/ConversationFlashList.tsx (3)
16-16: Consistent usage ofisConversationGroup.Replacing custom or version-based checks with
isConversationGroupimproves readability and consistency across components.
62-63: Appropriate branching for conversation type.This logic correctly renders a
V3GroupConversationListItemif the conversation is a group. The inline conditions remain cohesive and clear.
65-65: Fallback handling ensures DM item rendering.Retaining a separate branch for DM items maintains clarity, aligning with the updated approach of using the
isConversationGroupfunction without extra overhead.features/notifications/utils/background/protocolNotification.ts (2)
6-6: LGTM! Clean import organization.The imports are well-organized, with the new
handleV3MessageNotificationandisV3Topicimports properly added to support the enhanced notification routing.Also applies to: 11-11
45-49: Verify notification routing logic.The notification routing logic looks correct, but let's verify that all possible content topics are handled appropriately.
✅ Verification successful
Let me try a different approach to verify the notification routing logic.
Notification routing logic is correctly implemented
Based on the codebase analysis, the notification routing logic is properly handling all content topic types:
- V3 topics (
isV3Topic) are handled byhandleV3MessageNotification- Group welcome topics (
isGroupWelcomeContentTopic) are handled byhandleGroupWelcomeNotificationThe implementation is consistent with the topic type checks found in the codebase:
isV3Topicchecks for MLS group message topicsisGroupWelcomeContentTopicchecks for topics starting with "/xmtp/mls/1/w-"No other content topic types are defined in the codebase that would require handling in this notification router.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any unhandled content topics in the codebase # Search for content topic definitions and usages echo "Searching for content topic definitions and handlers..." rg --type typescript "contentTopic|isV3Topic|isGroupWelcomeContentTopic" -A 2 # Look for potential switch/if-else chains handling content topics echo "Checking for content topic handling patterns..." ast-grep --pattern 'if ($topic) { $$$ } else if ($topic) { $$$ }'Length of output: 408
Script:
#!/bin/bash # Search for content topic related code with ripgrep echo "Searching for content topic definitions and handlers..." rg "contentTopic|isV3Topic|isGroupWelcomeContentTopic" # Search for topic type checks and handlers echo "Searching for topic type checks..." rg "is.*Topic|handle.*Notification"Length of output: 15502
features/notifications/utils/background/groupMessageNotification.ts (1)
55-66: LGTM! Well-structured type definitions.The type definitions are clear and properly leverage TypeScript's type system with good separation between base and specific notification types.
| const displayedNotifications = await notifee.getDisplayedNotifications(); | ||
| const previousGroupIdNotification = | ||
| conversation?.topic && | ||
| displayedNotifications.find( | ||
| (n) => n.notification.android?.groupId === conversation?.topic | ||
| ); | ||
| const previousMessages = | ||
| previousGroupIdNotification?.notification.android?.style?.type === | ||
| AndroidStyle.MESSAGING | ||
| ? previousGroupIdNotification.notification.android?.style?.messages | ||
| : []; | ||
| if (previousGroupIdNotification?.notification.id) { | ||
| notifee.cancelDisplayedNotification( | ||
| previousGroupIdNotification.notification.id | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate code detected in notification handling.
The notification cancellation logic is duplicated between group and DM handlers. Consider extracting it to a shared function.
+ const cancelPreviousNotification = async (topic: string) => {
+ const displayedNotifications = await notifee.getDisplayedNotifications();
+ const previousGroupIdNotification =
+ displayedNotifications.find(
+ (n) => n.notification.android?.groupId === topic
+ );
+ if (previousGroupIdNotification?.notification.id) {
+ await notifee.cancelDisplayedNotification(
+ previousGroupIdNotification.notification.id
+ );
+ }
+ return previousGroupIdNotification;
+ };Committable suggestion skipped: line range outside the PR's diff.
| const displayedNotifications = await notifee.getDisplayedNotifications(); | ||
| const previousGroupIdNotification = | ||
| conversation.topic && | ||
| displayedNotifications.find( | ||
| (n) => n.notification.android?.groupId === conversation.topic | ||
| ); | ||
| const previousMessages = | ||
| previousGroupIdNotification?.notification.android?.style?.type === | ||
| AndroidStyle.MESSAGING | ||
| ? previousGroupIdNotification.notification.android?.style?.messages | ||
| : []; | ||
| if (previousGroupIdNotification?.notification.id) { | ||
| notifee.cancelDisplayedNotification( | ||
| previousGroupIdNotification.notification.id | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix potential race condition in notification handling.
There's a potential race condition between finding the previous notification and canceling it. Consider using a transaction-like approach.
- const displayedNotifications = await notifee.getDisplayedNotifications();
- const previousGroupIdNotification =
- conversation.topic &&
- displayedNotifications.find(
- (n) => n.notification.android?.groupId === conversation.topic
- );
- if (previousGroupIdNotification?.notification.id) {
- notifee.cancelDisplayedNotification(
- previousGroupIdNotification.notification.id
- );
- }
+ // Atomic operation to get and cancel notification
+ const previousNotificationId = await (async () => {
+ const displayedNotifications = await notifee.getDisplayedNotifications();
+ const previousGroupIdNotification =
+ conversation.topic &&
+ displayedNotifications.find(
+ (n) => n.notification.android?.groupId === conversation.topic
+ );
+ if (previousGroupIdNotification?.notification.id) {
+ await notifee.cancelDisplayedNotification(
+ previousGroupIdNotification.notification.id
+ );
+ }
+ return previousGroupIdNotification;
+ })();Committable suggestion skipped: line range outside the PR's diff.
| const spamScore = await computeSpamScoreGroupMessage( | ||
| xmtpClient, | ||
| conversation as GroupWithCodecsType, | ||
| message | ||
| ); | ||
| if (spamScore >= 0) return; |
There was a problem hiding this comment.
💡 Codebase verification
Type guard should be used before type casting
The code already imports isConversationGroup type guard but doesn't use it before the type casting. This could lead to runtime errors if the conversation is not actually a group conversation. You should modify the code to use the type guard before the type casting:
if (!isConversationGroup(conversation)) return;
const spamScore = await computeSpamScoreGroupMessage(
xmtpClient,
conversation, // type casting not needed here as TypeScript knows it's GroupWithCodecsType
message
);features/notifications/utils/background/groupMessageNotification.ts: Add type guard check before computing spam score
🔗 Analysis chain
Verify spam score computation type casting.
The type casting of conversation to GroupWithCodecsType might be unsafe. Consider using the isConversationGroup type guard here as well.
Let me try a different search pattern to gather more information.
Let me check the implementation of isConversationGroup type guard.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar type casts in the codebase
# Look for type casts to GroupWithCodecsType
echo "Searching for GroupWithCodecsType casts..."
rg --type typescript "as GroupWithCodecsType"
# Look for usage of isConversationGroup
echo "Checking isConversationGroup usage patterns..."
ast-grep --pattern 'if (isConversationGroup($conv)) { $$$ }'
Length of output: 343
Script:
#!/bin/bash
# Search for GroupWithCodecsType usage and type guards
echo "Searching for GroupWithCodecsType..."
rg "GroupWithCodecsType"
echo -e "\nSearching for isConversationGroup..."
rg "isConversationGroup"
echo -e "\nSearching for type guard implementations..."
ast-grep --pattern 'function isConversationGroup($param): $param is $type { $$$ }'
Length of output: 7201
Script:
#!/bin/bash
# Get the implementation of isConversationGroup
echo "Getting isConversationGroup implementation..."
rg -A 5 "export function isConversationGroup"
# Check the specific file for computeSpamScoreGroupMessage usage
echo -e "\nChecking spam score computation context..."
rg -A 5 "computeSpamScoreGroupMessage" "features/notifications/utils/background/groupMessageNotification.ts"
Length of output: 1281
alexrisch
force-pushed
the
ar/notification-badge-account
branch
from
df04d3c to
d4cbb2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ios/ConverseNotificationExtension/MMKV.swift (1)
84-85:⚠️ Potential issueHandle empty arrays to avoid index out-of-bounds.
The array access at index 0 without bounds checking could cause a crash if the array is empty.
🧹 Nitpick comments (6)
features/notifications/utils/resetNotifications.ts (1)
1-11: LGTM! Account-specific notification reset implementation looks good.The changes properly implement account-specific notification badge clearing with good use of early return pattern.
Consider adding error handling for invalid account parameter.
While the early return handles undefined/empty accounts, consider adding validation for malformed account strings.
export const resetNotifications = async ( account: string, timeout: number = 0 ): Promise<void> => { setTimeout(async () => { - if (!account) { + if (!account || !/^[a-zA-Z0-9_-]+$/.test(account)) { return; } clearNotificationBadgeForAccount(account); }, timeout); };ios/ConverseNotificationExtension/NotificationUtils.swift (1)
14-18: LGTM! Account-specific badge increment implementation looks good.The implementation correctly handles account-specific badge counting and updates the total badge count across all accounts.
Consider adding overflow protection.
The badge increment operation should handle potential integer overflow scenarios.
func incrementBadge(for content: UNMutableNotificationContent, account: String) { let newAccountBadgeCount = getBadgeByAccount(account: account) + 1 + guard newAccountBadgeCount > 0 else { + // Handle overflow by capping at max value + setBadgeByAccount(Int.max, account: account) + return + } setBadgeByAccount(newAccountBadgeCount, account: account) let newBadgeCount = getAllBadgeCounts() content.badge = NSNumber(value: newBadgeCount) }ios/ConverseNotificationExtension/MMKV.swift (1)
146-153: LGTM! Total badge count calculation looks good.The implementation correctly aggregates badge counts across all accounts.
Consider caching the total badge count.
For better performance, consider caching the total badge count and updating it incrementally instead of recalculating it every time.
+private var totalBadgeCount: Int = 0 + func setBadgeByAccount(_ badge: Int, account: String) { let mmkv = getMmkv() + let oldBadge = getBadgeByAccount(account: account) let key = getBadgeStorageKey(account: account) mmkv?.set(Double(badge), forKey: key) + totalBadgeCount += (badge - oldBadge) } func getAllBadgeCounts() -> Int { + if totalBadgeCount == 0 { var count = 0 let accounts = getAccounts() for account in accounts { let accountCount = getBadgeByAccount(account: account) count += accountCount } + totalBadgeCount = count + } + return totalBadgeCount }features/notifications/utils/background/groupMessageNotification.ts (3)
27-53: Consider handling edge cases in getSenderProfileInfo.The function returns an empty sender name when senderAddress is not found, but senderImage is undefined in this case. Consider returning both properties explicitly for consistency.
if (!senderAddress) return { senderName: "", + senderImage: undefined, };
213-222: Improve error handling for conversation sync.The code attempts to sync and find the conversation again if not found initially. Consider adding a retry limit to prevent infinite loops in case of persistent issues.
+ let retryCount = 0; + const MAX_RETRIES = 3; let conversation = await xmtpClient.conversations.findConversationByTopic( notification.contentTopic as ConversationTopic ); - if (!conversation) { + while (!conversation && retryCount < MAX_RETRIES) { + retryCount++; await xmtpClient.conversations.sync(); conversation = await xmtpClient.conversations.findConversationByTopic( notification.contentTopic as ConversationTopic ); - if (!conversation) throw new Error("Conversation not found"); } + if (!conversation) throw new Error("Conversation not found after retries");
269-271: Enhance error logging.The error message could be more descriptive by including the notification content topic.
logger.error( - `[GroupMessageNotification] Error handling group message notification: ${e}` + `[GroupMessageNotification] Error handling message notification for topic ${notification.contentTopic}: ${e}` );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
App.tsx(2 hunks)components/ConversationFlashList/ConversationFlashList.tsx(2 hunks)components/PinnedConversations/PinnedV3Conversation.tsx(2 hunks)features/notifications/notifications-constants.ts(1 hunks)features/notifications/utils/accountTopicSubscription.ts(2 hunks)features/notifications/utils/background/groupMessageNotification.ts(1 hunks)features/notifications/utils/background/handleBackgroundNotification.ts(1 hunks)features/notifications/utils/background/protocolNotification.ts(2 hunks)features/notifications/utils/handleForegroundNotification.ts(4 hunks)features/notifications/utils/index.ts(1 hunks)features/notifications/utils/notifications-badge.ts(1 hunks)features/notifications/utils/onInteractWithNotification.ts(1 hunks)features/notifications/utils/resetNotifications.ts(1 hunks)ios/ConverseNotificationExtension/MMKV.swift(3 hunks)ios/ConverseNotificationExtension/NotificationService.swift(1 hunks)ios/ConverseNotificationExtension/NotificationUtils.swift(1 hunks)ios/ConverseNotificationExtension/Profile.swift(3 hunks)ios/ConverseNotificationExtension/Xmtp/Messages.swift(1 hunks)utils/groupUtils/conversationContainerHelpers.ts(0 hunks)utils/logout/index.tsx(2 hunks)utils/profile/getProfile.ts(1 hunks)utils/sharedData.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- utils/groupUtils/conversationContainerHelpers.ts
✅ Files skipped from review due to trivial changes (1)
- features/notifications/notifications-constants.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- features/notifications/utils/background/handleBackgroundNotification.ts
- utils/sharedData.tsx
- ios/ConverseNotificationExtension/Xmtp/Messages.swift
- ios/ConverseNotificationExtension/Profile.swift
- features/notifications/utils/onInteractWithNotification.ts
- features/notifications/utils/accountTopicSubscription.ts
- features/notifications/utils/handleForegroundNotification.ts
- features/notifications/utils/index.ts
- components/PinnedConversations/PinnedV3Conversation.tsx
- ios/ConverseNotificationExtension/NotificationService.swift
- features/notifications/utils/notifications-badge.ts
- utils/profile/getProfile.ts
- App.tsx
- utils/logout/index.tsx
🔇 Additional comments (10)
components/ConversationFlashList/ConversationFlashList.tsx (3)
24-24: Great modularization of the type check.Importing
isConversationGroupfrom a dedicated utility module keeps the code organized and improves readability.
71-72: Clean conditional logic for conversation types.Using
isConversationGroup(item)here simplifies the item type checks, making the code more intuitive than unwrapping conversation objects. This approach increases maintainability.
74-74: Consistent rendering of direct message conversations.The fallback to
<V3DMListItem>is clear and continues the simplified approach to item rendering.ios/ConverseNotificationExtension/MMKV.swift (2)
68-69: LGTM! Profile decoding change looks good.The change to decode
Profileand return itssocialsproperty is a clean implementation.
127-144: LGTM! Badge management implementation looks clean.Good separation of concerns with dedicated functions for key generation and badge management.
Inconsistent data type usage for badges.
The code stores badges as Double but uses them as Int, which could lead to type-casting issues.
features/notifications/utils/background/protocolNotification.ts (2)
24-41: LGTM! Enhanced error handling and logging.The addition of try-catch block and detailed logging improves the robustness and debuggability of the notification handling process.
42-49: Verify error handling in getXmtpClient.The code assumes getXmtpClient will always return a valid client. Consider adding explicit error handling for the case where getXmtpClient fails or returns null.
features/notifications/utils/background/groupMessageNotification.ts (3)
91-106: Previous review comment about race condition is still valid.There's a potential race condition between finding the previous notification and canceling it.
162-177: Previous review comment about duplicate code is still valid.The notification cancellation logic is duplicated between group and DM handlers.
231-236: Previous review comment about type guard usage is still valid.The code should use isConversationGroup before type casting to GroupWithCodecsType.
Added handling for account specific badging for iOS Cleaned up styles for Android messages Fixed profile caching Fixe profile caching in iOS Added back sharedData setting
Removed old functions to use better typesafe
alexrisch
force-pushed
the
ar/notification-badge-account
branch
from
d4cbb2c to
0946ba4
Compare
Added handling for account specific badging for iOS
Cleaned up styles for Android messages
Fixed profile caching
Fixe profile caching in iOS
Added back sharedData setting
New Android Push Messages View
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation