Skip to content

Comments

Feature: Sync Mutations (Bookmarks, Collections, Collection Bookmarks, Notes)#105

Merged
AhmedNMahran merged 31 commits intomainfrom
feat/sync
Feb 21, 2026
Merged

Feature: Sync Mutations (Bookmarks, Collections, Collection Bookmarks, Notes)#105
AhmedNMahran merged 31 commits intomainfrom
feat/sync

Conversation

@AhmedNMahran
Copy link
Collaborator

@AhmedNMahran AhmedNMahran commented Feb 3, 2026

Related to Issue #87

Summary

This PR improves the synchronization engine by making it fully authentication-aware and refactoring the pipeline architecture for better maintainability. The core synchronization lifecycle is now tightly coupled with the user's authentication state via a reactive observer. Additionally, a new factory-based initialization system has been introduced to simplify the integration of synchronization features across Android and iOS platforms, eliminating manual object wiring in the UI layer.

Key Changes

1. Auth-Aware Synchronization Engine

  • Safety Guards: The SynchronizationClient now verifies authentication status via AuthenticationDataFetcher.isLoggedIn() before any sync operation begins. Triggers received while logged out are safely ignored.
  • Lifecycle Integration: Added triggerSyncImmediately() and cancelSyncing() to the sync client. These are automatically invoked by the service layer when authentication states change (Success vs Idle/Error).
  • Immediate Sync: Sync operations now launch with zero delay upon login, followed by the standard periodic intervals.

2. Enhanced Scheduler Logic

  • Cancellable Sessions: Implemented Scheduler.cancel() which halts pending jobs and resets the state to Idle. This allows the scheduler to be cleanly resumed when a user re-authenticates without stopping the underlying CoroutineScope.
  • Buffer Management: Buffered triggers are cleared on failure or cancellation to prevent stale synchronization attempts.

3. Factory-Based Initialization

  • SyncPipelineFactory: Introduced a centralized factory in the sync-pipelines module to encapsulate the complex setup of databases, repositories, and synchronization engines.
  • Boilerplate Removal: Significantly reduced the code required in platform activities and managers. The manual manual instantiation of 5+ repositories and the sync pipeline is now replaced by a single factory call.
  • Platform Support: Optimized for usage in both Kotlin (Android) and Swift (iOS), including specific convenience overloads for Swift interoperability.

4. Bookmark, Collection, Collection Bookmark, and Notes Actions Integration

  • Sync operations are now fully integrated into the authentication-aware pipeline.
  • Mutations are queued and processed only when the user is authenticated.
  • The factory pattern extends repository initialization for cleaner code organization.

Technical Details

  • Module: syncengine
    • Updated SynchronizationClient and Scheduler.
  • Module: sync-pipelines
    • New File: SyncPipelineFactory.kt (Central entry point for feature initialization).
    • Added SyncService and SyncViewModel.
  • Demo Apps:
    • Android: Updated MainActivity.kt to use SyncPipelineFactory.
    • iOS: Updated DatabaseManager.swift to utilize the factory and reflect class renames.

Testing

  • Integration Tests: Updated SynchronizationClientIntegrationTest.kt to verify that sync operations are blocked when logged out.

- rename classes
- update models to fix serialization errors
- uncomment sync adapters in SynchronizationClient.kt
…e triggering sync

- add `triggerSyncImmediately` and `cancelSyncing` to `SynchronizationClient` and `Scheduler`
- observe authentication state in `SyncService` to trigger or cancel sync based on login status
- add `isLoggedIn` to `AuthenticationDataFetcher` interface and implementations
- rename `MainSyncViewModel` to `SyncViewModel` in iOS and Kotlin
- integrate authentication actions (`login`, `logout`, `clearError`) directly into `SyncViewModel`
- remove `AuthViewModel` dependency from `SyncViewModel` and replace it with `AuthService`
- update Android and iOS demo apps to use the updated `SyncViewModel` and `SyncService` classes
@AhmedNMahran AhmedNMahran self-assigned this Feb 3, 2026
AhmedNMahran and others added 5 commits February 7, 2026 18:46
- delete bookmark (WIP)
- add ayah bookmark
- delete bookmark
- fix updating local bookmarks when receiving deleted mutation
- note
- update ui
@ahmedre
Copy link
Contributor

ahmedre commented Feb 8, 2026

ran this through a code-review from Codex, and it said:

  - [P0] Fix sync-pipelines JVM target depending on :auth without JVM variant — /Users/
    ahmedre/Developer/kmp/pr-sync/sync-pipelines/build.gradle.kts:16-36
    sync-pipelines/build.gradle.kts declares jvm() but commonMain.dependencies adds
    api(projects.auth), and :auth does not publish a jvm variant (only androidJvm +
    native). This makes :sync-pipelines:compileKotlinJvm (and any build that includes the
    JVM target) fail with a “No matching variant of project :auth was found” resolution
    error.
  - [P0] Implement LocalDataFetcher.fetchLocalModel in BookmarksSyncAdapterTest — /Users/
    ahmedre/Developer/kmp/pr-sync/syncengine/src/commonTest/kotlin/com/quran/shared/
    syncengine/BookmarksSyncAdapterTest.kt:31-37
    LocalDataFetcher gained the new abstract method fetchLocalModel(remoteID: String), but
    the anonymous LocalDataFetcher<SyncBookmark> in BookmarksSyncAdapterTest.kt does not
    implement it. This causes test compilation to fail
    (e.g., :syncengine:compileTestKotlinJvm).
  - [P0] Implement LocalDataFetcher.fetchLocalModel in
    SynchronizationClientIntegrationTest — /Users/ahmedre/Developer/kmp/pr-sync/
    syncengine/src/commonTest/kotlin/com/quran/shared/syncengine/
    SynchronizationClientIntegrationTest.kt:65-75
    The LocalDataFetcher<SyncBookmark> test double returned by createLocalMutationsFetcher
    doesn’t implement the new fetchLocalModel(remoteID: String) method, so syncengine/src/
    commonTest/.../SynchronizationClientIntegrationTest.kt no longer compiles for test
    targets.
  - [P3] Avoid detached Tasks in iOS SyncViewModel.observeData to prevent leaks — /Users/
    ahmedre/Developer/kmp/pr-sync/demo/apple/QuranSyncDemo/QuranSyncDemo/
    SyncViewModel.swift:25-46
    demo/apple/.../SyncViewModel.swift’s observeData() spawns two independent Task { ... }
    loops and then returns; these tasks are not tied to the caller’s SwiftUI .task
    cancellation and can keep running after the view disappears (also authTask/
    bookmarksTask are unused locals). This can lead to background work continuing
    unexpectedly and keeping self alive longer than intended.

@ahmedre
Copy link
Contributor

ahmedre commented Feb 8, 2026

side note - just merged #107 to fix tests on main so might want to rebase on that one

AhmedNMahran and others added 4 commits February 9, 2026 23:55
…d their bookmarks for UI

- update `SyncService` and `SyncViewModel` to provide a flow of `CollectionWithBookmarks`
- improve `CollectionBookmarksSyncAdapter` to map unknown bookmark types using local data
- implement `dirty` flag in `notes` table to better track unsynced changes
- refactor `SyncViewModel` in both Android and iOS demo apps to handle async/await operations and improve data observation
- add functionality to add random bookmarks directly to a specific collection in demo apps
- update `NotesRepositoryImpl` to fetch unsynced notes based on the new `dirty` flag instead of just timestamps
- refine `fetchLocalModel` in `CollectionBookmarksRepositoryDataFetcher` to return existing bookmarks by remote ID
the added `service.clear()` when cleared on both platforms
- improve note creation in Android and iOS demo apps to use random suras and ayahs instead of hardcoded dummy values
@AhmedNMahran AhmedNMahran changed the title Feature: Sync Bookmarks Feature: Sync Mutations (Bookmarks, Collections, Collection Bookmarks, Notes) Feb 14, 2026
AhmedNMahran and others added 2 commits February 14, 2026 22:21
- configure `oidcRedirectScheme` manifest placeholder in `sync-pipelines` and `auth` modules
AhmedNMahran and others added 4 commits February 14, 2026 22:39
Fix build errors
- add jvm target and ktor-client-okhttp dependency to auth module
- configure oidcRedirectScheme manifest placeholder in sync-pipelines and auth modules
…ions

- use suspend functions in `SyncViewModel.kt` instead of internal `viewmodelScope`
Sync Feature PR comments

refactor iOS architecture and remove unnecessary classes and functions

use suspend functions in SyncViewModel.kt instead of internal viewmodelScope
- Move `SyncViewModel` from the `sync-pipelines` module to the respective demo app modules (`demo/android` and `demo/apple`).
- Remove `SyncViewModel` creation logic from `SyncPipelineFactory` to decouple the factory from the UI layer.
- Update Android and iOS demo apps to instantiate `SyncViewModel` directly.
- Rename the `service` property to `syncService` in the Swift `SyncViewModel` for clarity.
Replaced dedicated observer methods with inlined loops in observeData.
used @mainactor [weak self] for task group additions.
Added guard let self = self else { break } inside for await loops to ensure background tasks terminate when the ViewModel is deallocated.
Copy link
Collaborator

@mohamede1945 mohamede1945 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving swift changes. Jazak Allah khyrn Ahmed

Copy link
Contributor

@ahmedre ahmedre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excellent work masha'Allah - some nits but please feel free to merge afterwards - jazakumAllah khairan!

@AhmedNMahran
Copy link
Collaborator Author

@ahmedre @mohamede1945 Jazakum Allah Khayran for the valuable comments. all nits are now resolved alhamdulillah, for the DI, will create an issue to track insha'Allah

@AhmedNMahran AhmedNMahran merged commit 5bbf94b into main Feb 21, 2026
1 check passed
@AhmedNMahran AhmedNMahran deleted the feat/sync branch February 21, 2026 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants