Conversation
|
""" WalkthroughA new utility class, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AppController
participant BackgroundTaskHelper
participant ReadableStream
participant Middleware
Client->>AppController: HTTP GET /stream or /error_stream
AppController->>BackgroundTaskHelper: (prepare data)
AppController->>ReadableStream: Create stream from async generator
AppController-->>Client: Streamed HTML response
Middleware->>ReadableStream: Listen for 'close' or 'error' event
ReadableStream-->>Middleware: Emits 'close' or 'error'
Middleware->>Middleware: Destroy context after stream ends or errors
Possibly related issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
plugin/tegg/lib/ctx_lifecycle_middleware.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
core/common-util/src/StreamUtil.ts (3)
8-14: Refactor static methods to use class name instead of 'this'The static analysis tool flags the use of
thisin static methods as potentially confusing. Using the class name is more explicit.Apply this refactor to make the code more maintainable:
static isStream(obj: any): boolean { return obj !== null && typeof obj === 'object' && typeof obj.pipe === 'function' && typeof obj.on === 'function' && obj instanceof EventEmitter; } static isReadableStream(obj: any): boolean { - return this.isStream(obj) && + return StreamUtil.isStream(obj) && typeof (obj as Readable).read === 'function' && typeof (obj as Readable).readable === 'boolean'; }Apply the same pattern to all other static methods.
Also applies to: 19-23, 28-32, 37-41, 46-50, 55-59, 70-74, 83-87, 124-128
135-141: Remove debug console.log statementThere's a console.log statement in the
handleEndfunction which should be removed before production.const handleEnd = () => { - console.log('handle end'); stream.removeListener('end', handleEnd); stream.removeListener('error', handleEnd); stream.removeListener('close', handleEnd); callback(); };
144-146: Consider using.once()instead of.on()for event handlersSince each event should only trigger the callback once, using
.once()is more appropriate and prevents potential memory leaks if the stream emits multiple events.// 添加事件监听器 - stream.on('end', handleEnd); - stream.on('error', handleEnd); - stream.on('close', handleEnd); + stream.once('end', handleEnd); + stream.once('error', handleEnd); + stream.once('close', handleEnd);plugin/tegg/lib/ctx_lifecycle_middleware.ts (1)
27-32: Function name has a typo: "doDestory" should be "doDestroy".The implementation of the error handling is good, but the function name contains a spelling error.
-function doDestory() { +function doDestroy() {And remember to update the function name references in the code below.
plugin/controller/test/fixtures/apps/http-inject-app/app/controller/AppController.ts (1)
48-66: Good implementation of streaming HTTP response.The stream implementation:
- Creates an async generator that yields HTML content in chunks with delays
- Sets appropriate headers for streaming content
- Properly uses
Readable.from()to create a stream from the generatorConsider removing the console.log statement on line 54 unless it's needed for debugging purposes.
- console.log('generate', i);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/common-util/index.ts(1 hunks)core/common-util/src/StreamUtil.ts(1 hunks)core/controller-decorator/test/fixtures/HTTPFooController.ts(1 hunks)plugin/controller/test/fixtures/apps/http-inject-app/app/controller/AppController.ts(3 hunks)plugin/controller/test/http/request.test.ts(1 hunks)plugin/tegg/lib/ctx_lifecycle_middleware.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
plugin/tegg/lib/ctx_lifecycle_middleware.ts (1)
core/common-util/src/StreamUtil.ts (1)
StreamUtil(4-148)
plugin/controller/test/fixtures/apps/http-inject-app/app/controller/AppController.ts (2)
plugin/controller/test/fixtures/apps/http-inject-app/app/middleware/count_mw.ts (1)
countMw(6-9)core/types/controller-decorator/model/types.ts (1)
EggContext(3-3)
🪛 Biome (1.9.4)
core/common-util/src/StreamUtil.ts
[error] 21-21: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 41-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 49-49: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 49-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 60-60: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 75-75: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 88-88: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 132-132: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 GitHub Check: Runner-macos (20)
core/common-util/src/StreamUtil.ts
[failure] 95-95:
'handleFinish' was used before it was defined
🪛 GitHub Check: Runner-ubuntu (20)
core/common-util/src/StreamUtil.ts
[failure] 95-95:
'handleFinish' was used before it was defined
🪛 GitHub Check: Runner-ubuntu (18)
core/common-util/src/StreamUtil.ts
[failure] 95-95:
'handleFinish' was used before it was defined
🪛 GitHub Check: Runner-macos (16)
core/common-util/src/StreamUtil.ts
[failure] 95-95:
'handleFinish' was used before it was defined
🪛 GitHub Check: Runner-macos (18)
core/common-util/src/StreamUtil.ts
[failure] 95-95:
'handleFinish' was used before it was defined
🪛 GitHub Check: Runner-ubuntu (16)
core/common-util/src/StreamUtil.ts
[failure] 95-95:
'handleFinish' was used before it was defined
🪛 GitHub Check: Runner-ubuntu (22)
core/common-util/src/StreamUtil.ts
[failure] 95-95:
'handleFinish' was used before it was defined
🪛 GitHub Actions: Continuous integration
core/common-util/src/StreamUtil.ts
[error] 95-95: ESLint: 'handleFinish' was used before it was defined. (@typescript-eslint/no-use-before-define)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (8)
core/controller-decorator/test/fixtures/HTTPFooController.ts (1)
9-9: Consistent vertical spacingAdding this blank line improves code readability by providing consistent spacing between function definitions.
core/common-util/src/StreamUtil.ts (1)
1-148: New StreamUtil class implements comprehensive stream handling capabilitiesThis utility class provides robust methods for stream type checking, error handling, and lifecycle management - addressing the core issue in the PR where contexts are destroyed prematurely before streams are fully consumed.
🧰 Tools
🪛 Biome (1.9.4)
[error] 21-21: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 41-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 49-49: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 49-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 60-60: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 75-75: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 88-88: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 132-132: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🪛 GitHub Check: Runner-macos (20)
[failure] 95-95:
'handleFinish' was used before it was defined🪛 GitHub Check: Runner-ubuntu (20)
[failure] 95-95:
'handleFinish' was used before it was defined🪛 GitHub Check: Runner-ubuntu (18)
[failure] 95-95:
'handleFinish' was used before it was defined🪛 GitHub Check: Runner-macos (16)
[failure] 95-95:
'handleFinish' was used before it was defined🪛 GitHub Check: Runner-macos (18)
[failure] 95-95:
'handleFinish' was used before it was defined🪛 GitHub Check: Runner-ubuntu (16)
[failure] 95-95:
'handleFinish' was used before it was defined🪛 GitHub Check: Runner-ubuntu (22)
[failure] 95-95:
'handleFinish' was used before it was defined🪛 GitHub Actions: Continuous integration
[error] 95-95: ESLint: 'handleFinish' was used before it was defined. (@typescript-eslint/no-use-before-define)
core/common-util/index.ts (1)
12-12: LGTM: StreamUtil export addedThe StreamUtil class is now properly exported from the barrel file, making it accessible throughout the project.
plugin/tegg/lib/ctx_lifecycle_middleware.ts (2)
4-4: Good addition of StreamUtil import.This import is essential for implementing the fix to prevent premature context destruction when handling stream responses.
36-40: Great fix for stream handling.This change correctly addresses the issue by deferring context destruction until after the stream is fully consumed. The implementation:
- Checks if the response body is a stream
- If it is, waits for the stream to end before destroying the context
- If not, destroys the context immediately
This prevents potential resource leaks and errors when streaming responses.
plugin/controller/test/fixtures/apps/http-inject-app/app/controller/AppController.ts (3)
2-2: Good addition of required imports.The imports for
setTimeout,Tracer, andReadableare appropriately added to support the new streaming functionality.Also applies to: 15-16
13-13: Import statement updated correctly for Inject and BackgroundTaskHelper.The necessary dependencies are properly imported to support the new functionality.
23-27: Proper use of dependency injection for required services.The
@Inject()decorator is used correctly to inject the required dependencies:tracerandbackgroundTaskHelper.
| it('stream should work', async () => { | ||
| await app.httpRequest() | ||
| .get('/apps/stream') | ||
| .expect(200) | ||
| .expect(res => { | ||
| console.log(res.text); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to the stream test
The test successfully verifies that the stream endpoint returns a 200 status code, but it would be more robust if it also verified some aspects of the response content beyond just logging it.
Consider enhancing the test with assertions:
it('stream should work', async () => {
await app.httpRequest()
.get('/apps/stream')
.expect(200)
.expect(res => {
- console.log(res.text);
+ assert(res.text.includes('<!DOCTYPE html>'), 'Response should contain HTML');
+ assert(res.text.length > 0, 'Response should not be empty');
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('stream should work', async () => { | |
| await app.httpRequest() | |
| .get('/apps/stream') | |
| .expect(200) | |
| .expect(res => { | |
| console.log(res.text); | |
| }); | |
| }); | |
| it('stream should work', async () => { | |
| await app.httpRequest() | |
| .get('/apps/stream') | |
| .expect(200) | |
| .expect(res => { | |
| assert(res.text.includes('<!DOCTYPE html>'), 'Response should contain HTML'); | |
| assert(res.text.length > 0, 'Response should not be empty'); | |
| }); | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/common-util/src/StreamUtil.ts (2)
19-41: Consider replacingthiswith the explicit class name to satisfy lint rulenoThisInStatic.All helpers use
this.*inside static methods (e.g.this.isStream(obj)).
Biome flags this as “confusing” (noThisInStatic). While it works, swapping to the
explicit class (StreamUtil.isStream) quiets the warning and makes the call‑site
crystal‑clear to readers who may not realisethisresolves to the constructor in
static context.- return this.isStream(obj) && + return StreamUtil.isStream(obj) &&You can apply the same substitution in
isWritableStream,isDuplexStream,
isTransformStream, etc., or disable the rule locally (// biome-ignore lint/complexity/noThisInStatic) if you prefer the current style.🧰 Tools
🪛 Biome (1.9.4)
[error] 21-21: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
124-147: Remove debugconsole.logand register listeners with.once.
console.log('handle end')is a stray debug line that will pollute stdout in
production.- The method currently attaches three permanent listeners with
stream.on.
Although they are removed insidehandleEnd, it is simpler and safer to use
.once, letting Node handle automatic removal and eliminating any chance of
duplicate callbacks if the stream emits multiple events in quick succession.- const handleEnd = () => { - console.log('handle end'); - stream.removeListener('end', handleEnd); - stream.removeListener('error', handleEnd); - stream.removeListener('close', handleEnd); - callback(); - }; - - // 添加事件监听器 - stream.on('end', handleEnd); - stream.on('error', handleEnd); - stream.on('close', handleEnd); + const handleEnd = () => callback(); + + // 使用 once 确保只调用一次并自动清理监听器 + stream.once('end', handleEnd); + stream.once('error', handleEnd); + stream.once('close', handleEnd);These tweaks keep production logs clean and make the listener lifecycle more
robust.🧰 Tools
🪛 Biome (1.9.4)
[error] 132-132: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/common-util/src/StreamUtil.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
core/common-util/src/StreamUtil.ts
[error] 21-21: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 41-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 49-49: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 49-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 60-60: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 75-75: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 88-88: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 132-132: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (javascript)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (18)
core/common-util/src/StreamUtil.ts
Outdated
| const handler = errorHandler || ((error: Error) => { | ||
| console.error('Stream error:', error); | ||
| }); | ||
|
|
||
| stream.on('error', handler); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Attach the error listener with .once to prevent accidental leak of handlers.
handleStreamError unconditionally registers an error listener with stream.on.
If a caller invokes this helper many times on the same stream (e.g. in a
framework’s middleware chain), each call adds another listener, risking the
Node.js “MaxListenersExceededWarning” and a small memory leak.
- stream.on('error', handler);
+ stream.once('error', handler);Using .once guarantees automatic removal after the first emission and removes
the burden from callers to clean up.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handler = errorHandler || ((error: Error) => { | |
| console.error('Stream error:', error); | |
| }); | |
| stream.on('error', handler); | |
| } | |
| const handler = errorHandler || ((error: Error) => { | |
| console.error('Stream error:', error); | |
| }); | |
| stream.once('error', handler); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/common-util/src/StreamUtil.ts (1)
31-33: Consider using.once()instead of.on()for event listeners.Using
.once()would automatically remove the listener after it fires once, which is safer and could prevent potential memory leaks if cleanup code doesn't execute for some reason.- stream.on('end', handleEnd); - stream.on('error', handleEnd); - stream.on('close', handleEnd); + stream.once('end', handleEnd); + stream.once('error', handleEnd); + stream.once('close', handleEnd);Though your current implementation does manually remove listeners, using
.once()provides an extra layer of safety.
🧹 Nitpick comments (2)
core/common-util/src/StreamUtil.ts (2)
4-35: Overall implementation is solid, but consider using namespaced functions instead of static class methods.The
StreamUtilimplementation correctly handles stream detection and end-of-stream notification. However, the static analysis tool suggests using simple namespaced functions instead of a class with only static methods, which is generally considered a better practice in TypeScript/JavaScript.-export class StreamUtil { - static isStream(obj: any): boolean { +export const StreamUtil = { + isStream(obj: any): boolean {This pattern makes the utility more aligned with functional programming principles while maintaining the same namespace organization.
🧰 Tools
🪛 Biome (1.9.4)
[error] 4-35: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 14-14: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
18-18: Consider a more type-safe approach for checking thedestroyedproperty.Casting to
anybypasses TypeScript's type checking. If thedestroyedproperty is expected on certain stream types, consider using a more specific type or interface.- if ((stream as any).destroyed) { + // Option 1: Type guard approach + if ('destroyed' in stream && stream.destroyed) {This approach maintains type safety while still checking for the property.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/common-util/src/StreamUtil.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
core/common-util/src/StreamUtil.ts
[error] 4-35: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 14-14: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (20)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
🔇 Additional comments (3)
core/common-util/src/StreamUtil.ts (3)
1-3: Well-structured imports using the Node.js protocol-style imports.Good use of the
node:prefix for Node.js core modules, which helps distinguish core modules from third-party ones.
5-11: Thorough stream detection implementation.The
isStreammethod properly checks for the essential characteristics of a Node.js stream (pipe method, on method, and EventEmitter inheritance).
23-29: Good cleanup implementation in the event handler.The handler properly removes all listeners before executing the callback, which prevents memory leaks and ensures the callback is only called once.
|
@coderabbitai 建一个 issue 跟进此 pr 会 pick 到 next 分支 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 704 @fengmk2 我已经创建了一个用于跟进此 PR 会被 cherry-pick 到 next 分支的 issue。 Issue 已成功创建：#304 该 issue 包含了 PR #302 的相关信息，并已将您设为负责人。 (_/) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/common-util/src/StreamUtil.ts (1)
9-9: Replacethiswith class name in static context.Using
thisin a static context can be confusing as it refers to the class itself, not an instance. It's clearer to use the class name directly.- if (!this.isStream(stream)) { + if (!StreamUtil.isStream(stream)) {🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🧹 Nitpick comments (2)
core/common-util/src/StreamUtil.ts (2)
3-6: Consider using imported type-checking utilities instead of custom implementation.Your
isStreamimplementation is correct, but consider using established type-checking libraries likeis-streamor the core Node.jstypes.isStream()utility for better maintainability and edge case handling.-import { Stream } from 'node:stream'; +import { Stream } from 'node:stream'; +import isStream from 'is-stream'; export class StreamUtil { static isStream(obj: any): boolean { - return obj instanceof Stream; + return isStream(obj); }🧰 Tools
🪛 Biome (1.9.4)
[error] 3-29: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
3-29: Consider refactoring from class to simple utility functions.The static analysis suggests avoiding classes that contain only static members. Consider refactoring to simple exported functions for better maintainability.
import { Stream } from 'node:stream'; -export class StreamUtil { - static isStream(obj: any): boolean { - return obj instanceof Stream; - } +export function isStream(obj: any): boolean { + return obj instanceof Stream; +} - static untilEnd(stream: Stream, callback: () => void): void { - if (!this.isStream(stream)) { - throw new Error('Invalid stream object'); - } +export function untilEnd(stream: Stream, callback: () => void): void { + if (!isStream(stream)) { + throw new Error('Invalid stream object'); + } - if ((stream as any).destroyed) { - callback(); - return; - } + if ((stream as any).destroyed) { + callback(); + return; + } - const handleEnd = () => { - stream.removeListener('end', handleEnd); - stream.removeListener('error', handleEnd); - stream.removeListener('close', handleEnd); - callback(); - }; + const handleEnd = () => { + stream.removeListener('end', handleEnd); + stream.removeListener('error', handleEnd); + stream.removeListener('close', handleEnd); + callback(); + }; - stream.on('end', handleEnd); - stream.on('error', handleEnd); - stream.on('close', handleEnd); - } -} + stream.once('end', handleEnd); + stream.once('error', handleEnd); + stream.once('close', handleEnd); +}🧰 Tools
🪛 Biome (1.9.4)
[error] 3-29: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 9-9: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/common-util/src/StreamUtil.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
core/common-util/src/StreamUtil.ts
[error] 3-29: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 9-9: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (16)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
🔇 Additional comments (2)
core/common-util/src/StreamUtil.ts (2)
13-16: LGTM! Good handling of already destroyed streams.Checking for
destroyedproperty and calling the callback immediately is a clean optimization that prevents unnecessary event handler registration.
18-23: LGTM! Proper event cleanup in handleEnd.The handleEnd function correctly removes all listeners before executing the callback, which prevents memory leaks.
core/common-util/src/StreamUtil.ts
Outdated
| stream.on('end', handleEnd); | ||
| stream.on('error', handleEnd); | ||
| stream.on('close', handleEnd); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use .once() instead of .on() for event listeners.
Using .on() can lead to memory leaks if untilEnd is called multiple times on the same stream. Since you intend for these handlers to execute only once, use .once() instead.
- stream.on('end', handleEnd);
- stream.on('error', handleEnd);
- stream.on('close', handleEnd);
+ stream.once('end', handleEnd);
+ stream.once('error', handleEnd);
+ stream.once('close', handleEnd);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stream.on('end', handleEnd); | |
| stream.on('error', handleEnd); | |
| stream.on('close', handleEnd); | |
| stream.once('end', handleEnd); | |
| stream.once('error', handleEnd); | |
| stream.once('close', handleEnd); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/common-util/src/StreamUtil.ts (1)
4-6: 💡 Verification agent🧩 Analysis chain
Consider using an existing stream detection library.
A previous comment mentioned "is.stream" which suggests there might be existing utilities for this purpose. Consider using established libraries like
is-streamnpm package instead of implementing this functionality from scratch for better robustness and maintenance:-import { Stream } from 'node:stream'; +import isStream from 'is-stream'; export class StreamUtil { static isStream(obj: any): boolean { - return obj instanceof Stream; + return isStream(obj); } }The
is-streampackage handles edge cases like stream-like objects that may not directly inherit from Node's Stream class.
🌐 Web query:
What is the is-stream npm package for Node.js and how does it detect streams?💡 Result:
Overview of the is-stream npm Package
The
is-streamnpm package is a utility for Node.js that allows developers to check if a given value is a Node.js stream. It provides simple APIs to detect various stream types, such as generic streams, readable streams, writable streams, duplex streams, and transform streams[1].Key Features
- isStream(stream, options?): Returns a boolean indicating whether the input is any type of Node.js stream.
- isWritableStream(stream, options?): Checks if the input is a writable stream (such as
stream.Writable,http.OutgoingMessage, etc.).- isReadableStream(stream, options?): Checks if the input is a readable stream (e.g.,
stream.Readable,http.IncomingMessage).- isDuplexStream(stream, options?): Checks for duplex streams.
- isTransformStream(stream, options?): Checks for transform streams.
- Options: You can pass an option called
checkOpen(default: true), which, when set, causes the function to returnfalseif the stream has already been closed[1].Usage Example
import fs from 'node:fs'; import { isStream } from 'is-stream'; isStream(fs.createReadStream('unicorn.png')); // => true isStream({}); // => falseThis detects whether the object passed is a stream[1].
How Does is-stream Detect Streams?
Detection is broadly based on checking for characteristic properties and functions associated with Node.js streams. Typically, streams in Node.js are objects that inherit from the
EventEmitterclass and implement methods such aspipe[3][4][8][10].Common checks include:
- The value is an object and not
null.- The value has a
pipemethod that is a function, which is a strong indicator of a stream in Node.js[10].- For specific stream types:
- Readable streams: Must have a
_readfunction and a_readableStateobject in addition to apipemethod[10].- Writable streams: Must have a
_writefunction and a_writableStateobject, plus apipemethod[10].- Duplex streams: Must qualify as both readable and writable[10].
Here’s an abstracted example based on standard detection logic:
// Generic stream check const isStream = val => val !== null && typeof val === 'object' && typeof val.pipe === 'function'; // Readable stream check const isReadableStream = val => isStream(val) && typeof val._read === 'function' && typeof val._readableState === 'object'; // Writable stream check const isWritableStream = val => isStream(val) && typeof val._write === 'function' && typeof val._writableState === 'object';[10]
Summary Table
Function Detects Key Properties Checked isStream Any Node.js stream object,.pipefunctionisReadableStream Readable stream .pipe,_readfunction,_readableStateobjectisWritableStream Writable stream .pipe,_writefunction,_writableStateobjectisDuplexStream Duplex stream (readable & writable) All of readable and writable checks isTransformStream Transform stream (duplex with transform) Further internal checks (not shown) Conclusion
The
is-streampackage provides a practical and reliable way to detect various types of Node.js streams by checking for the presence of essential properties and methods commonly implemented by Node.js stream classes[1][10]. This helps avoid subtle bugs and enables more robust handling of stream-based data processing.Citations:
- 1: https://www.npmjs.com/package/is-stream
- 2: https://www.npmjs.com/package/stream
- 3: https://nodejs.org/api/stream.html
- 4: http://nodesource.com/blog/understanding-streams-in-nodejs/
- 5: https://github.com/rvagg/isstream
- 6: https://snyk.io/blog/malicious-code-found-in-npm-package-event-stream/
- 7: https://www.npmjs.com/package/isstream
- 8: https://www.geeksforgeeks.org/node-js-streams/
- 9: https://blog.npmjs.org/post/180565383195/details-about-the-event-stream-incident
- 10: https://www.30secondsofcode.org/js/s/typecheck-nodejs-streams
Use
is-streamfor more comprehensive stream detectionOur current
instanceof Streamcheck only recognizes directnode:streaminstances and can miss stream-like objects. Theis-streampackage reliably detects all Node.js stream types (readable, writable, duplex, transform) by checking for characteristic methods and state, including handling closed streams via itscheckOpenoption.
- File to update:
core/common-util/src/StreamUtil.ts
- Replace the
Streamimport withis-stream- Update the
isStreammethod to delegate toisStream(obj)- import { Stream } from 'node:stream'; + import isStream from 'is-stream'; export class StreamUtil { static isStream(obj: any): boolean { - return obj instanceof Stream; + return isStream(obj); } }
🧹 Nitpick comments (1)
core/common-util/src/StreamUtil.ts (1)
3-7: Consider using a simple function instead of a class with only static members.The
StreamUtilclass contains only static members which is flagged by static analysis. For better maintainability and adherence to functional programming principles, consider refactoring to use a simple function:-export class StreamUtil { - static isStream(obj: any): boolean { - return obj instanceof Stream; - } -} +export function isStream(obj: any): boolean { + return obj instanceof Stream; +}If you need to keep the namespace for organization, you could use:
-export class StreamUtil { - static isStream(obj: any): boolean { - return obj instanceof Stream; - } -} +export const StreamUtil = { + isStream(obj: any): boolean { + return obj instanceof Stream; + } +};🧰 Tools
🪛 Biome (1.9.4)
[error] 3-7: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/common-util/src/StreamUtil.ts(1 hunks)plugin/controller/test/http/request.test.ts(1 hunks)plugin/tegg/lib/ctx_lifecycle_middleware.ts(2 hunks)plugin/tegg/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugin/tegg/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- plugin/controller/test/http/request.test.ts
- plugin/tegg/lib/ctx_lifecycle_middleware.ts
🧰 Additional context used
🪛 Biome (1.9.4)
core/common-util/src/StreamUtil.ts
[error] 3-7: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Analyze (javascript)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (16)
🔇 Additional comments (1)
core/common-util/src/StreamUtil.ts (1)
1-7: LGTM! Simple and focused utility implementation.The implementation is straightforward and correctly identifies Node.js Stream instances. This utility serves the PR's purpose well by enabling the detection of stream responses so the context isn't destroyed prematurely.
🧰 Tools
🪛 Biome (1.9.4)
[error] 3-7: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
core/common-util/src/StreamUtil.ts (2)
3-7: Consider using a simple exported function instead of a class with only static methodsThe static analysis correctly points out that classes with only static methods can be simplified to exported functions, which aligns with modern TypeScript best practices.
-export class StreamUtil { - static isStream(obj: any): boolean { - return obj instanceof Stream; - } -} +export function isStream(obj: any): boolean { + return obj instanceof Stream; +}If you prefer to keep the namespace for organizational purposes, you could use:
export const StreamUtil = { isStream(obj: any): boolean { return obj instanceof Stream; } };🧰 Tools
🪛 Biome (1.9.4)
[error] 3-7: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
4-6: Consider using a more robust stream detection approachWhile
instanceof Streamworks for Node.js native streams, it might not correctly identify stream-like objects that implement the stream interface but don't directly inherit from Stream (like some third-party libraries).You could consider:
- Using an established utility like
is-streamnpm package- Checking for stream-like behavior (presence of pipe method)
- static isStream(obj: any): boolean { - return obj instanceof Stream; - } + static isStream(obj: any): boolean { + return obj !== null && + typeof obj === 'object' && + (obj instanceof Stream || + typeof obj.pipe === 'function'); + }plugin/tegg/lib/ctx_lifecycle_middleware.ts (1)
28-33: Fix typo in function name: "doDestory" → "doDestroy"There's a typo in the function name which should be fixed for code clarity.
-function doDestory() { +function doDestroy() {Also apply this change to the function calls at lines 38 and 40.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/common-util/src/StreamUtil.ts(1 hunks)plugin/controller/test/http/request.test.ts(1 hunks)plugin/tegg/lib/ctx_lifecycle_middleware.ts(2 hunks)plugin/tegg/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugin/tegg/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/controller/test/http/request.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugin/tegg/lib/ctx_lifecycle_middleware.ts (1)
core/common-util/src/StreamUtil.ts (1)
StreamUtil(3-7)
🪛 Biome (1.9.4)
core/common-util/src/StreamUtil.ts
[error] 3-7: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (javascript)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
🔇 Additional comments (1)
plugin/tegg/lib/ctx_lifecycle_middleware.ts (1)
37-41: Verify middleware behavior when asynchronously destroying contextThe middleware completes without awaiting the completion of the stream events and context destruction. This means the response might be sent to the client with clean-up operations still pending.
Is this intentional? If not, consider one of these approaches:
- Make the middleware wait for all cleanup to complete (more resource-intensive)
- Document that cleanup happens asynchronously after the response is sent (current behavior)
If option 1 is preferred, you could implement:
- if (StreamUtil.isStream(ctx.response.body)) { - awaitEvent(ctx.response.body, 'close').then(doDestory); - } else { - doDestory(); - } + if (StreamUtil.isStream(ctx.response.body)) { + try { + await awaitEvent(ctx.response.body, 'close'); + } catch (e) { + ctx.logger.error('[tegg/ctxLifecycleMiddleware] error while waiting for stream close event:', e); + } + } + doDestory();
| if (StreamUtil.isStream(ctx.response.body)) { | ||
| awaitEvent(ctx.response.body, 'close').then(doDestory); | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider listening for other relevant stream events
Currently, the code only waits for the 'close' event, but depending on the stream type, other events might signal completion first. For better handling of different stream types, consider listening for multiple events like 'end' and 'finish' as well.
- if (StreamUtil.isStream(ctx.response.body)) {
- awaitEvent(ctx.response.body, 'close').then(doDestory);
+ if (StreamUtil.isStream(ctx.response.body)) {
+ // Different stream types may emit different events to signal completion
+ // - Readable streams emit 'end' when data flow is complete
+ // - Writable streams emit 'finish' when all data is written
+ // - All streams emit 'close' when the underlying resource is closed
+ const stream = ctx.response.body;
+ Promise.race([
+ awaitEvent(stream, 'close'),
+ awaitEvent(stream, 'end'),
+ awaitEvent(stream, 'finish'),
+ ]).then(doDestory);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (StreamUtil.isStream(ctx.response.body)) { | |
| awaitEvent(ctx.response.body, 'close').then(doDestory); | |
| } else { | |
| if (StreamUtil.isStream(ctx.response.body)) { | |
| // Different stream types may emit different events to signal completion | |
| // - Readable streams emit 'end' when data flow is complete | |
| // - Writable streams emit 'finish' when all data is written | |
| // - All streams emit 'close' when the underlying resource is closed | |
| const stream = ctx.response.body; | |
| Promise.race([ | |
| awaitEvent(stream, 'close'), | |
| awaitEvent(stream, 'end'), | |
| awaitEvent(stream, 'finish'), | |
| ]).then(doDestory); | |
| } else { |
🛠️ Refactor suggestion
Add error handling for the stream event promise
The current implementation doesn't handle errors that might occur while waiting for the stream event. This could lead to silent failures if the stream encounters an error.
- if (StreamUtil.isStream(ctx.response.body)) {
- awaitEvent(ctx.response.body, 'close').then(doDestory);
+ if (StreamUtil.isStream(ctx.response.body)) {
+ awaitEvent(ctx.response.body, 'close')
+ .then(doDestory)
+ .catch(e => {
+ e.message = '[tegg/ctxLifecycleMiddleware] error while waiting for stream close event: ' + e.message;
+ ctx.logger.error(e);
+ doDestory(); // Still attempt to destroy the context
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (StreamUtil.isStream(ctx.response.body)) { | |
| awaitEvent(ctx.response.body, 'close').then(doDestory); | |
| } else { | |
| if (StreamUtil.isStream(ctx.response.body)) { | |
| awaitEvent(ctx.response.body, 'close') | |
| .then(doDestory) | |
| .catch(e => { | |
| e.message = '[tegg/ctxLifecycleMiddleware] error while waiting for stream close event: ' + e.message; | |
| ctx.logger.error(e); | |
| doDestory(); // Still attempt to destroy the context | |
| }); | |
| } else { |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugin/tegg/lib/ctx_lifecycle_middleware.ts (1)
29-35: Consider listening for additional stream eventsCurrently, the code only listens for 'close' and 'error' events, but depending on the stream type, other events might signal completion first. For better handling of different stream types, consider listening for 'end' and 'finish' events as well.
if (StreamUtil.isStream(ctx.response.body)) { try { - await awaitFirst(ctx.response.body, [ 'close', 'error' ]); + await awaitFirst(ctx.response.body, [ 'close', 'error', 'end', 'finish' ]); } catch (error) { ctx.res.destroy(error); } }
🧹 Nitpick comments (2)
plugin/controller/test/fixtures/apps/http-inject-app/app/controller/AppController.ts (2)
48-66: Well-implemented streaming responseThe implementation of the streaming response is well structured. A few observations:
- Setting
X-Accel-Bufferingto 'no' correctly disables Nginx buffering if used- Using
Readable.fromto create a stream from the async generator is appropriate- The console.log statement might be intended for debugging but consider removing it or replacing with proper logging
async function* generate(count = 5, duration = 500) { yield '<html><head><title>hello stream</title></head><body>'; for (let i = 0; i < count; i++) { - console.log('generate', i); + // Use a logger instead of console.log for production code yield `<h2>流式内容${i + 1}，${Date()}</h2>`; await setTimeout(duration); } yield self.backgroundTaskHelper.toString(); yield '</body></html>'; }
68-89: Well-implemented error stream for testingThe error stream implementation is a good way to test error handling in streams. Same suggestion about console.log applies here.
async function* generate(count = 5, duration = 500) { yield '<html><head><title>hello stream</title></head><body>'; for (let i = 0; i < count; i++) { - console.log('generate', i); + // Use a logger instead of console.log for production code if (i > 2) { throw new Error('test'); } yield `<h2>流式内容${i + 1}，${Date()}</h2>`; await setTimeout(duration); } yield self.backgroundTaskHelper.toString(); yield '</body></html>'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
plugin/controller/package.json(0 hunks)plugin/controller/test/fixtures/apps/http-inject-app/app/controller/AppController.ts(3 hunks)plugin/controller/test/http/request.test.ts(1 hunks)plugin/tegg/lib/ctx_lifecycle_middleware.ts(2 hunks)plugin/tegg/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- plugin/controller/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- plugin/tegg/package.json
- plugin/controller/test/http/request.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugin/tegg/lib/ctx_lifecycle_middleware.ts (2)
core/common-util/src/StreamUtil.ts (1)
StreamUtil(3-7)core/eventbus-runtime/src/SingletonEventBus.ts (1)
awaitFirst(61-63)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (16)
🔇 Additional comments (3)
plugin/tegg/lib/ctx_lifecycle_middleware.ts (1)
4-5: Import necessary dependencies for stream handlingGood additions of the
StreamUtilfrom '@eggjs/tegg-common-util' andawaitFirstfrom 'await-first' to handle stream events properly.plugin/controller/test/fixtures/apps/http-inject-app/app/controller/AppController.ts (2)
13-16: Good additions of required importsThe imports added for dependency injection and stream handling are appropriate for the new functionality.
23-27: Properly injected dependenciesThe
@Inject()decorators are properly used to inject the required dependencies.
| ctx.logger.error(e); | ||
| }); | ||
| } | ||
| doDestory(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function call to match name
If you update the function name from doDestory to doDestroy as suggested above, remember to update the function call here as well.
- doDestory();
+ doDestroy();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| doDestory(); | |
| doDestroy(); |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/controller/test/http/request.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Runner-macos (18)
plugin/controller/test/http/request.test.ts
[failure] 68-68:
Expected indentation of 8 spaces but found 6
🪛 GitHub Check: Runner-macos (16)
plugin/controller/test/http/request.test.ts
[failure] 68-68:
Expected indentation of 8 spaces but found 6
🪛 GitHub Check: Runner-ubuntu (22)
plugin/controller/test/http/request.test.ts
[failure] 68-68:
Expected indentation of 8 spaces but found 6
🪛 GitHub Check: Runner-macos (20)
plugin/controller/test/http/request.test.ts
[failure] 68-68:
Expected indentation of 8 spaces but found 6
🪛 GitHub Check: Runner-ubuntu (18)
plugin/controller/test/http/request.test.ts
[failure] 68-68:
Expected indentation of 8 spaces but found 6
🪛 GitHub Check: Runner-ubuntu (16)
plugin/controller/test/http/request.test.ts
[failure] 68-68:
Expected indentation of 8 spaces but found 6
🪛 GitHub Check: Runner-ubuntu (20)
plugin/controller/test/http/request.test.ts
[failure] 68-68:
Expected indentation of 8 spaces but found 6
🪛 GitHub Actions: Continuous integration
plugin/controller/test/http/request.test.ts
[error] 68-68: Expected indentation of 8 spaces but found 6 (@typescript-eslint/indent)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
🔇 Additional comments (1)
plugin/controller/test/http/request.test.ts (1)
54-61: Good implementation of stream test with assertionThe test successfully verifies that the stream endpoint returns a 200 status code and checks for expected content in the response. This provides proper validation of the streaming functionality.
3 participants
untilEndmethod to properly handle stream consumptionplugin/tegg/lib/ctx_lifecycle_middleware.tsto ensure complete consumption of response.body streams before context destructionStreamUtil，新增untilEnd方法plugin/tegg/lib/ctx_lifecycle_middleware.ts当 response.body 为 Stream 时，确保消费完再出发 destorySummary by CodeRabbit
New Features
Bug Fixes
Tests
Style