fix(http-factory): harden review P0/P1/P2 fixes - #22
Merged
Conversation
Prevent double onError on refresh failure, keep headersProvider after explicit Authorization, prefer 401 refresh over generic retry, and make dedupe in-flight with configurable methods and stable keys.
Use neutral English default messages, rename authFailureCodes to refreshFailureCodes, and match skipRefreshUrls by path segments instead of substring includes.
Document and verify that proactive refresh failures still notify onError/onAuthFailure without blocking the current request.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
WalkthroughChangesHTTP 客户端行为
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpClient
participant RefreshManager
participant TokenStore
Client->>HttpClient: 发起请求
HttpClient-->>Client: 返回 401
HttpClient->>RefreshManager: 执行或复用刷新
RefreshManager->>TokenStore: 保存新 access token
RefreshManager-->>HttpClient: 返回刷新结果
HttpClient->>HttpClient: replayRequest 重试原请求
HttpClient-->>Client: 返回最终响应或错误
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/services/api/http-factory/index.ts (1)
71-98: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
instance.request的去重拦截覆盖不到http.get()
Axios 的get/delete/head/options别名方法绑定到内部context，实际调用的是context.request，不是外层重写后的instance.request。这样http.get()仍会绕过去重逻辑；把去重放到 adapter/interceptor，或显式覆盖完整调用入口。🤖 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 `@apps/web/src/services/api/http-factory/index.ts` around lines 71 - 98, Update the deduplication interception around instance.request so it also applies to Axios alias methods such as get, delete, head, and options, whose calls use the internal context.request. Move the GET deduplication logic into a shared adapter/interceptor path or explicitly override all relevant request entry points, while preserving request-level policy overrides and non-GET bypass behavior.
🤖 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
`@apps/web/src/services/api/http-factory/__tests__/http-client.edge-cases.test.ts`:
- Around line 507-545: 在“冷却期内 getAccessToken 为空时不重试原请求”测试中，补充对已声明的 onAuthFailure
mock 的调用断言，验证鉴权失败路径在抛出 Login session has expired 时也会调用该回调，并保持现有
refreshAccessToken 调用次数断言。
In
`@apps/web/src/services/api/http-factory/__tests__/http-client.refresh.test.ts`:
- Around line 42-69: 移除这些 shouldSkipRefresh 测试中的 as any，改用返回
InternalAxiosRequestConfig & RequestRetryState 的测试 helper，或复用现有测试配置类型构造请求配置；保持现有
URL 匹配断言不变，并确保测试继续遵循禁止 any 的类型约束。
In `@apps/web/src/services/api/http-factory/__tests__/http-client.test.ts`:
- Around line 1281-1305: Update the test “默认 key 会归一化 method 大小写并纳入 baseURL” to
issue concurrent requests with the same URL but different baseURL values, and
assert both requests are executed (requestCount equals 2). Preserve the existing
assertion that method case normalization deduplicates equivalent requests, while
explicitly verifying baseURL participates in the deduplication key.
- Around line 167-175: Remove the `any` types in `http-client.test.ts`: type
`retriedConfig` with a minimal structure containing the inspected `__skipDedupe`
field, and update the response handling around lines 1355–1364 to cast through
`unknown` before applying the project’s existing response type. Preserve the
current assertions and test behavior.
In
`@apps/web/src/services/api/http-factory/__tests__/http-client.token-normalization.test.ts`:
- Around line 179-268: Replace the fixed 10ms delays in both active-refresh
tests with deferred Promise synchronization. Resolve the deferred signal from
the onError callback, then await that signal before asserting refresh outcomes,
while preserving the existing onError and onAuthFailure expectations.
In `@apps/web/src/services/api/http-factory/dedupe-manager.ts`:
- Around line 19-62: 更新 DedupeManager 的 defaultGenerateKey，使去重 key 基于注入最终
headers 后的请求语义生成：使用最终 URI，纳入 paramsSerializer 处理后的参数，并保留 Date 等值的正确序列化；同时将
Authorization 和租户作用域加入 key，确保不同用户或租户的请求不会合并。调整调用时机，使其发生在请求拦截器完成身份与租户 headers
注入之后，并相应复用或更新 stableSerialize/normalizeForSerialize。
In `@apps/web/src/services/api/http-factory/index.ts`:
- Around line 53-56: Update the deduplication setup in the HTTP factory around
dedupePolicy and DedupeManager so request-level dedupePolicy values override the
client-level configuration. Resolve the effective enabled state and generateKey
from request options first, falling back to client defaults, then create
DedupeManager whenever the effective policy is enabled and use its effective key
generator.
- Around line 213-217: Update the request-header flow around headersProvider and
needToken to normalize and merge all caller/provider headers into an
AxiosHeaders instance before evaluating token requirements. Use AxiosHeaders.get
for case-insensitive detection of an existing access-token header, then only
read or refresh the local token when needed; inject it with AxiosHeaders.set so
custom authorization headers are correctly respected or overridden.
In `@apps/web/src/services/api/http-factory/reference/http-demo.ts`:
- Around line 70-73: 将 refreshFailureCodes 的中文注释限定为用于判断 refresh 请求响应是否表示 refresh
token 失效，避免将其描述为普通请求通用的鉴权失败配置；仅修改该注释，保留状态码列表不变。
In `@apps/web/src/services/api/http-factory/reference/README.md`:
- Around line 15-17: Update the README sections around the refresh-failure
documentation, including the references near lines 14, 274, 298, 315, and 320,
to match the current isRefreshFailure default behavior and tests. Remove claims
that every refresh failure invokes onAuthFailure, that missing access tokens
automatically trigger it, or that non-Error exceptions do so by default;
document the actual conditions and error propagation behavior instead.
---
Outside diff comments:
In `@apps/web/src/services/api/http-factory/index.ts`:
- Around line 71-98: Update the deduplication interception around
instance.request so it also applies to Axios alias methods such as get, delete,
head, and options, whose calls use the internal context.request. Move the GET
deduplication logic into a shared adapter/interceptor path or explicitly
override all relevant request entry points, while preserving request-level
policy overrides and non-GET bypass behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d99d3a73-7f8e-43b3-b4c3-de7c31e8911d
📒 Files selected for processing (13)
apps/web/src/services/api/http-factory/__tests__/http-client.edge-cases.test.tsapps/web/src/services/api/http-factory/__tests__/http-client.refresh.test.tsapps/web/src/services/api/http-factory/__tests__/http-client.test.tsapps/web/src/services/api/http-factory/__tests__/http-client.token-normalization.test.tsapps/web/src/services/api/http-factory/constants.tsapps/web/src/services/api/http-factory/dedupe-manager.tsapps/web/src/services/api/http-factory/index.tsapps/web/src/services/api/http-factory/reference/README.mdapps/web/src/services/api/http-factory/reference/http-demo.tsapps/web/src/services/api/http-factory/types/common.tsapps/web/src/services/api/http-factory/types/http-client-options.tsapps/web/src/services/api/http-factory/utils/refresh.tsapps/web/src/services/api/new-http.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/services/api/http-factory/types/http-client-options.ts (1)
24-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value接口命名不符合项目编码规范。
根据编码规范，TypeScript 接口应使用
I前缀（如IUser）。新增的RequestDedupePolicy接口缺少I前缀，应为IRequestDedupePolicy。同理，axios 模块增强中引用该类型处也需同步更新。注意到现有
DedupePolicy接口同样未遵循此规范，但新增代码应遵守规范。As per coding guidelines:
**/*.{ts,tsx}: Use PascalCase for type and interface names, withIprefix for interfaces (e.g.,IUser)♻️ 建议的命名修正
-/** - * 请求级合并配置。 - * 仅允许覆盖 enabled；generateKey 只能在客户端级配置。 - */ -export interface RequestDedupePolicy { - /** 是否启用请求合并。覆盖客户端级 enabled。 */ - enabled?: boolean; -} +/** + * 请求级合并配置。 + * 仅允许覆盖 enabled；generateKey 只能在客户端级配置。 + */ +export interface IRequestDedupePolicy { + /** 是否启用请求合并。覆盖客户端级 enabled。 */ + enabled?: boolean; +}以及同步更新 axios 模块增强：
declare module "axios" { interface AxiosRequestConfig { - dedupePolicy?: RequestDedupePolicy; + dedupePolicy?: IRequestDedupePolicy; } }🤖 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 `@apps/web/src/services/api/http-factory/types/http-client-options.ts` around lines 24 - 42, 将新增接口 RequestDedupePolicy 重命名为 IRequestDedupePolicy，并同步更新 AxiosRequestConfig.dedupePolicy 的类型引用；不要修改现有 DedupePolicy 或其他无关代码。Source: Coding guidelines
🤖 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 `@apps/web/src/services/api/http-factory/types/http-client-options.ts`:
- Around line 24-42: 将新增接口 RequestDedupePolicy 重命名为 IRequestDedupePolicy，并同步更新
AxiosRequestConfig.dedupePolicy 的类型引用；不要修改现有 DedupePolicy 或其他无关代码。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 59eaafb8-3625-4e36-a61f-41cbc910473d
📒 Files selected for processing (8)
apps/web/src/services/api/http-factory/__tests__/http-client.refresh.test.tsapps/web/src/services/api/http-factory/__tests__/http-client.test.tsapps/web/src/services/api/http-factory/__tests__/http-client.token-normalization.test.tsapps/web/src/services/api/http-factory/index.tsapps/web/src/services/api/http-factory/reference/README.mdapps/web/src/services/api/http-factory/reference/http-demo.tsapps/web/src/services/api/http-factory/types/common.tsapps/web/src/services/api/http-factory/types/http-client-options.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/src/services/api/http-factory/tests/http-client.token-normalization.test.ts
- apps/web/src/services/api/http-factory/reference/http-demo.ts
- apps/web/src/services/api/http-factory/tests/http-client.refresh.test.ts
- apps/web/src/services/api/http-factory/reference/README.md
- apps/web/src/services/api/http-factory/index.ts
- apps/web/src/services/api/http-factory/tests/http-client.test.ts
Owner
Author
@coderabbitai 忽视该问题 |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
|
Owner
Author
|
@coderabbitai approve |
Contributor
✅ Action performedComments resolved and changes approved. |
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
1 participant
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.
💻 变更类型 | Change Type
🔀 变更说明 | Description of Change
按 http-factory 高召回审查结论完成 P0/P1/P2 修复，重点收敛鉴权刷新、dedupe、错误处理与文档契约，避免错误登出、空 token 重试、请求挂起和配置误伤。
P0
defaultIsRefreshFailure：非 AxiosError 默认不再自动鉴权失败onBusinessResponse响应替换判定，避免{status,data}业务对象误替换P1
__skipDedupe字段，内部 refresh/retry 统一走replayRequestskipRefreshUrls收窄为 exact/prefix，移除中间段滑动匹配P2
rejectWithError收敛错误路径headersProvider并行获取new-http增加中文errorMessages验证
yarn workspace @my-first-nest/web test src/services/api/http-factoryyarn workspace @my-first-nest/web type-check📝 补充信息 | Additional Information
.tasks/2026-07-12-http-factory-high-recall-review/00-overview.mdexpiresAt === 0最终保持无效，不触发主动刷新🔗 关联 Issue | Related Issue
无
Summary by CodeRabbit
新功能
改进
refreshFailureCodes（相关判定逻辑同步更新）。