Adopt Rust-driven shared state and drop duplicated Kotlin logic (#175)#178
Merged
kwsantiago merged 7 commits intomainfrom Mar 2, 2026
Merged
Adopt Rust-driven shared state and drop duplicated Kotlin logic (#175)#178kwsantiago merged 7 commits intomainfrom
kwsantiago merged 7 commits intomainfrom
Conversation
Delete RelayConfigStore, ProxyConfigStore, ProfileRelayConfigStore, and BunkerConfigStore — config persistence now handled by Rust keep-mobile. Replace inline timestamp formatting with Rust formatTimestampDetailed() and all inline hex/npub truncation with Rust truncateStr(). Implement KeepStateCallback for push-based peers/connection state from Rust, removing the 10s polling loop for those values.
|
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 (15)
WalkthroughThe pull request removes four Kotlin storage classes (RelayConfigStore, ProfileRelayConfigStore, BunkerConfigStore, ProxyConfigStore) that persisted configuration locally, and migrates their functionality to KeepMobile uniffi-based APIs. Multiple UI screens and services are updated to access and persist config through KeepMobile instead of local stores. Additionally, string truncation/formatting logic is centralized via uniffi functions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
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 |
- Remove exception message from debug log to match consistent pattern - Add @volatile to currentRelays for thread safety - Add missing onDismiss() calls in delete non-active and switch error paths - Move FFI calls off main thread in BunkerScreen toggle/revoke handlers
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt (1)
49-50: Consider removing the fully-qualified name.The wildcard import
io.privkey.keep.uniffi.*at line 17 already importstruncateStr. The fully-qualified call is redundant.Suggested simplification
private fun truncateGroupPubkey(key: String): String = - io.privkey.keep.uniffi.truncateStr(key, 8u, 6u) + truncateStr(key, 8u, 6u)🤖 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 49 - 50, The method truncateGroupPubkey uses a fully-qualified call to io.privkey.keep.uniffi.truncateStr even though truncateStr is already available via the wildcard import io.privkey.keep.uniffi.*; simplify by replacing the fully-qualified call with a direct call to truncateStr(key, 8u, 6u) inside truncateGroupPubkey to remove redundancy and improve readability.app/src/main/kotlin/io/privkey/keep/ShareDetailsScreen.kt (1)
96-99: Note: Visible prefix reduced from 24 to 12 characters.The truncation parameters changed from showing 24 prefix characters to 12. For npub identification, the first 12 characters after "npub1" may provide less uniqueness for users to visually verify their key. Consider whether
truncateStr(npub, 16u, 8u)or similar would better balance compactness with identifiability.🤖 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/ShareDetailsScreen.kt` around lines 96 - 99, The displayed npub prefix was shortened to 12 chars which may reduce user identifiability; update the truncation call in ShareDetailsScreen (the Text that currently uses io.privkey.keep.uniffi.truncateStr(npub, 12u, 8u)) to a larger prefix such as truncateStr(npub, 16u, 8u) (or another agreed value like 24u,8u) so the visible prefix balances compactness and recognizability; modify only the parameters passed to truncateStr in that Text widget to the chosen prefix length.
🤖 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/KeepMobileApp.kt`:
- Around line 63-65: Replace the plain volatile property liveState:
KeepLiveState? with an observable state holder (e.g., a private
MutableStateFlow<KeepLiveState?> or MutableState<KeepLiveState?>) and expose it
as a read-only Flow/State (StateFlow or State) so callback-driven updates emit
to consumers; update all places that currently write to liveState (the setter
sites referenced around the callback handling code and where liveState is
assigned) to call emit/value assignment on the new observable (e.g.,
liveStateFlow.value = newState or liveStateMutable.value = newState), and in
Compose consumers collect it with collectAsStateWithLifecycle() (or
collectAsState()) instead of reading the plain property so UI recomposition
happens on updates.
- Around line 186-193: The updateBunkerService function currently attempts to
start/stop BunkerService regardless of whether mobile.saveBunkerConfig
succeeded; change it so the service is only started/stopped when the config
persist succeeds: call mobile.getBunkerConfig() then attempt
mobile.saveBunkerConfig(BunkerConfigInfo(enabled, current.authorizedClients))
and if that operation fails (handle the Result from runCatching or use
onSuccess/onFailure) log the error (as you already do with BuildConfig.DEBUG and
Log.e) and return early, otherwise proceed to invoke the selected action
(BunkerService::start or BunkerService::stop) with this.
In `@app/src/main/kotlin/io/privkey/keep/nip46/BunkerScreen.kt`:
- Around line 114-121: The saveBunkerRelays write path currently fires off
keepMobile.saveRelayConfig inside scope.launch without handling failures, so
wrap the save call in a suspend helper using runCatching (or try/catch) on the
IO dispatcher, show explicit UI failure feedback if it fails, and only update UI
state variables (relays / authorizedClients) after the save returns success;
specifically update the logic around saveBunkerRelays (and the similar blocks at
the other mentioned locations) to call a shared suspend helper that invokes
keepMobile.saveRelayConfig, checks the Result, posts an error message to the UI
on failure, and only commits the new relay/authorization state on success.
- Around line 97-101: The Compose code in BunkerScreen is directly calling
keepMobile.getBunkerConfig() and keepMobile.getRelayConfig(null) inside remember
(creating bunkerConfig and relays) which can block or throw; change this to
initialize bunkerConfig and relays as safe, empty/default Compose state and then
load the actual values inside a LaunchedEffect(Dispatchers.IO) using runCatching
to call keepMobile.getBunkerConfig() and keepMobile.getRelayConfig(null); on
success update the Compose state (bunkerConfig and relays) on the main thread,
and on failure log or handle the error so composition cannot crash or freeze.
In `@app/src/main/kotlin/io/privkey/keep/nip46/NostrConnectActivity.kt`:
- Around line 217-220: The current filter chain in NostrConnectActivity calls
isInternalHost(url) on the main thread (inside the .filter { ... } lambda during
onCreate), which does DNS resolution and can block the UI; refactor so only the
cheap checks (url.startsWith("wss://") and RELAY_URL_REGEX.matches(url)) run on
the main thread, and perform the isInternalHost(url) call off the main thread
(e.g., wrap the internal-host check in withContext(Dispatchers.IO) or run the
full host-checking filter in a coroutine/Flow using Dispatchers.IO) so URI
parsing and relay internal-host filtering happen asynchronously before updating
UI. Ensure you update the code paths that consume this filtered list to be
suspendable/async if needed and keep references to RELAY_URL_REGEX and
isInternalHost unchanged.
In `@app/src/main/kotlin/io/privkey/keep/nip55/AppPermissionsScreen.kt`:
- Around line 369-376: The current runCatching block around
getKeepMobile()/getBunkerConfig()/saveBunkerConfig(...) swallows failures so the
UI may report success while the pubkey remains in authorizedClients; change it
to explicitly capture the save result and surface errors: call
(context.applicationContext as? KeepMobileApp)?.getKeepMobile(), read config via
getBunkerConfig(), compute updated list, then call
saveBunkerConfig(BunkerConfigInfo(...)) and check its Boolean return value — if
it returns false or an exception occurs, propagate or report the error to the
caller/UI instead of ignoring it (use the same symbols: getKeepMobile,
getBunkerConfig, saveBunkerConfig, BunkerConfigInfo).
In `@app/src/main/kotlin/io/privkey/keep/RelayValidation.kt`:
- Around line 18-28: isInternalHost currently performs synchronous DNS lookup
with InetAddress.getAllByName(host) and can block the main thread when called
from NostrConnectActivity.parseNostrConnectUri() in onCreate; make the DNS work
asynchronous by turning isInternalHost into a suspend function (or providing an
isInternalHostAsync) and perform InetAddress.getAllByName(host) inside
withContext(Dispatchers.IO), then update callers (e.g., parseNostrConnectUri in
NostrConnectActivity) to call the suspend version from a coroutine (or use
lifecycleScope) so DNS resolution never runs on the UI thread.
---
Nitpick comments:
In `@app/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.kt`:
- Around line 49-50: The method truncateGroupPubkey uses a fully-qualified call
to io.privkey.keep.uniffi.truncateStr even though truncateStr is already
available via the wildcard import io.privkey.keep.uniffi.*; simplify by
replacing the fully-qualified call with a direct call to truncateStr(key, 8u,
6u) inside truncateGroupPubkey to remove redundancy and improve readability.
In `@app/src/main/kotlin/io/privkey/keep/ShareDetailsScreen.kt`:
- Around line 96-99: The displayed npub prefix was shortened to 12 chars which
may reduce user identifiability; update the truncation call in
ShareDetailsScreen (the Text that currently uses
io.privkey.keep.uniffi.truncateStr(npub, 12u, 8u)) to a larger prefix such as
truncateStr(npub, 16u, 8u) (or another agreed value like 24u,8u) so the visible
prefix balances compactness and recognizability; modify only the parameters
passed to truncateStr in that Text widget to the chosen prefix length.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
app/src/androidTest/kotlin/io/privkey/keep/storage/ProfileRelayConfigStoreTest.ktapp/src/main/kotlin/io/privkey/keep/AccountActions.ktapp/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/NostrFormatting.ktapp/src/main/kotlin/io/privkey/keep/RelayValidation.ktapp/src/main/kotlin/io/privkey/keep/SecurityCards.ktapp/src/main/kotlin/io/privkey/keep/SettingsCards.ktapp/src/main/kotlin/io/privkey/keep/ShareCards.ktapp/src/main/kotlin/io/privkey/keep/ShareDetailsScreen.ktapp/src/main/kotlin/io/privkey/keep/descriptor/WalletDescriptorScreen.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/NostrConnectActivity.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/SigningHistoryScreen.ktapp/src/main/kotlin/io/privkey/keep/storage/BunkerConfigStore.ktapp/src/main/kotlin/io/privkey/keep/storage/ProfileRelayConfigStore.ktapp/src/main/kotlin/io/privkey/keep/storage/ProxyConfigStore.ktapp/src/main/kotlin/io/privkey/keep/storage/RelayConfigStore.kt
💤 Files with no reviewable changes (5)
- app/src/androidTest/kotlin/io/privkey/keep/storage/ProfileRelayConfigStoreTest.kt
- app/src/main/kotlin/io/privkey/keep/storage/ProxyConfigStore.kt
- app/src/main/kotlin/io/privkey/keep/storage/ProfileRelayConfigStore.kt
- app/src/main/kotlin/io/privkey/keep/storage/RelayConfigStore.kt
- app/src/main/kotlin/io/privkey/keep/storage/BunkerConfigStore.kt
app/src/main/kotlin/io/privkey/keep/nip46/NostrConnectActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/io/privkey/keep/nip55/AppPermissionsScreen.kt
Outdated
Show resolved
Hide resolved
- Add @volatile to pinMismatch for thread safety - Flatten nested withContext in BunkerScreen revoke handler - Preserve authorized clients when disabling bunker on account switch
- Use mutableStateOf + main-thread hop for liveState per RMP pattern - Early return in updateBunkerService when config save fails - Move BunkerScreen FFI init to LaunchedEffect(Dispatchers.IO) - Add error handling to saveBunkerRelays and toggle/revoke handlers - Move isInternalHost off main thread in NostrConnectActivity - Add failure logging to revokeNip46Client - Remove FQ truncateStr call in WalletDescriptorScreen
- Use formatTimestampDetailed() in WalletDescriptorScreen and PermissionsManagementScreen - Remove startPeriodicPeerCheck and announceJob (replaced by push-based liveState)
Switch all callers to UniFFI-exposed functions from keep-mobile: - formatPubkeyDisplay, formatEventIdDisplay, hexToNpub - isHex64, isValidNsecFormat, nsecToHex, isValidBech32Char Depends on privkeyio/keep expose-nostr-formatting-uniffi branch.
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.
Delete RelayConfigStore, ProxyConfigStore, ProfileRelayConfigStore, and BunkerConfigStore — config persistence now handled by Rust keep-mobile. Replace inline timestamp formatting with Rust formatTimestampDetailed() and all inline hex/npub truncation with Rust truncateStr(). Implement KeepStateCallback for push-based peers/connection state from Rust, removing the 10s polling loop for those values.
Summary by CodeRabbit