Revert "feat: enable loading Roo modes from multiple files in .roo/roo_modes directory" #7332
Conversation
…o_modes …" This reverts commit faece9e.
dosubot
bot
added
size:XL
There was a problem hiding this comment.
Pull Request Overview
This PR reverts a feature that enabled loading Roo modes from multiple files in the .roo/roo_modes directory. The revert is temporary due to breaking existing modes import functionality.
- Removes support for loading modes from
.roo/modesdirectory files - Reverts to simpler precedence model with only settings and
.roomodesfiles - Removes associated test cases and directory watching functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/core/config/tests/CustomModesManager.spec.ts | Removes test cases for .roo/modes directory functionality and updates precedence expectations |
| src/core/config/CustomModesManager.ts | Removes directory-based mode loading, simplifies file watching, and reverts to two-file precedence model |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| sourceFile: filePath, | ||
| })) | ||
| // Add source to each mode | ||
| return result.data.customModes.map((mode) => ({ ...mode, source })) |
There was a problem hiding this comment.
The removal of the sourceFile property may break functionality that depends on tracking which file a mode originated from. This could impact mode updates and deletions that need to modify the correct source file.
| return result.data.customModes.map((mode) => ({ ...mode, source })) | |
| // Add source and sourceFile to each mode | |
| return result.data.customModes.map((mode) => ({ ...mode, source, sourceFile: filePath })) |
| const existingMode = existingModes.find((m) => m.slug === slug) | ||
|
|
||
| const isProjectMode = config.source === "project" | ||
| let targetPath: string |
There was a problem hiding this comment.
The logic for determining the target file for mode updates has been simplified but may not correctly handle existing modes. Without the sourceFile property, the system cannot track where an existing mode was originally loaded from, potentially causing modes to be written to the wrong file.
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for the PR! I've reviewed the revert and have some concerns about the approach. While I understand there's a breaking issue with the modes import functionality, a full revert may impact users who have already adopted the multi-file feature. I've left some suggestions inline to help improve the implementation.
| sourceFile: filePath, | ||
| })) | ||
| // Add source to each mode | ||
| return result.data.customModes.map((mode) => ({ ...mode, source })) |
There was a problem hiding this comment.
I agree with Copilot's concern here. Removing the sourceFile property could break functionality that depends on tracking which file a mode originated from. This is particularly important for the update and delete operations.
Is this removal necessary for fixing the breaking issue, or could we preserve this property while addressing the specific problem?
| const existingMode = existingModes.find((m) => m.slug === slug) | ||
|
|
||
| const isProjectMode = config.source === "project" | ||
| let targetPath: string |
There was a problem hiding this comment.
The simplified logic here may cause modes to be written to the wrong file. Without tracking the original source file, when a user updates a mode that was loaded from .roomodes, it might incorrectly be saved to the global settings file or vice versa.
Could we at least preserve the logic to check where the mode originally came from before deciding where to save updates?
| for (const mode of settingsModes) { | ||
| modesMap.set(mode.slug, mode) | ||
| } | ||
| const roomodesModes = roomodesPath ? await this.loadModesFromFile(roomodesPath) : [] |
There was a problem hiding this comment.
Since we're removing support for .roo/modes directories, should we add a warning or migration notice for users who might have already created these directories? They'll suddenly find their modes aren't being loaded without any explanation.
Consider adding a check for these directories and logging a deprecation warning.
Additional Review CommentsMissing Context About the Breaking Issue:
This makes it difficult to verify if a full revert is the best solution. Could you provide more context about the specific breaking issue? This would help determine if a targeted fix might be more appropriate than a complete revert. Alternative Approaches to Consider:
Timeline and Tracking:
This would help set expectations for users who were relying on this feature per issue #7202. |
- Add support for loading modes from .roo/modes directory (both global and project) - Implement proper precedence: project .roo/modes > .roomodes > global .roo/modes > settings - Update file watchers to monitor .roo/modes directories - Preserve original source file when updating modes - Add comprehensive tests for the new functionality - Support both .yaml and .yml file extensions This re-implements the feature that was reverted in #7332, addressing the issue reported by @farazoman where YAML files in $HOME/.roo/modes were not loading. Fixes #7202
Reverts #7203 temporarily as it breaks existing modes import functionality.
Important
Reverts loading modes from multiple files in
.roo/roo_modesdirectory, restoring original mode loading behavior inCustomModesManager.ts..roo/roo_modesdirectory inCustomModesManager.ts.loadModesFromDirectory()and related logic..roomodesand settings files..roo/modesdirectory inCustomModesManager.spec.ts.This description was created by
for 6266e46. You can customize this summary. It will automatically update as commits are pushed.