-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat(custom-instructions): migrate custom instructions to file-based storage #4002
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
daniel-lxs
left a comment
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.
Hey @devxpain, This looks like a great idea, it should help a lot with the memory issues we are having.
I left some questions that are worth looking into to prevent potential issues with the file.
But overall I like what you did here, let me know if you want to discuss this further.
src/core/webview/ClineProvider.ts
Outdated
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 noticed we're silently catching ENOENT errors here but throwing others. Is this intentional? Should we be consistent with error handling across all the new methods?
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.
ae788e7e93d6aac4ff507f369d335fc6eaff8845
src/utils/migrateSettings.ts
Outdated
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.
What happens if the migration fails partway through (e.g., file write succeeds but globalState update fails)? Should we wrap this in a try-catch and potentially rollback on failure to prevent data loss?
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.
753d77893507be4b8a87009b74aa608f5f1beb1f
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.
When users click the refresh button, should we provide some visual feedback (like a brief loading state or toast notification) to confirm the refresh happened? Right now it might not be clear if the action succeeded.
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.
070939c076ec08e6ed8dab1036791798d705cf3a
src/core/webview/ClineProvider.ts
Outdated
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.
What happens if the custom instructions file doesn't exist when a user clicks the open button? Should we create an empty file first to avoid VSCode showing an error?
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.
7b11bce832d589e4b0f400bd5f222c96cd84d07a
src/core/webview/ClineProvider.ts
Outdated
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 noticed there aren't any tests for these new methods. Should we add test coverage for:
File creation/deletion behavior
Migration logic
Error handling scenarios
The new UI interactions?
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.
46092af20e27cc7c5d96ede14322ac56af986e10
Right now, I’ve just made sure that pnpm test runs without any errors. I’ll add tests after confirming that the current changes are solid.
Hi @daniel-lxs, thanks for your feedback and review! |
|
Hi @daniel-lxs, I’ve pushed more commits based on your feedback. Instead of focusing only on As for the tests, I’ll add them once we confirm that the current approach is correct and stable. |
46092af to
17718ba
Compare
daniel-lxs
left a comment
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.
Hey @devxpain, Thank you for addressing my previous points so quickly.
I apologize for the delay in reviewing your PR.
I left some additional points I missed in my previous preview, hopefully with these changes we should be good.
Also I noticed the translations action is failing, it would be great if you can take a look at that.
src/core/webview/ClineProvider.ts
Outdated
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.
Any specific reason to have these methods inside ClineProvider?
If not, it might be a good idea to move them out since this class is already quite big.
src/utils/fs.ts
Outdated
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 error handling pattern for ENOENT is duplicated in multiple places:
- Here in
safeReadFile - In
ClineProvider.updateContent - In
custom-instructions.ts - In
custom-system-prompt.ts
It would be a good idea to consolidate this on a single helper function.
src/core/webview/ClineProvider.ts
Outdated
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 wonder if the error handling can be improved:
- When
filePathis undefined, log is created but an error is not thrown or the user is not notified - When file operations fail, errors are thrown but not caught by callers
- No validation for file size, content format, or concurrent modifications
It might be better to:
- Add proper error boundaries and user notifications
- Validate content before writing (size limits, format)
- Handle concurrent modification scenarios
- Consider using a try-catch wrapper for all file operations
Since this increases complexity to some degree it might be better to move these methods out of ClineProvider.
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.
The error handling here only logs to console but doesn't provide feedback to the user. When operations fail, users won't know what went wrong.
Consider:
- Sending error messages back to the webview
- Showing user-friendly error notifications
- Adding proper error recovery mechanisms
Hi @daniel-lxs, Thanks again for your feedback. If I do a full refactor too early, it might distract from the main focus, especially since I wasn’t sure about the direction you all want to take with the refactor. That’s why your feedback is so valuable to me. Thanks again — I’ll review your suggestions and address them as soon as possible. |
|
Hey @devxpain, No worries, the purpose of the feedback is not to point out something you did wrong, is to point out things that could be a bit better. The better we get your implementation the higher the chance is merged. If you have any questions please let me know, again this review is not meant to discourage you at all. |
…storage See discussion: RooCodeInc#4000 Migrated the storage of "Custom Instructions for All Modes" from VS Code's global state to a dedicated file: `...Code/User/globalStorage/rooveterinaryinc.roo-cline/settings/custom_instructions.md`. Key changes include: - Implemented a migration utility to automatically move existing custom instructions from global state to the new file upon extension update. - Introduced new commands and UI buttons within the Prompts view to: - Open the `custom_instructions.md` file directly in the editor for easy editing. - Refresh the custom instructions in the UI by re-reading them from the file, ensuring changes made directly to the file are reflected. - The `updateCustomInstructions` function now writes/deletes the file instead of updating global state. - The initial state and refreshes now read custom instructions directly from the file. This change provides a more robust and user-friendly way to manage custom instructions, allowing for external editing and version control.
- Moved `safeReadFile` function from multiple files to `src/utils/fs.ts` - Updated imports in affected files to use the centralized `safeReadFile` function - Simplified file reading logic by removing redundant implementations
- Wrap file write and global state update in try-catch block - Log error message if migration fails
- Refactor custom instructions management to a generic 'content' system, introducing `GlobalContentIds`. - Implement new `openContent`, `updateContent`, and `refreshContent` methods in `ClineProvider` to handle various content types. - Update webview message interfaces (`WebviewMessage`, `ExtensionMessage`) to support content IDs and a new `contentRefreshed` event. - Enhance the custom instructions UI in the webview with a dedicated `useRefreshableContent` hook. - Add visual feedback for content refresh operations, including a loading spinner and a success message.
- Replaced direct `vscode.commands.executeCommand("vscode.open")` call with the `openFile` utility.
- Centralizes file opening logic, improving reusability and maintainability within the webview provider.
- Ensure `safeReadFile` handles empty content gracefully to prevent potential errors. - Update Jest mocks for `fs` utility functions in tests to correctly spread original exports, improving test reliability. - Add comprehensive `globalState` mocking in `migrateSettings` tests for better test coverage.
- Move content-related methods (openContent, refreshContent, updateContent, readContent) from ClineProvider to a new ContentManager class. - Instantiate ContentManager in ClineProvider and delegate content operations to it. - Improve separation of concerns by centralizing content file system interactions and state updates within ContentManager.
|
stales |
Related GitHub Issue
Discussions: #4000
Description
Migrated the storage of "Custom Instructions for All Modes" from VS Code's global state to a dedicated file:
...Code/User/globalStorage/rooveterinaryinc.roo-cline/settings/custom_instructions.md.Key changes include:
custom_instructions.mdfile directly in the editor for easy editing.updateCustomInstructionsfunction now writes/deletes the file instead of updating global state.This change provides a more robust and user-friendly way to manage custom instructions, allowing for external editing and version control.
Test Procedure
Build .vsix and install then test manually
Type of Change
srcor test files.Pre-Submission Checklist
npm run lint).console.log) has been removed.npm test).mainbranch.npm run changesetif this PR includes user-facing changes or dependency updates.Screenshots / Videos
Documentation Updates
Additional Notes
Get in Touch
Important
Migrate custom instructions from VS Code's global state to a file-based system with new UI and commands for management.
custom_instructions.mdfile inClineProvider.ts.openCustomInstructionsFileandrefreshCustomInstructionscommands inwebviewMessageHandler.ts.PromptsView.tsxto include UI buttons for opening and refreshing custom instructions.migrateSettings.tsto move instructions from global state to file.PromptsView.tsxfor opening and refreshing thecustom_instructions.mdfile.prompts.jsonto include new strings for custom instructions management.This description was created by
for 799a1cde160fe0899430fa29c4b7cf96b6fc4161. You can customize this summary. It will automatically update as commits are pushed.