-
Notifications
You must be signed in to change notification settings - Fork 8
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
Universal settings #417
Universal settings #417
Conversation
Codecov Report
@@ Coverage Diff @@
## main #417 +/- ##
==========================================
- Coverage 80.87% 77.72% -3.16%
==========================================
Files 188 190 +2
Lines 11202 11547 +345
Branches 1140 1115 -25
==========================================
- Hits 9060 8975 -85
- Misses 2127 2556 +429
- Partials 15 16 +1
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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 is as far as I got today. It includes logic to wait for the server to start up (for the desktop app), use the settings API (for both desktop and web) and the start of codegen for our HTTP API.
"tasks": [ | ||
{ | ||
"type": "npm", | ||
"script": "ts:check", |
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 is a convenient way to see all errors in all files your project, rather than just the ones that are open in VS Code. (Useful after a refactor!)
python/web.py
Outdated
@@ -9,7 +9,7 @@ | |||
api.mount("/api/fs", http.filesystem_api) | |||
api.mount("/api/projects", http.project_api) | |||
api.mount("/api/meta", http.meta_api) | |||
api.mount("/api/settings", http.settings) | |||
api.mount("/api/settings", http.settings_api) |
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 was a fun one to track down 🙃
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.
Also for me! Should have told you about this.
|
||
useAsyncEffect( | ||
async (isMounted) => { | ||
if (!isServerRunning) { | ||
console.log('waiting for server startup'); |
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 did wind up needing to implement a more robust latch here -- we can't load the settings until the HTTP server is running and listening for connections. We may want to iterate on this as I think this does introduce a noticeable lag during startup, maybe 0.25–0.5s.
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.
So now we will actually take advantage of the splash screen after all :-).
@@ -35,7 +40,7 @@ export function AppStartup() { | |||
|
|||
if (isMounted()) { | |||
setInitialized(true); | |||
const projectDir = getCachedSetting('project.currentDir'); | |||
const projectDir = getCachedSetting('project.current_directory'); |
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.
Python & TS share types for SettingsSchema
now so there are a few changes like this. If we really want to camelcase everything then we should do it with some kind of middleware. But I'd rather have fewer types.
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 understand how sharing simplify the development. I also know that most times we will translate from a server type definition to a client definition so this is ok.
console.log('Starting RefStudio server'); | ||
const command = new TauriCommand('call-sidecar', ['serve']); | ||
command.stderr.addListener('data', (line) => { | ||
console.log('server stderr', line); | ||
if (line.includes('running on http')) { |
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 matches:
Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
Definitely open to other ideas on how to tell that the app is listening for connections!
src/settings/settingsManager.ts
Outdated
return value; | ||
}, | ||
syncCache: async () => { | ||
const newSettings = (await universalPost('/api/settings/', settings, 'PUT')) as SettingsSchema; |
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.
Note: the trailing slash is important. If you leave it off, you get confusing CORS errors from the dev server in web mode.
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 file gets a lot simpler and we get to drop the (runtime) dependency on tauri-settings, which is a big win. We could drop the dependency on its types in a follow-up.
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.
Reminder to clean-up this in #422
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 deleted these this to get tests passing, but we should update/rewrite them 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.
This papers over the differences between Tauri fetch and web fetch.
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.
Thanks for starting this one.
* This interface was referenced by `ApiSchema`'s JSON-Schema | ||
* via the `definition` "SettingsSchema". | ||
*/ | ||
export interface SettingsSchema { |
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.
These are all the types that appear in a FastAPI request/response type or are referenced from one of those types. Note how all the fields are optional, which is not what we want.
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.
Agree. Will let you find how we can improve this in a follow-up PR.
This PR connect the client code (web hosting) to backend APIs via (external) HTTP using standard fetch. A follow-up PR will existing after #417 is merged, that will implement the unified codebase for the desktop+web client code. It's also important to note that for this PR the goal was to not change the existing desktop behaviour, implementing if/else checks with import.meta.env.VITE_IS_WEB. For this PR the main changes where: remove in-memory implementation of fs.tsand stub code. Now the code throws exception. adopt the new project API and therefore, save the projectId in the client to use in every HTTP call. implement web.ts and *API.ts utilities to perform calls to the backend HTTP in a web context. Adjust the code to flow the projectId to every business logic call in order to support listing, opening, upload files, and also references operations (ingestion, list, delete, edit). Adopt the new HTTP api for the chat operation. The rewrite is still using the sidecar command. Notes: The code/files structure wasn't a priority for this PR. That organisation will be implemented in the follow-up PR. (web) the open project will open one of the previously created projects (in the web context) * Send env variables via header "X-..." in the HTTP sidecar interface * Adds a projectsAPI to create new projects in the Web * Support creating new projects for the Web * Fix py lint * WIP: Adopt fs api * Fix settings API import * handle env variable with middleware * Add web.ts with api calls to backend server * Remove outdated tests * Fix `yarn check` issues * Directly use !!import.meta.env.VITE_IS_WEB * Configure project name via API * Adopt HTTP ingestion for web * Add support to delete references * remove app startup utility * Open one of the existing projects instead of open hard-coded * Allow references to be edited (PATCH) * Add support to chat with references via HTTP web * Remove legacy fs.ts and stub * Fix unit tests * fix python tests * Fix tauri-wrapper signature * Fix yarn:check errors * Fix test * Remove unused args
python/sidecar/typing.py
Outdated
@@ -202,7 +207,7 @@ class SearchResponse(RefStudioModel): | |||
|
|||
|
|||
class OpenAISettings(RefStudioModel): | |||
api_key: str = "" | |||
api_key: str = os.environ.get("OPENAI_API_KEY") |
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 is where the key leaks -- when this schema gets converted to JSONSchema, the default value is serialized, leaking the key. TS doesn't have any notion of default values, so it goes away in api-types.ts
.
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've updated this to use the value "" instead of reading from os.env
Part of the work for #347
This PR includes
tauri-settings
Pending:
tauri-settings
Edit: Description updated by @cguedes