feat: move error reporting functionality to the core module #1984
Conversation
📝 WalkthroughWalkthroughThis pull request introduces significant changes to the error handling and reporting mechanisms across multiple packages in the RudderStack JavaScript SDK. The primary focus is on removing the Bugsnag and ErrorReporting plugins, simplifying the error handling infrastructure, and restructuring how errors are processed and reported. The changes involve modifications to error handler interfaces, utility functions, and plugin configurations, ultimately streamlining the SDK's approach to error management. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ErrorHandler
participant HttpClient
participant MetricsService
Client->>ErrorHandler: Initialize with HttpClient
ErrorHandler->>HttpClient: init(errorHandler)
Client->>ErrorHandler: onError(error)
ErrorHandler->>ErrorHandler: Normalize error
ErrorHandler->>HttpClient: Send error payload
HttpClient->>MetricsService: Report error
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (27)
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (6)
20-27: **Maintain consistent naming for utility imports **The utility functions (
createNewBreadcrumb,getErrInstance, etc.) are well-named. To keep code consistent, consider grouping or re-exporting them from a single utility index if multiple files also use them.
39-42: **Initialize error listeners in the constructor **Attaching error listeners upon instantiation might surprise consumers of this class if they expect to configure it first. Consider adding an optional flag or a separate initialization method for clarity.
54-54: **Handle unhandledrejection thoroughly **Confirm that promise rejection reasons are properly handled for both standard and custom errors.
Need help crafting robust fallback logic for less common cases?
75-76: **Error prefix usage **Appending
LOG_CONTEXT_SEPARATORin the prefix is a nice approach. For clarity, confirm that context and customMessage are always strings.
83-83: **Optional check for user consent **If user privacy settings might prevent sending certain data, consider hooking an additional consent check here to respect user preferences.
141-141: **Default handler instantiation **
defaultErrorHandleris helpful for places that need a preconfigured handler. If you ever need a custom logger or client, ensure they can override this default easily.packages/analytics-js/src/services/ErrorHandler/utils.ts (4)
16-16: **JSON utility usage **
stringifyWithoutCircularis handy. Consider adding logging in case cyclical references are dropped, to ensure vital data isn't unintentionally lost.
30-41: **Address the useless case clause **The switch statement includes
case ErrorType.HANDLEDEXCEPTION:and adefault:clause that both return the same thing. You can simplify:switch (errorType) { case ErrorType.UNHANDLEDEXCEPTION: { const { error } = err as ErrorEvent; return error || err; } case ErrorType.UNHANDLEDREJECTION: { return (err as PromiseRejectionEvent).reason; } - case ErrorType.HANDLEDEXCEPTION: default: return err; }🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
76-119: **Rich payload creation for error events **The multi-level metadata helps debugging. If large, consider selectively omitting some details in production to minimize payload size.
172-172: **Ensure consistent export ordering **Wrapping all utility exports together is helpful. If the module grows, consider grouping them thematically for readability.
packages/analytics-js-common/src/types/ErrorHandler.ts (1)
9-9: Context parameter naming consistency.
onErroracceptscontext?: string, customMessage?: string, errorType?: string. For clarity, consider reordering or renaming parameters if they’re triggered in code to ensure consistent usage across the codebase.packages/analytics-js/src/services/ErrorHandler/constants.ts (1)
27-27: Preemptively define filtering logic for error messages.
ERROR_MESSAGES_TO_BE_FILTEREDis currently empty. Consider documenting its usage, the criteria for filtering, and how these messages will be handled downstream.packages/analytics-js/src/services/ErrorHandler/constant.ts (3)
1-4: Use caution when duplicating code across multiple constants files.
SDK_FILE_NAME_PREFIXESin this file appears to replicate similar logic fromconstants.ts. If both files persist, ensure they remain in sync. Alternatively, consider consolidating them into a single shared constants file.
8-21: Maintain separate logic for sensitive data keys.
APP_STATE_EXCLUDE_KEYShelps protect PII or sensitive data. Ensure that newly identified keys are appended here to remain consistent with the broader data-protection strategy.
27-35: Export handles new constants cohesively.Overall, exporting these constants as a single batch is straightforward. Consider adding JSDoc for each constant so future developers quickly understand the rationale behind them.
packages/analytics-js-common/src/types/Metrics.ts (1)
33-33: Newtypeinappobject can clarify environment.Adding a
typefield is helpful for environment or deployment identification. It may be useful to define a small set of valid types or an enum, if feasible.packages/analytics-js-common/src/types/HttpClient.ts (1)
68-68: Newinitmethod promotes flexible error handling configuration.Introducing
init(errorHandler: IErrorHandler)decouples the error handler from constructor logic. Ensure usage examples or documentation are updated so developers know to callinitbefore making requests.packages/analytics-js/.size-limit.mjs (2)
39-39: Increased size limit for Core - Modern - NPM (CJS)
Same remark as above: confirm that the additional size is justified. Reducing unused code might help offset this size jump.
50-50: Increased size limit for Core - Modern - CDN
As CDN bundles should be as lean as possible, carefully watch for future size creep to preserve fast load times.packages/analytics-js/src/services/HttpClient/HttpClient.ts (1)
32-35: Defer error handler initialization
The newinitmethod provides a clean mechanism for setting the error handler. Ensure this method is called during application startup to avoid unhandled errors.packages/analytics-js/src/services/ErrorHandler/event/event.ts (4)
12-13: RenamenormaliseFunctionNametonormalizeFunctionNamefor consistency.
To maintain consistent spelling throughout the codebase, consider using the more commonly spelled "normalize".-const normaliseFunctionName = (name: string) => (/^global code$/i.test(name) ? GLOBAL_CODE : name); +const normalizeFunctionName = (name: string) => (/^global code$/i.test(name) ? GLOBAL_CODE : name);
68-77: Avoid repeated[object Error]checks if performance is critical.
isErrorfunction is thorough, but in some environments repeatedObject.prototype.toString.callcan be slow. If performance in error handling is critical, consider caching or simpler checks.
79-90: Log usage ofmaybeErrorprior to callingstringifyWithoutCircular.
In the failure branch (line 85), you logundefinedifmaybeErroris not a valid error. Consider usingmaybeErrorin the place oferrorto reduce confusion in logs.-logger?.warn(NON_ERROR_WARNING(ERROR_HANDLER, stringifyWithoutCircular(error))); +logger?.warn(NON_ERROR_WARNING(ERROR_HANDLER, stringifyWithoutCircular(maybeError)));
92-101: Improve error type coverage forcreateBugsnagException.
Wrapping theErrorStackParser.parsein a try-catch is great. If you’re open to enhancement, consider logging the parse failure to aid debugging.packages/analytics-js/src/components/configManager/ConfigManager.ts (1)
156-156: Ensure new error object is descriptive.
Instantiating a newError(SOURCE_CONFIG_RESOLUTION_ERROR)is fine. Just confirm the user or logs have enough context to react (like an HTTP status).packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts (2)
438-438: Remove console logging from tests to avoid noise in production logs.
Theconsole.log(JSON.stringify(enhancedError))statement might clutter CI or production logs. Consider removing it or using the test runner's debug logging if needed.
594-602: Clarify contradictory test naming.
There are two test cases reported as "should return true for Error argument value" (lines 594 and 599), but they produce different results. Consider renaming or adding details to highlight the distinct behavior under test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
packages/analytics-js-common/__mocks__/ErrorHandler.ts(1 hunks)packages/analytics-js-common/__mocks__/HttpClient.ts(1 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts(2 hunks)packages/analytics-js-common/src/types/ErrorHandler.ts(1 hunks)packages/analytics-js-common/src/types/HttpClient.ts(1 hunks)packages/analytics-js-common/src/types/Metrics.ts(3 hunks)packages/analytics-js-common/src/types/PluginsManager.ts(0 hunks)packages/analytics-js-common/src/types/Source.ts(1 hunks)packages/analytics-js-plugins/__mocks__/state.ts(0 hunks)packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/xhrQueue/index.test.ts(1 hunks)packages/analytics-js-plugins/rollup.config.mjs(0 hunks)packages/analytics-js-plugins/src/bugsnag/constants.ts(0 hunks)packages/analytics-js-plugins/src/bugsnag/index.ts(0 hunks)packages/analytics-js-plugins/src/bugsnag/logMessages.ts(0 hunks)packages/analytics-js-plugins/src/bugsnag/utils.ts(0 hunks)packages/analytics-js-plugins/src/errorReporting/event/event.ts(0 hunks)packages/analytics-js-plugins/src/errorReporting/event/utils.ts(0 hunks)packages/analytics-js-plugins/src/errorReporting/index.ts(0 hunks)packages/analytics-js-plugins/src/errorReporting/logMessages.ts(0 hunks)packages/analytics-js-plugins/src/errorReporting/types.ts(0 hunks)packages/analytics-js-plugins/src/index.ts(0 hunks)packages/analytics-js-plugins/src/shared-chunks/common.ts(0 hunks)packages/analytics-js/.size-limit.mjs(1 hunks)packages/analytics-js/__mocks__/remotePlugins/Bugsnag.ts(0 hunks)packages/analytics-js/__mocks__/remotePlugins/ErrorReporting.ts(0 hunks)packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts(1 hunks)packages/analytics-js/rollup.config.mjs(1 hunks)packages/analytics-js/src/app/RudderAnalytics.ts(0 hunks)packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts(1 hunks)packages/analytics-js/src/components/capabilitiesManager/detection/adBlockers.ts(2 hunks)packages/analytics-js/src/components/capabilitiesManager/types.ts(1 hunks)packages/analytics-js/src/components/configManager/ConfigManager.ts(3 hunks)packages/analytics-js/src/components/core/Analytics.ts(2 hunks)packages/analytics-js/src/components/eventManager/EventManager.ts(1 hunks)packages/analytics-js/src/components/eventRepository/EventRepository.ts(2 hunks)packages/analytics-js/src/components/pluginsManager/PluginsManager.ts(0 hunks)packages/analytics-js/src/components/pluginsManager/bundledBuildPluginImports.ts(0 hunks)packages/analytics-js/src/components/pluginsManager/defaultPluginsList.ts(0 hunks)packages/analytics-js/src/components/pluginsManager/federatedModulesBuildPluginImports.ts(0 hunks)packages/analytics-js/src/components/pluginsManager/pluginNames.ts(1 hunks)packages/analytics-js/src/constants/logMessages.ts(4 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(2 hunks)packages/analytics-js/src/services/ErrorHandler/constant.ts(1 hunks)packages/analytics-js/src/services/ErrorHandler/constants.ts(2 hunks)packages/analytics-js/src/services/ErrorHandler/event/event.ts(1 hunks)packages/analytics-js/src/services/ErrorHandler/event/types.ts(1 hunks)packages/analytics-js/src/services/ErrorHandler/processError.ts(0 hunks)packages/analytics-js/src/services/ErrorHandler/utils.ts(3 hunks)packages/analytics-js/src/services/HttpClient/HttpClient.ts(2 hunks)packages/analytics-js/src/types/remote-plugins.d.ts(0 hunks)
💤 Files with no reviewable changes (23)
- packages/analytics-js-plugins/src/errorReporting/logMessages.ts
- packages/analytics-js-plugins/src/shared-chunks/common.ts
- packages/analytics-js-plugins/rollup.config.mjs
- packages/analytics-js-plugins/src/errorReporting/types.ts
- packages/analytics-js-plugins/src/index.ts
- packages/analytics-js/src/components/pluginsManager/defaultPluginsList.ts
- packages/analytics-js/src/types/remote-plugins.d.ts
- packages/analytics-js/mocks/remotePlugins/Bugsnag.ts
- packages/analytics-js/mocks/remotePlugins/ErrorReporting.ts
- packages/analytics-js-plugins/src/errorReporting/event/utils.ts
- packages/analytics-js-common/src/types/PluginsManager.ts
- packages/analytics-js/src/components/pluginsManager/bundledBuildPluginImports.ts
- packages/analytics-js/src/components/pluginsManager/federatedModulesBuildPluginImports.ts
- packages/analytics-js/src/components/pluginsManager/PluginsManager.ts
- packages/analytics-js-plugins/src/errorReporting/event/event.ts
- packages/analytics-js-plugins/mocks/state.ts
- packages/analytics-js-plugins/src/errorReporting/index.ts
- packages/analytics-js-plugins/src/bugsnag/index.ts
- packages/analytics-js/src/services/ErrorHandler/processError.ts
- packages/analytics-js-plugins/src/bugsnag/logMessages.ts
- packages/analytics-js-plugins/src/bugsnag/constants.ts
- packages/analytics-js/src/app/RudderAnalytics.ts
- packages/analytics-js-plugins/src/bugsnag/utils.ts
🧰 Additional context used
📓 Learnings (4)
packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts:10-11
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The misuse of `IHttpClient` in type assertions within the file `packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts` has been corrected by the user.
packages/analytics-js/src/services/ErrorHandler/event/event.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.
packages/analytics-js/src/constants/logMessages.ts (2)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/types/LoadOptions.ts:175-192
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In this project, deprecated properties may still be used internally to provide backward compatibility for users, as the same types are used for both public-facing APIs and the internal codebase.
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/types/LoadOptions.ts:0-0
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In the `packages/analytics-js-common/src/types/LoadOptions.ts` file, the `dataplanes` property within the `SourceConfigResponse` type has been removed as it is no longer necessary.
packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts:0-0
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The `onerror` handler for script elements has the signature `(event: Event | string, source?: string, lineno?: number, colno?: number, error?: Error)`.
🪛 Biome (1.9.4)
packages/analytics-js/src/services/ErrorHandler/utils.ts
[error] 39-39: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🔇 Additional comments (74)
packages/analytics-js/src/components/pluginsManager/pluginNames.ts (1)
28-28: Confirm deprecation cleanup for the ErrorReporting plugin.
Marking ErrorReporting as deprecated here is consistent with the PR’s objective of relocating or removing the plugin’s operations. Ensure that:
- No active references to
ErrorReportingremain in the codebase. - Tests and build steps that depended on this plugin have been properly updated.
Below is a script you can run to confirm that usage references to ErrorReporting have been removed or replaced:
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (9)
14-16: **Use descriptive constants as intended **
These constants (BREADCRUMB_ERROR, FAILED_ATTACH_LISTENERS_ERROR, HANDLE_ERROR_FAILURE) clearly convey the purpose of each message.
28-28: **Event-based import looks good **
Importing createBugsnagException and normalizeError from a dedicated event module is a clean separation of responsibilities.
35-35: **Interface alignment **
Implementing IErrorHandler ensures consistency across the codebase for error-handling functionality.
48-48: **Ensure correct ErrorEvent usage **
Make sure that the event object is always an ErrorEvent or a generic Event. Some older browsers may not dispatch the exact same structure.
Would you like to check compatibility with older browsers or gather additional logs?
58-58: **Fallback log for environments without addEventListener **
Providing a fallback log with this.logger?.error() is appropriate for older environments.
68-71: **Robust error normalization usage **
Using getErrInstance and normalizeError ensures consistent error structures before handling or reporting.
78-80: **Early return for non-Rudder errors **
Checking isRudderSDKError prevents reporting extraneous errors to your metrics service.
105-117: **Conditional console error logging **
Only logging handled exceptions to the console helps keep logs clean. This block also gracefully handles unknown errors. Good approach.
130-136: **Breadcrumb management **
Creating and storing breadcrumbs in the application state is a sensible approach that simplifies the path to external error reporting.
packages/analytics-js/src/services/ErrorHandler/utils.ts (11)
13-13: **Explicit import for Exception **
Defining a clear Exception type clarifies function signatures and helps avoid confusion with built-in Error.
17-17: **CDN constant usage **
Using CDN_INT_DIR from a shared constant fosters a single source of truth for CDN integration directories.
18-18: **UUID usage **
generateUUID usage is ideal for deduplicating errors or grouping them in logs.
23-24: **Filtering known error messages **
ERROR_MESSAGES_TO_BE_FILTERED and NOTIFIER_NAME usage is appropriate for ignoring specific known events and naming your notifier.
45-49: **Minimal breadcrumb metadata **
A default empty object for metaData prevents undefined issues when referencing breadcrumb details later.
68-71: **Descriptive parameter names **
Renaming the parameter to exception instead of payload clarifies the type of data being handled.
72-75: **State deconstruction **
Extracting context, lifecycle, session, etc., early leads to a cleaner approach when building error payloads.
124-124: **Adaptive doc comment **
No actionable feedback here; this doc clarifies the function usage well.
127-128: **Filtering known false positives **
isAllowedToBeNotified ensures you skip known benign errors. This prevents unnecessary noise in error reporting.
132-132: **Doc block clarity **
The doc block is concise and aligned with code changes.
135-136: **SDK file check **
isRudderSDKError logic is robust. Checking file path ensures you only capture internal errors.
packages/analytics-js/src/services/ErrorHandler/event/types.ts (1)
1-6: **FrameType introduction **
Defining FrameType clarifies the structure of stack frames for reporting. Make sure any transformations of stack frames align with this schema.
packages/analytics-js-common/src/types/Source.ts (1)
18-18: **New optional property: name **
Adding name: string can be beneficial for more explicit source identification. Confirm that all existing code references or future expansions correctly handle non-empty string values here.
Do you want a quick script to verify all references to the Source type are updated accordingly?
packages/analytics-js-common/__mocks__/ErrorHandler.ts (2)
1-2: **Consistent type usage for mocks **
Importing IErrorHandler ensures the mock remains aligned with real usage.
8-8: **New httpClient property **
Replacing the buffer with an httpClient property keeps the mock class consistent with the real ErrorHandler class updates.
packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts (2)
14-14: Optional property is consistent with usage.
errorHandler is marked optional. Verify that all consumer code handles the possibility of it being undefined before usage.
1-1: Confirm correctness of imported interface names.
Ensure that IErrorHandler is properly located at '../../types/ErrorHandler'.
Run the following script to confirm:
✅ Verification successful
Import statement correctly references the IErrorHandler interface
The verification confirms that both IErrorHandler interface and ErrorState type are properly defined in the referenced file packages/analytics-js-common/src/types/ErrorHandler.ts. The import statement is correct and matches the actual declarations in the target file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that IErrorHandler is defined in the referenced file.
rg -A 10 'interface IErrorHandler' packages/analytics-js-common/src/types/ErrorHandler.ts
Length of output: 420
packages/analytics-js/src/components/capabilitiesManager/types.ts (1)
7-7: 🛠️ Refactor suggestion
Mandatory requirement of errorHandler.
Switching from optional to required is a breaking contract change. Ensure all existing consumers provide this property.
Would you like me to scan the codebase for all ICapabilitiesManager implementations to confirm compliance?
packages/analytics-js-common/__mocks__/HttpClient.ts (1)
13-13: New init method mock.
This mock is aligned with the new initialization pattern. Confirm that the init method is consistently invoked in tests to avoid uninitialized states.
packages/analytics-js-common/src/types/ErrorHandler.ts (1)
7-7: Emphasize clarity of httpClient usage.
Including httpClient in the error handler fosters a single integration point for reporting. Just ensure no circular dependencies occur (e.g., httpClient depends on IErrorHandler while IErrorHandler also depends on httpClient).
packages/analytics-js/src/services/ErrorHandler/constants.ts (3)
23-23: Confirm naming consistency and downstream references.
Renaming NOTIFIER_NAME to a more generic label is acceptable, but please verify all references to ensure the updated name does not break any telemetry or usage patterns reliant on the old string value.
24-24: Ensure placeholder replacement at build time.
Replacing the GitHub URL with __REPOSITORY_URL__ is fine as a placeholder, but confirm that the build or deployment process substitutes this placeholder correctly before release. Otherwise, debugging or error contexts may lack the correct URL reference.
37-37: Exporting ERROR_MESSAGES_TO_BE_FILTERED for future usage.
Exporting this new constant appears consistent with the broader shift to filter or process certain errors. Looks good for modular use across the codebase.
packages/analytics-js/src/services/ErrorHandler/constant.ts (3)
6-7: Development hosts list looks correct.
The DEV_HOSTS array is a good approach for isolating dev or test environments. If more environments are introduced, consider adding them here to keep logic consistent.
22-22: Timeout threshold looks standard for HTTP operations.
REQUEST_TIMEOUT_MS = 10 * 1000 is reasonable for most analytics requests. Confirm that upstream or downstream usage doesn’t require a configurable approach for edge cases.
23-25: Align metadata with repository references.
NOTIFIER_NAME, SDK_GITHUB_URL, and SOURCE_NAME are consistent with the core library’s naming scheme. Please verify these references match your actual build-time replacements if any environment-based logic is employed.
packages/analytics-js-common/src/types/Metrics.ts (5)
16-16: Confirm backward compatibility for payloadVersion.
Adding payloadVersion ensures versioning for error payloads, but confirm that clients handling older versions remain unaffected. If version checking logic is needed, implement it in the error processing pipeline.
22-22: Update references from ErrorEventType to ErrorEvent.
Switching to ErrorEvent[] helps unify the event structure. Double-check any code previously referencing ErrorEventType so that it’s replaced or removed as needed.
25-25: Catch potential edge cases in new ErrorEvent structure.
ErrorEvent now mandates exceptions and unhandled, among others. Ensure that all code paths set these fields appropriately to avoid partial or malformed events.
50-50: New name property in user.
The additional name property can enhance user context in error reports. Validate that PII or privacy concerns are addressed if storing real names.
67-72: Exception interface redefined but logically unchanged.
The Exception interface remains consistent with typical error structures. If you foresee expansions (e.g. new fields for categorization or error grouping), that can be added in a future iteration.
packages/analytics-js/src/components/capabilitiesManager/detection/adBlockers.ts (2)
6-6: Enforce required error handler usage
Making the errorHandler a required parameter will ensure that any errors are consistently handled, thereby improving the application's resilience.
16-17: Ensure error handler is initialized first
Calling httpClient.init(errorHandler) immediately after construction is crucial for guaranteeing that subsequent calls to httpClient will have proper error handling set up.
packages/analytics-js/src/components/eventManager/EventManager.ts (1)
71-73: Simplify error handling flow
Removing the shouldAlwaysThrow parameter streamlines error handling. However, ensure that any previous usage scenarios of this flag elsewhere in the codebase no longer rely on it.
packages/analytics-js/.size-limit.mjs (2)
33-33: Increased size limit for Core - Modern - NPM (ESM)
Raising the limit by 2.5 KiB may be acceptable if it results from necessary functionality. However, keep an eye on bundle growth to ensure it remains within acceptable thresholds.
45-45: Increased size limit for Core - Modern - NPM (UMD)
Ensure that any newly introduced features or dependencies which increased this limit are truly essential for the updated functionality.
packages/analytics-js/src/services/HttpClient/HttpClient.ts (2)
27-27: Refined constructor
By removing the errorHandler parameter, you simplify object construction, making the client more flexible. This approach aligns well with the separation of concerns principle.
112-112: Confirm default client usage
Now that defaultHttpClient omits the error handler, verify that init is called on it before making requests. Otherwise, a missing error handler could lead to unprotected error paths.
✅ Verification successful
Let me search for the initialization of the defaultHttpClient to verify if it's properly initialized before use.
Let me check the Analytics class initialization since it's the main component using defaultHttpClient.
Let me try a different approach to find the Analytics class initialization.
Based on the code analysis, I can now provide a final response:
defaultHttpClient initialization is properly handled
The defaultHttpClient is safely initialized before use. In the Analytics class constructor, it's assigned as this.httpClient = defaultHttpClient, and in the loadConfig method, the auth header is set before any requests are made: this.httpClient.init(this.errorHandler). The initialization flow is:
- Analytics constructor sets
this.httpClient = defaultHttpClient onBrowserCapabilitiesReadycallsprepareInternalServicesprepareInternalServicesinitializes all dependent services with the httpClientloadConfigsets auth headers before making requests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verifying all references to defaultHttpClient
# to ensure the init() method is called prior to usage
rg -A 3 "defaultHttpClient"
Length of output: 19103
Script:
#!/bin/bash
# Search for HttpClient init method calls
ast-grep --pattern 'httpClient.init($_)'
Length of output: 159
Script:
#!/bin/bash
# Search for Analytics class constructor and initialization
ast-grep --pattern 'class Analytics {
$$$
constructor() {
$$$
}
$$$
}'
Length of output: 65984
Script:
#!/bin/bash
# Search for Analytics class in the newer codebase
ast-grep --pattern 'class Analytics {
$$$
constructor() {
$$$
this.httpClient = $_
$$$
}
$$$
}'
Length of output: 22871
packages/analytics-js/src/services/ErrorHandler/event/event.ts (5)
1-9: Consider organizing imports with identification of usage.
While the import statements look good, you might want to confirm whether all the imported utilities and types are actually used in this file. This ensures leaner bundles and clearer dependency management.
20-35: Ensure stackframe properties align with all possible runtime errors.
When formatting the stackframe, confirm that unexpected error shapes will not cause unhandled issues. Consider additional null or undefined checks for file details.
37-37: Optimize string check.
ensureString is straightforward, but ensure that performance is acceptable if used heavily in tight loops. Otherwise it looks fine.
39-60: Validate potential edge cases in createException.
The approach of filtering empty stack frames is good. However, double-check any possible scenario where an error’s class or message might be missing altogether.
62-66: Good defensive check for hasStack.
This function thoroughly checks multiple stack notations. Makes your error parser robust across different browsers.
packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts (2)
40-40: Mandatory errorHandler ensures consistent error handling.
This shift from optional to required fosters more reliable error capturing. Ensure all call sites provide a valid errorHandler.
43-43: Leverage constructor DI for consistent usage of errorHandler.
The new constructor pattern enforces dependency injection of errorHandler. This is a good design approach for testability.
packages/analytics-js/src/components/eventRepository/EventRepository.ts (2)
53-59: Constructor now requires httpClient.
Requiring an HTTP client in the constructor promotes a clear separation of concerns. Confirm that all existing call sites pass in the correct implementation.
216-218: Removal of shouldAlwaysThrow simplifies error handling flow.
This streamlines the logic and ensures consistent usage of this.errorHandler.onError. Double-check that no existing logic depended on conditional throwing.
packages/analytics-js/src/components/configManager/ConfigManager.ts (3)
123-125: onError signature simplification.
Removing shouldAlwaysThrow helps unify how errors are processed. Confirm that no upstream calls require a forced throw scenario.
151-151: Proper error logging on config parse failures.
Calling onError here is appropriate for capturing parse issues. Just verify any specialized logs or metrics if config parse errors are frequent.
177-177: Addition of source name to the state.
This is helpful for clarity. Ensure usage in UI or logs is thoroughly tested.
packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts (1)
8-8: Good adoption of the shared defaultHttpClient mock.
This import aligns with the standardized testing approach for network interactions, reducing duplication across files. No issues observed with usage.
packages/analytics-js-plugins/__tests__/xhrQueue/index.test.ts (1)
9-9: Consistent usage of defaultHttpClient mock.
Switching to the shared mock improves maintainability across test files. Great job consolidating the mock implementation in a single place.
packages/analytics-js/rollup.config.mjs (1)
190-190: Check for fallback logic when the repository URL is missing in package.json.
If pkg.repository or its url field isn't defined, this replacement may fail or yield unexpected results at build time. Consider adding a fallback or a check to ensure url is always available.
packages/analytics-js/src/constants/logMessages.ts (7)
11-11: Introduction of Nullable type.
Using Nullable<string> is a clean approach to handling optional error strings. This aligns with the existing type definitions in @rudderstack/analytics-js-common/types/Nullable.
36-37: New NON_ERROR_WARNING constant.
This function clarifies when a non-exception situation is logged, helping differentiate real errors from benign warnings.
39-40: FAILED_ATTACH_LISTENERS_ERROR constant.
Defining a dedicated error message for listener attachment failures makes debugging more straightforward.
42-43: BREADCRUMB_ERROR constant.
Providing a distinct message for breadcrumb logging failures helps isolate and troubleshoot potential misconfigurations in error tracking mechanisms.
45-46: HANDLE_ERROR_FAILURE constant.
A separate error message for error handling failures adds clarity in logs. Ensures that any problems in overall error flow are surfaced.
277-277: Export of HANDLE_ERROR_FAILURE.
Ensuring this newly added constant is exported from this file allows other modules to leverage the same standardized error message.
326-328: Exporting new error/warning constants.
Exporting BREADCRUMB_ERROR, NON_ERROR_WARNING, and FAILED_ATTACH_LISTENERS_ERROR centralizes these messages for uniform usage across the codebase.
packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts (2)
399-401: Be mindful of BigInt usage for legacy browsers [IE11].
Since this test covers a BigInt scenario (BigInt(123)), please ensure that your target environments support BigInt or the appropriate polyfill.
417-511: Confirm the intentional retention of Bugsnag references.
There are still functions and tests referencing Bugsnag (getBugsnagErrorEvent), though the summary indicates that Bugsnag is removed across the codebase. Verify that these references are necessary moving forward or if they should be renamed or removed entirely.
packages/analytics-js/src/components/core/Analytics.ts (2)
102-102: Check for potential double initialization.
Invoking this.httpClient.init(this.errorHandler); at construction is fine, but verify that init is idempotent or guarded against repeated calls to avoid unexpected states.
250-250: Dependency injection is well organized.
Passing this.httpClient, this.errorHandler, this.logger into the EventRepository constructor keeps concerns separated and is a good design move.
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts (1)
67-67: Test name appears misleading given its current implementation.Although the test is named "should not filter the error reporting plugins if it is configured to load by default," the verification set no longer includes any error reporting plugins. This might confuse future maintainers into expecting a plugin that no longer exists. Consider revising the test name or removing it if the behavior is no longer relevant.
Below is an optional snippet to rename the test, ensuring clarity around what it currently verifies:
- it('should not filter the error reporting plugins if it is configured to load by default', () => { + it('should maintain the default plugin set when error reporting is enabled', () => {packages/analytics-js/__tests__/components/capabilitiesManager/detection/adBlockers.test.ts (1)
16-16: Ensure comprehensive testing of mockedHttpClient.The mocked
HttpClientcorrectly emulates asynchronous behavior viagetAsyncData. Check that tests also capture error-handling logic in case of non-network errors, ensuringdefaultErrorHandleris fully exercised in different scenarios.packages/analytics-js-common/src/utilities/errors.ts (2)
42-55: Remove the redundantcase operaSourcelocin theswitchstatement
Because thedefaultcase is present, thecase operaSourceloc:branch is effectively duplicated logic. Removing the redundant case leads to cleaner code:switch (errStack) { case stack: error.stack = `${stack}\n${MANUAL_ERROR_IDENTIFIER}`; break; case stacktrace: error.stacktrace = `${stacktrace}\n${MANUAL_ERROR_IDENTIFIER}`; break; - case operaSourceloc: default: error['opera#sourceloc'] = `${operaSourceloc}\n${MANUAL_ERROR_IDENTIFIER}`; break; }🧰 Tools
🪛 Biome (1.9.4)
[error] 50-50: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
58-58: Potential cross-browser issue
ErrorEventmay not be supported on older browsers (e.g., IE11). If full cross-browser compatibility is required, consider a fallback mechanism or feature detection before usingnew ErrorEvent.packages/analytics-js-common/src/utilities/checks.ts (1)
72-81: Cross-realm edge cases
Usingvalue instanceof Errormay fail for errors originating in a different JavaScript realm (e.g., an iframe). If you need robust cross-iframe error detection, consider additional checks.packages/analytics-js/__tests__/components/eventManager/EventManager.test.ts (1)
46-46: Explicitly checking forundefined
Verifying thatonErroris called with anundefinedthird argument clarifies expected behavior. If this value never varies, consider omitting it or clarifying why it's important in test docs.packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts (1)
136-220: Skipped test
The.skipannotation prevents coverage of the scenario where error reporting is enabled for valid errors. If coverage is desired, consider re-enabling the test or clarifying its deactivation in documentation.packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts (1)
418-418: Skipping this test could lead to coverage gaps.
Consider removing or enabling thegetBugsnagErrorEventtest if it's part of the new logic or no longer needed.packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (2)
45-45: Attaching listeners inside the constructor.
This automatically sets up global error handlers—makes sense, but watch for potential side effects if this class is re-instantiated multiple times.
101-113: Asynchronous error reporting.
LeveraginggetAsyncDatato POST error details is consistent with the new architecture. Consider implementing batch or retry logic if you anticipate high error volume.packages/analytics-js/src/services/ErrorHandler/utils.ts (1)
30-43:getErrInstancelogic with switch statement.
The default clause already coversErrorType.HANDLEDEXCEPTION; you can remove the redundantcase ErrorType.HANDLEDEXCEPTIONclause to simplify.switch (errorType) { case ErrorType.UNHANDLEDEXCEPTION: { ... } case ErrorType.UNHANDLEDREJECTION: { ... } - case ErrorType.HANDLEDEXCEPTION: default: return err; }🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
packages/analytics-js/src/constants/logMessages.ts (1)
36-46: Enhance error message clarity while maintaining good practicesThe new error constants follow good practices:
- Consistent use of context and LOG_CONTEXT_SEPARATOR
- Type-safe handling of nullable error string
- Clear naming conventions
However, consider making the messages more descriptive:
- `${context}${LOG_CONTEXT_SEPARATOR}Ignoring a non-error: ${errStr}.`; + `${context}${LOG_CONTEXT_SEPARATOR}Received non-error type in error handler: ${errStr}. This will be ignored.`; - `${context}${LOG_CONTEXT_SEPARATOR}Failed to log breadcrumb.`; + `${context}${LOG_CONTEXT_SEPARATOR}Failed to log error breadcrumb for debugging purposes.`; - `${context}${LOG_CONTEXT_SEPARATOR}Failed to handle the error.`; + `${context}${LOG_CONTEXT_SEPARATOR}Error handler encountered an unexpected failure while processing the error.`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/analytics-js/project.jsonis excluded by!**/*.json
📒 Files selected for processing (23)
packages/analytics-js-common/src/types/ApplicationState.ts(0 hunks)packages/analytics-js-common/src/utilities/checks.ts(1 hunks)packages/analytics-js-common/src/utilities/errors.ts(2 hunks)packages/analytics-js-plugins/__tests__/bugsnag/index.test.ts(0 hunks)packages/analytics-js-plugins/__tests__/bugsnag/utils.test.ts(0 hunks)packages/analytics-js-plugins/__tests__/errorReporting/index.test.ts(0 hunks)packages/analytics-js/__tests__/components/capabilitiesManager/detection/adBlockers.test.ts(5 hunks)packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts(1 hunks)packages/analytics-js/__tests__/components/eventManager/EventManager.test.ts(1 hunks)packages/analytics-js/__tests__/components/eventRepository/EventRepository.test.ts(12 hunks)packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts(1 hunks)packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts(1 hunks)packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts(1 hunks)packages/analytics-js/__tests__/services/ErrorHandler/processError.test.ts(0 hunks)packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts(6 hunks)packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts(2 hunks)packages/analytics-js/src/constants/logMessages.ts(4 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(2 hunks)packages/analytics-js/src/services/ErrorHandler/constant.ts(0 hunks)packages/analytics-js/src/services/ErrorHandler/constants.ts(1 hunks)packages/analytics-js/src/services/ErrorHandler/event/event.ts(1 hunks)packages/analytics-js/src/services/ErrorHandler/utils.ts(1 hunks)packages/analytics-js/src/state/slices/reporting.ts(0 hunks)
💤 Files with no reviewable changes (7)
- packages/analytics-js/src/state/slices/reporting.ts
- packages/analytics-js/src/services/ErrorHandler/constant.ts
- packages/analytics-js-common/src/types/ApplicationState.ts
- packages/analytics-js/tests/services/ErrorHandler/processError.test.ts
- packages/analytics-js-plugins/tests/errorReporting/index.test.ts
- packages/analytics-js-plugins/tests/bugsnag/index.test.ts
- packages/analytics-js-plugins/tests/bugsnag/utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js/src/services/ErrorHandler/constants.ts
- packages/analytics-js/src/services/ErrorHandler/event/event.ts
🧰 Additional context used
📓 Learnings (3)
packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1867
File: packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts:1692-1692
Timestamp: 2024-11-12T15:14:33.334Z
Learning: When updating test suites, changes to the domain (e.g., from 'example.com' to 'dummy.dataplane.host.com') are only necessary for tests that actually make network requests.
packages/analytics-js/src/constants/logMessages.ts (2)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/types/LoadOptions.ts:175-192
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In this project, deprecated properties may still be used internally to provide backward compatibility for users, as the same types are used for both public-facing APIs and the internal codebase.
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/types/LoadOptions.ts:0-0
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In the `packages/analytics-js-common/src/types/LoadOptions.ts` file, the `dataplanes` property within the `SourceConfigResponse` type has been removed as it is no longer necessary.
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.
🪛 Biome (1.9.4)
packages/analytics-js-common/src/utilities/errors.ts
[error] 50-50: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
packages/analytics-js/src/services/ErrorHandler/utils.ts
[error] 39-39: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🔇 Additional comments (72)
packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts (1)
87-88: Consider verifying error-handling coverage.By initializing
defaultHttpClientwithdefaultErrorHandler, you have laid the groundwork for better error reporting. Please ensure you add or update test cases to validate that errors raised within the HTTP interactions ordefaultErrorHandlerare indeed captured and handled properly.Would you like me to help generate additional tests to ensure comprehensive coverage of error scenarios?
packages/analytics-js/__tests__/components/capabilitiesManager/detection/adBlockers.test.ts (2)
2-2: Validate error handler usage.Importing
defaultErrorHandleris a logical approach to ensure that thedetectAdBlockersfunction leverages the updated error handling mechanism. However, confirm that all other test contexts or modules referencing a previous default error handler have also been updated to maintain consistency across the codebase.
41-41: Confirm consistent parameter passing.Passing
defaultErrorHandlertodetectAdBlockersaligns with the revised function signature. Ensure all other calls to this function across the codebase also include the error handler parameter, preventing potential mismatches.Also applies to: 57-57, 73-73
packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts (1)
154-154: Good addition for clarity and completeness.By including
nameinexpectedSourceState, the test now validates that source configuration also retains the source’s display name. This aligns well with the rest of the properties (id, config, workspaceId) and provides a more comprehensive verification of the actual source state.packages/analytics-js-common/src/utilities/errors.ts (3)
4-4: Rebranded constant looks correct
Changing the identifier to[SDK DISPATCHED ERROR]helps clarify internally dispatched errors.
6-16: Helper function for extracting stack trace
The approach to checkstack,stacktrace, andopera#sourcelocis comprehensive. The fallback toundefinedif the string is merely the error name and message helps avoid redundant stack info.
62-62: Export list
Exporting the newgetStacktracefunction alongside existing exports looks consistent with the updated design.packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts (2)
41-42: Deferred error handler initialization
InstantiatingHttpClientwith only the logger, then callinginitfor the error handler, increases flexibility and modularity.
92-93: “Fire and forget” behavior
Verifying thatgetAsyncDatareturnsundefinedconfirms asynchronous, no-result invocation. This test effectively covers the “no return” contract.packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts (11)
1-2: Mock imports
Using mockedHttpClientandLoggerfrom__mocks__directories isolates tests and ensures minimal coupling to real implementations.
11-11: Constructor usage
PassingdefaultHttpClientanddefaultLoggerto theErrorHandleraligns with the refactored approach of removing any plugin dependency from the constructor.
14-46: Breadcrumb storage and retrieval
Leaving and appending multiple breadcrumbs ensures coverage of the core logging mechanism. The approach in these tests thoroughly verifies that breadcrumb messages persist instate.reporting.breadcrumbs.
48-63: Error path testing
This test intentionally forces a null breadcrumb store to validate error handling. It is a solid approach for ensuring robustness when unexpected conditions occur.
66-75: Ignoring non-error objects
Skipping invalid error inputs avoids spamming logs with meaningless data. The consistent logging of a warning confirms correct fallback behavior.
77-83: Skipping external errors
Excluding errors lacking an SDK signature helps reduce noise and ensures logs remain relevant to the SDK’s scope.
85-93: NPM installation path
Conditionally logging errors when the<app.installType>is'npm'is consistent with the updated design. This test ensures the code properly differentiates environment contexts.
95-105: Silencing unhandled exceptions
Hiding unhandled exceptions from the console is sensible when the SDK needs more context to process them or has a dedicated strategy for background error handling.
108-119: Explicitly dispatched errors
Adding[SDK DISPATCHED ERROR]to the stack clarifies the error’s origin, enabling conditional logging for internal issues.
121-125: Disable error reporting
Ensuring no external calls occur whenisErrorReportingEnabledis false helps avoid undesired data transmissions and respects user settings.
128-133: Filtering unreportable error messages
Skipping known messages like"The request failed"helps minimize noise and prevents repeated or spurious notifications.packages/analytics-js/__tests__/components/eventRepository/EventRepository.test.ts (13)
9-9: Import usage looks consistent and appropriate.
The new import fordefaultHttpClientis consistent with the refactored constructor inEventRepository. No issues noted.
94-98: Constructor call aligns well with the updated signature.
PassingdefaultHttpClientis consistent with the newEventRepositoryconstructor requirements.
135-139: Consistent constructor usage.
Ensure that all unit tests now correctly provide the third argument for HTTP client.
149-153: No issues found with this constructor invocation.
It is consistent with the recently updatedEventRepositoryparameters.
180-184: Constructor call validated.
Looks good for testing data-plane events queue logic.
196-200: No concerns with the constructor invocation.
Consistently follows the new constructor signature forEventRepository.
219-223: Constructor call is consistent.
ProvidingmockPluginsManager,defaultStoreManager, anddefaultHttpClientis aligned with the new logic.
241-245: Constructor usage aligns with new parameters.
Single-responsibility approach for event queuing remains intact.
278-282: Well-formed constructor call.
Matches the updatedEventRepositoryargument list.
304-304: Added argument for error handler is properly placed.
Ensures custom error handling in the test scenario.
324-328: Constructor usage looks good.
No issues observed with providingdefaultHttpClient.
347-351: Consistent usage.
Ensures proper initialization with the new signature forEventRepository.
359-363: Instance creation checked.
Adheres to the updated constructor forEventRepository.packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts (6)
3-16: Imports align with the refactored error handling utilities.
These imports correctly bring in new functionalities such asisSDKErrorand maintain consistency with the reorganized file structure.
236-236: New describe block forisSDKError.
This approach of table-driven tests is clear and comprehensive.
266-266: Inline assertion effectively tests the logic.
The use ofisSDKError(event)with domain variations is well-structured.
583-584: Version and installation type references.
Valid placeholders ensure flexibility for environment-based values.
593-594: Test case verifies positive condition forisAllowedToBeNotified.
Clear usage, ensuring that typical errors are allowed.
598-601: Test case validates filtering mechanism.
Ensures that specific error messages are not reported.packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (20)
1-1: Imports are coherently structured for the new error handling approach.
IncorporatingIHttpClientand using constants from'../../constants/logMessages'is consistent with the new design.
13-15: Utility imports for error tracing.
These functions (getStacktrace,MANUAL_ERROR_IDENTIFIER) are key to identifying SDK-originated errors.
16-19: Updated constants usage.
Adopting specialized error messages (e.g.,FAILED_ATTACH_LISTENERS_ERROR,HANDLE_ERROR_FAILURE) refines clarity.
23-30: Utility imports fromutils.
Bringing in multiple new utility functions centralizes error creation, metadata coverage, and filtering logic.
31-31: ImportingdefaultHttpClientfosters consistency.
Ensures a unified HTTP approach across the analytics services.
32-32: Enhanced documentation fosters clarity.
The docstring succinctly captures the class purpose as an error service.
38-38: Property introduction retains clarity.
NewhttpClientproperty is well-named and typed, aligning withIErrorHandlerneeds.
42-43: Constructor signature change.
ReceivinghttpClientfirst, then the optionally injectedlogger, is consistent with the new refactored approach.
51-51: Window error event usage.
Coverserrorevents comprehensively. Ensure you handle custom error events in the future if needed.
57-57: Unhandled rejection listener.
Captures promise rejections—good practice for modern JavaScript apps.
61-61: Conditional fallback in non-browser environments.
Logging an error ifaddEventListeneris missing is a robust fallback.
71-74: Early exit for undefined normalization.
Prevents further processing on unrecognized or unparseable errors.
78-79: Prefix for error context.
Combining context and custom messages ensures consistent log labeling.
81-83: SDK dispatch detection.
Leverages stacktrace scanning for theMANUAL_ERROR_IDENTIFIER—a neat approach to differentiate SDK errors.
84-91: Conditional check for non-SDK errors.
Skipping reporting for external errors in certain install types is logical.
94-94: Ensuring error reporting is enabled before sending.
Excellent guard to avoid unnecessary HTTP calls.
116-124: Logging handled or SDK-triggered errors.
Allows console visibility while avoiding double-reporting.
125-126: Catch block ensures fallback logging.
Safely prevents infinite error loops on error handling.
137-143: Breadcrumb handling.
Adding the newly created breadcrumb to state is straightforward. This design fosters better post-mortem diagnostics.
148-148: Default error handler.
Centralizing the default instance is standard for global usage.packages/analytics-js/src/services/ErrorHandler/utils.ts (11)
1-29: Initial imports and constants usage.
These references lay the groundwork for the new error processing logic (e.g.,ErrorType,SDKError, etc.).
45-50: Streamlined breadcrumb creation.
Removing optional metadata simplifies usage and ensures a consistent structure.
52-55:getReleaseStagelogic.
Development host detection is limited toDEV_HOSTS. Keep it updated for new dev environments if needed.
57-60:getAppStateForMetadata:
Smart approach usingstringifyWithoutCircularto avoid potential cycles and large objects.
62-65:getURLWithoutQueryString:
Cleanly strips query parameters—ideal for preserving user privacy.
67-71:getBugsnagErrorEventsignature.
Renaming ensures clarity: you now passexceptioninstead of the older error format.
72-120: Constructing the Bugsnag event.
Captures environment data fromstatethoroughly. Great usage ofcloneto avoid mutating the exception object.
122-129:isAllowedToBeNotified:
Filters known noisy or irrelevant errors. Minimal overhead withsome()usage.
130-154:isSDKError:
Inspects file paths for known SDK prefixes, covering typical bundling scenarios. This is a robust heuristic.
156-169:getErrorDeliveryPayload:
Generates final JSON for metrics ingestion. Writing key, version, etc., are included for accurate tracing.
171-181: Exports are coherent.
The new utility functions unify the approach to error instance creation, serialization, and filtering.packages/analytics-js/src/constants/logMessages.ts (2)
11-11: LGTM: Import statement is correctly placedThe Nullable type import is appropriately added and used in the new NON_ERROR_WARNING constant.
Line range hint
277-328: LGTM: Exports are properly updatedThe export list is correctly maintained with:
- All new error constants added
- Proper alphabetical ordering
- Removal of plugin-specific error constants aligning with the move to core module
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts (1)
42-48: **Potential Edge Cases forgetURLWithoutQueryString**You might consider testing additional edge cases where
location.hrefis empty or lacks a query param to ensure the function behaves as expected in those scenarios.packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (1)
78-83: **Selective Reporting for Non-SDK Errors **While skipping non-SDK errors is valid, consider logging or monitoring the volume of filtered errors for better insight into user issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/analytics-js/package.jsonis excluded by!**/*.jsonpackages/analytics-js/project.jsonis excluded by!**/*.json
📒 Files selected for processing (5)
jest/jest.setup-dom.js(1 hunks)packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts(1 hunks)packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts(1 hunks)packages/analytics-js/src/constants/logMessages.ts(4 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js/src/constants/logMessages.ts
🧰 Additional context used
📓 Learnings (1)
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.
🔇 Additional comments (22)
packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts (9)
1-21: **Well-Structured Initial Setup **The ESLint directives and mock configuration are straightforward, and the imports (including type imports) neatly prepare the test environment.
23-40: **Comprehensive Test forcreateNewBreadcrumb**The test verifies all fields, including
metaData,type, and timestamps. This fosters confidence in breadcrumb creation.
50-83: **Extensive Hostname Testing ingetReleaseStage**This test suite covers localhost scenarios and other hostnames comprehensively. Good job ensuring that appropriate release stages are returned for each environment.
85-118: **Robust Pattern Matching inisSDKError**The varied file paths and conditions thoroughly confirm which errors originate from the SDK. This precise coverage helps avoid false positives.
120-264: **In-Depth Validation ofgetAppStateForMetadata**This section rigorously tests nested signals, arrays, and objects, ensuring correct JSON transformations. The usage of
APP_STATE_EXCLUDE_KEYSis well-verified.
266-575: **Detailed Testing forgetBugsnagErrorEvent**The test scenario captures comprehensive property checks, including
app,device, andmetaDatafields. It ensures the error event payload matches expected structure and data.
577-595: **Concise Check ingetErrorDeliveryPayload**Your approach to verifying output structure with
expect.any(String)ensures the payload is properly serialized. This test helps maintain consistent reporting behavior.
597-609: **Clear Criteria inisAllowedToBeNotified**The test verifies the filtering logic for allowed error messages, ensuring undesired errors are excluded from reporting.
611-643: **Comprehensive Coverage forgetErrInstance**All scenarios—handled, unhandled, and promise rejections—are tested. This strongly validates error extraction logic.
jest/jest.setup-dom.js (1)
13-27: **Minimal Mock forPromiseRejectionEvent**Conditionally attaching the mock class to the global scope ensures consistent behavior in test environments that lack
PromiseRejectionEvent. Neat solution!packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (7)
34-39: **Dependency Injection ofhttpClient**Replacing
pluginEnginewithhttpClientsimplifies error reporting logic and clarifies responsibilities. Great step toward more modular design.
45-54: **Global Error Listeners **Attaching event listeners for both
errorandunhandledrejectionensures robust coverage of runtime exceptions.
63-66: **Pre-check fornormalizedError**Early return if the error is undefined avoids unnecessary logic and possible null references.
93-105: **External Error Reporting viahttpClient**Sending error data as raw JSON is a clean, straightforward approach. This ensures no additional formatting overhead on the server side.
108-118: **Graceful Failure Handling inonError**Catching and logging errors from within
onErrorhelps prevent error loops. This is an excellent defensive measure.
129-134: **Breadcrumb Collection **Storing breadcrumbs in
state.reporting.breadcrumbsis simple yet effective. It preserves user context leading up to errors without extra overhead.
140-140: **Default Export Initialization **
defaultErrorHandlerwithdefaultHttpClientanddefaultLoggeris consistent with the revised constructor design.packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts (5)
1-4: **Updated Mocks Import **Pulling in
defaultHttpClientanddefaultLoggerfrom the mocks folder enhances test isolation.
12-12: **Correct Instantiation ofErrorHandler**Using
(defaultHttpClient, defaultLogger)aligns with the new constructor signature, removing reliance on the oldpluginEngine.
15-47: **Coverage for Global Error Handlers **Testing unhandled error and promise rejection events ensures that core listener functionality is triggered accurately.
48-97: **Breadcrumb Logging **Verifying breadcrumb appending and error handling is thorough. The scenario where
state.reporting.breadcrumbs.valueis null ensures resilience.
100-210: **RefinedonErrorTests **The suite addresses diverse error conditions and toggles for error reporting. This comprehensive approach validates the entire error-handling workflow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/analytics-js/src/components/core/Analytics.ts (1)
254-256: Consider reordering parameters for consistency.While the initialization is correct, consider reordering the parameters to maintain consistency with other service initializations in the file where core services (errorHandler, logger) typically come first:
- this.eventRepository = new EventRepository( - this.pluginsManager, - this.storeManager, - this.httpClient, - this.errorHandler, - this.logger, - ); + this.eventRepository = new EventRepository( + this.errorHandler, + this.logger, + this.pluginsManager, + this.storeManager, + this.httpClient, + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts(3 hunks)packages/analytics-js/__tests__/components/capabilitiesManager/detection/adBlockers.test.ts(3 hunks)packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts(3 hunks)packages/analytics-js/src/components/capabilitiesManager/detection/adBlockers.ts(1 hunks)packages/analytics-js/src/components/capabilitiesManager/types.ts(1 hunks)packages/analytics-js/src/components/core/Analytics.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js/tests/components/capabilitiesManager/detection/adBlockers.test.ts
🔇 Additional comments (13)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts (3)
2-2: Validate import of defaultHttpClient
Bringing indefaultHttpClientaligns with the new constructor requirements forCapabilitiesManager.
43-47: Successfully passing the required parameters
Instantiating theCapabilitiesManagerwithdefaultHttpClient,defaultErrorHandler, andmockLoggeris consistent with the updated constructor signature.
106-109: Omitting logger confirms fallback behavior
This test scenario properly verifies that no warnings are logged when the logger parameter is absent, adhering to the class's optional logger design.packages/analytics-js/src/components/capabilitiesManager/types.ts (2)
4-4: Added import for IHttpClient
This import ensures theICapabilitiesManagerinterface can declare a strict HTTP client dependency.
7-8: New required properties
Making bothhttpClientanderrorHandlermandatory inICapabilitiesManagerenforces consistency and ensures that error handling and network operations can be reliably injected.packages/analytics-js/src/components/capabilitiesManager/detection/adBlockers.ts (2)
1-1: HttpClient import introduced
ImportingIHttpClientis required to pass it as a parameter, consolidating ad-blocker detection under a unified HTTP mechanism.
4-4: Simplified function signature
Requiring a singlehttpClientparameter streamlines external dependencies and avoids optional logging or error handling logic here. This clarifies responsibility for error handling and logging outside the function.packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts (4)
15-15: Imported IHttpClient
IntroducingIHttpClientensures that network operations are consistently handled across the capabilities management workflow.
40-41: New required properties in class
DefininghttpClientanderrorHandleras mandatory class properties locks in consistent error and HTTP handling throughout this manager.
45-48: Constructor now mandates httpClient and errorHandler
Requiring these parameters promotes explicit dependency injection, fostering better testability and clearer ownership of error and HTTP handling.
112-112: Refined detectAdBlockers usage
Passingthis.httpClientensures the ad-blocker detection function can perform HEAD requests without relying on global or optional injections.packages/analytics-js/src/components/core/Analytics.ts (2)
101-106: LGTM! Proper initialization sequence and dependency injection.The initialization sequence is well-structured with proper dependency injection. The
httpClientis correctly initialized with theerrorHandlerbefore being passed to thecapabilitiesManager.
101-106: Verify consistent error handling initialization patterns.Let's verify that the error handling initialization pattern is consistent across the codebase.
Also applies to: 254-256
✅ Verification successful
Let me try a different pattern to search for error handling initialization.
Based on the search results, I can now provide a comprehensive analysis of the error handling initialization patterns:
Error handling initialization is consistent across the codebase
The error handling initialization follows a consistent pattern throughout the codebase:
- The
ErrorHandleris properly initialized in the Analytics core class and passed down to all dependent components- The
httpClient.init()is correctly called with the error handler instance- All major components (ConfigManager, CapabilitiesManager, EventManager, etc.) consistently receive the error handler through their constructors
- Components uniformly implement error handling by:
- Having an
errorHandlerproperty- Accepting it in their constructor
- Using it in their
onErrormethodsThe changes in lines 101-106 maintain this consistent pattern by:
- Initializing the HTTP client with the error handler
- Passing the error handler to the CapabilitiesManager constructor
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handler initialization patterns in the codebase echo "Searching for error handler initialization patterns..." ast-grep --pattern 'new $class($_, $errorHandler, $_)' echo "Searching for httpClient initialization patterns..." ast-grep --pattern '$httpClient.init($errorHandler)'Length of output: 343
Script:
#!/bin/bash # Search for error handler patterns with more context echo "Searching for error handler initialization patterns..." rg -n "errorHandler" -A 2 -B 2 echo "\nSearching for httpClient initialization patterns..." rg -n "httpClient.init" -A 2 -B 2Length of output: 70508
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/analytics-js/__tests__/browser.test.ts (5)
1-4: Consider removing global rule disabling if possible
Disablingeslintrules at the file level might hide issues beyond the intended scope. If these directives are not strictly necessary, consider removing or narrowing them to specific lines, improving overall code quality.
49-66: Use async/await to avoid callback-based tests
The current test relies on adonecallback to handle asynchronous flow. Converting to async/await can simplify the code, improve readability, and reduce the likelihood of callback-related errors.-it('should process the buffered API calls when SDK script is loaded', done => { +it('should process the buffered API calls when SDK script is loaded', async () => { // Mock XHR window.XMLHttpRequest = jest.fn(() => xhrMock) as unknown as typeof XMLHttpRequest; loadingSnippet(); window.rudderanalytics?.page(); - window.rudderanalytics?.ready(() => { - expect((window.rudderanalytics as any).push).not.toBe(Array.prototype.push); - expect(xhrMock.send).toHaveBeenCalledTimes(2); - done(); - }); + await new Promise(resolve => { + window.rudderanalytics?.ready(() => resolve(true)); + }); + + expect((window.rudderanalytics as any).push).not.toBe(Array.prototype.push); + expect(xhrMock.send).toHaveBeenCalledTimes(2); require(pathToSdk); });
69-80: Maintain consistent use of async/await in beforeEach
Similar to the previous suggestion, using async/await inbeforeEachcan improve the clarity of test setup and prevent potential race conditions if future tests become more complex.
83-91: Expand test assertions
You're currently validating that the expected number of requests is sent. Consider also verifying the correctness of payloads or request method attributes, especially if request structure is critical in production.
94-110: UUID generation test coverage
The test effectively checks for a UUID v4 pattern. If collisions or invalid generation are concerns, you may add coverage for scenarios like re-initializing the SDK multiple times under varied conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/analytics-js/__tests__/browser.test.ts(2 hunks)packages/analytics-js/__tests__/nativeSdkLoader.js(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/analytics-js/__tests__/nativeSdkLoader.js (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js/__tests__/nativeSdkLoader.js:31-33
Timestamp: 2024-11-12T15:14:23.319Z
Learning: The loading snippet in `packages/analytics-js/__tests__/nativeSdkLoader.js` is a standard part of the SDK, and no changes are desired on it.
🔇 Additional comments (4)
packages/analytics-js/__tests__/nativeSdkLoader.js (1)
59-59: Good use of nullish coalescing to safeguardloadOptions.
The introduction ofloadOptionsand usingloadOptions ?? {}ensures the snippet passes valid arguments to theloadmethod even when none are provided. This change adds flexibility without breaking existing functionality. Also, based on the retrieved learnings, no further modifications are needed here to preserve the snippet’s standard usage.packages/analytics-js/__tests__/browser.test.ts (3)
5-5: Check distribution script reference
Switching torsa.jsinstead ofrsa.min.jsmay slow down test execution and potentially mask differences between minified and non-minified builds. Ensure both files are tested if your deployment depends on the minified version.
112-133: Comprehensive user reset test
This test thoroughly covers clearing user identity and traits. Nice work ensuring that the anonymous ID remains intact. No changes needed.
135-155: Thorough coverage of reset(true) functionality
This test ensures that an entirely new anonymous ID is generated after a full reset, providing confidence that user session data is rebuilt correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
packages/analytics-js/__tests__/nativeSdkLoader.js (1)
4-4: Consider making the SDK version dynamic.The SDK version (3.0.10) is hardcoded in the snippet. This could become outdated and might need frequent updates.
Consider reading this value from package.json or environment variables to ensure it stays in sync with the actual SDK version.
packages/analytics-js/__tests__/browser.test.ts (5)
9-30: Consider extracting mock configuration to a separate test utilities file.The mock source configuration contains multiple test constants that could be reused across different test files. Consider moving this to a shared test utilities file to improve maintainability and reusability.
67-76: Enhance test cleanup.Consider clearing the mock's call history in the cleanup to ensure complete isolation between tests:
afterEach(() => { jest.resetModules(); + jest.clearAllMocks(); window.rudderanalytics = undefined; window.XMLHttpRequest = originalXMLHttpRequest; });
77-94: Enhance preload buffer test coverage.The test verifies the number of XHR calls but could be more thorough:
- Verify the content of each XHR call
- Add error cases (e.g., failed source configuration request)
Example enhancement:
it('should process the buffered API calls when SDK script is loaded', async () => { window.XMLHttpRequest = jest.fn(() => xhrMock) as unknown as typeof XMLHttpRequest; loadingSnippet(WRITE_KEY, DATA_PLANE_URL); const pageEvent = { name: 'test-page' }; const trackEvent = { event: 'test-event' }; window.rudderanalytics?.page(pageEvent); window.rudderanalytics?.track(trackEvent.event); await loadAndWaitForSDK(); expect((window.rudderanalytics as any).push).not.toBe(Array.prototype.push); expect(xhrMock.send).toHaveBeenCalledTimes(3); // Verify content of each call expect(JSON.parse(xhrMock.send.mock.calls[1][0])).toMatchObject({ type: 'page', ...pageEvent, }); expect(JSON.parse(xhrMock.send.mock.calls[2][0])).toMatchObject({ type: 'track', ...trackEvent, }); });
123-139: Enhance getAnonymousId test coverage.Consider adding these test cases:
- Test with invalid stored anonymous ID
- Test with maximum length anonymous ID
- Test persistence across page reloads
Example additional test:
it('should handle invalid stored anonymous ID', () => { // Simulate corrupted storage localStorage.setItem('rl_anonymous_id', 'invalid-uuid'); const anonId = window.rudderanalytics?.getAnonymousId(); // Should generate new valid UUID expect(anonId).toMatch(/^[\da-f]{8}-[\da-f]{4}-4[\da-f]{3}-[\da-f]{4}-[\da-f]{12}$/i); });
112-120: Add performance verification for batch API calls.The test verifies individual API calls but should also verify performance with batch operations:
Example enhancement:
it('should handle multiple API calls efficiently', () => { const startTime = performance.now(); // Make multiple API calls in quick succession for (let i = 0; i < 100; i++) { window.rudderanalytics?.track(`test-event-${i}`); } const endTime = performance.now(); const executionTime = endTime - startTime; // Verify execution time is within acceptable range expect(executionTime).toBeLessThan(1000); // 1 second // Verify all requests were made expect(xhrMock.send).toHaveBeenCalledTimes(101); // 100 events + 1 source config });packages/analytics-js-common/__tests__/utilities/errors.test.ts (5)
25-35: Decoratingstacktraceand performance considerations regardingdelete.
Usingdelete error.stackeffectively simulates different error properties. However, static analysis suggests avoiding thedeleteoperator for performance reasons. You may consider assigningundefinedinstead if you want to remove the property:- delete error.stack; + error.stack = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
37-47: Opera sourceloc test & performance hint.
Similar to the above, you can remove the property by settingerror.stack = undefinedinstead of usingdelete. This keeps the code consistent and aligns with performance best practices.🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
56-64: Stacktrace property check & performance hint.
You've tested fallback to theerror.stacktraceproperty. Likewise, consider replacing thedelete error.stackusage for performance reasons as suggested earlier.🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
66-74: Opera sourceloc property check & performance hint.
Once again, consider settingerror.stack = undefinedrather thandelete error.stackto align with best practices regarding object property removal.🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
76-81: Undefined scenario test.
This test ensures thatgetStacktracegracefully handles situations with no valid stack. The same suggestion applies about usingerror.stack = undefinedinstead ofdelete error.stack.🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/analytics-js-common/__tests__/utilities/errors.test.ts(1 hunks)packages/analytics-js-plugins/.size-limit.mjs(1 hunks)packages/analytics-js/.size-limit.mjs(1 hunks)packages/analytics-js/__tests__/browser.test.ts(1 hunks)packages/analytics-js/__tests__/nativeSdkLoader.js(2 hunks)packages/analytics-v1.1/.size-limit.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/analytics-js/__tests__/nativeSdkLoader.js (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js/__tests__/nativeSdkLoader.js:31-33
Timestamp: 2024-11-12T15:14:23.319Z
Learning: The loading snippet in `packages/analytics-js/__tests__/nativeSdkLoader.js` is a standard part of the SDK, and no changes are desired on it.
🪛 Biome (1.9.4)
packages/analytics-js-common/__tests__/utilities/errors.test.ts
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 41-41: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 60-60: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 70-70: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (19)
packages/analytics-js-plugins/.size-limit.mjs (2)
16-20: Confirm compliance with size limit update policy.You’ve updated the size limit to '14 KiB' for the Legacy plugins bundle. According to the file header, any changes to the size limits require approval from the code owners. Please ensure you have their consent, as these modifications might affect the release process.
24-24: Validate the feasibility of reducing the size limit for Modern plugins bundle.You’ve reduced the size limit to '6 KiB'. Ensure the actual bundle size remains within this new threshold, especially if future merges introduce additional code. Consider adding a build step to confirm the final output size remains under 6 KiB.
packages/analytics-js/__tests__/nativeSdkLoader.js (2)
59-59: Verify the impact of the new load method signature.The load method invocation has been modified to use the new parameters. This change:
- Makes the snippet more configurable but potentially less standardized
- Could affect existing implementations that expect the default behavior
Let's verify the standard load method signature across the codebase:
✅ Verification successful
The load method signature is consistent across the codebase
Based on the search results, the load method signature consistently follows the pattern:
rudderanalytics.load(writeKey, dataPlaneUrl, options)All examples and test files across different versions (v1, v1.1, v2, v3) use this same signature with three parameters:
- writeKey (required)
- dataPlaneUrl (required)
- options (optional configuration object)
The modification in the test file to use
loadOptions ?? {}as the third parameter maintains backward compatibility by:
- Preserving the same parameter order
- Providing an empty object as fallback when loadOptions is null/undefined, which matches the existing behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for load method signatures ast-grep --pattern 'rudderanalytics.load($$$)'Length of output: 10205
1-1:⚠️ Potential issueReconsider modifying the standard loading snippet.
Based on previous feedback, modifications to this loading snippet are not desired as it's a standard part of the SDK. Consider reverting the function signature change to maintain consistency.
Let's verify if this snippet is used elsewhere in the codebase:
packages/analytics-v1.1/.size-limit.js (2)
43-43: Confirm naming convention alignment with documentation.You changed the entry name to
'Core (v1.1) - Legacy - CDN'. Ensure all references in your documentation or usage instructions reflect this updated naming convention so users can easily recognize the version.
48-48: Approve naming update for Modern CDN build.Renaming to
'Core (v1.1) - Modern - CDN'looks consistent with the versioned naming scheme. Great job maintaining clarity in build names.packages/analytics-js/.size-limit.mjs (6)
10-27: Double-check updated limits for Core - Legacy entries.The adjusted limits for:
- ESM (line 10)
- CJS (line 16)
- UMD (line 22)
- CDN (line 27)
all appear consistent. Provide justification for lowering these limits and confirm with code owners, as the file’s header explicitly requests their review for size-limit changes.
33-50: Ensure alignment with the newly increased size limits for Core - Modern.The changes at:
- ESM (line 33)
- CJS (line 39)
- UMD (line 45)
- CDN (line 50)
increase the allowed size. Please confirm no unintended payload bloat has slipped in. A larger allowance should be matched by a rationale in the PR description or a comment in the code.
56-68: Validate the reduced size limits for Core (Bundled) - Legacy.Changes at:
- ESM (line 56)
- CJS (line 62)
- UMD (line 68)
reflect consistent decreases in the size-limits. Verify that removing or refactoring code (e.g., older plugin or error reporting logic) actually achieves these smaller footprints.
74-86: Confirm stability of Core (Bundled) - Modern with new lower limits.At lines 74, 80, and 86, the limit is decreased to 39 KiB. Ensure performance improvements or plugin refactoring have enabled this limit reduction without losing functionality.
92-104: Check the updated size constraints for Core (Content Script) - Legacy.Size limits are set to 48 KiB at lines 92, 98, and 104. Make sure all content-script functionalities remain intact after any plugin removals or error-handling consolidations.
110-122: Assess reduced size boundary for Core (Content Script) - Modern.For lines 110, 116, and 122, the size limit is 39 KiB. If these deltas result from code migrations or plugin changes, confirm those changes are stable and tested across browsers.
packages/analytics-js-common/__tests__/utilities/errors.test.ts (7)
1-1: Imports look good.
No issues found importingdispatchErrorEventandgetStacktrace.
5-6: Mock setup is clear.
You’ve succinctly introduceddispatchEventMockand preserved the original dispatch event. This is a good practice when mocking global functionality.
8-10: BeforeEach block.
ReplacingglobalThis.dispatchEventwith your mock for each test ensures clean isolation across tests.
12-14: AfterEach block.
RestoringglobalThis.dispatchEventpreserves the global state for subsequent tests. No concerns here.
17-23: Thorough test for dispatching error events.
Verifying thatdispatchErrorEventcorrectly appends[SDK DISPATCHED ERROR]to the stack and dispatches anErrorEventdemonstrates robust coverage of the new functionality.
50-54: ValidatinggetStacktracewith string stack.
This provides clear verification that the function returns the rawstackproperty when present.
83-87: Name and message equivalence check.
Great job verifying that a stack identical toname: messageyieldsundefined. This covers a corner case in error stack handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/analytics-js/__tests__/browser.test.ts (3)
54-65: Consider adding error handling for SDK loadingThe loading mechanism is straightforward, but it might benefit from a timeout mechanism to prevent indefinite waiting if the SDK fails to load or initialize properly.
const loadAndWaitForSDK = async () => { const readyPromise = new Promise(resolve => { window.rudderanalytics?.ready(() => resolve(true)); }); require(SDK_PATH); - await readyPromise; + return Promise.race([ + readyPromise, + new Promise((_, reject) => { + setTimeout(() => reject(new Error('SDK initialization timeout')), 5000); + }), + ]); };
78-95: Enhance test assertions for request validationWhile the test verifies the number of requests, it would be more robust to also verify the content and order of the requests.
expect(xhrMock.send).toHaveBeenCalledTimes(3); +// Verify the order and content of requests +expect(xhrMock.open.mock.calls).toEqual([ + ['GET', expect.stringContaining('/sourceConfig'), true], + ['POST', expect.stringContaining('/page'), true], + ['POST', expect.stringContaining('/track'), true], +]); + +// Verify the payload of the track event +const trackCallData = JSON.parse(xhrMock.send.mock.calls[2][0]); +expect(trackCallData.event).toBe('test-event');
177-177: Fix misleading commentThe comment "SDK remembers the previously generated anonymous ID and returns the same value" contradicts the test's purpose, which is to verify that the anonymous ID is cleared when reset is called with
true.- // SDK remembers the previously generated anonymous ID and returns the same value + // SDK should generate a new anonymous ID after full resetpackages/analytics-js/src/components/eventRepository/EventRepository.ts (1)
215-217: Consider improving error type safety.The error handling implementation is good, but we could enhance type safety.
Consider this improvement:
- onError(error: unknown, customMessage?: string): void { + onError(error: Error | unknown, customMessage?: string): void { if (this.errorHandler) { this.errorHandler.onError(error, EVENT_REPOSITORY, customMessage); } else { - throw error; + throw error instanceof Error ? error : new Error(String(error)); } }This change would:
- Better handle both Error objects and unknown errors
- Ensure consistent error throwing behavior
packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts (1)
112-112: LGTM with a suggestion for error handlingThe changes properly utilize the new
httpClientdependency and simplify error handling. Consider adding error context information to improve debugging:- this.errorHandler.onError(error, CAPABILITIES_MANAGER); + this.errorHandler.onError(error, CAPABILITIES_MANAGER, { + context: 'Error occurred while managing browser capabilities', + });Also applies to: 208-208
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/unit-tests-and-lint.ymlis excluded by!**/*.yml
📒 Files selected for processing (4)
packages/analytics-js/__tests__/browser.test.ts(1 hunks)packages/analytics-js/__tests__/components/utilities/event.test.ts(0 hunks)packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts(4 hunks)packages/analytics-js/src/components/eventRepository/EventRepository.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/analytics-js/tests/components/utilities/event.test.ts
🔇 Additional comments (5)
packages/analytics-js/__tests__/browser.test.ts (1)
6-30: LGTM: Well-structured configuration setupThe mock source configuration is comprehensive and follows a clear hierarchical structure. The configuration properly includes essential fields like writeKey, workspaceId, and feature flags for stats collection.
packages/analytics-js/src/components/eventRepository/EventRepository.ts (1)
53-59: LGTM! Verify impact of constructor signature change.The addition of httpClient parameter improves dependency injection and testability. This is a positive architectural change.
Let's verify the impact of this breaking change:
✅ Verification successful
Let me analyze the instantiations further to ensure we have all the necessary context.
Breaking change has been properly handled across the codebase
The constructor signature change has been properly implemented across all usages:
- The main usage in
Analytics.tscorrectly provides all required parameters includinghttpClient- All test files have been updated to include the
defaultHttpClientparameter- The only potential issue is in
analytics-v1.1/src/utils/EventRepository.jswhich uses an empty constructor, but this appears to be in a legacy v1.1 package that likely has its own implementationThe changes are consistent and well-maintained across the codebase, with proper dependency injection of the
httpClientparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for EventRepository instantiations that need updating ast-grep --pattern 'new EventRepository($$$)'Length of output: 8759
Script:
#!/bin/bash # Let's check the EventRepository.ts file to see the full constructor signature cat packages/analytics-js/src/components/eventRepository/EventRepository.tsLength of output: 7295
packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts (3)
15-15: LGTM: Import follows project conventionsThe import of
IHttpClienttype is properly placed and follows the project's import conventions.
45-48: LGTM: Constructor properly initializes required dependenciesThe constructor changes align with the property changes and properly initialize all dependencies in a logical order.
40-41: Verify the impact of breaking changesMaking
errorHandlerrequired and adding mandatoryhttpClientare breaking changes. Ensure all existing implementations ofCapabilitiesManagerare updated accordingly.✅ Verification successful
Let me verify the impact by checking the constructor signature and implementation.
No breaking changes detected in CapabilitiesManager usage
Based on the verification:
- All instances of
CapabilitiesManagerin the codebase are already providing bothhttpClientanderrorHandlerparameters:
- In
Analytics.ts:new CapabilitiesManager(this.httpClient, this.errorHandler, this.logger)- In test files:
new CapabilitiesManager(defaultHttpClient, defaultErrorHandler, mockLogger)- The only exception is in
examples/chrome-extension/content-script-v3/foreground.jswhich appears to be minified/bundled code and likely outdated example code.The changes to make these properties required align with their actual usage in the codebase, formalizing the existing implementation rather than introducing breaking changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CapabilitiesManager instantiations ast-grep --pattern 'new CapabilitiesManager($$$)' # Search for class extensions ast-grep --pattern 'class $_ extends CapabilitiesManager { $$$ }'Length of output: 2067
Script:
#!/bin/bash # Check the constructor implementation and signature ast-grep --pattern 'constructor($$$) { $$$ }' packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts # Get the file content to see the full context cat packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.tsLength of output: 7459
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (36)
packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts (1)
8-9: Well-structured centralization of mock dependencies!The migration of
defaultHttpClientanddefaultPluginsManagerfrom local to common mocks aligns well with the PR's objective of centralizing error reporting functionality. This change promotes consistency and reduces duplication across the codebase while maintaining the existing test behavior.Consider documenting these common mocks in the project's testing guidelines to encourage their reuse across other test files.
packages/analytics-js/__tests__/services/PluginEngine/PluginEngine.test.ts (1)
3-3: Consider adding targeted tests for logging.The addition of
defaultLoggeris good, but you might want to include a mocked logger or a spy to verify logging calls within your tests. This can help confirm whether the logger behaves as expected during plugin registration and invocation.packages/analytics-js/src/components/userSessionManager/utils.ts (1)
45-45: Sufficient logging message, but consider adding context on validity checks.The current log statement warns about an invalid session ID. Including more context in the message (e.g., reason for invalidation: non-positive integer, insufficient length) could further guide debugging or usage.
packages/analytics-js/__tests__/services/StoreManager/Store.test.ts (4)
4-7: Consider grouping or re-exporting shared modules to simplify imports.
You have introduced several imports for error handling, logging, and plugin management. If these modules are commonly imported as a bundle from a higher-level organization (e.g.,services/index), consider re-exporting them in a single aggregator file. This can reduce import lines and enhance maintainability.
39-43: Test with custom errorHandler and logger.
You’re passingdefaultErrorHandleranddefaultLoggerto theStore. It might be valuable to add one or two test cases using custom error/logging implementations to ensure theStorebehaves as expected when different handlers are plugged in. This can help validate the store’s tolerance for external or advanced usage scenarios.
94-98: Reuse store creation logic to avoid duplication.
The repeated constructor calls with the same parameters could be wrapped in a function or test fixture. This ensures a single source of truth for store instantiation, reducing test boilerplate and the risk of divergence.
155-159: Simulate additional quota-related errors.
You are testing theQuotaExceededErrorscenario for switching to an in-memory engine. Consider adding a scenario where the error is triggered intermittently partway through a batch of writes or with different storage backends. This helps validate robust fallback logic in real-world conditions.Do you want me to propose additional test cases to cover these edge scenarios?
packages/analytics-js/src/services/PluginEngine/PluginEngine.ts (5)
49-49: Add negative test coverage.This error-handling branch reports a missing plugin name. Strengthen reliability by adding a unit test that triggers this condition, helping ensure future refactors don’t break the error-handling logic.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 49-49: packages/analytics-js/src/services/PluginEngine/PluginEngine.ts#L49
Added line #L49 was not covered by tests
58-58: Test plugin duplicate scenario.Similarly, the code is missing coverage for this condition. Consider adding a test that attempts to register a plugin with the same name twice to validate that the error is properly thrown/logged.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 58-58: packages/analytics-js/src/services/PluginEngine/PluginEngine.ts#L58
Added line #L58 was not covered by tests
89-89: Ensure test coverage for unregistered plugin scenario.This block logs an error when a plugin is not found. Encourage a dedicated test that unregisters a non-existent plugin, verifying correct logging.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 89-89: packages/analytics-js/src/services/PluginEngine/PluginEngine.ts#L89
Added line #L89 was not covered by tests
100-100: Cover plugin index mismatch edge case.When the plugin index is -1, the code logs a potential engine bug. Add a test to confirm the plugin array is updated correctly and that this error is effectively handled.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 100-100: packages/analytics-js/src/services/PluginEngine/PluginEngine.ts#L100
Added line #L100 was not covered by tests
122-122: Clarify dependency errors.This logger call highlights missing dependencies. If a plugin references non-existent dependencies, the plugin is skipped. Consider adding a test to confirm that the plugin is excluded and the error is logged as expected.
packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts (1)
53-53: Remove optional chaining for a required property.
SinceerrorHandleris now mandatory, you can safely remove?., which simplifies the call:- this.errorHandler?.onError(error, EXTERNAL_SRC_LOADER); + this.errorHandler.onError(error, EXTERNAL_SRC_LOADER);packages/analytics-js/src/services/StoreManager/StoreManager.ts (1)
223-223: Eliminate optional chaining for mandated errorHandler.
SinceerrorHandleris a required property, remove?.for a clearer, more direct call:- this.errorHandler?.onError(error, STORE_MANAGER); + this.errorHandler.onError(error, STORE_MANAGER);packages/analytics-js-common/__mocks__/Store.ts (1)
64-68: Default store instantiation with new parametersIncluding
errorHandler,logger, andpluginsManagerin thedefaultStoreinstantiation ensures any initial mock behavior stays consistent with production usage. Consider adding basic unit tests if they are not already present, to validate these defaults.packages/analytics-js/src/components/configManager/util/cdnPaths.ts (1)
54-54: Updating docstring to mention the mandatory loggerThe updated documentation clarifies that a
loggerparameter is mandatory, enhancing discoverability for anyone integrating withgetIntegrationsCDNPath.packages/analytics-js-common/src/component-cookie/index.ts (1)
25-25: Silencing errors in decode might hamper debuggingCatching all decode errors without logging can mask unexpected encoding issues. Consider at least a debug or trace log to aid in troubleshooting cookie corruption or invalid data.
- } catch { + } catch (err) { // Do nothing as non-RS SDK cookies may not be URI encoded + // logger?.debug('Failed to decode cookie', err); return undefined; }packages/analytics-js/src/services/StoreManager/Store.ts (1)
32-34: Required properties enforce consistent error handling and logging.Changing
errorHandler,logger, andpluginsManagerfrom optional to required ensures these dependencies are always available. However, see line 214 for residual optional chaining onerrorHandler.packages/analytics-js/src/components/configManager/util/commonUtil.ts (3)
84-84: Remove optional chaining for the required logger parameter
Sinceloggeris now a required parameter, replacelogger?.withlogger.in this function to simplify logic and avoid potential confusion.- logger?.warn( + logger.warn(
225-225: Enforce non-optional logger usage
Same note as previous: sinceloggeris now guaranteed, consider removing optional chaining inside this method for clarity and consistency.
319-319: Remove logger optional calls
This updated signature ensuresloggeris always defined; removelogger?.warn()checks to align with the new requirement.packages/analytics-js-plugins/src/utilities/retryQueue/RetryQueue.ts (3)
137-138: Avoid storing references to storeManager’s errorHandler and logger if not overridden
Since these fields are being copied directly fromstoreManager, consider verifying if this duplication is truly needed. If you simply need to store them for short usage, you could reference them fromstoreManagerdirectly.
588-589: Reuse existing errorHandler and logger references
Similar to lines 137-138, directly referencingthis.storeManager.errorHandlerandthis.storeManager.loggermight reduce redundancy. Use local properties only if you anticipate reassigning or overriding them.
777-778: Keep references consistent
Same recommendation about referencingstoreManager.errorHandlerandstoreManager.logger. Keep your approach consistent to avoid confusion.packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (5)
260-260: Consider removing optional chaining
Usingthis.logger.warn(...)instead ofthis.logger?.warn(...)may be clearer. Now thatloggeris required, no need to check for undefined.
273-273: No need for optional check
Sinceloggeris always available, you can removethis.logger?.warn(...)in favor ofthis.logger.warn(...).
279-279: Maintain consistent logger usage
Likewise, remove?to standardize the call.
370-370: Remove optional chaining in error logs
Sinceloggeris guaranteed, preferthis.logger.error(...).
377-377: Ensure uniform logging approach
As with the other logger calls, removing?.is recommended.packages/analytics-js/src/components/eventManager/EventManager.ts (1)
71-72: Consider enhancing error contextWhile the simplified
onErrorimplementation is cleaner, consider adding the error type to provide more context for error reporting.- onError(error: unknown, customMessage?: string): void { - this.errorHandler.onError(error, EVENT_MANAGER, customMessage); + onError(error: unknown, customMessage?: string): void { + this.errorHandler.onError(error, EVENT_MANAGER, customMessage, ErrorType.HANDLEDEXCEPTION);packages/analytics-js/src/services/HttpClient/HttpClient.ts (1)
31-33: Consider adding validation in init method.While the implementation is correct, consider adding validation to ensure errorHandler is not undefined.
init(errorHandler: IErrorHandler) { + if (!errorHandler) { + throw new Error('ErrorHandler cannot be undefined'); + } this.errorHandler = errorHandler; }packages/analytics-js/src/services/ErrorHandler/event/event.ts (1)
21-36: Consider adding JSDoc for edge cases.While the implementation handles edge cases well, consider documenting the special handling of global code and missing file/method scenarios in the JSDoc.
/** * Takes a stacktrace.js style stackframe (https://github.com/stacktracejs/stackframe) * and returns a Bugsnag compatible stackframe (https://docs.bugsnag.com/api/error-reporting/#json-payload) + * @remarks + * - Handles missing file/method cases (e.g., from chrome terminal) + * - Normalizes global code references * @param frame * @returns */🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-33: packages/analytics-js/src/services/ErrorHandler/event/event.ts#L33
Added line #L33 was not covered by testspackages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (1)
63-118: Consider extracting error filtering logic.The error filtering logic (lines 78-82) could be moved to a separate method for better maintainability and testability.
+ private shouldReportError(bsException: Exception, stacktrace: string | undefined) { + return isSDKError(bsException) || + state.context.app.value.installType === 'npm' || + stacktrace?.includes(MANUAL_ERROR_IDENTIFIER); + } onError(error: SDKError, context = '', customMessage = '', errorType = ErrorType.HANDLEDEXCEPTION) { try { // ... existing code ... - if ( - !isSDKError(bsException) && - state.context.app.value.installType !== 'npm' && - !isSdkDispatched - ) { + if (!this.shouldReportError(bsException, stacktrace)) { return; }packages/analytics-js/__tests__/components/configManager/cdnPaths.test.ts (1)
47-54: Enhance error test coverage.The error test case could be improved by:
- Testing multiple invalid URL scenarios
- Verifying the exact error message format
Consider adding more test cases:
it('should throw error if invalid custom url is provided', () => { const errorSpy = jest.spyOn(defaultLogger, 'error'); - const integrationsCDNPath = getIntegrationsCDNPath(dummyVersion, false, '/', defaultLogger); + // Test various invalid URL scenarios + const invalidUrls = ['/', 'invalid', 'http://', 'https:/invalid.com']; + invalidUrls.forEach(url => { + const integrationsCDNPath = getIntegrationsCDNPath(dummyVersion, false, url, defaultLogger); + expect(integrationsCDNPath).toBeNull(); + expect(errorSpy).toHaveBeenCalledWith( + `ConfigManager:: The base URL "${url}" for integrations is not valid.`, + ); + }); expect(integrationsCDNPath).toBeNull(); expect(errorSpy).toHaveBeenCalledWith( 'ConfigManager:: The base URL "/" for integrations is not valid.', ); errorSpy.mockRestore(); });packages/analytics-js/src/constants/logMessages.ts (1)
38-45: Add JSDoc comments for new error messages.The new error message constants would benefit from JSDoc comments explaining their usage.
+/** + * Warning message for non-error situations that are being handled as errors + * @param context - The logging context + * @param errStr - The error string that was incorrectly treated as an error + */ const NON_ERROR_WARNING = (context: string, errStr: Nullable<string>): string => `${context}${LOG_CONTEXT_SEPARATOR}Ignoring a non-error: ${errStr}.`; +/** + * Error message for breadcrumb logging failures + * @param context - The logging context + */ const BREADCRUMB_ERROR = (context: string): string => `${context}${LOG_CONTEXT_SEPARATOR}Failed to log breadcrumb.`; +/** + * Error message for error handling failures + * @param context - The logging context + */ const HANDLE_ERROR_FAILURE = (context: string): string => `${context}${LOG_CONTEXT_SEPARATOR}Failed to handle the error.`;packages/analytics-js/src/components/core/Analytics.ts (1)
101-106: LGTM! Consider adding error handling for HTTP client initialization.The initialization sequence and dependency injection are well-structured. The HTTP client is correctly initialized before being passed to the CapabilitiesManager.
Consider wrapping the HTTP client initialization in a try-catch block to handle potential initialization failures:
- this.httpClient.init(this.errorHandler); + try { + this.httpClient.init(this.errorHandler); + } catch (err) { + this.errorHandler.onError(err, ANALYTICS_CORE, 'Failed to initialize HTTP client'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
packages/analytics-js-common/__mocks__/ErrorHandler.ts(1 hunks)packages/analytics-js-common/__mocks__/HttpClient.ts(1 hunks)packages/analytics-js-common/__mocks__/Store.ts(2 hunks)packages/analytics-js-common/__mocks__/StoreManager.ts(2 hunks)packages/analytics-js-common/src/component-cookie/index.ts(1 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts(2 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts(2 hunks)packages/analytics-js-common/src/types/ErrorHandler.ts(1 hunks)packages/analytics-js-common/src/types/HttpClient.ts(1 hunks)packages/analytics-js-common/src/types/Store.ts(2 hunks)packages/analytics-js-plugins/__mocks__/state.ts(1 hunks)packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/ketchConsentManager/utils.test.ts(0 hunks)packages/analytics-js-plugins/src/iubendaConsentManager/utils.ts(1 hunks)packages/analytics-js-plugins/src/ketchConsentManager/utils.ts(1 hunks)packages/analytics-js-plugins/src/utilities/retryQueue/RetryQueue.ts(3 hunks)packages/analytics-js/__tests__/components/configManager/cdnPaths.test.ts(4 hunks)packages/analytics-js/__tests__/components/eventManager/EventManager.test.ts(2 hunks)packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts(2 hunks)packages/analytics-js/__tests__/components/userSessionManager/utils.test.ts(3 hunks)packages/analytics-js/__tests__/components/utilities/consent.test.ts(4 hunks)packages/analytics-js/__tests__/services/PluginEngine/PluginEngine.test.ts(2 hunks)packages/analytics-js/__tests__/services/StoreManager/Store.test.ts(6 hunks)packages/analytics-js/src/app/RudderAnalytics.ts(1 hunks)packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts(4 hunks)packages/analytics-js/src/components/capabilitiesManager/types.ts(1 hunks)packages/analytics-js/src/components/configManager/ConfigManager.ts(7 hunks)packages/analytics-js/src/components/configManager/types.ts(1 hunks)packages/analytics-js/src/components/configManager/util/cdnPaths.ts(3 hunks)packages/analytics-js/src/components/configManager/util/commonUtil.ts(5 hunks)packages/analytics-js/src/components/core/Analytics.ts(2 hunks)packages/analytics-js/src/components/eventManager/EventManager.ts(3 hunks)packages/analytics-js/src/components/eventManager/RudderEventFactory.ts(1 hunks)packages/analytics-js/src/components/eventManager/utilities.ts(7 hunks)packages/analytics-js/src/components/eventRepository/EventRepository.ts(3 hunks)packages/analytics-js/src/components/pluginsManager/PluginsManager.ts(2 hunks)packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts(5 hunks)packages/analytics-js/src/components/userSessionManager/utils.ts(2 hunks)packages/analytics-js/src/components/utilities/consent.ts(2 hunks)packages/analytics-js/src/constants/logMessages.ts(5 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(2 hunks)packages/analytics-js/src/services/ErrorHandler/event/event.ts(1 hunks)packages/analytics-js/src/services/HttpClient/HttpClient.ts(3 hunks)packages/analytics-js/src/services/HttpClient/xhr/xhrResponseHandler.ts(1 hunks)packages/analytics-js/src/services/PluginEngine/PluginEngine.ts(8 hunks)packages/analytics-js/src/services/StoreManager/Store.ts(5 hunks)packages/analytics-js/src/services/StoreManager/StoreManager.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/analytics-js-plugins/tests/ketchConsentManager/utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/analytics-js/tests/components/eventManager/EventManager.test.ts
- packages/analytics-js-common/src/types/HttpClient.ts
- packages/analytics-js/src/app/RudderAnalytics.ts
- packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts
- packages/analytics-js-plugins/mocks/state.ts
- packages/analytics-js/src/components/capabilitiesManager/types.ts
- packages/analytics-js-common/mocks/HttpClient.ts
🧰 Additional context used
📓 Learnings (6)
packages/analytics-js/__tests__/components/utilities/consent.test.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js/__tests__/components/utilities/consent.test.ts:1-1
Timestamp: 2024-11-12T15:14:33.334Z
Learning: In `packages/analytics-js/__tests__/components/utilities/consent.test.ts`, the relative import path `../../../src/state` correctly refers to the `index.ts` file in the `/state` directory.
packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts:10-11
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The misuse of `IHttpClient` in type assertions within the file `packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts` has been corrected by the user.
packages/analytics-js/__tests__/components/configManager/cdnPaths.test.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1730
File: packages/analytics-js/__tests__/components/configManager/cdnPaths.test.ts:0-0
Timestamp: 2024-11-12T15:14:33.334Z
Learning: When suggesting changes to simplify string concatenation, ensure to replace unnecessary template literals with single quotes if no variable interpolation is required.
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.
packages/analytics-js/src/constants/logMessages.ts (2)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/types/LoadOptions.ts:0-0
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In the `packages/analytics-js-common/src/types/LoadOptions.ts` file, the `dataplanes` property within the `SourceConfigResponse` type has been removed as it is no longer necessary.
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/types/LoadOptions.ts:175-192
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In this project, deprecated properties may still be used internally to provide backward compatibility for users, as the same types are used for both public-facing APIs and the internal codebase.
packages/analytics-js/src/services/ErrorHandler/event/event.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/src/services/PluginEngine/PluginEngine.ts
[warning] 49-49: packages/analytics-js/src/services/PluginEngine/PluginEngine.ts#L49
Added line #L49 was not covered by tests
[warning] 58-58: packages/analytics-js/src/services/PluginEngine/PluginEngine.ts#L58
Added line #L58 was not covered by tests
[warning] 89-89: packages/analytics-js/src/services/PluginEngine/PluginEngine.ts#L89
Added line #L89 was not covered by tests
[warning] 100-100: packages/analytics-js/src/services/PluginEngine/PluginEngine.ts#L100
Added line #L100 was not covered by tests
packages/analytics-js/src/services/StoreManager/Store.ts
[warning] 148-148: packages/analytics-js/src/services/StoreManager/Store.ts#L148
Added line #L148 was not covered by tests
packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts
[warning] 112-112: packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts#L112
Added line #L112 was not covered by tests
[warning] 208-208: packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts#L208
Added line #L208 was not covered by tests
packages/analytics-js/src/components/configManager/ConfigManager.ts
[warning] 81-81: packages/analytics-js/src/components/configManager/ConfigManager.ts#L81
Added line #L81 was not covered by tests
[warning] 96-96: packages/analytics-js/src/components/configManager/ConfigManager.ts#L96
Added line #L96 was not covered by tests
[warning] 160-160: packages/analytics-js/src/components/configManager/ConfigManager.ts#L160
Added line #L160 was not covered by tests
[warning] 213-214: packages/analytics-js/src/components/configManager/ConfigManager.ts#L213-L214
Added lines #L213 - L214 were not covered by tests
packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts
[warning] 291-291: packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts#L291
Added line #L291 was not covered by tests
packages/analytics-js/src/constants/logMessages.ts
[warning] 23-23: packages/analytics-js/src/constants/logMessages.ts#L23
Added line #L23 was not covered by tests
packages/analytics-js/src/services/ErrorHandler/event/event.ts
[warning] 33-33: packages/analytics-js/src/services/ErrorHandler/event/event.ts#L33
Added line #L33 was not covered by tests
[warning] 57-57: packages/analytics-js/src/services/ErrorHandler/event/event.ts#L57
Added line #L57 was not covered by tests
[warning] 81-81: packages/analytics-js/src/services/ErrorHandler/event/event.ts#L81
Added line #L81 was not covered by tests
🔇 Additional comments (87)
packages/analytics-js/__tests__/services/PluginEngine/PluginEngine.test.ts (1)
38-38: Validate logger usage withinPluginEngine.Passing
defaultLoggerexplicitly here aligns with broader moves toward consistent logging. Ensure that thePluginEnginemethods exercising this logger are properly covered by tests to catch any logging-related errors or side effects in future changes.packages/analytics-js/src/components/userSessionManager/utils.ts (2)
39-39: Great improvement in type safety for the mandatory logger parameter.Having
logger: ILoggerset as a required parameter ensures consistent logging practices and reduces potential runtime errors from undefined loggers. Please verify that all invocations ofisManualSessionIdValidacross the codebase supply the correct logger instance.Would you like me to provide a short shell script to scan the codebase for invocations of
isManualSessionIdValidmissing a logger argument?
74-74: Requiringidandloggerin manual session generation improves reliability.By ensuring both
id(asnumber | undefined) and a validlogger, you’ve made the session creation process clearer and more robust. This requirement standardizes how sessions are generated and how logs are captured, reducing ambiguity for downstream code.packages/analytics-js/__tests__/services/StoreManager/Store.test.ts (3)
29-31: Validate plugin engine instantiation parameters.
Here, thepluginEngineis created withdefaultLogger, andpluginsManageruses both thedefaultErrorHandleranddefaultLogger. Check whetherpluginEnginerequires an explicit error handler, or if it depends on an upstream injection. If thepluginEngineinteracts with error states, you may want to supply it with an error handler or confirm it is intentionally omitted.
111-115: Return type considerations for.createValidKey.
When thevalidKeyscheck fails,.createValidKeyreturnsundefined. Ensure this scenario is tested thoroughly upstream and incorporate defensive checks where keys might be assumed to be non-undefined.
126-130: Confirm usage ofQueueStatuseskeys.
You are passingQueueStatusesas valid keys in some tests and not in others. It’s worth verifying that tests explicitly confirm the store’s behavior both with this defined set of valid keys and with smaller or extraneous sets.packages/analytics-js/src/services/PluginEngine/PluginEngine.ts (3)
32-32: Mandatory logger might introduce a breaking change.Changing
loggerfrom optional to mandatory is a valid design choice but could break existing code if consumers were not passing a logger. Proactively update documentation and references to ensure all call sites provide a logger object.
34-34: Reordered constructor parameters requires updates across the codebase.By requiring
loggeras the first parameter, code that previously calledPluginEnginewith the old signature will need refactoring. Confirm all initialization points have been updated accordingly.
[approve]
198-198: Validate default plugin engine instantiation.The new constructor usage is consistent with the revised signature. Confirm that other
PluginEngineconsumers also instantiate it correctly, especially if they relied on the previous parameter order.packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts (2)
13-14: Required error handler and logger look good.
Making these properties mandatory clarifies the contract, ensuring every instance has a well-defined error-handling and logging mechanism.
18-19: Constructor injection aligns with stricter requirements.
By making these parameters mandatory, you enforce consistent initialization, which helps avoid missing dependencies at runtime.packages/analytics-js/src/services/StoreManager/StoreManager.ts (3)
43-45: Mandatory errorHandler, logger, and pluginsManager are consistent.
Enforcing these dependencies up front ensures that store operations always have the necessary components available.
47-47: Constructor parameters enforce dependency injection.
RequiringpluginsManager,errorHandler, andloggerfosters more reliable initialization and easier testing.
110-111: Passing errorHandler and logger through store config is correct.
This aligns with the new requirement that each store must have a valid error handling and logging strategy.packages/analytics-js/src/services/HttpClient/xhr/xhrResponseHandler.ts (2)
7-8: Requiring responseText and onError params is valid.
Ensuring these parameters exist prevents unexpected runtime issues if a null or undefined argument is passed.
14-14: Handling JSON parse errors seamlessly.
InvokingonErrordirectly streamlines error handling, but confirm that all call sites provide a valid onError callback.packages/analytics-js-common/__mocks__/StoreManager.ts (2)
2-2: Added PluginsManager import ensures consistency with new requirements.
This mock import complements the mandatory pluginsManager changes across the codebase.
25-25: Providing defaultPluginsManager to Store constructor.
This update matches the shift to required plugins managers, maintaining consistency in mock scenarios.packages/analytics-js-common/src/types/Store.ts (3)
15-16: Mandatory errorHandler and logger reflect the updated contract.
This change ensures all stores consistently handle and log errors.
23-24: StoreManager interface strictly defines errorHandler and logger
Requiring these properties improves reliability in managing store-related operations.
40-42: Enforced dependencies in IStore interface.
Having a guaranteed errorHandler, logger, and pluginsManager helps avoid undefined references and strengthens the store’s functionality.packages/analytics-js-common/__mocks__/Store.ts (3)
1-6: Imports updated for error handling and plugins managementAdding types and default imports for the error handler, logger, and plugins manager is consistent with the PR's objective of centralizing error reporting. No issues detected here.
11-20: Constructor signature now requires more dependenciesThe new constructor injects
errorHandler,logger, andpluginsManager, ensuring that theStoreis fully equipped for error processing and logging. This refactoring appears logical and aligns with the broader design choices in the PR.
26-30: Property definitions for new dependenciesExplicitly declaring the
engine,originalEngine,errorHandler,logger, andpluginsManagerwith correct types increases clarity, facilitating better type checking and maintainability.packages/analytics-js/src/components/configManager/types.ts (1)
80-81: Error handler and logger fields made mandatoryMaking
errorHandlerandloggerrequired inIConfigManagerstrengthens the guarantee that these services are always available. Ensure you verify all existing implementations now provide these fields to comply with the updated interface.✅ Verification successful
All implementations correctly provide errorHandler and logger
Based on the verification results, the only implementation of
IConfigManageris theConfigManagerclass, which properly initializes botherrorHandlerandloggerthrough its constructor and maintains them as required class fields. The implementation is fully compliant with the updated interface requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all IConfigManager implementations pass logger/errorHandler. ast-grep --pattern $'class $_ implements IConfigManager { $$$ }' -A 15Length of output: 20972
packages/analytics-js/src/components/configManager/util/cdnPaths.ts (8)
3-4: New logger import for robust loggingThe addition of
ILoggerandNullablereferences aligns with the push for consistent logging and null-safe checks across these utilities.
22-24: Adding logger as a required parameterEnhancing
getSDKComponentBaseURLwith aloggerparameter helps unify error reporting. This is in line with the PR goal to centralize error handling.
29-30: Silent error replaced with logger callUsing
logger.errorinstead of throwing an exception helps avoid breaking flows and gives visibility into invalid URLs. Ensure you confirm that upstream code handlesnullreturns appropriately.
60-62: New parameter for custom integrations pathThe revised signature includes
logger, aligning with the ongoing standardization. Double-check that downstream calls expect the updated signature.
70-70: Passing logger to getSDKComponentBaseURLEnsures consistent logging for integration paths. No issues detected, but confirm that any calling tests are updated accordingly.
78-78: JSDoc updated to clarify logger usageAccurate documentation fosters better developer clarity.
84-86: Plugins path function now requires loggerEnforcing logging at the plugin path resolution stage prevents silent failures. This appears consistent with the broader refactor.
94-94: Logger forwarded to utility functionFurther ensures consistent error reporting for plugin path resolution.
packages/analytics-js-plugins/src/ketchConsentManager/utils.ts (1)
28-29: Good integration of logger and errorHandler
Ensuring thaterrorHandlerandloggerare consistently passed intosetStorefortifies error handling and logging throughout the consent flow.packages/analytics-js-plugins/src/iubendaConsentManager/utils.ts (1)
54-55: Consistent approach to error handling
Mirroring the pattern used inketchConsentManager, this consistent approach to passingerrorHandlerandloggerintosetStoreensures proper error tracing and logging for the Iubenda cookie flow.packages/analytics-js/__tests__/components/userSessionManager/utils.test.ts (4)
47-47: Validation check looks good
CallinggenerateManualTrackingSessionwith a valid session ID and checking its output ensures correct manual session handling.
55-55: Undefined session ID scenario
Providingundefinedtests the fallback logic, confirming that a new session ID is generated when none is provided.
65-65: Testing invalid session ID
Great job using@ts-expect-errorto confirm that an invalid input logs a warning and auto-generates a new session ID.
95-95: Enhanced coverage for invalid storage type
Validating an unexpected storage type showcases robust input handling and logging for unsupported options.packages/analytics-js/src/components/eventManager/RudderEventFactory.ts (1)
9-11: Mandatory logger injection
Switching from an optional to a required logger enforces consistent logging capability, eliminating silent failures.packages/analytics-js/src/components/utilities/consent.ts (2)
95-100: Mandatory logger parameter ensures consistent error reporting.By converting
loggerto a required parameter and removing optional checks (logger?.error→logger.error), this function now guarantees that error reporting will always be available. Make sure all existing callers supply theloggerargument.
118-118: Consistent enforcement of non-optional logger requirement.This matches the pattern in
getConsentManagerInfo, ensuring theloggerparameter is consistently required. Verify that all call sites provide a valid logger instance.packages/analytics-js/__tests__/components/utilities/consent.test.ts (5)
1-1: Relative import path updated.The path
../../../src/stateremains valid per prior learnings. No issue found.
7-7: New import for default logger.Importing
defaultLoggeraligns with the refactor to makeloggermandatory. Good consistency.
143-143: Test call with undefined consent options and default logger.Ensures that
getConsentManagementDatahandlesundefinedproperly while still receiving a logger. No concerns here.
166-166: Test call with valid consent options and default logger.Verifies correct handling of typical consent data and logging. Looks good.
186-186: Test call for unsupported consent provider now logs an error.Validates new parameter usage and ensures coverage of error cases. LGTM.
packages/analytics-js/src/services/StoreManager/Store.ts (4)
36-36: Revised constructor signature.Now that the constructor mandates
engineandpluginsManager, ensure references are updated throughout. No immediate concerns.
45-46: Direct assignment of required services.Assigning
config.errorHandlerandconfig.loggerunconditionally is consistent with making them required. Just confirm thatconfigis validated upstream to avoid null references.
112-112: Warn on storage quota exceeded.Directly calling
this.logger.warnavoids potential logging silences. Implementation looks sound.
148-148: Uncovered warning code path.Static analysis indicates this line is not yet covered by tests. Consider adding a test scenario for malformed or partially decrypted cookies that triggers
BAD_COOKIES_WARNING.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 148-148: packages/analytics-js/src/services/StoreManager/Store.ts#L148
Added line #L148 was not covered by testspackages/analytics-js/src/components/eventManager/utilities.ts (8)
100-108: Reserved keywords logging always available.Removing the optional
loggerparameter and callinglogger.warnunconditionally ensures consistent log output. The approach is sound.
121-121: Validation of reserved elements with a mandatory logger.No functional issues. Guaranteeing the logger ensures any warnings are correctly reported.
162-162:getMergedContextalways receives a logger.Ensures warnings like
INVALID_CONTEXT_OBJECT_WARNINGalways get logged. Implementation is consistent with the new contract.
182-182: Warn on invalidcontextobject.Directly invoking
logger.warnis clear and avoids silent failures. Looks good.
194-198:processOptionssignature includes a required logger.Ensures that all transformations log potential issues. Confirm downstream callers also supply the logger.
203-203: Statically merges context with logging.Line 203 calls
getMergedContextwith the new logger param. Great for debugging.
237-239: EnhancedgetEnrichedEventsignature.Mandating a logger is consistent with the rest of the code. No issues found.
328-328: Pass logger toprocessOptions.Makes the entire pipeline consistent in requiring a logger. Implementation is coherent.
packages/analytics-js/src/components/configManager/util/commonUtil.ts (2)
345-345: Good use of required logger
Enforcing alogger: ILoggerparameter helps maintain consistent logging across the source configuration logic.
163-163: Confirm all call sites pass a valid logger
Makingloggermandatory is beneficial for consistent logging, but ensure every caller ofupdateStorageStateFromLoadOptionssupplies a valid logger instance.✅ Verification successful
Logger is properly passed in all call sites
Based on the search results, I can confirm that
updateStorageStateFromLoadOptionsis called with a valid logger instance in all locations:
- In
ConfigManager.ts, it's called withthis.loggerwhich is an instance property- In tests (
commonUtil.test.ts), it's called withmockLoggerwhich is a mock logger instance- In the example extension (
foreground.js), it's also called withthis.loggerThere are no instances where the function is called without a logger parameter, ensuring consistent logging behavior throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that updateStorageStateFromLoadOptions is always called with a logger parameter rg -A 3 "updateStorageStateFromLoadOptions"Length of output: 14501
packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (3)
76-80: Mandatory dependencies improve reliability
Changing these properties (storeManager,pluginsManager,errorHandler,httpClient,logger) from optional to required ensures consistent behavior. Confirm that all external instantiations provide valid instances now that these can no longer beundefined.
[approve]
84-88: Required constructor parameters
Good move making these dependencies explicit. This helps prevent runtime errors due to missing dependencies.
291-291: Add test coverage for error handling
Static analysis indicates this line is not covered by tests. Consider adding a test to ensure thatthis.errorHandler.onError(...)is invoked under error conditions.✅ Verification successful
The previous command failed because the tests directory wasn't at the root. Let's try to locate test files and check for error handling coverage.
I found the test file location, let's examine its contents to verify the error handling test coverage.
Error handling is adequately tested
The error handling at line 291 inUserSessionManager.tsis covered by tests. Specifically, there's a test case inUserSessionManager.test.tsthat verifies error handling when setting server-side cookies fails:it('should handle errors when setting server-side cookies fails', () => { userSessionManager.onError = jest.fn(); // ... test setup ... expect(userSessionManager.onError).toHaveBeenCalledWith( new Error('test error'), 'Failed to set/remove cookies via server. As a fallback, the cookies will be managed client side.', ); });The test mocks the
onErrormethod and verifies that it's called with the correct error and message when an error occurs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Attempt to see if there's a test covering the onError call at line 291 rg -A 5 "onError" testsLength of output: 69
Script:
#!/bin/bash # First, let's find test files related to UserSessionManager fd -e test.ts -e spec.ts UserSessionManager # Then, let's search for error handling tests in the entire codebase rg -A 5 "onError" . --type tsLength of output: 68423
Script:
#!/bin/bash # Let's check the test coverage for UserSessionManager's error handling cat packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.tsLength of output: 65631
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 291-291: packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts#L291
Added line #L291 was not covered by testspackages/analytics-js-common/__mocks__/ErrorHandler.ts (2)
1-2: Keep imports minimal
These import statements look fine, but confirm that theIErrorHandlerusage aligns well with your new approach.
9-10: Mock now better reflects the real usage
Providing a concretehttpClientandloggerin the mock ensures the tests remain closer to production behavior.packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts (1)
14-15: LGTM! Improved type safety and consistencyGood changes that:
- Replace inline error handler type with the centralized
IErrorHandlerinterface- Make
loggerrequired, ensuring consistent logging behaviorpackages/analytics-js/src/components/eventManager/EventManager.ts (2)
17-18: LGTM! Required dependencies improve reliabilityMaking
errorHandlerandloggerrequired properties ensures proper error handling and logging throughout the component lifecycle.
31-32: Verify dependency injection in consuming codeThe constructor now requires
errorHandlerandlogger. While this improves type safety, we should verify the impact on existing instantiations.✅ Verification successful
All EventManager instantiations already provide the required dependencies
The verification shows that all existing instantiations of
EventManagerare already providing theerrorHandlerandloggerdependencies:
- In
Analytics.ts, the constructor call includes both dependencies:this.errorHandlerandthis.logger- In
EventManager.test.ts, the test instantiation providesmockErrorHandleranddefaultLogger- In
foreground.js, the constructor call includesthis.errorHandlerandthis.logger🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find EventManager instantiations ast-grep --pattern 'new EventManager($$$)'Length of output: 2052
packages/analytics-js/src/services/HttpClient/HttpClient.ts (2)
26-29: LGTM! Good separation of concerns.The constructor now only accepts the logger parameter, properly separating the logging and error handling responsibilities.
87-87: Verify error handler initialization.The optional chaining on errorHandler could mask initialization issues. Consider adding a check in the relevant test cases to ensure the error handler is properly initialized before use.
packages/analytics-js/src/services/ErrorHandler/event/event.ts (2)
63-74: LGTM! Robust error normalization.The error normalization logic properly handles different error types and includes appropriate logging for non-error cases.
33-33: Add test coverage for error handling edge cases.The following lines lack test coverage:
- Line 33: Default global code assignment
- Line 57: Error case in stack frame formatting
- Line 81: Error case in exception creation
Consider adding test cases for these scenarios.
Also applies to: 57-57, 81-81
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-33: packages/analytics-js/src/services/ErrorHandler/event/event.ts#L33
Added line #L33 was not covered by testspackages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (2)
38-41: LGTM! Clean dependency injection.The constructor now properly requires both httpClient and logger, making dependencies explicit and improving testability.
97-105: Consider implementing request retries.The error reporting HTTP request could benefit from retry logic for improved reliability. However, based on the past review comments, this can be addressed in a future iteration if needed.
packages/analytics-js/__tests__/components/configManager/cdnPaths.test.ts (2)
7-8: LGTM! Import statements are correctly added.The new imports for
SDK_CDN_BASE_URLanddefaultLoggerare properly organized and align with the changes in the test cases.
125-137: Verify error handling for plugins CDN path.The test case for invalid plugin URLs follows good practices by:
- Properly mocking and restoring the logger spy
- Verifying both the return value and error message
packages/analytics-js/src/components/eventRepository/EventRepository.ts (2)
34-35: LGTM! Improved error handling reliability.The changes enhance the reliability of error handling by:
- Making
errorHandlerandloggerrequired properties- Adding
httpClientas a required dependencyAlso applies to: 53-55
215-216: LGTM! Simplified error handling interface.The removal of the
shouldAlwaysThrowparameter streamlines the error handling interface, making it more consistent across the codebase.packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts (2)
40-42: LGTM! Enhanced dependency management.The changes improve the reliability of the
CapabilitiesManagerby:
- Making all critical dependencies (
httpClient,errorHandler,logger) required- Ensuring proper initialization in the constructor
Also applies to: 45-48
112-112: 🛠️ Refactor suggestionAdd test coverage for ad blocker detection.
The
detectAdBlockerscall withhttpClientis not covered by tests.Consider adding test cases:
describe('detectBrowserCapabilities', () => { it('should detect ad blockers when sendAdblockPage is true', () => { const mockHttpClient = { get: jest.fn() }; const manager = new CapabilitiesManager(mockHttpClient, errorHandler, logger); state.loadOptions.value.sendAdblockPage = true; state.lifecycle.sourceConfigUrl.value = 'https://example.com'; manager.detectBrowserCapabilities(); expect(mockHttpClient.get).toHaveBeenCalled(); }); });🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 112-112: packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts#L112
Added line #L112 was not covered by testspackages/analytics-js/src/components/configManager/ConfigManager.ts (1)
136-137: LGTM! Error handling simplified.The removal of the
shouldAlwaysThrowparameter simplifies the error handling pattern, making it more consistent.packages/analytics-js/src/components/pluginsManager/PluginsManager.ts (2)
Line range hint
37-45: LGTM! Improved dependency management.Making
errorHandlerandloggerrequired dependencies enhances type safety and removes the need for null checks.
342-342: LGTM! Simplified error handling.The error handling is now more direct without optional chaining, as the
errorHandleris guaranteed to be available.packages/analytics-js/src/constants/logMessages.ts (1)
22-26: LGTM! Enhanced error message clarity.The updated error message format with context and optional URL parameters improves error traceability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 23-23: packages/analytics-js/src/constants/logMessages.ts#L23
Added line #L23 was not covered by testspackages/analytics-js/src/components/core/Analytics.ts (2)
248-249: LGTM! Consistent error handling pattern.The addition of error handler and logger to UserSessionManager initialization follows the established pattern and maintains consistency across service initializations.
254-256: LGTM! Verify HTTP client usage in EventRepository.The addition of HTTP client, error handler, and logger follows the established pattern. Let's verify that the EventRepository properly utilizes the HTTP client.
✅ Verification successful
Based on the examination of the codebase, I can now provide the final response:
HTTP client is properly utilized through queue implementations
The EventRepository doesn't directly use the HTTP client but rather initializes three different types of queues that handle the HTTP communications:
- dataplaneEventsQueue: For data plane operations
- destinationsEventsQueue: For destination-specific operations
- dmtEventsQueue: For device mode transformations
Each queue is initialized with the HTTP client and follows the established pattern for making HTTP requests. The implementation is consistent with the architecture's design.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify HTTP client usage in EventRepository # Expected: Find HTTP client method calls within EventRepository ast-grep --pattern 'class EventRepository { $$$ this.httpClient.$_($$$) $$$ }'Length of output: 86
Script:
#!/bin/bash # Let's first locate the EventRepository file fd "EventRepository" # Then examine its content to understand HTTP client usage rg -A 5 "httpClient" --type tsLength of output: 25452
Script:
#!/bin/bash # Let's examine the EventRepository implementation to understand how it uses httpClient rg -A 10 "class EventRepository" packages/analytics-js/src/components/eventRepository/EventRepository.tsLength of output: 392
Script:
#!/bin/bash # Let's examine how EventRepository initializes and uses these queues rg -B 5 -A 15 "dataplaneEventsQueue|destinationsEventsQueue|dmtEventsQueue" packages/analytics-js/src/components/eventRepository/EventRepository.tsLength of output: 4996
packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (3)
34-41: Add parameter validation in constructorConsider adding validation for the required parameters to prevent potential runtime issues.
constructor(httpClient: IHttpClient, logger: ILogger) { + if (!httpClient) { + throw new Error('HttpClient is required for ErrorHandler'); + } + if (!logger) { + throw new Error('Logger is required for ErrorHandler'); + } this.httpClient = httpClient; this.logger = logger; this.attachErrorListeners(); }
44-54: Add error boundary protection for listener attachmentConsider wrapping the event listener attachments in try-catch blocks to handle potential attachment failures gracefully.
attachErrorListeners() { + try { (globalThis as typeof window).addEventListener('error', (event: ErrorEvent | Event) => { this.onError(event, ERROR_HANDLER, undefined, ErrorType.UNHANDLEDEXCEPTION); }); (globalThis as typeof window).addEventListener( 'unhandledrejection', (event: PromiseRejectionEvent) => { this.onError(event, ERROR_HANDLER, undefined, ErrorType.UNHANDLEDREJECTION); }, ); + } catch (err) { + this.logger.error('Failed to attach error listeners', err); + } }
Line range hint
140-143: Document singleton usage of defaultErrorHandlerConsider adding JSDoc comments to clarify the singleton nature and usage of the default instance.
+ /** + * Default singleton instance of ErrorHandler. + * Use this instance unless you need custom HTTP client or logger implementations. + */ const defaultErrorHandler = new ErrorHandler(defaultHttpClient, defaultLogger);packages/analytics-js/src/components/pluginsManager/PluginsManager.ts (1)
102-108: Consider adding JSDoc for deprecated plugin handling.The filtering logic for deprecated plugins is well-implemented. Consider adding documentation to explain the deprecation process and migration path for users.
Add JSDoc comment above the filter:
+ /** + * Filter out deprecated plugins and warn users about their usage + * @see deprecatedPluginsList for the complete list of deprecated plugins + */ pluginsToLoadFromConfig = pluginsToLoadFromConfig.filter(pluginName => {packages/analytics-js/src/services/StoreManager/StoreManager.ts (1)
47-47: Consider reordering constructor parametersThe constructor parameter order (
pluginsManager,errorHandler,logger) doesn't match the property declaration order (errorHandler,logger,pluginsManager). Consider aligning them for better maintainability.- constructor(pluginsManager: IPluginsManager, errorHandler: IErrorHandler, logger: ILogger) { + constructor(errorHandler: IErrorHandler, logger: ILogger, pluginsManager: IPluginsManager) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/analytics-js/__tests__/components/eventManager/EventManager.test.ts(2 hunks)packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts(5 hunks)packages/analytics-js/src/components/configManager/util/commonUtil.ts(12 hunks)packages/analytics-js/src/components/core/Analytics.ts(3 hunks)packages/analytics-js/src/components/pluginsManager/PluginsManager.ts(5 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(2 hunks)packages/analytics-js/src/services/StoreManager/Store.ts(5 hunks)packages/analytics-js/src/services/StoreManager/StoreManager.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js/tests/components/eventManager/EventManager.test.ts
- packages/analytics-js/src/components/configManager/util/commonUtil.ts
🧰 Additional context used
📓 Learnings (2)
packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1730
File: packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts:119-123
Timestamp: 2024-11-12T15:14:33.334Z
Learning: When validating custom polyfill URLs in `CapabilitiesManager`, a warning is sufficient if there is a fallback URL available.
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.
🔇 Additional comments (17)
packages/analytics-js/src/components/core/Analytics.ts (3)
101-106: LGTM! Improved error handling initialization.Good improvement to initialize the HTTP client with error handler before creating the capabilities manager. This ensures proper error handling is in place for all HTTP requests.
136-136: LGTM! Early log level configuration.Good practice to set the log level as early as possible in the initialization process.
248-249: LGTM! Consistent error handling across services.Good improvement to ensure consistent error handling by making errorHandler and logger required dependencies across all services.
Also applies to: 254-256
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (2)
1-28: LGTM! Clean reorganization of importsThe imports properly reflect the architectural shift from plugin-based to core module implementation.
128-137: LGTM! Clean breadcrumb implementationThe breadcrumb functionality is well-implemented with proper error handling.
packages/analytics-js/src/components/pluginsManager/PluginsManager.ts (3)
16-19: LGTM! Clean import organization.The grouped imports for error reporting constants improve code organization and readability.
344-344: LGTM! Simplified error handling.The direct use of
errorHandlerwithout optional chaining improves code clarity while maintaining robust error reporting.
Line range hint
203-209: LGTM! Clear warning messages.The warning message for misconfigured plugins is well-structured and provides clear information about the configuration status and affected plugins.
packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts (5)
15-15: LGTM! Good improvement in type safety.Making
httpClient,errorHandler, andloggerrequired properties ensures these essential dependencies are always available, reducing the potential for runtime errors.Also applies to: 40-42
45-48: LGTM! Clean constructor implementation.The constructor properly implements dependency injection, ensuring all required services are available at instantiation time.
128-128: LGTM! Appropriate warning level based on past feedback.The warning for invalid polyfill URL aligns with previous feedback that a warning is sufficient when a fallback URL is available.
208-208: LGTM! Simplified error handling.The error handling has been streamlined as
errorHandleris now guaranteed to be available.
112-112: Verify detectAdBlockers signature update.The function call has been updated to pass
httpClient. Let's verify the function signature in the implementation file.✅ Verification successful
Function signature change verified and correctly implemented
The verification confirms that the
detectAdBlockersfunction signature has been properly updated to accepthttpClientas its only parameter. This is evident from the implementation inpackages/analytics-js/src/components/capabilitiesManager/detection/adBlockers.ts:const detectAdBlockers = (httpClient: IHttpClient): void => {The change aligns with the usage in
CapabilitiesManager.tswhere it's being called withthis.httpClient. The implementation and tests have been updated accordingly to reflect this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify detectAdBlockers function signature update # Look for the function definition and its usage ast-grep --pattern 'function detectAdBlockers($_) { $$$ }'Length of output: 62
Script:
#!/bin/bash # Let's try a different approach to find detectAdBlockers implementation # First, let's search for the file containing detectAdBlockers rg -l "detectAdBlockers" # Then let's see the implementation with context rg -A 10 "detectAdBlockers"Length of output: 16012
packages/analytics-js/src/services/StoreManager/Store.ts (3)
Line range hint
36-47: LGTM: Constructor signature properly enforces required dependenciesThe constructor signature now correctly reflects that
engineandpluginsManagerare required, with proper initialization of all required properties. This ensures type-safety and runtime reliability.
112-112: LGTM: Simplified logger usageDirect logger usage without optional chaining improves code readability while maintaining type-safety, as the logger is now a required dependency.
Also applies to: 148-148
32-34: Architectural change: Core dependencies are now requiredMaking
errorHandler,logger, andpluginsManagerrequired properties aligns with moving error reporting to the core module. This ensures consistent error handling but represents a breaking change.Let's verify the impact on existing implementations:
✅ Verification successful
The previous pattern might have been too specific. Let's try these alternative approaches to understand the usage patterns of the Store class and assess the impact of making these properties required.
I need to see the actual implementation to understand how these dependencies are currently handled and verify the impact of making them required.
Making core dependencies required is a safe change
Based on the code analysis, making
errorHandler,logger, andpluginsManagerrequired properties is a safe change that aligns with the existing implementation:
The
Storeconstructor already expects these dependencies:
errorHandlerandloggerare provided viaconfigparameterpluginsManageris a direct constructor parameterThe only production usage is through
StoreManager, which properly provides all required dependencies:this.stores[storeConfig.id] = new Store(storeConfig, storageEngine, this.pluginsManager);
- All test cases also provide these dependencies:
new Store({ name: 'name', id: 'id', validKeys: QueueStatuses, errorHandler: defaultErrorHandler, logger: defaultLogger, }, getStorageEngine('localStorage'), pluginsManager);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Store instantiations that might be affected ast-grep --pattern 'new Store($config, $engine, $plugins)'Length of output: 60
Script:
#!/bin/bash # Let's try a different pattern to find Store instantiations and usages rg "new Store\(" -A 3 # Also search for Store class extensions or implementations ast-grep --pattern 'class $_ extends Store' # Search for Store type annotations rg "Store[\s]*[:<]"Length of output: 11572
Script:
#!/bin/bash # Let's check the Store constructor implementation and its usage in StoreManager cat packages/analytics-js/src/services/StoreManager/Store.ts cat packages/analytics-js/src/services/StoreManager/StoreManager.ts # Also check test files to understand how these dependencies are provided cat packages/analytics-js/__tests__/services/StoreManager/Store.test.tsLength of output: 19914
packages/analytics-js/src/services/StoreManager/StoreManager.ts (1)
110-111: LGTM: Proper dependency propagationThe store initialization correctly passes the required
errorHandlerandloggerto new stores, maintaining consistency with the new Store requirements.
packages/analytics-js/src/services/StoreManager/StoreManager.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/analytics-js/src/services/StoreManager/StoreManager.ts (1)
47-52: Consider parameter ordering and method binding improvements.While the constructor implementation is functionally correct, consider these improvements:
- Reorder parameters to follow common dependency injection patterns: business dependencies first, followed by cross-cutting concerns.
- Use an arrow function for
onErrorto avoid manual binding.- constructor(pluginsManager: IPluginsManager, errorHandler: IErrorHandler, logger: ILogger) { + constructor(pluginsManager: IPluginsManager, logger: ILogger, errorHandler: IErrorHandler) { this.errorHandler = errorHandler; this.logger = logger; this.pluginsManager = pluginsManager; - this.onError = this.onError.bind(this); } - onError(error: unknown) { + onError = (error: unknown) => { this.errorHandler.onError(error, STORE_MANAGER); - } + };packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts (1)
53-53: Consider adding debug logging.The error handling implementation is solid and aligns with the centralized error reporting goal. Consider adding a debug log before reporting the error to aid in local debugging.
- this.errorHandler.onError(error, EXTERNAL_SRC_LOADER); + this.logger.debug('External source loading failed', { error }); + this.errorHandler.onError(error, EXTERNAL_SRC_LOADER);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts(2 hunks)packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts(1 hunks)packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts(10 hunks)packages/analytics-js/src/constants/logMessages.ts(5 hunks)packages/analytics-js/src/services/StoreManager/StoreManager.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/analytics-js/tests/components/pluginsManager/PluginsManager.test.ts
- packages/analytics-js/tests/components/configManager/ConfigManager.test.ts
- packages/analytics-js/src/constants/logMessages.ts
🔇 Additional comments (6)
packages/analytics-js/src/services/StoreManager/StoreManager.ts (4)
43-45: LGTM! Properties correctly defined as required.The change from optional to required properties for
errorHandler,logger, andpluginsManageraligns well with the PR's objective of moving error reporting to the core module. This ensures that error handling, logging, and plugin management capabilities are always available.
110-111: LGTM! Proper error handler and logger propagation.The error handler and logger are correctly passed to each store during initialization, ensuring consistent error handling throughout the storage layer.
195-198: LGTM! Proper logger usage with context.The logger is correctly used to warn about storage type changes, providing clear context through the
STORE_MANAGERidentifier and detailed message construction.
223-223: LGTM! Optional chaining removed as previously suggested.The error handler is now correctly used without optional chaining, consistent with it being a required property.
packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts (2)
18-19: Well-structured constructor changes.The constructor signature changes are consistent with the property declarations while maintaining backward compatibility for the timeout parameter.
13-14: Verify migration path for existing consumers.The change from optional to required properties is a breaking change. While this aligns with the PR's objective of centralizing error reporting, we should ensure there's a clear migration path for existing consumers.
✅ Verification successful
All consumers already provide required properties - no migration needed
Based on the search results, all existing instantiations of
ExternalSrcLoaderalready provide botherrorHandlerandloggerparameters:
Analytics.ts:new ExternalSrcLoader(this.errorHandler, this.logger)CapabilitiesManager.ts:new ExternalSrcLoader(this.errorHandler, this.logger)- Chrome extension examples also properly initialize with both parameters
- Test file uses
defaultErrorHandleranddefaultLoggerThe change from optional to required properties is safe as all consumers are already compliant with the new signature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing instantiations of ExternalSrcLoader to identify affected consumers ast-grep --pattern 'new ExternalSrcLoader($$$)'Length of output: 1386
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (2)
44-54: Consider adding cleanup for event listeners.The event listeners are attached but never cleaned up, which could lead to memory leaks if the ErrorHandler instance is recreated multiple times.
class ErrorHandler implements IErrorHandler { httpClient: IHttpClient; logger: ILogger; + private errorListener: (event: ErrorEvent | Event) => void; + private rejectionListener: (event: PromiseRejectionEvent) => void; constructor(httpClient: IHttpClient, logger: ILogger) { this.httpClient = httpClient; this.logger = logger; this.attachErrorListeners(); } attachErrorListeners() { - (globalThis as typeof window).addEventListener('error', (event: ErrorEvent | Event) => { + this.errorListener = (event: ErrorEvent | Event) => { this.onError(event, ERROR_HANDLER, undefined, ErrorType.UNHANDLEDEXCEPTION); - }); + }; + (globalThis as typeof window).addEventListener('error', this.errorListener); - (globalThis as typeof window).addEventListener( - 'unhandledrejection', - (event: PromiseRejectionEvent) => { + this.rejectionListener = (event: PromiseRejectionEvent) => { this.onError(event, ERROR_HANDLER, undefined, ErrorType.UNHANDLEDREJECTION); - }, - ); + }; + (globalThis as typeof window).addEventListener('unhandledrejection', this.rejectionListener); } + cleanup() { + (globalThis as typeof window).removeEventListener('error', this.errorListener); + (globalThis as typeof window).removeEventListener('unhandledrejection', this.rejectionListener); + }
97-105: Add error handling for HTTP request.The HTTP request for error reporting lacks error handling. While this might be intentional to avoid recursive error handling, it's good practice to at least log failed attempts.
// send it to metrics service - this.httpClient?.getAsyncData({ + this.httpClient.getAsyncData({ url: state.metrics.metricsServiceUrl.value as string, options: { method: 'POST', data: getErrorDeliveryPayload(bugsnagPayload, state), sendRawData: true, }, isRawResponse: true, - }); + }).catch(err => { + // Log but don't recursively handle to avoid infinite loops + console.warn('Failed to send error to metrics service:', err); + });packages/analytics-js/src/services/ErrorHandler/utils.ts (2)
39-42: Remove redundant case clause.The
case ErrorType.HANDLEDEXCEPTIONis redundant as it falls through to the default case.case ErrorType.UNHANDLEDREJECTION: { return (err as PromiseRejectionEvent).reason; } - case ErrorType.HANDLEDEXCEPTION: default: return err;🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
137-156: Consider caching SDK file name prefixes.The
SDK_FILE_NAME_PREFIXES()function is called for every error check. Consider caching the result to improve performance.+const cachedPrefixes = SDK_FILE_NAME_PREFIXES(); + const isSDKError = (exception: Exception) => { const errorOrigin = exception.stacktrace?.[0]?.file; if (!errorOrigin || typeof errorOrigin !== 'string') { return false; } const srcFileName = errorOrigin.substring(errorOrigin.lastIndexOf('/') + 1); const paths = errorOrigin.split('/'); const parentFolderName = paths[paths.length - 2]; return ( parentFolderName === CDN_INT_DIR || - SDK_FILE_NAME_PREFIXES().some( + cachedPrefixes.some( prefix => srcFileName.startsWith(prefix) && srcFileName.endsWith('.js'), ) ); };packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts (2)
82-97: Add test for maximum breadcrumb limit.The test suite should verify that breadcrumbs are properly limited to prevent memory issues.
it('should limit the number of breadcrumbs', () => { const maxBreadcrumbs = 100; // Adjust based on your actual limit // Add more than the limit for (let i = 0; i < maxBreadcrumbs + 10; i++) { errorHandlerInstance.leaveBreadcrumb(`message ${i}`); } expect(state.reporting.breadcrumbs.value.length).toBe(maxBreadcrumbs); expect(state.reporting.breadcrumbs.value[maxBreadcrumbs - 1].name).toBe(`message ${maxBreadcrumbs + 9}`); });
170-193: Add test for error payload structure.The test verifies the HTTP call but doesn't validate the structure of the error payload.
Add assertions to verify the payload structure:
const sentPayload = JSON.parse( (defaultHttpClient.getAsyncData as jest.Mock).mock.calls[0][0].options.data ); expect(sentPayload).toMatchObject({ version: expect.any(String), message_id: expect.any(String), source: { name: expect.any(String), sdk_version: expect.any(String), write_key: 'dummy-write-key', install_type: 'npm', }, errors: { payloadVersion: '5', notifier: expect.any(Object), events: expect.any(Array), }, });packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts (5)
19-21: Consider using a more deterministic UUID mock patternWhile the current mock works, consider using a counter-based UUID mock for better test predictability and debugging. This helps track the order of UUID generation across different test cases.
jest.mock('@rudderstack/analytics-js-common/utilities/uuId', () => ({ - generateUUID: jest.fn().mockReturnValue('test_uuid'), + generateUUID: (() => { + let counter = 0; + return jest.fn(() => `test_uuid_${++counter}`); + })(), }));
28-40: Enhance test coverage for createNewBreadcrumbWhile the basic test case is good, consider adding tests for:
- Breadcrumb with metadata
- Edge cases with empty/null messages
- Different breadcrumb types
describe('createNewBreadcrumb', () => { it('should create and return a breadcrumb', () => { const msg = 'sample message'; const breadcrumb = createNewBreadcrumb(msg); expect(breadcrumb).toStrictEqual({ metaData: {}, type: 'manual', timestamp: expect.any(Date), name: msg, }); }); + + it('should create breadcrumb with metadata', () => { + const msg = 'sample message'; + const metadata = { key: 'value' }; + const breadcrumb = createNewBreadcrumb(msg, metadata); + expect(breadcrumb.metaData).toEqual(metadata); + }); + + it('should handle empty message', () => { + const breadcrumb = createNewBreadcrumb(''); + expect(breadcrumb.name).toBe(''); + }); });
42-48: Expand URL test cases for better coverageAdd test cases for:
- URLs without query parameters
- URLs with hash fragments
- URLs with multiple query parameters
- Edge cases (empty URL, malformed URL)
describe('getURLWithoutQueryString', () => { it('should return url without query param ', () => { (window as any).location.href = 'https://www.test-host.com?key1=1234&key2=true'; const urlWithoutSearchParam = getURLWithoutQueryString(); expect(urlWithoutSearchParam).toEqual('https://www.test-host.com/'); }); + + it('should handle URL with hash fragment', () => { + (window as any).location.href = 'https://www.test-host.com?key1=1234#section1'; + expect(getURLWithoutQueryString()).toEqual('https://www.test-host.com/'); + }); + + it('should handle URL without query params', () => { + (window as any).location.href = 'https://www.test-host.com'; + expect(getURLWithoutQueryString()).toEqual('https://www.test-host.com/'); + }); });
600-612: Enhance error notification filtering test coverageAdd more test cases to cover different error message patterns and edge cases.
describe('isAllowedToBeNotified', () => { + const testCases = [ + { message: 'dummy error', expected: true, desc: 'general error' }, + { message: 'The request failed', expected: false, desc: 'filtered error' }, + { message: '', expected: true, desc: 'empty message' }, + { message: null, expected: true, desc: 'null message' }, + { message: 'Failed to load resource', expected: false, desc: 'resource error' }, + ]; + + test.each(testCases)('should handle $desc correctly', ({ message, expected }) => { + const result = isAllowedToBeNotified({ message } as unknown as Exception); + expect(result).toBe(expected); + }); - it('should return true if the error is allowed to be notified', () => { - const result = isAllowedToBeNotified({ message: 'dummy error' } as unknown as Exception); - expect(result).toBeTruthy(); - }); - - it('should return false if the error is not allowed to be notified', () => { - const result = isAllowedToBeNotified({ - message: 'The request failed', - } as unknown as Exception); - expect(result).toBeFalsy(); - }); });
1-646: Add JSDoc comments for better test documentationConsider adding JSDoc comments to describe the purpose and requirements of each test suite. This will help other developers understand the test scenarios better.
+/** + * Test suite for error reporting utilities + * @group unit + * @group error-handling + */ describe('Error Reporting utilities', () => { // ... existing code });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/analytics-js-common/src/types/Metrics.ts(2 hunks)packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts(1 hunks)packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts(1 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(2 hunks)packages/analytics-js/src/services/ErrorHandler/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js-common/src/types/Metrics.ts
🧰 Additional context used
📓 Learnings (1)
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.
🪛 Biome (1.9.4)
packages/analytics-js/src/services/ErrorHandler/utils.ts
[error] 39-39: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🔇 Additional comments (2)
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (1)
34-41: LGTM! Constructor changes improve dependency management.The constructor now requires explicit dependencies (
httpClientandlogger), making the component's dependencies clear and improving testability.packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts (1)
1-646: Overall test implementation looks good!The test suite provides good coverage of the error handling utilities. The suggested improvements are mainly for enhancing maintainability and coverage.
packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts
Outdated
Show resolved
Hide resolved
|
PR Description
I've moved the error reporting functionality to the core SDK and deleted the ErrorReporting and Bugsnag plugins.
The SDK can report errors as soon as they load and don't have to wait for plugins to finish loading.
Now, there are no parts in the SDK that throw errors left to be handled by the global exception handlers.
Additional updates:
Linear task (optional)
https://linear.app/rudderstack/issue/SDK-2826/move-error-reporting-functionality-to-the-core-sdk
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the high-level release notes:
Deprecation
Error Handling
Architecture
Performance
The changes represent a significant restructuring of the error reporting system, focusing on simplification and modularity.