feat: Jump to last message WPB-6518 #17386

svitovyda · 2024-05-10T15:40:02Z

WPB-6518 [Web] Jump to last message in a conversation

Description

By the product/design team request, the user should jump to the very last message in the conversation, marking all the messages as read

Screenshots/Screencast (for UI changes)

Screen.Recording.2024-06-04.at.11.26.54.mov

Screen.Recording.2024-06-04.at.11.28.01.mov

Screen.Recording.2024-06-04.at.11.30.25.mov

Checklist

PR has been self reviewed by the author;
Hard-to-understand areas of the code have been commented;
If it is a core feature, unit tests have been added;

CLAassistant · 2024-05-10T15:40:07Z

All committers have signed the CLA.

src/script/components/LastMessageTracker/LastMessageTracker.tsx

src/script/components/MessagesList/Message/index.tsx

src/script/components/Conversation/Conversation.tsx

src/script/components/MessagesList/MessageList.tsx

codecov-commenter · 2024-05-29T21:21:15Z

Codecov Report

Attention: Patch coverage is 40.00000% with 39 lines in your changes missing coverage. Please review.

Project coverage is 46.29%. Comparing base (c0e7374) to head (10cf84d).
Report is 2 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev   #17386      +/-   ##
==========================================
+ Coverage   46.26%   46.29%   +0.03%     
==========================================
  Files         756      758       +2     
  Lines       24859    24893      +34     
  Branches     5700     5710      +10     
==========================================
+ Hits        11501    11525      +24     
- Misses      11916    11927      +11     
+ Partials     1442     1441       -1

setupTests.js

src/script/components/Conversation/Conversation.tsx

aweiss-dev · 2024-05-30T10:09:01Z

src/script/view_model/ContentViewModel.ts

@@ -239,7 +239,7 @@ export class ContentViewModel {
      exposeMessage: exposeMessageEntity,
      openFirstSelfMention = false,
      openNotificationSettings = false,
-    } = options;
+    } = options || {};


Options are not supposed to be null | undefined, what is this change for?

Our TS config allowed me to use this function without providing this parameter and I got an exception. And actually, in many cases we provide just {}, so I'd rather make options optional and remove those {}, WDYT?

That would be the preferred way to act on this issue. If it can be undefined, it needs to be typed as such 👍

src/script/components/Conversation/Conversation.tsx

atomrc · 2024-05-30T10:31:35Z

src/script/entity/Conversation.ts

@@ -163,6 +167,7 @@ export class Conversation {
  public readonly receiptMode: ko.Observable<RECEIPT_MODE>;
  public readonly removed_from_conversation: ko.PureComputed<boolean>;
  public readonly roles: ko.Observable<Record<string, string>>;
+  public readonly isLastMessageVisible: ko.Observable<boolean>;


This should not belong in the low level entity. Should be a view-only thing.
It should leave in Conversation.tsx only, I believe

It was my idea too, but then we should pass it to the MessageList to implement its updating, or pull all the updating logic into the Conversation, chaining props of onVisible and onVisibilityLost. It was quite ugly implementation. I also thought that some of the props and methods of ConversationEntity might not be in the perfect place (like initialMessage), and maybe we need a ConversationView service, WDYT?

I believe we do need a cleaner architecture for those values, yes. But maybe it's not the correct PR to tackle this. Probably a follow up PR would be nice.

src/script/components/Conversation/Conversation.tsx

svitovyda · 2024-06-03T17:02:00Z

src/script/components/MessagesList/MessageList.tsx

+  const jumpToLastMessage = () => {
+    if (conversation) {
+      // clean up anything like search result
+      setHighlightedMessage(undefined);


It feels that this cleanup should happen somewhere in the useEffect, when props and observables change, but I'm afraid to break something this way

svitovyda · 2024-06-03T17:05:28Z

src/script/components/MessagesList/MessageList.tsx

+  );
+};
+
+export interface JumpToLastMessageButtonProps extends HTMLProps<HTMLElement> {


we agreed to keep this component inside of MessageList.tsx as it contains positioning CSS and we didn't want to overcomplicate it by passing the custom CSS. It still should be a separate component with it's own state, as otherwise isLastMessageVisible will make the entire MessageList to re-render, unless we will implement memoisation of each element rendered.

export is for the test

I think You can move it outside MessageList, and use - memo(Component) with validating props :)

there were quite some issues with passing the css, we moved it back and forth a few times already. I'd leave it as is for now

svitovyda · 2024-06-03T17:06:51Z

src/script/components/MessagesList/MessageList.tsx

+  const [isLastMessageVisible, setIsLastMessageVisible] = useState(conversation.isLastMessageVisible());
+
+  useEffect(() => {
+    const subscription = conversation.isLastMessageVisible.subscribe(


I can't use here useKoSubscribableChildren as I need to add debouncing to the change of the state.

You can do debounce with useCallback component, then You can use useKoSubscribableChildren :)

You can do this smth like that:

const {isLastMessageVisible} = useKoSubscribableChildren(conversation, ['isLastMessageVisible']); // eslint-disable-next-line react-hooks/exhaustive-deps const debounced = useCallback( debounce((value: boolean) => value, DEBOUNCE_TIME), [], ); if (!debounced(isLastMessageVisible)) { return null; } return ...

I tried it, but there is also one more thing - the initial value of isLastMessageVisible should be set by the last fired value. Just waiting for the new event and debouncing makes it behave really weird. I suggest to keep my existing implementation

svitovyda · 2024-06-03T17:13:16Z

src/script/components/MessagesList/MessageList.tsx

+
+  useEffect(() => {
+    const subscription = conversation.isLastMessageVisible.subscribe(
+      debounce(value => {


debouncing is needed for messages, sent by self: at first the message from self gets mounted as not yet delivered, then it gets unmounted and mounted as received from the server. this makes the jump to the last message button blink. there is a complicated way to check if a message that is unmounted (its onVisibilityLost was called) has not yet been delivered, but we should somehow distinguish whether it was unmounted and not hidden by scrolling. I think debouncing is better, while it does add a bit of noticeable delay even with 200 ms

svitovyda · 2024-06-03T17:23:58Z

src/script/components/utils/InViewport.tsx

@@ -25,6 +25,7 @@ import {viewportObserver} from 'Util/DOM/viewportObserver';
 interface InViewportParams {
  onVisible: () => void;


in the current implementation, this callback will be called when most of the element is visible. and we need it this way to mark the message as read. if I reuse this callback, the button "jump to the last message" will be disappearing slightly different from how it behaves on our android app - not when a bit of the last message appears, but when the big chunk of it is visible. @atomrc suggested we leave it as it is. the other option would be to implement one callback onPartialVisibilityChange(visible: boolean) instead of onVisibilityLost() and call it with true differently than onVisible (as onVisible is used in many places, not only for messages)

przemvs · 2024-06-04T09:37:07Z

src/script/components/MessagesList/MessageList.styles.ts

+export const jumpToLastMessageButtonStyles: (mdBreakpoint: boolean) => CSSObject = (mdBreakpoint: boolean) => ({
+  position: 'absolute',
+  bottom: mdBreakpoint ? '100px' : '56px',


You can do smth like that:

Suggested change

export const jumpToLastMessageButtonStyles: (mdBreakpoint: boolean) => CSSObject = (mdBreakpoint: boolean) => ({

position: 'absolute',

bottom: mdBreakpoint ? '100px' : '56px',

export const jumpToLastMessageButtonStyles: (mdBreakpoint: boolean) => CSSObject = (mdBreakpoint: boolean) => ({

position: 'absolute',

bottom: '56px',

'@media (max-width: @screen-md-min)': {

bottom: '100px',

}

Looks like media queries don't accept less variables, so I just hardcoded 768px

przemvs · 2024-06-04T09:43:21Z

src/script/components/MessagesList/MessageList.tsx

+  conversation: Conversation;
+}
+
+export const JumpToLastMessageButton: FC<JumpToLastMessageButtonProps> = ({


You don't need to pass FC<..> here, You have already typed on line 379 :)

przemvs · 2024-06-04T09:46:39Z

src/script/components/MessagesList/MessageList.tsx

+      onClick={onGoToLastMessage}
+      css={jumpToLastMessageButtonStyles(mdBreakpoint)}
+    >
+      <ChevronIcon css={{rotate: '90deg', height: 16, width: 16, path: {fill: '#0667C8'}}} />


Please use variables for fill :)

where should I add it? this is a completely new color AFAIK, we don't have it in our ui-kit

Let me check, a new color is always a red flag

Suggested change

<ChevronIcon css={{rotate: '90deg', height: 16, width: 16, path: {fill: '#0667C8'}}} />

<ChevronIcon css={{rotate: '90deg', height: 16, width: 16, path: {fill: 'var(--accent-color)'}}} />

przemvs · 2024-06-04T09:48:27Z

src/script/entity/Conversation.ts

+export const isLastReceivedMessage = (messageEntity: Message, conversationEntity: Conversation): boolean => {
+  return messageEntity.timestamp() >= conversationEntity.last_event_timestamp();
+};


Why not to use hasLastReceivedMessageLoaded, You have declared it in this Class ;)

because in this case, I need to identify if the given (rendered) message is the last message, to add to it onVisible and onVisibilityLost. but in many cases it can be replaced with hasLastReceivedMessageLoaded - I'm planning to do the refactoring of it later, within the bug that I discovered. here I just wanted to move it out of React component, as a first step.

So please move this from Class :) We have some utils/helpers for cases like this one :)

done. but I think I will soon move it back as a method :)

…sage; Mark all the conversation as read on the "jump to last message" button click; Reload the conversation on the "jump to last message" click if the latest conversation messages were not load;

Add re-render of message list on click on button when last message is loaded;

Make isLastReceivedMessage independent function and use it for last message visibility; Fix tests;

…render; Add comments for jump to the last message function; Make options optional in `showConversation`; Fix CSS for the button and disable eslint for the WithConditionalCSSProp impert;

Make JumpToLastMessageButton part of MessageList file to not let it be used outside; Use debounce to avoid too often button visibility change; Make onVisibilityLost call on component unmount configurable;

…essages batch

Initial message is a static property that is read only when a conversation is loaded. Once loaded this value should not be read again and should not trigger re-rendering

Fix responsive positioning of the button;

Move isLastReceivedMessage to utils; Move all CSS for button into the styles file;

sonarcloud · 2024-06-04T18:21:51Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

otto-the-bot added type: feature / request ✨ 👕 size: M labels May 10, 2024

svitovyda commented May 10, 2024

View reviewed changes

src/script/components/LastMessageTracker/LastMessageTracker.tsx Outdated Show resolved Hide resolved

svitovyda commented May 10, 2024

View reviewed changes

src/script/components/MessagesList/Message/index.tsx Outdated Show resolved Hide resolved

svitovyda force-pushed the feat/WPB-6518-jump_to_last_message branch 2 times, most recently from caec31b to 691c0bc Compare May 16, 2024 12:26

svitovyda force-pushed the feat/WPB-6518-jump_to_last_message branch 3 times, most recently from 4e632c1 to 36006ba Compare May 29, 2024 20:32

svitovyda commented May 29, 2024

View reviewed changes

src/script/components/Conversation/Conversation.tsx Outdated Show resolved Hide resolved

svitovyda commented May 29, 2024

View reviewed changes

src/script/components/MessagesList/MessageList.tsx Show resolved Hide resolved

svitovyda commented May 29, 2024

View reviewed changes

src/script/components/MessagesList/MessageList.tsx Show resolved Hide resolved

svitovyda force-pushed the feat/WPB-6518-jump_to_last_message branch 2 times, most recently from a07a09f to 1672892 Compare May 29, 2024 20:50

svitovyda marked this pull request as ready for review May 29, 2024 20:51

svitovyda requested review from otto-the-bot and a team as code owners May 29, 2024 20:51

svitovyda force-pushed the feat/WPB-6518-jump_to_last_message branch 3 times, most recently from 678de8a to 21d2a75 Compare May 29, 2024 21:16

svitovyda commented May 29, 2024

View reviewed changes

setupTests.js Show resolved Hide resolved

svitovyda commented May 29, 2024

View reviewed changes

src/script/components/Conversation/Conversation.tsx Outdated Show resolved Hide resolved

aweiss-dev reviewed May 30, 2024

View reviewed changes

atomrc reviewed May 30, 2024

View reviewed changes

src/script/components/Conversation/Conversation.tsx Outdated Show resolved Hide resolved

atomrc reviewed May 30, 2024

View reviewed changes

src/script/components/Conversation/Conversation.tsx Outdated Show resolved Hide resolved

atomrc reviewed May 30, 2024

View reviewed changes

src/script/components/Conversation/Conversation.tsx Outdated Show resolved Hide resolved

atomrc reviewed May 30, 2024

View reviewed changes

src/script/components/Conversation/Conversation.tsx Outdated Show resolved Hide resolved

otto-the-bot added the 👕 size: L label Jun 3, 2024

svitovyda commented Jun 3, 2024

View reviewed changes

svitovyda force-pushed the feat/WPB-6518-jump_to_last_message branch from 0d40cd6 to 450a985 Compare June 4, 2024 08:56

przemvs reviewed Jun 4, 2024

View reviewed changes

svitovyda and others added 15 commits June 4, 2024 15:58

feat: Add "Jump to las message" button WPB-6518

9cdba8b

Show/hide "jump to last message" button on visibility of the last mes…

a376465

…sage; Mark all the conversation as read on the "jump to last message" button click; Reload the conversation on the "jump to last message" click if the latest conversation messages were not load;

Fix the way last message visibility is tracked;

12f4b3d

Add re-render of message list on click on button when last message is loaded;

runfix: add breakpoint to bottom positioning (#17483)

e0167f6

Fix the way conversation is marked as read;

2143685

Make isLastReceivedMessage independent function and use it for last message visibility; Fix tests;

Move JumpToLastMessageButton into a separate file

60cffd0

Add comment for a non-trivial usage of key to enforce component re-…

b0b8687

…render; Add comments for jump to the last message function; Make options optional in `showConversation`; Fix CSS for the button and disable eslint for the WithConditionalCSSProp impert;

Move jump to last message into MessageList;

d901309

Make JumpToLastMessageButton part of MessageList file to not let it be used outside; Use debounce to avoid too often button visibility change; Make onVisibilityLost call on component unmount configurable;

Fix behavior for the corner case of search result being in the last m…

9f8eebe

…essages batch

Add test for JumpToLastMessageButton, fix MessageList test

e816ba3

fix: Avoid listening to initialMessage change.

702115d

Initial message is a static property that is read only when a conversation is loaded. Once loaded this value should not be read again and should not trigger re-rendering

fixup! fix: Avoid listening to initialMessage change.

531f01b

Final fix of jump to last message animation

fdf3ae4

Extract styles into separate file

53c99e1

Fix the chewron color in jump to last message button;

3f2f8cf

Fix responsive positioning of the button;

svitovyda force-pushed the feat/WPB-6518-jump_to_last_message branch from 93706c5 to 3f2f8cf Compare June 4, 2024 13:58

Move JumpToLastMessageButton to separate file;

10cf84d

Move isLastReceivedMessage to utils; Move all CSS for button into the styles file;

svitovyda requested a review from przemvs June 4, 2024 18:26

		@@ -25,6 +25,7 @@ import {viewportObserver} from 'Util/DOM/viewportObserver';
		interface InViewportParams {
		onVisible: () => void;

	<ChevronIcon css={{rotate: '90deg', height: 16, width: 16, path: {fill: '#0667C8'}}} />
	<ChevronIcon css={{rotate: '90deg', height: 16, width: 16, path: {fill: 'var(--accent-color)'}}} />

feat: Jump to last message WPB-6518 #17386

Are you sure you want to change the base?

feat: Jump to last message WPB-6518 #17386

Conversation

svitovyda commented May 10, 2024 • edited by github-actions bot

Description

Screenshots/Screencast (for UI changes)

Checklist

CLAassistant commented May 10, 2024 • edited

codecov-commenter commented May 29, 2024 • edited

Codecov Report

aweiss-dev May 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svitovyda Jun 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 4, 2024

Quality Gate passed

svitovyda commented May 10, 2024 •

edited by github-actions bot

CLAassistant commented May 10, 2024 •

edited

codecov-commenter commented May 29, 2024 •

edited

aweiss-dev May 30, 2024 •

edited

svitovyda Jun 3, 2024 •

edited