Skip to content

Add timestamped comment button to video player#1641

Merged
steve8708 merged 6 commits into
mainfrom
ai_main_7490daa6fa0c4e338e78
Jun 29, 2026
Merged

Add timestamped comment button to video player#1641
steve8708 merged 6 commits into
mainfrom
ai_main_7490daa6fa0c4e338e78

Conversation

@SajalChaplot

@SajalChaplot SajalChaplot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Replaces the editable title in the reactions row with a "Comment" button that opens an overlay composer docked to the bottom of the video, capturing the exact playback timestamp when the comment is made.

Problem

The recording title was displayed in the bottom metadata row alongside reactions, which was not the intended location. There was also no way for viewers to leave a comment tied to a specific moment in the video.

Key Changes

  • New TimestampedCommentButton component – a small trigger button (shown only when comments are enabled) that captures the current video timestamp and opens the comment composer.
  • New TimestampedCommentBar component – an overlay composer that renders absolutely at the bottom of the video container, showing the captured timestamp, a resizable textarea, mention (@) and emoji shortcuts, and a "Comment at X:XX" submit button. Supports Cmd/Ctrl+Enter to submit and Escape to dismiss.
  • r.$recordingId.tsx updates:
    • Added commentOpen and commentAtMs state to track composer visibility and the pinned timestamp.
    • Made the video wrapper relative so the overlay positions correctly.
    • Replaced the EditableRecordingTitle in the reactions row with TimestampedCommentButton.
    • On successful comment submission, switches the side panel to "comments" and refetches player data.
Screen.Recording.2026-06-29.at.3.07.08.PM.mov

Edit in Builder  Preview


To clone this PR locally use the Github CLI with command gh pr checkout 1641

You can tag me at @BuilderIO for anything you want me to fix or change

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@builder-io-integration builder-io-integration Bot changed the title Update from the Builder.io agent Add timestamped comment button to video player Jun 29, 2026
@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@SajalChaplot SajalChaplot requested review from a team and NKoech123 and removed request for a team June 29, 2026 09:39
@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Here's a visual recap of what changed:

Visual recap

Open the full interactive recap

builder-io-integration[bot]

This comment was marked as outdated.

@SajalChaplot

Copy link
Copy Markdown
Contributor Author

@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable)

@builder-io-integration

Copy link
Copy Markdown
Contributor

Thanks for the review — addressed the items and the failing CI:

🟡 MEDIUM #1 — compact/mobile: comments tab selected but sheet not opened after submit — Fixed. onAdded now also calls setSidePanelOpen(true) when isCompactLayout, so the Activity sheet opens and the new comment is visible as confirmation (desktop already showed the persistent side panel).

CI — Lint & format (oxfmt --check) — Fixed. Ran oxfmt --write on the changed files (import ordering + button formatting).

CI — Security guards (guard:i18n-catalogs) — Fixed. The composer had raw visible strings. Moved them to i18n: added commentsPanel.commentButton, composerPlaceholder, mentionSomeone, addEmoji to the en-US source catalog (reusing common.cancel and commentsPanel.commentAt), wired them via useT(), and added translations to all 10 non‑English catalogs. pnpm guards now passes.

Typecheck passes. Pushed in 5fce19b.

Note: the review summary appears truncated mid-sentence, so I could only see the first MEDIUM finding in full. If there's a second finding (e.g., from the browser-testing pass), please post it and I'll address it.

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder reviewed your changes and found 1 potential issue 🔴

Review Details

This incremental update keeps the same overall approach for timestamped commenting on the Clips recording page, while improving it by adding localized strings for the new composer/button UI and fixing the earlier compact-layout visibility gap by opening the comments sheet after submit. The implementation still cleanly reuses the existing add-comment action and keeps the new flow scoped to the player route instead of adding backend surface area. Risk assessment: Low — this remains mostly a UI workflow change, but it affects a core user interaction on supported recording types.

At the current PR head, I found one remaining correctness issue. 🔴 HIGH: timestamp capture is still wrong for Loom-backed recordings because the new button first checks playerRef.current?.video?.currentTime, but Loom players render in an iframe and fall back to currentMs, which is not kept in sync with live Loom playback. That means comments can be pinned to 0:00 or the last seek position instead of the actual moment the user is watching. On the plus side, the compact/mobile regression from the previous revision appears addressed, and the locale coverage for the newly added strings is now present across the modified translation files. 🧪 Browser testing: Skipped — browser executor tools were unavailable in this environment, so UI verification could not be completed

<TimestampedCommentButton
enableComments={recording.enableComments}
onOpen={() => {
const liveCt = playerRef.current?.video?.currentTime;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Comment button captures stale timestamps for Loom-backed recordings

Loom-backed players render in an iframe, so playerRef.current?.video is always null here and this falls back to currentMs, which is only updated on seeks/initial load. Comments on Loom clips can therefore be pinned to 0:00 or the last seeked position instead of the moment the user clicked Comment.

Additional Info
Confirmed in `templates/clips/app/components/player/video-player.tsx`: Loom path updates `currentMs` on seek/initial start only and has no live playback time bridge, unlike native video `timeupdate`.

Fix in Builder

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — good catch on the source-of-truth. I've refactored both the Comment trigger and the existing reactions tray to share a single resolvePlaybackMs() helper (commit 6d7bd33), so they resolve the timestamp identically: native <video> reports a precise live currentTime, otherwise we use the last position the player reported via onTimeUpdate.

Being skeptical on the HIGH severity, though: for Loom-backed clips the precise-timestamp gap is an inherent platform limitation, not a new regression in this PR. Loom renders in a cross-origin iframe that exposes no live playback-time bridge (confirmed in video-player.tsx — Loom only fires onTimeUpdate on seek/initial start). The pre-existing reactions feature already had the exact same fallback behavior, which is why I unified them rather than special-casing comments.

Truly fixing live Loom timestamps requires adding a Loom embed time bridge (Loom Player postMessage/SDK) in video-player.tsx, which is a larger, separate change touching playback for reactions, the scrubber, and comments alike. Happy to do that as a focused follow-up if you'd like — let me know and I'll scope it. For native recordings (the default Clips capture path) timestamps are precise.

@builder-io-integration

Copy link
Copy Markdown
Contributor

There was a problem with your request, please try again later. Error id: 34414109a3984fa6ba8ca0716dca3e49

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder reviewed your changes and has a few items to flag 🟡

Review Details

This incremental update refines the timestamped comment flow by extracting a shared playback-time resolver, keeping the earlier compact-layout sheet-opening fix, and filling in localization strings for the new button/composer UI. The overall approach remains sound for native-video recordings: it reuses the existing add-comment action, keeps the workflow scoped to the player page, and avoids expanding backend surface area. Risk assessment: Low — this is still mainly a UI workflow change, but it affects comment creation behavior.

At the current head, I found one new issue in the overlay composer. 🟡 MEDIUM: the Cmd/Ctrl+Enter shortcut can fire submit() multiple times before the pending state rerenders, so users can create duplicate comments at the same timestamp by pressing the shortcut repeatedly. Separately, the previously reported Loom timestamp issue still appears unresolved, so I did not repost it. On the plus side, the compact/mobile visibility fix appears to remain in place and the new locale keys are present across the modified translation files. 🧪 Browser testing: Skipped — browser executor tools were unavailable in this environment, so interactive UI verification could not be completed

@steve8708

Copy link
Copy Markdown
Contributor

looks great @SajalChaplot! thanks for being on top of all the builder review bot feedback too and resolving them

@steve8708 steve8708 merged commit 1b85c56 into main Jun 29, 2026
110 checks passed
@steve8708 steve8708 deleted the ai_main_7490daa6fa0c4e338e78 branch June 29, 2026 15:35
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.

3 participants