Feat/increase target sdk #447
Conversation
📝 WalkthroughWalkthroughCentralizes Android storage and notification permission handling into a new PermissionManager, updates multiple UI components to use it, removes legacy permission helpers, and bumps minSdk 26→28 and targetSdk 28→31. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Composable / Activity
participant PM as PermissionManager
participant Launcher as ActivityResultLauncher
participant Android as Android System
User->>UI: Trigger action requiring storage/notification
UI->>PM: hasStorageAccess(context) / hasNotificationPermission(context)
alt Already granted
PM-->>UI: true
UI->>UI: Proceed with operation (scan/install/display)
else Not granted
PM-->>UI: false
UI->>PM: requestStorageAccess(context, legacyLauncher, allFilesLauncher) or requestNotificationPermission(context, launcher)
PM->>Launcher: launch appropriate intent/permission request
Launcher->>Android: show permission UI
Android-->>Launcher: user decision
Launcher-->>UI: result
UI->>PM: (optional) re-check hasStorageAccess / hasNotificationPermission
UI->>UI: Proceed or handle denial
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt (2)
892-912: Missing permission verification before game migration.The
permissionMovingInternalLaunchercallback ignores thepermissionresult map and immediately proceeds with file migration. If the user denies the permissions, the migration will still attempt to run, likely causing failures.🐛 Proposed fix
val permissionMovingInternalLauncher = rememberLauncherForActivityResult( contract = ActivityResultContracts.RequestMultiplePermissions(), - onResult = { permission -> - scope.launch { - showMoveDialog = true - StorageUtils.moveGamesFromOldPath( - Paths.get(Environment.getExternalStorageDirectory().absolutePath, "GameNative", "Steam").pathString, - oldGamesDirectory, - onProgressUpdate = { currentFile, fileProgress, movedFiles, totalFiles -> - current = currentFile - progress = fileProgress - moved = movedFiles - total = totalFiles - }, - onComplete = { - showMoveDialog = false - }, - ) + onResult = { permissions -> + val allGranted = permissions.values.all { it } + if (allGranted) { + scope.launch { + showMoveDialog = true + StorageUtils.moveGamesFromOldPath( + Paths.get(Environment.getExternalStorageDirectory().absolutePath, "GameNative", "Steam").pathString, + oldGamesDirectory, + onProgressUpdate = { currentFile, fileProgress, movedFiles, totalFiles -> + current = currentFile + progress = fileProgress + moved = movedFiles + total = totalFiles + }, + onComplete = { + showMoveDialog = false + }, + ) + } + } else { + Toast.makeText( + context, + context.getString(R.string.steam_storage_permission_required), + Toast.LENGTH_SHORT + ).show() } }, )
482-494: Use safe call operator fordownloadInfo.cancel().Kotlin's smart cast does not propagate through the intermediate
isDownloadingboolean variable. SinceSteamService.getAppDownloadInfo()returnsDownloadInfo?, the direct call at line 488 is unsafe. Use the safe call operator for consistency with line 1039 and other similar usages:if (isDownloading) { - downloadInfo.cancel() + downloadInfo?.cancel() }
🤖 Fix all issues with AI agents
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt`:
- Around line 930-934: The allFilesAccessLauncher callback
(rememberLauncherForActivityResult assigned to allFilesAccessLauncher) only
updates hasStoragePermission via PermissionManager.hasStorageAccess(context) but
does not mirror the denied-case handling performed in permissionLauncher (lines
around permissionLauncher) — you should add the same denial-handling logic:
detect when PermissionManager.hasStorageAccess(context) is false and then show
the same toast feedback and update/close any related dialog state variables
exactly as permissionLauncher does so dialogs aren't left visible; ensure you
reuse the same UI state setters and toast message used by permissionLauncher to
keep behavior consistent.
🧹 Nitpick comments (3)
app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt (1)
156-163: Folder is added before permission is confirmed.The current flow adds the folder via
onAddCustomGameFolder(path)at line 163 regardless of whether the permission request succeeds. If the user denies the permission in settings, the folder will still be added but won't be accessible.Consider either:
- Only adding the folder after confirming access, or
- Accepting this UX trade-off since
OpenDocumentTreeusually grants URI permissions for the selected folder anyway.If the current behavior is intentional (since
OpenDocumentTreegrants URI-specific access), the comment at lines 144-146 suggests this is expected. However, the!canAccesscheck at line 156 implies the folder isn't accessible, yet it's still added.Consider restructuring the flow
// Only request permissions if we can't access the folder AND it's outside the sandbox // (folders selected via OpenDocumentTree should already be accessible) if (!canAccess && !PermissionManager.hasStorageAccessForPath(context, path)) { PermissionManager.requestStorageAccess( context, storagePermissionLauncher, allFilesAccessLauncher, ) + // Note: Folder is added optimistically; if permission is denied, + // scanning will fail gracefully at runtime } onAddCustomGameFolder(path)app/src/main/java/app/gamenative/MainActivity.kt (1)
155-159: Redundant permission check - PermissionManager handles this internally.The condition
if (!hasNotificationPermission)is unnecessary sincePermissionManager.requestNotificationPermission()already returns early if permission is granted (see line 108 inPermissionManager.kt). ThehasNotificationPermissionstate is also only set totrueafter the launcher callback, not before the initial check.Simplify by removing redundant check
LaunchedEffect(Unit) { - if (!hasNotificationPermission) { - PermissionManager.requestNotificationPermission(context, permissionLauncher) - } + PermissionManager.requestNotificationPermission(context, permissionLauncher) }app/src/main/java/app/gamenative/utils/PermissionManager.kt (1)
39-46: Path check may have edge cases with normalization.The string-based path check at lines 40-41 could miss edge cases:
- Paths with trailing slashes:
/Android/data/app.gamenative/vs/Android/data/app.gamenative- Symbolic links or mount points that resolve to the sandbox
- Case sensitivity on some file systems
Consider normalizing the path before comparison.
Normalize paths for more robust comparison
fun hasStorageAccessForPath(context: Context, path: String): Boolean { - val isOutsideSandbox = !path.contains("/Android/data/${context.packageName}") && - !path.contains(context.dataDir.path) + val normalizedPath = java.io.File(path).canonicalPath + val externalSandbox = context.getExternalFilesDir(null)?.parentFile?.canonicalPath + val internalSandbox = context.dataDir.canonicalPath + + val isOutsideSandbox = (externalSandbox == null || !normalizedPath.startsWith(externalSandbox)) && + !normalizedPath.startsWith(internalSandbox) if (!isOutsideSandbox) { return true } return hasStorageAccess(context) }
app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
1 participant
Summary by cubic
Raised minSdk to 28 and targetSdk to 31 (Android 12), and centralized storage and notification permissions in a new PermissionManager. Updated screens to request All Files Access when needed and cleaned up legacy permission code.
Refactors
Migration
Written for commit c9157c1. Summary will update on new commits.
Summary by CodeRabbit
Chores
New Features
✏️ Tip: You can customize this high-level summary in your review settings.