Skip to content
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

Settings UI improvement #48

Merged
merged 16 commits into from
Mar 6, 2025
Merged

Settings UI improvement #48

merged 16 commits into from
Mar 6, 2025

Conversation

brichet
Copy link
Collaborator

@brichet brichet commented Feb 19, 2025

Fixes #35

record-2025-02-20_18.17.07.webm

This PR uses a react component in the settings panel, to make it more reactive and configurable.
With this new setting component:

  • settings are saved for each provider, and restored when loading the page again (use local storage)
  • the field containing the word 'key' are replaced by a password widget

It also use a simpler way to import generated schema, no need to set it up manually when adding a new provider.

@brichet brichet added the enhancement New feature or request label Feb 19, 2025
@brichet brichet marked this pull request as ready for review February 20, 2025 17:24
@brichet brichet changed the title Settings with a custom react component Settings UI improvement Feb 20, 2025
@brichet
Copy link
Collaborator Author

brichet commented Feb 20, 2025

This PR is missing some helpers about the use of each provider, as described in #35 (comment).

Since this is using a FormComponent, helper could easily be included above or below it.
A better user experience could be to include it between the provider select and the dynamic settings. This option needs some refactoring to split the form, and include the helper.

@brichet brichet marked this pull request as draft February 27, 2025 09:07
@brichet brichet marked this pull request as ready for review February 27, 2025 12:16
@brichet
Copy link
Collaborator Author

brichet commented Feb 27, 2025

Instructions added to the settings (currently only for MistralAI and ChromeAI), those from the readme.

record-2025-02-27_13.15.05.webm

@brichet brichet requested a review from jtpio February 27, 2025 12:18
@brichet
Copy link
Collaborator Author

brichet commented Feb 27, 2025

The instructions are displayed using the markdown renderer from Jupyterlab, provided by IRenderMimeRegistry.
The way Jupyterlab allows to render a specific widget in place of the regular settings form doesn't allow to pass some arguments to that widget. Therefore, we use a static variable to provide the IRenderMimeRegistry, I did not find a another option.
I explored the use of the formContext props, but it doesn't seems that we can pass additional properties to it currently (see here)

@jtpio
Copy link
Member

jtpio commented Feb 27, 2025

I explored the use of the formContext props, but it doesn't seems that we can pass additional properties to it currently (see here)

Thanks for looking into this 👍

Having a way to provide more parameters would indeed also be useful to pass a secret manager (#37), so the secrets could be saved elsewhere.

For now I guess we can use the trick of the static method, but maybe there is something so be done in JupyterLab to make this more flexible.

[key: string]: T;
}

const chromeAiInstructions = `
Copy link
Member

@jtpio jtpio Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably fine to leave them here in one file here for now.

Later when third-party extensions can register additional AI providers (#45) they could also provide their own instructions for their provider.

* Get the current provider from the local storage.
*/
getCurrentProvider(): string {
const settings = JSON.parse(localStorage.getItem(STORAGE_NAME) || '{}');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering how this would play with a custom overrides.json, in case some deployments would like to pre-configure a model (and some of its options) by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would probably overwrite it.
I don't really know how works overrides.json. Can we get its default values with annotatedDefaults() ?
In this case we could simply replace '{}' with this._settingsRegistry.annotatedDefaults().

BTW, we should probably rename this._settingsRegistry to this.settings for consistency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like annotatedDefaults() returns the overrides.json contents.

In the constructor, we could check if there is a value in localStorage.getItem(STORAGE_NAME).
If not, read the annotatedDefaults() and, if the provider is set in the default settings, fill the local storage with its contents and set the default provider as current. If the provider is not set in the default settings we can probably not use these values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jtpio I updated the PR according to my previous comment, using this._settings.default('AIprovider') instead of this._settingsRegistry.annotatedDefaults().

@brichet
Copy link
Collaborator Author

brichet commented Mar 5, 2025

@jtpio this PR is ready.

  • the AISettings component does not use a static rendermime registry anymore. The renderer uses a factory to provide the rendermime registry to the component (thanks again @jtpio for providing this solution)
  • if there are no settings in the local storage, it uses the user settings (which lead to the default settings if empty too)

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jtpio jtpio merged commit c7d428d into jupyterlite:main Mar 6, 2025
7 checks passed
@brichet brichet deleted the config_panel branch March 6, 2025 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a dedicated settings panel
2 participants