patch 1.4.19 #1610
patch 1.4.19 #1610
Conversation
WalkthroughAdds CHANGELOG entry v1.4.19, exposes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
commit: |
![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 (1)
CHANGELOG.md (1)
2-2: Minor: Add missing colon after "Improvement" for consistency.Line 2 lacks a colon after
Improvement, while line 6 hasBug fix:with a colon. For consistency with established formatting, add the colon.-Improvement +Improvement:
CHANGELOG.md
Outdated
| # 1.4.19 - 9 Dec 2025 | ||
| Improvement | ||
| - [#1588](https://github.com/elysiajs/elysia/pull/1588), [#1587](https://github.com/elysiajs/elysia/pull/1587) add seen weakset during mergeDeep | ||
| - [#1607](https://github.com/elysiajs/elysia/issues/1607), [#1606](https://github.com/elysiajs/elysia/issues/1606), [#1139](https://github.com/elysiajs/elysia/issues/1138) nested form data |
There was a problem hiding this comment.
Fix broken reference link on line 4.
The issue reference displays #1139 but the URL points to #1138. Update the URL to match the displayed issue number.
-- [#1607](https://github.com/elysiajs/elysia/issues/1607), [#1606](https://github.com/elysiajs/elysia/issues/1606), [#1139](https://github.com/elysiajs/elysia/issues/1138) nested form data
+- [#1607](https://github.com/elysiajs/elysia/issues/1607), [#1606](https://github.com/elysiajs/elysia/issues/1606), [#1139](https://github.com/elysiajs/elysia/issues/1139) nested form data📝 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.
| - [#1607](https://github.com/elysiajs/elysia/issues/1607), [#1606](https://github.com/elysiajs/elysia/issues/1606), [#1139](https://github.com/elysiajs/elysia/issues/1138) nested form data | |
| - [#1607](https://github.com/elysiajs/elysia/issues/1607), [#1606](https://github.com/elysiajs/elysia/issues/1606), [#1139](https://github.com/elysiajs/elysia/issues/1139) nested form data |
🤖 Prompt for AI Agents
In CHANGELOG.md around line 4, the displayed issue reference shows #1139 but its
URL points to .../issues/1138; update the third link's URL to .../issues/1139 so
the href matches the displayed issue number (change the 1138 in the URL to
1139).
| - [#1607](https://github.com/elysiajs/elysia/issues/1607), [#1606](https://github.com/elysiajs/elysia/issues/1606), [#1139](https://github.com/elysiajs/elysia/issues/1138) nested form data | ||
|
|
||
| Bug fix: | ||
| - [#1591](https://github.com/elysiajs/elysia/pull/1591), [#1590](https://github.com/elysiajs/elysia/pull/1591) merge set.headers without duplicating Response |
There was a problem hiding this comment.
Fix duplicate link reference on line 7.
Both references point to PR #1591. The second reference should point to #1590 instead.
-- [#1591](https://github.com/elysiajs/elysia/pull/1591), [#1590](https://github.com/elysiajs/elysia/pull/1591) merge set.headers without duplicating Response
+- [#1591](https://github.com/elysiajs/elysia/pull/1591), [#1590](https://github.com/elysiajs/elysia/pull/1590) merge set.headers without duplicating Response📝 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.
| - [#1591](https://github.com/elysiajs/elysia/pull/1591), [#1590](https://github.com/elysiajs/elysia/pull/1591) merge set.headers without duplicating Response | |
| - [#1591](https://github.com/elysiajs/elysia/pull/1591), [#1590](https://github.com/elysiajs/elysia/pull/1590) merge set.headers without duplicating Response |
🤖 Prompt for AI Agents
In CHANGELOG.md around line 7 the second PR link incorrectly repeats #1591;
update the second reference to point to #1590 so the line reads with distinct
links to PR #1591 and PR #1590, preserving the existing link format and
punctuation.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
813-833: Bug: encoded loose path is never actually registered (duplicate add ofloosePath)In the non‑AOT dynamic router branch:
const loosePath = getLoosePath(path) this.router.dynamic.add(method, loosePath, { ... }) const encoded = encodePath(loosePath) if (loosePath !== encoded) this.router.dynamic.add(method, loosePath, { ... })the second
addstill usesloosePathinstead ofencoded, so:
- The encoded loose path is never registered.
- The same path/method pair is added twice, which is redundant and could mask intent.
This should mirror the earlier
pathhandling and actually registerencoded:- const encoded = encodePath(loosePath) - if (loosePath !== encoded) - this.router.dynamic.add(method, loosePath, { + const encoded = encodePath(loosePath) + if (loosePath !== encoded) + this.router.dynamic.add(method, encoded, { validator, hooks, content: localHook?.type as string, handle, route: path })
🧹 Nitpick comments (1)
src/sucrose.ts (1)
547-583: Escapecontextbefore embedding it in regex to avoid syntax errors and static-analysis noise
isContextPassToFunctioncurrently builds regexes directly from thecontextstring:const captureFunction = new RegExp(`\\w\\((?:.*?)?${context}(?:.*?)?\\)`, 'gs') const exactParameter = new RegExp(`${context}(,|\\))`, 'gs')Because
contextis derived from the parameter string, it can sometimes include characters like{,},.or whitespace (e.g. rest-destructuring forms), which makes these patterns syntactically invalid and forces you into the try/catch path. It also triggers the static “regexp-from-variable” warning.You can keep the current matching strategy but escape
contextonce before use:- const captureFunction = new RegExp( - `\\w\\((?:.*?)?${context}(?:.*?)?\\)`, - 'gs' - ) - const exactParameter = new RegExp(`${context}(,|\\))`, 'gs') + const escaped = context.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + const captureFunction = new RegExp( + `\\w\\((?:.*?)?${escaped}(?:.*?)?\\)`, + 'gs' + ) + const exactParameter = new RegExp(`${escaped}(,|\\))`, 'gs')This avoids regex syntax errors, reduces reliance on the catch-all error handler, and should satisfy the static-analysis warning without changing semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)example/a.ts(1 hunks)src/index.ts(1 hunks)src/schema.ts(2 hunks)src/sucrose.ts(2 hunks)test/core/dynamic.test.ts(1 hunks)test/sucrose/sucrose.test.ts(1 hunks)test/types/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- example/a.ts
🧰 Additional context used
🧬 Code graph analysis (5)
test/types/utils.ts (1)
src/schema.ts (1)
getSchemaValidator(380-1041)
src/schema.ts (2)
src/index.ts (1)
UnwrapSchema(8223-8223)src/types.ts (1)
UnwrapSchema(457-495)
test/core/dynamic.test.ts (2)
src/index.ts (2)
Elysia(185-8135)Elysia(8137-8137)test/utils.ts (1)
req(1-2)
src/sucrose.ts (1)
src/universal/request.ts (1)
body(81-144)
test/sucrose/sucrose.test.ts (1)
src/sucrose.ts (1)
sucrose(652-766)
🪛 ast-grep (0.40.0)
src/sucrose.ts
[warning] 557-557: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${context}(,|\\)), 'gs')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (4)
src/index.ts (1)
584-601: Form-data coercion gate looks correct and avoids touching non-TypeBox schemasThe new
additionalCoercelogic that firstresolveSchema(cloned.body, models, modules)and then requiresKind in resolved && (hasType('File', resolved) || hasType('Files', resolved))before switching tocoerceFormData()is a good guardrail: it keeps form-data coercion limited to actual TypeBox file schemas and falls back tocoercePrimitiveRoot()otherwise. This aligns with the intent to support nested form data without impacting standard/custom schemas.Also applies to: 652-665
test/types/utils.ts (1)
1-17: Type-level test correctly validatessafeParse().datainferenceThe new test directly asserts that
getSchemaValidator(schema).safeParse(...).datais{ id: number; name: string }on success, which matches the intendedUnwrapSchemabehavior for object schemas. This is a good, minimal guard for future typing changes.test/sucrose/sucrose.test.ts (1)
311-330: New sucrose test usefully extends context-inference coverageThe
handle context pass to function with sub contextcase validates that passingcontextas the single handler parameter—and only accessing a nested property—still flips all inference flags totrue. This matches the newisContextPassToFunctionlogic and strengthens regression coverage around context-based inference.src/schema.ts (1)
24-35: UnwrapSchema integration and resolveSchema helper correctly align with runtime behavior
- Importing
UnwrapSchemaand updatingElysiaTypeCheck<T>soClean,parse, and the success branch ofsafeParseall returnUnwrapSchema<T>matches how TypeBox and StandardSchemaV1Like schemas are decoded at runtime. For TypeBox schemas,UnwrapSchemaextracts the static output type viaTImport['static']; for StandardSchemaV1Like, it usesSchema['~standard']['types']['output'].- The new
resolveSchemahelper (string → module$defs→ models map) centralizes schema resolution and is used from other modules to determine coercion strategy without duplicating lookup logic.- The
replaceSchemaTypeFromManyOptionscall for root object schemas withoutadditionalPropertiesset (wrapping them into an explicitt.Object(..., { additionalProperties: false })) is a targeted normalization that keeps response/body cleaning strict by default while honoring explicit settings.- The type-level test in
test/types/utils.tsconfirms thatgetSchemaValidator(t.Object({...})).safeParse().datais correctly typed as the unwrapped static type, validating the end-to-end behavior.
| console.log( | ||
| isContextPassToFunction(mainParameter, code, fnInference) | ||
| ) | ||
|
|
||
| if (!isContextPassToFunction(mainParameter, code, fnInference)) |
There was a problem hiding this comment.
Remove debug console.log from sucrose inference loop
Inside sucrose(), after trimming the handler body, you now log the result of isContextPassToFunction:
console.log(
isContextPassToFunction(mainParameter, code, fnInference)
)This will execute for every analyzed handler in production, producing noisy logs and adding overhead in hot paths where routing/inference is performed.
Recommend either removing this log entirely or guarding it behind an explicit debug flag / environment check so it doesn’t run in normal usage.
🤖 Prompt for AI Agents
In src/sucrose.ts around lines 733 to 737, a debug console.log prints the result
of isContextPassToFunction on every handler; remove that console.log or wrap it
with a conditional debug flag (e.g., if (process.env.DEBUG_SUCROSE === 'true') )
so it only runs during explicit debugging; keep the subsequent if check intact
and ensure no stray logging remains in the hot inference loop.
| it('call local afterResponse on aot: false', async () => { | ||
| let called = false | ||
|
|
||
| const app = new Elysia({ aot: false }) | ||
| .use(IdsModel) | ||
| .get('/', ({ query }) => query, { | ||
| name: 'ids' | ||
| }) | ||
|
|
||
| const data = await app | ||
| .handle(req('/?names=rapi&names=anis')) | ||
| .then((x) => x.json()) | ||
|
|
||
| expect(data).toEqual({ | ||
| names: ['rapi', 'anis'] | ||
| }) | ||
| }) | ||
| .guard( | ||
| { | ||
| afterResponse: () => { | ||
| called = true | ||
| } | ||
| }, | ||
| (app) => app.get('/test', () => 'afterResponse') | ||
| ) | ||
| .get('/', () => 'hi') | ||
|
|
||
| const value = await app.handle(req('/test')).then((x) => x.text()) | ||
| await Bun.sleep(6.7) | ||
|
|
||
| expect(value).toBe('afterResponse') | ||
| expect(called).toBeTrue() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Bun.sleep milliseconds unit documentation
💡 Result:
Bun.sleep expects milliseconds (number) — or a Date to sleep until. Example: await Bun.sleep(1000) sleeps ~1000 ms. Bun.sleepSync also uses milliseconds. [1][2][3]
Sources:
[1] Bun API — Bun.sleep.
[2] Bun runtime/utils documentation — Bun.sleep.
[3] Bun v0.5.8 release notes (sleepSync uses milliseconds).
🏁 Script executed:
cd test/core && wc -l dynamic.test.tsRepository: elysiajs/elysia
Length of output: 78
🏁 Script executed:
sed -n '697,750p' test/core/dynamic.test.tsRepository: elysiajs/elysia
Length of output: 1346
Make afterResponse timing in this test more robust and remove dead code
The new call local afterResponse on aot: false test correctly exercises a local afterResponse hook in dynamic mode, but it relies on await Bun.sleep(6.7) (6.7 milliseconds) as a timing buffer before asserting called === true. This is a very tight margin and can be flaky under CI load or slower environments where afterResponse scheduling may not complete within that window.
Consider either:
- Increasing the wait to a more conservative value (e.g. 50–100ms), or
- Refactoring to assert via a polling loop or explicit signal from the scheduler instead of a hard-coded timeout.
Additionally, the large commented-out block of duplicate "handle query array reference…" tests immediately below this test should be deleted entirely rather than left commented.
🤖 Prompt for AI Agents
In test/core/dynamic.test.ts around lines 697 to 716, the test uses a very short
fixed delay (await Bun.sleep(6.7)) before asserting the afterResponse hook was
called, which is flaky; replace that hard-coded 6.7ms wait with a more robust
approach such as increasing the delay to a conservative 50–100ms or, better,
poll/wait until the `called` flag becomes true with a short timeout to avoid
flakiness; also remove the large commented-out block of duplicate "handle query
array reference…" tests immediately below this test.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/index.ts (2)
591-609: Form-data detection viaresolveSchemais sound; consider deduplicating helperUsing
resolveSchema+hasType('File' | 'Files')to switch betweencoerceFormData()andcoercePrimitiveRoot()is a solid way to support nested/file schemas, including refs and named models, in both precompiled and lazy body validators.You have essentially identical closures in the precompile and lazy branches; factoring this into a small local helper (e.g.,
getBodyCoercion(cloned.body, models, modules)) would remove duplication without changing behavior.Also applies to: 671-685
3840-3873: Empty-prefixgrouptyping fix aligns with new tests; optional schema-path tweakSwitching the grouped instance’s
BasePathtoPrefix extends '' ? BasePath : JoinPath<BasePath, Prefix>fixes the double‑slash / extra segment issue forgroup('')at the type level, matching the newRoutes = 'ok'test.For full internal consistency, you might also want to apply the same conditional inside this overload’s
UnwrapRoute<..., JoinPath<BasePath, Prefix>>arguments (and possibly in the second overload), so schema unwrapping uses the same notion of “effective base path” whenPrefixis''. That’s type-level only and can be done later if you start relying on those paths more heavily.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)example/a.ts(1 hunks)src/index.ts(6 hunks)test/types/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- example/a.ts
🧰 Additional context used
🧬 Code graph analysis (2)
test/types/index.ts (1)
src/index.ts (2)
Elysia(190-8153)Elysia(8155-8155)
src/index.ts (2)
src/schema.ts (2)
resolveSchema(130-146)hasType(148-184)src/types.ts (1)
JoinPath(2582-2585)
🔇 Additional comments (2)
test/types/index.ts (1)
2925-2937: Empty-prefix group type test matches intent and implementationThe new
group('', ...)test correctly asserts that~Routesexposes a single'ok'key, matching the updatedgrouptyping for empty prefixes. Looks consistent with surrounding type tests and thegroupoverload change insrc/index.ts.src/index.ts (1)
45-52: Schema/coercion imports and new exports look consistentImporting
resolveSchemaalongsidehasType, moving coercion helpers (coercePrimitiveRoot,coerceFormData,queryCoercions,stringToStructureCoercions) into their own module, and re‑exportinggetSchemaValidator/getResponseSchemaValidator/replaceSchemaTypefrom the root all line up cleanly with the rest of the file and broaden the public API without altering existing behavior.Also applies to: 167-172, 8175-8176
Improvement
Bug fix:
Summary by CodeRabbit
Bug Fixes
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.