-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: respect customStoragePath setting for custom modes #7155
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
- Update CustomModesManager to use getSettingsDirectoryPath from storage.ts - Update globalContext functions to use storage utilities that respect customStoragePath - Ensures custom_modes.yaml is found when using customStoragePath setting Fixes #7154
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 bugs. Classic Tuesday.
@@ -246,7 +246,7 @@ export class CustomModesManager { | |||
} | |||
|
|||
public async getCustomModesFilePath(): Promise<string> { | |||
const settingsDir = await ensureSettingsDirectoryExists(this.context) | |||
const settingsDir = await getSettingsDirectoryPath(this.context.globalStorageUri.fsPath) |
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? You're passing context.globalStorageUri.fsPath
to getSettingsDirectoryPath()
, but that function expects the default storage path and internally calls getStorageBasePath()
to apply custom path logic. This could cause the custom path resolution to be applied twice, potentially breaking the path resolution.
Should this instead just pass the context or ensure the function signature matches the expected input?
const settingsDir = join(context.globalStorageUri.fsPath, "settings") | ||
await mkdir(settingsDir, { recursive: true }) | ||
return settingsDir | ||
return getSettingsDirectoryPath(context.globalStorageUri.fsPath) |
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 change looks correct in delegating to the storage utilities, but I'm wondering if there might be a subtle issue here. The getStorageBasePath()
function expects the default path and applies custom path logic on top of it. Since you're passing context.globalStorageUri.fsPath
directly, this should work as intended.
However, have we verified that all 17 call sites of ensureSettingsDirectoryExists()
are expecting this new behavior where custom paths are respected? This could be a breaking change for code that specifically needs the default storage location.
This PR doesn't work. The fix is incomplete and doesn't properly address the custom storage path issue. Closing. |
Description
This PR fixes an issue where the custom modes YAML file was not being found when users configured a custom storage path using the
roo-cline.customStoragePath
setting.Problem
When users set
roo-cline.customStoragePath
in their VSCode settings, theCustomModesManager
was still looking forcustom_modes.yaml
in the default global storage location instead of the custom path. This was because:CustomModesManager.getCustomModesFilePath()
was usingensureSettingsDirectoryExists()
fromglobalContext.ts
ensureSettingsDirectoryExists()
was directly usingcontext.globalStorageUri.fsPath
without checking for the custom storage path settingSolution
CustomModesManager
to usegetSettingsDirectoryPath()
fromstorage.ts
which properly respects thecustomStoragePath
settingglobalContext.ts
functions to use the storage utilities that handle custom paths correctlyTesting
Fixes #7154
Important
Fixes issue with
CustomModesManager
not respectingroo-cline.customStoragePath
by usinggetSettingsDirectoryPath()
.CustomModesManager.getCustomModesFilePath()
now usesgetSettingsDirectoryPath()
to respectroo-cline.customStoragePath
.globalContext.ts
to usegetSettingsDirectoryPath()
for custom paths.This description was created by
for caf8708. You can customize this summary. It will automatically update as commits are pushed.