-
Notifications
You must be signed in to change notification settings - Fork 3
Heal 92 migrate document preview #1025
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: HEAL-3-payment-review-refactor
Are you sure you want to change the base?
Heal 92 migrate document preview #1025
Conversation
…anage the changes on SwiftUI HEAL-92
… the `UIScrollView` capabilities to SwiftUI Add `GiniCarouselView` to show the pages of the document. Add `PaymentReviewContentView` to show the main content on the screen Add `PaymentReviewV2ViewController` to migrate the existing `PaymentReviewViewController` to SwiftUI Add `GiniZoomableImageView` to display the pages of the documents. HEAL-92
…SwiftUI` screen Add `fetchImages()` using `async` to be used in `SwiftUI` Remove unneeded `NSObject` inheritance. HEAL-92
…olor` Add `Font+Extensions` file to add a `Font` initializer from `UIFont` Add method to `FontProvider` to return a `SwiftUI Font` from a `TextStyle` HEAL-92
…ealthColors` to retrieve a `SwiftUI Color` HEAL-92
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates the document preview functionality from UIKit to SwiftUI and updates the minimum iOS deployment target from 13.0 to 14.0 across multiple packages. The migration introduces new SwiftUI views for displaying zoomable image carousels and adds utility methods to support SwiftUI color and font conversions.
Key Changes:
- Updated minimum iOS deployment target from 13.0 to 14.0 in Package.swift files and Xcode project settings
- Added SwiftUI extensions for GiniColor and FontProvider to enable seamless UIKit-to-SwiftUI conversions
- Implemented new SwiftUI-based document preview system with zoomable images and carousel functionality
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
HealthSDK/GiniHealthSDKExample/GiniHealthSDKExample.xcodeproj/project.pbxproj |
Updated iOS deployment target from 13.0 to 14.0 in project configurations |
HealthSDK/GiniHealthSDK/Sources/GiniHealthSDK/Core/GiniHealthColors.swift |
Added SwiftUI Color conversion method (has critical bug) and explicit type annotations |
HealthSDK/GiniHealthSDK/Package.swift |
Updated minimum platform version to iOS 14.0 |
GiniComponents/GiniUtilites/Sources/GiniUtilites/Font/FontProvider.swift |
Added SwiftUI Font conversion method (has critical bug) |
GiniComponents/GiniUtilites/Sources/GiniUtilites/Font/Font+Extensions.swift |
New file providing UIFont to SwiftUI Font conversion |
GiniComponents/GiniUtilites/Sources/GiniUtilites/Color/GiniColor.swift |
Added SwiftUI Color conversion method |
GiniComponents/GiniInternalPaymentSDK/Sources/GiniInternalPaymentSDK/PaymentReviewV2/Views/PaymentReviewContentView.swift |
New SwiftUI view for payment review with loading state and image carousel |
GiniComponents/GiniInternalPaymentSDK/Sources/GiniInternalPaymentSDK/PaymentReviewV2/Views/GiniZoomableScrollView.swift |
UIKit scroll view with zoom functionality for images |
GiniComponents/GiniInternalPaymentSDK/Sources/GiniInternalPaymentSDK/PaymentReviewV2/Views/GiniZoomableImageView.swift |
SwiftUI wrapper for zoomable image display |
GiniComponents/GiniInternalPaymentSDK/Sources/GiniInternalPaymentSDK/PaymentReviewV2/Views/GiniCarouselView.swift |
SwiftUI carousel view for paging through multiple images |
GiniComponents/GiniInternalPaymentSDK/Sources/GiniInternalPaymentSDK/PaymentReviewV2/PaymentReviewObservableModel.swift |
Observable model bridging UIKit PaymentReviewModel to SwiftUI |
GiniComponents/GiniInternalPaymentSDK/Sources/GiniInternalPaymentSDK/PaymentReviewV2/PaymentReivewV2ViewController.swift |
UIHostingController for SwiftUI payment review (filename has typo) |
GiniComponents/GiniInternalPaymentSDK/Sources/GiniInternalPaymentSDK/PaymentReview/PaymentReviewModel.swift |
Removed NSObject inheritance, added async fetchImages method, exposed cellViewModels |
GiniComponents/GiniInternalPaymentSDK/Package.swift |
Updated minimum platform version to iOS 14.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var selectedPaymentProvider: GiniHealthAPILibrary.PaymentProvider | ||
|
|
||
| private var cellViewModels: [PageCollectionCellViewModel] = [PageCollectionCellViewModel]() { | ||
| var cellViewModels: [PageCollectionCellViewModel] = [PageCollectionCellViewModel]() { |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the visibility of cellViewModels from private to internal exposes implementation details. Consider if this is intentional for the SwiftUI integration. If external access is needed, consider providing a computed property or a dedicated getter method to maintain better encapsulation.
| } | ||
| } | ||
| } | ||
|
|
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new async version of fetchImages() lacks documentation. Since there are now two methods with the same name but different signatures (one synchronous, one async), it's important to document when each should be used and the differences in their behavior. Add a doc comment explaining that this is the async/await version for SwiftUI integration.
| /// Async/await variant of ``fetchImages()`` tailored for SwiftUI and other async contexts. | |
| /// | |
| /// Use this version from SwiftUI (e.g. inside a `Task` or `.task` modifier) or when you are | |
| /// already in an async context and want structured concurrency instead of completion handlers. | |
| /// The synchronous ``fetchImages()`` overload should be used from legacy, non-async code. | |
| /// | |
| /// This method: | |
| /// - Loads all page previews concurrently using a task group. | |
| /// - Updates ``isImagesLoading`` and `cellViewModels` on the main actor. | |
| /// - Invokes ``onPreviewImagesFetched`` once all previews have been processed. |
| return traitCollection.userInterfaceStyle == .dark ? self.darkModeColor : self.lightModeColor | ||
| } | ||
| } | ||
|
|
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This public method lacks documentation. Add a doc comment explaining that this method converts the GiniColor to a SwiftUI Color, maintaining the light/dark mode support.
| /** | |
| Converts the `GiniColor` to a SwiftUI `Color`, maintaining the light/dark mode support | |
| based on the current user interface style. | |
| - returns: A SwiftUI `Color` that adapts to light and dark mode. | |
| */ |
| func preferredColor() -> Color { | ||
| Color(preferredColor()) | ||
| } |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method creates infinite recursion. The method preferredColor() returning Color calls itself recursively instead of calling the preferredColor() method that returns UIColor. This will cause a stack overflow at runtime. The method should call the UIColor version first and then convert it.
| return textStyleFonts[textStyle] ?? UIFont.systemFont(ofSize: 17) | ||
| } | ||
|
|
||
| public func font(for textStyle: UIFont.TextStyle) -> Font { |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method has an ambiguous signature. There are now two methods named font(for textStyle: UIFont.TextStyle) with different return types (UIFont and Font). This creates an ambiguity and the SwiftUI Font version will recursively call itself, causing infinite recursion. Consider renaming this method to something like swiftUIFont(for:) or using a different approach to disambiguate the methods.
| public func font(for textStyle: UIFont.TextStyle) -> Font { | |
| /** | |
| Retrieves the SwiftUI `Font` associated with a specific text style. | |
| - parameter textStyle: The text style for which to retrieve the SwiftUI font. | |
| - returns: A `SwiftUI.Font` created from the associated `UIFont`. | |
| */ | |
| public func swiftUIFont(for textStyle: UIFont.TextStyle) -> Font { |
| View model class for review screen | ||
| */ | ||
| public class PaymentReviewModel: NSObject { | ||
| public class PaymentReviewModel { |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the NSObject inheritance might be a breaking change if any code relies on Objective-C interoperability or NSObject-specific features like KVO. Ensure that this change has been tested across all consumers of this class and that no Objective-C bridging is required.
| public class PaymentReviewModel { | |
| public class PaymentReviewModel: NSObject { |
| ProgressView() | ||
| .frame(height: 400) | ||
| } else if !viewModel.cellViewModels.isEmpty { | ||
| let images = viewModel.cellViewModels.compactMap { $0.preview } | ||
| GiniCarouselView(images: images) | ||
| .frame(height: 500) | ||
| } |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation here is inconsistent with the rest of the code. The ProgressView and GiniCarouselView lines have excessive leading whitespace (multiple tabs/spaces) compared to the if statement they're nested under.
| ProgressView() | |
| .frame(height: 400) | |
| } else if !viewModel.cellViewModels.isEmpty { | |
| let images = viewModel.cellViewModels.compactMap { $0.preview } | |
| GiniCarouselView(images: images) | |
| .frame(height: 500) | |
| } | |
| ProgressView() | |
| .frame(height: 400) | |
| } else if !viewModel.cellViewModels.isEmpty { | |
| let images = viewModel.cellViewModels.compactMap { $0.preview } | |
| GiniCarouselView(images: images) | |
| .frame(height: 500) | |
| } |
| await viewModel.fetchImages() | ||
| } | ||
| } else { | ||
| // Fallback on earlier versions |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback for iOS versions earlier than 15.0 is empty. Since the minimum deployment target is now iOS 14.0, there should be a proper fallback implementation for iOS 14 users, or document why no fallback is needed. An empty fallback will result in a blank screen for iOS 14 users.
| // Fallback on earlier versions | |
| ScrollView { | |
| VStack(spacing: 16) { | |
| if viewModel.isImagesLoading { | |
| ProgressView() | |
| .frame(height: 400) | |
| } else if !viewModel.cellViewModels.isEmpty { | |
| let images = viewModel.cellViewModels.compactMap { $0.preview } | |
| GiniCarouselView(images: images) | |
| .frame(height: 500) | |
| } | |
| } | |
| .padding() | |
| } | |
| .onAppear { | |
| Task { | |
| await viewModel.fetchImages() | |
| } | |
| } |
| private let selectedPaymentProvider: PaymentProvider | ||
| private var isInfoBarHidden = true |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These properties are stored but never used within the class. Consider whether they're needed for future functionality or if they should be removed to keep the code clean.
| var viewModels: [PageCollectionCellViewModel] = [] | ||
| isImagesLoading = true | ||
|
|
||
| await withTaskGroup(of: PageCollectionCellViewModel?.self) { group in | ||
| for page in 1 ... document.pageCount { | ||
| group.addTask { [weak self] in | ||
| do { | ||
| guard let result = try await self?.fetchPreview(for: documentId, pageNumber: page) else { | ||
| return nil | ||
| } | ||
|
|
||
| return self?.proccessPreview(result) | ||
| } catch { | ||
| return nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for await cellViewModel in group { | ||
| guard let cellViewModel else { continue } | ||
|
|
||
| viewModels.append(cellViewModel) | ||
| } | ||
|
|
||
| isImagesLoading = false | ||
| cellViewModels.append(contentsOf: viewModels) |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ordering of results is not preserved when using TaskGroup. Images are appended as they complete, which may cause pages to appear out of order. Consider sorting the viewModels by page number before appending them to cellViewModels, or use a different approach to maintain page order.
| var viewModels: [PageCollectionCellViewModel] = [] | |
| isImagesLoading = true | |
| await withTaskGroup(of: PageCollectionCellViewModel?.self) { group in | |
| for page in 1 ... document.pageCount { | |
| group.addTask { [weak self] in | |
| do { | |
| guard let result = try await self?.fetchPreview(for: documentId, pageNumber: page) else { | |
| return nil | |
| } | |
| return self?.proccessPreview(result) | |
| } catch { | |
| return nil | |
| } | |
| } | |
| } | |
| for await cellViewModel in group { | |
| guard let cellViewModel else { continue } | |
| viewModels.append(cellViewModel) | |
| } | |
| isImagesLoading = false | |
| cellViewModels.append(contentsOf: viewModels) | |
| var viewModels: [PageCollectionCellViewModel?] = Array(repeating: nil, count: document.pageCount) | |
| isImagesLoading = true | |
| await withTaskGroup(of: (Int, PageCollectionCellViewModel?).self) { group in | |
| for page in 1 ... document.pageCount { | |
| group.addTask { [weak self] in | |
| do { | |
| guard let result = try await self?.fetchPreview(for: documentId, pageNumber: page) else { | |
| return (page, nil) | |
| } | |
| let cellViewModel = self?.proccessPreview(result) | |
| return (page, cellViewModel) | |
| } catch { | |
| return (page, nil) | |
| } | |
| } | |
| } | |
| for await (page, cellViewModel) in group { | |
| guard let cellViewModel else { continue } | |
| let index = page - 1 | |
| if index >= 0 && index < viewModels.count { | |
| viewModels[index] = cellViewModel | |
| } | |
| } | |
| isImagesLoading = false | |
| let orderedViewModels = viewModels.compactMap { $0 } | |
| cellViewModels.append(contentsOf: orderedViewModels) |




Pull Request Description
[PP-####]
Notes for Reviewers