Repository navigation
IntelliJ stability update - #50
Conversation
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
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.
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } catch (e: Error) { | ||
| // TODO("error type (ApiError)") | ||
| Logger.error("Failed to parse Git remote: ${e.message}") | ||
| } catch (e: Exception) { | ||
| Logger.warn("Failed to parse Git remote: ${e.message}") | ||
| setNewState(RepositoryManagerState.NoRepository) | ||
| return@launch |
There was a problem hiding this comment.
The catch (e: Exception) around the whole open() body will also catch API/network failures from api.getRepository(...), but the log message/state update treats them as a Git remote parsing problem (NoRepository). Split the try/catch so remote parsing errors are handled separately from API errors, and set a more accurate state/log message for request failures (or let them propagate to a higher-level handler).
| if (gitRepository != null && repositoryManager.currentRepository != gitRepository) | ||
| GlobalScope.launch { repositoryManager.open(gitRepository) } | ||
| scope.launch { repositoryManager.open(gitRepository) } | ||
|
|
||
| val remote = gitRepository?.remotes?.firstOrNull() | ||
| ?: throw IllegalStateException("No remote found in the Git repository") |
There was a problem hiding this comment.
initializeGit throws IllegalStateException when no remote is found. Since this runs inside the Git repo change listener, it can surface as an unhandled exception and destabilize the plugin for repos without remotes (or during transient Git states). Handle the "no remote" case gracefully (e.g., return a sentinel result / skip initialization and update plugin state) instead of throwing here.
| val gitResult = initializeGit(project, scope) | ||
| val repositoryManager = gitResult.first | ||
| val gitInfo = gitResult.second |
There was a problem hiding this comment.
initializeGit(project, scope) can throw (e.g., when no Git remote is configured). Since this call is inside the repository change listener without a try/catch, an exception here can abort the listener and spam IDE logs. Wrap this initialization in error handling and fall back to a safe state (e.g., skip starting services / set RepositoryManager to NoGitRepository).
| private val refreshTimeout: TimeoutManager = TimeoutManager() | ||
| private val api = Api() | ||
| private val api = service<Api>() | ||
| private var repositoryManager: RepositoryManager? = null |
There was a problem hiding this comment.
Api calls can now throw (via makeRequest), but PullRequest.refresh() / fetch*() paths currently don't have a top-level try/catch around the sequence of API calls. A single transient failure can abort the refresh coroutine and stop subsequent scheduled refreshes. Add appropriate exception handling around the refresh flow (and consider updating the state/notification) so refresh is resilient to network/API errors.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/main/kotlin/com/codacy/intellij/plugin/services/api/Api.kt:20
Apiis now registered as an APP-level service, but there is existing code that still retrieves it as a project service (e.g.project.service<Api>()inviews/LocalQuickFix.kt).project.service<Api>()will not resolve an APP service and will fail at runtime. Either keepApias a PROJECT service, or update those call sites to use the application service accessor (service<Api>()/ApplicationManager.getApplication().service<Api>()).
@Service(Service.Level.APP)
class Api {
private val config = service<Config>()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| override fun run(indicator: com.intellij.openapi.progress.ProgressIndicator) { | ||
| GlobalScope.launch { | ||
| currentRepository = gitRepository | ||
| serviceScope.launch { |
There was a problem hiding this comment.
open() can still run multiple background jobs concurrently for different GitRepository instances. Since the coroutine uses the captured gitRepository and updates shared fields (repository, state, enabledBranches, etc.) without verifying that currentRepository still matches, a slower previous open() can overwrite state after a newer repository is opened. Consider tracking/cancelling the in-flight open Job when a new repository is set, or add a guard before applying results (e.g., bail out if currentRepository !== gitRepository).
| serviceScope.launch { | |
| serviceScope.launch { | |
| // Bail out early if a newer repository has been opened while this job was queued/running | |
| if (currentRepository != gitRepository) { | |
| Logger.info("Repository changed during open; aborting outdated job") | |
| return@launch | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scope.launch { repositoryManager.open(gitRepository) } | ||
|
|
||
| val remote = gitRepository?.remotes?.firstOrNull() ?: return null | ||
| val gitInfo = GitRemoteParser.parseGitRemote(remote.firstUrl!!) |
There was a problem hiding this comment.
initializeGit dereferences remote.firstUrl!!, but firstUrl can be null (e.g., remote with no URLs configured). This will throw an NPE and break startup; consider using remote.firstUrl ?: return null (or selecting from pushUrls/fetchUrls) and treat it as “no remote info” like the earlier null-return path.
| val gitInfo = GitRemoteParser.parseGitRemote(remote.firstUrl!!) | |
| val remoteUrl = remote.firstUrl ?: return null | |
| val gitInfo = GitRemoteParser.parseGitRemote(remoteUrl) |
| fun getServiceWithoutRemote(project: Project): CodacyCliService { | ||
| return getService(Provider.GITHUB, "", "", project) | ||
| } |
There was a problem hiding this comment.
getServiceWithoutRemote initializes the service with empty org/repo. Because initService only sets provider/organization/repository when isServiceInstantiated is false (see initService guard earlier in this file), calling this once can permanently prevent later re-initialization with the real Git remote when it becomes available. Consider allowing initService to update repo context on subsequent calls, or avoid marking the service as instantiated until remote data exists.
| fun getServiceWithoutRemote(project: Project): AiAgentService { | ||
| val pathsBehaviour = PathsBehaviour.Factory.build() | ||
| val service = project.getService(AiAgentService::class.java) | ||
| service.initService(Provider.GITHUB, "", "", project, pathsBehaviour) | ||
| return service |
There was a problem hiding this comment.
getServiceWithoutRemote calls initService with empty org/repo. Since initService only assigns provider/organization/repository the first time (isServiceInstantiated guard), this can lock the service into a blank repo context and prevent later getService(project) from updating to the actual remote when it becomes available. Consider supporting re-initialization of the repo context (or deferring instantiation until remote info exists).
| if (gitResult == null) { | ||
| Logger.info("No git remote found, initializing services without remote info.") | ||
| CodacyCliService.getServiceWithoutRemote(project) | ||
| AiAgentService.getServiceWithoutRemote(project) | ||
| } else { |
There was a problem hiding this comment.
Calling CodacyCliService.getServiceWithoutRemote() / AiAgentService.getServiceWithoutRemote() here can permanently initialize those services with empty org/repo (both services’ initService methods only set repo context on first initialization). If a remote becomes available later in the session, subsequent getService(project) calls may not update the context. Consider not instantiating services until remote info exists, or ensure services can be re-initialized with real repo data once available.
| installCliBtn?.addActionListener { | ||
| val message = when (cliService?.codacyCliState) { | ||
| is CodacyCliState.ERROR, CodacyCliState.NOT_INSTALLED -> Messages.showYesNoDialog( | ||
| val svc = cliService ?: CodacyCliService.getServiceWithoutRemote(project).also { cliService = it } |
There was a problem hiding this comment.
This fallback initializes the CLI service with empty org/repo (getServiceWithoutRemote). Since CodacyCliService.initService only assigns repo context once, this can prevent the service from ever being updated with the real Git remote later. Prefer obtaining/initializing the service with real repo info when available, or adjust the service to support updating its repo context after initial instantiation.
| val svc = cliService ?: CodacyCliService.getServiceWithoutRemote(project).also { cliService = it } | |
| val svc = cliService ?: project.getService(CodacyCliService::class.java).also { cliService = it } |
| } else { | ||
| Logger.error("Failed to fetch data: HTTP response code $responseCode") | ||
| if (responseCode == HttpURLConnection.HTTP_UNAUTHORIZED) | ||
| config.storeApiToken("") | ||
| throw Exception("Failed to fetch data: HTTP response code $responseCode") | ||
| } |
There was a problem hiding this comment.
makeRequest now throws on non-200 responses and also rethrows on any exception, removing the previous “return empty instance” fallback. Several callers (e.g., PullRequest.refresh() in services/git/PullRequest.kt) call API methods from a GlobalScope.launch without try/catch, which will now surface as unhandled coroutine exceptions and can destabilize the plugin. Consider either restoring a safe fallback per endpoint, or auditing call sites to handle exceptions explicitly (and updating UI/state accordingly).
| project.messageBus.connect().subscribe(WidgetStateListener.CLI_TOPIC, object : WidgetStateListener { | ||
| override fun stateChanged(newState: ServiceState) { |
| project.messageBus.connect().subscribe(WidgetStateListener.AI_AGENT_TOPIC, object : WidgetStateListener { | ||
| override fun stateChanged(newState: ServiceState) { |
| val remote = gitRepository?.remotes?.firstOrNull() ?: return null | ||
| val gitInfo = GitRemoteParser.parseGitRemote(remote.firstUrl!!) | ||
|
|
||
| return Pair(repositoryManager, gitInfo) |
No description provided.