feat(trailbase-db-collection): add syncMode support with comprehensive test coverage #1006
Conversation
…progressive) with 93%+ coverage - Implement syncMode detection (defaults to eager for backward compatibility) - eager: synchronous full initialFetch on preload (existing behavior) - on-demand: skip initialFetch, expose loadSubset/unloadSubset for query-driven loads - progressive: skip initialFetch, expose loadSubset, run background full sync - Add getSyncMetadata() exposing syncMode and fullSyncComplete status - Implement safe stream reader handling with guarded release and error suppression - Add comprehensive test suite: 37 tests covering all sync modes and edge cases - Test coverage: 93.24% on trailbase.ts with focus on reader lifecycle, pagination, mutations, and metadata - Include clarifying comment explaining direct sync testing approach vs live query integration
🦋 Changeset detectedLatest commit: 7489ab1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
| } | ||
| } | ||
|
|
||
| const unloadSubset = (_opts: any = {}) => { |
There was a problem hiding this comment.
Setting unloadSubset doesn't seem to be required. Leaving it out strikes me as more explicit. Is this needed?
|
|
||
| // If we're in on-demand mode, expose loadSubset/unloadSubset handlers | ||
| if (internalSyncMode === `eager`) { | ||
| return |
There was a problem hiding this comment.
Looking at electric, it seems to do the opposite, i.e. fetch erverything in eager mode:
| if (eventReader) { | ||
| eventReader.cancel() | ||
| eventReader.releaseLock() | ||
| try { |
There was a problem hiding this comment.
Is all the eating of exceptions needed? It makes me a bit nervous to hush any potential issue, is there maybe some more explicit coordination that should be done? Is this what https://github.com/TanStack/db/blob/main/packages/electric-db-collection/src/electric.ts#L917 is for?
|
Thanks for looping me in. I will admit that I haven't followed the latest changes too closely. Progressive syncing looks pretty useful, so absolutely thanks for looking into it. The original integration was very closely modeled after electric's (which seems to have changed a bit), my default instinct is always to first look at what electric is doing. Independently and on a more existential level, I'm not a big user of AI myself. This is probably also more a question for the repo owners, I'm not sure how to best go about the tests. Generally, tests are great (unless they're bad) but they're also a continued maintenance commitment. I can try to review the 5x lines of test code but it does feel a bit like asymmetric warfare (cheap to generate, expensive to review). What do the owners recommend? |
|
I understand the asymmetry argument, and I shared a similar view for a long time. However, I feel like the quality of LLM-assisted development has improved quite substantially, in that it can now meaningfully augment engineering workflows -especially when used by someone who already understands the codebase and can supervise its output (hence I noted that I used an agent but might not be the best person to assess the final output). If you are unsure, I recommend trying it in a VS Codespace sandbox (green Code button, codespaces tab): in the project, use the LLM assistant in the right sidebar (for example using your comments/questions). Hands-on testing is often the fastest way to assess something. In my experience, LLMs are most valuable for semantic search, codebase exploration, routine code-generation tasks, and situations where knowledge with the codebase allows you to steer the model effectively. Obviously, I cannot speak on behalf of the maintainers, but I have noticed several PS: Indeed, it was the agent's idea to base the new implementation on the |
This Agent-fella must be pretty cool 😎 Just to clarify my stance to you and the repo owners (who are the ultimate gatekeepers), I'm not hating on AI. This is not a college math class, i.e. I care about the solution and now how you arrived there. You can probably drop the word "AI" from my previous blurb. This is more an early question on whether we want to both review and maintain yay amount of tests. Depending on their granularity and changes in pipeline (APIs and behavioral), maintaining them may become a prohibitive chore. I definitely wanted to bring it up before going down the rabbit hole of reviewing them 😅 |
2 participants
The 0.5 release mentions sync engines, incl trailbase, to have the latest 0.5 features, that is
syncModeHowever, that wasn't the case.
Hence, I supervised a coding agent in adding this feature. It looks well to me with high test coverage, but @ignatz & co should take a look.
🎯 Changes
feat(trailbase-db-collection): add syncMode support with comprehensive test coverage
✅ Checklist
pnpm test:pr. -> cmd doesnt exist🚀 Release Impact