Feature: Mobile OAuth #90
Conversation
…form-agnostic functions without expect/actual need
…UI .borderedProminent style in AuthView.swift
mohamede1945
left a comment
mohamede1945
left a comment
There was a problem hiding this comment.
Thanks for the PR. Please let me know if you have any questions about my comments.
| return asyncSequence(for: bookmarksRepository.getBookmarksFlow()).map { bookmarks in | ||
| bookmarks.compactMap { $0 as? Bookmark.PageBookmark } | ||
| } |
There was a problem hiding this comment.
Rather than getBookmarksFlow, I would prefer getPageBookmarks. It narrows the query scope to page bookmarks and cast the types in the kmp lib.
There was a problem hiding this comment.
@mohamede1945 I agree with you, but for this specific point, the aim was to fix compilation errors, so the changes made was to add missing functions and rename some variables to compile successfully without touching the logic.
should be done when handling the bookmarks logic insha'Allah.
| // Handle OAuth redirect | ||
| if url.scheme == "com.quran.oauth" && url.host == "callback" { | ||
| // Notify the app about the OAuth callback | ||
| NotificationCenter.default.post( |
There was a problem hiding this comment.
I would not use NotificationCenter here. I think just passing a closure around will be better from type safety and concurrency perspectives.
| /** | ||
| * ViewModel for managing OAuth authentication on iOS. | ||
| * | ||
| * Implements OAuth 2.0 with PKCE (RFC 7636) for secure mobile authentication. | ||
| * Handles: | ||
| * - PKCE code generation (code_verifier and code_challenge) | ||
| * - ASWebAuthenticationSession for secure OAuth consent screen | ||
| * - OAuth redirect callback processing | ||
| * - Token storage in iOS Keychain | ||
| * - Token refresh and validation | ||
| * | ||
| * Uses Combine framework for reactive state management and SwiftUI integration. | ||
| * Follows CLAUDE.md architecture by separating UI state (ObservableObject) from | ||
| * business logic (AuthenticationManager). | ||
| */ |
There was a problem hiding this comment.
I don't think this is what we really want to do here:
- I believe the idea is to use a kmp lib that we can share between iOS & Android rather than implementing the whole logic in iOS.
- In fact, we have this logic implemented in quran-ios repo, so even if we decided not to use the kmp lib, we don't want to rewrite the code we already have.
…thRepositoryImpl.kt
Thanks @mohamede1945 for your valuable comments, please check again now. Jazak Allah Khayran :) |
Based on issue #86