patch: 1.4.16 #1529
patch: 1.4.16 #1529
Conversation
|
Warning Rate limit exceeded@SaltyAom has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds Cloudflare-targeted test steps to CI and publish workflows; expands ValidationError with valueError/messageValue, new constructor/detail signature; adds websocket ping/pong handlers and changes Bun adapter websocket error return; extends macro typing and adds an introspect hook; updates examples, tests, package script and dependency, and adds CHANGELOG v1.4.16. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI Workflow
participant TestA as Test (existing)
participant TestCF as Test (bun run test:cf)
participant Publish as Publish / Preview
CI->>TestA: run existing test step
CI->>TestCF: run added `bun run test:cf`
TestA-->>CI: results
TestCF-->>CI: results
CI->>Publish: continue to Publish/Preview
Note right of TestCF `#ccffdf`: CF-targeted tests run before publish
sequenceDiagram
autonumber
participant Request as Incoming Request
participant Validator as Schema Validator
participant VE as ValueError
participant VErr as ValidationError
participant App as onError Handler
Request->>Validator: validate input
alt validation fails
Validator-->>VE: produce ValueError (path, message, value)
VE->>VErr: attach as `valueError`, normalize path/property
VErr->>App: call detail(message, allowUnsafeValidatorDetails?)
App-->>Request: return error payload
end
sequenceDiagram
autonumber
participant Client as WebSocket client
participant Server as Elysia ws handler
participant AppData as ws.data handlers
Client->>Server: connect -> ws created
Client->>Server: send ping
Server->>AppData: websocket.ping(ws) -> calls ws.data.ping?.(ws)
Client->>Server: send pong
Server->>AppData: websocket.pong(ws) -> calls ws.data.pong?.(ws)
Client->>Server: send message
Server->>AppData: message handler invoked
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/ci.yml(1 hunks).github/workflows/publish.yml(1 hunks)CHANGELOG.md(1 hunks)example/a.ts(1 hunks)package.json(1 hunks)src/error.ts(4 hunks)test/extends/error.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/extends/error.test.ts (2)
example/a.ts (1)
app(19-38)src/index.ts (4)
Elysia(185-8104)Elysia(8106-8106)error(3044-3084)post(5732-5823)
| let { message, path, value, type } = error | ||
|
|
||
| if (Array.isArray(path)) path = path[0] | ||
|
|
||
| const property = | ||
| typeof path === 'string' | ||
| ? path.slice(1).replaceAll('/', '.') | ||
| : 'unknown' | ||
|
|
||
| const property = path.slice(1).replaceAll('/', '.') | ||
| const isRoot = path === '' |
There was a problem hiding this comment.
Restore correct property names when handling array paths
When path comes in as an array (e.g., Zod issues return ['channel']), we collapse it to the first element and then call slice(1). That trims the first character, so "channel" is rendered as "hannel" and "otpTo" as "tpTo". The new Cloudflare test surfaces this regression in the response summary. Please preserve the full segment and only trim a leading slash when one is present.
I suggest transforming array paths into a dotted string and normalizing slashes conditionally:
- if (Array.isArray(path)) path = path[0]
-
- const property =
- typeof path === 'string'
- ? path.slice(1).replaceAll('/', '.')
- : 'unknown'
+ if (Array.isArray(path))
+ path = path
+ .map((segment) =>
+ typeof segment === 'number' ? `[${segment}]` : segment
+ )
+ .join('.')
+
+ const property =
+ typeof path === 'string'
+ ? (path.startsWith('/') ? path.slice(1) : path).replaceAll('/', '.')
+ : 'unknown'This keeps existing TypeBox behavior, fixes Zod summaries, and still handles slash-prefixed paths.
🤖 Prompt for AI Agents
In src/error.ts around lines 156-165, the code incorrectly trims the first
character of property names when path is an array (e.g., "channel" -> "hannel");
fix by first normalizing path into a string (if Array.isArray(path) join
segments with '.'), then only strip a leading slash when present (if the
normalized string startsWith('/') remove that single leading char), and finally
replace internal '/' with '.' to produce the property name; compute isRoot
against the normalized path (empty string) after this normalization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/ws/connection.test.ts (1)
2-2: Remove unused import.The
timport is not used anywhere in this file and should be removed to keep imports clean.Apply this diff:
-import { Elysia, t } from '../../src' +import { Elysia } from '../../src'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
example/a.ts(1 hunks)src/adapter/bun/index.ts(2 hunks)src/ws/index.ts(1 hunks)test/ws/connection.test.ts(2 hunks)test/ws/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
test/ws/connection.test.ts (2)
src/index.ts (4)
Elysia(185-8104)Elysia(8106-8106)ws(6707-6790)resolve(1689-1704)src/adapter/bun/index.ts (1)
ws(427-637)
example/a.ts (2)
src/index.ts (3)
Elysia(185-8104)Elysia(8106-8106)ws(6707-6790)src/adapter/bun/index.ts (1)
ws(427-637)
src/ws/index.ts (2)
src/adapter/bun/index.ts (1)
ws(427-637)src/index.ts (1)
ws(6707-6790)
src/adapter/bun/index.ts (1)
src/error.ts (1)
status(86-94)
🪛 ast-grep (0.39.7)
example/a.ts
[warning] 20-20: Detected insecure WebSocket connection using 'ws://' protocol. WebSocket connections should use secure SSL/TLS connections with 'wss://' protocol to prevent man-in-the-middle attacks and ensure data confidentiality.
Context: new WebSocket("ws://localhost:3005")
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://developer.mozilla.org/en-US/docs/Web/API/WebSocket
- https://cwe.mitre.org/data/definitions/319.html
(websocket-secure-connection)
🔇 Additional comments (6)
src/adapter/bun/index.ts (2)
13-13: LGTM! Centralized error handling import.The addition of the
statusimport supports the improved WebSocket error handling pattern used later in the file.
630-630: LGTM! Cleaner error handling.The refactor to use the centralized
status()helper improves consistency with the framework's error handling patterns while maintaining equivalent behavior.test/ws/utils.ts (1)
3-3: LGTM! Type widening improves test utility flexibility.Changing from
ServertoServer<any>allows the utility to accommodate WebSocket servers with various payload types, improving compatibility across test scenarios.test/ws/connection.test.ts (1)
177-212: LGTM! Ping/pong handlers properly tested.The test correctly validates that ping and pong handlers are invoked during WebSocket lifecycle. The implementation properly:
- Sets up WebSocket with lifecycle handlers
- Triggers ping/pong events
- Validates handler invocation with assertions
Note: The
Bun.sleep(3)introduces a small timing dependency, but 3ms should be sufficient for local test execution.src/ws/index.ts (1)
28-36: LGTM! Ping/pong handlers follow established patterns.The new
pingandponglifecycle handlers are implemented consistently with existing handlers, using optional chaining to safely delegate to user-defined functions. This extends the WebSocket API with standard lifecycle events.example/a.ts (1)
1-27: LGTM! Excellent demonstration of WebSocket ping/pong API.The example effectively demonstrates the new WebSocket lifecycle handlers including
ping()andpong(). The implementation clearly shows:
- Handler registration for ping, pong, and message events
- Client-side WebSocket usage
- Event triggering flow
The use of
ws://(line 21) is appropriate for localhost development examples. For production usage, developers should usewss://for secure connections.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
example/a.ts (1)
18-19: Clarify the macroblog message.The
console.logstill says'before handle a', which is confusing when you're inside macrob. Tweaking the string keeps the example self-explanatory.- console.log('before handle a') + console.log('before handle b')CHANGELOG.md (1)
4-4: Polish the changelog wording.Consider switching to “accepts” so the sentence reads smoothly: “ValidationError.detail now accepts an optional second parameter
allowUnsafeValidatorDetails.”src/index.ts (1)
5427-5432: Guard introspection hook invocation
value?.(localHook)still attempts to call any truthy value, so a macro that mistakenly setsintrospectto something non-callable (e.g.,truefrom a JS consumer) will throwTypeError: value is not a function. Please gate the call with an explicit function check before invoking.- if (k === 'introspect') { - value?.(localHook) - - delete localHook[key] - continue - } + if (k === 'introspect') { + if (typeof value === 'function') value(localHook) + + delete localHook[key] + continue + }
📜 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(4 hunks)src/types.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
example/a.ts (1)
test/utils.ts (1)
req(1-2)
src/index.ts (1)
src/types.ts (1)
InputSchema(699-712)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
example/a.ts (1)
1-1: Remove unused import.
InternalSymbolNameis imported but never used in this file.Apply this diff:
-import { InternalSymbolName } from 'typescript' import { Elysia, t } from '../src'src/index.ts (1)
5334-5334: Unprofessional comment should be revised.The comment
// @ts-ignore trust me brois informal and unprofessional for production code. Consider replacing it with a clear explanation of why the type assertion is necessary.Apply this diff:
- // @ts-ignore trust me bro + // @ts-ignore - Input schema is guaranteed to satisfy constraint at this point
📜 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/adapter/web-standard/index.ts(1 hunks)src/index.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
example/a.ts (2)
src/index.ts (1)
route(6610-6706)test/utils.ts (1)
req(1-2)
src/index.ts (2)
src/compose.ts (1)
composeHandler(447-2192)src/types.ts (1)
ComposedHandler(1876-1876)
🔇 Additional comments (6)
src/adapter/web-standard/index.ts (1)
96-97: LGTM! Clean consolidation of path/query extraction.The refactored single-line expression correctly handles both cases (with and without query string) and improves code readability.
CHANGELOG.md (1)
1-16: LGTM! Well-documented release notes.The changelog entry clearly documents the improvements and bug fixes for version 1.4.16, including proper issue references. The entry for issue #1528 aligns with the PR objectives.
src/index.ts (4)
913-932: LGTM! Lazy compilation pattern implemented correctly.The defensive check on line 928 ensures
router.history[index]exists before updatingcomposed. This handles both precompiled routes (where the route object is set after compilation) and lazy-compiled routes (where this update happens on first call).
946-998: LGTM! Handler compilation strategy is well-structured.The dual-path approach cleanly separates precompiled routes (lines 948-949) from lazy-compiled routes (lines 951-953), with the lazy path storing the compiled result in
route[index].composedon first invocation.
5240-5257: LGTM! Macro type enhancement is backward compatible.The addition of
[name in Name]?: boolean(lines 5243-5245) extends the macro system to support self-referential or recursive macro definitions. Since the property is optional, this change maintains backward compatibility with existing macro definitions.
5431-5436: LGTM! Introspection hook implemented correctly.The new introspection pathway invokes the introspection function with
localHookand removes the key after processing. This pattern is consistent with other macro hook handlers in this function (e.g., 'detail' handling on lines 5438-5446) and enables runtime inspection of hook configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/error.ts (1)
42-55: Type bug: undefined generic 'T' in SelectiveStatus return type confirmedThe bug is isolated to line 55 in
src/error.ts.SelectiveStatus<in out Res>declares onlyResas a generic parameter, yet its return type referencesElysiaCustomStatusResponse<Code, T>whereTis undefined in that scope. Other uses ofElysiaCustomStatusResponse<Code, T>in the codebase (lines 94 and context.ts line 86) correctly declareTas a constrained generic parameter.Apply fix A (inline the computed response type):
-export type SelectiveStatus<in out Res> = < +export type SelectiveStatus<in out Res> = < const Code extends | keyof Res | InvertedStatusMap[Extract<keyof InvertedStatusMap, keyof Res>] >( code: Code, response: Code extends keyof Res ? Res[Code] : Code extends keyof StatusMap ? // @ts-ignore StatusMap[Code] always valid because Code generic check Res[StatusMap[Code]] : never // @ts-ignore trust me bro -) => ElysiaCustomStatusResponse<Code, T> +) => ElysiaCustomStatusResponse< + Code, + Code extends keyof Res + ? Res[Code] + : Code extends keyof StatusMap + ? Res[StatusMap[Code]] + : never +>
♻️ Duplicate comments (1)
src/error.ts (1)
156-165: Don’t trim first character from property names; normalize path safelyArray paths (eg. Zod issues) become broken (“channel” → “hannel”) due to
slice(1)after collapsing to the first segment. Normalize by joining segments and stripping a single leading slash only when present.- let { message, path, value, type } = error + let { message, path, value, type } = error - if (Array.isArray(path)) path = path[0] + if (Array.isArray(path)) + path = path + .map((segment) => + typeof segment === 'number' ? `[${segment}]` : segment + ) + .join('.') - const property = - typeof path === 'string' - ? path.slice(1).replaceAll('/', '.') - : 'unknown' - - const isRoot = path === '' + const normalized = + typeof path === 'string' + ? (path.startsWith('/') ? path.slice(1) : path) + : '' + const property = normalized.replaceAll('/', '.') || 'root' + const isRoot = normalized === ''This preserves full property names and fixes Zod summaries.
Based on learnings
🧹 Nitpick comments (5)
CHANGELOG.md (1)
4-7: Tiny grammar nits in 1.4.16 entry
- “detail now accept” → “detail now accepts”.
- “merge multiple macro resolve response” → “responses”.
Apply:
- - ValidationError.detail now accept optional 2nd parameter `allowUnsafeValidatorDetails` + - ValidationError.detail now accepts optional 2nd parameter `allowUnsafeValidatorDetails` - - merge multiple macro resolve response + - merge multiple macro resolve responsesexample/a.ts (2)
3-4: Remove stray call to status(401)This value is unused and may trip linters. Delete it (or prefix with
voidif you must keep for side effects).-status(401)
16-16: Use a type alias for introspection instead of a runtime property accessAccessing
app['~Routes']...at runtime is unnecessary; make it type-only to avoid unused-expression linting.-app['~Routes']['multiple']['get']['response'] +type _MultipleResponse = (typeof app)['~Routes']['multiple']['get']['response']test/types/lifecycle/soundness.ts (1)
2238-2264: Remove duplicated assertionThe final
expectTypeOf<...>().toEqualTypeOf<...>()is repeated; keep one.- expectTypeOf< - (typeof app)['~Routes']['multiple']['get']['response'] - >().toEqualTypeOf<{ - 200: string - 401: 'Unauthorized' - 403: 'Forbidden' - }>() - - expectTypeOf< - (typeof app)['~Routes']['multiple']['get']['response'] - >().toEqualTypeOf<{ - 200: string - 401: 'Unauthorized' - 403: 'Forbidden' - }>() + expectTypeOf< + (typeof app)['~Routes']['multiple']['get']['response'] + >().toEqualTypeOf<{ + 200: string + 401: 'Unauthorized' + 403: 'Forbidden' + }>()src/types.ts (1)
1970-1976: Naming nit: avoid shadowing ‘Macro’The generic parameter name
MacroinMacroProperty<Macro,...>shadows theMacrointerface. Consider renaming the type parameter (e.g.,M) for clarity.-export interface MacroProperty< - in out Macro extends BaseMacro = {}, +export interface MacroProperty< + in out M extends BaseMacro = {}, ... - introspect?(option: Prettify<Macro>): unknown + introspect?(option: Prettify<M>): unknown
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)example/a.ts(1 hunks)src/error.ts(6 hunks)src/types.ts(5 hunks)test/types/lifecycle/soundness.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/types.ts (1)
src/error.ts (1)
ElysiaCustomStatusResponse(57-84)
example/a.ts (1)
src/error.ts (1)
status(86-94)
test/types/lifecycle/soundness.ts (1)
src/error.ts (1)
status(86-94)
src/error.ts (2)
src/index.ts (3)
SelectiveStatus(8177-8177)ElysiaCustomStatusResponse(8176-8176)error(3048-3088)test/types/lifecycle/soundness.ts (15)
error(60-62)error(126-128)error(193-195)error(275-277)error(695-697)error(722-724)error(754-756)error(787-789)error(1444-1446)error(1462-1464)error(1512-1514)error(1531-1533)error(1583-1585)error(1602-1604)error(1652-1654)
🔇 Additional comments (2)
src/types.ts (2)
969-981: Response merging logic looks correctWrapping status-mapped branches with
UnionToIntersectis appropriate for merging multiple resolve/macro outputs. Tests exercise this path.
1979-1993: Macro generic propagation verified through testsThe three test cases in
test/types/lifecycle/soundness.ts(lines 2196, 2218, 2238) confirm thatMacroProperty<Macro,...>correctly propagates status responses from theresolvefunction through the type system. All test variants compile without errors, validating the index signature implementation.
Improvement:
messageValueas an alias oferrorValueallowUnsafeValidatorDetailsBug fix:
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores