Skip to content

fix(aot): continue onError after undefined#1901

Open
lxy2500798479 wants to merge 1 commit into
elysiajs:mainfrom
lxy2500798479:fix/on-error-chain
Open

fix(aot): continue onError after undefined#1901
lxy2500798479 wants to merge 1 commit into
elysiajs:mainfrom
lxy2500798479:fix/on-error-chain

Conversation

@lxy2500798479

@lxy2500798479 lxy2500798479 commented Jun 1, 2026

Copy link
Copy Markdown

Summary

  • continue the AOT onError chain when a handler returns undefined or null
  • only run mapResponse/mapEarlyResponse for an onError result that actually handled the error
  • add regression coverage for nested guard errors with a logging onError before a response-producing onError

Fixes #1900

Reproduction

Minimal reproduction repo:

https://github.com/lxy2500798479/elysia-onerror-chain-repro

In AOT mode, a logging-only global onError followed by a response-producing global onError could skip the second handler when a nested guard threw. If a mapResponse hook was present, it could receive undefined and turn the error into a success-shaped response.

Boundary report

  • AOT and dynamic modes covered in the same regression test
  • nested group + use + onBeforeHandle throw path covered
  • earlier onError returning undefined covered
  • global mapResponse converting undefined covered
  • existing core error handling and dynamic mode tests still pass

Tests

  • bun test test/core/handle-error.test.ts
  • bun test test/core/handle-error.test.ts test/core/dynamic.test.ts
  • bun run test:types
  • bun test
  • bun run test:imports

@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

This PR fixes an AOT-mode bug where early onError handlers returning undefined incorrectly stopped the error chain. The code generator's error-handling condition now uses explicit null/undefined checks instead of truthiness, allowing undefined returns to properly continue to subsequent handlers. A test case verifies the fix across both AOT modes.

Changes

AOT onError Chain Continuation

Layer / File(s) Summary
Explicit null/undefined check in error handler codegen
src/compose.ts
Generated error-handling now explicitly checks er !== undefined && er !== null instead of truthiness, with adjusted block termination to allow chain continuation when er is undefined.
Test for continued error chain when handler returns undefined
test/core/handle-error.test.ts
Verifies that global logger onError returning undefined allows the next global error handler to run, produces the correct JSON response, and confirms hook execution order across both aot modes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • elysiajs/elysia#1450: Both PRs modify the same composeHandler error-handling control flow for AOT—the main PR tightens the "early error response" check for onError results, while the retrieved PR adds afterResponse() behavior when onError returns a value in AOT.

Suggested reviewers

  • SaltyAom

✨ fixing bugs that trivial handlers don't handle~~ ♡
your undefined kept escaping the chain, how amateur (´-ω-`)
now it knows its place and let's the flow continue, you're welcome~ ♡♡

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main fix: AOT mode's onError chain now continues when handlers return undefined, directly addressing issue #1900.
Linked Issues check ✅ Passed The PR implementation matches #1900's objectives: code changes ensure onError handlers returning undefined allow chain continuation, and mapResponse only runs for actual error responses.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the AOT onError chaining bug described in #1900; modifications to error-handling logic and regression test coverage are directly related.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ 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 coderabbitai Bot 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.

🧹 Nitpick comments (1)
test/core/handle-error.test.ts (1)

144-206: ⚡ Quick win

Nice catch, but you only tested half the nullish path, baka~ (  ̄▽ ̄)っ♡

Line 144 validates undefined, but the code change also treats null as “unhandled”. Add a sibling assertion for a first onError returning null so this contract doesn’t silently regress later, okay~

Suggested test extension
 it('continues to next onError when previous hook returns undefined', async () => {
   for (const aot of [true, false]) {
@@
   }
 })
+
+it('continues to next onError when previous hook returns null', async () => {
+  for (const aot of [true, false]) {
+    const calls: string[] = []
+
+    const logger = new Elysia({ name: `logger-on-error-null-${aot}` })
+      .onError({ as: 'global' }, () => {
+        calls.push('logger')
+        return null
+      })
+
+    const responseWrapper = new Elysia({ name: `response-wrapper-null-${aot}` })
+      .onError({ as: 'global' }, ({ error, set }) => {
+        calls.push('response-wrapper')
+        set.status = 200
+        return Response.json({ code: 403, msg: error instanceof Error ? error.message : 'Forbidden', data: null })
+      })
+
+    const guard = new Elysia({ name: `guard-null-${aot}` }).onBeforeHandle({ as: 'scoped' }, () => {
+      throw new Error('denied by nested guard')
+    })
+
+    const app = new Elysia({ aot })
+      .use(logger)
+      .use(responseWrapper)
+      .use(new Elysia({ prefix: '/api' }).group('', (app) => app.use(guard).post('/guarded', () => ({ ok: true }))))
+
+    const response = await app.handle(req('/api/guarded', { method: 'POST' }))
+
+    expect(response.status).toBe(200)
+    expect(await response.json()).toEqual({ code: 403, msg: 'denied by nested guard', data: null })
+    expect(calls).toEqual(['logger', 'response-wrapper'])
+  }
+})
🤖 Prompt for 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.

In `@test/core/handle-error.test.ts` around lines 144 - 206, Extend the test
"continues to next onError when previous hook returns undefined" to also cover
the null return path: add a parallel case where the first onError handler (the
logger or the one currently returning undefined) explicitly returns null instead
of undefined and assert the same behavior — app.handle(...) yields status 200
with JSON { code: 403, msg: 'denied by nested guard', data: null } and the call
order is ['logger','response-wrapper']; update the setup using the same Elysia
instances (logger, responseWrapper, guard, app) and duplicate the assertions for
the null-returning onError to ensure null is treated as "unhandled" just like
undefined.
🤖 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.

Nitpick comments:
In `@test/core/handle-error.test.ts`:
- Around line 144-206: Extend the test "continues to next onError when previous
hook returns undefined" to also cover the null return path: add a parallel case
where the first onError handler (the logger or the one currently returning
undefined) explicitly returns null instead of undefined and assert the same
behavior — app.handle(...) yields status 200 with JSON { code: 403, msg: 'denied
by nested guard', data: null } and the call order is
['logger','response-wrapper']; update the setup using the same Elysia instances
(logger, responseWrapper, guard, app) and duplicate the assertions for the
null-returning onError to ensure null is treated as "unhandled" just like
undefined.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cd816cfe-99c5-4476-8387-384724261ebd

📥 Commits

Reviewing files that changed from the base of the PR and between 56310be and 4a60ca0.

📒 Files selected for processing (2)
  • src/compose.ts
  • test/core/handle-error.test.ts

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.

AOT onError chain stops after an undefined handler when mapResponse is present

1 participant