-
-
Notifications
You must be signed in to change notification settings - Fork 14
refactor: replace MiseProjectService with MiseTaskResolver #396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…kDocumentationProvider
Summary of ChangesHello @134130, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring of the core task management services by deprecating and removing the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the project by replacing MiseProjectService with MiseTaskResolver, which is a good move towards a more on-demand and reactive architecture. The changes are mostly consistent and improve the codebase by using newer platform APIs like smartReadAction.
However, I've found a critical issue in MiseTomlFileVfsListener where file change listeners are not being registered, which will break cache invalidation. I've also noted a potential regression due to the removal of a PsiTreeChangeListener. Additionally, a test has been disabled, which should be addressed. Please see my detailed comments.
modules/core/src/main/kotlin/com/github/l34130/mise/core/MiseTomlFileVfsListener.kt
Outdated
Show resolved
Hide resolved
| fun `test tasks`() { | ||
| // TODO: fix the test directory | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the codebase by replacing MiseProjectService with MiseTaskResolver, removing the need for explicit initialization and centralizing task resolution logic. The refactoring eliminates the service layer that managed task caching and refresh logic, instead moving to an on-demand task resolution model with per-directory caching.
- Removed
MiseProjectServiceentirely and migrated its task retrieval functionality toMiseTaskResolver - Updated all callers to use
MiseTaskResolver.getMiseTasks()withrunBlockingfor synchronous access - Simplified VFS listener registration and event handling by making services disposable and self-registering
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/core/src/main/kotlin/com/github/l34130/mise/core/MiseProjectService.kt | Deleted the entire service class that was responsible for initializing and caching tasks |
| modules/core/src/main/kotlin/com/github/l34130/mise/core/MiseTaskResolver.kt | Added Disposable interface, cache invalidation on file changes, and convenience overload for getting tasks without explicit base directory |
| modules/core/src/main/kotlin/com/github/l34130/mise/core/MiseConfigFileResolver.kt | Added Disposable interface and self-registration for VFS change listening |
| modules/core/src/main/kotlin/com/github/l34130/mise/core/MiseTomlFileVfsListener.kt | Simplified listener registration API and changed Topic type from Runnable to Function0 |
| modules/core/src/main/kotlin/com/github/l34130/mise/core/MiseStartupActivity.kt | Removed initialization logic, leaving empty execute method |
| modules/core/src/main/kotlin/com/github/l34130/mise/core/toolwindow/nodes/MiseRootNode.kt | Removed service initialization waiting logic |
| modules/core/src/main/kotlin/com/github/l34130/mise/core/util/PathUtils.kt | Removed deprecation annotation from baseDirectory() function |
| modules/core/src/main/kotlin/com/github/l34130/mise/core/lang/completion/MiseTomlTaskCompletionProvider.kt | Replaced service call with MiseTaskResolver using runBlocking, removed smartReadAction wrapper |
| modules/core/src/main/kotlin/com/github/l34130/mise/core/lang/resolve/MiseTomlTaskDependsReferenceProvider.kt | Updated to use MiseTaskResolver with runBlocking for synchronous task retrieval |
| modules/core/src/main/kotlin/com/github/l34130/mise/core/lang/resolve/MiseTaskDocumentationProvider.kt | Updated to use MiseTaskResolver with runBlocking, added nullable cast for type safety |
| modules/products/diagram/src/main/kotlin/com/github/l34130/mise/diagram/MiseFullTaskGraphDataModel.kt | Updated to use MiseTaskResolver with runBlocking in field initialization |
| modules/products/diagram/src/main/kotlin/com/github/l34130/mise/diagram/MiseFileTaskGraphDataModel.kt | Updated to use MiseTaskResolver with runBlocking in apply block |
| modules/products/diagram/src/main/kotlin/com/github/l34130/mise/diagram/MiseSingleTaskGraphDataModel.kt | Updated to use MiseTaskResolver with runBlocking, added suppression annotation |
| modules/core/src/test/kotlin/com/github/l34130/mise/core/FileTestBase.kt | Removed MiseProjectService refresh call from test setup |
| modules/products/sh/src/test/kotlin/com/github/l34130/mise/sh/FileTestBase.kt | Removed MiseProjectService refresh call from test setup |
| modules/core/src/test/kotlin/com/github/l34130/mise/core/MiseServiceTest.kt | Updated to use MiseTaskResolver, disabled test with TODO comment |
| modules/core/src/test/kotlin/com/github/l34130/mise/core/execution/MiseTomlTaskRunConfigurationProducerTest.kt | Updated ConfigurationContext creation API call to use PsiLocation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| val project = element.project | ||
| val tasks = project.service<MiseProjectService>().getTasks() | ||
| val tasks = runBlocking { project.service<MiseTaskResolver>().getMiseTasks(element.project.baseDirectory()) } |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using runBlocking in a reference provider can freeze the UI during resolve operations, causing editor lag and poor user experience. Consider using cached values or IntelliJ's async reference resolution mechanisms to avoid blocking the UI thread.
| return result | ||
| } | ||
|
|
||
| override fun dispose() {} |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The empty dispose method should either contain cleanup logic or be removed if not needed. If MiseTaskResolver truly has no cleanup to perform, consider documenting why the Disposable interface is implemented without cleanup logic, as this can be confusing for maintainers.
| override fun dispose() {} | |
| override fun dispose() { | |
| // Clear cached tasks when the project/service is disposed. | |
| cache.clear() | |
| } |
| runBlocking { | ||
| project | ||
| .service<MiseTaskResolver>() | ||
| .getMiseTasks(project.baseDirectory()) | ||
| .map { MiseTaskGraphNode(MiseTaskGraphableTaskWrapper(it), provider) } | ||
| } |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using runBlocking in field initialization can block the thread during service construction, potentially causing startup delays or deadlocks. Consider lazy initialization or moving this computation to a suspend function that's called when needed.
| .apply { | ||
| val myTasks = MiseTomlTableTask.resolveAllFromTomlFile(tomlFile.tomlFile) | ||
| val tasks = project.service<MiseProjectService>().getTasks() | ||
| val tasks = runBlocking { project.service<MiseTaskResolver>().getMiseTasks(project.baseDirectory()) } |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using runBlocking in field initialization can block the thread during service construction. This is especially problematic in an apply block where exceptions might not be properly handled. Consider refactoring to use lazy initialization or a suspend function.
| // TODO: fix the test directory | ||
| return |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "TODO: fix the test directory" along with the early return indicates this test is disabled. Either fix the test to work properly with the new MiseTaskResolver implementation or remove it if it's no longer relevant.
| // TODO: fix the test directory | |
| return |
| private fun loadNodes(): List<MiseTaskGraphNode> = | ||
| sequence { | ||
| val tasks = project.service<MiseProjectService>().getTasks() | ||
| @Suppress("RunBlockingInSuspendFunction") |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Suppress annotation "RunBlockingInSuspendFunction" is misleading here. The loadNodes() function is not a suspend function - it returns a sequence. Using runBlocking in a regular function doesn't warrant this suppression. Consider either removing the suppression or refactoring this code to properly use coroutines.
| @Suppress("RunBlockingInSuspendFunction") |
| runBlocking { | ||
| val resolver = project.service<MiseTaskResolver>() | ||
| val tasks = resolver.getMiseTasks(originalFile.parent) | ||
|
|
||
| val psiElement = | ||
| when (task) { | ||
| is MiseShellScriptTask -> task.file.findPsiFile(project)!! | ||
| is MiseTomlTableTask -> task.keySegment | ||
| is MiseUnknownTask -> { | ||
| result.addElement( | ||
| LookupElementBuilder.create(task.name) | ||
| .withInsertHandler(StringLiteralInsertionHandler()) | ||
| .withTypeText(presentablePath(project, task.source)), | ||
| ) | ||
| continue | ||
| } | ||
| } | ||
| for (task in tasks) { | ||
| if (dependsArray?.elements?.any { it.stringValue == task.name } == true) continue | ||
| if (task.name == currentTaskSegment.name) continue | ||
|
|
||
| val path = | ||
| when { | ||
| task is MiseTomlTableTask && task.keySegment.containingFile.virtualFile == originalFile -> "current file" | ||
| else -> presentablePath(project, psiElement.containingFile.viewProvider.virtualFile.path) | ||
| val psiElement = | ||
| when (task) { | ||
| is MiseShellScriptTask -> task.file.findPsiFile(project)!! | ||
| is MiseTomlTableTask -> task.keySegment | ||
| is MiseUnknownTask -> { | ||
| result.addElement( | ||
| LookupElementBuilder.create(task.name) | ||
| .withInsertHandler(StringLiteralInsertionHandler()) | ||
| .withTypeText(presentablePath(project, task.source)), | ||
| ) | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| val path = | ||
| when { | ||
| task is MiseTomlTableTask && task.keySegment.containingFile.virtualFile == originalFile -> "current file" | ||
| else -> presentablePath(project, psiElement.containingFile.viewProvider.virtualFile.path) | ||
| } | ||
|
|
||
| result.addElement( | ||
| LookupElementBuilder | ||
| .createWithSmartPointer(task.name, psiElement) | ||
| .withInsertHandler(StringLiteralInsertionHandler()) | ||
| .withIcon(psiElement.getIcon(Iconable.ICON_FLAG_VISIBILITY)) | ||
| .withTypeText(path) | ||
| .withPriority(-path.length - 100.0), | ||
| ) | ||
| } | ||
| result.addElement( | ||
| LookupElementBuilder | ||
| .createWithSmartPointer(task.name, psiElement) | ||
| .withInsertHandler(StringLiteralInsertionHandler()) | ||
| .withIcon(psiElement.getIcon(Iconable.ICON_FLAG_VISIBILITY)) | ||
| .withTypeText(path) | ||
| .withPriority(-path.length - 100.0), | ||
| ) | ||
| } | ||
| } |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using runBlocking in a completion provider can freeze the UI thread while waiting for async operations to complete. This blocks the editor and creates a poor user experience. Consider using a cached value, returning preliminary results, or using IntelliJ's async completion APIs to avoid blocking.
| return result | ||
| } | ||
|
|
||
| override fun dispose() { } |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The empty dispose method should either contain cleanup logic or be removed if not needed. If MiseConfigFileResolver truly has no cleanup to perform, consider documenting why the Disposable interface is implemented without cleanup logic.
| override fun dispose() { } | |
| override fun dispose() { | |
| // Clear cached configuration files when the project service is disposed. | |
| cache.clear() | |
| } |
| val tasks = service.getTasks() | ||
| tasks.firstOrNull { it is MiseShellScriptTask && it.file == element.virtualFile } as MiseShellScriptTask | ||
| val service = element.project.service<MiseTaskResolver>() | ||
| val tasks = runBlocking { service.getMiseTasks(element.project.baseDirectory()) } |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using runBlocking in a documentation provider can freeze the UI when hovering over elements to view documentation. This creates a poor user experience with noticeable lag. Consider using cached values or returning placeholder documentation while loading asynchronously.
| override suspend fun execute(project: Project) { | ||
| val tomlService = project.service<MiseProjectService>() | ||
|
|
||
| tomlService.refresh() | ||
| } |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MiseStartupActivity class now has an empty execute method, making the entire activity a no-op. If startup initialization is no longer needed, consider removing this class entirely to avoid unnecessary overhead during project startup.
No description provided.