-
Couldn't load subscription status.
- Fork 2.4k
fix: identify mcp and slash command config path in multiple folder workspace #6904
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,6 @@ import { isBinaryFile } from "isbinaryfile" | |
| import { mentionRegexGlobal, commandRegexGlobal, unescapeSpaces } from "../../shared/context-mentions" | ||
|
|
||
| import { getCommitInfo, getWorkingState } from "../../utils/git" | ||
| import { getWorkspacePath } from "../../utils/path" | ||
|
|
||
| import { openFile } from "../../integrations/misc/open-file" | ||
| import { extractTextFromFile } from "../../integrations/misc/extract-text" | ||
|
|
@@ -49,16 +48,11 @@ function getUrlErrorMessage(error: unknown): string { | |
| return t("common:errors.url_fetch_failed", { error: errorMessage }) | ||
| } | ||
|
|
||
| export async function openMention(mention?: string): Promise<void> { | ||
| export async function openMention(cwd: string, mention?: string): Promise<void> { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good change to pass cwd as a parameter instead of calling getWorkspacePath() directly. This makes the function more testable and explicit about its dependencies. Just wondering - what happens if cwd is an empty string here? Should we validate it's not empty before proceeding? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This issue is no different than any other; it all comes down to the source of cwd . If cwd is an empty string, then getWorkspacePath , the root cause, must also return an empty string. Ultimately, I simply fixed getWorkspacePath at the moment the user clicks the plus button and tried to use it across all functions. |
||
| if (!mention) { | ||
| return | ||
| } | ||
|
|
||
| const cwd = getWorkspacePath() | ||
| if (!cwd) { | ||
| return | ||
| } | ||
|
|
||
| if (mention.startsWith("/")) { | ||
| // Slice off the leading slash and unescape any spaces in the path | ||
| const relPath = unescapeSpaces(mention.slice(1)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -138,6 +138,7 @@ export interface TaskOptions extends CreateTaskOptions { | |
| taskNumber?: number | ||
| onCreated?: (task: Task) => void | ||
| initialTodos?: TodoItem[] | ||
| workspacePath?: string | ||
| } | ||
|
|
||
| export class Task extends EventEmitter<TaskEvents> implements TaskLike { | ||
|
|
@@ -313,6 +314,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| taskNumber = -1, | ||
| onCreated, | ||
| initialTodos, | ||
| workspacePath, | ||
| }: TaskOptions) { | ||
| super() | ||
|
|
||
|
|
@@ -333,7 +335,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| // Normal use-case is usually retry similar history task with new workspace. | ||
| this.workspacePath = parentTask | ||
| ? parentTask.workspacePath | ||
| : getWorkspacePath(path.join(os.homedir(), "Desktop")) | ||
| : (workspacePath ?? getWorkspacePath(path.join(os.homedir(), "Desktop"))) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that if workspacePath is an empty string (which can happen from the provider), it gets stored as-is because empty string isn't nullish. This bypasses the Desktop fallback. Was this intentional? It seems like we'd want to use the Desktop fallback when there's no valid workspace path. Maybe consider: workspacePath && workspacePath.trim() ? workspacePath : getWorkspacePath(...) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current behavior is the same as the main branch. Previously, when a user created a task without opening any folders, path.join(os.homedir(), "Desktop") would be used as the workspace. This was completely meaningless because other functions often bypassed the Task's cwd and used getWorkspacePath to obtain the cwd. Now, I prioritize the Task's cwd, followed by the Provider's cwd, and finally using getWorkspacePath, which actually changes the original behavior. Previously, when using getWorkspacePath to obtain the workspace path, an empty string would be returned because the user had no folders open, even if the Task's cwd was path.join(os.homedir(), "Desktop") . However, after my changes, if I didn't do this, many functions would assume that the workspacePath existed, allowing users to perform slash commands and MCP project settings without opening a folder, which was undoubtedly confusing. At the same time, the code index would not be initialized because it couldn't detect any folders. |
||
|
|
||
| this.instanceId = crypto.randomUUID().slice(0, 8) | ||
| this.taskNumber = -1 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,13 +112,14 @@ export class ClineProvider | |
| private view?: vscode.WebviewView | vscode.WebviewPanel | ||
| private clineStack: Task[] = [] | ||
| private codeIndexStatusSubscription?: vscode.Disposable | ||
| private currentWorkspaceManager?: CodeIndexManager | ||
| private codeIndexManager?: CodeIndexManager | ||
| private _workspaceTracker?: WorkspaceTracker // workSpaceTracker read-only for access outside this class | ||
| protected mcpHub?: McpHub // Change from private to protected | ||
| private marketplaceManager: MarketplaceManager | ||
| private mdmService?: MdmService | ||
| private taskCreationCallback: (task: Task) => void | ||
| private taskEventListeners: WeakMap<Task, Array<() => void>> = new WeakMap() | ||
| private currentWorkspacePath: string | undefined | ||
|
|
||
| private recentTasksCache?: string[] | ||
|
|
||
|
|
@@ -136,6 +137,7 @@ export class ClineProvider | |
| mdmService?: MdmService, | ||
| ) { | ||
| super() | ||
| this.currentWorkspacePath = getWorkspacePath() | ||
|
|
||
| ClineProvider.activeInstances.add(this) | ||
|
|
||
|
|
@@ -707,7 +709,7 @@ export class ClineProvider | |
| this.log("Clearing webview resources for sidebar view") | ||
| this.clearWebviewResources() | ||
| // Reset current workspace manager reference when view is disposed | ||
| this.currentWorkspaceManager = undefined | ||
| this.codeIndexManager = undefined | ||
| } | ||
| }, | ||
| null, | ||
|
|
@@ -795,6 +797,7 @@ export class ClineProvider | |
| rootTask: historyItem.rootTask, | ||
| parentTask: historyItem.parentTask, | ||
| taskNumber: historyItem.number, | ||
| workspacePath: historyItem.workspace, | ||
| onCreated: this.taskCreationCallback, | ||
| enableBridge: BridgeOrchestrator.isEnabled(cloudUserInfo, remoteControlEnabled), | ||
| }) | ||
|
|
@@ -1434,6 +1437,11 @@ export class ClineProvider | |
| await this.postStateToWebview() | ||
| } | ||
|
|
||
| async refreshWorkspace() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The refreshWorkspace method updates currentWorkspacePath but doesn't seem to notify anything about the change. Should this also post a message to the webview or update the state somehow? Also, what happens if getWorkspacePath() throws here? There's no error handling. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, no processing is needed here, the behavior here is the same as before. It's just that before, the workspacePath (the directory where the current active file is located) was dynamically obtained through the provider's cwd method, but now it refreshes the provider's currentWorkspacePath first and then pushes it to the webview.postStateToWebview is called after refreshWorkspace. Maybe I can combine them into one method |
||
| this.currentWorkspacePath = getWorkspacePath() | ||
| await this.postStateToWebview() | ||
| } | ||
|
|
||
| async postStateToWebview() { | ||
| const state = await this.getStateToPostToWebview() | ||
| this.postMessageToWebview({ type: "state", state }) | ||
|
|
@@ -2159,7 +2167,7 @@ export class ClineProvider | |
| const currentManager = this.getCurrentWorkspaceCodeIndexManager() | ||
|
|
||
| // If the manager hasn't changed, no need to update subscription | ||
| if (currentManager === this.currentWorkspaceManager) { | ||
| if (currentManager === this.codeIndexManager) { | ||
| return | ||
| } | ||
|
|
||
|
|
@@ -2170,7 +2178,7 @@ export class ClineProvider | |
| } | ||
|
|
||
| // Update the current workspace manager reference | ||
| this.currentWorkspaceManager = currentManager | ||
| this.codeIndexManager = currentManager | ||
|
|
||
| // Subscribe to the new manager's progress updates if it exists | ||
| if (currentManager) { | ||
|
|
@@ -2531,7 +2539,7 @@ export class ClineProvider | |
| } | ||
|
|
||
| public get cwd() { | ||
| return getWorkspacePath() | ||
| return this.currentWorkspacePath || getWorkspacePath() | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ class WorkspaceTracker { | |
| private resetTimer: NodeJS.Timeout | null = null | ||
|
|
||
| get cwd() { | ||
| return getWorkspacePath() | ||
| return this.providerRef?.deref()?.cwd ?? getWorkspacePath() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting change to use the provider's cwd when available. This creates a dependency cycle though - WorkspaceTracker uses provider.cwd, and provider might use WorkspaceTracker. Is this intentional? Could this cause any issues with initialization order? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My answer is no. But this question is certainly interesting, and it will become even more useful later. Currently, I've discovered that its only purpose is to push workspaceUpdated every 300ms and return the file list corresponding to the cwd. workspaceUpdated is used in two places: first, as the file list carried in the context sent with the user message, and second, as an index (I used it in previous multi-workspace support, but it's no longer used, and I'll refactor the code later). The purpose of this change is to ensure that the cwd is consistent with the provider. Otherwise, if the user's active folder changes, the file list will be refreshed to the other folder. This will cause the file list carried in the context sent by the user to be inconsistent with the cwd used in the message, so this change is necessary. |
||
| } | ||
| constructor(provider: ClineProvider) { | ||
| this.providerRef = new WeakRef(provider) | ||
|
|
||
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 see we're calling refreshWorkspace() here after removing from the stack. What's the purpose of refreshing the workspace at this point? Is it to pick up the active editor's workspace folder?
Also wondering - if getWorkspacePath() returns an empty string during this refresh (no editor open), would that cause issues for the new task that's about to be created?
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 and the cmd property of the provider are the core modifications. The purpose is to switch the workspacePath only when the user clicks the plus sign, and to make all file-related operations rely on the Task's cwd first, and then the provider's cwd. In this way, when working in a multi-folder workspace, whether it is indexing, slash command or other actions, the user must switch the workspacePath by clicking the plus sign.
As for getting an empty string, this is not a problem.
getWorkspacePath basically cannot return an empty string. It first gets the workspacePath where the currently opened file is located, then the first folder of the multi-root workspace, and finally the empty string as the default value. When it is an empty string, it is only when the user does not open any directory. At this time, RooCode can be used to initiate tasks and conduct simple questions and answers, but it actually has no effect. All activities related to workspacePath are prohibited.
In fact, the source of all the confusion lies in the abuse of getWorkspacePath. The current main uses getWorkspacePath and vscode.workspace.workspaceFolders[0] extensively, which results in different functions sometimes forcing the first directory to be obtained, and sometimes based on the current active file directory. However, the cwd of the webview may be the one left before it switched the active file, which often leads to the inconsistency between the ui in the root workspace and the cwd of different background jobs, causing confusion.