feat: AI Diagnostic Assistant settings + Settings sidebar refactor - #50
Conversation
|
github-actions
Bot
added
size: XL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Settings UI into a native macOS-style sidebar/detail layout with dedicated App-level pages (General, Kubernetes, Notifications, and AI Assistant) and Context watchlist pages. It also introduces the AI Diagnostic Assistant feature, enabling users to configure an AI provider, store API keys securely in the macOS Keychain, and manually run connection tests. The reviewer's feedback focuses on improving robustness and idiomatic Swift usage, including: saving the API key to the Keychain before writing configuration to disk to prevent sync issues; tracking and cancelling active connection test tasks to avoid race conditions; setting appropriate Keychain accessibility attributes for login startup; implementing global keyboard shortcuts via hidden background buttons rather than list rows; and simplifying struct mutations by making configuration properties mutable.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try configStore.save(completedConfig) | ||
|
|
||
| if runtimeState.setupState.aiDiagnosticAssistant.hasAPIKeyDraft { | ||
| do { | ||
| try aiCredentialStore.saveAPIKey( | ||
| runtimeState.setupState.aiDiagnosticAssistant.apiKeyDraft, | ||
| for: completedConfig.aiDiagnosticAssistant.provider | ||
| ) | ||
| } catch { | ||
| runtimeState.markConfigurationSaveFailed(Self.aiCredentialSaveFailureMessage) | ||
| publishRuntimeState() | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Save the API key to the Keychain before saving the non-secret configuration to disk, and ensure the API key draft is trimmed of any accidental whitespaces or newlines. If the Keychain save fails, the transaction should abort, preventing the configuration on disk from becoming out of sync (e.g., updated to a new provider but missing the corresponding API key).
| try configStore.save(completedConfig) | |
| if runtimeState.setupState.aiDiagnosticAssistant.hasAPIKeyDraft { | |
| do { | |
| try aiCredentialStore.saveAPIKey( | |
| runtimeState.setupState.aiDiagnosticAssistant.apiKeyDraft, | |
| for: completedConfig.aiDiagnosticAssistant.provider | |
| ) | |
| } catch { | |
| runtimeState.markConfigurationSaveFailed(Self.aiCredentialSaveFailureMessage) | |
| publishRuntimeState() | |
| return false | |
| } | |
| } | |
| if runtimeState.setupState.aiDiagnosticAssistant.hasAPIKeyDraft { | |
| do { | |
| try aiCredentialStore.saveAPIKey( | |
| runtimeState.setupState.aiDiagnosticAssistant.apiKeyDraft.trimmingCharacters(in: .whitespacesAndNewlines), | |
| for: completedConfig.aiDiagnosticAssistant.provider | |
| ) | |
| } catch { | |
| runtimeState.markConfigurationSaveFailed(Self.aiCredentialSaveFailureMessage) | |
| publishRuntimeState() | |
| return false | |
| } | |
| } | |
| try configStore.save(completedConfig) |
| private let healthShiftAlertNotifier: any HealthShiftAlertNotifying | ||
| private let networkReachability: any NetworkReachability | ||
| private let podLogStreamer: any PodLogStreaming | ||
| private let aiCredentialStore: any AIProviderCredentialStoring | ||
| private let aiConnectionTester: AIProviderConnectionTester? |
There was a problem hiding this comment.
To prevent race conditions and redundant network requests when the user rapidly clicks the "Test Connection" button, track the active connection test task so it can be cancelled before starting a new one.
| private let healthShiftAlertNotifier: any HealthShiftAlertNotifying | |
| private let networkReachability: any NetworkReachability | |
| private let podLogStreamer: any PodLogStreaming | |
| private let aiCredentialStore: any AIProviderCredentialStoring | |
| private let aiConnectionTester: AIProviderConnectionTester? | |
| private let healthShiftAlertNotifier: any HealthShiftAlertNotifying | |
| private let networkReachability: any NetworkReachability | |
| private let podLogStreamer: any PodLogStreaming | |
| private let aiCredentialStore: any AIProviderCredentialStoring | |
| private let aiConnectionTester: AIProviderConnectionTester? | |
| private var aiConnectionTestTask: Task<Void, Never>? |
| func testAIConnection() { | ||
| guard let tester = aiConnectionTester else { | ||
| return | ||
| } | ||
|
|
||
| let provider = runtimeState.setupState.aiDiagnosticAssistant.config.provider | ||
| let config = runtimeState.setupState.aiDiagnosticAssistant.config | ||
| let draft = runtimeState.setupState.aiDiagnosticAssistant.hasAPIKeyDraft | ||
| ? runtimeState.setupState.aiDiagnosticAssistant.apiKeyDraft | ||
| : nil | ||
|
|
||
| runtimeState.setupState.applyAIDiagnosticAssistantTestConnectionResult(nil) | ||
| publishRuntimeState() | ||
|
|
||
| Task { | ||
| let result = await tester.testConnection(config: config, provider: provider, apiKeyOverride: draft) | ||
| runtimeState.setupState.applyAIDiagnosticAssistantTestConnectionResult(result) | ||
| publishRuntimeState() | ||
| } | ||
| } |
There was a problem hiding this comment.
Cancel any active connection test task before starting a new one to prevent race conditions and redundant network requests under rapid clicking.
func testAIConnection() {
guard let tester = aiConnectionTester else {
return
}
aiConnectionTestTask?.cancel()
let provider = runtimeState.setupState.aiDiagnosticAssistant.config.provider
let config = runtimeState.setupState.aiDiagnosticAssistant.config
let draft = runtimeState.setupState.aiDiagnosticAssistant.hasAPIKeyDraft
? runtimeState.setupState.aiDiagnosticAssistant.apiKeyDraft
: nil
runtimeState.setupState.applyAIDiagnosticAssistantTestConnectionResult(nil)
publishRuntimeState()
aiConnectionTestTask = Task {
let result = await tester.testConnection(config: config, provider: provider, apiKeyOverride: draft)
guard !Task.isCancelled else { return }
runtimeState.setupState.applyAIDiagnosticAssistantTestConnectionResult(result)
publishRuntimeState()
}
}| func saveAPIKey(_ key: String, for provider: AIProvider) throws { | ||
| let data = Data(key.utf8) | ||
| let query: [String: Any] = [ | ||
| kSecClass as String: kSecClassGenericPassword, | ||
| kSecAttrService as String: Self.service, | ||
| kSecAttrAccount as String: provider.rawValue | ||
| ] | ||
|
|
||
| let attributes: [String: Any] = [ | ||
| kSecValueData as String: data | ||
| ] | ||
|
|
||
| let updateStatus = SecItemUpdate(query as CFDictionary, attributes as CFDictionary) | ||
|
|
||
| switch updateStatus { | ||
| case errSecSuccess: | ||
| return | ||
| case errSecItemNotFound: | ||
| var addQuery = query | ||
| addQuery[kSecValueData as String] = data | ||
| let addStatus = SecItemAdd(addQuery as CFDictionary, nil) | ||
| if addStatus != errSecSuccess { | ||
| throw AIProviderCredentialStoreError.cannotSave | ||
| } | ||
| default: | ||
| throw AIProviderCredentialStoreError.cannotSave | ||
| } | ||
| } |
There was a problem hiding this comment.
Explicitly set kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock when saving or updating the API key. Since Kubebar supports "Start at Login", the app might launch and attempt to access the Keychain before the user has fully unlocked their session. Using kSecAttrAccessibleAfterFirstUnlock ensures the credential store remains accessible and reliable in these scenarios.
func saveAPIKey(_ key: String, for provider: AIProvider) throws {
let data = Data(key.utf8)
let query: [String: Any] = [
kSecClass as String: kSecClassGenericPassword,
kSecAttrService as String: Self.service,
kSecAttrAccount as String: provider.rawValue
]
let attributes: [String: Any] = [
kSecValueData as String: data,
kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlock
]
let updateStatus = SecItemUpdate(query as CFDictionary, attributes as CFDictionary)
switch updateStatus {
case errSecSuccess:
return
case errSecItemNotFound:
var addQuery = query
addQuery[kSecValueData as String] = data
addQuery[kSecAttrAccessible as String] = kSecAttrAccessibleAfterFirstUnlock
let addStatus = SecItemAdd(addQuery as CFDictionary, nil)
if addStatus != errSecSuccess {
throw AIProviderCredentialStoreError.cannotSave
}
default:
throw AIProviderCredentialStoreError.cannotSave
}
}| var body: some View { | ||
| VStack(alignment: .leading, spacing: 14) { | ||
| settingsTabs | ||
| VStack(alignment: .leading, spacing: 0) { | ||
| HStack(spacing: 0) { | ||
| settingsSidebar | ||
| Divider() | ||
| settingsDetail | ||
| } | ||
| footer | ||
| } | ||
| .padding(.horizontal, 24) | ||
| .padding(.top, 18) | ||
| .padding(.bottom, 16) | ||
| .frame(maxWidth: .infinity, maxHeight: .infinity, alignment: .topLeading) | ||
| } |
There was a problem hiding this comment.
Implement global keyboard shortcuts using hidden background buttons on the main container. This ensures the shortcuts work reliably and globally within the Settings window, as applying .keyboardShortcut directly to non-button list rows does not automatically trigger selection.
var body: some View {
VStack(alignment: .leading, spacing: 0) {
HStack(spacing: 0) {
settingsSidebar
Divider()
settingsDetail
}
footer
}
.background(
ForEach(Array(SetupFlowState.appPages.enumerated()), id: \\.element.id) { index, page in
Button("") {
selectedSettingsTabIDBinding.wrappedValue = page.id
}
.keyboardShortcut(KeyEquivalent(Character("\\(index + 1)")), modifiers: .command)
.buttonStyle(.plain)
.opacity(0)
.frame(width: 0, height: 0)
}
)
.frame(maxWidth: .infinity, maxHeight: .infinity, alignment: .topLeading)
}| ForEach(Array(SetupFlowState.appPages.enumerated()), id: \.element.id) { index, page in | ||
| sidebarRow( | ||
| title: page.title, | ||
| icon: page.systemImageName, | ||
| tabID: page.id, | ||
| help: page.helpText, | ||
| warning: false | ||
| ) | ||
| .keyboardShortcut(KeyEquivalent(Character("\(index + 1)")), modifiers: .command) | ||
| } |
There was a problem hiding this comment.
Remove the .keyboardShortcut modifier from the individual list rows since they are not buttons and will not trigger selection automatically. The shortcuts are more reliably handled via background buttons on the main container.
ForEach(Array(SetupFlowState.appPages.enumerated()), id: \\.element.id) { index, page in
sidebarRow(
title: page.title,
icon: page.systemImageName,
tabID: page.id,
help: page.helpText,
warning: false
)
}| public struct AIDiagnosticAssistantConfig: Codable, Equatable, Sendable { | ||
| public let provider: AIProvider | ||
| public var modelID: String | ||
| public let baseURL: String? | ||
|
|
||
| public init( | ||
| provider: AIProvider = .openAI, | ||
| modelID: String = "", | ||
| baseURL: String? = nil | ||
| ) { | ||
| self.provider = provider | ||
| self.modelID = modelID | ||
| self.baseURL = baseURL | ||
| } | ||
|
|
||
| public func with(modelID: String) -> AIDiagnosticAssistantConfig { | ||
| AIDiagnosticAssistantConfig( | ||
| provider: provider, | ||
| modelID: modelID, | ||
| baseURL: baseURL | ||
| ) | ||
| } | ||
|
|
||
| public func with(baseURL: String?) -> AIDiagnosticAssistantConfig { | ||
| AIDiagnosticAssistantConfig( | ||
| provider: provider, | ||
| modelID: modelID, | ||
| baseURL: baseURL | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Make all properties of AIDiagnosticAssistantConfig mutable (var) instead of a mix of let and var with custom with methods. Since AIDiagnosticAssistantConfig is a struct (value type), mutating its properties is safe and idiomatic in Swift. This simplifies the model, removes boilerplate code, and makes updates consistent across all fields.
public struct AIDiagnosticAssistantConfig: Codable, Equatable, Sendable {
public var provider: AIProvider
public var modelID: String
public var baseURL: String?
public init(
provider: AIProvider = .openAI,
modelID: String = "",
baseURL: String? = nil
) {
self.provider = provider
self.modelID = modelID
self.baseURL = baseURL
}
}| public mutating func updateAIDiagnosticAssistant(baseURL: String?) { | ||
| aiDiagnosticAssistant.config = aiDiagnosticAssistant.config.with(baseURL: baseURL) | ||
| configurationMessage = nil | ||
| } |
There was a problem hiding this comment.
Simplify the mutation of baseURL by directly assigning the value to the mutable property, matching the pattern used for modelID.
| public mutating func updateAIDiagnosticAssistant(baseURL: String?) { | |
| aiDiagnosticAssistant.config = aiDiagnosticAssistant.config.with(baseURL: baseURL) | |
| configurationMessage = nil | |
| } | |
| public mutating func updateAIDiagnosticAssistant(baseURL: String?) { | |
| aiDiagnosticAssistant.config.baseURL = baseURL | |
| configurationMessage = nil | |
| } |
1 participant
Summary
Add a configurable AI Diagnostic Assistant to Kubebar Settings with Keychain-backed API keys and manual Test Connection. Redesign the Settings window from a stacked TabView into a sidebar/detail layout.
AI Diagnostic Assistant
Settings Sidebar Layout
.appSettingsaliasCmd+1–Cmd+4for App pagesUI Polish
k.stackSF SymbolVerification
swift build+swift-quality-gate.sh localpassrtk git diff --checkcleancompile-and-run.shNotes