Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion app/actions/actionsTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ export const ROOM = createRequestTypes('ROOM', [
'FORWARD',
'USER_TYPING',
'HISTORY_REQUEST',
'HISTORY_FINISHED'
'HISTORY_FINISHED',
'HISTORY_UI_LOADER_PUSH',
'HISTORY_UI_LOADER_POP'
]);
export const INQUIRY = createRequestTypes('INQUIRY', [
...defaultTypes,
Expand Down
26 changes: 25 additions & 1 deletion app/actions/room.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,24 @@ export interface IRoomHistoryFinished extends Action {
loaderId: string;
}

export interface IRoomHistoryUiLoaderPush extends Action {
loaderId: string;
}

export interface IRoomHistoryUiLoaderPop extends Action {
loaderId: string;
}

export type TActionsRoom = TSubscribeRoom &
TUnsubscribeRoom &
ILeaveRoom &
IDeleteRoom &
IForwardRoom &
IUserTyping &
IRoomHistoryRequest &
IRoomHistoryFinished;
IRoomHistoryFinished &
IRoomHistoryUiLoaderPush &
IRoomHistoryUiLoaderPop;

export function subscribeRoom(rid: string): TSubscribeRoom {
return {
Expand Down Expand Up @@ -138,3 +148,17 @@ export function roomHistoryFinished({ loaderId }: { loaderId: string }): IRoomHi
loaderId
};
}

export function roomHistoryUiLoaderPush({ loaderId }: { loaderId: string }): IRoomHistoryUiLoaderPush {
return {
type: ROOM.HISTORY_UI_LOADER_PUSH,
loaderId
};
}

export function roomHistoryUiLoaderPop({ loaderId }: { loaderId: string }): IRoomHistoryUiLoaderPop {
return {
type: ROOM.HISTORY_UI_LOADER_POP,
loaderId
};
}
125 changes: 125 additions & 0 deletions app/lib/methods/loadMessagesForRoom.test.ts
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'
})
);
});
Copy link
Copy Markdown
Member

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_BATCHES cap (stops at 10 even if still under 50 visible).
  • Last batch < COUNT → no shouldAddLoader on final write.
  • Error mid-recursion → loader id gets popped (current test only asserts PUSH+POP on success).
  • sub.sysMes === true boolean → falls back to settings.
  • loaderItem provided → no intermediate loader inserted.
  • The duplicate-fetch flow flagged in loadMessagesForRoom.ts, once the useMessages guard is added.

});
89 changes: 75 additions & 14 deletions app/lib/methods/loadMessagesForRoom.ts
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 }) };

Expand All @@ -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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

shouldAddLoader is overwritten on every batch — final value reflects only the last one. That's the intended semantics (only the trailing batch's tail tells us whether more older history may exist), but it reads like a bug. Rename to lastBatchWasFull or add a one-line comment.


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({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Duplicate concurrent fetch in the exact scenario this PR targets.

Writing load-more-first-50 to DB here causes WatermelonDB's subscription in useMessages to emit. The auto-dispatch effect at app/views/RoomView/List/hooks/useMessages.ts:153-169 then fires roomHistoryRequest for that loader id — its guards (serverVersion >= 3.16.0 + hideSystemMessages.length > 0) are exactly the bug scenario. The saga watchHistoryRequests (app/sagas/room.js:16-32) has no in-flight dedup, so it spawns a second loadMessagesForRoom while batch 2 of the first is still recursing. Both converge in DB and MAX_BATCHES / MAX_AUTO_LOADS cap blast radius, but you get 2× HTTP traffic on cold open.

Cleanest fix: guard at useMessages.ts:164if (state.room.historyLoaders.includes(loaderId)) return;. The HISTORY_UI_LOADER_PUSH already populated historyLoaders, so the existing PUSH/POP plumbing covers it without new state.

rid: roomId,
update: [...allMessages, loadMoreMessage],
loaderItem: args.loaderItem
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Dead arg — the enclosing if (!args.loaderItem && batchesFetched === 1) guarantees args.loaderItem is undefined here. Drop the line.

});
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 }));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 finally (line 155) also pops. No double-pop — the outer uiLoaderId only gets assigned after await load(args) resolves, so on the throw path it stays null and the outer skips. But the split is subtle. Letting only the outer finally own cleanup is simpler.

}
throw e;
}
}

export function loadMessagesForRoom(args: {
Expand All @@ -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,
Expand All @@ -93,6 +150,10 @@ export function loadMessagesForRoom(args: {
} catch (e) {
log(e);
reject(e);
} finally {
if (uiLoaderId) {
store.dispatch(roomHistoryUiLoaderPop({ loaderId: uiLoaderId }));
}
}
});
}
13 changes: 13 additions & 0 deletions app/reducers/room.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import {
removedRoom,
roomHistoryFinished,
roomHistoryRequest,
roomHistoryUiLoaderPop,
roomHistoryUiLoaderPush,
subscribeRoom,
unsubscribeRoom
} from '../actions/room';
Expand Down Expand Up @@ -69,4 +71,15 @@ describe('test room reducer', () => {
const { historyLoaders } = mockedStore.getState().room;
expect(historyLoaders).toEqual([]);
});

it('should append loader id after HISTORY_UI_LOADER_PUSH', () => {
mockedStore.dispatch(roomHistoryUiLoaderPush({ loaderId: 'ui-loader' }));
expect(mockedStore.getState().room.historyLoaders).toContain('ui-loader');
});

it('should remove loader id after HISTORY_UI_LOADER_POP', () => {
mockedStore.dispatch(roomHistoryUiLoaderPush({ loaderId: 'pop-me' }));
mockedStore.dispatch(roomHistoryUiLoaderPop({ loaderId: 'pop-me' }));
expect(mockedStore.getState().room.historyLoaders).not.toContain('pop-me');
});
});
12 changes: 12 additions & 0 deletions app/reducers/room.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

HISTORY_UI_LOADER_PUSH/POP and HISTORY_REQUEST/HISTORY_FINISHED both mutate historyLoaders. The new push dedupes via .includes; the old one (line 61-65) doesn't. Naming is also asymmetric. Suggest renaming for symmetry — makes "this dispatches a saga" vs "this is pure UI state" obvious at the call site:

  • HISTORY_FETCH_REQUESTED / HISTORY_FETCH_COMPLETED (saga-triggering)
  • HISTORY_LOADER_ADDED / HISTORY_LOADER_REMOVED (reducer-only)

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;
}
Expand Down
Loading
Loading