feat(PowerSync): add attachments support - #1616
Conversation
Update main
Update From Upstream
chore: Update from upstream
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a TanStack DB-backed attachment queue with public exports, setup documentation, and tests. Attachments can be saved and deleted inside collection transactions, with examples for watching, syncing, linked-row updates, and cached URIs. ChangesTanStackDB attachment queue
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TanStackDBAttachmentQueue
participant attachmentsCollection
participant updateHook
TanStackDBAttachmentQueue->>attachmentsCollection: mutate attachment record
TanStackDBAttachmentQueue->>updateHook: mutate linked collection row
TanStackDBAttachmentQueue-->>TanStackDBAttachmentQueue: commit or rollback transaction
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/curly-planets-lead.md:
- Around line 1-3: The changeset for `@tanstack/powersync-db-collection` is
labeled patch, but this release introduces a new public export and raises the
`@powersync/common` peer minimum, so update the changeset in curly-planets-lead.md
to at least minor (or the appropriate breaking level per your policy). Keep the
package name the same and change only the release type so the generated release
notes reflect the expanded surface area and peer dependency bump.
In `@docs/collections/powersync-collection.md`:
- Around line 1195-1207: The watcher example in the referenced attachment-ID
flow is fire-and-forgetting onUpdate, which can let overlapping updates resolve
out of order. Update the example around livePhotoIds.stateWhenReady,
livePhotoIds.subscribeChanges, and the onUpdate callback to serialize each async
invocation by awaiting the previous one (for example through a chained promise
or equivalent queue) so the initial state and later change notifications are
processed in order. Also mirror the same awaited pattern in the test helper that
currently reproduces this race.
In `@packages/powersync-db-collection/src/attachments.ts`:
- Around line 79-80: The `addAttachment` flow in `Attachments` writes the blob
with `localStorage.saveFile()` before the transaction, but failures in `insert`,
`updateHook`, or `commit()` leave the file orphaned on disk. Update the
attachment write path to track the just-saved local URI and delete it on every
exception path, including transaction rollback and any thrown hook/commit error,
while keeping successful saves intact. Also add a regression test around
`updateHook` that throws to verify the DB changes roll back and the local file
is cleaned up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f1f73be-698d-4312-886c-aeb36c8b4f13
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.changeset/curly-planets-lead.mddocs/collections/powersync-collection.mdpackages/powersync-db-collection/package.jsonpackages/powersync-db-collection/src/attachments.tspackages/powersync-db-collection/src/index.tspackages/powersync-db-collection/tests/attachments.test.ts
|
@samwillis - could you review this one please? |
|
I ran the exact package suite locally and all 92 tests pass, but I still need a few correctness fixes before merging this:
The earlier review threads are marked resolved, but the current diff still has these behaviors. |
…tanstack-db into feat/powersync-attachments
|
Thanks for the feedback!
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/powersync-db-collection/src/attachments.ts (2)
36-41: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDocument (and consider enforcing) that
updateHookmust be synchronous.
Transaction.mutate's callback wrapping only covers the synchronous portion of the callback:registerTransaction/unregisterTransactionwrapcallback()directly, withunregisterTransactionin afinallythat runs as soon ascallback()returns. An asyncupdateHookthat does work after anawaitruns outside the transaction context, so its mutations would not be committed atomically with the attachment write — contradicting the doc's claim that "any mutations made to other collections are committed atomically with it." This is the exact concern flagged in the PR review (updateHook callbacks must be synchronous or implement an awaited atomic design) and is still not reflected in the docs or type signature.Update the doc to state the requirement explicitly, and apply the same clarification to
DeleteOptions.updateHook.📝 Proposed doc fix
/** - * Called within the same TanStackDB transaction as the attachment write, - * so any mutations made to other collections are committed atomically with it. + * Called synchronously within the same TanStackDB transaction as the attachment write, + * so any mutations made to other collections are committed atomically with it. + * An async callback escapes the transaction boundary: mutations made after an + * `await` inside this hook are not part of this transaction. */ updateHook?: (attachment: AttachmentQueueRow) => void🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/powersync-db-collection/src/attachments.ts` around lines 36 - 41, Update the updateHook documentation in the attachment write options to explicitly require a synchronous callback and warn that asynchronous work after an await is outside the transaction. Apply the same clarification to DeleteOptions.updateHook, while preserving the existing callback type and atomicity description.
73-96: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard against reusing an existing attachment id before writing the file.
The docstring for
id(Line 31) already documents this: reusing an existing id overwrites that attachment's local file, the insert is then rejected, and cleanup deletes the overwritten file — leaving the pre-existing record without a file.save()writes the file at Line 84 before any check againstthis.collection, so this data-loss path is real, not just theoretical, and is easy to prevent.Reject the id up front instead of documenting the footgun.
🛡️ Proposed fix
const resolvedId = id ?? (await this.generateAttachmentId()) + if (id !== undefined && this.collection.get(resolvedId)) { + throw new Error(`Attachment with id ${resolvedId} already exists`) + } const filename = `${resolvedId}.${fileExtension}`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/powersync-db-collection/src/attachments.ts` around lines 73 - 96, Update AttachmentQueue.save to validate an explicitly provided id against this.collection before calling localStorage.saveFile. Reject existing attachment ids with an error and preserve normal id generation and saving for new attachments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/powersync-db-collection/src/attachments.ts`:
- Around line 36-41: Update the updateHook documentation in the attachment write
options to explicitly require a synchronous callback and warn that asynchronous
work after an await is outside the transaction. Apply the same clarification to
DeleteOptions.updateHook, while preserving the existing callback type and
atomicity description.
- Around line 73-96: Update AttachmentQueue.save to validate an explicitly
provided id against this.collection before calling localStorage.saveFile. Reject
existing attachment ids with an error and preserve normal id generation and
saving for new attachments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 47ee24a9-dbe5-456e-9288-09f20abaf25b
📒 Files selected for processing (4)
.changeset/curly-planets-lead.mddocs/collections/powersync-collection.mdpackages/powersync-db-collection/src/attachments.tspackages/powersync-db-collection/tests/attachments.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/curly-planets-lead.md
- docs/collections/powersync-collection.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/powersync-db-collection/src/attachments.ts`:
- Around line 34-43: Update the SaveOptions and DeleteOptions updateHook types
to reject Promise or thenable return values, and add runtime checks around both
save and delete hook invocation to detect a returned thenable and fail before it
can escape the transaction. Add a regression test covering an async updateHook
and verify that mutations after await are not committed outside the attachment
transaction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c16d74c-cec4-4138-94d6-b0f7dde63c58
📒 Files selected for processing (2)
packages/powersync-db-collection/src/attachments.tspackages/powersync-db-collection/tests/attachments.test.ts
More templates
@tanstack/angular-db
@tanstack/browser-db-sqlite-persistence
@tanstack/capacitor-db-sqlite-persistence
@tanstack/cloudflare-durable-objects-db-sqlite-persistence
@tanstack/db
@tanstack/db-ivm
@tanstack/db-sqlite-persistence-core
@tanstack/electric-db-collection
@tanstack/electron-db-sqlite-persistence
@tanstack/expo-db-sqlite-persistence
@tanstack/node-db-sqlite-persistence
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/react-native-db-sqlite-persistence
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/tauri-db-sqlite-persistence
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Hey @tannerlinsley, friendly poke here :) |
|
@Chriztiaan @tannerlinsley I independently reproduced both reported bugs at I recommend holding merge for these two correctness fixes. The original suite passes: 95 runtime tests plus one type test. Eight added boundary cases yield four failures and four passing controls; the native-SDK failure is a causal control for the same upload race, not a fourth distinct bug. The added tests typecheck and lint clean. 1. Reusing an ID before a fresh collection is ready destroys the existing local fileSave an attachment, construct another collection against the same table without waiting for readiness, then save different bytes under the same ID/extension. The collection lookup misses the existing SQL row. The new save rejects, the original SQL row survives, but its original file is gone. The otherwise identical ready-collection case preserves the file and bytes. The guard must establish identity/ownership before a destructive file write. Waiting for collection readiness addresses this particular witness but is not an authoritative duplicate check for stale/partial caches. Failure cleanup must only remove resources owned by the failed operation. An additional boundary probe uses two queue instances sharing the same DB/storage and ID: one save succeeds, the other rejects, and the loser removes the winner's file. This needs either shared-resource ownership/reservation or an explicit, enforced single-owner contract; the existing per-instance mutex does not cover it. 2. Delete during upload is overwritten by SDK completionHold the remote upload; call the TanStack queue's delete with an updateHook that detaches the related row; verify SQLite contains QUEUED_DELETE and a null foreign key; release the upload. Two further real sync passes make zero remote delete calls, leaving the uploaded remote file behind. Delete-before-upload and delete-after-upload controls pass. The native SDK AttachmentQueue alone also reproduces this: saveFile → hold upload → deleteFile → release upload changes SQL state from QUEUED_DELETE (2) to SYNCED (3). The SDK captures a row before I/O and later saves the stale completed row unconditionally. Serializing the individual writes does not preserve a delete committed between them. This belongs at the SDK completion boundary, rather than a second attachment state machine in TanStack. Completion must reconcile with current committed intent and preserve outstanding remote deletion. Watcher/archive behavior also needs to be exercised before choosing the exact merge/update rule. Next step / coordinationWe're starting a bounded lifecycle oracle on top of this PR: independent desired intent and local/remote bytes; real SDK completion SQL; controlled transport timing; cold/warm caches; delete before/during/after upload; rollback and ownership assertions; fixed witnesses plus replayable generated histories. No production changes have been made by this review. Could the SDK maintainers coordinate the completion fix and confirm which SDK line this integration should target alongside #1688? That PR upgrades to v2 and currently conflicts with main. A source check of SDK main For completeness, the earlier concern about serializing successive watched snapshots does not reproduce: a controlled test confirms the SDK context already serializes those callbacks. The synchronous-hook documentation and minor changeset are also present. Executable test-only patch against fc55bee (intentionally RED)Run from diff --git a/packages/powersync-db-collection/tests/attachments.test.ts b/packages/powersync-db-collection/tests/attachments.test.ts
index 193f8fa0c..1cb5833e9 100644
--- a/packages/powersync-db-collection/tests/attachments.test.ts
+++ b/packages/powersync-db-collection/tests/attachments.test.ts
@@ -1,7 +1,9 @@
import { randomUUID } from 'node:crypto'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
+import pDefer from 'p-defer'
import {
+ AttachmentQueue,
AttachmentState,
AttachmentTable,
Schema,
@@ -212,6 +214,260 @@ describePowerSync(`PowerSync AttachmentQueue (TanStackDB)`, () => {
}
describe(`save`, () => {
+ it(`serializes successive watched snapshots through the SDK context`, async () => {
+ const fixture = await setup()
+ let update!: Parameters<WatchAttachments>[0]
+ const queue = fixture.createQueue({
+ archivedCacheLimit: 100,
+ watchAttachments: (callback) => {
+ update = callback
+ },
+ })
+ const record = await queue.save({
+ data: createMockJpegBuffer(),
+ fileExtension: `jpg`,
+ })
+ await queue.syncStorage()
+ await queue.startSync()
+ const entered = pDefer<void>()
+ const release = pDefer<void>()
+ const held = queue.withAttachmentContext(async () => {
+ entered.resolve()
+ await release.promise
+ })
+ await entered.promise
+ const completed: Array<number> = []
+ const first = update([]).then(() => {
+ completed.push(1)
+ })
+ const second = update([{ id: record.id, fileExtension: `jpg` }]).then(
+ () => {
+ completed.push(2)
+ },
+ )
+ try {
+ await Promise.resolve()
+ expect(completed).toEqual([])
+ release.resolve()
+ await Promise.all([held, first, second])
+ expect(completed).toEqual([1, 2])
+ expect(
+ await fixture.db.get(`SELECT state FROM attachments WHERE id = ?`, [
+ record.id,
+ ]),
+ ).toEqual({ state: AttachmentState.SYNCED })
+ } finally {
+ release.resolve()
+ await Promise.all([held, first, second])
+ }
+ })
+
+ it(`preserves the winner file across two queue instances sharing storage`, async () => {
+ const fixture = await setup()
+ const first = fixture.createQueue()
+ const second = fixture.createQueue()
+ const bothWritten = pDefer<void>()
+ const saveFile = fixture.localStorage.saveFile.bind(fixture.localStorage)
+ let writes = 0
+ vi.spyOn(fixture.localStorage, `saveFile`).mockImplementation(
+ async (...args) => {
+ const size = await saveFile(...args)
+ if (++writes === 2) bothWritten.resolve()
+ await bothWritten.promise
+ return size
+ },
+ )
+ const results = await Promise.allSettled([
+ first.save({
+ id: `shared-id`,
+ data: createMockJpegBuffer(),
+ fileExtension: `jpg`,
+ }),
+ second.save({
+ id: `shared-id`,
+ data: new Uint8Array([7, 8, 9]).buffer,
+ fileExtension: `jpg`,
+ }),
+ ])
+ expect(
+ results.filter((result) => result.status === `fulfilled`),
+ ).toHaveLength(1)
+ expect(
+ await fixture.localStorage.fileExists(
+ fixture.localStorage.getLocalUri(`shared-id.jpg`),
+ ),
+ ).toBe(true)
+ })
+
+ it.each([`ready`, `cold`] as const)(
+ `preserves an existing file when reused through a %s collection`,
+ async (phase) => {
+ const fixture = await setup()
+ const original = await fixture.createQueue().save({
+ id: `reused-after-reopen`,
+ data: createMockJpegBuffer(),
+ fileExtension: `jpg`,
+ })
+ const collection =
+ phase === `ready`
+ ? fixture.attachmentsCollection
+ : createCollection(
+ powerSyncCollectionOptions({
+ database: fixture.db,
+ table: APP_SCHEMA.props.attachments,
+ }),
+ )
+ onTestFinished(() => collection.cleanup())
+ const queue = new TanStackDBAttachmentQueue({
+ db: fixture.db,
+ attachmentsCollection: collection,
+ localStorage: fixture.localStorage,
+ remoteStorage: fixture.remoteStorage,
+ watchAttachments: () => {},
+ })
+ onTestFinished(() => queue.stopSync())
+ if (phase === `cold`)
+ expect(collection.get(original.id)).toBeUndefined()
+ await expect(
+ queue.save({
+ id: original.id,
+ data: new Uint8Array([7, 8, 9]).buffer,
+ fileExtension: `jpg`,
+ }),
+ ).rejects.toThrow()
+ expect(
+ await fixture.db.getOptional(
+ `SELECT id FROM attachments WHERE id = ?`,
+ [original.id],
+ ),
+ ).toEqual({ id: original.id })
+ expect(await fixture.localStorage.fileExists(original.local_uri!)).toBe(
+ true,
+ )
+ expect(
+ new Uint8Array(
+ await fixture.localStorage.readFile(original.local_uri!),
+ ),
+ ).toEqual(new Uint8Array(createMockJpegBuffer()))
+ },
+ )
+
+ it.each([`before`, `during`, `after`] as const)(
+ `preserves delete intent %s an SDK upload`,
+ async (timing) => {
+ const fixture = await setup()
+ const queue = fixture.createQueue()
+ const uploaded = pDefer<void>()
+ const release = pDefer<void>()
+ const remoteFiles = new Set<string>()
+ fixture.uploadFile.mockImplementation(async (_, attachment) => {
+ uploaded.resolve()
+ await release.promise
+ remoteFiles.add(attachment.id)
+ })
+ fixture.deleteFile.mockImplementation((attachment) => {
+ remoteFiles.delete(attachment.id)
+ return Promise.resolve()
+ })
+ const userId = randomUUID()
+ const record = await queue.save({
+ data: createMockJpegBuffer(),
+ fileExtension: `jpg`,
+ updateHook: (attachment) => {
+ fixture.usersCollection.insert({
+ id: userId,
+ name: `owner`,
+ email: null,
+ photo_id: attachment.id,
+ })
+ },
+ })
+ let sync: Promise<void> | undefined
+ try {
+ if (timing !== `before`) {
+ sync = queue.syncStorage()
+ await uploaded.promise
+ if (timing === `after`) {
+ release.resolve()
+ await sync
+ }
+ }
+ await queue.delete({
+ id: record.id,
+ updateHook: () => {
+ fixture.usersCollection.update(userId, (row) => {
+ row.photo_id = null
+ })
+ },
+ })
+ expect(
+ await fixture.db.get(`SELECT state FROM attachments WHERE id = ?`, [
+ record.id,
+ ]),
+ ).toEqual({ state: AttachmentState.QUEUED_DELETE })
+ expect(
+ await fixture.db.get(`SELECT photo_id FROM users WHERE id = ?`, [
+ userId,
+ ]),
+ ).toEqual({ photo_id: null })
+ release.resolve()
+ await sync
+ // Complete two real SDK passes, not a timeout waiting for a mock state.
+ await queue.syncStorage()
+ await queue.syncStorage()
+ expect(fixture.deleteFile).toHaveBeenCalledTimes(1)
+ expect(remoteFiles.has(record.id)).toBe(false)
+ expect(await fixture.localStorage.fileExists(record.local_uri!)).toBe(
+ false,
+ )
+ } finally {
+ release.resolve()
+ await sync
+ }
+ },
+ )
+
+ it(`keeps a native SDK delete queued when upload completion arrives`, async () => {
+ const fixture = await setup()
+ const queue = new AttachmentQueue({
+ db: fixture.db,
+ localStorage: fixture.localStorage,
+ remoteStorage: fixture.remoteStorage,
+ watchAttachments: () => {},
+ })
+ const uploaded = pDefer<void>()
+ const release = pDefer<void>()
+ fixture.uploadFile.mockImplementation(async () => {
+ uploaded.resolve()
+ await release.promise
+ })
+ const record = await queue.saveFile({
+ data: createMockJpegBuffer(),
+ fileExtension: `jpg`,
+ })
+ const sync = queue.syncStorage()
+ try {
+ await uploaded.promise
+ await queue.deleteFile({ id: record.id })
+ expect(
+ await fixture.db.get(`SELECT state FROM attachments WHERE id = ?`, [
+ record.id,
+ ]),
+ ).toEqual({ state: AttachmentState.QUEUED_DELETE })
+ release.resolve()
+ await sync
+ expect(
+ await fixture.db.get(`SELECT state FROM attachments WHERE id = ?`, [
+ record.id,
+ ]),
+ ).toEqual({ state: AttachmentState.QUEUED_DELETE })
+ } finally {
+ release.resolve()
+ await sync
+ await queue.stopSync()
+ }
+ })
+
it(`writes the local file and inserts a QUEUED_UPLOAD row into the collection`, async () => {
const { createQueue, attachmentsCollection, localStorage } = await setup()
const queue = createQueue()
|
|
Companion fixes and oracle: powersync-ja#10 (targets your feat/powersync-attachments branch). Tanner’s startup fix informed a targeted-ID live-query lease that also works with on-demand collections. It adds duplicate/file-ownership guards and preserves SDK filename-based storage relocation. Local gate: 121 tests pass, types/lint/build pass. The upload/delete completion race remains an upstream blocker: the companion includes five separately runnable failing native-SDK/integration checks for both success and retry completion, with no SDK workaround or expected-failure waiver. See the companion body and ATTACHMENT-ORACLE.md for commands and exact scope. |
4 participants
🎯 Changes
Derived from Steven's efforts in powersync-ja/powersync-js#983, and addresses #1563.
Problem
PowerSync ships an attachment helper for syncing files (photos, documents) between local and remote storage. It's separate from regular synced tables: a local-only attachments table tracks each file's lifecycle (QUEUED_UPLOAD, SYNCED, QUEUED_DELETE), and an AttachmentQueue drives uploads/downloads in the background.
TanStackDB, on the other hand, gives you an optimistic, reactive, joinable view over synced data. For users who want to use the attachment helper alongside the PowerSync+TanstackDB integration there are blockers. Saving a file (in the local-only attachments table) and associating it with a record (e.g. setting user.photo_id) are two independent writes which could make data races and fatal errors a problem for data consistency.
The original POC (powersync-js#983) proved this integration was viable. This PR productionises a a subset of it as reusable functionality.
Solution
A
TanStackDBAttachmentQueuethat extends the SDK's AttachmentQueue (for saving and deleting a file) and backs it with a TanStack DB collection.The package owns the collection-backed saveFile/delete implementation and leaves the wiring to the application (covered in documentation).
✅ Checklist
pnpm test.🚀 Release Impact
Summary by CodeRabbit