Skip to content

fix: correct typeof comparisons in browser SDK #1301

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.

Sign up for GitHub

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

Merged
joker23 merged 3 commits into from
Apr 21, 2026
Merged

fix: correct typeof comparisons in browser SDK #1301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28c8587
fix: correct typeof comparisons in browser SDK
joker23 Apr 21, 2026
0869f9d
test: add unit tests for typeof fix
joker23 Apr 21, 2026
074d3f3
test: add negative tests for isDocument and isWindow
joker23 Apr 21, 2026
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
2 changes: 1 addition & 1 deletion .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ module.exports = {
'eqeqeq': ['error', 'always', { null: 'ignore' }],
'prefer-const': ['error', { ignoreReadBeforeAssign: true }],
'no-var': 'error',
'valid-typeof': 'off',
'valid-typeof': 'error',
'no-restricted-syntax': [
'error',
{ selector: 'ForInStatement', message: 'Use Object.{keys,values,entries} instead.' },
Expand Down
29 changes: 29 additions & 0 deletions packages/sdk/browser/__tests__/BrowserApi.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { isDocument, isWindow } from '../src/BrowserApi';

it('isDocument returns true when document is defined', () => {
expect(isDocument()).toBe(true);
});

it('isDocument returns false when document is not defined', () => {
const original = Object.getOwnPropertyDescriptor(globalThis, 'document');
Object.defineProperty(globalThis, 'document', { value: undefined, configurable: true });
try {
expect(isDocument()).toBe(false);
} finally {
Object.defineProperty(globalThis, 'document', original!);
}
});

it('isWindow returns true when window is defined', () => {
expect(isWindow()).toBe(true);
});

it('isWindow returns false when window is not defined', () => {
const original = Object.getOwnPropertyDescriptor(globalThis, 'window');
Object.defineProperty(globalThis, 'window', { value: undefined, configurable: true });
try {
expect(isWindow()).toBe(false);
} finally {
Object.defineProperty(globalThis, 'window', original!);
}
});
18 changes: 17 additions & 1 deletion packages/sdk/browser/__tests__/platform/randomUuidV4.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { fallbackUuidV4, formatDataAsUuidV4 } from '../../src/platform/randomUuidV4';
import randomUuidV4, { fallbackUuidV4, formatDataAsUuidV4 } from '../../src/platform/randomUuidV4';

it('formats conformant UUID', () => {
// For this test we remove the random component and just inspect the variant and version.
Expand Down Expand Up @@ -26,3 +26,19 @@ it('formats conformant UUID', () => {
expect(specifierB).toEqual(0x8);
expect(specifierC).toEqual(0x8);
});

it('falls back when crypto.randomUUID is unavailable', () => {
const original = globalThis.crypto;
Object.defineProperty(globalThis, 'crypto', {
value: { getRandomValues: original.getRandomValues.bind(original) },
configurable: true,
});

try {
const uuid = randomUuidV4();
expect(uuid).toHaveLength(36);
expect(uuid[14]).toEqual('4');
} finally {
Object.defineProperty(globalThis, 'crypto', { value: original, configurable: true });
}
});
6 changes: 3 additions & 3 deletions packages/sdk/browser/src/BrowserApi.ts
View file
Open in desktop

@devin-ai-integration devin-ai-integration Bot Apr 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 addWindowEventListener guards on isDocument() instead of isWindow(), now exposed by the typeof fix

The addWindowEventListener function at packages/sdk/browser/src/BrowserApi.ts:53 checks isDocument() instead of isWindow() before accessing window. Before this PR, this didn't matter because both isDocument() and isWindow() always returned true (due to the broken typeof x !== undefined comparison). Now that the typeof checks are fixed, these functions correctly distinguish between document and window availability. In an environment where window is available but document is not, addWindowEventListener would incorrectly return a no-op instead of registering the event. This function is used by BrowserStateDetector.ts:21 (for pagehide events) and LocationWatcher.ts:46 (for popstate events).

(Refers to line 53)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@joker23 joker23 Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will address separately (this is an existing bug)

Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
*/

export function isDocument() {
return typeof document !== undefined;
return typeof document !== 'undefined';
}

export function isWindow() {
return typeof window !== undefined;
return typeof window !== 'undefined';
}

/**
Expand Down Expand Up @@ -91,7 +91,7 @@ export function getLocationHash(): string {
}

export function getCrypto(): Crypto {
if (typeof crypto !== undefined) {
if (typeof crypto !== 'undefined') {
return crypto;
}
// This would indicate running in an environment that doesn't have window.crypto or self.crypto.
Expand Down
2 changes: 1 addition & 1 deletion packages/sdk/browser/src/platform/randomUuidV4.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export function fallbackUuidV4(): string {
}

export default function randomUuidV4(): string {
if (typeof crypto !== undefined && typeof crypto.randomUUID === 'function') {
if (typeof crypto !== 'undefined' && typeof crypto.randomUUID === 'function') {

@devin-ai-integration devin-ai-integration Bot Apr 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 getRandom128bit uses bare crypto reference instead of typeof check, making Math.random fallback unreachable

In both packages/sdk/browser/src/platform/randomUuidV4.ts:39 and packages/telemetry/browser-telemetry/src/randomUuidV4.ts:42, getRandom128bit() checks if (crypto && crypto.getRandomValues) using a bare reference to crypto. If crypto is completely undefined (not just lacking getRandomValues), this throws a ReferenceError instead of falling through to the Math.random fallback. Before this PR, this code path was unreachable because the broken typeof crypto !== undefined in randomUuidV4() always evaluated to true, causing a crash earlier. Now that the typeof check in randomUuidV4() is fixed, the function correctly falls through to fallbackUuidV4() → getRandom128bit() when crypto is missing, but the bare crypto reference there will throw, making the Math.random fallback still unreachable in environments without crypto.

Prompt for agents 
The fix in randomUuidV4() now correctly handles the case where crypto is completely undefined by falling through to fallbackUuidV4(). However, getRandom128bit() (line 39 in packages/sdk/browser/src/platform/randomUuidV4.ts and line 42 in packages/telemetry/browser-telemetry/src/randomUuidV4.ts) uses a bare reference to crypto (if (crypto && crypto.getRandomValues)) which throws a ReferenceError if crypto is not defined at all. The Math.random fallback on lines 44-49 is unreachable in this case. The fix should change the crypto check in getRandom128bit() to use typeof crypto !== 'undefined' && crypto.getRandomValues in both files. This ensures the Math.random fallback is actually reachable when crypto is unavailable.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@joker23 joker23 Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

existing issue will address in another PR

return crypto.randomUUID();
}

Expand Down
2 changes: 1 addition & 1 deletion packages/telemetry/browser-telemetry/src/randomUuidV4.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export function fallbackUuidV4(): string {
}

export default function randomUuidV4(): string {
if (typeof crypto !== undefined && typeof crypto.randomUUID === 'function') {
if (typeof crypto !== 'undefined' && typeof crypto.randomUUID === 'function') {
return crypto.randomUUID();
}

Expand Down
Loading