Page MenuHomePhabricator

[lib] Introduce useGetLatestMessageEdit
ClosedPublic

Authored by ashoat on Jul 15 2024, 7:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Oct 19, 12:32 PM
Unknown Object (File)
Sat, Oct 19, 12:32 PM
Unknown Object (File)
Sat, Oct 19, 12:32 PM
Unknown Object (File)
Sat, Oct 19, 12:32 PM
Unknown Object (File)
Sat, Oct 19, 12:32 PM
Unknown Object (File)
Sat, Oct 19, 12:32 PM
Unknown Object (File)
Fri, Oct 18, 6:23 AM
Unknown Object (File)
Thu, Oct 10, 6:29 AM
Subscribers

Details

Summary

Now that we're exposing getRelatedMessages from SQLite instead of useGetLatestMessageEdit, we need some business logic on the JS side to construct the "latest message edit". We do this by swapping in the text of the latest edit, similar to how the equivalent code in keyserver works.

This diff replaces D12762.

Depends on D12740

Test Plan

In combination with the next diff, I tested a patch on both native and web that hit this code with a specific message ID and confirmed that it returned the message with the latest edit swapped in

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 15 2024, 7:51 PM
Harbormaster failed remote builds in B30352: Diff 42314!
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 15 2024, 8:53 PM
Harbormaster failed remote builds in B30357: Diff 42319!
This revision is now accepted and ready to land.Jul 22 2024, 1:24 AM

This solution requires us to query for every message separately. This seems very inefficient. Is this necessary?

It makes sense for the sidebar creation use case that I designed for, which is where this hook will be used. I don't think you'll want to use this particular hook, but I can see how for the search use case, it might make sense to replace getRelatedMessages with an API that can query a batch of message IDs at once. I haven't considered the performance implications here – would be good for you and @kamil to consider this, and make a decision on whether the API needs to be replaced

This revision was automatically updated to reflect the committed changes.