🥅 Add explicit Compose dialog error handling - #632
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4193c4ee9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| init { | ||
| registry.retain(session) | ||
| continuation.invokeOnCancellation { release() } |
There was a problem hiding this comment.
When the camera coroutine is cancelled while UIImagePickerController is visible—for example, when its Compose scope is disposed—this handler releases the registry's sole strong reference to CameraControllerDelegate without dismissing the controller. Because the delegate's cancellation/completion callbacks are the only code that dismisses the camera, the presented picker can remain without a live delegate and stop responding to completion or cancellation. Retain the controller and dismiss it from the cancellation handler before releasing the delegate.
Useful? React with 👍 / 👎.
| ): O = try { | ||
| primary() | ||
| } catch (_: ActivityNotFoundException) { | ||
| val fallbackLaunch = fallback ?: return null | ||
| } catch (primaryFailure: ActivityNotFoundException) { |
There was a problem hiding this comment.
If an Android suspend file-picker launch throws SecurityException, this helper neither attempts the configured visual-picker fallback nor converts the failure to FileKitPickerException; the exception is resumed by awaitActivityResult and escapes unchanged. This contradicts the newly documented failure contract and the Compose picker path, which now classifies the same launch-time exception as an operational picker failure. Handle SecurityException for both the primary and fallback launches and wrap an exhausted launch as FileKitPickerException.
Useful? React with 👍 / 👎.
b4193c4 to
d08d922
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d08d922ea9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pendingState.begin() | ||
| launch { | ||
| try { | ||
| block() |
There was a problem hiding this comment.
When a non-Android launcher's onResult or onError callback immediately retries by calling the same launcher's launch(), block() is still executing and pendingState.finish() has not run, so the retry throws IllegalStateException even though the dialog operation has already produced its terminal result. Android clears its pending state before dispatching callbacks, but this shared wrapper affects the JVM, iOS, and other non-Android launchers; clear the pending marker before terminal callback dispatch while retaining cleanup for cancellation and unexpected exceptions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b1c8c2a64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, | ||
| ) { | ||
| private var completed = false |
There was a problem hiding this comment.
When a photo-picker coroutine is cancelled while PHPickerViewController is visible—for example, because its Compose scope is disposed—getPhPickerResults uses this default cancellation handler, which immediately releases the registry's sole strong reference to both picker delegates without dismissing the controller. The camera-specific path now supplies a dismissal hook, but the photo picker does not; consequently its delegate can disappear before didFinishPicking runs, leaving the presented picker unable to complete its normal dismissal. Retain the controller and dismiss it during cancellation before releasing the session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c891da186e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fun retain(session: Session) { | ||
| check(sessions.add(session)) { "The dialog session is already retained." } |
There was a problem hiding this comment.
When two independent Compose launchers using the same captured LocalUIViewController launch the same dialog type before the first closes, this registry accepts both unique sessions. UIKit refuses the second presentViewController call because that presenter is already presenting the first controller, but provides no failure callback, so the second continuation and delegate remain retained indefinitely. Track presenter ownership or reject an overlapping session synchronously instead of allowing every distinct session into the set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d7a51c33c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| beforeCallback() | ||
| mode.consumeResult(result, onResult) |
There was a problem hiding this comment.
For non-Android SingleWithState and MultipleWithState launchers, openPicker() returns a cold Flow before the native picker is presented, so this call clears LauncherPendingState before mode.consumeResult() starts collecting it. A second launch() while the first picker is still visible therefore bypasses the single-pending guard, permitting concurrent desktop dialogs or producing an iOS overlap failure, contrary to the policy documented in docs/dialogs/file-picker.mdx. Keep the token until the flow reaches a terminal state, while still clearing it before dispatching that terminal callback.
Useful? React with 👍 / 👎.
| fun complete(result: Result) { | ||
| if (completed || !continuation.isActive) return | ||
| completed = true | ||
| release() | ||
| continuation.resume(result) |
There was a problem hiding this comment.
When an iOS document or photo picker completes and onResult immediately launches another dialog, this releases the presenter before UIKit has finished dismissing the first controller: DocumentPickerDelegate invokes completion directly, while PhPickerDelegate starts dismissal without waiting for its completion handler. The registry consequently accepts the retry, but UIKit can reject presentation during the active dismissal transition without a failure callback, leaving the new continuation retained indefinitely. Fresh evidence beyond the earlier overlap report is that complete() now drops presentation ownership before dismissal completion; release it only after the controller is fully dismissed.
Useful? React with 👍 / 👎.
1 participant
Summary
Compatibility
Verification
Manual UI smoke testing was not performed; native failure, cancellation, ownership, and terminal-state behavior is covered by deterministic adapter tests.