From d8dbb6cc22156d5257e4fb5a54ba6d0afd11636c Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Tue, 22 Apr 2025 10:43:13 +0530 Subject: [PATCH] MM-56548: [AI assisted]Add support for incremental thread loading using UpdateAt timestamp (#30486) Every time we load the RHS, we used to load the FULL thread always. Although the actual ThreadViewer React component is virtualized, and the server side API call is paginated, we still went through all the pages, to get the full thread and passed it on to the ThreadViewer. This would be for first loads, and subsequent loads of the same thread. This was a bug originally, but then it was a necessity after we applied websocket event scope because now we won't get emoji reactions of a thread if the user is not on the thread. To fix that, we enhance the thread loading functionality by adding support for fetching thread updates based on the UpdateAt timestamp. Now, for subsequent loads, we only get the changed posts in a thread. The implementation: - Adds new API parameters: fromUpdateAt and updatesOnly to the GetPostThread endpoint - Updates database queries to support sorting and filtering by UpdateAt - Implements thread state management to track the last update timestamp - Adds client-side support to use incremental loading for improved performance - Ensures proper validation for parameter combinations and error handling This change enables more efficient thread loading, particularly for long threads with frequent updates, by only fetching posts that have been updated since the last view. Caveats: For delta updates, the SQL query won't use the best index possible because we have an index for (CreateAt, Id), but no index for (UpdateAt, Id). However, from my tests, it is not as bad as it looks: ``` [loadtest] # EXPLAIN (ANALYZE, BUFFERS) SELECT * FROM Posts WHERE Posts.DeleteAt = 0 AND Posts.RootId = 'qbr5gctu9iyg8c36hpcq6f3w8e' AND Posts.UpdateAt > 1623445795824 ORDER BY UpdateAt ASC, Id ASC LIMIT 61; QUERY PLAN ------------------------------------------------------------------------------------------------------------------------------------------------- Limit (cost=8.31..8.31 rows=1 width=216) (actual time=0.047..0.049 rows=0 loops=1) Buffers: shared hit=2 -> Sort (cost=8.31..8.31 rows=1 width=216) (actual time=0.044..0.045 rows=0 loops=1) Sort Key: updateat, id Sort Method: quicksort Memory: 25kB Buffers: shared hit=2 -> Index Scan using idx_posts_root_id_delete_at on posts (cost=0.28..8.30 rows=1 width=216) (actual time=0.031..0.032 rows=0 loops=1) Index Cond: (((rootid)::text = 'qbr5gctu9iyg8c36hpcq6f3w8e'::text) AND (deleteat = 0)) Filter: (updateat > '1623445795824'::bigint) Buffers: shared hit=2 Planning: Buffers: shared hit=3 Planning Time: 0.508 ms Execution Time: 0.106 ms (14 rows) ``` We still get an index scan with index cond. Although there's a filter element, but atleast we get the whole thread with the index. My thinking is that while the whole thread might be large, but after that, updates on a thread should be incremental. Therefore, we should be okay without adding yet another index on the posts table. This is just the first step in what could be potentially improved further. 1. We shouldn't even be loading the full thread always. But rather let the virtualized viewer load more posts on demand. 2. If a post has been just reacted to, then we need not send the whole post down, but just the reaction. This further saves bandwidth. https://mattermost.atlassian.net/browse/MM-56548 TBD: Add load-test coverage to update the thread loading code ```release-note NONE ``` --------- Co-authored-by: Mattermost Build --- .gitignore | 2 + api/v4/source/posts.yaml | 12 ++ server/channels/api4/post.go | 29 ++++ server/channels/api4/post_test.go | 53 ++++++++ server/channels/store/sqlstore/post_store.go | 100 ++++++++++---- server/channels/store/storetest/post_store.go | 124 +++++++++++++++++- server/public/model/client4.go | 6 + server/public/model/post.go | 2 + webapp/channels/src/actions/views/threads.ts | 10 ++ .../src/actions/websocket_actions.jsx | 8 +- .../threading/thread_viewer/index.ts | 7 +- .../thread_viewer/thread_viewer.test.tsx | 62 ++++++++- .../threading/thread_viewer/thread_viewer.tsx | 28 +++- .../mattermost-redux/src/actions/posts.ts | 31 ++++- webapp/channels/src/reducers/views/threads.ts | 13 ++ .../channels/src/selectors/views/threads.ts | 4 + webapp/channels/src/types/store/views.ts | 1 + webapp/channels/src/utils/constants.tsx | 1 + webapp/platform/types/src/client4.ts | 2 + 19 files changed, 451 insertions(+), 44 deletions(-) diff --git a/.gitignore b/.gitignore index e483fb16e9..d2feb985d3 100644 --- a/.gitignore +++ b/.gitignore @@ -159,3 +159,5 @@ docker-compose.override.yaml .notice-work/ .aider* .env + +CLAUDE.md diff --git a/api/v4/source/posts.yaml b/api/v4/source/posts.yaml index 809c810382..4a8397535e 100644 --- a/api/v4/source/posts.yaml +++ b/api/v4/source/posts.yaml @@ -401,6 +401,12 @@ schema: type: integer default: 0 + - name: fromUpdateAt + in: query + description: The update_at timestamp to return the next page of posts from. You cannot set this flag with direction=down. + schema: + type: integer + default: 0 - name: direction in: query description: The direction to return the posts. Either up or down. @@ -425,6 +431,12 @@ schema: type: boolean default: false + - name: updatesOnly + in: query + description: This flag is used to make the API work with the updateAt value. If you set this flag, you must set a value for fromUpdateAt. + schema: + type: boolean + default: false responses: "200": description: Post list retrieval successful diff --git a/server/channels/api4/post.go b/server/channels/api4/post.go index f45e25da30..7c9b0ad3cb 100644 --- a/server/channels/api4/post.go +++ b/server/channels/api4/post.go @@ -671,6 +671,27 @@ func getPostThread(c *Context, w http.ResponseWriter, r *http.Request) { return } + var fromUpdateAt int64 + if fromUpdateAtStr := r.URL.Query().Get("fromUpdateAt"); fromUpdateAtStr != "" { + var err error + fromUpdateAt, err = strconv.ParseInt(fromUpdateAtStr, 10, 64) + if err != nil { + c.SetInvalidParamWithErr("fromUpdateAt", err) + return + } + } + + if fromUpdateAt != 0 && fromCreateAt != 0 { + c.SetInvalidParamWithDetails("fromUpdateAt", "both fromUpdateAt and fromCreateAt cannot be set") + return + } + + updatesOnly := r.URL.Query().Get("updatesOnly") == "true" + if updatesOnly && fromUpdateAt == 0 { + c.SetInvalidParamWithDetails("fromUpdateAt", "fromUpdateAt must be set if updatesOnly is set") + return + } + direction := "" if dir := r.URL.Query().Get("direction"); dir != "" { if dir != "up" && dir != "down" { @@ -679,14 +700,22 @@ func getPostThread(c *Context, w http.ResponseWriter, r *http.Request) { } direction = dir } + + if updatesOnly && direction == "up" { + c.SetInvalidParamWithDetails("updatesOnly", "updatesOnly flag cannot be used with up direction") + return + } + opts := model.GetPostsOptions{ SkipFetchThreads: r.URL.Query().Get("skipFetchThreads") == "true", CollapsedThreads: r.URL.Query().Get("collapsedThreads") == "true", CollapsedThreadsExtended: r.URL.Query().Get("collapsedThreadsExtended") == "true", + UpdatesOnly: updatesOnly, PerPage: perPage, Direction: direction, FromPost: fromPost, FromCreateAt: fromCreateAt, + FromUpdateAt: fromUpdateAt, } list, err := c.App.GetPostThread(c.Params.PostId, opts, c.AppContext.Session().UserId) if err != nil { diff --git a/server/channels/api4/post_test.go b/server/channels/api4/post_test.go index 76facb727f..8825a92ae9 100644 --- a/server/channels/api4/post_test.go +++ b/server/channels/api4/post_test.go @@ -3638,7 +3638,60 @@ func TestGetPostThread(t *testing.T) { require.Error(t, err) CheckForbiddenStatus(t, resp) + // Test the new query parameters - updatesOnly, fromUpdateAt // Sending some bad params + _, resp, err = client.GetPostThreadWithOpts(context.Background(), th.BasicPost.Id, "", model.GetPostsOptions{ + UpdatesOnly: true, // updatesOnly is true but fromUpdateAt is not set + }) + require.Error(t, err) + CheckBadRequestStatus(t, resp) + + // Test error when both fromUpdateAt and fromCreateAt are set + _, resp, err = client.GetPostThreadWithOpts(context.Background(), th.BasicPost.Id, "", model.GetPostsOptions{ + FromUpdateAt: 12345, + FromCreateAt: 12345, + }) + require.Error(t, err) + CheckBadRequestStatus(t, resp) + + // Test error when updatesOnly is used with direction="up" + _, resp, err = client.GetPostThreadWithOpts(context.Background(), th.BasicPost.Id, "", model.GetPostsOptions{ + UpdatesOnly: true, + FromUpdateAt: 12345, + Direction: "up", + }) + require.Error(t, err) + CheckBadRequestStatus(t, resp) + + // Test valid parameters + // This should work with proper parameters + _, resp, err = client.GetPostThreadWithOpts(context.Background(), th.BasicPost.Id, "", model.GetPostsOptions{ + UpdatesOnly: true, + FromUpdateAt: 12345, + Direction: "down", + }) + require.NoError(t, err) + CheckOKStatus(t, resp) + + list, resp, err = client.GetPostThreadWithOpts(context.Background(), th.BasicPost.Id, "", model.GetPostsOptions{ + UpdatesOnly: true, + Direction: "down", + FromUpdateAt: post.UpdateAt, + }) + require.NoError(t, err) + CheckOKStatus(t, resp) + assert.Len(t, list.Order, 1) + assert.Len(t, list.Posts, 1) + require.Equal(t, th.BasicPost.Id, list.Order[0], "wrong order") + + // Test with just fromUpdateAt parameter + _, resp, err = client.GetPostThreadWithOpts(context.Background(), th.BasicPost.Id, "", model.GetPostsOptions{ + FromUpdateAt: 12345, + }) + require.NoError(t, err) + CheckOKStatus(t, resp) + + // Sending other bad params unrelated to the new changes _, resp, err = client.GetPostThreadWithOpts(context.Background(), th.BasicPost.Id, "", model.GetPostsOptions{ CollapsedThreads: true, FromPost: "something", diff --git a/server/channels/store/sqlstore/post_store.go b/server/channels/store/sqlstore/post_store.go index b5d8efd6e0..a48959db99 100644 --- a/server/channels/store/sqlstore/post_store.go +++ b/server/channels/store/sqlstore/post_store.go @@ -627,36 +627,50 @@ func (s *SqlPostStore) getPostWithCollapsedThreads(id, userID string, opts model } } if sort != "" { - query = query.OrderBy("CreateAt " + sort + ", Id " + sort) + if opts.UpdatesOnly { + query = query.OrderBy("UpdateAt " + sort + ", Id " + sort) + } else { + query = query.OrderBy("CreateAt " + sort + ", Id " + sort) + } } if opts.FromCreateAt != 0 { + var direction sq.Sqlizer + var pagination sq.Sqlizer + if opts.Direction == "down" { - direction := sq.Gt{"Posts.CreateAt": opts.FromCreateAt} - if opts.FromPost != "" { - query = query.Where(sq.Or{ - direction, - sq.And{ - sq.Eq{"Posts.CreateAt": opts.FromCreateAt}, - sq.Gt{"Posts.Id": opts.FromPost}, - }, - }) - } else { - query = query.Where(direction) - } + direction = sq.Gt{"Posts.CreateAt": opts.FromCreateAt} + pagination = sq.Gt{"Posts.Id": opts.FromPost} } else { - direction := sq.Lt{"Posts.CreateAt": opts.FromCreateAt} - if opts.FromPost != "" { - query = query.Where(sq.Or{ - direction, - sq.And{ - sq.Eq{"Posts.CreateAt": opts.FromCreateAt}, - sq.Lt{"Posts.Id": opts.FromPost}, - }, - }) - } else { - query = query.Where(direction) - } + direction = sq.Lt{"Posts.CreateAt": opts.FromCreateAt} + pagination = sq.Lt{"Posts.Id": opts.FromPost} + } + + if opts.FromPost != "" { + query = query.Where(sq.Or{ + direction, + sq.And{ + sq.Eq{"Posts.CreateAt": opts.FromCreateAt}, + pagination, + }, + }) + } else { + query = query.Where(direction) + } + } + + if opts.FromUpdateAt != 0 && opts.Direction == "down" { + direction := sq.Gt{"Posts.UpdateAt": opts.FromUpdateAt} + if opts.FromPost != "" { + query = query.Where(sq.Or{ + direction, + sq.And{ + sq.Eq{"Posts.UpdateAt": opts.FromUpdateAt}, + sq.Gt{"Posts.Id": opts.FromPost}, + }, + }) + } else { + query = query.Where(direction) } } @@ -773,7 +787,11 @@ func (s *SqlPostStore) Get(ctx context.Context, id string, opts model.GetPostsOp } } if sort != "" { - query = query.OrderBy("CreateAt " + sort + ", Id " + sort) + if opts.UpdatesOnly { + query = query.OrderBy("UpdateAt " + sort + ", Id " + sort) + } else { + query = query.OrderBy("CreateAt " + sort + ", Id " + sort) + } } if opts.FromCreateAt != 0 { @@ -806,6 +824,36 @@ func (s *SqlPostStore) Get(ctx context.Context, id string, opts model.GetPostsOp } } + if opts.FromUpdateAt != 0 { + if opts.Direction == "down" { + direction := sq.Gt{"p.UpdateAt": opts.FromUpdateAt} + if opts.FromPost != "" { + query = query.Where(sq.Or{ + direction, + sq.And{ + sq.Eq{"p.UpdateAt": opts.FromUpdateAt}, + sq.Gt{"p.Id": opts.FromPost}, + }, + }) + } else { + query = query.Where(direction) + } + } else { + direction := sq.Lt{"p.UpdateAt": opts.FromUpdateAt} + if opts.FromPost != "" { + query = query.Where(sq.Or{ + direction, + sq.And{ + sq.Eq{"p.UpdateAt": opts.FromUpdateAt}, + sq.Lt{"p.Id": opts.FromPost}, + }, + }) + } else { + query = query.Where(direction) + } + } + } + if opts.PerPage != 0 { query = query.Limit(uint64(opts.PerPage + 1)) } diff --git a/server/channels/store/storetest/post_store.go b/server/channels/store/storetest/post_store.go index 7d23a0b6b5..e38205c522 100644 --- a/server/channels/store/storetest/post_store.go +++ b/server/channels/store/storetest/post_store.go @@ -838,7 +838,7 @@ func testPostStoreGetForThread(t *testing.T, rctx request.CTX, ss store.Store) { } r1, err = ss.Post().Get(context.Background(), o1.Id, opts, o1.UserId, map[string]bool{}) require.NoError(t, err) - require.Len(t, r1.Order, 2) // including the root post + require.Len(t, r1.Order, 2) require.Len(t, r1.Posts, 2) assert.True(t, *r1.HasNext) @@ -876,7 +876,7 @@ func testPostStoreGetForThread(t *testing.T, rctx request.CTX, ss store.Store) { } r1, err = ss.Post().Get(context.Background(), o1.Id, opts, o1.UserId, map[string]bool{}) require.NoError(t, err) - require.Len(t, r1.Order, 2) // including the root post + require.Len(t, r1.Order, 2) require.Len(t, r1.Posts, 2) assert.LessOrEqual(t, r1.Posts[r1.Order[1]].CreateAt, firstPostCreateAt) assert.False(t, *r1.HasNext) @@ -896,6 +896,126 @@ func testPostStoreGetForThread(t *testing.T, rctx request.CTX, ss store.Store) { assert.GreaterOrEqual(t, r1.Posts[r1.Order[1]].CreateAt, m1.CreateAt) assert.True(t, *r1.HasNext) }) + + t.Run("Pagination with UpdateAt", func(t *testing.T) { + teamID := model.NewId() + channel, err := ss.Channel().Save(rctx, &model.Channel{ + TeamId: teamID, + DisplayName: "DisplayName1", + Name: "channel" + model.NewId(), + Type: model.ChannelTypeOpen, + }, -1) + require.NoError(t, err) + + now := model.GetMillis() + o1 := &model.Post{CreateAt: now, ChannelId: channel.Id, UserId: model.NewId(), Message: NewTestID()} + o1, err = ss.Post().Save(rctx, o1) + require.NoError(t, err) + + // Create replies with explicit UpdateAt timestamps + o2 := &model.Post{CreateAt: now + 1, UpdateAt: now + 1, ChannelId: o1.ChannelId, UserId: model.NewId(), Message: NewTestID(), RootId: o1.Id} + _, err = ss.Post().Save(rctx, o2) + require.NoError(t, err) + + m1 := &model.Post{CreateAt: now + 2, UpdateAt: now + 2, ChannelId: o1.ChannelId, UserId: model.NewId(), Message: NewTestID(), RootId: o1.Id} + m1, err = ss.Post().Save(rctx, m1) + require.NoError(t, err) + + o3 := &model.Post{CreateAt: now + 3, UpdateAt: now + 3, ChannelId: o1.ChannelId, UserId: model.NewId(), Message: NewTestID(), RootId: o1.Id} + _, err = ss.Post().Save(rctx, o3) + require.NoError(t, err) + + o4 := &model.Post{CreateAt: now + 4, UpdateAt: now + 4, ChannelId: o1.ChannelId, UserId: model.NewId(), Message: NewTestID(), RootId: o1.Id} + o4, err = ss.Post().Save(rctx, o4) + require.NoError(t, err) + + // Test pagination with UpdateAt in "down" direction + opts := model.GetPostsOptions{ + UpdatesOnly: true, + CollapsedThreads: true, + PerPage: 2, + Direction: "down", + } + r1, err := ss.Post().Get(context.Background(), o1.Id, opts, o1.UserId, map[string]bool{}) + require.NoError(t, err) + require.Len(t, r1.Order, 3) // including the root post + require.Len(t, r1.Posts, 3) + assert.Equal(t, r1.Posts[r1.Order[0]].UpdateAt, o4.CreateAt) // The root post always get updated with the createAt of the latest post in the thread. + assert.True(t, *r1.HasNext) + + lastPostID := r1.Order[len(r1.Order)-1] + lastPostUpdateAt := r1.Posts[lastPostID].UpdateAt + + // Continue pagination using UpdateAt + opts = model.GetPostsOptions{ + UpdatesOnly: true, + CollapsedThreads: true, + PerPage: 2, + Direction: "down", + FromPost: lastPostID, + FromUpdateAt: lastPostUpdateAt, + } + r1, err = ss.Post().Get(context.Background(), o1.Id, opts, o1.UserId, map[string]bool{}) + require.NoError(t, err) + require.Len(t, r1.Order, 3) // including the root post + require.Len(t, r1.Posts, 3) + assert.GreaterOrEqual(t, r1.Posts[r1.Order[len(r1.Order)-1]].UpdateAt, lastPostUpdateAt) + assert.Equal(t, r1.Posts[r1.Order[0]].UpdateAt, o4.CreateAt) // The root post always get updated with the createAt of the latest post in the thread. + assert.False(t, *r1.HasNext) + + // Non-CRT mode with UpdateAt pagination + opts = model.GetPostsOptions{ + UpdatesOnly: true, + CollapsedThreads: false, + PerPage: 2, + Direction: "down", + SkipFetchThreads: false, + } + r1, err = ss.Post().Get(context.Background(), o1.Id, opts, o1.UserId, map[string]bool{}) + require.NoError(t, err) + // Ordering by updateAt will move the root post down, so we will get more posts in the thread. + require.Len(t, r1.Order, 3) + require.Len(t, r1.Posts, 3) + require.True(t, *r1.HasNext) + + lastPostID = r1.Order[len(r1.Order)-1] + lastPostUpdateAt = r1.Posts[lastPostID].UpdateAt + + opts = model.GetPostsOptions{ + UpdatesOnly: true, + CollapsedThreads: false, + PerPage: 3, + Direction: "down", + FromPost: lastPostID, + FromUpdateAt: lastPostUpdateAt, + SkipFetchThreads: false, + } + r1, err = ss.Post().Get(context.Background(), o1.Id, opts, o1.UserId, map[string]bool{}) + require.NoError(t, err) + require.Len(t, r1.Order, 3) + require.Len(t, r1.Posts, 3) + require.Equal(t, r1.Posts[r1.Order[0]].ReplyCount, int64(4)) + require.Equal(t, r1.Posts[r1.Order[1]].ReplyCount, int64(4)) + require.Equal(t, r1.Posts[r1.Order[2]].ReplyCount, int64(4)) + require.GreaterOrEqual(t, r1.Posts[r1.Order[len(r1.Order)-1]].UpdateAt, lastPostUpdateAt) + assert.False(t, *r1.HasNext) + + // Only with UpdateAt - direction down + opts = model.GetPostsOptions{ + UpdatesOnly: true, + CollapsedThreads: false, + PerPage: 1, + Direction: "down", + FromUpdateAt: m1.UpdateAt, + SkipFetchThreads: false, + } + r1, err = ss.Post().Get(context.Background(), o1.Id, opts, o1.UserId, map[string]bool{}) + require.NoError(t, err) + require.Len(t, r1.Order, 2) + require.Len(t, r1.Posts, 2) + require.GreaterOrEqual(t, r1.Posts[r1.Order[1]].UpdateAt, m1.UpdateAt) + require.True(t, *r1.HasNext) + }) } func testPostStoreGetSingle(t *testing.T, rctx request.CTX, ss store.Store) { diff --git a/server/public/model/client4.go b/server/public/model/client4.go index 8c150e7255..aedb244f42 100644 --- a/server/public/model/client4.go +++ b/server/public/model/client4.go @@ -4168,6 +4168,9 @@ func (c *Client4) GetPostThreadWithOpts(ctx context.Context, postID string, etag if opts.SkipFetchThreads { values.Set("skipFetchThreads", "true") } + if opts.UpdatesOnly { + values.Set("updatesOnly", "true") + } if opts.PerPage != 0 { values.Set("perPage", strconv.Itoa(opts.PerPage)) } @@ -4177,6 +4180,9 @@ func (c *Client4) GetPostThreadWithOpts(ctx context.Context, postID string, etag if opts.FromCreateAt != 0 { values.Set("fromCreateAt", strconv.FormatInt(opts.FromCreateAt, 10)) } + if opts.FromUpdateAt != 0 { + values.Set("fromUpdateAt", strconv.FormatInt(opts.FromUpdateAt, 10)) + } if opts.Direction != "" { values.Set("direction", opts.Direction) } diff --git a/server/public/model/post.go b/server/public/model/post.go index 7c77bc99f6..d29170977e 100644 --- a/server/public/model/post.go +++ b/server/public/model/post.go @@ -403,7 +403,9 @@ type GetPostsOptions struct { CollapsedThreadsExtended bool FromPost string // PostId after which to send the items FromCreateAt int64 // CreateAt after which to send the items + FromUpdateAt int64 // UpdateAt after which to send the items. This cannot be used with FromCreateAt. Direction string // Only accepts up|down. Indicates the order in which to send the items. + UpdatesOnly bool // This flag is used to make the API work with the updateAt value. IncludeDeleted bool IncludePostPriority bool } diff --git a/webapp/channels/src/actions/views/threads.ts b/webapp/channels/src/actions/views/threads.ts index 1e310478c2..c4a81c5772 100644 --- a/webapp/channels/src/actions/views/threads.ts +++ b/webapp/channels/src/actions/views/threads.ts @@ -24,6 +24,16 @@ export function updateThreadLastOpened(threadId: string, lastViewedAt: number) { }; } +export function updateThreadLastUpdateAt(threadId: string, lastUpdateAt: number) { + return { + type: Threads.CHANGED_LAST_UPDATE_AT, + data: { + threadId, + lastUpdateAt, + }, + }; +} + export function setSelectedThreadId(teamId: string, threadId: string | undefined) { return { type: Threads.CHANGED_SELECTED_THREAD, diff --git a/webapp/channels/src/actions/websocket_actions.jsx b/webapp/channels/src/actions/websocket_actions.jsx index b2d08ae1da..e1f3c60808 100644 --- a/webapp/channels/src/actions/websocket_actions.jsx +++ b/webapp/channels/src/actions/websocket_actions.jsx @@ -847,9 +847,11 @@ async function handlePostDeleteEvent(msg) { } } else { const res = await dispatch(getPostThread(post.root_id)); - const {order, posts} = res.data; - const rootPost = posts[order[0]]; - dispatch(receivedPost(rootPost)); + if (res.data) { + const {order, posts} = res.data; + const rootPost = posts[order[0]]; + dispatch(receivedPost(rootPost)); + } } } diff --git a/webapp/channels/src/components/threading/thread_viewer/index.ts b/webapp/channels/src/components/threading/thread_viewer/index.ts index 813c902e88..3ae3996284 100644 --- a/webapp/channels/src/components/threading/thread_viewer/index.ts +++ b/webapp/channels/src/components/threading/thread_viewer/index.ts @@ -22,8 +22,9 @@ import {getThread} from 'mattermost-redux/selectors/entities/threads'; import {getCurrentUserId} from 'mattermost-redux/selectors/entities/users'; import {selectPostCard} from 'actions/views/rhs'; -import {updateThreadLastOpened} from 'actions/views/threads'; +import {updateThreadLastOpened, updateThreadLastUpdateAt} from 'actions/views/threads'; import {getHighlightedPostId, getSelectedPostFocussedAt} from 'selectors/rhs'; +import {getThreadLastUpdateAt} from 'selectors/views/threads'; import {getSocketStatus} from 'selectors/views/websocket'; import type {GlobalState} from 'types/store'; @@ -51,11 +52,13 @@ function makeMapStateToProps() { let postIds: string[] = []; let userThread: UserThread | null = null; let channel: Channel | undefined; + let lastUpdateAt: number = 0; if (selected) { postIds = getPostIdsForThread(state, selected.id); userThread = getThread(state, selected.id); channel = getChannel(state, selected.channel_id); + lastUpdateAt = getThreadLastUpdateAt(state, selected.id); } return { @@ -71,6 +74,7 @@ function makeMapStateToProps() { highlightedPostId, selectedPostFocusedAt, enableWebSocketEventScope, + lastUpdateAt, }; }; } @@ -85,6 +89,7 @@ function mapDispatchToProps(dispatch: Dispatch) { selectPostCard, updateThreadLastOpened, updateThreadRead, + updateThreadLastUpdateAt, }, dispatch), }; } diff --git a/webapp/channels/src/components/threading/thread_viewer/thread_viewer.test.tsx b/webapp/channels/src/components/threading/thread_viewer/thread_viewer.test.tsx index e29ca45ad3..6387008803 100644 --- a/webapp/channels/src/components/threading/thread_viewer/thread_viewer.test.tsx +++ b/webapp/channels/src/components/threading/thread_viewer/thread_viewer.test.tsx @@ -54,6 +54,7 @@ describe('components/threading/ThreadViewer', () => { getThread: jest.fn(), updateThreadRead: jest.fn(), updateThreadLastOpened: jest.fn(), + updateThreadLastUpdateAt: jest.fn(), fetchRHSAppsBindings: jest.fn(), }; @@ -70,8 +71,23 @@ describe('components/threading/ThreadViewer', () => { rootPostId: post.id, isThreadView: true, enableWebSocketEventScope: false, + lastUpdateAt: 1234, }; + beforeEach(() => { + // Reset and redefine the mock before each test + actions.getPostThread.mockReset().mockResolvedValue({ + data: { + order: ['post1', 'post2', 'post3'], + posts: { + post1: TestHelper.getPostMock({id: 'post1', update_at: 1000}), + post2: TestHelper.getPostMock({id: 'post2', update_at: 2000}), + post3: TestHelper.getPostMock({id: 'post3', update_at: 1500}), + }, + }, + }); + }); + test('should match snapshot', async () => { const reset = fakeDate(new Date(1502715365000)); @@ -92,7 +108,7 @@ describe('components/threading/ThreadViewer', () => { wrapper.setProps({socketConnectionStatus: false}); wrapper.setProps({socketConnectionStatus: true}); - return expect(actions.getPostThread).toHaveBeenCalledWith(post.id, true); + return expect(actions.getPostThread).toHaveBeenCalledWith(post.id, true, 1234); }); test('should not break if root post is a fake post', () => { @@ -244,4 +260,48 @@ describe('components/threading/ThreadViewer', () => { expect(actions.fetchRHSAppsBindings).not.toHaveBeenCalledWith('channel_id', 'id'); }); + + test('should update thread with highest update_at value when lastUpdateAt is 0', async () => { + const {actions} = baseProps; + + shallow( + , + ); + + await new Promise(process.nextTick); + + // Verify getPostThread was called with lastUpdateAt = 0 + expect(actions.getPostThread).toHaveBeenCalledWith(post.id, true, 0); + + // Verify updateThreadLastUpdateAt was called with the highest update_at value + expect(actions.updateThreadLastUpdateAt).toHaveBeenCalledWith(post.id, 2000); + }); + + test('should handle case where root post has the highest update_at value', async () => { + // Mock with root post having highest update_at + actions.getPostThread.mockReset().mockResolvedValue({ + data: { + order: ['post1', 'post2', 'post3'], + posts: { + post1: TestHelper.getPostMock({id: 'post1', update_at: 9000}), // Highest value (root post) + post2: TestHelper.getPostMock({id: 'post2', update_at: 2000}), + post3: TestHelper.getPostMock({id: 'post3', update_at: 1500}), + }, + }, + }); + + shallow( + , + ); + + await new Promise(process.nextTick); + + // Verify updateThreadLastUpdateAt was called with the highest update_at value + expect(actions.updateThreadLastUpdateAt).toHaveBeenCalledWith(post.id, 9000); + }); }); diff --git a/webapp/channels/src/components/threading/thread_viewer/thread_viewer.tsx b/webapp/channels/src/components/threading/thread_viewer/thread_viewer.tsx index bf2b271f59..03d8791598 100644 --- a/webapp/channels/src/components/threading/thread_viewer/thread_viewer.tsx +++ b/webapp/channels/src/components/threading/thread_viewer/thread_viewer.tsx @@ -40,11 +40,12 @@ export type Props = Attrs & { actions: { fetchRHSAppsBindings: (channelId: string, rootID: string) => unknown; getNewestPostThread: (rootId: string) => Promise; - getPostThread: (rootId: string, fetchThreads: boolean) => Promise; + getPostThread: (rootId: string, fetchThreads: boolean, lastUpdateAt: number) => Promise; getThread: (userId: string, teamId: string, threadId: string, extended: boolean) => Promise; selectPostCard: (post: Post) => void; updateThreadLastOpened: (threadId: string, lastViewedAt: number) => unknown; updateThreadRead: (userId: string, teamId: string, threadId: string, timestamp: number) => unknown; + updateThreadLastUpdateAt: (threadId: string, lastUpdateAt: number) => unknown; }; useRelativeTimestamp?: boolean; postIds: string[]; @@ -54,6 +55,7 @@ export type Props = Attrs & { inputPlaceholder?: string; rootPostId: string; enableWebSocketEventScope: boolean; + lastUpdateAt: number; }; type State = { @@ -95,7 +97,6 @@ export default class ThreadViewer extends React.PureComponent { } const selectedChanged = this.props.selected.id !== prevProps.selected?.id; - if (reconnected || selectedChanged) { this.onInit(reconnected); } @@ -108,7 +109,7 @@ export default class ThreadViewer extends React.PureComponent { } if (this.props.appsEnabled && ( - this.props.channel?.id !== prevProps.channel?.id || this.props.selected.id !== prevProps.selected?.id + this.props.channel?.id !== prevProps.channel?.id || selectedChanged )) { this.props.actions.fetchRHSAppsBindings(this.props.channel?.id || '', this.props.selected.id); } @@ -173,7 +174,26 @@ export default class ThreadViewer extends React.PureComponent { // scrolls to either bottom or new messages line private onInit = async (reconnected = false): Promise => { this.setState({isLoading: !reconnected}); - await this.props.actions.getPostThread(this.props.selected?.id || this.props.rootPostId, !reconnected); + const res = await this.props.actions.getPostThread(this.props.selected?.id || this.props.rootPostId, !reconnected, this.props.lastUpdateAt); + + if (this.props.selected && res.data) { + const {order, posts} = res.data; + if (order.length > 0 && posts[order[0]]) { + let highestUpdateAt = posts[order[0]].update_at; + + // Check all posts to find the highest update_at + for (const postId in posts) { + if (Object.hasOwn(posts, postId)) { + const post = posts[postId]; + if (post.update_at > highestUpdateAt) { + highestUpdateAt = post.update_at; + } + } + } + + this.props.actions.updateThreadLastUpdateAt(this.props.selected.id, highestUpdateAt); + } + } if ( this.props.isCollapsedThreadsEnabled && diff --git a/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts b/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts index 2d41c90558..b7ff7af196 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts @@ -614,11 +614,20 @@ async function getPaginatedPostThread(rootId: string, options: FetchPaginatedThr if (result.has_next) { const [nextPostId] = list.order!.slice(-1); const nextPostPointer = list.posts[nextPostId]; - const newOptions = { - ...options, - fromCreateAt: nextPostPointer.create_at, - fromPost: nextPostId, - }; + let newOptions; + if (options.updatesOnly) { + newOptions = { + ...options, + fromUpdateAt: nextPostPointer.update_at, + fromPost: nextPostId, + }; + } else { + newOptions = { + ...options, + fromCreateAt: nextPostPointer.create_at, + fromPost: nextPostId, + }; + } return getPaginatedPostThread(rootId, newOptions, list); } @@ -626,15 +635,23 @@ async function getPaginatedPostThread(rootId: string, options: FetchPaginatedThr return list; } -export function getPostThread(rootId: string, fetchThreads = true): ActionFuncAsync { +export function getPostThread(rootId: string, fetchThreads = true, lastUpdateAt = 0): ActionFuncAsync { return async (dispatch, getState) => { const state = getState(); const collapsedThreadsEnabled = isCollapsedThreadsEnabled(state); const enabledUserStatuses = getIsUserStatusesConfigEnabled(state); let posts; + const options: FetchPaginatedThreadOptions = { + fetchThreads, + collapsedThreads: collapsedThreadsEnabled, + }; + if (lastUpdateAt !== 0) { + options.updatesOnly = true; + options.fromUpdateAt = lastUpdateAt; + } try { - posts = await getPaginatedPostThread(rootId, {fetchThreads, collapsedThreads: collapsedThreadsEnabled}); + posts = await getPaginatedPostThread(rootId, options); } catch (error) { forceLogoutIfNecessary(error, dispatch, getState); dispatch(logError(error)); diff --git a/webapp/channels/src/reducers/views/threads.ts b/webapp/channels/src/reducers/views/threads.ts index 9156c84474..4a87294755 100644 --- a/webapp/channels/src/reducers/views/threads.ts +++ b/webapp/channels/src/reducers/views/threads.ts @@ -51,6 +51,18 @@ export const lastViewedAt = (state: ViewsState['threads']['lastViewedAt'] = {}, } }; +export const lastUpdateAt = (state: ViewsState['threads']['lastUpdateAt'] = {}, action: MMAction) => { + switch (action.type) { + case Threads.CHANGED_LAST_UPDATE_AT: + return { + ...state, + [action.data.threadId]: action.data.lastUpdateAt, + }; + default: + return state; + } +}; + export function manuallyUnread(state: ViewsState['threads']['manuallyUnread'] = {}, action: MMAction) { switch (action.type) { case Threads.CHANGED_LAST_VIEWED_AT: @@ -90,4 +102,5 @@ export default combineReducers({ lastViewedAt, manuallyUnread, toastStatus, + lastUpdateAt, }); diff --git a/webapp/channels/src/selectors/views/threads.ts b/webapp/channels/src/selectors/views/threads.ts index 97e7c4f00e..da3abd798e 100644 --- a/webapp/channels/src/selectors/views/threads.ts +++ b/webapp/channels/src/selectors/views/threads.ts @@ -87,6 +87,10 @@ export const isThreadManuallyUnread = (state: GlobalState, threadId: UserThread[ return state.views.threads.manuallyUnread[threadId] || false; }; +export const getThreadLastUpdateAt = (state: GlobalState, threadId: UserThread['id']): number => { + return state.views.threads.lastUpdateAt[threadId] || 0; +}; + // Returns a selector that, given the state and an object containing an array of postIds and an optional // timestamp of when the channel was last read, returns a memoized array of postIds interspersed with // day indicators, an optional new message indicator and create comment. diff --git a/webapp/channels/src/types/store/views.ts b/webapp/channels/src/types/store/views.ts index af281da2ba..3a2fc3295e 100644 --- a/webapp/channels/src/types/store/views.ts +++ b/webapp/channels/src/types/store/views.ts @@ -207,6 +207,7 @@ export type ViewsState = { threads: { selectedThreadIdInTeam: RelationOneToOne; lastViewedAt: {[id: string]: number}; + lastUpdateAt: {[id: string]: number}; manuallyUnread: {[id: string]: boolean}; toastStatus: boolean; }; diff --git a/webapp/channels/src/utils/constants.tsx b/webapp/channels/src/utils/constants.tsx index ed9b10ad07..1826eee776 100644 --- a/webapp/channels/src/utils/constants.tsx +++ b/webapp/channels/src/utils/constants.tsx @@ -747,6 +747,7 @@ export const Threads = { CHANGED_SELECTED_THREAD: 'changed_selected_thread', CHANGED_LAST_VIEWED_AT: 'changed_last_viewed_at', MANUALLY_UNREAD_THREAD: 'manually_unread_thread', + CHANGED_LAST_UPDATE_AT: 'changed_last_update_at', }; export const CloudBanners = { diff --git a/webapp/platform/types/src/client4.ts b/webapp/platform/types/src/client4.ts index d3453c9c39..5661097810 100644 --- a/webapp/platform/types/src/client4.ts +++ b/webapp/platform/types/src/client4.ts @@ -37,9 +37,11 @@ export type FetchPaginatedThreadOptions = { fetchThreads?: boolean; collapsedThreads?: boolean; collapsedThreadsExtended?: boolean; + updatesOnly?: boolean; // This indicates the API is meant to be used to only get delta updates. direction?: 'up'|'down'; fetchAll?: boolean; perPage?: number; fromCreateAt?: number; + fromUpdateAt?: number; fromPost?: string; }