-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: add workspace-specific provider settings #7867
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
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 wrote this code with MD5 hashing. Even I know that's a security theater at best.
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.
Security concern: MD5 is cryptographically weak and shouldn't be used even for non-security purposes. Consider using SHA-256 instead:
| return crypto.createHash("md5").update(workspacePath).digest("hex").substring(0, 8) | |
| const crypto = require("crypto") | |
| return crypto.createHash("sha256").update(workspacePath).digest("hex").substring(0, 8) |
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.
Performance issue: Using require('crypto') synchronously inside a method could cause performance issues. Consider importing crypto at the module level instead.
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.
Missing error context: This generic error doesn't provide enough context. Consider:
| throw new Error("No settings found in source scope") | |
| throw new Error(`No settings found in ${fromWorkspace ? 'workspace' : 'global'} scope to copy`) |
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.
Inefficient scope switching: This logic switches scope multiple times in a loop. Could we batch the operations to minimize scope changes? Consider getting all configs first, then switching scope once to save them all.
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.
Missing validation: Should we validate that the workspace path exists before generating an ID? An invalid path could lead to unexpected behavior.
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.
i18n missing: These strings should use the translation system. The t() function is already imported but not used for all strings.
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.
Test coverage gap: Consider adding tests for error scenarios like invalid workspace paths, storage failures, and concurrent access issues.
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.
Code duplication: This settings copying logic is duplicated from the toggleWorkspaceProviderSettings handler above. Could we extract this into a shared helper function?
- Add workspace-scoped storage support to ProviderSettingsManager - Add toggle between workspace and global settings in UI - Add visual indicators (Globe/Folder icons) for current scope - Add VSCode configuration option for workspace provider settings - Support migration of settings between global and workspace scopes - Update tests to support new workspace settings functionality Fixes #7865
77d08c7 to
c486de8
Compare
| <div className="flex flex-col"> | ||
| <span className="text-sm font-medium"> | ||
| {isUsingWorkspaceSettings | ||
| ? t("settings:providers.workspaceSettings") || "Workspace Settings" |
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.
Avoid inline fallback strings in translation calls. Instead of using '|| "Workspace Settings"' (and similar fallbacks) in the UI, rely on the translation system and supply defaults in the translation files.
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
This PR implements workspace-specific provider settings as requested in #7863.
Summary
This feature allows users to save AI provider configurations per workspace instead of globally across all VS Code workspaces. Different projects can now use different AI providers without affecting other workspaces.
Changes
Core Implementation
getWorkspaceId()method to generate unique workspace identifiers using MD5 hashingsetUseWorkspaceSettings()to switch between storage scopescopySettings()method to sync settings between workspace and global scopesConfiguration
useWorkspaceProviderSettingsboolean setting to toggle between workspace and global settingspackages/types/src/global-settings.tsUI Components
WorkspaceSettingsToggleReact component for the settings UIMessage Handling
toggleWorkspaceProviderSettingsmessage handlercopyProviderSettingsmessage handler for copying settings between scopesTesting
Features
✅ Toggle between workspace and global provider settings
✅ Copy settings from global to workspace (and vice versa)
✅ Backward compatible - existing global settings are preserved
✅ Secure storage using VS Code's secrets API
✅ Unique workspace identification that's stable across sessions
✅ Clean UI integration with clear status indicators
Testing
Screenshots
The new workspace settings toggle appears in the Settings view, allowing users to easily switch between workspace-specific and global provider configurations.
Fixes #7863
Important
This PR adds workspace-specific provider settings, allowing users to toggle between global and workspace-specific configurations with UI support and comprehensive testing.
ProviderSettingsManagernow supports workspace-specific settings with methods likesetUseWorkspaceSettings()andgetStorageKey().useWorkspaceProviderSettingssetting added to toggle between global and workspace settings.toggleWorkspaceProviderSettingsmessage handler added inwebviewMessageHandler.ts.WorkspaceSettingsTogglecomponent added toApiConfigManager.tsxfor UI integration.This description was created by
for c486de8. You can customize this summary. It will automatically update as commits are pushed.