Integrate mobile sync auth and bookmark sign-in UI by AhmedNMahran · Pull Request #757 · quran/quran-ios

AhmedNMahran · 2026-03-08T00:56:41Z

Summary

This PR integrates mobile-sync-spm into quran-ios behind the QURAN_SYNC compilation flag and adds the minimal UI needed to expose Quran.com sync in the app.

It currently covers:

KMP-backed auth integration through mobile-sync-spm
KMP-backed page bookmark persistence
minimal sign-in/sign-out UI in Settings
a bookmarks sync banner while signed out
local development docs for enabling and testing the flow

Main changes

Sync package integration

updates mobile-sync-spm to 0.0.4
keeps the sync-specific implementation behind QURAN_SYNC
wires the example app target to compile with QURAN_SYNC

Auth integration

AuthenticationClientMobileSyncImpl remains the app-facing auth implementation for sync
MobileSyncSession now uses the mobile-sync-spm 0.0.4 wrapper path
the sync auth path accepts:
- QURAN_OAUTH_CLIENT_ID
- QURAN_OAUTH_ISSUER_URL
- optional QURAN_OAUTH_CLIENT_SECRET
redirect URI and scopes come from the package defaults

Bookmark integration

page bookmarks are persisted through mobile-sync-spm when sync is enabled
the existing Core Data bookmark persistence remains the fallback when sync is disabled

UI

Settings shows Quran.com sign-in/sign-out actions when sync is compiled in
Bookmarks shows a dismissible sync banner while signed out
Notes were intentionally left unchanged because note sync is not wired yet

Tests

adds QuranProfileService coverage for missing-client login/logout behavior

Local setup

Compile flag

Enable QURAN_SYNC for the example app target.

Runtime environment

For the sync path:

QURAN_OAUTH_CLIENT_ID
QURAN_OAUTH_ISSUER_URL

Optional:

QURAN_OAUTH_CLIENT_SECRET

QURAN_OAUTH_CLIENT_SECRET is only needed when the configured OAuth client is registered as a confidential client. If the environment supports a public PKCE client, leave it unset.

Package defaults used by the sync path:

redirect URI: com.quran.oauth://callback
post logout redirect URI: com.quran.oauth://callback
scopes: openid,offline_access,content,user,bookmark,sync,collection,reading_session,preference,note

When sync is not compiled in, the native fallback auth client reads the app-level OAuth configuration:

QURAN_OAUTH_CLIENT_ID
QURAN_OAUTH_ISSUER_URL
QURAN_OAUTH_REDIRECT_URL
QURAN_OAUTH_SCOPES
optional QURAN_OAUTH_CLIENT_SECRET

Manual verification

Enable QURAN_SYNC
Configure the runtime OAuth environment
Sign in from Settings or Bookmarks
Add a page bookmark
Restart the app and verify the bookmark still appears
Remove the bookmark and verify it disappears locally
If another synced client is available, verify the bookmark syncs across devices

Notes

The sync auth path now supports confidential-client token exchange through QURAN_OAUTH_CLIENT_SECRET because that was required to match the currently working demo setup.
If/when the OAuth client is configured as a public PKCE client, the secret can remain unset.

codecov · 2026-03-08T19:11:06Z

Codecov Report

❌ Patch coverage is 29.34473% with 248 lines in your changes missing coverage. Please review.
✅ Project coverage is 36.54%. Comparing base (d9fc366) to head (1e4e99a).
⚠️ Report is 85 commits behind head on main.

Files with missing lines	Patch %	Lines
...thenticationClient/Sources/MobileSyncSession.swift	0.00%	76 Missing ⚠️
Features/BookmarksFeature/BookmarksView.swift	0.00%	56 Missing ⚠️
...t/Sources/AuthenticationClientMobileSyncImpl.swift	0.00%	33 Missing ⚠️
...ce/Sources/MobileSyncPageBookmarkPersistence.swift	0.00%	20 Missing ⚠️
Features/BookmarksFeature/BookmarksViewModel.swift	0.00%	20 Missing ⚠️
...atures/SettingsFeature/SettingsRootViewModel.swift	0.00%	14 Missing ⚠️
Features/SettingsFeature/SettingsRootView.swift	0.00%	12 Missing ⚠️
...tionClient/Sources/AuthentincationClientImpl.swift	0.00%	7 Missing ⚠️
...rofileService/Tests/QuranProfileServiceTests.swift	89.85%	7 Missing ⚠️
Features/BookmarksFeature/BookmarksBuilder.swift	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #757      +/-   ##
==========================================
- Coverage   40.92%   36.54%   -4.39%     
==========================================
  Files         525      560      +35     
  Lines       20880    21505     +625     
==========================================
- Hits         8546     7859     -687     
- Misses      12334    13646    +1312

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mohamede1945

Jazak Allah Khyrn for the implementation.

I've added comments where it's needed, please review them and let me know if there is anything I might have misunderstood.

Also, it would be great to create a doc with some of the details you added in the PR description about how to do local development, etc.

Lastly, the PR seems to lack unit tests, please add tests for the new services created. No need to test UI, just the business logic.

mohamede1945 · 2026-03-08T19:31:55Z

Data/AuthenticationClient/Sources/AuthenticationClientMobileSyncImpl.swift

@@ -0,0 +1,166 @@
+import Foundation
+import UIKit
+#if QURAN_SYNC


While this looks like a good guard to prevent leaking the new code into the app, but in our situation we don't need that. The drawback is that it complicates the implementation. But it's fine to ship with this new code, we just want to guard the UI and simplify this implementation.

mohamede1945 · 2026-03-08T19:40:13Z

Data/AuthenticationClient/Sources/AuthenticationClientMobileSyncImpl.swift

+            }
+
+            do {
+                _ = try await asyncFunction(for: authService.login())


We need a way to hide these kotlin bridges (e.g. asyncFunction) behind the sync APIs. For example, creating a wrapper in the mobile-sync-spm package.

Basically, I don't want clients to import KMPNativeCoroutinesAsync, MobileSync should be enough, IMO.

mohamede1945 · 2026-03-08T19:42:36Z

Data/AuthenticationClient/Sources/AuthenticationClientMobileSyncImpl.swift

+                    authService.getAuthHeaders { headers, error in
+                        if let error {
+                            continuation.resume(throwing: AuthenticationClientError.clientIsNotAuthenticated(error))
+                        } else {
+                            continuation.resume(returning: headers ?? [:])
+                        }
+                    }


This looks like a missed asyncFunction, right?

mohamede1945 · 2026-03-08T19:44:18Z

Data/AuthenticationClient/Sources/AuthentincationClientImpl.swift

        return authenticationState
    }

+    public func logout() async throws {


FYI, we need to delete the previous impl in a separate PR inshaa'Allah.

mohamede1945 · 2026-03-08T19:46:58Z

Package.swift

+let quranSyncCompilationConditions = ProcessInfo.processInfo.environment["QURAN_SYNC_SWIFT_ACTIVE_COMPILATION_CONDITIONS"] ?? ""
+let quranSyncEnabled = ProcessInfo.processInfo.environment["QURAN_SYNC"] == "1"
+    || quranSyncCompilationConditions.split(separator: " ").contains("QURAN_SYNC")
+let quranSyncSettings: [SwiftSetting] = quranSyncEnabled ? [
+    .define("QURAN_SYNC"),
+] : []
+
+let settings = (enforceSwiftConcurrencyChecks ? swiftConcurrencySettings : []) + quranSyncSettings


We shouldn't do this, we should only manipulate the flag from the Xcode project.

mohamede1945 · 2026-03-08T20:49:59Z

Data/PageBookmarkPersistence/Sources/MobileSyncPageBookmarkPersistence.swift

+        private let bookmarksRepository: any BookmarksRepository
+        private let syncService: SyncService?
+        private let legacyContext: NSManagedObjectContext?
+        private let subject = CurrentValueSubject<[PageBookmarkPersistenceModel], Never>([])


We shouldn't have any local mutable state here, the object should be a wrapper around the mobile-sync corresponding object.

We should also make this a Sendable struct instead of a class, please

mohamede1945 · 2026-03-08T20:52:08Z

Data/PageBookmarkPersistence/Sources/MobileSyncPageBookmarkPersistence.swift

+            guard let legacyStack else {
+                preconditionFailure("CoreDataStack is required when Quran sync is disabled")
+            }
+            fallback = CoreDataPageBookmarkPersistence(stack: legacyStack)


The decision to use MobileSyncPageBookmarkPersistence or CoreDataPageBookmarkPersistence should be in the DI layer (i.e. Container.swift) not here.

mohamede1945 · 2026-03-08T20:56:31Z

Data/PageBookmarkPersistence/Sources/MobileSyncPageBookmarkPersistence.swift

+
+        public func insertPageBookmark(_ page: Int) async throws {
+            if let syncService {
+                _ = try await asyncFunction(for: syncService.addBookmark(page: Int32(page)))


We also should make sure to have these asyncfunction wrappers in the spm package please

mohamede1945 · 2026-03-08T20:58:12Z

Data/PageBookmarkPersistence/Sources/MobileSyncPageBookmarkPersistence.swift

+        public func insertPageBookmark(_ page: Int) async throws {
+            if let syncService {
+                _ = try await asyncFunction(for: syncService.addBookmark(page: Int32(page)))
+                syncService.triggerSync()


Should triggerSync be the responsibility of this adapter/wrapper? I would suggest mobile-sync to encapsulate these cases. Mobile app can still do the periodic, coming from the background sync but not data mutation syncs.

@ahmedre what do you think?

mohamede1945 · 2026-03-08T21:00:37Z

Example/QuranEngineApp.xcodeproj/project.pbxproj

 				MARKETING_VERSION = 1.20.3;
 				PRODUCT_BUNDLE_IDENTIFIER = com.quran.QuranEngineApp;
 				PRODUCT_NAME = "$(TARGET_NAME)";
+				SWIFT_ACTIVE_COMPILATION_CONDITIONS = "$(inherited) $(QURAN_SYNC_SWIFT_ACTIVE_COMPILATION_CONDITIONS)";


I don't think we should define another flag QURAN_SYNC_SWIFT_ACTIVE_COMPILATION_CONDITIONS, QURAN_SYNC should be enough

AhmedNMahran · 2026-03-11T05:00:57Z

@mohamede1945 Jazak Allah Khayran for the detailed and instructed change request and comments,
I tried to address the comments based on my understanding,
while working on the pr comments:

updated the mobile-sync environment selection
pumped version 0.0.4 and published to maven central
pumped version 0.0.4 for the mobile-sync-spm
added the app container and syncviewmodel from the demo to use similar logic in quran-ios to move-out the setup headache from the ios client to the spm.
added unit tests.
all other changes that you can see in the pr :D

mohamede1945 · 2026-03-11T18:30:23Z

@AhmedNMahran thanks for the changes! Just one major point:

added the app container and syncviewmodel from the demo to use similar logic in quran-ios to move-out the setup headache from the ios client to the spm.

I don't think I had requested this change, and I'm not in support of it for a few reasons:

I believe the setup should live in the library where it's actually used. This gives each client the flexibility to customize the setup as needed.
I don't think we should rely on or share demo code. If the logic is meant to be shared, it should live in the shared library and be used by both platforms. Otherwise, platform-specific code should remain in the respective platform implementation.
We previously discussed and agreed not to share a ViewModel between platforms.

For these reasons, I'd prefer that we revert this approach and keep the setup within the iOS client for now.

AhmedNMahran · 2026-03-11T22:30:34Z

@AhmedNMahran thanks for the changes! Just one major point:

added the app container and syncviewmodel from the demo to use similar logic in quran-ios to move-out the setup headache from the ios client to the spm.

I don't think I had requested this change, and I'm not in support of it for a few reasons:

I believe the setup should live in the library where it's actually used. This gives each client the flexibility to customize the setup as needed.

I don't think we should rely on or share demo code. If the logic is meant to be shared, it should live in the shared library and be used by both platforms. Otherwise, platform-specific code should remain in the respective platform implementation.

We previously discussed and agreed not to share a ViewModel between platforms.

For these reasons, I'd prefer that we revert this approach and keep the setup within the iOS client for now.

Thank you @mohamede1945 I didn't mean that,
I don't share viewmodel between platforms, I'm using an iOS-specific setup in the quran-sync-spm and initialize it from the iOS client. no shared viewmodel is being used, I am sticking to our previous discussion on the mobile-sync lib that we shouldn't reuse shared viewmodel.

so, the currently used ViewModel is the correct usage, iOS-only, consumed from iOS client.

AhmedNMahran added 2 commits March 8, 2026 02:36

Integrate mobile sync auth and bookmark UI

0249595

Merge origin/main into codex/mobile-sync-spm-ui

b9b7466

AhmedNMahran mentioned this pull request Mar 8, 2026

Integrate mobile sync auth and bookmark sign-in UI #756

Closed

Fix bookmark persistence conformance and formatting

974c7d0

AhmedNMahran marked this pull request as ready for review March 8, 2026 01:49

AhmedNMahran added 2 commits March 8, 2026 20:55

Stabilize flaky package tests

700a2bc

Fix package test compile errors

20c90c7

mohamede1945 requested changes Mar 8, 2026

View reviewed changes

AhmedNMahran added 4 commits March 9, 2026 00:18

Refine mobile sync integration

33158b5

Simplify sync wrapper gating

997cd86

Handle mobile sync auth failures safely

1382001

Update mobile sync docs and auth coverage for 0.0.4

44fb08d

AhmedNMahran requested a review from mohamede1945 March 11, 2026 04:54

Align sync setup docs with current auth config

1e4e99a

Conversation

AhmedNMahran commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Main changes

Sync package integration

Auth integration

Bookmark integration

UI

Tests

Local setup

Compile flag

Runtime environment

Manual verification

Notes

Uh oh!

codecov bot commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mohamede1945 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AhmedNMahran commented Mar 11, 2026

Uh oh!

mohamede1945 commented Mar 11, 2026

Uh oh!

AhmedNMahran commented Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AhmedNMahran commented Mar 8, 2026 •

edited

Loading

codecov bot commented Mar 8, 2026 •

edited

Loading