From 7676cf66f0ab4f7969ca4bc34931e057e3435975 Mon Sep 17 00:00:00 2001 From: Duncan Hsu Date: Wed, 17 Jun 2026 17:41:43 -0700 Subject: [PATCH 1/3] feat(activity-feed-v2): sync sidebar timestamps with video time format Sidebar comment timestamps (both the editor checkbox and posted comment badges) now respect the time format selected in the video player controls (standard, timecode, or frame numbers). Communication uses data attributes on .bp-media-container observed via MutationObserver. --- .../activity-feed-v2/ActivityFeedV2.tsx | 1 + .../activity-feed-v2/FeedItemRow.tsx | 20 ++- .../__tests__/FeedItemRow.test.tsx | 103 ++++++++++++ .../__tests__/useTimeFormat.test.tsx | 146 ++++++++++++++++++ .../__tests__/useVideoTimestamp.test.tsx | 81 ++++++++++ .../activity-feed-v2/useTimeFormat.ts | 98 ++++++++++++ .../activity-feed-v2/useVideoTimestamp.ts | 5 +- src/utils/__tests__/timestamp.test.js | 66 ++++++++ src/utils/timestamp.ts | 34 +++- 9 files changed, 549 insertions(+), 5 deletions(-) create mode 100644 src/elements/content-sidebar/activity-feed-v2/__tests__/useTimeFormat.test.tsx create mode 100644 src/elements/content-sidebar/activity-feed-v2/useTimeFormat.ts diff --git a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx index 7ed9e1b5ac..1b222e041f 100644 --- a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx +++ b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx @@ -358,6 +358,7 @@ const ActivityFeedV2 = ({ key={item.id} currentUserId={currentUserId} isDisabled={isDisabled} + isVideo={isVideo} item={item} onAnnotationCopyLink={onAnnotationCopyLink} onAnnotationDelete={onAnnotationDelete} diff --git a/src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx b/src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx index 8be0c04ade..0a8b48b6fe 100644 --- a/src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx +++ b/src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx @@ -14,6 +14,7 @@ import type { TaskCollabStatus, TaskNew } from '../../../common/types/tasks'; import { dispatchReplyDelete, dispatchReplyEdit, logEditError, serializeEditorContent } from './helpers'; import { annotationTargetToBadge } from './transformers'; +import { formatByTimeFormat, useTimeFormat } from './useTimeFormat'; import { seekVideoToMs } from './useVideoTimestamp'; import type { OnReplyDelete, OnReplyUpdate, TransformedFeedItem, UserSelectorProps } from './types'; @@ -30,6 +31,7 @@ import { type FeedItemRowProps = { currentUserId?: string; isDisabled: boolean; + isVideo?: boolean; item: TransformedFeedItem; onAnnotationCopyLink?: (params: { annotationId: string; fileVersionId: string }) => void; onAnnotationDelete?: (params: { id: string; permissions: AnnotationPermission }) => void; @@ -80,6 +82,7 @@ const FeedItemRow = ({ currentUserId, isDisabled, item, + isVideo = false, onAnnotationCopyLink, onAnnotationDelete, onAnnotationEdit, @@ -98,6 +101,7 @@ const FeedItemRow = ({ onVersionHistoryClick, userSelectorProps, }: FeedItemRowProps) => { + const { timeFormat, fps } = useTimeFormat(isVideo); switch (item.type) { case 'comment': { const { permissions } = item; @@ -134,10 +138,14 @@ const FeedItemRow = ({ }; const timestampMs = item.annotationTimestampMs; const handleBadgeClick = timestampMs !== undefined ? () => seekVideoToMs(timestampMs) : undefined; + const commentAnnotationTarget = + item.annotationTarget && timestampMs !== undefined + ? { ...item.annotationTarget, timestamp: formatByTimeFormat(timestampMs, timeFormat, fps) } + : item.annotationTarget; return ( ({ seekVideoToMs: jest.fn(), })); +const mockTimeFormat: { timeFormat: string; fps: number } = { timeFormat: 'standard', fps: 24 }; +jest.mock('../useTimeFormat', () => ({ + ...jest.requireActual('../useTimeFormat'), + useTimeFormat: () => mockTimeFormat, +})); + const mockedSerializeEditorContent = jest.mocked(serializeEditorContent); const mockedDispatchReplyDelete = jest.mocked(dispatchReplyDelete); const mockedDispatchReplyEdit = jest.mocked(dispatchReplyEdit); @@ -952,4 +958,101 @@ describe('elements/content-sidebar/activity-feed-v2/FeedItemRow', () => { ).not.toThrow(); }); }); + + describe('time format-aware badge rendering', () => { + test('should format comment badge timestamp using the current time format', () => { + mockTimeFormat.timeFormat = 'timecode'; + mockTimeFormat.fps = 24; + + const timestampedComment: TransformedCommentItem = { + ...mockComment, + annotationTarget: { timestamp: '0:08', type: AnnotationBadgeType.Frame }, + annotationTimestampMs: 8055, + }; + render(); + + expect(lastThreadedAnnotationProps.annotationTarget).toEqual({ + timestamp: '00:00:08:01', + type: AnnotationBadgeType.Frame, + }); + }); + + test('should format comment badge timestamp as frame number', () => { + mockTimeFormat.timeFormat = 'frames'; + mockTimeFormat.fps = 24; + + const timestampedComment: TransformedCommentItem = { + ...mockComment, + annotationTarget: { timestamp: '0:10', type: AnnotationBadgeType.Frame }, + annotationTimestampMs: 10000, + }; + render(); + + expect(lastThreadedAnnotationProps.annotationTarget).toEqual({ + timestamp: '240', + type: AnnotationBadgeType.Frame, + }); + }); + + test('should not modify comment badge when annotationTimestampMs is undefined', () => { + mockTimeFormat.timeFormat = 'timecode'; + mockTimeFormat.fps = 24; + + render(); + + expect(lastThreadedAnnotationProps.annotationTarget).toBeUndefined(); + }); + + test('should format annotation badge timestamp for frame-type annotations', () => { + mockTimeFormat.timeFormat = 'frames'; + mockTimeFormat.fps = 30; + + const frameAnnotation: TransformedAnnotationItem = { + ...mockAnnotation, + annotation: { + ...mockAnnotation.annotation, + target: { location: { type: 'frame', value: 5000 }, type: 'region', x: 0, y: 0 }, + } as TransformedAnnotationItem['annotation'], + }; + + const badge: AnnotationBadgeTargetType = { timestamp: '0:05', type: AnnotationBadgeType.Frame }; + mockedAnnotationTargetToBadge.mockReturnValue(badge); + + render(); + + expect(lastThreadedAnnotationProps.annotationTarget).toEqual({ + timestamp: '150', + type: AnnotationBadgeType.Frame, + }); + }); + + test('should not modify annotation badge for non-frame targets', () => { + mockTimeFormat.timeFormat = 'timecode'; + mockTimeFormat.fps = 24; + + const badge: AnnotationBadgeTargetType = { page: 3, type: AnnotationBadgeType.Point }; + mockedAnnotationTargetToBadge.mockReturnValue(badge); + + render(); + + expect(lastThreadedAnnotationProps.annotationTarget).toBe(badge); + }); + + test('should use standard format when isVideo is false', () => { + mockTimeFormat.timeFormat = 'standard'; + mockTimeFormat.fps = 24; + + const timestampedComment: TransformedCommentItem = { + ...mockComment, + annotationTarget: { timestamp: '0:08', type: AnnotationBadgeType.Frame }, + annotationTimestampMs: 8055, + }; + render(); + + expect(lastThreadedAnnotationProps.annotationTarget).toEqual({ + timestamp: '0:08', + type: AnnotationBadgeType.Frame, + }); + }); + }); }); diff --git a/src/elements/content-sidebar/activity-feed-v2/__tests__/useTimeFormat.test.tsx b/src/elements/content-sidebar/activity-feed-v2/__tests__/useTimeFormat.test.tsx new file mode 100644 index 0000000000..7cbddbe3a6 --- /dev/null +++ b/src/elements/content-sidebar/activity-feed-v2/__tests__/useTimeFormat.test.tsx @@ -0,0 +1,146 @@ +import * as React from 'react'; +import { act, render, screen } from '@testing-library/react'; + +import { formatByTimeFormat, useTimeFormat } from '../useTimeFormat'; + +const TestHarness = ({ enabled }: { enabled: boolean }) => { + const { timeFormat, fps } = useTimeFormat(enabled); + return ( +
+ {timeFormat} + {String(fps)} +
+ ); +}; + +describe('useTimeFormat', () => { + afterEach(() => { + document.querySelectorAll('.bp-media-container').forEach(node => node.remove()); + }); + + test('should return standard format and default fps when disabled', () => { + render(); + expect(screen.getByTestId('format').textContent).toBe('standard'); + expect(screen.getByTestId('fps').textContent).toBe('24'); + }); + + test('should return standard format when no media container exists', () => { + render(); + expect(screen.getByTestId('format').textContent).toBe('standard'); + expect(screen.getByTestId('fps').textContent).toBe('24'); + }); + + test('should read initial data attributes from media container', () => { + const container = document.createElement('div'); + container.className = 'bp-media-container'; + container.setAttribute('data-time-format', 'timecode'); + container.setAttribute('data-fps', '30'); + document.body.appendChild(container); + + render(); + expect(screen.getByTestId('format').textContent).toBe('timecode'); + expect(screen.getByTestId('fps').textContent).toBe('30'); + }); + + test('should update when data-time-format attribute changes', async () => { + const container = document.createElement('div'); + container.className = 'bp-media-container'; + container.setAttribute('data-time-format', 'standard'); + container.setAttribute('data-fps', '24'); + document.body.appendChild(container); + + render(); + expect(screen.getByTestId('format').textContent).toBe('standard'); + + await act(async () => { + container.setAttribute('data-time-format', 'frames'); + }); + expect(screen.getByTestId('format').textContent).toBe('frames'); + }); + + test('should update when data-fps attribute changes', async () => { + const container = document.createElement('div'); + container.className = 'bp-media-container'; + container.setAttribute('data-time-format', 'timecode'); + container.setAttribute('data-fps', '24'); + document.body.appendChild(container); + + render(); + expect(screen.getByTestId('fps').textContent).toBe('24'); + + await act(async () => { + container.setAttribute('data-fps', '60'); + }); + expect(screen.getByTestId('fps').textContent).toBe('60'); + }); + + test('should observe a late-appearing media container', async () => { + render(); + expect(screen.getByTestId('format').textContent).toBe('standard'); + + await act(async () => { + const container = document.createElement('div'); + container.className = 'bp-media-container'; + container.setAttribute('data-time-format', 'frames'); + container.setAttribute('data-fps', '30'); + document.body.appendChild(container); + }); + + expect(screen.getByTestId('format').textContent).toBe('frames'); + expect(screen.getByTestId('fps').textContent).toBe('30'); + }); + + test('should fall back to default fps when attribute is invalid', () => { + const container = document.createElement('div'); + container.className = 'bp-media-container'; + container.setAttribute('data-time-format', 'timecode'); + container.setAttribute('data-fps', 'bad'); + document.body.appendChild(container); + + render(); + expect(screen.getByTestId('fps').textContent).toBe('24'); + }); + + test('should fall back to standard when data-time-format is absent', () => { + const container = document.createElement('div'); + container.className = 'bp-media-container'; + document.body.appendChild(container); + + render(); + expect(screen.getByTestId('format').textContent).toBe('standard'); + }); + + test('should reset to defaults when transitioning from enabled to disabled', async () => { + const container = document.createElement('div'); + container.className = 'bp-media-container'; + container.setAttribute('data-time-format', 'timecode'); + container.setAttribute('data-fps', '30'); + document.body.appendChild(container); + + const { rerender } = render(); + expect(screen.getByTestId('format').textContent).toBe('timecode'); + expect(screen.getByTestId('fps').textContent).toBe('30'); + + rerender(); + expect(screen.getByTestId('format').textContent).toBe('standard'); + expect(screen.getByTestId('fps').textContent).toBe('24'); + }); +}); + +describe('formatByTimeFormat', () => { + test('should format as standard time', () => { + expect(formatByTimeFormat(43500, 'standard', 24)).toBe('0:43'); + expect(formatByTimeFormat(3661000, 'standard', 24)).toBe('1:01:01'); + }); + + test('should format as timecode', () => { + expect(formatByTimeFormat(61500, 'timecode', 30)).toBe('00:01:01:15'); + expect(formatByTimeFormat(0, 'timecode', 24)).toBe('00:00:00:00'); + }); + + test('should format as frame number string', () => { + expect(formatByTimeFormat(10000, 'frames', 24)).toBe('240'); + expect(formatByTimeFormat(1000, 'frames', 30)).toBe('30'); + expect(formatByTimeFormat(0, 'frames', 24)).toBe('0'); + }); +}); diff --git a/src/elements/content-sidebar/activity-feed-v2/__tests__/useVideoTimestamp.test.tsx b/src/elements/content-sidebar/activity-feed-v2/__tests__/useVideoTimestamp.test.tsx index 87a4a58245..1f0a31926a 100644 --- a/src/elements/content-sidebar/activity-feed-v2/__tests__/useVideoTimestamp.test.tsx +++ b/src/elements/content-sidebar/activity-feed-v2/__tests__/useVideoTimestamp.test.tsx @@ -291,6 +291,87 @@ describe('useVideoTimestamp', () => { }); }); +describe('useVideoTimestamp time format integration', () => { + afterEach(() => { + document.querySelectorAll('.bp-media-container').forEach(node => node.remove()); + }); + + test('should format timestamp as timecode when data-time-format is timecode', async () => { + const video = createVideoElement(8.055); + const cleanup = mountVideoInDom(video); + try { + const container = document.querySelector('.bp-media-container')!; + container.setAttribute('data-time-format', 'timecode'); + container.setAttribute('data-fps', '24'); + + render(); + act(() => { + screen.getByText('press').click(); + }); + + expect(screen.getByTestId('timestamp').textContent).toBe('00:00:08:01'); + } finally { + cleanup(); + } + }); + + test('should format timestamp as frame number when data-time-format is frames', async () => { + const video = createVideoElement(10); + const cleanup = mountVideoInDom(video); + try { + const container = document.querySelector('.bp-media-container')!; + container.setAttribute('data-time-format', 'frames'); + container.setAttribute('data-fps', '24'); + + render(); + act(() => { + screen.getByText('press').click(); + }); + + expect(screen.getByTestId('timestamp').textContent).toBe('240'); + } finally { + cleanup(); + } + }); + + test('should update formatted timestamp when time format changes after capture', async () => { + const video = createVideoElement(10); + const cleanup = mountVideoInDom(video); + try { + const container = document.querySelector('.bp-media-container')!; + container.setAttribute('data-time-format', 'standard'); + container.setAttribute('data-fps', '24'); + + render(); + act(() => { + screen.getByText('press').click(); + }); + expect(screen.getByTestId('timestamp').textContent).toBe('0:10'); + + await act(async () => { + container.setAttribute('data-time-format', 'frames'); + }); + expect(screen.getByTestId('timestamp').textContent).toBe('240'); + } finally { + cleanup(); + } + }); + + test('should default to standard format when no data attribute is set', () => { + const video = createVideoElement(43.5); + const cleanup = mountVideoInDom(video); + try { + render(); + act(() => { + screen.getByText('press').click(); + }); + expect(screen.getByTestId('timestamp').textContent).toBe('0:43'); + } finally { + cleanup(); + } + }); +}); + describe('seekVideoToMs', () => { afterEach(() => { document.querySelectorAll('.bp-media-container').forEach(node => node.remove()); diff --git a/src/elements/content-sidebar/activity-feed-v2/useTimeFormat.ts b/src/elements/content-sidebar/activity-feed-v2/useTimeFormat.ts new file mode 100644 index 0000000000..b5706c3c0f --- /dev/null +++ b/src/elements/content-sidebar/activity-feed-v2/useTimeFormat.ts @@ -0,0 +1,98 @@ +import * as React from 'react'; + +import { + convertMillisecondsToFrames, + convertMillisecondsToTimecode, + convertMillisecondsToTimestamp, +} from '../../../utils/timestamp'; + +const VIDEO_CONTAINER_SELECTOR = '.bp-media-container'; +const DEFAULT_FPS = 24; + +export type TimeFormat = 'standard' | 'timecode' | 'frames'; + +export function formatByTimeFormat(ms: number, format: TimeFormat, fps: number): string { + switch (format) { + case 'timecode': + return convertMillisecondsToTimecode(ms, fps); + case 'frames': + return String(convertMillisecondsToFrames(ms, fps)); + case 'standard': + default: + return convertMillisecondsToTimestamp(ms); + } +} + +export interface UseTimeFormatResult { + timeFormat: TimeFormat; + fps: number; +} + +export const useTimeFormat = (enabled: boolean): UseTimeFormatResult => { + const [timeFormat, setTimeFormat] = React.useState('standard'); + const [fps, setFps] = React.useState(DEFAULT_FPS); + + React.useEffect(() => { + if (!enabled) { + setTimeFormat('standard'); + setFps(DEFAULT_FPS); + return undefined; + } + + if (typeof document === 'undefined') { + return undefined; + } + + let attrObserver: MutationObserver | null = null; + let bodyObserver: MutationObserver | null = null; + let observedContainer: Element | null = null; + + const readAttributes = (container: Element): void => { + const format = (container.getAttribute('data-time-format') as TimeFormat) || 'standard'; + const fpsAttr = Number(container.getAttribute('data-fps')); + setTimeFormat(format); + setFps(fpsAttr > 0 ? fpsAttr : DEFAULT_FPS); + }; + + const observeContainer = (container: Element): void => { + if (container === observedContainer) return; + attrObserver?.disconnect(); + readAttributes(container); + if (typeof MutationObserver !== 'undefined') { + attrObserver = new MutationObserver(() => readAttributes(container)); + attrObserver.observe(container, { + attributes: true, + attributeFilter: ['data-time-format', 'data-fps'], + }); + } + observedContainer = container; + }; + + const tryObserve = (): void => { + const container = document.querySelector(VIDEO_CONTAINER_SELECTOR); + if (container) { + observeContainer(container); + } + }; + + tryObserve(); + + // Watch for late-appearing container + if (typeof MutationObserver !== 'undefined') { + bodyObserver = new MutationObserver(() => { + const container = document.querySelector(VIDEO_CONTAINER_SELECTOR); + if (container && container !== observedContainer) { + observeContainer(container); + } + }); + bodyObserver.observe(document.body, { childList: true, subtree: true }); + } + + return () => { + attrObserver?.disconnect(); + bodyObserver?.disconnect(); + }; + }, [enabled]); + + return { timeFormat, fps }; +}; diff --git a/src/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.ts b/src/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.ts index d426572512..1957dff173 100644 --- a/src/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.ts +++ b/src/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.ts @@ -1,6 +1,6 @@ import * as React from 'react'; -import { convertMillisecondsToTimestamp } from '../../../utils/timestamp'; +import { formatByTimeFormat, useTimeFormat } from './useTimeFormat'; const VIDEO_CONTAINER_SELECTOR = '.bp-media-container'; @@ -45,6 +45,7 @@ export interface UseVideoTimestampResult { export const useVideoTimestamp = (enabled: boolean): UseVideoTimestampResult => { const [isPressed, setIsPressed] = React.useState(false); const [timestampMs, setTimestampMs] = React.useState(0); + const { timeFormat, fps } = useTimeFormat(enabled); const isPressedRef = React.useRef(isPressed); const isLoadingRef = React.useRef(false); @@ -158,7 +159,7 @@ export const useVideoTimestamp = (enabled: boolean): UseVideoTimestampResult => }, [enabled]); return { - formattedTimestamp: convertMillisecondsToTimestamp(timestampMs), + formattedTimestamp: formatByTimeFormat(timestampMs, timeFormat, fps), isPressed, onPressedChange, timestampMs, diff --git a/src/utils/__tests__/timestamp.test.js b/src/utils/__tests__/timestamp.test.js index dd2b6da290..058801b846 100644 --- a/src/utils/__tests__/timestamp.test.js +++ b/src/utils/__tests__/timestamp.test.js @@ -1,5 +1,7 @@ import { + convertMillisecondsToFrames, convertMillisecondsToHMMSS, + convertMillisecondsToTimecode, convertMillisecondsToTimestamp, convertSecondsToHMMSS, convertTimestampToSeconds, @@ -88,6 +90,70 @@ describe('utils/timestamp', () => { }); }); + describe('convertMillisecondsToTimecode', () => { + test('should format zero as 00:00:00:00', () => { + expect(convertMillisecondsToTimecode(0, 24)).toBe('00:00:00:00'); + }); + + test('should format milliseconds into HH:MM:SS:FF at 24fps', () => { + expect(convertMillisecondsToTimecode(1000, 24)).toBe('00:00:01:00'); + expect(convertMillisecondsToTimecode(60000, 24)).toBe('00:01:00:00'); + expect(convertMillisecondsToTimecode(3600000, 24)).toBe('01:00:00:00'); + }); + + test('should calculate frame remainder correctly', () => { + // 1500ms at 24fps = 36 frames total → 1 sec + 12 frames + expect(convertMillisecondsToTimecode(1500, 24)).toBe('00:00:01:12'); + // 41.666ms per frame at 24fps, so 42ms ≈ 1 frame + expect(convertMillisecondsToTimecode(42, 24)).toBe('00:00:00:01'); + }); + + test('should work with different fps values', () => { + // 1000ms at 30fps = 30 frames → exactly 1 second + expect(convertMillisecondsToTimecode(1000, 30)).toBe('00:00:01:00'); + // 500ms at 30fps = 15 frames + expect(convertMillisecondsToTimecode(500, 30)).toBe('00:00:00:15'); + }); + + test('should handle complex values', () => { + // 61500ms at 30fps = 1845 frames → 1min 1sec 15frames + expect(convertMillisecondsToTimecode(61500, 30)).toBe('00:01:01:15'); + }); + + test('should handle invalid input', () => { + expect(convertMillisecondsToTimecode(-1, 24)).toBe('00:00:00:00'); + expect(convertMillisecondsToTimecode(NaN, 24)).toBe('00:00:00:00'); + }); + }); + + describe('convertMillisecondsToFrames', () => { + test('should return 0 for zero milliseconds', () => { + expect(convertMillisecondsToFrames(0, 24)).toBe(0); + }); + + test('should convert milliseconds to frame count at 24fps', () => { + expect(convertMillisecondsToFrames(1000, 24)).toBe(24); + expect(convertMillisecondsToFrames(2000, 24)).toBe(48); + expect(convertMillisecondsToFrames(10000, 24)).toBe(240); + }); + + test('should convert milliseconds to frame count at 30fps', () => { + expect(convertMillisecondsToFrames(1000, 30)).toBe(30); + expect(convertMillisecondsToFrames(500, 30)).toBe(15); + }); + + test('should floor partial frames', () => { + // 100ms at 24fps = 2.4 frames → floor to 2 + expect(convertMillisecondsToFrames(100, 24)).toBe(2); + }); + + test('should handle invalid input', () => { + expect(convertMillisecondsToFrames(-1, 24)).toBe(0); + expect(convertMillisecondsToFrames(NaN, 24)).toBe(0); + expect(convertMillisecondsToFrames(0, 24)).toBe(0); + }); + }); + describe('convertSecondsToHHMMSS', () => { test('should convert seconds to HH:MM:SS format correctly', () => { expect(convertSecondsToHMMSS(0)).toBe('0:00:00'); diff --git a/src/utils/timestamp.ts b/src/utils/timestamp.ts index 7e5cdd0334..cfa4b74206 100644 --- a/src/utils/timestamp.ts +++ b/src/utils/timestamp.ts @@ -65,4 +65,36 @@ const convertMillisecondsToTimestamp = (timestampInMilliseconds: number): string return `${hours.toString()}:${minutes.toString().padStart(2, '0')}:${paddedSeconds}`; }; -export { convertMillisecondsToHMMSS, convertMillisecondsToTimestamp, convertSecondsToHMMSS, convertTimestampToSeconds }; +const convertMillisecondsToTimecode = (timestampInMilliseconds: number, fps: number): string => { + const seconds = timestampInMilliseconds && timestampInMilliseconds > 0 ? timestampInMilliseconds / 1000 : 0; + const val = Number.isFinite(seconds) ? seconds : 0; + const totalFrames = Math.floor(val * fps); + + const hours = Math.floor(totalFrames / (fps * 3600)); + const minutes = Math.floor((totalFrames % (fps * 3600)) / (fps * 60)); + const secs = Math.floor((totalFrames % (fps * 60)) / fps); + const frames = totalFrames % Math.round(fps); + + const hh = hours.toString().padStart(2, '0'); + const mm = minutes.toString().padStart(2, '0'); + const ss = secs.toString().padStart(2, '0'); + const ff = frames.toString().padStart(2, '0'); + + return `${hh}:${mm}:${ss}:${ff}`; +}; + +const convertMillisecondsToFrames = (timestampInMilliseconds: number, fps: number): number => { + if (!timestampInMilliseconds || timestampInMilliseconds < 0) { + return 0; + } + return Math.floor((timestampInMilliseconds / 1000) * fps); +}; + +export { + convertMillisecondsToFrames, + convertMillisecondsToHMMSS, + convertMillisecondsToTimecode, + convertMillisecondsToTimestamp, + convertSecondsToHMMSS, + convertTimestampToSeconds, +}; From 97ff9db04243747129e7c1089dd748c25708c059 Mon Sep 17 00:00:00 2001 From: Duncan Hsu Date: Wed, 17 Jun 2026 17:41:46 -0700 Subject: [PATCH 2/3] fix(video): fix timecode calculations --- src/constants.js | 1 + src/utils/__tests__/timestamp.test.js | 18 ++++++++++++++++-- src/utils/timestamp.ts | 21 ++++++++++++--------- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/src/constants.js b/src/constants.js index 51c8b59b0f..ac345919ba 100644 --- a/src/constants.js +++ b/src/constants.js @@ -564,3 +564,4 @@ export const KEYS = { /* ----------------- Other ----------------------- */ export const ONE_HOUR_MS = 3600000; // 60 * 60 * 1000 +export const DEFAULT_VIDEO_FPS = 24; diff --git a/src/utils/__tests__/timestamp.test.js b/src/utils/__tests__/timestamp.test.js index 058801b846..85629e2134 100644 --- a/src/utils/__tests__/timestamp.test.js +++ b/src/utils/__tests__/timestamp.test.js @@ -120,10 +120,17 @@ describe('utils/timestamp', () => { expect(convertMillisecondsToTimecode(61500, 30)).toBe('00:01:01:15'); }); - test('should handle invalid input', () => { + test('should handle invalid milliseconds input', () => { expect(convertMillisecondsToTimecode(-1, 24)).toBe('00:00:00:00'); expect(convertMillisecondsToTimecode(NaN, 24)).toBe('00:00:00:00'); }); + + test('should fall back to 24fps when fps is invalid', () => { + expect(convertMillisecondsToTimecode(1000, 0)).toBe('00:00:01:00'); + expect(convertMillisecondsToTimecode(1000, -1)).toBe('00:00:01:00'); + expect(convertMillisecondsToTimecode(1000, NaN)).toBe('00:00:01:00'); + expect(convertMillisecondsToTimecode(1000, Infinity)).toBe('00:00:01:00'); + }); }); describe('convertMillisecondsToFrames', () => { @@ -147,11 +154,18 @@ describe('utils/timestamp', () => { expect(convertMillisecondsToFrames(100, 24)).toBe(2); }); - test('should handle invalid input', () => { + test('should handle invalid milliseconds input', () => { expect(convertMillisecondsToFrames(-1, 24)).toBe(0); expect(convertMillisecondsToFrames(NaN, 24)).toBe(0); expect(convertMillisecondsToFrames(0, 24)).toBe(0); }); + + test('should fall back to 24fps when fps is invalid', () => { + expect(convertMillisecondsToFrames(1000, 0)).toBe(24); + expect(convertMillisecondsToFrames(1000, -1)).toBe(24); + expect(convertMillisecondsToFrames(1000, NaN)).toBe(24); + expect(convertMillisecondsToFrames(1000, Infinity)).toBe(24); + }); }); describe('convertSecondsToHHMMSS', () => { diff --git a/src/utils/timestamp.ts b/src/utils/timestamp.ts index cfa4b74206..3025bcc96b 100644 --- a/src/utils/timestamp.ts +++ b/src/utils/timestamp.ts @@ -2,8 +2,8 @@ * @file Utility functions for timestamp formatting and conversion */ -// @ts-ignore: ONE_HOUR_MS is a constant from a non ts file -import { ONE_HOUR_MS } from '../constants'; +// @ts-ignore: constants from a non ts file +import { ONE_HOUR_MS, DEFAULT_VIDEO_FPS } from '../constants'; /** * Converts a timestamp representation to seconds @@ -67,13 +67,15 @@ const convertMillisecondsToTimestamp = (timestampInMilliseconds: number): string const convertMillisecondsToTimecode = (timestampInMilliseconds: number, fps: number): string => { const seconds = timestampInMilliseconds && timestampInMilliseconds > 0 ? timestampInMilliseconds / 1000 : 0; - const val = Number.isFinite(seconds) ? seconds : 0; - const totalFrames = Math.floor(val * fps); + const validSeconds = Number.isFinite(seconds) ? seconds : 0; + const validFps = Number.isFinite(fps) && fps > 0 ? fps : DEFAULT_VIDEO_FPS; + const totalFrames = Math.floor(validSeconds * validFps); + const frameBase = Math.round(validFps); - const hours = Math.floor(totalFrames / (fps * 3600)); - const minutes = Math.floor((totalFrames % (fps * 3600)) / (fps * 60)); - const secs = Math.floor((totalFrames % (fps * 60)) / fps); - const frames = totalFrames % Math.round(fps); + const hours = Math.floor(totalFrames / (frameBase * 3600)); + const minutes = Math.floor((totalFrames % (frameBase * 3600)) / (frameBase * 60)); + const secs = Math.floor((totalFrames % (frameBase * 60)) / frameBase); + const frames = totalFrames % frameBase; const hh = hours.toString().padStart(2, '0'); const mm = minutes.toString().padStart(2, '0'); @@ -87,7 +89,8 @@ const convertMillisecondsToFrames = (timestampInMilliseconds: number, fps: numbe if (!timestampInMilliseconds || timestampInMilliseconds < 0) { return 0; } - return Math.floor((timestampInMilliseconds / 1000) * fps); + const validFps = Number.isFinite(fps) && fps > 0 ? fps : DEFAULT_VIDEO_FPS; + return Math.floor((timestampInMilliseconds / 1000) * validFps); }; export { From 84e2f4c93392d7482b613e26c834e3c8307d911c Mon Sep 17 00:00:00 2001 From: Duncan Hsu Date: Wed, 17 Jun 2026 17:41:48 -0700 Subject: [PATCH 3/3] refactor(activity-feed-v2): address PR comments --- .../activity-feed-v2/ActivityFeedV2.tsx | 7 ++- .../activity-feed-v2/FeedItemRow.tsx | 10 +++-- .../__tests__/FeedItemRow.test.tsx | 40 ++++------------- .../__tests__/useVideoTimestamp.test.tsx | 43 ++++++++----------- .../activity-feed-v2/useTimeFormat.ts | 10 ++--- .../activity-feed-v2/useVideoTimestamp.ts | 8 ++-- 6 files changed, 47 insertions(+), 71 deletions(-) diff --git a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx index 1b222e041f..a2f0adcc8b 100644 --- a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx +++ b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx @@ -17,6 +17,7 @@ import TaskModal from '../TaskModal'; import FeedItemRow from './FeedItemRow'; import { serializeEditorContent } from './helpers'; import { transformFeedItem } from './transformers'; +import { useTimeFormat } from './useTimeFormat'; import { useVideoTimestamp } from './useVideoTimestamp'; import type { ActivityFeedV2Props, TransformedFeedItem, UserContact } from './types'; @@ -290,13 +291,14 @@ const ActivityFeedV2 = ({ const isVideo = file?.extension ? FILE_EXTENSIONS.video.includes(file.extension) : false; const fileVersionId = file?.file_version?.id; const allowVideoTimestamps = isVideo && isTimestampedCommentsEnabled && Boolean(fileVersionId); + const { timeFormat, fps } = useTimeFormat(isVideo); const { formattedTimestamp, isPressed: isTimestampPressed, onPressedChange, timestampMs, - } = useVideoTimestamp(allowVideoTimestamps); + } = useVideoTimestamp(allowVideoTimestamps, timeFormat, fps); const editorVideoTimestamp = allowVideoTimestamps ? { formattedTimestamp, isPressed: isTimestampPressed, onPressedChange } @@ -357,8 +359,8 @@ const ActivityFeedV2 = ({ ))} diff --git a/src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx b/src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx index 0a8b48b6fe..bd1538642d 100644 --- a/src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx +++ b/src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx @@ -11,10 +11,11 @@ import { ActivityFeed } from '@box/activity-feed'; import type { Annotation, AnnotationPermission } from '../../../common/types/annotations'; import type { BoxCommentPermission, CommentFeedItemType, FeedItemStatus } from '../../../common/types/feed'; import type { TaskCollabStatus, TaskNew } from '../../../common/types/tasks'; +import type { TimeFormat } from './useTimeFormat'; import { dispatchReplyDelete, dispatchReplyEdit, logEditError, serializeEditorContent } from './helpers'; import { annotationTargetToBadge } from './transformers'; -import { formatByTimeFormat, useTimeFormat } from './useTimeFormat'; +import { formatByTimeFormat } from './useTimeFormat'; import { seekVideoToMs } from './useVideoTimestamp'; import type { OnReplyDelete, OnReplyUpdate, TransformedFeedItem, UserSelectorProps } from './types'; @@ -30,8 +31,8 @@ import { type FeedItemRowProps = { currentUserId?: string; + fps: number; isDisabled: boolean; - isVideo?: boolean; item: TransformedFeedItem; onAnnotationCopyLink?: (params: { annotationId: string; fileVersionId: string }) => void; onAnnotationDelete?: (params: { id: string; permissions: AnnotationPermission }) => void; @@ -61,6 +62,7 @@ type FeedItemRowProps = { onTaskEdit?: (task: TaskNew) => void; onTaskView?: (id: string, isCreator: boolean) => void; onVersionHistoryClick?: (version: { id: string; version_number: number }) => void; + timeFormat: TimeFormat; userSelectorProps: UserSelectorProps; }; @@ -80,9 +82,9 @@ const buildReplyPost = const FeedItemRow = ({ currentUserId, + fps, isDisabled, item, - isVideo = false, onAnnotationCopyLink, onAnnotationDelete, onAnnotationEdit, @@ -99,9 +101,9 @@ const FeedItemRow = ({ onTaskEdit, onTaskView, onVersionHistoryClick, + timeFormat, userSelectorProps, }: FeedItemRowProps) => { - const { timeFormat, fps } = useTimeFormat(isVideo); switch (item.type) { case 'comment': { const { permissions } = item; diff --git a/src/elements/content-sidebar/activity-feed-v2/__tests__/FeedItemRow.test.tsx b/src/elements/content-sidebar/activity-feed-v2/__tests__/FeedItemRow.test.tsx index 14b9914125..18392fb715 100644 --- a/src/elements/content-sidebar/activity-feed-v2/__tests__/FeedItemRow.test.tsx +++ b/src/elements/content-sidebar/activity-feed-v2/__tests__/FeedItemRow.test.tsx @@ -57,12 +57,6 @@ jest.mock('../useVideoTimestamp', () => ({ seekVideoToMs: jest.fn(), })); -const mockTimeFormat: { timeFormat: string; fps: number } = { timeFormat: 'standard', fps: 24 }; -jest.mock('../useTimeFormat', () => ({ - ...jest.requireActual('../useTimeFormat'), - useTimeFormat: () => mockTimeFormat, -})); - const mockedSerializeEditorContent = jest.mocked(serializeEditorContent); const mockedDispatchReplyDelete = jest.mocked(dispatchReplyDelete); const mockedDispatchReplyEdit = jest.mocked(dispatchReplyEdit); @@ -205,7 +199,9 @@ const mockAppActivity: TransformedFeedItem = { }; const defaultProps = { + fps: 24, isDisabled: false, + timeFormat: 'standard' as const, userSelectorProps, }; @@ -961,15 +957,12 @@ describe('elements/content-sidebar/activity-feed-v2/FeedItemRow', () => { describe('time format-aware badge rendering', () => { test('should format comment badge timestamp using the current time format', () => { - mockTimeFormat.timeFormat = 'timecode'; - mockTimeFormat.fps = 24; - const timestampedComment: TransformedCommentItem = { ...mockComment, annotationTarget: { timestamp: '0:08', type: AnnotationBadgeType.Frame }, annotationTimestampMs: 8055, }; - render(); + render(); expect(lastThreadedAnnotationProps.annotationTarget).toEqual({ timestamp: '00:00:08:01', @@ -978,15 +971,12 @@ describe('elements/content-sidebar/activity-feed-v2/FeedItemRow', () => { }); test('should format comment badge timestamp as frame number', () => { - mockTimeFormat.timeFormat = 'frames'; - mockTimeFormat.fps = 24; - const timestampedComment: TransformedCommentItem = { ...mockComment, annotationTarget: { timestamp: '0:10', type: AnnotationBadgeType.Frame }, annotationTimestampMs: 10000, }; - render(); + render(); expect(lastThreadedAnnotationProps.annotationTarget).toEqual({ timestamp: '240', @@ -995,18 +985,12 @@ describe('elements/content-sidebar/activity-feed-v2/FeedItemRow', () => { }); test('should not modify comment badge when annotationTimestampMs is undefined', () => { - mockTimeFormat.timeFormat = 'timecode'; - mockTimeFormat.fps = 24; - - render(); + render(); expect(lastThreadedAnnotationProps.annotationTarget).toBeUndefined(); }); test('should format annotation badge timestamp for frame-type annotations', () => { - mockTimeFormat.timeFormat = 'frames'; - mockTimeFormat.fps = 30; - const frameAnnotation: TransformedAnnotationItem = { ...mockAnnotation, annotation: { @@ -1018,7 +1002,7 @@ describe('elements/content-sidebar/activity-feed-v2/FeedItemRow', () => { const badge: AnnotationBadgeTargetType = { timestamp: '0:05', type: AnnotationBadgeType.Frame }; mockedAnnotationTargetToBadge.mockReturnValue(badge); - render(); + render(); expect(lastThreadedAnnotationProps.annotationTarget).toEqual({ timestamp: '150', @@ -1027,27 +1011,21 @@ describe('elements/content-sidebar/activity-feed-v2/FeedItemRow', () => { }); test('should not modify annotation badge for non-frame targets', () => { - mockTimeFormat.timeFormat = 'timecode'; - mockTimeFormat.fps = 24; - const badge: AnnotationBadgeTargetType = { page: 3, type: AnnotationBadgeType.Point }; mockedAnnotationTargetToBadge.mockReturnValue(badge); - render(); + render(); expect(lastThreadedAnnotationProps.annotationTarget).toBe(badge); }); - test('should use standard format when isVideo is false', () => { - mockTimeFormat.timeFormat = 'standard'; - mockTimeFormat.fps = 24; - + test('should use standard format when timeFormat is standard', () => { const timestampedComment: TransformedCommentItem = { ...mockComment, annotationTarget: { timestamp: '0:08', type: AnnotationBadgeType.Frame }, annotationTimestampMs: 8055, }; - render(); + render(); expect(lastThreadedAnnotationProps.annotationTarget).toEqual({ timestamp: '0:08', diff --git a/src/elements/content-sidebar/activity-feed-v2/__tests__/useVideoTimestamp.test.tsx b/src/elements/content-sidebar/activity-feed-v2/__tests__/useVideoTimestamp.test.tsx index 1f0a31926a..8dde6b23f1 100644 --- a/src/elements/content-sidebar/activity-feed-v2/__tests__/useVideoTimestamp.test.tsx +++ b/src/elements/content-sidebar/activity-feed-v2/__tests__/useVideoTimestamp.test.tsx @@ -2,6 +2,7 @@ import * as React from 'react'; import { act, render, screen } from '@testing-library/react'; import { seekVideoToMs, useVideoTimestamp } from '../useVideoTimestamp'; +import type { TimeFormat } from '../useTimeFormat'; const createVideoElement = (currentTime: number = 0): HTMLVideoElement => { const video = document.createElement('video'); @@ -31,8 +32,16 @@ const mountVideoInDom = (video: HTMLVideoElement) => { return () => container.remove(); }; -const TestHarness = ({ enabled }: { enabled: boolean }) => { - const { formattedTimestamp, isPressed, onPressedChange, timestampMs } = useVideoTimestamp(enabled); +const TestHarness = ({ + enabled, + fps = 24, + timeFormat = 'standard', +}: { + enabled: boolean; + fps?: number; + timeFormat?: TimeFormat; +}) => { + const { formattedTimestamp, isPressed, onPressedChange, timestampMs } = useVideoTimestamp(enabled, timeFormat, fps); return (
{formattedTimestamp} @@ -296,15 +305,11 @@ describe('useVideoTimestamp time format integration', () => { document.querySelectorAll('.bp-media-container').forEach(node => node.remove()); }); - test('should format timestamp as timecode when data-time-format is timecode', async () => { + test('should format timestamp as timecode when timeFormat is timecode', () => { const video = createVideoElement(8.055); const cleanup = mountVideoInDom(video); try { - const container = document.querySelector('.bp-media-container')!; - container.setAttribute('data-time-format', 'timecode'); - container.setAttribute('data-fps', '24'); - - render(); + render(); act(() => { screen.getByText('press').click(); }); @@ -315,15 +320,11 @@ describe('useVideoTimestamp time format integration', () => { } }); - test('should format timestamp as frame number when data-time-format is frames', async () => { + test('should format timestamp as frame number when timeFormat is frames', () => { const video = createVideoElement(10); const cleanup = mountVideoInDom(video); try { - const container = document.querySelector('.bp-media-container')!; - container.setAttribute('data-time-format', 'frames'); - container.setAttribute('data-fps', '24'); - - render(); + render(); act(() => { screen.getByText('press').click(); }); @@ -334,30 +335,24 @@ describe('useVideoTimestamp time format integration', () => { } }); - test('should update formatted timestamp when time format changes after capture', async () => { + test('should update formatted timestamp when timeFormat prop changes after capture', () => { const video = createVideoElement(10); const cleanup = mountVideoInDom(video); try { - const container = document.querySelector('.bp-media-container')!; - container.setAttribute('data-time-format', 'standard'); - container.setAttribute('data-fps', '24'); - - render(); + const { rerender } = render(); act(() => { screen.getByText('press').click(); }); expect(screen.getByTestId('timestamp').textContent).toBe('0:10'); - await act(async () => { - container.setAttribute('data-time-format', 'frames'); - }); + rerender(); expect(screen.getByTestId('timestamp').textContent).toBe('240'); } finally { cleanup(); } }); - test('should default to standard format when no data attribute is set', () => { + test('should default to standard format', () => { const video = createVideoElement(43.5); const cleanup = mountVideoInDom(video); try { diff --git a/src/elements/content-sidebar/activity-feed-v2/useTimeFormat.ts b/src/elements/content-sidebar/activity-feed-v2/useTimeFormat.ts index b5706c3c0f..8bbed27d57 100644 --- a/src/elements/content-sidebar/activity-feed-v2/useTimeFormat.ts +++ b/src/elements/content-sidebar/activity-feed-v2/useTimeFormat.ts @@ -5,9 +5,9 @@ import { convertMillisecondsToTimecode, convertMillisecondsToTimestamp, } from '../../../utils/timestamp'; +import { DEFAULT_VIDEO_FPS } from '../../../constants'; -const VIDEO_CONTAINER_SELECTOR = '.bp-media-container'; -const DEFAULT_FPS = 24; +export const VIDEO_CONTAINER_SELECTOR = '.bp-media-container'; export type TimeFormat = 'standard' | 'timecode' | 'frames'; @@ -30,12 +30,12 @@ export interface UseTimeFormatResult { export const useTimeFormat = (enabled: boolean): UseTimeFormatResult => { const [timeFormat, setTimeFormat] = React.useState('standard'); - const [fps, setFps] = React.useState(DEFAULT_FPS); + const [fps, setFps] = React.useState(DEFAULT_VIDEO_FPS); React.useEffect(() => { if (!enabled) { setTimeFormat('standard'); - setFps(DEFAULT_FPS); + setFps(DEFAULT_VIDEO_FPS); return undefined; } @@ -51,7 +51,7 @@ export const useTimeFormat = (enabled: boolean): UseTimeFormatResult => { const format = (container.getAttribute('data-time-format') as TimeFormat) || 'standard'; const fpsAttr = Number(container.getAttribute('data-fps')); setTimeFormat(format); - setFps(fpsAttr > 0 ? fpsAttr : DEFAULT_FPS); + setFps(fpsAttr > 0 ? fpsAttr : DEFAULT_VIDEO_FPS); }; const observeContainer = (container: Element): void => { diff --git a/src/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.ts b/src/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.ts index 1957dff173..fe597b893a 100644 --- a/src/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.ts +++ b/src/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.ts @@ -1,8 +1,7 @@ import * as React from 'react'; -import { formatByTimeFormat, useTimeFormat } from './useTimeFormat'; - -const VIDEO_CONTAINER_SELECTOR = '.bp-media-container'; +import { formatByTimeFormat, VIDEO_CONTAINER_SELECTOR } from './useTimeFormat'; +import type { TimeFormat } from './useTimeFormat'; const findVideoElement = (): HTMLVideoElement | null => { if (typeof document === 'undefined') { @@ -42,10 +41,9 @@ export interface UseVideoTimestampResult { * - Toggle off->on: captures current time and pauses the video if it was playing. * - New video src: captured value resets to 0; pressed state persists. */ -export const useVideoTimestamp = (enabled: boolean): UseVideoTimestampResult => { +export const useVideoTimestamp = (enabled: boolean, timeFormat: TimeFormat, fps: number): UseVideoTimestampResult => { const [isPressed, setIsPressed] = React.useState(false); const [timestampMs, setTimestampMs] = React.useState(0); - const { timeFormat, fps } = useTimeFormat(enabled); const isPressedRef = React.useRef(isPressed); const isLoadingRef = React.useRef(false);