PageBookamrks – New Synced DB

[mohannad-hassan]

First draft: shows a working example for a GRDB stack for page bookmarks. Tests will be similar to the CoreData variant, except that the GRDB's integration with Combine doesn't guarantee that publishers will signal every time the resources is mutated, instead it might aggregate a few mutations together.

Also, still remains to test the publisher function in DatabaseConnection.

[codecov]

Codecov Report

Attention: Patch coverage is 91.55844% with 13 lines in your changes missing coverage. Please review.

Project coverage is 41.11%. Comparing base (d9fc366) to head (7b5b640).
Report is 42 commits behind head on main.

Files with missing lines	Patch %	Lines
...ce/Sources/GRDBSyncedPageBookmarkPersistence.swift	75.00%	12 Missing ⚠️
...Tests/GRDBSyncedPageBookmarkPersistenceTests.swift	97.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #683      +/-   ##
==========================================
+ Coverage   40.92%   41.11%   +0.18%     
==========================================
  Files         525      541      +16     
  Lines       20880    21838     +958     
==========================================
+ Hits         8546     8978     +432     
- Misses      12334    12860     +526     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mohamede1945]

Amazing work, Mohannad! I like the test cases a lot too. Just a couple of small changes and we should be good to go inshaa'Allah.

[mohamede1945]

For Sync'd bookmarks, I believe page should be the key not the remoteID. Hence, I suggest the removeBookmark to take the page instead of remote Id.

[mohannad-hassan]

Since synched bookmarks reflect the state in the server, I guess it's safer to assume the logic of the backend. The usecase for removing a bookmark from this repository is when we receive a delete event in synchronizing with the server.

There's also another possibility where the app deletes a bookmark locally, but then this will go through the same channel when synchronizing with the server, and we'd retain the remote ID in the local DB as well, but not as a primary key.

[mohamede1945]

I think we should stick to bookmark.page being the primary key to make it explicit that we would never have 2 bookmark entries with the same page.

[mohannad-hassan]

How about we add a unique constraint on page and keep the primary key for remote_id?

This way we can keep the two ideas; signaling that page is unique and that the design of the synced database follows the server's design.

BTW, this will be different in both the local database and the app's model.