-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: slash commands stored in correct workspace folder in multi-root workspaces #6701
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
…workspaces - Added findWorkspaceWithRoo() utility to locate the workspace containing .roo directory - Updated webviewMessageHandler to use the new utility instead of hardcoded first workspace - This ensures slash commands are created in the correct .roo/commands directory Fixes #6700
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.
Reviewed my own code. Found it suspiciously working on the first try. Must be a bug in the bug fix.
| // Check each workspace folder for a .roo directory | ||
| for (const folder of workspaceFolders) { | ||
| const rooPath = path.join(folder.uri.fsPath, ".roo") | ||
| if (await fileExistsAtPath(rooPath)) { |
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 function works well for the intended use case, but I noticed it doesn't handle potential errors from fileExistsAtPath(). If the filesystem check throws an error (e.g., permission issues), it would bubble up rather than falling back gracefully. Consider wrapping the check in a try-catch:
| if (await fileExistsAtPath(rooPath)) { | |
| for (const folder of workspaceFolders) { | |
| const rooPath = path.join(folder.uri.fsPath, ".roo") | |
| try { | |
| if (await fileExistsAtPath(rooPath)) { | |
| return folder | |
| } | |
| } catch (error) { | |
| // Log error but continue checking other folders | |
| console.warn(`Failed to check .roo in ${folder.uri.fsPath}:`, error) | |
| } | |
| } |
| }, | ||
| })) | ||
|
|
||
| import { arePathsEqual, getReadablePath, getWorkspacePath, findWorkspaceWithRoo } from "../path" |
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 understand the vitest setup constraints mentioned in the PR description, but could we consider adding at least some basic test coverage for findWorkspaceWithRoo()? Even if we can't test the actual filesystem operations, we could test the logic with mocked fileExistsAtPath responses. This is a critical function that determines where commands are stored.
| * | ||
| * @returns The workspace folder containing .roo, or undefined if none found | ||
| */ | ||
| export async function findWorkspaceWithRoo(): Promise<vscode.WorkspaceFolder | undefined> { |
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.
For workspaces with many folders, this function performs filesystem checks on every command creation. Have you considered caching the result and invalidating it when workspace folders change? This could improve performance in large multi-root workspaces:
// At module level
let cachedWorkspaceWithRoo: vscode.WorkspaceFolder | undefined;
let cachedWorkspaceFoldersHash: string | undefined;
// In the function, check if cache is valid
const currentHash = JSON.stringify(workspaceFolders.map(f => f.uri.fsPath));
if (cachedWorkspaceFoldersHash === currentHash && cachedWorkspaceWithRoo) {
return cachedWorkspaceWithRoo;
}|
See my comment on the issue, #6700 (comment). It appears the issue is more general than just slash commands. Roo generally does not identify the |
|
Closing for now, this ultimately will still select the first workspace that contains a |
This PR fixes issue #6700 where slash commands were being stored in the wrong folder in multi-root workspaces.
Problem
In multi-root workspaces, slash commands were always being created in the
.roo/commandsdirectory of the first workspace folder, regardless of which workspace actually contained the.roodirectory.Solution
findWorkspaceWithRoo()utility function that searches through all workspace folders to find the one containing a.roodirectorywebviewMessageHandler.tsto use this new utility instead of hardcoding the first workspace folder.roodirectory is found in any workspaceTesting
findWorkspaceWithRoocould not be added due to vitest setup constraints)Fixes #6700
Important
Fixes slash command storage in multi-root workspaces by identifying the correct workspace folder containing
.roo.findWorkspaceWithRoo()inpath.tsto locate the workspace containing.roo.webviewMessageHandler.tsto usefindWorkspaceWithRoo()for command storage..roodirectory is found.path.spec.ts.This description was created by
for 821812f. You can customize this summary. It will automatically update as commits are pushed.