Skip to content

Commit 627851a

Browse files
kantordkantord
authored
fix: omit optional env vars when they are left empty (#614)
* fix: omit optional env vars when they are left empty fixes #611 * refactor tests * normalize whitespace-only env var form fiels * refactor: make env var normalization consistent
1 parent dddf5dc commit 627851a

File tree

7 files changed

+252
-66
lines changed

7 files changed

+252
-66
lines changed

‎renderer/src/common/lib/utils.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,11 @@ import { twMerge } from 'tailwind-merge'
44
export function cn(...inputs: ClassValue[]) {
55
return twMerge(clsx(inputs))
66
}
7+
8+
/**
9+
* Checks if an environment variable value is effectively empty (null, undefined, or whitespace-only).
10+
* This is used consistently across form validation and orchestration logic for environment variables.
11+
*/
12+
export function isEmptyEnvVar(value: string | undefined | null): boolean {
13+
return !value || value.trim() === ''
14+
}

‎renderer/src/features/mcp-servers/lib/__tests__/orchestrate-run-custom-server.test.tsx‎

Lines changed: 142 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -33,17 +33,43 @@ vi.mock('sonner', async () => {
3333
}
3434
})
3535

36+
/**
37+
* Creates reusable mocks for testing the orchestrateRunCustomServer function
38+
*/
39+
function createTestMocks(options?: {
40+
saveSecretImplementation?: () => Promise<{ key: string }>
41+
getIsServerReadyImplementation?: () => Promise<boolean>
42+
}) {
43+
return {
44+
mockSaveSecret: vi
45+
.fn()
46+
.mockImplementation(
47+
options?.saveSecretImplementation ||
48+
(() => Promise.resolve({ key: 'test-key' }))
49+
),
50+
mockCreateWorkload: vi.fn(),
51+
mockGetIsServerReady: vi
52+
.fn()
53+
.mockImplementation(
54+
options?.getIsServerReadyImplementation || (() => Promise.resolve(true))
55+
),
56+
mockQueryClient: {
57+
invalidateQueries: vi.fn(),
58+
} as unknown as QueryClient,
59+
}
60+
}
61+
3662
beforeEach(() => {
3763
vi.clearAllMocks()
3864
})
3965

4066
it('submits without any optional fields', async () => {
41-
const mockSaveSecret = vi.fn()
42-
const mockCreateWorkload = vi.fn()
43-
const mockGetIsServerReady = vi.fn().mockResolvedValue(true)
44-
const mockQueryClient = {
45-
invalidateQueries: vi.fn(),
46-
} as unknown as QueryClient
67+
const {
68+
mockSaveSecret,
69+
mockCreateWorkload,
70+
mockGetIsServerReady,
71+
mockQueryClient,
72+
} = createTestMocks()
4773

4874
const DATA: FormSchemaRunMcpCommand = {
4975
image: 'ghcr.io/github/github-mcp-server',
@@ -109,12 +135,12 @@ it('handles new secrets properly', async () => {
109135
})
110136
)
111137

112-
const mockSaveSecret = vi.fn()
113-
const mockCreateWorkload = vi.fn()
114-
const mockGetIsServerReady = vi.fn().mockResolvedValue(true)
115-
const mockQueryClient = {
116-
invalidateQueries: vi.fn(),
117-
} as unknown as QueryClient
138+
const {
139+
mockSaveSecret,
140+
mockCreateWorkload,
141+
mockGetIsServerReady,
142+
mockQueryClient,
143+
} = createTestMocks()
118144

119145
const DATA = {
120146
image: 'ghcr.io/github/github-mcp-server',
@@ -181,12 +207,12 @@ it('handles existing secrets from the store properly', async () => {
181207
})
182208
)
183209

184-
const mockSaveSecret = vi.fn()
185-
const mockCreateWorkload = vi.fn()
186-
const mockGetIsServerReady = vi.fn().mockResolvedValue(true)
187-
const mockQueryClient = {
188-
invalidateQueries: vi.fn(),
189-
} as unknown as QueryClient
210+
const {
211+
mockSaveSecret,
212+
mockCreateWorkload,
213+
mockGetIsServerReady,
214+
mockQueryClient,
215+
} = createTestMocks()
190216

191217
const DATA: FormSchemaRunMcpCommand = {
192218
image: 'ghcr.io/github/github-mcp-server',
@@ -233,12 +259,12 @@ it('handles naming collisions with secrets from the store', async () => {
233259
})
234260
)
235261

236-
const mockSaveSecret = vi.fn()
237-
const mockCreateWorkload = vi.fn()
238-
const mockGetIsServerReady = vi.fn().mockResolvedValue(true)
239-
const mockQueryClient = {
240-
invalidateQueries: vi.fn(),
241-
} as unknown as QueryClient
262+
const {
263+
mockSaveSecret,
264+
mockCreateWorkload,
265+
mockGetIsServerReady,
266+
mockQueryClient,
267+
} = createTestMocks()
242268

243269
const DATA: FormSchemaRunMcpCommand = {
244270
image: 'ghcr.io/github/github-mcp-server',
@@ -284,12 +310,14 @@ it('handles naming collisions with secrets from the store', async () => {
284310
})
285311

286312
it('handles both new and existing secrets', async () => {
287-
const mockSaveSecret = vi.fn().mockResolvedValue({ key: 'new_secret_key' })
288-
const mockCreateWorkload = vi.fn()
289-
const mockGetIsServerReady = vi.fn().mockResolvedValue(true)
290-
const mockQueryClient = {
291-
invalidateQueries: vi.fn(),
292-
} as unknown as QueryClient
313+
const {
314+
mockSaveSecret,
315+
mockCreateWorkload,
316+
mockGetIsServerReady,
317+
mockQueryClient,
318+
} = createTestMocks({
319+
saveSecretImplementation: () => Promise.resolve({ key: 'new_secret_key' }),
320+
})
293321

294322
const DATA: FormSchemaRunMcpCommand = {
295323
image: 'ghcr.io/github/github-mcp-server',
@@ -338,12 +366,14 @@ it('handles both new and existing secrets', async () => {
338366

339367
it('handles error when saving a secret fails', async () => {
340368
const mockError = new Error('Failed to save secret')
341-
const mockSaveSecret = vi.fn().mockRejectedValue(mockError)
342-
const mockCreateWorkload = vi.fn()
343-
const mockGetIsServerReady = vi.fn()
344-
const mockQueryClient = {
345-
invalidateQueries: vi.fn(),
346-
} as unknown as QueryClient
369+
const {
370+
mockSaveSecret,
371+
mockCreateWorkload,
372+
mockGetIsServerReady,
373+
mockQueryClient,
374+
} = createTestMocks({
375+
saveSecretImplementation: () => Promise.reject(mockError),
376+
})
347377

348378
const DATA: FormSchemaRunMcpCommand = {
349379
image: 'ghcr.io/github/github-mcp-server',
@@ -380,12 +410,12 @@ it('handles error when saving a secret fails', async () => {
380410
})
381411

382412
it('handles environment variables properly', async () => {
383-
const mockSaveSecret = vi.fn()
384-
const mockCreateWorkload = vi.fn()
385-
const mockGetIsServerReady = vi.fn().mockResolvedValue(true)
386-
const mockQueryClient = {
387-
invalidateQueries: vi.fn(),
388-
} as unknown as QueryClient
413+
const {
414+
mockSaveSecret,
415+
mockCreateWorkload,
416+
mockGetIsServerReady,
417+
mockQueryClient,
418+
} = createTestMocks()
389419

390420
const DATA: FormSchemaRunMcpCommand = {
391421
image: 'ghcr.io/github/github-mcp-server',
@@ -415,13 +445,63 @@ it('handles environment variables properly', async () => {
415445
})
416446
})
417447

448+
it('filters out environment variables with empty values', async () => {
449+
const {
450+
mockSaveSecret,
451+
mockCreateWorkload,
452+
mockGetIsServerReady,
453+
mockQueryClient,
454+
} = createTestMocks()
455+
456+
const DATA: FormSchemaRunMcpCommand = {
457+
image: 'ghcr.io/github/github-mcp-server',
458+
name: 'env-var-filter-test',
459+
transport: 'stdio',
460+
type: 'docker_image',
461+
envVars: [
462+
{ name: 'DEBUG', value: 'true' },
463+
{ name: 'PORT', value: '8080' },
464+
{ name: 'OPTIONAL_VAR', value: '' }, // Empty value should be omitted
465+
{ name: 'ANOTHER_OPTIONAL', value: ' ' }, // Whitespace-only should be omitted
466+
{ name: 'REQUIRED_VAR', value: 'some-value' },
467+
],
468+
secrets: [],
469+
cmd_arguments: '',
470+
}
471+
472+
await orchestrateRunCustomServer({
473+
createWorkload: mockCreateWorkload,
474+
data: DATA,
475+
getIsServerReady: mockGetIsServerReady,
476+
queryClient: mockQueryClient,
477+
saveSecret: mockSaveSecret,
478+
})
479+
480+
expect(mockCreateWorkload).toHaveBeenCalledWith({
481+
body: expect.objectContaining({
482+
env_vars: ['DEBUG=true', 'PORT=8080', 'REQUIRED_VAR=some-value'],
483+
}),
484+
})
485+
// OPTIONAL_VAR and ANOTHER_OPTIONAL should be omitted
486+
expect(mockCreateWorkload).toHaveBeenCalledWith(
487+
expect.not.objectContaining({
488+
body: expect.objectContaining({
489+
env_vars: expect.arrayContaining([
490+
'OPTIONAL_VAR=',
491+
'ANOTHER_OPTIONAL= ',
492+
]),
493+
}),
494+
})
495+
)
496+
})
497+
418498
it('handles command arguments properly', async () => {
419-
const mockSaveSecret = vi.fn()
420-
const mockCreateWorkload = vi.fn()
421-
const mockGetIsServerReady = vi.fn().mockResolvedValue(true)
422-
const mockQueryClient = {
423-
invalidateQueries: vi.fn(),
424-
} as unknown as QueryClient
499+
const {
500+
mockSaveSecret,
501+
mockCreateWorkload,
502+
mockGetIsServerReady,
503+
mockQueryClient,
504+
} = createTestMocks()
425505

426506
const DATA: FormSchemaRunMcpCommand = {
427507
image: 'ghcr.io/github/github-mcp-server',
@@ -449,12 +529,14 @@ it('handles command arguments properly', async () => {
449529
})
450530

451531
it('shows warning toast when server is not ready', async () => {
452-
const mockSaveSecret = vi.fn()
453-
const mockCreateWorkload = vi.fn()
454-
const mockGetIsServerReady = vi.fn().mockResolvedValue(false)
455-
const mockQueryClient = {
456-
invalidateQueries: vi.fn(),
457-
} as unknown as QueryClient
532+
const {
533+
mockSaveSecret,
534+
mockCreateWorkload,
535+
mockGetIsServerReady,
536+
mockQueryClient,
537+
} = createTestMocks({
538+
getIsServerReadyImplementation: () => Promise.resolve(false),
539+
})
458540

459541
const DATA: FormSchemaRunMcpCommand = {
460542
image: 'ghcr.io/github/github-mcp-server',
@@ -486,12 +568,12 @@ it('shows warning toast when server is not ready', async () => {
486568
})
487569

488570
it('handles package manager type properly', async () => {
489-
const mockSaveSecret = vi.fn()
490-
const mockCreateWorkload = vi.fn()
491-
const mockGetIsServerReady = vi.fn().mockResolvedValue(true)
492-
const mockQueryClient = {
493-
invalidateQueries: vi.fn(),
494-
} as unknown as QueryClient
571+
const {
572+
mockSaveSecret,
573+
mockCreateWorkload,
574+
mockGetIsServerReady,
575+
mockQueryClient,
576+
} = createTestMocks()
495577

496578
const DATA: FormSchemaRunMcpCommand = {
497579
package_name: 'my-package',

‎renderer/src/features/mcp-servers/lib/orchestrate-run-custom-server.tsx‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import type { DefinedSecret, PreparedSecret } from '@/common/types/secrets'
2121
import { prepareSecretsWithoutNamingCollision } from '@/common/lib/secrets/prepare-secrets-without-naming-collision'
2222
import { trackEvent } from '@/common/lib/analytics'
2323
import { restartClientNotification } from './restart-client-notification'
24+
import { isEmptyEnvVar } from '@/common/lib/utils'
2425

2526
type SaveSecretFn = UseMutateAsyncFunction<
2627
V1CreateSecretResponse,
@@ -116,9 +117,12 @@ async function saveSecrets(
116117

117118
/**
118119
* Maps environment variables from the form into the format expected by the API.
120+
* Filters out environment variables with empty or whitespace-only values.
119121
*/
120122
function mapEnvVars(envVars: { name: string; value: string }[]) {
121-
return envVars.map((envVar) => `${envVar.name}=${envVar.value}`)
123+
return envVars
124+
.filter((envVar) => !isEmptyEnvVar(envVar.value))
125+
.map((envVar) => `${envVar.name}=${envVar.value}`)
122126
}
123127

124128
/**

‎renderer/src/features/registry-servers/components/__tests__/form-run-from-registry.test.tsx‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -486,6 +486,68 @@ describe('FormRunFromRegistry', () => {
486486
)
487487
})
488488

489+
it('validates that whitespace-only values are invalid for required fields', async () => {
490+
const mockInstallServerMutation = vi.fn()
491+
mockUseRunFromRegistry.mockReturnValue({
492+
installServerMutation: mockInstallServerMutation,
493+
checkServerStatus: vi.fn(),
494+
isErrorSecrets: false,
495+
isPendingSecrets: false,
496+
})
497+
498+
const server = { ...REGISTRY_SERVER }
499+
server.env_vars = ENV_VARS_REQUIRED
500+
501+
render(
502+
<QueryClientProvider client={queryClient}>
503+
<FormRunFromRegistry
504+
isOpen={true}
505+
onOpenChange={vi.fn()}
506+
server={server}
507+
/>
508+
</QueryClientProvider>
509+
)
510+
511+
await waitFor(() => {
512+
expect(screen.getByRole('dialog')).toBeVisible()
513+
})
514+
515+
// Fill in server name
516+
await userEvent.type(
517+
screen.getByLabelText('Server name'),
518+
'my-awesome-server',
519+
{
520+
initialSelectionStart: 0,
521+
initialSelectionEnd: REGISTRY_SERVER.name?.length,
522+
}
523+
)
524+
525+
// Fill required fields with whitespace-only values
526+
await userEvent.type(screen.getByLabelText('ENV_VAR value'), ' ')
527+
await userEvent.type(screen.getByLabelText('SECRET value'), '\t\n ')
528+
529+
await userEvent.click(
530+
screen.getByRole('button', { name: 'Install server' })
531+
)
532+
533+
await waitFor(() => {
534+
expect(mockInstallServerMutation).not.toHaveBeenCalled()
535+
})
536+
537+
// Check that validation errors are shown for whitespace-only values
538+
await waitFor(() => {
539+
expect(screen.getByLabelText('SECRET value')).toHaveAttribute(
540+
'aria-invalid',
541+
'true'
542+
)
543+
})
544+
545+
expect(screen.getByLabelText('ENV_VAR value')).toHaveAttribute(
546+
'aria-invalid',
547+
'true'
548+
)
549+
})
550+
489551
it('shows loading state when submitting', async () => {
490552
const mockInstallServerMutation = vi.fn()
491553
mockUseRunFromRegistry.mockReturnValue({

0 commit comments

Comments
 (0)