Conversation
PR Reviewer Guide 🔍(Review updated until commit d8bb9c7)Here are some key observations to aid the review process:
|
| val shouldShow = rememberSaveable { mutableStateOf(false) } | ||
| remember { SupportedApiManager(permissionState, shouldShow) } | ||
| } |
There was a problem hiding this comment.
Suggestion: Add permissionString as a key parameter to both rememberSaveable and remember calls. This ensures that internal state is properly reset when the customPermission parameter changes, preventing stale state from persisting across different permission types. [possible issue, importance: 7]
| val shouldShow = rememberSaveable { mutableStateOf(false) } | |
| remember { SupportedApiManager(permissionState, shouldShow) } | |
| } | |
| val permissionState = rememberPermissionState(permissionString) | |
| val shouldShow = rememberSaveable(permissionString) { mutableStateOf(false) } | |
| remember(permissionString) { SupportedApiManager(permissionState, shouldShow) } |
| sealed interface CustomPermissionManager { | ||
| val shouldShow: Boolean | ||
|
|
||
| fun askPermission() | ||
| fun dismissAndAskPermission() | ||
| } |
There was a problem hiding this comment.
Maybe PermissionManagerState or something similar?
sirambd
left a comment
There was a problem hiding this comment.
We need to support required permissions as well
|
Persistent review updated to latest commit 830a2eb |
| remember { UnsupportedApiManager } | ||
| } else { | ||
| val permissionState = rememberPermissionState(permissionString) | ||
| val shouldShow = rememberSaveable { mutableStateOf(false) } |
There was a problem hiding this comment.
Suggestion: Add permissionType as a key to rememberSaveable to prevent state restoration issues when the permission type changes. Without a key, the shouldShow value persists across different permission types if the composable is recomposed at the same tree position, causing incorrect UI state (e.g., showing rationale dialog for a different permission). [possible issue, importance: 8]
| val shouldShow = rememberSaveable { mutableStateOf(false) } | |
| val shouldShow = rememberSaveable(permissionType) { mutableStateOf(false) } |
| @Stable | ||
| @OptIn(ExperimentalPermissionsApi::class) | ||
| internal class SupportedApiManager( | ||
| private val permissionState: PermissionState, | ||
| private val _shouldShow: MutableState<Boolean>, | ||
| ) : PermissionManagerState { |
There was a problem hiding this comment.
Suggestion: Remove the @Stable annotation because PermissionState from Accompanist is likely an unstable type. Marking the class as @Stable when it holds an unstable property can cause Compose to skip necessary recompositions when the permission status changes, leading to stale UI state. [possible issue, importance: 7]
| @Stable | |
| @OptIn(ExperimentalPermissionsApi::class) | |
| internal class SupportedApiManager( | |
| private val permissionState: PermissionState, | |
| private val _shouldShow: MutableState<Boolean>, | |
| ) : PermissionManagerState { | |
| @OptIn(ExperimentalPermissionsApi::class) | |
| internal class SupportedApiManager( | |
| private val permissionState: PermissionState, | |
| private val _shouldShow: MutableState<Boolean>, | |
| ) : PermissionManagerState { |
830a2eb to
fdc6f09
Compare
|
Persistent review updated to latest commit fdc6f09 |
| remember { UnsupportedApiPermissionManagerState } | ||
| } else { | ||
| val permissionState = rememberPermissionState(permissionString) | ||
| val shouldDisplayRationale = rememberSaveable { mutableStateOf(false) } |
There was a problem hiding this comment.
Suggestion: Add permissionType as a key parameter to rememberSaveable to ensure the rationale display state is properly associated with the specific permission type. Without this key, the saved state could incorrectly persist across different permission types if the parameter changes during recompositions. [general, importance: 6]
| val shouldDisplayRationale = rememberSaveable { mutableStateOf(false) } | |
| val shouldDisplayRationale = rememberSaveable(permissionType) { mutableStateOf(false) } |
| override fun askPermission() { | ||
| if (permissionState.status.shouldShowRationale) { | ||
| _shouldDisplayRationale.value = true | ||
| } else { | ||
| permissionState.launchPermissionRequest() | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Check if the permission is already granted before launching the permission request. Currently, if the permission is granted, shouldShowRationale is false and launchPermissionRequest() is called unnecessarily. Add an explicit check for permissionState.status.isGranted to return early and avoid redundant system calls. [general, importance: 5]
| override fun askPermission() { | |
| if (permissionState.status.shouldShowRationale) { | |
| _shouldDisplayRationale.value = true | |
| } else { | |
| permissionState.launchPermissionRequest() | |
| } | |
| } | |
| override fun askPermission() { | |
| when { | |
| permissionState.status.isGranted -> return | |
| permissionState.status.shouldShowRationale -> _shouldDisplayRationale.value = true | |
| else -> permissionState.launchPermissionRequest() | |
| } | |
| } |
|
Persistent review updated to latest commit 1678945 |
| @Composable | ||
| override fun guardedCallback(action: () -> Unit): () -> Unit = with(permissionState) { | ||
| val isGrantedFlow = remember(permission) { snapshotFlow { status.isGranted } } | ||
| val actionFired: CompletableJob = remember(permission) { Job() } | ||
|
|
||
| LaunchedEffect(permission) { | ||
| actionFired.join() | ||
| isGrantedFlow.first { it } | ||
| action() | ||
| } | ||
|
|
||
| return { | ||
| if (status.isGranted) { | ||
| action() | ||
| } else { | ||
| launchPermissionRequest() | ||
| actionFired.complete() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Replace the CompletableJob with a MutableState flag to prevent coroutine leaks and crashes. The current implementation leaks the LaunchedEffect coroutine when permission is already granted (since actionFired is never completed), and crashes with IllegalStateException if the callback is invoked multiple times while permission is denied (completing an already-completed job). [possible issue, importance: 9]
| @Composable | |
| override fun guardedCallback(action: () -> Unit): () -> Unit = with(permissionState) { | |
| val isGrantedFlow = remember(permission) { snapshotFlow { status.isGranted } } | |
| val actionFired: CompletableJob = remember(permission) { Job() } | |
| LaunchedEffect(permission) { | |
| actionFired.join() | |
| isGrantedFlow.first { it } | |
| action() | |
| } | |
| return { | |
| if (status.isGranted) { | |
| action() | |
| } else { | |
| launchPermissionRequest() | |
| actionFired.complete() | |
| } | |
| } | |
| } | |
| @Composable | |
| override fun guardedCallback(action: () -> Unit): () -> Unit = with(permissionState) { | |
| val isGrantedFlow = remember(permission) { snapshotFlow { status.isGranted } } | |
| val pendingAction = remember { mutableStateOf(false) } | |
| LaunchedEffect(permission, pendingAction.value) { | |
| if (pendingAction.value) { | |
| isGrantedFlow.first { it } | |
| action() | |
| pendingAction.value = false | |
| } | |
| } | |
| return { | |
| if (status.isGranted) { | |
| action() | |
| } else { | |
| pendingAction.value = true | |
| launchPermissionRequest() | |
| } | |
| } | |
| } |
1678945 to
6d914f3
Compare
|
Persistent review updated to latest commit 6d914f3 |
| @Composable | ||
| override fun guardedUnlessPermissionIsGranted(action: () -> Unit): () -> Unit = with(permissionState) { | ||
| val isGrantedFlow = remember(permission) { snapshotFlow { status.isGranted } } | ||
| val actionFired: CompletableJob = remember(permission) { Job() } | ||
|
|
||
| LaunchedEffect(permission) { | ||
| actionFired.join() | ||
| isGrantedFlow.first { it } | ||
| action() | ||
| } | ||
|
|
||
| return { | ||
| if (status.isGranted) { | ||
| action() | ||
| } else { | ||
| launchPermissionRequest() | ||
| actionFired.complete() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: The CompletableJob approach has two critical issues: 1) If permission is already granted when the returned lambda is invoked, actionFired.complete() is never called, causing the LaunchedEffect coroutine to leak by suspending indefinitely at actionFired.join(). 2) If the user invokes the returned lambda multiple times while permission is not granted, actionFired.complete() throws an IllegalStateException (already completed).
Replace CompletableJob with a MutableState<Boolean> flag to safely trigger the permission observation and prevent both the coroutine leak and the crash on multiple invocations. [possible issue, importance: 9]
| @Composable | |
| override fun guardedUnlessPermissionIsGranted(action: () -> Unit): () -> Unit = with(permissionState) { | |
| val isGrantedFlow = remember(permission) { snapshotFlow { status.isGranted } } | |
| val actionFired: CompletableJob = remember(permission) { Job() } | |
| LaunchedEffect(permission) { | |
| actionFired.join() | |
| isGrantedFlow.first { it } | |
| action() | |
| } | |
| return { | |
| if (status.isGranted) { | |
| action() | |
| } else { | |
| launchPermissionRequest() | |
| actionFired.complete() | |
| } | |
| } | |
| } | |
| @Composable | |
| override fun guardedUnlessPermissionIsGranted(action: () -> Unit): () -> Unit = with(permissionState) { | |
| val isGrantedFlow = remember(permission) { snapshotFlow { status.isGranted } } | |
| val pendingAction = remember(permission) { mutableStateOf(false) } | |
| LaunchedEffect(permission, pendingAction.value) { | |
| if (pendingAction.value) { | |
| isGrantedFlow.first { it } | |
| action() | |
| pendingAction.value = false | |
| } | |
| } | |
| return { | |
| if (status.isGranted) { | |
| action() | |
| } else { | |
| launchPermissionRequest() | |
| pendingAction.value = true | |
| } | |
| } | |
| } |
6d914f3 to
b91aed2
Compare
b91aed2 to
d8bb9c7
Compare
|
Persistent review updated to latest commit d8bb9c7 |
|
| return fun() { | ||
| if (status.isGranted) { | ||
| action() | ||
| } else { | ||
| launchPermissionRequest() | ||
| actionFired.complete() | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: When the permission is already granted, the action callback is invoked twice: once immediately in this lambda and again inside the LaunchedEffect after actionFired.complete() resumes the coroutine. Use an atomic flag or CompletableDeferred to ensure the action executes only once. [possible issue, importance: 9]
| return fun() { | |
| if (status.isGranted) { | |
| action() | |
| } else { | |
| launchPermissionRequest() | |
| actionFired.complete() | |
| } | |
| } | |
| val actionExecuted = remember(permission) { AtomicBoolean(false) } | |
| return fun() { | |
| if (status.isGranted) { | |
| if (actionExecuted.compareAndSet(false, true)) action() | |
| } else { | |
| launchPermissionRequest() | |
| actionFired.complete() | |
| } | |
| } |
| val permissionState = rememberPermissionState(permissionString) | ||
| val shouldDisplayRationale = rememberSaveable { mutableStateOf(false) } | ||
| remember { SupportedApiPermissionManagerState(permissionState, shouldDisplayRationale) } |
There was a problem hiding this comment.
Suggestion: The remember block for SupportedApiPermissionManagerState lacks keys, causing it to retain the old permissionState instance if permissionType changes (e.g., from Notification to WriteExternalStorage). Add permissionState as a key to ensure the state manager updates correctly when the permission type changes. [possible issue, importance: 8]
| val permissionState = rememberPermissionState(permissionString) | |
| val shouldDisplayRationale = rememberSaveable { mutableStateOf(false) } | |
| remember { SupportedApiPermissionManagerState(permissionState, shouldDisplayRationale) } | |
| val permissionState = rememberPermissionState(permissionString) | |
| val shouldDisplayRationale = rememberSaveable { mutableStateOf(false) } | |
| remember(permissionState) { SupportedApiPermissionManagerState(permissionState, shouldDisplayRationale) } |
There was a problem hiding this comment.
Pull request overview
Adds a new PermissionManager Android library module intended to centralize permission handling via Jetpack Compose + Accompanist, and wires it into the multi-module build.
Changes:
- Registers the new
:PermissionManagerGradle module and publishes a new version-catalog alias for it. - Introduces a Compose-first
PermissionManagerStateAPI with supported/unsupported-API implementations. - Adds
accompanist-permissionsto the version catalog and uses it in the new module.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle.kts | Includes the new :PermissionManager module in the build. |
| gradle/core.versions.toml | Adds Accompanist permissions dependency + a new infomaniak-core-permissionmanager catalog entry. |
| PermissionManager/build.gradle.kts | New Android library module configuration and dependencies (Compose + Accompanist). |
| PermissionManager/src/main/kotlin/com/infomaniak/core/permissionmanager/PermissionManagerState.kt | Public rememberPermissionManagerState entry point + PermissionManagerState API. |
| PermissionManager/src/main/kotlin/com/infomaniak/core/permissionmanager/SupportedApiPermissionManagerState.kt | Supported-permission implementation backed by Accompanist PermissionState. |
| PermissionManager/src/main/kotlin/com/infomaniak/core/permissionmanager/UnsupportedApiPermissionManagerState.kt | No-op implementation for permissions not applicable on the current API level. |
| PermissionManager/src/main/kotlin/com/infomaniak/core/permissionmanager/PermissionType.kt | Adds PermissionType enum mapping app concepts to Android permission strings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| val permissionState = rememberPermissionState(permissionString) | ||
| val shouldDisplayRationale = rememberSaveable { mutableStateOf(false) } | ||
| remember { SupportedApiPermissionManagerState(permissionState, shouldDisplayRationale) } |
There was a problem hiding this comment.
SupportedApiPermissionManagerState(...) is created with remember { ... } without a key, so if permissionType changes across recompositions the returned PermissionManagerState will keep referencing the old permissionState/rationale state and may request the wrong permission. Key the remember to permissionString/permissionState (or avoid remember here) so the state instance is recreated when the permission changes.
| remember { SupportedApiPermissionManagerState(permissionState, shouldDisplayRationale) } | |
| remember(permissionString, permissionState, shouldDisplayRationale) { | |
| SupportedApiPermissionManagerState(permissionState, shouldDisplayRationale) | |
| } |
| val isGrantedFlow = remember(permission) { snapshotFlow { status.isGranted } } | ||
| val actionFired: CompletableJob = remember(permission) { Job() } | ||
|
|
||
| LaunchedEffect(permission) { | ||
| actionFired.join() | ||
| isGrantedFlow.first { it } | ||
| action() | ||
| } |
There was a problem hiding this comment.
waitUntilGranted() starts a LaunchedEffect that suspends on actionFired.join(). If the returned lambda only ever takes the status.isGranted branch, actionFired is never completed and the coroutine will remain suspended for the lifetime of the composition. Consider restructuring so no long-lived suspended effect is created unless a permission request was actually initiated (e.g., gate the effect behind a requested state).



Add a 'PermissionManager' that allow you to easily manage permission across your application.