-
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
fix: Many fixes #1554
fix: Many fixes #1554
Conversation
0958d01
to
62dbde9
Compare
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 |
62dbde9
to
b4c84c8
Compare
await client.conversations.sync(); | ||
await client.conversations.syncAllConversations("allowed"); |
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.
Why would we remove this?
...ation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversations.tsx
Outdated
Show resolved
Hide resolved
features/conversation-list/hooks/use-conversation-list-item-context-menu-default-props.tsx
Outdated
Show resolved
Hide resolved
const { theme } = useAppTheme(); | ||
const colorScheme = theme.isDark ? "dark" : "light"; | ||
|
||
const currentAccount = useCurrentAccount()!; | ||
|
||
const { data: conversationId } = useQuery({ | ||
...getConversationQueryOptions({ |
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.
Shouldn't this be memoized?
I guess we are pretty close to react compiler usage
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.
IMO that doesn't change anything since it's with a useQuery but might be wrong
logger.warn( | ||
`[useConversationQuery] Conversation not found in local DB, syncing conversations` | ||
); | ||
await client.conversations.sync(); |
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.
Is this syncing all conversations now?
If not it will cause the list to sync, but conversations will be out of date
// Only the unknown conversations | ||
isConversationConsentUnknown(conversation) && | ||
// Only the conversations with last message | ||
conversation.lastMessage |
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 don't think we should be checking on lastMessage here, there's times where it can be empty
We will need to also make sure the requests isn't incrementing based on those either
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.
Ahh I thought conversations without lastMessage would always be non-relevant but good will remove.
features/consent/use-dm-consent.tsx
Outdated
}) { | ||
const { peerInboxId, conversationId, topic } = args; | ||
|
||
export function useDmConsent() { |
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.
ForCurrentInbox/Account
const currentAccount = useCurrentAccount()!; | ||
|
||
return useMutation({ | ||
mutationFn: async (args: { consent: "allow" | "deny" }) => { | ||
mutationFn: async (args: { |
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.
does moving the arguments to the functions instead of the hook fix anything or is it just more idiomatic?
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 just feel like if we're only using the args when calling the mutation, might as well pass them when we call the mutation.
No description provided.