While working on improving message liking, came across this edge case where users were able to react to the source message of a sidebar. We shouldn't allow users to do this, so I implemented a check on the client and keyserver to ensure we catch this behavior. In regards to displaying the reactions for the source message from the original thread to the sidebar thread, this linear task tracks this work and is something we will be addressing in the near future.
Details
Details
Please watch the demo videos to see the changes I made:
Before:
After:
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Client side check makes sense, I think we should add a condition to the fetchServerThreadInfos() call to narrow down the result set
We shouldn't allow users to do this
Don't have context, but it seems like a reasonable thing to allow?
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 222 ↗ | (On Diff #20038) | Are you sure we don't want to pass in a condition argument to fetchServerThreadInfos? It looks like we only need the threadInfo for threadID, can we add that as a WHERE condition to avoid querying unnecessary data? |
Comment Actions
Don't have context, but it seems like a reasonable thing to allow?
Our long-term plan is to share reacts between the source message in the parent and in the sidebar. I think that's what @ginsu was referring to in the diff description:
In regards to displaying the reactions for the source message from the original thread to the sidebar thread, this linear task tracks this work and is something we will be addressing in the near future.
I would just add that beyond displaying the same reactions, we should reenable reacting on the SIDEBAR_SOURCE, but just redirect those reacts to the original message in the parent.
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 222 ↗ | (On Diff #20038) | This is a good point, but I'd make an either further one: I don't see why we need to fetch this. Can't we just check if the target message is of type SIDEBAR_SOURCE? |
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 222 ↗ | (On Diff #20038) | oh yeah |
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 222 ↗ | (On Diff #20038) | Not sure if I am calling fetchMessageInfoByID incorrectly or I found a bug with fetchMessageInfoByID, but when I logged out the type of targetMessageInfo which should be of type SIDEBAR_SOURCE in this responder, I instead got type of TEXT. I did some more digging and I found out that the targetMessageInfo that is being returned is actually from the parent thread and not this sidebar thread (the threadID in targetMessageInfo is different from the one I am just logging above). Let me know if I should create a task to fix this or if there is something you guys can easily see that I am missing |
Comment Actions
Our long-term plan is to share reacts between the source message in the parent and in the sidebar. I think that's what @ginsu was referring to in the diff description:
Yup that is correct, thank you for making that more clear!
I would just add that beyond displaying the same reactions, we should reenable reacting on the SIDEBAR_SOURCE, but just redirect those reacts to the original message in the parent.
I like this idea a lot, I will update the linear task to include this
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 222 ↗ | (On Diff #20038) | Not sure if it's a bug or by design that fetchMessageInfoByID returns the source of a sidebar. Can you check where fetchMessageInfoByID is used and try to figure out if that behavior is good or bad? |
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 222 ↗ | (On Diff #20038) | Okay after some digging, I have come to the conclusion that fetchMessageInfoByID is actually working correctly as expected. The issue lies in the tooltip route params which has a messageInfo field that is returning the original source of the sidebar from the parent thread, instead of the sidebar source. Will look into this more and make a diff if changes are necessary |
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 222 ↗ | (On Diff #20038) | As far as I remember fetchMessageInfoByID doesn't replace a message with its source. Instead it creates a derived messages (sources) and pass them to appropriate rawMessageInfoFromServerDBRow so that a correct message can be constructed. On the frontend in createChatMessageItems we're replacing messages with their sources, which should make reacting to the original message a lot easier. |