Merged
Conversation
Security: remove debug log leaking group pubkey, add timeout to BunkerService runBlocking, narrow catch(Throwable) to catch(Exception). P10 Rule 5: add require() preconditions to AndroidKeystoreStorage and BunkerService security-critical functions. P10 Rule 7: replace !! with safe local variable captures, fix delegated property smart cast issues. P10 Rule 4: decompose oversized functions in MainActivity, ExportShareScreen, BunkerScreen, AppPermissionsScreen, PermissionsManagementScreen, SigningHistoryScreen, ImportShareScreen, and Nip55ContentProvider.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughThis pull request refactors the application's Kotlin UI and storage layers by extracting large composable functions into modular components, centralizing dialog logic into reusable composables, hardening input validation in storage operations, and reorganizing permission/policy checking in the content provider. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/io/privkey/keep/nip55/SigningHistoryScreen.kt (1)
43-43:⚠️ Potential issue | 🟡 Minor
loadErroris set but never displayed in the UI.The
loadErrorstate is updated on failure (lines 59, 61, 75) but is never rendered anywhere in the composable. Users won't see feedback when log loading fails. Consider displaying it (e.g., via aStatusCardorText) or removing the variable if error display isn't needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/io/privkey/keep/nip55/SigningHistoryScreen.kt` at line 43, The loadError state in SigningHistoryScreen is updated on failures but never rendered; either display it or remove it: locate the mutable state var loadError and, inside SigningHistoryScreen's Compose tree (e.g., under the header or where empty/error states are shown), add a conditional UI that shows loadError when non-null (use your existing StatusCard or a Text with appropriate style) so users see the error, or if you intentionally don't want to surface load errors remove the loadError variable and its assignments (in functions that set it) to avoid dead state.
🧹 Nitpick comments (6)
app/src/main/kotlin/io/privkey/keep/nip46/Nip46ApprovalActivity.kt (1)
183-185: Re-throwCancellationExceptionto preserve structured concurrency.
catch (e: Exception)will interceptkotlinx.coroutines.CancellationException(which extendsIllegalStateException → RuntimeException → Exception). If thelifecycleScopeis cancelled whilestore.grantPermission(...)is executing insidewithContext(Dispatchers.IO), the cancellation signal is swallowed, the error path logs it as a routine failure, and execution falls through tofinish()— breaking Kotlin's structured-concurrency contract.The original
catch (t: Throwable)had the same flaw; this change is the right moment to fix it.♻️ Proposed fix
- } catch (e: Exception) { - if (BuildConfig.DEBUG) Log.e(TAG, "Failed to persist permission: ${e::class.simpleName}") + } catch (e: Exception) { + if (e is kotlinx.coroutines.CancellationException) throw e + if (BuildConfig.DEBUG) Log.e(TAG, "Failed to persist permission: ${e::class.simpleName}") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/io/privkey/keep/nip46/Nip46ApprovalActivity.kt` around lines 183 - 185, The catch block in Nip46ApprovalActivity (around the store.grantPermission call executed from lifecycleScope/withContext(Dispatchers.IO)) is catching Exception and thus swallowing kotlinx.coroutines.CancellationException; adjust the handler so CancellationException is re-thrown (e.g., detect kotlin.coroutines.cancellation.CancellationException or Throwable of that type and throw it) before logging/handling other exceptions so structured concurrency is preserved and cancellations propagate correctly.app/src/main/kotlin/io/privkey/keep/nip46/BunkerScreen.kt (1)
262-297: Consider disabling the "Add" button or showing a loading indicator during async validation.When the user taps "Add", the
isInternalHostcheck (single relay) oraddBunkerRelays(bunker URL) runs asynchronously. During this time the button remains tappable with no visual feedback. A brief loading state would improve UX and prevent duplicate submissions, though the outcome is idempotent so this isn't a correctness issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/io/privkey/keep/nip46/BunkerScreen.kt` around lines 262 - 297, Add a transient loading state to prevent duplicate taps and show feedback: introduce a mutable Boolean (e.g. isAdding/isLoading) that is checked at the start of the TextButton onClick and used to disable the Add button and show a small loading indicator in the button. Set isLoading = true immediately before launching async work (both the scope.launch branch that calls addBunkerRelays and the withContext(Dispatchers.IO) BunkerConfigStore.isInternalHost branch), and ensure you reset isLoading = false in all completion paths (onSuccess/onFailure, internal host branch, and after onRelaysUpdated/dismissDialog) so the button re-enables and the indicator hides; keep existing variables like newRelayUrl, addBunkerRelays, validateRelayUrl, onRelaysUpdated and dismissDialog intact.app/src/main/kotlin/io/privkey/keep/nip55/AppPermissionsScreen.kt (1)
193-208:AppPermissionsListContenthas a large parameter surface (14 params) with embedded business logic.This composable receives stores and coroutineScope directly, coupling UI to the data layer. Consider extracting the business operations into callbacks passed from the parent in a future refactor, similar to how
ImportShareInputFieldsonly receives display state and event callbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/io/privkey/keep/nip55/AppPermissionsScreen.kt` around lines 193 - 208, AppPermissionsListContent currently takes many data-layer types (SignPolicyStore, PermissionStore, CoroutineScope) and implements business logic; refactor by removing stores and coroutineScope from its signature and replace them with explicit UI event callbacks and minimal display state (e.g., onLoadPermissions(), onRevokeAllRequested(), onUpdateExpiry(expiry: Long), onSaveSettings(settings: Nip55AppSettings?), onChangeAppState(state: AppState), plus any simple data lists or flags needed for rendering), then move the actual business operations (calls into SignPolicyStore/PermissionStore and coroutineScope launches) up to the parent caller that owns those stores; update callers to pass the new callbacks and keep AppPermissionsListContent as a pure UI/composable that only receives display state and event handlers.app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt (2)
65-65: Evicting rate-limit history should also clean per‑client stateWhen the cap is hit, only
clientRequestHistoryis evicted.clientBackoffUntilandclientConsecutiveRequestscan still grow without bound and retain stale backoff for evicted clients. Consider clearing related per‑client state when evicting.♻️ Suggested tweak to evict related state
if (clientRequestHistory.size >= MAX_TRACKED_CLIENTS && !clientRequestHistory.containsKey(clientPubkey)) { - clientRequestHistory.keys.firstOrNull()?.let { clientRequestHistory.remove(it) } + clientRequestHistory.keys.firstOrNull()?.let { evicted -> + clientRequestHistory.remove(evicted) + clientBackoffUntil.remove(evicted) + clientConsecutiveRequests.remove(evicted) + } }Also applies to: 171-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt` at line 65, The eviction currently only removes entries from clientRequestHistory (MAX_TRACKED_CLIENTS) but leaves related per-client state in clientBackoffUntil and clientConsecutiveRequests, letting those maps grow and preserving stale backoff; update the eviction logic (the block handling MAX_TRACKED_CLIENTS) to also delete the same client keys from clientBackoffUntil and clientConsecutiveRequests whenever you evict from clientRequestHistory (ensure you use the same clientId/key used by clientRequestHistory removal), and apply the same cleanup in the other eviction site that mirrors this logic so all three maps stay in sync.
41-41: Consider surfacing timeout vs. "no decision"
withTimeoutOrNullreturnsnullon timeout, which is indistinguishable from "no stored decision." If you want visibility into timeouts, considerwithTimeout+TimeoutCancellationExceptionlogging.🔍 Optional timeout visibility
return runCatching { runBlocking(Dispatchers.IO) { - withTimeoutOrNull(5_000L) { - store.getPermissionDecision(callerPackage, requestType, eventKind) - } + try { + withTimeout(5_000L) { + store.getPermissionDecision(callerPackage, requestType, eventKind) + } + } catch (e: TimeoutCancellationException) { + if (BuildConfig.DEBUG) Log.w(TAG, "Permission lookup timed out") + null + } } }.getOrNull()Also applies to: Nip55ContentProvider.kt
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt` at line 41, The current use of withTimeoutOrNull in BunkerService.kt (and similarly in Nip55ContentProvider.kt) hides whether a null result came from a timeout or from "no stored decision"; replace with withTimeout around the same call and add a catch for kotlinx.coroutines.TimeoutCancellationException in the surrounding coroutine scope to explicitly log/handle timeouts (include function names where used, e.g., the method that calls withTimeoutOrNull in BunkerService and the corresponding provider method in Nip55ContentProvider), so the catch can emit a clear processLogger/error or metrics entry indicating a timeout vs a genuine null decision; keep the original null-handling logic separate from the timeout handling.app/src/main/kotlin/io/privkey/keep/nip55/PermissionsManagementScreen.kt (1)
225-266: Optional: memoize grouped permissions to reduce recomposition work.
groupByruns on every recomposition. If the list is large, consider memoizing bypermissionsto avoid repeated grouping.♻️ Suggested tweak
- val groupedPermissions = permissions.groupBy { it.callerPackage } + val groupedPermissions = remember(permissions) { + permissions.groupBy { it.callerPackage } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/io/privkey/keep/nip55/PermissionsManagementScreen.kt` around lines 225 - 266, The grouping of permissions in PermissionsGroupedList is recomputed on every recomposition; replace the direct call to permissions.groupBy { it.callerPackage } with a memoized value (e.g., use remember or derivedStateOf keyed on permissions) so groupedPermissions is only recalculated when the permissions list changes, leaving the rest of the function (LazyColumn, AppPermissionHeader, PermissionCard) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/io/privkey/keep/nip55/AppPermissionsScreen.kt`:
- Around line 167-178: The current coroutine in coroutineScope.launch swallows
errors from permissionStore.revokePermission(packageName) and
revokeNip46Client(context, pubkey) because both calls use runCatching with no
handling, so failures still cause onDismissDialog() and onDismissScreen() to
run; update the block in AppPermissionsScreen.kt to inspect the runCatching
results (or use try/catch) for permissionStore.revokePermission and
revokeNip46Client, log the exception (use Timber or Log) in debug builds and
surface a user-visible failure (e.g., show a Toast or Snackbar) if either
revocation fails, and only call onDismissDialog() / onDismissScreen() when
revocation succeeds (or branch so NIP-46 errors don’t hide permission revocation
failures); reference permissionStore.revokePermission(packageName),
revokeNip46Client(context, pubkey), coroutineScope.launch,
withContext(Dispatchers.IO), onDismissDialog, and onDismissScreen when making
these changes.
In `@app/src/main/kotlin/io/privkey/keep/nip55/PermissionsManagementScreen.kt`:
- Around line 103-126: The onDecisionChange handler currently silently returns
when findRequestType(permission.requestType) is null; update it to surface an
error before returning: inside the PermissionsGroupedList onDecisionChange
lambda (where findRequestType is called), when requestType is null set a visible
error state (e.g., set loadError or trigger the existing snackbar mechanism)
with a clear message like "Unknown request type" and then return, so users get
feedback; keep the subsequent coroutineScope launch path unchanged and reference
findRequestType, onDecisionChange, permissionStore.updatePermissionDecision, and
loadError/snackbar state so the fix is applied in the correct handler.
---
Outside diff comments:
In `@app/src/main/kotlin/io/privkey/keep/nip55/SigningHistoryScreen.kt`:
- Line 43: The loadError state in SigningHistoryScreen is updated on failures
but never rendered; either display it or remove it: locate the mutable state var
loadError and, inside SigningHistoryScreen's Compose tree (e.g., under the
header or where empty/error states are shown), add a conditional UI that shows
loadError when non-null (use your existing StatusCard or a Text with appropriate
style) so users see the error, or if you intentionally don't want to surface
load errors remove the loadError variable and its assignments (in functions that
set it) to avoid dead state.
---
Nitpick comments:
In `@app/src/main/kotlin/io/privkey/keep/nip46/BunkerScreen.kt`:
- Around line 262-297: Add a transient loading state to prevent duplicate taps
and show feedback: introduce a mutable Boolean (e.g. isAdding/isLoading) that is
checked at the start of the TextButton onClick and used to disable the Add
button and show a small loading indicator in the button. Set isLoading = true
immediately before launching async work (both the scope.launch branch that calls
addBunkerRelays and the withContext(Dispatchers.IO)
BunkerConfigStore.isInternalHost branch), and ensure you reset isLoading = false
in all completion paths (onSuccess/onFailure, internal host branch, and after
onRelaysUpdated/dismissDialog) so the button re-enables and the indicator hides;
keep existing variables like newRelayUrl, addBunkerRelays, validateRelayUrl,
onRelaysUpdated and dismissDialog intact.
In `@app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt`:
- Line 65: The eviction currently only removes entries from clientRequestHistory
(MAX_TRACKED_CLIENTS) but leaves related per-client state in clientBackoffUntil
and clientConsecutiveRequests, letting those maps grow and preserving stale
backoff; update the eviction logic (the block handling MAX_TRACKED_CLIENTS) to
also delete the same client keys from clientBackoffUntil and
clientConsecutiveRequests whenever you evict from clientRequestHistory (ensure
you use the same clientId/key used by clientRequestHistory removal), and apply
the same cleanup in the other eviction site that mirrors this logic so all three
maps stay in sync.
- Line 41: The current use of withTimeoutOrNull in BunkerService.kt (and
similarly in Nip55ContentProvider.kt) hides whether a null result came from a
timeout or from "no stored decision"; replace with withTimeout around the same
call and add a catch for kotlinx.coroutines.TimeoutCancellationException in the
surrounding coroutine scope to explicitly log/handle timeouts (include function
names where used, e.g., the method that calls withTimeoutOrNull in BunkerService
and the corresponding provider method in Nip55ContentProvider), so the catch can
emit a clear processLogger/error or metrics entry indicating a timeout vs a
genuine null decision; keep the original null-handling logic separate from the
timeout handling.
In `@app/src/main/kotlin/io/privkey/keep/nip46/Nip46ApprovalActivity.kt`:
- Around line 183-185: The catch block in Nip46ApprovalActivity (around the
store.grantPermission call executed from
lifecycleScope/withContext(Dispatchers.IO)) is catching Exception and thus
swallowing kotlinx.coroutines.CancellationException; adjust the handler so
CancellationException is re-thrown (e.g., detect
kotlin.coroutines.cancellation.CancellationException or Throwable of that type
and throw it) before logging/handling other exceptions so structured concurrency
is preserved and cancellations propagate correctly.
In `@app/src/main/kotlin/io/privkey/keep/nip55/AppPermissionsScreen.kt`:
- Around line 193-208: AppPermissionsListContent currently takes many data-layer
types (SignPolicyStore, PermissionStore, CoroutineScope) and implements business
logic; refactor by removing stores and coroutineScope from its signature and
replace them with explicit UI event callbacks and minimal display state (e.g.,
onLoadPermissions(), onRevokeAllRequested(), onUpdateExpiry(expiry: Long),
onSaveSettings(settings: Nip55AppSettings?), onChangeAppState(state: AppState),
plus any simple data lists or flags needed for rendering), then move the actual
business operations (calls into SignPolicyStore/PermissionStore and
coroutineScope launches) up to the parent caller that owns those stores; update
callers to pass the new callbacks and keep AppPermissionsListContent as a pure
UI/composable that only receives display state and event handlers.
In `@app/src/main/kotlin/io/privkey/keep/nip55/PermissionsManagementScreen.kt`:
- Around line 225-266: The grouping of permissions in PermissionsGroupedList is
recomputed on every recomposition; replace the direct call to
permissions.groupBy { it.callerPackage } with a memoized value (e.g., use
remember or derivedStateOf keyed on permissions) so groupedPermissions is only
recalculated when the permissions list changes, leaving the rest of the function
(LazyColumn, AppPermissionHeader, PermissionCard) unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/src/main/kotlin/io/privkey/keep/ExportShareScreen.ktapp/src/main/kotlin/io/privkey/keep/ImportNsecScreen.ktapp/src/main/kotlin/io/privkey/keep/ImportShareScreen.ktapp/src/main/kotlin/io/privkey/keep/KeepMobileApp.ktapp/src/main/kotlin/io/privkey/keep/MainActivity.ktapp/src/main/kotlin/io/privkey/keep/nip46/BunkerScreen.ktapp/src/main/kotlin/io/privkey/keep/nip46/BunkerService.ktapp/src/main/kotlin/io/privkey/keep/nip46/Nip46ApprovalActivity.ktapp/src/main/kotlin/io/privkey/keep/nip55/AppPermissionsScreen.ktapp/src/main/kotlin/io/privkey/keep/nip55/ConnectedAppsScreen.ktapp/src/main/kotlin/io/privkey/keep/nip55/Nip55ContentProvider.ktapp/src/main/kotlin/io/privkey/keep/nip55/PermissionsManagementScreen.ktapp/src/main/kotlin/io/privkey/keep/nip55/SigningHistoryScreen.ktapp/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt
app/src/main/kotlin/io/privkey/keep/nip55/PermissionsManagementScreen.kt
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security: remove debug log leaking group pubkey, add timeout to BunkerService runBlocking, narrow catch(Throwable) to catch(Exception).
P10 Rule 5: add require() preconditions to AndroidKeystoreStorage and BunkerService security-critical functions.
P10 Rule 7: replace !! with safe local variable captures, fix delegated property smart cast issues.
P10 Rule 4: decompose oversized functions in MainActivity, ExportShareScreen, BunkerScreen, AppPermissionsScreen, PermissionsManagementScreen, SigningHistoryScreen, ImportShareScreen, and Nip55ContentProvider.
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor