Feature/group enhance by Tlaster · Pull Request #1934 · DimensionDev/Flare

Tlaster · 2026-03-27T04:25:31Z

No description provided.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 52e375c306

ℹ️ 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".

chatgpt-codex-connector · 2026-03-27T04:30:25Z

iosApp/flare/UI/Screen/SearchScreen.swift

+                                }
+                            }
+                        } label: {
+                            if let selectedAccount = searchPresenter.state.selectedAccount {


Render fallback menu label when selected account is nil

The account switcher can disappear entirely here because the menu label is only rendered inside if let selectedAccount = .... In SearchPresenter, selectedAccount stays nil for guest searches (accountType is not .Specific), so opening Search in that context with multiple accounts leaves no visible control to pick an account, unlike the previous in-list selector.

Useful? React with 👍 / 👎.

Copilot

Pull request overview

This PR enhances group/tab configuration across shared logic and clients by introducing shared group config commit logic, sanitizing duplicate tab keys, and updating iOS/Compose UIs to use menu/disclosure-based selection/editing patterns.

Changes:

Added shared GroupConfigPresenter + TabSettings.upsertGroupConfig() with tab deduplication and safe upsert/delete behavior.
Introduced TabSettings.sanitizeDuplicateTabKeys() and applied it during account tab sync/cleanup to remove duplicate main/sub tab keys.
Refactored iOS/desktop/android tab/group settings screens (menus, disclosure groups, edit support) and added regression tests.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
shared/src/commonTest/kotlin/dev/dimension/flare/ui/presenter/home/GroupConfigPresenterTest.kt	Adds test ensuring group upsert deduplicates sub-tabs and preserves key.
shared/src/commonTest/kotlin/dev/dimension/flare/data/repository/AccountTabSyncCoordinatorSanitizerTest.kt	Adds test for duplicate-key sanitization across main/sub tabs.
shared/src/commonMain/kotlin/dev/dimension/flare/ui/presenter/home/GroupConfigPresenter.kt	Introduces shared presenter + `upsertGroupConfig()` with dedup + update/delete semantics.
shared/src/commonMain/kotlin/dev/dimension/flare/data/repository/AccountTabSyncCoordinator.kt	Applies sanitization in sync operations and defines sanitization helpers.
shared/src/commonMain/kotlin/dev/dimension/flare/data/network/nostr/NostrService.kt	Minor import reordering.
iosApp/flare/UI/Screen/TabSettingsScreen.swift	Refactors add-tab sheet into disclosure sections and reusable row UI.
iosApp/flare/UI/Screen/SearchScreen.swift	Moves account selection into toolbar menu.
iosApp/flare/UI/Screen/NotificationScreen.swift	Replaces account bar with menu-based selector and unread summary.
iosApp/flare/UI/Screen/LocalHistoryScreen.swift	Moves segmented picker into toolbar menu and simplifies list layout.
iosApp/flare/UI/Screen/HomeTimelineScreen.swift	Updates tab selection menu interaction.
iosApp/flare/UI/Screen/GroupConfigScreen.swift	Uses shared group config presenter + adds edit-tab sheet and dedup on init.
iosApp/flare/UI/Screen/DiscoverScreen.swift	Updates account selection menu interaction and UI.
desktopApp/src/main/kotlin/dev/dimension/flare/ui/screen/home/GroupConfigScreen.kt	Hooks desktop group config into shared presenter + adds edit-tab dialog.
desktopApp/src/main/kotlin/dev/dimension/flare/ui/screen/home/AddTabDialog.kt	Refactors add-tab dialog layout (side list + content).
app/src/main/java/dev/dimension/flare/ui/screen/home/GroupConfigScreen.kt	Hooks Android group config into shared presenter + adds edit-tab dialog.

Comments suppressed due to low confidence (1)

iosApp/flare/UI/Screen/DiscoverScreen.swift:54

Same issue as SearchScreen: Toggle in a Menu suggests a persistent boolean setting and is announced as a switch, but this UI is a single-selection account chooser. Use a Picker(selection:) or Button actions with a checkmark on the selected item to communicate 'radio' selection correctly (and avoid implying multiple accounts can be selected).

                                Toggle(isOn: Binding(get: {
                                    presenter.state.selectedAccount?.key == account.key
                                }, set: { value in
                                    if value {
                                        presenter.state.setAccount(profile: account)
                                        searchPresenter.state.setAccount(profile: account)
                                    }
                                })) {
                                    Label {
                                        Text(account.handle.canonical)
                                    } icon: {
                                        AvatarView(data: account.avatar)
                                    }
                                }
                            }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-27T05:14:29Z

iosApp/flare/UI/Screen/SearchScreen.swift

+        .toolbar {
+            if case .success(let data) = onEnum(of: searchPresenter.state.accounts) {
+                let accounts = data.data
+                if accounts.count > 1 {
+                    ToolbarItem(placement: .primaryAction) {
+                        Menu {
+                            ForEach(0..<accounts.count, id: \.self) { index in
+                                let account = accounts[index] as! UiProfile
+                                Toggle(isOn: Binding(get: {
+                                    searchPresenter.state.selectedAccount?.key == account.key
+                                }, set: { value in
+                                    if value {
+                                        searchPresenter.state.setAccount(profile: account)
+                                    }
+                                })) {
+                                    Label {
+                                        Text(account.handle.canonical)
+                                    } icon: {
+                                        AvatarView(data: account.avatar)
+                                    }
+                                }
+                            }
+                        } label: {
+                            if let selectedAccount = searchPresenter.state.selectedAccount {
+                                HStack {
+                                    AvatarView(data: selectedAccount.avatar)
+                                        .frame(width: 24, height: 24)
+                                    Text(selectedAccount.handle.canonical)
+                                    Image("fa-chevron-down")
+                                        .font(.footnote)
+                                        .foregroundStyle(.secondary)
+                                        .scaledToFit()
+                                        .frame(width: 8, height: 8)
+                                        .padding(8)
+                                        .background(
+                                            Circle()
+                                                .fill(Color.secondary.opacity(0.2))
+                                        )
+                                        .scaleEffect(0.66)
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        }


Using Toggle inside a Menu for single-account selection is misleading for assistive tech (it’s announced/behaves like an on/off setting, and menus with toggles imply multi-select). Prefer a Picker(selection:) (menu-style) or Button rows that show a checkmark for the currently selected account. Also, the Menu label renders nothing when selectedAccount is nil (lines 90-107), which can make the toolbar item effectively invisible/un-tappable—provide a fallback label (e.g., 'Select account' + icon) when there’s no selection yet.

Copilot · 2026-03-27T05:14:29Z

iosApp/flare/UI/Screen/HomeTimelineScreen.swift

+                                Toggle(isOn: Binding(get: {
+                                    selectedTabIndex == index
+                                }, set: { value in
+                                    if value {
+                                        selectedTabIndex = index
+                                    }
+                                })) {
                                    Label {
                                        TabTitle(title: item.metaData.title)
                                    } icon: {


Toggle is not a good fit for selecting one tab from a list (it implies an independent on/off state per row). Prefer a Button per menu item with a checkmark for the current selection, or a Picker(selection:) bound to selectedTabIndex so the control semantics match a single-choice selection.

Copilot · 2026-03-27T05:14:29Z

iosApp/flare/UI/Screen/NotificationScreen.swift

+        Menu {
+            ForEach(items, id: \.stableKey) { item in
+                Toggle(isOn: Binding(get: {
+                    selectedAccount?.key == item.profile.key
+                }, set: { value in
+                    if value {
+                        onSelect(item.profile)
+                    }
+                })) {
+                    Label {
+                        Text(item.profile.handle.canonical)
+                    } icon: {
+                        AvatarView(data: item.profile.avatar)
+                            .overlay(alignment: .bottomTrailing) {
+                            }
+                    }
+                    if item.badge > 0 {
+                        Text("\(item.badge)")
+                    }
+                }
+            }
+        } label: {
+            if let selectedAccount {
+                HStack(spacing: 8) {
+                    AvatarView(data: selectedAccount.avatar)
+                        .frame(width: 24, height: 24)
+                    VStack(alignment: .leading, spacing: 2) {
+                        Text(selectedAccount.handle.canonical)
+                            .lineLimit(1)
+                        if let unreadSummary {
+                            Text("New from \(unreadSummary)")
+                                .font(.caption2)
+                                .foregroundStyle(.secondary)
+                                .lineLimit(1)
+                        }
+                    }


Two concrete issues here: (1) Toggle inside Menu for selecting a single account has incorrect semantics (toggles imply an independent on/off setting and may be announced as a switch). Use a single-selection control pattern (Picker(selection:) or Button + checkmark). (2) Text(\"New from \\(unreadSummary)\") is a hardcoded English string and won’t be localized; use a localized key/format instead. Also, the overlay(alignment:) { } block is empty (lines 183-184) and should be removed to avoid dead UI code.

Copilot · 2026-03-27T05:14:30Z

iosApp/flare/UI/Screen/TabSettingsScreen.swift

+    private func binding(for key: String) -> Binding<Bool> {
+        Binding(
+            get: { expandedSections.contains(key) },
+            set: { isExpanded in
+                if isExpanded {
+                    expandedSections.insert(key)
+                } else {
+                    expandedSections.remove(key)
+                }
+            }
+        )
+    }


This binding(for:) helper is duplicated (same implementation exists again in AccountTabListView). Consider extracting it into a shared helper (e.g., a small extension on Binding<Set<String>> or a private utility) to avoid drift and keep expansion behavior consistent across sections.

Copilot · 2026-03-27T05:14:30Z

shared/src/commonMain/kotlin/dev/dimension/flare/data/repository/AccountTabSyncCoordinator.kt

+internal fun TabSettings.sanitizeDuplicateTabKeys(): TabSettings {
+    val sanitizedTabs =
+        mainTabs
+            .mapNotNull { it.sanitizeDuplicateTabKeys() }
+            .distinctBy { it.key }
+    return if (sanitizedTabs == mainTabs) {
+        this
+    } else {
+        copy(mainTabs = sanitizedTabs)
+    }
+}


TabSettings.sanitizeDuplicateTabKeys() is a broadly useful TabSettings utility, but it’s currently defined in AccountTabSyncCoordinator.kt (a coordinator-specific file). Since it’s now imported/used by GroupConfigPresenter as well, consider moving this extension (and its helper) into a more appropriate shared location (e.g., a TabSettings-focused file in the repository/model layer). This reduces coupling and makes the API easier to discover and reuse.

Copilot · 2026-03-27T05:14:30Z

iosApp/flare/UI/Screen/GroupConfigScreen.swift

+        _tabs = State(initialValue: Array((item?.subTimelineTabItem ?? []).reduce(into: [TimelineTabItem]()) { result, tab in
+            if !result.contains(where: { $0.key == tab.key }) {
+                result.append(tab)
+            }
+        }))


The init-time deduplication uses an O(n²) contains(where:) loop inside reduce, and the outer Array(...) wrapper is redundant since the accumulator is already [TimelineTabItem]. Consider simplifying this by tracking seen keys (e.g., a Set) while preserving order. This will be clearer and scale better if groups contain many tabs.

Suggested change

_tabs = State(initialValue: Array((item?.subTimelineTabItem ?? []).reduce(into: [TimelineTabItem]()) { result, tab in

if !result.contains(where: { $0.key == tab.key }) {

result.append(tab)

}

}))

_tabs = State(initialValue: {

let initialTabs = item?.subTimelineTabItem ?? []

var seenKeys = Set<AnyHashable>()

var uniqueTabs: [TimelineTabItem] = []

for tab in initialTabs {

let key = AnyHashable(tab.key)

if !seenKeys.contains(key) {

seenKeys.insert(key)

uniqueTabs.append(tab)

}

}

return uniqueTabs

}())

Tlaster added 5 commits March 27, 2026 11:47

fix group dup

b7b43c4

add group tab edit

80da575

update ios add tab design

8763589

update desktop tab add ui

edd52e0

update ios notification ux

52e375c

Tlaster requested a review from Copilot March 27, 2026 04:25

chatgpt-codex-connector bot reviewed Mar 27, 2026

View reviewed changes

update ios local history ux

b16c054

Copilot started reviewing on behalf of Tlaster March 27, 2026 05:09 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

Tlaster merged commit e310956 into master Mar 27, 2026
16 of 17 checks passed

Tlaster deleted the feature/group_enhance branch March 27, 2026 05:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/group enhance#1934

Feature/group enhance#1934
Tlaster merged 6 commits intomasterfrom
feature/group_enhance

Tlaster commented Mar 27, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Mar 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-        _tabs = State(initialValue: Array((item?.subTimelineTabItem ?? []).reduce(into: [TimelineTabItem]()) { result, tab in
-            if !result.contains(where: { $0.key == tab.key }) {
-                result.append(tab)
-            }
-        }))
+        _tabs = State(initialValue: {
+            let initialTabs = item?.subTimelineTabItem ?? []
+            var seenKeys = Set<AnyHashable>()
+            var uniqueTabs: [TimelineTabItem] = []
+            for tab in initialTabs {
+                let key = AnyHashable(tab.key)
+                if !seenKeys.contains(key) {
+                    seenKeys.insert(key)
+                    uniqueTabs.append(tab)
+                }
+            }
+            return uniqueTabs
+        }())

Conversation

Tlaster commented Mar 27, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants