Sidecar server#407
Conversation
Codecov Report
@@ Coverage Diff @@
## main #407 +/- ##
==========================================
- Coverage 83.18% 82.98% -0.21%
==========================================
Files 183 184 +1
Lines 10648 10787 +139
Branches 1128 1144 +16
==========================================
+ Hits 8858 8952 +94
- Misses 1775 1820 +45
Partials 15 15
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
danvk
left a comment
danvk
left a comment
There was a problem hiding this comment.
Let me know your thoughts about testing. It's important code, but the test is going to involve enough mocks that I'm not sure it will really be that helpful.
| * If an existing server not managed by us is already running, it will be killed. (This can happen | ||
| * when you run Tauri Desktop in dev mode and make TypeScript changes.) | ||
| */ | ||
| export function runRefStudioServerOnDesktop() { |
There was a problem hiding this comment.
There's a possible race condition where we start the server and try to fetch something from it (maybe settings) before it's ready to respond. I'm hoping this won't come up in practice. If it is, we'll want to expose something like function waitForServer(): Promise<void> and await that before running tauriFetch. But this will introduce another difference between desktop and web (where you can assume the server is always running) so I'd really like to avoid that if we can.
There was a problem hiding this comment.
I think it's ok. AppStartup (and the splashscreen) is there to ensure we don't call any other API (or render the app) before everything is initialised.
There was a problem hiding this comment.
Yeah, I think we're going to need some kind of waiting logic. Here's what I see in the debug logs. Note that we make at least 3 API calls (init settings, ingest references, read file tree) before the HTTP server reports that it's listening:
| type ServerState = StartingState | ReadyState; | ||
|
|
||
| // A null value means that no one has requested a server yet. | ||
| let serverProcess: ServerState | null = null; |
There was a problem hiding this comment.
Lots of ceremony here but I really want to make sure that multiple calls to getOrStartServer don't result in multiple servers being started.
cguedes
left a comment
cguedes
left a comment
There was a problem hiding this comment.
Just left a comment about a typing definition.
I agree that making (unit) tests for this would be more complex than the value we get. This is something we can test quickly as we build e2e tests for the app!
| type ServerState = StartingState | ReadyState; | ||
|
|
||
| // A null value means that no one has requested a server yet. | ||
| let serverProcess: ServerState | null = null; |
2 participants
See #406
This PR adds a
servesubcommand to the Python sidecar that runs a web server. Eventually this should be the only command, but for now it coexists with existing sidecar commands. This will let us share much more code between the web and desktop versions of RefStudio.When the app starts, it uses
Command.spawnto run theservecommand, starting a web server. This server's port is in the allowlist intauri.conf.jsonwhich allows us to connect to it using Tauri's version offetch.A few notes:
/api/meta/shutdownendpoint.Forwarded logs from the server:
Killing the server triggers a restart, after which the API is still usable:
kill-and-restart.mov