Conversation
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
Pull request overview
This pull request implements an account switcher feature that allows users to switch between their primary Google account and associated brand accounts (YouTube channels) within the Kaset app. The implementation adds UI components for account selection, integrates with the YouTube Music API's account management endpoints, and ensures content refreshes when switching accounts.
Changes:
- Added account management infrastructure (
AccountService,UserAccountmodel,AccountsListParser) - Implemented UI components for account switching (
SidebarProfileView,AccountSwitcherPopover,AccountRowView,ToastView) - Enhanced API client to support brand account context in requests via
onBehalfOfUserparameter - Added comprehensive unit tests and UI tests for the new functionality
- Updated API explorer tool to support brand account discovery and testing
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Core/Services/Auth/AccountService.swift | Service managing account state, switching, and persistence |
| Core/Models/UserAccount.swift | Model representing user accounts (primary/brand) |
| Core/Models/AccountsListResponse.swift | Response model for accounts list API |
| Core/Services/API/Parsers/AccountsListParser.swift | Parser for accounts list API responses |
| Views/macOS/SidebarProfileView.swift | Profile section at bottom of sidebar with account switcher trigger |
| Views/macOS/AccountSwitcherPopover.swift | Popover UI for switching between accounts |
| Views/macOS/AccountRowView.swift | Individual account row component |
| Views/macOS/ToastView.swift | Toast notification for errors |
| Core/Services/API/YTMusicClient.swift | Enhanced to support brand account requests |
| Views/macOS/MainWindow.swift | Integrated account service and refresh logic |
| Tests/KasetTests/AccountServiceTests.swift | Unit tests for AccountService |
| Tests/KasetTests/UserAccountTests.swift | Unit tests for UserAccount model |
| Tests/KasetTests/AccountsListParserTests.swift | Unit tests for parser |
| Tests/KasetUITests/AccountSwitcherUITests.swift | UI tests for account switcher |
| Tools/api-explorer.swift | Enhanced with brand account support |
| docs/api-discovery.md | Documentation for brand account API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Views/macOS/AccountRowView.swift
Outdated
| @available(macOS 26.0, *) | ||
| #Preview("Account Without Handle") { | ||
| let account = UserAccount( | ||
| id: "nohanlde", |
There was a problem hiding this comment.
Corrected spelling of 'nohanlde' to 'nohandle'.
| id: "nohanlde", | |
| id: "nohandle", |
| .onChange(of: self.accountService.currentAccount?.id) { _, newAccountId in | ||
| // Refresh all content when user switches accounts | ||
| guard newAccountId != nil else { return } | ||
| DiagnosticsLogger.auth.info("Account switched, refreshing content...") | ||
| // Clear API cache to ensure fresh data for new account | ||
| Task { @MainActor in | ||
| APICache.shared.invalidateAll() | ||
| URLCache.shared.removeAllCachedResponses() | ||
| await self.refreshAllContent() | ||
| } | ||
| } |
There was a problem hiding this comment.
The onChange handler triggers on initial account load after login, not just switches. This causes unnecessary cache invalidation and content refresh when the app starts. Consider tracking previous account ID to only refresh on actual switches, or check if this is the first account load.
| private(set) var accounts: [UserAccount] = [] | ||
|
|
||
| /// Currently selected/active account. | ||
| private(set) var currentAccount: UserAccount? |
There was a problem hiding this comment.
The AccountService test helpers expose internal state via setAccountsForTesting and setCurrentAccountForTesting methods, but tests don't cover the actual fetchAccounts() method that populates this state from the API. Consider adding tests that verify the full flow including API response parsing and account selection logic.
| /// Provider for the current brand account ID. | ||
| /// Set this after initialization to enable brand account API requests. | ||
| /// Returns nil for primary account, brand ID string for brand accounts. | ||
| var brandIdProvider: (() -> String?)? |
There was a problem hiding this comment.
The brandIdProvider closure property is not marked as @Sendable despite being used in an async context within the buildContext() method. While the current implementation using a weak reference in KasetApp.swift is safe, consider marking this as @Sendable to enforce thread-safety requirements and prevent future issues if the implementation changes.
| var brandIdProvider: (() -> String?)? | |
| var brandIdProvider: (@Sendable () -> String?)? |
| var hashData = jsonData | ||
| if !brandId.isEmpty { |
There was a problem hiding this comment.
Appending brandId as raw UTF-8 bytes to the hash data could cause collisions if endpoint/body combinations produce JSON that ends with bytes matching a brandId prefix. Consider using a separator or structured format (e.g., JSON with explicit brandId field) to ensure unambiguous cache key generation.
| var hashData = jsonData | |
| if !brandId.isEmpty { | |
| var hashData = Data() | |
| hashData.append(jsonData) | |
| if !brandId.isEmpty { | |
| // Use a NUL byte as a separator to avoid ambiguity between JSON and brandId bytes | |
| hashData.append(0) |
Description
fixes #20
AI Prompt (Optional)
🤖 AI Prompt Used
AI Tool:
Type of Change
Related Issues
Changes Made
Testing
xcodebuild test -only-testing:KasetTests)Checklist
swiftlint --strict && swiftformat .Screenshots
Additional Notes