Conversation
There was a problem hiding this comment.
Pull request overview
Adds UX improvements across iOS/Compose login and account management, including QR-based Nostr Connect onboarding and safer account removal flows.
Changes:
- Implement QR-based Nostr Connect login session flow and expose QR state via
NostrLoginState. - Update Compose login UI to use a single Nostr credential field and render a QR code + cancel flow.
- Improve iOS/Android/Desktop account management UX (logout/delete confirmations, toolbar/tab picker behavior, sidebar restructuring).
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/presenter/login/NostrLoginPresenter.kt | Adds QR login session state + start/cancel APIs for Nostr login presenter. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/network/nostr/NostrService.kt | Introduces PendingQrLogin session, builds nostrconnect:// URI, and awaits remote signer approval. |
| iosApp/flare/UI/Screen/ProfileScreen.swift | Refactors profile tab selection UI; adds toolbar-based picker visibility logic on compact devices. |
| iosApp/flare/UI/Screen/LocalHistoryScreen.swift | Minor picker sizing tweak (fixedSize). |
| iosApp/flare/UI/Screen/AccountManagementScreen.swift | Adds logout confirmation alert before removing an account locally. |
| iosApp/flare/UI/FlareRoot.swift | Restructures iPad/sidebar tab layout using SecondaryTabsPresenter and static sidebar routes. |
| gradle/libs.versions.toml | Adds qrose dependency version + library entry. |
| desktopApp/src/main/kotlin/dev/dimension/flare/ui/screen/settings/SettingsScreen.kt | Adds a confirmation dialog before deleting accounts in desktop settings. |
| desktopApp/src/main/composeResources/values/strings.xml | Adds desktop confirmation string for account removal. |
| compose-ui/src/commonMain/kotlin/dev/dimension/flare/ui/screen/login/ServiceSelectionScreenContent.kt | Updates Nostr login UI: single credential input + QR code login UI (qrose). |
| compose-ui/src/commonMain/kotlin/dev/dimension/flare/ui/screen/login/NostrInputPresenter.kt | Simplifies Nostr input state to a single credential field and unified canLogin. |
| compose-ui/src/commonMain/composeResources/values/strings.xml | Updates Nostr login copy and adds QR/cancel strings. |
| compose-ui/src/commonMain/composeResources/values-ja-rJP/strings.xml | Japanese updates for new Nostr login copy + QR/cancel strings. |
| compose-ui/build.gradle.kts | Adds qrose dependency to commonMain. |
| app/src/main/res/values/strings.xml | Adds Android confirmation string for account removal. |
| app/src/main/java/dev/dimension/flare/ui/screen/settings/AccountsScreen.kt | Adds confirmation dialog before deleting an account (including swipe-to-dismiss path). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| override fun startQrLogin() { | ||
| scope.launch { | ||
| runCatching { | ||
| loading = true | ||
| error = null | ||
| pendingQrLogin?.close() | ||
| val session = NostrService.beginQrLogin() | ||
| pendingQrLogin = session | ||
| qrConnectUri = session.connectUri | ||
| qrWaiting = true | ||
| loading = false | ||
| runCatching { | ||
| withTimeout(2.minutes) { | ||
| session.awaitAccount() | ||
| } | ||
| }.onSuccess { imported -> | ||
| if (pendingQrLogin === session) { | ||
| loading = true | ||
| pendingQrLogin = null | ||
| qrConnectUri = null | ||
| qrWaiting = false | ||
| loginWith(imported) | ||
| session.close() | ||
| loading = false | ||
| } | ||
| }.onFailure { | ||
| if (pendingQrLogin === session) { | ||
| error = it | ||
| pendingQrLogin = null | ||
| qrConnectUri = null | ||
| qrWaiting = false | ||
| session.close() | ||
| } | ||
| } | ||
| }.onFailure { | ||
| it.printStackTrace() | ||
| } |
There was a problem hiding this comment.
In startQrLogin(), the outer runCatching { ... }.onFailure { printStackTrace() } can leave the UI in an inconsistent state (e.g., loading may remain true, qrWaiting/qrConnectUri may remain set, and no error is surfaced) when beginQrLogin() or other setup code throws. Consider handling this failure by setting error, resetting loading/qrWaiting/qrConnectUri/pendingQrLogin, and ensuring any partially-created session is closed (e.g., via try/finally).
| loginWith(imported) | ||
| session.close() | ||
| loading = false |
There was a problem hiding this comment.
Inside the QR login success path, loginWith(imported) is called without a surrounding runCatching/try/finally. If loginWith throws (e.g., account persistence fails), loading will stay true and the session may never be closed. Wrap loginWith in error handling and ensure loading is reset and session.close() runs in a finally block.
| loginWith(imported) | |
| session.close() | |
| loading = false | |
| try { | |
| loginWith(imported) | |
| } catch (t: Throwable) { | |
| error = t | |
| t.printStackTrace() | |
| } finally { | |
| session.close() | |
| loading = false | |
| } |
| val bunkerUri = | ||
| runCatching { | ||
| connect.bunkerUri().use { it.toString() } | ||
| }.getOrDefault(connectUri) | ||
| val signerRelay = | ||
| connect.relays().firstOrNull()?.use { it.toString() } | ||
| return ImportedAccount.RemoteSigner( | ||
| pubkeyHex = pubkeyHex, | ||
| npub = bech32PublicKey(pubkeyHex), | ||
| signerCredential = | ||
| NostrSignerCredential.Bunker( | ||
| uri = bunkerUri, | ||
| userPubkeyHex = pubkeyHex, | ||
| signerRelay = signerRelay, | ||
| secret = appSecret, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
connect.relays() appears to return closeable relay objects (given the .use { ... }). Currently only the first relay is closed; any remaining relay objects from connect.relays() will leak. Consider mapping the relays to strings while closing each element (or otherwise ensuring all returned relay handles are closed).
| ToolbarItem(placement: .primaryAction) { | ||
| profileTabPicker(tabs: tabs) | ||
| .pickerStyle(.menu) | ||
| .fixedSize() |
There was a problem hiding this comment.
profileTabPicker uses EmptyView() as the Picker label, but the same picker is rendered with .pickerStyle(.menu) in the compact toolbar. For menu-style pickers the label is what the user taps; an empty label can make the control effectively invisible/unusable and harms accessibility. Provide a visible label (e.g., current tab title or an icon) and/or an explicit accessibility label when using the menu style.
| .fixedSize() | |
| .fixedSize() | |
| .overlay( | |
| Image(systemName: "ellipsis.circle") | |
| ) | |
| .accessibilityLabel(Text("Profile tabs")) |
![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: 6195eccd94
ℹ️ 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".
| TabView(selection: $selectedTab) { | ||
| if horizontalSizeClass == .regular { | ||
| ForEach(tabs.primary, id: \.key) { data in | ||
| let badge = if data is NotificationTabItem || data is AllNotificationTabItem { | ||
| Int(notificationBadgePresenter.state.count) | ||
| } else { | ||
| 0 | ||
| } | ||
| Router { onNavigate in | ||
| data.view(onNavigate: onNavigate) | ||
| } | ||
| .tabItem { | ||
| Label { | ||
| TabTitle(title: data.metaData.title) | ||
| } icon: { | ||
| TabIcon(icon: data.metaData.icon, accountType: data.account, iconOnly: true) | ||
| } | ||
| } | ||
| .badge(badge) | ||
| .tag(data.key) | ||
| } | ||
| ForEach(tabs.secondary, id: \.key) { data in | ||
| Router { onNavigate in | ||
| data.view(onNavigate: onNavigate) | ||
| } | ||
| .tabItem { | ||
| Label { | ||
| TabTitle(title: data.metaData.title) | ||
| } icon: { | ||
| TabIcon(icon: data.metaData.icon, accountType: data.account, iconOnly: true) | ||
| } | ||
| } | ||
| .tag(data.key) | ||
| } | ||
| if let profileRoute = tabs.extraProfileRoute { | ||
| Router { onNavigate in | ||
| profileRoute.view(onNavigate: onNavigate) | ||
| } | ||
| .tabItem { | ||
| Label { | ||
| TabTitle(title: profileRoute.metaData.title) | ||
| } icon: { | ||
| TabIcon(icon: profileRoute.metaData.icon, accountType: profileRoute.account, iconOnly: true) | ||
| } | ||
| } | ||
| .tag(profileRoute.key) | ||
| ForEach(tabs.primary, id: \.key) { data in | ||
| let badge = if data is NotificationTabItem || data is AllNotificationTabItem { | ||
| Int(notificationBadgePresenter.state.count) | ||
| } else { |
There was a problem hiding this comment.
BackportFlareRoot now builds its TabView from tabs.primary only, while the previous regular-size branch that added tabs.secondary, extraProfileRoute, and the Settings tab was removed. Because FlareApp.swift routes all iOS versions below 18 to this view, iPad users on older iOS lose direct navigation to those sections entirely. Please reintroduce equivalent tabs/navigation for the backport path.
Useful? React with 👍 / 👎.
| }.onFailure { | ||
| it.printStackTrace() |
There was a problem hiding this comment.
If startQrLogin() fails before awaitAccount() handling starts (for example, beginQrLogin() throws), the outer onFailure only prints the stack trace and never resets loading or surfaces an error. In that failure mode the UI can remain stuck in a loading-disabled state without actionable feedback, so this path should set error and restore loading = false.
Useful? React with 👍 / 👎.
No description provided.