fix(pins): apply pins to messages being loaded by fetching more #16897
Conversation
jrainville
requested review from
noeliaSD,
a team,
iurimatias,
igor-sirotin and
osmaczko
and removed request for
a team and
noeliaSD
Jenkins BuildsClick to see older builds (28)
|
src/app/modules/main/chat_section/chat_content/messages/module.nim
Outdated
Show resolved
Hide resolved
jrainville
force-pushed
the
fix/old-pins-looking-normal
branch
from
308337a to
27ef71f
Compare
|
hmmm, doesn't status-go provide pinnedBy already for each message? 🤔 |
I would feel quite silly if that was the case. Let's see |
Ok, I don't feel silly in the end @igor-sirotin . It's indeed not available from the {
"id": "abe40742d35c03ed75c32c2def0a137b5b93a12bac6dd2daca7472f3623f1a24",
"whisperTimestamp": 1733419934000,
"from": "0x04ea5307274c69ee2075c82e9cd22de9bcc370d2d05620c06b1255a99cae6d697075525d747f3698ed1358c50f022ffd25d7d200e0aa51d7f6ce6c504457392e2f",
"alias": "zQ3...FqioYB",
"identicon": "",
"seen": true,
"quotedMessage": {
"id": "0xe386b48e38591d3dd79532cc83b6bb55780c877a10644f7c53658468a21702b1",
"contentType": 1,
"from": "0x04ea5307274c69ee2075c82e9cd22de9bcc370d2d05620c06b1255a99cae6d697075525d747f3698ed1358c50f022ffd25d7d200e0aa51d7f6ce6c504457392e2f",
"text": "allo 2",
"parsedText": [
{
"type": "paragraph",
"children": [
{
"literal": "allo 2"
}
]
}
],
"albumImagesCount": 0
},
"rtl": false,
"lineCount": 0,
"text": "",
"chatId": "0x04ea5307274c69ee2075c82e9cd22de9bcc370d2d05620c06b1255a99cae6d697075525d747f3698ed1358c50f022ffd25d7d200e0aa51d7f6ce6c504457392e2f",
"localChatId": "0x04ea5307274c69ee2075c82e9cd22de9bcc370d2d05620c06b1255a99cae6d697075525d747f3698ed1358c50f022ffd25d7d200e0aa51d7f6ce6c504457392e2f",
"clock": 1733419933645,
"replace": "",
"responseTo": "0xe386b48e38591d3dd79532cc83b6bb55780c877a10644f7c53658468a21702b1",
"ensName": "",
"displayName": "",
"gapParameters": {},
"timestamp": 1733419934000,
"contentType": 14,
"messageType": 1,
"compressedKey": "zQ3shvQoxa3rU3HkuGFgjgxi8ZCwQkiiHgTUNQy86pNFqioYB",
"emojiHash": [
"skipped"
]
}, |
jrainville
force-pushed
the
fix/old-pins-looking-normal
branch
from
27ef71f to
7df8fa8
Compare
There was a problem hiding this comment.
@jrainville you're right, it doesn't exist indeed. But do you think we should just add it there and not mess on the Nim side?
common.Message already provides data from not this specific message, like quotedMessage. So I think we could hydrate it with pinnedBy somewhere in prepareMessage.
What do you think?
@igor-sirotin I think it would need to be done in the persistence level since the message itself doesn't have any idea it has been pinned. Therefore, IMO, the real solution would have been to just have the Now, we can still just I don't know if we need to ask other people before making that decision in case there is something else we haven't thought of in terms of performance? |
I prefer not to mess up and keep tables as separate as possible, especially for the
Yes I think you're right, better
I don't know, not a big pro in database world, I guess we can probably do some benchmarks or ask @osmaczko what he thinks. |
Our custom On a serious note, I'd keep them separate, I'd even simplify the NIM part and do the join in QML using the above ^^ |
jrainville
force-pushed
the
fix/old-pins-looking-normal
branch
from
7df8fa8 to
666b15b
Compare
|
@igor-sirotin I went with the You can find the status-go PR here: status-im/status-go#6226 I changed the code here completely too. It's way simpler now |
| @@ -107,7 +107,7 @@ proc createMessageItemFromDto(self: Module, message: MessageDto, communityId: st | |||
| imagesAlbum.add(msg.image) | |||
| albumMessageIds.add(msg.id) | |||
|
|
|||
| return msg_item_qobj.newMessageItem(msg_item.initItem( | |||
| return msg_item_qobj.newMessageItem(msg_item.initMessageItem( | |||
There was a problem hiding this comment.
I renamed this function because it was so annoying trying to see which function to modify when every Item has the same init function. Now it's gonna be way clearer.
|
@iurimatias you have been randomly chosen to test this PR
|
jrainville
force-pushed
the
fix/old-pins-looking-normal
branch
from
666b15b to
c71a9ef
Compare
Fixes #16896 The problem was that we relied on the `SIGNAL_PINNED_MESSAGES_LOADED` event to apply the pinned status to messages, but that only happens at the start and a lot of messages are not loaded at start if they are not in the first 30 messages. To fix this, I just added `pinnedBy` to the Message object in status-go. This way, we can easily tell straight from the MessageDto if a message is pinned and by whom.
jrainville
force-pushed
the
fix/old-pins-looking-normal
branch
from
c71a9ef to
cdeaf56
Compare
|
The allure report failed to generate, but the tests passed. I'll force the merge because I've been stuck on that for 2 days |
What does the PR do
Fixes #16896
status-go PR status-im/status-go#6226
The problem was that we relied on the
SIGNAL_PINNED_MESSAGES_LOADEDevent to apply the pinned status to messages, but that only happens at the start and a lot of messages are not loaded at start if they are not in the first 30 messages.To fix this, I just added
pinnedByto the Message object in status-go. This way, we can easily tell straight from the MessageDto if a message is pinned and by whom.Affected areas
Messages (list)
Screenshot of functionality (including design for comparison)
old-pins.webm
Impact on end user
Fixed a bug where old pins showed as normal messages
How to test
Scroll back up
Risk
Tick one:
Worst case, the bug still happens, or the table of pins in the code doesn't get up to date and we get a weird state