feat(PowerSync): add attachments support - #1616
Open
Chriztiaan wants to merge 18 commits into
Open
Conversation
Update main
Update From Upstream
chore: Update from upstream
Contributor
📝 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 |
Chriztiaan
marked this pull request as ready for review
June 25, 2026 07:45
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Contributor
Author
|
@samwillis - could you review this one please? |
Member
|
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 tasks
…tanstack-db into feat/powersync-attachments
Contributor
Author
|
Thanks for the feedback!
|
Contributor
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
Contributor
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 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