Open
Conversation
WalkthroughAdds a Wallet Descriptor feature: a new WalletDescriptorCard UI, WalletDescriptorScreen composable, DescriptorSessionManager (state, callbacks, pending proposals), PollResult descriptorCount wiring, account-switch cleanup, and unit tests for the session manager. Changes
Sequence DiagramssequenceDiagram
actor User
participant UI as "WalletDescriptorScreen\nrgba(54,162,235,0.5)"
participant Backend as "KeepMobile\nrgba(75,192,192,0.5)"
participant Manager as "DescriptorSessionManager\nrgba(255,159,64,0.5)"
User->>UI: Open Wallet Descriptor screen
UI->>Backend: walletDescriptorList()
Backend-->>UI: descriptors
UI->>Manager: register callbacks / observe state
Manager-->>UI: emit current state
sequenceDiagram
actor User
participant UI as "WalletDescriptorScreen\nrgba(54,162,235,0.5)"
participant Backend as "KeepMobile\nrgba(75,192,192,0.5)"
participant Manager as "DescriptorSessionManager\nrgba(255,159,64,0.5)"
User->>UI: Propose new descriptor
UI->>Backend: proposeDescriptor()
Backend-->>Manager: onProposed(sessionId)
Manager->>Manager: state = Proposed(sessionId)
Manager-->>UI: state update (pending)
User->>UI: Approve / contribute
UI->>Backend: approveDescriptor() / contribute
Backend-->>Manager: onContributionNeeded/onContributed
Manager-->>UI: state updates (ContributionNeeded/Contributed)
Backend-->>Manager: onComplete(sessionId, external, internal)
Manager->>Manager: state = Complete(...)
Manager-->>UI: emit completion (enable export/delete)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Suggested reviewers
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 |
![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
🧹 Nitpick comments (1)
app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt (1)
52-100: Avoid duplicate pending proposals if callbacks repeat.Defensive dedupe by sessionId prevents duplicate rows and repeated actions when the backend replays events.
♻️ Proposed dedupe safeguard
override fun onContributionNeeded(proposal: DescriptorProposal) { - _pendingProposals.update { it + proposal } + _pendingProposals.update { existing -> + if (existing.any { it.sessionId == proposal.sessionId }) existing else existing + proposal + } _state.value = DescriptorSessionState.ContributionNeeded(proposal) }🤖 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/descriptor/WalletDescriptorScreen.kt` around lines 52 - 100, The pending proposals list can receive duplicates when callbacks repeat; update the onContributionNeeded handler in DescriptorSessionManager (inside createCallbacks) to dedupe by sessionId before appending: instead of _pendingProposals.update { it + proposal } only add the proposal when no existing item has the same DescriptorProposal.sessionId (e.g., _pendingProposals.update { list -> if (list.any { it.sessionId == proposal.sessionId }) list else list + proposal }); keep removePendingProposal, clearAll and other state transitions 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/descriptor/WalletDescriptorScreen.kt`:
- Around line 102-266: The refreshDescriptors function currently calls
keepMobile.walletDescriptorList() without error handling, which can crash the
UI; update refreshDescriptors to wrap the network call in runCatching (or
try/catch) inside the existing scope.launch/withContext(Dispatchers.IO) block,
only assign to the descriptors state on success, and handle failures by showing
a Toast (or logging) instead of throwing; reference the refreshDescriptors
function, the descriptors mutable state, and keepMobile.walletDescriptorList to
locate where to add runCatching/onFailure handling.
---
Nitpick comments:
In `@app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt`:
- Around line 52-100: The pending proposals list can receive duplicates when
callbacks repeat; update the onContributionNeeded handler in
DescriptorSessionManager (inside createCallbacks) to dedupe by sessionId before
appending: instead of _pendingProposals.update { it + proposal } only add the
proposal when no existing item has the same DescriptorProposal.sessionId (e.g.,
_pendingProposals.update { list -> if (list.any { it.sessionId ==
proposal.sessionId }) list else list + proposal }); keep removePendingProposal,
clearAll and other state transitions unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/main/kotlin/io/privkey/keep/ConnectionCards.ktapp/src/main/kotlin/io/privkey/keep/KeepMobileApp.ktapp/src/main/kotlin/io/privkey/keep/MainActivity.ktapp/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt
app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt
Show resolved
Hide resolved
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/MainActivity.kt (1)
403-428:⚠️ Potential issue | 🟠 Major
walletDescriptorList()failure in the polling loop would halt all polling.Line 413 adds
keepMobile.walletDescriptorList().sizeto the polling block. If this call throws (e.g., network error), the entirerepeatloop terminates because there's no try-catch around the polling body. This is a pre-existing fragility for all calls in this block, but descriptor list fetching (potentially a network call) increases the surface area.Consider wrapping the descriptor count fetch independently so a failure doesn't halt peer/pending-count polling:
Proposed safeguard
val pc = if (h) keepMobile.getPendingRequests().size else 0 - val dc = if (h) keepMobile.walletDescriptorList().size else 0 + val dc = if (h) runCatching { keepMobile.walletDescriptorList().size }.getOrDefault(0) else 0 PollResult(h, s, a, k, p, pc, dc)🤖 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/MainActivity.kt` around lines 403 - 428, The polling loop inside LaunchedEffect constructs a PollResult by calling keepMobile.walletDescriptorList().size which, if it throws, will cancel the entire repeat loop; change the descriptor count fetch to be isolated and resilient—call walletDescriptorList() inside its own try/catch or runCatching when building the PollResult (or compute dc separately before constructing PollResult), and on failure return a safe fallback (e.g., previous descriptorCount or 0) and log the error instead of letting the exception propagate; ensure the rest of the PollResult values (hasShare, getPeers, getPendingRequests, etc.) remain untouched so polling continues even if walletDescriptorList() fails.
♻️ Duplicate comments (1)
app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt (1)
120-130: Previous review finding addressed —refreshDescriptorsis now guarded withrunCatching.The error handling wraps the IO call and shows a toast on failure. Looks good.
🤖 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/descriptor/WalletDescriptorScreen.kt` around lines 120 - 130, The previous issue has been addressed: refreshDescriptors now wraps the IO call in runCatching and shows a toast on failure; no further changes required—leave the refreshDescriptors function (scope.launch { runCatching { withContext(Dispatchers.IO) { keepMobile.walletDescriptorList() } }... }) and its descriptors assignment/toast handling as-is.
🧹 Nitpick comments (4)
app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt (3)
55-103:DescriptorSessionManageras a global singleton limits testability and multi-account scenarios.A singleton
objectcouples the state machine to global scope, making it harder to test in isolation (as noted in the test file) and impossible to run concurrent sessions for different accounts. Consider refactoring to a class instance managed by DI if multi-account or parallel descriptor flows become a requirement.🤖 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/descriptor/WalletDescriptorScreen.kt` around lines 55 - 103, DescriptorSessionManager is defined as a global object which prevents isolated testing and multi-account/parallel sessions; refactor it into a regular class (e.g., class DescriptorSessionManager) so state flows (_state, _pendingProposals) and methods (createCallbacks, clearSessionState, clearAll, removePendingProposal) become instance members, then register/provide that class via your DI container or construct per-test/per-account; update all call sites and tests to obtain an instance (instead of referencing the object) and ensure createCallbacks returns callbacks bound to the instance so session events modify the instance state rather than global state.
483-501: Redundant re-parsing on button click.Lines 484–486 already parse and validate
threshold/timelockMonthsto computevalid, which disables the button. Lines 489–491 re-parse and re-validate on click. This is defensive, but the double work could be simplified by passing the pre-validated values directly.🤖 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/descriptor/WalletDescriptorScreen.kt` around lines 483 - 501, The confirm button currently parses threshold and timelockMonths twice (once to compute valid and again inside the onClick), so parse them once into local values (e.g., parsedThreshold and parsedTimelock) outside the TextButton and use those for both the enabled check and the onClick; update the confirmButton lambda (the TextButton in WalletDescriptorScreen) to use the pre-parsed parsedThreshold/parsedTimelock and call onPropose(network, listOf(RecoveryTierConfig(parsedThreshold, parsedTimelock))) directly, removing the redundant re-parsing and duplicate range checks while keeping the same validity constraints (1u..15u and 1u..120u).
62-89: Non-atomic dual-state mutation in callbacks.In
onContributionNeeded,onComplete, andonFailed, two separateStateFlows (_pendingProposalsand_state) are updated sequentially. Between these two writes, an observer could see an inconsistent snapshot (e.g., proposals updated but state still stale). In practice this is low-risk since Compose batches recompositions, but worth noting if these callbacks can fire off the main thread.🤖 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/descriptor/WalletDescriptorScreen.kt` around lines 62 - 89, The callbacks update two separate StateFlows (_pendingProposals and _state) non-atomically in onContributionNeeded, onComplete and onFailed; protect these paired writes by serializing them with a small mutex (e.g., declare a private val descriptorCallbacksMutex = Mutex()) and wrap the paired updates inside descriptorCallbacksMutex.withLock { ... } so onContributionNeeded does descriptorCallbacksMutex.withLock { _pendingProposals.update { it + proposal }; _state.value = DescriptorSessionState.ContributionNeeded(proposal) } and similarly wrap removePendingProposal + _state.value assignments in onComplete and onFailed (and update the removePendingProposal implementation to use the same mutex if it mutates _pendingProposals) to ensure observers never see a partial state.app/src/test/kotlin/io/privkey/keep/descriptor/DescriptorSessionManagerTest.kt (1)
12-17: Singleton-based test isolation relies on sequential execution.Since
DescriptorSessionManageris a globalobject, these tests are only safe when run sequentially. JUnit 4 does this by default within a single class, but if any other test class (or future parallel test runner) also interacts withDescriptorSessionManager, you could see flaky failures. Consider noting this constraint or, longer-term, refactoringDescriptorSessionManagerto be injectable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/kotlin/io/privkey/keep/descriptor/DescriptorSessionManagerTest.kt` around lines 12 - 17, Tests rely on the global DescriptorSessionManager singleton which can cause cross-test or cross-class flakiness under parallel execution; update the test to explicitly document this constraint and make the expectation clear (add a comment above setup() referencing DescriptorSessionManager.clearAll()), and as a longer-term fix refactor the singleton into an injectable instance (replace the DescriptorSessionManager object with a class/instance or introduce a provider interface and use dependency injection in the code under test) so tests can create and inject isolated instances instead of relying on the global clearAll().
🤖 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/descriptor/WalletDescriptorScreen.kt`:
- Around line 225-241: The dialog is being closed regardless of success because
showProposeDialog = false is set outside the runCatching block; update the
ProposeDescriptorDialog onPropose handler so that you only set showProposeDialog
= false when keepMobile.walletDescriptorPropose succeeds (e.g., move the
assignment into the runCatching success path or use onSuccess), and optionally
preserve the input state on failure so the user doesn't need to re-enter data;
look for the onPropose lambda and the runCatching surrounding
keepMobile.walletDescriptorPropose to make this change.
- Around line 264-282: The delete dialog is always dismissed because
showDeleteConfirm = null is executed regardless of success; change the flow so
showDeleteConfirm is only cleared when the deletion actually succeeds (e.g.,
move the assignment into the success path of runCatching or call it inside
.onSuccess after keepMobile.walletDescriptorDelete and refreshDescriptors
complete), and leave the DeleteDescriptorDialog open on .onFailure while still
showing the Toast; update the handlers around DeleteDescriptorDialog,
runCatching, keepMobile.walletDescriptorDelete and refreshDescriptors
accordingly so failures do not close the dialog prematurely.
---
Outside diff comments:
In `@app/src/main/kotlin/io/privkey/keep/MainActivity.kt`:
- Around line 403-428: The polling loop inside LaunchedEffect constructs a
PollResult by calling keepMobile.walletDescriptorList().size which, if it
throws, will cancel the entire repeat loop; change the descriptor count fetch to
be isolated and resilient—call walletDescriptorList() inside its own try/catch
or runCatching when building the PollResult (or compute dc separately before
constructing PollResult), and on failure return a safe fallback (e.g., previous
descriptorCount or 0) and log the error instead of letting the exception
propagate; ensure the rest of the PollResult values (hasShare, getPeers,
getPendingRequests, etc.) remain untouched so polling continues even if
walletDescriptorList() fails.
---
Duplicate comments:
In `@app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt`:
- Around line 120-130: The previous issue has been addressed: refreshDescriptors
now wraps the IO call in runCatching and shows a toast on failure; no further
changes required—leave the refreshDescriptors function (scope.launch {
runCatching { withContext(Dispatchers.IO) { keepMobile.walletDescriptorList() }
}... }) and its descriptors assignment/toast handling as-is.
---
Nitpick comments:
In `@app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt`:
- Around line 55-103: DescriptorSessionManager is defined as a global object
which prevents isolated testing and multi-account/parallel sessions; refactor it
into a regular class (e.g., class DescriptorSessionManager) so state flows
(_state, _pendingProposals) and methods (createCallbacks, clearSessionState,
clearAll, removePendingProposal) become instance members, then register/provide
that class via your DI container or construct per-test/per-account; update all
call sites and tests to obtain an instance (instead of referencing the object)
and ensure createCallbacks returns callbacks bound to the instance so session
events modify the instance state rather than global state.
- Around line 483-501: The confirm button currently parses threshold and
timelockMonths twice (once to compute valid and again inside the onClick), so
parse them once into local values (e.g., parsedThreshold and parsedTimelock)
outside the TextButton and use those for both the enabled check and the onClick;
update the confirmButton lambda (the TextButton in WalletDescriptorScreen) to
use the pre-parsed parsedThreshold/parsedTimelock and call onPropose(network,
listOf(RecoveryTierConfig(parsedThreshold, parsedTimelock))) directly, removing
the redundant re-parsing and duplicate range checks while keeping the same
validity constraints (1u..15u and 1u..120u).
- Around line 62-89: The callbacks update two separate StateFlows
(_pendingProposals and _state) non-atomically in onContributionNeeded,
onComplete and onFailed; protect these paired writes by serializing them with a
small mutex (e.g., declare a private val descriptorCallbacksMutex = Mutex()) and
wrap the paired updates inside descriptorCallbacksMutex.withLock { ... } so
onContributionNeeded does descriptorCallbacksMutex.withLock {
_pendingProposals.update { it + proposal }; _state.value =
DescriptorSessionState.ContributionNeeded(proposal) } and similarly wrap
removePendingProposal + _state.value assignments in onComplete and onFailed (and
update the removePendingProposal implementation to use the same mutex if it
mutates _pendingProposals) to ensure observers never see a partial state.
In
`@app/src/test/kotlin/io/privkey/keep/descriptor/DescriptorSessionManagerTest.kt`:
- Around line 12-17: Tests rely on the global DescriptorSessionManager singleton
which can cause cross-test or cross-class flakiness under parallel execution;
update the test to explicitly document this constraint and make the expectation
clear (add a comment above setup() referencing
DescriptorSessionManager.clearAll()), and as a longer-term fix refactor the
singleton into an injectable instance (replace the DescriptorSessionManager
object with a class/instance or introduce a provider interface and use
dependency injection in the code under test) so tests can create and inject
isolated instances instead of relying on the global clearAll().
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/kotlin/io/privkey/keep/MainActivity.ktapp/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.ktapp/src/test/kotlin/io/privkey/keep/descriptor/DescriptorSessionManagerTest.kt
app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt
Show resolved
Hide resolved
app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt (1)
67-69: Deduplicate pending proposals bysessionIdbefore appending.Line 68 always appends, so callback retries/replays can create duplicate pending rows for the same session. Guarding by
sessionIdkeeps UI/actions idempotent.Proposed refinement
override fun onContributionNeeded(proposal: DescriptorProposal) { - _pendingProposals.update { it + proposal } + _pendingProposals.update { current -> + if (current.any { it.sessionId == proposal.sessionId }) current else current + proposal + } _state.value = DescriptorSessionState.ContributionNeeded(proposal) }🤖 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/descriptor/WalletDescriptorScreen.kt` around lines 67 - 69, onContributionNeeded currently always appends the incoming DescriptorProposal to _pendingProposals, causing duplicates if the callback is retried; update the override fun onContributionNeeded(proposal: DescriptorProposal) to check existing _pendingProposals for a matching sessionId on DescriptorProposal and only append if no entry with the same sessionId exists, then continue to set _state.value = DescriptorSessionState.ContributionNeeded(proposal); reference the _pendingProposals.update call and the DescriptorProposal.sessionId property when implementing the guard to keep the action idempotent.
🤖 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/descriptor/WalletDescriptorScreen.kt`:
- Around line 122-128: The onFailure handlers attached to your runCatching
blocks (e.g., the block wrapping withContext(Dispatchers.IO) {
keepMobile.walletDescriptorList() } that sets descriptors and shows Toasts)
currently swallow all throwables including CancellationException; update each
onFailure { ... } to rethrow if the caught exception is a CancellationException
(e.g., if (it is CancellationException) throw it) and only show the error Toast
or handle non-cancellation exceptions otherwise so coroutine cancellation
semantics are preserved.
---
Nitpick comments:
In `@app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt`:
- Around line 67-69: onContributionNeeded currently always appends the incoming
DescriptorProposal to _pendingProposals, causing duplicates if the callback is
retried; update the override fun onContributionNeeded(proposal:
DescriptorProposal) to check existing _pendingProposals for a matching sessionId
on DescriptorProposal and only append if no entry with the same sessionId
exists, then continue to set _state.value =
DescriptorSessionState.ContributionNeeded(proposal); reference the
_pendingProposals.update call and the DescriptorProposal.sessionId property when
implementing the guard to keep the action idempotent.
app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt (1)
262-275:⚠️ Potential issue | 🟡 MinorKeep the export dialog open when export fails.
showExportDialog = nullat Line 274 runs even after failure, so a transient export error forces users to reopen and reselect format.Suggested fix
onExport = { format -> scope.launch { runCatching { - val exported = withContext(Dispatchers.IO) { + withContext(Dispatchers.IO) { keepMobile.walletDescriptorExport(descriptor.groupPubkey, format) } - copySensitiveText(context, exported) - Toast.makeText(context, "Copied to clipboard", Toast.LENGTH_SHORT).show() + }.onSuccess { exported -> + copySensitiveText(context, exported) + Toast.makeText(context, "Copied to clipboard", Toast.LENGTH_SHORT).show() + showExportDialog = null }.onFailure { e -> if (e is CancellationException) throw e Log.w(TAG, "Failed to export descriptor", e) Toast.makeText(context, "Export failed", Toast.LENGTH_LONG).show() } - showExportDialog = null } },🤖 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/descriptor/WalletDescriptorScreen.kt` around lines 262 - 275, The export dialog is being closed unconditionally because showExportDialog = null runs after runCatching even on failure; change the flow so showExportDialog is cleared only on successful export—e.g., use runCatching(...).onSuccess { showExportDialog = null } (or move the assignment into the success block after copySensitiveText) while keeping the current CancellationException rethrow behavior and existing error handling for keepMobile.walletDescriptorExport and copySensitiveText.
🧹 Nitpick comments (1)
app/src/test/kotlin/io/privkey/keep/descriptor/DescriptorSessionManagerTest.kt (1)
101-110: Add a regression test for duplicatesessionIdproposals.
DescriptorSessionManagerdeduplicates pending proposals bysessionIdinapp/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt(Lines 73-75), but this suite doesn’t currently lock that behavior.Suggested test case
+ `@Test` + fun `duplicate sessionId does not duplicate pending proposals`() = runTest { + val callbacks = DescriptorSessionManager.createCallbacks() + val p1 = makeProposal(sessionId = "dup", network = "bitcoin") + val p2 = makeProposal(sessionId = "dup", network = "signet") + + callbacks.onContributionNeeded(p1) + callbacks.onContributionNeeded(p2) + + val pending = DescriptorSessionManager.pendingProposals.first() + assertEquals(1, pending.size) + assertEquals("dup", pending[0].sessionId) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/kotlin/io/privkey/keep/descriptor/DescriptorSessionManagerTest.kt` around lines 101 - 110, The test currently checks accumulation of proposals but doesn’t assert the deduplication-by-sessionId behavior; add a new or update the existing test in DescriptorSessionManagerTest to submit two proposals with the same sessionId (using DescriptorSessionManager.createCallbacks() and callbacks.onContributionNeeded(...)) and one with a different sessionId, then read DescriptorSessionManager.pendingProposals.first() and assert that proposals are deduplicated by sessionId (i.e., size reflects unique sessionIds and the duplicate sessionId appears only once); reference the methods createCallbacks, onContributionNeeded and the pendingProposals flow to locate and modify the test.
🤖 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/MainActivity.kt`:
- Around line 399-404: The current runCatching around
keepMobile.walletDescriptorSetCallbacks swallows all non-cancellation
exceptions; update the onFailure handler so that CancellationException is
rethrown but any other Throwable is surfaced (e.g., logged and/or reported)
instead of ignored. Specifically, modify the runCatching { ... }.onFailure block
that surrounds
keepMobile.walletDescriptorSetCallbacks(DescriptorSessionManager.createCallbacks())
so non-cancellation errors are logged with a clear message (include the
exception) and/or reported to your error-tracking system, ensuring failures to
register descriptor callbacks are visible while preserving cancellation
behavior; keep the surrounding lifecycleOwner.lifecycle.repeatOnLifecycle usage
intact.
---
Duplicate comments:
In `@app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt`:
- Around line 262-275: The export dialog is being closed unconditionally because
showExportDialog = null runs after runCatching even on failure; change the flow
so showExportDialog is cleared only on successful export—e.g., use
runCatching(...).onSuccess { showExportDialog = null } (or move the assignment
into the success block after copySensitiveText) while keeping the current
CancellationException rethrow behavior and existing error handling for
keepMobile.walletDescriptorExport and copySensitiveText.
---
Nitpick comments:
In
`@app/src/test/kotlin/io/privkey/keep/descriptor/DescriptorSessionManagerTest.kt`:
- Around line 101-110: The test currently checks accumulation of proposals but
doesn’t assert the deduplication-by-sessionId behavior; add a new or update the
existing test in DescriptorSessionManagerTest to submit two proposals with the
same sessionId (using DescriptorSessionManager.createCallbacks() and
callbacks.onContributionNeeded(...)) and one with a different sessionId, then
read DescriptorSessionManager.pendingProposals.first() and assert that proposals
are deduplicated by sessionId (i.e., size reflects unique sessionIds and the
duplicate sessionId appears only once); reference the methods createCallbacks,
onContributionNeeded and the pendingProposals flow to locate and modify the
test.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/kotlin/io/privkey/keep/MainActivity.ktapp/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.ktapp/src/test/kotlin/io/privkey/keep/descriptor/DescriptorSessionManagerTest.kt
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/main/kotlin/io/privkey/keep/MainActivity.kt (2)
399-407:⚠️ Potential issue | 🟡 MinorCallback registration failure still lacks user-visible notification.
Log.ewas added (addressing logging from the prior review), but the user-facingToastwas not. If callback registration fails silently, descriptor session updates stop working without any indication to the user.🛡️ Proposed fix to surface the failure
}.onFailure { if (it is CancellationException) throw it Log.e("MainActivity", "Failed to set descriptor callbacks", it) + Toast.makeText(appContext, "Descriptor updates unavailable", Toast.LENGTH_SHORT).show() }🤖 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/MainActivity.kt` around lines 399 - 407, The runCatching block that calls keepMobile.walletDescriptorSetCallbacks(DescriptorSessionManager.createCallbacks()) logs failures but doesn't notify the user; update the onFailure handler to rethrow CancellationException as it does, and for other exceptions show a user-visible Toast (or Snackbar) with a short error message (e.g., "Failed to set descriptor callbacks") in addition to calling Log.e, so the user is informed when descriptor callback registration fails; ensure you use the Activity context (this or lifecycleOwner) when creating the Toast and keep the existing Log.e invocation.
416-420:⚠️ Potential issue | 🟡 MinorSilent swallow of
walletDescriptorList()failures in the polling hot path.Non-
CancellationExceptionfailures are silently collapsed to0on every 10-second poll cycle with no log output, making it impossible to distinguish "no descriptors" from "call failed".🐛 Proposed fix to log failures
val dc = if (h) { runCatching { keepMobile.walletDescriptorList().size } - .onFailure { if (it is CancellationException) throw it } + .onFailure { e -> + if (e is CancellationException) throw e + Log.w("MainActivity", "Failed to fetch descriptor count", e) + } .getOrDefault(0) } else 0🤖 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/MainActivity.kt` around lines 416 - 420, The polling hot path currently swallows all non-CancellationException errors from keepMobile.walletDescriptorList() by using runCatching().getOrDefault(0); change that runCatching block to rethrow CancellationException but log other exceptions before returning 0 so failures aren’t indistinguishable from "no descriptors". Specifically, update the block around keepMobile.walletDescriptorList() (the dc assignment) to: use runCatching { keepMobile.walletDescriptorList().size } .onFailure { if (it is CancellationException) throw it else Log.e("MainActivity", "walletDescriptorList failed", it) } .getOrDefault(0) (or use the existing app logger if one exists) so errors are recorded while preserving the 0 fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/kotlin/io/privkey/keep/MainActivity.kt`:
- Around line 399-407: The runCatching block that calls
keepMobile.walletDescriptorSetCallbacks(DescriptorSessionManager.createCallbacks())
logs failures but doesn't notify the user; update the onFailure handler to
rethrow CancellationException as it does, and for other exceptions show a
user-visible Toast (or Snackbar) with a short error message (e.g., "Failed to
set descriptor callbacks") in addition to calling Log.e, so the user is informed
when descriptor callback registration fails; ensure you use the Activity context
(this or lifecycleOwner) when creating the Toast and keep the existing Log.e
invocation.
- Around line 416-420: The polling hot path currently swallows all
non-CancellationException errors from keepMobile.walletDescriptorList() by using
runCatching().getOrDefault(0); change that runCatching block to rethrow
CancellationException but log other exceptions before returning 0 so failures
aren’t indistinguishable from "no descriptors". Specifically, update the block
around keepMobile.walletDescriptorList() (the dc assignment) to: use runCatching
{ keepMobile.walletDescriptorList().size } .onFailure { if (it is
CancellationException) throw it else Log.e("MainActivity", "walletDescriptorList
failed", it) } .getOrDefault(0) (or use the existing app logger if one exists)
so errors are recorded while preserving the 0 fallback.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt (1)
291-304:⚠️ Potential issue | 🟡 MinorKeep the export dialog open when export fails.
The dialog is closed regardless of outcome, so transient failures force the user to re-open and repeat actions.
Suggested fix
scope.launch { runCatching { val exported = withContext(Dispatchers.IO) { keepMobile.walletDescriptorExport(descriptor.groupPubkey, format) } copySensitiveText(context, exported) Toast.makeText(context, "Copied to clipboard", Toast.LENGTH_SHORT).show() + }.onSuccess { + showExportDialog = null }.onFailure { e -> if (e is CancellationException) throw e Log.w(TAG, "Failed to export descriptor", e) Toast.makeText(context, "Export failed", Toast.LENGTH_LONG).show() } isExporting = false - showExportDialog = null }🤖 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/descriptor/WalletDescriptorScreen.kt` around lines 291 - 304, The export dialog is always closed because isExporting is set to false and showExportDialog is cleared after the runCatching block; change this so that showExportDialog is only cleared when the export succeeds. Specifically, move the showExportDialog = null (and any UI success state transitions) into the success path of the runCatching (e.g., after copySensitiveText/Toast on success) and ensure isExporting is still reset to false in both success and failure paths; keep the existing CancellationException rethrow and leave showExportDialog untouched inside the onFailure block so the dialog remains open on error.
🤖 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/descriptor/WalletDescriptorScreen.kt`:
- Around line 209-219: The removePendingProposal call and inFlightSessions
update are executed regardless of whether the backend action succeeded; change
the flow so DescriptorSessionManager.removePendingProposal(proposal.sessionId)
and inFlightSessions = inFlightSessions - proposal.sessionId are only executed
after a successful call to block(proposal.sessionId). In the scope.launch block
around runCatching { withContext(Dispatchers.IO) { block(proposal.sessionId) }
}, move the removal/update into the success path (e.g., in .onSuccess or after
checking result.isSuccess) while keeping the existing .onFailure behavior that
rethrows CancellationException and logs/shows the toast on errors.
In `@app/src/main/kotlin/io/privkey/keep/MainActivity.kt`:
- Around line 419-424: The current logic forces dc to 0 on any transient failure
when fetching keepMobile.walletDescriptorList(), which can show a spurious empty
state; change it to preserve the previous descriptor count until a successful
fetch by making the fetch update conditional: run the runCatching block and only
replace the existing dc value when it succeeds (rethrow CancellationException),
otherwise leave the prior dc unchanged; apply the same pattern for the other
occurrences (the similar blocks around lines 432 and 1213) and ensure the value
passed into PollResult(...) is the preserved previous count unless the fetch
succeeds.
---
Duplicate comments:
In `@app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt`:
- Around line 291-304: The export dialog is always closed because isExporting is
set to false and showExportDialog is cleared after the runCatching block; change
this so that showExportDialog is only cleared when the export succeeds.
Specifically, move the showExportDialog = null (and any UI success state
transitions) into the success path of the runCatching (e.g., after
copySensitiveText/Toast on success) and ensure isExporting is still reset to
false in both success and failure paths; keep the existing CancellationException
rethrow and leave showExportDialog untouched inside the onFailure block so the
dialog remains open on error.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/kotlin/io/privkey/keep/MainActivity.ktapp/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.ktapp/src/test/kotlin/io/privkey/keep/descriptor/DescriptorSessionManagerTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/test/kotlin/io/privkey/keep/descriptor/DescriptorSessionManagerTest.kt
app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.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.
Summary
Test Results
Tested on-device with 2x Pixel 9a + keep-desktop (2-of-3 FROST group, signet):
Device Configuration
Key Findings
Summary by CodeRabbit
Release Notes
New Features
Tests