From 64be76d2aaa9ff26779bd7324f091b1f7029f710 Mon Sep 17 00:00:00 2001 From: Jackie Jou Date: Mon, 22 Jun 2026 16:59:23 -0700 Subject: [PATCH 1/2] fix(activity-feed-v2): resolve avatar URLs via getAvatarUrl callback --- .../activity-feed-v2/ActivityFeedV2.tsx | 7 +- .../__tests__/transformers.test.ts | 114 ++++++++++++- .../__tests__/useAvatarUrls.test.tsx | 157 ++++++++++++++++++ .../activity-feed-v2/transformers.ts | 60 ++++--- .../content-sidebar/activity-feed-v2/types.ts | 7 +- .../activity-feed-v2/useAvatarUrls.ts | 108 ++++++++++++ 6 files changed, 425 insertions(+), 28 deletions(-) create mode 100644 src/elements/content-sidebar/activity-feed-v2/__tests__/useAvatarUrls.test.tsx create mode 100644 src/elements/content-sidebar/activity-feed-v2/useAvatarUrls.ts diff --git a/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx b/src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx index a2f0adcc8b..7c0d9b0e96 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 { useAvatarUrls } from './useAvatarUrls'; import { useTimeFormat } from './useTimeFormat'; import { useVideoTimestamp } from './useVideoTimestamp'; @@ -203,16 +204,18 @@ const ActivityFeedV2 = ({ [getTaskCollaborators], ); + const avatarUrls = useAvatarUrls(feedItems, getAvatarUrl); + const transformedItems: TransformedFeedItem[] = React.useMemo(() => { if (!feedItems) return []; return feedItems.reduce((acc, item) => { - const transformed = transformFeedItem(item, currentUserId); + const transformed = transformFeedItem(item, currentUserId, avatarUrls); if (transformed) { acc.push(transformed); } return acc; }, []); - }, [currentUserId, feedItems]); + }, [avatarUrls, currentUserId, feedItems]); const filteredItems = React.useMemo(() => { const filtered = transformedItems.filter(item => { diff --git a/src/elements/content-sidebar/activity-feed-v2/__tests__/transformers.test.ts b/src/elements/content-sidebar/activity-feed-v2/__tests__/transformers.test.ts index 593764b840..6fbd773511 100644 --- a/src/elements/content-sidebar/activity-feed-v2/__tests__/transformers.test.ts +++ b/src/elements/content-sidebar/activity-feed-v2/__tests__/transformers.test.ts @@ -330,7 +330,7 @@ describe('elements/content-sidebar/activity-feed-v2/transformers', () => { const result = transformTaskToProps(mockTask as unknown as TaskNew); expect(result.assignees).toHaveLength(1); expect(result.assignees[0]).toEqual({ - avatarUrl: 'https://example.com/assignee.png', + avatarUrl: undefined, completedAt: undefined, id: '100', name: 'Assignee One', @@ -342,7 +342,7 @@ describe('elements/content-sidebar/activity-feed-v2/transformers', () => { test('should transform task author from created_by.target', () => { const result = transformTaskToProps(mockTask as unknown as TaskNew); expect(result.author).toEqual({ - avatarUrl: 'https://example.com/creator.png', + avatarUrl: undefined, id: '200', name: 'Creator', }); @@ -405,7 +405,7 @@ describe('elements/content-sidebar/activity-feed-v2/transformers', () => { expect(result.versionNumber).toBe(5); expect(result.actionType).toBe('upload'); expect(result.authorName).toBe('Uploader'); - expect(result.avatarUrl).toBe('https://example.com/uploader.png'); + expect(result.avatarUrl).toBeUndefined(); expect(result.createdAt).toBe(new Date('2024-04-01T00:00:00Z').getTime()); }); @@ -879,4 +879,112 @@ describe('elements/content-sidebar/activity-feed-v2/transformers', () => { }); }); }); + + describe('avatarUrls', () => { + const commentWithoutAvatar = { + created_at: '2024-01-01T00:00:00Z', + created_by: { email: 'u@example.com', id: '42', name: 'No-Avatar User', type: 'user' }, + id: 'comment-x', + message: 'hi', + modified_at: '2024-01-01T00:00:00Z', + permissions: { can_delete: false, can_edit: false, can_reply: false, can_resolve: false }, + tagged_message: 'hi', + type: 'comment', + }; + + test('transformCommentToMessages resolves author avatar from the map', () => { + const [message] = transformCommentToMessages(commentWithoutAvatar as unknown as Comment, { + '42': 'fetched://avatar', + }); + expect(message.author.avatarUrl).toBe('fetched://avatar'); + }); + + test('transformCommentToMessages leaves avatar undefined when id not in map', () => { + const [message] = transformCommentToMessages(commentWithoutAvatar as unknown as Comment, { + '999': 'other://avatar', + }); + expect(message.author.avatarUrl).toBeUndefined(); + }); + + test('transformAnnotationToMessages resolves both root and reply authors from the map', () => { + const annotation = { + created_at: '2024-02-01T00:00:00Z', + created_by: { id: '1', name: 'A', type: 'user' }, + description: { message: 'text' }, + id: 'a-1', + modified_at: '2024-02-01T00:00:00Z', + permissions: { can_delete: true, can_edit: true, can_reply: true, can_resolve: true }, + replies: [ + { + created_at: '2024-02-02T00:00:00Z', + created_by: { id: '2', name: 'B', type: 'user' }, + id: 'r-1', + message: 'reply', + modified_at: '2024-02-02T00:00:00Z', + permissions: { can_delete: false, can_edit: false, can_reply: false, can_resolve: false }, + tagged_message: 'reply', + type: 'comment', + }, + ], + type: 'annotation', + }; + const [root, reply] = transformAnnotationToMessages(annotation as unknown as Annotation, { + '1': 'a://1', + '2': 'a://2', + }); + expect(root.author.avatarUrl).toBe('a://1'); + expect(reply.author.avatarUrl).toBe('a://2'); + }); + + test('transformTaskToProps resolves author and assignee avatars from the map', () => { + const task = { + assigned_to: { + entries: [ + { + id: 'tc-1', + permissions: { can_delete: false, can_update: true }, + role: 'ASSIGNEE', + status: 'NOT_STARTED', + target: { id: '100', name: 'Assignee', type: 'user' }, + type: 'task_collaborator', + }, + ], + next_marker: null, + }, + completion_rule: 'ANY_ASSIGNEE', + created_at: '2024-03-01T00:00:00Z', + created_by: { + id: 'tc-c', + role: 'CREATOR', + status: 'NOT_STARTED', + target: { id: '200', name: 'Creator', type: 'user' }, + type: 'task_collaborator', + }, + id: 'task-x', + permissions: { can_delete: false, can_update: false }, + status: 'NOT_STARTED', + task_type: 'GENERAL', + type: 'task', + }; + const result = transformTaskToProps(task as unknown as TaskNew, undefined, { + '100': 'a://100', + '200': 'a://200', + }); + expect(result.author.avatarUrl).toBe('a://200'); + expect(result.assignees[0].avatarUrl).toBe('a://100'); + }); + + test('transformVersionToProps resolves the actor avatar from the map', () => { + const version = { + action_type: 'created', + created_at: '2024-04-01T00:00:00Z', + id: 'v-1', + modified_by: { id: '300', name: 'Uploader', type: 'user' }, + type: 'file_version', + version_number: '1', + }; + const result = transformVersionToProps(version as unknown as BoxItemVersion, { '300': 'a://300' }); + expect(result.avatarUrl).toBe('a://300'); + }); + }); }); diff --git a/src/elements/content-sidebar/activity-feed-v2/__tests__/useAvatarUrls.test.tsx b/src/elements/content-sidebar/activity-feed-v2/__tests__/useAvatarUrls.test.tsx new file mode 100644 index 0000000000..7db6fa3e18 --- /dev/null +++ b/src/elements/content-sidebar/activity-feed-v2/__tests__/useAvatarUrls.test.tsx @@ -0,0 +1,157 @@ +import { renderHook, waitFor } from '@testing-library/react'; + +import { useAvatarUrls } from '../useAvatarUrls'; + +import { + FEED_ITEM_TYPE_ANNOTATION, + FEED_ITEM_TYPE_COMMENT, + FEED_ITEM_TYPE_TASK, + FEED_ITEM_TYPE_VERSION, +} from '../../../../constants'; + +import type { FeedItem } from '../../../../common/types/feed'; + +const commentItem = (id: string, authorId: string, replyAuthorIds: string[] = []): FeedItem => + ({ + created_at: '2024-01-01T00:00:00Z', + created_by: { id: authorId, name: 'A', type: 'user' }, + id, + replies: replyAuthorIds.map(rid => ({ + created_at: '2024-01-01T00:00:00Z', + created_by: { id: rid, name: 'R', type: 'user' }, + id: `${id}-r-${rid}`, + type: 'comment', + })), + type: FEED_ITEM_TYPE_COMMENT, + }) as unknown as FeedItem; + +const annotationItem = (id: string, authorId: string): FeedItem => + ({ + created_at: '2024-01-01T00:00:00Z', + created_by: { id: authorId, name: 'A', type: 'user' }, + id, + type: FEED_ITEM_TYPE_ANNOTATION, + }) as unknown as FeedItem; + +const taskItem = (id: string, creatorId: string, assigneeIds: string[]): FeedItem => + ({ + assigned_to: { + entries: assigneeIds.map(aid => ({ + id: `${id}-tc-${aid}`, + target: { id: aid, name: 'Assignee', type: 'user' }, + type: 'task_collaborator', + })), + }, + created_by: { target: { id: creatorId, name: 'Creator', type: 'user' } }, + id, + type: FEED_ITEM_TYPE_TASK, + }) as unknown as FeedItem; + +const versionItem = (id: string, modifierId: string): FeedItem => + ({ + id, + modified_by: { id: modifierId, name: 'Modifier', type: 'user' }, + type: FEED_ITEM_TYPE_VERSION, + }) as unknown as FeedItem; + +describe('elements/content-sidebar/activity-feed-v2/useAvatarUrls', () => { + test('returns empty map when getAvatarUrl is not provided', () => { + const { result } = renderHook(() => useAvatarUrls([commentItem('c1', '1')])); + expect(result.current).toEqual({}); + }); + + test('returns empty map when feedItems is empty', () => { + const getAvatarUrl = jest.fn(); + const { result } = renderHook(() => useAvatarUrls([], getAvatarUrl)); + expect(result.current).toEqual({}); + expect(getAvatarUrl).not.toHaveBeenCalled(); + }); + + test('resolves avatar URLs for unique authors across feed item types', async () => { + const getAvatarUrl = jest.fn(async (id: string) => `url-for-${id}`); + const items: FeedItem[] = [ + commentItem('c1', '1', ['2']), + annotationItem('a1', '3'), + taskItem('t1', '4', ['5', '6']), + versionItem('v1', '7'), + ]; + const { result } = renderHook(() => useAvatarUrls(items, getAvatarUrl)); + await waitFor(() => expect(Object.keys(result.current)).toHaveLength(7)); + expect(result.current).toEqual({ + '1': 'url-for-1', + '2': 'url-for-2', + '3': 'url-for-3', + '4': 'url-for-4', + '5': 'url-for-5', + '6': 'url-for-6', + '7': 'url-for-7', + }); + expect(getAvatarUrl).toHaveBeenCalledTimes(7); + }); + + test('deduplicates identical user ids across items', async () => { + const getAvatarUrl = jest.fn(async (id: string) => `url-${id}`); + const items: FeedItem[] = [commentItem('c1', '1', ['1']), commentItem('c2', '1'), taskItem('t1', '1', ['1'])]; + const { result } = renderHook(() => useAvatarUrls(items, getAvatarUrl)); + await waitFor(() => expect(result.current['1']).toBe('url-1')); + expect(getAvatarUrl).toHaveBeenCalledTimes(1); + }); + + test('does not refetch ids already resolved on subsequent renders', async () => { + const getAvatarUrl = jest.fn(async (id: string) => `url-${id}`); + const initialItems: FeedItem[] = [commentItem('c1', '1')]; + const { result, rerender } = renderHook(({ items }) => useAvatarUrls(items, getAvatarUrl), { + initialProps: { items: initialItems }, + }); + await waitFor(() => expect(result.current['1']).toBe('url-1')); + expect(getAvatarUrl).toHaveBeenCalledTimes(1); + + rerender({ items: [commentItem('c1', '1'), commentItem('c2', '2')] }); + await waitFor(() => expect(result.current['2']).toBe('url-2')); + expect(getAvatarUrl).toHaveBeenCalledTimes(2); + expect(getAvatarUrl.mock.calls.filter(([id]) => id === '1')).toHaveLength(1); + }); + + test('omits ids whose fetch fails or returns nullish', async () => { + const getAvatarUrl = jest.fn(async (id: string) => { + if (id === '1') throw new Error('boom'); + if (id === '2') return null; + return `url-${id}`; + }); + const { result } = renderHook(() => useAvatarUrls([commentItem('c', '1', ['2', '3'])], getAvatarUrl)); + await waitFor(() => expect(result.current['3']).toBe('url-3')); + expect(result.current['1']).toBeUndefined(); + expect(result.current['2']).toBeUndefined(); + }); + + test('retries ids whose previous fetch failed', async () => { + let attempts = 0; + const getAvatarUrl = jest.fn(async (id: string) => { + attempts += 1; + if (id === '1' && attempts === 1) throw new Error('transient'); + return `url-${id}`; + }); + const items = [commentItem('c', '1')]; + const { result, rerender } = renderHook(({ extra }) => useAvatarUrls([...items, ...extra], getAvatarUrl), { + initialProps: { extra: [] as FeedItem[] }, + }); + await waitFor(() => expect(getAvatarUrl).toHaveBeenCalledTimes(1)); + expect(result.current['1']).toBeUndefined(); + + rerender({ extra: [commentItem('c2', '2')] }); + await waitFor(() => expect(result.current['1']).toBe('url-1')); + expect(result.current['2']).toBe('url-2'); + }); + + test('returns stable result when feedItems is null or undefined', () => { + const getAvatarUrl = jest.fn(); + const { result, rerender } = renderHook( + ({ items }: { items: FeedItem[] | null | undefined }) => useAvatarUrls(items, getAvatarUrl), + { initialProps: { items: null } }, + ); + expect(result.current).toEqual({}); + rerender({ items: undefined }); + expect(result.current).toEqual({}); + expect(getAvatarUrl).not.toHaveBeenCalled(); + }); +}); diff --git a/src/elements/content-sidebar/activity-feed-v2/transformers.ts b/src/elements/content-sidebar/activity-feed-v2/transformers.ts index 7518ebbd93..7198ad800e 100644 --- a/src/elements/content-sidebar/activity-feed-v2/transformers.ts +++ b/src/elements/content-sidebar/activity-feed-v2/transformers.ts @@ -28,6 +28,7 @@ import type { TaskNew } from '../../../common/types/tasks'; import type { AnnotationBadgeTargetType, AppActivityItemProps, + AvatarUrlMap, TaskItemProps, TransformedFeedItem, VersionItemProps, @@ -132,8 +133,11 @@ const toUpdatedAt = (createdAt: string, modifiedAt: string): number | undefined return toUnixMs(modifiedAt); }; -const toUserAuthor = (user?: User | null): TextMessageAuthorType => ({ - avatarUrl: user?.avatar_url, +const resolveAvatarUrl = (userId: string | undefined, avatarUrls?: AvatarUrlMap): string | undefined => + userId ? avatarUrls?.[userId] : undefined; + +const toUserAuthor = (user?: User | null, avatarUrls?: AvatarUrlMap): TextMessageAuthorType => ({ + avatarUrl: resolveAvatarUrl(user?.id, avatarUrls), email: user?.email ?? user?.login ?? '', id: Number(user?.id) || 0, name: user?.name ?? '', @@ -148,11 +152,15 @@ const toPermissions = ( canResolve: permissions?.can_resolve ?? false, }); -const commentToTextMessage = (comment: Comment, prebuiltCleanText?: string): TextMessageType => { +const commentToTextMessage = ( + comment: Comment, + prebuiltCleanText?: string, + avatarUrls?: AvatarUrlMap, +): TextMessageType => { const cleanText = prebuiltCleanText ?? extractTimestampMarkup(comment.tagged_message || comment.message || '').cleanText; return { - author: toUserAuthor(comment.created_by), + author: toUserAuthor(comment.created_by, avatarUrls), createdAt: toUnixMs(comment.created_at) ?? 0, id: comment.id, message: textToDocumentNode(cleanText, comment.created_by?.id ?? ''), @@ -161,9 +169,9 @@ const commentToTextMessage = (comment: Comment, prebuiltCleanText?: string): Tex }; }; -export const transformCommentToMessages = (comment: Comment): TextMessageType[] => { - const root = commentToTextMessage(comment); - const replies = (comment.replies ?? []).map(reply => commentToTextMessage(reply)); +export const transformCommentToMessages = (comment: Comment, avatarUrls?: AvatarUrlMap): TextMessageType[] => { + const root = commentToTextMessage(comment, undefined, avatarUrls); + const replies = (comment.replies ?? []).map(reply => commentToTextMessage(reply, undefined, avatarUrls)); return [root, ...replies]; }; @@ -193,23 +201,27 @@ export const annotationTargetToBadge = (target?: Target): AnnotationBadgeTargetT } }; -export const transformAnnotationToMessages = (annotation: Annotation): TextMessageType[] => { +export const transformAnnotationToMessages = (annotation: Annotation, avatarUrls?: AvatarUrlMap): TextMessageType[] => { const messageText = annotation.description?.message ?? ''; const root: TextMessageType = { - author: toUserAuthor(annotation.created_by), + author: toUserAuthor(annotation.created_by, avatarUrls), createdAt: toUnixMs(annotation.created_at) ?? 0, id: annotation.id, message: textToDocumentNode(messageText, annotation.created_by?.id ?? ''), permissions: toPermissions(annotation.permissions), updatedAt: toUpdatedAt(annotation.created_at, annotation.modified_at), }; - const replies = (annotation.replies ?? []).map(reply => commentToTextMessage(reply)); + const replies = (annotation.replies ?? []).map(reply => commentToTextMessage(reply, undefined, avatarUrls)); return [root, ...replies]; }; -export const transformTaskToProps = (task: TaskNew, currentUserId?: string): TaskItemProps => ({ +export const transformTaskToProps = ( + task: TaskNew, + currentUserId?: string, + avatarUrls?: AvatarUrlMap, +): TaskItemProps => ({ assignees: (task.assigned_to?.entries ?? []).map(entry => ({ - avatarUrl: entry.target?.avatar_url, + avatarUrl: resolveAvatarUrl(entry.target?.id, avatarUrls), completedAt: toUnixMs(entry.completed_at), id: entry.target?.id ?? entry.id, name: entry.target?.name ?? '', @@ -219,7 +231,7 @@ export const transformTaskToProps = (task: TaskNew, currentUserId?: string): Tas status: entry.status as TaskItemProps['assignees'][number]['status'], })), author: { - avatarUrl: task.created_by?.target?.avatar_url, + avatarUrl: resolveAvatarUrl(task.created_by?.target?.id, avatarUrls), id: task.created_by?.target?.id ?? '', name: task.created_by?.target?.name ?? '', }, @@ -268,15 +280,15 @@ const getVersionAction = (version: BoxItemVersion): VersionItemProps['actionType return mapActionTypeString(version.action_type) ?? 'upload'; }; -const getVersionUser = (version: BoxItemVersion): User | undefined => +export const getVersionUser = (version: BoxItemVersion): User | undefined => version.restored_by || version.trashed_by || version.promoted_by || version.modified_by || undefined; -export const transformVersionToProps = (version: BoxItemVersion): VersionItemProps => { +export const transformVersionToProps = (version: BoxItemVersion, avatarUrls?: AvatarUrlMap): VersionItemProps => { const user = getVersionUser(version); return { actionType: getVersionAction(version), authorName: user?.name ?? version.uploader_display_name, - avatarUrl: user?.avatar_url, + avatarUrl: resolveAvatarUrl(user?.id, avatarUrls), createdAt: toUnixMs(version.created_at), id: version.id, versionNumber: parseInt(version.version_number, 10) || version.version_end || 0, @@ -291,7 +303,11 @@ export const transformAppActivityToProps = (item: BUIEAppActivityItem): AppActiv renderedText: item.rendered_text ?? '', }); -export const transformFeedItem = (item: FeedItem, currentUserId?: string): TransformedFeedItem | null => { +export const transformFeedItem = ( + item: FeedItem, + currentUserId?: string, + avatarUrls?: AvatarUrlMap, +): TransformedFeedItem | null => { switch (item.type) { case FEED_ITEM_TYPE_COMMENT: { const comment = item as unknown as Comment; @@ -303,8 +319,8 @@ export const transformFeedItem = (item: FeedItem, currentUserId?: string): Trans timestampMarkup, timestampMs, } = extractTimestampMarkup(rawText); - const root = commentToTextMessage(comment, cleanText); - const replies = (comment.replies ?? []).map(reply => commentToTextMessage(reply)); + const root = commentToTextMessage(comment, cleanText, avatarUrls); + const replies = (comment.replies ?? []).map(reply => commentToTextMessage(reply, undefined, avatarUrls)); return { annotationTarget, annotationTimestampMarkup: timestampMarkup, @@ -327,7 +343,7 @@ export const transformFeedItem = (item: FeedItem, currentUserId?: string): Trans annotation, id: annotation.id, isResolved: annotationIsResolved, - messages: transformAnnotationToMessages(annotation), + messages: transformAnnotationToMessages(annotation, avatarUrls), permissions: annotation.permissions ?? {}, resolvedAt: annotationIsResolved ? toUnixMs(annotation.resolution?.resolved_at) : undefined, resolvedBy: annotationIsResolved ? annotation.resolution?.resolved_by?.name : undefined, @@ -340,14 +356,14 @@ export const transformFeedItem = (item: FeedItem, currentUserId?: string): Trans return { id: item.id, originalTask: task, - props: transformTaskToProps(task, currentUserId), + props: transformTaskToProps(task, currentUserId, avatarUrls), type: 'task', }; } case FEED_ITEM_TYPE_VERSION: return { id: item.id, - props: transformVersionToProps(item as unknown as BoxItemVersion), + props: transformVersionToProps(item as unknown as BoxItemVersion, avatarUrls), type: 'version', }; case FEED_ITEM_TYPE_APP_ACTIVITY: diff --git a/src/elements/content-sidebar/activity-feed-v2/types.ts b/src/elements/content-sidebar/activity-feed-v2/types.ts index 75aa4757af..e0d0984af6 100644 --- a/src/elements/content-sidebar/activity-feed-v2/types.ts +++ b/src/elements/content-sidebar/activity-feed-v2/types.ts @@ -14,6 +14,11 @@ import type { TaskAssigneeCollection, TaskCollabStatus, TaskNew, TaskUpdatePaylo export type { AppActivityItemProps, TaskItemProps, VersionItemProps } from '@box/activity-feed'; export type { AnnotationBadgeTargetType } from '@box/threaded-annotations'; +// Keyed by Box user id. Populated asynchronously by useAvatarUrls. +export type AvatarUrlMap = Readonly>; + +export type GetAvatarUrl = (userId: string) => Promise; + export type UserContact = { email: string; id: number; @@ -86,7 +91,7 @@ export type ActivityFeedV2Props = { feedItems?: FeedItems; file?: ActivityFeedV2File; getApproverWithQuery?: (searchStr: string) => void; - getAvatarUrl?: (userId: string) => Promise; + getAvatarUrl?: GetAvatarUrl; getMentionAsync?: (searchStr: string) => Promise>>; getTaskCollaborators?: (task: TaskNew) => Promise; hasTasks?: boolean; diff --git a/src/elements/content-sidebar/activity-feed-v2/useAvatarUrls.ts b/src/elements/content-sidebar/activity-feed-v2/useAvatarUrls.ts new file mode 100644 index 0000000000..813a09cbd9 --- /dev/null +++ b/src/elements/content-sidebar/activity-feed-v2/useAvatarUrls.ts @@ -0,0 +1,108 @@ +import * as React from 'react'; + +import type { Annotation } from '../../../common/types/annotations'; +import type { Comment, FeedItem } from '../../../common/types/feed'; +import type { BoxItemVersion } from '../../../common/types/core'; +import type { TaskNew } from '../../../common/types/tasks'; + +import { getVersionUser } from './transformers'; + +import type { AvatarUrlMap, GetAvatarUrl } from './types'; + +import { + FEED_ITEM_TYPE_ANNOTATION, + FEED_ITEM_TYPE_COMMENT, + FEED_ITEM_TYPE_TASK, + FEED_ITEM_TYPE_VERSION, +} from '../../../constants'; + +const collectUserIdsFromComment = (comment: Comment, sink: Set) => { + if (comment.created_by?.id) sink.add(comment.created_by.id); + (comment.replies ?? []).forEach(reply => { + if (reply.created_by?.id) sink.add(reply.created_by.id); + }); +}; + +const collectUserIds = (feedItems?: FeedItem[] | null): string[] => { + const ids = new Set(); + (feedItems ?? []).forEach(item => { + switch (item.type) { + case FEED_ITEM_TYPE_COMMENT: + collectUserIdsFromComment(item as unknown as Comment, ids); + break; + case FEED_ITEM_TYPE_ANNOTATION: { + const annotation = item as unknown as Annotation; + if (annotation.created_by?.id) ids.add(annotation.created_by.id); + (annotation.replies ?? []).forEach(reply => { + if (reply.created_by?.id) ids.add(reply.created_by.id); + }); + break; + } + case FEED_ITEM_TYPE_TASK: { + const task = item as unknown as TaskNew; + const authorId = task.created_by?.target?.id; + if (authorId) ids.add(authorId); + (task.assigned_to?.entries ?? []).forEach(entry => { + if (entry.target?.id) ids.add(entry.target.id); + }); + break; + } + case FEED_ITEM_TYPE_VERSION: { + const actor = getVersionUser(item as unknown as BoxItemVersion); + if (actor?.id) ids.add(actor.id); + break; + } + default: + break; + } + }); + return Array.from(ids); +}; + +export const useAvatarUrls = (feedItems?: FeedItem[] | null, getAvatarUrl?: GetAvatarUrl): AvatarUrlMap => { + const [avatarUrls, setAvatarUrls] = React.useState({}); + const inFlightIdsRef = React.useRef>(new Set()); + const resolvedIdsRef = React.useRef>(new Set()); + + React.useEffect(() => { + if (!getAvatarUrl) return undefined; + const userIds = collectUserIds(feedItems); + const pendingIds = userIds.filter(id => !resolvedIdsRef.current.has(id) && !inFlightIdsRef.current.has(id)); + if (pendingIds.length === 0) return undefined; + + let cancelled = false; + pendingIds.forEach(id => inFlightIdsRef.current.add(id)); + + Promise.all( + pendingIds.map(async id => { + try { + const url = await getAvatarUrl(id); + return [id, url] as const; + } catch { + return [id, null] as const; + } + }), + ).then(entries => { + // Only successful resolutions move to resolvedIdsRef, so transient + // failures stay eligible for retry on the next render. + pendingIds.forEach(id => inFlightIdsRef.current.delete(id)); + if (cancelled) return; + + const resolved: Record = {}; + entries.forEach(([id, url]) => { + if (url) { + resolved[id] = url; + resolvedIdsRef.current.add(id); + } + }); + if (Object.keys(resolved).length === 0) return; + setAvatarUrls(prev => ({ ...prev, ...resolved })); + }); + + return () => { + cancelled = true; + }; + }, [feedItems, getAvatarUrl]); + + return avatarUrls; +}; From 334d3979233f005541c85b7d7d4622042176628a Mon Sep 17 00:00:00 2001 From: Jackie Jou Date: Mon, 22 Jun 2026 17:18:29 -0700 Subject: [PATCH 2/2] fix(activity-feed-v2): release in-flight avatar ids on cleanup --- .../__tests__/useAvatarUrls.test.tsx | 86 +++++++++++++------ .../activity-feed-v2/useAvatarUrls.ts | 37 ++++---- 2 files changed, 78 insertions(+), 45 deletions(-) diff --git a/src/elements/content-sidebar/activity-feed-v2/__tests__/useAvatarUrls.test.tsx b/src/elements/content-sidebar/activity-feed-v2/__tests__/useAvatarUrls.test.tsx index 7db6fa3e18..6e099f7754 100644 --- a/src/elements/content-sidebar/activity-feed-v2/__tests__/useAvatarUrls.test.tsx +++ b/src/elements/content-sidebar/activity-feed-v2/__tests__/useAvatarUrls.test.tsx @@ -1,4 +1,5 @@ import { renderHook, waitFor } from '@testing-library/react'; +import noop from 'lodash/noop'; import { useAvatarUrls } from '../useAvatarUrls'; @@ -11,52 +12,52 @@ import { import type { FeedItem } from '../../../../common/types/feed'; -const commentItem = (id: string, authorId: string, replyAuthorIds: string[] = []): FeedItem => +const commentItem = (itemId: string, authorId: string, replyAuthorIds: string[] = []): FeedItem => ({ created_at: '2024-01-01T00:00:00Z', - created_by: { id: authorId, name: 'A', type: 'user' }, - id, - replies: replyAuthorIds.map(rid => ({ + created_by: { id: authorId, name: 'Author', type: 'user' }, + id: itemId, + replies: replyAuthorIds.map(replyAuthorId => ({ created_at: '2024-01-01T00:00:00Z', - created_by: { id: rid, name: 'R', type: 'user' }, - id: `${id}-r-${rid}`, + created_by: { id: replyAuthorId, name: 'Reply Author', type: 'user' }, + id: `${itemId}-reply-${replyAuthorId}`, type: 'comment', })), type: FEED_ITEM_TYPE_COMMENT, }) as unknown as FeedItem; -const annotationItem = (id: string, authorId: string): FeedItem => +const annotationItem = (itemId: string, authorId: string): FeedItem => ({ created_at: '2024-01-01T00:00:00Z', - created_by: { id: authorId, name: 'A', type: 'user' }, - id, + created_by: { id: authorId, name: 'Author', type: 'user' }, + id: itemId, type: FEED_ITEM_TYPE_ANNOTATION, }) as unknown as FeedItem; -const taskItem = (id: string, creatorId: string, assigneeIds: string[]): FeedItem => +const taskItem = (itemId: string, creatorId: string, assigneeIds: string[]): FeedItem => ({ assigned_to: { - entries: assigneeIds.map(aid => ({ - id: `${id}-tc-${aid}`, - target: { id: aid, name: 'Assignee', type: 'user' }, + entries: assigneeIds.map(assigneeId => ({ + id: `${itemId}-collaborator-${assigneeId}`, + target: { id: assigneeId, name: 'Assignee', type: 'user' }, type: 'task_collaborator', })), }, created_by: { target: { id: creatorId, name: 'Creator', type: 'user' } }, - id, + id: itemId, type: FEED_ITEM_TYPE_TASK, }) as unknown as FeedItem; -const versionItem = (id: string, modifierId: string): FeedItem => +const versionItem = (itemId: string, modifierId: string): FeedItem => ({ - id, + id: itemId, modified_by: { id: modifierId, name: 'Modifier', type: 'user' }, type: FEED_ITEM_TYPE_VERSION, }) as unknown as FeedItem; describe('elements/content-sidebar/activity-feed-v2/useAvatarUrls', () => { test('returns empty map when getAvatarUrl is not provided', () => { - const { result } = renderHook(() => useAvatarUrls([commentItem('c1', '1')])); + const { result } = renderHook(() => useAvatarUrls([commentItem('comment-1', '1')])); expect(result.current).toEqual({}); }); @@ -70,10 +71,10 @@ describe('elements/content-sidebar/activity-feed-v2/useAvatarUrls', () => { test('resolves avatar URLs for unique authors across feed item types', async () => { const getAvatarUrl = jest.fn(async (id: string) => `url-for-${id}`); const items: FeedItem[] = [ - commentItem('c1', '1', ['2']), - annotationItem('a1', '3'), - taskItem('t1', '4', ['5', '6']), - versionItem('v1', '7'), + commentItem('comment-1', '1', ['2']), + annotationItem('annotation-1', '3'), + taskItem('task-1', '4', ['5', '6']), + versionItem('version-1', '7'), ]; const { result } = renderHook(() => useAvatarUrls(items, getAvatarUrl)); await waitFor(() => expect(Object.keys(result.current)).toHaveLength(7)); @@ -91,7 +92,11 @@ describe('elements/content-sidebar/activity-feed-v2/useAvatarUrls', () => { test('deduplicates identical user ids across items', async () => { const getAvatarUrl = jest.fn(async (id: string) => `url-${id}`); - const items: FeedItem[] = [commentItem('c1', '1', ['1']), commentItem('c2', '1'), taskItem('t1', '1', ['1'])]; + const items: FeedItem[] = [ + commentItem('comment-1', '1', ['1']), + commentItem('comment-2', '1'), + taskItem('task-1', '1', ['1']), + ]; const { result } = renderHook(() => useAvatarUrls(items, getAvatarUrl)); await waitFor(() => expect(result.current['1']).toBe('url-1')); expect(getAvatarUrl).toHaveBeenCalledTimes(1); @@ -99,17 +104,17 @@ describe('elements/content-sidebar/activity-feed-v2/useAvatarUrls', () => { test('does not refetch ids already resolved on subsequent renders', async () => { const getAvatarUrl = jest.fn(async (id: string) => `url-${id}`); - const initialItems: FeedItem[] = [commentItem('c1', '1')]; + const initialItems: FeedItem[] = [commentItem('comment-1', '1')]; const { result, rerender } = renderHook(({ items }) => useAvatarUrls(items, getAvatarUrl), { initialProps: { items: initialItems }, }); await waitFor(() => expect(result.current['1']).toBe('url-1')); expect(getAvatarUrl).toHaveBeenCalledTimes(1); - rerender({ items: [commentItem('c1', '1'), commentItem('c2', '2')] }); + rerender({ items: [commentItem('comment-1', '1'), commentItem('comment-2', '2')] }); await waitFor(() => expect(result.current['2']).toBe('url-2')); expect(getAvatarUrl).toHaveBeenCalledTimes(2); - expect(getAvatarUrl.mock.calls.filter(([id]) => id === '1')).toHaveLength(1); + expect(getAvatarUrl.mock.calls.filter(([userId]) => userId === '1')).toHaveLength(1); }); test('omits ids whose fetch fails or returns nullish', async () => { @@ -118,7 +123,7 @@ describe('elements/content-sidebar/activity-feed-v2/useAvatarUrls', () => { if (id === '2') return null; return `url-${id}`; }); - const { result } = renderHook(() => useAvatarUrls([commentItem('c', '1', ['2', '3'])], getAvatarUrl)); + const { result } = renderHook(() => useAvatarUrls([commentItem('comment-1', '1', ['2', '3'])], getAvatarUrl)); await waitFor(() => expect(result.current['3']).toBe('url-3')); expect(result.current['1']).toBeUndefined(); expect(result.current['2']).toBeUndefined(); @@ -131,18 +136,43 @@ describe('elements/content-sidebar/activity-feed-v2/useAvatarUrls', () => { if (id === '1' && attempts === 1) throw new Error('transient'); return `url-${id}`; }); - const items = [commentItem('c', '1')]; + const items = [commentItem('comment-1', '1')]; const { result, rerender } = renderHook(({ extra }) => useAvatarUrls([...items, ...extra], getAvatarUrl), { initialProps: { extra: [] as FeedItem[] }, }); await waitFor(() => expect(getAvatarUrl).toHaveBeenCalledTimes(1)); expect(result.current['1']).toBeUndefined(); - rerender({ extra: [commentItem('c2', '2')] }); + rerender({ extra: [commentItem('comment-2', '2')] }); await waitFor(() => expect(result.current['1']).toBe('url-1')); expect(result.current['2']).toBe('url-2'); }); + test('refetches in-flight ids after deps change mid-fetch', async () => { + let resolveFirstCall: (value: string) => void = noop; + const firstCallPromise = new Promise(resolve => { + resolveFirstCall = resolve; + }); + const getAvatarUrl = jest.fn(async (userId: string) => { + if (userId === '1' && getAvatarUrl.mock.calls.length === 1) return firstCallPromise; + return `url-${userId}`; + }); + const initialItems: FeedItem[] = [commentItem('comment-1', '1')]; + const { result, rerender } = renderHook(({ items }) => useAvatarUrls(items, getAvatarUrl), { + initialProps: { items: initialItems }, + }); + await waitFor(() => expect(getAvatarUrl).toHaveBeenCalledTimes(1)); + + // Dependency change while id '1' is still in flight cancels the original + // promise; the next effect run should reissue the fetch for '1'. + rerender({ items: [commentItem('comment-1', '1'), commentItem('comment-2', '2')] }); + await waitFor(() => expect(result.current['2']).toBe('url-2')); + + resolveFirstCall('stale'); + await waitFor(() => expect(getAvatarUrl.mock.calls.filter(([userId]) => userId === '1')).toHaveLength(2)); + expect(result.current['1']).toBe('url-1'); + }); + test('returns stable result when feedItems is null or undefined', () => { const getAvatarUrl = jest.fn(); const { result, rerender } = renderHook( diff --git a/src/elements/content-sidebar/activity-feed-v2/useAvatarUrls.ts b/src/elements/content-sidebar/activity-feed-v2/useAvatarUrls.ts index 813a09cbd9..f38d316ffc 100644 --- a/src/elements/content-sidebar/activity-feed-v2/useAvatarUrls.ts +++ b/src/elements/content-sidebar/activity-feed-v2/useAvatarUrls.ts @@ -16,47 +16,47 @@ import { FEED_ITEM_TYPE_VERSION, } from '../../../constants'; -const collectUserIdsFromComment = (comment: Comment, sink: Set) => { - if (comment.created_by?.id) sink.add(comment.created_by.id); +const collectUserIdsFromComment = (comment: Comment, userIds: Set) => { + if (comment.created_by?.id) userIds.add(comment.created_by.id); (comment.replies ?? []).forEach(reply => { - if (reply.created_by?.id) sink.add(reply.created_by.id); + if (reply.created_by?.id) userIds.add(reply.created_by.id); }); }; const collectUserIds = (feedItems?: FeedItem[] | null): string[] => { - const ids = new Set(); + const userIds = new Set(); (feedItems ?? []).forEach(item => { switch (item.type) { case FEED_ITEM_TYPE_COMMENT: - collectUserIdsFromComment(item as unknown as Comment, ids); + collectUserIdsFromComment(item as unknown as Comment, userIds); break; case FEED_ITEM_TYPE_ANNOTATION: { const annotation = item as unknown as Annotation; - if (annotation.created_by?.id) ids.add(annotation.created_by.id); + if (annotation.created_by?.id) userIds.add(annotation.created_by.id); (annotation.replies ?? []).forEach(reply => { - if (reply.created_by?.id) ids.add(reply.created_by.id); + if (reply.created_by?.id) userIds.add(reply.created_by.id); }); break; } case FEED_ITEM_TYPE_TASK: { const task = item as unknown as TaskNew; const authorId = task.created_by?.target?.id; - if (authorId) ids.add(authorId); + if (authorId) userIds.add(authorId); (task.assigned_to?.entries ?? []).forEach(entry => { - if (entry.target?.id) ids.add(entry.target.id); + if (entry.target?.id) userIds.add(entry.target.id); }); break; } case FEED_ITEM_TYPE_VERSION: { const actor = getVersionUser(item as unknown as BoxItemVersion); - if (actor?.id) ids.add(actor.id); + if (actor?.id) userIds.add(actor.id); break; } default: break; } }); - return Array.from(ids); + return Array.from(userIds); }; export const useAvatarUrls = (feedItems?: FeedItem[] | null, getAvatarUrl?: GetAvatarUrl): AvatarUrlMap => { @@ -66,12 +66,14 @@ export const useAvatarUrls = (feedItems?: FeedItem[] | null, getAvatarUrl?: GetA React.useEffect(() => { if (!getAvatarUrl) return undefined; + const inFlightIds = inFlightIdsRef.current; + const resolvedIds = resolvedIdsRef.current; const userIds = collectUserIds(feedItems); - const pendingIds = userIds.filter(id => !resolvedIdsRef.current.has(id) && !inFlightIdsRef.current.has(id)); + const pendingIds = userIds.filter(id => !resolvedIds.has(id) && !inFlightIds.has(id)); if (pendingIds.length === 0) return undefined; let cancelled = false; - pendingIds.forEach(id => inFlightIdsRef.current.add(id)); + pendingIds.forEach(id => inFlightIds.add(id)); Promise.all( pendingIds.map(async id => { @@ -83,16 +85,16 @@ export const useAvatarUrls = (feedItems?: FeedItem[] | null, getAvatarUrl?: GetA } }), ).then(entries => { - // Only successful resolutions move to resolvedIdsRef, so transient - // failures stay eligible for retry on the next render. - pendingIds.forEach(id => inFlightIdsRef.current.delete(id)); if (cancelled) return; + // Only successful resolutions move to resolvedIds, so transient + // failures stay eligible for retry on the next render. const resolved: Record = {}; entries.forEach(([id, url]) => { + inFlightIds.delete(id); if (url) { resolved[id] = url; - resolvedIdsRef.current.add(id); + resolvedIds.add(id); } }); if (Object.keys(resolved).length === 0) return; @@ -101,6 +103,7 @@ export const useAvatarUrls = (feedItems?: FeedItem[] | null, getAvatarUrl?: GetA return () => { cancelled = true; + pendingIds.forEach(id => inFlightIds.delete(id)); }; }, [feedItems, getAvatarUrl]);