Conversation
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR moves the webapp’s Pyodide execution off the main UI thread into a dedicated Web Worker, replacing direct Pyodide/PyProxy usage in React with a typed, promise-based request/response API. It also updates bundling/docs to stop relying on a global loadPyodide() script tag and instead load Pyodide from the CDN inside the worker.
Changes:
- Introduce a
PyodideClientthat communicates with a dedicated worker for prepare/run/generate-HTML operations. - Add a new
pyodide-worker.tsmodule that loads Pyodide (via CDN) and runs repo-review logic inside the worker. - Update build/docs/html entrypoints to remove the Pyodide global script requirement and emit the worker bundle.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/repo-review-app/utils/pyodide.ts | Replaces direct Pyodide calls with a worker-backed PyodideClient RPC wrapper. |
| src/repo-review-app/utils/pyodide-worker.ts | New worker implementation that loads Pyodide and runs repo-review tasks. |
| src/repo-review-app/utils/pyodide-common.ts | Shared request/response + data types for the UI↔worker protocol. |
| src/repo-review-app/repo-review-app.tsx | Switches the UI to use PyodideClient and removes PyProxy lifecycle handling. |
| src/repo-review-app/index.html | Removes the global Pyodide <script> tag. |
| package.json | Updates build steps to emit the worker bundle alongside the app. |
| docs/webapp.md | Updates integration notes to reflect worker-based Pyodide loading. |
| docs/live-demo.md | Removes the Pyodide <script> tag from the demo page. |
| .gitignore | Ignores the emitted minified worker bundle under docs static assets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| workerScope.addEventListener( | ||
| "message", | ||
| (event: MessageEvent<WorkerRequest>) => { | ||
| void handleRequest(event.data); | ||
| }, |
There was a problem hiding this comment.
handleRequest() is invoked without awaiting/serializing, so multiple messages can run concurrently in the same worker. Since all requests share a single Pyodide interpreter (and mutate shared globals like deps_for_install and _repo_review_last_*), overlapping runPythonAsync/runPython calls can race or fail (Pyodide is generally not re-entrant). Consider introducing a single request/operation queue (similar to prepareQueue) and chaining all request handlers through it so only one Pyodide operation executes at a time.
| "build": "bunx esbuild src/repo-review-app/repo-review-app.tsx --bundle --minify --sourcemap --outfile=docs/_static/scripts/repo-review-app.min.js --platform=browser --format=esm", | ||
| "build-html": "bun build src/repo-review-app/index.html --outdir=out --minify", | ||
| "build": "bunx esbuild src/repo-review-app/repo-review-app.tsx src/repo-review-app/utils/pyodide-worker.ts --bundle --minify --sourcemap --outdir=docs/_static/scripts --outbase=src/repo-review-app --entry-names=[dir]/[name].min --platform=browser --format=esm", | ||
| "build-html": "bun build src/repo-review-app/index.html --outdir=out --minify && bunx esbuild src/repo-review-app/utils/pyodide-worker.ts --bundle --minify --sourcemap --outdir=out/utils --platform=browser --entry-names=[dir]/[name].min --format=esm", |
There was a problem hiding this comment.
The build-html esbuild command uses --outdir=out/utils together with --entry-names=[dir]/[name].min but does not set --outbase. With esbuild defaults, [dir] will include src/repo-review-app/utils, so the worker will likely be emitted under a nested path like out/utils/src/repo-review-app/utils/pyodide-worker.min.js, while the runtime code resolves the worker as ./utils/pyodide-worker.min.js next to the main bundle. Adjust the command to set an appropriate --outbase (or change --entry-names / --outdir) so the emitted worker path matches what getWorkerUrl() expects.
| "build-html": "bun build src/repo-review-app/index.html --outdir=out --minify && bunx esbuild src/repo-review-app/utils/pyodide-worker.ts --bundle --minify --sourcemap --outdir=out/utils --platform=browser --entry-names=[dir]/[name].min --format=esm", | |
| "build-html": "bun build src/repo-review-app/index.html --outdir=out --minify && bunx esbuild src/repo-review-app/utils/pyodide-worker.ts --bundle --minify --sourcemap --outdir=out/utils --outbase=src/repo-review-app --platform=browser --entry-names=[dir]/[name].min --format=esm", |
This was started with copilot in VSCode, auto model.
The first commit works with
bundle serve, but with a more complex blob API. The later comments should work most places except bun serve, and require a new file.