feat: Adopt HTTP apis for web deployment by cguedes · Pull Request #393 · refstudio/refstudio

cguedes · 2023-08-22T14:54:18Z

Part of work for #347 and closes #381

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)

codecov · 2023-08-22T14:59:51Z

Codecov Report

Merging #393 (890d347) into main (dfd2a6d) will increase coverage by 1.16%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main     #393      +/-   ##
==========================================
+ Coverage   82.65%   83.81%   +1.16%     
==========================================
  Files         186       17     -169     
  Lines       10884     1168    -9716     
  Branches     1142        0    -1142     
==========================================
- Hits         8996      979    -8017     
+ Misses       1873      189    -1684     
+ Partials       15        0      -15

Files Changed	Coverage Δ
python/web.py	`0.00% <0.00%> (ø)`
python/sidecar/http.py	`81.92% <100.00%> (ø)`

... and 168 files with indirect coverage changes

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

…-with-the-new-http-backend-api

danvk

A few comments for you. In general we should try to avoid as much "adapter" code as we can since both the server and client are under our control. Rather than adapting the server response in client code, just change the server to return what we want.

danvk · 2023-08-24T15:33:07Z

+
+export async function getRemoteProject(projectId: string) {
+  const res = await fetch(`/api/projects/${projectId}`);
+  const data: ProjectIdResponse = res.json() as unknown as ProjectIdResponse;


We should adapt the codegen setup before we go too far down this path. I can take a crack at that.

You are right. We need to iron the projects API in line with what we have discussed early (ex: stop exposing the internal server path). Created a ticket for that here.

danvk · 2023-08-24T15:33:52Z

+    },
+  });
+  const payload = (await response.json()) as Record<string, string>;
+  const projectId = Object.keys(payload)[0];


Let's update the Python API to produce the response type that we want.

Same comment as above.

danvk · 2023-08-24T15:37:03Z

+export async function readProjectFilesFromWeb(projectId: string) {
+  const projectInfo = await getRemoteProject(projectId);
+  const relativePaths = projectInfo.filepaths
+    .map((path) => path.replace(projectInfo.project_path + '/', '')) // Remove project path from API output


should this logic all be on the python side or here?

Yes, something for #414

danvk · 2023-08-24T15:37:38Z

+// ########################################################################################
+// FILESYSTEM
+// ########################################################################################
+export const writeTextFileFromWeb = async (projectId: string, filePath: string, contents: string) => {


export const writeTextFileFromWeb = writeRemoteTextFile;

Thanks. In the end maybe I won't have the ...FromWeb and ...Remote... versions. So I will keep this in mind.

danvk · 2023-08-24T15:51:21Z

-      .filter((x) => !!x)
-      .join('/')) as NormalizedPath;
-}
-const fs = new Map<NormalizedPath, FakeNode>();


Very happy that this code had such a short life :)

:-) me too.

danvk · 2023-08-24T15:52:10Z

-export const readBinaryFile: typeof tauriFs.readBinaryFile = async (filePath) => {
-  const f = await readTextFile(filePath);
-  return new TextEncoder().encode(f);
+/** MIGRATED */


If this needs to stay in, you can use @deprecated to make it appear struck-through as a reminder at call sites.

I'm sure we will remove this :-) very soon.

danvk · 2023-08-24T15:56:02Z

-  return Promise.all(entries.map(convertTauriFileEntryToFileEntry));
+  if (isRunningOnWeb()) {
+    console.log('reading file structure from web');
+    const entries = await readProjectFilesFromWeb(currentProjectId);


Try to factor out as much shared code as you can, i.e.

let entries; if (isRunningOnWeb()) { entries = await readProjectFilesFromWeb(currentProjectId); } else { entries = await readDir(systemBaseDir, { recursive: true }); } return Promise.all(entries.map(convertTauriFileEntryToFileEntry));

the appeal of the tauri-api-stubs approach was that it naturally avoided this kind of duplication.

Right. This time I've prioritised making the two code paths clear. I think that this will very soon be the same code for desktop and web.

danvk · 2023-08-24T15:59:33Z

 }

+let currentProjectId = '';
+export function setCurrentProjectId(projectId: string) {


This is duplicative of the atom state. We should either pass the project ID to the filesystem functions or, if that involves too much repetition, we could create a FilesystemForProject class that takes it once in the constructor.

Sharp eyes @danvk :-).

I wasn't sure (still not 100% sure) how to make the current project id available throughout the app.
I started with storing the id in the project store, but then figured out that for the filesystem calls would be much simpler to just save the currentProjectId and use it for the "web".

With the "unification" of desktop and web I think that this will be cleaner in your next review.

danvk · 2023-08-24T16:00:29Z

 # -----------
 @sidecar_api.post("/ingest")
-async def http_ingest(project_id: str) -> IngestResponse:
+async def http_ingest(project_id: str, request: Request) -> IngestResponse:


why do these handlers all need to take a request now?

They needed before to set env vars from the headers. Now that code was moved to a middleware and I haven't removed the unused parameters. You can see that change here.

Shouldn't this be caught by the python linter? (during yarn check and git push)

…-with-the-new-http-backend-api

sehyod · 2023-08-25T11:07:59Z

 # -----------
 @sidecar_api.post("/rewrite")
-async def http_rewrite(req: RewriteRequest) -> RewriteResponse:
+async def http_rewrite(req: RewriteRequest, request: Request) -> RewriteResponse:


What is the second request parameter and why do we need it? It seems we don't use it anyway

Good catch. We don't need it anymore. Removing....

sehyod · 2023-08-25T11:27:02Z

-  const response = await callSidecar('ingest', { pdf_directory: String(uploadsDir) });
-  return parsePdfIngestionResponse(response);
+export async function runPDFIngestion(projectId?: string): Promise<ReferenceItem[]> {
+  if (import.meta.env.VITE_IS_WEB) {


I don't really like having to make this check in each function. I would rather define an IngestionAPI abstract interface, with the methods we need, and then create 2 implementations WebIngestionAPI and SidecarIngestionAPI of this interface and export the correct implementation depending on VITE_IS_WEB

I haven't implement that bc we will remove the calls via sidecar for the references/ingestion process. Maybe I should make them into this branch actually!

sehyod · 2023-08-25T11:38:15Z

+export async function runPDFIngestion(projectId?: string): Promise<ReferenceItem[]> {
+  if (import.meta.env.VITE_IS_WEB) {
+    if (!projectId) {
+      throw new Error('Project ID is required for web version');


Couldn't we make the parameter mandatory instead? The frontend should not depend on the implementation and should send an id in any case, so it's not useful to make it optional, even if we don't use it in the sidecar implementation

Yes. That will be implemented in the follow-up PR with the unified desktop+web code to use HTTP.

sehyod · 2023-08-25T11:43:54Z

-      : await save({ defaultPath: await getNewProjectsBaseDir() });
-    if (typeof newProjectPath === 'string') {
-      await newProject(newProjectPath);
+    if (import.meta.env.VITE_IS_WEB) {


nit: Maybe this logic shouldn't live in this file

…-with-the-new-http-backend-api

cguedes · 2023-08-25T12:48:56Z



-@references_api.put("/{project_id}/{reference_id}")
+@references_api.patch("/{project_id}/{reference_id}")


@gjreda FYI I've updated this to PATCH.

cguedes · 2023-08-25T13:04:13Z

-export const writeBinaryFile = import.meta.env.VITE_IS_WEB ? stubFs.writeBinaryFile : tauriFs.writeBinaryFile;
-export const renameFile = import.meta.env.VITE_IS_WEB ? stubFs.renameFile : tauriFs.renameFile;
+export const createDir = import.meta.env.VITE_IS_WEB
+  ? dontUseFsInWeb<ReturnType<typeof tauriFs.createDir>>


This is just a sanity check. We will remove this in the follow-up PR.

cguedes · 2023-08-25T13:05:23Z

+  const file = new File([content], fileName, { type: 'text/plain' });
+  data.append('file', file);
+
+  await fetch(`/api/fs/${projectId}/${filePath}`, {


You are right. As this PR was exploratory, I didn't implemented those validations. Will make sure we have an issue to fix this within the context of #302 .

cguedes · 2023-08-25T13:06:20Z

+// -------------
+// Web
+// -------------
+export const newWebProjectAtom = atom(null, async (_, set, projectId: string, path: string, name: string) => {


This will be unified with the desktop version in the follow-up PR.

cguedes added 3 commits August 22, 2023 15:50

Send env variables via header "X-..." in the HTTP sidecar interface

d0dc6eb

Adds a projectsAPI to create new projects in the Web

ff1fdf7

Support creating new projects for the Web

3fb0059

cguedes linked an issue Aug 22, 2023 that may be closed by this pull request

Reimplement flow for PDF uploads to work with the new HTTP backend API #381

Closed

cguedes commented Aug 22, 2023

View reviewed changes

Comment thread python/sidecar/http.py Outdated

cguedes added 3 commits August 22, 2023 16:04

Fix py lint

4dc7921

WIP: Adopt fs api

99c8a28

Merge branch 'main' into 381-reimplement-flow-for-pdf-uploads-to-work…

515fac9

…-with-the-new-http-backend-api

cguedes changed the title ~~feat: Reimplement flow for pdf uploads to work with the new http backend api 🚧~~ 🚧 feat: Reimplement flow for pdf uploads to work with the new http backend api Aug 23, 2023

cguedes added 8 commits August 23, 2023 11:57

Merge branch 'main' into 381-reimplement-flow-for-pdf-uploads-to-work…

47c3454

…-with-the-new-http-backend-api

Merge branch 'main' into 381-reimplement-flow-for-pdf-uploads-to-work…

4ea5347

…-with-the-new-http-backend-api

Merge branch 'main' into 381-reimplement-flow-for-pdf-uploads-to-work…

becbbfa

…-with-the-new-http-backend-api

Fix settings API import

b474daf

handle env variable with middleware

5c26167

Add web.ts with api calls to backend server

63d7263

Remove outdated tests

236f25c

Fix yarn check issues

bd4121e

danvk reviewed Aug 24, 2023

View reviewed changes

cguedes mentioned this pull request Aug 24, 2023

Improve the projects API (output payload and types) #414

Closed

cguedes added 10 commits August 24, 2023 22:24

Merge branch 'main' into 381-reimplement-flow-for-pdf-uploads-to-work…

67d457d

…-with-the-new-http-backend-api

Directly use !!import.meta.env.VITE_IS_WEB

3a9e9e4

Configure project name via API

a0422ae

Adopt HTTP ingestion for web

7704cca

Add support to delete references

1d4a860

remove app startup utility

6b0efcc

Open one of the existing projects instead of open hard-coded

f497f3c

Allow references to be edited (PATCH)

31864ec

Add support to chat with references via HTTP web

4481912

Remove legacy fs.ts and stub

2c5aced

cguedes added 4 commits August 25, 2023 11:46

Fix unit tests

362c047

fix python tests

8e1df4a

Fix tauri-wrapper signature

95f2237

Fix yarn:check errors

8f3467f

sehyod requested changes Aug 25, 2023

View reviewed changes

cguedes added 2 commits August 25, 2023 13:37

Fix test

5fdd31d

Remove unused args

d730eb4

cguedes changed the title ~~🚧 feat: Reimplement flow for pdf uploads to work with the new http backend api~~ 🚧 feat: Adopt HTTP apis for web deployment Aug 25, 2023

Merge branch 'main' into 381-reimplement-flow-for-pdf-uploads-to-work…

890d347

…-with-the-new-http-backend-api

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

cguedes commented Aug 25, 2023

View reviewed changes

sehyod approved these changes Aug 25, 2023

View reviewed changes

cguedes changed the title ~~🚧 feat: Adopt HTTP apis for web deployment~~ feat: Adopt HTTP apis for web deployment Aug 25, 2023

cguedes merged commit 325c4e7 into main Aug 25, 2023

cguedes deleted the 381-reimplement-flow-for-pdf-uploads-to-work-with-the-new-http-backend-api branch August 25, 2023 13:36

cguedes mentioned this pull request Aug 25, 2023

Universal Desktop + Web codebase #422

Closed



		@references_api.put("/{project_id}/{reference_id}")
		@references_api.patch("/{project_id}/{reference_id}")

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Aug 22, 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cguedes commented Aug 22, 2023 •

edited

Loading

codecov Bot commented Aug 22, 2023 •

edited

Loading