Paths

Table of Contentst

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)
	Sun, Dec 22, 6:49 AM

	Unknown Object (File)
	Sat, Dec 14, 7:02 AM

	Unknown Object (File)
	Fri, Dec 13, 6:11 PM

	Unknown Object (File)
	Tue, Dec 10, 12:04 AM

	Unknown Object (File)
	Sat, Dec 7, 11:38 PM

	Unknown Object (File)
	Fri, Dec 6, 9:51 PM

	Unknown Object (File)
	Thu, Dec 5, 1:56 AM

	Unknown Object (File)
	Thu, Dec 5, 1:56 AM

Subscribers

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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

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

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`

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

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

"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

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 ↗	(On Diff #24386)	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

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

Path

Size

lib/

selectors/

chat-selectors.js

39 lines

Diff 24469

lib/selectors/chat-selectors.js

Loading...