Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(MessagesList): fix resizeObserver and scrolledToBottom state update #14327

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Feb 10, 2025

☑️ Resolves

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏡 After

2025-02-10_09h13_03.mp4

🔬 Test

  • removing :key:
    • ResizeObserver works after conversation switch
    • nothing is broken, if we get rid of 'force-refresh' of whole component
  • non-scrollable chats do not have 🔽 button:
    • on open (on open with focusing message)
    • on resize
    • on messages list (height) change
  • scrollable chats do not have 🔽 button:
    • when scrolling down
    • when nothing to scroll
    • when new message arrives, and there's a mismatch of last read (follow-up from above)

🏁 Checklist

  • 🌏 Tested with different browsers / clients:
    • Chromium (Chrome / Edge / Opera / Brave)
    • Firefox
    • Safari
    • Talk Desktop
    • Not risky to browser differences / client
  • 🖌️ Design was reviewed, approved or inspired by the design team
  • ⛑️ Tests are included or not possible
  • 📗 User documentation in https://github.com/nextcloud/documentation/tree/master/user_manual/talk has been updated or is not required

@ShGKme
Copy link
Contributor

ShGKme commented Feb 10, 2025

Could you explain the change?

@Antreesy Antreesy marked this pull request as draft February 17, 2025 18:04
@Antreesy Antreesy force-pushed the fix/noid/message-list-sticky branch 2 times, most recently from 4729338 to b6dcc12 Compare February 18, 2025 13:39
@Antreesy Antreesy changed the title fix(MessagesList): do not replace container when switching conversations fix(MessagesList): fix resizeObserver and scrolledToBottom state update Feb 18, 2025
@@ -932,24 +934,25 @@ export default {
return
}

this.setChatScrolledToBottom(false)
Copy link
Contributor Author

@Antreesy Antreesy Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After swtiching to conversation, handleScroll is triggered. Regardless of container height, it sets value to false (whereas it shouldn't for non-scrollable chats)
For scrollable chats it also shouldn't set to false, when receiiving message (handleScroll is triggered with 'down' direction)
For just opened chats, however, it should still set to 'false', if chat is not at the bottom (we know that from previous 'if' block'

@Antreesy Antreesy marked this pull request as ready for review February 18, 2025 13:43
@Antreesy Antreesy requested a review from DorraJaouad February 19, 2025 11:55
Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sthg is wrong, chat is sticky when document is not focused and it scrolls to bottom ⚠️

@Antreesy Antreesy force-pushed the fix/noid/message-list-sticky branch from b6dcc12 to b38f22a Compare February 21, 2025 15:26
@Antreesy Antreesy force-pushed the fix/noid/message-list-sticky branch from 62f0414 to 3f15670 Compare February 24, 2025 17:23
@Antreesy Antreesy requested a review from DorraJaouad February 24, 2025 17:27
@Antreesy
Copy link
Contributor Author

Antreesy commented Feb 24, 2025

Rebased, see last 3 commits:

  • post-review fix
  • update of last read message with your last posted message (based on temp messages)
  • fix for scrollToBottom after switching to conversation

… actual scroll up, or chat is opened not at the bottom

Signed-off-by: Maksim Sukharev <[email protected]>
- text messages already covered in postNewMessage
- this fix is for file shares (we wait for server response to replace local temp messages)

Signed-off-by: Maksim Sukharev <[email protected]>
@Antreesy Antreesy force-pushed the fix/noid/message-list-sticky branch from 3f15670 to 0d2ea65 Compare February 25, 2025 14:13
Comment on lines 588 to +589
const tempMessages = context.getters.getTemporaryReferences(token, message.referenceId)
tempMessages.forEach(tempMessage => {
context.commit('deleteMessage', { token, id: tempMessage.id })
})
if (tempMessages.length > 0) {
Copy link
Contributor

@DorraJaouad DorraJaouad Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// If successful, deletes the temporary message from the store context.dispatch('removeTemporaryMessageFromStore', { token, id: temporaryMessage.id })

temp message is removed before processing the message in postNewMessage

Also, processMessage in multiple points, I wouldn't risk updating the last read message there. As suggested before, it should rather focus on post new message only. What's odd is that the current addition already exists there but it wasn't working efficiently

https://github.com/nextcloud/spreed/blob/main/src/store/messagesStore.js#L1276-L1291

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it exists and works for text messages only. File share (with /without caption) is a one-way, a we will receive a feedback only from message polling

postNewMessage can do it, as we receive a message object as a response and can replace it safely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants