Skip to content

feat(activity-feed-v2): display audio comment time ranges in the composer and badge - #4820

Merged
mergify[bot] merged 3 commits into
box:masterfrom
kduncanhsu:kduncanhsu/comment-range-display
Sep 8, 2026
Merged

feat(activity-feed-v2): display audio comment time ranges in the composer and badge#4820
mergify[bot] merged 3 commits into
box:masterfrom
kduncanhsu:kduncanhsu/comment-range-display

Conversation

@kduncanhsu

@kduncanhsu kduncanhsu commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Renders a selected time range as a readable label in the two places a user sees it: the timestamp checkbox while composing, and the badge on a posted comment. This is presentation only — it consumes the end timestamp that earlier changes already parse from comment markup and hold in composer state, and turns milliseconds into a string. No persisted data changes.

Comments that carry only a single timestamp render exactly as they do today, on both surfaces.

Changes

  • useTimeFormat.ts — adds formatTimeRange, which formats each bound through the existing formatByTimeFormat and joins them with a separator.
  • FeedItemRow.tsx — the Frame badge target is formatted as a range when the comment has an end timestamp. Clicking the badge still seeks to the start.
  • useMediaTimestamp.ts — the composer label becomes a range once a range has been selected, and stays a single time until then.

Notes on the approach

One formatter, two surfaces. Both consumers take an opaque display string — the composer's timestamp toggle takes formattedTimestamp: string and the badge's Frame variant takes a timestamp string — so a single shared formatter serves both and keeps the separator character decided in one place. This also means no change is needed in the shared annotations package.

Both bounds respect the active time format. Ranges are formatted per-bound rather than by formatting the start and appending a raw end, so standard, timecode, and frames all work, including the fps-dependent frame math.

The composer label is derived in the hook, not at the call site. The hook already owns the end timestamp and produces formattedTimestamp, so the range label falls out of the existing wiring without touching ActivityFeedV2.

Accessibility is unchanged. The badge's accessible name comes from its visible label, so a range announces with the start time first. The checkbox keeps its own static aria-label.

Test plan

  • Unit tests cover the range formatter directly across all three time formats, and assert each bound matches what the single-value formatter produces.
  • Badge tests cover range rendering, per-format rendering, and that a click still seeks to the start.
  • Composer tests cover the collapsed label, the range label after a selection commits, per-format rendering, and the return to a single time after a reset.
  • Full activity-feed-v2 suite, lint, and typecheck pass.

Summary by CodeRabbit

  • New Features

    • Comment annotations and media timestamps now display start–end ranges when available.
    • Time ranges respect the selected standard, timecode, or frame-based format.
    • Selecting a range annotation seeks to its starting timestamp.
  • Tests

    • Added coverage for range formatting, display, selection behavior, and resetting to single timestamps.

@kduncanhsu
kduncanhsu requested review from a team as code owners September 4, 2026 23:53
@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: e0a1d652-9caf-4c9f-9176-071a851d3ae6

📥 Commits

Reviewing files that changed from the base of the PR and between 3a6ba6a and 51c5fe4.

📒 Files selected for processing (6)
  • src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx
  • src/elements/content-sidebar/activity-feed-v2/__tests__/FeedItemRow.test.tsx
  • src/elements/content-sidebar/activity-feed-v2/__tests__/useMediaTimestamp.test.tsx
  • src/elements/content-sidebar/activity-feed-v2/__tests__/useTimeFormat.test.tsx
  • src/elements/content-sidebar/activity-feed-v2/useMediaTimestamp.ts
  • src/elements/content-sidebar/activity-feed-v2/useTimeFormat.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


Walkthrough

The change adds shared time-range formatting. Media timestamp labels and comment annotation badges now display start and end timestamps when available, while preserving single-timestamp behavior and seeking to the range start.

Changes

Timestamp Range Support

Layer / File(s) Summary
Range formatter and media timestamp handling
src/elements/content-sidebar/activity-feed-v2/useTimeFormat.ts, src/elements/content-sidebar/activity-feed-v2/useMediaTimestamp.ts, src/elements/content-sidebar/activity-feed-v2/__tests__/*
formatTimeRange formats both bounds with the active time format and an en dash. useMediaTimestamp uses range formatting when an end timestamp exists. Tests cover standard, timecode, frames, collapsed ranges, and reset behavior.
Annotation range badge rendering
src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx, src/elements/content-sidebar/activity-feed-v2/__tests__/FeedItemRow.test.tsx
Comment annotation badges render a range when annotationTimestampEndMs is present. Clicking a range badge seeks to its start timestamp.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to c2100

Audio comment ranges now display both selected timestamps in the active format, while existing single timestamps and badge seeking behavior remain unchanged. No concrete current-head merge risk remains.

Suggested reviewers: jackiejou

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 6 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: displaying audio comment time ranges in the composer and posted-comment badge.
Description check ✅ Passed The description is complete and directly related to the change. It explains the behavior, implementation, scope, accessibility impact, and test plan. The repository template contains only merge-proces…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

A rabbit formats bounds in a row
With an en dash to make ranges show
Badges point where the clips begin
Timecodes keep their shape within
Tests hop along, and all timestamps glow

Comment @coderabbitai help to get the list of available commands.

@kduncanhsu
kduncanhsu force-pushed the kduncanhsu/comment-range-display branch from 51c5fe4 to d2ba1aa Compare September 4, 2026 23:59
@kduncanhsu
kduncanhsu marked this pull request as draft September 4, 2026 23:59
@kduncanhsu
kduncanhsu marked this pull request as ready for review September 5, 2026 00:07
kduncanhsu and others added 2 commits September 8, 2026 11:47
Format a comment that carries an end timestamp as a range on its badge,
using a shared formatter so both bounds respect the active time format.
Clicking the badge still seeks to the start of the range.

The composer half of the range display is deferred: the composer cannot
hold a range until the waveform handles are wired up, so that label
change lands with the composer state it depends on.

Co-authored-by: Cursor <cursoragent@cursor.com>
…eckbox

Label the timestamp checkbox with both bounds once a range has been
selected, falling back to the single captured time until then. The hook
already owns the end timestamp and derives the formatted label, so this
reuses the same range formatter the posted badge uses and needs no
change at the call site.

Co-authored-by: Cursor <cursoragent@cursor.com>
@kduncanhsu
kduncanhsu force-pushed the kduncanhsu/comment-range-display branch from d2ba1aa to 8c04129 Compare September 8, 2026 18:48
@jpan-box

jpan-box commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

the hardcoded separator caught my eye just because it's... well, hardcoded, but unless there is any elegant way around that i'm fine with it as is!

@mergify

mergify Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-09-08 21:19 UTC · Rule: Automatic strict merge · triggered by rule Automatic merge queue
  • Checks passed · in-place
  • Merged2026-09-08 21:59 UTC · at 5a32c450977bd187d0bd25dd5406b7da81e99936 · squash

This pull request spent 40 minutes 13 seconds in the queue, including 13 minutes 43 seconds running CI.

Required conditions to merge
  • github-review-approved [🛡 GitHub branch protection]
  • any of [🛡 GitHub branch protection]:
    • check-success = Summary
    • check-neutral = Summary
    • check-skipped = Summary
  • any of [🛡 GitHub branch protection]:
    • check-success = lint_test_build
    • check-neutral = lint_test_build
    • check-skipped = lint_test_build
  • any of [🛡 GitHub branch protection]:
    • check-success = license/cla
    • check-neutral = license/cla
    • check-skipped = license/cla
  • any of [🛡 GitHub branch protection]:
    • check-success = lint_pull_request
    • check-neutral = lint_pull_request
    • check-skipped = lint_pull_request

@mergify
mergify Bot merged commit 5a32c45 into box:master Sep 8, 2026
9 of 10 checks passed
@mergify mergify Bot removed the queued label Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants