Details

Reviewers

atul
ginsu
tomek
inka

Commits

rCOMM6bda99d0eafd: [web] Display the pinned messages for the thread when the banner is clicked

Summary

The pinned messages modal for the thread should display when the user clicks on the banner indicating the number of pinned messages for that given thread. Each message should display the inline engagement (reactions and replies), based on the designs. This is different from the message being displayed in the toggle pin modal, as the designs there indicated that we wanted to remove the inline engagement.

Depends on D7310

Linear: https://linear.app/comm/issue/ENG-3449/display-the-pinned-messages-for-the-thread-when-the-banner-is-clicked

There is a follow-up task to help refactor usability for the messages search screen here: https://linear.app/comm/issue/ENG-3644/refactor-file-names-details-to-make-the-thread-pin-modal-reusable-by

Test Plan

Pin several messages, with / without inline engagement and a mix between text and media. Confirm that the media gallery looks as expected.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Apr 11 2023, 5:47 AM

Herald added a subscriber: ashoat. · View Herald TranscriptApr 11 2023, 5:47 AM

rohan edited the summary of this revision. (Show Details)Apr 11 2023, 5:48 AM

rohan added a parent revision: D7310: [web] Display the number of pinned messages in a banner across chat.

rohan edited the summary of this revision. (Show Details)

rohan edited the test plan for this revision. (Show Details)Apr 11 2023, 5:57 AM

rohan attached a referenced file: F478961: Screen Recording 2023-04-11 at 8.57.23 AM.mov. (Show Details)

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 11 2023, 6:02 AM

Harbormaster failed remote builds in B18228: Diff 24964!

Not blocking review while I figure out a failed test in D7309

rohan added a child revision: D7382: [web] Allow interaction with inline engagement from the thread pinned messages modal.Apr 11 2023, 6:09 AM

rohan edited the summary of this revision. (Show Details)Apr 11 2023, 2:48 PM

rohan added a reviewer: inka.

tomek requested changes to this revision.Apr 12 2023, 5:34 AM

tomek added inline comments.

web/chat/thread-top-bar.react.js
40–42 ↗	(On Diff #24964)	As noted in D7309 - it would be great if we could avoid this.
web/modals/chat/thread-pinned-messages-modal.css
2–6 ↗	(On Diff #24964)	I think it is more common to use border instead of background to render a line
6 ↗	(On Diff #24964)	Can we use a const?
web/modals/chat/thread-pinned-messages-modal.react.js
33 ↗	(On Diff #24964)	Why do we need this invariant?
34–35 ↗	(On Diff #24964)	A similar code can be found in D7310 - can we avoid duplicating it? Also `singleOrPlural` isn't a great name... maybe something like `pluralizedMessage` or `messageNoun` might work better?
39 ↗	(On Diff #24964)	Can we use `dispatchActionPromise` for this? Without this it will be hard to e.g. show a loading state.
44 ↗	(On Diff #24964)	What `translated` means in this context?
47–50 ↗	(On Diff #24964)	You can use a shorthand
66 ↗	(On Diff #24964)	Why do we need to check this?
74–82 ↗	(On Diff #24964)	This might be really inefficient - for all the messages we iterate through every message `O(n^2)`. We can e.g. create an object/map which contains id => messageInfo mappings and could be used instead of `findIndex` call.
88–94 ↗	(On Diff #24964)	This might be even `O(n^2logn)` - can we create a mapping from id to an index and use it instead of `findIndex`?
100–123 ↗	(On Diff #24964)	Use a shorthand
108–120 ↗	(On Diff #24964)	This looks really similar to D7309 - can we reuse?
125–135 ↗	(On Diff #24964)

This revision now requires changes to proceed.Apr 12 2023, 5:34 AM

By the way - on the local environment fetching messages is fast. On the prod it might be a lot slower - should we display a spinner or something? How would the UI look if messages are being fetched?

Also, do we plan to fetch all the pinned messages at once or fetch them incrementally while the user scrolls? I guess incremental approach would be nice, but might increase the complexity and it's not too likely to have a lot of pinned messages.

On the prod it might be a lot slower - should we display a spinner or something? How would the UI look if messages are being fetched?

Should be covered in designs – I think the default behavior is to show a spinner at the bottom of the list, so if there is nothing fetched yet that spinner should be the only thing in the list. Worth checking with Ted though

rohan added inline comments.Apr 12 2023, 11:39 PM

web/chat/thread-top-bar.react.js
40–42 ↗	(On Diff #24964)	Responded to your comment in D7309!
web/modals/chat/thread-pinned-messages-modal.react.js
34–35 ↗	(On Diff #24964)	Yeah, I agree that we can avoid duplicating it. In terms of making this code more reusable for the message search modal, I'm removing the hard coded modalName from the component in the revision, since the modal name should be different for the search modal.
44 ↗	(On Diff #24964)	I used `translated` for 'converted' or 'changed', since we created a `MessageInfo` from a RawMessageInfo`, but happy to rename if needed
108–120 ↗	(On Diff #24964)	It's similar but not identical since here we want to render the inline engagement, so we maintain the properties that were defaulted to `false` and `{}` in D7309. I can look into seeing if I can somehow unify this creation though to reduce repeated logic

ESLint + Address Feedback

In D7380#220210, @tomek wrote:

By the way - on the local environment fetching messages is fast. On the prod it might be a lot slower - should we display a spinner or something? How would the UI look if messages are being fetched?

Yeah, I think the behavior should follow similar to how the loader displays when fetching pinned messages

Also, do we plan to fetch all the pinned messages at once or fetch them incrementally while the user scrolls? I guess incremental approach would be nice, but might increase the complexity and it's not too likely to have a lot of pinned messages.

At the moment there's no pagination in the query, but there's a task for this: https://linear.app/comm/issue/ENG-3397/add-pagination-to-fetching-message-pins-for-a-thread

web/modals/chat/thread-pinned-messages-modal.react.js
39 ↗	(On Diff #24964)	Sure makes sense, I'll start working on that locally

Harbormaster failed remote builds in B18297: Diff 25075!Apr 12 2023, 11:57 PM

tomek accepted this revision.Apr 13 2023, 3:32 AM

tomek added inline comments.

web/modals/chat/thread-pinned-messages-modal.react.js
68–73 ↗	(On Diff #25075)	This is a strange pattern - a result of `map` should be used somehow and not ignored. Also, a function used in `map` should be pure. If we're calling this just for its side effects, we can use e.g. `forEach`. If we want to keep the `map`, we can create an array of key-value pairs and use it as `Map`'s constructor argument.
75–77 ↗	(On Diff #25075)	This comment is slightly misleading - we're not calling `sort` here
39 ↗	(On Diff #24964)	Are you going to update this diff with it or create a new one? This change should be simple and updating this one probably makes more sense.

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

rohan added inline comments.Apr 14 2023, 11:10 AM

web/modals/chat/thread-pinned-messages-modal.react.js
68–73 ↗	(On Diff #25075)	Makes sense, I'll consider which one makes the most sense and go with that
75–77 ↗	(On Diff #25075)	Thanks for catching that, I forgot to update that comment
39 ↗	(On Diff #24964)	Yeah I'm going to update this diff

Rebase for passing tests and address feedback to use dispatchActionPromise

Harbormaster completed remote builds in B18633: Diff 25523.Apr 21 2023, 5:05 AM

Replace invariant that is not needed

Harbormaster completed remote builds in B18636: Diff 25526.Apr 21 2023, 5:46 AM

Rename ThreadPinnedMessagesModal -> MessageResultsModal

Harbormaster completed remote builds in B18645: Diff 25536.Apr 21 2023, 8:00 AM

General refactoring to make the modal more reusable (first run-through)

Harbormaster completed remote builds in B18778: Diff 25727.Apr 25 2023, 4:44 PM

Rebase

Harbormaster completed remote builds in B18782: Diff 25731.Apr 25 2023, 5:50 PM

Closed by commit rCOMM6bda99d0eafd: [web] Display the pinned messages for the thread when the banner is clicked (authored by RohanK6 <rohan.khanderia@gmail.com>, committed by rohan). · Explain WhyApr 25 2023, 6:00 PM

This revision was automatically updated to reflect the committed changes.

rohan added a commit: rCOMM6bda99d0eafd: [web] Display the pinned messages for the thread when the banner is clicked.

rohan mentioned this in D7673: [native] Display the pinned messages for the thread in the new screen.Apr 27 2023, 11:21 AM

rohan mentioned this in rCOMMdb4d128b7604: [native] Display the pinned messages for the thread in the new screen.May 8 2023, 12:38 PM

Diff 25731

View Options

lib/actions/message-actions.js

This file was added.

				// @flow

				import invariant from 'invariant';
				import Alert from 'react-native/Libraries/Alert/Alert';

				import {
				sendReactionMessage,
				sendReactionMessageActionTypes,
				} from 'lib/actions/message-actions';
				import type { BindServerCall, DispatchFunctions } from 'lib/utils/action-utils';

				import type { TooltipRoute } from '../navigation/tooltip.react';

				function onPressReact(
				route:
				\| TooltipRoute<'TextMessageTooltipModal'>
				\| TooltipRoute<'MultimediaMessageTooltipModal'>,
				dispatchFunctions: DispatchFunctions,
				bindServerCall: BindServerCall,
				) {
				const messageID = route.params.item.messageInfo.id;
				const threadID = route.params.item.threadInfo.id;
				tomekUnsubmitted Not Done Inline Actions This approach seems to be overly complicated. Can't we just write a function that receives `messageID`, `threadID` and `reaction`? This function can be named after an action that is performed, so `react` or `sendReaction`. tomek: This approach seems to be overly complicated. Can't we just write a function that receives…

				invariant(messageID, 'messageID should be set');
				invariant(threadID, 'threadID should be set');

				const callSendReactionMessage = bindServerCall(sendReactionMessage);

				const reactionMessagePromise = (async () => {
				try {
				const result = await callSendReactionMessage(threadID, messageID, '👍');
				return {
				targetMessageID: messageID,
				serverID: result.id,
				threadID,
				time: result.newMessageInfo.time,
				interface: result.interface,
				newMessageInfos: [result.newMessageInfo],
				};
				} catch (e) {
				Alert.alert(
				tomekUnsubmitted Not Done Inline Actions It sounds risky to show one alert after every failed message. A user might react to a lot of messages in short period and a network issue could result in a lot of alerts being shown. Other apps sometimes choose to silently fail when a reaction fails. tomek: It sounds risky to show one alert after every failed message. A user might react to a lot of…
				ginsuAuthorUnsubmitted Done Inline Actions Totally see your concern here! I spoke to @atul about this today, and we both agreed that we want this to be a temporary solution, but since reaction messages don't have retry behavior we think it is best to show some sort of feedback to the user in case something failed. ginsu: Totally see your concern here! I spoke to @atul about this today, and we both agreed that we…
				'Couldn’t send the reaction',
				'Please try again later',
				[{ text: 'OK' }],
				{
				cancelable: false,
				tomekUnsubmitted Not Done Inline Actions Is there a reason to set cancelable to false? tomek: Is there a reason to set cancelable to false?
				ginsuAuthorUnsubmitted Done Inline Actions Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by default and can be enabled by providing an optional Options parameter with the cancelable property set to true. Here is more context I also found the alert on report-utils to follow the same patern ginsu: Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by…
				},
				);
				throw e;
				}
				})();

				dispatchFunctions.dispatchActionPromise(
				sendReactionMessageActionTypes,
				reactionMessagePromise,
				);
				}

				export { onPressReact };

View Options

lib/types/redux-types.js

This file was added.

				// @flow

				import invariant from 'invariant';
				import Alert from 'react-native/Libraries/Alert/Alert';

				import {
				sendReactionMessage,
				sendReactionMessageActionTypes,
				} from 'lib/actions/message-actions';
				import type { BindServerCall, DispatchFunctions } from 'lib/utils/action-utils';

				import type { TooltipRoute } from '../navigation/tooltip.react';

				function onPressReact(
				route:
				\| TooltipRoute<'TextMessageTooltipModal'>
				\| TooltipRoute<'MultimediaMessageTooltipModal'>,
				dispatchFunctions: DispatchFunctions,
				bindServerCall: BindServerCall,
				) {
				const messageID = route.params.item.messageInfo.id;
				const threadID = route.params.item.threadInfo.id;
				tomekUnsubmitted Not Done Inline Actions This approach seems to be overly complicated. Can't we just write a function that receives `messageID`, `threadID` and `reaction`? This function can be named after an action that is performed, so `react` or `sendReaction`. tomek: This approach seems to be overly complicated. Can't we just write a function that receives…

				invariant(messageID, 'messageID should be set');
				invariant(threadID, 'threadID should be set');

				const callSendReactionMessage = bindServerCall(sendReactionMessage);

				const reactionMessagePromise = (async () => {
				try {
				const result = await callSendReactionMessage(threadID, messageID, '👍');
				return {
				targetMessageID: messageID,
				serverID: result.id,
				threadID,
				time: result.newMessageInfo.time,
				interface: result.interface,
				newMessageInfos: [result.newMessageInfo],
				};
				} catch (e) {
				Alert.alert(
				tomekUnsubmitted Not Done Inline Actions It sounds risky to show one alert after every failed message. A user might react to a lot of messages in short period and a network issue could result in a lot of alerts being shown. Other apps sometimes choose to silently fail when a reaction fails. tomek: It sounds risky to show one alert after every failed message. A user might react to a lot of…
				ginsuAuthorUnsubmitted Done Inline Actions Totally see your concern here! I spoke to @atul about this today, and we both agreed that we want this to be a temporary solution, but since reaction messages don't have retry behavior we think it is best to show some sort of feedback to the user in case something failed. ginsu: Totally see your concern here! I spoke to @atul about this today, and we both agreed that we…
				'Couldn’t send the reaction',
				'Please try again later',
				[{ text: 'OK' }],
				{
				cancelable: false,
				tomekUnsubmitted Not Done Inline Actions Is there a reason to set cancelable to false? tomek: Is there a reason to set cancelable to false?
				ginsuAuthorUnsubmitted Done Inline Actions Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by default and can be enabled by providing an optional Options parameter with the cancelable property set to true. Here is more context I also found the alert on report-utils to follow the same patern ginsu: Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by…
				},
				);
				throw e;
				}
				})();

				dispatchFunctions.dispatchActionPromise(
				sendReactionMessageActionTypes,
				reactionMessagePromise,
				);
				}

				export { onPressReact };

View Options

web/chat/thread-top-bar.react.js

This file was added.

				// @flow

				import invariant from 'invariant';
				import Alert from 'react-native/Libraries/Alert/Alert';

				import {
				sendReactionMessage,
				sendReactionMessageActionTypes,
				} from 'lib/actions/message-actions';
				import type { BindServerCall, DispatchFunctions } from 'lib/utils/action-utils';

				import type { TooltipRoute } from '../navigation/tooltip.react';

				function onPressReact(
				route:
				\| TooltipRoute<'TextMessageTooltipModal'>
				\| TooltipRoute<'MultimediaMessageTooltipModal'>,
				dispatchFunctions: DispatchFunctions,
				bindServerCall: BindServerCall,
				) {
				const messageID = route.params.item.messageInfo.id;
				const threadID = route.params.item.threadInfo.id;
				tomekUnsubmitted Not Done Inline Actions This approach seems to be overly complicated. Can't we just write a function that receives `messageID`, `threadID` and `reaction`? This function can be named after an action that is performed, so `react` or `sendReaction`. tomek: This approach seems to be overly complicated. Can't we just write a function that receives…

				invariant(messageID, 'messageID should be set');
				invariant(threadID, 'threadID should be set');

				const callSendReactionMessage = bindServerCall(sendReactionMessage);

				const reactionMessagePromise = (async () => {
				try {
				const result = await callSendReactionMessage(threadID, messageID, '👍');
				return {
				targetMessageID: messageID,
				serverID: result.id,
				threadID,
				time: result.newMessageInfo.time,
				interface: result.interface,
				newMessageInfos: [result.newMessageInfo],
				};
				} catch (e) {
				Alert.alert(
				tomekUnsubmitted Not Done Inline Actions It sounds risky to show one alert after every failed message. A user might react to a lot of messages in short period and a network issue could result in a lot of alerts being shown. Other apps sometimes choose to silently fail when a reaction fails. tomek: It sounds risky to show one alert after every failed message. A user might react to a lot of…
				ginsuAuthorUnsubmitted Done Inline Actions Totally see your concern here! I spoke to @atul about this today, and we both agreed that we want this to be a temporary solution, but since reaction messages don't have retry behavior we think it is best to show some sort of feedback to the user in case something failed. ginsu: Totally see your concern here! I spoke to @atul about this today, and we both agreed that we…
				'Couldn’t send the reaction',
				'Please try again later',
				[{ text: 'OK' }],
				{
				cancelable: false,
				tomekUnsubmitted Not Done Inline Actions Is there a reason to set cancelable to false? tomek: Is there a reason to set cancelable to false?
				ginsuAuthorUnsubmitted Done Inline Actions Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by default and can be enabled by providing an optional Options parameter with the cancelable property set to true. Here is more context I also found the alert on report-utils to follow the same patern ginsu: Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by…
				},
				);
				throw e;
				}
				})();

				dispatchFunctions.dispatchActionPromise(
				sendReactionMessageActionTypes,
				reactionMessagePromise,
				);
				}

				export { onPressReact };

View Options

web/modals/chat/message-results-modal.css

This file was added.

				// @flow

				import invariant from 'invariant';
				import Alert from 'react-native/Libraries/Alert/Alert';

				import {
				sendReactionMessage,
				sendReactionMessageActionTypes,
				} from 'lib/actions/message-actions';
				import type { BindServerCall, DispatchFunctions } from 'lib/utils/action-utils';

				import type { TooltipRoute } from '../navigation/tooltip.react';

				function onPressReact(
				route:
				\| TooltipRoute<'TextMessageTooltipModal'>
				\| TooltipRoute<'MultimediaMessageTooltipModal'>,
				dispatchFunctions: DispatchFunctions,
				bindServerCall: BindServerCall,
				) {
				const messageID = route.params.item.messageInfo.id;
				const threadID = route.params.item.threadInfo.id;
				tomekUnsubmitted Not Done Inline Actions This approach seems to be overly complicated. Can't we just write a function that receives `messageID`, `threadID` and `reaction`? This function can be named after an action that is performed, so `react` or `sendReaction`. tomek: This approach seems to be overly complicated. Can't we just write a function that receives…

				invariant(messageID, 'messageID should be set');
				invariant(threadID, 'threadID should be set');

				const callSendReactionMessage = bindServerCall(sendReactionMessage);

				const reactionMessagePromise = (async () => {
				try {
				const result = await callSendReactionMessage(threadID, messageID, '👍');
				return {
				targetMessageID: messageID,
				serverID: result.id,
				threadID,
				time: result.newMessageInfo.time,
				interface: result.interface,
				newMessageInfos: [result.newMessageInfo],
				};
				} catch (e) {
				Alert.alert(
				tomekUnsubmitted Not Done Inline Actions It sounds risky to show one alert after every failed message. A user might react to a lot of messages in short period and a network issue could result in a lot of alerts being shown. Other apps sometimes choose to silently fail when a reaction fails. tomek: It sounds risky to show one alert after every failed message. A user might react to a lot of…
				ginsuAuthorUnsubmitted Done Inline Actions Totally see your concern here! I spoke to @atul about this today, and we both agreed that we want this to be a temporary solution, but since reaction messages don't have retry behavior we think it is best to show some sort of feedback to the user in case something failed. ginsu: Totally see your concern here! I spoke to @atul about this today, and we both agreed that we…
				'Couldn’t send the reaction',
				'Please try again later',
				[{ text: 'OK' }],
				{
				cancelable: false,
				tomekUnsubmitted Not Done Inline Actions Is there a reason to set cancelable to false? tomek: Is there a reason to set cancelable to false?
				ginsuAuthorUnsubmitted Done Inline Actions Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by default and can be enabled by providing an optional Options parameter with the cancelable property set to true. Here is more context I also found the alert on report-utils to follow the same patern ginsu: Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by…
				},
				);
				throw e;
				}
				})();

				dispatchFunctions.dispatchActionPromise(
				sendReactionMessageActionTypes,
				reactionMessagePromise,
				);
				}

				export { onPressReact };

View Options

web/modals/chat/message-results-modal.react.js

[web] Display the pinned messages for the thread when the banner is clicked
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 25731

lib/actions/message-actions.js

lib/types/redux-types.js

web/chat/thread-top-bar.react.js

web/modals/chat/message-results-modal.css

web/modals/chat/message-results-modal.react.js

[web] Display the pinned messages for the thread when the banner is clickedClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 25731

lib/actions/message-actions.js

lib/types/redux-types.js

web/chat/thread-top-bar.react.js

web/modals/chat/message-results-modal.css

web/modals/chat/message-results-modal.react.js

[web] Display the pinned messages for the thread when the banner is clicked
ClosedPublic
Actions

Revision Contents
Changeset List