Desktop: - No Custom Headers field for custom OpenAI-compatible providers #6681
Desktop: - No Custom Headers field for custom OpenAI-compatible providers #6681zanesq merged 14 commits intoblock:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for custom HTTP headers to custom OpenAI-compatible providers in the Desktop app, along with UI fixes for modal scrolling and duplicate state declaration.
Changes:
- Added custom headers section to CustomProviderForm with validation and add/remove/edit functionality
- Fixed modal scrolling by adding max-height and overflow to DialogContent
- Removed duplicate isFilePickerOpen state declaration in ChatInput
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| CustomProviderForm.tsx | Added custom headers feature with state management, validation (empty fields, spaces in keys), UI components (input fields, add/remove buttons), and form submission logic |
| ProviderGrid.tsx | Fixed modal scrolling by adding max-h-[90vh] and overflow-y-auto to DialogContent to handle long forms |
| ChatInput.tsx | Fixed duplicate isFilePickerOpen declaration by moving it earlier and removing the duplicate; included minor formatting cleanup |
| const handleAddHeader = () => { | ||
| const keyEmpty = !newHeaderKey.trim(); | ||
| const valueEmpty = !newHeaderValue.trim(); | ||
| const keyHasSpaces = newHeaderKey.includes(' '); | ||
|
|
||
| if (keyEmpty || valueEmpty) { | ||
| setInvalidHeaderFields({ | ||
| key: keyEmpty, | ||
| value: valueEmpty, | ||
| }); | ||
| setHeaderValidationError('Both header name and value must be provided'); | ||
| return; | ||
| } | ||
|
|
||
| if (keyHasSpaces) { | ||
| setInvalidHeaderFields({ | ||
| key: true, | ||
| value: false, | ||
| }); | ||
| setHeaderValidationError('Header name cannot contain spaces'); | ||
| return; | ||
| } | ||
|
|
||
| setHeaderValidationError(null); | ||
| setInvalidHeaderFields({ key: false, value: false }); | ||
| setHeaders([...headers, { key: newHeaderKey, value: newHeaderValue }]); | ||
| setNewHeaderKey(''); | ||
| setNewHeaderValue(''); | ||
| }; |
There was a problem hiding this comment.
The validation doesn't check for duplicate header keys. If a user adds the same header name twice (e.g., "Authorization" twice), both will be accepted but only the last value will be kept when converting to the object format in handleSubmit (lines 136-144). Consider adding a check to prevent duplicate keys.
|
Thanks, does the goose backend actually use these new header fields in the LLM requests? |
Yes, the backend fully supports and uses custom headers. In Moreover, this feature is already in CLI. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
ui/desktop/src/components/settings/providers/ProviderGrid.tsx:207
- When editing a provider, the headers field from editingProvider.config.headers is not being passed to the CustomProviderForm. This means existing headers will be lost when editing a provider. Add headers to the initialData object: headers: editingProvider.config.headers ?? null
const initialData = editingProvider && {
engine: editingProvider.config.engine,
display_name: editingProvider.config.display_name,
api_url: editingProvider.config.base_url,
api_key: '',
models: editingProvider.config.models.map((m) => m.name),
supports_streaming: editingProvider.config.supports_streaming ?? true,
};
ui/desktop/src/components/ChatInput.tsx:696
- Callee is not a function: it has type undefined.
const compressedDataUrl = await compressImageDataUrl(dataUrl);
ui/desktop/src/components/ChatInput.tsx:1039
- Callee is not a function: it has type undefined.
const compressedDataUrl = await compressImageDataUrl(dataUrl);
| api_key: apiKey, | ||
| models: modelList, | ||
| supports_streaming: supportsStreaming, | ||
| headers: Object.keys(headersObject).length > 0 ? headersObject : null, |
There was a problem hiding this comment.
The backend API does not support the 'headers' field in UpdateCustomProviderRequest. Looking at crates/goose-server/src/routes/config_management.rs (lines 82-89), the UpdateCustomProviderRequest struct only includes: engine, display_name, api_url, api_key, models, and supports_streaming. Sending headers will cause the backend to reject this request or silently ignore the field.
The backend must be updated to accept headers before this frontend change can work. The DeclarativeProviderConfig in the backend (crates/goose/src/config/declarative_providers.rs:39) already has support for headers, but the create_custom_provider and update_custom_provider functions do not accept headers as parameters.
There was a problem hiding this comment.
@Lymah123 can you review these copilot reviews and resolve or comment?
| if (initialData.headers) { | ||
| const headerList = Object.entries(initialData.headers).map(([key, value]) => ({ | ||
| key, | ||
| value, | ||
| })); | ||
| setHeaders(headerList); | ||
| } |
There was a problem hiding this comment.
The initialData prop is typed as UpdateCustomProviderRequest in the function signature, but UpdateCustomProviderRequest doesn't have a headers field according to types.gen.ts. The code tries to access initialData.headers on line 53, which would cause a TypeScript error. Either UpdateCustomProviderRequest needs to be updated to include headers, or a different type should be used for initialData.
| const handleHeaderChange = (index: number, field: 'key' | 'value', value: string) => { | ||
| const updatedHeaders = [...headers]; | ||
| updatedHeaders[index][field] = value; | ||
| setHeaders(updatedHeaders); |
There was a problem hiding this comment.
Header editing (handleHeaderChange) doesn't validate the changes. Users can edit existing headers to have invalid values (e.g., spaces in key, empty key or value). Consider adding validation when submitting the form, or preventing invalid edits in real-time.
| setHeaders(updatedHeaders); | |
| setHeaders(updatedHeaders); | |
| const currentHeader = updatedHeaders[index]; | |
| const keyEmpty = !currentHeader.key.trim(); | |
| const valueEmpty = !currentHeader.value.trim(); | |
| const keyHasSpaces = currentHeader.key.includes(' '); | |
| if (keyEmpty || valueEmpty) { | |
| setInvalidHeaderFields({ | |
| key: keyEmpty, | |
| value: valueEmpty, | |
| }); | |
| setHeaderValidationError('Both header name and value must be provided'); | |
| return; | |
| } | |
| if (keyHasSpaces) { | |
| setInvalidHeaderFields({ | |
| key: true, | |
| value: false, | |
| }); | |
| setHeaderValidationError('Header name cannot contain spaces'); | |
| return; | |
| } | |
| clearHeaderValidation(); |
| onClick={handleAddHeader} | ||
| variant="ghost" | ||
| type="button" | ||
| className="flex items-center justify-start gap-1 px-2 pr-4 text-sm rounded-full text-textStandard bg-background-default border border-borderSubtle hover:border-borderStandard transition-colors min-w-[60px] h-9 [&>svg]:size-4!" |
There was a problem hiding this comment.
The Tailwind important modifier syntax is incorrect. The ! should be placed before the property name, not after. Change [&>svg]:size-4! to [&>svg]:!size-4 to match the established pattern used in HeadersSection.tsx.
| className="flex items-center justify-start gap-1 px-2 pr-4 text-sm rounded-full text-textStandard bg-background-default border border-borderSubtle hover:border-borderStandard transition-colors min-w-[60px] h-9 [&>svg]:size-4!" | |
| className="flex items-center justify-start gap-1 px-2 pr-4 text-sm rounded-full text-textStandard bg-background-default border border-borderSubtle hover:border-borderStandard transition-colors min-w-[60px] h-9 [&>svg]:!size-4" |
| key: keyEmpty, | ||
| value: valueEmpty, | ||
| }); | ||
| setHeaderValidationError('Both header name and value must be provided'); |
There was a problem hiding this comment.
The validation error message is inconsistent with the established pattern in HeadersSection.tsx. Change "Both header name and value must be provided" to "Both header name and value must be entered" to maintain consistency across the codebase. (Reference: ui/desktop/src/components/settings/extensions/modal/HeadersSection.tsx:53)
|
The PR is ready for review. |
| base_url: api_url, | ||
| models: model_infos, | ||
| headers: existing_config.headers, | ||
| headers: headers.or(existing_config.headers), |
There was a problem hiding this comment.
I think this logic means:
If headers is Some(...), use the new headers
If headers is None, keep existing headers
This makes it impossible to clear headers once set. If a user wants to remove all custom headers, passing None will just keep the old ones.
Maybe the UI should distinguish between "not provided" and "explicitly empty"?
change to something like
headers: if headers.is_some() { headers } else { existing_config.headers },
// Or better: always use the provided headers value (including None to clear)
headers: headers,
And update the UI to explicitly send {} when user wants no headers.
There was a problem hiding this comment.
Alright, I'd do that. Thanks.
| Ok(provider_config) | ||
| } | ||
|
|
||
| #[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
The function now has 8 parameters.
Rather than adding the clippy allow maybe create a struct like UpdateCustomProviderParams to group these? This would also make future additions cleaner.
| api_key: apiKey, | ||
| models: modelList, | ||
| supports_streaming: supportsStreaming, | ||
| headers: Object.keys(headersObject).length > 0 ? headersObject : null, |
There was a problem hiding this comment.
@Lymah123 can you review these copilot reviews and resolve or comment?
|
Thanks for the feedback and the suggestions @zanesq . I will look into that. Yes, I will resolve the Copilot reviews. I was waiting for human reviews and working on the suggestions together. |
- Added custom headers section to CustomProviderForm.tsx - Fixed modal scrolling issue in ProviderGrid.tsx - Fixed duplicate variable declaration in ChatInput.tsx Signed-off-by: Lymah123 <fimihanodunola625@gmail.com>
Signed-off-by: Lymah123 <fimihanodunola625@gmail.com>
- Added #[allow(clippy::too_many_arguments)] to update_custom_provider - Resolved all merge conflict markers in ChatInput.tsx Signed-off-by: Lymah123 <fimihanodunola625@gmail.com>
- Remove unused StreamableHttpOptions import - Extract URLs from StreamableHttpOptions for parse_cli_flag_extensions - Remove extra blank line for fmt check Signed-off-by: Lymah123 <fimihanodunola625@gmail.com>
| .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; | ||
| >>>>>>> 04269837bc8 (Backend: Add headers parameter to update_custom_provider) |
There was a problem hiding this comment.
Merge conflict markers left in the code. Line 749 already ends the function call with )?; but lines 750-751 contain a duplicate error handling chain and a merge conflict marker that should be removed.
| .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; | |
| >>>>>>> 04269837bc8 (Backend: Add headers parameter to update_custom_provider) |
| let streamable_urls: Vec<String> = session_config | ||
| .streamable_http_extensions | ||
| .iter() | ||
| .map(|opt| opt.url.clone()) | ||
| .collect(); | ||
|
|
||
| let cli_flag_extensions_to_load = parse_cli_flag_extensions( | ||
| &session_config.extensions, | ||
| &session_config.streamable_http_extensions, | ||
| &streamable_urls, |
There was a problem hiding this comment.
Type mismatch in function call. The function parse_cli_flag_extensions expects &[StreamableHttpOptions] (defined at line 33 of builder.rs), but this change passes &[String]. The StreamableHttpOptions struct contains both url and timeout information (used at line 59 in parse_cli_flag_extensions), so extracting just the URLs will break the functionality and cause a compilation error.
Signed-off-by: Lymah123 <fimihanodunola625@gmail.com>
Hello @zanesq ! |
|
@Lymah123 apologies been real busy, can you pull in main and resolve the conflicts then I will do some more testing on this |
…rForm Signed-off-by: Lymah123 <fimihanodunola625@gmail.com>
Signed-off-by: Lymah123 <fimihanodunola625@gmail.com>
| const keyEmpty = !newHeaderKey.trim(); | ||
| const valueEmpty = !newHeaderValue.trim(); | ||
| const keyHasSpaces = newHeaderKey.includes(' '); | ||
| const isDuplicate = headers.some(h => h.key.trim() === newHeaderKey.trim()); |
There was a problem hiding this comment.
Duplicate header detection is case-sensitive, but HTTP header names are case-insensitive; allowing both X-Foo and x-foo will lead to one overwriting the other when converted into a real header map. Normalize names (e.g., compare toLowerCase().trim()) for duplicate checks and ideally also when serializing to the request object.
| const isDuplicate = headers.some(h => h.key.trim() === newHeaderKey.trim()); | |
| const normalizedNewKey = newHeaderKey.trim().toLowerCase(); | |
| const isDuplicate = headers.some(h => h.key.trim().toLowerCase() === normalizedNewKey); |
| const keyEmpty = !newKey.trim(); | ||
| const valueEmpty = !newValue.trim(); | ||
| const keyHasSpaces = newKey.includes(' '); | ||
| const isDuplicate = headers.some(h => h.key.trim() === newKey.trim()); |
There was a problem hiding this comment.
Duplicate header detection is currently case-sensitive, but HTTP header names are case-insensitive; this can allow logically-duplicate headers that later overwrite each other at request time. Normalize header names (e.g., trim().toLowerCase()) in the duplicate check (and ideally during add) to match runtime behavior.
| const isDuplicate = headers.some(h => h.key.trim() === newKey.trim()); | |
| const normalizedNewKey = newKey.trim().toLowerCase(); | |
| const isDuplicate = headers.some( | |
| h => h.key.trim().toLowerCase() === normalizedNewKey | |
| ); |
| const isDuplicate = formData.headers.some((h, i) => i !== index && h.key.trim() === value.trim()); | ||
| if (isDuplicate && value.trim() !== '') { |
There was a problem hiding this comment.
The duplicate-header guard compares keys case-sensitively, but HTTP header names are case-insensitive, so users can enter Authorization and authorization and have one overwrite the other when building the request headers. Normalize to a consistent case for duplicate detection (and consider normalizing on write).
| const isDuplicate = formData.headers.some((h, i) => i !== index && h.key.trim() === value.trim()); | |
| if (isDuplicate && value.trim() !== '') { | |
| const trimmedNewKey = value.trim(); | |
| const normalizedNewKey = trimmedNewKey.toLowerCase(); | |
| const isDuplicate = formData.headers.some( | |
| (h, i) => i !== index && h.key.trim().toLowerCase() === normalizedNewKey, | |
| ); | |
| if (isDuplicate && trimmedNewKey !== '') { |
| if (initialData.headers) { | ||
| const headerList = Object.entries(initialData.headers).map(([key, value]) => ({ | ||
| key, | ||
| value, | ||
| })); |
There was a problem hiding this comment.
Custom headers won’t be preserved when editing an existing provider because initialData.headers is the only source for initializing headers, but the modal currently builds initialData without copying editingProvider.config.headers; this will cause updates to overwrite existing headers with an empty set. Pass headers: editingProvider.config.headers ?? undefined/null into initialData (and consider omitting headers on submit when unchanged) so edits don’t accidentally clear server-side config.
| const headersObject = headers.reduce( | ||
| (acc, header) => { | ||
| if (header.key.trim() && header.value.trim()) { | ||
| acc[header.key.trim()] = header.value.trim(); | ||
| } | ||
| return acc; | ||
| }, | ||
| {} as Record<string, string> | ||
| ); | ||
|
|
There was a problem hiding this comment.
newHeaderKey/newHeaderValue aren’t included in the submit payload, so if the user fills the fields and presses Enter or clicks “Create/Update” without clicking “Add”, the header is silently dropped. Consider handling Enter to call handleAddHeader, and/or blocking submit while there’s pending header input (similar to the extensions HeadersSection pattern) or auto-adding the pending header during submit.
| const headersObject = headers.reduce( | |
| (acc, header) => { | |
| if (header.key.trim() && header.value.trim()) { | |
| acc[header.key.trim()] = header.value.trim(); | |
| } | |
| return acc; | |
| }, | |
| {} as Record<string, string> | |
| ); | |
| const headersObject: Record<string, string> = {}; | |
| // Include any pending header input so it isn't dropped on submit | |
| if (newHeaderKey?.trim() && newHeaderValue?.trim()) { | |
| headersObject[newHeaderKey.trim()] = newHeaderValue.trim(); | |
| } | |
| headers.forEach((header) => { | |
| const key = header.key.trim(); | |
| const value = header.value.trim(); | |
| if (key && value) { | |
| headersObject[key] = value; | |
| } | |
| }); |
Signed-off-by: Lymah123 <fimihanodunola625@gmail.com>
Signed-off-by: Lymah123 <fimihanodunola625@gmail.com>
Signed-off-by: Lymah123 <fimihanodunola625@gmail.com>
| const isDuplicate = headers.some((h, i) => i !== index && h.key.trim() === value.trim()); | ||
| if (isDuplicate && value.trim() !== '') { | ||
| return; | ||
| } |
There was a problem hiding this comment.
handleHeaderChange prevents duplicate header names using a case-sensitive comparison, but headers are case-insensitive and handleAddHeader normalizes to lowercase; this can allow duplicates like Authorization vs authorization which then get silently overwritten when building headersObject on submit—normalize the duplicate check (and stored keys) consistently (e.g., trim + lowercase).
| const isDuplicate = headers.some((h, i) => i !== index && h.key.trim() === value.trim()); | |
| if (isDuplicate && value.trim() !== '') { | |
| return; | |
| } | |
| const normalizedValue = value.trim().toLowerCase(); | |
| const isDuplicate = headers.some( | |
| (h, i) => i !== index && h.key.trim().toLowerCase() === normalizedValue, | |
| ); | |
| if (isDuplicate && normalizedValue !== '') { | |
| return; | |
| } | |
| const updatedHeaders = [...headers]; | |
| updatedHeaders[index].key = normalizedValue; | |
| setHeaders(updatedHeaders); | |
| return; |
| models: modelList, | ||
| supports_streaming: supportsStreaming, | ||
| requires_auth: requiresApiKey, | ||
| headers: Object.keys(headersObject).length > 0 ? headersObject : {}, |
There was a problem hiding this comment.
headers is always sent as {} when no headers are present, which deserializes as Some(empty) on the backend and can unintentionally clear existing headers on update (the server merges via params.headers.or(existing_config.headers)); send undefined/null (omit the field) when there are no headers to preserve existing behavior and match the CLI’s Option<HashMap> semantics.
| headers: Object.keys(headersObject).length > 0 ? headersObject : {}, | |
| headers: Object.keys(headersObject).length > 0 ? headersObject : undefined, |
|
@zanesq fixed! |
Signed-off-by: Lymah123 <fimihanodunola625@gmail.com>
* main: fix text editor view broken (#7167) docs: White label guide (#6857) Add PATH detection back to developer extension (#7161) docs: pin version in ci/cd (#7168) Desktop: - No Custom Headers field for custom OpenAI-compatible providers (#6681) feat: edit model and extensions of a recipe from GUI (#6804) feat: MCP support for agentic CLI providers (#6972)
* origin/main: (33 commits) fix: replace panic with proper error handling in get_tokenizer (#7175) Lifei/smoke test for developer (#7174) fix text editor view broken (#7167) docs: White label guide (#6857) Add PATH detection back to developer extension (#7161) docs: pin version in ci/cd (#7168) Desktop: - No Custom Headers field for custom OpenAI-compatible providers (#6681) feat: edit model and extensions of a recipe from GUI (#6804) feat: MCP support for agentic CLI providers (#6972) docs: keyring fallback to secrets.yaml (#7165) feat: load provider/model specified inside the recipe config (#6884) fix ask-ai bot hitting tool call limits (#7162) fix flatpak icon (#7154) [docs] Skills Marketplace UI Improvements (#7158) More no-window flags (#7122) feat: Allow overriding default bat themes using environment variables (#7140) Make the system prompt smaller (#6991) Pre release script (#7145) Spelling (#7137) feat(mcp): upgrade rmcp to 0.15.0 and advertise MCP Apps UI extension capability (#6927) ...
Summary
This PR:
Type of Change
AI Assistance
Testing
Related Issues
closes #6048
Discussion: LINK (if any)
Screenshots/Demos (for UX changes)
Before:
After: