Repository navigation
Conversation
A Clip block's comments, kept in Clips, now show as cards in the page's comment margin at their moment of the video. Readers can comment at the playhead, reply, and resolve through new Content actions that use the existing Clips peer route. Behind the content.clip-embed flag.
A read of a Clip's comment list that landed while a write waited replaced the optimistic entry, and a refusal then took back what the read brought. Writes now wait beside the list and show over every read until Clips refuses them or the read after Clips took them lands. Also from review: a folded Clip stack shows its first comment before the playhead reaches any; Comment goes away once Clips says comments are off; the open-threads fixture matches the contract's bounds; strict-lint and escape-hatch findings on added lines.
A write Clips took was dropped once the reread settled, even when that read failed or never ran, so a saved comment vanished and a resolved thread came back. It now stays until a read of the list made after Clips took it lands.
|
Here's a visual recap of what changed: Open the full interactive recap |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 5 potential issues 🔴
Review Details
Review summary
PR #8377 adds Clip comment threads to Content’s page margin, adds moment-based commenting/replies/resolution actions, and expands the Clips peer route to return bounded open threads and replies. The architecture appropriately keeps comment data in Clips, checks Content page roles before writes, and retains Clips as the authority for comment settings and per-thread resolution. The implementation also adds useful contract and optimistic-update coverage. This is high risk because it adds cross-service comment mutations and authorization paths.
Key findings
- 🔴 HIGH: A 502/ambiguous failure posting a new Clip thread can permanently discard the user’s draft; retries also lack durable idempotency.
- 🟡 MEDIUM: Optimistic entries can be cleared by a stale or bounded list response;
replyCountis lost in the margin;nulltimestamps coerce to 0; and reply counting can scan all comments for a recording. - 🟢 The pending-thread reply race and unmarked 2,000-character truncation are lower-severity usability issues.
🧪 Browser testing: Skipped — the browser-test-planner agent is unavailable in this environment (the dev server is running).
| }), | ||
| mentions: mentionsJsonFor(pendingText, pendingMentions, agent), | ||
| }); | ||
| const result = clip |
There was a problem hiding this comment.
🔴 Ambiguous Clip create failures discard the submitted draft
A Clips timeout/unreachable response is a 502, which this shared catch classifies as ambiguous and therefore skips restoreSubmittedDraft; finishSubmission then removes the only draft snapshot, while the Clip write hook drops its optimistic entry. Since the write also has no durable idempotency key, the user loses the text on failure and a retry after an actually committed timeout could instead create a duplicate; preserve/reconcile the draft and make Clip writes idempotent.
There was a problem hiding this comment.
Fixed in 0f3f7d4: a Clip comment that fails to post now gives its draft back on any failure, a 502 included. A text comment that may have been saved keeps a card that reconciles it; a Clip comment has no such card, so the draft is the only copy. Test: gives the draft back when Clips answers 502.
The duplicate-on-retry half isn't fixed here. Clips stores no id per write, so it can't tell a retry from a new comment, and teaching it to means a change to Clips' recording_comments table, which this PR stays out of. It's listed under the PR's known limits as a follow-up.
| getDb() | ||
| .select({ threadId: comments.threadId, replies: count() }) | ||
| .from(comments) | ||
| .where(inThreads) |
There was a problem hiding this comment.
🟡 Reply counts scan all comments for the recording on every list request
This count query filters by recording_id and thread_id, but the existing index only covers (recording_id, created_at), so PostgreSQL must scan the recording’s comments to count replies for the listed threads. The scan runs on each open-threads request and grows with the full comment history; add an index suited to (recording_id, thread_id) or otherwise avoid scanning unrelated comments.
There was a problem hiding this comment.
Declining this one in this PR. The count reads one recording's comments only: recording_id leads the existing (recording_id, created_at) index, so the scan is bounded by that Clip's history, not the table. An index on (recording_id, thread_id) would be a new numbered Clips migration, building an index on a production table at startup, and this PR changes no Clips schema. If a Clip with a long comment history reads slowly, that index is the follow-up.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- A Clip comment that fails to post gives its draft back, a timeout included: no card stays behind to reconcile it the way text comments do. - A saved write waits for a list read begun after Clips took it. A read already on its way is cancelled first, since the reread would otherwise join a first read that has no list yet. - A new thread stays shown while the list holds threads back and doesn't list it. - Folded stacks show Clips' reply count, which counts replies the list held back. - add-clip-comment refuses a null or blank videoTimestampMs instead of reading it as 0:00.
There was a problem hiding this comment.
Builder reviewed your changes — no new findings
Review Details
Incremental review summary
The PR head advanced from 22fa64e1 to 0f3f7d4b (+183/−17 across 12 files). This delta rejects null/blank timestamps, restores a Clip comment draft on ambiguous failures, preserves the server’s reply count in margin cards, and changes optimistic-write reconciliation to cancel prior reads and retain omitted new threads from truncated lists. The timestamp and reply-count fixes address two previously reported issues, and I resolved those review threads. Two independent code-review agents found no new issue in this delta.
This remains a high-risk PR because it adds cross-service comment mutations. Existing comments remain open for duplicate risk after ambiguous writes (the draft is now restored, but no idempotency key is added), bounded reply reconciliation, and the per-recording reply-count scan. The bounded-read concern still applies to replies omitted from the capped reply list; the new settlement check only verifies presence for new threads.
🧪 Browser testing: Skipped — the browser-test-planner agent is unavailable in this environment.
People reviewing a video in a Content page need to read and leave comments at the moments that matter, beside the page's own comments. Before this PR, a Clip placed in a page (behind the
content.clip-embedflag, off in production) showed its Clips comments only as scrubber markers you hover. You couldn't read a whole thread or its replies, reply, resolve, or comment at the playhead from the page. Video comments never appeared in the margin where the page's text comments are.This is step A5 of "Clips videos embedded natively in Content docs". It builds on #8166 (comments on a whole video), #8090 (Clips' side), #8147 (playing a Clip in a page) and #8182/#8183 (uploading a Clip from a page), all merged. All of it is behind
content.clip-embed. With the flag off, nothing here runs.Approach
Each open Clip thread is its own comment card in the page's margin. The margin already takes a list of threads and places each beside the text or block it points at. A Clip's open threads join that list as ordinary threads with a
clipfield (recording, Clips thread id, moment, and whether the reader may reply or resolve). Their margin ids areclip:<recordingId>:<threadId>, so they can't collide with the page's own ids. The card's quote line reads the moment ("At 0:06") in the reader's language. The comments stay in Clips: Content stores nothing new, so every page holding the Clip, a duplicated page included, shows the same ones.The Clip block and the margin meet through a small per-page registry (
clip-embed/clip-comments.ts). Each Clip block publishes its threads, a seek function and its playhead. The page editor publishes what the margin can do (start a comment at a moment, open a thread) and what it has open. A block can't reach the margin's React state directly, and the margin can't reach a video element, so each side registers what it offers and looks up what it needs.Stacks work as they do for text. The Clip's player carries its threads' ids in
data-clip-comment-threads, so the stack layout measures it as it measures a block with several threads: two or more open threads fold into one card. That attribute has its own name so that clicking or resting on the player, which you need to do to play it, never opens a thread. A folded Clip stack fronts the thread the playhead last passed. A new comment opens on top of a folded stack without opening it. There is no unresolved count, and focusing the block doesn't open anything.Comment at the playhead. The player's new Comment button pauses the video and opens the margin's composer reading "At m:ss" before anything is typed. Posting calls Content's
add-clip-comment. The card and its scrubber marker show at once with a temporary id. If Clips refuses, both go away and the draft comes back with the same error toast text comments use. A timeout gives the draft back too: unlike a text comment, a Clip comment has no card that later finds out whether it was saved. Clicking a card's moment moves the video there, and clicking a scrubber marker opens its card. Replies useadd-clip-commentwith the thread's id, and Resolve usesresolve-clip-comment. Both show at once and are taken back on failure.Writes reuse the existing trust path between Content and Clips. The new actions check, in Content, that the caller is signed in and can comment on the page, and that the flag is on for them. Only then does Content mint the same one-minute token as before, with the existing
content-clip:commentscope, and post to Clips' existing comment route from #8090 (POST /api/content-clips/:recordingId/comments, operationsadd,reply,resolve). Clips still makes the final call: comments turned off (clip_comments_disabled), or a thread that isn't the reader's to resolve (clip_forbidden). A signed-out reader of a public page never gets a comment token. They see the Clip's markers and no Comment button.Who sees what is decided by Clips, as before. When a Clip's comments are off, Clips' list answers only its owner, so other readers see no cards or markers. The owner sees the cards read-only, with no Comment button or Resolve, since Clips refuses every write while comments are off. The Comment button shows only when Clips' playback answer says the reader may comment and the reader can comment on the page.
What changed
templates/clips/server/lib/content-clip-peer.ts: one Clips change, on the existing comment list route.GET …/comments?view=open-threadsreturns open threads only, each with its text cut to 2,000 characters instead of the 280 a marker preview gets, itscreatedAt, itsreplyCount, and its replies oldest first. Replies are capped at 500 across the list, taken from the earliest thread on. It keeps the route's access check, its order, and the rule that a signed-out reader gets no commenter's email. Withoutview, the route answers exactly as before. The comment-length and timestamp limits it already enforced on writes moved into the shared contract (shared/content-clip-contract.ts) so Content validates against the same numbers.templates/content/actions/:add-clip-comment(new; it takes a moment given as text, as the CLI passes it, and refuses a null or blank one rather than reading it as 0:00),resolve-clip-comment(new), andlist-clip-comments, which now asks for the open-threads view and returns{ threads, truncated }instead of markers. All three are agent tools now._clip-embed-access.tsgainsauthorizeClipCommenter, which requires a signed-in reader with comment access to the page and fails withclip_comment_requiredotherwise.templates/content/server/lib/clips-peer.ts:writeClipCommentposts a write with the comment scope.requestClipsand it share one request helper. The comment list's byte cap grows to cover the longest list the open-threads view can return.clip-embed/use-clip-comment-writes.ts: add, reply and resolve show at once. Each write waits beside the Clip's comment list, not in it, and shows over every read of that list until Clips refuses it or a read begun after Clips took it lands. A read already on its way is cancelled first: otherwise the reread would join a first read that has no list yet, and that read could land after the save without the new comment. A new thread also stays while the list holds threads back and doesn't list it. A refusal takes back only that write, so a read that landed meanwhile keeps what it brought.use-clip-playback.tslays the waiting writes over the list for the margin and the markers.CommentsSidebar.tsx,DocumentEditor.tsx,comment-stacks.ts,CommentStack.tsx,comment-block-anchors.ts: Clip threads in the margin list, their quote line, their place in stacks, and the composer for a Clip moment. A folded stack fronting a Clip thread shows Clips' reply count, which counts replies the list held back. Text-comment paths are unchanged.ClipEmbedBlock.tsx,ClipPlayer.tsx,player-controls.tsx,scrubber.tsx: the Comment button, markers that open cards, the playhead the folded stack follows, and the block's highlight while one of its threads is open.discussion-panel.tsxanduse-comments.ts: a thread on a Clip counts as being "in the text", not a page comment. The Discussion panel and its count don't include Clip threads.document-editingskill, the Comments capability record (content.comment.page-owned) with an acceptance story, and the parity-matrix row for Clip embeds.Safety and operations
content.clip-embed, off by default, evaluated on the server. With it off, the Comment button, the margin cards and all three actions are refused before Content reaches Clips.view=open-threadsand answers without replies; this Content refuses that answer, and the player then says comments couldn't load, rather than showing threads as if no one had replied. With the flag off, nothing calls either route.recording_commentstable through Clips' existing route.Verification
All of this is on 84d5c29 unless it says otherwise.
CommentsSidebar.clip-comments.test.tsxcovers the composer showing "At 0:06", the post going toadd-clip-commentwithvideoTimestampMs: 6000, a 403 bringing the draft back, a quote click moving the video, replies, Resolve only for the author or owner, and read-only when Clips refuses comments.use-clip-comment-writes.test.tsxcovers add, reply and resolve showing at once and being taken back, a waiting reply surviving a read that brings someone else's reply and then taking back only itself, a waiting thread and resolve surviving a read, and a saved write staying when the read after it fails.clip-comments.test.tscovers margin threads, who may resolve, and the thread the playhead last passed, including the first one before the playhead reaches any.ClipEmbedBlock.test.tsxcovers the button, markers and seeking, and the button going away once Clips says comments are off.clip-embed-actions.test.tscovers the three actions' access checks. In Clips,content-clips.test.tscovers the open-threads view.pnpm --filter content typecheckpasses.pnpm --filter clips typecheckpassed on 0c687f4, and no Clips file has changed since.CommentsSidebar.clip-comments.test.tsxpass on 84d5c29: 154 tests.pnpm --filter content test: 9,459 passed and 2 failed, both outside this PR, and both pass when run alone: the 10k-document search budget indocument-search-ranking.perf.test.ts(the machine was busy) and the GitHub folder freshness test ingithub-source.db.test.ts.content-clips.test.tspasses.pnpm guards: 99 checks pass, including strict-lint, no-new-escape-hatches and no-silent-coercion.compiler-strictnesscould not run here because it can't read the Clips tsconfig on Windows, so CI is its check.--noUncheckedIndexedAccess --exactOptionalPropertyTypesreports nothing on lines this branch adds,pnpm guard:strict-lintpasses, Content typecheck passes, and the clip-embed and comments sidebar tests pass (394).pnpm guard:i18n-catalogsandpnpm guard:i18n-changed-copypass. The two new strings are inapp/clip-embed-messages.ts, which the changed-copy guard doesn't read, so I checked by hand thatcommentandcommentMomentare in English and all ten locales.pnpm test:content-product-impact(its CLI tests spawnnode_modules/.bin/tsx, a shell script) and the typecheck step ofpnpm lint(spawn EINVAL).dev@local.test), at 0683159, before the review fixes:list-clip-commentsfound the comment in Clips.CommentsSidebar.clip-comments.test.tsx); a new thread survives a read begun before Clips took it that lands late, and reads that hold it back, then goes once a complete read doesn't list it (use-clip-comment-writes.test.tsx); a folded stack counts a Clip thread's replies from Clips (comment-stacks.test.ts);add-clip-commenttakes "4200" and refusesnull,""and" "(clip-embed-actions.test.ts). Content typecheck passes; the touched suites pass (271 tests);pnpm guard:strict-lint, no-silent-coercion, no-oracle-loosening, no-source-reading-tests, no-new-escape-hatches, action-output-schema and no-bare-error-in-actions pass; the strict compiler check reports nothing on lines this branch adds. The full Content suite and manual QA weren't rerun for this round..tmp/a5-review-clips/): before, on main, a marker preview with an empty margin; after, commenting at 0:10, the stack following the playhead and a marker opening its card, and the rollback with the save held 1.2 s.Review focus
actions/_clip-embed-access.tsauthorizeClipCommenter: can any path mint a comment token for a signed-out reader, or for a reader who can only view the page?use-clip-comment-writes.ts: the waiting writes live in a module-level store keyed by page and Clip. Can one be left behind, shown twice, or dropped before Clips' list has it?listCommentThreads/listThreadRepliesin Clips'content-clip-peer.ts: the open-threads view must stay within the access check and bounds the old list had.CommentsSidebar.tsx: the Clip paths should leave text comments, plain-video comments and stacks unchanged.Known limits and follow-ups
🤖 Generated with Claude Code