feat(activity-feed-v2): Wire edit, copy link, scroll to end#4557
Open
jackiejou wants to merge 7 commits into
Open
feat(activity-feed-v2): Wire edit, copy link, scroll to end#4557jackiejou wants to merge 7 commits into
jackiejou wants to merge 7 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds optional copy-link, edit, and reply-update callbacks to ActivitySidebar → ActivityFeedV2 → FeedItemRow, centralizes editor serialization with error handling, implements two-effect deep-link vs end-scroll behavior in ActivityFeedV2, expands tests for scroll and edit flows, and bumps several ChangesActivity Feed Callbacks, Serialization & Scroll
sequenceDiagram
participant ActivitySidebar
participant ActivityFeedV2
participant FeedItemRow
rect rgba(100,149,237,0.5)
ActivitySidebar->>ActivityFeedV2: pass callbacks (copy/edit/reply handlers)
end
rect rgba(34,139,34,0.5)
ActivityFeedV2->>FeedItemRow: forward callbacks and manage scroll effects
FeedItemRow->>FeedItemRow: serializeEditorContent -> serializeMentionMarkup
FeedItemRow->>ActivityFeedV2: call onReplyUpdate / onAnnotationEdit / onCommentUpdate
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jackiejou
force-pushed
the
chore/bump-activity-feed-threaded-annotations
branch
from
6e77e5c to
f0c4d28
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx (1)
195-205: 💤 Low valueConsider optimizing effect dependencies.
The scroll-to-end effect includes
activeFeedEntryIdin its dependencies but immediately returns early when it's truthy (line 197). This causes the effect to re-run unnecessarily wheneveractiveFeedEntryIdchanges between truthy values.♻️ Optional optimization to reduce effect re-runs
Memoize the boolean to avoid effect re-runs when activeFeedEntryId changes between truthy values:
+ const hasDeepLink = React.useMemo(() => Boolean(activeFeedEntryId), [activeFeedEntryId]); + React.useEffect(() => { - const hasDeepLink = Boolean(activeFeedEntryId); if (hasScrolledToEndRef.current || hasDeepLink || !scrollHandle || filteredItems.length === 0) { return; } const lastItemId = filteredItems[filteredItems.length - 1].id; const didScroll = scrollHandle.scrollTo(lastItemId); if (didScroll) { hasScrolledToEndRef.current = true; } - }, [activeFeedEntryId, filteredItems, scrollHandle]); + }, [hasDeepLink, filteredItems, scrollHandle]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx` around lines 195 - 205, The useEffect that scrolls to the end is re-run whenever activeFeedEntryId changes even when it remains truthy; change the dependency to a stable boolean (e.g., hasDeepLink) by memoizing Boolean(activeFeedEntryId) with useMemo or compute it outside the dependency array so the effect only depends on hasDeepLink, filteredItems, and scrollHandle; update the effect signature and dependency list that references hasScrolledToEndRef, activeFeedEntryId (replace with memoized hasDeepLink), filteredItems, scrollHandle, and keep the same early-return logic and lastItemId/scrollHandle.scrollTo flow.src/elements/content-sidebar/activity-feed-v2/types.ts (1)
95-102: ⚡ Quick winConsider using an object parameter for consistency.
The
onReplyUpdatecallback uses six positional parameters, while other callbacks in this interface (onAnnotationCopyLink,onAnnotationEdit,onCommentCopyLink) use a params object. Positional parameters are harder to call correctly and less self-documenting.♻️ Refactor to object-based params for consistency
- onReplyUpdate?: ( - id: string, - parentId: string, - text: string, - permissions: BoxCommentPermission, - onSuccess?: (() => void) | null, - onError?: (() => void) | null, - ) => void; + onReplyUpdate?: (params: { + id: string; + parentId: string; + text: string; + permissions: BoxCommentPermission; + onSuccess?: (() => void) | null; + onError?: (() => void) | null; + }) => void;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/elements/content-sidebar/activity-feed-v2/types.ts` around lines 95 - 102, The onReplyUpdate callback currently takes six positional args (id, parentId, text, permissions: BoxCommentPermission, onSuccess, onError) which is inconsistent with other handlers that accept a params object; change onReplyUpdate to accept a single params object (e.g., { id: string; parentId: string; text: string; permissions: BoxCommentPermission; onSuccess?: () => void; onError?: () => void }) and update all call sites to pass an object instead of positional args so it matches patterns used by onAnnotationCopyLink/onAnnotationEdit/onCommentCopyLink and improves readability and safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx`:
- Around line 195-205: The useEffect that scrolls to the end is re-run whenever
activeFeedEntryId changes even when it remains truthy; change the dependency to
a stable boolean (e.g., hasDeepLink) by memoizing Boolean(activeFeedEntryId)
with useMemo or compute it outside the dependency array so the effect only
depends on hasDeepLink, filteredItems, and scrollHandle; update the effect
signature and dependency list that references hasScrolledToEndRef,
activeFeedEntryId (replace with memoized hasDeepLink), filteredItems,
scrollHandle, and keep the same early-return logic and
lastItemId/scrollHandle.scrollTo flow.
In `@src/elements/content-sidebar/activity-feed-v2/types.ts`:
- Around line 95-102: The onReplyUpdate callback currently takes six positional
args (id, parentId, text, permissions: BoxCommentPermission, onSuccess, onError)
which is inconsistent with other handlers that accept a params object; change
onReplyUpdate to accept a single params object (e.g., { id: string; parentId:
string; text: string; permissions: BoxCommentPermission; onSuccess?: () => void;
onError?: () => void }) and update all call sites to pass an object instead of
positional args so it matches patterns used by
onAnnotationCopyLink/onAnnotationEdit/onCommentCopyLink and improves readability
and safety.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a17a2f57-d556-4324-a604-229c068aba4e
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
package.jsonsrc/elements/content-sidebar/ActivitySidebar.jssrc/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsxsrc/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsxsrc/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsxsrc/elements/content-sidebar/activity-feed-v2/__tests__/FeedItemRow.test.tsxsrc/elements/content-sidebar/activity-feed-v2/types.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx`:
- Around line 163-176: The handlers call onCommentUpdate, onReplyUpdate and
onAnnotationEdit with serialized.text even when it is only whitespace; after
const serialized = serializeEditorContent(content) add a guard that returns
early if serialized?.text.trim().length === 0 to block whitespace-only edits,
and apply the same check in the other edit handler(s) that use
onAnnotationEdit/onReplyUpdate (the other block that computes replyPermissions
via findMessagePermissions and calls onReplyUpdate). Ensure you perform the trim
check before any permission lookups or callback invocation so no whitespace-only
text is dispatched.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc4771fe-89c3-4642-8ea1-3bb685c9a574
📒 Files selected for processing (4)
src/elements/content-sidebar/ActivitySidebar.jssrc/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsxsrc/elements/content-sidebar/activity-feed-v2/__tests__/FeedItemRow.test.tsxsrc/elements/content-sidebar/activity-feed-v2/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/content-sidebar/ActivitySidebar.js
- src/elements/content-sidebar/activity-feed-v2/tests/FeedItemRow.test.tsx
- Bump @box/activity-feed to 1.19.4 and @box/threaded-annotations to 1.89.1
with matching blueprint, collaboration-popover, readable-time, and
user-selector peer versions
- Route the new ThreadedAnnotation onEdit to onCommentUpdate,
onAnnotationEdit, or onReplyUpdate based on message id, and pass
isEditDisabled when the thread is disabled or resolved
- Add onCommentCopyLink and onAnnotationCopyLink pass-throughs with
{ id, rootId } so hosts can build permalinks
- Pass page number alongside highlightedText for highlight annotation
badges so they can fall back to the page
- Scroll the activity tab to the deep-linked entry on reload, or to the
last visible row when no deep link is active; both retry until the
target row is mounted and run once thereafter
- Log via console.error and onEditError for malformed editor content and
missing reply permissions, matching the repo errorCallback pattern
Align onReplyUpdate with the other new callbacks (onAnnotationCopyLink, onAnnotationEdit, onCommentCopyLink) which take a single params object. ActivitySidebar adapts the object back to its legacy positional updateReply signature at the prop-passing site.
Matches the existing guard in buildReplyPost so comment, annotation, and reply edits with only whitespace (or empty content) no longer fire onCommentUpdate, onAnnotationEdit, or onReplyUpdate.
jackiejou
force-pushed
the
chore/bump-activity-feed-threaded-annotations
branch
from
bdb70c6 to
5ae7b51
Compare
Annotation deep links target the thread root regardless of which message was clicked, so the field is renamed from id to annotationId to make the root-only contract self-documenting alongside the per-message id used by onCommentCopyLink.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx`:
- Around line 236-244: The onCopyLink closure in FeedItemRow uses
item.annotation.file_version?.id as a guard but then accesses
item.annotation.file_version.id at invocation, which can race if file_version
becomes null; to fix, capture the fileVersionId at render time (e.g., const
fileVersionId = item.annotation.file_version?.id) and use that captured
fileVersionId and item.id when building the onAnnotationCopyLink call inside the
onCopyLink prop, returning undefined if fileVersionId is falsy so the closure
never dereferences a potentially null file_version.
- Around line 161-176: The two handleEdit handlers (the comment one starting
with "const handleEdit = (id: string, content: unknown)" and the annotation
variant later) are dropping the onSuccess/onError callbacks passed by the
activity-feed library; update both handlers to accept the full signature (id,
content/text, status?, hasMention?, permissions?, onSuccess?, onError?) or at
minimum capture the final two callback parameters and forward them when invoking
onCommentUpdate and onReplyUpdate (and in the annotation handler) so calls like
onCommentUpdate?.(id, serialized.text, undefined, serialized.hasMention,
permissions, onSuccess, onError) and onReplyUpdate?.({ id, parentId: item.id,
permissions: replyPermissions, text: serialized.text }, onSuccess, onError) (or
the equivalent positional forwarding) are made; keep the existing validation
(serializeEditorContent, findMessagePermissions, console.error) intact and only
add the extra parameters to the forwarded calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32e79de8-7c40-4045-aa90-b55e70c4b883
📒 Files selected for processing (4)
src/elements/content-sidebar/ActivitySidebar.jssrc/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsxsrc/elements/content-sidebar/activity-feed-v2/__tests__/FeedItemRow.test.tsxsrc/elements/content-sidebar/activity-feed-v2/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sidebar/activity-feed-v2/tests/FeedItemRow.test.tsx
- Move serializeEditorContent, findMessagePermissions, logEditError, and the new dispatchReplyEdit (object-param) into helpers.ts; FeedItemRow now imports them and tests mock them via the standard jest.mocked pattern - Move annotationTargetToBadge into transformers.ts where the other shape converters live, and exhaust the Target discriminant directly instead of casting through string - Extend the Target type with TargetDrawing and TargetHighlight to match the runtime shapes that box-annotations emits, and pass through the highlight target text so the badge can display the excerpt - Promote OnReplyUpdate to a named export so types.ts, FeedItemRow, and helpers all reference one definition - Narrow logEditError return type to undefined and reuse vendor's ReturnType for serializeEditorContent so the shapes track upstream - Split tests across helpers.test.ts (helper internals), transformers test (badge mapping), and FeedItemRow tests (row-level wiring)
Expose the show-resolved and show-only-mentions-of-me filters as controlled-or-uncontrolled props on ActivityFeedV2, with ActivitySidebar plumbing the same props through to the row. The change handlers update local state only when the matching value prop is omitted, and always notify the consumer when provided. This keeps the controlled mode authoritative and makes the listener-only combination work as a notification channel.
The UAA file activities endpoint only returns reply data when the request sets shouldShowReplies. Previously this gated on the v1 hasReplies prop alone, which meant ActivityFeedV2 received empty reply lists when the host opted into v2 without flipping the legacy hasReplies. Send shouldShowReplies when either hasReplies or threadedRepliesV2 is enabled. The legacy fetchRepliesForFeedItems expansion path still gates on hasReplies because v2 displays replies inline.
| scrollHandle.scrollTo(activeFeedEntryId); | ||
| } | ||
| }, [activeFeedEntryId, scrollHandle]); | ||
| const scrolledEntryIdRef = React.useRef<string | null>(null); |
Contributor
There was a problem hiding this comment.
nit: can we make this an empty string by default so we don't have mixed types?
| return serializeMentionMarkup(content as EditorContent); | ||
| } catch (error) { | ||
| // eslint-disable-next-line no-console | ||
| console.error('ActivityFeedV2: failed to serialize editor content', error); |
Contributor
There was a problem hiding this comment.
do we want to surface this error in the console?
Contributor
Author
There was a problem hiding this comment.
we have other console.errors like in ActivitySidebar so I kept the same behavior.
ahorowitz123
approved these changes
May 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
New / changed callbacks
onCommentCopyLink: (params: { id }) => void— id is the clicked message (root or reply); URL shape is?sb=/activity/comments/{id}onAnnotationCopyLink: (params: { annotationId, fileVersionId }) => void— annotation deep links always target the thread root, so the clicked-id is dropped; URL shape is?sb=/activity/annotations/{fileVersionId}/{annotationId}. Hidden when the annotation has nofile_versiononAnnotationEdit: (params: { id, permissions, text }) => void— fires for root annotation editsonReplyUpdate: (params: { id, parentId, permissions, text, onSuccess?, onError? }) => void— fires for reply edits under both comments and annotationsNew filter props
showResolved?: boolean+onShowResolvedChange?: (checked: boolean) => voidshowOnlyMentionsMe?: boolean+onShowOnlyMentionsMeChange?: (checked: boolean) => voidProvide both for controlled mode, neither for uncontrolled, or just the change handler to observe internal state changes.
Other behavior
onEditErroris wired to log viaconsole.errorand fall through to the vendor's default editor error messagehelpers.ts, with focused unit tests;Targettype extended withTargetDrawingandTargetHighlightto match the runtime shapesTest plan
onCommentCopyLink/onAnnotationCopyLinkfrom the host; verify Copy link appears in the menu and fires with the new payload shapes; omit the handlers and verify the menu item is hidden/file/<id>?sb=/activity/annotations/<versionId>/<entryId>and reload; verify the feed scrolls to that entry once items loadtextshows the excerpt in the badge, and a highlight withouttextfalls back to the page numbershowResolved+onShowResolvedChangefrom the host (controlled); verify the toggle reflects the host value and notifies the host on each click