From d5927b21f266f063e9a7beca3ff32ea5092ef66f Mon Sep 17 00:00:00 2001 From: zhirongwang Date: Thu, 17 Sep 2026 18:00:36 -0700 Subject: [PATCH 1/2] fix(content-preview): keep compare open when clicking compared-pane annotation Clicking an annotation on the compared (older-version) pane rewrites the activity path's fileVersionId and fires onVersionChange; SidebarPanels also emits onVersionChange(null) when leaving the versions route. A comparing ContentPreview forwarded both to the host, which unmounted the compared pane. - Tag annotation-driven version changes with origin: 'annotation' in withSidebarAnnotations and SidebarPanels - ContentPreview.onVersionChange suppresses forwarding to the host when isComparing and origin is 'annotation', so compare stays open while the sidebar switches to the annotation thread - Compared pane inherits host showAnnotations/boxAnnotations so annotations render on the compared version, with create/discoverability off (PREVIEW-1818) PREVIEW-1895 Co-authored-by: Cursor --- src/elements/common/flowTypes.js | 3 + .../content-preview/ContentPreview.js | 20 +++- .../__tests__/ContentPreview.test.js | 59 +++++++++++ src/elements/content-sidebar/SidebarPanels.js | 11 ++- ...idebarPanels.annotationCompare.rtl.test.js | 97 +++++++++++++++++++ .../__tests__/SidebarPanels.test.js | 13 +++ .../content-sidebar/withSidebarAnnotations.js | 2 + 7 files changed, 199 insertions(+), 6 deletions(-) create mode 100644 src/elements/content-sidebar/__tests__/SidebarPanels.annotationCompare.rtl.test.js diff --git a/src/elements/common/flowTypes.js b/src/elements/common/flowTypes.js index b24bb84a8c..84232c4716 100644 --- a/src/elements/common/flowTypes.js +++ b/src/elements/common/flowTypes.js @@ -35,6 +35,9 @@ type Page = { type AdditionalVersionInfo = { currentVersionId?: ?string, + // Annotation-path version changes vs versions-sidebar clicks. Compare mode + // must not forward annotation-driven changes to the host (that unmounts the compared pane). + origin?: 'annotation', updateVersionToCurrent: () => void, }; diff --git a/src/elements/content-preview/ContentPreview.js b/src/elements/content-preview/ContentPreview.js index b2b371fa87..6e37e09083 100644 --- a/src/elements/content-preview/ContentPreview.js +++ b/src/elements/content-preview/ContentPreview.js @@ -1572,8 +1572,13 @@ class ContentPreview extends React.PureComponent { const { onVersionChange }: Props = this.props; this.updateVersionToCurrent = additionalVersionInfo.updateVersionToCurrent; - onVersionChange(version, additionalVersionInfo); - // Host still gets the event so the compared pane can follow comparedVersion. + // Annotation clicks in the compared pane rewrite the activity path's fileVersionId. + // Forwarding that to the host as a version change unmounts side-by-side compare. + // Versions-sidebar clicks do not set origin and still notify the host. + if (!(this.props.isComparing && additionalVersionInfo.origin === 'annotation')) { + onVersionChange(version, additionalVersionInfo); + } + // Host still gets non-annotation events so the compared pane can follow comparedVersion. // The left pane stays on current for the whole comparison session. if (!this.props.isComparing) { this.setState({ @@ -1958,7 +1963,6 @@ function ContentPreviewWithComparison(props: ContentPreviewProps) { accessPattern={undefined} advancedContentInsights={undefined} autoFocus={false} - boxAnnotations={undefined} collection={EMPTY_COLLECTION} componentRef={undefined} comparedSlotRef={undefined} @@ -1986,8 +1990,16 @@ function ContentPreviewWithComparison(props: ContentPreviewProps) { previewVersion={comparedVersion} resin={undefined} renderCustomPreview={undefined} - showAnnotations={false} + // Inherit host showAnnotations + boxAnnotations so this pane + // creates a second annotator (PREVIEW-1818). Create stays off: + // controls only hide the toolbar; discoverability would still + // open the comment composer on text select. + enableAnnotationsDiscoverability={false} + enableAnnotationsImageDiscoverability={false} + enableAnnotationsOnlyControls={false} showAnnotationsControls={false} + showAnnotationsDrawing={false} + showAnnotationsDrawingCreate={false} />, comparedSlot, ) diff --git a/src/elements/content-preview/__tests__/ContentPreview.test.js b/src/elements/content-preview/__tests__/ContentPreview.test.js index 07903b72f8..b3e8e9a025 100644 --- a/src/elements/content-preview/__tests__/ContentPreview.test.js +++ b/src/elements/content-preview/__tests__/ContentPreview.test.js @@ -1796,6 +1796,41 @@ describe('elements/content-preview/ContentPreview', () => { expect(onVersionChange).toHaveBeenCalledWith(version, {}); expect(wrapper.state('selectedVersion')).toBeUndefined(); }); + + test('should not notify the host for annotation-driven version changes when comparing', () => { + const onVersionChange = jest.fn(); + const wrapper = getWrapper({ isComparing: true, onVersionChange }); + const instance = wrapper.instance(); + const version = { id: '12345' }; + + instance.onVersionChange(version, { origin: 'annotation', updateVersionToCurrent: jest.fn() }); + + expect(onVersionChange).not.toHaveBeenCalled(); + expect(wrapper.state('selectedVersion')).toBeUndefined(); + }); + + test('should not notify the host for the annotation-driven version reset when comparing', () => { + // SidebarPanels resets the version (null) when the sidebar leaves the versions route. + // When the exit is caused by opening an annotation thread, the reset is tagged with + // origin so the side-by-side comparison stays open. + const onVersionChange = jest.fn(); + const wrapper = getWrapper({ isComparing: true, onVersionChange }); + const instance = wrapper.instance(); + + instance.onVersionChange(null, { origin: 'annotation' }); + + expect(onVersionChange).not.toHaveBeenCalled(); + }); + + test('should notify the host for the annotation-driven version reset when not comparing', () => { + const onVersionChange = jest.fn(); + const wrapper = getWrapper({ onVersionChange }); + const instance = wrapper.instance(); + + instance.onVersionChange(null, { origin: 'annotation' }); + + expect(onVersionChange).toHaveBeenCalledWith(null, { origin: 'annotation' }); + }); }); describe('handleAnnotationSelect', () => { @@ -2888,6 +2923,30 @@ describe('elements/content-preview/ContentPreview', () => { expect(wrapper.childAt(1).props().children.props.loadingIndicatorDelayMs).toBe(0); }); + test('should inherit host annotations on the compared instance but keep create controls off', () => { + const boxAnnotations = jest.fn(); + const wrapper = shallow( + , + ); + + wrapper.childAt(0).props().comparedSlotRef(document.createElement('div')); + wrapper.update(); + + const comparedProps = wrapper.childAt(1).props().children.props; + expect(comparedProps.showAnnotations).toBe(true); + expect(comparedProps.boxAnnotations).toBe(boxAnnotations); + expect(comparedProps.showAnnotationsControls).toBe(false); + expect(comparedProps.enableAnnotationsDiscoverability).toBe(false); + expect(comparedProps.showAnnotationsDrawingCreate).toBe(false); + }); + test('should not forward the host onMetric to the compared instance', () => { const onMetric = jest.fn(); const wrapper = shallow( diff --git a/src/elements/content-sidebar/SidebarPanels.js b/src/elements/content-sidebar/SidebarPanels.js index 212e763c62..6aa4397b1e 100644 --- a/src/elements/content-sidebar/SidebarPanels.js +++ b/src/elements/content-sidebar/SidebarPanels.js @@ -130,6 +130,7 @@ const LoadableVersionsSidebar = SidebarUtils.getAsyncSidebarContent( ); const SIDEBAR_PATH_VERSIONS = '/:sidebar(activity|details)/versions/:versionId?'; +const SIDEBAR_PATH_ANNOTATIONS = '/:sidebar/annotations/:fileVersionId/:annotationId?'; class SidebarPanels extends React.Component { boxAISidebar: ElementRefType = React.createRef(); @@ -168,9 +169,15 @@ class SidebarPanels extends React.Component { const { location, onVersionChange } = this.props; const { location: prevLocation } = prevProps; - // Reset the current version id if the wrapping versions route is no longer active + // Reset the current version id if the wrapping versions route is no longer active. + // Tag annotation-driven exits (versions -> annotation thread) so hosts comparing + // versions side by side can ignore the reset and keep the comparison open. if (onVersionChange && this.getVersionsMatchPath(prevLocation) && !this.getVersionsMatchPath(location)) { - onVersionChange(null); + if (matchPath(location.pathname, SIDEBAR_PATH_ANNOTATIONS)) { + onVersionChange(null, { origin: 'annotation' }); + } else { + onVersionChange(null); + } } } diff --git a/src/elements/content-sidebar/__tests__/SidebarPanels.annotationCompare.rtl.test.js b/src/elements/content-sidebar/__tests__/SidebarPanels.annotationCompare.rtl.test.js new file mode 100644 index 0000000000..389ddeeaf4 --- /dev/null +++ b/src/elements/content-sidebar/__tests__/SidebarPanels.annotationCompare.rtl.test.js @@ -0,0 +1,97 @@ +// Integration test for the side-by-side version compare flow: clicking an annotation on the +// compared (older-version) pane emits `annotations_active_change` on the shared annotator +// event manager. The main pane's withAnnotations picks it up, withSidebarAnnotations pushes +// the annotation thread path, and every resulting onVersionChange call must carry +// `origin: 'annotation'` so a comparing ContentPreview can suppress them and keep the +// comparison open (see ContentPreview.onVersionChange). +import * as React from 'react'; +import { EventEmitter } from 'events'; +import { Router, withRouter } from 'react-router-dom'; +import { createMemoryHistory } from 'history'; +import { render, act } from '../../../test-utils/testing-library'; +import { SidebarPanelsComponent } from '../SidebarPanels'; +import withSidebarAnnotations from '../withSidebarAnnotations'; +import withAnnotations from '../../common/annotator-context/withAnnotations'; +import withAnnotatorContext from '../../common/annotator-context/withAnnotatorContext'; + +jest.mock('../SidebarUtils'); + +describe('compared-pane annotation click -> sidebar switches to the annotation thread', () => { + const file = { + id: 'f1', + file_version: { id: 'CURRENT' }, + }; + + const oldVersion = { type: 'file_version', id: 'OLD' }; + const feedAPI = { + getCachedItems: jest.fn().mockReturnValue({ items: [oldVersion] }), + }; + const api = { getFeedAPI: () => feedAPI }; + + test('pushes the annotations path and tags all version changes with an annotation origin', () => { + // Mirror the production SidebarPanels composition: withSidebarAnnotations inside, + // withAnnotatorContext outside, router outermost. + const SidebarChain = withRouter(withAnnotatorContext(withSidebarAnnotations(SidebarPanelsComponent))); + + let capturedOnAnnotator = null; + const onVersionChange = jest.fn(); + + // Versions panel is open on the compared (older) version, like when compare is open + const history = createMemoryHistory({ initialEntries: ['/activity/versions/OLD'] }); + + // Stand-in for ContentPreview: captures the onAnnotator injected by withAnnotations + // and renders the sidebar chain, like ContentPreview renders ContentSidebar. + const Inner = props => { + capturedOnAnnotator = props.onAnnotator; + return ( + + + + ); + }; + + const Wrapped = withAnnotations(Inner); + render(); + + expect(capturedOnAnnotator).toEqual(expect.any(Function)); + + // Stand-in for the main pane's annotator; delegates to the process-wide EventManager + // singleton in production, so it receives events emitted by the compared pane's store. + const annotator = new EventEmitter(); + act(() => { + capturedOnAnnotator(annotator); + }); + + // Simulate the compared-pane annotation click: its store emits ACTIVE_CHANGE + // with the compared (older) file version id. + act(() => { + annotator.emit('annotations_active_change', { annotationId: 'ann1', fileVersionId: 'OLD' }); + }); + + // The sidebar must switch from the versions panel to the annotation thread + expect(history.location.pathname).toBe('/activity/annotations/OLD/ann1'); + + // SidebarPanels resets the version on leaving the versions route, and + // withSidebarAnnotations reports the annotation's version; both must be tagged + // with origin 'annotation' so a comparing ContentPreview suppresses them. + expect(onVersionChange).toHaveBeenCalled(); + onVersionChange.mock.calls.forEach(([, additionalVersionInfo]) => { + expect(additionalVersionInfo).toMatchObject({ origin: 'annotation' }); + }); + expect(onVersionChange).toHaveBeenCalledWith(null, { origin: 'annotation' }); + expect(onVersionChange).toHaveBeenCalledWith( + oldVersion, + expect.objectContaining({ currentVersionId: 'CURRENT', origin: 'annotation' }), + ); + }); +}); diff --git a/src/elements/content-sidebar/__tests__/SidebarPanels.test.js b/src/elements/content-sidebar/__tests__/SidebarPanels.test.js index 62d9b236f9..ab38d91552 100644 --- a/src/elements/content-sidebar/__tests__/SidebarPanels.test.js +++ b/src/elements/content-sidebar/__tests__/SidebarPanels.test.js @@ -723,6 +723,19 @@ describe('elements/content-sidebar/SidebarPanels', () => { wrapper.setProps({ location: { pathname } }); expect(onVersionChange).toBeCalledWith(null); }); + + test.each([ + ['/activity/versions/123', '/activity/annotations/123/456'], + ['/activity/versions/123', '/activity/annotations/123'], + ['/details/versions/123', '/activity/annotations/456/789'], + ])( + 'should tag the version reset with an annotation origin when transitioning to an annotations path', + (prevPathname, pathname) => { + const wrapper = getWrapper({ location: { pathname: prevPathname }, onVersionChange }); + wrapper.setProps({ location: { pathname } }); + expect(onVersionChange).toBeCalledWith(null, { origin: 'annotation' }); + }, + ); }); describe('multiple customSidebarPanels rendering', () => { diff --git a/src/elements/content-sidebar/withSidebarAnnotations.js b/src/elements/content-sidebar/withSidebarAnnotations.js index 74ca2db3a4..20ca452969 100644 --- a/src/elements/content-sidebar/withSidebarAnnotations.js +++ b/src/elements/content-sidebar/withSidebarAnnotations.js @@ -448,6 +448,7 @@ export default function withSidebarAnnotations( if (version) { onVersionChange(version, { currentVersionId: currentFileVersionId, + origin: 'annotation', updateVersionToCurrent: () => { const currentVersionNavigation = this.getInternalAnnotationsNavigation(currentFileVersionId); @@ -466,6 +467,7 @@ export default function withSidebarAnnotations( if (version) { onVersionChange(version, { currentVersionId: currentFileVersionId, + origin: 'annotation', updateVersionToCurrent: () => history.push(getAnnotationsPath(currentFileVersionId)), }); } From c8788cdc5a305212e36dd504db5b964740683758 Mon Sep 17 00:00:00 2001 From: zhirongwang Date: Fri, 18 Sep 2026 14:46:45 -0700 Subject: [PATCH 2/2] fix(content-preview): route compared-pane annotation scroll and retry sidebar scroll While comparing, send annotations on another version to that pane instead of switching this one. Retry Activity Feed scroll until the active thread is in the feed so a version switch does not miss it. --- .../content-preview/ContentPreview.js | 66 ++++++++++++---- .../__tests__/ContentPreview.test.js | 78 ++++++++++++++++++- .../activity-feed/ActivityFeed.js | 35 ++++++++- .../__tests__/ActivityFeed.test.js | 28 +++++++ 4 files changed, 188 insertions(+), 19 deletions(-) diff --git a/src/elements/content-preview/ContentPreview.js b/src/elements/content-preview/ContentPreview.js index 6e37e09083..107c3f0f9e 100644 --- a/src/elements/content-preview/ContentPreview.js +++ b/src/elements/content-preview/ContentPreview.js @@ -89,6 +89,11 @@ type StartAt = { value: number, }; +type AnnotationScrollRequest = { + annotation: Annotation, + deferScrollToOnload?: boolean, +}; + type Props = { accessPattern?: 'file_list' | 'direct_link' | 'shared_link', advancedContentInsights: { @@ -117,6 +122,7 @@ type Props = { * future major version. */ metadataApiHost?: string, + annotationScrollRequest?: ?AnnotationScrollRequest, appHost: string, autoFocus: boolean, boxAnnotations?: Object, @@ -153,6 +159,7 @@ type Props = { onAnnotatorEvent: Function, onBeforeNavigate?: (targetFileId: string) => boolean | Promise, onClose?: Function, + onComparedAnnotationSelect: (annotation: Annotation, deferScrollToOnload?: boolean) => void, onContentInsightsEventReport: Function, onDownload: Function, onLoad: Function, @@ -354,6 +361,7 @@ class ContentPreview extends React.PureComponent { loadingIndicatorDelayMs: 0, onAnnotator: noop, onAnnotatorEvent: noop, + onComparedAnnotationSelect: noop, onContentInsightsEventReport: noop, onDownload: noop, onError: noop, @@ -633,6 +641,15 @@ class ContentPreview extends React.PureComponent { this.setState({ selectedVersion: undefined }); } + // The compared pane has no sidebar of its own, so the pane that does forwards + // annotations belonging to this version. A pane mounting for a newly compared version + // already deep links through fileOptions, so this only covers one already showing it. + const { annotationScrollRequest } = this.props; + if (annotationScrollRequest && annotationScrollRequest !== prevProps.annotationScrollRequest) { + const { annotation, deferScrollToOnload } = annotationScrollRequest; + this.handleAnnotationSelect(annotation, deferScrollToOnload); + } + if (haveExperiencesChanged && this.preview && this.preview.updateExperiences) { this.preview.updateExperiences(previewExperiences); } @@ -1569,18 +1586,20 @@ class ContentPreview extends React.PureComponent { * @param {object} [additionalVersionInfo] - extra info about the version */ onVersionChange = (version?: BoxItemVersion, additionalVersionInfo: AdditionalVersionInfo = {}): void => { - const { onVersionChange }: Props = this.props; + const { isComparing, onVersionChange }: Props = this.props; + const { currentVersionId, origin } = additionalVersionInfo; this.updateVersionToCurrent = additionalVersionInfo.updateVersionToCurrent; - // Annotation clicks in the compared pane rewrite the activity path's fileVersionId. - // Forwarding that to the host as a version change unmounts side-by-side compare. - // Versions-sidebar clicks do not set origin and still notify the host. - if (!(this.props.isComparing && additionalVersionInfo.origin === 'annotation')) { + // Annotation clicks rewrite the activity path's fileVersionId. While comparing, the host + // reads a change back to the current version as leaving comparison and unmounts the + // compared pane, so those stay local. Annotations on any other version are forwarded so + // the compared pane follows the thread. Versions-sidebar clicks do not set origin. + const isReturnToCurrentVersion = !version || version.id === currentVersionId; + if (!(isComparing && origin === 'annotation' && isReturnToCurrentVersion)) { onVersionChange(version, additionalVersionInfo); } - // Host still gets non-annotation events so the compared pane can follow comparedVersion. // The left pane stays on current for the whole comparison session. - if (!this.props.isComparing) { + if (!isComparing) { this.setState({ selectedVersion: version, }); @@ -1620,11 +1639,9 @@ class ContentPreview extends React.PureComponent { videoPlayer.addEventListener('loadeddata', handleLoadedData); }; - handleAnnotationSelect = ({ file_version, id, target }: Annotation, deferScrollToOnload: boolean = false) => { - if (this.props.isComparing) { - return; - } - + handleAnnotationSelect = (annotation: Annotation, deferScrollToOnload: boolean = false) => { + const { file_version, id, target } = annotation; + const { isComparing, onComparedAnnotationSelect }: Props = this.props; const { location = {} } = target; const { file } = this.state; const annotationFileVersionId = getProp(file_version, 'id'); @@ -1632,8 +1649,20 @@ class ContentPreview extends React.PureComponent { const currentPreviewFileVersionId = getProp(this.getVersionToPreview(), 'id', currentFileVersionId); const unit = startAtTypes[location.type]; const viewer = this.getViewer(); + const isOtherVersion = !!annotationFileVersionId && annotationFileVersionId !== currentPreviewFileVersionId; - if (unit && annotationFileVersionId && annotationFileVersionId !== currentPreviewFileVersionId) { + // Each pane stays on its own version for the whole comparison, so route by version + // instead of switching: annotations on the compared version belong to the other pane. + if (isComparing) { + if (isOtherVersion) { + onComparedAnnotationSelect(annotation, deferScrollToOnload); + } else { + this.emitScrollToAnnotation(id, target); + } + return; + } + + if (unit && isOtherVersion) { // Frame.value is milliseconds; Preview SDK startAt expects seconds for video. const value = location.type === 'frame' ? convertTimestampToSeconds(location.value) : location.value; this.setState({ @@ -1944,16 +1973,25 @@ const MemoConnectedContentPreview = React.memo(ConnectedContentPreview); function ContentPreviewWithComparison(props: ContentPreviewProps) { const { comparedVersion, ...rest } = props; const [comparedSlot, setComparedSlot] = React.useState(null); + const [annotationScrollRequest, setAnnotationScrollRequest] = React.useState(null); const comparedVersionId = comparedVersion && comparedVersion.id; const isComparing = comparedVersionId != null && comparedVersionId !== ''; + // A new object every time so selecting the same annotation twice still scrolls. + const handleComparedAnnotationSelect = React.useCallback( + (annotation, deferScrollToOnload) => setAnnotationScrollRequest({ annotation, deferScrollToOnload }), + [], + ); + return ( {comparedSlot && isComparing ? createPortal( @@ -1962,6 +2000,7 @@ function ContentPreviewWithComparison(props: ContentPreviewProps) { key={comparedVersionId} accessPattern={undefined} advancedContentInsights={undefined} + annotationScrollRequest={annotationScrollRequest} autoFocus={false} collection={EMPTY_COLLECTION} componentRef={undefined} @@ -1979,6 +2018,7 @@ function ContentPreviewWithComparison(props: ContentPreviewProps) { onAnnotator={noop} onAnnotatorEvent={noop} onBeforeNavigate={undefined} + onComparedAnnotationSelect={noop} onContentInsightsEventReport={noop} onError={noop} onLoad={noop} diff --git a/src/elements/content-preview/__tests__/ContentPreview.test.js b/src/elements/content-preview/__tests__/ContentPreview.test.js index b3e8e9a025..deaa50242b 100644 --- a/src/elements/content-preview/__tests__/ContentPreview.test.js +++ b/src/elements/content-preview/__tests__/ContentPreview.test.js @@ -1797,18 +1797,35 @@ describe('elements/content-preview/ContentPreview', () => { expect(wrapper.state('selectedVersion')).toBeUndefined(); }); - test('should not notify the host for annotation-driven version changes when comparing', () => { + test('should not notify the host when an annotation returns to the current version while comparing', () => { const onVersionChange = jest.fn(); const wrapper = getWrapper({ isComparing: true, onVersionChange }); const instance = wrapper.instance(); const version = { id: '12345' }; - instance.onVersionChange(version, { origin: 'annotation', updateVersionToCurrent: jest.fn() }); + instance.onVersionChange(version, { + currentVersionId: '12345', + origin: 'annotation', + updateVersionToCurrent: jest.fn(), + }); expect(onVersionChange).not.toHaveBeenCalled(); expect(wrapper.state('selectedVersion')).toBeUndefined(); }); + test('should notify the host for an annotation on another version so the compared pane follows', () => { + const onVersionChange = jest.fn(); + const wrapper = getWrapper({ isComparing: true, onVersionChange }); + const instance = wrapper.instance(); + const version = { id: '999' }; + const additionalVersionInfo = { currentVersionId: '12345', origin: 'annotation' }; + + instance.onVersionChange(version, additionalVersionInfo); + + expect(onVersionChange).toHaveBeenCalledWith(version, additionalVersionInfo); + expect(wrapper.state('selectedVersion')).toBeUndefined(); + }); + test('should not notify the host for the annotation-driven version reset when comparing', () => { // SidebarPanels resets the version (null) when the sidebar leaves the versions route. // When the exit is caused by opening an annotation thread, the reset is tagged with @@ -1833,6 +1850,63 @@ describe('elements/content-preview/ContentPreview', () => { }); }); + describe('handleAnnotationSelect while comparing', () => { + const getAnnotation = fileVersionId => ({ + id: 'anno-1', + file_version: { id: fileVersionId }, + target: { location: { type: 'page', value: 3 } }, + }); + + const getComparingWrapper = (overrideProps = {}) => { + const wrapper = getWrapper({ isComparing: true, ...overrideProps }); + const instance = wrapper.instance(); + instance.setState({ file: { id: '123', file_version: { id: 'CURRENT' } } }); + instance.emitScrollToAnnotation = jest.fn(); + return { instance, wrapper }; + }; + + test('should scroll in this pane for an annotation on the version it previews', () => { + const onComparedAnnotationSelect = jest.fn(); + const { instance } = getComparingWrapper({ onComparedAnnotationSelect }); + const annotation = getAnnotation('CURRENT'); + + instance.handleAnnotationSelect(annotation); + + expect(instance.emitScrollToAnnotation).toHaveBeenCalledWith('anno-1', annotation.target); + expect(onComparedAnnotationSelect).not.toHaveBeenCalled(); + }); + + test('should hand an annotation on another version to the compared pane', () => { + const onComparedAnnotationSelect = jest.fn(); + const { instance } = getComparingWrapper({ onComparedAnnotationSelect }); + const annotation = getAnnotation('OLD'); + + instance.handleAnnotationSelect(annotation, true); + + expect(onComparedAnnotationSelect).toHaveBeenCalledWith(annotation, true); + expect(instance.emitScrollToAnnotation).not.toHaveBeenCalled(); + }); + + test('should not change the version this pane previews', () => { + const { instance, wrapper } = getComparingWrapper(); + + instance.handleAnnotationSelect(getAnnotation('OLD')); + + expect(wrapper.state('startAt')).toBeUndefined(); + }); + + test('should scroll to a forwarded annotation when the request prop changes', () => { + const wrapper = getWrapper(); + const instance = wrapper.instance(); + instance.handleAnnotationSelect = jest.fn(); + const annotation = getAnnotation('OLD'); + + wrapper.setProps({ annotationScrollRequest: { annotation, deferScrollToOnload: false } }); + + expect(instance.handleAnnotationSelect).toHaveBeenCalledWith(annotation, false); + }); + }); + describe('handleAnnotationSelect', () => { test.each` annotationFileVersionId | selectedVersionId | locationType | setStateCount diff --git a/src/elements/content-sidebar/activity-feed/activity-feed/ActivityFeed.js b/src/elements/content-sidebar/activity-feed/activity-feed/ActivityFeed.js index 3051da14d6..42b2d4cb94 100644 --- a/src/elements/content-sidebar/activity-feed/activity-feed/ActivityFeed.js +++ b/src/elements/content-sidebar/activity-feed/activity-feed/ActivityFeed.js @@ -116,6 +116,8 @@ class ActivityFeed extends React.Component { feedContainer: null | HTMLElement; + hasPendingActiveScroll: boolean = false; + componentDidMount() { this.resetFeedScroll(); } @@ -140,27 +142,52 @@ class ActivityFeed extends React.Component { this.resetFeedScroll(); } - if (didLoadFeedItems || hasActiveFeedEntryIdChanged) { + // Switching file versions replaces the items, so the active entry can be missing from + // the feed at the moment its id changes. Re-arm on any feed change to catch it later. + if (didLoadFeedItems || hasActiveFeedEntryIdChanged || prevFeedItems !== currFeedItems) { + this.hasPendingActiveScroll = true; + } + + if (this.hasPendingActiveScroll) { this.scrollToActiveFeedItemOrErrorMessage(); } } + hasActiveFeedItem(): boolean { + const { activeFeedEntryId, feedItems = [] } = this.props; + + return feedItems.some(item => { + const { id, replies } = (item: Object); + return id === activeFeedEntryId || (!!replies && replies.some(reply => reply.id === activeFeedEntryId)); + }); + } + scrollToActiveFeedItemOrErrorMessage() { const { current: activeFeedItemRef } = this.activeFeedItemRef; - const { activeFeedEntryId } = this.props; + const { activeFeedEntryId, feedItems } = this.props; // if there is no active item, do not scroll if (!activeFeedEntryId) { + this.hasPendingActiveScroll = false; return; } - // if there was supposed to be an active feed item but the feed item does not exist - // scroll to the bottom to show the inline error message if (activeFeedItemRef === null) { + // The active item can arrive after the id does, e.g. while the items for another + // file version are still being fetched. Stay pending so a later update scrolls to + // it, rather than treating it as missing on the first try. + if (feedItems === undefined || this.hasActiveFeedItem()) { + return; + } + + // if there was supposed to be an active feed item but the feed item does not exist + // scroll to the bottom to show the inline error message + this.hasPendingActiveScroll = false; this.resetFeedScroll(); return; } + this.hasPendingActiveScroll = false; scrollIntoView(activeFeedItemRef); } diff --git a/src/elements/content-sidebar/activity-feed/activity-feed/__tests__/ActivityFeed.test.js b/src/elements/content-sidebar/activity-feed/activity-feed/__tests__/ActivityFeed.test.js index 40dcdfe8ad..58eeff1e0b 100644 --- a/src/elements/content-sidebar/activity-feed/activity-feed/__tests__/ActivityFeed.test.js +++ b/src/elements/content-sidebar/activity-feed/activity-feed/__tests__/ActivityFeed.test.js @@ -409,6 +409,34 @@ describe('elements/content-sidebar/ActivityFeed/activity-feed/ActivityFeed', () expect(scrollIntoView).not.toHaveBeenCalled(); }); + test('should scroll to the active feed item once it arrives in a later update', () => { + const activeFeedEntryId = comments.entries[0].id; + const wrapper = getWrapper({ activeFeedEntryId, feedItems: [] }); + const instance = wrapper.instance(); + const li = document.createElement('li'); + + // The id changes before the item exists in the feed, so there is nothing to scroll to yet + wrapper.setProps({ activeFeedEntryId: 'another-id' }); + expect(scrollIntoView).not.toHaveBeenCalled(); + + instance.activeFeedItemRef.current = li; + wrapper.setProps({ feedItems: [{ id: 'another-id', type: FEED_ITEM_TYPE_COMMENT }] }); + + expect(scrollIntoView).toHaveBeenCalledWith(li); + }); + + test('should scroll to the bottom when the active feed item is absent from a loaded feed', () => { + const wrapper = getWrapper({ activeFeedEntryId: 'missing-id', feedItems: [] }); + const instance = wrapper.instance(); + instance.feedContainer = { scrollTop: 0, scrollHeight: 100 }; + instance.activeFeedItemRef.current = null; + + wrapper.setProps({ feedItems: [{ id: 'some-other-id', type: FEED_ITEM_TYPE_COMMENT }] }); + + expect(scrollIntoView).not.toHaveBeenCalled(); + expect(instance.feedContainer.scrollTop).toEqual(100); + }); + test('should show input when commentFormFocusHandler is called', () => { const wrapper = getWrapper();