fix: Persisted queries #1320
fix: Persisted queries #1320
Conversation
WalkthroughThe pull request introduces significant changes to the hydration logic in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
components/StateHandlers/HydrationStateHandler.tsx(2 hunks)package.json(1 hunks)queries/useInboxProfileSocialsQuery.ts(2 hunks)queries/useProfileSocialsQuery.ts(2 hunks)queries/useV3ConversationListQuery.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
queries/useV3ConversationListQuery.ts (1)
Learnt from: alexrisch
PR: ephemeraHQ/converse-app#1313
File: queries/useV3ConversationListQuery.ts:196-268
Timestamp: 2024-12-05T18:33:24.472Z
Learning: In the TypeScript file `queries/useV3ConversationListQuery.ts`, helper functions like `updateGroupImageToConversationListQuery`, `updateGroupNameToConversationListQuery`, and `updateGroupDescriptionToConversationListQuery` are used to reduce dependency on exact types or creating objects that get passed unnecessarily, even though they internally use `updateGroupMetadataToConversationListQuery`.
🔇 Additional comments (4)
queries/useProfileSocialsQuery.ts (1)
11-11: Ensure secure handling of persisted query data
Since the query data is being persisted using reactQueryPersister, verify that any sensitive information within ProfileSocials is securely handled, and consider encrypting the persisted data if necessary.
Also applies to: 48-48
queries/useInboxProfileSocialsQuery.ts (1)
12-12: Ensure secure handling of persisted query data
Since the query data in InboxProfileSocials is being persisted using reactQueryPersister, verify that any sensitive information is securely handled, and consider encrypting the persisted data if necessary.
Also applies to: 52-52
queries/useV3ConversationListQuery.ts (1)
82-92: Refactoring to centralize query configuration improves maintainability
The introduction of v3ConversationListQueryConfig simplifies the query configuration and promotes consistency across the codebase.
package.json (1)
72-75: Confirm the shift to exact versioning for @tanstack dependencies
You've updated the @tanstack dependencies to exact versions. Ensure this change is intentional, as using exact versions will prevent receiving automatic updates that may include important fixes and improvements.
| // }); | ||
| // Fetching persisted conversation lists for all accounts | ||
| // We may want to fetch only the selected account's conversation list | ||
| // in the future, but this is simple for now, and want to get feedback to really confirm |
There was a problem hiding this comment.
What are we confirming?
There was a problem hiding this comment.
Confirming actual timing of real users
Difficult to gauge in dev, but benchmarks on the SDK side are good
alexrisch
force-pushed
the
ar/hydration
branch
from
9c52c46 to
4507643
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
components/StateHandlers/HydrationStateHandler.tsx (1)
23-25: Consider documenting the rationale for fetching all accountsThe comment suggests this is a temporary solution pending feedback. Consider documenting:
- Performance implications of fetching all accounts
- Criteria for deciding when to switch to selected account only
- Metrics being collected for the feedback
queries/useV3ConversationListQuery.ts (1)
106-112: Consider adding retry configuration for network resilienceThe fetch operations (
fetchPersistedConversationListQuery,fetchConversationListQuery,prefetchConversationListQuery) could benefit from retry configuration to handle temporary network issues.Consider adding retry configuration to the query config:
const v3ConversationListQueryConfig = ( account: string, context: string, includeSync: boolean = true ) => ({ queryKey: conversationListKey(account), queryFn: () => v3ConversationListQueryFn(account, context, includeSync), staleTime: 2000, enabled: !!account, + retry: 3, + retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000), });Also applies to: 116-118, 122-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
components/StateHandlers/HydrationStateHandler.tsx(2 hunks)package.json(1 hunks)queries/useInboxProfileSocialsQuery.ts(2 hunks)queries/useProfileSocialsQuery.ts(2 hunks)queries/useV3ConversationListQuery.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- queries/useProfileSocialsQuery.ts
- queries/useInboxProfileSocialsQuery.ts
- package.json
🧰 Additional context used
📓 Learnings (1)
queries/useV3ConversationListQuery.ts (1)
Learnt from: alexrisch
PR: ephemeraHQ/converse-app#1313
File: queries/useV3ConversationListQuery.ts:196-268
Timestamp: 2024-12-05T18:33:24.472Z
Learning: In the TypeScript file `queries/useV3ConversationListQuery.ts`, helper functions like `updateGroupImageToConversationListQuery`, `updateGroupNameToConversationListQuery`, and `updateGroupDescriptionToConversationListQuery` are used to reduce dependency on exact types or creating objects that get passed unnecessarily, even though they internally use `updateGroupMetadataToConversationListQuery`.
🔇 Additional comments (4)
components/StateHandlers/HydrationStateHandler.tsx (2)
8-8: LGTM: New import aligns with PR objectives
The addition of fetchPersistedConversationListQuery import supports the PR's goal of fetching persisted conversations before dismissing the splash screen.
27-42:
Enhance error handling for Promise.allSettled
While the implementation correctly uses Promise.allSettled for concurrent fetching, it doesn't handle rejected promises. This could lead to silent failures.
Consider this enhancement:
- await Promise.allSettled(
+ const results = await Promise.allSettled(
accounts.map(async (account) => {
const accountStartTime = new Date().getTime();
logger.debug(
`[Hydration] Fetching persisted conversation list for ${account}`
);
await getXmtpClient(account);
await fetchPersistedConversationListQuery(account);
const accountEndTime = new Date().getTime();
logger.debug(
`[Hydration] Done fetching persisted conversation list for ${account} in ${
(accountEndTime - accountStartTime) / 1000
} seconds`
);
})
);
+ const failures = results.filter(r => r.status === 'rejected');
+ if (failures.length > 0) {
+ logger.error('[Hydration] Failed to fetch some persisted conversations:',
+ failures.map(f => (f as PromiseRejectedResult).reason));
+ }queries/useV3ConversationListQuery.ts (2)
83-92: LGTM: Well-structured query configuration centralization
The new v3ConversationListQueryConfig function effectively centralizes the configuration, improving maintainability and reducing duplication. The staleTime of 2000ms seems reasonable for this use case.
129-132: LGTM: Consistent usage of query configuration
The modifications to invalidateGroupsConversationListQuery, getConversationListQueryData, and setConversationListQueryData maintain consistency by using the centralized configuration.
Also applies to: 140-141, 150-151
Added persistance by query Bumped ReactQuery Versions Added handling to fetch persisted conversations before dismissing the splash screen
alexrisch
force-pushed
the
ar/hydration
branch
from
4507643 to
66cea07
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
queries/useV3ConversationListQuery.ts (1)
Line range hint
100-151: Consider adding error boundaries for query failuresWhile the refactoring to use centralized configuration is good, consider adding error boundaries or retry logic for network-related failures in the query functions.
Example for fetchPersistedConversationListQuery:
export const fetchPersistedConversationListQuery = (account: string) => { return queryClient.fetchQuery( v3ConversationListQueryConfig( account, "fetchPersistedConversationListQuery", false - ) + ), + { + retry: 2, + retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000) + } ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
components/StateHandlers/HydrationStateHandler.tsx(2 hunks)package.json(1 hunks)queries/useInboxProfileSocialsQuery.ts(2 hunks)queries/useProfileSocialsQuery.ts(2 hunks)queries/useV3ConversationListQuery.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- queries/useProfileSocialsQuery.ts
- queries/useInboxProfileSocialsQuery.ts
- package.json
🧰 Additional context used
📓 Learnings (1)
queries/useV3ConversationListQuery.ts (1)
Learnt from: alexrisch
PR: ephemeraHQ/converse-app#1313
File: queries/useV3ConversationListQuery.ts:196-268
Timestamp: 2024-12-05T18:33:24.472Z
Learning: In the TypeScript file `queries/useV3ConversationListQuery.ts`, helper functions like `updateGroupImageToConversationListQuery`, `updateGroupNameToConversationListQuery`, and `updateGroupDescriptionToConversationListQuery` are used to reduce dependency on exact types or creating objects that get passed unnecessarily, even though they internally use `updateGroupMetadataToConversationListQuery`.
🔇 Additional comments (3)
components/StateHandlers/HydrationStateHandler.tsx (2)
7-8: LGTM: Clean import addition
The import of fetchPersistedConversationListQuery aligns with the PR objective to fetch persisted conversations before dismissing the splash screen.
23-42:
Handle rejected promises from Promise.allSettled
While the concurrent fetching implementation is good, the code doesn't handle potential failures from Promise.allSettled. This could lead to silent failures if fetching fails for some accounts.
Consider handling the results:
- await Promise.allSettled(
+ const results = await Promise.allSettled(
accounts.map(async (account) => {
const accountStartTime = new Date().getTime();
logger.debug(
`[Hydration] Fetching persisted conversation list for ${account}`
);
await getXmtpClient(account);
await fetchPersistedConversationListQuery(account);
const accountEndTime = new Date().getTime();
logger.debug(
`[Hydration] Done fetching persisted conversation list for ${account} in ${
(accountEndTime - accountStartTime) / 1000
} seconds`
);
})
);
+
+ // Log any failures
+ results.forEach((result, index) => {
+ if (result.status === 'rejected') {
+ logger.error(
+ `[Hydration] Failed to fetch conversations for account ${accounts[index]}:`,
+ result.reason
+ );
+ }
+ });queries/useV3ConversationListQuery.ts (1)
83-92: LGTM: Well-structured query configuration
Good centralization of query configuration with proper defaults and type safety. The inclusion of context and includeSync parameters provides flexibility for different use cases.
Added persistance by query Bumped ReactQuery Versions Added handling to fetch persisted conversations before dismissing the splash screen
Added persistance by query
Bumped ReactQuery Versions
Added handling to fetch persisted conversations before dismissing the splash screen
Closes #1321
Summary by CodeRabbit
Release Notes
New Features
Improvements
Dependency Updates
@tanstacklibrary packages to version5.62.2for improved performance and stability.