Repository navigation
Conversation
…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
|
Here's a visual recap of what changed: Open the full interactive recap |
…Clip in the folder it was moved into
|
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
|
…lips-content-upload-clips
…nished it first, and treat an upload whose page is in Trash as inactive
… made, and leave an upload whose final chunk Clips took to finish when the page cancels
…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
|
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
Details are in the threads on 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 Tests:
Known limits, also listed in the PR body:
|
…lips-content-upload-clips
…lips-content-upload-clips # Conflicts: # templates/clips/.agents/skills/video-sharing/SKILL.md
…lips-content-upload-clips
…anges a final chunk in flight, a parked upload cancels, and an abandoned claim expires
…ake a live claim over, and keep a failed start's binding for the next repeat to start again with the same recording
|
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, |
|
On review 5646137579: both findings are fixed in 4c6c367.
Both threads have replies with their tests. |
…ft no recording, and that an ended key's file starts again under a new key
…he lease decides between the chunk and the reaper, and drop the reaper's phase check
There was a problem hiding this comment.
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.
| } else if (args.folderId == null) { | ||
| whereClauses.push(not(uploadedFromContent())); |
There was a problem hiding this comment.
🟡 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.
| ? binding | ||
| : uploadStarting(); | ||
| } | ||
| if (standing.phase === "finishing" || standing.phase === "finished") { |
There was a problem hiding this comment.
🟡 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.

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
mainonce those merge. Everything new answers only a Content upload token, and Content mints one only while itscontent.clip-embedflag 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 newcontent-clip:uploadscope 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 carryclient_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 arecording_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 withresolveOrgIdForEmail, which only reads. If either is missing, the answer isaccount_requiredand 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-statussays where a new upload would go, as theContentClipStoragethe contract defines:ready(the workspace has Builder.io or custom storage, or a local development Clips keeps chunks in SQL),freewithremaining,limitandmaxClipBytes,used_up,nonewith the free storage's reason, oraccount_required. It is the same decisioncreate-recordingmakes, read without reserving anything.POST /api/content-clips/uploadstakes{ fileName, sizeBytes, mimeType, blockId, uploadKey }(MP4, WebM or MOV; names up to 255 characters; a 4 KiB body at most). It writes the page'suploadbinding 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 codecreate-recording-uploaduses (extracted intostartRecordingUpload), and returns a ticketedchunkUrl. A start Clips can't take is a 409storage_requiredwith the currentstorage, a 413upload_too_largewithmaxBytes, or a 429too_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
uploadKeyper 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: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'screate-recording-uploadstill keeps one ticket.upload_already_finishedwithrecordingIdandstatus, and no ticket.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).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 getupload_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
uploadbinding'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-chunktakes the ticket in its query string. Content posts each chunk with no cookie, no custom headers and noContent-Type, so the request is a CORS "simple request" and the browser sends no preflight. The route answers withAccess-Control-Allow-Originonly forpairedContentOrigin(): the deployment's built-in Content on Clips' own lane, never a directory entry and never the request's ownOrigin. 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 anOPTIONSto this path itself, before the auth guard: 204 withPOSTfor 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 activeuploadbinding. 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 isuploadingorparkedis cancelled.Where an upload stands is one answer.
readUploadPhaseinserver/lib/final-chunk-claim.tsreads 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 isprocessingorready;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
parkedorfinishedinto the upload state, and only then gives the claim up. So a reader that finds no claim finds where the upload went.upload_finishing. Content sends either one again a few seconds later. A live claim is never taken over.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 readsuploadingagain, and the next final chunk or cancel takes it over.waitingForStorage, or 200 withstatus), and Clips takes no bytes. One that finds the uploadendedis refused with a 409. A final chunk that fails gives the claim up, with the upload stilluploading.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 itended, or the reaper skips the row. The reaper reads no phase, soupload-lease.tsis 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, withstatusprocessingorready): Clips had the whole file first. The Clip is left alone.upload_finishing(409): a final chunk holds the upload, and nothing changed.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
uploadbinding. If someone moves one into a folder, that folder lists it, and Library still doesn't. A new From Content view (/from-content,list-recordingsviewfrom-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: theuploadscope, theclient_ipclaim, the accepted video types, theContentClipStoragestates, 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 underserver/routes/api/content-clips/andserver/routes/api/uploads/[recordingId]/(content-chunk.post.ts,content-cancel.post.ts) call them.server/plugins/auth.tslists the four paths as public (each checks its own token or ticket), andserver/middleware/auth.tsanswers the browser routes' preflight before the auth guard.templates/clips/server/routes/api/uploads/[recordingId]/abort.post.ts:handleAbortRecordingUploadcan take the request from a caller that already read it, so the cancel route reuses it. WithonlyWhileUploading, which only the cancel route passes, it changes nothing unless the upload isuploadingorparked.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, plusreissueRecordingUploadfor a repeat.recording-upload-ticket.tsgainsrequireUploadTicketOwnerForTokenfor a ticket that arrives in the query string, andaddRecordingUploadTicket, which keeps a recording's earlier tokens.templates/clips/server/db/schema.tsandserver/plugins/db.ts:upload_key,upload_mime_type,upload_size_bytesandupload_start_failed_atonrecording_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 asverifyContentPeerToken, which checks everything except the recording id, andPRIVATE_HEADERSis exported. Playback and comments still require their token'srecording_idto 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
content.clip-embedis 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.CONTENT_CLIPS_PEER_SECRETfrom Let Content document readers play and comment on Clips uploaded to the document #8090. Without it, the new Content routes answer 503content_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.recording_content_bindings. Migration 94 addsupload_keyand a unique index on (Content app, page, editor, key). Migration 95 addsupload_mime_typeandupload_size_bytes. Migration 96 addsupload_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 (originupload) 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 recordsparkedorfinishedin the upload's existing state. Trashing either, and what happens when its page is trashed, is step A6.Origin(not from a browser) still need the ticket.Verification
At 658a8f1, whose
templates/clipstree is the same in Save a video uploaded into a Content page as a Clip #8183's head, 05ba5a8:content-clip-uploads,upload-lease,recording-upload-ticket, theuploads/[recordingId]route files,list-recordings,view-screenanduse-navigation-state.pnpm typecheckin Clips,oxfmt --checkandoxlinton the changed files are clean. The one lint warning, an unsortedsort()in a From Content test, predates this round.pnpm guardsat 05ba5a8: all 93 checks pass, includingguard:i18n-catalogsandguard:i18n-changed-copy.Each review round's tests were written first:
content-clip-uploads, 3 inabort.postand 1 inrecording-upload-ticketfailed 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.tsis new in this PR. Besides the tests above, it covers:account_requiredfor a missing account and for an account with no workspace, with neither created;storage, and a file larger than the free storage takes refused with 413 before anything is made;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;already_finishedand nevercancelled;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(probeMediaDurationMsqueue tests), which fail the same way onorigin/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
verifyUploaderandhandleContentClipUploadStart: can an upload token do anything other than start one private Clip, owned by the editor and bound to the page Content named?answerContentChunkPreflight: is there any request that getsAccess-Control-Allow-Originfor an origin other than the paired Content, or that allows credentials?handleContentClipCancelandcancelAnswer: can a ticket cancel anything other than its own Content upload while it isuploadingorparked? Can an upload Clips finished ever be answeredcancelled?final-chunk-claim.tsandtakeFinalChunk: can a final chunk and a cancel both win? That would mean a whole file failed, or a Clip finished after its cancel answeredcancelled. 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?repeatedStartandtakeOverStart: 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'screate-recording-uploadshould 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 leavecreate-recording-upload's own behavior unchanged.list-recordings: an upload should never appear in Library's own list, a folder it was moved into should list it, and thefrom-contentcount read should stay cheap.Known limits and follow-ups
/api/uploads/<id>/chunkchecks it withrequireRecordingUploadTicketOwnerand 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.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.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.upload_startingare defensive.trashedyet. Chunks, From Content, its page links and playback already treat a binding in Trash as inactive; A6 is what writestrashed./r/page. This happens for every Clip, not only uploads (pre-existing).🤖 Generated with Claude Code