Support abort signal#1785
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: 186b20f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds AbortSignal support to StliteKernel HTTP requests by threading an optional signal through sendHttpRequest and internal _asyncPostMessage, performing pre-flight abort checks, registering/removing abort listeners during message channel lifecycle, and adding a test that asserts immediate-abort behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Caller
participant Kernel as StliteKernel
participant Channel as MessageChannel
participant Worker as Worker
rect rgb(240,248,255)
Note over User,Worker: Normal request flow
User->>Kernel: sendHttpRequest(request, signal)
Kernel->>Kernel: if signal.aborted -> reject AbortError
Kernel->>Channel: create MessageChannel
Kernel->>Kernel: register abort listener (closes channel & rejects)
Kernel->>Worker: post message with channel.port2
Worker->>Worker: perform HTTP work
Worker->>Channel: post reply on port2
Channel->>Kernel: port1 receives reply
Kernel->>Kernel: remove abort listener, resolve promise
end
rect rgb(255,240,245)
Note over User,Kernel: Abort path
User->>Kernel: sendHttpRequest(request, signal)
Kernel->>Kernel: signal.aborted or abort event fires
Kernel->>Channel: close channel
Kernel->>Kernel: remove listener
Kernel->>User: reject with AbortError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Package Stats on
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds support for AbortSignal to the kernel's HTTP request functionality, enabling callers to cancel in-flight HTTP requests. This is a common pattern for handling request cancellation in JavaScript/TypeScript applications.
Key changes:
- Added an optional
AbortSignalparameter tosendHttpRequestand the internal_asyncPostMessagemethod - Implemented abort signal handling logic to reject promises and clean up resources when requests are aborted
- Added comprehensive test coverage for the abort functionality
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| streamlit | Subproject commit update |
| packages/kernel/src/kernel.ts | Added AbortSignal parameter support to HTTP request methods with proper signal handling and cleanup |
| packages/kernel/src/kernel.abort.test.ts | New test file validating that HTTP requests can be properly aborted using AbortSignal |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/kernel/src/kernel.abort.test.ts (2)
6-32: Add SharedWorker test coverage.Based on learnings, kernel tests should cover both Dedicated Worker and SharedWorker code paths when testing kernel worker logic. The current test only uses
StliteWorker(Dedicated Worker).Consider parameterizing the test suite or adding a separate suite to test both worker types:
suite.each([ { workerType: "dedicated", createWorker: () => new StliteWorker() }, { workerType: "shared", createWorker: () => new StliteWorker({ shared: true }) } ])("StliteKernel.sendHttpRequest with AbortSignal ($workerType)", { timeout: 60 * 1000 }, ({ createWorker }) => { let kernel: StliteKernel; beforeAll(async () => { const worker = createWorker(); // ... rest of setup }); // ... tests });Based on learnings, both worker types should be tested.
34-51: Test logic looks good.The test correctly validates that
sendHttpRequestrejects when aborted via AbortSignal. The immediate abort after callingsendHttpRequesteffectively tests the synchronous abort path.Optionally, consider adding a test case for in-flight abort to verify cancellation during an actual request:
test("kernel.sendHttpRequest() should be aborted during in-flight request", async () => { const controller = new AbortController(); const signal = controller.signal; const promise = kernel.sendHttpRequest( { method: "GET", path: "/healthz", headers: {}, body: "", }, signal, ); // Abort after a small delay to ensure request is in-flight setTimeout(() => controller.abort(), 10); await expect(promise).rejects.toThrow(/Aborted/); });This would verify abort handling after the message has been posted to the worker.
packages/kernel/src/kernel.ts (1)
556-568: Argument parsing logic is correct with a minor cleanup opportunity.The logic properly handles both overload signatures. The pre-flight abort check at lines 566-568 is a good optimization that rejects immediately if the signal is already aborted.
Minor: The type assertion at line 563 is unnecessary since TypeScript can infer the type from the control flow:
- } else if (arg2) { - signal = arg2 as AbortSignal; - } + } else if (arg2) { + signal = arg2; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/kernel/src/kernel.abort.test.ts(1 hunks)packages/kernel/src/kernel.ts(3 hunks)streamlit(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{js,jsx,ts,tsx,json,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Follow Prettier formatting (2 spaces, trailing commas, semicolons by default) as enforced by the project's configuration
Files:
packages/kernel/src/kernel.tspackages/kernel/src/kernel.abort.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace ESLint configs for linting
Files:
packages/kernel/src/kernel.tspackages/kernel/src/kernel.abort.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript
Import external dependencies before workspace packages, which come before relative imports in TypeScript files
Export types with.d.tsextension and useexport typefor type-only exports
Files:
packages/kernel/src/kernel.tspackages/kernel/src/kernel.abort.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use camelCase for JavaScript/TypeScript variable and function names
Always use async/await for asynchronous operations instead of promises with.then()
Prefix unused variables with underscore (e.g.,_unused) instead of leaving them as-is
Files:
packages/kernel/src/kernel.tspackages/kernel/src/kernel.abort.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use kebab-case for utility and non-component TypeScript filenames
Files:
packages/kernel/src/kernel.tspackages/kernel/src/kernel.abort.test.ts
{.prettierrc,**/*.{md,json,ts,tsx,js,jsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier with 2-space indentation, semicolons enabled, trailing commas for ES5, and 80-character line width
Files:
packages/kernel/src/kernel.tspackages/kernel/src/kernel.abort.test.ts
packages/{kernel,browser,react,desktop}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
path-browserifyfor cross-platform path handling in browser/worker code instead of Node.jspathmodule
Files:
packages/kernel/src/kernel.tspackages/kernel/src/kernel.abort.test.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
@stlite/commonpackage for shared TypeScript types and utilities across all packages
Files:
packages/kernel/src/kernel.tspackages/kernel/src/kernel.abort.test.ts
packages/kernel/**/*.{ts,test.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Test both Dedicated Worker and SharedWorker code paths when modifying kernel worker logic
Files:
packages/kernel/src/kernel.tspackages/kernel/src/kernel.abort.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx}: Keep test specs beside components with naming conventionComponent.test.tsx
Mock Pyodide hooks with the provided fixtures in Vitest tests
Files:
packages/kernel/src/kernel.abort.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Co-locate test files with source files using
*.test.ts,*.test.tsx,*.spec.ts, or*.spec.tsxnaming convention
Files:
packages/kernel/src/kernel.abort.test.ts
packages/kernel/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
@vitest/web-workerplugin for testing Web Worker and SharedWorker code paths
Files:
packages/kernel/src/kernel.abort.test.ts
🧠 Learnings (4)
📚 Learning: 2025-12-08T07:43:57.409Z
Learnt from: CR
Repo: whitphx/stlite PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-08T07:43:57.409Z
Learning: Applies to packages/kernel/**/*.{ts,test.ts} : Test both Dedicated Worker and SharedWorker code paths when modifying kernel worker logic
Applied to files:
packages/kernel/src/kernel.abort.test.ts
📚 Learning: 2025-12-08T07:43:57.409Z
Learnt from: CR
Repo: whitphx/stlite PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-08T07:43:57.409Z
Learning: Applies to packages/kernel/**/*.test.ts : Use `vitest/web-worker` plugin for testing Web Worker and SharedWorker code paths
Applied to files:
packages/kernel/src/kernel.abort.test.ts
📚 Learning: 2025-12-08T07:43:57.409Z
Learnt from: CR
Repo: whitphx/stlite PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-08T07:43:57.409Z
Learning: Applies to packages/kernel/py/stlite-lib/**/*_test.py : Use pytest with pytest-asyncio for async Python tests in stlite-lib
Applied to files:
packages/kernel/src/kernel.abort.test.ts
📚 Learning: 2025-12-08T07:43:57.409Z
Learnt from: CR
Repo: whitphx/stlite PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-08T07:43:57.409Z
Learning: Applies to packages/browser/e2e-tests/**/*.test.ts : Use Playwright for E2E testing of browser-based Stlite applications
Applied to files:
packages/kernel/src/kernel.abort.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-sharing-editor
- GitHub Check: test-stlite-lib
- GitHub Check: test-kernel
🔇 Additional comments (4)
streamlit (1)
1-1: Missing implementation files for review.The provided file shows only a submodule commit reference update. However, according to the PR objectives and AI summary, the actual AbortSignal support implementation is in
packages/kernel/src/kernel.tsand the corresponding test inpackages/kernel/src/kernel.abort.test.ts. These files are not included in the provided review context.To conduct a thorough line-by-line review of the AbortSignal implementation, please provide:
packages/kernel/src/kernel.ts— the main implementation with signal parameter handlingpackages/kernel/src/kernel.abort.test.ts— the new test fileAlternatively, if this is a submodule-only update, please clarify:
- Whether the streamlit submodule commit update is intentional for this PR or a side effect
- Whether the kernel.ts changes are in a separate PR or in a different branch
packages/kernel/src/kernel.ts (3)
323-342: Clean API extension.The addition of the optional
signalparameter tosendHttpRequestis backwards compatible and properly threads the AbortSignal through to the internal implementation.
542-550: Overload signatures are well-designed.The updated overloads consistently place the optional
signalparameter as the final argument, maintaining backwards compatibility while enabling abort support.
570-601: Abort listener lifecycle management is well-implemented.The implementation correctly handles the AbortSignal lifecycle:
- Registers the listener with
{ once: true }to prevent duplicate invocations- Cleans up the listener when a reply is received to prevent memory leaks
- Properly closes the MessageChannel port in both abort and normal completion paths
- Uses standard
DOMExceptionwith "AbortError" nameThe test in
kernel.abort.test.tsvalidates the abort listener behavior: it registers the listener and then triggers an abort, confirming the promise rejects with an AbortError. Consider adding integration tests that verify abort behavior during longer-running HTTP request processing to ensure abort works correctly across the full request lifecycle.Likely an incorrect or invalid review comment.
Contributor
Bundle visualizer reports for
|
Contributor
|
Deployment completed successfully (log). Importable URLs:
import { StliteApp, createKernel } from "https://b2341f7d.stlite-react-preview.pages.dev/stlite.js";
import "https://b2341f7d.stlite-react-preview.pages.dev/stlite.css"; |
Contributor
|
Deployment completed successfully (log).
|
Contributor
|
Deployment completed successfully (log).
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
<title>Stlite Browser preview</title>
<link rel="stylesheet" href="https://cba4cb37.stlite-browser-preview.pages.dev/stlite.css" />
</head>
<body>
<noscript>You need to enable JavaScript to run this app.</noscript>
<div id="root"></div>
<script type="module">
import { mount } from "https://cba4cb37.stlite-browser-preview.pages.dev/stlite.js"
mount(
{
entrypoint: "streamlit_app.py",
files: {
"streamlit_app.py": `
import streamlit as st
st.write("Hello world")
`,
},
requirements: [],
},
document.getElementById("root"),
);
</script>
</body>
</html> |
Contributor
Package Stats on
|
Contributor
Bundle visualizer reports for
|
Contributor
|
Deployment completed successfully (log).
|
Contributor
|
Deployment completed successfully (log). Importable URLs:
import { StliteApp, createKernel } from "https://27c6fe13.stlite-react-preview.pages.dev/stlite.js";
import "https://27c6fe13.stlite-react-preview.pages.dev/stlite.css"; |
Contributor
|
Deployment completed successfully (log).
|
Contributor
|
Deployment completed successfully (log).
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
<title>Stlite Browser preview</title>
<link rel="stylesheet" href="https://0f5cc9c3.stlite-browser-preview.pages.dev/stlite.css" />
</head>
<body>
<noscript>You need to enable JavaScript to run this app.</noscript>
<div id="root"></div>
<script type="module">
import { mount } from "https://0f5cc9c3.stlite-browser-preview.pages.dev/stlite.js"
mount(
{
entrypoint: "streamlit_app.py",
files: {
"streamlit_app.py": `
import streamlit as st
st.write("Hello world")
`,
},
requirements: [],
},
document.getElementById("root"),
);
</script>
</body>
</html> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.