Skip to content

Play a pasted Clips link inside a Content page, with its comment markers - #8147

Open
3mdistal wants to merge 17 commits into
clips-content-bindingsfrom
clips-content-clip-block
Open

3mdistal wants to merge 17 commits into
clips-content-bindingsfrom
clips-content-clip-block

Conversation

@3mdistal

@3mdistal 3mdistal commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

A Clips recording pasted into a Content page should play there, for the people who can read the page, with the comments its viewers left on it. Today the video block's Link tab accepts any URL and saves it as a plain <video>: a Clips share link or a YouTube link becomes a black player stuck at 0:00 that never plays, and it stays in the page that way.

This is the Content side of "Clips videos embedded natively in Content docs" (step A3). It is stacked on #8090 (Clips' server side, A2), which stacks on #8088 and #8059, and it will be rebased onto main once those merge. Every new Clip path is behind the content.clip-embed flag, which is off by default; with the flag off, Clips links are refused in the Link tab and existing Clip blocks say Clips in pages aren't turned on. Three changes apply with the flag off too: the Link tab now refuses YouTube links and web pages, which never played as a video and only saved a dead player; a copied registry block, such as an Inline database, keeps its data when pasted instead of dropping out of the page on save; and a public page names each registry block while it loads instead of printing the block's source.

Approach

A pasted Clips link becomes a Clip block, not a video. The Link tab decides what a link is before anything is saved, without Content's server fetching it:

  • A link to the Clips app this Content deployment is paired with (/r/, /share/ or /embed/ plus a recording id) becomes <ClipEmbed id="…" recordingId="…" origin="link" /> in the page body. A share id is the recording id, so no call to Clips is needed to resolve it.
  • A video page on YouTube, Vimeo or Loom, a Clips page that isn't a recording, or a malformed URL is refused with a message saying why.
  • Any other link, including one ending in .mp4, is loaded by the pasting reader's own browser as a video before it's saved; if it doesn't load as one within 8 seconds, it's refused ("That link isn't a video file."). A file name proves nothing: a link ending in .mp4 can still answer with a sign-in page.

Content finds Clips from deployment config only. The Clips URL comes from Content's built-in app list (getBuiltinAgents("content"), with the deployment's URL override and the beta lane mapping), never from a user's or organization's agent directory. That URL is where tokens go and which links count as Clips links, so it must not be something a workspace member can point elsewhere.

Every read checks the page first, then asks Clips. The player calls two read actions, get-clip-playback and list-clip-comments. Each one first checks that the caller can read the page that holds the block, and that the flag is on for them; only then does it mint a one-minute token with the page-access secret Content and Clips share (#8090's CONTENT_CLIPS_PEER_SECRET) and ask Clips. Clips decides whether that page reader may see the recording. Minting lives in one function, mintContentClipToken. The token carries the reader, a scope, the page id, and the recording id, and its audience is Clips' /content-clips endpoint. Content reads no more of Clips' answer than the longest one the contract allows: 64 KiB for playback (Clips cuts the title to 500 characters), and about 2.9 MiB for a comment list (500 threads with every field at its cut length). A longer answer gets the same "Clips isn't responding" state as one Content can't read.

Signed-out readers get public read only. On a public page (/p/:id), a reader with no session, or with Content's anonymous public-viewer identity, may play a Clip only when the page itself is public. Their token uses the content-clip:public-read scope and a fixed subject instead of an email, so Clips plays only a recording that is public, or one that was uploaded into that page. A signed-in reader needs ordinary read access to the page and gets content-clip:read with their email and name. A signed-out reader has no organization, so the flag is evaluated under the page's organization.

Distinct states, not one generic failure. Clips not set up (clips_not_configured, 503), Clips not answering, Clips refusing too many requests (429), no access or no such recording (the same message for both, so a page can't probe which recordings exist), an expired or password-protected Clip, the flag off, and a recording that can't play each show their own message. A short media grant is held until 30 seconds before it expires or until the video reports a media error, then refreshed; a second error on a fresh grant shows "can't be played" instead of retrying forever. If Clips doesn't answer a refresh while the video plays, the video keeps playing on its grant and the refresh is asked again after 5, 15 and 60 seconds while the grant lasts. The outage message replaces the player only when the source itself fails: a media error, or the grant running out with no refresh answered. A refusal (the flag off, no access, an expired Clip) still stops at once. Through an outage nothing asks on a timer: the tab becoming visible or the window regaining focus asks once, and Try again stays. When Clips answers, the player picks up at the time and play or pause state the reader left. A refreshed source picks up where the reader was, even when the source before it failed before loading. Try again shows the loading placeholder and then plays only what its own request returned, or shows that request's failure. If the comment list fails, the player still plays and says the comments couldn't load, keeping any markers it already had.

The player is Clips' own. Controls, scrubber, speed, volume and full screen are copied from Clips, and comment markers are grouped by second the way Clips groups them. Hovering a marker shows the comment; clicking one seeks there. When Clips returns no comments (comments turned off), there are no markers. Content asks Clips for the first 500 comment threads by video time, and Clips reads only that many and cuts each one's text to the 280 characters a marker shows; when a Clip has more, the player says it is showing the first 500 instead of looking complete. A Clip whose source is a YouTube video plays in YouTube's no-cookie player, and only for a bare youtube.com/embed/<id> URL from Clips.

The block stores only the recording id. Media URLs, posters, titles, durations and comments are fetched per view and never saved in the page. A player belongs to one page and one recording, so a block that ends up under another page starts a new player and no grant crosses pages. Both reads declare persistInBrowser: false so that short-lived grants and comment text stay out of the browser's saved query cache. Core's action loaders drop that setting today, in dev and production, so until #7969 lands the browser still saves both answers (see Safety).

What changed

  • templates/content/actions/: get-clips-links, get-clip-playback, list-clip-comments (all agentTool: false, GET, read-only) and _clip-embed-access.ts, which holds the page check and flag check they share.
  • templates/content/server/lib/clips-peer.ts: where Clips lives, the token, and the one request helper that reads Clips' answers through a byte cap per route and maps them to typed results.
  • templates/clips/server/lib/content-clip-peer.ts (Clips' route from Let Content document readers play and comment on Clips uploaded to the document #8090): this PR also bounds Clips' comment route. GET /api/content-clips/:recordingId/comments now takes ?limit=<n> (1 to 500; a request with no limit gets 500) and reads only thread starts with LIMIT n+1, instead of running the whole list-comments query for Content to cut down. It answers { comments, truncated }, keeping list-comments' access check, order and author names, and cuts each thread start's text to CONTENT_CLIP_COMMENT_PREVIEW_CHARS (280) and its author's name and email to CONTENT_CLIP_COMMENT_AUTHOR_CHARS (320), so the longest list has a known size. The playback answer's title is cut to CONTENT_CLIP_RECORDING_TITLE_CHARS (500). It also changes the route's rate limit. A request counts against a reader's bucket for a Clip only after Clips has opened the recording, so a made-up recording id makes no bucket there. A request whose recording doesn't open counts against a separate miss bucket for that reader, page and recording. When either bucket is full, Clips refuses that Clip before any database read. When the map of buckets is full, Clips drops its oldest bucket rather than refuse a new reader. So a Clip that opened once and then went away stops costing lookups once its misses are spent, and one reader's made-up ids can't refuse anyone's first open of a real Clip. The contract constants and the video-sharing skill describe all of this.
  • templates/content/shared/: the ClipEmbed registry block, the Link tab's link rules (video-link.ts), the Clips contract types, and the flag.
  • templates/content/app/components/editor/clip-embed/: the block, player, scrubber and controls; extensions/VideoLinkForm.tsx is the Link tab, and VideoBlock.tsx swaps itself for a Clip block when the form returns a Clips link.
  • templates/content/app/routes/p.$id.tsx: the public page provides a Clip-only block registry so signed-out readers get the player. Before the editor mounts, the page's body comes from parsing the saved page with Content's own parser: each registry block shows its saved title or its registry label instead of its MDX source, and a Clip is a link to its recording on Clips. That HTML is the one cached public shell, so it uses only the saved block and deployment config, never the reader's session or a call to Clips.
  • view-screen names a selected Clip's recording, so the agent knows which Clip the user means. It quotes only the ids and block types Content writes.
  • BubbleToolbar.tsx: the text toolbar no longer appears over a selected registry block (it had nothing to format there).
  • packages/toolkit/src/editor/RegistryBlockNode.tsx: a registry block copied out of the editor carries its MDX source in data-block-source, so a pasted copy keeps its data and still saves instead of dropping out of the page. Clipboard HTML can come from any page, so a pasted block is read only through its data-* attributes: a malformed block type drops it, a malformed id is replaced with a fresh one, and the new readPastedSource option keeps a pasted source only when the app reads it back. With that option set, a pasted block that carries no source is dropped, since it would save as nothing. Content's reader (extensions/registryBlocks.ts) accepts a source only when it parses as exactly one registered block of the declared type and id. Toolkit patch changeset included.
  • i18n for every new string in every configured locale; a Content parity-matrix row for the three hidden reads.

Safety and operations

  • Rollout: flag content.clip-embed, off by default. Turning it off stops new playback reads; Clip blocks already in pages stay in the body and show the "not turned on" message on their next read.
  • Turning the flag off, then reloading: until Keep all action settings when actions are loaded, in dev and production #7969 lands, the first reload after the flag is turned off can still play a Clip for up to about four and a half minutes. Core's action loaders drop persistInBrowser: false, so the browser saved the Clip's grant and restores it on reload, and the player doesn't ask again until that grant's refresh comes due. The block's comment read is refused at once, and that refusal clears the saved cache, so later reloads show "not turned on" straight away. With Keep all action settings when actions are loaded, in dev and production #7969, a reload asks Content and shows "not turned on" at once.
  • Configuration: playback needs CONTENT_CLIPS_PEER_SECRET set to the same value on Content and Clips (from Let Content document readers play and comment on Clips uploaded to the document #8090). Without it the player says Clips isn't set up; nothing else changes.
  • Data: no schema change and no migration. Page bodies gain a <ClipEmbed … /> tag; nothing else is written.
  • Deploy order: Clips and Content must both run this PR's code before the flag turns on. A Content from before this PR sends no limit, which Clips now answers with the first 500 threads, so that read stays bounded. This PR's Content, though, refuses a comment list without truncated, and a Clips from before this PR sends uncut comments and titles that can go over Content's caps. With the flag off, nothing calls either route.
  • Made-up recording ids: each one costs Clips one database lookup the first time a reader names it on a page. Repeats of it are refused once its misses are spent. Nothing bounds a caller who keeps naming new ones until Content limits signed-out reads per visitor (see Known limits).
  • Rollback: turn the flag off. Reverting the code instead would leave any saved <ClipEmbed … /> tags in page bodies with nothing to render them.

Verification

  • At 6554fcb, in Content: the clip-embed folder and the Clip actions pass (4 files, 88 tests). They include: a refresh Clips doesn't answer while the video plays, which leaves the video alone and asks again; a later try's grant swapped in place, with the regular schedule after it; the outage shown once the grant runs out unrefreshed, or as soon as the source fails; a refusal still stopping at once; no asks while the tab is hidden and one when it shows; focus asking again until Clips answers; and, in the block, the player picking up at the same time and play state after an outage. The 7 new or changed hook tests failed before the change, the first with "expected 'refresh_failed' to be null", and the resume test failed without the saved place ("expected +0 to be 42"). pnpm typecheck in Content is clean.
  • On a local Content dev server, get-clip-playback and list-clip-comments answer with x-agent-native-browser-persist: allow despite persistInBrowser: false. That is the Keep all action settings when actions are loaded, in dev and production #7969 loader bug behind the flag-off delay above.
  • At 5e2d1fc, in Content: the 8 test files the Clip block's changes touch pass (167 tests: the clip-embed folder, clips-peer, the Clip actions, registryBlocks, the registry round trip and the public page route). They include a Clip block moved to a page whose check fails, which now shows that failure instead of the old page's video; a 429 showing its own message; a comment list without truncated refused; the longest comment list the contract allows being read; and, for each route, an answer exactly at the byte cap read and one a byte over refused. pnpm typecheck in Content is clean.
  • At 0fa3338, in Clips: the 88 tests in server/lib/content-clips.test.ts pass. They include made-up recording ids answering 404 without pushing a reader's spent bucket out of a shrunk limiter; a full map of opened Clips dropping its oldest bucket instead of refusing a new reader, while a reader at their limit stays refused when the map has room; a made-up id refused with 429 once its own misses are spent, while the reader's other ids and the Clip that opened for them still answer; a Clip that opened and then lost its binding refused with 429 once its misses are spent; signed-out readers' made-up ids not stopping another signed-out reader's first open of a real Clip; the oldest misses forgotten, not refused, when the map is full; a request with no limit getting the first 500 threads; and a 5,000-character comment, 400-character names and a 5,000-character title coming back cut to the contract's lengths. pnpm typecheck in Clips is clean.
  • At e65f245, the toolkit's RegistryBlockNode.spec.ts passes (12 tests), including a pasted block with no source dropped when the app reads sources.
  • The new tests for each review fix failed before it. The 429, missing-truncated and longest-list tests check behavior that was already right, as do the tests that made-up ids don't push a spent bucket out and that a reader at their limit stays refused.
  • At 0fa3338, pnpm guards against clips-content-bindings: all 93 checks pass, including guard:i18n-catalogs, guard:i18n-changed-copy and guard:ssr-cache-shell.
  • Earlier rounds, not rerun here: manual QA on local Content and Clips before the review fixes, and an independent model review of the diff. The fixes made since are covered by the tests above.

Review focus

  • use-clip-playback.ts: does anything still ask on a timer through an outage, or ask while the tab is hidden, and does every refusal still stop at once?
  • actions/_clip-embed-access.ts: is there any path that mints a token or reaches Clips before the page check passes, or that gives a signed-out reader more than public-read?
  • server/lib/clips-peer.ts: are the token's claims exactly what Let Content document readers play and comment on Clips uploaded to the document #8090's Clips side verifies, and does every Clips answer map to the right typed state?
  • shared/video-link.ts: the Link tab's accept/refuse rules, especially links Content's own server never fetches.
  • Flag evaluation for signed-out readers under the page's organization.
  • listCommentThreads in Clips' content-clip-peer.ts: the bounded read must keep what list-comments checked and returned for a marker.
  • Clips' rate limit in content-clip-peer.ts: a request counts against its Clip's bucket once the recording has opened, and against that reader's miss bucket for the page and recording otherwise. Can any request do database work without counting against one of them, or let one reader's requests refuse another's?
  • The byte caps in Content's clips-peer.ts: do they hold the longest answer the contract allows?
  • RegistryBlockNode.tsx and Content's readPastedContentBlock: now that a copy carries its source, what can pasted HTML from another page bring into a block?
  • p.$id.tsx's first paint: it must stay one impersonal shell for every visitor.

Known limits and follow-ups

  • Signed-out readers share one rate limit per page and Clip. Clips allows 600 reads a minute per reader, page and Clip in each process, and every signed-out reader of a page counts as one reader: Content vouches for them all with one fixed subject and, by design, sends no visitor identity. A page view makes two reads, plus a grant refresh about every four and a half minutes, so a very popular public page could reach it; its readers then see "Too many requests. Try again in a minute." A per-visitor limit needs Content to limit signed-out reads per visitor before it mints a token. That is a prerequisite for A6, when the flag turns on. It is also what bounds made-up recording ids. Clips keeps misses per Clip, not per page, so that one visitor's made-up ids can't refuse another's first open of a real Clip. A caller who keeps naming new made-up ids therefore costs Clips one lookup each, until Content limits each visitor before it mints a token.
  • A page in someone's Personal space has no organization. While the flag is rolled out by organization only, signed-out readers of such a public page see "not turned on" even though its signed-in owner sees the player. It fails closed; a global rule covers it.
  • Exports: HTML and PDF export print a registry block's tag as text, for every registry block (Inline databases too). Markdown export keeps <ClipEmbed … /> as is. Separate fix.
  • Comment list in the database: the bounded read finds one Clip's comments through the (recording_id, created_at) index, then sorts that Clip's thread starts by video time before the limit. It returns, sends and parses at most 501.
  • Slow video hosts: the Link tab gives a pasted link 8 seconds to load as a video. A host that answers slower is refused, and pasting it again retries.
  • Pasted copies: a pasted Clip keeps the original block id in its source until its next edit; existing registry-block behavior.
  • Not checked: whether the recording is still in the page's saved body. A just-pasted block plays before its save lands, so Content leaves the recording decision to Clips.
  • Later steps: uploading a Clip from a page (A4), margin comments on a Clip (A5), agent tools, the template changelog entry and removing the flag (A6). The Clip feature's changelog entry waits for A6, when it turns on; this PR records only the two fixes that apply with the flag off: the public page's first paint and pasted registry blocks keeping their data.

🤖 Generated with Claude Code

A Clips link pasted into a video block's Link tab becomes a Clip block that
plays in Content's own player, with the Clip's comments as markers on the
scrubber. Content asks Clips for playback and comments over the peer
contract, signed with the Content-Clips peer secret, and checks document
access first, so signed-out readers of public pages can watch too.

Registry blocks now keep their source on the clipboard, so a copied Clip
pastes with its data instead of dropping out of the save.
@github-actions

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.

… and keep Clip reads out of the browser's saved cache
@3mdistal

Copy link
Copy Markdown
Contributor Author

The review body's LOW finding (private GET results kept in the browser's persisted query cache) is fixed in b9acde2. get-clip-playback and list-clip-comments now set persistInBrowser: false, the framework's existing opt-out: the action route then leaves off X-Agent-Native-Browser-Persist: allow, so the client never writes playback grants or comment text to disk. A test in actions/clip-embed-actions.test.ts covers both actions.

builder-io-integration[bot]

This comment was marked as outdated.

… through a failed source, and check pasted blocks

- The public page's first paint parses the saved page and shows each
  registry block by its saved title or registry label; a Clip is a link to
  its recording on Clips, built from the block and deployment config only.
- A source swapped out before its metadata loaded no longer leaves its
  restore behind, and the next source restores the reader's place.
- The toolkit reads a pasted block only through its data attributes, checks
  the type and id format, and keeps a pasted source only when the app reads
  it back. Content keeps one only when it is exactly one registered block of
  the declared type, and view-screen quotes only ids and types Content writes.
@3mdistal

3mdistal commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

The incremental review's two low findings are fixed in 2b1f055:

  • Stale playback state after a failed source: a source swapped out before its metadata loaded no longer leaves its restore behind, so the next source restores the reader's own place.
  • Pasted registry metadata: the toolkit reads a pasted block only through its data attributes, checks the type and id format, and keeps a pasted source only when the app reads it back. Content keeps a pasted source only when it parses as exactly one registered block of the declared type. view-screen quotes only the ids and types Content writes, so clipboard HTML can't put arbitrary text into the agent's selected-block hint or the saved page.

Both are covered by new tests in RegistryBlockNode.spec.ts, registryBlocks.spec.ts, registryBlocks.roundtrip.test.ts, view-screen.test.ts, clip-media.spec.ts.

@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 4 potential issues 🔴

Review Details

Incremental review — PR #8147

This update adds native Clips embeds to Content pages, including playback/comment-marker reads, public-page rendering, clipboard serialization for registry blocks, and bounded comment-thread reads on the Clips peer route. I classified this as high risk because it crosses public-read authorization boundaries and introduces peer API behavior. The overall design is thoughtful: reads are page-authorized before token minting, peer tokens are scoped and short-lived, response fields are schema-validated, comment queries are capped, and private playback/comment data is excluded from browser persistence.

Findings

  • 🔴 HIGH: A playback grant is retained when the same Clip block is rendered under a different document, potentially carrying access from the prior page across navigation.
  • 🟡 MEDIUM: The Content peer client buffers the full response before schema validation, with no response-byte cap.
  • 🟡 MEDIUM: All anonymous readers of a given page/Clip share one read-rate-limit bucket.
  • 🟡 MEDIUM: Pasting a registry block without data-block-source bypasses the provided source validator and can create a block that serializes to nothing.
  • LOW: Unbound recording IDs can allocate rate-limit buckets before the Clips binding check, potentially exhausting the bucket map.

The previously open public-SSR issue is fixed: the initial server render now displays a named registry-block fallback, and that thread has been resolved. Browser testing was skipped because the browser-test-planner agent is unavailable in this environment; the dev server itself is running. No tests were run as part of this review.

Comment thread templates/content/app/components/editor/clip-embed/ClipEmbedBlock.tsx Outdated
Comment thread templates/content/server/lib/clips-peer.ts Outdated
Comment thread templates/clips/server/lib/content-clip-peer.ts
Comment thread packages/toolkit/src/editor/RegistryBlockNode.tsx Outdated
…ount only opened Clips against the rate limit, and drop pasted blocks with no source

- The Clip block's player is keyed by page and recording, so a block that
  ends up under another page never keeps the last page's grant.
- Content reads each Clips answer through a byte cap sized from the
  contract. Clips now cuts a listed comment's text to 280 characters and its
  author's name and email to 320, so the longest list has a known size.
- Clips counts a request against its rate limit only after it has opened the
  recording, so made-up recording ids make no buckets.
- With readPastedSource set, a pasted registry block with no source is
  dropped instead of saving as nothing. Changeset and Content changelog
  updated.
@3mdistal

Copy link
Copy Markdown
Contributor Author

On the review's LOW note (rate-limit buckets were made per recording id before Clips checked the recording was bound to the page or public): fixed in e65f245.

A request now counts against a reader's bucket only after Clips has opened the recording for that reader. That means it exists, the reader may see it through the page's binding or because it's public, and it isn't trashed, expired or password-protected. A made-up recording id gets a 404 without making a bucket, so it can't fill the 5,000-bucket map or count against anyone else. A reader whose bucket is already full is still refused with 429 before Clips reads anything, so a real flood stays cheap to refuse.

Test in content-clips.test.ts: with the map shrunk to two buckets, three made-up ids get 404, two real readers still read, and a third real reader gets 429. Before the fix, the third made-up id got 429.

builder-io-integration[bot]

This comment was marked as outdated.

Clips now counts requests whose recording doesn't open against a bucket
per reader and page, and refuses them before any database read once it
is spent, unless the Clip already opened for that reader. A comment
list with no limit gets the most threads the contract allows, and the
playback answer's title is cut to a length both contract files declare.
@3mdistal

Copy link
Copy Markdown
Contributor Author

On the review body's mixed-versions point (no thread), fixed in 5e2d1fc:

  • Clips accepts a comment list with no limit and answers with the most threads the contract allows, 500. A Content from before this PR sends no limit, so it gets a bounded answer instead of a 400. A limit that is present but not a whole number from 1 to 500 is still refused. Test: a Clip with 501 threads, read with no limit, returns 500 and truncated: true.
  • Content stays strict about truncated. A comment list without it is unreadable (clips_unreachable), not a clean empty list. clip-embed-actions.test.ts already covered this, so it's unchanged.
  • Clips cuts the playback answer's title to CONTENT_CLIP_RECORDING_TITLE_CHARS (500). Both contract files declare it, so the title can't push a playback answer past Content's 64 KiB cap. Test: a 5,000-character title comes back as 500.
  • The PR body's Safety and operations section now says Clips and Content must both run this PR's code before the flag turns on. The reason: a Clips from before this PR sends no truncated and doesn't cut comments or titles. With the flag off, nothing calls either route, and neither side of this stack has deployed.

builder-io-integration[bot]

This comment was marked as outdated.

… open

A Clip's misses now share its own key, and a Clip is refused when either
its opened bucket or its miss bucket is full. A Clip that opened once and
then went away stays bounded, and made-up ids spend only their own misses.

@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

Incremental review — PR #8147

The latest update moves miss accounting to a per-Clip key. I verified that this fixes two prior concerns: a Clip that becomes unavailable is still checked against its miss bucket, and misses for one Clip no longer block another Clip’s first open. I resolved those two threads. The existing thread about the shared anonymous per-Clip read bucket remains unchanged and is still open; I did not repost it.

New finding

  • 🟡 MEDIUM: Per-Clip miss keys allow a caller to rotate through distinct invalid recording IDs and receive a fresh miss allowance for each. Each miss still performs several Clips database lookups, while the miss-map capacity evicts old keys rather than imposing an aggregate request limit.

The implementation otherwise applies the new per-Clip miss key consistently, and the tests cover unavailable Clips and cross-Clip isolation. This remains high risk because the path is reachable through anonymous public-page reads. Browser testing was skipped because the browser-test-planner agent is unavailable; the dev server is running. No tests were run during this review.

Comment thread templates/clips/server/lib/content-clip-peer.ts
@3mdistal

Copy link
Copy Markdown
Contributor Author

Round 6 is in 6554fcb: two things A4's QA found.

1. When a grant refresh failed, it stopped a Clip that was still playing, and the player never came back.

  • Fixed in use-clip-playback.ts. A refresh Clips doesn't answer now leaves the video playing on its grant and asks again after 5, 15 and 60 seconds, for as long as the grant lasts.
  • "Clips isn't responding" now shows only when the source itself fails: a media error, or the grant running out with no refresh answered.
  • A refusal still stops at once: the flag off, no access, or an expired Clip.
  • Through an outage nothing asks on a timer. The tab becoming visible or the window regaining focus asks once, a hidden tab never asks, and Try again stays.
  • When Clips answers, ClipPlayer picks up at the time and play or pause state the reader left, using the place it saved as the outage message replaced it.
  • Tests use a fake clock. The 7 new or changed hook tests failed before the change, and the block's resume test failed without the saved place.

2. After the flag was turned off, a reload still played some Clips for a while.

  • It isn't core's flag cache: there isn't one. The flag is read from settings on every request, cached only within that request, and an unreadable flag counts as off. Content's server keeps no cache on this path either.
  • The cause is core's action loaders. In dev and in production they drop persistInBrowser, so get-clip-playback and list-clip-comments still send x-agent-native-browser-persist: allow; a local Content dev server confirms the header. The browser saved each grant, and the first reload after the flip restored it. The player then played it until its refresh came due, up to about four and a half minutes.
  • A4's log fits this. One page load 30 seconds after the flip had 4 players and 3 "not turned on" blocks, which no single read of the flag can produce. Over the next two minutes the players switched off one by one, as grants saved shortly before the flip came due for refresh.
  • Those 4 blocks' fresh comment reads were refused straight away. That refusal clears the saved cache, which is why later reloads showed "not turned on" right away.
  • Keep all action settings when actions are loaded, in dev and production #7969 already fixes the loader, so I haven't patched around it here. The PR body's Safety section now says how long turning the flag off takes until Keep all action settings when actions are loaded, in dev and production #7969 lands, and the Approach section no longer claims the reads stay out of the browser's cache.

Checks:

  • Content: the clip-embed folder and the Clip actions pass (4 files, 88 tests), and pnpm typecheck is clean.
  • oxfmt --check passes on the touched files.
  • pnpm guards against clips-content-bindings: all 93 checks pass.
  • guard:i18n-catalogs and guard:i18n-changed-copy pass. This round adds no copy.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

@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

Incremental review — PR #8147

The latest update replaces the process-wide 429 on a full opened-Clip bucket map with eviction. I confirmed that this fixes the prior “new Clip opens rejected when the map is full” thread and resolved it. The two other open comments remain applicable: anonymous viewers share the per-Clip bucket, and distinct invalid IDs still have separate miss buckets; I left both open and did not repost them.

New finding

  • 🟡 MEDIUM: The new eviction path removes the oldest bucket even when its rate-limit window is still active. A burst of distinct opened-Clip keys can therefore evict an active counter; the evicted key’s next request creates a fresh window, weakening per-reader/Clip limits.

The fix avoids a process-wide outage, but should preserve active counters under capacity pressure or otherwise ensure eviction cannot grant a new allowance. Risk remains high because this affects public-read peer rate limits. Browser testing was skipped because the planner agent is unavailable; the dev server is running. No tests were run in this review.

Comment thread templates/clips/server/lib/content-clip-peer.ts

@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

Incremental review — PR #8147

The latest head merges the Clips comment-visibility and share-page changes. I reviewed their interaction with the Clips peer rate limiter, public-read authorization, document-scoped media grants, and playback refresh/resume. No distinct new confirmed issue was identified in this update. The three open rate-limit threads remain applicable: anonymous readers share a per-Clip bucket; per-Clip miss keys allow rotation through distinct invalid IDs; and capacity eviction can reset active buckets. I left those threads open and did not repost them.

The prior opened-map-full failure is already addressed by the eviction change, but the live-counter reset risk is covered by its existing open thread. The newer comment visibility path keeps reads and reactions behind their respective access checks, and I found no new public grant or playback regression. Risk remains high because the code handles public reads and peer authorization. Browser testing was skipped because browser-test-planner is unavailable; the dev server is running. No tests were run as part of this review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants