Conversation
…on leak createInitialState assigned nested objects (modelRedirects, allowedModels, groupPriorities, anthropicAdaptiveThinking) by reference from sourceProvider, causing clone form edits to mutate the React Query cache and corrupt the source provider display. Use structuredClone on sourceProvider at init time and on defaultInitialState in RESET_FORM to ensure full isolation.
Summary of ChangesHello @hank9999, 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! 此拉取请求解决了在供应商克隆或编辑时，由于表单状态对源供应商数据进行浅拷贝，导致修改表单时意外污染 React Query 缓存中源数据的问题。通过引入 Highlights
Changelog
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
|
📝 Walkthrough演进概览该PR修改了提供程序表单上下文的初始化和重置逻辑，使用structuredClone确保嵌套对象的深度克隆，并新增了全面的单元测试来验证深度复制的安全性。 变更内容
预估代码审查工作量🎯 3 (中等复杂度) | ⏱️ ~25 分钟 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context.tsx
Show resolved
Hide resolved
![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 很好地解决了供应商克隆时因浅拷贝导致的数据污染问题。
核心改动是：
- 在
createInitialState中对源provider对象使用structuredClone进行深拷贝，有效切断了表单状态与 React Query 缓存中数据的引用关系。 - 在
providerFormReducer的RESET_FORMaction 中，同样对defaultInitialState进行了处理，防止了重置表单时可能出现的引用共享问题。
你还添加了详尽的单元测试 provider-form-clone-deep-copy.test.ts，覆盖了克隆、编辑和新建等多种场景，确保了修复的正确性和健壮性，这非常棒。
代码整体质量很高。我只在 RESET_FORM 的实现上有一个小小的建议，旨在提高代码的清晰度，请查看具体评论。
再次感谢你的高质量提交！
src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context.tsx
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| case "RESET_FORM": | ||
| case "RESET_FORM": { | ||
| const fresh = structuredClone(defaultInitialState); | ||
| return { |
There was a problem hiding this comment.
[MEDIUM] [TEST-MISSING-CRITICAL] RESET_FORM deep-copy change is untested
Why this is a problem: This PR changes reset behavior to avoid shared references (const fresh = structuredClone(defaultInitialState);), but there is no unit test that would fail if RESET_FORM regresses back to reference-sharing.
Violated guideline: "Test Coverage - All new features must have unit test coverage of at least 80%" (CLAUDE.md)
Suggested fix (extend tests/unit/dashboard/provider-form-clone-deep-copy.test.ts):
import { describe, expect, it } from "vitest";
import {
createInitialState,
providerFormReducer,
} from "@/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context";
describe("providerFormReducer RESET_FORM deep-copy safety", () => {
it("returns fresh nested objects on each reset", () => {
const base = createInitialState("create");
const s1 = providerFormReducer(base, { type: "RESET_FORM" });
const s2 = providerFormReducer(base, { type: "RESET_FORM" });
expect(s1.routing.modelRedirects).not.toBe(s2.routing.modelRedirects);
expect(s1.routing.allowedModels).not.toBe(s2.routing.allowedModels);
expect(s1.routing.groupPriorities).not.toBe(s2.routing.groupPriorities);
});
it("preserves activeTab", () => {
const withTab = providerFormReducer(createInitialState("create"), {
type: "SET_ACTIVE_TAB",
payload: "routing",
});
const reset = providerFormReducer(withTab, { type: "RESET_FORM" });
expect(reset.ui.activeTab).toBe("routing");
});
});
There was a problem hiding this comment.
Applied PR size label size/S on #767.
Posted 1 inline review comment:
- [MEDIUM] [TEST-MISSING-CRITICAL] Missing unit coverage for RESET_FORM deep-copy behavior (
src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context.tsx:327).
Submitted the required review summary via gh pr review --comment.
There was a problem hiding this comment.
Code Review Summary
Clean, well-scoped bug fix that correctly applies structuredClone to sever shared references between form state and React Query cache. The fix is minimal and targeted. One gap in test coverage noted below.
PR Size: S
- Lines changed: 169
- Files changed: 2
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 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
High Priority Issues (Should Fix)
[Medium] [TEST-MISSING-CRITICAL] tests/unit/dashboard/provider-form-clone-deep-copy.test.ts:158 -- The 13 new tests thoroughly cover createInitialState deep-copy isolation, but the RESET_FORM reducer fix (the second structuredClone call at provider-form-context.tsx:327) has no test coverage. The PR description identifies both sites as having the same bug class, so both should be tested.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Missing
RESET_FORMreducer test - Code clarity - Good
Automated review by Claude AI
| expect(state.routing.anthropicAdaptiveThinking).toBeNull(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[Medium] [TEST-MISSING-CRITICAL] No test coverage for the RESET_FORM reducer's structuredClone fix
Why this is a problem: The PR description explicitly identifies the RESET_FORM reducer as having the same class of shallow-copy bug, and the fix adds structuredClone(defaultInitialState) at provider-form-context.tsx:327. However, the 13 new test cases only cover createInitialState -- the RESET_FORM reducer path has zero test coverage. If this structuredClone call were accidentally removed in a future change, no test would catch the regression.
Per CLAUDE.md: "All new features must have unit test coverage of at least 80%"
Suggested fix: Add test cases for the RESET_FORM reducer in this file:
import { providerFormReducer, createInitialState } from "@/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context";
describe("providerFormReducer RESET_FORM", () => {
it("returns a state with no shared references across resets", () => {
const initial = createInitialState("create");
const reset1 = providerFormReducer(initial, { type: "RESET_FORM" });
const reset2 = providerFormReducer(initial, { type: "RESET_FORM" });
expect(reset1.routing.modelRedirects).not.toBe(reset2.routing.modelRedirects);
expect(reset1.routing.allowedModels).not.toBe(reset2.routing.allowedModels);
expect(reset1.routing.groupPriorities).not.toBe(reset2.routing.groupPriorities);
});
it("preserves the active tab from current state", () => {
const initial = createInitialState("create");
const withTab = { ...initial, ui: { ...initial.ui, activeTab: "routing" as const } };
const reset = providerFormReducer(withTab, { type: "RESET_FORM" });
expect(reset.ui.activeTab).toBe("routing");
});
});
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
* fix(circuit-breaker): key errors should not trip endpoint circuit breaker Remove 3 recordEndpointFailure calls from response-handler streaming error paths (fake-200, non-200 HTTP, stream abort). These are key-level errors where the endpoint itself responded successfully. Only forwarder-level failures (timeout, network error) and probe failures should penalize the endpoint circuit breaker. Previously, a single bad API key could trip the endpoint breaker (threshold=3, open=5min), making ALL keys on that endpoint unavailable. * chore: format code (dev-3d584e5) * Merge pull request #767 from ding113/fix/provider-clone-deep-copy fix: 修复供应商克隆时因浅拷贝引用共享导致源供应商数据被意外污染的问题 * 增强配置表单输入警告提示 (#768) * feat: 增强配置表单输入警告提示 * fix: 修复 expiresAt 显示与配额刷新输入边界 * fix: 修复 expiresAt 解析兜底并改善刷新间隔输入体验 * fix: 刷新间隔输入取整并复用 clamp --------- Co-authored-by: tesgth032 <tesgth032@users.noreply.github.com> * feat(circuit-breaker): endpoint CB default-off + 524 decision chain audit (#773) * feat(circuit-breaker): endpoint circuit breaker default-off + 524 decision chain audit - Add ENABLE_ENDPOINT_CIRCUIT_BREAKER env var (default: false) to gate endpoint-level circuit breaker - Gate isEndpointCircuitOpen, recordEndpointFailure, recordEndpointSuccess, triggerEndpointCircuitBreakerAlert behind env switch - Add initEndpointCircuitBreaker() startup cleanup: clear stale Redis keys when feature disabled - Gate endpoint filtering in endpoint-selector (getPreferredProviderEndpoints, getEndpointFilterStats) - Fix 524 vendor-type timeout missing from decision chain: add chain entry with reason=vendor_type_all_timeout in forwarder - Add vendor_type_all_timeout to ProviderChainItem reason union type (both backend session.ts and frontend message.ts) - Add timeline rendering for vendor_type_all_timeout in provider-chain-formatter - Replace hardcoded Chinese strings in provider-selector circuit_open details with i18n keys - Add i18n translations for vendor_type_all_timeout and filterDetails (5 languages: zh-CN, zh-TW, en, ja, ru) - Enhance LogicTraceTab to render filterDetails via i18n lookup with fallback - Add endpoint_pool_exhausted and vendor_type_all_timeout to provider-chain-popover isActualRequest/getItemStatus - Add comprehensive unit tests for all changes (endpoint-circuit-breaker, endpoint-selector, provider-chain-formatter) * fix(i18n): fix Russian grammar errors and rate_limited translations - Fix Russian: "конечная точкаов" -> "конечных точек" (11 occurrences) - Fix Russian: "Ограничение стоимости" -> "Ограничение скорости" (rate_limited) - Fix zh-CN: "费用限制" -> "速率限制" (filterDetails.rate_limited) - Fix zh-TW: "費用限制" -> "速率限制" (filterDetails.rate_limited) - Add initEndpointCircuitBreaker() to dev environment in instrumentation.ts * fix(circuit-breaker): vendor type CB respects ENABLE_ENDPOINT_CIRCUIT_BREAKER Make vendor type circuit breaker controlled by the same ENABLE_ENDPOINT_CIRCUIT_BREAKER switch as endpoint circuit breaker. When disabled (default), vendor type CB will never trip or block providers, resolving user confusion about "vendor type temporary circuit breaker" skip reasons in decision chain. Changes: - Add ENABLE_ENDPOINT_CIRCUIT_BREAKER check in isVendorTypeCircuitOpen() - Add switch check in recordVendorTypeAllEndpointsTimeout() - Add tests for switch on/off behavior Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * 修复 Key 并发限制继承用户并发上限 (#772) * fix: Key 并发上限默认继承用户限制 - RateLimitGuard：Key limitConcurrentSessions=0 时回退到 User limitConcurrentSessions\n- Key 配额/使用量接口：并发上限按有效值展示\n- 单测覆盖并发继承逻辑；补齐 probe 测试的 endpoint-circuit-breaker mock 导出\n- 同步更新 biome.json schema 版本以匹配当前 Biome CLI * docs: 补齐并发上限解析工具注释 * refactor: 合并 Key 限额查询并补充并发单测 - getKeyQuotaUsage/getKeyLimitUsage：通过 leftJoin 一次取回 User 并发上限，避免额外查询\n- 新增 resolveKeyConcurrentSessionLimit 单测，覆盖关键分支\n- 修复 vacuum-filter bench 中的 Biome 报错 * fix: my-usage 并发上限继承用户限制 - getMyQuota：Key 并发为 0/null 时回退到 User 并发上限，保持与 Guard/Key 配额一致\n- 新增单测覆盖 Key->User 并发继承 * test: 补齐 my-usage 并发继承场景 - MyUsageQuota.keyLimitConcurrentSessions 收敛为 number（0 表示无限制）\n- OpenAPI 响应 schema 同步为非 nullable\n- my-usage 并发继承测试补充 Key>0 与 User=0 场景 --------- Co-authored-by: tesgth032 <tesgth032@users.noreply.github.com> --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: hank9999 <hank9999@qq.com> Co-authored-by: tesgth032 <tesgth032@hotmail.com> Co-authored-by: tesgth032 <tesgth032@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
2 participants
Summary
Fix shallow-copy reference sharing in the provider clone flow that caused mutations in the cloned form to pollute the source provider data held in React Query cache.
Two
structuredClonecalls were added:createInitialState-- deep-copies the source provider before extracting fields into form state.RESET_FORMreducer -- deep-copies thedefaultInitialStatesingleton to prevent shared references after form reset.Problem
When cloning a provider,
createInitialStateassigned nested objects (modelRedirects,allowedModels,groupPriorities,anthropicAdaptiveThinking) from the source provider directly into the new form state. These objects shared the same memory reference as the React Query cache entry for the source provider. Editing any of these fields in the clone form would mutate the cached source provider, causing the original provider to display incorrect data in the UI.The same class of bug existed in the
RESET_FORMreducer, which spread the module-leveldefaultInitialStatesingleton directly -- meaning a reset form could share mutable references with future form instances.Solution
Apply
structuredCloneat the entry point ofcreateInitialStateto deep-copy the entire source provider object before any field extraction:And in the
RESET_FORMreducer, clone the default state before spreading:This severs all nested references in a single operation rather than cloning individual fields.
Changes
Core Changes
src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context.tsx--structuredCloneapplied to source provider increateInitialStateand todefaultInitialStateinRESET_FORMreducerTests
tests/unit/dashboard/provider-form-clone-deep-copy.test.ts(new, 158 lines) -- 13 test cases covering:modelRedirects,allowedModels,groupPriorities,anthropicAdaptiveThinking_Copyname suffix and empty keyBreaking Changes
None. The function signatures and exported API are unchanged. The only behavioral difference is that form state no longer shares object references with the source provider.
Testing
Automated Tests
provider-form-clone-deep-copy.test.tsManual Testing
modelRedirects,allowedModels,groupPriorities, andanthropicAdaptiveThinkingconfiguredChecklist
Description enhanced by Claude AI