feat(PowerSync): add attachments support - #1616
Conversation
Update main
Update From Upstream
chore: Update from upstream
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 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 covering watching, syncing, detaching references, and reading cached URIs. ChangesTanStackDB attachment queue
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TanStackDBAttachmentQueue
participant withAttachmentContext
participant PowerSyncTransactor
participant attachmentsCollection
participant updateHook
TanStackDBAttachmentQueue->>withAttachmentContext: delete()
withAttachmentContext->>PowerSyncTransactor: open non-auto-committing transaction
PowerSyncTransactor->>attachmentsCollection: load attachment by id
PowerSyncTransactor->>attachmentsCollection: set QUEUED_DELETE and reset has_synced
TanStackDBAttachmentQueue->>updateHook: clear linked foreign key
PowerSyncTransactor-->>TanStackDBAttachmentQueue: commit
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)packages/powersync-db-collection/package.jsonTraceback (most recent call last): 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![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
| --- | ||
| '@tanstack/powersync-db-collection': patch | ||
| --- |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
patch is too small for this release.
This PR adds a new public export and raises the package's @powersync/common peer minimum from ^1.41.0 to ^1.57.0. Shipping that as a patch hides a new surface area and can break consumers that have not upgraded their PowerSync peer yet. Please bump this changeset to at least minor (or whatever level your peer-dependency policy treats as breaking).
🤖 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 @.changeset/curly-planets-lead.md around lines 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.
| // 1. Report the initial set of referenced attachment IDs. | ||
| const initialState = await livePhotoIds.stateWhenReady() | ||
| onUpdate(Array.from(initialState.values()).map(mapper)) | ||
|
|
||
| // 2. Re-emit the whole set on every change (the queue expects the holistic state). | ||
| livePhotoIds.subscribeChanges(() => { | ||
| onUpdate(livePhotoIds.map(mapper)) | ||
| }) | ||
|
|
||
| // 3. Clean up when sync stops. | ||
| abortSignal.addEventListener("abort", () => livePhotoIds.cleanup(), { | ||
| once: true, | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Serialize the async onUpdate calls in this example.
The watcher example fire-and-forgets onUpdate for both the initial state and subsequent changes. If two updates arrive close together, those promises can resolve out of order and stale attachment sets can win. Please show an awaited/serialized pattern here; the test helper currently mirrors the same race.
🤖 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 `@docs/collections/powersync-collection.md` around lines 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.
| const localUri = this.localStorage.getLocalUri(filename) | ||
| const size = await this.localStorage.saveFile(localUri, data) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Roll back the saved file when the transaction fails.
saveFile() runs before the collection transaction starts. If insert, updateHook, or commit() throws, the DB work is rolled back but the blob stays on disk, so this API is no longer atomic and leaks orphaned local files. Please clean up the just-written file on every failure path and add a regression test that throws from updateHook.
Also applies to: 99-116
🤖 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 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.
|
@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. |
3 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