Migrate mode definitions from JSON to YAML #2342
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Collaborator
Author
|
@ellipsis-agent review this |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 5568768 in 2 minutes and 19 seconds
More details
- Looked at
660lines of code in6files - Skipped
1files when reviewing. - Skipped posting
13drafted comments based on config settings.
1. package.json:422
- Draft comment:
Ensure that adding 'js-yaml' and its types is intentional and consistent with our ESLint/Prettier rules (see our Development Standards). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold70%
The comment is asking the PR author to ensure that adding 'js-yaml' and its types is intentional and consistent with ESLint/Prettier rules. This is a violation of the rule against asking the author to confirm their intention or ensure consistency. The comment does not provide a specific suggestion or point out a specific issue, making it not useful.
2. src/core/config/CustomModesManager.ts:367
- Draft comment:
Consider extracting the dual write (YAML and legacy JSON) in updateCustomMode into a helper method for better modularity. - Reason this comment was not posted:
Confidence changes required:70%<= threshold70%
None
3. src/core/config/CustomModesManager.ts:321
- Draft comment:
The merging logic uses a reverse precedence mapping from multiple sources. Ensure that this behavior is well documented and tested as it’s key to determining mode source. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold70%
This comment is asking the PR author to ensure that the behavior is documented and tested, which violates the rule against asking for confirmation or testing. It doesn't provide a specific suggestion or point out a specific issue with the code.
4. src/core/config/CustomModesManager.ts:468
- Draft comment:
In deleteCustomMode, review the file deletion logic from both YAML and legacy JSON formats carefully to ensure no mode is left undeleted. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold70%
This comment is asking the PR author to review the file deletion logic to ensure completeness, which violates the rule against asking the author to double-check things. It doesn't provide a specific suggestion or point out a clear issue.
5. src/utils/yaml.ts:11
- Draft comment:
Returning a default value on YAML parse errors might hide issues. Consider logging more details or handling errors explicitly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 70%
The comment raises a valid concern about error handling, but the code's behavior is intentional and documented. The function already logs errors. The pattern of returning a default value is a common and valid approach for configuration file handling. The comment doesn't provide clear actionable feedback on what should be done differently.
The comment might have a point about error visibility - in production, console.error logs could be missed. Perhaps throwing or using structured logging would be better.
The function's purpose appears to be graceful handling of missing/invalid config files, which is a valid use case. The error is logged, and the behavior is clearly documented.
Delete the comment. The code's error handling approach is intentional, documented, and reasonable for its purpose.
6. src/utils/modes.ts:79
- Draft comment:
In saveModeToYaml, destructuring out 'source' (and any rules) is good; ensure that future changes in ModeConfig are also reflected here to prevent missing data. - Reason this comment was not posted:
Confidence changes required:60%<= threshold70%
None
7. package.json:422
- Draft comment:
New dependencies '@types/js-yaml' and 'js-yaml' have been added. Ensure ESLint/Prettier configurations cover YAML files for consistent code style. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%
None
8. src/core/config/CustomModesManager.ts:24
- Draft comment:
Avoid performing async operations in the constructor. Consider using an async initializer or factory method for improved testability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. src/core/config/CustomModesManager.ts:76
- Draft comment:
The 'rules' file is read but its content is not used. If excluding rules in the YAML output is intentional, consider clarifying or removing the unused variable. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 70%
The code is part of a migration function that's moving modes from JSON to YAML format. The comment on line 83 explicitly states "don't include rules in the YAML file since we're keeping separate rules files". This suggests the rules are intentionally read but not used, as they're being kept in their original separate files rather than migrated. The unused variable is actually documenting the intentional behavior.
The code could be clearer by either removing the unused read operation entirely or adding a comment explaining why the rules are read but not used.
While the code could be clearer, the existing comment on line 83 does explain the intention. The rules read operation may also serve as validation that the rules file exists and is readable.
The comment raises a valid point about code clarity, but the behavior appears intentional and is documented. The suggestion to remove the code would actually remove useful validation.
10. src/core/config/CustomModesManager.ts:1
- Draft comment:
Ensure comprehensive unit tests are added for the new YAML migration and merging functionality to verify correct precedence and error handling. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. package.json:56
- Draft comment:
There appears to be an inconsistent naming usage: the commands and package name use 'roo-cline' (e.g., 'roo-cline.plusButtonClicked') while the submenu id is 'roo-code.contextMenu'. To keep things clear, please consider aligning the naming (choosing either 'roo-cline' or 'roo-code' consistently). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. src/shared/globalFileNames.ts:16
- Draft comment:
Typo: The enum member 'LEGACY_ROOMODES' (and corresponding string value '.roomodes') appears to have a typo. If this is intended to represent legacy mode configurations, consider renaming it to something like 'LEGACY_ROO_MODES' or 'LEGACY_MODES' to avoid confusion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 70%
The enum member LEGACY_ROOMODES refers to a specific file path ".roomodes". This is likely an intentional name that matches an existing file structure. Suggesting a rename could actually break functionality if this path is used in the codebase. The comment assumes it's a typo without strong evidence, and changing file paths is risky without understanding the full system context.
Maybe the file path really is incorrect and needs to be changed for consistency with the new .roo/modes structure?
Since this is explicitly marked as LEGACY in the enum name, it's likely intentionally preserving an old file path for backward compatibility. The new path format is clearly shown in MODES_DIRECTORY.
Delete the comment. The enum member likely correctly references an existing legacy file path, and suggesting a rename without strong evidence could cause breaking changes.
13. src/utils/fs.ts:37
- Draft comment:
The parameter name in the JSDoc for fileExistsAtPath is inconsistent. The docblock uses '@param path' but the actual parameter is 'filePath'. Please update the JSDoc to use '@param filePath' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_ltlwsEd3MFG0KgTr
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| private readonly context: vscode.ExtensionContext, | ||
| private readonly onUpdate: () => Promise<void>, | ||
| ) { | ||
| // TODO: We really shouldn't have async methods in the constructor. |
Contributor
There was a problem hiding this comment.
Avoid async operations in the constructor. Consider moving migration logic to an initialization method that’s explicitly called after construction.
| // Create maps to store modes by source | ||
| const projectModes = new Map<string, ModeConfig>() | ||
| const globalModes = new Map<string, ModeConfig>() | ||
| // Create a map to merge modes with correct precedence |
Contributor
There was a problem hiding this comment.
Merging logic for modes is duplicated in getCustomModes and refreshMergedState. Refactor this into a shared helper to reduce code duplication and ease maintenance.
| ) | ||
| } | ||
| } catch (error) { | ||
| console.error("Error during workspace mode migration:", error) |
Contributor
There was a problem hiding this comment.
Instead of using console.error in migration methods, use the centralized logger (logger.error) for consistent error logging.
Suggested change
| console.error("Error during workspace mode migration:", error) | |
| logger.error("Error during workspace mode migration:", error) |
| } | ||
| } | ||
| } catch (yamlError) { | ||
| console.error("Error clearing global YAML modes:", yamlError) |
Contributor
There was a problem hiding this comment.
For error handling when clearing YAML modes, consider using the centralized logger instead of console.error for better consistency.
Suggested change
| console.error("Error clearing global YAML modes:", yamlError) | |
| logger.error("Error clearing global YAML modes:", yamlError) |
|
I see there is automatic migration for global modes, but project modes get left behind. I would suggest prompting the user to migrate their modes, offering an automatic migration using the same code you're using for global -- and warning of "future deprecation of JSON support" -- and until deprecating support, try to load both JSON and YAML variants from projects. |
|
I'm a dunce, "migrateWorkspaceModes" works on project modes. The difference in terminology tripped me up. |
Collaborator
|
ZZzzzz :P I need this @mrubens, I think this change will increase the pace people are experimenting with custom modes. Right now when you use AI to tweak your custom mode there is no good way to see the changes using the git dif view and so I personally don't tweak modes as often as I would like. and when I do I don't manually review them as much as I should. 
|
|
@hannesrudolph try using Word Wrap. Code_uwNBHHspcZ.mp4Still, YAML would be a lot better. |
Collaborator
Oh yes totally I made word wrap default after that screenshot was taken. Thank you |
Collaborator
Author
hannesrudolph
moved this from PR [Pre Approval Review]
to PR [Draft/WIP]
in Roo Code Roadmap
2 tasks
Member
|
Seems completed on #3711 |
github-project-automation
bot
moved this from PR [Draft/WIP]
to Done
in Roo Code Roadmap
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
I'm really regretting using JSON for the mode definitions. It's hard for AI to create the format, and hard for humans to read and edit it. I want to experiment with a YAML-based format instead that makes it easier to use multi-line strings for role definitions and custom instructions.
Implementation
The bulk of the challenge here is around how to implement the migration - not 100% solid yet.
But at a high level, I want to create a new
.roo/modes/folder and have one file for each mode in there.This PR is rough and not well tested, but wanted to put it out there as a draft.
Important
Migrates mode definitions from JSON to YAML format, adding utilities for YAML handling and ensuring backward compatibility with existing JSON modes.
CustomModesManager.ts: AddsmigrateToYaml(),migrateWorkspaceModes(), andmigrateGlobalModes()to automatically convert JSON mode definitions to YAML.loadAllModesFromYaml()andsaveModeToYaml()frommodes.tsto handle YAML operations.modes.ts: Introduces functions likegetModesDirectory(),getModeYamlPath(),loadModeFromYaml(), andsaveModeToYaml()for managing YAML files.yaml.ts: AddsreadYamlFile()andwriteYamlFile()for reading and writing YAML data.CustomModesManager.ts.getCustomModes(),updateCustomMode(), anddeleteCustomMode()to handle both YAML and JSON formats.js-yamltopackage.jsonfor YAML parsing.ModeFileLocationsinglobalFileNames.tsto include YAML directory paths.This description was created by
for 5568768. It will automatically update as commits are pushed.