fix: Splash Screen not dismissing #1525
Conversation
Cleans up 'async' usage in hydration manager Adds more logs Removes account from profile related queries
WalkthroughThis pull request involves a comprehensive refactoring of profile-related functions across multiple files, primarily focusing on removing the Changes
Suggested reviewers
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (2)
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 (
|
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (3)
hooks/useProfilesSocials.ts (1)
Line range hint
1-11: Remove unused import.The
useCurrentAccountimport is no longer used after removing the account dependency.Apply this diff:
-import { useCurrentAccount } from "@data/store/accountsStore"; import { useProfileSocialsQueries } from "@queries/useProfileSocialsQuery";ios/ConverseNotificationExtension/Profile.swift (1)
Line range hint
44-100: Add request timeout configuration.The Alamofire requests lack timeout configuration, which could lead to indefinite waits in poor network conditions. Consider adding appropriate timeouts.
AF.request(profileURI, method: .get, parameters: ["address": address]) + .timeout(interval: 30) // Add 30-second timeout .validate() .responseData { response inios/ConverseNotificationExtension/MMKV.swift (1)
Line range hint
59-98: Add cleanup for old storage format.While removing
accountfrom storage keys simplifies the implementation, old keys should be cleaned up to prevent storage bloat.func getProfilesStore(address: String) -> ProfileSocials? { let mmkv = getMmkv() let key = "profileSocials-\(address.lowercased())" + // Clean up old format keys + if let oldData = mmkv?.string(forKey: "profileSocials-\(getCurrentAccount() ?? "")-\(address.lowercased())") { + mmkv?.removeValue(forKey: "profileSocials-\(getCurrentAccount() ?? "")-\(address.lowercased())") + mmkv?.set(oldData, forKey: key) + } // ... rest of the implementation }
🧹 Nitpick comments (5)
ios/ConverseNotificationExtension/Xmtp/Messages.swift (1)
Line range hint
81-95: Consider adding fallback display names for failed profile retrievals.The notification content relies on successful profile retrieval, but there's no explicit error handling. Consider adding fallback display names (e.g., shortened wallet address) when profile retrieval fails.
Example improvement:
if let senderProfileSocials = await getInboxIdProfile(inboxId: decodedMessage.senderInboxId) { bestAttemptContent.subtitle = getPreferredName(address: decodedMessage.senderInboxId, socials: senderProfileSocials) +} else { + bestAttemptContent.subtitle = formatShortAddress(decodedMessage.senderInboxId) }utils/profile/getProfile.ts (1)
Line range hint
19-24: Optimize profile data fetching for splash screen performance.The multiple storage checks and data fetching paths could impact splash screen dismissal time. Consider:
- Implementing parallel fetching for both storage keys
- Adding error boundaries to prevent fetch failures from blocking the splash screen
Would you like me to propose an optimized implementation that addresses these performance concerns?
components/StateHandlers/HydrationStateHandler.tsx (1)
51-53: Update log message to reflect actual state.The debug message suggests all conversation lists are prefetched, but since the fetches aren't awaited, this might not be true when the log is printed.
- logger.debug( - "[Hydration] Done prefetching all accounts conversation lists" - ); + logger.debug( + "[Hydration] Initiated prefetching of all accounts conversation lists" + );ios/ConverseNotificationExtension/Profile.swift (1)
11-27: Enhance error handling in profile fetching.While removing the
accountparameter simplifies the API, the error handling could be more robust. Consider adding proper error types and handling network-specific errors.+enum ProfileError: Error { + case networkError(Error) + case decodingError(Error) + case invalidAddress +} -func getProfile(address: String) async -> ProfileSocials? { +func getProfile(address: String) async throws -> ProfileSocials { guard !address.isEmpty else { - return nil + throw ProfileError.invalidAddress } // ... rest of the implementation }queries/useProfileSocialsQuery.ts (1)
Line range hint
24-34: Optimize batching configuration for profile fetches.The current 10ms window might be too short for larger batches, potentially causing unnecessary API calls. Consider adjusting the window based on typical usage patterns.
scheduler: windowedFiniteBatchScheduler({ - windowMs: 10, + windowMs: 50, // Increase window to allow more batching maxBatchSize: 150, + // Add rate limiting + maxRequestsPerSecond: 50, }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
components/AccountSettingsButton.tsx(1 hunks)components/StateHandlers/HydrationStateHandler.tsx(2 hunks)components/StateHandlers/MainIdentityStateHandler.tsx(1 hunks)features/groups/invite-to-group/invite-users-to-exisiting-group.screen.tsx(2 hunks)hooks/useProfileSocials.ts(1 hunks)hooks/useProfilesSocials.ts(1 hunks)ios/ConverseNotificationExtension/MMKV.swift(3 hunks)ios/ConverseNotificationExtension/Profile.swift(3 hunks)ios/ConverseNotificationExtension/Xmtp/Messages.swift(2 hunks)queries/useProfileSocialsQuery.ts(8 hunks)screens/NewConversation/NewConversation.tsx(2 hunks)screens/Onboarding/OnboardingUserProfileScreen.tsx(1 hunks)utils/profile/getProfile.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
components/StateHandlers/HydrationStateHandler.tsx (1)
Learnt from: technoplato
PR: ephemeraHQ/converse-app#1325
File: components/StateHandlers/HydrationStateHandler.tsx:18-20
Timestamp: 2024-12-09T14:51:51.177Z
Learning: In `components/StateHandlers/HydrationStateHandler.tsx`, the calls to `getInstalledWallets(false);` are in separate branches of an `if-else` statement and are required for both cases when `accounts.length === 0` and when `accounts.length > 0`. I should carefully examine conditional structures before suggesting code is redundant.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (10)
ios/ConverseNotificationExtension/Xmtp/Messages.swift (2)
92-92: LGTM! Consistent with the group message changes.The simplified profile retrieval is consistently applied in both group and DM contexts.
81-81: LGTM! Verify profile retrieval still works as expected.The simplified function signature aligns with the PR's cleanup objectives. The change makes sense as sender profiles should be retrievable using just their inboxId.
Let's verify that all calls to
getInboxIdProfilehave been updated consistently:✅ Verification successful
✅ Verified: Profile retrieval refactoring is complete and consistent
All instances of
getInboxIdProfilehave been updated to use the new signature without theaccountparameter:
- Function definition in
Profile.swift- Both calls in
Messages.swift🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining calls to getInboxIdProfile with the account parameter # Expect: No matches with the old signature containing 'account:' rg -g '*.swift' 'getInboxIdProfile.*account:'Length of output: 47
Script:
#!/bin/bash # First, find the function definition echo "=== Function Definition ===" ast-grep --pattern 'func getInboxIdProfile' # Then find all calls to this function echo -e "\n=== Function Calls ===" rg -g '*.swift' 'getInboxIdProfile\s*\(' -A 1Length of output: 1136
hooks/useProfileSocials.ts (1)
Line range hint
1-6: LGTM! Verify query invalidation.The simplification of
useProfileSocialshook by removing the account dependency is a good change that aligns with the PR objectives.Let's verify that all query invalidations have been updated accordingly:
✅ Verification successful
✅ Query invalidation verification completed
The query implementation and all its invalidations have been successfully updated to use the simplified single-parameter pattern. All active usages are consistent with the new implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining uses of useProfileSocialsQuery with multiple parameters rg "useProfileSocialsQuery\([^)]*,[^)]*\)"Length of output: 44
Script:
#!/bin/bash # Find implementation of useProfileSocialsQuery ast-grep --pattern 'export const useProfileSocialsQuery = $_' # Search for any query invalidation patterns related to profile socials rg -i "invalidate.*profile.*social" -A 2Length of output: 3725
components/StateHandlers/MainIdentityStateHandler.tsx (1)
17-19: Verify invalidation timing impact on splash screen.The profile invalidation timing in
useEffectcould affect the splash screen dismissal. Please ensure this doesn't cause unnecessary re-renders or delay the splash screen dismissal.Let's check for other similar invalidation patterns:
components/StateHandlers/HydrationStateHandler.tsx (1)
16-21: 🛠️ Refactor suggestionConsider propagating wallet installation errors.
While error logging is good, silently catching errors during wallet installation checks could lead to the splash screen getting stuck if the operation fails. Consider propagating the error or implementing a retry mechanism.
try { await getInstalledWallets(false); } catch (e) { logger.error("[Hydration] Error getting installed wallets", e); + // Implement retry logic or error propagation + throw new Error("Failed to check installed wallets"); }queries/useProfileSocialsQuery.ts (1)
15-22: Add migration strategy for cached profile data.The removal of
accountfrom query keys will invalidate existing cached data. Consider adding a migration strategy to prevent unnecessary refetches.Consider implementing a version-based cache key system:
const profileSocialsQueryKey = (peerAddress: string): QueryKey => [ "profileSocials", "v2", // Add version to support future migrations peerAddress?.toLowerCase(), ];Also applies to: 132-136
components/AccountSettingsButton.tsx (1)
63-63: LGTM! Simplified function call by removing redundant argument.The change correctly removes the redundant second argument to
invalidateProfileSocialsQuerywhile maintaining the same functionality.screens/Onboarding/OnboardingUserProfileScreen.tsx (1)
390-390: LGTM! Simplified function call by removing redundant argument.The change correctly removes the redundant second argument to
invalidateProfileSocialsQuerywhile maintaining the same functionality.features/groups/invite-to-group/invite-users-to-exisiting-group.screen.tsx (1)
199-199: LGTM! Simplified function calls by removing redundant arguments.The changes correctly remove the redundant first argument (
currentAccount()) tosetProfileRecordSocialsQueryDatawhile maintaining the same functionality.Also applies to: 236-236
screens/NewConversation/NewConversation.tsx (1)
277-277: LGTM! Simplified function calls by removing redundant arguments.The changes correctly remove the redundant first argument (
currentAccount()) tosetProfileRecordSocialsQueryDatawhile maintaining the same functionality.Also applies to: 338-338
| currentAccount, | ||
| address | ||
| ); | ||
| const addressStorageKey = profileSocialsQueryStorageKey(address); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing unused currentAccount parameter from storage key generation.
The storage key no longer uses the currentAccount parameter, which is consistent with other changes. However, this creates an inconsistency with the function signature that still requires it.
Consider updating the function signature to match the new pattern:
-export const getProfile = async (
- currentAccount: string,
- inboxId: InboxId,
- address: string | undefined
-): Promise<IProfileSocials | undefined | null>
+export const getProfile = async (
+ inboxId: InboxId,
+ address: string | undefined
+): Promise<IProfileSocials | undefined | null>Committable suggestion skipped: line range outside the PR's diff.
| accounts.map((account) => { | ||
| const accountStartTime = new Date().getTime(); | ||
| logger.debug( | ||
| `[Hydration] Fetching persisted conversation list for ${account}` | ||
| ); | ||
|
|
||
| prefetchInboxIdQuery({ account }); | ||
| prefetchConversationsQuery({ account }); | ||
|
|
||
| const accountEndTime = new Date().getTime(); | ||
| logger.debug( | ||
| `[Hydration] Done fetching persisted conversation list for ${account} in ${ | ||
| (accountEndTime - accountStartTime) / 1000 | ||
| } seconds` | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Fix potential race condition in conversation fetching.
The change from Promise.allSettled to map without awaiting means the hydration state might be set before conversations are fully loaded. This could explain the splash screen dismissal issues mentioned in the PR title.
- accounts.map((account) => {
+ await Promise.all(accounts.map(async (account) => {
const accountStartTime = new Date().getTime();
logger.debug(
`[Hydration] Fetching persisted conversation list for ${account}`
);
- prefetchInboxIdQuery({ account });
- prefetchConversationsQuery({ account });
+ await prefetchInboxIdQuery({ account });
+ await prefetchConversationsQuery({ account });
const accountEndTime = new Date().getTime();
logger.debug(
`[Hydration] Done fetching persisted conversation list for ${account} in ${
(accountEndTime - accountStartTime) / 1000
} seconds`
);
- });
+ }));📝 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.
| accounts.map((account) => { | |
| const accountStartTime = new Date().getTime(); | |
| logger.debug( | |
| `[Hydration] Fetching persisted conversation list for ${account}` | |
| ); | |
| prefetchInboxIdQuery({ account }); | |
| prefetchConversationsQuery({ account }); | |
| const accountEndTime = new Date().getTime(); | |
| logger.debug( | |
| `[Hydration] Done fetching persisted conversation list for ${account} in ${ | |
| (accountEndTime - accountStartTime) / 1000 | |
| } seconds` | |
| ); | |
| }); | |
| await Promise.all(accounts.map(async (account) => { | |
| const accountStartTime = new Date().getTime(); | |
| logger.debug( | |
| `[Hydration] Fetching persisted conversation list for ${account}` | |
| ); | |
| await prefetchInboxIdQuery({ account }); | |
| await prefetchConversationsQuery({ account }); | |
| const accountEndTime = new Date().getTime(); | |
| logger.debug( | |
| `[Hydration] Done fetching persisted conversation list for ${account} in ${ | |
| (accountEndTime - accountStartTime) / 1000 | |
| } seconds` | |
| ); | |
| })); |
| func saveProfileSocials(address: String, socials: ProfileSocials) { | ||
| let updatedAt = Int(Date().timeIntervalSince1970) | ||
| let newProfile = Profile(updatedAt: updatedAt, socials: socials) | ||
| let mmkv = getMmkv() | ||
| if let jsonData = try? JSONEncoder().encode(newProfile), let jsonString = String(data: jsonData, encoding: .utf8) { | ||
| mmkv?.set(jsonString, forKey: "profileSocials-\(account.lowercased())-\(address.lowercased())") | ||
| mmkv?.set(jsonString, forKey: "profileSocials-\(address.lowercased())") | ||
| } | ||
| } | ||
|
|
||
| func saveInboxIdProfileSocials(account: String, inboxId: String, socials: ProfileSocials) { | ||
| func saveInboxIdProfileSocials(inboxId: String, socials: ProfileSocials) { | ||
| let updatedAt = Int(Date().timeIntervalSince1970) | ||
| let newProfile = Profile(updatedAt: updatedAt, socials: socials) | ||
| let mmkv = getMmkv() | ||
| if let jsonData = try? JSONEncoder().encode(newProfile), let jsonString = String(data: jsonData, encoding: .utf8) { | ||
| mmkv?.set(jsonString, forKey: "inboxProfileSocials-\(account.lowercased())-\(inboxId.lowercased())") | ||
| mmkv?.set(jsonString, forKey: "inboxProfileSocials-\(inboxId.lowercased())") | ||
| } | ||
| } |
There was a problem hiding this comment.
Implement safer JSON handling.
The current implementation force-unwraps JSON operations, which could crash the app. Consider implementing proper error handling.
- if let jsonData = try? JSONEncoder().encode(newProfile), let jsonString = String(data: jsonData, encoding: .utf8) {
+ do {
+ let jsonData = try JSONEncoder().encode(newProfile)
+ guard let jsonString = String(data: jsonData, encoding: .utf8) else {
+ logger.error("Failed to encode profile data to string")
+ return
+ }
mmkv?.set(jsonString, forKey: "profileSocials-\(address.lowercased())")
- }
+ } catch {
+ logger.error("Failed to encode profile: \(error)")
+ }Committable suggestion skipped: line range outside the PR's diff.
Co-authored-by: alexrisch <[email protected]>
Cleans up 'async' usage in hydration manager
Adds more logs
Removes account from profile related queries
Summary by CodeRabbit
Refactoring
accountparameters across multiple components and hooks.Performance
Technical Improvements
Version Updates