Skip to content

Conversation

@sunnylqm
Copy link
Contributor

@sunnylqm sunnylqm commented Nov 2, 2025

should fix #1520
thanks for this comment @cyannuk #861 (comment)

Summary by CodeRabbit

  • Refactor
    • Simplified internal promise handling in request routing to streamline async flow while preserving behavior.

@coderabbitai
Copy link

coderabbitai bot commented Nov 2, 2025

Walkthrough

Rewrites the HEAD handling promise chain in src/compose.ts to pass the response through a nested promise callback (using an inline arrow) instead of a pre-bound local constant, preserving response header setting and response construction semantics.

Changes

Cohort / File(s) Summary
HEAD request promise chain change
src/compose.ts
Replaces local const _res = ht[<method>].composed(c) + separate getResponseLength(_res).then(...) pattern with a single chained promise: ht[<method>].composed(c).then(_res => getResponseLength(_res).then(...)), keeping header mutation and Response creation inside the nested promise.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Router
  participant Handler as RouteHandler
  participant Composer as composed()
  participant Length as getResponseLength
  participant ResponseObj as Response

  Client->>Router: HEAD /path
  Router->>Handler: invoke handler context (c)
  Handler->>Composer: ht[method].composed(c)
  Composer-->>Length: promise resolves with _res
  Length-->>Composer: resolve length
  Composer->>ResponseObj: set Content-Length header
  Composer-->>Client: return Response
  note right of Composer #DFF2E1: Inline chained promise used\n(_res => getResponseLength(_res).then(...))
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify promise chaining and _res scoping inside nested callbacks.
  • Confirm HEAD behavior works for both sync and async route handlers and that Content-Length is set.
  • Run or review tests for HEAD and GET parity.

Poem

🐰 I hopped through promises, one by one,
Found _res lost where shadows run,
Chained it tight, a gentle shove —
Headers set, the response fits like a glove. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "fix head request" is directly related to the main change in the pull request. The raw summary shows the modification is specifically to the HEAD handling path in src/compose.ts, and the change restructures the promise chain to fix the scope issue that causes async HEAD requests to crash. The title clearly and concisely summarizes the primary objective of fixing HEAD request handling, which aligns with the linked issue #1520. The title is specific enough for a teammate scanning history to understand the core change without being vague or overly generic.
Linked Issues Check ✅ Passed The code changes in src/compose.ts directly address the primary objective from linked issue #1520. The issue reports that async GET handlers crash on HEAD requests with a TypeError indicating _res.headers.set is undefined, caused by incorrect promise chaining where getResponseLength(_res) is called before _res is available. The fix restructures the promise chain to nest getResponseLength within a .then(_res => ...) callback, ensuring _res is properly scoped and available when needed. This change preserves the existing semantics while fixing the scope issue that caused the 500 error, directly resolving the reported bug.
Out of Scope Changes Check ✅ Passed The changes are narrowly focused on fixing the specific bug reported in issue #1520. The raw summary indicates modifications are limited to the HEAD handling path in src/compose.ts, with no changes to exported entities, public API signatures, or manifest files. The restructuring of the promise chain in this single location directly targets the root cause of the async HEAD request crash without introducing unrelated modifications. No out-of-scope changes have been introduced beyond what is necessary to resolve the reported issue.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c5a97b5 and 2e8b82f.

📒 Files selected for processing (1)
  • src/compose.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/compose.ts

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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 94abb3c and c5a97b5.

📒 Files selected for processing (1)
  • src/compose.ts (1 hunks)

@SaltyAom SaltyAom merged commit 8c66cf0 into elysiajs:main Nov 3, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Async handlers crash on HEAD request

2 participants