Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR strengthens TypeScript typings across the codebase: introduces a reusable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (3)
test/unit/path-rewriter.spec.ts (1)
161-161: Redundant cast:as unknown as anyis unnecessary.The async function
async () => {}already satisfiesPathRewriteConfig(matches the async function variant). The double cast is redundant.♻️ Proposed simplification
it('should not throw when async function config is provided', () => { - expect(badFn((async () => {}) as unknown as any)).not.toThrow(Error); + expect(badFn(async () => '')).not.toThrow(Error); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/path-rewriter.spec.ts` at line 161, The test assertion uses a redundant double-cast on the async lambda; update the call to badFn to pass the async function directly (replace badFn((async () => {}) as unknown as any) with badFn(async () => {})) so the async function variant of PathRewriteConfig is used without unnecessary casts; ensure the assertion still reads expect(badFn(async () => {})).not.toThrow(Error) and run typesafe checks for badFn and PathRewriteConfig.src/types.ts (1)
61-65: Inconsistent return types between sync and async path rewrite functions.The sync function variant allows
string | undefinedreturn, but the async variant requiresPromise<string>. Given thatapplyPathRewriteinsrc/http-proxy-middleware.ts(line 218) handlesundefinedwithif (typeof path === 'string'), the async variant should likely also allow returningundefined:♻️ Proposed fix for consistent return types
export type PathRewriteConfig<TReq extends http.IncomingMessage = http.IncomingMessage> = | { [regexp: string]: string } | ((path: string, req: TReq) => string | undefined) - | ((path: string, req: TReq) => Promise<string>); + | ((path: string, req: TReq) => Promise<string | undefined>);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types.ts` around lines 61 - 65, The PathRewriteConfig type is inconsistent: the synchronous function variant allows returning string | undefined but the async variant forces Promise<string>; update the async variant to Promise<string | undefined> so applyPathRewrite's logic (which checks if typeof path === 'string') works for both sync and async rewrites; modify the union variant in the PathRewriteConfig declaration to use ((path: string, req: TReq) => Promise<string | undefined>) and keep references to PathRewriteConfig and applyPathRewrite to locate the change.src/path-rewriter.ts (1)
33-45: Type signature inconsistency:rewritePathshould acceptreqparameter for consistency.The
rewritePathfunction only acceptspath: string, butapplyPathRewriteinsrc/http-proxy-middleware.ts(line 216) calls the rewriter with two arguments:pathRewriter(req.url as string, req).While JavaScript silently ignores extra arguments so this works at runtime, the inconsistent signature creates a confusing union return type. Consider adding the unused
reqparameter for type consistency:♻️ Proposed fix for signature consistency
- function rewritePath(path: string) { + function rewritePath(path: string, _req?: TReq) { let result = path; for (const rule of rulesCache) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/path-rewriter.ts` around lines 33 - 45, The rewritePath function signature is inconsistent with how it's invoked (applyPathRewrite calls pathRewriter(req.url as string, req)); update rewritePath to accept the second parameter (e.g., req: IncomingMessage | Request | any) even if unused, so the type signature matches callers and avoids union return type confusion. Edit the function declaration for rewritePath to add the req parameter, keep the internals using only the path and rulesCache/regex logic, and ensure any exported/typed declarations referencing rewritePath are updated to the new (path, req) signature for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/response-interceptor.ts`:
- Around line 123-127: The handler is assuming header values are always present
and casts value to string[] before operating on them, which can throw if value
is undefined (notably for 'set-cookie'); update the logic around the value
variable used before response.setHeader so you first check for undefined and
only process when value is defined, normalize string into an array safely (e.g.,
if value is a string convert to [value]), then map over the array calling
replace (or skip elements that are undefined), and finally call
response.setHeader(key, value) with the guarded/normalized readonly string[];
reference the value variable, the response.setHeader call, and the 'set-cookie'
header handling in your fix.
- Around line 109-110: The assignment currently uses unsafe casts for
originalResponse.statusCode/statusMessage; replace them with type guards that
only assign when values are defined and of the correct primitive type (e.g.,
check typeof originalResponse.statusCode === 'number' before setting
response.statusCode, and typeof originalResponse.statusMessage === 'string'
before setting response.statusMessage) so you don't overwrite response with
undefined or invalid values; keep the variables response and originalResponse
and update the logic where they are set in response-interceptor.ts accordingly,
leaving defaults untouched when guards fail.
In `@test/e2e/test-kit.ts`:
- Line 10: The createApp signature currently accepts only RequestHandler[] which
forces casting for Express error middleware; change the parameter type to accept
both request and error handlers (e.g., update createApp(...middlewares:
Array<RequestHandler | ErrorRequestHandler>): Express) so consumers can pass
ErrorRequestHandler instances without casting; adjust the import/usage to
reference Express's RequestHandler and ErrorRequestHandler types where createApp
is defined and update any call sites/tests if needed.
---
Nitpick comments:
In `@src/path-rewriter.ts`:
- Around line 33-45: The rewritePath function signature is inconsistent with how
it's invoked (applyPathRewrite calls pathRewriter(req.url as string, req));
update rewritePath to accept the second parameter (e.g., req: IncomingMessage |
Request | any) even if unused, so the type signature matches callers and avoids
union return type confusion. Edit the function declaration for rewritePath to
add the req parameter, keep the internals using only the path and
rulesCache/regex logic, and ensure any exported/typed declarations referencing
rewritePath are updated to the new (path, req) signature for consistency.
In `@src/types.ts`:
- Around line 61-65: The PathRewriteConfig type is inconsistent: the synchronous
function variant allows returning string | undefined but the async variant
forces Promise<string>; update the async variant to Promise<string | undefined>
so applyPathRewrite's logic (which checks if typeof path === 'string') works for
both sync and async rewrites; modify the union variant in the PathRewriteConfig
declaration to use ((path: string, req: TReq) => Promise<string | undefined>)
and keep references to PathRewriteConfig and applyPathRewrite to locate the
change.
In `@test/unit/path-rewriter.spec.ts`:
- Line 161: The test assertion uses a redundant double-cast on the async lambda;
update the call to badFn to pass the async function directly (replace
badFn((async () => {}) as unknown as any) with badFn(async () => {})) so the
async function variant of PathRewriteConfig is used without unnecessary casts;
ensure the assertion still reads expect(badFn(async () =>
{})).not.toThrow(Error) and run typesafe checks for badFn and PathRewriteConfig.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39b7d1a9-5d2d-45fe-9d5e-948a2bf1225c
📒 Files selected for processing (15)
src/handlers/response-interceptor.tssrc/http-proxy-middleware.tssrc/path-rewriter.tssrc/router.tssrc/types.tstest/e2e/express-error-middleware.spec.tstest/e2e/express-router.spec.tstest/e2e/http-proxy-middleware.spec.tstest/e2e/router.spec.tstest/e2e/test-kit.tstest/types.spec.tstest/unit/path-filter.spec.tstest/unit/path-rewriter.spec.tstest/unit/router.spec.tstsconfig.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores