Conversation
Codacy's Analysis Summary0 new issue (≤ 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
This PR improves IntelliJ plugin stability by refining coroutine lifecycles and service registration. However, several critical issues must be addressed before merging. Specifically, there are thread-safety risks in the global scope management within StartupListener.kt and a remaining race condition in RepositoryManager.kt initialization.
Furthermore, the implementation of API error handling in Api.kt contradicts the objective of providing meaningful error messages, as it currently wraps all failures in generic exceptions, obscuring the root cause. While the PR aims for better resource management, the continued use of GlobalScope in several files undermines the transition to structured concurrency.
About this PR
- Systemic issue:
GlobalScope.launchis still used inRepositoryManager.ktandConfig.kt. This bypasses the structured concurrency improvements introduced inStartupListener.ktand can lead to leaked coroutines when the plugin is unloaded. - The use of
printlninApi.kt(line 38) is inconsistent with the use of aLoggerelsewhere in the codebase. Replace with a standard logger for consistency and better log management. - The PR description is empty. Please provide context regarding why these specific stability changes were prioritized and any potential side effects considered.
Suggestions for missing tests
- Verify that closing a project triggers the Disposer to cancel the specific CoroutineScope associated with that project.
- Verify that unloading the plugin (DynamicPluginListener) invokes StartupListener.cancelAllScopes() and successfully terminates pending tasks.
- Verify that Api.makeRequest throws an exception containing the original cause when a network error occurs, instead of returning a default instance.
- Verify that RepositoryManager.open sets the currentRepository state before launching the background metadata fetching task.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that closing a project triggers the Disposer to cancel the specific CoroutineScope associated with that project.
2. Verify that unloading the plugin (DynamicPluginListener) invokes StartupListener.cancelAllScopes() and successfully terminates pending tasks.
3. Verify that Api.makeRequest throws an exception containing the original cause when a network error occurs, instead of returning a default instance.
4. Verify that RepositoryManager.open sets the currentRepository state before launching the background metadata fetching task.
🗒️ Improve review quality by adding custom instructions
| throw Exception("Failed to fetch data: HTTP response code $responseCode") | ||
| } | ||
| } | ||
| catch (e: Exception) { |
There was a problem hiding this comment.
🔴 HIGH RISK
Re-throwing a generic Exception obscures the actual failure type (e.g., IOException or JsonSyntaxException). Refactor makeRequest to catch and handle specific exceptions or wrap them in a custom CodacyApiException to preserve context for callers.
| } | ||
|
|
||
| if (currentRepository != gitRepository) { | ||
| currentRepository = gitRepository |
There was a problem hiding this comment.
🟡 MEDIUM RISK
While moving the currentRepository assignment helps, it is still not atomic across different threads. Concurrent calls to open (e.g., from both startup and git events) can result in redundant background initialization tasks. Use a Mutex or AtomicReference to synchronize this check-and-set operation.
|
|
||
| companion object { | ||
| private const val MIN_REFRESH_INTERVAL_MS = 5000L | ||
| private val activeScopes = mutableListOf<CoroutineScope>() |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The activeScopes list is a shared mutable list in a companion object accessed by multiple project-level coroutines. Using a non-thread-safe ArrayList can lead to ConcurrentModificationException during project disposal or plugin unloading.
| private val activeScopes = mutableListOf<CoroutineScope>() | |
| private val activeScopes = java.util.Collections.synchronizedList(mutableListOf<CoroutineScope>()) |
| Logger.info("Fetched ${enabledBranches.size} enabled branches") | ||
| } catch (e: Exception) { | ||
| Logger.error("Failed to fetch enabled branches: ${e.message}") | ||
| Logger.warn("Failed toAf fetch enabled branches: ${e.message}") |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: Typo in log message: 'toAf' should be 'to'.
| Logger.warn("Failed toAf fetch enabled branches: ${e.message}") | |
| Logger.warn("Failed to fetch enabled branches: ${e.message}") |
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving IntelliJ plugin stability by migrating core components to IntelliJ services (instead of manual instantiation) and by introducing lifecycle-managed coroutine scopes to reduce background-work leaks during project disposal / plugin unload.
Changes:
- Replace
Api()/Config()instantiations with IntelliJ service lookups (service<Api>(),Config.instance). - Introduce a per-project
CoroutineScopeinStartupListener, cancel it on project dispose, and cancel all scopes on plugin unload. - Update logging severity and bump plugin version to
0.1.0with a changelog entry.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/kotlin/com/codacy/intellij/plugin/services/git/RepositoryManager.kt | Uses DI for Api/Config, minor flow adjustment in repo open, logging changes. |
| src/main/kotlin/com/codacy/intellij/plugin/services/git/PullRequest.kt | Uses DI for Api. |
| src/main/kotlin/com/codacy/intellij/plugin/services/common/Config.kt | Uses DI Api when fetching user profile after storing token. |
| src/main/kotlin/com/codacy/intellij/plugin/services/api/Api.kt | Declares Api as an APP-level service; makeRequest now throws instead of returning default instances; logging tweak. |
| src/main/kotlin/com/codacy/intellij/plugin/listeners/StartupListener.kt | Adds project-tied coroutine scope, cancels on dispose/unload, avoids GlobalScope, adds initial repository open. |
| src/main/kotlin/com/codacy/intellij/plugin/listeners/PluginStateListener.kt | Cancels all tracked scopes on plugin unload. |
| gradle.properties | Version bump to 0.1.0. |
| CHANGELOG.md | Adds 0.1.0 entry. |
Comments suppressed due to low confidence (1)
src/main/kotlin/com/codacy/intellij/plugin/listeners/StartupListener.kt:129
- initializeGit throws IllegalStateException (and uses !! on remote.firstUrl) when no remote/URL is present. Since this runs inside the GitRepository change listener, it can surface as an unexpected runtime exception during normal IDE usage (e.g., newly inited repo with no remote). Handle this case gracefully (log + return early / skip initialization) instead of throwing.
val remote = gitRepository?.remotes?.firstOrNull()
?: throw IllegalStateException("No remote found in the Git repository")
val gitInfo = GitRemoteParser.parseGitRemote(remote.firstUrl!!)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ProgressManager.getInstance().run(object : Task.Backgroundable(project, "Opening repository", false) { | ||
| override fun run(indicator: com.intellij.openapi.progress.ProgressIndicator) { | ||
| GlobalScope.launch { | ||
| currentRepository = gitRepository | ||
| try { |
There was a problem hiding this comment.
This launches work in GlobalScope inside a project-scoped service, so the job will outlive the project/plugin unload and cannot be cancelled (potential leak / background work after dispose). Prefer launching on a structured CoroutineScope tied to the project lifecycle (e.g., a scope stored in the service and cancelled on dispose, or passed in from StartupListener) instead of GlobalScope.
| private val activeScopes = mutableListOf<CoroutineScope>() | ||
|
|
||
| fun cancelAllScopes() { | ||
| activeScopes.forEach { it.cancel() } | ||
| activeScopes.clear() | ||
| } |
There was a problem hiding this comment.
activeScopes is a mutableList accessed from multiple lifecycle callbacks (runActivity, project dispose, beforePluginUnload). As written, cancelAllScopes() iterates while other code may remove scopes, which can throw ConcurrentModificationException. Use a thread-safe collection (e.g., CopyOnWriteArrayList / synchronizedList) or synchronize access around add/remove/iterate/clear.
| scope.launch { | ||
| service<Api>().listTools() | ||
| } |
There was a problem hiding this comment.
Api.listTools() can now throw (Api.makeRequest throws on failures). This launch block doesn't handle exceptions, so failures will be reported as uncaught coroutine exceptions in the IDE logs. Wrap this call in try/catch (and log at warn) or use runCatching so startup remains quiet/stable when the API is unavailable.
|
|
||
| ### Changed | ||
|
|
||
| - Stability update |
There was a problem hiding this comment.
Changelog entry has a trailing space after "Stability update"; consider trimming it to keep the changelog clean.
| - Stability update | |
| - Stability update |
| Logger.info("Fetched ${enabledBranches.size} enabled branches") | ||
| } catch (e: Exception) { | ||
| Logger.error("Failed to fetch enabled branches: ${e.message}") | ||
| Logger.warn("Failed toAf fetch enabled branches: ${e.message}") |
There was a problem hiding this comment.
Log message has a typo ("Failed toAf fetch enabled branches"); please correct it so logs are searchable and readable.
| Logger.warn("Failed toAf fetch enabled branches: ${e.message}") | |
| Logger.warn("Failed to fetch enabled branches: ${e.message}") |
No description provided.