Universal settings by danvk · Pull Request #417 · refstudio/refstudio

danvk · 2023-08-24T19:45:36Z

Part of the work for #347

This PR includes

Desktop app wait for the server to be ready before start (during splash-screen, in AppStartup)
Enhanced codegen for API types (from python)
Removed (funcional) dependency on tauri-settings
Adopt the new settings
Universal GET and POST functions to make HTTP requests via tauri (desktop) or native fetch (web)

Pending:

Review structure (schema) of settings and try to make it a key/val store instead of the current object tree
Remove dependency of tauri-settings
(for Dev) Automatically load .env settings (as defaults) on server startup

Edit: Description updated by @cguedes

codecov · 2023-08-24T19:47:12Z

Codecov Report

Merging #417 (68a0a08) into main (325c4e7) will decrease coverage by 3.16%.
Report is 1 commits behind head on main.
The diff coverage is 11.98%.

@@            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

Files Changed	Coverage Δ
python/generate_schema.py	`0.00% <0.00%> (ø)`
src/api/api-types.ts	`0.00% <0.00%> (ø)`
src/application/listeners/projectEventListeners.ts	`46.93% <0.00%> (ø)`
src/api/api.ts	`13.46% <13.46%> (ø)`
src/settings/settingsManager.ts	`45.20% <25.00%> (-41.82%)`	⬇️
src/api/server.ts	`34.53% <28.88%> (-34.77%)`	⬇️
src/AppStartup.tsx	`91.93% <66.66%> (-4.56%)`	⬇️
python/sidecar/typing.py	`97.60% <100.00%> (+0.02%)`	⬆️
src/api/sidecar.ts	`52.68% <100.00%> (ø)`
src/features/ai/settings/OpenAiSettingsPane.tsx	`100.00% <100.00%> (ø)`
... and 2 more

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

danvk

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.

danvk · 2023-08-24T20:56:21Z

+	"tasks": [
+		{
+			"type": "npm",
+			"script": "ts:check",


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!)

danvk · 2023-08-24T20:56:52Z

 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)


this was a fun one to track down 🙃

Also for me! Should have told you about this.

danvk · 2023-08-24T20:59:36Z

  useAsyncEffect(
    async (isMounted) => {
+      if (!isServerRunning) {
+        console.log('waiting for server startup');


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.

So now we will actually take advantage of the splash screen after all :-).

danvk · 2023-08-24T21:00:34Z

        if (isMounted()) {
          setInitialized(true);
-          const projectDir = getCachedSetting('project.currentDir');
+          const projectDir = getCachedSetting('project.current_directory');


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.

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.

danvk · 2023-08-24T21:01:48Z

  const command = new TauriCommand('call-sidecar', ['serve']);
  command.stderr.addListener('data', (line) => {
    console.log('server stderr', line);
+    if (line.includes('running on http')) {


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!

danvk · 2023-08-24T21:05:39Z

+      return value;
+    },
+    syncCache: async () => {
+      const newSettings = (await universalPost('/api/settings/', settings, 'PUT')) as SettingsSchema;


Note: the trailing slash is important. If you leave it off, you get confusing CORS errors from the dev server in web mode.

danvk · 2023-08-24T21:07:14Z

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.

Reminder to clean-up this in #422

danvk · 2023-08-24T21:07:47Z

I deleted these this to get tests passing, but we should update/rewrite them instead.

danvk · 2023-08-24T21:08:50Z

This papers over the differences between Tauri fetch and web fetch.

Thanks for starting this one.

danvk · 2023-08-24T21:11:12Z

+ * This interface was referenced by `ApiSchema`'s JSON-Schema
+ * via the `definition` "SettingsSchema".
+ */
+export interface SettingsSchema {


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.

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

danvk · 2023-08-25T14:24:55Z


 class OpenAISettings(RefStudioModel):
-    api_key: str = ""
+    api_key: str = os.environ.get("OPENAI_API_KEY")


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.

I've updated this to use the value "" instead of reading from os.env

danvk added 5 commits August 24, 2023 14:48

barebones codegen

238c863

Attempt univeral implementation of settings

8c050c9

Make DeepReadonly deep

7933d50

pass type checker

629853d

trailing slash is critical

4b19ca9

danvk added 8 commits August 24, 2023 16:03

implement the latch

cab85f2

knip

2625864

Fix OpenAiSettingsPane.test.tsx

08cedf9

Fix AppStartup.test.tsx

e57784a

try to load from dotenv

cafb42a

remove settings test for now

a61ac72

more codegen

a637e13

codegen

b5a06a9

danvk commented Aug 24, 2023

View reviewed changes

cguedes mentioned this pull request Aug 25, 2023

feat: Adopt HTTP apis for web deployment #393

Merged

cguedes mentioned this pull request Aug 25, 2023

Allow Ref Studio to run locally as a web app #347

Closed

Merge branch 'main' into universal-settings

48a93bd

danvk commented Aug 25, 2023

View reviewed changes

cguedes added 3 commits August 25, 2023 16:39

Don't read default API key from env

c44907c

Strongly typed universalGet and universalPost

2ddcec1

Add missing typing

68a0a08

cguedes marked this pull request as ready for review August 25, 2023 16:16

cguedes requested a review from sehyod August 25, 2023 16:16

sehyod previously approved these changes Aug 25, 2023

View reviewed changes

Comment thread src/api/api.ts Outdated

cguedes added 3 commits August 25, 2023 17:53

Remove method parameter from universalPost

c992730

Add universalPut

283ac0d

Fix yarn check

54732bc

cguedes dismissed sehyod’s stale review via 54732bc August 25, 2023 17:02

cguedes approved these changes Aug 25, 2023

View reviewed changes

cguedes merged commit 7209b2e into main Aug 25, 2023

cguedes deleted the universal-settings branch August 25, 2023 17:07

Uh oh!

Conversation

danvk commented Aug 24, 2023 • edited by cguedes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

danvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danvk commented Aug 24, 2023 •

edited by cguedes

Loading

codecov Bot commented Aug 24, 2023 •

edited

Loading