Paths

Table of Contentst

-
lib/selectors/
-
selectors/
1/1
chat-selectors.js
-
native/chat/
-
chat/
1/1
composed-message.react.js
1/1
utils.js
-
web/chat/
-
chat/
1/1
composed-message.react.js

[native] Display edited message content in the pending threads
ClosedPublic
Actions

Authored by • kuba on Apr 4 2023, 11:40 AM.

Details

Reviewers

michal
inka
ginsu

Commits

rCOMM1f331c5ce520: [native] Display edited message content in the pending threads

Summary

~~Previously: [fix] Fix 'Maximum update depth exceeded' on native when opening tooltip~~
Can't open the tooltip menu, because of maximum update depth exceeded warning.
Issue: https://linear.app/comm/issue/ENG-3584/cant-open-tooltips-on-native

Changed the way in which the updated content in the pending thread was displayed. Now, a new edit message is added to the pending thread, instead of modifying the existing one.

Test Plan

Run the app on iOS, open the tooltip menu, it should work. Checked on web if everything still works. Checked if height measurement doesn't display warnings in the console.

Diff Detail

Repository

rCOMM Comm

Branch

kuba/messageediting

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• kuba created this revision.Apr 4 2023, 11:40 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptApr 4 2023, 11:40 AM

Harbormaster completed remote builds in B17980: Diff 24620.Apr 4 2023, 11:57 AM

• kuba requested review of this revision.Apr 4 2023, 11:57 AM

It would be nice if you could explain a bit more precisely what the problem was. From your fix it seems the problem was with a pending thread only? But from the description of the problem it seems that the problem was with opening any tooltip. Does it work in other cases?

Why don't you display the edited label when the thread is pending?

web/chat/composed-message.react.js
128	Hardcoding this label in two places is error-prone. Can you create it in one place and export it?

@kuba has not made it clear, but it appears that this diff solves a push-blocking issue: ENG-3584

If so, and if this diff cannot be landed within a matter of hours, then the offending commit should be immediately reverted.

@kuba, can you rebase this on master so that it includes the un-revert of your reverted diff?

Rebase to master

Address @inka comments

Harbormaster completed remote builds in B18071: Diff 24760.Apr 7 2023, 4:04 AM

New function to display labels

In D7301#217546, @inka wrote:

It would be nice if you could explain a bit more precisely what the problem was. From your fix it seems the problem was with a pending thread only? But from the description of the problem it seems that the problem was with opening any tooltip. Does it work in other cases?

The problem was with changing the content of the messageInfo of the message. If the message content was changed it forced re-rendering and went into an infinite loop. The tooltip just triggered this bug, which froze the whole app. Right now, I changed how edited messages content in pending threads is updated, and prevented from going into an infinite loop.

I tested it (on iOS and the web) and right now it works, I couldn't trigger this same issue again.

Why don't you display the edited label when the thread is pending?

It was discussed here: https://phab.comm.dev/D7141?id=23989#inline-47335

In case, that we created a sidebar from an edited message we can't determine if the source message was edited for the sidebar thread. Therefore, I don't display the Edited label here. To have consistent behavior, I don't display the Edited label in the pending thread as well. Otherwise, it would disappear anyway after sending the message with creates the sidebar.

To make it clear, if we edit the source message after creating the sidebar, the Edit label is displayed as expected (both on the main thread and sidebar).

web/chat/composed-message.react.js
128	I put it into a separate method. I hope that was what you meant.

Harbormaster completed remote builds in B18265: Diff 25026.Apr 12 2023, 1:51 AM

Thank you for explaining

This revision is now accepted and ready to land.Apr 12 2023, 3:02 AM

• kuba added a child revision: D7342: [native] Add EditState to the InputState.Apr 12 2023, 3:18 AM

• kuba retitled this revision from [fix] Fix 'Maximum update depth exceeded' on native when opening tooltip to [native] Display edited message content in the pending threads.Apr 12 2023, 6:02 AM

• kuba edited the summary of this revision. (Show Details)

Closed by commit rCOMM1f331c5ce520: [native] Display edited message content in the pending threads (authored by kosmydel <kukubaczek@gmail.com>). · Explain WhyApr 12 2023, 6:05 AM

This revision was automatically updated to reflect the committed changes.

kosmydel <kukubaczek@gmail.com> added a commit: rCOMM1f331c5ce520: [native] Display edited message content in the pending threads.

Revision Contents
Changeset List

Path

Size

lib/

selectors/

chat-selectors.js

30 lines

native/

chat/

composed-message.react.js

10 lines

utils.js

8 lines

web/

chat/

composed-message.react.js

10 lines

Diff 24620

View Options

lib/selectors/chat-selectors.js

	Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
	const styles = StyleSheet.create({			const styles = StyleSheet.create({
	dummyMessage: {			dummyMessage: {
	paddingHorizontal: 12,			paddingHorizontal: 12,
	paddingVertical: 6,			paddingVertical: 6,
	},			},
	message: {			message: {
	overflow: 'hidden',			overflow: 'hidden',
	paddingHorizontal: 12,			paddingHorizontal: 12,
	paddingVertical: 6,			paddingVertical: 8,
	},			},
	text: {			text: {
	fontFamily: 'Arial',			fontFamily: 'Arial',
	fontSize: 18,			fontSize: 14,
				benschacAuthorUnsubmitted Done Inline Actions I actually think this is a bit too small. But that's what's in the design. https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455 @ashoat - I think 16px looks better. benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
				lineHeight: 22,
	},			},
	});			});

	export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };			export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };

View Options

native/chat/composed-message.react.js

	Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
	const styles = StyleSheet.create({			const styles = StyleSheet.create({
	dummyMessage: {			dummyMessage: {
	paddingHorizontal: 12,			paddingHorizontal: 12,
	paddingVertical: 6,			paddingVertical: 6,
	},			},
	message: {			message: {
	overflow: 'hidden',			overflow: 'hidden',
	paddingHorizontal: 12,			paddingHorizontal: 12,
	paddingVertical: 6,			paddingVertical: 8,
	},			},
	text: {			text: {
	fontFamily: 'Arial',			fontFamily: 'Arial',
	fontSize: 18,			fontSize: 14,
				benschacAuthorUnsubmitted Done Inline Actions I actually think this is a bit too small. But that's what's in the design. https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455 @ashoat - I think 16px looks better. benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
				lineHeight: 22,
	},			},
	});			});

	export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };			export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };

View Options

native/chat/utils.js

	Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
	const styles = StyleSheet.create({			const styles = StyleSheet.create({
	dummyMessage: {			dummyMessage: {
	paddingHorizontal: 12,			paddingHorizontal: 12,
	paddingVertical: 6,			paddingVertical: 6,
	},			},
	message: {			message: {
	overflow: 'hidden',			overflow: 'hidden',
	paddingHorizontal: 12,			paddingHorizontal: 12,
	paddingVertical: 6,			paddingVertical: 8,
	},			},
	text: {			text: {
	fontFamily: 'Arial',			fontFamily: 'Arial',
	fontSize: 18,			fontSize: 14,
				benschacAuthorUnsubmitted Done Inline Actions I actually think this is a bit too small. But that's what's in the design. https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455 @ashoat - I think 16px looks better. benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
				lineHeight: 22,
	},			},
	});			});

	export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };			export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };

View Options

web/chat/composed-message.react.js

	Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
	const styles = StyleSheet.create({			const styles = StyleSheet.create({
	dummyMessage: {			dummyMessage: {
	paddingHorizontal: 12,			paddingHorizontal: 12,
	paddingVertical: 6,			paddingVertical: 6,
	},			},
	message: {			message: {
	overflow: 'hidden',			overflow: 'hidden',
	paddingHorizontal: 12,			paddingHorizontal: 12,
	paddingVertical: 6,			paddingVertical: 8,
	},			},
	text: {			text: {
	fontFamily: 'Arial',			fontFamily: 'Arial',
	fontSize: 18,			fontSize: 14,
				benschacAuthorUnsubmitted Done Inline Actions I actually think this is a bit too small. But that's what's in the design. https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455 @ashoat - I think 16px looks better. benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
				lineHeight: 22,
	},			},
	});			});

	export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };			export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };

[native] Display edited message content in the pending threadsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 24620

lib/selectors/chat-selectors.js

native/chat/composed-message.react.js

native/chat/utils.js

web/chat/composed-message.react.js

[native] Display edited message content in the pending threads
ClosedPublic
Actions

Revision Contents
Changeset List