-
Notifications
You must be signed in to change notification settings - Fork 1
Stream worker output immediately via callbacks instead of buffering in arrays #149
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
Conversation
…f accumulating in arrays Co-authored-by: na-trium-144 <100704180+na-trium-144@users.noreply.github.com>
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
my-code | ace27ff | Commit Preview URL Branch Preview URL |
Dec 16 2025, 04:26 PM |
|
コールバックが良いのかReadableStreamが良いのか悩みどころ |
app/terminal/worker/jsEval.worker.ts
Outdated
| if (currentOutputCallback) { | ||
| currentOutputCallback({ type: "stdout", message: format(...args) }); | ||
| } |
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.
@copilot currentOutputCallback?.(...) のほうが短いと思います。
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.
Changed to use optional chaining currentOutputCallback?.(...) for cleaner syntax in commit 9a08803.
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.
同様のコードがある箇所を全部指摘してあげないと指摘箇所しかやらないのかこいつは
| } finally { | ||
| currentOutputCallback = null; | ||
| } |
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.
@copilot finally節を追加するならflushOutput();もfinallyに移動できます。
runFile()内についても同様
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.
Moved flushOutput() calls to finally blocks in both runCode() and runFile() functions in commit 9a08803.
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.
と思ったけど、エラーメッセージのあとにflushするのは違うな
あとでrevertする
…move flushOutput to finally Co-authored-by: na-trium-144 <100704180+na-trium-144@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Changes to stream worker output immediately
Summary
All three worker files (jsEval.worker.ts, ruby.worker.ts, pyodide.worker.ts) have been successfully refactored to stream output immediately to the calling code via callbacks instead of accumulating output in arrays. This enables real-time output streaming and reduces memory usage.
Key Changes:
onOutputcallback parameter torunCodeandrunFilemethodscurrentOutputCallbackproxy()to wrap callbacks and collect streamed outputImprovements from PR feedback:
?.())write()methods for real-time outputflushOutput()moved tofinallyblocks to ensure proper cleanupOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.