From d195f091450807e28a67dae075c54aef0c5214a1 Mon Sep 17 00:00:00 2001 From: Duncan Hsu Date: Thu, 17 Sep 2026 17:51:42 -0700 Subject: [PATCH] feat(activity-feed-v2): uncheck timestamp toggle when viewer dismisses range draft When the user clicks the waveform outside an open comment range, the viewer emits comment_range_draft_dismiss. Treat that as toggle-off so the checkbox, pinned range, and draft handles stay in sync. Co-authored-by: Cursor --- .../__tests__/ActivityFeedV2.test.tsx | 36 ++++++++++++++ .../__tests__/useMediaTimestamp.test.tsx | 48 +++++++++++++++++++ .../activity-feed-v2/useMediaTimestamp.ts | 31 +++++++++--- 3 files changed, 108 insertions(+), 7 deletions(-) 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 ba65401d1c..7a5f2ccf3c 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 @@ -1175,6 +1175,7 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { }; return { commitDrag: (payload: unknown) => listeners.comment_range_draft_change?.(payload), + dismissDraft: () => listeners.comment_range_draft_dismiss?.(undefined), getViewer: () => viewer, rangeEmits: () => viewer.emit.mock.calls.filter(([event]) => String(event).startsWith('comment_range_draft')), @@ -1310,6 +1311,41 @@ describe('elements/content-sidebar/activity-feed-v2/ActivityFeedV2', () => { cleanup(); } }); + + test('should uncheck the timestamp toggle when the viewer dismisses the draft', async () => { + const { cleanup, media } = mountAudio(); + const { commitDrag, dismissDraft, getViewer, rangeEmits } = createRangeViewer(); + try { + render( + , + ); + Object.defineProperty(media, 'currentTime', { configurable: true, value: 8.055, writable: true }); + await act(async () => { + lastEditorProps.videoTimestamp?.onPressedChange(true); + }); + await act(async () => { + commitDrag({ endMs: 12000, startMs: 8055 }); + }); + expect(lastEditorProps.videoTimestamp?.isPressed).toBe(true); + + await act(async () => { + dismissDraft(); + }); + + expect(lastEditorProps.videoTimestamp?.isPressed).toBe(false); + expect(lastEditorProps.videoTimestamp?.formattedTimestamp).toBe('0:08'); + expect(rangeEmits().pop()).toEqual(['comment_range_draft_clear', undefined]); + } finally { + cleanup(); + } + }); }); describe('filter controls', () => { diff --git a/src/elements/content-sidebar/activity-feed-v2/__tests__/useMediaTimestamp.test.tsx b/src/elements/content-sidebar/activity-feed-v2/__tests__/useMediaTimestamp.test.tsx index b2917292aa..c413139d37 100644 --- a/src/elements/content-sidebar/activity-feed-v2/__tests__/useMediaTimestamp.test.tsx +++ b/src/elements/content-sidebar/activity-feed-v2/__tests__/useMediaTimestamp.test.tsx @@ -642,6 +642,52 @@ describe('useMediaTimestamp range selection', () => { } }); + test('should uncheck the toggle and drop the range when the viewer dismisses the draft', () => { + const { cleanup, emitFromViewer, viewer } = renderWithRange(); + try { + act(() => screen.getByText('press').click()); + act(() => emitFromViewer('comment_range_draft_change', { endMs: 50000, startMs: 44000 })); + act(() => emitFromViewer('comment_range_draft_dismiss', undefined)); + + expect(screen.getByTestId('pressed').textContent).toBe('false'); + expect(screen.getByTestId('end-ms').textContent).toBe('undefined'); + expect(emittedEvents(viewer).pop()).toEqual(['comment_range_draft_clear', undefined]); + } finally { + cleanup(); + } + }); + + test('should ignore a dismiss when the toggle is already off', () => { + const { cleanup, emitFromViewer, viewer } = renderWithRange(); + try { + act(() => emitFromViewer('comment_range_draft_dismiss', undefined)); + + expect(screen.getByTestId('pressed').textContent).toBe('false'); + expect(emittedEvents(viewer)).toHaveLength(0); + } finally { + cleanup(); + } + }); + + test('should let the start follow pause and seek again after a dismiss', () => { + const { audio, cleanup, emitFromViewer } = renderWithRange(); + try { + act(() => screen.getByText('press').click()); + act(() => emitFromViewer('comment_range_draft_change', { endMs: 50000, startMs: 44000 })); + act(() => emitFromViewer('comment_range_draft_dismiss', undefined)); + act(() => screen.getByText('press').click()); + + Object.defineProperty(audio, 'currentTime', { configurable: true, value: 61, writable: true }); + act(() => audio.dispatchEvent(new Event('pause'))); + + expect(screen.getByTestId('pressed').textContent).toBe('true'); + expect(screen.getByTestId('ms').textContent).toBe('61000'); + expect(screen.getByTestId('end-ms').textContent).toBe('undefined'); + } finally { + cleanup(); + } + }); + test.each([ ['a malformed payload', undefined], ['a non-numeric start', { endMs: 50000, startMs: 'nope' }], @@ -773,6 +819,7 @@ describe('useMediaTimestamp range selection', () => { try { render( (isPreviewLoaded ? harness.viewer : null)} isAudioPlayerV2 />); expect(harness.hasListener('comment_range_draft_change')).toBe(false); + expect(harness.hasListener('comment_range_draft_dismiss')).toBe(false); isPreviewLoaded = true; act(() => jest.advanceTimersByTime(500)); @@ -816,6 +863,7 @@ describe('useMediaTimestamp range selection', () => { expect(emittedEvents(viewer)).toHaveLength(0); expect(hasListener('comment_range_draft_change')).toBe(false); + expect(hasListener('comment_range_draft_dismiss')).toBe(false); expect(screen.getByTestId('ms').textContent).toBe('43500'); } finally { cleanup(); diff --git a/src/elements/content-sidebar/activity-feed-v2/useMediaTimestamp.ts b/src/elements/content-sidebar/activity-feed-v2/useMediaTimestamp.ts index 0c5e8149fe..3a55b928e5 100644 --- a/src/elements/content-sidebar/activity-feed-v2/useMediaTimestamp.ts +++ b/src/elements/content-sidebar/activity-feed-v2/useMediaTimestamp.ts @@ -9,6 +9,7 @@ const VIEWER_POLL_MS = 500; export const EVENT_RANGE_DRAFT = 'comment_range_draft'; export const EVENT_RANGE_DRAFT_CHANGE = 'comment_range_draft_change'; export const EVENT_RANGE_DRAFT_CLEAR = 'comment_range_draft_clear'; +export const EVENT_RANGE_DRAFT_DISMISS = 'comment_range_draft_dismiss'; const findMediaElement = (): HTMLMediaElement | null => { if (typeof document === 'undefined') { @@ -72,6 +73,7 @@ const readRangeChange = (payload: unknown): { endMs?: number; startMs: number } * - Pressed on while media is playing: captured value frozen until pause/seek. * - Pressed on while media is paused: captured value updates on pause/seek. * - Toggle off->on: captures current time and pauses the media if it was playing. + * - Viewer dismisses the draft: same as the user toggling off. * - New media src: captured value resets to 0; pressed state persists. A dragged range survives * untouched, since a src change on the same element is a token refresh, not different content. * - New media element: any selected range is dropped. @@ -131,17 +133,21 @@ export const useMediaTimestamp = ( } }, [enabled]); + const uncheckTimestamp = React.useCallback(() => { + isPressedRef.current = false; + setIsPressed(false); + isRangePinnedRef.current = false; + setTimestampEndMs(undefined); + emitClear(); + }, [emitClear]); + const onPressedChange = React.useCallback( (pressed: boolean) => { if (!enabled) { return; } if (!pressed) { - isPressedRef.current = false; - setIsPressed(false); - isRangePinnedRef.current = false; - setTimestampEndMs(undefined); - emitClear(); + uncheckTimestamp(); return; } const media = findMediaElement(); @@ -159,7 +165,7 @@ export const useMediaTimestamp = ( setTimestampEndMs(undefined); emitDraft(capturedMs); }, - [emitClear, emitDraft, enabled], + [emitDraft, enabled, uncheckTimestamp], ); React.useEffect(() => { @@ -259,6 +265,15 @@ export const useMediaTimestamp = ( setTimestampEndMs(change.endMs); }; + // Click-outside on the waveform. The viewer has already taken its handles down, so the + // clear this echoes back is a no-op there, and there is nothing to echo with no draft up. + const handleRangeDismiss = () => { + if (!isPressedRef.current) { + return; + } + uncheckTimestamp(); + }; + // Poll for the viewer until we find one. let attachedViewer: ViewerHandle | null = null; let pollId: ReturnType | undefined; @@ -269,6 +284,7 @@ export const useMediaTimestamp = ( return; } viewer.addListener(EVENT_RANGE_DRAFT_CHANGE, handleRangeChange); + viewer.addListener(EVENT_RANGE_DRAFT_DISMISS, handleRangeDismiss); attachedViewer = viewer; clearInterval(pollId); }; @@ -281,8 +297,9 @@ export const useMediaTimestamp = ( return () => { clearInterval(pollId); attachedViewer?.removeListener(EVENT_RANGE_DRAFT_CHANGE, handleRangeChange); + attachedViewer?.removeListener(EVENT_RANGE_DRAFT_DISMISS, handleRangeDismiss); }; - }, [getViewer, isRangeEnabled]); + }, [getViewer, isRangeEnabled, uncheckTimestamp]); // Take down any handles still up for a composer that is going away. React.useEffect(