-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Profile Caching #1441
feat: Profile Caching #1441
Conversation
Added initial data to profile queries Cleaned up hydration handler to improve time to responsive Added prefetch to V3 DMs Updated useMessageIsUnread to handle empty/loading state Added additional performance logs
WalkthroughThe pull request introduces several modifications across multiple components and query-related files, focusing on enhancing data fetching, type safety, and performance logging. The changes primarily involve updating the hydration state handling, improving conversation message prefetching, refining query configurations, and adding performance measurement for message retrieval. The modifications aim to optimize the data fetching process and provide more robust error handling and type definitions. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
]); | ||
prefetchInboxIdQuery({ account }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this out as it's a prefetch, we don't want to await it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put it there and we might need some refactoring for this but I think having the InboxId available is a must before showing the content to the user. Since we use it so much.
But again, with the refactor of accounts maybe that will be somewhere else or etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be cached better now, so I think it will be a better experience, where previously it was not cached
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/V3DMListItem.tsx (1)
166-170
: Handle prefetch errors and potential offline scenarios.You're calling
prefetchConversationMessages
before navigating. While this strategy is good for performance, it might be beneficial to handle possible rejections (e.g., network issues, query failures) to provide user feedback. Consider including a catch or error boundary to handle failures gracefully, ensuring the app doesn't navigate with incomplete or stale data.const onPress = useCallback(async () => { try { - prefetchConversationMessages(currentAccount, topic); + await prefetchConversationMessages(currentAccount, topic); } catch (error) { + logger.error("Failed to prefetch conversation messages", error); // Possibly show a toast or fallback UI } navigate("Conversation", { topic }); }, [topic, currentAccount]);queries/useConversationMessages.ts (1)
Line range hint
85-92
: Prefetch function is straightforward, but consider error handling.The
prefetchConversationMessages
function focuses on performance optimization. However, you may want to handle scenarios where the query fails and log warnings or errors for better observability.export const prefetchConversationMessages = async ( account: string, topic: ConversationTopic ) => { + try { return queryClient.prefetchQuery( getConversationMessagesQueryOptions(account, topic) ); + } catch (err) { + logger.warn("[prefetchConversationMessages] failed to prefetch", err); + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
components/StateHandlers/HydrationStateHandler.tsx
(1 hunks)components/V3DMListItem.tsx
(2 hunks)components/V3GroupConversationListItem.tsx
(1 hunks)features/conversation-list/hooks/useMessageIsUnread.ts
(2 hunks)queries/useConversationMessages.ts
(1 hunks)queries/useInboxProfileSocialsQuery.ts
(4 hunks)queries/useProfileSocialsQuery.ts
(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/V3GroupConversationListItem.tsx
🧰 Additional context used
📓 Learnings (1)
features/conversation-list/hooks/useMessageIsUnread.ts (1)
Learnt from: alexrisch
PR: ephemeraHQ/converse-app#1067
File: components/Chat/Message/MessageActions.tsx:297-297
Timestamp: 2024-11-12T10:18:48.668Z
Learning: Due to current performance issues, directly accessing `useFramesStore.getState()` inside `useMemo` is acceptable in certain cases, even though it's generally considered an anti-pattern.
🔇 Additional comments (14)
features/conversation-list/hooks/useMessageIsUnread.ts (2)
4-4
: Consolidate imports or avoid unused ones if possible.
The addition of useChatStore
appears to be necessary here. However, ensure no other unused imports linger, as they can lead to confusion over dependencies in the long run.
28-28
: Helpful check to prevent runtime errors.
Adding this safeguard ensures that the code doesn't break when topicsData[topic]
is undefined. This makes the hook more robust, especially in scenarios where the topic might not be fully loaded yet.
components/StateHandlers/HydrationStateHandler.tsx (2)
36-36
: Code comment clarification is good.
Explicitly noting that this call will create the client and set the conversation list from persistence helps future maintainers understand the underlying process. Good practice.
39-39
: Deferred prefetch is logically consistent.
Calling prefetchInboxIdQuery
after resolving the persisted conversation lists ensures that the inbox ID is always fetched in the correct sequence. This reduces the likelihood of race conditions.
queries/useInboxProfileSocialsQuery.ts (4)
2-2
: Enhanced type safety recognized.
Importing QueryKey
and useQueries
from @tanstack/react-query
is a solid approach toward maintaining strong type definitions in your query configuration.
14-17
: Clearer return type for query keys.
Declaring the return type as QueryKey
clarifies the type signature and improves IntelliSense for consumers.
40-43
: Explicit promise return type.
Defining the function to return Promise<IProfileSocials[] | null>
clarifies behavior for consumers, ensuring they handle both array and null scenarios.
72-82
: Initial data logic for rehydration is well-structured.
The check for !account || !inboxId
and presence in mmkv
ensures the correct initial data is supplied or undefined if unavailable. This pattern helps maintain consistent, reactive data throughout the app.
queries/useProfileSocialsQuery.ts (4)
2-2
: Adoption of QueryKey for improved type definitions.
Importing and using QueryKey
from @tanstack/react-query
is commendable; it reduces ambiguity around query keys in your application.
15-18
: Function signature strongly typed.
Providing a typed return for profileSocialsQueryKey
intensifies clarity on how query keys are structured, benefiting debugging and future refactors.
68-75
: Graceful handling of initial data.
This approach properly checks MMKV storage before returning any data. It’s beneficial for offline or initial load states, preventing extraneous network calls when data is already available.
111-111
: Refined fetch query type.
Switching to fetchQuery<IProfileSocials | null>
appropriately reflects the possibility of no data (null). This reduces type confusion for consumers of the fetchQuery result.
components/V3DMListItem.tsx (1)
30-30
: Import statement looks good.
Importing prefetchConversationMessages
is consistent with its usage below. There's no immediate concern here.
queries/useConversationMessages.ts (1)
32-48
: Performance logging implementation is well-structured.
Measuring fetch and processing times provides valuable insight into performance bottlenecks. The overall logic is coherent. Ensure that large logs—including message counts—do not negatively impact log parsing or lead to excessive verbosity.
Removed syncing on prefetch of conversation messages
@@ -63,7 +69,18 @@ const inboxProfileSocialsQueryConfig = ( | |||
// And automatic retries if there was an error fetching | |||
refetchOnMount: false, | |||
staleTime: 1000 * 60 * 60 * 24, | |||
// persister: reactQueryPersister, | |||
initialData: (): IProfileSocials[] | null | undefined => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return type should be inferred no?
@@ -62,6 +65,15 @@ const profileSocialsQueryConfig = (account: string, peerAddress: string) => ({ | |||
// And automatic retries if there was an error fetching | |||
refetchOnMount: false, | |||
staleTime: 1000 * 60 * 60 * 24, | |||
initialData: (): ProfileSocialsData => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return type should be inferred no?
@@ -96,7 +108,7 @@ export const fetchProfileSocialsQuery = ( | |||
account: string, | |||
peerAddress: string | |||
) => { | |||
return queryClient.fetchQuery<ProfileSocialsData>( | |||
return queryClient.fetchQuery<IProfileSocials | null>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this I love having something like in useConversationMessages
export type ConversationMessagesQueryData = Awaited<
ReturnType<typeof conversationMessagesQueryFn>
>;
and reusing ConversationMessagesQueryData
everywhere we use queryClient...
const fetchInboxProfileSocials = async ( | ||
account: string, | ||
inboxId: InboxId | ||
): Promise<IProfileSocials[] | null> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return type should be inferred no?
Added initial data to profile queries
Cleaned up hydration handler to improve time to responsive Added prefetch to V3 DMs
Updated useMessageIsUnread to handle empty/loading state Added additional performance logs
Summary by CodeRabbit
Release Notes
Performance Improvements
Query Enhancements
Code Optimization
Prefetching