Add timestamped comment button to video player#1641
Conversation
…7490daa6fa0c4e338e78
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed:
Open the full interactive recap |
|
@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) |
|
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. CI — Lint & format ( CI — Security guards ( 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. |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
🔴 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`.
There was a problem hiding this comment.
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.
|
There was a problem with your request, please try again later. Error id: |
There was a problem hiding this comment.
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
|
looks great @SajalChaplot! thanks for being on top of all the builder review bot feedback too and resolving them |
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
TimestampedCommentButtoncomponent – a small trigger button (shown only when comments are enabled) that captures the current video timestamp and opens the comment composer.TimestampedCommentBarcomponent – 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. SupportsCmd/Ctrl+Enterto submit andEscapeto dismiss.r.$recordingId.tsxupdates:commentOpenandcommentAtMsstate to track composer visibility and the pinned timestamp.relativeso the overlay positions correctly.EditableRecordingTitlein the reactions row withTimestampedCommentButton.Screen.Recording.2026-06-29.at.3.07.08.PM.mov
To clone this PR locally use the Github CLI with command
gh pr checkout 1641You can tag me at @BuilderIO for anything you want me to fix or change