fix(ios): resolve PLYPresentationView blank-on-reopen and freeze on iOS#221
fix(ios): resolve PLYPresentationView blank-on-reopen and freeze on iOS#221kherembourg merged 6 commits intomainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers critical fixes for two iOS-specific bugs affecting the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several important fixes for iOS, addressing a blank paywall issue on reuse and an application freeze caused by a deadlock, specifically by transitioning from synchronous semaphore usage to an asynchronous completion-based approach for fetching presentations and correcting a logical error in purchase result handling. While these changes improve stability, a critical race condition was identified in the new metadata processing logic where concurrent writes to a mutable dictionary are not fully synchronized, which could lead to application crashes. Additionally, areas for improvement include thread safety consistency and data type correctness across the native-to-JS bridge.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR fixes critical iOS bugs in the PLYPresentationView component that occur when reusing the same PLYPresentation object across multiple navigation events. The primary issues addressed are: (1) blank paywall on second visit due to incorrect placement ID fallback logic, and (2) app freeze caused by semaphore-based deadlock in async metadata handling.
Changes:
- Fixed placement ID fallback to use
presentation?.placementIdwhenplacementIdprop is nil - Replaced semaphore-based synchronization with completion-based async pattern to eliminate deadlock
- Fixed logical operator bug in result comparison (
|| PLYProductViewControllerResultRestored→|| result == PLYProductViewControllerResultRestored) - Added proper cleanup of previous view/controller in
setupView()and thread-safe dictionary updates - Added cancellation pattern to TypeScript
useEffectto prevent stale callbacks - Updated example app to reproduce the client scenario (fetch once, reuse across navigations)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/purchasely/ios/PurchaselyView.swift |
Captures effectivePlacementId before guard to fix blank paywall on reuse; adds cleanup of previous view/controller; fixes memory leak with [weak self] |
packages/purchasely/ios/PurchaselyViewManager.swift |
Forwards resolve to current view after it's created to handle race condition |
packages/purchasely/ios/PurchaselyRN.m |
Replaces semaphore deadlock with async completion pattern; fixes logical operator bug; adds main queue dispatch and serial queue for thread safety; adds default case to switch statement |
packages/purchasely/src/components/PLYPresentationView.tsx |
Adds cancellation flag to prevent stale async callbacks after unmount; wraps in try-catch for error handling |
example/src/Home.tsx |
Fetches presentation once at mount and caches in ref for testing reuse scenario |
example/src/Paywall.tsx |
Reads presentation from route params instead of fetching locally; adds Back button for navigation testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dispatch_sync(dictQueue, ^{ | ||
| [resultDict setObject:value forKey:key]; | ||
| }); |
There was a problem hiding this comment.
The dispatch_sync to dictQueue for non-string values is unnecessary here. Since we're still in the synchronous for-loop and no async callbacks have been dispatched yet, you can directly write to resultDict without synchronization. The dictQueue synchronization is only needed for the async string value callbacks.
| dispatch_sync(dictQueue, ^{ | |
| [resultDict setObject:value forKey:key]; | |
| }); | |
| [resultDict setObject:value forKey:key]; |
Bug #1 — Paywall blank after close/reopen (iOS only): - setupView() now removes previous subview and controller before adding new ones, preventing stale view accumulation - createNativeViewController() completion closure now properly calls onPresentationClosedPromise instead of discarding the result - prefetchPresentationViewController() uses [weak self] capture to avoid retain cycles - PurchaselyViewManager.onPresentationClosed() now forwards the resolve block directly to the current PurchaselyView, fixing the race between view() (called during render) and the useEffect that registers the promise resolver Bug #2 — App freeze with onPresentationClosed (~2/3 times): - PLYPresentationView: add cancelled flag + cleanup return in useEffect to prevent dangling async handleClose() calls accumulating across re-renders and component unmounts - PurchaselyRN.m: replace blocking dispatch_semaphore_wait in resultDictionaryForFetchPresentation with a fully async buildResultForFetchPresentation:completion: — eliminates the main-thread deadlock that occurred when getStringWith:completion: callbacks dispatched back to the already-blocked main queue Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix always-true OR condition in resultDictionaryForPresentationController: `|| PLYProductViewControllerResultRestored` → `|| result == ...` (was causing hidePresentation to fire even on cancellation) - Dispatch finalize() to main queue inside dispatch_group_notify to ensure resolve() is always called on the main thread (eliminates off-main-thread RCT bridge call risk) - Protect concurrent NSMutableDictionary writes in metadata loop with a dedicated serial queue (dictQueue) — NSMutableDictionary is not thread-safe for concurrent mutation from SDK callbacks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Controller When PLYPresentationView is used with the `presentation` prop only (no `placementId` prop), the placementId prop is nil. On the second visit, the presentation has already been consumed from presentationsLoaded, so the guard fails and falls back to createNativeViewController with nil — returning an empty view. Fix: capture effectivePlacementId = placementId ?? presentation?.placementId before the guard, so the fallback correctly uses the presentation's own placementId to recreate the view controller on subsequent visits. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…entationView bug Fetch the nested presentation once on Home mount and pass it via navigation params to Paywall, reusing the same object across visits. This replicates the Bayard scenario (fetch once, reuse) and makes the blank-on-reopen bug reproducible locally. Also adds a Back button to Paywall for easier navigation during testing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Use NSNull() instead of [] for nil plan in createNativeViewController, matching the TypeScript PurchaselyPlan | null type on the JS side - Synchronize non-string metadata writes through dictQueue to eliminate race condition between loop (main thread) and async string callbacks - Dispatch finalize() to main queue in the no-metadata path for consistency with the metadata path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove no-op willMove(toParent:)/removeFromParent() calls in setupView() since _controller is never added as a child VC; only view cleanup needed - Log unexpected onPresentationClosed errors in PLYPresentationView instead of silently swallowing them (cancellation errors remain suppressed) - Remove debug console.log from Home.tsx example Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kherembourg
force-pushed
the
fix/ios-ply-presentation-view-bugs
branch
from
6853f6a to
5844125
Compare
Context
This PR fixes two iOS bugs reported by a client currently under active investigation. The issues affect the
PLYPresentationViewembedded paywall component when the samePLYPresentationobject is reused across multiple navigations — which is the standard pattern when a presentation is fetched once and passed via thepresentationprop.Notion ticket: https://www.notion.so/313b887cdced812382b9ec0e931707f2
Bugs Fixed
Bug 1 — Blank paywall on second visit (iOS)
When
PLYPresentationViewis used with thepresentationprop (noplacementIdprop), the native iOS layer consumes the presentation frompresentationsLoadedon first display. On the second visit, the same presentation object is passed but is no longer inpresentationsLoaded. The guard fails and falls back tocreateNativeViewController(placementId: nil)— returningnil— resulting in a blank view.Root cause:
getPresentationControllerusedplacementId(the prop, alwaysnilin this usage pattern) as the fallback, instead ofpresentation?.placementId.Fix: Capture
effectivePlacementId = placementId ?? presentation?.placementIdbefore the guard, so the fallback correctly recreates the view controller using the presentation's own placement ID.Bug 2 — App freeze (~2/3 of the time) on
onPresentationClosedSentry reported "application hanging for more than 2 seconds" triggered by a
dispatch_semaphore_waiton the main thread inPurchaselyRN.m, causing a deadlock when the completion block was dispatched back to the main queue.Fix: Replaced the semaphore pattern with an async completion-based approach (
buildResultForFetchPresentation:completion:).Additional P1 fixes (found in code review)
|| PLYProductViewControllerResultRestored→|| result == PLYProductViewControllerResultRestoredfinalize()indispatch_async(dispatch_get_main_queue(), ...)to prevent UI updates off the main threaddictQueueto protect concurrent writes toresultDict[weak self]capture inprefetchPresentationViewControllerto prevent retain cycleonPresentationClosednow correctly forwardsresolveto the current view after it's setFiles Changed
packages/purchasely/ios/PurchaselyView.swifteffectivePlacementIdfallback; clean up old view/controller insetupView()packages/purchasely/ios/PurchaselyViewManager.swiftresolveto current view inonPresentationClosedpackages/purchasely/ios/PurchaselyRN.mpackages/purchasely/src/components/PLYPresentationView.tsxcancelledflag inuseEffectto prevent stale async callbacksexample/src/Home.tsxexample/src/Paywall.tsxroute.params; add Back button for testingTest Plan
mainwith the updated example (presentation fetched once, reused across visits)PurchaselyViewManager.ktdoes not consume/remove presentations frompresentationsLoadedNotes
🤖 Investigated and fixed with Claude Code