diff --git a/.changeset/giphy-collapse-state-scroll.md b/.changeset/giphy-collapse-state-scroll.md new file mode 100644 index 0000000000000..13f4ed28c72b0 --- /dev/null +++ b/.changeset/giphy-collapse-state-scroll.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': patch +--- + +Fixes an issue where message attachments lose their collapsed state when scrolled out of view. diff --git a/apps/meteor/client/components/message/content/Attachments.tsx b/apps/meteor/client/components/message/content/Attachments.tsx index 70574ad6a1e53..bac6ba1fbeac4 100644 --- a/apps/meteor/client/components/message/content/Attachments.tsx +++ b/apps/meteor/client/components/message/content/Attachments.tsx @@ -7,11 +7,18 @@ export type AttachmentsProps = { attachments: MessageAttachmentBase[]; id?: string | undefined; source?: AudioAttachmentSource; + /** Prefixes nested attachments' collapse-state keys so they don't collide with the top-level ones. */ + keyPrefix?: string; }; -const Attachments = ({ attachments, id, source }: AttachmentsProps) => { +const Attachments = ({ attachments, id, source, keyPrefix }: AttachmentsProps) => { return ( - <>{attachments?.map((attachment, index) => )} + <> + {attachments?.map((attachment, index) => { + const path = keyPrefix ? `${keyPrefix}-${index}` : String(index); + return ; + })} + ); }; diff --git a/apps/meteor/client/components/message/content/attachments/AttachmentsItem.tsx b/apps/meteor/client/components/message/content/attachments/AttachmentsItem.tsx index 82d0ed9ccf60a..2bf82fa59d255 100644 --- a/apps/meteor/client/components/message/content/attachments/AttachmentsItem.tsx +++ b/apps/meteor/client/components/message/content/attachments/AttachmentsItem.tsx @@ -10,19 +10,20 @@ import type { AudioAttachmentSource } from './file/AudioAttachment'; export type AttachmentsItemProps = { attachment: MessageAttachmentBase; id: string | undefined; + path: string; source?: AudioAttachmentSource; }; -const AttachmentsItem = ({ attachment, id, source }: AttachmentsItemProps) => { +const AttachmentsItem = ({ attachment, id, path, source }: AttachmentsItemProps) => { if (isFileAttachment(attachment)) { return ; } if (isQuoteAttachment(attachment)) { - return ; + return ; } - return ; + return ; }; export default memo(AttachmentsItem); diff --git a/apps/meteor/client/components/message/content/attachments/DefaultAttachment.tsx b/apps/meteor/client/components/message/content/attachments/DefaultAttachment.tsx index 42e774b2e55dc..3e3240e90a2d5 100644 --- a/apps/meteor/client/components/message/content/attachments/DefaultAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/DefaultAttachment.tsx @@ -25,10 +25,12 @@ const applyMarkdownIfRequires = ( variant: ComponentProps['variant'] = 'inline', ): ReactNode => (list?.includes(key) ? : text); -export type DefaultAttachmentProps = MessageAttachmentDefault; +export type DefaultAttachmentProps = MessageAttachmentDefault & { + collapseKey?: string; +}; -const DefaultAttachment = (attachment: DefaultAttachmentProps) => { - const [collapsed, toggleCollapse] = useCollapse(!!attachment.collapsed); +const DefaultAttachment = ({ collapseKey, ...attachment }: DefaultAttachmentProps) => { + const [collapsed, toggleCollapse] = useCollapse(!!attachment.collapsed, collapseKey); return ( { +export const QuoteAttachment = ({ attachment, source, path }: QuoteAttachmentProps) => { const formatTime = useTimeAgo(); const displayAvatarPreference = useUserPreference('displayAvatars'); @@ -71,6 +73,7 @@ export const QuoteAttachment = ({ attachment, source }: QuoteAttachmentProps) => attachments={attachment.attachments} id={attachment.attachments[0]?.title_link} source={source && { rid: source.rid, mid: source.mid, name: attachment.author_name }} + keyPrefix={path} /> )} diff --git a/apps/meteor/client/components/message/hooks/useCollapse.ts b/apps/meteor/client/components/message/hooks/useCollapse.ts index 2dd422f37b93c..2ba0ec8584630 100644 --- a/apps/meteor/client/components/message/hooks/useCollapse.ts +++ b/apps/meteor/client/components/message/hooks/useCollapse.ts @@ -2,8 +2,30 @@ import { useToggle } from '@rocket.chat/fuselage-hooks'; import { useAttachmentIsCollapsedByDefault } from '@rocket.chat/ui-contexts'; import { useCallback } from 'react'; -export const useCollapse = (attachmentCollapsed?: boolean) => { +import { useIsCollapsibleToggled } from './useIsCollapsibleToggled'; +import { RoomManager, useOpenedRoom } from '../../../lib/RoomManager'; + +// `key` identifies this collapsible within the room's store, so its toggled state survives +// the row unmounting and remounting (e.g. virtua recycling it on scroll), it falls back to +// plain local state. +export const useCollapse = (attachmentCollapsed?: boolean, key?: string) => { const collapseByDefault = useAttachmentIsCollapsedByDefault(); - const [collapsed, toggleCollapsed] = useToggle(collapseByDefault || attachmentCollapsed); - return [collapsed, useCallback(() => toggleCollapsed(), [toggleCollapsed])] as const; + const defaultCollapsed = !!(collapseByDefault || attachmentCollapsed); + + const rid = useOpenedRoom(); + const toggled = useIsCollapsibleToggled(key); + const [localCollapsed, toggleLocalCollapsed] = useToggle(defaultCollapsed); + + const togglePersistedCollapsed = useCallback(() => { + if (!key || !rid) { + return; + } + RoomManager.getStore(rid)?.toggleCollapsible(key); + }, [key, rid]); + + if (key) { + return [toggled !== defaultCollapsed, togglePersistedCollapsed] as const; + } + + return [localCollapsed, () => toggleLocalCollapsed()] as const; }; diff --git a/apps/meteor/client/components/message/hooks/useIsCollapsibleToggled.spec.ts b/apps/meteor/client/components/message/hooks/useIsCollapsibleToggled.spec.ts new file mode 100644 index 0000000000000..7afaaaf3b4d6a --- /dev/null +++ b/apps/meteor/client/components/message/hooks/useIsCollapsibleToggled.spec.ts @@ -0,0 +1,75 @@ +import { act, renderHook } from '@testing-library/react'; + +import { useIsCollapsibleToggled } from './useIsCollapsibleToggled'; +import { RoomManager } from '../../../lib/RoomManager'; +import { MAX_TOGGLED_COLLAPSIBLES_PER_ROOM } from '../../../lib/constants'; + +jest.mock('../../../../app/ui-utils/client/lib/RoomHistoryManager', () => ({ + RoomHistoryManager: {}, +})); + +it('should not be toggled by default', () => { + RoomManager.open('room-a'); + const { result } = renderHook(() => useIsCollapsibleToggled('key-a')); + expect(result.current).toBe(false); +}); + +it('should reflect an already-toggled key in the opened room store', () => { + RoomManager.open('room-b'); + RoomManager.getStore('room-b')?.toggleCollapsible('key-b'); + + const { result } = renderHook(() => useIsCollapsibleToggled('key-b')); + expect(result.current).toBe(true); +}); + +it('should re-render when the store toggles after mount', () => { + RoomManager.open('room-c'); + const { result } = renderHook(() => useIsCollapsibleToggled('key-c')); + expect(result.current).toBe(false); + + act(() => { + RoomManager.getStore('room-c')?.toggleCollapsible('key-c'); + }); + expect(result.current).toBe(true); +}); + +it('should not leak a toggle to a different key in the same room', () => { + RoomManager.open('room-d'); + RoomManager.getStore('room-d')?.toggleCollapsible('key-d1'); + + const { result } = renderHook(() => useIsCollapsibleToggled('key-d2')); + expect(result.current).toBe(false); +}); + +it('should be a no-op when there is no key', () => { + RoomManager.open('room-e'); + const { result } = renderHook(() => useIsCollapsibleToggled(undefined)); + expect(result.current).toBe(false); +}); + +it('should be a no-op when there is no opened room', () => { + const currentlyOpened = RoomManager.opened; + if (currentlyOpened) { + RoomManager.back(currentlyOpened); + } + + const { result } = renderHook(() => useIsCollapsibleToggled('key-f')); + expect(result.current).toBe(false); +}); + +it('should evict the oldest toggle once a room store is full, keeping it bounded', () => { + RoomManager.open('room-bound'); + const store = RoomManager.getStore('room-bound'); + if (!store) { + throw new Error('store was not created'); + } + + const total = MAX_TOGGLED_COLLAPSIBLES_PER_ROOM + 1; + for (let i = 0; i < total; i++) { + store.toggleCollapsible(`bulk-${i}`); + } + + expect(store.isCollapsibleToggled('bulk-0')).toBe(false); + expect(store.isCollapsibleToggled('bulk-1')).toBe(true); + expect(store.isCollapsibleToggled(`bulk-${total - 1}`)).toBe(true); +}); diff --git a/apps/meteor/client/components/message/hooks/useIsCollapsibleToggled.ts b/apps/meteor/client/components/message/hooks/useIsCollapsibleToggled.ts new file mode 100644 index 0000000000000..567f74353ab81 --- /dev/null +++ b/apps/meteor/client/components/message/hooks/useIsCollapsibleToggled.ts @@ -0,0 +1,25 @@ +import { useMemo, useSyncExternalStore } from 'react'; + +import { RoomManager, getCollapsibleEventKey, useOpenedRoom } from '../../../lib/RoomManager'; + +export const useIsCollapsibleToggled = (key: string | undefined): boolean => { + const rid = useOpenedRoom(); + + const { subscribe, getSnapshot } = useMemo(() => { + const store = rid && key ? RoomManager.getStore(rid) : undefined; + + if (!store || !key) { + return { + subscribe: () => () => undefined, + getSnapshot: () => false, + }; + } + + return { + subscribe: (cb: () => void) => store.on(getCollapsibleEventKey(key), cb), + getSnapshot: () => store.isCollapsibleToggled(key), + }; + }, [rid, key]); + + return useSyncExternalStore(subscribe, getSnapshot); +}; diff --git a/apps/meteor/client/components/message/variants/RoomMessage.spec.tsx b/apps/meteor/client/components/message/variants/RoomMessage.spec.tsx index fe931e262a352..a3be23018eb43 100644 --- a/apps/meteor/client/components/message/variants/RoomMessage.spec.tsx +++ b/apps/meteor/client/components/message/variants/RoomMessage.spec.tsx @@ -44,6 +44,11 @@ jest.mock('../../../views/room/MessageList/hooks/useAutoTranslate', () => ({ }), })); jest.mock('../../../lib/actionLinks', () => undefined); +jest.mock('../../../lib/RoomManager', () => ({ + RoomManager: { getStore: jest.fn() }, + useOpenedRoom: () => undefined, + getCollapsibleEventKey: (key: string) => `collapsibleToggled-${key}`, +})); it('should show normal message', () => { render( diff --git a/apps/meteor/client/lib/RoomManager.ts b/apps/meteor/client/lib/RoomManager.ts index cbdeb6f0f0843..6070f1949c710 100644 --- a/apps/meteor/client/lib/RoomManager.ts +++ b/apps/meteor/client/lib/RoomManager.ts @@ -2,14 +2,20 @@ import type { IRoom } from '@rocket.chat/core-typings'; import { Emitter } from '@rocket.chat/emitter'; import { useMemo, useSyncExternalStore } from 'react'; +import { MAX_TOGGLED_COLLAPSIBLES_PER_ROOM } from './constants'; import { getConfig } from './utils/getConfig'; import { LegacyRoomManager } from '../../app/ui-utils/client'; import { RoomHistoryManager } from '../../app/ui-utils/client/lib/RoomHistoryManager'; const debug = !!(getConfig('debug') || getConfig('debug-RoomStore')); +type CollapsibleKey = `collapsibleToggled-${string}`; + +export const getCollapsibleEventKey = (key: string): CollapsibleKey => `collapsibleToggled-${key}`; + class RoomStore extends Emitter<{ changed: undefined; + [key: CollapsibleKey]: undefined; }> { lastTime?: Date; @@ -19,6 +25,8 @@ class RoomStore extends Emitter<{ atBottom = true; + private readonly toggledCollapsibles = new Set(); + constructor(readonly rid: string) { super(); @@ -40,6 +48,29 @@ class RoomStore extends Emitter<{ this.emit('changed'); } } + + toggleCollapsible(key: string): void { + if (this.toggledCollapsibles.has(key)) { + this.toggledCollapsibles.delete(key); + this.emit(getCollapsibleEventKey(key)); + return; + } + + if (this.toggledCollapsibles.size >= MAX_TOGGLED_COLLAPSIBLES_PER_ROOM) { + const oldest = this.toggledCollapsibles.values().next().value; + if (oldest !== undefined) { + this.toggledCollapsibles.delete(oldest); + this.emit(getCollapsibleEventKey(oldest)); + } + } + + this.toggledCollapsibles.add(key); + this.emit(getCollapsibleEventKey(key)); + } + + isCollapsibleToggled(key: string): boolean { + return this.toggledCollapsibles.has(key); + } } const debugRoomManager = !!(getConfig('debug') || getConfig('debug-RoomManager')); diff --git a/apps/meteor/client/lib/constants.ts b/apps/meteor/client/lib/constants.ts index 70cae5deff224..77c91c1256da0 100644 --- a/apps/meteor/client/lib/constants.ts +++ b/apps/meteor/client/lib/constants.ts @@ -3,3 +3,4 @@ export const BIO_TEXT_MAX_LENGTH = 260; export const VIDEOCONF_STACK_MAX_USERS = 6; export const NAVIGATION_REGION_ID = 'navigation-region'; export const MAX_FILE_SIZE_PREVIEW = 10485760; // 10MB +export const MAX_TOGGLED_COLLAPSIBLES_PER_ROOM = 1000; diff --git a/apps/meteor/client/views/room/MessageList/hooks/useKeepMountedMessages.ts b/apps/meteor/client/views/room/MessageList/hooks/useKeepMountedMessages.ts index b7267446b7c36..f486ade9b1f65 100644 --- a/apps/meteor/client/views/room/MessageList/hooks/useKeepMountedMessages.ts +++ b/apps/meteor/client/views/room/MessageList/hooks/useKeepMountedMessages.ts @@ -7,6 +7,11 @@ export const useKeepMountedMessages = (messages: IMessage[], canPreview: boolean // attachments (audio/video players) and URL previews (e.g. YouTube iframes). // Otherwise virtua recycles them on scroll-to-bottom (new message / reaction // growing a message) and the iframe flickers. + // + // Don't extend this to other message/attachment types just to preserve some local + // UI state. Every match here stays mounted for the life of the list regardless of + // scroll position, so it only scales for the "needs a live browser resource" case + // above and adding more to it defeats virtualization in media-heavy channels. const hasUrlPreview = message.urls?.some((url) => Object.keys(url.meta ?? {}).length > 0 || !!url.headers) ?? false; if ((message.files?.length ?? 0) > 0 || hasUrlPreview) { acc.push(index + offset); diff --git a/apps/meteor/tests/e2e/message-attachment-collapse.spec.ts b/apps/meteor/tests/e2e/message-attachment-collapse.spec.ts new file mode 100644 index 0000000000000..a842daf089193 --- /dev/null +++ b/apps/meteor/tests/e2e/message-attachment-collapse.spec.ts @@ -0,0 +1,204 @@ +import { faker } from '@faker-js/faker'; + +import { Users } from './fixtures/userStates'; +import { HomeChannel } from './page-objects'; +import { createTargetChannel, deleteChannel } from './utils'; +import { setUserPreferences } from './utils/setUserPreferences'; +import type { BaseTest } from './utils/test'; +import { expect, test } from './utils/test'; + +test.use({ storageState: Users.admin.state }); + +const TRANSPARENT_PIXEL = + 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII='; + +const postAttachmentMessage = async ( + api: BaseTest['api'], + channel: string, + { text, collapsed }: { text: string; collapsed?: boolean }, +): Promise => { + await api.post('/chat.postMessage', { + channel, + attachments: [{ title: 'GIPHY', text, image_url: TRANSPARENT_PIXEL, collapsed }], + }); +}; + +const floodChannel = (api: BaseTest['api'], channel: string, count: number): Promise => + Promise.all(Array.from({ length: count }, () => api.post('/chat.postMessage', { channel, text: faker.lorem.paragraphs(2) }))); + +test.describe.serial('Message Attachment Collapse', () => { + let poHomeChannel: HomeChannel; + + const scrollUp = async (): Promise => { + const scroller = poHomeChannel.content.mainMessageListScroller; + + await scroller.evaluate((el) => { + (el.firstElementChild as HTMLElement).scrollTop = 0; + }); + }; + + const waitForFloodToSettle = async (api: BaseTest['api'], channel: string): Promise => { + const marker = `flood settled ${faker.string.uuid()}`; + await api.post('/chat.postMessage', { channel, text: marker }); + await expect(poHomeChannel.content.mainMessageList.getByText(marker)).toBeVisible(); + }; + + test.describe('Default preference (expanded by default)', () => { + let targetChannel: string; + + test.beforeAll(async ({ api }) => { + targetChannel = await createTargetChannel(api); + }); + + test.beforeEach(async ({ page }) => { + poHomeChannel = new HomeChannel(page); + await poHomeChannel.gotoChannel(targetChannel); + }); + + test.afterAll(async ({ api }) => { + await deleteChannel(api, targetChannel); + }); + + test('should collapse and re-expand an attachment on toggle click', async ({ api }) => { + const text = `attachment text ${faker.string.uuid()}`; + await postAttachmentMessage(api, targetChannel, { text }); + + await expect(poHomeChannel.content.mainMessageList.getByText(text)).toBeVisible(); + + await poHomeChannel.content.mainMessageList.getByRole('button', { name: 'Collapse' }).last().click(); + await expect(poHomeChannel.content.mainMessageList.getByText(text)).toBeHidden(); + + await poHomeChannel.content.mainMessageList.getByRole('button', { name: 'Uncollapse' }).last().click(); + await expect(poHomeChannel.content.mainMessageList.getByText(text)).toBeVisible(); + }); + + test('should not affect a different message when one attachment is toggled', async ({ api }) => { + const textA = `attachment text A ${faker.string.uuid()}`; + const textB = `attachment text B ${faker.string.uuid()}`; + await postAttachmentMessage(api, targetChannel, { text: textA }); + await postAttachmentMessage(api, targetChannel, { text: textB }); + + // Collapse only the most recent message's attachment (textB). + await poHomeChannel.content.mainMessageList.getByRole('button', { name: 'Collapse' }).last().click(); + + await expect(poHomeChannel.content.mainMessageList.getByText(textA)).toBeVisible(); + await expect(poHomeChannel.content.mainMessageList.getByText(textB)).toBeHidden(); + }); + + test('should keep a manually collapsed attachment collapsed after it is scrolled out of view and back', async ({ api }) => { + const text = `attachment text ${faker.string.uuid()}`; + await postAttachmentMessage(api, targetChannel, { text }); + + await expect(poHomeChannel.content.mainMessageList.getByText(text)).toBeVisible(); + await poHomeChannel.content.mainMessageList.getByRole('button', { name: 'Collapse' }).last().click(); + await expect(poHomeChannel.content.mainMessageList.getByText(text)).toBeHidden(); + + await floodChannel(api, targetChannel, 30); + await waitForFloodToSettle(api, targetChannel); + + await expect(async () => { + await scrollUp(); + await expect(poHomeChannel.content.mainMessageList.getByText(text)).toBeHidden({ timeout: 1000 }); + }).toPass(); + }); + }); + + test.describe('"Collapse Embedded Media by Default" preference enabled', () => { + let targetChannel: string; + + test.beforeAll(async ({ api }) => { + targetChannel = await createTargetChannel(api); + await setUserPreferences(api, { collapseMediaByDefault: true }); + }); + + test.beforeEach(async ({ page }) => { + poHomeChannel = new HomeChannel(page); + await poHomeChannel.gotoChannel(targetChannel); + }); + + test.afterAll(async ({ api }) => { + await setUserPreferences(api, { collapseMediaByDefault: false }); + await deleteChannel(api, targetChannel); + }); + + test('should render a new attachment collapsed by default', async ({ api }) => { + const text = `attachment text ${faker.string.uuid()}`; + await postAttachmentMessage(api, targetChannel, { text }); + + await expect(poHomeChannel.content.mainMessageList.getByRole('button', { name: 'Uncollapse' }).last()).toBeVisible(); + await expect(poHomeChannel.content.mainMessageList.getByText(text)).toBeHidden(); + }); + + test('should keep a manually expanded attachment expanded after it is scrolled out of view and back', async ({ api }) => { + const text = `attachment text ${faker.string.uuid()}`; + await postAttachmentMessage(api, targetChannel, { text }); + + await expect(poHomeChannel.content.mainMessageList.getByText(text)).toBeHidden(); + await poHomeChannel.content.mainMessageList.getByRole('button', { name: 'Uncollapse' }).last().click(); + await expect(poHomeChannel.content.mainMessageList.getByText(text)).toBeVisible(); + + await floodChannel(api, targetChannel, 30); + await waitForFloodToSettle(api, targetChannel); + + await expect(async () => { + await scrollUp(); + await expect(poHomeChannel.content.mainMessageList.getByText(text)).toBeVisible({ timeout: 1000 }); + }).toPass(); + }); + }); + + test.describe('Nested attachments (quoted messages)', () => { + let targetChannel: string; + + test.beforeAll(async ({ api }) => { + targetChannel = await createTargetChannel(api); + }); + + test.beforeEach(async ({ page }) => { + poHomeChannel = new HomeChannel(page); + await poHomeChannel.gotoChannel(targetChannel); + }); + + test.afterAll(async ({ api }) => { + await deleteChannel(api, targetChannel); + }); + + test('should keep a quoted attachment collapse state independent from the original message it quotes', async ({ api }) => { + const text = `nested attachment text ${faker.string.uuid()}`; + const quoteReplyText = `quoting the attachment ${faker.string.uuid()}`; + await postAttachmentMessage(api, targetChannel, { text, collapsed: false }); + + await expect(poHomeChannel.content.mainMessageList.getByText(text)).toBeVisible(); + + await poHomeChannel.content.lastUserMessage.hover(); + await poHomeChannel.content.btnQuoteMessage.click(); + await poHomeChannel.content.sendMessage(quoteReplyText); + + const originalRow = poHomeChannel.content.messageListItems.filter({ hasText: text, hasNotText: quoteReplyText }); + const quoteRow = poHomeChannel.content.messageListItems.filter({ hasText: quoteReplyText }); + + // The original message's attachment is untouched by quoting it. + await expect(originalRow.getByText(text)).toBeVisible(); + + const quoteTextVisibleBeforeToggle = await quoteRow.getByText(text).isVisible(); + + // Toggling the ORIGINAL message's attachment must not affect the nested copy inside the quote. + await originalRow.getByRole('button', { name: 'Collapse' }).click(); + await expect(originalRow.getByText(text)).toBeHidden(); + if (quoteTextVisibleBeforeToggle) { + await expect(quoteRow.getByText(text)).toBeVisible(); + } else { + await expect(quoteRow.getByText(text)).toBeHidden(); + } + + // Toggling the NESTED copy inside the quote must not affect the now collapsed original. + await quoteRow.getByRole('button', { name: quoteTextVisibleBeforeToggle ? 'Collapse' : 'Uncollapse' }).click(); + if (quoteTextVisibleBeforeToggle) { + await expect(quoteRow.getByText(text)).toBeHidden(); + } else { + await expect(quoteRow.getByText(text)).toBeVisible(); + } + await expect(originalRow.getByText(text)).toBeHidden(); + }); + }); +});