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.

Sign up for GitHub

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
+80 −16
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
07d921e
fix(MessagesList): do not replace container when switching conversations
Antreesy Feb 9, 2025
80c7649
fix(MessagesList): check if chats are non-scrollable
Antreesy Feb 21, 2025
3f6b9d8
fix(MessagesList): show 'scroll to bottom' only if there is an actual…
Antreesy Feb 21, 2025
6a6a668
fixup! fix(MessagesList): check if chats are non-scrollable
Antreesy Feb 24, 2025
413abe7
fixup! fix(MessagesList): show 'scroll to bottom' only if there is an…
Antreesy Feb 24, 2025
0d2ea65
fix(messagesStore): update lastReadMessage if posted by same actor
Antreesy Feb 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 34 additions & 13 deletions src/components/MessagesList/MessagesList.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
<!-- size and remain refer to the amount and initial height of the items that
are outside of the viewport -->
<div ref="scroller"
:key="token"
class="scroller messages-list__scroller"
:class="{'scroller--chatScrolledToBottom': isChatScrolledToBottom,
'scroller--isScrolling': isScrolling}"
Expand Down Expand Up @@ -301,11 +300,14 @@ export default {
// scroll to bottom if needed
this.scrollToBottom({ smooth: false })

if (this.conversation?.type === CONVERSATION.TYPE.NOTE_TO_SELF) {
this.$nextTick(() => {
this.$nextTick(() => {
this.checkChatNotScrollable()

if (this.conversation?.type === CONVERSATION.TYPE.NOTE_TO_SELF) {
this.updateTasksCount()
})
}
}
})

},
},

Expand All @@ -321,8 +323,7 @@ export default {
this.$nextTick(() => {
this.checkSticky()
// setting wheel event for non-scrollable chat
const isScrollable = this.$refs.scroller.scrollHeight > this.$refs.scroller.clientHeight
if (!this.isChatBeginningReached && !isScrollable) {
if (!this.isChatBeginningReached && this.checkChatNotScrollable()) {
this.$refs.scroller.addEventListener('wheel', this.handleWheelEvent, { passive: true })
}
})
Expand Down Expand Up @@ -386,6 +387,8 @@ export default {
this.$refs.scroller.scrollTo({
top: this.$refs.scroller.scrollHeight,
})
} else {
this.checkChatNotScrollable()
}
},

Expand Down Expand Up @@ -660,6 +663,7 @@ export default {
if (token && this.isParticipant && !this.isInLobby) {
// prevent sticky mode before we have loaded anything
this.isInitialisingMessages = true
this.previousScrollTopValue = null
const focusMessageId = this.getMessageIdFromHash()

this.$store.dispatch('setVisualLastReadMessageId', { token, id: this.conversation.lastReadMessage })
Expand Down Expand Up @@ -852,7 +856,12 @@ export default {
if (this.isScrolling) {
clearTimeout(this.endScrollTimeout)
}
this.isScrolling = this.previousScrollTopValue > event.target.scrollTop ? 'up' : 'down'
if (this.previousScrollTopValue === null) {
// Chat is just opened, we do not know direction yet
this.isScrolling = null
} else {
this.isScrolling = this.previousScrollTopValue > event.target.scrollTop ? 'up' : 'down'
}
this.previousScrollTopValue = event.target.scrollTop
this.endScrollTimeout = setTimeout(this.endScroll, 3000)
// handle sticky date
Expand Down Expand Up @@ -905,7 +914,9 @@ export default {
return
}

this.setChatScrolledToBottom(false)
Copy link
Contributor Author

@Antreesy Antreesy Feb 18, 2025 •
edited
Loading

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'

if (scrollHeight > clientHeight && this.isScrolling !== 'down') {
this.setChatScrolledToBottom(false)
}

if ((scrollHeight > clientHeight && scrollTop < 800 && this.isScrolling === 'up')
|| skipHeightCheck) {
Expand Down Expand Up @@ -1162,10 +1173,7 @@ export default {
this.$refs.scroller.scrollTop += this.$refs.scroller.offsetHeight / 4
}

if (this.$refs.scroller && this.$refs.scroller.clientHeight === this.$refs.scroller.scrollHeight) {
// chat is not scrollable
this.setChatScrolledToBottom(true)
}
this.checkChatNotScrollable()

if (highlightAnimation && scrollElement === element) {
// element is visible, highlight it
Expand Down Expand Up @@ -1287,6 +1295,18 @@ export default {
this.chatExtrasStore.setTasksCounters({ tasksCount, tasksDoneCount })
},

checkChatNotScrollable() {
const isNotScrollable = this.$refs.scroller
? this.$refs.scroller.clientHeight === this.$refs.scroller.scrollHeight
: false

if (isNotScrollable && !this.isChatScrolledToBottom) {
this.setChatScrolledToBottom(true)
}

return isNotScrollable
},

handleWheelEvent(event) {
// If messages fit in the viewport and user scrolls up, we need to trigger the loading of older messages
if (event.deltaY < 0) {
Expand All @@ -1297,6 +1317,7 @@ export default {
return
}

this.isScrolling = 'up'
this.debounceHandleScroll({ skipHeightCheck: true })
}
},
Expand Down
18 changes: 15 additions & 3 deletions src/store/messagesStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,9 +586,21 @@ const actions = {

if (message.referenceId) {
const tempMessages = context.getters.getTemporaryReferences(token, message.referenceId)
tempMessages.forEach(tempMessage => {
context.commit('deleteMessage', { token, id: tempMessage.id })
})
if (tempMessages.length > 0) {
Comment on lines 588 to +589
Copy link
Contributor

@DorraJaouad DorraJaouad Feb 26, 2025 •
edited
Loading

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

// Replacing temporary placeholder message with posted message / file share
const conversation = context.getters.conversation(token)

if (conversation
&& message.actorId === context.getters.getActorId()
&& message.actorType === context.getters.getActorType()
&& message.id > conversation.lastReadMessage) {
context.dispatch('updateLastReadMessage', { token, id: message.id, updateVisually: true })
}

tempMessages.forEach(tempMessage => {
context.commit('deleteMessage', { token, id: tempMessage.id })
})
}
}

if (message.systemMessage === 'poll_voted') {
Expand Down
31 changes: 31 additions & 0 deletions src/store/messagesStore.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,37 @@ describe('messagesStore', () => {
expect(store.getters.messagesList(TOKEN)).toStrictEqual([message1])
})

test('updates last read message when replacing matching temporary message', () => {
conversationMock.mockReturnValueOnce({
token: TOKEN,
lastReadMessage: 100,
lastMessage: {
id: 123,
},
})
const response = generateOCSResponse({ payload: conversation })
updateLastReadMessage.mockResolvedValueOnce(response)

const temporaryMessage = {
id: 'temp-1',
referenceId: 'reference-1',
token: TOKEN,
}
store.dispatch('addTemporaryMessage', { token: TOKEN, message: temporaryMessage })

const message1 = {
id: 123,
token: TOKEN,
actorId: 'actor-id-1',
actorType: ATTENDEE.ACTOR_TYPE.USERS,
referenceId: 'reference-1',
}

store.dispatch('processMessage', { token: TOKEN, message: message1 })
expect(store.getters.messagesList(TOKEN)).toStrictEqual([message1])
expect(updateLastReadMessage).toHaveBeenCalledWith(TOKEN, message1.id)
})

test('replaces existing message', () => {
const message1 = {
id: 1,
Expand Down
Loading