Skip to content

Commit cd42ebd

Browse files
BYKBYK
committed
address issues raised by agents
1 parent bcff50d commit cd42ebd

File tree

7 files changed

+95
-27
lines changed

7 files changed

+95
-27
lines changed

‎packages/browser/src/utils/spotlightConfig.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ export function getSpotlightConfig(): boolean | string | undefined {
5050
}
5151

5252
// Not a boolean, treat as custom URL string
53+
// Note: empty/whitespace strings are filtered by resolveSpotlightOptions
5354
if (DEBUG_BUILD) {
5455
debug.log(`[Spotlight] Found ${key}=${value} (custom URL) in environment variables`);
5556
}

‎packages/browser/test/sdk.test.ts‎

Lines changed: 18 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -251,9 +251,8 @@ describe('init', () => {
251251
globalThis.process = {
252252
env: {
253253
SENTRY_SPOTLIGHT: 'true',
254-
},
255-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
256-
} as any;
254+
} as Record<string, string>,
255+
} as NodeJS.Process;
257256

258257
// @ts-expect-error this is fine for testing
259258
const initAndBindSpy = vi.spyOn(SentryCore, 'initAndBind').mockImplementationOnce(() => {});
@@ -271,9 +270,8 @@ describe('init', () => {
271270
globalThis.process = {
272271
env: {
273272
SENTRY_SPOTLIGHT: 'false',
274-
},
275-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
276-
} as any;
273+
} as Record<string, string>,
274+
} as NodeJS.Process;
277275

278276
// @ts-expect-error this is fine for testing
279277
const initAndBindSpy = vi.spyOn(SentryCore, 'initAndBind').mockImplementationOnce(() => {});
@@ -291,9 +289,8 @@ describe('init', () => {
291289
globalThis.process = {
292290
env: {
293291
SENTRY_SPOTLIGHT: 'true',
294-
},
295-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
296-
} as any;
292+
} as Record<string, string>,
293+
} as NodeJS.Process;
297294

298295
// @ts-expect-error this is fine for testing
299296
const initAndBindSpy = vi.spyOn(SentryCore, 'initAndBind').mockImplementationOnce(() => {});
@@ -312,9 +309,8 @@ describe('init', () => {
312309
globalThis.process = {
313310
env: {
314311
SENTRY_SPOTLIGHT: 'http://env:5678/stream',
315-
},
316-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
317-
} as any;
312+
} as Record<string, string>,
313+
} as NodeJS.Process;
318314

319315
// @ts-expect-error this is fine for testing
320316
const initAndBindSpy = vi.spyOn(SentryCore, 'initAndBind').mockImplementationOnce(() => {});
@@ -332,9 +328,8 @@ describe('init', () => {
332328
globalThis.process = {
333329
env: {
334330
SENTRY_SPOTLIGHT: 'http://env:5678/stream',
335-
},
336-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
337-
} as any;
331+
} as Record<string, string>,
332+
} as NodeJS.Process;
338333

339334
// @ts-expect-error this is fine for testing
340335
const initAndBindSpy = vi.spyOn(SentryCore, 'initAndBind').mockImplementationOnce(() => {});
@@ -347,23 +342,22 @@ describe('init', () => {
347342
expect(spotlightIntegration).toBeDefined();
348343
});
349344

350-
it('respects priority order: SENTRY_SPOTLIGHT over PUBLIC_SENTRY_SPOTLIGHT', () => {
345+
it('respects priority order: PUBLIC_SENTRY_SPOTLIGHT over SENTRY_SPOTLIGHT', () => {
351346
originalProcess = globalThis.process;
352347
globalThis.process = {
353348
env: {
354-
SENTRY_SPOTLIGHT: 'true',
355-
PUBLIC_SENTRY_SPOTLIGHT: 'false',
356-
},
357-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
358-
} as any;
349+
PUBLIC_SENTRY_SPOTLIGHT: 'true',
350+
SENTRY_SPOTLIGHT: 'false',
351+
} as Record<string, string>,
352+
} as NodeJS.Process;
359353

360354
// @ts-expect-error this is fine for testing
361355
const initAndBindSpy = vi.spyOn(SentryCore, 'initAndBind').mockImplementationOnce(() => {});
362356
const options = getDefaultBrowserOptions({ dsn: PUBLIC_DSN, spotlight: undefined });
363357
init(options);
364358

365359
const optionsPassed = initAndBindSpy.mock.calls[0]?.[1];
366-
// Spotlight integration should be added (SENTRY_SPOTLIGHT=true wins)
360+
// Spotlight integration should be added (PUBLIC_SENTRY_SPOTLIGHT=true wins)
367361
const spotlightIntegration = optionsPassed?.integrations.find((i: Integration) => i.name === 'SpotlightBrowser');
368362
expect(spotlightIntegration).toBeDefined();
369363
});
@@ -373,9 +367,8 @@ describe('init', () => {
373367
globalThis.process = {
374368
env: {
375369
NEXT_PUBLIC_SENTRY_SPOTLIGHT: 'true',
376-
},
377-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
378-
} as any;
370+
} as Record<string, string>,
371+
} as NodeJS.Process;
379372

380373
// @ts-expect-error this is fine for testing
381374
const initAndBindSpy = vi.spyOn(SentryCore, 'initAndBind').mockImplementationOnce(() => {});

‎packages/browser/test/utils/spotlightConfig.test.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,26 @@ describe('getSpotlightConfig', () => {
5959
expect(getSpotlightConfig()).toBe(customUrl);
6060
});
6161

62+
it('returns undefined when SENTRY_SPOTLIGHT is an empty string', () => {
63+
globalThis.process = {
64+
env: {
65+
SENTRY_SPOTLIGHT: '',
66+
} as Record<string, string>,
67+
} as NodeJS.Process;
68+
69+
expect(getSpotlightConfig()).toBeUndefined();
70+
});
71+
72+
it('returns undefined when SENTRY_SPOTLIGHT is whitespace only', () => {
73+
globalThis.process = {
74+
env: {
75+
SENTRY_SPOTLIGHT: ' ',
76+
} as Record<string, string>,
77+
} as NodeJS.Process;
78+
79+
expect(getSpotlightConfig()).toBeUndefined();
80+
});
81+
6282
it('parses various truthy values correctly', () => {
6383
const truthyValues = ['true', '1', 'yes', 'on', 't', 'y', 'TRUE', 'YES'];
6484

‎packages/core/src/utils/resolveSpotlightOptions.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,14 @@ export function resolveSpotlightOptions(
1717

1818
if (typeof optionsSpotlight === 'string') {
1919
// Custom URL provided - ignore env vars
20-
return optionsSpotlight;
20+
// Treat empty strings as undefined to prevent invalid URL connections
21+
return optionsSpotlight.trim() === '' ? undefined : optionsSpotlight;
2122
}
2223

2324
// optionsSpotlight is true or undefined
2425
const envBool = typeof envSpotlight === 'boolean' ? envSpotlight : undefined;
25-
const envUrl = typeof envSpotlight === 'string' ? envSpotlight : undefined;
26+
// Treat empty/whitespace-only strings as undefined to prevent invalid URL connections
27+
const envUrl = typeof envSpotlight === 'string' && envSpotlight.trim() !== '' ? envSpotlight : undefined;
2628

2729
return optionsSpotlight === true
2830
? (envUrl ?? true) // true: use env URL if present, otherwise true

‎packages/core/test/utils/resolveSpotlightOptions.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,4 +54,37 @@ describe('resolveSpotlightOptions', () => {
5454
const envUrl = 'http://localhost:8969/stream';
5555
expect(resolveSpotlightOptions(undefined, envUrl)).toBe(envUrl);
5656
});
57+
58+
describe('empty string handling', () => {
59+
it('returns undefined when options.spotlight is an empty string', () => {
60+
expect(resolveSpotlightOptions('', undefined)).toBeUndefined();
61+
expect(resolveSpotlightOptions('', true)).toBeUndefined();
62+
expect(resolveSpotlightOptions('', 'http://env:8969')).toBeUndefined();
63+
});
64+
65+
it('returns undefined when options.spotlight is whitespace only', () => {
66+
expect(resolveSpotlightOptions(' ', undefined)).toBeUndefined();
67+
expect(resolveSpotlightOptions('\t\n', true)).toBeUndefined();
68+
});
69+
70+
it('returns undefined when env is an empty string and options.spotlight is undefined', () => {
71+
expect(resolveSpotlightOptions(undefined, '')).toBeUndefined();
72+
});
73+
74+
it('returns undefined when env is whitespace only and options.spotlight is undefined', () => {
75+
expect(resolveSpotlightOptions(undefined, ' ')).toBeUndefined();
76+
expect(resolveSpotlightOptions(undefined, '\t\n')).toBeUndefined();
77+
});
78+
79+
it('returns true when options.spotlight is true and env is empty string', () => {
80+
expect(resolveSpotlightOptions(true, '')).toBe(true);
81+
expect(resolveSpotlightOptions(true, ' ')).toBe(true);
82+
});
83+
84+
it('returns valid URL when options.spotlight is valid URL even if env is empty', () => {
85+
const validUrl = 'http://localhost:8969/stream';
86+
expect(resolveSpotlightOptions(validUrl, '')).toBe(validUrl);
87+
expect(resolveSpotlightOptions(validUrl, ' ')).toBe(validUrl);
88+
});
89+
});
5790
});

‎packages/node-core/src/sdk/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,7 @@ function getClientOptions(
187187
// Parse spotlight configuration with proper precedence per spec
188188
const envBool = envToBool(process.env.SENTRY_SPOTLIGHT, { strict: true });
189189
const envSpotlight = envBool !== null ? envBool : process.env.SENTRY_SPOTLIGHT;
190+
// Note: resolveSpotlightOptions handles empty/whitespace string filtering
190191
const spotlight = resolveSpotlightOptions(options.spotlight, envSpotlight);
191192

192193
const tracesSampleRate = getTracesSampleRate(options.tracesSampleRate);

‎packages/node-core/test/sdk/init.test.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,24 @@ describe('init()', () => {
323323
expect(client?.getOptions().spotlight).toBe(true);
324324
expect(client?.getOptions().integrations.some(integration => integration.name === 'Spotlight')).toBe(true);
325325
});
326+
327+
it('treats empty string env var as undefined (no spotlight)', () => {
328+
process.env.SENTRY_SPOTLIGHT = '';
329+
330+
const client = init({ dsn: PUBLIC_DSN });
331+
332+
expect(client?.getOptions().spotlight).toBeUndefined();
333+
expect(client?.getOptions().integrations.some(integration => integration.name === 'Spotlight')).toBe(false);
334+
});
335+
336+
it('treats whitespace-only string env var as undefined (no spotlight)', () => {
337+
process.env.SENTRY_SPOTLIGHT = ' ';
338+
339+
const client = init({ dsn: PUBLIC_DSN });
340+
341+
expect(client?.getOptions().spotlight).toBeUndefined();
342+
expect(client?.getOptions().integrations.some(integration => integration.name === 'Spotlight')).toBe(false);
343+
});
326344
});
327345
});
328346
});

0 commit comments

Comments
 (0)