-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: respect custom storage path for global custom modes #8126
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 ensureSettingsDirectoryExists() to use getSettingsDirectoryPath() - This ensures custom storage path setting is honored when loading custom modes - Also update createModeInstructions() and migrateSettings() for consistency Fixes #8122
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but somehow still broken.
| await mkdir(settingsDir, { recursive: true }) | ||
| // Use getSettingsDirectoryPath which respects the custom storage path setting | ||
| const settingsDir = await getSettingsDirectoryPath(context.globalStorageUri.fsPath) | ||
| // getSettingsDirectoryPath already creates the directory, so we just return the 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.
Is this comment on line 11 accurate? Since getSettingsDirectoryPath already creates the directory (as seen in storage.ts line 66), could we simplify this function to just be a direct pass-through? Something like:
| // getSettingsDirectoryPath already creates the directory, so we just return the path | |
| export async function ensureSettingsDirectoryExists(context: ExtensionContext): Promise<string> { | |
| return getSettingsDirectoryPath(context.globalStorageUri.fsPath) | |
| } |
This would eliminate the redundant variable assignment and make the code more concise.
| await mkdir(settingsDir, { recursive: true }) | ||
| // Use getSettingsDirectoryPath which respects the custom storage path setting | ||
| const settingsDir = await getSettingsDirectoryPath(context.globalStorageUri.fsPath) | ||
| // getSettingsDirectoryPath already creates the directory, so we just return the 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.
Minor observation: The original function had explicit mkdir with error handling, but now we're delegating all error handling to getSettingsDirectoryPath. Is this intentional? The errors will still bubble up, but we lose the ability to add any function-specific error context here if needed in the future.
|
|
||
| const settingsDir = path.join(context.globalStorageUri.fsPath, "settings") | ||
| // Use getSettingsDirectoryPath to respect custom storage path setting | ||
| const settingsDir = await 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.
Good consistency update! This ensures the help text shows the correct path when a custom storage location is configured. Users will see the actual path where their modes will be saved.
… use no-create in migrations and instructions to avoid side effects
Description
This PR fixes an issue where the custom storage path setting was not being respected when loading global custom modes. The extension would always look for custom modes in the default storage location, even when a custom path was configured.
Problem
When users configured a custom storage path via the
roo-cline.customStoragePathsetting, the extension would:Solution
Updated
ensureSettingsDirectoryExists()to usegetSettingsDirectoryPath()which properly respects the custom storage path setting. This ensures that all operations (read/write) for global settings use the same path resolution logic.Changes Made
ensureSettingsDirectoryExists()to usegetSettingsDirectoryPath()instead of hardcoding the pathgetSettingsDirectoryPath()for displaying the correct path in instructionsgetSettingsDirectoryPath()for consistencyTesting
Impact
This fix ensures that:
CustomModesManager.getCustomModesFilePath()correctly uses the custom storage pathSimpleInstaller.getModeFilePath()correctly uses the custom storage pathFixes #8122
Important
Fixes custom storage path handling for global custom modes by updating path resolution logic in key functions.
ensureSettingsDirectoryExists()inglobalContext.tsto usegetSettingsDirectoryPath().createModeInstructions()increate-mode.tsto usegetStorageBasePath().migrateSettings()inmigrateSettings.tsto usegetStorageBasePath().getStorageBasePath()instorage.tsnow optionally avoids creating directories for path resolution.getSettingsDirectoryPath()instorage.tsensures directory creation.This description was created by
for 604a606. You can customize this summary. It will automatically update as commits are pushed.