fix(video-annotations): Add version links for video annotations #4379
Conversation
WalkthroughThe rendering condition for the AnnotationActivityLink in video annotations was refined. The link now displays when hasVersions is true and either the annotation is not a video type or it is a video annotation that is not the current version. Corresponding test cases were renamed and expanded to validate this behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
bfoxx1906
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
167-176: Version-link visibility logic correctly extended for video annotationsThe new condition matches the intended behavior: non-video annotations still show the link whenever
hasVersionsis true, and video annotations now show it only whenhasVersionsis true and the annotation is not the current version. This is behaviorally consistent with the existinglinkMessage/linkValuelogic.If you want to slightly simplify the expression, you could use the equivalent form that directly encodes the “all versions except current video” rule:
- <ActivityTimestamp date={createdAtTimestamp} /> - {hasVersions && ((isVideoAnnotation && !isCurrentVersion) || !isVideoAnnotation) && ( + <ActivityTimestamp date={createdAtTimestamp} /> + {hasVersions && !(isVideoAnnotation && isCurrentVersion) && ( <AnnotationActivityLink className="bcs-AnnotationActivity-link" data-resin-target="annotationLink" id={id} isDisabled={isFileVersionUnavailable} message={activityLinkMessage} onClick={handleSelect} /> )}src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (1)
389-397: Video-annotation tests accurately cover current vs non-current version behaviorThese two tests nicely pin down the new behavior for video annotations:
isCurrentVersion: true+hasVersions: true→ noAnnotationActivityLink.isCurrentVersion: false+hasVersions: true→AnnotationActivityLinkpresent with the expectedannotationActivityVersionLinkmessage and version'2'.That gives good confidence the new conditional in
AnnotationActivitywon’t regress.Tiny optional nit: the first test name reads “Link link”; you could clean that up if you touch this again:
-test('should not show Annotation Activity Link link for video annotations when is current version and hasVersions is true', () => { +test('should not show Annotation Activity Link for video annotations when is current version and hasVersions is true', () => {Also applies to: 399-412
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js(1 hunks)src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T17:50:47.187Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4228
File: src/elements/content-sidebar/activity-feed/comment/Comment.js:147-148
Timestamp: 2025-08-19T17:50:47.187Z
Learning: In the box-ui-elements codebase, ActivityThreadReplyForm.js and AnnotationThreadCreate.js component files exist but are not actively used/imported anywhere, so they don't need updates for prop propagation.
Applied to files:
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js
🧬 Code graph analysis (1)
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (1)
src/elements/content-sidebar/activity-feed/annotations/messages.js (1)
messages(3-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (1)
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (1)
478-481: Additional assertion for non-video link message guards against regressionsThe new expectation that
AnnotationActivityLinkusesannotationActivityPageItemwith{ number: 1 }for non-video annotations ensures the page-style text isn’t accidentally changed by the video/version logic. This complements the other tests well and keeps non-video behavior explicitly covered.
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js
Show resolved
Hide resolved
Merge Queue Status
✅ The pull request has been merged This pull request spent 10 seconds in the queue, with no time running CI. Required conditions to merge
|
3 participants
Video Annotations do not currently include the version number link like annotations do for other file types. This PR updates the logic to include the version link if there are versions and the annotation is a video annotations that does not belong to the current version. The logic change can be seen here. Unit tests were also updated to verify the conditional and also update one of the existing tests to make it more comprehensive(Pr Ref)
Video Annotations Before:
Video Annotations After:
Current Non Video Annotations Functionality Unchanged:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.