Skip to content

Take a video uploaded in a Content page as a private Clip bound to the page - #8182

Open
3mdistal wants to merge 16 commits into
clips-content-clip-blockfrom
clips-content-upload-clips
Open

3mdistal wants to merge 16 commits into
clips-content-clip-blockfrom
clips-content-upload-clips

Conversation

@3mdistal

@3mdistal 3mdistal commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

A video uploaded into a Content page should become a Clip: saved in Clips, played in the page with Clips' own player, and visible to exactly the people who can see the page. Clips can already play such a Clip for a page's readers (#8090 binds a Clip to a page, and #8147 plays one in Content), but nothing can make one. Clips has no way to start an upload on behalf of a Content page's editor. Its upload routes need a Clips session cookie, or a ticket in a header, which from another origin means a CORS preflight that Clips' auth guard answers with the app-wide policy. And a page's uploads would land in the editor's Library, mixed with their own recordings.

This is the Clips side of step A4 of "Clips videos embedded natively in Content docs". The Content side, which builds the upload in the editor, is #8183, stacked on this one. This PR is stacked on #8147 (A3), which stacks on #8090, #8088 and #8059, and it will be rebased onto main once those merge. Everything new answers only a Content upload token, and Content mints one only while its content.clip-embed flag is on (off by default). Until someone uploads from a page, Clips looks and behaves as it does today.

Approach

Content vouches for the editor; Clips decides where the file goes. Content checks that the caller can edit the page and that the flag is on, then mints a one-minute token with the page-access secret the two apps share (#8090's CONTENT_CLIPS_PEER_SECRET). The new content-clip:upload scope carries the page id and the editor's email and name. Unlike a reader's token, it names no recording, because Clips makes the recording. It may also carry client_ip, the browser's address as Content's edge saw it, so Clips' free storage can apply its existing per-network cap to the person uploading instead of to Content's server. A token with a recording_id, or with no email, is refused.

Clips never creates an account or a workspace. The editor's Clips account is read straight from Clips' "user" table, and their workspace with resolveOrgIdForEmail, which only reads. If either is missing, the answer is account_required and Content asks the editor to open Clips once, which sets both up the usual way. Clips never switches anyone's active workspace.

Two server-to-server routes for Content:

  • GET /api/content-clips/upload-status says where a new upload would go, as the ContentClipStorage the contract defines: ready (the workspace has Builder.io or custom storage, or a local development Clips keeps chunks in SQL), free with remaining, limit and maxClipBytes, used_up, none with the free storage's reason, or account_required. It is the same decision create-recording makes, read without reserving anything.
  • POST /api/content-clips/uploads takes { fileName, sizeBytes, mimeType, blockId, uploadKey } (MP4, WebM or MOV; names up to 255 characters; a 4 KiB body at most). It writes the page's upload binding first, so the recording never exists without the page that may show it. It then starts a private recording the editor owns, through the same code create-recording-upload uses (extracted into startRecordingUpload), and returns a ticketed chunkUrl. A start Clips can't take is a 409 storage_required with the current storage, a 413 upload_too_large with maxBytes, or a 429 too_many_uploads. A start that fails deletes its binding only when nothing can be left: a refusal raised before any recording or free clip, or an error whose cleanup Clips confirmed. Any other failure keeps the binding and marks it (upload_start_failed_at), so a repeat can start it again under the same recording id.

A start Content repeats reaches the upload it already made. Content makes one uploadKey per picked file and sends it with every start of that file. Content's own action never retries. A page repeats a start for a file it held after a storage refusal ("Check again", or coming back to the tab), for a start that got no answer, and while Clips answers that the first start is still being made (#8183). A storage refusal leaves no binding, so a repeat reaches an earlier upload only when an earlier start of the file got to Clips. The binding keeps the key, the file's type and size, and a unique index on page, editor and key, so two starts with one key can't both make a recording. A repeat:

  • while the upload is uploading: gets the same recording, another ticket, and the chunk plan made from the first start's type and size. Earlier tickets keep working, because two overlapping repeats each hand one back: a recording keeps its four newest, and still counts as one upload toward the cap of 20. An agent's create-recording-upload still keeps one ticket.
  • while a final chunk holds the upload, or once Clips has the whole file and is making the Clip: a 409 upload_already_finished with recordingId and status, and no ticket.
  • for a file of another type or size, from another block, or for an upload that ended (cancelled, or failed, including a start that failed after making its recording) or is parked waiting for storage: a 409 upload_key_used, and nothing changes. Content then starts the file again under a new key (Save a video uploaded into a Content page as a Clip #8183).
  • before the first start has made its recording: a 409 upload_starting. Once that start is marked failed, or after ten minutes (RESERVATION_GRACE_MS) with no recording, the next repeat takes it over and makes the recording under the same id. The takeover is a compare-and-swap on the binding, so of overlapping repeats one takes it and the rest get upload_starting. Clips reserves a free clip per recording id, so a takeover never holds a second one.

A repeat skips the storage check, since its first start may have taken the last free clip.

The recording stays private. Who can watch it is decided by the page: #8090's resolver plays an upload binding's Clip for whoever Content vouches can read that page, including signed-out readers of a public page.

The browser posts the file straight to Clips. The file never passes through Content's server. POST /api/uploads/:recordingId/content-chunk takes the ticket in its query string. Content posts each chunk with no cookie, no custom headers and no Content-Type, so the request is a CORS "simple request" and the browser sends no preflight. The route answers with Access-Control-Allow-Origin only for pairedContentOrigin(): the deployment's built-in Content on Clips' own lane, never a directory entry and never the request's own Origin. It never allows credentials, and it strips whatever CORS headers were set earlier. A request from any other origin gets a 403 before the ticket is read. Clips' own app-wide CORS policy can allow other origins and credentials, so the Clips middleware answers an OPTIONS to this path itself, before the auth guard: 204 with POST for the paired origin, and 403 for anything else. The ticket is checked by the same code as an agent's chunk ticket. The route also requires the recording to have an active upload binding. So it can't be used to post into a recording that didn't start from a page, and a chunk for an upload whose page is in Trash gets a 409.

A page can cancel an upload it gave up. The start answer also carries a cancelUrl: POST /api/uploads/:recordingId/content-cancel, with the same ticket in its query string and the same CORS rules and preflight as the chunk route. Content calls it when its tab gives an upload up: the block couldn't be put in the page, the block was removed while the file was sending or while Clips held it waiting for storage, or a chunk failed for good. The route runs Clips' own abort (handleAbortRecordingUpload). That marks the recording failed, cleans up its chunks, and releases its free clip once the cleanup is confirmed. Only an upload that is uploading or parked is cancelled.

Where an upload stands is one answer. readUploadPhase in server/lib/final-chunk-claim.ts reads three places, in this order: the final chunk's claim, the upload's application state, then the recording row. It says the upload is:

  • uploading;
  • finishing, while a final chunk holds it;
  • parked, when Clips has the whole file and waits for storage;
  • finished, when the row is processing or ready;
  • ended, when it was cancelled or failed.

The repeated start, the cancel and the Content chunk route all use it, and nothing else reads those three together.

The final chunk claims the upload, and a cancel never changes a final chunk in flight. Before Clips takes the final chunk's bytes, on the buffered and the resumable path alike, the Content chunk route claims the upload. The claim changes the upload state with the same compare-and-swap the abort uses, so a cancel that read the state before the claim fails its own and reads again. Once Clips accepts the chunk, the route writes parked or finished into the upload state, and only then gives the claim up. So a reader that finds no claim finds where the upload went.

  • While the claim is live, a cancel or another send of the final chunk changes nothing and answers 409 upload_finishing. Content sends either one again a few seconds later. A live claim is never taken over.
  • A claim lasts FINAL_CHUNK_CLAIM_TTL_MS, two minutes, a little over Clips' 75 s function limit. An older claim was left by a request that was cut off, so the upload reads uploading again, and the next final chunk or cancel takes it over.
  • A final chunk sent again after Clips accepted the first answers with that one's success (202 waitingForStorage, or 200 with status), and Clips takes no bytes. One that finds the upload ended is refused with a 409. A final chunk that fails gives the claim up, with the upload still uploading.

The row's lease decides between the final chunk and the lease reaper. The reaper fails a row whose lease ran out, in one conditional update. Before it claims, the final chunk renews the row's lease, in one owner-scoped conditional update, only while the row is uploading. The lease becomes at least two minutes from now, so it outlasts the claim. Either the reaper failed the row first and the final chunk finds it ended, or the reaper skips the row. The reaper reads no phase, so upload-lease.ts is unchanged from the base branch.

A cancel's answer is where the upload stands after the abort, never the abort's own result:

  • cancelled (200): the upload ended, including a repeat and a parked upload.
  • already_finished (409, with status processing or ready): Clips had the whole file first. The Clip is left alone.
  • upload_finishing (409): a final chunk holds the upload, and nothing changed.
  • Anything else is a refusal with only error.

A page in Trash can still cancel its upload, because cancelling only frees it.

From Content, not Library. Library leaves out Clips that have an upload binding. If someone moves one into a folder, that folder lists it, and Library still doesn't. A new From Content view (/from-content, list-recordings view from-content) lists those whose binding is active, each with an "Open page" link back to its Content page. A single test, isActiveUploadBinding (activeUploadBinding() in SQL), decides which upload bindings are active. Chunks, From Content, its page links and playback grants all use it. Library leaves out upload Clips whatever their binding's state. The nav entry appears only for someone who has uploaded from a page. A Clip that was made in Clips and then linked into a page stays in Library.

What changed

  • templates/clips/shared/content-clip-contract.ts: the upload scope, the client_ip claim, the accepted video types, the ContentClipStorage states, the refusal codes, how a cancel is sent, and what it and a final chunk answer (CONTENT_CLIP_CANCEL_OUTCOME, CONTENT_CLIP_CHUNK_FINISHING).
  • templates/clips/server/lib/content-clip-uploads.ts: the uploader check, the two Content routes, a repeated start and its takeover, the chunk and cancel routes, and their preflight answer. The route files under server/routes/api/content-clips/ and server/routes/api/uploads/[recordingId]/ (content-chunk.post.ts, content-cancel.post.ts) call them. server/plugins/auth.ts lists the four paths as public (each checks its own token or ticket), and server/middleware/auth.ts answers the browser routes' preflight before the auth guard.
  • templates/clips/server/routes/api/uploads/[recordingId]/abort.post.ts: handleAbortRecordingUpload can take the request from a caller that already read it, so the cancel route reuses it. With onlyWhileUploading, which only the cancel route passes, it changes nothing unless the upload is uploading or parked.
  • templates/clips/server/lib/final-chunk-claim.ts: readUploadPhase; the final chunk's claim, with its lease renewal; where an accepted final chunk left the upload; and giving the claim up.
  • templates/clips/server/lib/recording-upload-start.ts: create-recording-upload's recording and ticket setup, moved here unchanged so both callers share it, plus reissueRecordingUpload for a repeat. recording-upload-ticket.ts gains requireUploadTicketOwnerForToken for a ticket that arrives in the query string, and addRecordingUploadTicket, which keeps a recording's earlier tokens.
  • templates/clips/server/db/schema.ts and server/plugins/db.ts: upload_key, upload_mime_type, upload_size_bytes and upload_start_failed_at on recording_content_bindings, and the unique index on page, editor and key (migrations 94, 95 and 96).
  • templates/clips/server/lib/content-clip-peer.ts (Let Content document readers play and comment on Clips uploaded to the document #8090's code): token verification is split out as verifyContentPeerToken, which checks everything except the recording id, and PRIVATE_HEADERS is exported. Playback and comments still require their token's recording_id to match, as before.
  • templates/clips/server/lib/content-uploads.ts, actions/list-recordings.ts, view-screen.ts, navigate.ts: the active-upload test, the From Content filter and page links, with the view in the agent's screen state and navigation. A folder lists an upload moved into it. server/lib/content-doc-grant.ts (Let Content document readers play and comment on Clips uploaded to the document #8090's code) uses the same test for playback grants. It already refused a binding in Trash, so its behavior is unchanged.
  • templates/clips/app/: the From Content route, nav entry, empty state and card link; strings in all 11 locales.
  • templates/clips/.agents/skills/video-sharing/SKILL.md: how a Content upload works, where it stands, how the page cancels one, and where it's listed.

Safety and operations

  • Rollout: everything new is reached only with a Content upload token, which Content mints only while content.clip-embed is on. With no uploads, Library lists the same Clips as before and the From Content entry doesn't show. Library now makes one extra count read when it loads, for that entry.
  • Configuration: uses CONTENT_CLIPS_PEER_SECRET from Let Content document readers play and comment on Clips uploaded to the document #8090. Without it, the new Content routes answer 503 content_clips_not_configured, like playback. The chunk route needs the built-in Content to be known for Clips' lane; with no paired origin, every browser chunk is refused.
  • Free clips: a Content upload reserves a free clip exactly as an upload in Clips does, and a start taken over keeps the one its recording id already holds. An upload the Content page gives up is cancelled at once, which releases its free clip. Clips' lease reaper is the backstop: an upload left by a closed tab, or one whose cancel Clips never confirmed, holds its free clip until the reaper fails it, about an hour after its last chunk, and the clip is then released. A start that fails after reserving, and whose cleanup Clips can't confirm, keeps its binding and its reservation for a repeat to go on with.
  • The reaper and a final chunk: a final chunk renews its row's lease to at least two minutes from now before it claims, and never shortens it. A row the reaper failed first stays failed, and the chunk is refused. A claim that loses its compare-and-swap after renewing only leaves the lease two minutes longer.
  • Data: three additive migrations on Let Content document readers play and comment on Clips uploaded to the document #8090's recording_content_bindings. Migration 94 adds upload_key and a unique index on (Content app, page, editor, key). Migration 95 adds upload_mime_type and upload_size_bytes. Migration 96 adds upload_start_failed_at. All four columns allow null, and the index ignores rows with no key, so links are unaffected. Each upload writes one binding (origin upload) and one private recording. A final chunk writes one small application-state row for its claim, which comes off when the chunk is done or counts as none after two minutes, and records parked or finished in the upload's existing state. Trashing either, and what happens when its page is trashed, is step A6.
  • Network: the chunk and cancel routes accept requests from one browser origin, with no credentials; requests with no Origin (not from a browser) still need the ticket.
  • Deploy order: this PR's Clips must be live before Content's upload turns on. An older Clips has none of these routes, so Content's upload is refused there and nothing is saved.
  • Rollback: turn the flag off in Content, and no new uploads start. Reverting this PR would leave uploaded Clips in their owners' Library, with their bindings unused.

Verification

  • At 658a8f1, whose templates/clips tree is the same in Save a video uploaded into a Content page as a Clip #8183's head, 05ba5a8:

    • The Clips upload tests pass: 335 tests in 13 files. They are content-clip-uploads, upload-lease, recording-upload-ticket, the uploads/[recordingId] route files, list-recordings, view-screen and use-navigation-state.
    • pnpm typecheck in Clips, oxfmt --check and oxlint on the changed files are clean. The one lint warning, an unsorted sort() in a From Content test, predates this round.
    • pnpm guards at 05ba5a8: all 93 checks pass, including guard:i18n-catalogs and guard:i18n-changed-copy.
  • Each review round's tests were written first:

    • 658a8f1 (lease): "leaves the recording uploading when the reaper runs between a final chunk's claim and Clips taking its bytes, and the chunk finishes" runs Clips' real reaper against a real final chunk. It fails on f2e6784, where the reaper fails the row, and fails again with the renewal removed. "refuses a final chunk once the reaper failed its expired upload, and changes nothing" covers the other order and passes on both.
    • f2e6784: corrects comments only. Its new test, "refuses the key of a start that failed after making its recording, and makes nothing more", passes on the earlier code too.
    • 4c6c367: 8 tests failed on b274c79. They cover a failed start kept and taken over under the same recording, overlapping takeovers, a stale start taken over, a final chunk sent while an earlier send holds the claim, and a final chunk sent again once the upload is finished or parked. Two more, a confirmed failure dropping its binding and an expired claim taken over, pass on both.
    • 11a4449: 12 tests failed on 26eae9a. They cover the phase reader, a cancel against a final chunk in flight, a parked upload's cancel, and an abandoned claim expiring.
    • 66e6440 and earlier: 10 tests in content-clip-uploads, 3 in abort.post and 1 in recording-upload-ticket failed on 528c120, covering overlapping repeats, the four-ticket limit, a repeat for another file, upload_starting, and the final chunk against a cancel and a repeat.
  • server/lib/content-clip-uploads.test.ts is new in this PR. Besides the tests above, it covers:

    • token checks: a read, comment or public-reader token, a token naming a recording, another secret, and another audience are all refused, and nothing is written;
    • account_required for a missing account and for an account with no workspace, with neither created;
    • every storage state, a used-up start refused with 409 and its storage, and a file larger than the free storage takes refused with 413 before anything is made;
    • a start that isn't a video Clips takes (wrong type, zero size, bad block id, extra field, bad JSON) answering 400, and an oversized start body answering 413;
    • one private recording owned by the editor and bound to the page as an upload, which the page's readers can watch and a reader of another page can't, and which an upload token can't open for playback;
    • the chunk route: the paired Content's chunk taken with that origin alone allowed and no credentials, another origin refused with no readable answer, the preflight answered through Clips' auth middleware, a bad ticket's answer readable by the paired page, and a ticket refused when its recording didn't start from a page, including an agent upload's ticket;
    • the cancel: Clips' own abort run with the ticket alone and answered cancelled, a repeated cancel answered the same without aborting again, another recording's ticket and an expired one refused, another origin refused, its preflight answered, and a Clip that has its whole file left alone;
    • finalization winning the race with a cancel, in each shape the abort can report it (verification pending, already ready, a stale attempt), answered already_finished and never cancelled;
    • an upload whose page is in Trash: its chunks refused, missing from From Content and its page links, still out of Library, and still cancellable;
    • From Content versus Library, a folder listing an upload moved into it while Library still leaves it out, and only the signed-in person's own uploads under From Content.

    The 429 refusal has no test of its own. The existing content-clips.test.ts (playback and comments) still passes on the split-out token check.

  • The full Clips suite was last run at 66e6440, not since: all 4,202 tests passed except 2 in server/lib/video-frame.test.ts (probeMediaDurationMs queue tests), which fail the same way on origin/main.

  • An independent model review of the whole A4 diff found six issues. The one in this PR was the chunk route's preflight, which reached the auth guard and got the app-wide CORS policy. All six are fixed, with tests.

  • Manual QA on local Content and Clips with fake accounts and data, recorded at real speed: every storage state, uploads listed under From Content while a linked Clip stays in Library, another reader watching a Clip being made, an outsider refused in Clips, and an unfinished upload failed by Clips' real lease reaper. Save a video uploaded into a Content page as a Clip #8183 has the details. The recordings stay local. That pass came before the cancel route, the folder listing, repeated starts, the phase reader, the final-chunk claim and the lease renewal. Those have tests but no manual pass.

Review focus

  • verifyUploader and handleContentClipUploadStart: can an upload token do anything other than start one private Clip, owned by the editor and bound to the page Content named?
  • The chunk and cancel routes and answerContentChunkPreflight: is there any request that gets Access-Control-Allow-Origin for an origin other than the paired Content, or that allows credentials?
  • handleContentClipCancel and cancelAnswer: can a ticket cancel anything other than its own Content upload while it is uploading or parked? Can an upload Clips finished ever be answered cancelled?
  • final-chunk-claim.ts and takeFinalChunk: can a final chunk and a cancel both win? That would mean a whole file failed, or a Clip finished after its cancel answered cancelled. Can the reaper fail a recording whose final chunk holds a claim? Does every way out of the final chunk, including a thrown error, give the claim up?
  • repeatedStart and takeOverStart: does a repeat ever make a second recording, hold a second free clip, change an upload, or use another slot toward the ticket cap? An agent's create-recording-upload should still keep one ticket.
  • readUploaderAccount: it must never create an account or a workspace, or depend on the editor's active workspace being switched.
  • startRecordingUpload: the extraction should leave create-recording-upload's own behavior unchanged.
  • The From Content and Library split in list-recordings: an upload should never appear in Library's own list, a folder it was moved into should list it, and the from-content count read should stay cheap.

Known limits and follow-ups

  • The ticket doesn't pin the file's total size, type or chunk count. The ticket's owner may post any chunks into its own recording, as with an agent's upload ticket today. Clips' storage limits still apply. Tightening the ticket is a separate contract change for both upload paths.
  • A Content ticket also works on the agent chunk route. /api/uploads/<id>/chunk checks it with requireRecordingUploadTicketOwner and doesn't take the final-chunk claim, so a cancel racing a final chunk sent there can fail a whole upload. Content never posts there, and only the ticket holder's own upload is affected.
  • A final chunk cut off partway holds the upload for two minutes. Until its claim expires, cancels and repeats of the final chunk answer upload_finishing. Content keeps sending the final chunk for up to 150 s, and a cancel for about 17 s (Save a video uploaded into a Content page as a Clip #8183), so a cancel in that window can end unconfirmed and leave the upload to the reaper.
  • Cancel retries need the page open. They run in the page's tab. A tab closed between them leaves the upload to the reaper.
  • A start that failed after making its recording can't be taken over. Its key answers upload_key_used, and Content starts the file under a new key, which makes a new recording. The failed one stays failed, as any failed start in Clips does.
  • Repeats are rare today. They are reached only when a held file's start got to Clips and its answer was lost, so overlapping repeats, takeovers and upload_starting are defensive.
  • Nothing moves a binding to trashed yet. Chunks, From Content, its page links and playback already treat a binding in Trash as inactive; A6 is what writes trashed.
  • From Content Clips highlight Library in Clips' nav and breadcrumb when one is open, because the player page doesn't know which list it came from.
  • Signed-in readers see signed-out chrome on Clips' /r/ page. This happens for every Clip, not only uploads (pre-existing).
  • Later steps: margin comments on a Clip (A5); Trash for uploaded Clips and their pages, agent tools, the changelog entry and turning the flag on (A6). The changelog entry waits for A6, when the feature turns on.

🤖 Generated with Claude Code

…ts chunks from the paired Content, and list it under From Content instead of Library
…one that fails the schema, and document Content uploads in the Clips sharing skill
…alone, before the auth guard can allow other origins and credentials
@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.

builder-io-integration[bot]

This comment was marked as outdated.

@3mdistal

Copy link
Copy Markdown
Contributor Author

On the review's LOW item (an uploaded Clip moved into a folder disappearing from that folder): fixed in d307746.

The rule that keeps Content uploads out of the list now applies only to Library's own list. A folder someone moved an uploaded Clip into lists it, Library still leaves it out, and From Content still lists it. The same change is in list-recordings and view-screen, and the video-sharing skill says so.

content-clip-uploads.test.ts adds "lists an upload someone moved into a folder in that folder, and still not in Library".

…nished it first, and treat an upload whose page is in Trash as inactive
builder-io-integration[bot]

This comment was marked as outdated.

… made, and leave an upload whose final chunk Clips took to finish when the page cancels
builder-io-integration[bot]

This comment was marked as outdated.

…se a repeat for another file, settle a final chunk against a cancel with one claim on both chunk paths, and ask a repeat that beats its recording to try again
builder-io-integration[bot]

This comment was marked as outdated.

@3mdistal

Copy link
Copy Markdown
Contributor Author

Round 3, covering the two review bodies and the LOW. Heads are #8182 66e6440 and #8183 ea7b351. The other findings in review 5640159007 are answered in their four threads.

Review 5639839501: a final chunk racing a cancel. The upload lease can't settle this, because renewUploadLease is a heartbeat that any chunk request renews, not an exclusive hold. A cancel can't tell from it whether the final chunk is in flight. 528c120 made a Content cancel leave alone any upload whose row had moved past uploading. 66e6440 closes the window before that with a final-chunk claim: the Content chunk route claims the upload before Clips takes the final chunk's bytes, in one compare-and-swap with the upload state. On both chunk paths, whichever comes first wins:

  • A cancel that lands first gets the chunk refused.
  • A cancel that finds the claim leaves the upload to finish.
  • If the claimed chunk then fails, it carries out the cancel itself.

Details are in the threads on content-clip-uploads.ts:358 and abort.post.ts:320.

Review 5640159007, LOW: a repeat that arrives before the first recording exists. Fixed in 66e6440. A repeat whose binding has no recording yet answers 409 upload_starting while the binding is younger than RESERVATION_GRACE_MS (ten minutes). After that it answers upload_key_used, since the first start has evidently failed. Both contracts carry the code, with a comment on it. Content's tab (ea7b351) repeats the same key after 2, 5 and 10 seconds. If the last repeat still answers upload_starting, it shows the existing "The video didn't upload. Try again." message, so there's no new copy.

Tests:

  • In this PR: asks a repeat that lands before the first start's recording exists to try again and refuses a key whose binding never got its recording, once that's stale.
  • In Save a video uploaded into a Content page as a Clip #8183: answers that an earlier start of the same file is still making its Clip, asks again, with the same key, while Clips is still making the first start's Clip, and says the video didn't upload when Clips is still making the first start's Clip after every retry.

Known limits, also listed in the PR body:

  • A Content ticket can also post to the agent chunk route (/api/uploads/<id>/chunk), which doesn't take the claim. Only that ticket's own upload is affected.
  • A final-chunk request that dies partway (its process is killed) leaves its claim behind. Cancels and repeats then answer "finished" until the lease reaper fails the upload.
  • Today's Content repeats a start for one file only from a file held after a storage refusal, through "Check again" or by coming back to the tab. A storage refusal leaves no binding, so the repeat paths are reached only when such a start got to Clips and its answer was lost. Overlapping repeats and upload_starting are defensive rather than common.

builder-io-integration[bot]

This comment was marked as outdated.

…lips-content-upload-clips

# Conflicts:
#	templates/clips/.agents/skills/video-sharing/SKILL.md
…anges a final chunk in flight, a parked upload cancels, and an abandoned claim expires
builder-io-integration[bot]

This comment was marked as outdated.

…ake a live claim over, and keep a failed start's binding for the next repeat to start again with the same recording
builder-io-integration[bot]

This comment was marked as outdated.

@3mdistal

Copy link
Copy Markdown
Contributor Author

On review 5645717485: its four open threads (r4368635610, r4368814315, r4368814319, r4368814322) had one cause. Where an upload stands was spread over the recording row, the upload state and the final chunk's claim, and each path read a different part of it. In 11a4449 one reader, readUploadPhase, answers that question. The repeated start, the cancel, the Content chunk route and the lease reaper all use it, the deferred cancel is gone, and a claim expires after FINAL_CHUNK_CLAIM_TTL_MS. Each thread has a reply with its tests.

@3mdistal

Copy link
Copy Markdown
Contributor Author

On review 5646137579: both findings are fixed in 4c6c367.

  • A final chunk never takes over a live claim. Another send answers upload_finishing while the claim holds, and the first send's success once the upload is parked or finished (r4373079356).
  • A failed start keeps its binding unless the failure confirms nothing was left. A later repeat takes it over with a compare-and-swap and starts it again under the same recording id, so one free clip is held (r4373079359).

Both threads have replies with their tests.

…ft no recording, and that an ended key's file starts again under a new key
builder-io-integration[bot]

This comment was marked as outdated.

…he lease decides between the chunk and the reaper, and drop the reaper's phase check

@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 2 potential issues 🟡

Review Details

Review summary

This update adds a Clips “From Content” list and links, and hardens upload coordination by extending the recording lease before a final-chunk claim. The lease/reaper race from the previous review is fixed: final-chunk claims now renew the lease only while the row is uploading, and the reaper’s conditional update rechecks the current lease atomically. I resolved the prior thread PRRT_kwDORlS_j86wHc8u accordingly.

Risk: High — the PR handles cross-service authorization, private media access, and upload state transitions. The peer-token scope, page binding, ticketed chunk transport, and additive schema changes are generally well separated, and the new tests exercise many important interleavings.

Findings

  • 🟡 MEDIUM: Folder-scoped Library results bypass the active Content-binding filter, so uploads from trashed pages can reappear there.
  • 🟡 MEDIUM: A repeated start treats a live final-chunk claim as finished, although that chunk can still fail; the client may stop retrying before the upload is actually accepted.
  • ℹ️ LOW: A failure to release the final-chunk claim can replace a successful chunk response, causing retries to wait for the claim TTL.

🧪 Browser testing: Skipped — the browser-test-planner agent is unavailable in this environment. The dev server is running, but no browser verification was possible.

Comment on lines +217 to +218
} else if (args.folderId == null) {
whereClauses.push(not(uploadedFromContent()));

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.

🟡 Folder views can expose uploads from trashed Content pages

When folderId is set, this branch skips both upload-binding filters, so a Content upload moved into a Clips folder remains visible after its page binding is trashed. Keep active uploads visible in folders, but exclude upload-origin recordings whose binding is no longer active; view-screen.ts has the same condition.

Fix in Builder

? binding
: uploadStarting();
}
if (standing.phase === "finishing" || standing.phase === "finished") {

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.

🟡 A live final-chunk claim is reported as finished

finishing only means a final chunk has claimed the upload; handleRecordingChunk can still fail, release the claim, and leave the row uploading. Returning upload_already_finished here can lead a repeated-start client to stop retrying before the file is accepted; return a retryable upload_starting response for finishing, and reserve upload_already_finished for finished.

Fix in Builder

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.

1 participant