Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: slash commands stored in correct workspace folder in multi-root workspaces #6701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/core/webview/webviewMessageHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import { getOpenAiModels } from "../../api/providers/openai"
import { getVsCodeLmModels } from "../../api/providers/vscode-lm"
import { openMention } from "../mentions"
import { TelemetrySetting } from "../../shared/TelemetrySetting"
import { getWorkspacePath } from "../../utils/path"
import { getWorkspacePath, findWorkspaceWithRoo } from "../../utils/path"
import { ensureSettingsDirectoryExists } from "../../utils/globalContext"
import { Mode, defaultModeSlug } from "../../shared/modes"
import { getModels, flushModels } from "../../api/providers/fetchers/modelCache"
Expand Down Expand Up @@ -2477,13 +2477,13 @@ export const webviewMessageHandler = async (
const globalConfigDir = path.join(os.homedir(), ".roo")
commandsDir = path.join(globalConfigDir, "commands")
} else {
// Project commands
const workspaceRoot = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath
if (!workspaceRoot) {
// Project commands - find the workspace with .roo directory
const workspaceWithRoo = await findWorkspaceWithRoo()
if (!workspaceWithRoo) {
vscode.window.showErrorMessage(t("common:errors.no_workspace_for_project_command"))
break
}
commandsDir = path.join(workspaceRoot, ".roo", "commands")
commandsDir = path.join(workspaceWithRoo.uri.fsPath, ".roo", "commands")
}

// Ensure the commands directory exists
Expand Down
45 changes: 34 additions & 11 deletions src/utils/__tests__/path.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,18 @@
import os from "os"
import * as path from "path"

import { arePathsEqual, getReadablePath, getWorkspacePath } from "../path"
// Use vi.hoisted to ensure mocks are available during module loading
const { mockWorkspaceFolders, mockGetWorkspaceFolder, mockFileExistsAtPath } = vi.hoisted(() => {
const mockWorkspaceFolders = vi.fn()
const mockGetWorkspaceFolder = vi.fn()
const mockFileExistsAtPath = vi.fn()
return { mockWorkspaceFolders, mockGetWorkspaceFolder, mockFileExistsAtPath }
})

// Mock modules
// Mock modules before imports
vi.mock("../fs", () => ({
fileExistsAtPath: mockFileExistsAtPath,
}))

vi.mock("vscode", () => ({
window: {
Expand All @@ -16,23 +25,37 @@ vi.mock("vscode", () => ({
},
},
workspace: {
workspaceFolders: [
get workspaceFolders() {
return mockWorkspaceFolders()
},
getWorkspaceFolder: mockGetWorkspaceFolder,
},
}))

import { arePathsEqual, getReadablePath, getWorkspacePath, findWorkspaceWithRoo } from "../path"
Copy link
Contributor Author

@roomote roomote bot Aug 5, 2025

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.

import { fileExistsAtPath } from "../fs"

describe("Path Utilities", () => {
const originalPlatform = process.platform
// Helper to mock VS Code configuration

beforeEach(() => {
// Reset mocks before each test
vi.clearAllMocks()
// Set default workspace folders
mockWorkspaceFolders.mockReturnValue([
{
uri: { fsPath: "/test/workspace" },
name: "test",
index: 0,
},
],
getWorkspaceFolder: vi.fn().mockReturnValue({
])
mockGetWorkspaceFolder.mockReturnValue({
uri: {
fsPath: "/test/workspaceFolder",
},
}),
},
}))
describe("Path Utilities", () => {
const originalPlatform = process.platform
// Helper to mock VS Code configuration
})
})

afterEach(() => {
Object.defineProperty(process, "platform", {
Expand Down
31 changes: 31 additions & 0 deletions src/utils/path.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as path from "path"
import os from "os"
import * as vscode from "vscode"
import { fileExistsAtPath } from "./fs"

/*
The Node.js 'path' module resolves and normalizes paths differently depending on the platform:
Expand Down Expand Up @@ -130,3 +131,33 @@ export const getWorkspacePathForContext = (contextPath?: string): string => {
// Fall back to current behavior
return getWorkspacePath()
}

/**
* Finds the workspace folder that contains a .roo directory.
* In multi-root workspaces, this ensures we find the correct workspace
* rather than just using the first one.
*
* @returns The workspace folder containing .roo, or undefined if none found
*/
export async function findWorkspaceWithRoo(): Promise<vscode.WorkspaceFolder | undefined> {
Copy link
Contributor Author

@roomote roomote bot Aug 5, 2025

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;
}

const workspaceFolders = vscode.workspace.workspaceFolders
if (!workspaceFolders || workspaceFolders.length === 0) {
return undefined
}

// If there's only one workspace folder, return it
if (workspaceFolders.length === 1) {
return workspaceFolders[0]
}

// 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)) {
Copy link
Contributor Author

@roomote roomote bot Aug 5, 2025

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:

Suggested change
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)
}
}

return folder
}
}

// If no .roo directory found in any workspace, return the first one as fallback
return workspaceFolders[0]
}
Loading