fix: make media uploads reliable across storage backends - #2273
Conversation
🦋 Changeset detectedLatest commit: ecdf0b2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | ecdf0b2 | Jul 30 2026, 11:21 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | ecdf0b2 | Jul 30 2026, 11:20 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | ecdf0b2 | Jul 30 2026, 11:21 AM |
Scope checkThis PR changes 2,626 lines across 32 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | ecdf0b2 | Jul 30 2026, 11:21 AM |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
There was a problem hiding this comment.
This is the right change for the stated problem. Uploading to storage backends that cannot produce signed URLs — especially the native Cloudflare R2 binding — needs a same-origin streaming fallback, and the PR adds that path without breaking the S3-style signed-URL flow. The atomic publish-via-upload-attempt table, the cleanup ordering (DB row deleted before storage object), and the size/MIME/hash guards are all sound and well covered by tests.
I reviewed the full diff, the changed routes, the repository/Clean-up code, the admin client, the migration, the OpenAPI updates, and the regression tests. No blocking logic bug, regression, security issue, or AGENTS.md convention violation stood out. The only things I noticed are two comment blocks that drift into PR-style narrative rather than code explanations — worth tightening before merge, but not blockers.
Findings
-
[suggestion]
packages/core/src/astro/routes/api/media/[id]/confirm.ts:187-194This comment block narrates the PR’s rationale ("the signed-URL flow uploads directly to storage, so this confirm is the only point at which the server sees the bytes", "buffering a large original into a Worker heap to compute a 32px blurhash would OOM"). It repeats design trade-offs that belong in the PR description rather than the code. Per AGENTS.md, comments should explain non-obvious invariants, not re-state the change proposal.
// LQIP is best-effort; oversized images skip server-side placeholders. -
[suggestion]
packages/admin/src/lib/api/media.ts:16-19The constant name already says what it is; the surrounding JSDoc justifies it by pointing at a remote schema and asks readers to "keep in sync". Cross-file notes that require manual synchronization age quickly and violate the comment-discipline rule against justification/narrative. If both sides need the same limit, export it from a shared package; otherwise the name is enough.
export const MEDIA_SEARCH_MAX_LENGTH = 200;
There was a problem hiding this comment.
This is the right change for the stated problem and the latest revision is clean. The same-origin streaming fallback lets native R2/local storage work without signed URLs, while the S3 signed-URL flow is preserved. The atomic publishPendingStorageKey update, the upload-attempt cleanup table, and the delete-before-storage cleanup ordering all prevent the races this PR targets. MIME-type/size guards on deduplication, optional client hashing, and the 8 MiB LQIP cap are sensible.
I re-read the full diff plus the changed routes, repository, cleanup code, admin client, migration, OpenAPI document, and regression tests. Both comment-style findings from the previous review are fixed: confirm.ts now has only the concise LQIP invariant, and the stale cross-file JSDoc on MEDIA_SEARCH_MAX_LENGTH is gone.
No new blocking logic bug, regression, security issue, or AGENTS.md convention violation stands out. The OpenAPI schema for POST /media/upload-url correctly models the union of upload-target and existing-media responses, the migration indexes follow naming conventions, authorization uses the existing media:upload/media:edit_any permissions, and the extensive concurrency/race test coverage exercises the publication, retry, cleanup, and deletion paths.
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
b9a022a to
ecdf0b2
Compare
There was a problem hiding this comment.
Re-review: no new blockers
This is the right change for the stated problem. The same-origin streaming fallback lets native R2/local storage work without signed URLs, while the S3 signed-URL flow is preserved. The atomic publishPendingStorageKey update, the upload-attempt cleanup table, and the delete-before-storage cleanup ordering all prevent the races this PR targets. MIME-type/size guards on deduplication, optional client hashing, and the 8 MiB LQIP cap remain sensible.
I re-read the full diff plus the changed routes, repository, cleanup code, admin client, migration, OpenAPI document, and regression tests. Both comment-style findings from the previous review are still fixed: confirm.ts has only the concise LQIP invariant, and the stale cross-file JSDoc on MEDIA_SEARCH_MAX_LENGTH is gone.
No new blocking logic bug, regression, security issue, or AGENTS.md convention violation stands out in this revision. The OpenAPI schema for POST /media/upload-url continues to model the union of upload-target and existing-media responses correctly, migration indexes follow naming conventions, authorization uses the existing media:upload/media:edit_any permissions, and the concurrency/race test coverage exercises the publication, retry, cleanup, and deletion paths.
LGTM.
What does this PR do?
Fixes media uploads for storage backends, including native Cloudflare R2 bindings, that cannot generate signed upload URLs. The upload negotiation endpoint now returns an authenticated same-origin streaming endpoint in that case, while preserving direct signed uploads for S3-compatible storage.
The upload lifecycle is hardened end to end:
Includes the additive upload-attempt migration, OpenAPI/schema updates, release notes, architecture documentation, and regression coverage for the admin client, upload routes, storage, SQLite, PostgreSQL, and D1 behavior.
Closes #2256
Closes #2265
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain. (N/A: no new user-visible admin strings.)AI-generated code disclosure
Screenshots / test output
No visual UI changes.
/var/...versus/private/var/....Try this PR
Open a fresh playground →
A full working EmDash site, deployed from this branch. Each visit gets its own session-scoped sandbox: no login needed and no shared state. Try the admin, edit content, hit the public site.
Tracks
fix/media-upload-2265. Updated automatically when the playground redeploys.