-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Store persistent chat sessions in the workspace by default #16847
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
base: master
Are you sure you want to change the base?
Store persistent chat sessions in the workspace by default #16847
Conversation
Implement an optional visibleWhen property on preference schema that provides for a very simple condition to show a preference in the UI when some other preference does or does not have some value. In support of eclipse-theia#16565. Signed-off-by: Christian W. Damus <cdamus@eclipsesource.com>
Add a preference for workspace-local versus global storage of chat sessions. Workspace storage is now the default and global storage is as before. The global storage location is now configurable, defaulting to $HOME/.theia/chatSessions as before. Use the new capability for simple preference visibility conditions to show either an optional global storage path or the workspace-relative storage preference according to the current storage scope. Finally, update the Show Chats command to not require the AI Chat view to be open. It will be opened on selection of a chat. This is now consistent with the New Chat command. Fixes eclipse-theia#16565 Signed-off-by: Christian W. Damus <cdamus@eclipsesource.com>
|
General comments before I take a look at the code:
I'm likely not that keen on this feature. I understand that it has some value for avoiding confusion - you see the path setting for the storage scope you're using, and not the other one that would have no effect - but it introduces complexity into the preferences UI that isn't completely transparent to the user. I can very easily imagine some user seeing mention of one of the scope preferences, then opening up the preferences only to find that they don't see that particular scope's path preference, then reaching out to their IDE team (this used to be me, whence the vividness of this scenario in my mind) and then having them explain that to see that setting, you first have to change another setting. Of course the other scenario is likely as well - they reach out because they changed the wrong scope and didn't see the behavior they expected - but we could probably avoid that with some path validation: insist on absolute paths for the global one and relative for the workspace one.
But if my arguments above aren't persuasive, I'd probably prefer just supporting any when clause, rather than restricting it to |
What I would prefer TBH would be possibly an even further departure from what is currently supported. I would like to see:
If we'd rather not make any changes to the preferences UI for this feature, then I would suggest an alternative:
Overriding the path setting itself at workspace scope should give users all the flexibility they need. |
This would be possible in principle: the preference rendering system is extensible so that if we had a base preference The main entry point into that system is preference-node-renderer-creator.ts, if you're interested in going down that path. |
Cool, thanks for that! I'll give it a try. So hold off on review if you'd like 😀 |
This reverts commit 84167b0. Signed-off-by: Christian W. Damus <cdamus@eclipsesource.com> # Conflicts: # packages/ai-chat/src/browser/chat-session-store-impl.spec.ts
|
Thanks @colin-grant-work for the pointers to custom preference renderers. I'm very much happier with the result of refactoring the UI in commit c672104 for a complex preference setting. And I've reverted the changes in the preference framework for visible-when conditions. |
Instead of having several separate preferences for chat session storage, model it as a complex object with a custom renderer that groups the path settings under a disclosure triangle. Signed-off-by: Christian W. Damus <cdamus@eclipsesource.com>
0c79d3c to
c672104
Compare
Signed-off-by: Christian W. Damus <cdamus@eclipsesource.com>
package-lock.json
Outdated
| "version": "12.2.3", | ||
| "resolved": "https://registry.npmjs.org/@types/markdown-it/-/markdown-it-12.2.3.tgz", | ||
| "integrity": "sha512-GKMHFfv3458yYy+v/N8gjufHO6MSZKCOXpZc5GXIWWy8uldwfmPn98vp81gZ5f9SVw8YYBctgfJ22a2d7AOMeQ==", | ||
| "dev": true, |
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.
@ndoschek, these are the kinds of changes I also see if I run npm install locally. The deletion of these lines is likely unintentional, but their addition was also recent and likely unintentional. Do we have a policy / desired target for this kind of metadata in the package-lock.json?
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 lock file only changed because I had to add a dependency from the ai-chat package to the preferences package. I'm loath to manually edit a lock file.
packages/ai-chat-ui/src/browser/session-storage-preference-renderer.ts
Outdated
Show resolved
Hide resolved
| if (!storageConfig.workspacePath.trim()) { | ||
| this.logger.debug('Workspace storage path is empty: session persistence disabled.'); | ||
| return undefined; | ||
| } |
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 could be moved up to before we try to retrieve the workspaceRoot. (If we retain this behavior)
| const roots = await this.workspaceService.roots; | ||
| if (roots.length > 0) { | ||
| return roots[0].resource; | ||
| } | ||
| return undefined; |
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.
If we're watching for workspace changes (and it looks like we are), this doesn't really need to be the async version; you can use this.workspaceService.tryGetRoots to get the current list, and if the list changes, we'll update anyway.
- import the CSS where it's needed - freeze the SessionStorageValue::DEFAULT and spread as required - show the focus border on the disclosure triangle when focused Signed-off-by: Christian W. Damus <cdamus@eclipsesource.com>
Signed-off-by: Christian W. Damus <cdamus@eclipsesource.com>
Do not hard-code the .theia/ directory name nor reference it in user facing text. Signed-off-by: Christian W. Damus <cdamus@eclipsesource.com>
- use explicit default values for both paths - do not allow empty inputs - validate global path for being absolute - validate workspace path for being relative and not escapting the workspace directory Signed-off-by: Christian W. Damus <cdamus@eclipsesource.com>
Instead show an info label explaining that a path is not used. Signed-off-by: Christian W. Damus <cdamus@eclipsesource.com>
Listen for workspace changes generally to determine when it happens that what formerly was the first root of the workspace no longer is, and in that case invalidate the storage. Don't listen for workspace changes in global scope. Signed-off-by: Christian W. Damus <cdamus@eclipsesource.com>
|
Thanks @colin-grant-work for your thorough review and excellent comments. I've pushed changes that I think, overall, address all of your feedback and make this solution so much better.
|
Signed-off-by: Christian W. Damus <cdamus@eclipsesource.com>
|
Oh somebody changed the API of preference node renderers today. Clever 😉 must be coincidence. I've rescued my renderer in commit 26e8928. |
What it does
Implements storage of chat session persistence — initially implemented with #16486 — in the workspace by default.
A new complex preference settings is added with three details:
'workspace'and'global', workspace being the default, determining whether chat session storage is per-workspace (and so in the workspace root directory) or global (shared by all workspaces, as previously). It has two sub-preferences:'workspace'. The default is.theia/chatSessions. If set empty, then there is no persistence of chat sessions (this does not mean to use the workspace root folder itself as the store; I suppose a user could set.if they want to annoy themselves)'global'. When set empty, which it is by default, then the implied location is$HOME/.theia/chatSessions.The object-valued preference setting is edited in a new custom preference renderer that organizes the path inputs behind a disclosure triangle. Each of these inputs has its own revert button to restore the default setting. The preference object persists only those of its properties that do not have their default value. This ensures that if a workspace overrides the preference, user-specific default paths are not persisted in version control repositories.
How to test
Play with the new preferences for chat session storage and see how they interact with the "Show Chats" command. You should be able to store a different set of chats in global storage versus workspace storage, and set up multiple stores of each kind via the path preferences. Whatever the preference settings, the Show Chats command should show what actually is in that store and it should not be available when there are no chats, neither in the in-memory history nor the store.
Follow-ups
Breaking changes
I suppose that, technically, changing the default storage location of chat sessions breaks the behaviour previously established and will appear at first to be a case of data loss for users, although reversion to the global scope is available. It does not seem worth the effort to provide a tool to import the chat session store into the current workspace.
There is no meaningful API breaking change.
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers