Skip to content
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

Refactor chat store logic into dedicated React contexts #3725

Merged
merged 51 commits into from
Apr 16, 2024

Conversation

adzialocha
Copy link
Member

@adzialocha adzialocha commented Mar 7, 2024

  • Remove ChatStore
  • Move "global" chat state to ChatContext

Closes: #3675

@adzialocha adzialocha added the refactor This PR or issue is about refactoring label Mar 7, 2024
@adzialocha
Copy link
Member Author

I think the refactoring is so far complete, I'll look into bugs now which probably occurred after touching so much

@adzialocha
Copy link
Member Author

I'm not sure why this is stuck on throwing linting errors about things which do not exist anymore, restarting the CI didn't help ..

In any case, this PR is ready for review and merging.

Copy link
Member

@Simon-Laux Simon-Laux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When switching accounts the account sidebar flickers, we should somehow prevent it from completely reloading (visually popping in unread counters) in that case.

Generally the code looks really nice now! Thanks very much for taking on this refactoring, it's a huge leap forward and was easy to review 🚀 🌟

src/renderer/components/MessageListView.tsx Show resolved Hide resolved
src/renderer/hooks/useSendCallInvitation.ts Outdated Show resolved Hide resolved
src/renderer/contexts/ChatContext.tsx Outdated Show resolved Hide resolved
src/renderer/contexts/ChatContext.tsx Show resolved Hide resolved
return
}

if (!chat) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can also check for chat type, if it's not a 1on1/DM chat then a contact update does not affect it AFAIK

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not 100% sure why this update is required at all, could you explain what this is for, then I'll add it as a comment. Thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess if contact name, avatar or recently seen changes on a 1on1 chat. maybe only the recently seen in reality I don't know in my head if the other events trigger a chat update instead or also rely on the ContactsChanged event

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still confused why this update is required and why it is only for 1:1 chats. Maybe I'll leave it as-is for now (as this is how it was before as well) and we can add commentary / adjustments later?

src/renderer/hooks/usePrevious.ts Show resolved Hide resolved
Copy link
Contributor

@nicodh nicodh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good refactoring and cleanup to me!
Only some ideas for naming and comments. In general I prefer more verbose comments, they are removed anyway when compiled.

src/renderer/contexts/ChatContext.tsx Outdated Show resolved Hide resolved
src/renderer/hooks/useSelectLastChat.ts Outdated Show resolved Hide resolved
src/renderer/hooks/useChat.ts Outdated Show resolved Hide resolved
@adzialocha adzialocha merged commit e2f9cdb into master Apr 16, 2024
8 checks passed
@adzialocha adzialocha deleted the adz/chat-store-refactor branch April 16, 2024 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor This PR or issue is about refactoring
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Refactor to remove possible races when switching account in message list / chatstore
3 participants