Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions webview-ui/src/components/settings/SettingsView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -207,17 +207,18 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t

const apiConfiguration = useMemo(() => cachedState.apiConfiguration ?? {}, [cachedState.apiConfiguration])

useEffect(() => {
// Update only when currentApiConfigName is changed.
// Expected to be triggered by loadApiConfiguration/upsertApiConfiguration.
if (prevApiConfigName.current === currentApiConfigName) {
return
}

// Synchronous state update during render when profile changes.
// This follows the React pattern for "adjusting state when a prop changes"
// and ensures cachedState is updated BEFORE child components (like ApiOptions)
// render, so they receive the correct apiConfiguration immediately.
// Using useEffect here would cause a timing issue: the key-based remount of
// ApiOptions would occur before cachedState is updated, initializing local
// state from the stale (old profile) config.
if (prevApiConfigName.current !== currentApiConfigName) {
setCachedState((prevCachedState) => ({ ...prevCachedState, ...extensionState }))
prevApiConfigName.current = currentApiConfigName
setChangeDetected(false)
}, [currentApiConfigName, extensionState])
}

// Bust the cache when settings are imported.
useEffect(() => {
Expand Down Expand Up @@ -767,6 +768,7 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
}
/>
<ApiOptions
key={currentApiConfigName}
uriScheme={uriScheme}
apiConfiguration={apiConfiguration}
setApiConfigurationField={setApiConfigurationField}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,20 @@ export const OpenAICompatible = ({
return Object.entries(headers)
})

// Sync local customHeaders state when apiConfiguration changes externally
// (e.g., on profile switch). This mirrors the pattern in ApiOptions.tsx.
useEffect(() => {
const propHeaders = apiConfiguration?.openAiHeaders || {}
if (JSON.stringify(customHeaders) !== JSON.stringify(Object.entries(propHeaders))) {
setCustomHeaders(Object.entries(propHeaders))
}
}, [apiConfiguration?.openAiHeaders]) // eslint-disable-line react-hooks/exhaustive-deps
Comment on lines +55 to +62
Copy link
Contributor Author

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} on ApiOptions in SettingsView.tsx already causes OpenAICompatible to fully unmount and remount when the profile changes, so the useState initializer at line 50 reads fresh values from the new apiConfiguration directly. The sync effects would only fire if openAiHeaders changed without a currentApiConfigName change (no remount).

Also, the comment says this "mirrors the pattern in ApiOptions.tsx," but the ApiOptions version at line 146 includes customHeaders in its dependency array whereas this version omits it with an eslint-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.


// Sync azureApiVersionSelected when apiConfiguration changes externally
useEffect(() => {
setAzureApiVersionSelected(!!apiConfiguration?.azureApiVersion)
}, [apiConfiguration?.azureApiVersion])

const handleAddCustomHeader = useCallback(() => {
// Only update the local state to show the new row in the UI.
setCustomHeaders((prev) => [...prev, ["", ""]])
Expand Down Expand Up @@ -88,15 +102,19 @@ export const OpenAICompatible = ({

// Helper to convert array of tuples to object

// Add effect to update the parent component's state when local headers change
// Debounced write-back: update the parent config when local headers change.
// Guard against no-op writes to prevent stale overwrites on profile switch.
useEffect(() => {
const timer = setTimeout(() => {
const currentConfigHeaders = apiConfiguration?.openAiHeaders || {}
const headerObject = convertHeadersToObject(customHeaders)
setApiConfigurationField("openAiHeaders", headerObject, false)
if (JSON.stringify(currentConfigHeaders) !== JSON.stringify(headerObject)) {
setApiConfigurationField("openAiHeaders", headerObject, false)
}
}, 300)

return () => clearTimeout(timer)
}, [customHeaders, setApiConfigurationField])
}, [customHeaders, apiConfiguration?.openAiHeaders, setApiConfigurationField])

const handleInputChange = useCallback(
<K extends keyof ProviderSettings, E>(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test uses rerender to simulate a profile switch, but in production the key={currentApiConfigName} on ApiOptions causes OpenAICompatible to be unmounted and remounted -- not re-rendered in place. The test therefore exercises the sync effect path (lines 57-62) rather than the actual profile-switch code path.

Additionally, when the sync + guard work correctly, writeBackCalls is empty and the for loop body never executes, making the assertion vacuously true. Consider adding an explicit expect(writeBackCalls).toHaveLength(0) to make the expected outcome clear and to catch regressions where a stale write-back does fire.

Suggested change
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" })
}
const writeBackCalls = mockSetApiConfigurationField.mock.calls.filter(
(call: any[]) => call[0] === "openAiHeaders",
)
// No write-back should occur since local state is synced to new headers
expect(writeBackCalls).toHaveLength(0)

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)
})
})
Loading