-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: enable slash commands in all modes (fixes #6745) #6746
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
- Modified handleWebviewAskResponse in Task.ts to detect and process slash commands
- Commands starting with "/" are now intercepted and replaced with their content
- Supports command arguments using {{args}} placeholder
- Added tests to verify slash command functionality
- Made handleWebviewAskResponse async to support dynamic imports
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.
I reviewed my own code and found several issues. Classic me.
| } | ||
|
|
||
| handleWebviewAskResponse(askResponse: ClineAskResponse, text?: string, images?: string[]) { | ||
| async handleWebviewAskResponse(askResponse: ClineAskResponse, text?: string, images?: string[]) { |
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.
Critical issue: This method is now async, but the caller setMessageResponse on line 732 doesn't await it. This could lead to race conditions where the slash command processing happens after other operations.
| async handleWebviewAskResponse(askResponse: ClineAskResponse, text?: string, images?: string[]) { | |
| public async setMessageResponse(text: string, images?: string[]) { | |
| await this.handleWebviewAskResponse("messageResponse", text, images) | |
| } |
| vi.clearAllMocks() | ||
| }) | ||
|
|
||
| describe("handleWebviewAskResponse", () => { |
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 don't actually test the implementation. They only mock the commands module and verify concepts without creating a Task instance or calling handleWebviewAskResponse. Could we add integration tests that verify:
- Slash commands are properly detected and processed
- Command arguments are correctly substituted
- Non-existent commands fall back to normal handling
- The async behavior works correctly
| } | ||
| } catch (error) { | ||
| // If there's an error getting the command, fall through to normal handling | ||
| console.debug(`Failed to get command '${commandName}':`, 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.
Is this intentional? When slash command lookup fails, we're only logging at debug level. This might make it hard to diagnose why slash commands aren't working. Consider using console.warn or console.error for better visibility.
| if (text && text.trim().startsWith("/")) { | ||
| const trimmedText = text.trim() | ||
| const spaceIndex = trimmedText.indexOf(" ") | ||
| const commandName = spaceIndex === -1 ? trimmedText.slice(1) : trimmedText.slice(1, spaceIndex) |
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.
Edge case: What happens if someone types just / or / (slash followed by spaces)? This would result in an empty command name. Should we validate that commandName is non-empty before attempting to look it up?
| handleWebviewAskResponse(askResponse: ClineAskResponse, text?: string, images?: string[]) { | ||
| async handleWebviewAskResponse(askResponse: ClineAskResponse, text?: string, images?: string[]) { | ||
| // Check if the text is a slash command | ||
| if (text && text.trim().startsWith("/")) { |
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.
Could we extract this slash command handling logic into a separate method like processSlashCommand(text: string): Promise<{isCommand: boolean, content?: string}> for better readability and testability? The current method is getting quite long.
|
Closing for now, not enough info |
This PR fixes issue #6745 where slash commands were not working correctly in Orchestrator and Code modes.
Problem
Slash commands (like
/commit) were being treated as regular text messages in non-Ask modes, causing Roo to attempt to implement the slash command functionality instead of executing it.Solution
Modified the
handleWebviewAskResponsemethod inTask.tsto:/getCommandfunction{{args}}placeholderChanges
handleWebviewAskResponseasync to support dynamic importsTesting
Fixes #6745
Important
Fixes slash command handling in
Task.tsby detecting and processing commands, supporting arguments, and adding tests.handleWebviewAskResponseinTask.tsby detecting commands starting with/, parsing them, and replacing with command content.{{args}}placeholder.handleWebviewAskResponseasync to support dynamic imports.webviewMessageHandler.tsto await the async method.Task.test.tsfor slash command detection, argument handling, and non-existent command handling.This description was created by
for d2725fe. You can customize this summary. It will automatically update as commits are pushed.