Reuse MessageStore in useDraft hook #5471
Conversation
nicodh
force-pushed
the
extract-usedraft-hook
branch
from
18b004d to
637bfb3
Compare
nicodh
force-pushed
the
extract-usedraft-hook
branch
from
637bfb3 to
b2d7b12
Compare
nicodh
force-pushed
the
extract-usedraft-hook
branch
from
b2d7b12 to
769050a
Compare
There was a problem hiding this comment.
Yes, this looks closer to what I meant. The message list buggines seems to be gone now since my last review. And yes, this seems to fix the bug this is intending to fix, despite some functionality being now lost due to the migration to the messageCache as the "source of truth" (e.g. there is now this "jump without quoting" behavior).
The commit history is messy, but I would still vote for a fast-forward merge, because otherwise it's gonna be impossible to decipher the changes to useDraft, since it has been moved.
nicodh
force-pushed
the
extract-usedraft-hook
branch
from
f2a7030 to
ffc6fda
Compare
packages/frontend/src/components/message/MessageListAndComposer.tsx
Outdated
Show resolved
Hide resolved
|
About performance: for some reason it feels very sluggish, but only in dev mode. If you build frontend in prod mode, then it's very snappy! |
nicodh
force-pushed
the
extract-usedraft-hook
branch
from
ffc6fda to
91522de
Compare
There was a problem hiding this comment.
Looking at the state of the code, I think we are finally (almost) ready, thanks!
As I mentioned, it would be very nice to have a separate commit for useDraft being moved to a separate file, otherwise the changes that this MR does to it are gonna be undecipherable.
However, there has been a lot of changing stuff back and forth, so it would also be nice to squash the last few commits.
If you agree with the suggestion, I could take care of that.
| const { | ||
| store: messageListStore, | ||
| state: messageListState, | ||
| fetchMoreBottom, | ||
| fetchMoreTop, | ||
| } = useMessageList(accountId, chat.id) |
There was a problem hiding this comment.
Maybe not destructuring these would be tidier, but alright.
But there is a separate commit for extract useDraft: |
|
Yes, sorry, I meant that we should not squash that commit, but squash the other ones (or almost all the other ones) |
That means I have to merge locally or is that possible in the github UI? |
|
No I don't think it's possible on GitHub, but one could squash locally and then force push to this MR. |
nicodh
force-pushed
the
extract-usedraft-hook
branch
from
41917a8 to
047f845
Compare
Damn, I just wanted to expand the "Squash & Merge button" to select the other option and then it just squashed an merged it immediately. Bad UI! |
|
FTR we force-pushed. This is merged as 6fc4777 (and the parent commit). |
As discussed here #5337 (comment) "make the message list available to the shortcut handling code"
To avoid loading messages when navigation up or down when selecting messages for replyTo reuse the MessageStore which already holds the visible messages in messageCache
Special case: if there is already a quoted message in a draft, and the quoted message is not in messageCache (i.e. not visible) - we jump to that message on first shortcut event and only the next up or down will navigate to the next message.