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:
📝 WalkthroughWalkthrough将大量固定 vh/dvh 高度改为基于新 CSS 变量 --cch-viewport-height-* 的计算并添加 safe-area 支持；providers 表单移动端导航与步骤进度更新；新增 ClientRestrictionsEditor；E2E 改为登录令牌流程；Redis 客户端类型与生命周期重构。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary of ChangesHello @tesgth032, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求主要解决了服务商表单在小屏和移动设备上的自适应问题，特别是修复了提交按钮被底部导航遮挡的缺陷。通过调整表单布局和统一对话框内容样式，提升了用户体验。此外，还包含了对单元测试配置的改进，以增强测试的稳定性和兼容性。 Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <div className="order-2 md:order-1 shrink-0"> | ||
| {/* Tab Navigation */} | ||
| <FormTabNav | ||
| activeTab={state.ui.activeTab} | ||
| onTabChange={handleTabChange} | ||
| disabled={isPending} | ||
| tabStatus={getTabStatus()} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
On small screens (< md breakpoint), the nav appears below content due to order-2 md:order-1, but on mobile the nav should typically be at the bottom for thumb accessibility. Check that this ordering matches the intended UX.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/index.tsx
Line: 556-564
Comment:
On small screens (< `md` breakpoint), the nav appears below content due to `order-2 md:order-1`, but on mobile the nav should typically be at the bottom for thumb accessibility. Check that this ordering matches the intended UX.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
本次 PR 主要包含两方面的改进：修复服务商表单在小屏幕/移动端的自适应布局问题，以及提升测试的稳定性。在 UI 方面，通过将之前在小屏幕上遮挡内容区域的固定底部导航栏，调整为文档流的一部分，成功解决了提交按钮被遮挡的问题。同时，统一了多个组件中 DialogContent 的样式，采用 p-0 和 overflow-hidden 等一致的布局属性，避免了双层内边距等问题，提升了布局的统一性和可维护性。使用 Flexbox 的 order 属性进行响应式布局调整，是一个清晰且有效的实现方式。在测试稳定性方面，简化了 docker-executor.test.ts 中对 Node.js 内置模块的 mock 方式，这有助于避免在某些 Vitest 环境下因模块提升（hoisting）而导致的加载错误。此外，更新了 Vitest 配置，允许通过环境变量设置 maxWorkers，从而更好地控制测试期间的资源占用。总体而言，这些变更是目标明确、实现清晰的改进，同时提升了用户体验和开发环境的稳定性。代码质量很高，我没有发现需要修改的问题。
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR addresses a mobile/small-screen UI issue where the provider form submit button was being obscured by the fixed bottom navigation. The changes are well-targeted and minimal.
PR Size: S
- Lines changed: 63 (35 additions, 28 deletions)
- Files changed: 8
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Change Analysis
UI Layout Fix (Primary Change)
form-tab-nav.tsx:214: Changed mobile navigation fromfixed bottom-0 left-0 right-0 z-50toshrink-0 relative- correctly removes the fixed positioning that was covering the submit buttonprovider-form/index.tsx:553-570: WrappedFormTabNavin a div withorder-2 md:order-1and changed content area toorder-1 md:order-2- this properly reorders the layout for mobile while maintaining desktop order- Removed
pb-24 md:pb-6padding from content area since fixed positioning is no longer used
Dialog Consistency
- 6 files: Added
overflow-hidden p-0 gap-0to DialogContent for consistent padding/overflow handling across all provider dialogs
Test Infrastructure
docker-executor.test.ts: Simplified mock pattern by removingimportOriginal, which resolves Vitest compatibility issuesvitest.config.ts: AddedparsePositiveInthelper andmaxWorkersconfig to limit worker count (defaults to 8, configurable viaVITEST_MAX_WORKERS)
Validation Performed
- Verified
parsePositiveInthandles all edge cases (undefined, NaN, non-positive values) - Confirmed the layout reordering logic is correct for both mobile and desktop viewports
- Checked that the mock changes don't affect test coverage
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate (CSS changes, test infrastructure improved)
- Code clarity - Good
Automated review by Claude AI
![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
🧹 Nitpick comments (2)
src/app/[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx (2)
268-268:findIndex回调参数t遮蔽了外层翻译函数t第 32 行定义了
const t = useTranslations("settings.providers.form")，而此处TAB_CONFIG.findIndex((t) => t.id === activeTab)中的参数t遮蔽了外层的翻译函数。虽然当前逻辑正确（t在此 lambda 内未被调用为翻译函数），但容易造成误解，建议改用更具描述性的参数名。建议修复
- width: `${((TAB_CONFIG.findIndex((t) => t.id === activeTab) + 1) / TAB_CONFIG.length) * 100}%`, + width: `${((TAB_CONFIG.findIndex((tab) => tab.id === activeTab) + 1) / TAB_CONFIG.length) * 100}%`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx at line 268, The findIndex callback parameter shadows the outer translation function const t = useTranslations("settings.providers.form"); update TAB_CONFIG.findIndex((t) => t.id === activeTab) to use a descriptive name (e.g., tab or cfg) to avoid shadowing and confusion—locate the expression in form-tab-nav.tsx where width is computed and replace the callback parameter name while keeping the same comparison against activeTab.
263-272: 进度条初始宽度硬编码为"20%"，与 TAB 数量耦合
initial={{ width: "20%" }}硬编码对应 5 个 Tab（1/5 = 20%）。若未来TAB_CONFIG数量变化，初始动画值将不匹配，导致组件首次挂载时出现不必要的动画跳跃（从错误的初始值动画到正确目标值）。建议修复：动态计算初始宽度
- initial={{ width: "20%" }} + initial={{ width: `${(1 / TAB_CONFIG.length) * 100}%` }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx around lines 263 - 272, The progress bar's initial width is hardcoded to "20%" causing jump when TAB_CONFIG length changes; update the motion.div initial width to compute dynamically using TAB_CONFIG and activeTab (e.g., find the index via TAB_CONFIG.findIndex(t => t.id === activeTab) and set initial width to ((indexFound >= 0 ? indexFound + 1 : 0) / TAB_CONFIG.length) * 100 + '%') so initial and animate widths match; reference the motion.div, TAB_CONFIG and activeTab when making this change and ensure you handle index not found fallback to 0%.
🤖 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/app/`[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx:
- Line 214: The custom class safe-area-bottom used in components like
form-tab-nav (component FormTabNav) and settings-nav is not defined in Tailwind
or globals.css, so add a utility definition in src/app/globals.css inside `@layer`
utilities that defines .safe-area-bottom to apply bottom padding using the iOS
safe-area inset (e.g. padding-bottom: env(safe-area-inset-bottom); with a
sensible fallback), then rebuild so Tailwind includes the new utility.
---
Nitpick comments:
In
`@src/app/`[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx:
- Line 268: The findIndex callback parameter shadows the outer translation
function const t = useTranslations("settings.providers.form"); update
TAB_CONFIG.findIndex((t) => t.id === activeTab) to use a descriptive name (e.g.,
tab or cfg) to avoid shadowing and confusion—locate the expression in
form-tab-nav.tsx where width is computed and replace the callback parameter name
while keeping the same comparison against activeTab.
- Around line 263-272: The progress bar's initial width is hardcoded to "20%"
causing jump when TAB_CONFIG length changes; update the motion.div initial width
to compute dynamically using TAB_CONFIG and activeTab (e.g., find the index via
TAB_CONFIG.findIndex(t => t.id === activeTab) and set initial width to
((indexFound >= 0 ? indexFound + 1 : 0) / TAB_CONFIG.length) * 100 + '%') so
initial and animate widths match; reference the motion.div, TAB_CONFIG and
activeTab when making this change and ensure you handle index not found fallback
to 0%.
src/app/[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx
Show resolved
Hide resolved
| initial={{ width: stepProgressWidth }} | ||
| animate={{ width: stepProgressWidth }} |
There was a problem hiding this comment.
initial should animate from current width, not jump to target width. Setting both to same value disables entry animation.
| initial={{ width: stepProgressWidth }} | |
| animate={{ width: stepProgressWidth }} | |
| initial={{ width: "0%" }} | |
| animate={{ width: stepProgressWidth }} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx
Line: 270-271
Comment:
`initial` should animate from current width, not jump to target width. Setting both to same value disables entry animation.
```suggestion
initial={{ width: "0%" }}
animate={{ width: stepProgressWidth }}
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/app/[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx (2)
45-47:stepProgressWidth在horizontal布局下被无效计算
layout === "horizontal"时组件在第 49 行提前返回，但这三个变量在每次渲染时都会被计算。建议将此段逻辑移入vertical分支内部，或包裹在layout !== "horizontal"条件中，避免冗余运算。♻️ 建议重构
- const activeTabIndex = TAB_CONFIG.findIndex((tab) => tab.id === activeTab); - const stepNumber = activeTabIndex >= 0 ? activeTabIndex + 1 : 0; - const stepProgressWidth = `${(stepNumber / TAB_CONFIG.length) * 100}%`; if (layout === "horizontal") { return ( ... ); } + const activeTabIndex = TAB_CONFIG.findIndex((tab) => tab.id === activeTab); + const stepNumber = activeTabIndex >= 0 ? activeTabIndex + 1 : 0; + const stepProgressWidth = `${(stepNumber / TAB_CONFIG.length) * 100}%`; return ( ... );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx around lines 45 - 47, The computed values activeTabIndex, stepNumber and stepProgressWidth are always calculated even when layout === "horizontal" (where the component returns early); move the logic that derives activeTabIndex, stepNumber and stepProgressWidth into the vertical branch (or wrap them with an if (layout !== "horizontal") guard) so those values are only computed when needed—use the existing TAB_CONFIG and activeTab symbols to locate and relocate the calculations (or wrap them) near the rendering logic that handles the vertical layout.
270-271:initial与animate值相同，mount 动画被抑制
initial在首次挂载时仅生效一次；因其值与animate完全相同，首次渲染不会产生入场动画。若希望进度条从0%滑入当前位置，应将initial改为"0%"；若刻意跳过入场动画，则可直接删除initial属性（framer-motion 默认行为与此等效）。♻️ 可选调整（带入场动画）
<motion.div className="h-full bg-primary" - initial={{ width: stepProgressWidth }} + initial={{ width: "0%" }} animate={{ width: stepProgressWidth }} transition={{ type: "spring", stiffness: 300, damping: 30 }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx around lines 270 - 271, The component sets the Framer Motion props initial and animate both to the same value (stepProgressWidth), so the mount animation is suppressed; change initial to a fixed starting value (e.g., "0%") to animate the progress bar in from zero, or remove the initial prop entirely if you want to skip the entrance animation—update the JSX where initial={{ width: stepProgressWidth }} / animate={{ width: stepProgressWidth }} (reference: stepProgressWidth in form-tab-nav.tsx) accordingly.src/app/globals.css (1)
138-141: Tailwind v4 中自定义工具类应使用@utility而非@layer utilities在 Tailwind v4 中，
@layer utilities不再由 Tailwind 特殊处理，因此定义在其中的类不会支持hover:、focus:或响应式前缀（如md:）等变体。应改用@utility指令以确保变体正常工作。该类已在以下位置使用：
src/app/[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx:218（无固定定位）src/app/[locale]/settings/_components/settings-nav.tsx:222（带fixed bottom-0）建议按照 Tailwind v4 最佳实践改为
@utility语法：♻️ 建议使用 `@utility` 指令
- /* iOS 安全区：避免底部固定导航被 Home Indicator 挡住 */ - .safe-area-bottom { - padding-bottom: env(safe-area-inset-bottom, 0px); - } +/* iOS 安全区：为底部内容提供 Home Indicator 安全间距 */ +@utility safe-area-bottom { + padding-bottom: env(safe-area-inset-bottom, 0px); +}注：
@utility应置于@layer utilities块之外，与@layer base同级。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/globals.css` around lines 138 - 141, The custom .safe-area-bottom utility is defined in a plain CSS layer so Tailwind v4 won't apply hover/focus/responsive variants; move its definition out of any `@layer` utilities block and convert it to a Tailwind `@utility` declaration (placed at the same level as `@layer` base) that sets padding-bottom: env(safe-area-inset-bottom, 0px); this ensures variant support for usages in components such as form-tab-nav.tsx (line with no fixed positioning) and settings-nav.tsx (where it’s used with fixed bottom-0).
🤖 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/app/`[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx:
- Around line 267-274: The visual progress indicator in form-tab-nav.tsx (the
outer div + motion.div used for step progress) lacks accessibility semantics;
update the element representing progress (the motion.div with className "h-full
bg-primary") to include role="progressbar" and ARIA attributes aria-valuenow
(current step progress value), aria-valuemin="0" and aria-valuemax="100", and a
descriptive aria-label or aria-labelledby so screen readers can announce
progress; ensure the code reads the same stepProgressWidth/value used for the
animated width to compute aria-valuenow.
---
Nitpick comments:
In
`@src/app/`[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx:
- Around line 45-47: The computed values activeTabIndex, stepNumber and
stepProgressWidth are always calculated even when layout === "horizontal" (where
the component returns early); move the logic that derives activeTabIndex,
stepNumber and stepProgressWidth into the vertical branch (or wrap them with an
if (layout !== "horizontal") guard) so those values are only computed when
needed—use the existing TAB_CONFIG and activeTab symbols to locate and relocate
the calculations (or wrap them) near the rendering logic that handles the
vertical layout.
- Around line 270-271: The component sets the Framer Motion props initial and
animate both to the same value (stepProgressWidth), so the mount animation is
suppressed; change initial to a fixed starting value (e.g., "0%") to animate the
progress bar in from zero, or remove the initial prop entirely if you want to
skip the entrance animation—update the JSX where initial={{ width:
stepProgressWidth }} / animate={{ width: stepProgressWidth }} (reference:
stepProgressWidth in form-tab-nav.tsx) accordingly.
In `@src/app/globals.css`:
- Around line 138-141: The custom .safe-area-bottom utility is defined in a
plain CSS layer so Tailwind v4 won't apply hover/focus/responsive variants; move
its definition out of any `@layer` utilities block and convert it to a Tailwind
`@utility` declaration (placed at the same level as `@layer` base) that sets
padding-bottom: env(safe-area-inset-bottom, 0px); this ensures variant support
for usages in components such as form-tab-nav.tsx (line with no fixed
positioning) and settings-nav.tsx (where it’s used with fixed bottom-0).
src/app/[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx
Outdated
Show resolved
Hide resolved
| return combined | ||
| .split(/,(?=[^;]+?=)/g) | ||
| .map((s) => s.trim()) | ||
| .filter(Boolean); |
There was a problem hiding this comment.
Regex /,(?=[^;]+?=)/g may incorrectly split Set-Cookie headers containing dates (e.g., Expires=Wed, 21 Oct 2026). The lookahead (?=[^;]+?=) prevents splitting at commas within individual cookie attributes, but date values like Expires=Wed, 21 Oct 2026 will still match because "21" is followed by =.
| return combined | |
| .split(/,(?=[^;]+?=)/g) | |
| .map((s) => s.trim()) | |
| .filter(Boolean); | |
| // Use getSetCookie() if available, otherwise parse manually | |
| // Note: Splitting Set-Cookie headers is complex due to date formats | |
| return combined | |
| .split(/,\s*(?=[a-zA-Z][\w-]*=)/g) | |
| .map((s) => s.trim()) | |
| .filter(Boolean); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/_helpers/auth.ts
Line: 18-21
Comment:
Regex `/,(?=[^;]+?=)/g` may incorrectly split Set-Cookie headers containing dates (e.g., `Expires=Wed, 21 Oct 2026`). The lookahead `(?=[^;]+?=)` prevents splitting at commas within individual cookie attributes, but date values like `Expires=Wed, 21 Oct 2026` will still match because "21" is followed by `=`.
```suggestion
// Use getSetCookie() if available, otherwise parse manually
// Note: Splitting Set-Cookie headers is complex due to date formats
return combined
.split(/,\s*(?=[a-zA-Z][\w-]*=)/g)
.map((s) => s.trim())
.filter(Boolean);
```
How can I resolve this? If you propose a fix, please make it concise.| <div className="order-2 md:order-1 shrink-0"> | ||
| {/* Tab Navigation */} | ||
| <FormTabNav | ||
| activeTab={state.ui.activeTab} | ||
| onTabChange={handleTabChange} | ||
| disabled={isPending} | ||
| tabStatus={getTabStatus()} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Wrapping FormTabNav in a separate div with shrink-0 prevents the nav from being a direct flex child of the parent. This adds an extra DOM node that may not be necessary.
| <div className="order-2 md:order-1 shrink-0"> | |
| {/* Tab Navigation */} | |
| <FormTabNav | |
| activeTab={state.ui.activeTab} | |
| onTabChange={handleTabChange} | |
| disabled={isPending} | |
| tabStatus={getTabStatus()} | |
| /> | |
| </div> | |
| {/* Tab Navigation */} | |
| <FormTabNav | |
| className="order-2 md:order-1 shrink-0" | |
| activeTab={state.ui.activeTab} | |
| onTabChange={handleTabChange} | |
| disabled={isPending} | |
| tabStatus={getTabStatus()} | |
| /> |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/index.tsx
Line: 556-564
Comment:
Wrapping `FormTabNav` in a separate `div` with `shrink-0` prevents the nav from being a direct flex child of the parent. This adds an extra DOM node that may not be necessary.
```suggestion
{/* Tab Navigation */}
<FormTabNav
className="order-2 md:order-1 shrink-0"
activeTab={state.ui.activeTab}
onTabChange={handleTabChange}
disabled={isPending}
tabStatus={getTabStatus()}
/>
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| // ==================== 辅助函数 ==================== | ||
|
|
||
| beforeAll(async () => { | ||
| if (!ADMIN_KEY) return; | ||
| authToken = await loginAndGetAuthToken(API_BASE_URL, ADMIN_KEY); | ||
| }); |
There was a problem hiding this comment.
Using beforeAll for async setup means if login fails, subsequent tests will fail with unclear errors about missing authToken. The error message on line 40-41 helps, but tests will still run and fail individually.
Consider using beforeAll with a skip condition or making the login failure more visible.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/api-complete.test.ts
Line: 30-36
Comment:
Using `beforeAll` for async setup means if login fails, subsequent tests will fail with unclear errors about missing `authToken`. The error message on line 40-41 helps, but tests will still run and fail individually.
Consider using `beforeAll` with a skip condition or making the login failure more visible.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/e2e/users-keys-complete.test.ts (1)
37-37:afterAll声明在beforeAll之前，且存在sessionToken/authToken双重命名存在两个小问题：
声明顺序：模块级别的
afterAll（第 171 行）出现在beforeAll（第 192 行）之前。虽然 Vitest 执行时始终先运行beforeAll再运行afterAll，与代码顺序无关，但与api-complete.test.ts及notification-settings.test.ts中"先beforeAll后afterAll"的惯例不一致，会给读者造成困惑。命名不一致：本文件模块变量命名为
sessionToken，但callApi的参数命名为authToken（默认值为sessionToken），在同一文件内形成双重命名，可统一为sessionToken。♻️ 建议调整声明顺序并统一命名
将
beforeAll移至afterAll之前（与其他测试文件对齐）：+beforeAll(async () => { + if (!ADMIN_KEY) return; + sessionToken = await loginAndGetAuthToken(API_BASE_URL, ADMIN_KEY); +}); + afterAll(async () => { if (!sessionToken) return; // ... cleanup ... }); - -beforeAll(async () => { - if (!ADMIN_KEY) return; - sessionToken = await loginAndGetAuthToken(API_BASE_URL, ADMIN_KEY); -});统一
callApi参数命名：async function callApi( module: string, action: string, body: Record<string, unknown> = {}, - authToken = sessionToken + sessionToken: string | undefined = sessionToken ) { - if (!authToken) { + if (!sessionToken) { throw new Error("E2E tests require ADMIN_TOKEN/TEST_ADMIN_TOKEN (used to login)"); } const response = await fetch(url, { headers: { - Authorization: `Bearer ${authToken}`, - Cookie: `auth-token=${authToken}`, + Authorization: `Bearer ${sessionToken}`, + Cookie: `auth-token=${sessionToken}`, }, });Also applies to: 65-65, 172-172, 192-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/users-keys-complete.test.ts` at line 37, Move the module-level beforeAll so it appears before the module-level afterAll (make the order match other tests) and rename the inconsistent authToken identifier to sessionToken: update the callApi function signature/default to use sessionToken and replace all authToken uses with sessionToken, ensuring the module variable let sessionToken: string | undefined; is the single source of truth and that afterAll (cleanup) stays after the now-earlier beforeAll setup.tests/e2e/api-complete.test.ts (1)
16-17:callApi/beforeAll认证逻辑在三个 E2E 测试文件中重复
callApi（含 Bearer + Cookie 头）和beforeAll登录流程在api-complete.test.ts、notification-settings.test.ts、users-keys-complete.test.ts中几乎完全相同，可考虑将其提取到tests/e2e/_helpers/api.ts，减少日后维护负担。Also applies to: 33-36, 47-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api-complete.test.ts` around lines 16 - 17, Extract the duplicated authentication and request helper into a shared module: create tests/e2e/_helpers/api.ts that exports a callApi function which builds requests with Bearer + Cookie headers and a helper to perform the beforeAll login flow (reusing loginAndGetAuthToken) so tests can call that in their beforeAll; then update api-complete.test.ts, notification-settings.test.ts, and users-keys-complete.test.ts to import and use callApi and the shared beforeAll/login helper instead of duplicating the logic in each file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/_helpers/auth.ts`:
- Around line 24-27: The extractCookieValue function builds a RegExp from the
cookieName which can lead to ReDoS or incorrect matches if cookieName contains
regex metacharacters; change extractCookieValue to parse the Set-Cookie string
with string operations instead: trim the header, split on ';' to get individual
cookie segments, find the segment that starts with cookieName + '=' (or compare
the segment name using substring/indexOf), then return the value via substring
or split('=') instead of using new RegExp; update references to
extractCookieValue accordingly.
---
Nitpick comments:
In `@tests/e2e/api-complete.test.ts`:
- Around line 16-17: Extract the duplicated authentication and request helper
into a shared module: create tests/e2e/_helpers/api.ts that exports a callApi
function which builds requests with Bearer + Cookie headers and a helper to
perform the beforeAll login flow (reusing loginAndGetAuthToken) so tests can
call that in their beforeAll; then update api-complete.test.ts,
notification-settings.test.ts, and users-keys-complete.test.ts to import and use
callApi and the shared beforeAll/login helper instead of duplicating the logic
in each file.
In `@tests/e2e/users-keys-complete.test.ts`:
- Line 37: Move the module-level beforeAll so it appears before the module-level
afterAll (make the order match other tests) and rename the inconsistent
authToken identifier to sessionToken: update the callApi function
signature/default to use sessionToken and replace all authToken uses with
sessionToken, ensuring the module variable let sessionToken: string | undefined;
is the single source of truth and that afterAll (cleanup) stays after the
now-earlier beforeAll setup.
tests/e2e/_helpers/auth.ts
Outdated
| const shouldRetry = response.status === 503 && errorCode === "SESSION_CREATE_FAILED"; | ||
| if (!shouldRetry || attempt >= maxAttempts) { | ||
| throw new Error(`[e2e] login failed: ${response.status} ${text}`); | ||
| } |
There was a problem hiding this comment.
Exponential backoff grows too slowly - 100 * 2^(attempt-1) reaches only 512ms at attempt 4. Since sleep caps at 1000ms, attempts 4-10 all wait the same 1s. Use 200 * 2^(attempt-1) or increase the cap.
| } | |
| await sleep(Math.min(2000, 200 * 2 ** (attempt - 1))); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/_helpers/auth.ts
Line: 62
Comment:
Exponential backoff grows too slowly - `100 * 2^(attempt-1)` reaches only 512ms at attempt 4. Since `sleep` caps at 1000ms, attempts 4-10 all wait the same 1s. Use `200 * 2^(attempt-1)` or increase the cap.
```suggestion
await sleep(Math.min(2000, 200 * 2 ** (attempt - 1)));
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| if (attempt >= maxAttempts) { | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
Same backoff issue - grows too slowly with 100ms base. Also, this retry is for non-503 errors which seems overly aggressive.
| await sleep(Math.min(2000, 200 * 2 ** (attempt - 1))); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/_helpers/auth.ts
Line: 83
Comment:
Same backoff issue - grows too slowly with 100ms base. Also, this retry is for non-503 errors which seems overly aggressive.
```suggestion
await sleep(Math.min(2000, 200 * 2 ** (attempt - 1)));
```
How can I resolve this? If you propose a fix, please make it concise.| disabled={isPending} | ||
| tabStatus={getTabStatus()} | ||
| /> | ||
| <div className="order-2 md:order-1 shrink-0"> |
There was a problem hiding this comment.
Verify that the submit button (likely in a footer component) is accessible on mobile now that nav is repositioned below content.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/index.tsx
Line: 556
Comment:
Verify that the submit button (likely in a footer component) is accessible on mobile now that nav is repositioned below content.
How can I resolve this? If you propose a fix, please make it concise.| maxConcurrency: 5, // 最大并发测试数 | ||
| pool: "threads", // 使用线程池（推荐） | ||
| // 高核机器/Windows 下 threads worker 过多可能触发 EMFILE / 资源争用导致用例超时。 | ||
| // 允许通过环境变量覆盖：VITEST_MAX_WORKERS=... |
There was a problem hiding this comment.
Default of 8 workers might be too conservative for modern machines - consider Math.min(os.cpus().length, 16) or a higher default (12-16) for better performance on multi-core systems.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: vitest.config.ts
Line: 100
Comment:
Default of 8 workers might be too conservative for modern machines - consider `Math.min(os.cpus().length, 16)` or a higher default (12-16) for better performance on multi-core systems.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/_helpers/auth.ts`:
- Around line 59-85: The thrown error inside the try when !shouldRetry is being
caught by the outer catch and causes unintended retries; update the login loop
so non-retriable failures are rethrown immediately instead of being swallowed:
either move the throw for the !shouldRetry branch out of the try (so it escapes
the loop) or tag the thrown Error (e.g., add a nonRetriable flag) and in the
catch check that flag (using shouldRetry/attempt/maxAttempts) and rethrow
immediately; adjust handling of lastError/sleep accordingly so only truly
retriable 503/SESSION_CREATE_FAILED paths are retried.
---
Duplicate comments:
In `@tests/e2e/_helpers/auth.ts`:
- Around line 24-27: The extractCookieValue function uses new RegExp with a
concatenated cookieName which opens a ReDoS/unsafe-regex risk; replace the regex
approach in extractCookieValue with deterministic string parsing: trim the
header, split it on ';' into cookie segments, iterate segments and for each trim
and check startsWith(`${cookieName}=`) (or use indexOf) to extract and return
the substring after '=' (or null if not found). Ensure you only perform plain
string operations (no RegExp) and preserve existing return semantics (string |
null).
| <div className="absolute top-0 left-0 right-0 h-0.5 bg-muted"> | ||
| <motion.div | ||
| className="h-full bg-primary" | ||
| initial={{ width: "20%" }} | ||
| animate={{ | ||
| width: `${((TAB_CONFIG.findIndex((t) => t.id === activeTab) + 1) / TAB_CONFIG.length) * 100}%`, | ||
| }} | ||
| initial={{ width: stepProgressWidth }} | ||
| animate={{ width: stepProgressWidth }} |
There was a problem hiding this comment.
animation now disabled - initial and animate both set to stepProgressWidth, causing no visual transition when tabs change
| <div className="absolute top-0 left-0 right-0 h-0.5 bg-muted"> | |
| <motion.div | |
| className="h-full bg-primary" | |
| initial={{ width: "20%" }} | |
| animate={{ | |
| width: `${((TAB_CONFIG.findIndex((t) => t.id === activeTab) + 1) / TAB_CONFIG.length) * 100}%`, | |
| }} | |
| initial={{ width: stepProgressWidth }} | |
| animate={{ width: stepProgressWidth }} | |
| initial={{ width: 0 }} | |
| animate={{ width: stepProgressWidth }} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx
Line: 267-271
Comment:
animation now disabled - `initial` and `animate` both set to `stepProgressWidth`, causing no visual transition when tabs change
```suggestion
initial={{ width: 0 }}
animate={{ width: stepProgressWidth }}
```
How can I resolve this? If you propose a fix, please make it concise.| // Skip Redis connection during Next.js production build phase (avoid connection attempts) | ||
| if (process.env.NEXT_PHASE === "phase-production-build") { | ||
| return null; |
There was a problem hiding this comment.
removing CI === "true" check allows Redis connections during CI, but the comment only mentions Next.js build phase - verify CI environment can handle Redis connections or update logic
if CI should still skip Redis (but E2E needs it), consider:
| // Skip Redis connection during Next.js production build phase (avoid connection attempts) | |
| if (process.env.NEXT_PHASE === "phase-production-build") { | |
| return null; | |
| // Skip Redis during Next.js build, but allow in CI for E2E tests | |
| if (process.env.NEXT_PHASE === "phase-production-build") { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/redis/client.ts
Line: 77-79
Comment:
removing `CI === "true"` check allows Redis connections during CI, but the comment only mentions Next.js build phase - verify CI environment can handle Redis connections or update logic
if CI should still skip Redis (but E2E needs it), consider:
```suggestion
// Skip Redis during Next.js build, but allow in CI for E2E tests
if (process.env.NEXT_PHASE === "phase-production-build") {
```
How can I resolve this? If you propose a fix, please make it concise.
tests/e2e/_helpers/auth.ts
Outdated
| throw new Error(`[e2e] login failed: ${response.status} ${text}`); | ||
| } | ||
|
|
||
| await sleep(Math.min(1000, 100 * 2 ** (attempt - 1))); |
There was a problem hiding this comment.
slow exponential backoff - 100 * 2^(attempt-1) only reaches 512ms at attempt 4, then Math.min(1000, ...) caps remaining attempts at 1s. First 4 attempts happen within ~1.5s total
| await sleep(Math.min(1000, 100 * 2 ** (attempt - 1))); | |
| await sleep(Math.min(2000, 200 * 2 ** (attempt - 1))); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/_helpers/auth.ts
Line: 64
Comment:
slow exponential backoff - `100 * 2^(attempt-1)` only reaches 512ms at attempt 4, then `Math.min(1000, ...)` caps remaining attempts at 1s. First 4 attempts happen within ~1.5s total
```suggestion
await sleep(Math.min(2000, 200 * 2 ** (attempt - 1)));
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| break; | ||
| } | ||
|
|
||
| await sleep(Math.min(1000, 100 * 2 ** (attempt - 1))); |
There was a problem hiding this comment.
retry logic applies to all errors (network, parse failures, etc), not just transient server issues - may mask bugs in test setup
| await sleep(Math.min(1000, 100 * 2 ** (attempt - 1))); | |
| await sleep(Math.min(2000, 200 * 2 ** (attempt - 1))); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/_helpers/auth.ts
Line: 84
Comment:
retry logic applies to all errors (network, parse failures, etc), not just transient server issues - may mask bugs in test setup
```suggestion
await sleep(Math.min(2000, 200 * 2 ** (attempt - 1)));
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/redis/client.ts (1)
94-96: 建议：连接永久断开后重置单例，避免返回僵尸客户端
retryStrategy返回非数字值时，ioredis 将停止重连，"the connection will be lost forever unless the user calls redis.connect() manually"。当前实现中，
retryStrategy在第 5 次后返回null（第 108 行），但模块级redisClient单例并未被清空。此后所有getRedisClient()调用都会在第 94-96 行命中缓存并返回这个不可用的死客户端，而不会尝试重新初始化。现在 CI 环境也会建立真实连接，该问题在 CI 流水线中更容易被触发（例如 Redis 服务启动滞后时）。
♻️ 建议：监听 `end` 事件时重置单例
redisClient.on("close", () => { logger.warn("[Redis] Connection closed", { redisUrl: safeRedisUrl }); }); + + redisClient.on("end", () => { + logger.warn("[Redis] Connection permanently closed, resetting client", { redisUrl: safeRedisUrl }); + redisClient = null; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/redis/client.ts` around lines 94 - 96, The module-level singleton redisClient can become a "dead" client when retryStrategy returns null (stops reconnection), so update the Redis client initialization (the code that creates redisClient and defines retryStrategy) to attach an 'end' (and optionally 'close') event handler that sets the module-level redisClient = null so subsequent getRedisClient() calls will reinitialize a new client; also consider checking client status in getRedisClient() (e.g., not just truthy but connected/ready) before returning the cached instance to avoid returning a zombie client.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/redis/client.ts`:
- Around line 94-96: The module-level singleton redisClient can become a "dead"
client when retryStrategy returns null (stops reconnection), so update the Redis
client initialization (the code that creates redisClient and defines
retryStrategy) to attach an 'end' (and optionally 'close') event handler that
sets the module-level redisClient = null so subsequent getRedisClient() calls
will reinitialize a new client; also consider checking client status in
getRedisClient() (e.g., not just truthy but connected/ready) before returning
the cached instance to avoid returning a zombie client.
|
已按 CodeRabbit 建议补齐并修正：
另外修复了 PR 中 API E2E Tests 的失败：
当前 Actions：build/lint/typecheck/unit/integration/api tests 均已通过。 |
src/app/globals.css
Outdated
| @supports (height: 100dvh) { | ||
| .max-h-\[50vh\] { | ||
| max-height: 50dvh; | ||
| } | ||
| .max-h-\[70vh\] { | ||
| max-height: 70dvh; | ||
| } | ||
| .max-h-\[80vh\] { | ||
| max-height: 80dvh; | ||
| } | ||
| .max-h-\[85vh\] { | ||
| max-height: 85dvh; | ||
| } | ||
| .max-h-\[90vh\] { | ||
| max-height: 90dvh; | ||
| } | ||
| .max-h-\[95vh\] { | ||
| max-height: 95dvh; | ||
| } |
There was a problem hiding this comment.
these CSS overrides using @supports (height: 100dvh) will only apply to browsers that support dvh, but the classes like max-h-[85vh] will still use vh in browsers that don't support dvh
the --cch-viewport-height CSS variable approach used elsewhere in the PR (lines 48, 82-86) is more consistent - it provides a single source of truth that falls back from dvh to vh
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/globals.css
Line: 151-169
Comment:
these CSS overrides using `@supports (height: 100dvh)` will only apply to browsers that support `dvh`, but the classes like `max-h-[85vh]` will still use `vh` in browsers that don't support `dvh`
the `--cch-viewport-height` CSS variable approach used elsewhere in the PR (lines 48, 82-86) is more consistent - it provides a single source of truth that falls back from `dvh` to `vh`
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| @@ -553,18 +553,20 @@ function ProviderFormContent({ | |||
| <form onSubmit={handleSubmit} className="flex flex-col h-full max-h-[85vh]"> | |||
There was a problem hiding this comment.
form still uses max-h-[85vh] which will be overridden to 85dvh by the CSS in globals.css - but this creates inconsistency with other viewport calculations that use the --cch-viewport-height variable
| <form onSubmit={handleSubmit} className="flex flex-col h-full max-h-[85vh]"> | |
| <form onSubmit={handleSubmit} className="flex flex-col h-full max-h-[calc(var(--cch-viewport-height,100vh)*0.85)]"> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/index.tsx
Line: 553
Comment:
form still uses `max-h-[85vh]` which will be overridden to `85dvh` by the CSS in `globals.css` - but this creates inconsistency with other viewport calculations that use the `--cch-viewport-height` variable
```suggestion
<form onSubmit={handleSubmit} className="flex flex-col h-full max-h-[calc(var(--cch-viewport-height,100vh)*0.85)]">
```
How can I resolve this? If you propose a fix, please make it concise.| <div className="order-2 md:order-1 shrink-0"> | ||
| {/* Tab Navigation */} | ||
| <FormTabNav | ||
| activeTab={state.ui.activeTab} | ||
| onTabChange={handleTabChange} | ||
| disabled={isPending} | ||
| tabStatus={getTabStatus()} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
wrapping FormTabNav in a div with shrink-0 creates an extra DOM layer - the wrapper div prevents FormTabNav from being a direct flex child, which may cause layout issues if FormTabNav has its own flex/sizing logic
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/index.tsx
Line: 556-564
Comment:
wrapping `FormTabNav` in a `div` with `shrink-0` creates an extra DOM layer - the wrapper div prevents `FormTabNav` from being a direct flex child, which may cause layout issues if `FormTabNav` has its own flex/sizing logic
How can I resolve this? If you propose a fix, please make it concise.
tests/e2e/_helpers/auth.ts
Outdated
| throw new Error(`[e2e] login failed: ${response.status} ${text}`); | ||
| } | ||
|
|
||
| await sleep(Math.min(1000, 100 * 2 ** (attempt - 1))); |
There was a problem hiding this comment.
exponential backoff base (100ms) is too small - with cap at 1000ms, attempts reach max delay quickly: 100ms, 200ms, 400ms, 800ms, then 1000ms for attempts 5-10
| await sleep(Math.min(1000, 100 * 2 ** (attempt - 1))); | |
| await sleep(Math.min(2000, 200 * 2 ** (attempt - 1))); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/_helpers/auth.ts
Line: 64
Comment:
exponential backoff base (100ms) is too small - with cap at 1000ms, attempts reach max delay quickly: 100ms, 200ms, 400ms, 800ms, then 1000ms for attempts 5-10
```suggestion
await sleep(Math.min(2000, 200 * 2 ** (attempt - 1)));
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
tests/e2e/_helpers/auth.ts
Outdated
| } catch (error) { | ||
| lastError = error; | ||
| if (attempt >= maxAttempts) { | ||
| break; | ||
| } | ||
|
|
||
| await sleep(Math.min(1000, 100 * 2 ** (attempt - 1))); | ||
| } |
There was a problem hiding this comment.
catch block retries for ALL errors (network failures, parse errors, etc), not just transient server issues - this masks legitimate failures and causes tests to hang for extended periods
the retry should only happen for specific retryable errors, or this block should be removed entirely since the non-ok response case above already handles 503 retries
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/_helpers/auth.ts
Line: 78-85
Comment:
catch block retries for ALL errors (network failures, parse errors, etc), not just transient server issues - this masks legitimate failures and causes tests to hang for extended periods
the retry should only happen for specific retryable errors, or this block should be removed entirely since the non-ok response case above already handles 503 retries
How can I resolve this? If you propose a fix, please make it concise.
✅ Actions performedReview triggered.
|
| return combined | ||
| .split(/,(?=[^;]+?=)/g) | ||
| .map((s) => s.trim()) | ||
| .filter(Boolean); |
There was a problem hiding this comment.
regex /,(?=[^;]+?=)/g can incorrectly split Set-Cookie headers containing RFC-compliant date values like Expires=Wed, 21 Oct 2026 07:28:00 GMT
the lookahead (?=[^;]+?=) prevents splitting at commas within individual cookie attributes, but dates like Expires=Wed, 21 Oct 2026 still match because the pattern sees 21 followed by space and other text, then eventually finds = in GMT or subsequent attributes
if the server sends Expires with RFC 1123 dates, this will break cookie parsing
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/_helpers/auth.ts
Line: 18-21
Comment:
regex `/,(?=[^;]+?=)/g` can incorrectly split `Set-Cookie` headers containing RFC-compliant date values like `Expires=Wed, 21 Oct 2026 07:28:00 GMT`
the lookahead `(?=[^;]+?=)` prevents splitting at commas within individual cookie attributes, but dates like `Expires=Wed, 21 Oct 2026` still match because the pattern sees `21` followed by space and other text, then eventually finds `=` in `GMT` or subsequent attributes
if the server sends `Expires` with RFC 1123 dates, this will break cookie parsing
How can I resolve this? If you propose a fix, please make it concise.| }); | ||
| } catch (error) { | ||
| lastError = error; | ||
| if (attempt >= maxAttempts) { | ||
| break; | ||
| } | ||
|
|
||
| await sleep(Math.min(1000, 100 * 2 ** (attempt - 1))); | ||
| continue; |
There was a problem hiding this comment.
retry on ALL fetch errors (network, DNS, timeouts) masks legitimate test setup failures - if API_BASE_URL is misconfigured or network is down, tests hang for ~6 seconds (10 attempts × ~600ms avg) instead of failing fast
only retry transient errors like ECONNRESET / ETIMEDOUT, or remove this catch block entirely since line 82-93 already handles HTTP 503 retries
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/_helpers/auth.ts
Line: 59-67
Comment:
retry on ALL fetch errors (network, DNS, timeouts) masks legitimate test setup failures - if `API_BASE_URL` is misconfigured or network is down, tests hang for ~6 seconds (10 attempts × ~600ms avg) instead of failing fast
only retry transient errors like `ECONNRESET` / `ETIMEDOUT`, or remove this catch block entirely since line 82-93 already handles HTTP 503 retries
How can I resolve this? If you propose a fix, please make it concise.| if (attempt >= maxAttempts) { | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
exponential backoff base is too small - with 100 * 2^(attempt-1) and cap at 1000ms, delays are: 100ms, 200ms, 400ms, 800ms, then 1000ms for attempts 5-10
attempts reach max delay very quickly, meaning attempts 5-10 all wait the same 1 second - consider using a larger base like 200ms or increasing the cap
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/_helpers/auth.ts
Line: 65
Comment:
exponential backoff base is too small - with `100 * 2^(attempt-1)` and cap at 1000ms, delays are: 100ms, 200ms, 400ms, 800ms, then 1000ms for attempts 5-10
attempts reach max delay very quickly, meaning attempts 5-10 all wait the same 1 second - consider using a larger base like 200ms or increasing the cap
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| initial={{ width: stepProgressWidth }} | ||
| animate={{ width: stepProgressWidth }} |
There was a problem hiding this comment.
both initial and animate set to same value stepProgressWidth disables the animation - no visual transition when tabs change
remove initial prop to animate from current position
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx
Line: 277-278
Comment:
both `initial` and `animate` set to same value `stepProgressWidth` disables the animation - no visual transition when tabs change
remove `initial` prop to animate from current position
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx (1)
76-82:useEffect双向同步——已正确修复当前代码
setClientRestrictionsEnabled(hasAnyClientRestrictions)同时处理了false→true和true→false两个方向，与之前审查建议一致。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx around lines 76 - 82, The two-way sync using hasAnyClientRestrictions with clientRestrictionsEnabled is correct—keep the initial state const [clientRestrictionsEnabled, setClientRestrictionsEnabled] = useState(hasAnyClientRestrictions) and the useEffect that calls setClientRestrictionsEnabled(hasAnyClientRestrictions) with [hasAnyClientRestrictions] as the dependency; do not add additional syncing logic or duplicate effect handlers, and remove any accidental duplicate comments or redundant code related to this synchronization.
🧹 Nitpick comments (3)
src/components/form/client-restrictions-editor.tsx (2)
64-74: TagInput 缺少onInvalidTag回调——标签被静默拒绝时用户无任何反馈第 65-73 行的
TagInput设置了maxTagLength={64}和maxTags={50}，但未传入onInvalidTag。当 TagInput 内部因超长、超数量或重复等原因拒绝标签时，用户不会收到任何错误提示——与routing-section.tsx中通过onInvalidTag+toast.error提供反馈的做法不一致。♻️ 建议补充
onInvalidTag支持在
ClientRestrictionListEditorProps中增加可选回调：interface ClientRestrictionListEditorProps { label: string; values: string[]; placeholder?: string; disabled?: boolean; getPresetLabel: (presetValue: string) => string; onTogglePreset: (presetValue: string, checked: boolean) => void; onCustomChange: (customValues: string[]) => void; + onInvalidTag?: (tag: string, reason: string) => void; className?: string; }-function ClientRestrictionListEditor({ +function ClientRestrictionListEditor({ label, values, placeholder, disabled, getPresetLabel, onTogglePreset, onCustomChange, + onInvalidTag, className, }: ClientRestrictionListEditorProps) {<TagInput value={customValues} onChange={onCustomChange} placeholder={placeholder} maxTagLength={64} maxTags={50} disabled={disabled} validateTag={() => true} + onInvalidTag={onInvalidTag} />
ClientRestrictionsEditorProps中同样可以暴露该字段，或由消费方（routing-section.tsx）传入onInvalidTag并向下透传，以保持与其他TagInput用法的一致性。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/form/client-restrictions-editor.tsx` around lines 64 - 74, The TagInput here is missing an onInvalidTag handler so rejected tags (too long, too many, duplicates) give no feedback; update the props interfaces ClientRestrictionListEditorProps and ClientRestrictionsEditorProps to include an optional onInvalidTag?: (reason: string) => void, expose/accept that prop in the component that renders TagInput, and pass it down to TagInput alongside value={customValues} and onChange={onCustomChange}; consumers (e.g., routing-section.tsx) can then provide an onInvalidTag that calls toast.error to show the user feedback.
126-144:handleAllowedCustomChange/handleBlockedCustomChange中存在冗余的规范化计算
mergePresetAndCustomClients(blocked, splitPresetAndCustomClients(blocked).customValues)等价于blocked本身（在数据已规范化的前提下），实际上只是去重。若blocked/allowed在写入时始终经过mergePresetAndCustomClients或togglePresetSelection维护，可直接用new Set(...)替代：♻️ 建议简化
const handleAllowedCustomChange = (customValues: string[]) => { - const blockedMerged = mergePresetAndCustomClients( - blocked, - splitPresetAndCustomClients(blocked).customValues - ); - const blockedValueSet = new Set(blockedMerged); + const blockedValueSet = new Set(blocked); const filteredCustomValues = customValues.filter((value) => !blockedValueSet.has(value)); onAllowedChange(mergePresetAndCustomClients(allowed, filteredCustomValues)); }; const handleBlockedCustomChange = (customValues: string[]) => { - const allowedMerged = mergePresetAndCustomClients( - allowed, - splitPresetAndCustomClients(allowed).customValues - ); - const allowedValueSet = new Set(allowedMerged); + const allowedValueSet = new Set(allowed); const filteredCustomValues = customValues.filter((value) => !allowedValueSet.has(value)); onBlockedChange(mergePresetAndCustomClients(blocked, filteredCustomValues)); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/form/client-restrictions-editor.tsx` around lines 126 - 144, The handlers perform redundant normalization by calling mergePresetAndCustomClients(..., splitPresetAndCustomClients(...).customValues) even though allowed/blocked are already normalized; replace those expressions with a simple Set created from the current allowed or blocked arrays (e.g., const blockedValueSet = new Set(blocked) / const allowedValueSet = new Set(allowed)), use that set to filter the incoming customValues, and then call onAllowedChange(mergePresetAndCustomClients(allowed, filteredCustomValues)) or onBlockedChange(mergePresetAndCustomClients(blocked, filteredCustomValues)) respectively to produce the final normalized value; update handleAllowedCustomChange and handleBlockedCustomChange accordingly and remove the redundant split/merge calls.tests/unit/lib/endpoint-circuit-breaker.test.ts (1)
490-493: 建议在waitForMockCalled前显式调用vi.useRealTimers()，与其他用例保持一致该测试未启用 Fake Timers，因此第 492 行的
waitForMockCalled当前可正常运行。但与第 125–126 行、第 453–454 行的模式不一致——那两处均在调用前先显式恢复真实计时器。若将来此测试需要用vi.useFakeTimers()冻结Date.now()，遗漏该调用将直接导致无限挂起（见上方第 28–33 行的说明）。♻️ 建议修改
expect(getEndpointCircuitStateSync(200)).toBe("open"); + + // 与其他用例保持一致：确保真实计时器生效后再轮询告警。 + vi.useRealTimers(); await waitForMockCalled(sendAlertMock); expect(sendAlertMock).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/lib/endpoint-circuit-breaker.test.ts` around lines 490 - 493, Insert a call to vi.useRealTimers() immediately before the await waitForMockCalled(sendAlertMock) call in the test so timers are restored (matching the pattern used elsewhere); specifically, update the test surrounding waitForMockCalled and sendAlertMock to call vi.useRealTimers() first to avoid hanging if fake timers are enabled in other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/lib/endpoint-circuit-breaker.test.ts`:
- Around line 28-33: The helper waitForMockCalled currently silent-exits on
timeout and can hang under fake timers; update waitForMockCalled to (1) throw a
clear Error like "timeout waiting for mock" when the timeout elapses instead of
returning void, and (2) guard against fake timers by temporarily switching to
real timers (save current timer mode, call vi.useRealTimers(), then restore the
original timer mode after await loop) so Date.now and setTimeout advance;
reference the waitForMockCalled function and use
vi.useRealTimers()/vi.useFakeTimers() (or equivalent restore) around the polling
loop to ensure it never deadlocks under fake timers.
---
Duplicate comments:
In
`@src/app/`[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx:
- Around line 76-82: The two-way sync using hasAnyClientRestrictions with
clientRestrictionsEnabled is correct—keep the initial state const
[clientRestrictionsEnabled, setClientRestrictionsEnabled] =
useState(hasAnyClientRestrictions) and the useEffect that calls
setClientRestrictionsEnabled(hasAnyClientRestrictions) with
[hasAnyClientRestrictions] as the dependency; do not add additional syncing
logic or duplicate effect handlers, and remove any accidental duplicate comments
or redundant code related to this synchronization.
---
Nitpick comments:
In `@src/components/form/client-restrictions-editor.tsx`:
- Around line 64-74: The TagInput here is missing an onInvalidTag handler so
rejected tags (too long, too many, duplicates) give no feedback; update the
props interfaces ClientRestrictionListEditorProps and
ClientRestrictionsEditorProps to include an optional onInvalidTag?: (reason:
string) => void, expose/accept that prop in the component that renders TagInput,
and pass it down to TagInput alongside value={customValues} and
onChange={onCustomChange}; consumers (e.g., routing-section.tsx) can then
provide an onInvalidTag that calls toast.error to show the user feedback.
- Around line 126-144: The handlers perform redundant normalization by calling
mergePresetAndCustomClients(..., splitPresetAndCustomClients(...).customValues)
even though allowed/blocked are already normalized; replace those expressions
with a simple Set created from the current allowed or blocked arrays (e.g.,
const blockedValueSet = new Set(blocked) / const allowedValueSet = new
Set(allowed)), use that set to filter the incoming customValues, and then call
onAllowedChange(mergePresetAndCustomClients(allowed, filteredCustomValues)) or
onBlockedChange(mergePresetAndCustomClients(blocked, filteredCustomValues))
respectively to produce the final normalized value; update
handleAllowedCustomChange and handleBlockedCustomChange accordingly and remove
the redundant split/merge calls.
In `@tests/unit/lib/endpoint-circuit-breaker.test.ts`:
- Around line 490-493: Insert a call to vi.useRealTimers() immediately before
the await waitForMockCalled(sendAlertMock) call in the test so timers are
restored (matching the pattern used elsewhere); specifically, update the test
surrounding waitForMockCalled and sendAlertMock to call vi.useRealTimers() first
to avoid hanging if fake timers are enabled in other tests.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (4)
src/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsxsrc/app/global-error.tsxsrc/components/form/client-restrictions-editor.tsxtests/unit/lib/endpoint-circuit-breaker.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/global-error.tsx
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| const handleAllowedCustomChange = (customValues: string[]) => { | ||
| const blockedMerged = mergePresetAndCustomClients( | ||
| blocked, | ||
| splitPresetAndCustomClients(blocked).customValues | ||
| ); | ||
| const blockedValueSet = new Set(blockedMerged); | ||
| const filteredCustomValues = customValues.filter((value) => !blockedValueSet.has(value)); | ||
| onAllowedChange(mergePresetAndCustomClients(allowed, filteredCustomValues)); | ||
| }; |
There was a problem hiding this comment.
Inefficient re-splitting of already-known preset values
On line 132-135, splitPresetAndCustomClients(blocked) is called to get custom values, but this is redundant — the caller (routing-section.tsx) already maintains the full blocked array. The function then merges presets back in at line 138 using mergePresetAndCustomClients(allowed, filteredCustomValues), which re-splits internally.
This creates unnecessary work: split → filter → merge (which splits again). Consider passing the current preset selections separately or restructuring to avoid multiple split/merge cycles.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/form/client-restrictions-editor.tsx
Line: 131-139
Comment:
**Inefficient re-splitting of already-known preset values**
On line 132-135, `splitPresetAndCustomClients(blocked)` is called to get custom values, but this is redundant — the caller (`routing-section.tsx`) already maintains the full `blocked` array. The function then merges presets back in at line 138 using `mergePresetAndCustomClients(allowed, filteredCustomValues)`, which re-splits internally.
This creates unnecessary work: split → filter → merge (which splits again). Consider passing the current preset selections separately or restructuring to avoid multiple split/merge cycles.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| const [clientRestrictionsEnabled, setClientRestrictionsEnabled] = | ||
| useState(hasAnyClientRestrictions); | ||
|
|
||
| useEffect(() => { | ||
| setClientRestrictionsEnabled(hasAnyClientRestrictions); | ||
| }, [hasAnyClientRestrictions]); |
There was a problem hiding this comment.
useState initializer runs on every render
useState(hasAnyClientRestrictions) on line 77-78 computes the initial value on every render (since hasAnyClientRestrictions is recalculated each time), even though useState only uses it on mount. Use a function initializer to avoid unnecessary computation:
| const [clientRestrictionsEnabled, setClientRestrictionsEnabled] = | |
| useState(hasAnyClientRestrictions); | |
| useEffect(() => { | |
| setClientRestrictionsEnabled(hasAnyClientRestrictions); | |
| }, [hasAnyClientRestrictions]); | |
| const [clientRestrictionsEnabled, setClientRestrictionsEnabled] = useState(() => | |
| allowedClients.length > 0 || blockedClients.length > 0 | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx
Line: 77-82
Comment:
**useState initializer runs on every render**
`useState(hasAnyClientRestrictions)` on line 77-78 computes the initial value on every render (since `hasAnyClientRestrictions` is recalculated each time), even though `useState` only uses it on mount. Use a function initializer to avoid unnecessary computation:
```suggestion
const [clientRestrictionsEnabled, setClientRestrictionsEnabled] = useState(() =>
allowedClients.length > 0 || blockedClients.length > 0
);
```
How can I resolve this? If you propose a fix, please make it concise.| useEffect(() => { | ||
| setClientRestrictionsEnabled(hasAnyClientRestrictions); | ||
| }, [hasAnyClientRestrictions]); |
There was a problem hiding this comment.
useEffect creates infinite update risk if parent re-adds restrictions
When clientRestrictionsEnabled is true and user edits the lists externally (e.g., via form reset or batch operations that directly modify allowedClients/blockedClients to non-empty arrays), this effect will re-run and call setClientRestrictionsEnabled(true) again. While React will skip the state update if the value is already true, this creates a dependency on state that the component itself controls, which can cause subtle bugs.
The real concern: if external code clears both lists (allowedClients = [], blockedClients = []) while the toggle is still enabled, the effect will disable the toggle, but the user's intent (to keep it enabled and add new restrictions) is lost.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx
Line: 80-82
Comment:
**useEffect creates infinite update risk if parent re-adds restrictions**
When `clientRestrictionsEnabled` is `true` and user edits the lists externally (e.g., via form reset or batch operations that directly modify `allowedClients`/`blockedClients` to non-empty arrays), this effect will re-run and call `setClientRestrictionsEnabled(true)` again. While React will skip the state update if the value is already `true`, this creates a dependency on state that the component itself controls, which can cause subtle bugs.
The real concern: if external code clears both lists (`allowedClients = []`, `blockedClients = []`) while the toggle is still enabled, the effect will disable the toggle, but the user's intent (to keep it enabled and add new restrictions) is lost.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| if (redisClient.status === "end") { | ||
| redisClient = null; | ||
| } else { | ||
| return redisClient; | ||
| } | ||
| } |
There was a problem hiding this comment.
Race condition: client.status check may not be reliable
If redisClient.status === "end" on line 99, you reset redisClient = null and fall through to create a new client. However, between the status check and the new client creation, the old client might not be fully cleaned up. ioredis maintains internal state that may not be immediately garbage-collected.
Consider explicitly calling disconnect() on the ended client before nulling it:
| if (redisClient.status === "end") { | |
| redisClient = null; | |
| } else { | |
| return redisClient; | |
| } | |
| } | |
| if (redisClient) { | |
| if (redisClient.status === "end") { | |
| redisClient.disconnect(false); | |
| redisClient = null; | |
| } else { | |
| return redisClient; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/redis/client.ts
Line: 99-104
Comment:
**Race condition: client.status check may not be reliable**
If `redisClient.status === "end"` on line 99, you reset `redisClient = null` and fall through to create a new client. However, between the status check and the new client creation, the old client might not be fully cleaned up. ioredis maintains internal state that may not be immediately garbage-collected.
Consider explicitly calling `disconnect()` on the ended client before nulling it:
```suggestion
if (redisClient) {
if (redisClient.status === "end") {
redisClient.disconnect(false);
redisClient = null;
} else {
return redisClient;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| client.on("end", () => { | ||
| if (redisClient !== client) return; | ||
| logger.warn("[Redis] Connection ended, resetting client", { redisUrl: safeRedisUrl }); | ||
| redisClient = null; | ||
| }); |
There was a problem hiding this comment.
double-null assignment between end handler and closeRedis — when quit() fires, this handler sets redisClient = null at line 145, then closeRedis's finally block also tries to set it at line 170 (but the guard will be false)
safe but confusing — add a comment explaining the interaction
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/redis/client.ts
Line: 142-146
Comment:
double-null assignment between `end` handler and `closeRedis` — when `quit()` fires, this handler sets `redisClient = null` at line 145, then `closeRedis`'s finally block also tries to set it at line 170 (but the guard will be false)
safe but confusing — add a comment explaining the interaction
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx (1)
80-82: 双向同步逻辑已正确实现
setClientRestrictionsEnabled(hasAnyClientRestrictions)已涵盖true→false和false→true两个方向，先前提出的问题已修复。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx around lines 80 - 82, The effect updating client restriction state is already correctly implemented; keep the useEffect that calls setClientRestrictionsEnabled(hasAnyClientRestrictions) with hasAnyClientRestrictions in the dependency array (in routing-section.tsx), since this covers both true→false and false→true transitions—no code changes required for useEffect, use setClientRestrictionsEnabled and hasAnyClientRestrictions as-is.
🧹 Nitpick comments (1)
src/components/form/client-restrictions-editor.tsx (1)
131-149:handleAllowedCustomChange和handleBlockedCustomChange中的冗余运算
mergePresetAndCustomClients(X, splitPresetAndCustomClients(X).customValues)将X拆分后再原样合并，结果等价于X本身，因此new Set(blockedMerged)≡new Set(blocked)（handleBlockedCustomChange同理）。建议简化：优化建议
const handleAllowedCustomChange = (customValues: string[]) => { - const blockedMerged = mergePresetAndCustomClients( - blocked, - splitPresetAndCustomClients(blocked).customValues, - ); - const blockedValueSet = new Set(blockedMerged); + const blockedValueSet = new Set(blocked); const filteredCustomValues = customValues.filter((value) => !blockedValueSet.has(value)); onAllowedChange(mergePresetAndCustomClients(allowed, filteredCustomValues)); }; const handleBlockedCustomChange = (customValues: string[]) => { - const allowedMerged = mergePresetAndCustomClients( - allowed, - splitPresetAndCustomClients(allowed).customValues, - ); - const allowedValueSet = new Set(allowedMerged); + const allowedValueSet = new Set(allowed); const filteredCustomValues = customValues.filter((value) => !allowedValueSet.has(value)); onBlockedChange(mergePresetAndCustomClients(blocked, filteredCustomValues)); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/form/client-restrictions-editor.tsx` around lines 131 - 149, The two handlers perform redundant split/merge operations that return the original list; simplify handleAllowedCustomChange and handleBlockedCustomChange by using the original arrays when building the exclusion Set (use new Set(blocked) in handleAllowedCustomChange and new Set(allowed) in handleBlockedCustomChange) and keep the rest of the logic (filtering customValues and calling onAllowedChange/onBlockedChange with mergePresetAndCustomClients(..., filteredCustomValues)) unchanged; update only the Set construction to remove the unnecessary calls to mergePresetAndCustomClients(... splitPresetAndCustomClients(...).customValues).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/app/`[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx:
- Around line 80-82: The effect updating client restriction state is already
correctly implemented; keep the useEffect that calls
setClientRestrictionsEnabled(hasAnyClientRestrictions) with
hasAnyClientRestrictions in the dependency array (in routing-section.tsx), since
this covers both true→false and false→true transitions—no code changes required
for useEffect, use setClientRestrictionsEnabled and hasAnyClientRestrictions
as-is.
---
Nitpick comments:
In `@src/components/form/client-restrictions-editor.tsx`:
- Around line 131-149: The two handlers perform redundant split/merge operations
that return the original list; simplify handleAllowedCustomChange and
handleBlockedCustomChange by using the original arrays when building the
exclusion Set (use new Set(blocked) in handleAllowedCustomChange and new
Set(allowed) in handleBlockedCustomChange) and keep the rest of the logic
(filtering customValues and calling onAllowedChange/onBlockedChange with
mergePresetAndCustomClients(..., filteredCustomValues)) unchanged; update only
the Set construction to remove the unnecessary calls to
mergePresetAndCustomClients(... splitPresetAndCustomClients(...).customValues).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (3)
src/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsxsrc/components/form/client-restrictions-editor.tsxtests/unit/lib/endpoint-circuit-breaker.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/lib/endpoint-circuit-breaker.test.ts
|
本次补充优化（commit 5a1290d）：
验证：
|
| const [clientRestrictionsEnabled, setClientRestrictionsEnabled] = | ||
| useState(hasAnyClientRestrictions); |
There was a problem hiding this comment.
hasAnyClientRestrictions computed on every render but only used during mount
| const [clientRestrictionsEnabled, setClientRestrictionsEnabled] = | |
| useState(hasAnyClientRestrictions); | |
| const [clientRestrictionsEnabled, setClientRestrictionsEnabled] = useState(() => | |
| allowedClients.length > 0 || blockedClients.length > 0 | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx
Line: 77-78
Comment:
`hasAnyClientRestrictions` computed on every render but only used during mount
```suggestion
const [clientRestrictionsEnabled, setClientRestrictionsEnabled] = useState(() =>
allowedClients.length > 0 || blockedClients.length > 0
);
```
How can I resolve this? If you propose a fix, please make it concise.
|
@coderabbitai review 请基于最新提交复审。如果没有阻塞性问题，麻烦给出 APPROVED 以清除历史 CHANGES_REQUESTED。 |
|
您在本次提交中完成的优化非常棒： ✅ 客户端限制编辑器：白名单/黑名单双列布局，支持预设建议与自定义输入，并提供实时验证反馈 您已完成所有必要的验证（typecheck/lint/test/build），并且持续按照审查建议迭代优化。整个 PR 的移动端适配方案（CSS 变量、safe-area、dvh fallback）和稳健性改进（Redis 生命周期、E2E 认证）都已就绪。 没有发现阻塞性问题，我现在将批准此 PR 以清除之前的 CHANGES_REQUESTED 状态。 🧠 Learnings used✅ Actions performedComments resolved and changes approved. Review triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/app/[locale]/settings/providers/_components/model-multi-select.tsx (2)
61-79: 建议用useMemo替代 IIFE 计算displayedModels。当前写法在每次渲染时都执行
Set构建与两次数组遍历。改用useMemo可在availableModels与selectedModels未变化时跳过重新计算，在模型列表较大时尤为明显。♻️ 改用 useMemo
- const displayedModels = (() => { - const seen = new Set<string>(); - const merged: string[] = []; - - for (const model of availableModels) { - if (seen.has(model)) continue; - seen.add(model); - merged.push(model); - } - - // 关键：把已选中但不在远端列表的自定义模型也渲染出来，保证可取消选中 - for (const model of selectedModels) { - if (seen.has(model)) continue; - seen.add(model); - merged.push(model); - } - - return merged; - })(); + const displayedModels = useMemo(() => { + const seen = new Set<string>(); + const merged: string[] = []; + + for (const model of availableModels) { + if (seen.has(model)) continue; + seen.add(model); + merged.push(model); + } + + // 关键：把已选中但不在远端列表的自定义模型也渲染出来，保证可取消选中 + for (const model of selectedModels) { + if (seen.has(model)) continue; + seen.add(model); + merged.push(model); + } + + return merged; + }, [availableModels, selectedModels]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/model-multi-select.tsx around lines 61 - 79, Replace the IIFE that computes displayedModels with a useMemo hook so the Set construction and array traversals run only when availableModels or selectedModels change; specifically, move the logic inside a useMemo(() => { ... }, [availableModels, selectedModels]) and return the merged array, keeping the same de-duplication and adding selectedModels not present in availableModels so existing behavior of displayedModels, availableModels, and selectedModels is preserved.
166-189: 建议将SourceIndicator提取到组件外部。目前
SourceIndicator在ModelMultiSelect函数体内定义，每次父组件渲染都会产生新的函数引用。React 按引用比较组件类型，因此每次渲染都会 unmount 旧的SourceIndicator并 mount 新的，造成不必要的 DOM 重建，并可能导致 Tooltip 动画抖动。建议将其提取为独立的具名组件（通过 props 传入
loading、modelSource），或改写为返回 JSX 的普通辅助函数（通过变量而非组件标签调用）。♻️ 建议改写为普通辅助函数（最小改动）
- // 数据来源指示器 - const SourceIndicator = () => { - if (loading) return null; - - const isUpstream = modelSource === "upstream"; - const Icon = isUpstream ? Cloud : Database; - const label = isUpstream ? t("sourceUpstream") : t("sourceFallback"); - const description = isUpstream ? t("sourceUpstreamDesc") : t("sourceFallbackDesc"); - - return ( - <TooltipProvider> - ... - </TooltipProvider> - ); - };在组件外部定义（接收显式 props）：
+interface SourceIndicatorProps { + loading: boolean; + modelSource: ModelSource; + sourceUpstream: string; + sourceFallback: string; + sourceUpstreamDesc: string; + sourceFallbackDesc: string; +} + +function SourceIndicator({ + loading, + modelSource, + sourceUpstream, + sourceFallback, + sourceUpstreamDesc, + sourceFallbackDesc, +}: SourceIndicatorProps) { + if (loading) return null; + const isUpstream = modelSource === "upstream"; + const Icon = isUpstream ? Cloud : Database; + const label = isUpstream ? sourceUpstream : sourceFallback; + const description = isUpstream ? sourceUpstreamDesc : sourceFallbackDesc; + return ( + <TooltipProvider> + ... + </TooltipProvider> + ); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/model-multi-select.tsx around lines 166 - 189, The SourceIndicator function is defined inside ModelMultiSelect causing remounts; extract it to a top-level named component or convert it to a pure helper function that takes explicit props (loading, modelSource, t) and returns JSX; reference the existing symbols (SourceIndicator, ModelMultiSelect, loading, modelSource, t, Cloud, Database, TooltipProvider/Tooltip/TooltipTrigger/TooltipContent) and update usages in ModelMultiSelect to call the helper (e.g. SourceIndicator({loading, modelSource, t}) or render <SourceIndicator loading={loading} modelSource={modelSource} t={t} />) so the component/function lives outside the parent render to avoid new refs on each render.src/components/form/client-restrictions-editor.tsx (1)
90-94: 建议对suggestions使用useMemo避免每次渲染重新构建
getPresetLabel由父组件（routing-section.tsx第 280-282 行）以内联箭头函数形式传入，每次父组件渲染都会产生新的函数引用，导致suggestions数组在每次渲染时都被重新映射。虽然数组仅有 4 个元素，但包裹useMemo是更好的实践。♻️ 建议改为 useMemo
+import { useMemo } from "react"; export function ClientRestrictionsEditor({ ..., getPresetLabel, ... }) { - const suggestions: TagInputSuggestion[] = CLIENT_RESTRICTION_PRESET_OPTIONS.map((option) => ({ - value: option.value, - label: getPresetLabel(option.value), - keywords: [...option.aliases], - })); + const suggestions: TagInputSuggestion[] = useMemo( + () => + CLIENT_RESTRICTION_PRESET_OPTIONS.map((option) => ({ + value: option.value, + label: getPresetLabel(option.value), + keywords: [...option.aliases], + })), + [getPresetLabel], + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/form/client-restrictions-editor.tsx` around lines 90 - 94, The suggestions array is rebuilt on every render because getPresetLabel is passed as an inline function from the parent, so wrap the suggestions computation in React.useMemo to memoize the mapped array; locate the const suggestions in client-restrictions-editor.tsx and change it to compute CLIENT_RESTRICTION_PRESET_OPTIONS.map(...) inside useMemo with getPresetLabel and CLIENT_RESTRICTION_PRESET_OPTIONS as dependencies to avoid unnecessary remapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/`[locale]/settings/providers/_components/model-multi-select.tsx:
- Around line 61-79: Replace the IIFE that computes displayedModels with a
useMemo hook so the Set construction and array traversals run only when
availableModels or selectedModels change; specifically, move the logic inside a
useMemo(() => { ... }, [availableModels, selectedModels]) and return the merged
array, keeping the same de-duplication and adding selectedModels not present in
availableModels so existing behavior of displayedModels, availableModels, and
selectedModels is preserved.
- Around line 166-189: The SourceIndicator function is defined inside
ModelMultiSelect causing remounts; extract it to a top-level named component or
convert it to a pure helper function that takes explicit props (loading,
modelSource, t) and returns JSX; reference the existing symbols
(SourceIndicator, ModelMultiSelect, loading, modelSource, t, Cloud, Database,
TooltipProvider/Tooltip/TooltipTrigger/TooltipContent) and update usages in
ModelMultiSelect to call the helper (e.g. SourceIndicator({loading, modelSource,
t}) or render <SourceIndicator loading={loading} modelSource={modelSource} t={t}
/>) so the component/function lives outside the parent render to avoid new refs
on each render.
In `@src/components/form/client-restrictions-editor.tsx`:
- Around line 90-94: The suggestions array is rebuilt on every render because
getPresetLabel is passed as an inline function from the parent, so wrap the
suggestions computation in React.useMemo to memoize the mapped array; locate the
const suggestions in client-restrictions-editor.tsx and change it to compute
CLIENT_RESTRICTION_PRESET_OPTIONS.map(...) inside useMemo with getPresetLabel
and CLIENT_RESTRICTION_PRESET_OPTIONS as dependencies to avoid unnecessary
remapping.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (3)
src/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsxsrc/app/[locale]/settings/providers/_components/model-multi-select.tsxsrc/components/form/client-restrictions-editor.tsx
| maxTagLength={64} | ||
| maxTags={50} | ||
| disabled={disabled} | ||
| validateTag={() => true} |
There was a problem hiding this comment.
validateTag={() => true} makes onInvalidTag partially dead code
Setting validateTag={() => true} bypasses the DEFAULT_TAG_PATTERN check in TagInput (see tag-input.tsx:228), so the invalid_format branch in the onInvalidTag handler passed from routing-section.tsx:288 will never fire. The base validation for empty, duplicate, too_long, and max_tags still works because it goes through validateBaseTag first, but format validation is silently skipped.
If the intent is to allow any format (since client restriction values can be arbitrary User-Agent substrings), this is acceptable — but consider removing invalid_format from the onInvalidTag handler in routing-section.tsx to avoid confusion, or add a comment here explaining why format validation is intentionally skipped.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/form/client-restrictions-editor.tsx
Line: 53
Comment:
**`validateTag={() => true}` makes `onInvalidTag` partially dead code**
Setting `validateTag={() => true}` bypasses the `DEFAULT_TAG_PATTERN` check in `TagInput` (see `tag-input.tsx:228`), so the `invalid_format` branch in the `onInvalidTag` handler passed from `routing-section.tsx:288` will never fire. The base validation for `empty`, `duplicate`, `too_long`, and `max_tags` still works because it goes through `validateBaseTag` first, but format validation is silently skipped.
If the intent is to allow any format (since client restriction values can be arbitrary User-Agent substrings), this is acceptable — but consider removing `invalid_format` from the `onInvalidTag` handler in `routing-section.tsx` to avoid confusion, or add a comment here explaining why format validation is intentionally skipped.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| targetIds: [] as number[], | ||
| }; | ||
|
|
||
| beforeAll(async () => { |
There was a problem hiding this comment.
beforeAll at module scope, outside run() block
Same pattern as api-complete.test.ts (which was already flagged in a prior thread): the beforeAll hook here runs at file scope, not inside the run(...) describe block. When ADMIN_KEY is absent, run is describe.skip, but beforeAll still executes (though it early-returns). Also in users-keys-complete.test.ts:189-192.
For consistency and proper scoping, both beforeAll and afterAll should be moved inside the run(...) block:
| targetIds: [] as number[], | |
| }; | |
| beforeAll(async () => { | |
| run("通知设置 - Webhook 目标与绑定（E2E）", () => { | |
| beforeAll(async () => { | |
| if (!ADMIN_KEY) return; | |
| authToken = await loginAndGetAuthToken(API_BASE_URL, ADMIN_KEY); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/notification-settings.test.ts
Line: 56-59
Comment:
**`beforeAll` at module scope, outside `run()` block**
Same pattern as `api-complete.test.ts` (which was already flagged in a prior thread): the `beforeAll` hook here runs at file scope, not inside the `run(...)` describe block. When `ADMIN_KEY` is absent, `run` is `describe.skip`, but `beforeAll` still executes (though it early-returns). Also in `users-keys-complete.test.ts:189-192`.
For consistency and proper scoping, both `beforeAll` and `afterAll` should be moved inside the `run(...)` block:
```suggestion
run("通知设置 - Webhook 目标与绑定（E2E）", () => {
beforeAll(async () => {
if (!ADMIN_KEY) return;
authToken = await loginAndGetAuthToken(API_BASE_URL, ADMIN_KEY);
});
```
How can I resolve this? If you propose a fix, please make it concise.
|
已按 CodeRabbit 的 nitpick 做了小幅性能优化（不改行为）：
本地已跑：bun run typecheck / bun run test / bun run build 通过。 |
|
补充修复本地 standalone 启动时静态资源 404 的问题：
本机已验证： |
| maxTagLength={64} | ||
| maxTags={50} | ||
| disabled={disabled} | ||
| validateTag={() => true} |
There was a problem hiding this comment.
validateTag={() => true} bypasses format validation - the invalid_format case in routing-section.tsx:288 onInvalidTag handler will never fire
if arbitrary UA strings are intentional, this is fine, but consider removing the dead invalid_format branch from the handler or adding a comment explaining why format validation is disabled
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/form/client-restrictions-editor.tsx
Line: 54
Comment:
`validateTag={() => true}` bypasses format validation - the `invalid_format` case in `routing-section.tsx:288` onInvalidTag handler will never fire
if arbitrary UA strings are intentional, this is fine, but consider removing the dead `invalid_format` branch from the handler or adding a comment explaining why format validation is disabled
How can I resolve this? If you propose a fix, please make it concise.| const [clientRestrictionsEnabled, setClientRestrictionsEnabled] = | ||
| useState(hasAnyClientRestrictions); |
There was a problem hiding this comment.
hasAnyClientRestrictions computed on every render but only used during mount - use function initializer to avoid unnecessary computation:
| const [clientRestrictionsEnabled, setClientRestrictionsEnabled] = | |
| useState(hasAnyClientRestrictions); | |
| const [clientRestrictionsEnabled, setClientRestrictionsEnabled] = useState(() => | |
| allowedClients.length > 0 || blockedClients.length > 0 | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx
Line: 77-78
Comment:
`hasAnyClientRestrictions` computed on every render but only used during mount - use function initializer to avoid unnecessary computation:
```suggestion
const [clientRestrictionsEnabled, setClientRestrictionsEnabled] = useState(() =>
allowedClients.length > 0 || blockedClients.length > 0
);
```
How can I resolve this? If you propose a fix, please make it concise.| animate={{ | ||
| width: `${((TAB_CONFIG.findIndex((t) => t.id === activeTab) + 1) / TAB_CONFIG.length) * 100}%`, | ||
| }} | ||
| initial={{ width: stepProgressWidth }} |
There was a problem hiding this comment.
progress bar animation disabled - both initial and animate set to same value stepProgressWidth, causing no visual transition when tabs change
remove initial prop to animate from current position:
| initial={{ width: stepProgressWidth }} | |
| animate={{ width: stepProgressWidth }} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx
Line: 277
Comment:
progress bar animation disabled - both `initial` and `animate` set to same value `stepProgressWidth`, causing no visual transition when tabs change
remove `initial` prop to animate from current position:
```suggestion
animate={{ width: stepProgressWidth }}
```
How can I resolve this? If you propose a fix, please make it concise.| }); | ||
| } catch (error) { | ||
| lastError = error; | ||
| if (attempt >= maxAttempts) { | ||
| break; | ||
| } | ||
|
|
||
| await sleep(Math.min(1000, 100 * 2 ** (attempt - 1))); |
There was a problem hiding this comment.
catch block retries on ALL fetch errors (network failures, DNS issues, timeouts) - masks legitimate test setup problems and causes tests to hang ~6s instead of failing fast
only retry specific recoverable errors like ECONNRESET/ETIMEDOUT, or remove this catch since line 82-93 already handles HTTP 503 retries
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/_helpers/auth.ts
Line: 59-66
Comment:
catch block retries on ALL fetch errors (network failures, DNS issues, timeouts) - masks legitimate test setup problems and causes tests to hang ~6s instead of failing fast
only retry specific recoverable errors like `ECONNRESET`/`ETIMEDOUT`, or remove this catch since line 82-93 already handles HTTP 503 retries
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
the tooltip wraps the entire move the Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx
Line: 550
Comment:
`Info` icon positioned inside `Select` breaks interaction - clicking the icon area won't open the select dropdown because it's overlaid on top
the tooltip wraps the entire `div` containing the `Select`, so hovering anywhere (including the select trigger) will show the tooltip, which conflicts with the select's own UI
move the `Info` icon outside the `Select` and only wrap the icon with the tooltip trigger
How can I resolve this? If you propose a fix, please make it concise. |
1 participant
Summary
node .next/standalone/server.js）缺失静态资源导致样式加载失败的问题（补齐.next/static与public）。Changes
Providers UI/UX
Viewport / safe-area
--cch-viewport-height*与.safe-area-bottom，并在多处布局/弹层改用变量驱动的 height/max-height，减少移动端地址栏伸缩造成的抖动。Infra / Tests / Redis
.next/standalone。Testing
bun run buildbun run lintbun run lint:fixbun run typecheckbun run testGreptile Summary
This PR is a large, multi-faceted changeset that addresses mobile/small-screen UX issues in the Provider settings form, introduces a unified viewport height strategy via CSS variables, adds a new "client restrictions" toggle+editor feature, and improves infrastructure (Redis client lifecycle, standalone build, E2E auth, test stability).
--cch-viewport-height*CSS variables inglobals.csswithdvhfallback via@supports, then migrates ~30 dialog/layout files from hardcodedvh/dvhto the new variables. Adds.safe-area-bottomutility for iOS home indicator padding.ClientRestrictionsEditorcomponent usingTagInputwith suggestion support and mutual exclusion logic (adding to allowed auto-removes from blocked and vice versa).orderutilities, changes mobile nav fromfixedto relative positioning, adds ARIAprogressbarattributes, and appliessafe-area-bottompadding.ModelSourceIndicatorto a standalone component, memoizesdisplayedModels, and addsmax-w-[calc(100vw-2rem)]to prevent Popover overflow on small screens.buildRedisOptionsForUrlingetRedisClient, adds stale client detection viastatus === "end", anendevent handler for auto-reset, and a robustcloseRediswith error fallback todisconnect().loginAndGetAuthTokenhelper that logs in via API and extracts session cookies, replacing direct token usage. All E2E tests migrated to this flow.copy-version-to-standalone.cjsnow also copies.next/staticandpublicto the standalone output.maxWorkersis now configurable.key-list-header.tsxreplaced with translation keys across all 5 languages. New client restriction toggle/description strings added.Confidence Score: 4/5
src/app/[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx(animation disabled),tests/e2e/_helpers/auth.ts(retry/backoff concerns from previous threads),src/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx(client restrictions toggle state management)Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["globals.css<br/>--cch-viewport-height* variables<br/>dvh fallback via @supports"] --> B["Layouts & Pages<br/>min-h-[var(--cch-viewport-height)]"] A --> C["Dialogs & Drawers<br/>max-h-[var(--cch-viewport-height-*)]"] A --> D["Provider Form<br/>max-h-[var(--cch-viewport-height-85)]"] D --> E["FormTabNav<br/>Mobile: relative + safe-area-bottom<br/>Desktop: sidebar"] D --> F["RoutingSection"] F --> G{"Client Restrictions<br/>Toggle Enabled?"} G -- Yes --> H["ClientRestrictionsEditor"] H --> I["TagInput (Allowed)<br/>with preset suggestions"] H --> J["TagInput (Blocked)<br/>with preset suggestions"] I -- "mutual exclusion" --> J J -- "mutual exclusion" --> I G -- "No (disable)" --> K["Clear both lists<br/>SET_ALLOWED_CLIENTS: []<br/>SET_BLOCKED_CLIENTS: []"] L["E2E Tests"] --> M["loginAndGetAuthToken<br/>POST /api/auth/login"] M --> N["Extract auth-token<br/>from Set-Cookie"] N --> O["API calls with<br/>Bearer + Cookie auth"]Last reviewed commit: a7573cb