feat(activity-feed-v2): uncheck timestamp toggle when viewer dismisses range draft by kduncanhsu · Pull Request #4841 · box/box-ui-elements

kduncanhsu · 2026-09-18T00:53:49Z

Summary

Listen for comment_range_draft_dismiss from the audio viewer when the user clicks the waveform outside an open draft range.
Uncheck the timestamp toggle and drop the pinned range, matching a manual toggle-off.
Echo comment_range_draft_clear only if the toggle was on, so the composer and viewer stay in sync.

Test plan

Toggle the timestamp checkbox on, drag a range, then click outside the range on the waveform: checkbox unchecks and handles disappear.
Toggle the timestamp checkbox on with no drag (collapsed handles), then click outside: checkbox unchecks.
With the checkbox off, clicking the waveform does not change the checkbox or emit comment_range_draft_clear.
After a dismiss, toggle the checkbox back on: it captures the current playhead and start follows pause/seek again.
Unit tests: useMediaTimestamp.test.tsx and ActivityFeedV2.test.tsx.

Made with Cursor

Summary by CodeRabbit

Bug Fixes
- Improved audio comment timestamp behavior when dismissing a committed range.
- Dismissing a range now clears the timestamp selection, restores the single-timestamp display, and keeps the timestamp synchronized with playback pause and seek actions.
- Timestamp controls now reset consistently when a waveform range is dismissed or toggled off.

…s range draft When the user clicks the waveform outside an open comment range, the viewer emits comment_range_draft_dismiss. Treat that as toggle-off so the checkbox, pinned range, and draft handles stay in sync. Co-authored-by: Cursor <cursoragent@cursor.com>

coderabbitai · 2026-09-18T00:54:38Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: a6c5911d-f4bf-454d-9b18-d2dd2c369cfa

📥 Commits

Reviewing files that changed from the base of the PR and between a4ca178 and d195f09.

📒 Files selected for processing (3)

src/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsx
src/elements/content-sidebar/activity-feed-v2/__tests__/useMediaTimestamp.test.tsx
src/elements/content-sidebar/activity-feed-v2/useMediaTimestamp.ts

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

Walkthrough

Changes

Audio draft dismissal

Layer / File(s)	Summary
Dismissal handling `src/elements/content-sidebar/activity-feed-v2/useMediaTimestamp.ts`	The hook handles `comment_range_draft_dismiss` by clearing the pressed state, pinned range, and end timestamp. It emits `comment_range_draft_clear` and removes the listener during cleanup.
Dismissal validation `src/elements/content-sidebar/activity-feed-v2/__tests__/*`	Tests cover dismissal resets, ignored dismissal when inactive, resumed pause and seek tracking, and listener registration conditions.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant AudioRangeViewer
  participant useMediaTimestamp
  participant TimestampToggle
  AudioRangeViewer->>useMediaTimestamp: comment_range_draft_dismiss
  useMediaTimestamp->>TimestampToggle: clear pressed state and timestamp range
  useMediaTimestamp->>AudioRangeViewer: comment_range_draft_clear

Suggested reviewers: greg-in-a-box

Merge Risk: ⚪ Minimal · up to d195f

The timestamp dismissal behavior clears the range and keeps the viewer and composer synchronized, with coverage for the relevant lifecycle and re-enable flows. No merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: unchecking the timestamp toggle when the viewer dismisses a range draft.
Description check	✅ Passed	The description includes a clear summary of the behavior change and a relevant test plan covering the affected scenarios and unit tests. The repository template contains only merge-process guidance an…
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…
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 a new PR

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 taps the waveform bright
The draft steps softly out of sight
The toggle rests, the range is clear
A fresh pause may start anew here
The viewer and hook now hop in tune}

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

greg-in-a-box

Review

Looks good — clean wiring for comment_range_draft_dismiss.

Sharing the toggle-off path through uncheckTimestamp keeps click-outside and the checkbox in lockstep (pressed off, pinned range dropped, clear echoed only when the toggle was on). Listener attach/remove matches the existing comment_range_draft_change pattern, and the unit + ActivityFeedV2 coverage hits the important cases (dismiss while on, ignore while off, re-toggle after dismiss).

Nit (non-blocking): the test plan calls out dismiss with the toggle on but no prior drag (collapsed handles). That path is the same code as the dragged case, but a one-liner unit test would lock it in.

Approving.

greg-in-a-box

Review

Looks good — this mirrors manual toggle-off cleanly and keeps the composer/viewer in sync.

What works well

Extracting uncheckTimestamp so dismiss and toggle-off share one path avoids drift.
Guarding dismiss with isPressedRef so an off toggle does not emit a spurious comment_range_draft_clear.
Listener add/remove for comment_range_draft_dismiss matches the existing change-listener pattern.
Solid coverage: dismiss-with-range, ignore-when-off, re-press after dismiss, plus the ActivityFeedV2 integration path.

Nit (non-blocking)

The PR test plan calls out “toggle on, no drag, then click outside.” That path is the same as dismiss-with-range in production (isPressedRef only), but a tiny unit for collapsed handles would lock the contract in tests. Optional.

LGTM.

greg-in-a-box

Verdict: Approve

Clean consumer for comment_range_draft_dismiss: extract shared uncheckTimestamp, listen/cleanup next to the existing range-change handler, and only echo comment_range_draft_clear when the toggle was on.

What looks good

Dismiss matches manual toggle-off (pressed off, pin dropped, end cleared, clear emitted) without resetting the captured start ms.
Early return when already unchecked avoids a spurious clear.
Hook tests cover dismiss-with-range, ignore-when-off, and re-press following pause/seek; ActivityFeedV2 covers the checkbox + clear emit path; attach/detach expectations include the new event.

Notes (non-blocking)

Manual test plan’s “toggle on, no drag, then click outside” path isn’t covered by a dedicated unit case (only via press + change + dismiss). Behavior should already be correct via the isPressedRef guard; optional follow-up.
Relies on the preview viewer actually emitting comment_range_draft_dismiss — assumed by the PR description / companion change.

CI was still settling when I reviewed; lint_pull_request was green. No material issues for merge from the diff.

mergify · 2026-09-18T19:27:00Z

kduncanhsu requested review from a team as code owners September 18, 2026 00:53

greg-in-a-box approved these changes Sep 18, 2026

View reviewed changes

abhishek1128 approved these changes Sep 18, 2026

View reviewed changes

kduncanhsu added the ready-to-merge label Sep 18, 2026

mergify Bot added the queued label Sep 18, 2026

Merge branch 'master' into feat/comment-range-draft-dismiss

9db702e

mergify Bot merged commit 28e9e65 into box:master Sep 18, 2026
9 of 10 checks passed

mergify Bot removed the queued label Sep 18, 2026

Navigation Menu

feat(activity-feed-v2): uncheck timestamp toggle when viewer dismisses range draft - #4841

feat(activity-feed-v2): uncheck timestamp toggle when viewer dismisses range draft#4841
mergify[bot] merged 2 commits into
box:masterfrom
kduncanhsu:feat/comment-range-draft-dismiss

kduncanhsu commented Sep 18, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Sep 18, 2026 •

edited

Loading

Uh oh!

greg-in-a-box left a comment

Uh oh!

greg-in-a-box left a comment

Uh oh!

greg-in-a-box left a comment

Uh oh!

mergify Bot commented Sep 18, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kduncanhsu commented Sep 18, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

greg-in-a-box left a comment

Choose a reason for hiding this comment

Review

Uh oh!

greg-in-a-box left a comment

Choose a reason for hiding this comment

Review

Uh oh!

greg-in-a-box left a comment

Choose a reason for hiding this comment

Verdict: Approve

What looks good

Notes (non-blocking)

Uh oh!

mergify Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kduncanhsu commented Sep 18, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 18, 2026 •

edited

Loading

mergify Bot commented Sep 18, 2026 •

edited

Loading