-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: load 50 messages with hideSystemMessages as true on old workspaces #7305
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
base: develop
Are you sure you want to change the base?
Changes from all commits
f0ec783
66048a6
f91dcac
2853f44
1c024bb
2088928
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| import { loadMessagesForRoom } from './loadMessagesForRoom'; | ||
| import sdk from '../services/sdk'; | ||
| import { ROOM } from '../../actions/actionsTypes'; | ||
| import { getMessageById } from '../database/services/Message'; | ||
| import updateMessages from './updateMessages'; | ||
| import { store } from '../store/auxStore'; | ||
|
|
||
| jest.mock('../services/sdk', () => ({ | ||
| __esModule: true, | ||
| default: { | ||
| get: jest.fn() | ||
| } | ||
| })); | ||
|
|
||
| jest.mock('../database/services/Message', () => ({ | ||
| getMessageById: jest.fn() | ||
| })); | ||
|
|
||
| jest.mock('../database/services/Subscription', () => ({ | ||
| getSubscriptionByRoomId: jest.fn(() => Promise.resolve(null)) | ||
| })); | ||
|
|
||
| jest.mock('../store/auxStore', () => ({ | ||
| store: { | ||
| getState: jest.fn(() => ({ | ||
| settings: { Hide_System_Messages: ['uj'] } | ||
| })), | ||
| dispatch: jest.fn() | ||
| } | ||
| })); | ||
|
|
||
| jest.mock('./updateMessages', () => jest.fn()); | ||
|
|
||
| const mockedSdkGet = sdk.get as jest.MockedFunction<typeof sdk.get>; | ||
| const mockedGetMessageById = getMessageById as jest.MockedFunction<typeof getMessageById>; | ||
| const mockedUpdateMessages = updateMessages as jest.MockedFunction<typeof updateMessages>; | ||
| const mockedDispatch = store.dispatch as jest.MockedFunction<typeof store.dispatch>; | ||
|
|
||
| const buildMessage = ({ id, ts, t }: { id: string; ts: string; t?: string }) => | ||
| ({ | ||
| _id: id, | ||
| rid: 'ROOM_ID', | ||
| ts, | ||
| ...(t ? { t } : {}) | ||
| } as any); | ||
|
|
||
| describe('loadMessagesForRoom', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| mockedGetMessageById.mockResolvedValue(null); | ||
| mockedUpdateMessages.mockResolvedValue(0); | ||
| }); | ||
|
|
||
| it('fetches additional history batches until it fills the visible page when hidden system messages consume the first batch', async () => { | ||
| const firstBatch = Array.from({ length: 50 }, (_, index) => | ||
| buildMessage({ | ||
| id: `first-${index + 1}`, | ||
| ts: new Date(Date.UTC(2024, 0, 1, 0, 0, 50 - index)).toISOString(), | ||
| t: index < 49 ? 'uj' : undefined | ||
| }) | ||
| ); | ||
| const secondBatch = Array.from({ length: 50 }, (_, index) => | ||
| buildMessage({ | ||
| id: `second-${index + 1}`, | ||
| ts: new Date(Date.UTC(2023, 11, 31, 23, 59, 50 - index)).toISOString(), | ||
| t: index === 49 ? 'uj' : undefined | ||
| }) | ||
| ); | ||
|
|
||
| mockedSdkGet | ||
| .mockResolvedValueOnce({ success: true, messages: firstBatch } as any) | ||
| .mockResolvedValueOnce({ success: true, messages: secondBatch } as any); | ||
|
|
||
| await loadMessagesForRoom({ | ||
| rid: 'ROOM_ID', | ||
| t: 'c' | ||
| }); | ||
|
|
||
| expect(mockedSdkGet).toHaveBeenCalledTimes(2); | ||
| expect(mockedSdkGet).toHaveBeenNthCalledWith( | ||
| 2, | ||
| 'channels.history', | ||
| expect.objectContaining({ | ||
| roomId: 'ROOM_ID', | ||
| latest: firstBatch[firstBatch.length - 1].ts | ||
| }) | ||
| ); | ||
|
|
||
| expect(mockedUpdateMessages).toHaveBeenCalledTimes(2); | ||
| expect(mockedUpdateMessages).toHaveBeenNthCalledWith( | ||
| 1, | ||
| expect.objectContaining({ | ||
| rid: 'ROOM_ID', | ||
| update: expect.arrayContaining([ | ||
| expect.objectContaining({ _id: 'first-50' }), | ||
| expect.objectContaining({ _id: 'load-more-first-50', t: 'load_more' }) | ||
| ]) | ||
| }) | ||
| ); | ||
| expect(mockedUpdateMessages).toHaveBeenNthCalledWith( | ||
| 2, | ||
| expect.objectContaining({ | ||
| rid: 'ROOM_ID', | ||
| update: expect.arrayContaining([ | ||
| expect.objectContaining({ _id: 'first-50' }), | ||
| expect.objectContaining({ _id: 'second-49' }), | ||
| expect.objectContaining({ _id: 'load-more-second-50', t: 'load_more' }) | ||
| ]) | ||
| }) | ||
| ); | ||
|
|
||
| expect(mockedDispatch).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| type: ROOM.HISTORY_UI_LOADER_PUSH, | ||
| loaderId: 'load-more-first-50' | ||
| }) | ||
| ); | ||
| expect(mockedDispatch).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| type: ROOM.HISTORY_UI_LOADER_POP, | ||
| loaderId: 'load-more-first-50' | ||
| }) | ||
| ); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,28 +1,55 @@ | ||
| import dayjs from '../dayjs'; | ||
| import { MessageTypeLoad } from '../constants/messageTypeLoad'; | ||
| import { roomHistoryUiLoaderPop, roomHistoryUiLoaderPush } from '../../actions/room'; | ||
| import { type IMessage, type TMessageModel } from '../../definitions'; | ||
| import log from './helpers/log'; | ||
| import { getMessageById } from '../database/services/Message'; | ||
| import { getSubscriptionByRoomId } from '../database/services/Subscription'; | ||
| import { type RoomTypes, roomTypeToApiType } from './roomTypeToApiType'; | ||
| import sdk from '../services/sdk'; | ||
| import { store } from '../store/auxStore'; | ||
| import updateMessages from './updateMessages'; | ||
| import { generateLoadMoreId } from './helpers/generateLoadMoreId'; | ||
|
|
||
| const COUNT = 50; | ||
| const MAX_BATCHES = 10; | ||
|
|
||
| async function load({ rid: roomId, latest, t }: { rid: string; latest?: Date; t: RoomTypes }): Promise<IMessage[]> { | ||
| const apiType = roomTypeToApiType(t); | ||
| const isVisibleMainRoomMessage = (message: IMessage, hideSystemMessages: string[]) => | ||
| !message.tmid && (!message.t || !hideSystemMessages.includes(message.t)); | ||
|
|
||
| async function resolveHideSystemMessages(rid: string): Promise<string[]> { | ||
| const sub = await getSubscriptionByRoomId(rid); | ||
| if (Array.isArray(sub?.sysMes)) { | ||
| return sub.sysMes; | ||
| } | ||
| const fromSettings = store.getState().settings.Hide_System_Messages; | ||
| return Array.isArray(fromSettings) ? fromSettings : []; | ||
| } | ||
|
|
||
| async function load(args: { | ||
| rid: string; | ||
| latest?: Date; | ||
| t: RoomTypes; | ||
| loaderItem?: TMessageModel; | ||
| }): Promise<{ messages: IMessage[]; shouldAddLoader: boolean; uiLoaderId: string | null }> { | ||
| const roomId = args.rid; | ||
| const hideSystemMessages = await resolveHideSystemMessages(roomId); | ||
| const apiType = roomTypeToApiType(args.t); | ||
| if (!apiType) { | ||
| return []; | ||
| return { messages: [], shouldAddLoader: false, uiLoaderId: null }; | ||
| } | ||
|
|
||
| const allMessages: IMessage[] = []; | ||
| let mainMessagesCount = 0; | ||
| let visibleMainMessagesCount = 0; | ||
| let batchesFetched = 0; | ||
| let shouldAddLoader = false; | ||
| let uiLoaderId: string | null = null; | ||
|
|
||
| async function fetchBatch(lastTs?: string): Promise<void> { | ||
| if (allMessages.length >= COUNT) { | ||
| if (visibleMainMessagesCount >= COUNT || batchesFetched >= MAX_BATCHES) { | ||
| return; | ||
| } | ||
| batchesFetched += 1; | ||
|
|
||
| const params = { roomId, showThreadMessages: false, count: COUNT, ...(lastTs && { latest: lastTs }) }; | ||
|
|
||
|
|
@@ -47,21 +74,48 @@ async function load({ rid: roomId, latest, t }: { rid: string; latest?: Date; t: | |
|
|
||
| const batch = data.messages as IMessage[]; | ||
| allMessages.push(...batch); | ||
| shouldAddLoader = batch.length === COUNT; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| const mainMessagesInBatch = batch.filter(message => !message.tmid); | ||
| mainMessagesCount += mainMessagesInBatch.length; | ||
| const visibleMainMessagesInBatch = batch.filter(message => isVisibleMainRoomMessage(message, hideSystemMessages)); | ||
| visibleMainMessagesCount += visibleMainMessagesInBatch.length; | ||
|
|
||
| const needsMoreMainMessages = mainMessagesCount < COUNT; | ||
| const needsMoreVisibleMainMessages = visibleMainMessagesCount < COUNT && batch.length === COUNT; | ||
|
|
||
| if (needsMoreMainMessages) { | ||
| if (needsMoreVisibleMainMessages) { | ||
| const lastMessage = batch[batch.length - 1]; | ||
|
|
||
| if (!args.loaderItem && batchesFetched === 1) { | ||
| const loadMoreMessage = { | ||
| _id: generateLoadMoreId(lastMessage._id as string), | ||
| rid: lastMessage.rid, | ||
| ts: dayjs(lastMessage.ts).subtract(1, 'millisecond').toString(), | ||
| t: MessageTypeLoad.MORE, | ||
| msg: lastMessage.msg | ||
| } as IMessage; | ||
|
|
||
| await updateMessages({ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicate concurrent fetch in the exact scenario this PR targets. Writing Cleanest fix: guard at |
||
| rid: roomId, | ||
| update: [...allMessages, loadMoreMessage], | ||
| loaderItem: args.loaderItem | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dead arg — the enclosing |
||
| }); | ||
| store.dispatch(roomHistoryUiLoaderPush({ loaderId: loadMoreMessage._id })); | ||
| uiLoaderId = loadMoreMessage._id; | ||
| } | ||
|
|
||
| await fetchBatch(lastMessage.ts as string); | ||
| } | ||
| } | ||
|
|
||
| const startTimestamp = latest ? new Date(latest).toISOString() : undefined; | ||
| await fetchBatch(startTimestamp); | ||
| return allMessages; | ||
| const startTimestamp = args.latest ? new Date(args.latest).toISOString() : undefined; | ||
| try { | ||
| await fetchBatch(startTimestamp); | ||
| return { messages: allMessages, shouldAddLoader, uiLoaderId }; | ||
| } catch (e) { | ||
| if (uiLoaderId) { | ||
| store.dispatch(roomHistoryUiLoaderPop({ loaderId: uiLoaderId })); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cleanup paths are split: this inner catch pops on throw + rethrows, and the outer |
||
| } | ||
| throw e; | ||
| } | ||
| } | ||
|
|
||
| export function loadMessagesForRoom(args: { | ||
|
|
@@ -71,12 +125,15 @@ export function loadMessagesForRoom(args: { | |
| loaderItem?: TMessageModel; | ||
| }): Promise<void> { | ||
| return new Promise(async (resolve, reject) => { | ||
| let uiLoaderId: string | null = null; | ||
| try { | ||
| const data = await load(args); | ||
| const { messages, shouldAddLoader, uiLoaderId: pushedLoaderId } = await load(args); | ||
| uiLoaderId = pushedLoaderId; | ||
| const data = messages; | ||
| if (data?.length) { | ||
| const lastMessage = data[data.length - 1]; | ||
| const lastMessageRecord = await getMessageById(lastMessage._id as string); | ||
| if (!lastMessageRecord && data.length === COUNT) { | ||
| if (!lastMessageRecord && shouldAddLoader) { | ||
| const loadMoreMessage = { | ||
| _id: generateLoadMoreId(lastMessage._id as string), | ||
| rid: lastMessage.rid, | ||
|
|
@@ -93,6 +150,10 @@ export function loadMessagesForRoom(args: { | |
| } catch (e) { | ||
| log(e); | ||
| reject(e); | ||
| } finally { | ||
| if (uiLoaderId) { | ||
| store.dispatch(roomHistoryUiLoaderPop({ loaderId: uiLoaderId })); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,6 +68,18 @@ export default function (state = initialState, action: TActionsRoom): IRoom { | |
| ...state, | ||
| historyLoaders: state.historyLoaders.filter(loaderId => loaderId !== action.loaderId) | ||
| }; | ||
| case ROOM.HISTORY_UI_LOADER_PUSH: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| return { | ||
| ...state, | ||
| historyLoaders: state.historyLoaders.includes(action.loaderId) | ||
| ? state.historyLoaders | ||
| : [...state.historyLoaders, action.loaderId] | ||
| }; | ||
| case ROOM.HISTORY_UI_LOADER_POP: | ||
| return { | ||
| ...state, | ||
| historyLoaders: state.historyLoaders.filter(loaderId => loaderId !== action.loaderId) | ||
| }; | ||
| default: | ||
| return state; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverage gap: a single happy-path test. Suggest adding cases for:
MAX_BATCHEScap (stops at 10 even if still under 50 visible).< COUNT→ noshouldAddLoaderon final write.sub.sysMes === trueboolean → falls back to settings.loaderItemprovided → no intermediate loader inserted.loadMessagesForRoom.ts, once theuseMessagesguard is added.