Page MenuHomePhabricator
Differential D7140

[lib] Displaying edited content in a pending thread
ClosedPublic
Actions

Authored by • kuba on Mar 22 2023, 5:52 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 4:37 AM
Unknown Object (File)
Fri, Nov 8, 7:38 AM
Unknown Object (File)
Wed, Nov 6, 3:38 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, Nov 4, 11:16 PM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
michal
inka
ginsu
ashoat
Commits
rCOMM108dae894c4e: [lib] Displaying edited content in a pending thread
Summary

When the user wants to create a thread from an edited message, we want him to see the updated content of the message.

Test Plan
  • [web & iOS] edited a message, clicked on it to create a sidebar from it, I can see updated content,
  • [web] on the pending thread route refreshed site, checked how it behaves: it just redirects to the main thread.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• kuba created this revision.Mar 22 2023, 5:52 AM
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptMar 22 2023, 5:52 AM
• kuba added a parent revision: D7139: [lib] Added method to call edit_message endpoint.Mar 22 2023, 5:53 AM
• kuba updated this revision to Diff 23988.Mar 22 2023, 5:56 AM

Reformat

• kuba added a child revision: D7141: [keyserver] Display edited message in the sidebar if the source message is edited.Mar 22 2023, 5:58 AM
Harbormaster completed remote builds in B17519: Diff 23987.Mar 22 2023, 6:08 AM
Harbormaster completed remote builds in B17520: Diff 23988.Mar 22 2023, 6:12 AM
• kuba requested review of this revision.Mar 22 2023, 6:12 AM
ashoat added inline comments.Mar 22 2023, 3:11 PM
lib/selectors/chat-selectors.js
622–623 ↗(On Diff #23988)

These two can be combined to reduce one array pass. Note that the callback you pass to useSelector will be executed any time any data in Redux changes, so it's a good idea to prioritize performance here

(Flow needs the Boolean pass to be separate however)

626 ↗(On Diff #23988)

This isn't exactly identical, but I think it works here

• kuba updated this revision to Diff 24060.Mar 24 2023, 3:09 AM
• kuba marked 2 inline comments as done.

Address comments

Harbormaster completed remote builds in B17572: Diff 24060.Mar 24 2023, 3:24 AM
ashoat added inline comments.Mar 24 2023, 10:52 AM
lib/selectors/chat-selectors.js
624–625 ↗(On Diff #24060)

Personally I think this condition is more clear if you start by checking message.type, since most message types don't have message.targetMessageID

• kuba updated this revision to Diff 24266.Mar 28 2023, 2:42 AM
• kuba marked an inline comment as done.

Removed 'Edited' label from the pending thread to match D7141 changes

• kuba updated this revision to Diff 24267.Mar 28 2023, 2:45 AM

Removed accidentally added empty line

Harbormaster completed remote builds in B17730: Diff 24266.Mar 28 2023, 2:57 AM
Harbormaster completed remote builds in B17731: Diff 24267.Mar 28 2023, 3:00 AM
• kuba mentioned this in D7141: [keyserver] Display edited message in the sidebar if the source message is edited.Mar 28 2023, 3:04 AM
ashoat added inline comments.Mar 28 2023, 5:51 AM
lib/selectors/chat-selectors.js
606–628 ↗(On Diff #24267)

This is a very expensive calculation, since you go through every single message in the MessageStore for this thread

As mentioned above:

Note that the callback you pass to useSelector will be executed any time any data in Redux changes

I think it would be good here to pull this out of the useSelector, and put it in a useMemo below. That way, if the underlying data (containingThread.messageIDs?) doesn't change, then the calculation will not have to be run again

I'm not sure this is possible in this case, but I suspect it is. Can you think of a good useMemo we can pull out of this?

• kuba added inline comments.Mar 30 2023, 2:59 AM
lib/selectors/chat-selectors.js
606–628 ↗(On Diff #24267)

I added it as a follow-up task: https://linear.app/comm/issue/ENG-3563/add-usememo-to-pending-thread-edit-messages

ashoat requested changes to this revision.Mar 30 2023, 4:43 AM

"Add useMemo" isn't a good follow-up task, please respond to the diff comments here directly

This revision now requires changes to proceed.Mar 30 2023, 4:43 AM
• kuba updated this revision to Diff 24386.Mar 30 2023, 6:59 AM

Moved editedText to useMemo

Harbormaster completed remote builds in B17818: Diff 24386.Mar 30 2023, 7:16 AM
ashoat accepted this revision.Mar 30 2023, 2:56 PM
ashoat added inline comments.
lib/selectors/chat-selectors.js
598

In the codebase we typically do import * as React from 'react'; and then do React.useMemo here

This revision is now accepted and ready to land.Mar 30 2023, 2:56 PM
• kuba updated this revision to Diff 24467.Mar 31 2023, 2:25 AM
• kuba marked an inline comment as done.

Changed useMemo usage

Harbormaster completed remote builds in B17863: Diff 24467.Mar 31 2023, 2:40 AM
Closed by commit rCOMM108dae894c4e: [lib] Displaying edited content in a pending thread (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: rCOMM108dae894c4e: [lib] Displaying edited content in a pending thread.
• kuba added a reverting change: D7325: Revert "[lib] Displaying edited content in a pending thread".Apr 5 2023, 12:41 PM
kosmydel <kukubaczek@gmail.com> added a reverting change: rCOMM5751c10d5cf1: Revert "[lib] Displaying edited content in a pending thread".Apr 5 2023, 12:59 PM

Revision Contents
Changeset List

PathSize
lib/
selectors/
40 lines

Diff 24386

View Options

lib/selectors/chat-selectors.js

Loading...