From 7d400fce2366c8c04aa6a55325920be64bb88df2 Mon Sep 17 00:00:00 2001 From: Jackie Jou Date: Tue, 26 May 2026 16:31:15 -0700 Subject: [PATCH 1/6] feat(activity-feed-v2): improve comment posting UX Mention popover (V2): - Open the mention popover on @ before any character is typed - Show the start prompt while empty and "No users found" once the user types - Skip the API round-trip on empty or whitespace-only queries Comment text (V1 + V2): - Trim leading and trailing whitespace from comment text before sending to the API. Inner whitespace is preserved. V1 post button: - Disable the post button when the editor is empty or whitespace-only, matching the validation already enforced on submit. V2 auto-scroll after posting: - After a successful post, scroll to the new item using a snapshot of ids taken at post time, so a concurrent push from another user does not hijack the viewport. - Await onCommentCreate so rejections are logged and do not arm the scroll for an unrelated update. --- .../__tests__/utils.test.js | 14 + .../draft-js-mention-selector/utils.js | 2 +- .../activity-feed-v2/ActivityFeedV2.scss | 4 + .../activity-feed-v2/ActivityFeedV2.tsx | 49 ++- .../__tests__/ActivityFeedV2.test.tsx | 303 +++++++++++++++++- .../__tests__/helpers.test.ts | 13 + .../activity-feed-v2/helpers.ts | 3 +- .../activity-feed/comment-form/CommentForm.js | 3 +- .../comment-form/CommentFormControls.js | 5 +- .../__tests__/CommentForm.test.js | 16 + .../__tests__/CommentFormControls.test.js | 39 +++ 11 files changed, 433 insertions(+), 18 deletions(-) create mode 100644 src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentFormControls.test.js diff --git a/src/components/form-elements/draft-js-mention-selector/__tests__/utils.test.js b/src/components/form-elements/draft-js-mention-selector/__tests__/utils.test.js index dcfe057adc..a7f7dae0e1 100644 --- a/src/components/form-elements/draft-js-mention-selector/__tests__/utils.test.js +++ b/src/components/form-elements/draft-js-mention-selector/__tests__/utils.test.js @@ -236,5 +236,19 @@ describe('components/form-elements/draft-js-mention-selector/utils', () => { expect(getFormattedCommentText(dummyEditorState)).toEqual(expected); }); + + test.each` + input | expected + ${' hello '} | ${'hello'} + ${'\n\nhello\n\n'} | ${'hello'} + ${'\thello\t'} | ${'hello'} + ${' \t\nhello\n '} | ${'hello'} + ${'hello world'} | ${'hello world'} + ${' hello world '} | ${'hello world'} + `('should trim leading and trailing whitespace from "$input"', ({ input, expected }) => { + const dummyEditorState = EditorState.createWithContent(ContentState.createFromText(input)); + + expect(getFormattedCommentText(dummyEditorState)).toEqual({ text: expected, hasMention: false }); + }); }); }); diff --git a/src/components/form-elements/draft-js-mention-selector/utils.js b/src/components/form-elements/draft-js-mention-selector/utils.js index d404eb6eb0..af2a808232 100644 --- a/src/components/form-elements/draft-js-mention-selector/utils.js +++ b/src/components/form-elements/draft-js-mention-selector/utils.js @@ -163,7 +163,7 @@ function getFormattedCommentText(editorState: EditorState): { hasMention: boolea // Concatenate the array of block strings with newlines // (Each block represents a paragraph) - return { text: resultStringArr.join('\n'), hasMention }; + return { text: resultStringArr.join('\n').trim(), hasMention }; } export { diff --git a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.scss b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.scss index 3df463eb02..23c9b84f2e 100644 --- a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.scss +++ b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.scss @@ -53,4 +53,8 @@ button svg { pointer-events: auto; } + + &-mentionEmpty { + padding: var(--bp-space-030) var(--bp-space-040); + } } diff --git a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx index 1f93b1675e..690f758d55 100644 --- a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx +++ b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx @@ -8,14 +8,14 @@ import * as React from 'react'; import noop from 'lodash/noop'; -import { useIntl } from 'react-intl'; +import { FormattedMessage, useIntl } from 'react-intl'; import { ActivityFeed, useActivityFeedScroll } from '@box/activity-feed'; -import { serializeMentionMarkup } from '@box/threaded-annotations'; import TaskModal from '../TaskModal'; import FeedItemRow from './FeedItemRow'; +import { serializeEditorContent } from './helpers'; import { transformFeedItem } from './transformers'; import type { ActivityFeedV2Props, TransformedFeedItem, UserContact } from './types'; @@ -24,6 +24,7 @@ import type { TaskAssigneeCollection, TaskNew } from '../../../common/types/task import { TASK_COMPLETION_RULE_ALL, TASK_EDIT_MODE_EDIT, TASK_TYPE_APPROVAL } from '../../../constants'; import commonMessages from '../../common/messages'; +import draftJsMentionSelectorMessages from '../../../components/form-elements/draft-js-mention-selector/messages'; import messages from '../messages'; import './ActivityFeedV2.scss'; @@ -68,14 +69,16 @@ const ActivityFeedV2 = ({ const scrolledEntryIdRef = React.useRef(null); const hasScrolledToEndRef = React.useRef(false); + const knownIdsBeforePostRef = React.useRef | null>(null); const fetchUsers = React.useCallback( async (inputValue: string): Promise => { - if (!getMentionAsync) { + const trimmed = inputValue.trim(); + if (!trimmed || !getMentionAsync) { return []; } try { - const entries = await getMentionAsync(inputValue); + const entries = await getMentionAsync(trimmed); return entries.map((c: Record) => ({ email: (c.email as string) ?? (c.login as string) ?? '', id: Number(c.id) || 0, @@ -114,10 +117,20 @@ const ActivityFeedV2 = ({ const userSelectorProps = React.useMemo( () => ({ + allowEmptyQuery: true, ariaRoleDescription: intl.formatMessage(messages.mentionUserSelectorRoleDescription), fetchAvatarUrls, fetchUsers, loadingAriaLabel: intl.formatMessage(messages.mentionUserSelectorLoading), + renderEmpty: (value: string) => ( +
+ +
+ ), }), [fetchAvatarUrls, fetchUsers, intl], ); @@ -247,19 +260,33 @@ const ActivityFeedV2 = ({ } }, [activeFeedEntryId, filteredItems, scrollHandle]); + // Scroll only to items added since the post snapshot, so a concurrent push from + // another user doesn't hijack the viewport. + React.useEffect(() => { + const knownIds = knownIdsBeforePostRef.current; + if (!knownIds || !scrollHandle) return; + const newItem = filteredItems.find(item => !knownIds.has(item.id)); + if (!newItem) return; + if (scrollHandle.scrollTo(newItem.id)) { + knownIdsBeforePostRef.current = null; + } + }, [filteredItems, scrollHandle]); + const handleCommentPost = React.useCallback( async (content: unknown) => { if (!onCommentCreate) return; - let serialized; + const serialized = serializeEditorContent(content); + if (!serialized || !serialized.text) return; try { - serialized = serializeMentionMarkup(content as Parameters[0]); - } catch { - return; + const snapshot = new Set(filteredItems.map(item => item.id)); + await onCommentCreate(serialized.text, serialized.hasMention); + knownIdsBeforePostRef.current = snapshot; + } catch (error) { + // eslint-disable-next-line no-console + console.error('ActivityFeedV2: failed to post comment', error); } - if (!serialized.text.trim()) return; - onCommentCreate(serialized.text, serialized.hasMention); }, - [onCommentCreate], + [filteredItems, onCommentCreate], ); return ( diff --git a/src/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsx b/src/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsx index 9b3aee1118..87fe599957 100644 --- a/src/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsx +++ b/src/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsx @@ -1,9 +1,15 @@ import * as React from 'react'; +import { ActivityFeed } from '@box/activity-feed'; + import { act, render, screen } from '../../../../test-utils/testing-library'; import ActivityFeedV2 from '..'; import type { ActivityFeedV2Props } from '../ActivityFeedV2'; +type EditorProps = React.ComponentProps; + +const mockSerializeMentionMarkup = jest.fn((doc: unknown) => ({ hasMention: false, text: JSON.stringify(doc) })); + jest.mock('@box/threaded-annotations', () => ({ AnnotationBadgeType: { Drawing: 'drawing', @@ -12,7 +18,7 @@ jest.mock('@box/threaded-annotations', () => ({ Point: 'point', Region: 'region', }, - serializeMentionMarkup: (doc: unknown) => ({ hasMention: false, text: JSON.stringify(doc) }), + serializeMentionMarkup: (doc: unknown) => mockSerializeMentionMarkup(doc), })); const mockScrollTo = jest.fn(() => true); @@ -20,6 +26,7 @@ const mockScrollTo = jest.fn(() => true); type FilterOptionProps = { checked?: boolean; onCheckedChange?: (checked: boolean) => void }; let lastShowResolvedOptionProps: FilterOptionProps = {}; let lastMentionMeOptionProps: FilterOptionProps = {}; +let lastEditorProps: Partial = {}; jest.mock('@box/activity-feed', () => { const actual = jest.requireActual('@box/activity-feed'); @@ -37,7 +44,10 @@ jest.mock('@box/activity-feed', () => {
ThreadedAnnotation
); ActivityFeedList.Version = (props: { id: string }) =>
Version
; - const ActivityFeedEditor = () =>
Editor
; + const ActivityFeedEditor = (props: Partial) => { + lastEditorProps = props; + return
Editor
; + }; const ActivityFeedHeader = ({ children }: { children: React.ReactNode }) => (
{children}
); @@ -153,6 +163,11 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { mockScrollTo.mockReturnValue(true); lastShowResolvedOptionProps = {}; lastMentionMeOptionProps = {}; + lastEditorProps = {}; + mockSerializeMentionMarkup.mockImplementation((doc: unknown) => ({ + hasMention: false, + text: JSON.stringify(doc), + })); }); afterEach(() => { @@ -323,6 +338,129 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { expect(screen.getByTestId('activity-feed-root')).toBeVisible(); }); + describe('mention popover behavior', () => { + test('should pass allowEmptyQuery=true so the popover opens on @ before any character is typed', () => { + render(); + + expect(lastEditorProps.userSelectorProps?.allowEmptyQuery).toBe(true); + }); + + test('should skip the API call when fetchUsers is invoked with an empty query', async () => { + const getMentionAsync = jest.fn().mockResolvedValue([{ id: '1', name: 'foo' }]); + render( + , + ); + + const result = await lastEditorProps.userSelectorProps?.fetchUsers?.(''); + + expect(getMentionAsync).not.toHaveBeenCalled(); + expect(result).toEqual([]); + }); + + test('should skip the API call when fetchUsers is invoked with a whitespace-only query', async () => { + const getMentionAsync = jest.fn().mockResolvedValue([{ id: '1', name: 'foo' }]); + render( + , + ); + + const result = await lastEditorProps.userSelectorProps?.fetchUsers?.(' '); + + expect(getMentionAsync).not.toHaveBeenCalled(); + expect(result).toEqual([]); + }); + + test('should call getMentionAsync with the trimmed value and shape results for a non-empty query', async () => { + const getMentionAsync = jest.fn().mockResolvedValue([ + { email: 'a@b.com', id: '7', name: 'Alice' }, + { id: '8', login: 'bob@b.com', name: 'Bob' }, + ]); + render( + , + ); + + const result = await lastEditorProps.userSelectorProps?.fetchUsers?.(' al '); + + expect(getMentionAsync).toHaveBeenCalledWith('al'); + expect(result).toEqual([ + { email: 'a@b.com', id: 7, name: 'Alice', value: '7' }, + { email: 'bob@b.com', id: 8, name: 'Bob', value: '8' }, + ]); + }); + + test('should render the V1-style start prompt via renderEmpty when value is empty', () => { + render(); + + const empty = lastEditorProps.userSelectorProps?.renderEmpty?.(''); + render(<>{empty}); + + expect(screen.getByText('Mention someone to notify them')).toBeVisible(); + }); + + test('should render the V1-style start prompt via renderEmpty when value is whitespace-only', () => { + render(); + + const empty = lastEditorProps.userSelectorProps?.renderEmpty?.(' '); + render(<>{empty}); + + expect(screen.getByText('Mention someone to notify them')).toBeVisible(); + }); + + test('should render the no-users-found message via renderEmpty when value is non-empty', () => { + render(); + + const empty = lastEditorProps.userSelectorProps?.renderEmpty?.('xyz'); + render(<>{empty}); + + expect(screen.getByText('No users found')).toBeVisible(); + }); + }); + + describe('comment posting', () => { + test('should call onCommentCreate with trimmed text when the editor posts content', async () => { + mockSerializeMentionMarkup.mockReturnValue({ hasMention: true, text: ' hello world ' }); + const onCommentCreate = jest.fn(); + render( + , + ); + + await lastEditorProps.onPost?.({ type: 'doc', content: [] }); + + expect(onCommentCreate).toHaveBeenCalledWith('hello world', true); + }); + + test('should skip onCommentCreate when the trimmed text is empty', async () => { + mockSerializeMentionMarkup.mockReturnValue({ hasMention: false, text: ' \n\t ' }); + const onCommentCreate = jest.fn(); + render( + , + ); + + await lastEditorProps.onPost?.({ type: 'doc', content: [] }); + + expect(onCommentCreate).not.toHaveBeenCalled(); + }); + }); + describe('filter controls', () => { test('should default showResolved and showOnlyMentionsMe to false in the filter menu', () => { render(); @@ -522,4 +660,165 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { expect(mockScrollTo).toHaveBeenCalledTimes(1); }); }); + + describe('scroll to user post', () => { + const newComment = { ...mockComment, id: 'new-comment', tagged_message: 'fresh post' }; + const strangerComment = { ...mockComment, id: 'stranger-comment', tagged_message: 'unrelated' }; + + test('should scroll to the new item once feedItems updates after a post', async () => { + const onCommentCreate = jest.fn(); + const { rerender } = render( + , + ); + mockScrollTo.mockClear(); + + await lastEditorProps.onPost?.({ type: 'doc', content: [] }); + expect(onCommentCreate).toHaveBeenCalled(); + expect(mockScrollTo).not.toHaveBeenCalled(); + + rerender( + , + ); + + expect(mockScrollTo).toHaveBeenLastCalledWith('new-comment'); + }); + + test('should not scroll when onCommentCreate rejects (post failed)', async () => { + const consoleError = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const onCommentCreate = jest.fn().mockRejectedValue(new Error('network error')); + const { rerender } = render( + , + ); + mockScrollTo.mockClear(); + + await lastEditorProps.onPost?.({ type: 'doc', content: [] }); + rerender( + , + ); + + expect(consoleError).toHaveBeenCalledWith('ActivityFeedV2: failed to post comment', expect.any(Error)); + expect(mockScrollTo).not.toHaveBeenCalled(); + consoleError.mockRestore(); + }); + + test('should not scroll to a concurrent push that arrives without a user post', async () => { + const { rerender } = render( + , + ); + mockScrollTo.mockClear(); + + rerender( + , + ); + + expect(mockScrollTo).not.toHaveBeenCalled(); + }); + + test('should scroll to the user post even when a stranger post lands in the same render', async () => { + const onCommentCreate = jest.fn(); + const { rerender } = render( + , + ); + mockScrollTo.mockClear(); + + await lastEditorProps.onPost?.({ type: 'doc', content: [] }); + rerender( + , + ); + + expect(mockScrollTo).toHaveBeenCalledWith('new-comment'); + expect(mockScrollTo).not.toHaveBeenCalledWith('stranger-comment'); + }); + + test('should not scroll when serializeEditorContent yields no text', async () => { + mockSerializeMentionMarkup.mockReturnValue({ hasMention: false, text: ' ' }); + const onCommentCreate = jest.fn(); + const { rerender } = render( + , + ); + mockScrollTo.mockClear(); + + await lastEditorProps.onPost?.({ type: 'doc', content: [] }); + rerender( + , + ); + + expect(onCommentCreate).not.toHaveBeenCalled(); + expect(mockScrollTo).not.toHaveBeenCalled(); + }); + + test('should retry the scroll on the next feedItems change when scrollTo returns false', async () => { + mockScrollTo.mockReturnValue(false); + const onCommentCreate = jest.fn(); + const { rerender } = render( + , + ); + mockScrollTo.mockClear(); + mockScrollTo.mockReturnValue(false); + + await lastEditorProps.onPost?.({ type: 'doc', content: [] }); + rerender( + , + ); + expect(mockScrollTo).toHaveBeenLastCalledWith('new-comment'); + + mockScrollTo.mockClear(); + mockScrollTo.mockReturnValue(true); + rerender( + , + ); + + expect(mockScrollTo).toHaveBeenLastCalledWith('new-comment'); + }); + }); }); diff --git a/src/elements/content-sidebar/activity-feed-v2/__tests__/helpers.test.ts b/src/elements/content-sidebar/activity-feed-v2/__tests__/helpers.test.ts index a591d24cb3..3763b2cf09 100644 --- a/src/elements/content-sidebar/activity-feed-v2/__tests__/helpers.test.ts +++ b/src/elements/content-sidebar/activity-feed-v2/__tests__/helpers.test.ts @@ -43,6 +43,19 @@ describe('elements/content-sidebar/activity-feed-v2/helpers', () => { expect(mockedSerialize).toHaveBeenCalledWith(content); }); + test.each` + input | expected + ${' hello '} | ${'hello'} + ${'\n\nhello world\n\n'} | ${'hello world'} + ${'\thello\t'} | ${'hello'} + ${' \t\nhello\n '} | ${'hello'} + ${' leading and inner '} | ${'leading and inner'} + `('should trim leading and trailing whitespace from "$input"', ({ input, expected }) => { + mockedSerialize.mockReturnValue({ hasMention: true, text: input }); + + expect(serializeEditorContent({})).toEqual({ hasMention: true, text: expected }); + }); + test('should log via console.error and return null when serialize throws', () => { const consoleError = jest.spyOn(console, 'error').mockImplementation(() => undefined); mockedSerialize.mockImplementation(() => { diff --git a/src/elements/content-sidebar/activity-feed-v2/helpers.ts b/src/elements/content-sidebar/activity-feed-v2/helpers.ts index 4547b83f2e..daf7b22496 100644 --- a/src/elements/content-sidebar/activity-feed-v2/helpers.ts +++ b/src/elements/content-sidebar/activity-feed-v2/helpers.ts @@ -13,7 +13,8 @@ type EditorContent = Parameters[0]; export const serializeEditorContent = (content: unknown): ReturnType | null => { try { - return serializeMentionMarkup(content as EditorContent); + const serialized = serializeMentionMarkup(content as EditorContent); + return { ...serialized, text: serialized.text.trim() }; } catch (error) { // eslint-disable-next-line no-console console.error('ActivityFeedV2: failed to serialize editor content', error); diff --git a/src/elements/content-sidebar/activity-feed/comment-form/CommentForm.js b/src/elements/content-sidebar/activity-feed/comment-form/CommentForm.js index fb1f1bf336..f74791970f 100644 --- a/src/elements/content-sidebar/activity-feed/comment-form/CommentForm.js +++ b/src/elements/content-sidebar/activity-feed/comment-form/CommentForm.js @@ -147,6 +147,7 @@ class CommentForm extends React.Component { const allowVideoTimestamps = isVideo && istimestampedCommentsEnabled; const timestampLabel = allowVideoTimestamps ? formatMessage(messages.commentTimestampLabel) : undefined; const { commentEditorState } = this.state; + const isPostDisabled = !commentEditorState?.getCurrentContent().getPlainText().trim(); const inputContainerClassNames = classNames('bcs-CommentForm', className, { 'bcs-is-open': isOpen, }); @@ -186,7 +187,7 @@ class CommentForm extends React.Component { )} - {isOpen && } + {isOpen && } diff --git a/src/elements/content-sidebar/activity-feed/comment-form/CommentFormControls.js b/src/elements/content-sidebar/activity-feed/comment-form/CommentFormControls.js index 2fa86fc65c..c0e23db1e0 100644 --- a/src/elements/content-sidebar/activity-feed/comment-form/CommentFormControls.js +++ b/src/elements/content-sidebar/activity-feed/comment-form/CommentFormControls.js @@ -13,15 +13,16 @@ import messages from './messages'; import { ACTIVITY_TARGETS } from '../../../common/interactionTargets'; type Props = { + isDisabled?: boolean, onCancel: Function, }; -const CommentInputControls = ({ onCancel }: Props): React.Node => ( +const CommentInputControls = ({ isDisabled, onCancel }: Props): React.Node => (
- +
diff --git a/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js b/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js index 07a6dde59a..f11fcc9b43 100644 --- a/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js +++ b/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js @@ -169,4 +169,20 @@ describe('elements/content-sidebar/ActivityFeed/comment-form/CommentForm', () => }); expect(wrapper.find('DraftJSMentionSelector').at(0).prop('timestampLabel')).toBeUndefined(); }); + + describe('post button disabled state', () => { + const findControls = wrapper => wrapper.find('CommentInputControls'); + + test('should pass isDisabled=true to the controls when the editor is empty', () => { + const wrapper = getWrapper({ isOpen: true }); + + expect(findControls(wrapper).prop('isDisabled')).toBe(true); + }); + + test('should pass isDisabled=false to the controls when the editor has non-whitespace content', () => { + const wrapper = getWrapper({ isOpen: true, tagged_message: 'hello' }); + + expect(findControls(wrapper).prop('isDisabled')).toBe(false); + }); + }); }); diff --git a/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentFormControls.test.js b/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentFormControls.test.js new file mode 100644 index 0000000000..eb0e972328 --- /dev/null +++ b/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentFormControls.test.js @@ -0,0 +1,39 @@ +import * as React from 'react'; +import { render, screen } from '@testing-library/react'; +import { IntlProvider } from 'react-intl'; + +import CommentFormControls from '../CommentFormControls'; + +jest.mock('react-intl', () => ({ + ...jest.requireActual('react-intl'), + FormattedMessage: ({ defaultMessage }) => {defaultMessage}, +})); + +const renderControls = (props = {}) => + render( + + {}} {...props} /> + , + ); + +describe('elements/content-sidebar/ActivityFeed/comment-form/CommentFormControls', () => { + test('should render Post button enabled by default', () => { + renderControls(); + const post = screen.getByRole('button', { name: 'Post' }); + expect(post).not.toHaveAttribute('aria-disabled'); + expect(post).not.toHaveClass('is-disabled'); + }); + + test('should disable Post button when isDisabled is true', () => { + renderControls({ isDisabled: true }); + const post = screen.getByRole('button', { name: 'Post' }); + expect(post).toHaveAttribute('aria-disabled', 'true'); + expect(post).toHaveClass('is-disabled'); + }); + + test('should leave Cancel button enabled even when Post is disabled', () => { + renderControls({ isDisabled: true }); + const cancel = screen.getByRole('button', { name: 'Cancel' }); + expect(cancel).not.toHaveAttribute('aria-disabled'); + }); +}); From d485c37ad7e3f19a963ab276f283c05a6a3bc283 Mon Sep 17 00:00:00 2001 From: Jackie Jou Date: Tue, 26 May 2026 16:58:07 -0700 Subject: [PATCH 2/6] fix(activity-feed): drop optional chain on commentEditorState for flow commentEditorState is initialized in the constructor and is never undefined in production. The optional chain was a workaround for a test that mocked EditorState.moveFocusToEnd globally and leaked the mock across tests. Restore the original after each test instead. --- .../activity-feed/comment-form/CommentForm.js | 2 +- .../__tests__/CommentForm.test.js | 34 +++++++++++++------ 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/src/elements/content-sidebar/activity-feed/comment-form/CommentForm.js b/src/elements/content-sidebar/activity-feed/comment-form/CommentForm.js index f74791970f..186a4bfa6a 100644 --- a/src/elements/content-sidebar/activity-feed/comment-form/CommentForm.js +++ b/src/elements/content-sidebar/activity-feed/comment-form/CommentForm.js @@ -147,7 +147,7 @@ class CommentForm extends React.Component { const allowVideoTimestamps = isVideo && istimestampedCommentsEnabled; const timestampLabel = allowVideoTimestamps ? formatMessage(messages.commentTimestampLabel) : undefined; const { commentEditorState } = this.state; - const isPostDisabled = !commentEditorState?.getCurrentContent().getPlainText().trim(); + const isPostDisabled = !commentEditorState.getCurrentContent().getPlainText().trim(); const inputContainerClassNames = classNames('bcs-CommentForm', className, { 'bcs-is-open': isOpen, }); diff --git a/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js b/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js index f11fcc9b43..03fb0915a2 100644 --- a/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js +++ b/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js @@ -130,20 +130,32 @@ describe('elements/content-sidebar/ActivityFeed/comment-form/CommentForm', () => expect(wrapper.find('DraftJSMentionSelector').at(0).prop('placeholder')).toEqual('Your comment goes here'); }); - test('should not focus on textbox when shouldFocusOnOpen is false', () => { - const mockFocusFunc = jest.fn(); - EditorState.moveFocusToEnd = mockFocusFunc; + describe('moveFocusToEnd', () => { + let originalMoveFocusToEnd; - getWrapperRTL(); - expect(mockFocusFunc).not.toHaveBeenCalled(); - }); + beforeEach(() => { + originalMoveFocusToEnd = EditorState.moveFocusToEnd; + }); + + afterEach(() => { + EditorState.moveFocusToEnd = originalMoveFocusToEnd; + }); - test('should focus on textbox when shouldFocusOnOpen is true', () => { - const mockFocusFunc = jest.fn(); - EditorState.moveFocusToEnd = mockFocusFunc; + test('should not focus on textbox when shouldFocusOnOpen is false', () => { + const mockFocusFunc = jest.fn(); + EditorState.moveFocusToEnd = mockFocusFunc; - getWrapperRTL({ shouldFocusOnOpen: true }); - expect(mockFocusFunc).toHaveBeenCalled(); + getWrapperRTL(); + expect(mockFocusFunc).not.toHaveBeenCalled(); + }); + + test('should focus on textbox when shouldFocusOnOpen is true', () => { + const mockFocusFunc = jest.fn(state => state); + EditorState.moveFocusToEnd = mockFocusFunc; + + getWrapperRTL({ shouldFocusOnOpen: true }); + expect(mockFocusFunc).toHaveBeenCalled(); + }); }); test('should enable timestamp when file is a video and timestampedComments is enabled', () => { From 1d5022865e0f78dc5e068315bd9de18d1599ec41 Mon Sep 17 00:00:00 2001 From: Jackie Jou Date: Tue, 26 May 2026 17:26:54 -0700 Subject: [PATCH 3/6] fix(activity-feed): mock moveFocusToEnd as passthrough in reply tests The existing tests overwrote EditorState.moveFocusToEnd with a bare jest.fn() that returns undefined. Once CommentForm reads the editor state during render to compute isPostDisabled, the undefined return crashed the component. Make the mock return its input so the editor state stays valid while the spy still tracks calls. --- .../activity-feed/comment/__tests__/BaseComment.test.js | 2 +- .../activity-feed/comment/__tests__/CreateReply.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/elements/content-sidebar/activity-feed/comment/__tests__/BaseComment.test.js b/src/elements/content-sidebar/activity-feed/comment/__tests__/BaseComment.test.js index 27842de4ac..81716684e2 100644 --- a/src/elements/content-sidebar/activity-feed/comment/__tests__/BaseComment.test.js +++ b/src/elements/content-sidebar/activity-feed/comment/__tests__/BaseComment.test.js @@ -405,7 +405,7 @@ describe('elements/content-sidebar/ActivityFeed/comment/BaseComment', () => { }); test('should focus on the edit CommentForm when it is opened', () => { - const mockFocusFunc = jest.fn(); + const mockFocusFunc = jest.fn(editor => editor); EditorState.moveFocusToEnd = mockFocusFunc; getWrapper({ canEdit: true }); diff --git a/src/elements/content-sidebar/activity-feed/comment/__tests__/CreateReply.test.js b/src/elements/content-sidebar/activity-feed/comment/__tests__/CreateReply.test.js index 7e78b5b202..4156bd043c 100644 --- a/src/elements/content-sidebar/activity-feed/comment/__tests__/CreateReply.test.js +++ b/src/elements/content-sidebar/activity-feed/comment/__tests__/CreateReply.test.js @@ -130,7 +130,7 @@ describe('elements/content-sidebar/ActivityFeed/comment/CreateReply', () => { }); test('reply form should be focused when opened', () => { - const mockFocusFunc = jest.fn(); + const mockFocusFunc = jest.fn(editor => editor); EditorState.moveFocusToEnd = mockFocusFunc; getWrapper({ showReplyForm: true }); From 5aaa5c483b796a3dfd41585e6a1392ed1757aa42 Mon Sep 17 00:00:00 2001 From: Jackie Jou Date: Tue, 26 May 2026 18:35:26 -0700 Subject: [PATCH 4/6] fix(activity-feed-v2): scroll only to current-user post-snapshot inserts The scroll-after-post effect picked the first item missing from the pre-post snapshot, which could match a concurrent push from another user that landed in the same render. Filter the candidate to comments or annotations whose author id matches currentUserId so a teammate post does not hijack the viewport. Also adds a whitespace-only post button disabled test in V1 to lock in the trim-based behavior. --- .../activity-feed-v2/ActivityFeedV2.tsx | 15 ++-- .../__tests__/ActivityFeedV2.test.tsx | 80 +++++++++++++------ .../__tests__/CommentForm.test.js | 6 ++ 3 files changed, 73 insertions(+), 28 deletions(-) diff --git a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx index 690f758d55..86546ad5ba 100644 --- a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx +++ b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx @@ -260,17 +260,22 @@ const ActivityFeedV2 = ({ } }, [activeFeedEntryId, filteredItems, scrollHandle]); - // Scroll only to items added since the post snapshot, so a concurrent push from - // another user doesn't hijack the viewport. + // Scroll only to comments/annotations the current user authored after the post + // snapshot, so a concurrent push from another user doesn't hijack the viewport. React.useEffect(() => { const knownIds = knownIdsBeforePostRef.current; - if (!knownIds || !scrollHandle) return; - const newItem = filteredItems.find(item => !knownIds.has(item.id)); + if (!knownIds || !scrollHandle || !currentUserId) return; + const newItem = filteredItems.find(item => { + if (knownIds.has(item.id)) return false; + if (item.type !== 'comment' && item.type !== 'annotation') return false; + const author = item.messages[0]?.author; + return author ? String(author.id) === currentUserId : false; + }); if (!newItem) return; if (scrollHandle.scrollTo(newItem.id)) { knownIdsBeforePostRef.current = null; } - }, [filteredItems, scrollHandle]); + }, [currentUserId, filteredItems, scrollHandle]); const handleCommentPost = React.useCallback( async (content: unknown) => { diff --git a/src/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsx b/src/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsx index 87fe599957..01ec5754e9 100644 --- a/src/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsx +++ b/src/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsx @@ -662,14 +662,25 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { }); describe('scroll to user post', () => { - const newComment = { ...mockComment, id: 'new-comment', tagged_message: 'fresh post' }; - const strangerComment = { ...mockComment, id: 'stranger-comment', tagged_message: 'unrelated' }; + const numericCurrentUser: ActivityFeedV2Props['currentUser'] = { id: '10', name: 'Current User', type: 'user' }; + const userComment = { + ...mockComment, + created_by: { id: '10', name: 'Current User', type: 'user' }, + id: 'new-comment', + tagged_message: 'fresh post', + }; + const strangerComment = { + ...mockComment, + created_by: { id: '99', name: 'Stranger', type: 'user' }, + id: 'stranger-comment', + tagged_message: 'unrelated', + }; - test('should scroll to the new item once feedItems updates after a post', async () => { + test('should scroll to the new user item once feedItems updates after a post', async () => { const onCommentCreate = jest.fn(); const { rerender } = render( , @@ -682,8 +693,8 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { rerender( , ); @@ -696,7 +707,7 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { const onCommentCreate = jest.fn().mockRejectedValue(new Error('network error')); const { rerender } = render( , @@ -706,8 +717,8 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { await lastEditorProps.onPost?.({ type: 'doc', content: [] }); rerender( , ); @@ -720,7 +731,7 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { test('should not scroll to a concurrent push that arrives without a user post', async () => { const { rerender } = render( , ); @@ -728,7 +739,7 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { rerender( , ); @@ -736,11 +747,11 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { expect(mockScrollTo).not.toHaveBeenCalled(); }); - test('should scroll to the user post even when a stranger post lands in the same render', async () => { + test('should scroll past a stranger insert and target the user-authored item only', async () => { const onCommentCreate = jest.fn(); const { rerender } = render( , @@ -750,8 +761,8 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { await lastEditorProps.onPost?.({ type: 'doc', content: [] }); rerender( , ); @@ -760,12 +771,35 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { expect(mockScrollTo).not.toHaveBeenCalledWith('stranger-comment'); }); + test('should not scroll when only a stranger insert lands after a user post', async () => { + const onCommentCreate = jest.fn(); + const { rerender } = render( + , + ); + mockScrollTo.mockClear(); + + await lastEditorProps.onPost?.({ type: 'doc', content: [] }); + rerender( + , + ); + + expect(mockScrollTo).not.toHaveBeenCalled(); + }); + test('should not scroll when serializeEditorContent yields no text', async () => { mockSerializeMentionMarkup.mockReturnValue({ hasMention: false, text: ' ' }); const onCommentCreate = jest.fn(); const { rerender } = render( , @@ -775,8 +809,8 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { await lastEditorProps.onPost?.({ type: 'doc', content: [] }); rerender( , ); @@ -790,7 +824,7 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { const onCommentCreate = jest.fn(); const { rerender } = render( , @@ -801,8 +835,8 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { await lastEditorProps.onPost?.({ type: 'doc', content: [] }); rerender( , ); @@ -812,8 +846,8 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { mockScrollTo.mockReturnValue(true); rerender( , ); diff --git a/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js b/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js index 03fb0915a2..d0b3147be1 100644 --- a/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js +++ b/src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js @@ -191,6 +191,12 @@ describe('elements/content-sidebar/ActivityFeed/comment-form/CommentForm', () => expect(findControls(wrapper).prop('isDisabled')).toBe(true); }); + test('should pass isDisabled=true to the controls when the editor has only whitespace', () => { + const wrapper = getWrapper({ isOpen: true, tagged_message: ' \n\t ' }); + + expect(findControls(wrapper).prop('isDisabled')).toBe(true); + }); + test('should pass isDisabled=false to the controls when the editor has non-whitespace content', () => { const wrapper = getWrapper({ isOpen: true, tagged_message: 'hello' }); From 3c9c76e25d578cc5279d59e92011658bb6a2991e Mon Sep 17 00:00:00 2001 From: Jackie Jou Date: Tue, 26 May 2026 20:52:23 -0700 Subject: [PATCH 5/6] fix(activity-feed-v2): match editor width to list row inset The list rows are inset via padding on each li, but the editor was a sibling of the list with no equivalent inset, so it stretched edge to edge while rows did not. Wrap the editor in a sibling div with the same horizontal padding so the two surfaces line up. --- .../activity-feed-v2/ActivityFeedV2.scss | 5 +++++ .../activity-feed-v2/ActivityFeedV2.tsx | 12 +++++++----- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.scss b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.scss index 23c9b84f2e..3038378867 100644 --- a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.scss +++ b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.scss @@ -20,6 +20,11 @@ padding-left: var(--bp-space-040); } + &-editor { + padding-right: var(--bp-space-040); + padding-left: var(--bp-space-040); + } + // Forms: BUIE sets width, padding, border, box-shadow, border-radius on // contenteditable and inputs via _forms.scss inside .bcs scope. div[contenteditable='true'], diff --git a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx index 86546ad5ba..ca8b14f21b 100644 --- a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx +++ b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx @@ -351,11 +351,13 @@ const ActivityFeedV2 = ({ )} - +
+ +
Date: Tue, 26 May 2026 22:00:49 -0700 Subject: [PATCH 6/6] fix(activity-feed-v2): override vendor editor wrapper padding The previous commit added padding to our wrapper, which compounded with the vendor editor wrapper padding instead of replacing it. Target the vendor wrapper directly via the BUIE wrapper child so the editor outline lines up with the list rows. --- .../content-sidebar/activity-feed-v2/ActivityFeedV2.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.scss b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.scss index 3038378867..c2ceaec619 100644 --- a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.scss +++ b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.scss @@ -20,7 +20,7 @@ padding-left: var(--bp-space-040); } - &-editor { + &-editor > div { padding-right: var(--bp-space-040); padding-left: var(--bp-space-040); }