fix: improve AI provider connection test error handling#407
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAPI keys are trimmed on init across providers; providers now parse JSON error messages and map HTTP errors accordingly. AnthropicProvider fetches models from the API with a local fallback and adjusted testConnection behavior. Settings UI prevents testing when required API keys are blank. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TablePro/Core/AI/OpenAICompatibleProvider.swift (1)
149-158:⚠️ Potential issue | 🟡 MinorInconsistent error handling for non-200 responses.
The test only throws for HTTP 401, but other error codes (403, 429, 500, etc.) will still return
trueif the response is JSON. This differs fromGeminiProvider.testConnection()which throws for all non-200 responses. Consider aligning the behavior:🔧 Proposed fix to handle all HTTP errors
if httpResponse.statusCode == 401 { throw AIProviderError.authenticationFailed("") } + // Throw mapped error for other non-success status codes + if httpResponse.statusCode >= 400 { + let body = String(data: data, encoding: .utf8) ?? "" + throw mapHTTPError(statusCode: httpResponse.statusCode, body: body) + } + // Non-JSON response means wrong endpoint (e.g., HTML 404 page) if !isJSON { return false }Note: This would require capturing
datafromsession.data(for: request)at Line 139.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/AI/OpenAICompatibleProvider.swift` around lines 149 - 158, The testConnection implementation in OpenAICompatibleProvider currently only throws on 401 and returns true for other JSON non-200 responses; align it with GeminiProvider.testConnection by treating any non-2xx HTTP status as an error: capture the response body data from session.data(for: request) (as noted at the earlier call site), check if httpResponse.statusCode is in 200..<300 and if not, throw an appropriate AIProviderError (use AIProviderError.authenticationFailed("") for 401 and a generic HTTP error for other codes, including any message parsed from the captured data/JSON) instead of returning true; reference the testConnection method and AIProviderError types to locate where to change the status code check and error construction.
🧹 Nitpick comments (2)
TablePro/Views/Settings/AISettingsView.swift (1)
549-553: Consider simplifying the guard condition for readability.The double-negative logic is hard to parse. Consider restructuring for clarity:
♻️ Proposed refactor
func testProvider() { - guard !editingAPIKey.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty || !draft.type.requiresAPIKey else { + let trimmedKey = editingAPIKey.trimmingCharacters(in: .whitespacesAndNewlines) + if draft.type.requiresAPIKey && trimmedKey.isEmpty { testResult = .failure(String(localized: "API key is required")) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Settings/AISettingsView.swift` around lines 549 - 553, The guard in testProvider uses a double-negative which is hard to read; replace it with a straightforward conditional that first checks if draft.type.requiresAPIKey is true and then checks whether editingAPIKey.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty, setting testResult = .failure(...) and returning in that case, otherwise continue — reference the testProvider function and the editingAPIKey, draft.type.requiresAPIKey and testResult symbols when making the change.TablePro/Core/AI/GeminiProvider.swift (1)
224-233: Consider extracting shared error parsing logic.This
parseErrorMessageimplementation is identical acrossGeminiProvider,OpenAICompatibleProvider, andAnthropicProvider. Could be extracted to a shared utility or protocol extension to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/AI/GeminiProvider.swift` around lines 224 - 233, The parseErrorMessage implementation is duplicated in GeminiProvider, OpenAICompatibleProvider, and AnthropicProvider; extract it into a single shared implementation (either a utility function or a protocol extension) and have each provider call that shared routine instead of keeping local copies. For example, create a common helper (e.g., an ErrorParsing utility function or an ErrorParsable protocol with a default parseErrorMessage(_:)->String? extension) and replace the parseErrorMessage implementations in GeminiProvider, OpenAICompatibleProvider, and AnthropicProvider with calls to that shared helper to remove duplication and centralize parsing logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@TablePro/Core/AI/OpenAICompatibleProvider.swift`:
- Around line 149-158: The testConnection implementation in
OpenAICompatibleProvider currently only throws on 401 and returns true for other
JSON non-200 responses; align it with GeminiProvider.testConnection by treating
any non-2xx HTTP status as an error: capture the response body data from
session.data(for: request) (as noted at the earlier call site), check if
httpResponse.statusCode is in 200..<300 and if not, throw an appropriate
AIProviderError (use AIProviderError.authenticationFailed("") for 401 and a
generic HTTP error for other codes, including any message parsed from the
captured data/JSON) instead of returning true; reference the testConnection
method and AIProviderError types to locate where to change the status code check
and error construction.
---
Nitpick comments:
In `@TablePro/Core/AI/GeminiProvider.swift`:
- Around line 224-233: The parseErrorMessage implementation is duplicated in
GeminiProvider, OpenAICompatibleProvider, and AnthropicProvider; extract it into
a single shared implementation (either a utility function or a protocol
extension) and have each provider call that shared routine instead of keeping
local copies. For example, create a common helper (e.g., an ErrorParsing utility
function or an ErrorParsable protocol with a default
parseErrorMessage(_:)->String? extension) and replace the parseErrorMessage
implementations in GeminiProvider, OpenAICompatibleProvider, and
AnthropicProvider with calls to that shared helper to remove duplication and
centralize parsing logic.
In `@TablePro/Views/Settings/AISettingsView.swift`:
- Around line 549-553: The guard in testProvider uses a double-negative which is
hard to read; replace it with a straightforward conditional that first checks if
draft.type.requiresAPIKey is true and then checks whether
editingAPIKey.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty, setting
testResult = .failure(...) and returning in that case, otherwise continue —
reference the testProvider function and the editingAPIKey,
draft.type.requiresAPIKey and testResult symbols when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ecd2f6b-7772-458c-9aa6-7937e14cb500
📒 Files selected for processing (4)
TablePro/Core/AI/AnthropicProvider.swiftTablePro/Core/AI/GeminiProvider.swiftTablePro/Core/AI/OpenAICompatibleProvider.swiftTablePro/Views/Settings/AISettingsView.swift
…aming, fix body.count
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/v1/modelsAPI with hardcoded fallback; test connection now treats 400 (billing) as success since the key is valid; parse JSON error responses for clean messages; 401 shows "Authentication failed. Check your API key." instead of raw API textreturn false; parse JSON error responses; trim API keysTest plan
Summary by CodeRabbit
Bug Fixes
Improvements
New Features