chore: temp reverts #1924
chore: temp reverts #1924
Conversation
📝 WalkthroughWalkthroughThis pull request introduces significant updates to JSON utility functions and their associated test suites across the codebase. Key changes include the replacement of the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1924 +/- ##
==========================================
- Coverage 58.27% 58.22% -0.05%
==========================================
Files 485 485
Lines 16602 16593 -9
Branches 3335 3324 -11
==========================================
- Hits 9674 9661 -13
- Misses 5713 5734 +21
+ Partials 1215 1198 -17 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (28)
packages/analytics-js/src/app/RudderAnalytics.ts (4)
291-291: Avoid logging warnings inside performance-critical codeLogging warnings, such as
PAGE_UNLOAD_ON_BEACON_DISABLED_WARNING, during critical operations can impact performance and clutter logs.If the warning is essential, consider documenting it or handling it in a way that minimizes performance impact.
115-119: Implement error handling insetDefaultInstanceKeyif logic expandsThe comment suggests adding a try-catch block if the method's implementation grows. Currently, the method is simple, but if future changes are made, ensure that proper error handling is included.
Plan ahead by implementing error handling now to prevent potential issues later.
Line range hint
237-259: EnsuretrackPageLoadedEventhandles events array correctlyThe method checks if
events.length === 0or if it includesPageLifecycleEvents.LOADED, but ifeventsisundefined, this could cause an error.Consider adding a default value or null check for
events:trackPageLoadedEvent( events: PageLifecycleEvents[] = [], options: ApiOptions, preloadedEventsArray: PreloadedEventCall[], ) { if (events.length === 0 || events.includes(PageLifecycleEvents.LOADED)) { // existing code } }
Line range hint
263-291: Clarify the conditionals insetupPageUnloadTrackingThe nested conditionals could be simplified for better readability and maintainability.
Consider refactoring the conditionals to make the logic clearer.
packages/analytics-js/src/components/core/Analytics.ts (2)
124-125: Unnecessary Cloning of Primitive ValuesThe
clonefunction is used onwriteKeyanddataPlaneUrl, which are strings. Cloning primitive types is redundant since they are immutable in JavaScript/TypeScript.Recommend removing the
clonecalls for these variables to simplify the code.Apply this diff to remove unnecessary cloning:
batch(() => { - state.lifecycle.writeKey.value = clone(writeKey); - state.lifecycle.dataPlaneUrl.value = clone(dataPlaneUrl); + state.lifecycle.writeKey.value = writeKey; + state.lifecycle.dataPlaneUrl.value = dataPlaneUrl; state.loadOptions.value = normalizeLoadOptions(state.loadOptions.value, loadOptions); });
62-67: Standardize Error Logging for Better TraceabilityThe imported error messages from
'../../constants/logMessages'are used for logging errors. Ensure that all error logs include context such as error codes or identifiers to facilitate easier debugging and monitoring.Consider enhancing the error messages or logging format to include additional diagnostic information.
packages/analytics-js-common/__tests__/utilities/errors.test.ts (1)
5-16: Consider adding more test cases for error scenarios.While the happy path is covered, the test suite could benefit from additional test cases:
- Error scenarios (e.g., when error is null/undefined)
- Different error types (e.g., TypeError, SyntaxError)
- Errors with custom properties
Also, consider using
beforeEach/afterEachhooks for cleaner setup/teardown of the mock.Here's a suggested enhancement:
describe('dispatchErrorEvent', () => { + let originalDispatchEvent; + let mockDispatchEvent; + + beforeEach(() => { + mockDispatchEvent = jest.fn(); + originalDispatchEvent = globalThis.dispatchEvent; + globalThis.dispatchEvent = mockDispatchEvent; + }); + + afterEach(() => { + globalThis.dispatchEvent = originalDispatchEvent; + }); + it('should dispatch an error event', () => { - const dispatchEvent = jest.fn(); - const originalDispatchEvent = globalThis.dispatchEvent; - - globalThis.dispatchEvent = dispatchEvent; const error = new Error('Test error'); dispatchErrorEvent(error); - expect(dispatchEvent).toHaveBeenCalledWith(new ErrorEvent('error', { error })); - - // Cleanup - globalThis.dispatchEvent = originalDispatchEvent; + expect(mockDispatchEvent).toHaveBeenCalledWith(new ErrorEvent('error', { error })); }); + + it('should handle null/undefined errors', () => { + dispatchErrorEvent(null); + expect(mockDispatchEvent).not.toHaveBeenCalled(); + }); + + it('should handle different error types', () => { + const typeError = new TypeError('Type error'); + dispatchErrorEvent(typeError); + expect(mockDispatchEvent).toHaveBeenCalledWith(new ErrorEvent('error', { error: typeError })); + }); });packages/analytics-js/src/components/core/utilities.ts (2)
15-15: Consider adding more comprehensive write key validationWhile the basic validation is good, consider adding:
- Maximum length check to prevent DoS
- Character set validation to ensure only valid characters are used
-const isWriteKeyValid = (writeKey: string) => isString(writeKey) && writeKey.trim().length > 0; +const isWriteKeyValid = (writeKey: string) => { + const trimmed = writeKey.trim(); + const MAX_LENGTH = 64; // adjust as per your requirements + const VALID_PATTERN = /^[a-zA-Z0-9_-]+$/; + return ( + isString(writeKey) && + trimmed.length > 0 && + trimmed.length <= MAX_LENGTH && + VALID_PATTERN.test(trimmed) + ); +};
17-17: Consider adding data plane specific URL validationThe current implementation only validates if it's a valid URL. Consider adding specific validation for data plane URL format and allowed protocols.
-const isDataPlaneUrlValid = (dataPlaneUrl: string) => isValidURL(dataPlaneUrl); +const isDataPlaneUrlValid = (dataPlaneUrl: string) => { + if (!isValidURL(dataPlaneUrl)) { + return false; + } + try { + const url = new URL(dataPlaneUrl); + // Ensure secure protocol + if (!['https:', 'http:'].includes(url.protocol)) { + return false; + } + // Add any other data plane specific validations + return true; + } catch { + return false; + } +};packages/analytics-js/src/components/configManager/util/validate.ts (1)
Line range hint
17-52: Add error handling and improve domain parsing robustnessThe domain-related utility functions could benefit from additional error handling and edge case coverage:
- URL parsing could throw errors with invalid inputs
- The domain parsing logic might not handle all TLD patterns correctly
- Parameters should be validated
Consider applying these improvements:
const getTopDomain = (url: string) => { + if (!url) { + throw new Error('URL is required'); + } + + try { // Create a URL object const urlObj = new URL(url); // Extract the host and protocol const { host, protocol } = urlObj; // Split the host into parts const parts: string[] = host.split('.'); let topDomain; - // Handle different cases, especially for co.uk or similar TLDs - if (parts.length > 2) { - // Join the last two parts for the top-level domain - topDomain = `${parts[parts.length - 2]}.${parts[parts.length - 1]}`; - } else { - // If only two parts or less, return as it is - topDomain = host; + + // Handle known multi-part TLDs + const multiPartTlds = ['co.uk', 'com.au', 'co.jp']; + const joined = parts.slice(-2).join('.'); + + if (parts.length > 2 && !multiPartTlds.includes(joined)) { + topDomain = joined; + } else if (parts.length > 3 && multiPartTlds.includes(joined)) { + // For cases like example.co.uk + topDomain = `${parts[parts.length - 3]}.${joined}`; + } else { + topDomain = host; } return { topDomain, protocol }; + } catch (error) { + throw new Error(`Invalid URL provided: ${error.message}`); + } };packages/analytics-js-plugins/src/beaconQueue/utilities.ts (1)
Line range hint
34-41: Consider adding payload size validationThe function creates a Blob from stringified event data but doesn't validate the payload size. Large batches of events could potentially exceed browser limitations for Beacon API.
Consider adding size validation before Blob creation:
const MAX_BEACON_SIZE = 64 * 1024; // 64KB is a safe limit for most browsers const blobPayload = json.stringifyData(data); if (blobPayload && blobPayload.length > MAX_BEACON_SIZE) { logger?.warn(`Payload size ${blobPayload.length} exceeds recommended limit of ${MAX_BEACON_SIZE} bytes`); }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-34: packages/analytics-js-plugins/src/beaconQueue/utilities.ts#L34
Added line #L34 was not covered by testspackages/analytics-js/src/services/ErrorHandler/processError.ts (2)
Line range hint
39-39: Address TODO comment about device mode integrations.The comment indicates pending work to remove legacy error handling once all device mode integrations migrate to v3 script loader module.
Would you like me to:
- Create a tracking issue for this TODO?
- Help identify which device mode integrations still need migration?
Line range hint
52-54: Consider enhancing script loading error message.The error message could be more helpful for debugging by including additional context such as error type and loading stage.
Consider this enhancement:
- const errorMessage = `Error in loading a third-party script from URL ${eventTarget?.src} with ID ${eventTarget?.id}.`; + const errorMessage = `Error in loading a third-party script from URL ${eventTarget?.src} with ID ${eventTarget?.id}. Type: ${error.type}, Stage: ${eventTarget?.readyState}`;packages/analytics-js-plugins/src/utilities/eventsDelivery.ts (1)
37-46: Consider handling potential stringification failuresWhile the size validation logic is sound, consider handling cases where payload stringification might fail.
Consider this safer implementation:
const validateEventPayloadSize = (event: RudderEvent, logger?: ILogger) => { - const payloadStr = getDeliveryPayload(event); + let payloadStr: string; + try { + payloadStr = getDeliveryPayload(event); + } catch (error) { + logger?.error('Failed to stringify event payload for size validation'); + return; + } const payloadSize = payloadStr.length; if (payloadSize > EVENT_PAYLOAD_SIZE_BYTES_LIMIT) { logger?.warn(packages/analytics-js-cookies/src/cookiesUtilities.ts (2)
16-16: Document the purpose of the boolean parameter in stringifyDataThe implementation looks robust with proper error handling and null checks. However, the purpose of the
falseparameter instringifyData(value, false)should be documented for better maintainability.Consider adding a comment explaining what the boolean parameter controls:
- const strValue = stringifyData(value, false); + // false: don't include null values in the stringified output + const strValue = stringifyData(value, false);
Line range hint
1-99: Well-structured encryption utilities with clear separation of concernsThe file maintains a clean architecture with:
- Clear separation between browser and Node.js encryption methods
- Consistent error handling patterns
- Type-safe implementations
- Proper encapsulation of internal functions
Consider adding JSDoc comments for the exported functions to document their usage patterns and encryption mechanisms for better maintainability.
packages/analytics-js-plugins/src/xhrQueue/utilities.ts (1)
Line range hint
64-84: LGTM! Consider destructuring optimization.The function changes look good, maintaining the core logic while removing the logger dependency. The code handles both batch and single event scenarios appropriately.
Consider using object destructuring for cleaner code in the single event case:
- const { url: eventUrl, event, headers: eventHeaders } = itemData; - const finalEvent = eventsDelivery.getFinalEventForDeliveryMutator(event, currentTime); - data = eventsDelivery.getDeliveryPayload(finalEvent); - headers = clone(eventHeaders); - url = eventUrl; + const { url: eventUrl, event, headers: eventHeaders } = itemData; + ({ + data, + headers, + url + } = { + data: eventsDelivery.getDeliveryPayload( + eventsDelivery.getFinalEventForDeliveryMutator(event, currentTime) + ), + headers: clone(eventHeaders), + url: eventUrl, + });packages/analytics-js/src/services/HttpClient/xhr/xhrRequestHandler.ts (1)
Line range hint
65-74: LGTM with a minor suggestion for error handlingThe payload stringification and error handling look good. However, consider enhancing the error message to include more context about why the stringification failed.
Consider this improvement:
payload = stringifyData(options.data); if (isNull(payload)) { reject({ - error: new Error(XHR_PAYLOAD_PREP_ERROR), + error: new Error(`${XHR_PAYLOAD_PREP_ERROR}. Failed to stringify data: ${typeof options.data}`), undefined, options, });packages/analytics-js-common/src/types/IRudderAnalytics.ts (1)
191-191: Breaking change: Document the semantic difference between null and undefinedThe return type change from
Nullable<number>toNullable<number> | undefinedallows for more precise error states. However, the semantic difference between null and undefined should be documented.Consider adding JSDoc comments to clarify:
/** * To fetch the current sessionId + * @returns + * - number: Valid session ID + * - null: Session exists but has no ID + * - undefined: No session exists */ getSessionId(): Nullable<number> | undefined;packages/analytics-js-plugins/src/errorReporting/utils.ts (2)
191-191: LGTM: Type-safe error payload serializationThe change maintains type safety through explicit typing with
MetricServicePayloadwhile standardizing the serialization approach.Consider removing the type assertion by ensuring
stringifyDatareturn type matches the expected string type:- return stringifyData<MetricServicePayload>(data) as string; + return stringifyData<MetricServicePayload>(data);
19-19: Consider standardizing error serialization across the codebaseThe changes standardize JSON serialization by using the common
stringifyDatautility. To maintain consistency:
- Ensure all error reporting components use the same serialization approach
- Document the expected behavior of circular reference handling
- Consider adding integration tests for the complete error reporting flow
Also applies to: 70-71, 191-191
packages/analytics-js/src/services/StoreManager/Store.ts (1)
208-208: Consider additional validation for encrypted data format.The modified return statement handles undefined/null values more explicitly, but consider adding validation for the encrypted data format to prevent potential issues with existing stored data.
- return typeof formattedValue === 'undefined' ? value : (formattedValue ?? ''); + if (typeof formattedValue === 'undefined') { + return value; + } + + // Validate encrypted data format if needed + if (this.isEncrypted && formattedValue !== null) { + // Add validation logic here + } + + return formattedValue ?? '';packages/analytics-js/src/components/configManager/ConfigManager.ts (1)
Line range hint
67-116: Consider adding defensive checks in init methodThe implementation properly handles state management, URL sanitization, and configuration setup. However, with the removal of
validateLoadArgs, consider adding basic defensive checks:init() { this.attachEffects(); + // Basic validation until formal validation is restored + if (!state.lifecycle.writeKey.value || !state.lifecycle.dataPlaneUrl.value) { + this.onError(new Error('Missing required configuration: writeKey or dataPlaneUrl')); + return; + } const { logLevel, configUrl,packages/analytics-js-common/__tests__/utilities/json.test.ts (2)
7-75: Consider adding more edge cases to thestringifyDatatest suite.While the current test cases cover the basic functionality well, consider adding tests for:
- Empty objects and arrays
- Objects with special characters in keys/values
- Deeply nested objects to verify stack overflow prevention
- Large objects to verify performance characteristics
151-179: Extract duplicate warning message to a constant.The warning message is repeated multiple times in the test assertions. Consider extracting it to a constant to improve maintainability.
+ const BAD_DATA_WARNING_MESSAGE = 'JSON:: A bad data (like circular reference, BigInt) has been detected in the object and the property "%s" has been dropped from the output.'; expect(mockLogger.warn).toHaveBeenNthCalledWith( 1, - 'JSON:: A bad data (like circular reference, BigInt) has been detected in the object and the property "a" has been dropped from the output.', + BAD_DATA_WARNING_MESSAGE.replace('%s', 'a'), );packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (1)
60-70: Consider consolidating sanitization callsThe sanitization implementation looks correct, but we could potentially optimize it by consolidating the sanitization calls into a helper function to reduce code duplication.
- const sanitizedCategory = getSanitizedValue(category); - const sanitizedName = getSanitizedValue(name); - const sanitizedProperties = getSanitizedValue(properties); - const sanitizedOptions = getSanitizedValue(options); - const sanitizedCallback = getSanitizedValue(callback); + const [ + sanitizedCategory, + sanitizedName, + sanitizedProperties, + sanitizedOptions, + sanitizedCallback, + ] = [category, name, properties, options, callback].map(getSanitizedValue);packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (1)
331-348: Add input validation for cookie optionsWhile the serialization change is correct, consider adding validation for the cookie options before serialization to prevent potential security issues.
makeRequestToSetCookie( encryptedCookieData: EncryptedCookieData[], callback: AsyncRequestCallback<any>, ) { + // Validate cookie options + const cookieOptions = { + maxAge: state.storage.cookie.value?.maxage, + path: state.storage.cookie.value?.path, + domain: state.storage.cookie.value?.domain, + sameSite: state.storage.cookie.value?.samesite, + secure: state.storage.cookie.value?.secure, + expires: state.storage.cookie.value?.expires, + }; + + if (!cookieOptions.secure && cookieOptions.sameSite === 'none') { + this.logger?.warn('Cookies with SameSite=None must also specify Secure attribute'); + } + this.httpClient?.getAsyncData({ url: state.serverCookies.dataServiceUrl.value as string, options: { method: 'POST', data: stringifyData( { reqType: 'setCookies', workspaceId: state.source.value?.workspaceId, data: { - options: { - maxAge: state.storage.cookie.value?.maxage, - path: state.storage.cookie.value?.path, - domain: state.storage.cookie.value?.domain, - sameSite: state.storage.cookie.value?.samesite, - secure: state.storage.cookie.value?.secure, - expires: state.storage.cookie.value?.expires, - }, + options: cookieOptions, cookies: encryptedCookieData, }, }, false, ),packages/analytics-js/__tests__/components/core/Analytics.test.ts (1)
132-201: Consider adding positive test cases for valid inputs.While the error cases are well covered, it would be beneficial to add test cases that verify the behavior with valid inputs:
- Valid write key with special characters
- Valid data plane URL with different formats (HTTP/HTTPS, with/without path)
+ it('should load successfully with valid write key and data plane URL', () => { + const startLifecycleSpy = jest.spyOn(analytics, 'startLifecycle'); + const errorSpy = jest.spyOn(analytics.logger, 'error'); + + // Test with valid write key containing special characters + analytics.load('valid-key_123!@#', sampleDataPlaneUrl, { logLevel: 'ERROR' }); + expect(state.lifecycle.status.value).toBe('browserCapabilitiesReady'); + expect(startLifecycleSpy).toHaveBeenCalledTimes(1); + expect(errorSpy).not.toHaveBeenCalled(); + + // Test with different valid URL formats + analytics.load(dummyWriteKey, 'http://example.com/path', { logLevel: 'ERROR' }); + analytics.load(dummyWriteKey, 'https://sub.example.com', { logLevel: 'ERROR' }); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (5)
.eslintrc.jsonis excluded by!**/*.json.github/workflows/deploy-npm.ymlis excluded by!**/*.yml.github/workflows/deploy-sanity-suite.ymlis excluded by!**/*.yml.github/workflows/deploy.ymlis excluded by!**/*.yml.github/workflows/publish-new-release.ymlis excluded by!**/*.yml
📒 Files selected for processing (46)
CODEOWNERS(1 hunks)packages/analytics-js-common/__tests__/utilities/errors.test.ts(1 hunks)packages/analytics-js-common/__tests__/utilities/json.test.ts(1 hunks)packages/analytics-js-common/src/constants/integrations/integration_cname.js(1 hunks)packages/analytics-js-common/src/constants/logMessages.ts(1 hunks)packages/analytics-js-common/src/constants/loggerContexts.ts(2 hunks)packages/analytics-js-common/src/types/ApiObject.ts(1 hunks)packages/analytics-js-common/src/types/IRudderAnalytics.ts(2 hunks)packages/analytics-js-common/src/utilities/checks.ts(2 hunks)packages/analytics-js-common/src/utilities/errors.ts(2 hunks)packages/analytics-js-common/src/utilities/eventMethodOverloads.ts(6 hunks)packages/analytics-js-common/src/utilities/json.ts(1 hunks)packages/analytics-js-common/src/utilities/object.ts(2 hunks)packages/analytics-js-cookies/__tests__/cookieUtilities.test.ts(0 hunks)packages/analytics-js-cookies/src/cookiesUtilities.ts(2 hunks)packages/analytics-js-plugins/__tests__/bugsnag/utils.test.ts(0 hunks)packages/analytics-js-plugins/__tests__/errorReporting/utils.test.ts(0 hunks)packages/analytics-js-plugins/__tests__/utilities/queue.test.ts(0 hunks)packages/analytics-js-plugins/__tests__/xhrQueue/utilities.test.ts(0 hunks)packages/analytics-js-plugins/src/beaconQueue/utilities.ts(1 hunks)packages/analytics-js-plugins/src/bugsnag/constants.ts(1 hunks)packages/analytics-js-plugins/src/bugsnag/utils.ts(1 hunks)packages/analytics-js-plugins/src/errorReporting/constants.ts(1 hunks)packages/analytics-js-plugins/src/errorReporting/event/event.ts(2 hunks)packages/analytics-js-plugins/src/errorReporting/utils.ts(3 hunks)packages/analytics-js-plugins/src/utilities/eventsDelivery.ts(2 hunks)packages/analytics-js-plugins/src/xhrQueue/index.ts(2 hunks)packages/analytics-js-plugins/src/xhrQueue/utilities.ts(3 hunks)packages/analytics-js/__tests__/app/RudderAnalytics.test.ts(4 hunks)packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts(0 hunks)packages/analytics-js/__tests__/components/configManager/validate.test.ts(0 hunks)packages/analytics-js/__tests__/components/core/Analytics.test.ts(1 hunks)packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts(4 hunks)packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts(0 hunks)packages/analytics-js/src/app/RudderAnalytics.ts(13 hunks)packages/analytics-js/src/components/configManager/ConfigManager.ts(1 hunks)packages/analytics-js/src/components/configManager/util/validate.ts(1 hunks)packages/analytics-js/src/components/core/Analytics.ts(2 hunks)packages/analytics-js/src/components/core/utilities.ts(2 hunks)packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts(6 hunks)packages/analytics-js/src/constants/logMessages.ts(1 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(2 hunks)packages/analytics-js/src/services/ErrorHandler/processError.ts(2 hunks)packages/analytics-js/src/services/HttpClient/HttpClient.ts(1 hunks)packages/analytics-js/src/services/HttpClient/xhr/xhrRequestHandler.ts(2 hunks)packages/analytics-js/src/services/StoreManager/Store.ts(3 hunks)
💤 Files with no reviewable changes (8)
- packages/analytics-js-cookies/tests/cookieUtilities.test.ts
- packages/analytics-js-plugins/tests/bugsnag/utils.test.ts
- packages/analytics-js-plugins/tests/errorReporting/utils.test.ts
- packages/analytics-js-plugins/tests/utilities/queue.test.ts
- packages/analytics-js-plugins/tests/xhrQueue/utilities.test.ts
- packages/analytics-js/tests/components/configManager/ConfigManager.test.ts
- packages/analytics-js/tests/components/configManager/validate.test.ts
- packages/analytics-js/tests/services/HttpClient/HttpClient.test.ts
✅ Files skipped from review due to trivial changes (3)
- CODEOWNERS
- packages/analytics-js-common/src/constants/integrations/integration_cname.js
- packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts
🧰 Additional context used
📓 Learnings (8)
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-common/src/utilities/sanitize.ts:0-0
Timestamp: 2024-11-12T15:14:23.319Z
Learning: The previous suggestions on the `getSanitizedValue` function in `packages/analytics-js-common/src/utilities/sanitize.ts` are no longer valid.
packages/analytics-js-common/src/utilities/json.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-common/src/utilities/json.ts:9-27
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In `packages/analytics-js-common/src/utilities/json.ts`, the `stringifyData` function assumes that data is sanitized beforehand, so additional error handling within this function is not necessary.
packages/analytics-js-common/src/utilities/object.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-common/src/utilities/object.ts:12-13
Timestamp: 2024-11-12T15:14:33.334Z
Learning: In `packages/analytics-js-common/src/utilities/object.ts`, the `isObject` function is intended to check if a value is of type 'object', including when the value is `null`. Null checking is handled separately in functions like `isObjectAndNotNull`.
packages/analytics-js-plugins/src/utilities/eventsDelivery.ts (2)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the issue regarding inconsistent error handling approaches in `getDMTDeliveryPayload` is no longer valid.
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:25-26
Timestamp: 2024-11-12T15:14:33.334Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the `getDeliveryPayload` function does not require additional error handling for stringification failures.
packages/analytics-js-plugins/src/xhrQueue/index.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-plugins/src/xhrQueue/index.ts:34-34
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The deprecated plugins have been removed from the `PluginName` type, so the explicit `PluginName` type annotation is no longer necessary in `packages/analytics-js-plugins/src/xhrQueue/index.ts`.
packages/analytics-js/src/components/configManager/util/validate.ts (2)
Learnt from: MoumitaM
PR: rudderlabs/rudder-sdk-js#1777
File: packages/analytics-js/src/components/configManager/util/validate.ts:0-0
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The function `isTopLevelDomain` in `validate.ts` was renamed to `isWebpageTopLevelDomain` for clarity.
Learnt from: MoumitaM
PR: rudderlabs/rudder-sdk-js#1777
File: packages/analytics-js/src/components/configManager/util/validate.ts:0-0
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The function `isTopLevelDomain` in `validate.ts` was renamed to `isWebpageTopLevelDomain` for clarity.
packages/analytics-js/src/constants/logMessages.ts (2)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1730
File: packages/analytics-js/src/constants/logMessages.ts:0-0
Timestamp: 2024-11-12T15:14:23.319Z
Learning: The `DATA_PLANE_URL_VALIDATION_ERROR` function in `packages/analytics-js/src/constants/logMessages.ts` should use `string | undefined` instead of `any` for the `dataPlaneUrl` parameter to enhance type safety.
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1730
File: packages/analytics-js/src/constants/logMessages.ts:0-0
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The `DATA_PLANE_URL_VALIDATION_ERROR` function in `packages/analytics-js/src/constants/logMessages.ts` should use `string | undefined` instead of `any` for the `dataPlaneUrl` parameter to enhance type safety.
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1907
File: packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts:172-174
Timestamp: 2024-11-12T15:14:23.319Z
Learning: The function `onError` in `packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts` is acceptable as currently implemented, and refactoring suggestions are not required unless necessary.
🪛 GitHub Check: codecov/patch
packages/analytics-js-common/src/utilities/errors.ts
[warning] 15-15: packages/analytics-js-common/src/utilities/errors.ts#L15
Added line #L15 was not covered by tests
packages/analytics-js-plugins/src/beaconQueue/utilities.ts
[warning] 34-34: packages/analytics-js-plugins/src/beaconQueue/utilities.ts#L34
Added line #L34 was not covered by tests
🪛 Biome
packages/analytics-js/src/app/RudderAnalytics.ts
[error] 65-65: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor is ignored.
(lint/correctness/noConstructorReturn)
🔇 Additional comments (74)
packages/analytics-js-common/src/utilities/json.ts (1)
17-27: stringifyData function implementation is appropriate
The stringifyData function effectively excludes null values and specified keys during JSON stringification using a custom replacer function. The implementation aligns with the intended functionality.
packages/analytics-js/src/app/RudderAnalytics.ts (6)
372-378: Review the necessity of try-catch blocks in performance-critical methods
Wrapping methods like page in try-catch blocks may have performance implications, especially if these methods are called frequently.
[performance_issue]
Evaluate whether it's necessary to have try-catch blocks in these methods or if error handling can be managed differently to avoid potential performance degradation.
399-405: Check error handling consistency in track method
Similar to the page method, ensure that adding try-catch blocks in the track method does not adversely affect performance.
[performance_issue]
Consider alternative error handling strategies that are both efficient and maintainable.
148-179: Sanitize loadOptions before use
The load method now uses getSanitizedValue(loadOptions), which is a good practice to prevent potential errors from malformed input.
503-509: Handle potential undefined return value in getAnonymousId
The method may return undefined, so callers should ensure they handle this case to avoid runtime errors.
Check all usages of getAnonymousId to ensure they handle undefined values appropriately.
61-67:
Avoid returning a value from the constructor
Returning a value from the constructor is non-standard and may lead to unexpected behavior. Constructors should not return values as it can cause the created object to be undefined or an unexpected object.
Consider refactoring the code to use a factory method or ensure that the class follows the singleton pattern without returning from the constructor. Here's a possible refactor:
class RudderAnalytics implements IRudderAnalytics<IAnalytics> {
static globalSingleton: Nullable<RudderAnalytics> = null;
constructor() {
if (RudderAnalytics.globalSingleton) {
// Initialization has already been done
return;
}
// Proceed with initialization
// ...
RudderAnalytics.globalSingleton = this;
}
static getInstance(): RudderAnalytics {
if (!RudderAnalytics.globalSingleton) {
new RudderAnalytics();
}
return RudderAnalytics.globalSingleton as RudderAnalytics;
}
}⛔ Skipped due to learnings
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1907
File: packages/analytics-js/src/app/RudderAnalytics.ts:68-68
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In `packages/analytics-js/src/app/RudderAnalytics.ts`, the constructor of the `RudderAnalytics` class intentionally returns `RudderAnalytics.globalSingleton` to implement a singleton pattern. In JavaScript, returning an object from a constructor causes the `new` operator to return that object instead of a new instance. Therefore, when reviewing, avoid flagging returns from constructors as issues if they are intended for singleton implementations.
🧰 Tools
🪛 Biome
[error] 65-65: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor is ignored.
(lint/correctness/noConstructorReturn)
592-597: Ensure consent options are correctly sanitized
In the consent method, getSanitizedValue is used on options. Verify that this sanitation aligns with expected consent option formats.
Run the following script to verify the sanitization:
✅ Verification successful
Let me gather more information about the getSanitizedValue function and consent options validation.
Let me check the implementation of getSanitizedValue specifically.
Based on the gathered information, I can now provide the final response:
Consent options are properly sanitized and validated
The implementation shows a robust two-level sanitization and validation process:
getSanitizedValuehandles data sanitization by safely handling circular references, BigInt values, and maintaining data typesgetValidPostConsentOptionsvalidates and normalizes the consent options by:- Setting default values for core consent flags
- Safely cloning and validating storage options
- Properly handling integration configurations
- Validating boolean flags for consent behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of `consent` method with various options.
# Search for all calls to `consent` and review the options passed
rg 'consent\((.*?)\)'
Length of output: 2767
Script:
#!/bin/bash
# Search for getSanitizedValue implementation and usage
ast-grep --pattern 'function getSanitizedValue'
# Search for ConsentOptions type definition
rg -A 10 'type ConsentOptions'
# Search for consent options validation
rg 'getValidPostConsentOptions|getConsentManagementData'
Length of output: 8620
Script:
#!/bin/bash
# Search for getSanitizedValue implementation
rg -A 10 'getSanitizedValue'
# Search for consent validation utilities
rg -A 15 'const getValidPostConsentOptions'
Length of output: 27476
packages/analytics-js/src/components/core/Analytics.ts (3)
70-70: Check for Potential Circular Dependencies
Importing dispatchSDKEvent, isDataPlaneUrlValid, and isWriteKeyValid from './utilities' could introduce circular dependencies if utilities.ts imports from modules that also depend on Analytics.ts.
Ensure that there are no circular dependencies that could lead to runtime errors.
Run the following script to detect circular dependencies:
#!/bin/bash
# Description: Detect circular dependencies involving 'utilities.ts'.
# Install depcruise if not already installed (npm install -g dependency-cruiser).
# Analyze dependencies starting from 'Analytics.ts'.
depcruise --include-only "^src" --output-type err src/components/core/Analytics.ts107-107:
Potential Breaking Change: Updated load Method Signature
The load method now requires writeKey and dataPlaneUrl as mandatory string parameters. This change may break existing integrations that use the previous method signature with optional parameters.
Consider providing backward compatibility or overloading the method to handle the previous signature. Alternatively, ensure all consumers of this method are updated to reflect the new required parameters.
Run the following script to find all usages of the load method that may be affected:
112-115: 🛠️ Refactor suggestion
Ensure Robust Validation Logic for writeKey and dataPlaneUrl
The validation functions isWriteKeyValid and isDataPlaneUrlValid are critical for preventing invalid configurations. Verify that these functions comprehensively handle edge cases and provide clear error messages to aid in troubleshooting.
To review the implementation of the validation functions, run:
Also applies to: 117-119
packages/analytics-js-common/src/types/ApiObject.ts (1)
13-14: Verify impact of allowing undefined array elements
The type definition change is valid, but allowing undefined in arrays could affect data consistency during serialization/deserialization. Since this appears to be a temporary revert, consider:
- Documenting why undefined values need to be allowed in arrays
- Adding a TODO comment indicating this is temporary
- Creating a follow-up ticket to revert back to stricter typing
Let's verify the impact on data handling:
✅ Verification successful
Allowing undefined array elements is safe with current JSON handling
The codebase already handles undefined values safely:
- The JSON stringification process in
utilities/json.tsuses a custom replacer that properly handles edge cases - The string utility has try-catch protection around JSON operations
- The type change aligns with the actual runtime behavior of the JSON serialization
This type definition change is not introducing any new risks as the codebase is already equipped to handle undefined array elements properly through its sanitization and error handling mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing array handling in the codebase
# Description: Look for array processing logic that might be affected by undefined values
# Search for array processing in utility functions
rg -l "JSON\.(parse|stringify)" packages/analytics-js-common/src/
# Look for validation logic that might need to handle undefined array elements
ast-grep --pattern 'function $NAME($_) {
$$$
JSON.$METHOD($$$)
$$$
}'
Length of output: 371
Script:
#!/bin/bash
# Let's examine the JSON handling implementations to understand the impact
# Check the implementation of JSON handling utilities
rg -A 10 "JSON\.(parse|stringify)" packages/analytics-js-common/src/utilities/json.ts packages/analytics-js-common/src/utilities/string.ts
# Look for array handling logic specifically
ast-grep --pattern 'function stringifyWithoutCircular($_) {
$$$
}'
# Check if there are any validation or sanitization of array elements
ast-grep --pattern 'function $NAME($_) {
$$$
Array.$METHOD($$$)
$$$
}'
Length of output: 3096
packages/analytics-js-common/__tests__/utilities/errors.test.ts (3)
1-1: LGTM! Clean import statement.
The import statement is correctly importing the function under test.
3-4: LGTM! Well-structured test suite.
The test suite follows Jest's best practices with clear, descriptive naming.
1-18: Verify integration with error handling system.
Let's verify how this test integrates with the broader error handling system.
✅ Verification successful
Let me check the implementation of the error handling utility to ensure proper integration.
Test correctly verifies error event dispatch functionality
The test properly validates the core functionality of dispatchErrorEvent utility. The implementation shows that:
- The utility adds a manual error identifier to the stack trace
- It uses the standard
ErrorEventinterface - It properly integrates with the global error handling system through
window.dispatchEvent
The test covers these aspects adequately by:
- Mocking the global dispatch function
- Verifying the correct event type and error payload
- Properly cleaning up after the test
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related error handling implementations and tests
# Find related error handling files
echo "Finding related error handling files..."
fd -e ts -e js "error" packages/analytics-js-common/src
# Check for other test files testing error scenarios
echo "\nChecking for related test files..."
fd -e test.ts -e test.js "error" packages/analytics-js-common/__tests__
# Look for error event dispatching patterns
echo "\nChecking for error event dispatch patterns..."
rg -l "dispatchErrorEvent|ErrorEvent" packages/analytics-js-common
Length of output: 1186
Script:
#!/bin/bash
# Check the implementation of dispatchErrorEvent and related error handling
echo "Checking error utility implementation..."
cat packages/analytics-js-common/src/utilities/errors.ts
echo "\nChecking error handler type definition..."
cat packages/analytics-js-common/src/types/ErrorHandler.ts
echo "\nChecking error constants..."
cat packages/analytics-js-common/src/constants/errors.ts
Length of output: 2956
packages/analytics-js/src/components/core/utilities.ts (2)
1-2: LGTM! Well-structured imports
The imports are properly scoped to specific utility functions from the common package, following good practices for module imports.
19-19: LGTM! Clear and explicit exports
The named exports make it clear what functionality is available from this module.
packages/analytics-js-common/src/constants/loggerContexts.ts (2)
14-14: Verify all usages of the replaced RS_APP constant
The constant RS_APP has been replaced with RSA. Let's ensure all references are updated.
✅ Verification successful
Based on the search results, I can now generate the final response:
No issues found with RS_APP to RSA migration
The codebase search shows:
- No remaining references to the old
RS_APPconstant - The new
RSAconstant is properly used in the analytics components - No instances of the old string literal 'RudderStackApplication'
The migration appears to be complete and consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of RS_APP
rg "RS_APP" --type ts
# Search for current usage of RSA to verify the migration
rg "RSA" --type ts
# Search for string literals that might need updating
rg "'RudderStackApplication'" --type ts
rg "\"RudderStackApplication\"" --type ts
Length of output: 969
14-14: Clarify if this is a temporary or permanent change
The PR is titled "chore: temp reverts" but this constant rename from RS_APP to RSA appears to be a permanent change. Could you please clarify if this is intended to be temporary? If so, we should document this with a TODO comment and create a tracking issue for the revert.
Also applies to: 31-31
✅ Verification successful
Based on the git history output, I can now provide a clear response about the nature of this change:
This appears to be part of a temporary revert cycle, not a permanent change
The git history shows this is part of a series of reverts and re-applications:
- The constant was originally introduced as
RS_APPin commit 4f25a03 - Changed to
RSAin commit 9fbaf81 as part of error handling changes - Reverted back to
RS_APPin commit 890fb7b ("fix: revert sanitization changes") - Changed again to
RSAin commit 4b5aa80 which re-applies the previously reverted changes
Since this is part of the "chore: temp reverts" PR and the history shows an active revert cycle, this change is indeed temporary in nature. No additional documentation or tracking issue is needed as this is part of a planned temporary revert-and-reapply cycle.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check commit history for context
git log -p --all -S "RS_APP" -- "packages/analytics-js-common/src/constants/loggerContexts.ts"
Length of output: 7504
packages/analytics-js-common/src/constants/logMessages.ts (2)
12-13: LGTM! Well-structured warning message.
The consolidated warning message is well-implemented:
- Clear indication of the issue (bad data types)
- Specific examples in parentheses (circular reference, BigInt)
- Consistent use of LOG_CONTEXT_SEPARATOR
- Proper TypeScript type annotations
12-21: Verify PR objective alignment.
The changes appear to be refactoring warning messages rather than implementing temporary reverts as mentioned in the PR title "chore: temp reverts". Please clarify if this is intentional or if these changes should be part of a different PR.
Let's check for any revert-related commits or changes:
✅ Verification successful
Changes are part of a temporary revert cycle and align with PR objective
The git history shows this is indeed part of a temporary revert cycle:
- Initial features were added in feat: sanitize input data #1902 (sanitize input data) and feat: add error handling to all public apis #1907 (error handling)
- These were temporarily reverted in fix: revert sanitization changes #1916
- Now being re-applied through revert commits (4a9bd24, 4b5aa80)
The changes to logMessages.ts are specifically part of this revert cycle, where BAD_DATA_WARNING is being restored after being temporarily reverted. This aligns with the PR's "temp reverts" objective.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if this is actually a revert of previous changes
# Check commit messages for revert indicators
git log --grep="revert" --grep="restore" -i --format="%h %s" HEAD~10..HEAD
# Check if these constants were modified recently
git log -p --since="1 month ago" -- packages/analytics-js-common/src/constants/logMessages.ts
Length of output: 5884
packages/analytics-js-common/src/utilities/errors.ts (3)
2-2: LGTM: Import change and constant definition are well-structured
The change to use stringifyData aligns with the codebase-wide standardization of JSON serialization methods, and the constant follows clear naming conventions.
Also applies to: 4-4
29-29: LGTM: Exports are properly structured
The export statement cleanly exposes all necessary functionality while maintaining named exports pattern.
22-27: Verify ErrorEvent compatibility across browsers
The new error dispatch functionality uses the global ErrorEvent API. As per the PR objectives, this needs cross-browser testing (Chrome, Firefox, and IE11).
packages/analytics-js-plugins/src/errorReporting/constants.ts (2)
20-20: Good security practice: Excluding auth token from error reports
Adding 'authToken' to the excluded keys prevents sensitive authentication information from being exposed in error reports.
Line range hint 8-21: Verify consistent token exclusion across error reporting
Let's ensure this security enhancement is consistently applied across all error reporting configurations in the codebase.
✅ Verification successful
Token exclusion is consistently implemented across error reporting configurations
The verification shows that the authToken exclusion is properly implemented across the error reporting system:
-
It's included in
APP_STATE_EXCLUDE_KEYSin both:- packages/analytics-js-plugins/src/errorReporting/constants.ts
- packages/analytics-js-plugins/src/bugsnag/constants.ts
-
The exclusion is effectively used in the error reporting utils through
getAppStateForMetadata()function which applies these exclusions when preparing error metadata. -
The only other occurrence of
authTokenis in deviceModeTransformation plugin which is unrelated to error reporting and uses it for its intended purpose of authentication.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other files that might need similar auth token exclusion
# and verify consistency across error reporting configurations
# Search for similar constant definitions that might need the update
rg -l "APP_STATE_EXCLUDE_KEYS|ERROR_REPORTING|BUGSNAG" --type typescript
# Look for any potential auth token references in error reporting related files
rg -i "authToken|auth_token|authentication" --type typescript packages/analytics-js-plugins/src/
Length of output: 243
Script:
#!/bin/bash
# Search for similar constant definitions and error reporting configurations
rg -l "APP_STATE_EXCLUDE_KEYS|ERROR_REPORTING|BUGSNAG"
# Look for any potential auth token references in error reporting related files
rg -i "authToken|auth_token|authentication" packages/analytics-js-plugins/src/
Length of output: 1761
Script:
#!/bin/bash
# Let's check the content of bugsnag/constants.ts and deviceModeTransformation/index.ts
# to understand the context of authToken usage
# Check bugsnag constants
cat packages/analytics-js-plugins/src/bugsnag/constants.ts
# Check deviceModeTransformation
cat packages/analytics-js-plugins/src/deviceModeTransformation/index.ts
# Also check error reporting utils to see how these constants are used
cat packages/analytics-js-plugins/src/errorReporting/utils.ts
Length of output: 11994
packages/analytics-js-plugins/src/bugsnag/constants.ts (2)
34-34: LGTM! Good security practice.
Adding 'authToken' to the excluded keys prevents sensitive authentication data from being exposed in error reports. This aligns well with existing security practices of excluding PII and sensitive data.
34-34: Verify if this is actually a temporary revert.
The PR is titled "chore: temp reverts" but this change appears to be a security enhancement rather than a temporary revert. Please clarify if this change is intended to be temporary or permanent.
✅ Verification successful
Let me generate additional scripts to get more context about these changes.
Based on the git history, I can see a pattern of authToken being added and removed multiple times, but I need to see the actual commit messages to understand the context better.
This is not a temporary revert - it's a permanent security enhancement that was temporarily reverted and now restored
The git history shows this is part of a feature PR (#1902) titled "feat: sanitize input data" that was temporarily reverted due to issues (#1916) and has now been restored. The authToken addition to APP_STATE_EXCLUDE_KEYS is a security enhancement to prevent sensitive data from being exposed, and its current presence is the intended permanent state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check git history to verify if this is reverting a previous change
# Check for any recent commits that removed 'authToken' from APP_STATE_EXCLUDE_KEYS
git log -p -- "packages/analytics-js-plugins/src/bugsnag/constants.ts" | grep -B 5 -A 5 "authToken"
# Check for any related issues or PRs
gh issue list --search "authToken bugsnag"
gh pr list --search "authToken bugsnag"
Length of output: 1138
Script:
#!/bin/bash
# Get the commit messages and dates for the commits that modified authToken
git log --format="%h %as %s" -p -- "packages/analytics-js-plugins/src/bugsnag/constants.ts" | grep -B 1 "authToken"
# Check PR title and description for the current changes
gh pr view --json title,body
Length of output: 289
Script:
#!/bin/bash
# Get full commit details including messages for commits touching this file
git log --format="%H%n%as%n%B%n----------------------" packages/analytics-js-plugins/src/bugsnag/constants.ts | head -n 20
# Also check for any related files that might give context
fd bugsnag --type f --exec grep -l "authToken" {}
Length of output: 754
packages/analytics-js/src/components/configManager/util/validate.ts (3)
Line range hint 8-15: LGTM! Well-structured validation functions
The validation functions follow good practices with proper type checking, null handling, and single responsibility principle.
1-6: Verify the removal of utility imports
The removal of isValidURL and isString imports aligns with the removal of validation functions. However, let's verify if these utilities are needed elsewhere in the codebase.
Line range hint 54-60: Verify handling of removed validations
The removal of validateWriteKey and validateDataPlaneUrl functions suggests this validation has moved elsewhere. Let's verify the new validation implementation.
✅ Verification successful
Let me try a different search strategy to locate the validation logic.
Based on the search results, I can see that the validation logic for writeKey and dataPlaneUrl has been moved to the core utilities module in packages/analytics-js/src/components/core/utilities.ts. The validation functions are now:
isWriteKeyValid- Validates that writeKey is a non-empty stringisDataPlaneUrlValid- Validates that dataPlaneUrl is a valid URL string
These validations are imported and used in the Analytics core component. The error messages are defined in logMessages.ts.
Therefore, I can conclude that the validation functionality has not been removed but rather relocated and improved. The validation is still being handled properly in the new location.
The validation functionality is properly maintained in the new location
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for new validation implementations
echo "Searching for writeKey validation..."
ast-grep --pattern 'function $_(writeKey$_) {
$$$
}'
echo "Searching for dataPlaneUrl validation..."
ast-grep --pattern 'function $_(dataPlaneUrl$_) {
$$$
}'
Length of output: 291
Script:
#!/bin/bash
# Search for validation-related code with broader patterns
echo "Searching for writeKey validation..."
rg -i "writekey.*valid|valid.*writekey" -A 3
echo "\nSearching for dataPlaneUrl validation..."
rg -i "dataplane.*valid|valid.*dataplane|dataplaneurl.*valid|valid.*dataplaneurl" -A 3
echo "\nSearching for validation in config-related files..."
fd "config|validate" --type f --exec rg -l "writeKey|dataPlaneUrl"
Length of output: 302435
packages/analytics-js-plugins/src/beaconQueue/utilities.ts (1)
34-34: Verify the impact of switching to stringifyData
The change from stringifyWithoutCircular to stringifyData might have implications:
- Cross-browser compatibility needs verification as the PR requires testing across Chrome, Firefox, and IE11
- Handling of circular references might be affected since related test cases were removed
✅ Verification successful
Change to stringifyData is consistent with codebase patterns
The verification reveals that:
stringifyDatais the standard JSON stringification method used across the codebase, including in similar payload serialization scenarios- The method is well-tested in
packages/analytics-js-common/__tests__/utilities/json.test.tswith comprehensive test cases covering:- Handling of null values
- Circular reference handling
- Multi-level object serialization
- The change aligns with the codebase's consistent usage pattern in other queue implementations (xhrQueue, errorReporting)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of stringifyWithoutCircular to ensure consistent changes
rg "stringifyWithoutCircular" --type ts
# Look for any existing tests of stringifyData to understand its behavior
rg "stringifyData.*test" --type ts
Length of output: 76
Script:
#!/bin/bash
# Search for stringifyData usage in the codebase
rg "stringifyData" --type ts -A 2
# Look for json utility files that might contain these methods
fd "json" --type f --extension ts
# Search for test files related to JSON serialization
fd "test" --type f --extension ts | rg -i "json|serialize"
Length of output: 13039
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-34: packages/analytics-js-plugins/src/beaconQueue/utilities.ts#L34
Added line #L34 was not covered by tests
packages/analytics-js/src/services/ErrorHandler/processError.ts (1)
1-1: LGTM! Consistent with codebase-wide refactor.
The replacement of stringifyWithoutCircular with stringifyData aligns with the broader initiative to standardize data serialization across the codebase.
Let's verify this refactor is consistent across the codebase:
Also applies to: 22-22
✅ Verification successful
Refactor verified: stringifyWithoutCircular correctly replaced with stringifyData
The verification confirms:
- No instances of
stringifyWithoutCircularexist in the analytics-js package stringifyDatais consistently imported from '@rudderstack/analytics-js-common/utilities/json'- The remaining
stringifyWithoutCircularusages are only in v1.1 legacy code and examples
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that stringifyWithoutCircular has been completely replaced with stringifyData
# Check if there are any remaining instances of stringifyWithoutCircular
echo "Checking for any remaining usage of stringifyWithoutCircular..."
rg "stringifyWithoutCircular"
# Check the new stringifyData usage pattern
echo "Verifying consistent import pattern for stringifyData..."
rg "import.*stringifyData.*from.*@rudderstack/analytics-js-common/utilities/json"
Length of output: 11341
Script:
#!/bin/bash
# Let's verify if stringifyData is consistently used in the analytics-js package
# and if stringifyWithoutCircular is only present in v1.1 and examples
# Check for any stringifyData usage in analytics-js
echo "Checking stringifyData usage in analytics-js..."
rg -l "stringifyData" packages/analytics-js/
# Check if stringifyWithoutCircular exists in analytics-js
echo "Checking if stringifyWithoutCircular exists in analytics-js..."
rg -l "stringifyWithoutCircular" packages/analytics-js/
# Check the actual implementation of stringifyData
echo "Checking stringifyData implementation..."
ast-grep --pattern 'export const stringifyData = $_'
Length of output: 845
packages/analytics-js-common/src/utilities/checks.ts (3)
84-84: LGTM!
The export follows the established pattern and is correctly placed alongside other type checking utilities.
Line range hint 38-84: Verify if this change aligns with PR objectives
The PR is titled "chore: temp reverts", but this appears to be a new feature addition for BigInt support rather than a revert. Could you clarify if this change is intentional as part of the temporary reverts?
Let's check the git history to understand if this is actually a revert:
✅ Verification successful
Based on the git history output, I can now generate the final response:
The BigInt support is part of a feature revert cycle
The git history shows this is actually part of a revert chain:
- Initially added in PR feat: sanitize input data #1902 "feat: sanitize input data"
- Temporarily reverted in PR fix: revert sanitization changes #1916
- Now being restored via reverting the revert
This change aligns with the PR objectives as it's restoring previously implemented functionality through a revert operation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check git history for any previous BigInt-related changes
git log -p --all -S "isBigInt" --since="6 months ago"
Length of output: 20040
38-43: Verify IE11 compatibility and enhance documentation
The implementation is correct, but there are two considerations:
- Since IE11 support is mentioned in the PR objectives, note that
BigIntis not supported in IE11. This might need special handling or polyfilling. - The documentation could be enhanced with an example for better clarity.
Let's check for any existing IE11-specific handling in the codebase:
Consider enhancing the documentation:
/**
* Checks if the input is a BigInt
* @param value input value
* @returns True if the input is a BigInt
+ * @example
+ * isBigInt(BigInt(9007199254740991)) // returns true
+ * isBigInt(123) // returns false
*/packages/analytics-js-plugins/src/utilities/eventsDelivery.ts (3)
6-6: LGTM! Good use of shared utility
The import of stringifyData from the common utilities package promotes code reuse and consistency across the codebase.
28-29: LGTM! Good null handling
The function properly preserves the nullable return type from stringifyData, making it more robust and type-safe.
25-26: Verify stringifyData's null handling
The function assumes stringifyData will always return a string, but the type cast might hide potential null values.
Let's verify the implementation of stringifyData:
packages/analytics-js-plugins/src/errorReporting/event/event.ts (2)
4-4: LGTM: Import change aligns with standardization efforts
The change from stringifyWithoutCircular to stringifyData is consistent with the codebase-wide standardization of JSON serialization utilities.
71-71: Verify the purpose of the false parameter in stringifyData
The stringifyData(error, false) call includes a boolean parameter, but its purpose isn't immediately clear. Please ensure this parameter correctly handles circular references in error objects as intended.
Let's check the implementation and usage of stringifyData:
✅ Verification successful
Let me gather more information about the stringifyData implementation to understand the purpose of the boolean parameter.
Based on the implementation and usage patterns found in the codebase, I can now provide the final response:
The false parameter in stringifyData(error, false) is correctly used
The stringifyData utility's second parameter excludeNull controls whether null values should be excluded from the stringified output. When set to false, it includes null values in the output, which is the desired behavior for error reporting to ensure all error details are captured, including null values. This is consistent with other usages in the codebase where complete data representation is needed, such as in cookies, store management, and user session handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the stringifyData implementation and its usage patterns
ast-grep --pattern 'export function stringifyData($_,$_) {
$$$
}'
# Look for other usages of stringifyData with boolean parameters
rg 'stringifyData\([^,]+,\s*(true|false)\)' -t ts
Length of output: 1078
Script:
#!/bin/bash
# Search for the stringifyData implementation in json utilities
fd "json\.(ts|js)" --exec cat {}
# Look for any tests or documentation related to stringifyData
rg -A 5 "stringifyData" --type ts
Length of output: 26133
packages/analytics-js-cookies/src/cookiesUtilities.ts (1)
5-5: ```shell
#!/bin/bash
Let's check the implementation and tests of stringifyData in the common package
Check the implementation
rg -A 10 "export function stringifyData" packages/analytics-js-common/
Check the tests specifically for circular references
rg -A 10 -B 2 "circular" packages/analytics-js-common/tests/utilities/json.test.ts
</details>
<details>
<summary>packages/analytics-js/src/services/HttpClient/HttpClient.ts (1)</summary>
Line range hint `65-77`: **LGTM! The logger parameter removal is consistent with the getData method.**
The change aligns with the overall pattern of removing logger parameters from xhr requests while maintaining error handling through the onError callback.
Let's verify the consistency of this change across related files:
<details>
<summary>✅ Verification successful</summary>
**Removal of logger parameter from xhrRequest is correct and consistent**
The verification confirms that:
1. The `xhrRequest` function implementation only accepts `options` and `timeout` parameters
2. All usages of `xhrRequest` in the codebase follow this signature
3. While the `HttpClient` class has a logger property, it's not being passed to the `xhrRequest` function anywhere
</details>
<details>
<summary>🏁 Scripts executed</summary>
The following scripts were executed for the analysis:
Script:
```shell
#!/bin/bash
# Description: Verify logger parameter usage in xhr-related files
# Expected: No instances of passing logger to xhrRequest should remain
# Check for any remaining logger parameters in xhrRequest calls
ast-grep --pattern 'xhrRequest($$$logger$$$)'
# Check the implementation of xhrRequest to confirm logger parameter removal
rg -A 5 "function xhrRequest" packages/analytics-js/src/services/HttpClient/xhr/
Length of output: 130
Script:
#!/bin/bash
# Let's try a different approach to verify the changes
# First, let's find the xhr implementation file
fd xhr.ts
# Then check all files that might use xhrRequest
rg "xhrRequest" --type ts
# Look for any logger-related parameters in xhr implementation
rg "logger" --type ts packages/analytics-js/src/services/HttpClient/
Length of output: 1432
Script:
#!/bin/bash
# Let's check the implementation of xhrRequest to understand its parameters
# Get the xhrRequest implementation
rg -A 10 "const xhrRequest = \(" packages/analytics-js/src/services/HttpClient/xhr/xhrRequestHandler.ts
# Check the interface definition
rg "interface.*HttpClient" -A 10 packages/analytics-js/src/services/HttpClient/
Length of output: 517
packages/analytics-js-plugins/src/xhrQueue/utilities.ts (1)
14-16: LGTM! Verify JSON serialization compatibility.
The function signature and implementation changes look good. The removal of the logger parameter and transition to stringifyData simplifies the code.
Let's verify that the new stringifyData method is consistently used across the codebase:
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the JSON serialization compatibility:
JSON serialization changes are consistent and safe
The codebase shows a clear migration pattern:
stringifyWithoutCircularis only present in v1.1 and older code paths (packages/analytics-js-common/src/v1.1/,packages/analytics-v1.1/)stringifyDatais consistently used in the new codebase, including the modified file and its dependencies- Both implementations handle circular references and null values similarly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of stringifyWithoutCircular
# to ensure complete migration to stringifyData
# Search for any remaining instances of stringifyWithoutCircular
rg "stringifyWithoutCircular"
# Search for new stringifyData usage for context
rg "stringifyData" -A 2
Length of output: 22433
packages/analytics-js-common/src/utilities/object.ts (3)
12-13: LGTM! Implementation aligns with intended behavior.
Based on previous learnings, this implementation correctly returns true for both regular objects and null, as the null check is handled separately in functions like isObjectAndNotNull.
20-20: LGTM! Clear and efficient implementation.
The modified implementation:
- Maintains the existing behavior
- Leverages the new
isObjectutility for better code organization - Correctly implements the type predicate
121-121: LGTM! Export follows consistent pattern.
The isObject utility is now available for use in other modules, following the file's established export pattern.
packages/analytics-js/src/services/HttpClient/xhr/xhrRequestHandler.ts (1)
3-3: Verify handling of circular references with new stringifyData utility
The switch from stringifyWithoutCircular to stringifyData might affect how circular references are handled in the request payload.
Let's verify the implementation of the new utility:
✅ Verification successful
Let me gather more information about the implementation of stringifyData to verify its handling of circular references.
Based on the test cases and implementation shown, I can now generate the final response:
The switch to stringifyData maintains proper handling of circular references
The new stringifyData utility works in conjunction with getSanitizedValue which properly handles circular references by replacing them with a '[Circular Reference]' placeholder. The test cases demonstrate comprehensive coverage including:
- Circular reference detection and replacement
- Proper handling of reused objects
- Support for various data types including nested objects
- Optional null value exclusion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of stringifyData to ensure it handles circular references
# Search for stringifyData implementation and its tests
rg -A 10 "export.*stringifyData" --type ts
rg -A 10 "describe.*stringifyData" --type ts
Length of output: 1231
Script:
#!/bin/bash
# Get the implementation of stringifyData from json.ts
rg -B 5 -A 20 "function stringifyData" packages/analytics-js-common/src/utilities/json.ts
# Get the complete test file to understand all test cases
cat packages/analytics-js-common/__tests__/utilities/json.test.ts
Length of output: 7351
packages/analytics-js-common/src/types/IRudderAnalytics.ts (1)
88-88: Breaking change: Verify all consumers handle undefined return value
The return type change from T to T | undefined improves type safety by explicitly indicating that the analytics instance might not exist. However, this is a breaking change that requires verification.
Let's verify the impact on consumers:
✅ Verification successful
Let me gather more information about the usage of getAnalyticsInstance across the codebase to ensure we have a complete picture.
Return type change is properly handled across the codebase
Based on the evidence from the codebase:
- The implementation in
RudderAnalytics.tsalready handles undefined returns with proper error handling and try-catch blocks - All consumers of
getAnalyticsInstance()use the optional chaining operator (?.) when calling methods - The implementation in the Chrome extension also handles the case when analytics instance doesn't exist
The type change from T to T | undefined is actually aligning the type definition with the existing implementation and usage patterns, making it more type-safe rather than introducing a breaking change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of getAnalyticsInstance
# We want to ensure all call sites properly handle the undefined case
ast-grep --pattern 'getAnalyticsInstance($$$)'
# Search for potential type assertions that might break
rg -A 2 'getAnalyticsInstance.*!'
Length of output: 3627
Script:
#!/bin/bash
# Search for all files that import or use IRudderAnalytics interface
rg -l "IRudderAnalytics"
# Search for class implementations of getAnalyticsInstance
ast-grep --pattern 'getAnalyticsInstance($$$) {
$$$
}'
# Search for null checks or error handling around getAnalyticsInstance calls
rg -A 3 "getAnalyticsInstance\("
Length of output: 17879
packages/analytics-js-plugins/src/xhrQueue/index.ts (2)
69-69: Verify logger removal impact on error tracking.
The removal of the logger parameter from getRequestInfo aligns with the changes. However, let's verify that error logging hasn't been impacted.
105-105: Verify batch payload generation logging.
The removal of the logger parameter from getBatchDeliveryPayload aligns with the changes. Let's verify that any critical logging hasn't been lost.
✅ Verification successful
Based on the git history shown in the previous output, I can generate the final response:
The removal of logger parameter is safe and aligns with the codebase changes.
The git history shows that the logger parameter was introduced during the batching support implementation (commit 0421663) and was later removed as part of input data sanitization changes (commit b71c44a). The removal was intentional and part of a broader refactoring effort. There's no evidence of critical logging being lost, as the logger was primarily used for error handling in the XHR queue implementation itself, not within the getBatchDeliveryPayload function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if getBatchDeliveryPayload has any internal logging that might be affected
# Look for any logging calls within getBatchDeliveryPayload implementation
ast-grep --pattern 'function getBatchDeliveryPayload($_) {
$$$
logger.$$$
$$$
}'
# Also check if there are any tests verifying the logging behavior
fd -e test.ts -e spec.ts -e test.js -e spec.js --exec rg -l "getBatchDeliveryPayload.*logger"
Length of output: 184
Script:
#!/bin/bash
# Let's first find the implementation of getBatchDeliveryPayload
ast-grep --pattern 'function getBatchDeliveryPayload' -A 20
# Also search for any direct usage of logger in the file
rg "logger\." packages/analytics-js-plugins/src/xhrQueue/index.ts
# Check the git history for any removed logging
git log -p -S "logger" -- packages/analytics-js-plugins/src/xhrQueue/index.ts
Length of output: 26519
Script:
#!/bin/bash
# Let's find the implementation of getBatchDeliveryPayload to see if it had any logging
rg -A 10 "export const getBatchDeliveryPayload" packages/analytics-js-plugins/src/utilities/queue.ts
# Also check if there are any tests that verify logging behavior
rg -l "getBatchDeliveryPayload.*logger" packages/analytics-js-plugins/src/**/*.test.ts
Length of output: 373
packages/analytics-js-plugins/src/errorReporting/utils.ts (2)
19-19: LGTM: Import change aligns with code reuse principles
The replacement of the previous stringification utility with a common implementation promotes better code maintainability and consistency.
70-71: Verify the impact of enabling circular reference handling
The change from false to true in the stringifyData parameters modifies how circular references are handled. This could affect the application state serialization behavior.
Let's verify the usage and impact:
✅ Verification successful
Let me gather more information to understand the context better.
Let me gather one final piece of information to understand the stringifyData implementation.
Based on the gathered information, I can now provide the final response:
The change in circular reference handling is intentional and safe
The modification from false to true in stringifyData is consistent with the implementation across both error reporting utilities (Bugsnag and ErrorReporting). The tests verify that this behavior correctly handles object serialization while excluding specified keys through APP_STATE_EXCLUDE_KEYS. The change ensures consistent state serialization behavior across the error reporting system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests related to app state serialization
rg -l "getAppStateForMetadata" --type=test
# Look for any other uses of stringifyData with similar patterns
ast-grep --pattern 'stringifyData($_, true, $_)'
Length of output: 247
Script:
#!/bin/bash
# Look for the test file content to understand the behavior
rg "stringifyData.*true" -A 5 packages/analytics-js-common/__tests__/utilities/json.test.ts
# Find the implementation of stringifyData
ast-grep --pattern 'function stringifyData'
# Look for any other uses of getAppStateForMetadata
rg "getAppStateForMetadata" -A 3
Length of output: 6466
Script:
#!/bin/bash
# Look for the implementation and tests of stringifyData to understand the behavior difference between true/false
rg "stringifyData.*function" -A 10 packages/analytics-js-common/src/utilities/json.ts
# Look for tests that verify the behavior
rg "describe.*stringifyData" -A 10 packages/analytics-js-common/__tests__/utilities/json.test.ts
Length of output: 548
packages/analytics-js-plugins/src/bugsnag/utils.ts (1)
68-69:
Add error handling for JSON parsing and verify stringification behavior.
The changes to getAppStateForMetadata have potential issues:
- The removal of the null check before
JSON.parsecould lead to runtime errors - The switch to
json.stringifyDatamight change how circular references are handled in error reports
Add error handling to prevent runtime errors:
const getAppStateForMetadata = (state: ApplicationState): Record<string, any> | undefined => {
const stateStr = json.stringifyData(state, true, APP_STATE_EXCLUDE_KEYS);
- return JSON.parse(stateStr);
+ try {
+ return stateStr ? JSON.parse(stateStr) : undefined;
+ } catch (e) {
+ return undefined;
+ }
};Let's verify the impact of the stringification changes:
packages/analytics-js/src/services/StoreManager/Store.ts (2)
3-3: Verify the impact of serialization function change.
The change from stringifyWithoutCircular to stringifyData is part of a broader update. Let's verify all usages to ensure consistent implementation.
#!/bin/bash
# Description: Verify the migration from stringifyWithoutCircular to stringifyData
# and ensure no old references remain
# Check for any remaining references to stringifyWithoutCircular
echo "Checking for remaining references to stringifyWithoutCircular:"
rg "stringifyWithoutCircular"
# Check the new stringifyData usage pattern
echo "Checking stringifyData usage patterns:"
rg "stringifyData\((.*?)\)" -A 2110-110: Verify stringifyData parameters and error logging.
The change simplifies the parameters passed to the serialization function by removing the logger. Ensure that error scenarios are still properly logged and that the false parameter maintains the same behavior as before.
✅ Verification successful
The change from stringifyWithoutCircular to stringifyData is safe and maintains functionality.
The code review shows that:
stringifyDataproperly handles thefalseparameter to include null values, which matches the previous behavior- Error logging is handled separately through
getSanitizedValuefunction, so removing the logger parameter from serialization doesn't impact error reporting - The test coverage confirms that both null handling and key exclusion work as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the stringifyData implementation and its error handling
# Check the stringifyData implementation
echo "Checking stringifyData implementation:"
ast-grep --pattern 'function stringifyData($_) {
$$$
}'
# Check for related test coverage
echo "Checking test coverage:"
fd -e test.ts -e spec.ts | xargs rg "stringifyData"
Length of output: 1709
Script:
#!/bin/bash
# Let's check the implementation of stringifyData in the common utilities
echo "Checking stringifyData implementation in common utilities:"
rg -A 10 "export (const|function) stringifyData"
# Let's also check the previous implementation that was replaced
echo "Checking previous stringifyWithoutCircular implementation:"
rg -A 10 "export (const|function) stringifyWithoutCircular"
# Check imports and usage of both functions
echo "Checking imports and usage:"
rg "import.*stringifyWithoutCircular|import.*stringifyData"
Length of output: 3450
Script:
#!/bin/bash
# Let's check the implementation in the common package
echo "Checking stringifyData implementation in json.ts:"
rg -A 15 "export (const|function) stringifyData" packages/analytics-js-common/src/utilities/json.ts
# Let's also check the test cases to understand the parameters
echo "Checking test cases for stringifyData:"
rg -A 10 "describe\('stringifyData" packages/analytics-js-common/__tests__/utilities/json.test.ts
Length of output: 762
Script:
#!/bin/bash
# Let's check the actual content of the json.ts file
echo "Content of json.ts file:"
cat packages/analytics-js-common/src/utilities/json.ts
# Let's also check the complete test file
echo "Content of json.test.ts file:"
cat packages/analytics-js-common/__tests__/utilities/json.test.ts
Length of output: 10864
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (2)
17-17: LGTM!
The import statement follows the established pattern and is appropriately placed with other error-related imports.
172-174: Verify manual error handling implementation across the codebase
Let's ensure consistent handling of manual errors across the codebase.
✅ Verification successful
Let me gather more information about error handling patterns to ensure a comprehensive verification.
Let me generate the final response based on the findings from the code search results.
Error handling implementation is consistent across the codebase
The error handling implementation in ErrorHandler.ts is consistent with other error handling patterns in the codebase. The code search reveals:
- The error handling follows a standard pattern of logging errors and optionally throwing them
- The error stack/message is properly formatted before logging
- Error reporting plugins (like Bugsnag) are properly integrated
- Network errors and retries are handled consistently
The changes to check for MANUAL_ERROR_IDENTIFIER in the stack trace is properly scoped and doesn't require similar updates elsewhere in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of MANUAL_ERROR_IDENTIFIER
echo "Searching for MANUAL_ERROR_IDENTIFIER usage patterns..."
rg "MANUAL_ERROR_IDENTIFIER" -B 2 -A 2
# Search for similar error handling patterns
echo "Searching for similar error handling patterns..."
ast-grep --pattern 'if ($error instanceof ErrorEvent) {
$$$
}'
Length of output: 2617
Script:
#!/bin/bash
# Search for error handling patterns with ErrorEvent
echo "Searching for ErrorEvent type checks..."
rg "error instanceof ErrorEvent" -B 2 -A 2
# Search for error.stack usage patterns
echo "Searching for error.stack access patterns..."
rg "error\?\.stack" -B 2 -A 2
# Search for error logging patterns
echo "Searching for error logging patterns..."
rg "\.error\(.*error" -B 2 -A 2
Length of output: 460622
packages/analytics-js/src/components/configManager/ConfigManager.ts (1)
13-13:
Verify validation coverage after removal of validateLoadArgs
The removal of validateLoadArgs import suggests that critical configuration parameters (writeKey, dataPlaneUrl) might no longer be validated. Since this is marked as a temporary revert:
- Please clarify the timeline and tracking for restoring these validations
- Confirm if these validations have been moved elsewhere in the codebase
Let's verify if these validations exist elsewhere:
Consider adding a TODO comment with a tracking issue to ensure these validations are restored, or document why they are no longer needed.
packages/analytics-js-common/__tests__/utilities/json.test.ts (2)
234-247: LGTM! Comprehensive primitive type coverage.
The test effectively covers all primitive types and common JavaScript objects, ensuring robust sanitization across different data types.
182-232: LGTM! Thorough verification of object reuse handling.
The test case effectively verifies that the sanitization process:
- Correctly handles reused arrays and objects
- Maintains data integrity in nested structures
- Properly manages object references
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (5)
181-189: LGTM! Consistent sanitization pattern applied
The sanitization implementation follows the same pattern as other functions, ensuring consistent and safe input handling.
262-269: LGTM! Proper null handling maintained
The function correctly preserves the special null handling for userId while adding sanitization. The type assertions and conditional logic are properly maintained.
303-311: LGTM! Sanitization correctly implemented
The sanitization is properly implemented while maintaining the function's original behavior and type safety.
9-9: Verify the implementation of getSanitizedValue
The newly imported getSanitizedValue function is being used extensively for input sanitization. Let's verify its implementation to ensure it handles all edge cases properly.
✅ Verification successful
getSanitizedValue implementation is well-tested and safe to use
The function is properly implemented with comprehensive test coverage:
- Handles primitive types, objects, and arrays correctly
- Properly manages edge cases like null, undefined values
- Safely handles circular references and BigInt values
- Includes logging capability for problematic cases
- Has thorough test coverage including all data types and edge cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of getSanitizedValue function
# and its test coverage
# Check the implementation
echo "Checking implementation of getSanitizedValue:"
rg -A 10 "export.*getSanitizedValue" ./packages/analytics-js-common/src/
# Check test coverage
echo "Checking test coverage:"
rg -A 10 "getSanitizedValue" ./packages/analytics-js-common/__tests__/
Length of output: 354
Script:
#!/bin/bash
# Let's try a different approach to locate the implementation and tests
# First, let's find the json.ts file that contains getSanitizedValue
echo "Looking for json.ts file:"
fd "json.ts" ./packages/analytics-js-common/src/
# Then let's examine its content
echo -e "\nChecking implementation in json.ts:"
rg -A 10 "getSanitizedValue" ./packages/analytics-js-common/src/utilities/json.ts
# Let's also check for any test files related to json utilities
echo -e "\nLooking for related test files:"
fd "json" ./packages/analytics-js-common/__tests__/
# And examine their content if found
echo -e "\nChecking test coverage in json tests:"
rg "getSanitizedValue" ./packages/analytics-js-common/__tests__/ -A 5
Length of output: 5018
368-376: Verify test coverage for edge cases
The implementation looks correct, but let's ensure we have proper test coverage for edge cases, especially around null handling and type conversions.
✅ Verification successful
Looking at the test files, I can see that there are comprehensive test cases for eventMethodOverloads.test.ts that cover the groupArgumentsToCallOptions function, including edge cases for null handling and type conversions. The tests verify:
- Handling of null groupId
- Type conversion of groupId to string
- Handling of undefined traits and options
- Proper sanitization of all input parameters
- Various method overload combinations
Test coverage for edge cases is sufficient
The implementation is well-tested with proper coverage for null handling, type conversions, and all possible parameter combinations. No additional test cases are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for groupArgumentsToCallOptions edge cases
# Look for test cases covering null handling and type conversions
echo "Checking test coverage for edge cases:"
rg -A 10 "groupArgumentsToCallOptions.*null" ./packages/analytics-js-common/__tests__/
rg -A 10 "groupArgumentsToCallOptions.*undefined" ./packages/analytics-js-common/__tests__/
Length of output: 354
Script:
#!/bin/bash
# Let's try a different approach to find test files and their content
# First, find test files related to eventMethodOverloads
fd "eventMethodOverloads.*test" ./packages/analytics-js-common/__tests__/
# Then search for group-related test cases in all test files
rg -l "test.*group" ./packages/analytics-js-common/__tests__/
# Finally, let's look at the content of any test file that might contain group tests
fd ".*test.*\.(ts|js)" ./packages/analytics-js-common/__tests__/ --exec cat {}
Length of output: 61869
packages/analytics-js/src/constants/logMessages.ts (2)
69-70: LGTM! Improved type safety in function signature.
The updated signature enforces stricter type checking by requiring both parameters as strings, which aligns with the previous learning about enhancing type safety in error message functions.
76-76: LGTM! Improved error message clarity.
The updated error message is more specific and clearly indicates that the callback must be a function.
packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (5)
35-35: LGTM: Import statement is correct
The import statement correctly references the new stringifyData utility.
372-373: LGTM: Consistent serialization in cookie value comparison
The change ensures consistent serialization when comparing cookie values.
472-476: LGTM: Improved anonymous ID validation
The changes improve type safety and make the behavior more explicit when handling invalid anonymous IDs.
676-676: LGTM: Explicit boolean comparison
The strict equality comparison makes the reset behavior more explicit and predictable.
307-307: Verify cookie encryption with the new serialization
The change to stringifyData looks correct, but since this affects cookie encryption, we should verify the encryption process still works as expected.
✅ Verification successful
Let me gather more information about the cookie encryption functionality by checking the test file mentioned in the results.
Let me check the implementation of the stringifyData function to verify it maintains the same behavior as stringifyWithoutCircular.
Let me check one final thing - the actual implementation of the stringifyData function from the common utilities package.
Let me check the actual implementation of stringifyData in the common utilities package.
The cookie encryption change is safe to proceed with
The change from stringifyWithoutCircular to stringifyData is verified to be safe because:
- The
stringifyDatafunction maintains the same behavior when called withfalseparameter - it will not exclude null values - The test coverage shows proper encryption and validation of cookie values
- The implementation is used consistently across the codebase for cookie-related operations
- The function is part of the core common utilities package and is well-tested
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any encryption-related test files
fd -e test.ts -e spec.ts | xargs rg -l "encrypt.*cookie"
# Look for any encryption-related error handling
rg "encrypt.*error" -A 5
Length of output: 125272
Script:
#!/bin/bash
# Check the test file for cookie encryption tests
rg -A 10 "encrypt.*cookie" packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts
Length of output: 2461
Script:
#!/bin/bash
# Search for stringifyData implementation
rg -A 10 "stringifyData" packages/analytics-js/src/
Length of output: 12577
Script:
#!/bin/bash
# Look for stringifyData implementation in common utilities
rg -A 10 "export.*stringifyData"
# Also check for any imports of this function
rg "import.*stringifyData"
Length of output: 1717
Script:
#!/bin/bash
# Look for stringifyData implementation in the json utilities file
rg -A 15 "stringifyData" packages/analytics-js-common/src/utilities/json.ts
Length of output: 695
packages/analytics-js/__tests__/components/core/Analytics.test.ts (2)
132-166: LGTM! Comprehensive test coverage for write key validation.
The test cases thoroughly validate the write key parameter with different invalid inputs:
- Empty string
- Whitespace-only string
- Non-string type (object)
Each case includes proper assertions for:
- Lifecycle status
- Error logging
- Prevention of SDK initialization
168-201: LGTM! Thorough test coverage for data plane URL validation.
The test cases effectively validate the data plane URL parameter with different invalid inputs:
- Empty string
- Undefined value
- Malformed URL
Each case includes proper assertions for:
- Lifecycle status
- Error logging
- Prevention of SDK initialization
| const getReplacer = (logger?: ILogger): ((key: string, value: any) => any) => { | ||
| const ancestors: any[] = []; // Array to track ancestor objects | ||
|
|
||
| // Using a regular function to use `this` for the parent context | ||
| return function replacer(key, value): any { | ||
| if (isBigInt(value)) { | ||
| logger?.warn(BAD_DATA_WARNING(JSON_UTIL, key)); | ||
| return '[BigInt]'; // Replace BigInt values | ||
| } | ||
|
|
||
| // `this` is the object that value is contained in, i.e., its direct parent. | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore-next-line | ||
| while (ancestors.length > 0 && ancestors[ancestors.length - 1] !== this) { | ||
| ancestors.pop(); | ||
| ancestors.pop(); // Remove ancestors that are no longer part of the chain | ||
| } | ||
|
|
||
| // Check for circular references (if the value is already in the ancestors) | ||
| if (ancestors.includes(value)) { | ||
| logger?.warn(CIRCULAR_REFERENCE_WARNING(JSON_STRINGIFY, key)); | ||
| logger?.warn(BAD_DATA_WARNING(JSON_UTIL, key)); | ||
| return '[Circular Reference]'; | ||
| } | ||
|
|
||
| // Add current value to ancestors | ||
| ancestors.push(value); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issue with ancestors array management in getReplacer
The ancestors array is used to track ancestor objects for circular reference detection. However, the current implementation does not remove the current value from ancestors after processing, which may lead to incorrect circular reference detection or memory issues due to the ancestors array growing indefinitely.
To ensure proper management of the ancestors array, consider removing the current value from ancestors after it has been processed. Here's how you can adjust the replacer function:
// Add current value to ancestors
ancestors.push(value);
// Process nested properties (handled internally by JSON.stringify)
+ // Remove the current value from ancestors
+ ancestors.pop();
return value;Alternatively, you can refactor the code to use a WeakSet for tracking ancestors, which automatically handles object dereferencing and memory management:
- const ancestors: any[] = []; // Array to track ancestor objects
+ const ancestors = new WeakSet(); // WeakSet to track ancestor objects
// Using a regular function to use `this` for the parent context
return function replacer(key, value): any {
if (isBigInt(value)) {
logger?.warn(BAD_DATA_WARNING(JSON_UTIL, key));
return '[BigInt]'; // Replace BigInt values
}
// Check for circular references
- if (ancestors.includes(value)) {
+ if (ancestors.has(value)) {
logger?.warn(BAD_DATA_WARNING(JSON_UTIL, key));
return '[Circular Reference]';
}
// Add current value to ancestors
- ancestors.push(value);
+ ancestors.add(value);
// Process nested properties (handled internally by JSON.stringify)
+ // Remove the current value from ancestors
+ ancestors.delete(value);
return value;
};Committable suggestion skipped: line range outside the PR's diff.
| getAnalyticsInstance(writeKey?: string): IAnalytics | undefined { | ||
| try { | ||
| let instanceId = writeKey; | ||
| if (!isString(instanceId) || !instanceId) { | ||
| instanceId = this.defaultAnalyticsKey; | ||
| } | ||
|
|
||
| const analyticsInstanceExists = Boolean(this.analyticsInstances[instanceId]); | ||
| const analyticsInstanceExists = Boolean(this.analyticsInstances[instanceId]); | ||
|
|
||
| if (!analyticsInstanceExists) { | ||
| this.analyticsInstances[instanceId] = new Analytics(); | ||
| } | ||
| if (!analyticsInstanceExists) { | ||
| this.analyticsInstances[instanceId] = new Analytics(); | ||
| } | ||
|
|
||
| return this.analyticsInstances[instanceId] as IAnalytics; | ||
| return this.analyticsInstances[instanceId] as IAnalytics; | ||
| } catch (error: any) { | ||
| dispatchErrorEvent(error); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Ensure getAnalyticsInstance consistently returns an IAnalytics instance
The getAnalyticsInstance method may return undefined if an error occurs, which could lead to undefined being used where an IAnalytics instance is expected. This could cause runtime errors in methods that depend on this instance.
Consider handling errors internally and ensuring that the method always returns a valid IAnalytics instance. Alternatively, propagate the error appropriately.
| it('should dispatch an error event if an exception is thrown during the ready call', () => { | ||
| const dispatchEventSpy = jest.spyOn(window, 'dispatchEvent'); | ||
|
|
||
| // Intentionally cause an error during the ready call | ||
| const getAnalyticsInstanceSpy = jest | ||
| .spyOn(rudderAnalytics, 'getAnalyticsInstance') | ||
| .mockImplementation(() => { | ||
| throw new Error('Error in getAnalyticsInstance'); | ||
| }); | ||
|
|
||
| const callback = () => console.log('Ready!'); | ||
|
|
||
| rudderAnalytics.ready(callback); | ||
| expect(readySpy).toHaveBeenCalledWith(expect.any(Function)); | ||
|
|
||
| expect(dispatchEventSpy).toHaveBeenCalledWith( | ||
| new ErrorEvent('error', { | ||
| error: new Error('Error in getAnalyticsInstance'), | ||
| }), | ||
| ); | ||
|
|
||
| dispatchEventSpy.mockRestore(); | ||
| getAnalyticsInstanceSpy.mockRestore(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repetitive error handling test cases to reduce duplication
The error handling test cases for methods like ready, page, track, identify, alias, group, reset, getAnonymousId, setAnonymousId, startSession, setAuthToken, consent, getUserId, getUserTraits, getGroupId, getGroupTraits, endSession, and getSessionId follow a similar pattern. They mock getAnalyticsInstance to throw an error and assert that an error event is dispatched. Consider refactoring these tests using a helper function to reduce code duplication and improve maintainability.
Proposed refactor:
Define a helper function for testing error handling:
function testErrorDispatch(
methodName: string,
methodArgs: any[] = [],
): void {
it(`should dispatch an error event if an exception is thrown during the ${methodName} call`, () => {
const dispatchEventSpy = jest.spyOn(window, 'dispatchEvent');
// Intentionally cause an error during the method call
const getAnalyticsInstanceSpy = jest
.spyOn(rudderAnalytics, 'getAnalyticsInstance')
.mockImplementation(() => {
throw new Error('Error in getAnalyticsInstance');
});
// Call the method with provided arguments
// @ts-ignore
rudderAnalytics[methodName](...methodArgs);
expect(dispatchEventSpy).toHaveBeenCalledWith(
new ErrorEvent('error', {
error: new Error('Error in getAnalyticsInstance'),
}),
);
dispatchEventSpy.mockRestore();
getAnalyticsInstanceSpy.mockRestore();
});
}Then, replace individual test cases with calls to this helper function:
testErrorDispatch('ready', [() => console.log('Ready!')]);
testErrorDispatch('page', ['category']);
testErrorDispatch('track', ['event']);
testErrorDispatch('identify', ['1234']);
testErrorDispatch('alias', ['abc']);
testErrorDispatch('group', ['5678']);
testErrorDispatch('reset', [true]);
testErrorDispatch('getAnonymousId');
testErrorDispatch('setAnonymousId', ['id', 'param']);
testErrorDispatch('startSession', [1234]);
testErrorDispatch('setAuthToken', ['token']);
testErrorDispatch('consent', [{
consentManagement: {
allowedConsentIds: ['1'],
deniedConsentIds: ['2'],
},
}]);
testErrorDispatch('getUserId');
testErrorDispatch('getUserTraits');
testErrorDispatch('getGroupId');
testErrorDispatch('getGroupTraits');
testErrorDispatch('endSession');
testErrorDispatch('getSessionId');This approach reduces redundancy and simplifies the test suite.
Also applies to: 254-275, 281-302, 309-330, 337-358, 365-386, 393-414, 428-450, 457-478, 506-525, 542-563, 581-606, 609-631, 634-656, 659-681, 684-706, 709-754
| const dispatchEvent = jest.fn(); | ||
| const originalDispatchEvent = globalThis.dispatchEvent; | ||
|
|
||
| globalThis.dispatchEvent = dispatchEvent; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a safer approach for mocking global objects.
Direct manipulation of globalThis can be risky. Consider using Jest's spyOn for safer global object mocking.
Here's a suggested improvement:
-const dispatchEvent = jest.fn();
-const originalDispatchEvent = globalThis.dispatchEvent;
-globalThis.dispatchEvent = dispatchEvent;
+const dispatchEvent = jest.spyOn(globalThis, 'dispatchEvent').mockImplementation();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const dispatchEvent = jest.fn(); | |
| const originalDispatchEvent = globalThis.dispatchEvent; | |
| globalThis.dispatchEvent = dispatchEvent; | |
| const dispatchEvent = jest.spyOn(globalThis, 'dispatchEvent').mockImplementation(); |
| const isWriteKeyValid = (writeKey: string) => isString(writeKey) && writeKey.trim().length > 0; | ||
|
|
||
| const isDataPlaneUrlValid = (dataPlaneUrl: string) => isValidURL(dataPlaneUrl); | ||
|
|
||
| export { dispatchSDKEvent, isWriteKeyValid, isDataPlaneUrlValid }; |
There was a problem hiding this comment.
💡 Codebase verification
Based on the results, I can generate the final response as we have clear evidence about the usage of both old and new validation functions.
Migration to new validation functions is incomplete
The old validateLoadArgs is still present in examples/chrome-extension/content-script-v3/foreground.js while the new validation functions (isWriteKeyValid, isDataPlaneUrlValid) are properly implemented in the core Analytics component. This inconsistency needs to be addressed:
examples/chrome-extension/content-script-v3/foreground.js: Update to use the new validation functions instead of the oldvalidateLoadArgs
🔗 Analysis chain
Verify all validation usages are updated
Since these validation functions replace the existing validateLoadArgs, we should verify that all places using the old validation have been updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to validateLoadArgs
rg "validateLoadArgs"
# Search for places where these new functions are used
rg "isWriteKeyValid|isDataPlaneUrlValid"
# Look for any load method implementations that might need these validations
ast-grep --pattern 'load($$$) {
$$$
}'
Length of output: 1967
| @@ -10,11 +12,18 @@ | |||
| const getMutatedError = (err: any, issue: string): Error => { | |||
| let finalError = err; | |||
| if (!isTypeOfError(err)) { | |||
| finalError = new Error(`${issue}: ${stringifyWithoutCircular(err as Record<string, any>)}`); | |||
| finalError = new Error(`${issue}: ${stringifyData(err as Record<string, any>)}`); | |||
There was a problem hiding this comment.
Add test coverage for non-Error object serialization
This line handles the critical path of converting non-Error objects into Error instances, but lacks test coverage according to static analysis.
Would you like me to help generate test cases? Here's a suggested test structure:
describe('getMutatedError', () => {
it('should handle non-Error objects correctly', () => {
const nonError = { custom: 'error' };
const result = getMutatedError(nonError, 'Test Issue');
expect(result).toBeInstanceOf(Error);
expect(result.message).toContain('Test Issue');
expect(result.message).toContain('"custom":"error"');
});
});🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 15-15: packages/analytics-js-common/src/utilities/errors.ts#L15
Added line #L15 was not covered by tests
| @@ -31,7 +31,7 @@ | |||
| }; | |||
|
|
|||
| try { | |||
| const blobPayload = json.stringifyWithoutCircular(data, true); | |||
| const blobPayload = json.stringifyData(data); | |||
There was a problem hiding this comment.
Add test coverage for the modified line
The static analysis indicates that line 34 using stringifyData is not covered by tests. This is particularly important as this function handles batch event delivery payload creation.
Would you like me to help generate test cases that cover:
- Success scenario with valid event data
- Edge cases with special characters or large payloads
- Error scenarios when stringification fails
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-34: packages/analytics-js-plugins/src/beaconQueue/utilities.ts#L34
Added line #L34 was not covered by tests
| } else if ((error as any).error?.stack?.includes(MANUAL_ERROR_IDENTIFIER)) { | ||
| this.logger?.error('An unknown error occurred:', (error as ErrorEvent).error?.message); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and error handling consistency
The current implementation has several areas for improvement:
- Type safety issues with unsafe casting and deep optional chaining
- Generic error message that could be more informative
- Inconsistent error handling compared to other paths
Consider applying these improvements:
- } else if ((error as any).error?.stack?.includes(MANUAL_ERROR_IDENTIFIER)) {
- this.logger?.error('An unknown error occurred:', (error as ErrorEvent).error?.message);
+ } else if (error instanceof ErrorEvent && error.error?.stack?.includes(MANUAL_ERROR_IDENTIFIER)) {
+ const errorContext = 'Manual Error Handler';
+ const errorMessage = error.error?.message || 'No error message provided';
+ this.logger?.error(`${errorContext}${LOG_CONTEXT_SEPARATOR}${errorMessage}`);This refactor:
- Adds proper type checking with
instanceof - Maintains consistency with other error message formatting
- Provides better context in the error message
- Handles cases where error message might be undefined
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if ((error as any).error?.stack?.includes(MANUAL_ERROR_IDENTIFIER)) { | |
| this.logger?.error('An unknown error occurred:', (error as ErrorEvent).error?.message); | |
| } | |
| } else if (error instanceof ErrorEvent && error.error?.stack?.includes(MANUAL_ERROR_IDENTIFIER)) { | |
| const errorContext = 'Manual Error Handler'; | |
| const errorMessage = error.error?.message || 'No error message provided'; | |
| this.logger?.error(`${errorContext}${LOG_CONTEXT_SEPARATOR}${errorMessage}`); | |
| } |
| const DATA_PLANE_URL_VALIDATION_ERROR = (context: string, dataPlaneUrl: string): string => | ||
| `${context}${LOG_CONTEXT_SEPARATOR}The data plane URL "${dataPlaneUrl}" is invalid. It must be a valid URL string. Please check that the data plane URL is correct and try again.`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider keeping dataPlaneUrl parameter optional.
Based on previous learnings, the dataPlaneUrl parameter should use string | undefined type instead of required string to maintain consistency with the established pattern and handle cases where the URL might be undefined.
-const DATA_PLANE_URL_VALIDATION_ERROR = (context: string, dataPlaneUrl: string): string =>
+const DATA_PLANE_URL_VALIDATION_ERROR = (context: string, dataPlaneUrl: string | undefined): string =>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const DATA_PLANE_URL_VALIDATION_ERROR = (context: string, dataPlaneUrl: string): string => | |
| `${context}${LOG_CONTEXT_SEPARATOR}The data plane URL "${dataPlaneUrl}" is invalid. It must be a valid URL string. Please check that the data plane URL is correct and try again.`; | |
| const DATA_PLANE_URL_VALIDATION_ERROR = (context: string, dataPlaneUrl: string | undefined): string => | |
| `${context}${LOG_CONTEXT_SEPARATOR}The data plane URL "${dataPlaneUrl}" is invalid. It must be a valid URL string. Please check that the data plane URL is correct and try again.`; |
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/analytics-js-common/__tests__/utilities/json.test.ts (4)
Line range hint
251-320: Remove duplicate test suite forstringifyDataThe test suite for
stringifyDataappears twice in the file. The second occurrence (lines 251-320) is an exact duplicate of the first test suite (lines 7-74). Please remove the duplicate test suite to maintain code clarity and prevent redundant test execution.
83-107: Improve test description and add edge casesThe test description "should sanitize json without excluding null and undefined values" doesn't fully reflect what's being tested. The test also verifies handling of function values and nested objects.
Consider:
- Renaming the test to better reflect its coverage: "should preserve all value types including functions, null, and undefined"
- Adding edge cases for arrow functions with and without return values
109-179: Consolidate warning message assertionsThe test has multiple similar assertions for warning messages that could be consolidated using a more maintainable approach.
Consider refactoring the warning assertions:
- expect(mockLogger.warn).toHaveBeenCalledTimes(6); - - expect(mockLogger.warn).toHaveBeenNthCalledWith( - 1, - 'JSON:: A bad data (like circular reference, BigInt) has been detected in the object and the property "a" has been dropped from the output.', - ); - // ... more similar assertions ... + const expectedWarnings = [ + 'a', 'e', 'i', 'myself3', 'myself2', 'myself' + ].map(prop => + `JSON:: A bad data (like circular reference, BigInt) has been detected in the object and the property "${prop}" has been dropped from the output.` + ); + + expect(mockLogger.warn).toHaveBeenCalledTimes(expectedWarnings.length); + expectedWarnings.forEach((warning, index) => { + expect(mockLogger.warn).toHaveBeenNthCalledWith(index + 1, warning); + });
234-247: Add more edge cases for data type handlingWhile the test covers basic data types, it would be beneficial to add more edge cases.
Consider adding test cases for:
- Empty arrays
- Arrays with mixed types
- Objects with Symbol keys
- Objects with getter/setter properties
- Regular expressions
- Error objects
- Map and Set objects
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/analytics-js-common/__tests__/utilities/json.test.ts(1 hunks)packages/analytics-js/.size-limit.mjs(1 hunks)
🔇 Additional comments (2)
packages/analytics-js/.size-limit.mjs (1)
122-122:
The file explicitly states "DO NOT EDIT the size limits configured in this file. Should you update them, please contact CODEOWNERS." Please address the following:
- Has this change been approved by CODEOWNERS?
- What's causing the size increase? (Is it due to the new
stringifyDataand enhanced error handling?) - Since the PR is titled "chore: temp reverts", is this size limit increase temporary? If so, please add a TODO comment with the timeline for reverting it back.
Let's check the size impact of recent changes:
✅ Verification successful
From analyzing the git history, I can now provide a definitive response about the size limit changes:
The size limit increase from '39 KiB' to '39.5 KiB' is due to reverting back two recent features:
- Error handling for all public APIs (PR feat: add error handling to all public apis #1907)
- Input data sanitization (PR feat: sanitize input data #1902)
These reverts were intentional as indicated by:
- PR title "chore: temp reverts"
- Commit messages explicitly mentioning the reverts
- The changes reintroduce previously removed code like
stringifyWithoutCircular
Since this is a temporary revert of recent features with a planned re-implementation, the size limit increase is justified and has been properly documented in the commit history.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check recent commits affecting the UMD bundle size
# Look for recent changes in relevant files
echo "=== Recent changes in JSON utility and error handling files ==="
git log -p --since="2 weeks ago" -- \
"**/json.ts" \
"**/errors.ts" \
"**/xhrRequestHandler.ts" \
"**/logMessages.ts"
# Check if there are any existing issues or PRs about bundle size
echo "=== Related issues/PRs ==="
gh search issues "repo:rudderlabs/rudder-sdk-js is:open bundle size content script"
Length of output: 31481
Script:
#!/bin/bash
# Let's analyze the size impact of recent changes by checking:
# 1. Size limit history
# 2. Recent changes in package.json (dependencies)
# 3. Recent changes in build configuration
# Check size limit history
echo "=== Size limit history ==="
git log -p -- "packages/analytics-js/.size-limit.mjs"
# Check recent package.json changes
echo "=== Recent package.json changes ==="
git log -p --since="2 weeks ago" -- "packages/analytics-js/package.json"
# Check recent build config changes
echo "=== Recent build config changes ==="
git log -p --since="2 weeks ago" -- \
"packages/analytics-js/rollup.config.js" \
"packages/analytics-js/rollup.config.mjs" \
"packages/analytics-js/webpack.config.js" \
"packages/analytics-js/tsconfig.json"
Length of output: 27481
packages/analytics-js-common/__tests__/utilities/json.test.ts (1)
1-1: Verify PR intention
The PR is titled "chore: temp reverts" but the changes appear to introduce new functionality (stringifyData and getSanitizedValue) rather than reverting changes. Could you please clarify if this is intentional?
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/analytics-js-common/src/utilities/json.ts (3)
Line range hint
29-39: Consider restoring circular reference detectionThe function only handles BigInt values, but the removed
getCircularReplaceralso handled circular references. This could lead to stack overflow errors if circular references are present in the data.Consider adding circular reference detection:
const getReplacer = (logger?: ILogger): ((key: string, value: any) => any) => { + const ancestors = new WeakSet(); // Use WeakSet for better memory management function replacer(key, value): any { if (isBigInt(value)) { logger?.warn(BAD_DATA_WARNING(JSON_UTIL, key)); return '[BigInt]'; } + + if (typeof value === 'object' && value !== null) { + if (ancestors.has(value)) { + logger?.warn(BAD_DATA_WARNING(JSON_UTIL, key)); + return '[Circular Reference]'; + } + ancestors.add(value); + } return value; } return replacer; };
Line range hint
41-66: Add depth limit protection to prevent stack overflowThe recursive traversal could potentially hit maximum call stack with deeply nested objects.
Consider adding a depth limit:
-const traverseWithThis = (obj: any, replacer: (key: string, value: any) => any): any => { +const traverseWithThis = ( + obj: any, + replacer: (key: string, value: any) => any, + depth: number = 0, + maxDepth: number = 100 +): any => { + if (depth >= maxDepth) { + return '[Max Depth Exceeded]'; + } const result = Array.isArray(obj) ? [] : {}; for (const key in obj) { if (Object.hasOwnProperty.call(obj, key)) { const value = obj[key]; const sanitizedValue = replacer.call(obj, key, value); if (isObjectLiteralAndNotNull(sanitizedValue) || Array.isArray(sanitizedValue)) { - (result as any)[key] = traverseWithThis(sanitizedValue, replacer); + (result as any)[key] = traverseWithThis(sanitizedValue, replacer, depth + 1, maxDepth); } else { (result as any)[key] = sanitizedValue; } } } return result; };
Line range hint
73-83: Enhance function documentationWhile the function has JSDoc comments, it could benefit from more detailed documentation about:
- Expected input types and their handling
- Examples of sanitized output
- Warning about potential limitations
Consider expanding the documentation:
/** * Recursively traverses an object similar to JSON.stringify, * sanitizing BigInts and circular references + * + * @example + * getSanitizedValue({ n: 42n, circular: {} }) // { n: '[BigInt]', circular: {} } + * * @param value Input object * @param logger Logger instance + * @throws {Error} If maximum recursion depth is exceeded * @returns Sanitized value */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/analytics-js-common/__tests__/utilities/json.test.ts(1 hunks)packages/analytics-js-common/src/utilities/json.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js-common/tests/utilities/json.test.ts
🧰 Additional context used
📓 Learnings (1)
packages/analytics-js-common/src/utilities/json.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-common/src/utilities/json.ts:9-27
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In `packages/analytics-js-common/src/utilities/json.ts`, the `stringifyData` function assumes that data is sanitized beforehand, so additional error handling within this function is not necessary.
🔇 Additional comments (1)
packages/analytics-js-common/src/utilities/json.ts (1)
Line range hint 18-27: Verify data sanitization before using stringifyData
Based on previous learnings, this function assumes data is pre-sanitized. However, there's no runtime validation to ensure this assumption holds true. Consider adding a warning in the documentation about this requirement.
Let's check if there are any direct usages that might pass unsanitized data:
|
Hello! This PR has been open for 20 days without any activity. Therefore, it's considered as stale and is scheduled to be closed in 10 days. If you're still working on this, please remove the 'Stale' label or add a comment to keep it open. Thanks for your contribution! |
PR Description
Please include a summary of the change along with the relevant motivation and context.
Linear task (optional)
Linear task link
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
stringifyDataandgetSanitizedValuefunctions for improved JSON serialization and sanitization.dispatchErrorEventfunction.BAD_DATA_WARNING.Bug Fixes
BigInthandling.Chores
stringifyWithoutCircularwithstringifyData, improving code clarity and maintainability.