Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds route parameter validation to ChangesValidated handler validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/utils/internal/promise.ts`:
- Around line 7-9: Update the `chain` function to detect promise-like values by
checking for a callable `then` property instead of using `instanceof Promise`,
and invoke that thenable’s `then` with `fn`. Preserve direct synchronous
invocation for non-thenable values.
In `@src/utils/internal/validate.ts`:
- Around line 194-219: Update applyParams in validatedParams to merge validated
parameter values into the existing event.context.params object key-by-key,
matching applyHeaders and applyQuery, instead of replacing the container.
Preserve unvalidated router parameters while applying validated values and
returning the merged params.
🪄 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: 1114706e-ec2c-4db2-9edb-5b8e57fba25c
📒 Files selected for processing (6)
src/handler.tssrc/utils/internal/promise.tssrc/utils/internal/validate.tssrc/utils/request.tstest/handler.test.tstest/unit/types.test-d.ts
There was a problem hiding this comment.
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/handler.test.ts`:
- Around line 461-462: Replace both runtime-matrix requests in the test around
the mounted route with ctx.fetch, using the handlers mounted through the test
context instead of app.request. Preserve the existing URLs and assertions while
ensuring both requests use the shared cross-runtime URL resolution path.
🪄 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: becf4674-6630-4fda-9b60-0c6f9d49862c
📒 Files selected for processing (2)
src/utils/internal/validate.tstest/handler.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/internal/validate.ts
Coercing params schemas put non-strings in `context.params`; caller-side `decode: true` crashed on them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Document the sequential params → headers → query validation order and short-circuit, the `context.params` write-back side effect, and the `decodeParams` option. Also note why `StringsOnly` infers `never` for coerced (non-string) params/query fields. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Followup for #1491, discussed in #1437
This adds params validation in
defineValidatedHandler, while also chaining promises as discussed in the prior PR, fast-tracking for sync operations.The only limitation of this is that types do not actually rappresent what
event.context.paramsare, since they are hardcoded intoRecord<string, string>. For this I would propose a followup PR which would also allow to properly type theH3EventContextand add room for future improvement that I've suggested in #1437 (comment)Summary by CodeRabbit
paramsvalidation to validated handlers, including optionaldecodeParamsto decode values before validation.routerParams, and standardizing query string typing.