Skip to content

Commit

Permalink
Timeline: When scrolling far into history, discard newest messages
Browse files Browse the repository at this point in the history 
Co-authored-by: Scott Nonnenberg <scott@signal.org>
  • Loading branch information
@automated-signal @scottnonnenberg-signal
automated-signal and scottnonnenberg-signal committed May 11, 2022
1 parent 19fe677 commit f416155
Show file tree
Hide file tree
Showing 3 changed files with 171 additions and 33 deletions.
67 changes: 58 additions & 9 deletions ts/components/conversation/Timeline.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,12 @@ import type { PropsType as SmartContactSpoofingReviewDialogPropsType } from '../
import type { GroupNameCollisionsWithIdsByTitle } from '../../util/groupMemberNameCollisions';
import { hasUnacknowledgedCollisions } from '../../util/groupMemberNameCollisions';
import { TimelineFloatingHeader } from './TimelineFloatingHeader';
import type { TimelineMessageLoadingState } from '../../util/timelineUtil';
import {
ScrollAnchor,
UnreadIndicatorPlacement,
getScrollAnchorBeforeUpdate,
getWidthBreakpoint,
ScrollAnchor,
TimelineMessageLoadingState,
UnreadIndicatorPlacement,
} from '../../util/timelineUtil';
import {
getScrollBottom,
Expand All @@ -53,6 +53,7 @@ const AT_BOTTOM_DETECTOR_STYLE = { height: AT_BOTTOM_THRESHOLD };

const MIN_ROW_HEIGHT = 18;
const SCROLL_DOWN_BUTTON_THRESHOLD = 8;
const LOAD_NEWER_THRESHOLD = 5;

export type WarningType =
| {
Expand Down Expand Up @@ -109,7 +110,13 @@ type PropsHousekeepingType = {
contactSpoofingReview?: ContactSpoofingReviewPropType;

discardMessages: (
_: Readonly<{ conversationId: string; numberToKeepAtBottom: number }>
_: Readonly<
| {
conversationId: string;
numberToKeepAtBottom: number;
}
| { conversationId: string; numberToKeepAtTop: number }
>
) => void;
getTimestampForMessage: (messageId: string) => undefined | number;
getPreferredBadge: PreferredBadgeSelectorType;
Expand Down Expand Up @@ -487,10 +494,15 @@ export class Timeline extends React.Component<
if (newestBottomVisibleMessageId) {
this.markNewestBottomVisibleMessageRead();

const rowIndex = getRowIndexFromElement(newestBottomVisible);
const maxRowIndex = items.length - 1;

if (
!messageLoadingState &&
!haveNewest &&
newestBottomVisibleMessageId === last(items)
isNumber(rowIndex) &&
maxRowIndex >= 0 &&
rowIndex >= maxRowIndex - LOAD_NEWER_THRESHOLD
) {
loadNewerMessages(newestBottomVisibleMessageId);
}
Expand Down Expand Up @@ -633,8 +645,16 @@ export class Timeline extends React.Component<
_prevState: Readonly<StateType>,
snapshot: Readonly<SnapshotType>
): void {
const { items: oldItems } = prevProps;
const { discardMessages, id, items: newItems } = this.props;
const {
items: oldItems,
messageLoadingState: previousMessageLoadingState,
} = prevProps;
const {
discardMessages,
id,
items: newItems,
messageLoadingState,
} = this.props;

const containerEl = this.containerRef.current;
if (containerEl && snapshot) {
Expand Down Expand Up @@ -662,12 +682,33 @@ export class Timeline extends React.Component<
this.updateIntersectionObserver();

// This condition is somewhat arbitrary.
const numberToKeepAtBottom = this.maxVisibleRows * 2;
const shouldDiscardOlderMessages: boolean =
this.isAtBottom() && newItems.length >= this.maxVisibleRows * 1.5;
this.isAtBottom() && newItems.length > numberToKeepAtBottom;
if (shouldDiscardOlderMessages) {
discardMessages({
conversationId: id,
numberToKeepAtBottom: this.maxVisibleRows,
numberToKeepAtBottom,
});
}

const loadingStateThatJustFinished:
| undefined
| TimelineMessageLoadingState =
!messageLoadingState && previousMessageLoadingState
? previousMessageLoadingState
: undefined;
const numberToKeepAtTop = this.maxVisibleRows * 5;
const shouldDiscardNewerMessages: boolean =
!this.isAtBottom() &&
loadingStateThatJustFinished ===
TimelineMessageLoadingState.LoadingOlderMessages &&
newItems.length > numberToKeepAtTop;

if (shouldDiscardNewerMessages) {
discardMessages({
conversationId: id,
numberToKeepAtTop,
});
}
} else {
Expand Down Expand Up @@ -1195,6 +1236,14 @@ function getMessageIdFromElement(
return element instanceof HTMLElement ? element.dataset.messageId : undefined;
}

function getRowIndexFromElement(
element: undefined | Element
): undefined | number {
return element instanceof HTMLElement && element.dataset.itemIndex
? parseInt(element.dataset.itemIndex, 10)
: undefined;
}

function showDebugLog() {
window.showDebugLog();
}
90 changes: 66 additions & 24 deletions ts/state/ducks/conversations.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ import type { NoopActionType } from './noop';
import { conversationJobQueue } from '../../jobs/conversationJobQueue';
import type { TimelineMessageLoadingState } from '../../util/timelineUtil';
import { isGroup } from '../../util/whatTypeOfConversation';
import { missingCaseError } from '../../util/missingCaseError';

// State

Expand Down Expand Up @@ -465,7 +466,13 @@ type CustomColorRemovedActionType = {
};
type DiscardMessagesActionType = {
type: typeof DISCARD_MESSAGES;
payload: { conversationId: string; numberToKeepAtBottom: number };
payload: Readonly<
| {
conversationId: string;
numberToKeepAtBottom: number;
}
| { conversationId: string; numberToKeepAtTop: number }
>;
};
type SetPreJoinConversationActionType = {
type: 'SET_PRE_JOIN_CONVERSATION';
Expand Down Expand Up @@ -2195,35 +2202,70 @@ export function reducer(
}

if (action.type === DISCARD_MESSAGES) {
const { conversationId, numberToKeepAtBottom } = action.payload;
const { conversationId } = action.payload;
if ('numberToKeepAtBottom' in action.payload) {
const { numberToKeepAtBottom } = action.payload;
const conversationMessages = getOwn(
state.messagesByConversation,
conversationId
);
if (!conversationMessages) {
return state;
}

const conversationMessages = getOwn(
state.messagesByConversation,
conversationId
);
if (!conversationMessages) {
return state;
}
const { messageIds: oldMessageIds } = conversationMessages;
if (oldMessageIds.length <= numberToKeepAtBottom) {
return state;
}

const { messageIds: oldMessageIds } = conversationMessages;
if (oldMessageIds.length <= numberToKeepAtBottom) {
return state;
const messageIdsToRemove = oldMessageIds.slice(0, -numberToKeepAtBottom);
const messageIdsToKeep = oldMessageIds.slice(-numberToKeepAtBottom);

return {
...state,
messagesLookup: omit(state.messagesLookup, messageIdsToRemove),
messagesByConversation: {
...state.messagesByConversation,
[conversationId]: {
...conversationMessages,
messageIds: messageIdsToKeep,
},
},
};
}

const messageIdsToRemove = oldMessageIds.slice(0, -numberToKeepAtBottom);
const messageIdsToKeep = oldMessageIds.slice(-numberToKeepAtBottom);
if ('numberToKeepAtTop' in action.payload) {
const { numberToKeepAtTop } = action.payload;
const conversationMessages = getOwn(
state.messagesByConversation,
conversationId
);
if (!conversationMessages) {
return state;
}

return {
...state,
messagesLookup: omit(state.messagesLookup, messageIdsToRemove),
messagesByConversation: {
...state.messagesByConversation,
[conversationId]: {
...conversationMessages,
messageIds: messageIdsToKeep,
const { messageIds: oldMessageIds } = conversationMessages;
if (oldMessageIds.length <= numberToKeepAtTop) {
return state;
}

const messageIdsToRemove = oldMessageIds.slice(numberToKeepAtTop);
const messageIdsToKeep = oldMessageIds.slice(0, numberToKeepAtTop);

return {
...state,
messagesLookup: omit(state.messagesLookup, messageIdsToRemove),
messagesByConversation: {
...state.messagesByConversation,
[conversationId]: {
...conversationMessages,
messageIds: messageIdsToKeep,
},
},
},
};
};
}

throw missingCaseError(action.payload);
}

if (action.type === 'SET_PRE_JOIN_CONVERSATION') {
Expand Down
47 changes: 47 additions & 0 deletions ts/test-electron/state/ducks/conversations_test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const {
closeRecommendedGroupSizeModal,
conversationStoppedByMissingVerification,
createGroup,
discardMessages,
openConversationInternal,
repairNewestMessage,
repairOldestMessage,
Expand Down Expand Up @@ -1306,6 +1307,52 @@ describe('both/state/ducks/conversations', () => {
});
});

describe('DISCARD_MESSAGES', () => {
const startState: ConversationsStateType = {
...getEmptyState(),
messagesLookup: {
[messageId]: getDefaultMessage(messageId),
[messageIdTwo]: getDefaultMessage(messageIdTwo),
[messageIdThree]: getDefaultMessage(messageIdThree),
},
messagesByConversation: {
[conversationId]: {
metrics: {
totalUnseen: 0,
},
scrollToMessageCounter: 0,
messageIds: [messageId, messageIdTwo, messageIdThree],
},
},
};

it('eliminates older messages', () => {
const toDiscard = {
conversationId,
numberToKeepAtBottom: 2,
};
const state = reducer(startState, discardMessages(toDiscard));

assert.deepEqual(
state.messagesByConversation[conversationId]?.messageIds,
[messageIdTwo, messageIdThree]
);
});

it('eliminates newer messages', () => {
const toDiscard = {
conversationId,
numberToKeepAtTop: 2,
};
const state = reducer(startState, discardMessages(toDiscard));

assert.deepEqual(
state.messagesByConversation[conversationId]?.messageIds,
[messageId, messageIdTwo]
);
});
});

describe('SET_PRE_JOIN_CONVERSATION', () => {
const startState = {
...getEmptyState(),
Expand Down

0 comments on commit f416155

Please sign in to comment.