Skip to content

test: cover cloned pipeable node responses#1414

Merged
pi0 merged 4 commits into
h3js:mainfrom
pupuking723:test/node-pipeable-clone-regression
Jul 2, 2026
Merged

test: cover cloned pipeable node responses#1414
pi0 merged 4 commits into
h3js:mainfrom
pupuking723:test/node-pipeable-clone-regression

Conversation

@pupuking723

@pupuking723 pupuking723 commented Jun 13, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Closes #1278

Adds a node-only regression test for pipe-able handler return values when an onResponse hook clones the response. The current implementation already returns the expected body; this keeps the reported regression covered.

Verification:

  • pnpm exec vitest --run test/app.test.ts
  • pnpm exec oxfmt --check test/app.test.ts
  • pnpm exec oxlint test/app.test.ts

AI assistance disclosure: I used Codex while preparing this test and reviewed the change before submitting.

Summary by CodeRabbit

  • Tests
    • Added test coverage ensuring response body piping remains intact when responses are cloned inside response hooks, verifying end-to-end response delivery remains correct.

@pupuking723
pupuking723 requested a review from pi0 as a code owner June 13, 2026 12:51
@coderabbitai

coderabbitai Bot commented Jun 13, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f31f1dd7-5d24-4f89-9a2a-03cb139a9d36

📥 Commits

Reviewing files that changed from the base of the PR and between 45fafca and c1ce3b1.

📒 Files selected for processing (1)
  • test/app.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/app.test.ts
📝 Walkthrough

Walkthrough

A test case was added to verify that pipeable response bodies remain intact after response.clone() is called within an onResponse hook. The import statement was updated to include the onResponse helper, and a Node-only regression test was implemented.

Changes

Pipeable Response Body Clone Test

Layer / File(s) Summary
Pipeable response body clone test
test/app.test.ts		 Import onResponse helper and add a Node-only test that reproduces issue #1278: register an onResponse hook that calls response.clone(), mount a route returning a pipeable object that writes "test" via pipe(writable), and assert the fetched response text is "test".

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 I cloned a response, gave stream a song,
Piped tiny letters that traveled along,
Hooks checked the echo, the bytes stayed true,
A test scribbles "test" and says "we're through",
Hops of joy — the stream sings anew!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'test: cover cloned pipeable node responses' clearly and concisely describes the main change: adding test coverage for a specific regression scenario.
Linked Issues check ✅ Passed The PR successfully addresses issue #1278 by adding a regression test that covers the exact scenario reported: verifying pipeable response bodies work correctly when response.clone() is called in onResponse hooks.
Out of Scope Changes check ✅ Passed All changes are directly scoped to adding test coverage for the reported regression; no unrelated modifications are present in the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai Bot reviewed Jun 13, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/app.test.ts`:
- Line 3: In test/app.test.ts replace the single barrel import "import {
HTTPError, fromNodeHandler, onResponse } from '../src/index.ts';" with direct
imports for each symbol: import HTTPError from error (HTTPError), import
fromNodeHandler from adapters (fromNodeHandler), and import onResponse from
utils/middleware (onResponse); this fixes the re-export issue by importing each
symbol from its original module rather than the barrel.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6b38c095-8215-4805-bf22-28177d9b19d2

📥 Commits

Reviewing files that changed from the base of the PR and between 7eb018e and 45fafca.

📒 Files selected for processing (1)
  • test/app.test.ts

Comment thread test/app.test.ts Outdated
pi0x
pi0x approved these changes Jul 2, 2026
View reviewed changes

@pi0x pi0x left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this review was performed by AI agents (Claude Code) on behalf of the maintainer, in two independent passes.

Pass 1 reviewed the diff for correctness, edge cases, and test coverage. Pass 2 (a fresh agent, adversarial by design) independently re-read the diff, checked for merge conflicts/CI status, and tried to find a reason to block approval before this review was posted. Both passes agree this change is correct, adequately tested, and safe to merge.

Caveat: this repo's real CI workflow (lint + build + vitest) requires maintainer approval to run on fork PRs and has not executed on this PR yet — only automated third-party checks (CodeRabbit, Socket Security) have run. Please trigger/approve the CI workflow before merging.

@pi0
pi0 merged commit 37467ce into h3js:main Jul 2, 2026
7 checks passed
pi0x pushed a commit to mixelburg/h3 that referenced this pull request Jul 2, 2026
@pupuking723 @pi0
test: cover cloned pipeable node responses (h3js#1414)
e964dfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@pi0x pi0x pi0x approved these changes
@pi0 pi0 Awaiting requested review from pi0 pi0 is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[node] stream doesn't get sent if response is cloned

3 participants

@pupuking723 @pi0x @pi0