feat: configuration screen #153
Conversation
|
Nice! In addition to access to this screen from the keyboard, we should probably put a little gear icon down in the lower left corner like in VS Code. Currently I configure 3 things with OpenAI based on what I saw in @gjreda's checkin: Also, I configure these settings now through environment variables. Should this configuration screen auto-populate with what it finds in environment variables? If so, we should probably also add some text for each configuration variable to indicate what environment variable name we use. I tend to think of the configuration hierarchy as starting with environment variables, which can be overridden by config files, which can be overridden by command-line flags. Given that we expect most users to install this application and run it from a launcher or by clicking an icon, I'm not sure if it makes sense to give every configuration variable a corresponding environment variable, but it sure is convenient to not have to set these variables each time I try out the app... I'm thinking at least for OpenAI we do honor environment variables for now. |
|
I think that using environment variables as defaults (show in the config screen) is a nice to have (mostly for development - and maybe for some reasonable defaults if we include some sort of .env in the build). Adding command-line arguments seems a bit to much for now. Also, I'm going to save the settings in a permanent storage (.json file) to persist settings over app runs. |
cguedes
force-pushed
the
76-configuration-screen
branch
from
79487f2
to
5df23c1
Compare
There was a problem hiding this comment.
I'm also curious about the relationship between .env and settings.json. Is the latter populated from the former only once when you first start the app? That may be unavoidable, but I can definitely imagine it being a source of confusion in the future.
If we anticipate having lots of settings, it might be convenient to use a library like this one to generate a basic form UI from the schema for the settings:
https://github.com/rjsf-team/react-jsonschema-form
I believe VS Code does something like this. Here's an example of how the settings are specified for an extension:
https://github.com/asciidoctor/asciidoctor-vscode/blob/50515506f5c55eb14caee419bd15bda67b501700/package.json#L267
src/settings/SettingsModal.tsx
Outdated
| await unregister(SETTINGS_SHORTCUT_TOGGLE); | ||
| await unregister(SETTINGS_SHORTCUT_CLOSE); | ||
| await register(SETTINGS_SHORTCUT_TOGGLE, () => onToggle(!open)); | ||
| await register(SETTINGS_SHORTCUT_CLOSE, () => onToggle(false)); |
There was a problem hiding this comment.
Presumably you don't want ESC to be registered globally, only when the Settings modal is open?
My suggestion would be:
SettingsModalis always open when it's in the virtual DOM.- Its visibility is owned by the parent (
App) which also registers the Cmd, shortcut (and presumably never needs to unregister it). SettingsModalregisters the ESC shortcut and unregisters it when it unmounts. It can just have anonCloseprop.
There was a problem hiding this comment.
My (initial) idea was to have a drop-in component for the settings that handles the open/close behaviour. Meanwhile, with the sidebar action to open the settings I've have to move the open/close control to the App.
With the addition of the window menu entries and handles I rewrite this component to:
- Stop using Tauri's global shortcut (that feature is for scenarios when you want a OS-wide shortcut trigger for the app)
- Adopt Tauri's events for the communication between Rust and the front-end
- New event for the settings menu click (tauri://menu/settings)
- Emit settings menu event when on clicks in the Settings icon in the sidebar
src/settings/settings.ts
Outdated
| }, | ||
| }, | ||
| { | ||
| fileName: 'settings', |
There was a problem hiding this comment.
Does this persist settings in settings.json?
There was a problem hiding this comment.
Yes, the lib appends the extension and ignores the extension if we define it.
I've also made the dir explicit to help discover where the file is.
- accelerator `Cmd+,` instead of globalShortcut - rust module for menu - backend (rust) -> frontend comm via events
I see your confusion. The idea is to be able to bundle a .env file with default configurations that is read (once) to bootstrap de settings (settings.json). From that point onwards the app only read that file. I think that for now, too keep it simple, we should have the defaults (ex: Also bc for the backend development (CC @gjreda) that file would be useful to store keys that would be received via sidebar when the app is running. |
Codecov Report
@@ Coverage Diff @@
## main #153 +/- ##
==========================================
+ Coverage 57.19% 63.25% +6.05%
==========================================
Files 70 82 +12
Lines 3413 4022 +609
Branches 226 285 +59
==========================================
+ Hits 1952 2544 +592
- Misses 1440 1457 +17
Partials 21 21
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
I'm probably not the best person to review the front-end code here, but the way we are doing the new |
This PR adds a configuration and underlying settings support to the app.
Features:
refstudio -> settingsor keyboardCmd+,shortcutAPP_DATA_DIR- (default to Tauri'sappDataDir())PROJECT_NAME- (default to project-x)OPENAI_API_KEY- (default fromOPENAI_API_KEYenv)OPENAI_CHAT_MODEL- (default fromOPENAI_CHAT_MODELorgpt-3.5-turbo)OPENAI_COMPLETE_MODEL- (default fromOPENAI_COMPLETE_MODELenv ordavinci)SIDECAR_ENABLE_LOGGING- (default fromSIDECAR_ENABLE_LOGGINGenv orfalse)SIDECAR_LOG_DIR- (default fromSIDECAR_LOG_DIRenv or/tmp)useAsyncEffectutility to simplify async effectsnoopto simplify tests with mandatory handler we don't need to use/testTasks
Cmd+,OPENAI_API_KEY,OPENAI_COMPLETE_MODEL,OPENAI_CHAT_MODEL