From f254ed679be3c16a76e363ddff542e7a7b721042 Mon Sep 17 00:00:00 2001 From: Hampus Date: Fri, 11 Sep 2026 22:02:24 +0200 Subject: [PATCH] fix(app): never lower the read-state unread watermark (#2704) --- .../messaging/commands/MessageCommands.tsx | 1 - .../read_state/state/ReadStates.test.ts | 45 ++---- .../features/read_state/state/ReadStates.ts | 18 +-- .../ReadStatesDeletedTailMessage.test.ts | 133 ++++++++++++++++++ 4 files changed, 145 insertions(+), 52 deletions(-) create mode 100644 fluxer_app/src/features/read_state/state/ReadStatesDeletedTailMessage.test.ts diff --git a/fluxer_app/src/features/messaging/commands/MessageCommands.tsx b/fluxer_app/src/features/messaging/commands/MessageCommands.tsx index 581a8271d..7cbe87236 100644 --- a/fluxer_app/src/features/messaging/commands/MessageCommands.tsx +++ b/fluxer_app/src/features/messaging/commands/MessageCommands.tsx @@ -279,7 +279,6 @@ function handleMessageFetchSuccess( channelId, isAfter: pageState.isAfter, messages, - tailProbeWatermarkId: tailProbe?.watermarkMessageId ?? null, }); MessageReferences.handleMessagesFetchSuccess(channelId, messages); void requestMissingGuildMembers(channelId, messages); diff --git a/fluxer_app/src/features/read_state/state/ReadStates.test.ts b/fluxer_app/src/features/read_state/state/ReadStates.test.ts index 49182c758..b2ff3662a 100644 --- a/fluxer_app/src/features/read_state/state/ReadStates.test.ts +++ b/fluxer_app/src/features/read_state/state/ReadStates.test.ts @@ -118,55 +118,32 @@ describe('ReadStates unread invariant', () => { expect(ReadStates.hasUnread(channelId)).toBe(true); }); - it('still lets its own probe lower a watermark a passive update raised', () => { + it('never lowers the watermark when an after page comes back empty', () => { const {channelId} = seedReadChannel(); loadedMessages.push({id: ID.ack, author: {id: 'someone'}}); ReadStates.handlePassiveLastMessageUpdates({[channelId]: ID.newer}, 'guild-1'); - ReadStates.handleLoadMessages({channelId, isAfter: true, messages: [], tailProbeWatermarkId: ID.newer}); - expect(ReadStates.lastMessageId(channelId)).toBe(ID.ack); - expect(ReadStates.hasUnread(channelId)).toBe(false); + ReadStates.handleLoadMessages({channelId, isAfter: true, messages: []}); + expect(ReadStates.lastMessageId(channelId)).toBe(ID.newer); + expect(ReadStates.hasUnread(channelId)).toBe(true); }); - it('lowers a watermark its own probe finds nothing behind', () => { - const {channelId, state} = seedReadChannel(); - state.lastMessageId = ID.newer; - loadedMessages.push({id: ID.ack, author: {id: 'someone'}}); - ReadStates.handleLoadMessages({channelId, isAfter: true, messages: [], tailProbeWatermarkId: ID.newer}); - expect(ReadStates.lastMessageId(channelId)).toBe(ID.ack); - expect(ReadStates.hasUnread(channelId)).toBe(false); - }); - - it('keeps a watermark that is ahead when an ordinary after page comes back empty', () => { + it('keeps a watermark that points at a message no longer in the channel', () => { const {channelId, state} = seedReadChannel(); state.lastMessageId = ID.newer; loadedMessages.push({id: ID.ack, author: {id: 'someone'}}); ReadStates.handleLoadMessages({channelId, isAfter: true, messages: []}); expect(ReadStates.lastMessageId(channelId)).toBe(ID.newer); + expect(ReadStates.hasUnread(channelId)).toBe(true); }); - it('keeps a watermark that advanced while its own probe was in flight', () => { + it('acks up to the watermark so a deleted newest message cannot keep the channel unread', () => { const {channelId, state} = seedReadChannel(); state.lastMessageId = ID.newer; loadedMessages.push({id: ID.ack, author: {id: 'someone'}}); - ReadStates.handleLoadMessages({channelId, isAfter: true, messages: [], tailProbeWatermarkId: ID.ack}); - expect(ReadStates.lastMessageId(channelId)).toBe(ID.newer); - }); - - it('keeps a watermark that is ahead when the page was not an after page', () => { - const {channelId, state} = seedReadChannel(); - state.lastMessageId = ID.newer; - loadedMessages.push({id: ID.ack, author: {id: 'someone'}}); - ReadStates.handleLoadMessages({channelId, messages: [], tailProbeWatermarkId: ID.newer}); - expect(ReadStates.lastMessageId(channelId)).toBe(ID.newer); - }); - - it('keeps a watermark that is ahead when the window is not at the live edge', () => { - const {channelId, state} = seedReadChannel(); - hasNewestMessages = false; - state.lastMessageId = ID.newer; - loadedMessages.push({id: ID.ack, author: {id: 'someone'}}); - ReadStates.handleLoadMessages({channelId, isAfter: true, messages: [], tailProbeWatermarkId: ID.newer}); - expect(ReadStates.lastMessageId(channelId)).toBe(ID.newer); + ReadStates.handleLoadMessages({channelId, isAfter: true, messages: []}); + ReadStates.handleChannelAckWithStickyUnread({channelId}); + expect(ReadStates.ackMessageId(channelId)).toBe(ID.newer); + expect(ReadStates.hasUnread(channelId)).toBe(false); }); it('anchors the divider when a window is loaded whose ack sits outside it', () => { diff --git a/fluxer_app/src/features/read_state/state/ReadStates.ts b/fluxer_app/src/features/read_state/state/ReadStates.ts index fc4bb0f46..e4e2eb45b 100644 --- a/fluxer_app/src/features/read_state/state/ReadStates.ts +++ b/fluxer_app/src/features/read_state/state/ReadStates.ts @@ -439,12 +439,7 @@ class ReadStates { }); } - handleLoadMessages(action: { - channelId: string; - isAfter?: boolean; - messages: Array; - tailProbeWatermarkId?: string | null; - }): void { + handleLoadMessages(action: {channelId: string; isAfter?: boolean; messages: Array}): void { const state = this.get(action.channelId); state.messagesLoaded = true; const messages = Messages.getMessages(action.channelId); @@ -453,17 +448,6 @@ class ReadStates { state.lastMessageId = newestMessage.id; } const landedOnNewestWindow = messages.hasNewestMessages(); - if ( - action.isAfter && - action.tailProbeWatermarkId != null && - action.tailProbeWatermarkId === state.lastMessageId && - action.messages.length === 0 && - landedOnNewestWindow && - newestMessage != null && - isNewerMessageId(state.lastMessageId, newestMessage.id) - ) { - state.lastMessageId = newestMessage.id; - } const landedOnAck = state.ackMessageId != null && messages.jumpDestinationId === state.ackMessageId; if (state.hasUnread() || landedOnNewestWindow || landedOnAck) { state.rebuild(); diff --git a/fluxer_app/src/features/read_state/state/ReadStatesDeletedTailMessage.test.ts b/fluxer_app/src/features/read_state/state/ReadStatesDeletedTailMessage.test.ts new file mode 100644 index 000000000..efedb71c7 --- /dev/null +++ b/fluxer_app/src/features/read_state/state/ReadStatesDeletedTailMessage.test.ts @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +import {Endpoints} from '@app/features/app/constants/Endpoints'; +import {Channel} from '@app/features/channel/models/Channel'; +import {ACK_BATCH_DELAY_MS, type GatewayReadState} from '@app/features/read_state/state/read_states/shared'; +import {ChannelTypes} from '@fluxer/constants/src/ChannelConstants'; +import type {Channel as WireChannel} from '@fluxer/schema/src/domains/channel/ChannelSchemas'; +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; + +const channels = new Map(); +let pinnedToEnd = false; +let automaticAck = false; +let hasNewest = true; +const loadedMessages: Array<{id: string; author: {id: string}}> = []; + +vi.mock('@app/features/app/state/RuntimeConfig', () => ({default: {localInstanceDomain: 'fluxer.test'}})); +vi.mock('@app/features/channel/state/Channels', () => ({default: {getChannel: (id: string) => channels.get(id)}})); +vi.mock('@app/features/messaging/state/MessagingMessages', () => ({ + default: { + getMessages: () => ({ + get hasMoreBefore() { + return false; + }, + get length() { + return loadedMessages.length; + }, + jumpDestinationId: null, + hasNewestMessages: () => hasNewest, + has: (id: string) => loadedMessages.some((m) => m.id === id), + last: () => loadedMessages[loadedMessages.length - 1], + forEachBuffered: (cb: (m: unknown) => void) => { + for (const m of loadedMessages) cb(m); + }, + }), + }, +})); +vi.mock('@app/features/user/state/Users', () => ({ + default: {getCurrentUser: () => ({id: 'me'}), cacheUsers: () => {}}, +})); +vi.mock('@app/features/relationship/state/Relationships', () => ({default: {isBlocked: () => false}})); +vi.mock('@app/features/member/state/GuildMembers', () => ({default: {getMember: () => null}})); +vi.mock('@app/features/user/state/UserGuildSettings', () => ({ + default: { + isEveryoneMentionSuppressed: () => false, + isRoleMentionSuppressed: () => false, + isGuildOrChannelMuted: () => false, + }, +})); +vi.mock('@app/features/ui/state/Dimension', () => ({default: {channelPinnedToEnd: () => pinnedToEnd}})); +vi.mock('@app/features/notification/state/NotificationAutoAck', () => ({ + default: {isAutomaticAckEnabled: () => automaticAck, disableForChannel: () => {}}, +})); +vi.mock('@app/features/platform/transport/RestTransport', () => ({ + http: {post: vi.fn(async () => ({body: {read_states: []}})), get: vi.fn()}, +})); + +const {default: ReadStates} = await import('@app/features/read_state/state/ReadStates'); +const {http} = await import('@app/features/platform/transport/RestTransport'); + +const CHANNEL = '1431490439357088063'; +const PHANTOM = '1547836192152621056'; +const REAL = '1546984669772255232'; +const OLD_ACK = '1546000000000000000'; + +function guildText(lastMessageId: string): WireChannel { + return {id: CHANNEL, type: ChannelTypes.GUILD_TEXT, guild_id: '1431490056488128806', last_message_id: lastMessageId}; +} + +function readState(ackMessageId: string): GatewayReadState { + return {id: CHANNEL, last_message_id: ackMessageId, mention_count: 0, version: '1'}; +} + +function ready(ackMessageId: string): void { + channels.clear(); + channels.set(CHANNEL, new Channel(guildText(PHANTOM))); + ReadStates.handleGatewayReady({readState: [readState(ackMessageId)], channels: [guildText(PHANTOM)]}); +} + +function lastAckedMessageId(): string | null { + const calls = vi.mocked(http.post).mock.calls; + if (calls.length === 0) return null; + const body = calls[calls.length - 1][1] as {body: {read_states: Array<{message_id: string}>}}; + return body.body.read_states[0].message_id; +} + +describe('channel whose newest message was deleted', () => { + beforeEach(() => { + vi.useFakeTimers(); + loadedMessages.length = 0; + pinnedToEnd = false; + automaticAck = false; + hasNewest = true; + vi.mocked(http.post).mockClear(); + }); + + afterEach(() => { + ReadStates.clearAll(); + vi.useRealTimers(); + }); + + it('acks up to the channel watermark rather than the newest surviving message', async () => { + ready(OLD_ACK); + expect(ReadStates.hasUnread(CHANNEL)).toBe(true); + loadedMessages.push({id: REAL, author: {id: 'tuna'}}); + ReadStates.handleLoadMessages({channelId: CHANNEL, isAfter: false, messages: [{id: REAL}] as never}); + ReadStates.handleLoadMessages({channelId: CHANNEL, isAfter: true, messages: []}); + expect(ReadStates.lastMessageId(CHANNEL)).toBe(PHANTOM); + pinnedToEnd = true; + automaticAck = true; + ReadStates.handleChannelAckWithStickyUnread({channelId: CHANNEL}); + await vi.advanceTimersByTimeAsync(ACK_BATCH_DELAY_MS); + expect(http.post).toHaveBeenCalledWith(Endpoints.READ_STATES_ACK, { + body: {read_states: [{channel_id: CHANNEL, message_id: PHANTOM}]}, + }); + expect(ReadStates.hasUnread(CHANNEL)).toBe(false); + }); + + it('stays read after a reload', async () => { + ready(OLD_ACK); + loadedMessages.push({id: REAL, author: {id: 'tuna'}}); + ReadStates.handleLoadMessages({channelId: CHANNEL, isAfter: false, messages: [{id: REAL}] as never}); + ReadStates.handleLoadMessages({channelId: CHANNEL, isAfter: true, messages: []}); + pinnedToEnd = true; + automaticAck = true; + ReadStates.handleChannelAckWithStickyUnread({channelId: CHANNEL}); + await vi.advanceTimersByTimeAsync(ACK_BATCH_DELAY_MS); + const acked = lastAckedMessageId(); + expect(acked).not.toBeNull(); + ReadStates.clearAll(); + ready(acked as string); + expect(ReadStates.hasUnread(CHANNEL)).toBe(false); + }); +});