-
Notifications
You must be signed in to change notification settings - Fork 3k
fix: refresh provider settings on profile switch in Settings view #11460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -313,3 +313,88 @@ describe("OpenAICompatible Component - includeMaxTokens checkbox", () => { | |||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| describe("OpenAICompatible Component - Profile Switch Sync", () => { | ||||||||||||||||||||||||||||
| const mockSetApiConfigurationField = vi.fn() | ||||||||||||||||||||||||||||
| const mockOrganizationAllowList = { | ||||||||||||||||||||||||||||
| allowAll: true, | ||||||||||||||||||||||||||||
| providers: {}, | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| beforeEach(() => { | ||||||||||||||||||||||||||||
| vi.clearAllMocks() | ||||||||||||||||||||||||||||
| vi.useFakeTimers() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| afterEach(() => { | ||||||||||||||||||||||||||||
| vi.useRealTimers() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| it("should sync custom headers when apiConfiguration changes (profile switch)", () => { | ||||||||||||||||||||||||||||
| const initialConfig: Partial<ProviderSettings> = { | ||||||||||||||||||||||||||||
| openAiHeaders: { "X-Profile": "old-profile" }, | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const { rerender } = render( | ||||||||||||||||||||||||||||
| <OpenAICompatible | ||||||||||||||||||||||||||||
| apiConfiguration={initialConfig as ProviderSettings} | ||||||||||||||||||||||||||||
| setApiConfigurationField={mockSetApiConfigurationField} | ||||||||||||||||||||||||||||
| organizationAllowList={mockOrganizationAllowList} | ||||||||||||||||||||||||||||
| />, | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Simulate profile switch: re-render with new headers | ||||||||||||||||||||||||||||
| const newConfig: Partial<ProviderSettings> = { | ||||||||||||||||||||||||||||
| openAiHeaders: { "X-Profile": "new-profile", Authorization: "Bearer token123" }, | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| rerender( | ||||||||||||||||||||||||||||
| <OpenAICompatible | ||||||||||||||||||||||||||||
| apiConfiguration={newConfig as ProviderSettings} | ||||||||||||||||||||||||||||
| setApiConfigurationField={mockSetApiConfigurationField} | ||||||||||||||||||||||||||||
| organizationAllowList={mockOrganizationAllowList} | ||||||||||||||||||||||||||||
| />, | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // After the debounced write-back fires, it should not overwrite the new headers | ||||||||||||||||||||||||||||
| // with old ones. Advance timers to trigger the debounced write-back. | ||||||||||||||||||||||||||||
| vi.advanceTimersByTime(350) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // The write-back should be guarded: since the local state is synced to new headers, | ||||||||||||||||||||||||||||
| // and the new headers match the config, no write should occur (no-op guard) | ||||||||||||||||||||||||||||
| const writeBackCalls = mockSetApiConfigurationField.mock.calls.filter( | ||||||||||||||||||||||||||||
| (call: any[]) => call[0] === "openAiHeaders", | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| // If there are write-back calls, they should be writing the NEW headers, not the old ones | ||||||||||||||||||||||||||||
| for (const call of writeBackCalls) { | ||||||||||||||||||||||||||||
| const writtenHeaders = call[1] as Record<string, string> | ||||||||||||||||||||||||||||
| expect(writtenHeaders).not.toEqual({ "X-Profile": "old-profile" }) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+365
to
+372
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test uses Additionally, when the sync + guard work correctly,
Suggested change
Fix it with Roo Code or mention @roomote and request a fix. |
||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| it("should not write back headers when they match the current config", () => { | ||||||||||||||||||||||||||||
| const config: Partial<ProviderSettings> = { | ||||||||||||||||||||||||||||
| openAiHeaders: { "X-Custom": "value" }, | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| render( | ||||||||||||||||||||||||||||
| <OpenAICompatible | ||||||||||||||||||||||||||||
| apiConfiguration={config as ProviderSettings} | ||||||||||||||||||||||||||||
| setApiConfigurationField={mockSetApiConfigurationField} | ||||||||||||||||||||||||||||
| organizationAllowList={mockOrganizationAllowList} | ||||||||||||||||||||||||||||
| />, | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Clear initial calls | ||||||||||||||||||||||||||||
| mockSetApiConfigurationField.mockClear() | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Advance past the debounce timer | ||||||||||||||||||||||||||||
| vi.advanceTimersByTime(350) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // The write-back should detect that the headers are unchanged and skip the write | ||||||||||||||||||||||||||||
| const headerWriteCalls = mockSetApiConfigurationField.mock.calls.filter( | ||||||||||||||||||||||||||||
| (call: any[]) => call[0] === "openAiHeaders", | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| expect(headerWriteCalls).toHaveLength(0) | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These sync effects are redundant for the profile-switch scenario described in the PR. The
key={currentApiConfigName}onApiOptionsinSettingsView.tsxalready causesOpenAICompatibleto fully unmount and remount when the profile changes, so theuseStateinitializer at line 50 reads fresh values from the newapiConfigurationdirectly. The sync effects would only fire ifopenAiHeaderschanged without acurrentApiConfigNamechange (no remount).Also, the comment says this "mirrors the pattern in ApiOptions.tsx," but the
ApiOptionsversion at line 146 includescustomHeadersin its dependency array whereas this version omits it with aneslint-disable. Consider either removing these effects if the key-based remount is the intended mechanism, or updating the comments to clarify they're defensive measures for non-profile-switch config changes.Fix it with Roo Code or mention @roomote and request a fix.