Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 675c02d1d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| onDispose { | ||
| pendingCallback = null | ||
| closeable.close() |
There was a problem hiding this comment.
Clearing pendingCallback in onDispose drops the only completion path for an in-flight AmberIntentLauncherRegistry.launch call if the host activity is recreated (for example, rotation/background restore) while Amber is open. In that case the suspended coroutine in launch() never resumes, the login flow stays in loading state, and subsequent launches can block behind the same mutex, so the callback should be owned by the registry/request rather than reset on composition disposal.
Useful? React with 👍 / 👎.
| relays | ||
| .ifEmpty { defaultNostrRelays } | ||
| .map(String::trim) | ||
| .filter(String::isNotEmpty) | ||
| .distinct() |
There was a problem hiding this comment.
resolvePublicRelays returns relay strings after only trim/dedup, so malformed relay entries from user metadata can be saved into account credentials. Those values are later parsed with RustRelayUrl.parse during service startup, which throws and makes the Nostr service unusable for that account; filtering/validation should happen here (or fallback to bootstrap relays when parsing fails).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds multi-signer support for Nostr accounts, including Android Amber integration, plus improved handling of read-only keys and “fallback” Nostr profile names/handles across cache merges.
Changes:
- Introduce
AmberSignerBridgeabstraction with Android implementation and stubs for Apple/JVM, wired via Koin DI. - Expand Nostr account credential model to support multiple signer types (local key, bunker/nostrconnect, Amber) and allow read-only accounts.
- Update login UI/flow to accept npub/hex/nsec input, add bunker + Amber connect actions, and harden Nostr write paths with
requireWritable().
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/src/jvmMain/kotlin/dev/dimension/flare/di/PlatformModule.jvm.kt | Registers JVM Amber bridge stub in DI. |
| shared/src/jvmMain/kotlin/dev/dimension/flare/data/network/nostr/AmberSignerBridge.jvm.kt | Adds JVM AmberSignerBridge unsupported implementation. |
| shared/src/commonTest/kotlin/dev/dimension/flare/data/network/nostr/NostrServiceTest.kt | Updates tests for new credential/signer model and read-only behavior. |
| shared/src/commonTest/kotlin/dev/dimension/flare/data/database/cache/mapper/MicroblogTest.kt | Adds regression test for preserving real Nostr profile vs fallback handle/name. |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/presenter/login/NostrLoginPresenter.kt | Adds Amber connect flow and switches login to generic “input” import. |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/model/UiProfile.kt | Prevents fallback Nostr handle/name from overwriting richer profile data. |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/model/UiAccount.kt | Introduces NostrSignerCredential + migration helpers (effectiveSigner, normalized, etc.). |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/network/nostr/NostrService.kt | Refactors signing to support read-only/local/bunker/Amber; adds relay resolution and write guards. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/network/nostr/NostrCompat.kt | Adds parsePublicKeyHex helper for npub/nprofile/hex parsing. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/network/nostr/AmberSignerBridge.kt | Defines cross-platform Amber bridge interface + unsupported bridge implementation. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/datasource/nostr/NostrDataSource.kt | Switches service lifecycle keying to signer identity and passes Amber bridge. |
| shared/src/appleMain/kotlin/dev/dimension/flare/di/PlatformModule.apple.kt | Registers Apple Amber bridge stub in DI. |
| shared/src/appleMain/kotlin/dev/dimension/flare/data/network/nostr/AmberSignerBridge.apple.kt | Adds Apple AmberSignerBridge unsupported implementation. |
| shared/src/androidMain/kotlin/dev/dimension/flare/ui/common/AmberSignerLauncherBinding.kt | Binds an ActivityResult launcher to the Amber intent registry. |
| shared/src/androidMain/kotlin/dev/dimension/flare/di/PlatformModule.android.kt | Registers Android Amber intent registry + Android Amber bridge in DI. |
| shared/src/androidMain/kotlin/dev/dimension/flare/data/network/nostr/AmberSignerBridge.android.kt | Implements Amber connect + event signing via intents/content provider. |
| shared/build.gradle.kts | Adds Android dependencies needed for Compose launcher binding + Koin compose inject. |
| compose-ui/src/commonMain/kotlin/dev/dimension/flare/ui/screen/login/ServiceSelectionScreenContent.kt | Updates Nostr login UI to support account/bunker inputs + Amber button. |
| compose-ui/src/commonMain/kotlin/dev/dimension/flare/ui/screen/login/NostrInputPresenter.kt | Tracks separate account and bunker inputs + enables states. |
| compose-ui/src/commonMain/composeResources/values/strings.xml | Adds new Nostr login strings (account/bunker/Amber). |
| compose-ui/src/commonMain/composeResources/values-ja-rJP/strings.xml | Adds Japanese translations for new Nostr login strings. |
| app/src/main/java/dev/dimension/flare/ui/AppContainer.kt | Installs the Amber launcher binding at app root. |
| app/src/main/AndroidManifest.xml | Adds nostrsigner scheme to package visibility queries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ), | ||
| ) | ||
| if (pubKey == accountKey.id) { | ||
| if (canSign && pubKey == accountKey.id) { |
There was a problem hiding this comment.
pubKey is a RustPublicKey, but here it's compared to accountKey.id (a String). This condition will never be true unless RustPublicKey.equals(String) is specially implemented, so the Delete action may never appear even for the author. Compare against pubKeyHex (or pubKey.toHex()) instead.
| if (canSign && pubKey == accountKey.id) { | |
| if (canSign && pubKeyHex == accountKey.id) { |
| internal sealed interface NostrSignerCredential { | ||
| val stableId: String | ||
|
|
||
| @Immutable | ||
| @Serializable | ||
| @SerialName("NostrSignerLocalKey") | ||
| data class LocalKey( | ||
| val nsec: String, | ||
| ) : NostrSignerCredential { | ||
| override val stableId: String | ||
| get() = "local:$nsec" | ||
| } | ||
|
|
||
| @Immutable | ||
| @Serializable | ||
| @SerialName("NostrSignerBunker") | ||
| data class Bunker( | ||
| val uri: String, | ||
| val userPubkeyHex: String? = null, | ||
| val signerRelay: String? = null, | ||
| val secret: String? = null, | ||
| ) : NostrSignerCredential { | ||
| override val stableId: String | ||
| get() = "bunker:$uri" | ||
| } |
There was a problem hiding this comment.
stableId for NostrSignerCredential.LocalKey includes the full nsec, and Bunker includes the full uri (which may embed secrets). This unnecessarily propagates sensitive material into in-memory IDs (and anything that might log/trace them). Use a non-secret identifier instead (e.g., derived pubkey hex, or a one-way hash of the secret/URI).
| PlatformTextField( | ||
| state = state.nostrInputState.accountInput, | ||
| label = { | ||
| PlatformText(text = stringResource(Res.string.nostr_login_nsec_hint)) | ||
| PlatformText(text = stringResource(Res.string.nostr_login_account_hint)) | ||
| }, | ||
| enabled = !state.nostrLoginState.loading, | ||
| modifier = Modifier.width(300.dp), | ||
| keyboardOptions = | ||
| KeyboardOptions( | ||
| keyboardType = KeyboardType.Password, | ||
| keyboardType = KeyboardType.Text, | ||
| imeAction = ImeAction.Done, | ||
| autoCorrectEnabled = false, | ||
| ), |
There was a problem hiding this comment.
The Nostr login input now accepts nsec (private key), but it's collected via PlatformTextField with KeyboardType.Text, which will display the secret in clear text. Consider using a secure/password-style field (or masking when input starts with nsec1 / looks like a private key) with an explicit “reveal” toggle to avoid exposing private keys on screen/recordings.
| return object : NostrLoginState { | ||
| override val loading: Boolean = loading | ||
| override val error: Throwable? = error | ||
| override val amberAvailable: Boolean = amberSignerBridge.isAvailable() |
There was a problem hiding this comment.
amberAvailable calls amberSignerBridge.isAvailable() during body() composition. On Android this queries PackageManager and may run on every recomposition. Cache this with remember { ... } / produceState (and update explicitly if needed) to avoid repeated package manager scans during UI updates.
| return object : NostrLoginState { | |
| override val loading: Boolean = loading | |
| override val error: Throwable? = error | |
| override val amberAvailable: Boolean = amberSignerBridge.isAvailable() | |
| val amberAvailable = remember { amberSignerBridge.isAvailable() } | |
| return object : NostrLoginState { | |
| override val loading: Boolean = loading | |
| override val error: Throwable? = error | |
| override val amberAvailable: Boolean = amberAvailable |
| public fun BindAmberSignerLauncher() { | ||
| val registry = koinInject<AmberIntentLauncherRegistry>() | ||
| val launcher = | ||
| rememberLauncherForActivityResult( | ||
| contract = ActivityResultContracts.StartActivityForResult(), | ||
| ) { result -> | ||
| pendingCallback?.invoke( | ||
| AmberIntentResult( | ||
| resultCode = result.resultCode, | ||
| data = result.data, | ||
| ), | ||
| ) | ||
| pendingCallback = null | ||
| } | ||
|
|
||
| DisposableEffect(registry, launcher) { | ||
| val closeable = | ||
| registry.attach { intent: Intent, callback: (AmberIntentResult) -> Unit -> | ||
| pendingCallback = callback | ||
| launcher.launch(intent) | ||
| } | ||
| onDispose { | ||
| pendingCallback = null | ||
| closeable.close() | ||
| } |
There was a problem hiding this comment.
pendingCallback is a file-level mutable var shared across compositions. If BindAmberSignerLauncher is disposed (or re-attached) while an AmberIntentLauncherRegistry.launch() call is in-flight, onDispose clears pendingCallback, so the ActivityResult handler will drop the result and the awaiting coroutine may never resume. Consider moving the callback/continuation management into AmberIntentLauncherRegistry (e.g., store a single pending continuation and fail/cancel it on detach), and avoid global state.
| internal suspend fun importAccount(input: String): ImportedAccount { | ||
| val value = input.removePrefix("nostr:").trim() | ||
| require(value.isNotEmpty()) { "A public key, private key, or bunker URI is required" } | ||
|
|
||
| parseSecret(value)?.use { secretKey -> | ||
| val pubkeyHex = | ||
| RustKeys(secretKey).use { keys -> | ||
| keys.publicKey().use { it.toHex() } | ||
| } | ||
| ImportedAccount( | ||
| return ImportedAccount.LocalKey( | ||
| pubkeyHex = pubkeyHex, | ||
| npub = bech32PublicKey(pubkeyHex), | ||
| nsec = secretKey.toBech32(), | ||
| ) | ||
| } ?: error("A public key or secret key is required") | ||
| } | ||
|
|
||
| parsePublicKeyHex(value)?.let { pubkeyHex -> | ||
| return ImportedAccount.ReadOnly( | ||
| pubkeyHex = pubkeyHex, | ||
| npub = bech32PublicKey(pubkeyHex), | ||
| ) | ||
| } | ||
|
|
||
| if (value.startsWith("bunker://", ignoreCase = true) || value.startsWith("nostrconnect://", ignoreCase = true)) { | ||
| return importBunkerAccount(value) | ||
| } | ||
|
|
||
| error("Unsupported Nostr account input") |
There was a problem hiding this comment.
New account-import paths were added (read-only npub/hex, and bunker:// / nostrconnect:// remote signer). There are tests for secret-only and read-only inputs, but no coverage for bunker/nostrconnect import (success + invalid URI/error cases). Adding tests here would help prevent regressions in remote-signer onboarding.
No description provided.