Page MenuHomePhabricator
Differential D7141

[keyserver] Display edited message in the sidebar if the source message is edited
ClosedPublic
Actions

Authored by • kuba on Mar 22 2023, 5:58 AM.
Tags
None
Referenced Files
F3350291: D7141.diff
Fri, Nov 22, 9:45 PM
Unknown Object (File)
Mon, Nov 4, 11:17 PM
Unknown Object (File)
Mon, Nov 4, 11:17 PM
Unknown Object (File)
Mon, Nov 4, 11:16 PM
Unknown Object (File)
Mon, Nov 4, 11:16 PM
Unknown Object (File)
Mon, Nov 4, 11:16 PM
Unknown Object (File)
Mon, Nov 4, 11:16 PM
Unknown Object (File)
Mon, Oct 28, 6:04 AM
View All Files
Subscribers
ashoat
atul
rohan
tomek

Details

Reviewers
michal
inka
ginsu
ashoat
rohan
Commits
rCOMM18211251deb0: [keyserver] Display edited message in the sidebar if the source message is…
Summary

In case the user creates a sidebar from the message which has been edited, we need to add an edit message to a new thread to display updated content to the client.

In case the user creates a sidebar from the message which has been edited, we need to display updated content to the client. We are doing it by changing the content of sourceMessage of SIDEBAR_SOURCE.

Test Plan

Create a new pending thread from the edited message. Send a message to it. A new thread is created, and the updated content of the first message is visible. The edited source_message is successfully sent to the client.
Create a new pending thread from an unedited message. Send a message and check if everything works as before.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• kuba created this revision.Mar 22 2023, 5:58 AM
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptMar 22 2023, 5:58 AM
• kuba added a parent revision: D7140: [lib] Displaying edited content in a pending thread.Mar 22 2023, 5:58 AM
• kuba added a child revision: D7142: [lib] Checking if the user can edit the message.Mar 22 2023, 6:01 AM
Harbormaster completed remote builds in B17521: Diff 23989.Mar 22 2023, 6:11 AM
• kuba requested review of this revision.Mar 22 2023, 6:11 AM
ashoat requested changes to this revision.Mar 22 2023, 3:08 PM
ashoat added inline comments.
keyserver/src/creators/thread-creator.js
457 ↗(On Diff #23989)

Can you clarify why this is necessary? My understanding is that SIDEBAR_SOURCE works with a "pointer" to a message already, so it seems that we should be able to adjust that code (fetchDerivedMessages) to just call fetchLatestEditMessageContentByID instead of creating extra messages

(On the other hand, any edits to the SIDEBAR_SOURCE that occur after sidebar creation will need to be mirrored in the sidebar with an EDIT_MESSAGE like you are doing here.)

keyserver/src/fetchers/message-fetchers.js
728–731 ↗(On Diff #23989)
  1. What are these parentheses for? You can run git grep AND to see how we typically format SQL queries like this
  2. We typically give FROM its own line
  3. There's no point declaring a table alias if you are only looking at one table
  4. Please remove ALL trailing spaces from this diff
  5. The semicolon is unnecessary unless you use { multipleStatements: true }
This revision now requires changes to proceed.Mar 22 2023, 3:08 PM
ashoat added inline comments.Mar 22 2023, 3:15 PM
keyserver/src/fetchers/message-fetchers.js
726 ↗(On Diff #23989)

You should never use any unless you have a really good reason. You should expect your reviewer will ALWAYS request changes if they see any anywhere unless it is justified

• kuba updated this revision to Diff 24031.Mar 23 2023, 8:43 AM
• kuba marked 2 inline comments as done.

Addressed some of the comments

keyserver/src/creators/thread-creator.js
457 ↗(On Diff #23989)

Currently, editing messages work in this way:
Every edit message is added (mirrored) to both the parent thread and the sidebar thread (this is how I understood how to do it, but we duplicate the data). It is mirrored both on thread creation (this diff) and on sidebar source message edit (https://phab.comm.dev/D7145).

I will check how derived messages work and see if I can easily add an extra 'edit_message' to the derived message, next to sourceMessage, and then display it.

Harbormaster completed remote builds in B17554: Diff 24031.Mar 23 2023, 8:58 AM
ashoat added a reviewer: rohan.Mar 23 2023, 7:24 PM
ashoat added a subscriber: rohan.
ashoat added inline comments.
keyserver/src/creators/thread-creator.js
457 ↗(On Diff #23989)

Adding @rohan here – he is looking at fetchDerivedMessages right now in D7148

keyserver/src/fetchers/message-fetchers.js
733 ↗(On Diff #24031)

Two more nits to match the style in the codebase:

  1. Can you give LIMIT 1 its own line?
  2. Can you put "`;" on its own line?
ashoat requested changes to this revision.Mar 23 2023, 7:25 PM
ashoat mentioned this in D7148: [keyserver/lib] Introduce a new message type and spec for toggling pins.

Back to your queue for nits and fetchDerivedMessages

This revision now requires changes to proceed.Mar 23 2023, 7:26 PM
• kuba added inline comments.Mar 27 2023, 9:01 AM
keyserver/src/creators/thread-creator.js
457 ↗(On Diff #23989)

I want to make sure this approach is correct:

  • we want to have 2 separate `EDIT_MESSAGEs for both the main thread and sidebar,
  • we don't want to copy the content of edited messages,
  • sidebar's EDIT_MESSAGE should only point to the EDIT_MESSAGE in the main thread and load the main EDIT_MESSAGE as a derived message (and its content).

In that way, I don't copy the content of the message. Do I understand it right and should I do it in that way?

Previously, I thought we wanted to: load the latest EDIT_MESSAGE together with the SIDEBAR_SOURCE message, and don't create 2 copies of EDIT_MESSAGEs when editing message with sidebar.

ashoat added inline comments.Mar 27 2023, 4:20 PM
keyserver/src/creators/thread-creator.js
457 ↗(On Diff #23989)

We only want to have 2 separate EDIT_MESSAGES if it is necessary. If the edit occurs before the sidebar is created, we should not need an EDIT_MESSAGE in the sidebar. On the other hand, if the edit occurs after the sidebar is created, there is no other way to do it.

For the case where the edit occurs before the sidebar is created, my suggestion is that we should be able to adjust fetchDerivedMessages so that it fetches the latest edit of the SIDEBAR_SOURCE rather than the original SIDEBAR_SOURCE. This message should look like the original SIDEBAR_SOURCE (eg. a TEXT message), but with the content replaced based on the EDIT_MESSAGE.

Does that make sense? Do you think this is possible?

• kuba mentioned this in D7140: [lib] Displaying edited content in a pending thread.Mar 28 2023, 2:42 AM
• kuba updated this revision to Diff 24268.Mar 28 2023, 2:56 AM

Changed the way of displaying new content of edited message in the sidebar

• kuba retitled this revision from [keyserver] Add the latest edit message to the new sidebar if the source message is edited to [keyserver] Display edited message in the sidebar if the source message is edited ….Mar 28 2023, 2:58 AM
• kuba edited the summary of this revision. (Show Details)
• kuba retitled this revision from [keyserver] Display edited message in the sidebar if the source message is edited … to [keyserver] Display edited message in the sidebar if the source message is edited.
• kuba marked 3 inline comments as done.Mar 28 2023, 3:04 AM
• kuba added inline comments.
keyserver/src/creators/thread-creator.js
457 ↗(On Diff #23989)

This makes sense. I updated the diff (and the other one related to pending threads D7140)

However, there is one issue with it that we have to think through:
On the client side, I cannot determine whether the message has been edited or not. Because of that, I can't display the Edited label properly for the SIDEBAR_SOURCE message.

I believe, that we can leave it like this because when we create the sidebar, we can say that the first message has not been (yet) edited. If we edit the message after the sidebar creation, the label displays correctly.

Harbormaster completed remote builds in B17732: Diff 24268.Mar 28 2023, 3:12 AM
ashoat accepted this revision.Mar 28 2023, 5:58 AM

Perfect!

keyserver/src/creators/thread-creator.js
457 ↗(On Diff #23989)

I agree, I think is a reasonable approach. Thanks for bringing it to my attention!

keyserver/src/fetchers/message-fetchers.js
766 ↗(On Diff #24268)

We could potentially implement this by calling fetchLatestEditMessageContentByIDs... I don't feel strongly, though

This revision is now accepted and ready to land.Mar 28 2023, 5:58 AM
• kuba edited the test plan for this revision. (Show Details)Mar 29 2023, 4:11 AM
• kuba updated this revision to Diff 24321.Mar 29 2023, 4:13 AM
• kuba marked 2 inline comments as done.

Removed redundant SQL query, used fetchLatestEditMessageContentByIDs in fetchLatestEditMessageContentByID

Harbormaster completed remote builds in B17774: Diff 24321.Mar 29 2023, 4:54 AM
Closed by commit rCOMM18211251deb0: [keyserver] Display edited message in the sidebar if the source message is… (authored by kosmydel <kukubaczek@gmail.com>). · Explain WhyMar 31 2023, 2:50 AM
This revision was automatically updated to reflect the committed changes.
kosmydel <kukubaczek@gmail.com> added a commit: rCOMM18211251deb0: [keyserver] Display edited message in the sidebar if the source message is….
• kuba mentioned this in D7301: [native] Display edited message content in the pending threads.Apr 12 2023, 1:36 AM

Revision Contents
Changeset List

PathSize
keyserver/
src/
creators/
20 lines
fetchers/
57 lines
lib/
types/
4 lines

Diff 24321

View Options

keyserver/src/creators/thread-creator.js

Loading...
View Options

keyserver/src/fetchers/message-fetchers.js

Loading...
View Options

lib/types/message-types.js

Loading...