Repository navigation
feat(sheets)!: upload cell media through the tenant storage endpoint - #59
Conversation
- SheetsConfig.storage routes upload discovery, the presigned upload and
preview lookups to the storage endpoint; cell patches stay on the data
endpoint. Without storage or an injected uploader, media cells are
read-only with a tooltip.
- Uploads omit bucketKey and request the public default (isPublic: true)
unless the host sets upload.bucketKey. Discovery prefers uploadAppFile
and requires the file projection.
- Image/upload cells store the file projection {id, key, bucket_id, mime,
size, filename, url?}; previews resolve signed downloadUrls in batches
and refresh before expiry. Attachment cells store the public URL, or
fail with a clear cause when the bucket has no public URL prefix.
- Failed PUTs no longer delete the files row (the server expires it).
- Media writes use the table's real key and inflection; draft rows create
once and retry against the created row; clear persists null.
- Remove the unused upload editors and useSheetsImageUpload; update docs.
BREAKING CHANGE: SheetsUploadResult is now { file: SheetsFileValue; url?: string }
instead of { url, ...metadata }, so injected executeUpload functions must return
the file projection. executeFieldUpload takes (target, file, transport).
useSheetsImageUpload is removed; use useSheetsFieldUpload.
Editor overlays grow from the anchor cell's corner (0.97 to 1, 250ms) and fade out to 0.99 over 150ms on close, reading the host's --dropdown-* tokens with the transitions.dev defaults. The close plays on an inert copy of the last frame; reduced motion skips both. Placement measures layout size and re-clamps when the editor resizes.
|
Review complete. 🟡 2 medium 💬 Inline comments (2)
This PR moves sheet media handling to a presigned-upload flow: capability discovery against a storage endpoint, a
Reviewed commit: a91d6f8 |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reworks the sheets media pipeline around a dedicated presigned-upload seam with batched signed-URL previews, a new attachment cell type, and registry/docs updates.
Key findings
- 🟡 Null signed-URL results refetch on every remount and focus — use-sheets-media-url.ts:80
- 🟡 Attachment upload on private bucket leaves orphan file row — use-sheets-upload.ts:71
| if (isAttachment) { | ||
| if (!projection.url) throw new Error('attachment columns need a public bucket with a public URL prefix'); | ||
| value = projection.url; |
There was a problem hiding this comment.
🟡 bug · medium
Attachment upload on private bucket leaves orphan file row
For attachment columns, executeFieldUpload runs the full upload first — the server creates the file row and the PUT stores the bytes — and only afterwards throws when projection.url is missing (packages/sheets/src/hooks/use-sheets-upload.ts:72). On a private bucket with no public URL prefix every attachment upload therefore succeeds at the storage layer and then fails, leaving an orphaned file row on each attempt, since the post-upload error path performs no cleanup.
📋 Prompt for AI Agents
In packages/sheets/src/hooks/use-sheets-upload.ts around lines 63-80, the attachment public-URL check (if (!projection.url) throw ...) at line 72 runs after await executeUpload(...) has already created the server file row and completed the PUT, so on private buckets every attempt orphans a file row. Move the gate ahead of the upload: before calling executeUpload, determine whether the target column/bucket yields a public url (e.g. via discovered storage capability or config) and throw the 'attachment columns need a public bucket' error before any server row is created; keep the post-upload check as a defensive assertion only.
There was a problem hiding this comment.
Not changing this one. Sheets always uploads with isPublic: true and no bucketKey, so files land in default-public. A missing url there means the server's public URL prefix isn't configured, and the client can't learn that before it uploads. Deleting file rows from the client is out of scope for this phase. Superseded or orphaned rows are left to the server-side sweep planned for the backend follow-up, so the post-upload error stays as the guard.
There was a problem hiding this comment.
Understood — that's a deliberate design decision rather than an oversight. The reasoning makes sense: since uploads always target the default-public bucket, a missing url reflects a server misconfiguration that the client cannot detect pre-flight, and server-side cleanup is the planned remediation path. The post-upload check remains a valid defensive guard in that model.
I'll leave this open since the original concern (orphaned file rows on failure) still exists structurally, but I accept that the fix is deferred to a backend sweep rather than a client-side pre-flight gate.
- Cache unresolved signed-URL lookups for a minute so remounts and focus don't resend them. - Resolve download URLs on storage endpoints without an upload mutation. - Refetch the host grid whenever a draft row was persisted, even when the created row has no resolvable identity. - Expose upload progress as a progressbar and announce the busy status.
SheetsConfig.storage: { endpoint, execute? }sends upload discovery, the presigned upload and preview lookups to the tenant storage endpoint. Cell patches stay on the data endpoint. Without storage or an injected uploader, media cells are read-only and show a tooltip.bucketKeyand sendisPublic: true, so files land in the tenant'sdefault-publicbucket.upload.bucketKeyremains an explicit override. Discovery prefersuploadAppFileand requires thefileprojection.{ id, key, bucket_id, mime, size, filename, url? }. Previews resolve signeddownloadUrls in batches of up to 100 and refresh at about 50 min. Attachment cells store the public URL, or report that the bucket needs a public URL prefix.null.prepackbuilds for data, sheets and blocks-ui.Breaking:
SheetsUploadResultis now{ file: SheetsFileValue; url?: string }, so injectedexecuteUploadfunctions must return the file projection.executeFieldUpload(target, file, transport)has a new signature.useSheetsImageUploadis removed; useuseSheetsFieldUpload.Before/after screenshots to follow.
🤖 Generated with Claude Code
https://claude.ai/code/session_017WbsG7JQsKEMKNgVsc4psk