feat: multi-account support — domain model + UI components (#86)#160
feat: multi-account support — domain model + UI components (#86)#160hanrw merged 5 commits intotddworks:mainfrom
Conversation
Introduce domain-layer abstractions for supporting multiple accounts per AI provider (addresses tddworks#86, tddworks#92, tddworks#105): - ProviderAccount: value type representing a named account within a provider, with compound ID format (providerId.accountId) that is backward compatible with single-account providers (default account ID equals provider ID) - MultiAccountProvider: opt-in protocol extending AIProvider for providers that support multiple accounts. Includes account switching, per-account refresh, aggregate status, and best-available-account queries. Existing single-account providers are unchanged. - MultiAccountSettingsRepository: settings protocol for persisting account configurations per provider, with ProviderAccountConfig (Codable) for JSON serialization - ProviderAccountTests + ProviderAccountConfigTests: Chicago School TDD tests verifying state and identity behavior Phase 1 establishes the domain model foundation. Phase 2 will add UI (account tabs/switcher in ProviderSectionView). Phase 3 will add per-provider probe configs for multi-account (Claude profiles, Codex workspaces, etc.).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds multi-account support: new Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User (UI)"
participant Picker as "AccountPickerView"
participant Provider as "MultiAccountProvider"
participant Repo as "MultiAccountSettingsRepository"
participant Monitor as "QuotaMonitor"
User->>Picker: tap account pill
Picker->>Provider: switchAccount(to: accountId)
Provider->>Repo: setActiveAccountId(accountId, forProvider:)
Provider->>Monitor: refreshAccount(accountId) async
Monitor-->>Provider: UsageSnapshot
Provider-->>Picker: update activeAccount & accountSnapshots
Picker-->>User: render active state and statuses
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (2)
Sources/Domain/Provider/ProviderAccount.swift (1)
69-84: Consider edge case: emptydisplayNameproduces emptyinitialLetter.If
labelis empty,nil, andaccountIdis an empty string, thendisplayNamereturns""andinitialLetteralso returns"". While unlikely in practice (accountId should be non-empty), the avatar circle would have no letter.A defensive fallback could return a placeholder like
"?"for empty display names.🛡️ Optional defensive fallback
public var initialLetter: String { - String(displayName.prefix(1)).uppercased() + let letter = String(displayName.prefix(1)).uppercased() + return letter.isEmpty ? "?" : letter }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Domain/Provider/ProviderAccount.swift` around lines 69 - 84, The initialLetter computed property can return an empty string if displayName is empty; update ProviderAccount.initialLetter to defensively handle that by checking displayName (or its prefix) and returning a single-character fallback (e.g. "?") when empty, otherwise returning String(displayName.prefix(1)).uppercased(); change only the logic inside the initialLetter getter to perform this empty-check and uppercase transformation so avatar circles always show a character.Tests/DomainTests/Provider/ProviderAccountTests.swift (1)
29-38: Misleading test name: the assertion proves inequality, not equality.The test name states "Two accounts with same accountId and providerId are equal" but Line 35 asserts
#expect(a != b). The test actually demonstrates thatProviderAccountcompares all fields (includinglabel), so accounts with different labels are not equal despite matching IDs.Consider renaming to clarify the behavior being tested.
✏️ Suggested rename
- `@Test`("Two accounts with same accountId and providerId are equal") + `@Test`("Accounts with same IDs but different labels are not equal") func equalityByIdAndProvider() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/DomainTests/Provider/ProviderAccountTests.swift` around lines 29 - 38, Rename or revise the test so its name matches the asserted behavior: either rename the test function equalityByIdAndProvider to something like test_accountsWithSameIdAndProvider_butDifferentLabels_areNotEqual, or change the assertions to match an equality expectation; specifically update the test function equalityByIdAndProvider that constructs ProviderAccount instances a and b (same accountId/providerId, different label) so the name reflects that ProviderAccount equality compares all fields (label differs) and therefore assert a != b while verifying a.id == b.id remains true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Domain/Provider/ProviderAccount.swift`:
- Around line 69-84: The initialLetter computed property can return an empty
string if displayName is empty; update ProviderAccount.initialLetter to
defensively handle that by checking displayName (or its prefix) and returning a
single-character fallback (e.g. "?") when empty, otherwise returning
String(displayName.prefix(1)).uppercased(); change only the logic inside the
initialLetter getter to perform this empty-check and uppercase transformation so
avatar circles always show a character.
In `@Tests/DomainTests/Provider/ProviderAccountTests.swift`:
- Around line 29-38: Rename or revise the test so its name matches the asserted
behavior: either rename the test function equalityByIdAndProvider to something
like test_accountsWithSameIdAndProvider_butDifferentLabels_areNotEqual, or
change the assertions to match an equality expectation; specifically update the
test function equalityByIdAndProvider that constructs ProviderAccount instances
a and b (same accountId/providerId, different label) so the name reflects that
ProviderAccount equality compares all fields (label differs) and therefore
assert a != b while verifying a.id == b.id remains true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc6a98ea-5bde-4294-96dd-a7b713ffaf0b
📒 Files selected for processing (5)
Sources/Domain/Provider/MultiAccountSettingsRepository.swiftSources/Domain/Provider/MultiAccountSupport.swiftSources/Domain/Provider/ProviderAccount.swiftTests/DomainTests/Provider/ProviderAccountConfigTests.swiftTests/DomainTests/Provider/ProviderAccountTests.swift
Add SwiftUI views for multi-account provider support: - AccountPickerView: compact horizontal pill-based account switcher shown in the provider section header when a provider conforms to MultiAccountProvider and has >1 account. Uses the same visual language as the existing ProviderPill component. - AccountManagementCard: settings card for managing accounts on a multi-account provider. Shows account list with status indicators, active account checkmark, switch buttons, and an add-account placeholder. Uses DisclosureGroup consistent with existing settings cards (ProvidersCard, BackgroundSyncCard, etc.). Both views are additive — they only render when a provider conforms to MultiAccountProvider (from Phase 1). Single-account providers are completely unaffected. Integration points (not wired yet — requires a concrete MultiAccountProvider implementation): - AccountPickerView goes in MenuContentView.accountCard() header - AccountManagementCard goes in SettingsContentView after the provider-specific config cards
marcusquinn
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Sources/App/Views/Settings/AccountManagementCard.swift (2)
11-11: Unusedmonitorproperty.
monitor: QuotaMonitoris declared but never referenced in the view. If it's planned for future use (e.g., triggering refreshes), consider adding a// TODOcomment. Otherwise, remove it to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Views/Settings/AccountManagementCard.swift` at line 11, The AccountManagementCard view declares an unused property "monitor: QuotaMonitor" — either remove the unused property or mark intent with a TODO; update the AccountManagementCard struct by deleting the "monitor: QuotaMonitor" declaration if it's not needed, or replace it with a comment like "// TODO: add usage for monitor to trigger quota refreshes" and keep the property only if you will reference it elsewhere (e.g., in init or body). Ensure any initializers or callers that passed a QuotaMonitor are updated accordingly if you remove the property.
138-139: Consider providing feedback when account switch fails.
switchAccount(to:)returns aBoolindicating success, but the result is discarded. If the switch fails (e.g., invalid account ID), the user receives no feedback. This is low risk since accounts come from the provider's own list, but a defensive check could improve UX.💡 Optional: Add feedback on failure
Button { - provider.switchAccount(to: account.accountId) + let success = provider.switchAccount(to: account.accountId) + if !success { + // Log or show brief feedback + AppLog.warning("Failed to switch to account: \(account.accountId)", category: .ui) + } } label: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Views/Settings/AccountManagementCard.swift` around lines 138 - 139, The Button action currently calls provider.switchAccount(to: account.accountId) and ignores its Bool result; change it to capture the return value and present user feedback on failure: call let success = provider.switchAccount(to: account.accountId) and if success is false set a local `@State` flag (e.g., showSwitchError) or error message state and present an Alert or inline error in AccountManagementCard to inform the user the account switch failed; ensure you reference provider.switchAccount(to:) and the Button action and add the `@State` properties and Alert presentation in the view.Sources/App/Views/AccountPickerView.swift (2)
34-34: Consider markingAccountPillasprivate.Since
AccountPillis only used within this file as an implementation detail ofAccountPickerView, limiting its visibility would better encapsulate the module's internal structure.🔒 Proposed scope change
-struct AccountPill: View { +private struct AccountPill: View {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Views/AccountPickerView.swift` at line 34, The AccountPill view is currently internal but is only used inside this file; change its declaration to private to encapsulate it. Locate the struct AccountPill: View and mark it private (private struct AccountPill: View) so it remains file-scoped and cannot be accessed externally; ensure any references from AccountPickerView or other symbols in this file still compile after the visibility change.
43-73: Consider adding accessibility labels for screen reader support.The button lacks an accessibility label, which means VoiceOver users won't have meaningful context about which account they're selecting or its active state.
♿ Proposed accessibility improvement
} .buttonStyle(.plain) .onHover { isHovering = $0 } + .accessibilityLabel("\(account.displayName) account\(isActive ? ", active" : "")") + .accessibilityHint(isActive ? "Currently selected" : "Double-tap to switch") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Views/AccountPickerView.swift` around lines 43 - 73, Add VoiceOver support to the account selection Button by giving it a descriptive accessibility label and traits: on the Button(action: action) that wraps the avatar and display name, set an accessibilityLabel that includes account.displayName and the active state (use isActive to append "selected" or similar), add an accessibilityHint like "Double tap to select this account", and apply accessibilityAddTraits(.isSelected) when isActive; optionally set an accessibilityIdentifier using account.displayName or a stable account id for UI tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/App/Views/Settings/AccountManagementCard.swift`:
- Line 15: The Add Account sheet state (showAddSheet) is toggled but never used
to present a sheet; attach a .sheet(isPresented: $showAddSheet) modifier to the
parent view (where the button toggles showAddSheet) to present the add-account
view (or a placeholder) when true, referencing showAddSheet and the add-account
view struct (or a temporary View) so tapping the button actually displays the
sheet; if the sheet view is not ready, disable the Add Account button or present
a placeholder view in the sheet to avoid a no-op tap.
---
Nitpick comments:
In `@Sources/App/Views/AccountPickerView.swift`:
- Line 34: The AccountPill view is currently internal but is only used inside
this file; change its declaration to private to encapsulate it. Locate the
struct AccountPill: View and mark it private (private struct AccountPill: View)
so it remains file-scoped and cannot be accessed externally; ensure any
references from AccountPickerView or other symbols in this file still compile
after the visibility change.
- Around line 43-73: Add VoiceOver support to the account selection Button by
giving it a descriptive accessibility label and traits: on the Button(action:
action) that wraps the avatar and display name, set an accessibilityLabel that
includes account.displayName and the active state (use isActive to append
"selected" or similar), add an accessibilityHint like "Double tap to select this
account", and apply accessibilityAddTraits(.isSelected) when isActive;
optionally set an accessibilityIdentifier using account.displayName or a stable
account id for UI tests.
In `@Sources/App/Views/Settings/AccountManagementCard.swift`:
- Line 11: The AccountManagementCard view declares an unused property "monitor:
QuotaMonitor" — either remove the unused property or mark intent with a TODO;
update the AccountManagementCard struct by deleting the "monitor: QuotaMonitor"
declaration if it's not needed, or replace it with a comment like "// TODO: add
usage for monitor to trigger quota refreshes" and keep the property only if you
will reference it elsewhere (e.g., in init or body). Ensure any initializers or
callers that passed a QuotaMonitor are updated accordingly if you remove the
property.
- Around line 138-139: The Button action currently calls
provider.switchAccount(to: account.accountId) and ignores its Bool result;
change it to capture the return value and present user feedback on failure: call
let success = provider.switchAccount(to: account.accountId) and if success is
false set a local `@State` flag (e.g., showSwitchError) or error message state and
present an Alert or inline error in AccountManagementCard to inform the user the
account switch failed; ensure you reference provider.switchAccount(to:) and the
Button action and add the `@State` properties and Alert presentation in the view.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c39880ea-a2cf-43ba-ac2e-023d519d345a
📒 Files selected for processing (2)
Sources/App/Views/AccountPickerView.swiftSources/App/Views/Settings/AccountManagementCard.swift
|
|
||
| @Environment(\.appTheme) private var theme | ||
| @State private var isExpanded = false | ||
| @State private var showAddSheet = false |
There was a problem hiding this comment.
"Add Account" button sets state but the sheet is never presented.
showAddSheet is toggled to true on Line 161, but there's no .sheet(isPresented: $showAddSheet) modifier attached to the view. Tapping the button will silently do nothing.
🐛 Proposed fix to wire up the sheet
Add the sheet modifier to the view. If the add-account sheet view isn't implemented yet, consider disabling the button or adding a placeholder:
.buttonStyle(.plain)
}
}
+
+// Add to body, after .background(...):
+.sheet(isPresented: $showAddSheet) {
+ // TODO: Replace with actual AddAccountSheet view
+ Text("Add Account Sheet - Coming Soon")
+}Or if deferred to a later phase, disable the button for now:
.buttonStyle(.plain)
+ .disabled(true) // TODO: Enable when AddAccountSheet is implemented
}
}Also applies to: 159-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/App/Views/Settings/AccountManagementCard.swift` at line 15, The Add
Account sheet state (showAddSheet) is toggled but never used to present a sheet;
attach a .sheet(isPresented: $showAddSheet) modifier to the parent view (where
the button toggles showAddSheet) to present the add-account view (or a
placeholder) when true, referencing showAddSheet and the add-account view struct
(or a temporary View) so tapping the button actually displays the sheet; if the
sheet view is not ready, disable the Add Account button or present a placeholder
view in the sheet to avoid a no-op tap.
|
Hey @hanrw — happy to discuss the Phase 3 integration if useful. A few notes on how we'd envision the wiring:
Settings persistence:
UI wiring (2 lines essentially):
We use a similar multi-account pool pattern in our own tooling (rotating across multiple Claude/Codex OAuth accounts for headless dispatch), so the domain model is battle-tested for the account identity, status aggregation, and best-available-account selection patterns. Let us know if you'd like us to take a crack at the Phase 3 wiring in a follow-up PR, or if you'd prefer to handle it yourselves given the probe internals. |
|
|
Great progress on Phase 3! The UI looks clean — the account switcher placement in the sidebar makes sense for discoverability. A few thoughts on the prototype:
Looking forward to seeing this merged. The domain model foundation from Phase 1 should make Phase 3 straightforward to wire up. |
i've just put all the html resources under docs/features/multi-account if it's help |
|
@marcusquinn here's some build issues which can't be merged right now. |
|
some update from your comments. 
|
Phase 1-3 Review & Contribution OfferWent through the full codebase to understand the integration surface. The domain model is clean — happy to help accelerate Phase 3 with concrete PRs if useful. Phase 1 (This PR) — Build FixThe CI failures are Likely fix depends on your Mockable version — either re-declare parent requirements in the child protocols so the macro picks them up, or add manual mock conformance extensions. Your call on which pattern fits the project. The CodeRabbit Phase 3 — Concrete PRs We Can ContributeWe've mapped the integration points against the existing code. Here's what we could take on as separate follow-up PRs (each self-contained and independently mergeable): PR A:
PR B:
PR C: UI wiring in
PR D (optional):
Happy to start with PR A (persistence) since it's the most self-contained and unblocks the rest. Let us know which ones you'd like us to take on vs. handle yourselves. |
|
Thanks for the update on the build issues, @hanrw. I'll take a look at the failing checks and get them resolved. Regarding the layer diagram — that's a helpful visualisation of the architecture. I'll review the structure and make sure the multi-account components align with the existing patterns. Will push fixes for the build issues and update this PR. Let me know if there are specific areas you'd like me to prioritise. |
|
@marcusquinn sounds great and all looks good |
|
the build issue may related to: Kolos65/Mockable#128 |
The @mockable macro cannot generate stubs for inherited protocol requirements — this is a known limitation (Kolos65/Mockable#128). MockMultiAccountProvider was missing AIProvider+Identifiable stubs, and MockMultiAccountSettingsRepository was missing ProviderSettingsRepository stubs, causing 22 build errors in CI. Remove @mockable from both child protocols. When tests need mocks, use the aggregate-protocol pattern recommended by the Mockable maintainer.
|
Good find — that's exactly it. Mockable's Just pushed a fix: removed CI should go green now — the only errors were the 22 missing-conformance failures from those two generated mocks. |
|
CI still failed |
|
Fixed — the test target was missing Just pushed the one-line fix (d6a3cb3). CI should go green now. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #160 +/- ##
==========================================
+ Coverage 80.06% 80.37% +0.31%
==========================================
Files 102 106 +4
Lines 7741 7879 +138
==========================================
+ Hits 6198 6333 +135
- Misses 1543 1546 +3
🚀 New features to boost your workflow:
|
|
@marcusquinn Thanks for your great work! I've created some new tickets for Phase 3. |
2 participants
Summary
Introduces multi-account support for AI providers, addressing #86 (and related #92, #105). This PR adds the domain model foundation (Phase 1) and UI components (Phase 2) as additive, non-breaking changes.
Phase 1: Domain Model
New Domain Types
ProviderAccount— Value type representing a named account within a provider. Uses compound ID format (providerId.accountId) that is backward compatible: the default account's ID equals the bare provider ID.MultiAccountProvider— Opt-in protocol extendingAIProviderfor providers that support multiple accounts. Includes:switchAccount(to:))refreshAccount(_:))MultiAccountSettingsRepository— Settings protocol for persisting account configurations per provider, withProviderAccountConfig(Codable) for JSON serialization.AIProviderRepositoryextension — Convenience methods for querying multi-account providers and total account counts.Tests (Chicago School TDD)
ProviderAccountTests— Identity (compound IDs, backward compat), display name fallback chain, initial letterProviderAccountConfigTests— Domain model conversion, Codable round-trip, equalityPhase 2: UI Components
New Views
AccountPickerView— Compact horizontal pill-based account switcher for the provider section header. Uses the same visual language as the existingProviderPillcomponent. Only renders when a provider conforms toMultiAccountProviderand has >1 account.AccountManagementCard— Settings card for managing accounts on a multi-account provider. Shows account list with status indicators, active account checkmark, switch buttons, and an add-account placeholder. UsesDisclosureGroupconsistent with existing settings cards.Integration Points (not wired yet)
These views are ready to integrate once a concrete provider implements
MultiAccountProvider:AccountPickerView→MenuContentView.accountCard()headerAccountManagementCard→SettingsContentViewafter provider-specific config cardsDesign Decisions
AIProvider— zero migration cost for existing providers.claude.personal) — existingQuotaMonitor.provider(for:)lookup works unchanged.[String: String]) — different providers need different config (CLI profiles, API tokens, config paths).Next Steps (Phase 3)
ClaudeProviderimplementMultiAccountProvideras the first concrete exampleAccountPickerViewintoMenuContentViewAccountManagementCardintoSettingsContentViewMultiAccountSettingsRepositoryinJSONSettingsRepositoryVerification
All files pass
swiftc -parsesyntax validation. Tests use Swift Testing (@Suite,@Test,#expect), consistent with the existing test suite.Closes #86
Summary by CodeRabbit
New Features
Tests
Chores