Improve plugin error handling scenarios#49
Conversation
Codacy's Analysis Summary4 new issues, 2 flagged as potential false positives (≤ 1 medium issue)
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
The PR successfully introduces several safety checks and error handling mechanisms to improve the plugin's stability. However, it fails to meet standards due to multiple 'Too Generic Exception Caught' issues and high cyclomatic complexity in the PullRequest service. Additionally, the use of GlobalScope for background tasks is a concern for plugin lifecycle management.
About this PR
- Please provide a description for the PR. It should summarize the changes and list the specific error scenarios that were tested to ensure completeness.
Suggestions for missing tests
- Verify that the plugin does not crash when the Codacy API returns a non-200 response in
Api.kt. - Verify that
StartupListenerhandles cases whereinitializeGitthrows anIllegalStateException(e.g., missing remote). - Verify that the PR summary tool window displays gracefully when the login URL contains no query parameters.
- Verify that the
CommitIssuesInspectioncorrectly skips files where the inspection report points to a line number outside the current document range.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that the plugin does not crash when the Codacy API returns a non-200 response in `Api.kt`.
2. Verify that `StartupListener` handles cases where `initializeGit` throws an `IllegalStateException` (e.g., missing remote).
3. Verify that the PR summary tool window displays gracefully when the login URL contains no query parameters.
4. Verify that the `CommitIssuesInspection` correctly skips files where the inspection report points to a line number outside the current document range.
🗒️ Improve review quality by adding custom instructions
| @@ -193,30 +199,35 @@ class PullRequest( | |||
| ProgressManager.getInstance().run(object: Task.Backgroundable(project, "Refreshing pull request", false) { | |||
| override fun run(indicator: com.intellij.openapi.progress.ProgressIndicator) { | |||
There was a problem hiding this comment.
🟡 MEDIUM RISK
Codacy flagged this method with high cyclomatic complexity. Consider refactoring the fetching logic into smaller, dedicated methods within the PullRequest class.
Try running the following prompt in your IDE agent:
Refactor the
runmethod in thePullRequest.refreshtask to extract the fetching logic (metadata, gates, issues, files) into a single private method to reduce complexity.
| @@ -193,30 +199,35 @@ class PullRequest( | |||
| ProgressManager.getInstance().run(object: Task.Backgroundable(project, "Refreshing pull request", false) { | |||
| override fun run(indicator: com.intellij.openapi.progress.ProgressIndicator) { | |||
| GlobalScope.launch { | |||
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Since this code is already running inside Task.Backgroundable.run, which is executed on a background thread, there is no need to wrap the entire logic in GlobalScope.launch. This adds unnecessary overhead and lifecycle risks.
|
|
||
| AiAgentService.getService(project) | ||
|
|
||
| GlobalScope.launch { |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Avoid using GlobalScope. Instead, use the project's coroutine scope or service<Api>(). Instantiating Api() manually bypasses the service manager.
Try running the following prompt in your IDE agent:
Refactor the coroutine launch at line 63 in StartupListener.kt to use a proper project-level coroutine scope and use the Api service instead of manual instantiation.
| loadPullRequest() | ||
| } | ||
| } catch (e: Error) { | ||
| } catch (e: Exception) { |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Changing the catch block from Error to Exception is a significant improvement as most API and logic failures throw Exception, which was previously slipping through. Resetting loadAttempts ensures the retry logic starts fresh on branch changes.
There was a problem hiding this comment.
Pull request overview
This PR tightens up several plugin error-handling paths to avoid crashes/NPEs during initialization, analysis, and UI interactions, and shifts API failures toward explicit exception propagation.
Changes:
- Added defensive guards/try-catch blocks around service initialization and background tasks (CLI service, PR refresh, startup init).
- Improved resilience in UI flows (status bar popup only shown when it has actions; null-safe token query access; bounds-checking document line access).
- Refactored API usage to rely more on IntelliJ services and changed
Api.makeRequestto rethrow on failure instead of returning empty instances.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/kotlin/com/codacy/intellij/plugin/views/SarifExternalAnnotator.kt |
Wraps CLI service access in try/catch to avoid annotator crashes. |
src/main/kotlin/com/codacy/intellij/plugin/views/CommitIssuesInspection.kt |
Adds bounds check for line access to prevent out-of-range document offsets. |
src/main/kotlin/com/codacy/intellij/plugin/views/CodacyPullRequestSummaryToolWindowFactory.kt |
Makes token query handling null-safe in the local HTTP callback. |
src/main/kotlin/com/codacy/intellij/plugin/views/CodacyCliStatusBarWidget.kt |
Avoids showing an empty popup; makes AI agent menu conditional. |
src/main/kotlin/com/codacy/intellij/plugin/services/git/RepositoryManager.kt |
Switches Api/Config to service retrieval; broadens exception handling; resets retry attempts on branch change. |
src/main/kotlin/com/codacy/intellij/plugin/services/git/PullRequest.kt |
Uses Api service; tolerates coverage-report failures; improves refresh robustness with try/catch and nullable head SHA handling. |
src/main/kotlin/com/codacy/intellij/plugin/services/common/ConfigConfigurable.kt |
Prevents settings UI from failing if CLI version fetch throws. |
src/main/kotlin/com/codacy/intellij/plugin/services/api/Api.kt |
Removes silent fallback instance creation; rethrows request exceptions. |
src/main/kotlin/com/codacy/intellij/plugin/listeners/StartupListener.kt |
Wraps project init and tool list fetch with exception handling and logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| GlobalScope.launch { | ||
| try { | ||
| Api().listTools() | ||
| } catch (e: Exception) { | ||
| Logger.warn("Failed to fetch tools list: ${e.message}") | ||
| } | ||
| } |
There was a problem hiding this comment.
Api().listTools() creates a new Api instance, but other code (e.g. quick fixes) uses the IDE service instance (project.service<Api>()) where tools remains null. Please fetch tools via the shared service instance (or move tool caching out of the Api instance) so getTool() works consistently.
| val cliService = try { | ||
| CodacyCliService.getService(file.project) | ||
| } catch (e: Exception) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
The exception from CodacyCliService.getService(file.project) is swallowed and the annotator silently disables itself (return null). Please log this (ideally with stack trace) at least at warn level so users/devs can diagnose why annotations stop working.
| val cli = try { | ||
| CodacyCliService.getService(file.project) | ||
| } catch (e: Exception) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
In doAnnotate, failures to obtain the CLI service are silently ignored (return null). This makes analysis failures hard to diagnose; please log the exception (with stack trace) and consider recording a single warning per session to avoid log spam.
| val tagNames = try { | ||
| fetchAvailableCliVersions() | ||
| } catch (e: Exception) { | ||
| emptyList() | ||
| } |
There was a problem hiding this comment.
fetchAvailableCliVersions() errors are swallowed and replaced with an empty list. Please log the exception (at least warn) so failures (curl missing, network issues, GitHub API changes) can be diagnosed instead of silently clearing the versions dropdown.
| server.createContext("/token") { exchange -> | ||
| try { | ||
| val token = exchange.requestURI.query.split("=").last() | ||
| val token = exchange.requestURI.query?.split("=")?.last() ?: "" | ||
| if (token.isNotEmpty()) { |
There was a problem hiding this comment.
Token parsing here is still fragile: splitting on "=" doesn't handle multiple query params, and when token is blank you never send a response/close the exchange (request can hang). Consider properly parsing the query for the token parameter, URL-decoding it, and always replying (e.g., 400 on missing/blank token) while closing the exchange.
| } catch (e: Error) { | ||
| } catch (e: Exception) { | ||
| // TODO("error type (ApiError)") | ||
| Logger.error("Failed to parse Git remote: ${e.message}") |
There was a problem hiding this comment.
This catch now handles any Exception, not just parsing problems, but the log message still says "Failed to parse Git remote". Please update the message to reflect the broader failure (parsing vs API calls) and include the exception object so the stack trace is captured.
| Logger.error("Failed to parse Git remote: ${e.message}") | |
| Logger.error("Failed to load repository (Git remote parsing or API call): ${e.message}", e) |
No description provided.