From 39a0b1efc36781a402eb1b9e9ef11eb5618985df Mon Sep 17 00:00:00 2001 From: jackiejou <21050234+jackiejou@users.noreply.github.com> Date: Mon, 4 May 2026 17:11:38 -0700 Subject: [PATCH 1/2] feat(popup): Show resolved state and user avatars in thread --- src/@types/model.ts | 5 +++ src/common/BaseAnnotator.ts | 2 + src/components/Popups/PopupV2.tsx | 43 +++++++++++++++++-- .../Popups/__tests__/PopupV2-test.tsx | 17 ++++++-- src/store/options/__tests__/selectors-test.ts | 2 + src/store/options/reducer.ts | 4 +- src/store/options/selectors.ts | 2 + src/store/options/types.ts | 2 + 8 files changed, 69 insertions(+), 8 deletions(-) diff --git a/src/@types/model.ts b/src/@types/model.ts index e5bbbb037..49ca4caa2 100644 --- a/src/@types/model.ts +++ b/src/@types/model.ts @@ -12,6 +12,11 @@ export interface Annotation { modified_by: User; permissions: Permissions; replies?: Array; + resolution?: { + resolved_at: string; + resolved_by: User; + } | null; + status?: 'open' | 'resolved'; target: Target; type: 'annotation'; } diff --git a/src/common/BaseAnnotator.ts b/src/common/BaseAnnotator.ts index 7b658298d..1c8a6c0d7 100644 --- a/src/common/BaseAnnotator.ts +++ b/src/common/BaseAnnotator.ts @@ -94,11 +94,13 @@ export default class BaseAnnotator extends EventEmitter { }, common: { color: initialColor, mode: initialMode }, options: { + apiHost, features, fileId: file.id, fileVersionId: fileOptionsVersionId ?? fileVersionId, isCurrentFileVersion: !fileOptionsVersionId || fileOptionsVersionId === currentFileVersionId, permissions: file.permissions, + token, viewMode: initialViewMode, }, }; diff --git a/src/components/Popups/PopupV2.tsx b/src/components/Popups/PopupV2.tsx index 76aa86640..cd8ec17a5 100644 --- a/src/components/Popups/PopupV2.tsx +++ b/src/components/Popups/PopupV2.tsx @@ -10,6 +10,7 @@ import { serializeMentionMarkup, } from '@box/threaded-annotations'; import type { DocumentNodeV2, TextMessageTypeV2 } from '@box/threaded-annotations'; +import type { FetchedAvatarUrls, UserContactType } from '@box/user-selector'; import type { JSONContent } from '@tiptap/core'; import FocusTrap from 'box-ui-elements/es/components/focus-trap/FocusTrap'; @@ -21,6 +22,7 @@ import { updateAnnotationAction, } from '../../store/annotations/actions'; import { getAnnotation } from '../../store/annotations/selectors'; +import { getApiHost, getToken } from '../../store/options'; import { fetchCollaboratorsAction } from '../../store/users/actions'; import type { AppState, AppThunkDispatch } from '../../store/types'; @@ -57,6 +59,9 @@ const createDocumentNode = (content: JSONContent | null): DocumentNodeV2 => { return { type: 'doc', content: [content] } as DocumentNodeV2; }; +const getAvatarUrl = (apiHost: string, token: string, userId: string): string => + `${apiHost}/2.0/users/${userId}/avatar?access_token=${token}&pic_type=large`; + const PopupV2 = ({ annotationId, onSubmit, reference }: Props): JSX.Element => { const intl = useIntl(); const dispatch = useDispatch(); @@ -64,6 +69,8 @@ const PopupV2 = ({ annotationId, onSubmit, reference }: Props): JSX.Element => { const popperRef = React.useRef(); const optionsRef = React.useRef>(getPopupOptions()); + const apiHost = useSelector(getApiHost); + const token = useSelector(getToken); const annotation = useSelector((state: AppState) => annotationId ? getAnnotation(state, annotationId) : undefined, ); @@ -83,15 +90,40 @@ const PopupV2 = ({ annotationId, onSubmit, reference }: Props): JSX.Element => { }, []); const threadMessages: TextMessageTypeV2[] = React.useMemo( - () => (annotation ? annotationToMessages(annotation) : []), - [annotation], + () => { + if (!annotation) return []; + return annotationToMessages(annotation).map(msg => ({ + ...msg, + author: { + ...msg.author, + avatarUrl: getAvatarUrl(apiHost, token, String(msg.author.id)), + }, + })); + }, + [annotation, apiHost, token], ); + const isResolved = annotation?.status === 'resolved'; + const resolvedBy = isResolved + ? annotation?.resolution?.resolved_by?.name ?? annotation?.modified_by?.name + : undefined; + const resolvedAtSource = isResolved + ? annotation?.resolution?.resolved_at ?? annotation?.modified_at + : undefined; + const resolvedAt = resolvedAtSource ? new Date(resolvedAtSource).getTime() : undefined; + const userSelectorProps = React.useMemo( () => ({ allowEmptyQuery: true, ariaRoleDescription: intl.formatMessage(messages.ariaLabelMentionSelector), - fetchAvatarUrls: async () => ({}), + fetchAvatarUrls: async (userContacts: UserContactType[]): Promise => { + const urls: FetchedAvatarUrls = {}; + userContacts.forEach(({ id }) => { + const key = String(id); + urls[key] = getAvatarUrl(apiHost, token, key); + }); + return urls; + }, fetchUsers: async (query: string) => { const action = await dispatch(fetchCollaboratorsAction(query)); if (fetchCollaboratorsAction.fulfilled.match(action)) { @@ -101,7 +133,7 @@ const PopupV2 = ({ annotationId, onSubmit, reference }: Props): JSX.Element => { }, loadingAriaLabel: intl.formatMessage(messages.ariaLabelMentionLoading), }), - [dispatch, intl], + [apiHost, dispatch, intl, token], ); const handlePost = React.useCallback( @@ -169,6 +201,7 @@ const PopupV2 = ({ annotationId, onSubmit, reference }: Props): JSX.Element => { {annotationId ? ( { onResolve={handleResolve} onThreadDelete={handleThreadDelete} onUnresolve={handleUnresolve} + resolvedAt={resolvedAt} + resolvedBy={resolvedBy} userSelectorProps={userSelectorProps} /> ) : ( diff --git a/src/components/Popups/__tests__/PopupV2-test.tsx b/src/components/Popups/__tests__/PopupV2-test.tsx index 5c6917456..134af2ef0 100644 --- a/src/components/Popups/__tests__/PopupV2-test.tsx +++ b/src/components/Popups/__tests__/PopupV2-test.tsx @@ -67,6 +67,14 @@ jest.mock('../../../store/users/actions', () => ({ const mockUseDispatch = useDispatch as jest.MockedFunction; const mockUseSelector = useSelector as jest.MockedFunction; +// useSelector is called for: apiHost, token, annotation +const mockSelectorValues = (annotation?: unknown): void => { + mockUseSelector + .mockReturnValueOnce('https://api.box.com') // apiHost + .mockReturnValueOnce('test-token') // token + .mockReturnValueOnce(annotation); // annotation +}; + describe('PopupV2', () => { const mockDispatch = jest.fn(); @@ -105,7 +113,7 @@ describe('PopupV2', () => { }; beforeEach(() => { - mockUseSelector.mockReturnValue(undefined); + mockSelectorValues(undefined); }); test('should render MessageEditorV2 with FocusTrap and MentionContextProvider', () => { @@ -139,7 +147,7 @@ describe('PopupV2', () => { }; beforeEach(() => { - mockUseSelector.mockReturnValue(mockAnnotation); + mockSelectorValues(mockAnnotation); }); test('should render ThreadedAnnotationsV2 with FocusTrap and MentionContextProvider', () => { @@ -160,7 +168,8 @@ describe('PopupV2', () => { }); test('should render empty messages when annotation is not found', () => { - mockUseSelector.mockReturnValue(undefined); + mockUseSelector.mockReset(); + mockSelectorValues(undefined); render(); expect(screen.getByTestId('threaded-annotations-v2').getAttribute('data-messages-count')).toBe('0'); @@ -185,12 +194,14 @@ describe('PopupV2', () => { }); test('should set aria-label on popup container', () => { + mockSelectorValues(undefined); render(); expect(screen.getByRole('presentation')).toHaveAttribute('aria-label', 'Comment'); }); test('should render portal container for threaded-annotations popovers', () => { + mockSelectorValues(undefined); render(); const portal = screen.getByRole('presentation').querySelector('[data-threaded-annotations-portal]'); diff --git a/src/store/options/__tests__/selectors-test.ts b/src/store/options/__tests__/selectors-test.ts index 2f13904d5..3595e968d 100644 --- a/src/store/options/__tests__/selectors-test.ts +++ b/src/store/options/__tests__/selectors-test.ts @@ -11,6 +11,7 @@ import { describe('store/options/selectors', () => { const optionsState = { + apiHost: 'https://api.box.com', features: { enabledFeature: true }, fileId: '12345', fileVersionId: '67890', @@ -21,6 +22,7 @@ describe('store/options/selectors', () => { }, rotation: 0, scale: 1, + token: 'test-token', viewMode: 'annotations' as const, }; diff --git a/src/store/options/reducer.ts b/src/store/options/reducer.ts index c6afb0765..be642d9d7 100644 --- a/src/store/options/reducer.ts +++ b/src/store/options/reducer.ts @@ -9,7 +9,8 @@ import { setViewModeAction, } from './actions'; -export const initialState = { +export const initialState: OptionsState = { + apiHost: '', features: {}, fileId: null, fileVersionId: null, @@ -17,6 +18,7 @@ export const initialState = { permissions: {}, rotation: 0, scale: 1, + token: '', viewMode: 'annotations' as ViewMode, }; diff --git a/src/store/options/selectors.ts b/src/store/options/selectors.ts index 2649128c0..0f7031d87 100644 --- a/src/store/options/selectors.ts +++ b/src/store/options/selectors.ts @@ -6,6 +6,7 @@ import { ViewMode } from './types'; type State = Pick; +export const getApiHost = (state: State): string => state.options.apiHost; export const getFeatures = (state: State): Features => state.options.features; export const getViewMode = (state: State): ViewMode => state.options.viewMode; export const getFileId = (state: State): string | null => state.options.fileId; @@ -14,5 +15,6 @@ export const getIsCurrentFileVersion = (state: State): boolean => state.options. export const getPermissions = (state: State): Permissions => state.options.permissions; export const getRotation = (state: State): number => state.options.rotation; export const getScale = (state: State): number => state.options.scale; +export const getToken = (state: State): string => state.options.token; export const isFeatureEnabled = (state: State, featurename: string): boolean => getProp(getFeatures(state), featurename, false); diff --git a/src/store/options/types.ts b/src/store/options/types.ts index af31957f3..9abfd9ab0 100644 --- a/src/store/options/types.ts +++ b/src/store/options/types.ts @@ -5,6 +5,7 @@ import { Permissions } from '../../@types'; export type ViewMode = 'annotations' | 'boundingBoxes'; export type OptionsState = { + apiHost: string; features: Features; fileId: string | null; fileVersionId: string | null; @@ -12,5 +13,6 @@ export type OptionsState = { permissions: Permissions; rotation: number; scale: number; + token: string; viewMode: ViewMode; }; From f236aa66627b9d59fe5752fe62c11d4472d73909 Mon Sep 17 00:00:00 2001 From: jackiejou <21050234+jackiejou@users.noreply.github.com> Date: Tue, 5 May 2026 10:56:42 -0700 Subject: [PATCH 2/2] fix(popup): Use Authorization header for avatar requests Fetch user avatars via an Authorization header and render them from blob URLs, so the access token is no longer appended to the avatar URL as a query parameter. Blob URLs are cached and revoked on unmount or when credentials change. In-flight requests compare captured credentials against the current ref so a token swap cannot land a stale blob in the cache. --- src/components/Popups/PopupV2.tsx | 123 +++++++++++++++--- .../Popups/__tests__/PopupV2-test.tsx | 63 +++++++-- 2 files changed, 154 insertions(+), 32 deletions(-) diff --git a/src/components/Popups/PopupV2.tsx b/src/components/Popups/PopupV2.tsx index cd8ec17a5..13512f84e 100644 --- a/src/components/Popups/PopupV2.tsx +++ b/src/components/Popups/PopupV2.tsx @@ -59,8 +59,20 @@ const createDocumentNode = (content: JSONContent | null): DocumentNodeV2 => { return { type: 'doc', content: [content] } as DocumentNodeV2; }; -const getAvatarUrl = (apiHost: string, token: string, userId: string): string => - `${apiHost}/2.0/users/${userId}/avatar?access_token=${token}&pic_type=large`; +// Callers render initials as a fallback on null. +// A persistent null across all users usually indicates a stale token. +const fetchAvatarBlob = async (apiHost: string, token: string, userId: string): Promise => { + try { + const response = await fetch(`${apiHost}/2.0/users/${userId}/avatar?pic_type=large`, { + headers: { Authorization: `Bearer ${token}` }, + }); + if (!response.ok) return null; + const blob = await response.blob(); + return URL.createObjectURL(blob); + } catch { + return null; + } +}; const PopupV2 = ({ annotationId, onSubmit, reference }: Props): JSX.Element => { const intl = useIntl(); @@ -75,6 +87,37 @@ const PopupV2 = ({ annotationId, onSubmit, reference }: Props): JSX.Element => { annotationId ? getAnnotation(state, annotationId) : undefined, ); + const [avatarBlobs, setAvatarBlobs] = React.useState>({}); + const avatarCacheRef = React.useRef>(new Map()); + const credentialsRef = React.useRef({ apiHost, token }); + credentialsRef.current = { apiHost, token }; + + const getOrFetchAvatarBlob = React.useCallback( + async (userId: string): Promise => { + const cached = avatarCacheRef.current.get(userId); + if (cached) return cached; + const capturedApiHost = apiHost; + const capturedToken = token; + const url = await fetchAvatarBlob(capturedApiHost, capturedToken, userId); + if (!url) return null; + if ( + credentialsRef.current.apiHost !== capturedApiHost || + credentialsRef.current.token !== capturedToken + ) { + URL.revokeObjectURL(url); + return null; + } + const existing = avatarCacheRef.current.get(userId); + if (existing) { + URL.revokeObjectURL(url); + return existing; + } + avatarCacheRef.current.set(userId, url); + return url; + }, + [apiHost, token], + ); + React.useEffect(() => { if (popupRef.current) { popperRef.current = createPopper(reference, popupRef.current, optionsRef.current); @@ -85,23 +128,58 @@ const PopupV2 = ({ annotationId, onSubmit, reference }: Props): JSX.Element => { }; }, [reference]); + React.useEffect(() => { + const cache = avatarCacheRef.current; + return () => { + cache.forEach(url => URL.revokeObjectURL(url)); + cache.clear(); + }; + }, []); + + React.useEffect(() => { + avatarCacheRef.current.forEach(url => URL.revokeObjectURL(url)); + avatarCacheRef.current.clear(); + setAvatarBlobs({}); + }, [apiHost, token]); + const handleEvent = React.useCallback((event: React.SyntheticEvent) => { event.stopPropagation(); }, []); - const threadMessages: TextMessageTypeV2[] = React.useMemo( - () => { - if (!annotation) return []; - return annotationToMessages(annotation).map(msg => ({ - ...msg, - author: { - ...msg.author, - avatarUrl: getAvatarUrl(apiHost, token, String(msg.author.id)), - }, - })); - }, - [annotation, apiHost, token], - ); + React.useEffect(() => { + if (!annotation) return undefined; + + const userIds = Array.from( + new Set(annotationToMessages(annotation).map(msg => String(msg.author.id))), + ); + let cancelled = false; + + Promise.all(userIds.map(async id => [id, await getOrFetchAvatarBlob(id)] as const)).then(entries => { + if (cancelled) return; + setAvatarBlobs(prev => { + const next = { ...prev }; + entries.forEach(([id, url]) => { + if (url) next[id] = url; + }); + return next; + }); + }); + + return () => { + cancelled = true; + }; + }, [annotation, getOrFetchAvatarBlob]); + + const threadMessages: TextMessageTypeV2[] = React.useMemo(() => { + if (!annotation) return []; + return annotationToMessages(annotation).map(msg => ({ + ...msg, + author: { + ...msg.author, + avatarUrl: avatarBlobs[String(msg.author.id)], + }, + })); + }, [annotation, avatarBlobs]); const isResolved = annotation?.status === 'resolved'; const resolvedBy = isResolved @@ -118,10 +196,15 @@ const PopupV2 = ({ annotationId, onSubmit, reference }: Props): JSX.Element => { ariaRoleDescription: intl.formatMessage(messages.ariaLabelMentionSelector), fetchAvatarUrls: async (userContacts: UserContactType[]): Promise => { const urls: FetchedAvatarUrls = {}; - userContacts.forEach(({ id }) => { - const key = String(id); - urls[key] = getAvatarUrl(apiHost, token, key); - }); + await Promise.all( + userContacts.map(async ({ id }) => { + const key = String(id); + const blobUrl = await getOrFetchAvatarBlob(key); + if (blobUrl) { + urls[key] = blobUrl; + } + }), + ); return urls; }, fetchUsers: async (query: string) => { @@ -133,7 +216,7 @@ const PopupV2 = ({ annotationId, onSubmit, reference }: Props): JSX.Element => { }, loadingAriaLabel: intl.formatMessage(messages.ariaLabelMentionLoading), }), - [apiHost, dispatch, intl, token], + [dispatch, getOrFetchAvatarBlob, intl], ); const handlePost = React.useCallback( diff --git a/src/components/Popups/__tests__/PopupV2-test.tsx b/src/components/Popups/__tests__/PopupV2-test.tsx index 134af2ef0..4c6a19d86 100644 --- a/src/components/Popups/__tests__/PopupV2-test.tsx +++ b/src/components/Popups/__tests__/PopupV2-test.tsx @@ -1,7 +1,8 @@ import React from 'react'; -import { render, screen } from '@testing-library/react'; +import { act, render, screen } from '@testing-library/react'; import { useDispatch, useSelector } from 'react-redux'; import PopupV2, { Props } from '../PopupV2'; +import { getApiHost, getToken } from '../../../store/options'; jest.mock('react-redux', () => ({ useDispatch: jest.fn(), @@ -67,16 +68,34 @@ jest.mock('../../../store/users/actions', () => ({ const mockUseDispatch = useDispatch as jest.MockedFunction; const mockUseSelector = useSelector as jest.MockedFunction; -// useSelector is called for: apiHost, token, annotation const mockSelectorValues = (annotation?: unknown): void => { - mockUseSelector - .mockReturnValueOnce('https://api.box.com') // apiHost - .mockReturnValueOnce('test-token') // token - .mockReturnValueOnce(annotation); // annotation + mockUseSelector.mockImplementation(selector => { + if (selector === getApiHost) return 'https://api.box.com'; + if (selector === getToken) return 'test-token'; + return annotation; + }); }; describe('PopupV2', () => { const mockDispatch = jest.fn(); + const mockFetch = jest.fn(); + const originalFetch = window.fetch; + const originalCreateObjectURL = window.URL.createObjectURL; + const originalRevokeObjectURL = window.URL.revokeObjectURL; + + beforeAll(() => { + window.fetch = mockFetch as unknown as typeof fetch; + window.URL.createObjectURL = jest.fn().mockReturnValue('blob:mock-url'); + window.URL.revokeObjectURL = jest.fn(); + }); + + afterAll(() => { + window.fetch = originalFetch; + window.URL.createObjectURL = originalCreateObjectURL; + window.URL.revokeObjectURL = originalRevokeObjectURL; + }); + + const flushPromises = (): Promise => act(() => new Promise(resolve => { setTimeout(resolve, 0); })); const mockAnnotation = { created_at: '2026-01-01T00:00:00Z', @@ -100,6 +119,10 @@ describe('PopupV2', () => { beforeEach(() => { mockUseDispatch.mockReturnValue(mockDispatch); + mockFetch.mockResolvedValue({ + blob: () => Promise.resolve(new Blob(['avatar'])), + ok: true, + }); }); afterEach(() => { @@ -150,8 +173,9 @@ describe('PopupV2', () => { mockSelectorValues(mockAnnotation); }); - test('should render ThreadedAnnotationsV2 with FocusTrap and MentionContextProvider', () => { + test('should render ThreadedAnnotationsV2 with FocusTrap and MentionContextProvider', async () => { render(); + await flushPromises(); expect(screen.getByTestId('focus-trap')).toBeVisible(); expect(screen.getByTestId('mention-context')).toBeVisible(); @@ -159,24 +183,26 @@ describe('PopupV2', () => { expect(screen.queryByTestId('message-editor-v2')).toBeNull(); }); - test('should render with isAnnotations=true and messages from annotation', () => { + test('should render with isAnnotations=true and messages from annotation', async () => { render(); + await flushPromises(); const thread = screen.getByTestId('threaded-annotations-v2'); expect(thread.getAttribute('data-is-annotations')).toBe('true'); expect(thread.getAttribute('data-messages-count')).toBe('1'); }); - test('should render empty messages when annotation is not found', () => { - mockUseSelector.mockReset(); + test('should render empty messages when annotation is not found', async () => { mockSelectorValues(undefined); render(); + await flushPromises(); expect(screen.getByTestId('threaded-annotations-v2').getAttribute('data-messages-count')).toBe('0'); }); - test('should pass all action callbacks to ThreadedAnnotationsV2', () => { + test('should pass all action callbacks to ThreadedAnnotationsV2', async () => { render(); + await flushPromises(); const thread = screen.getByTestId('threaded-annotations-v2'); expect(thread.getAttribute('data-has-on-post')).toBe('true'); @@ -185,12 +211,25 @@ describe('PopupV2', () => { expect(thread.getAttribute('data-has-on-unresolve')).toBe('true'); }); - test('should set popupThreadV2 as resin component', () => { + test('should set popupThreadV2 as resin component', async () => { render(); + await flushPromises(); const popup = screen.getByRole('presentation'); expect(popup).toHaveAttribute('data-resin-component', 'popupThreadV2'); }); + + test('should fetch avatars with Authorization header and no access_token query param', async () => { + render(); + await flushPromises(); + + expect(mockFetch).toHaveBeenCalledWith( + 'https://api.box.com/2.0/users/100/avatar?pic_type=large', + { headers: { Authorization: 'Bearer test-token' } }, + ); + const [calledUrl] = mockFetch.mock.calls[0]; + expect(calledUrl).not.toContain('access_token'); + }); }); test('should set aria-label on popup container', () => {