test(proxy): cover pre-aborted abort listener review#1114
Conversation
📝 WalkthroughWalkthrough为客户端中止监听器的绑定函数补充 JSDoc，说明幂等清理语义和当 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分钟 Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds documentation to the bindClientAbortListener function to clarify its synchronous execution behavior when a signal is already aborted. Additionally, it updates test helper configurations to use the openai-compatible provider type and introduces a new unit test to ensure that stream cancellation is correctly handled and invoked exactly once when the client signal is pre-aborted. I have no feedback to provide.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| controller.abort(); | ||
| const addSpy = vi.spyOn(controller.signal, "addEventListener"); | ||
| const removeSpy = vi.spyOn(controller.signal, "removeEventListener"); | ||
| const localAbortSpy = vi.spyOn(AbortController.prototype, "abort"); |
There was a problem hiding this comment.
vi.spyOn(AbortController.prototype, "abort") intercepts every .abort() call on every AbortController instance for the duration of this test — including any controllers that mock infrastructure (e.g., AsyncTaskManager.register's returned controller) might abort. toHaveBeenCalledTimes(1) is therefore sensitive to internal implementation details: adding a second internal controller that also calls .abort() will silently break this test.
Consider holding a reference to the specific internal controller (e.g., by spying on the AbortController constructor and capturing this) or documenting explicitly in a comment why exactly one call is expected.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/proxy/response-handler-abort-listener-cleanup.test.ts
Line: 289
Comment:
**Global prototype spy may produce a fragile assertion**
`vi.spyOn(AbortController.prototype, "abort")` intercepts every `.abort()` call on every `AbortController` instance for the duration of this test — including any controllers that mock infrastructure (e.g., `AsyncTaskManager.register`'s returned controller) might abort. `toHaveBeenCalledTimes(1)` is therefore sensitive to internal implementation details: adding a second internal controller that also calls `.abort()` will silently break this test.
Consider holding a reference to the specific internal controller (e.g., by spying on the `AbortController` constructor and capturing `this`) or documenting explicitly in a comment why exactly one call is expected.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/proxy/response-handler-abort-listener-cleanup.test.ts (1)
284-306: 新增 stream 预中止用例覆盖到位，但 prototype spy 范围偏宽。用例正确覆盖了
signal.aborted === true时bindClientAbortListener同步执行onAbort、不注册/不移除监听器、cancelTask仅触发一次的语义。唯一可改进点：
vi.spyOn(AbortController.prototype, "abort")会捕获测试期间所有AbortController实例的abort()调用（包括第 23 行AsyncTaskManager.register返回的 controller，以及dispatch内部可能创建的其它 controller）。当前toHaveBeenCalledTimes(1)通过依赖于实现细节，未来若内部多创建一个被 abort 的 controller，本用例会出现误报。如希望将断言锚定在“某个特定本地 controller”，可以考虑改为对调用结果
Response关联的 reader/upstream controller 做更精确的断言，或仅断言>= 1：♻️ 可选的稳健化改写
- expect(testState.cancelTask).toHaveBeenCalledTimes(1); - expect(localAbortSpy).toHaveBeenCalledTimes(1); + expect(testState.cancelTask).toHaveBeenCalledTimes(1); + // 至少触发一次本地 AbortController.abort()，避免与其他内部 controller 的 abort 调用耦合 + expect(localAbortSpy.mock.calls.length).toBeGreaterThanOrEqual(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/response-handler-abort-listener-cleanup.test.ts` around lines 284 - 306, The prototype-wide spy vi.spyOn(AbortController.prototype, "abort") is too broad and may catch aborts from other controllers created during dispatch; replace it with a targeted assertion: either spy on the specific controller instance you care about (the local controller created/registered during AsyncTaskManager.register or returned/accessible via testState/session) instead of the prototype, or keep the prototype spy but relax the assertion to expect at least one call (e.g., expect(localAbortSpy).toHaveBeenCalled(); or toBeGreaterThanOrEqual(1)). Update the test around the symbols controller, ProxyResponseHandler.dispatch, bindClientAbortListener, and testState.cancelTask accordingly so the abort assertion is anchored to the intended controller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/proxy/response-handler-abort-listener-cleanup.test.ts`:
- Around line 284-306: The prototype-wide spy
vi.spyOn(AbortController.prototype, "abort") is too broad and may catch aborts
from other controllers created during dispatch; replace it with a targeted
assertion: either spy on the specific controller instance you care about (the
local controller created/registered during AsyncTaskManager.register or
returned/accessible via testState/session) instead of the prototype, or keep the
prototype spy but relax the assertion to expect at least one call (e.g.,
expect(localAbortSpy).toHaveBeenCalled(); or toBeGreaterThanOrEqual(1)). Update
the test around the symbols controller, ProxyResponseHandler.dispatch,
bindClientAbortListener, and testState.cancelTask accordingly so the abort
assertion is anchored to the intended controller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a6d9374-6dbb-473b-9e95-e6e7052524cb
📒 Files selected for processing (2)
src/app/v1/_lib/proxy/client-abort-listener.tstests/unit/proxy/response-handler-abort-listener-cleanup.test.ts
github-actions
Bot
added
area:core
javascript
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR.
PR Size: XS
- Lines changed: 34
- Files changed: 2
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate
- Code clarity - Good
Automated review by Codex AI
There was a problem hiding this comment.
- Reviewed PR
#1114(32additions /2deletions across2files) and applied thesize/XSlabel. - Checked diff-only lines across logic/security/error handling/types/comments/tests/simplification and found no issues that met the reporting threshold (>= 80 confidence) after context validation, so no inline review comments were posted.
- Submitted the required review summary via
gh pr review --comment.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Documentation and test coverage improvements for pre-aborted abort listener handling in the proxy pipeline. This is a follow-up PR addressing review feedback from #1113.
Related Issues & PRs
Changes
Documentation
bindClientAbortListener()explaining synchronousonAbortinvocation for pre-aborted signalsTest Improvements
Provider.providerTypevalues with the real union type ("openai-compatible"instead of"openai")invokes stream cancel once when client signal is already abortedcancelTaskis called exactly once andlocalAbortfires onceonAbortwas invoked synchronouslyFiles Changed
src/app/v1/_lib/proxy/client-abort-listener.tstests/unit/proxy/response-handler-abort-listener-cleanup.test.tsValidation
Checklist
This PR addresses review feedback from #1113 to ensure complete test coverage of edge cases in the abort listener lifecycle.
Greptile Summary
This follow-up PR adds a JSDoc comment to
bindClientAbortListenerdocumenting its synchronous-invocation behavior for pre-aborted signals, fixesproviderTypefixture values ("openai"→"openai-compatible"), and adds a new test case covering the streaming + pre-aborted signal edge case.Confidence Score: 5/5
Safe to merge — documentation and test-only changes with no production logic modifications.
No production logic was changed; changes are limited to a JSDoc comment and test improvements. The only finding is a P2 style inconsistency between two assertion strengths in the test file.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[bindClientAbortListener called] --> B{signal null/undefined?} B -- yes --> C[return no-op cleanup] B -- no --> D{signal.aborted?} D -- yes --> E[invoke onAbort synchronously] E --> F[return no-op cleanup] D -- no --> G[addEventListener abort, once:true] G --> H[return idempotent cleanup fn] H --> I{cleanup called?} I -- yes --> J[removeEventListener abort] I -- already cleaned --> K[no-op]Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "test(proxy): avoid global abort prototyp..." | Re-trigger Greptile