-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add built-in slash commands #6793
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
Conversation
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.
Thank you for your contribution! I've reviewed the changes and found critical issues that need attention before this can be merged.
src/assets/built-in-commands/init.md
Outdated
| description: "Set up rules for a project" | ||
| --- | ||
|
|
||
| TBD |
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.
This file only contains 'TBD' but the tests expect actual content. The tests are checking for strings like 'Initialize Roo Project', '.roo/rules/', 'What this command does:', 'Recommended starter rules:', etc. Could you please implement the actual content for this command?
The test expects sections like:
- Initialize Roo Project
- What this command does
- Recommended starter rules
- Getting Started
- Example rule file structure
Without this content, all the tests will fail.
| description: "Create a custom mode" | ||
| --- | ||
|
|
||
| TBD |
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.
This file only contains 'TBD' but the tests expect comprehensive content. The tests are checking for 'Create a Custom Mode', 'What are Modes?', 'Mode Configuration', 'YAML', 'instructions', 'file_restrictions', 'tools', etc.
The test expects sections like:
- Create a Custom Mode
- What are Modes?
- Mode Configuration
- Mode Properties
- Creating Your Mode
- Mode Examples
- Best Practices
Could you implement the actual content to match what the tests expect?
| const commands = await getBuiltInCommands() | ||
|
|
||
| const initCommand = commands.find((cmd) => cmd.name === "init") | ||
| expect(initCommand).toBeDefined() |
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.
These tests will fail because they expect content that doesn't exist in the actual command files. The test expectations don't match the current 'TBD' placeholders. Should these tests be marked as pending/skipped until the actual content is implemented?
| // In development: __dirname = /path/to/src/services/command | ||
| // In compiled: __dirname = /path/to/src/dist/services/command | ||
|
|
||
| if (__dirname.includes("/dist/")) { |
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.
Is using __dirname.includes("/dist/") the most reliable way to determine the environment? This could be fragile if the build structure changes. Consider using a more robust approach like checking for a specific marker file or using an environment variable.
| } | ||
| } catch (error) { | ||
| // Directory doesn't exist or can't be read - this is fine, just return empty array | ||
| console.warn("Built-in commands directory not found or not readable:", error) |
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's an inconsistency in error handling - here you use console.warn but similar errors in commands.ts are silently ignored. Should we have a consistent error handling strategy across both files?
| results.push({ | ||
| type: ContextMenuOptionType.SectionHeader, | ||
| label: "Custom Commands", | ||
| label: "Commands", |
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 label change from 'Custom Commands' to 'Commands' might be confusing for users who have existing custom commands. Would 'Commands (Built-in & Custom)' be clearer to indicate both types are shown?
8abe61a to
c97e3dd
Compare
No description provided.