feat: set proper grouping hash for all errors #2246
Conversation
📝 Walkthrough""" WalkthroughThis change refactors error handling throughout the analytics codebase by standardizing the use of a structured error info object in all Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant ErrorHandler
participant Logger
Note over Component: An error occurs in a component (e.g., plugin, manager, loader)
Component->>ErrorHandler: onError({ error, context, customMessage, errorType?, groupingHash? })
ErrorHandler->>Logger: Log error with context and message
ErrorHandler->>ErrorHandler: Generate grouping hash (if applicable)
ErrorHandler-->>Component: (Optional) error reporting, grouping, or further handling
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🔭 Outside diff range comments (1)
packages/analytics-js-plugins/src/deviceModeDestinations/index.ts (1)
74-81:⚠️ Potential issueUpdate callback type to expose the new error parameter
externalScriptOnLoadis still typed as(id?: string) => unknown, but the loader now invokes it as(id: string, err?: SDKError).
Consumers supplying a callback that handles the error will get a type error, and consumers unaware of the second parameter won’t see it in autocomplete.- externalScriptOnLoad?: (id?: string) => unknown, + externalScriptOnLoad?: (id: string, err?: SDKError) => unknown,This cascades to every place the plugin is used, so consider exporting a dedicated type alias from
ExternalSrcLoaderto keep them in sync.
🧹 Nitpick comments (7)
packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts (2)
174-178: Consider adding integration context to the timeout error
INTEGRATION_READY_TIMEOUT_ERRORis called with only the timeout value, so the resultingError.messagedoes not indicate which integration failed to become ready. When multiple integrations are loaded in parallel this can make debugging difficult.-reject(new Error(INTEGRATION_READY_TIMEOUT_ERROR(READY_CHECK_TIMEOUT_MS))); +reject( + new Error( + INTEGRATION_READY_TIMEOUT_ERROR(dest.displayName, READY_CHECK_TIMEOUT_MS), + ), +);(Assuming the helper can accept
displayNameas its first argument.)
258-264: Grouping-hash still varies per integration, partially defeating grouping
groupingHashis computed withdest.displayName, so separate integrations of the same type (“Google Analytics”, “Google Analytics 2”) will still produce different hashes and appear in different groups.
If the goal is to ignore IDs entirely, consider passing a literal constant (or a value derived only from the error type) to guarantee a single Bugsnag group.- groupingHash: INTEGRATION_READY_CHECK_ERROR(dest.displayName), + groupingHash: INTEGRATION_READY_CHECK_ERROR(), // no dynamic partspackages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts (1)
28-37: Shadowedidparameter harms readabilityThe arrow-function parameter
idshadows the outer-scopeidfromconfig, which makes it harder to tell which value is being passed to the caller. Renaming the inner variable clarifies intent.- .then((id: string) => { + .then((resolvedId: string) => { if (!isFireAndForget) { - callback(id); + callback(resolvedId); } }) - .catch((err: SDKError) => { + .catch((err: SDKError) => { if (!isFireAndForget) { - callback(id, err); + callback(config.id, err); } });packages/analytics-js-plugins/src/deviceModeDestinations/index.ts (1)
96-105: Minor: duplicate work inside inline callback
customMessageis built twice (INTEGRATION_SDK_LOAD_ERROR(...)) – once forcustomMessageand once forgroupingHash. Building it once and re-using the result saves a function call and keeps both fields guaranteed identical.- ((id: string, err?: SDKError) => { + ((id: string, err?: SDKError) => { if (err) { - const customMessage = INTEGRATION_SDK_LOAD_ERROR(dest.displayName); + const customMessage = INTEGRATION_SDK_LOAD_ERROR(dest.displayName); errorHandler?.onError({ error: err, context: DEVICE_MODE_DESTINATIONS_PLUGIN, customMessage, - groupingHash: customMessage, + groupingHash: customMessage, });packages/analytics-js/src/components/pluginsManager/PluginsManager.ts (3)
280-280: Consider adding a groupingHash for consistency.This error handler call is missing both a custom message and a groupingHash, unlike the other updated calls in this file. For consistency and better error grouping, consider adding these parameters.
- this.onError(err); + this.onError(err, 'Failed to register remote plugins', 'RemotePluginsRegistrationError');
291-291: Consider adding groupingHash parameter for extension point errors.For consistent error grouping, consider adding a groupingHash parameter to these extension point errors. This would help group similar extension point errors together in Bugsnag.
- this.onError(e, extPoint); + this.onError(e, extPoint, `ExtensionPointError_${extPoint}`);Also applies to: 303-303
317-317: Consider adding groupingHash for plugin registration/unregistration errors.For consistent error handling across the codebase, consider adding a groupingHash parameter to these plugin registration and unregistration errors to ensure proper grouping in Bugsnag.
For the register error:
- this.onError(e, `Failed to register plugin "${plugin.name}"`); + this.onError(e, `Failed to register plugin "${plugin.name}"`, `PluginRegistrationError_${plugin.name}`);For the unregister error:
- this.onError(e, `Failed to unregister plugin "${localPlugin().name}"`); + this.onError(e, `Failed to unregister plugin "${localPlugin().name}"`, `PluginUnregistrationError_${localPlugin().name}`);Also applies to: 328-328
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 1eeee94 and 63badf12085bbc00fec7e52fbf6bd48782687dbe.
⛔ Files ignored due to path filters (1)
nx.jsonis excluded by!**/*.json
📒 Files selected for processing (43)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts(4 hunks)packages/analytics-js-common/src/constants/logMessages.ts(1 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts(3 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts(3 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts(1 hunks)packages/analytics-js-common/src/types/ErrorHandler.ts(1 hunks)packages/analytics-js-plugins/__tests__/customConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/iubendaConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/ketchConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/oneTrustConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/storageMigrator/index.test.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/customConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/index.ts(5 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts(4 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts(1 hunks)packages/analytics-js-plugins/src/iubendaConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/ketchConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts(2 hunks)packages/analytics-js-plugins/src/oneTrustConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/storageMigrator/index.ts(1 hunks)packages/analytics-js-plugins/src/storageMigrator/logMessages.ts(1 hunks)packages/analytics-js/.size-limit.mjs(5 hunks)packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts(2 hunks)packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts(1 hunks)packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts(11 hunks)packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts(2 hunks)packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts(2 hunks)packages/analytics-js/src/components/configManager/ConfigManager.ts(1 hunks)packages/analytics-js/src/components/core/Analytics.ts(2 hunks)packages/analytics-js/src/components/pluginsManager/PluginsManager.ts(4 hunks)packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts(2 hunks)packages/analytics-js/src/constants/logMessages.ts(1 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(5 hunks)packages/analytics-js/src/services/ErrorHandler/utils.ts(3 hunks)packages/analytics-js/src/services/HttpClient/HttpClient.ts(1 hunks)packages/analytics-js/src/services/StoreManager/Store.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (15)
packages/analytics-js-plugins/src/customConsentManager/index.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-10-28T08:03:12.163Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the issue regarding inconsistent error handling approaches in `getDMTDeliveryPayload` is no longer valid.
packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-10-28T08:03:12.163Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the issue regarding inconsistent error handling approaches in `getDMTDeliveryPayload` is no longer valid.
packages/analytics-js-plugins/src/ketchConsentManager/index.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-10-28T08:03:12.163Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the issue regarding inconsistent error handling approaches in `getDMTDeliveryPayload` is no longer valid.
packages/analytics-js-plugins/__tests__/iubendaConsentManager/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-10-08T15:52:59.819Z
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-plugins/src/oneTrustConsentManager/index.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-10-28T08:03:12.163Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the issue regarding inconsistent error handling approaches in `getDMTDeliveryPayload` is no longer valid.
packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-10-28T08:03:12.163Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the issue regarding inconsistent error handling approaches in `getDMTDeliveryPayload` is no longer valid.
packages/analytics-js-plugins/src/storageMigrator/index.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-10-28T08:03:12.163Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the issue regarding inconsistent error handling approaches in `getDMTDeliveryPayload` is no longer valid.
packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.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-09T06:41:24.959Z
Learning: The `onerror` handler for script elements has the signature `(event: Event | string, source?: string, lineno?: number, colno?: number, error?: Error)`.
packages/analytics-js-plugins/src/iubendaConsentManager/index.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-10-28T08:03:12.163Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the issue regarding inconsistent error handling approaches in `getDMTDeliveryPayload` is no longer valid.
packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.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-08T06:58:29.573Z
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/__tests__/services/HttpClient/HttpClient.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-10-08T15:52:59.819Z
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-plugins/src/beaconQueue/index.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-plugins/src/beaconQueue/index.ts:84-87
Timestamp: 2024-10-15T11:40:19.087Z
Learning: In the `BeaconQueue` plugin of the RudderStack Analytics JS SDK, when catching errors in the `queueProcessCallback`, the error should not be passed to the `done` callback; instead, `done(null)` is used to remove the item from the queue without retrying.
packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-10-28T08:03:12.163Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the issue regarding inconsistent error handling approaches in `getDMTDeliveryPayload` is no longer valid.
packages/analytics-js/src/components/core/Analytics.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js/src/components/core/Analytics.ts:125-127
Timestamp: 2024-10-28T08:19:43.438Z
Learning: In `packages/analytics-js/src/components/core/Analytics.ts`, inputs to the `clone` function are sanitized prior, so error handling for clone operations is not needed.
packages/analytics-js-plugins/src/deviceModeDestinations/index.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-10-28T08:03:12.163Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the issue regarding inconsistent error handling approaches in `getDMTDeliveryPayload` is no longer valid.
🧬 Code Graph Analysis (18)
packages/analytics-js-plugins/__tests__/iubendaConsentManager/index.test.ts (2)
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (1)
defaultErrorHandler(196-196)packages/analytics-js-common/__mocks__/ErrorHandler.ts (1)
defaultErrorHandler(16-16)
packages/analytics-js-plugins/__tests__/oneTrustConsentManager/index.test.ts (1)
packages/analytics-js-common/__mocks__/ErrorHandler.ts (1)
defaultErrorHandler(16-16)
packages/analytics-js-common/src/constants/logMessages.ts (1)
packages/analytics-js-common/src/utilities/checks.ts (1)
isString(84-84)
packages/analytics-js-plugins/src/storageMigrator/index.ts (1)
packages/analytics-js-plugins/src/storageMigrator/logMessages.ts (1)
STORAGE_MIGRATION_ERROR(4-4)
packages/analytics-js-plugins/__tests__/customConsentManager/index.test.ts (2)
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (1)
defaultErrorHandler(196-196)packages/analytics-js-common/__mocks__/ErrorHandler.ts (1)
defaultErrorHandler(16-16)
packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts (1)
packages/analytics-js-common/src/constants/logMessages.ts (1)
SCRIPT_LOAD_ERROR(27-27)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (2)
packages/analytics-js-common/src/types/ErrorHandler.ts (1)
SDKError(5-5)packages/analytics-js-common/__fixtures__/fixtures.ts (1)
dummyDataplaneHost(198-198)
packages/analytics-js-common/src/types/ErrorHandler.ts (2)
examples/chrome-extension/content-script-v3/foreground.js (1)
ErrorType(609-609)packages/analytics-js-common/src/types/HttpClient.ts (1)
IHttpClient(57-68)
packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (2)
packages/analytics-js-common/src/constants/loggerContexts.ts (1)
USER_SESSION_MANAGER(22-22)packages/analytics-js/src/constants/logMessages.ts (1)
FAILED_SETTING_COOKIE_FROM_SERVER_GLOBAL_ERROR(315-315)
packages/analytics-js/src/services/ErrorHandler/utils.ts (2)
packages/analytics-js-common/src/types/ErrorHandler.ts (1)
SDKError(5-5)packages/analytics-js-common/src/types/ApplicationState.ts (1)
ApplicationState(189-206)
packages/analytics-js/src/services/HttpClient/HttpClient.ts (1)
packages/analytics-js-common/src/constants/loggerContexts.ts (1)
HTTP_CLIENT(30-30)
packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts (1)
packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts (1)
INTEGRATION_EVENT_FORWARDING_ERROR(14-14)
packages/analytics-js/src/components/core/Analytics.ts (2)
packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts (1)
ExternalSrcLoader(41-41)packages/analytics-js-common/src/constants/loggerContexts.ts (1)
ANALYTICS_CORE(32-32)
packages/analytics-js-plugins/__tests__/ketchConsentManager/index.test.ts (2)
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (1)
defaultErrorHandler(196-196)packages/analytics-js-common/__mocks__/ErrorHandler.ts (1)
defaultErrorHandler(16-16)
packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts (2)
packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts (4)
INTEGRATION_READY_TIMEOUT_ERROR(24-24)DESTINATION_INTEGRATIONS_DATA_ERROR(23-23)INTEGRATION_READY_CHECK_ERROR(25-25)INTEGRATION_INIT_ERROR(22-22)packages/analytics-js-plugins/src/deviceModeDestinations/constants.ts (2)
READY_CHECK_TIMEOUT_MS(8-8)DEVICE_MODE_DESTINATIONS_PLUGIN(9-9)
packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts (2)
examples/chrome-extension/content-script-v3/foreground.js (1)
DEFAULT_EXT_SRC_LOAD_TIMEOUT_MS(541-541)packages/analytics-js-common/src/types/ErrorHandler.ts (1)
SDKError(5-5)
packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts (1)
packages/analytics-js-common/src/types/ErrorHandler.ts (1)
SDKError(5-5)
packages/analytics-js-plugins/src/beaconQueue/logMessages.ts (2)
packages/analytics-js-common/src/constants/logMessages.ts (1)
LOG_CONTEXT_SEPARATOR(25-25)packages/analytics-js-plugins/src/shared-chunks/common.ts (1)
LOG_CONTEXT_SEPARATOR(31-31)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Bundle size checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Security and code quality checks
🔇 Additional comments (76)
packages/analytics-js/.size-limit.mjs (1)
10-10:Details
❓ Verification inconclusive
Size limits increased across multiple build targets
The size limits have been increased by approximately 0.5 KiB across multiple build targets to accommodate the error handling refactoring. This aligns with the changes made to standardize error object structures throughout the codebase.
Also applies to: 33-33, 39-39, 45-45, 56-56, 80-80, 86-86, 116-116
🏁 Script executed:
#!/bin/bash # Check the actual bundle sizes compared to the new limits # Get the current file sizes of the bundles echo "Checking current bundle sizes against new limits:" filesList=( "dist/npm/legacy/esm/index.mjs:48.5 KiB" "dist/npm/modern/esm/index.mjs:27.5 KiB" "dist/npm/modern/cjs/index.cjs:27.5 KiB" "dist/npm/modern/umd/index.js:27.5 KiB" "dist/npm/legacy/bundled/esm/index.mjs:48.5 KiB" "dist/npm/modern/bundled/cjs/index.cjs:40 KiB" "dist/npm/modern/bundled/umd/index.js:40 KiB" "dist/npm/modern/content-script/cjs/index.cjs:40 KiB" ) for item in "${filesList[@]}"; do file="${item%%:*}" limit="${item##*:}" if [ -f "$file" ]; then size=$(du -k "$file" | cut -f1) size_kb=$(echo "scale=2; $size" | bc) limit_kb=$(echo "$limit" | sed 's/ KiB//') echo "$file: Current size: $size_kb KB, New limit: $limit_kb KB" else echo "$file not found" fi doneLength of output: 2383
Action Required: Verify Bundle Sizes Post-Build
I wasn’t able to locate any
dist/artifacts in the review environment, so please:
Run your build (e.g.
npm run build) to generate the bundles underpackages/analytics-js/dist/….Re-run the size-check script against these paths to ensure each target now falls within the updated limits:
• dist/npm/legacy/esm/index.mjs limit 48.5 KiB
• dist/npm/modern/esm/index.mjs limit 27.5 KiB
• dist/npm/modern/cjs/index.cjs limit 27.5 KiB
• dist/npm/modern/umd/index.js limit 27.5 KiB
• dist/npm/legacy/bundled/esm/index.mjs limit 48.5 KiB
• dist/npm/modern/bundled/cjs/index.cjs limit 40 KiB
• dist/npm/modern/bundled/umd/index.js limit 40 KiB
• dist/npm/modern/content-script/cjs/index.cjs limit 40 KiBOnce confirmed, we can safely approve the size-limit bump that accommodates the error-handling refactor.
packages/analytics-js-plugins/src/customConsentManager/index.ts (1)
77-81: Error handling now uses structured objectsThe error handler call has been updated to use a structured object instead of positional parameters, which aligns with the PR objective of standardizing error handling and improving Bugsnag error grouping.
packages/analytics-js-plugins/src/ketchConsentManager/index.ts (1)
111-115: Error handling now uses structured objectsThe error handler call has been updated to use a structured object instead of positional parameters, which aligns with the PR objective of standardizing error handling and improving Bugsnag error grouping.
packages/analytics-js-plugins/__tests__/customConsentManager/index.test.ts (1)
254-259: Test assertion updated to match new error handler interfaceThe test assertion has been updated to match the new error handler interface that accepts a single object with named properties instead of three separate arguments. This correctly verifies the updated implementation.
packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts (1)
93-96: Alignment with new error handler API structure.The error handling has been updated to use the new object-based parameter structure for
onError(), which aligns with the broader refactoring effort to standardize error handling throughout the codebase.This change will make it easier to add a
groupingHashin the future if needed for better error grouping in Bugsnag, which is the main objective of this PR.packages/analytics-js/src/constants/logMessages.ts (1)
41-41: Simplified BREADCRUMB_ERROR message to a static string.The constant has been changed from what was likely a function that included context to a simple string. This aligns with the new error handling pattern where context is provided separately in the error object structure rather than embedded in the message.
This change supports the PR objective by allowing for more consistent error grouping since the error message is now static and won't include variable context information.
packages/analytics-js-plugins/src/storageMigrator/logMessages.ts (1)
1-2: Improved error message formatting.Adding double quotes around the key variable in the error message improves readability and better matches the formatting style used in other error messages across the codebase.
This small formatting improvement contributes to the overall consistency of error messages, which indirectly helps with proper error grouping.
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (1)
140-140: Updated test expectation to match simplified error message.The test has been updated to expect a more generic error message that omits destination-specific information. This change aligns with the PR's objective to improve error grouping by making error messages more consistent.
The simplified timeout error message
"A timeout of 11000 ms occurred"will allow for better grouping of similar timeout errors in error reporting tools like Bugsnag, regardless of which destination caused the timeout.packages/analytics-js-plugins/src/oneTrustConsentManager/index.ts (1)
118-122: Consistent error handling structure - looks good!The refactoring from individual parameters to a structured object follows the standard pattern for error handling across the codebase. This change supports the PR objective of improving error message grouping in Bugsnag.
packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts (2)
170-175: Updated test expectations to match new error handler interface.The test assertion now correctly expects a structured object with named properties rather than positional arguments.
188-191: Updated test expectations to match new error handler interface.The test assertion now correctly expects a structured object with named properties rather than positional arguments.
packages/analytics-js-plugins/src/iubendaConsentManager/index.ts (1)
101-105: Consistent error handling structure - looks good!The refactoring from individual parameters to a structured object follows the standard pattern for error handling across the codebase. This change supports the PR objective of improving error message grouping in Bugsnag.
packages/analytics-js-plugins/__tests__/iubendaConsentManager/index.test.ts (1)
320-325: Updated test expectations to match new error handler interface.The test assertion correctly expects the new structured object format with named properties: error, context, and customMessage.
packages/analytics-js-plugins/__tests__/oneTrustConsentManager/index.test.ts (1)
183-188: Approval: Test updated to use new error handler interface structureThe test assertion has been properly updated to use the new object-based error handler interface, which aligns with the broader refactoring for standardized error handling throughout the codebase.
packages/analytics-js-plugins/__tests__/ketchConsentManager/index.test.ts (1)
310-315: Approval: Test updated to use new error handler interface structureThe test assertion has been correctly updated to use the new object-based error handler interface, maintaining consistency with the standardized error handling approach implemented across the codebase.
packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts (1)
2106-2106: Approval: Added grouping hash to error handler callThe change adds a third parameter to the
onErrormethod call to set the grouping hash value. Using the same string as both the error message and grouping hash ensures consistent error grouping in error reporting tools like Bugsnag.packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts (4)
187-192: Approval: Updated test assertion for object-based error handler interfaceTest assertion correctly updated to verify that
onErroris called with a single structured object containing error details, including explicit undefined values for optional parameters.
201-205: Approval: Updated test assertion for error with custom messageTest assertion correctly updated to verify the error handler call with a custom message in the new object-based format.
212-218: Approval: Updated test assertion for SyntaxError handlingTest assertion properly updated to verify error handler interaction with SyntaxError objects in the new object-based format.
226-231: Approval: Updated test assertion with explicit undefined valuesTest assertion correctly updated to verify the error handler call with explicit undefined values for optional parameters, maintaining test precision.
packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts (1)
19-20: Improved error message format for better grouping and readabilityThe update to
DMT_EXCEPTIONprovides a clearer and more descriptive error message than the previous format. This change aligns with the PR's goal of improving error message grouping in Bugsnag by using consistent formatting with quoted destination names.packages/analytics-js-common/src/constants/logMessages.ts (2)
1-1: Added necessary import for supporting the enhanced error messageThe added import for
isStringis required for the enhancedSCRIPT_LOAD_ERRORfunction implementation.
8-9: Enhanced error message with event type contextThe
SCRIPT_LOAD_ERRORfunction has been improved to include the error event type in the message. This provides better context for debugging and will help with proper error grouping in Bugsnag by creating distinct error messages based on the failure type rather than generic messages.packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts (1)
149-154: Standardized error handling with structured object parameterThe error handler call has been updated to use a structured object containing error details instead of multiple positional arguments. This change is part of the broader standardization effort for error handling throughout the codebase and aligns with the PR objective of setting proper grouping hash for all errors.
packages/analytics-js-plugins/src/beaconQueue/logMessages.ts (2)
12-13: Enhanced error message with URL contextThe
BEACON_QUEUE_SEND_ERRORfunction now accepts and includes the URL in the error message, providing better context for debugging and error grouping.
15-16: Clarified error behavior with retry informationThe
BEACON_QUEUE_DELIVERY_ERRORmessage now explicitly states that failed events will be retried, which provides important information about the system behavior when failures occur.packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts (2)
49-49: Constructor updated to align with ExternalSrcLoader changesThe constructor now correctly instantiates ExternalSrcLoader with only the logger parameter, reflecting the removal of the errorHandler dependency from ExternalSrcLoader. This change centralizes error handling through CapabilitiesManager's own onError method.
203-208: Standardized error handling with grouping hashThe onError method has been properly updated to accept an optional groupingHash parameter and now calls the error handler with a structured object. This change aligns with the PR goal of standardizing error handling to improve Bugsnag grouping for error messages.
packages/analytics-js-plugins/__tests__/storageMigrator/index.test.ts (1)
123-128: Updated test expectation for new error handler interfaceThe test expectations have been correctly updated to match the new error handler interface that now uses a single object parameter containing error details and grouping information instead of multiple positional arguments.
packages/analytics-js-plugins/src/storageMigrator/index.ts (1)
65-71: Implemented standardized error handling with grouping hashThe error handler call has been properly refactored to use a single object parameter with appropriate error information and grouping hash. Using the custom error message as the grouping hash is a good approach as it will group similar storage migration errors together in monitoring tools.
packages/analytics-js/src/services/HttpClient/HttpClient.ts (1)
84-89: Standardized error handling with grouping hash supportThe onError method has been correctly updated to accept an optional groupingHash parameter and pass it through to the error handler in a structured object. This change ensures consistent error reporting across the codebase and enables proper error grouping in Bugsnag.
packages/analytics-js/src/components/configManager/ConfigManager.ts (1)
142-148: Well-aligned change for improved error groupingThe method signature has been updated to include an optional
groupingHashparameter, and the error handler call now uses the new object-based parameter style. This change is consistent with the PR objective of improving error grouping in Bugsnag.packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts (2)
11-12: Terminology update to align with integration-focused namingThe constant has been renamed from
DESTINATION_EVENT_FORWARDING_ERRORtoINTEGRATION_EVENT_FORWARDING_ERRORwith an improved message that more precisely indicates it's the integration for a destination that failed to receive the forwarded event.
14-14: Export statement correctly updatedThe export statement has been properly updated to reflect the renamed constant.
packages/analytics-js-plugins/src/beaconQueue/index.ts (2)
78-82: Improved error handling and logging for beacon failuresThe conditional handling of
navigator.sendBeaconhas been refactored to provide clearer success and failure paths. Thedonecallback is now only called on success, which is consistent with error handling patterns in the codebase.
84-89: Updated error handling to new object-based styleThe error handler call now uses the standardized object-based parameter style with proper error, context, and custom message properties. This aligns with the PR objective of consistent error reporting.
Based on the learnings in memory: the removal of the
donecall in the catch block is intentional, as errors should not be passed to thedonecallback. Instead, not callingdoneallows the queue system to handle retries according to its policy.packages/analytics-js-common/src/types/ErrorHandler.ts (2)
7-13: Well-designed ErrorInfo type for standardized error handlingThe new
ErrorInfotype effectively encapsulates all error-related information, including the optionalgroupingHashproperty which is key to the PR objective. This structured approach allows for more consistent error reporting and better error grouping.
19-19: Interface updated to use the new ErrorInfo typeThe
IErrorHandlerinterface'sonErrormethod signature has been updated to accept a singleErrorInfoobject instead of multiple parameters. This standardizes the error handling interface across the codebase and makes it more flexible for future extensions.packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (2)
347-354: LGTM: UpdatedonErrormethod to support groupingHash parameter.The modification to accept an optional
groupingHashparameter and structure the error handler call with a single object parameter improves error handling consistency and aligns with the PR objective of proper grouping hash for errors.
449-453: Good improvement for error grouping.Using the error message as the grouping hash is a good practice here, as it ensures similar errors will be properly grouped together in Bugsnag. This helps with error tracking and debugging.
packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts (3)
93-93: LGTM: Return type refined to ensure consistent return value.Changing the return type from
Promise<string | undefined>toPromise<string>improves type safety and ensures the function always resolves with a string.
108-111: LGTM: Improved error handler to support event parameter.This implementation properly handles the event parameter in the onerror callback, which aligns with the standard onerror event handler signature. The event information is now included in the error message for better debugging context.
121-121: LGTM: Added fallback context for caught errors.Including 'unknown' as the event context for caught errors provides consistent error messages even when the exact event information is not available.
packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts (2)
2-2: LGTM: Switched to using SDKError type.Replacing the IErrorHandler import with SDKError is a good simplification that focuses on the error type rather than the handler interface.
6-6: LGTM: Improved callback signature.Updating the callback signature to accept an ID string and optional SDKError parameter improves error propagation by passing errors directly to the callback instead of relying on a separate error handler.
packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts (3)
24-29: LGTM: Updated test expectations for unhandled errors.Test cases correctly updated to use the new structured error object parameter format, maintaining the same expectations while aligning with the updated interface.
Also applies to: 41-46
92-97: LGTM: Added groupingHash expectation in breadcrumb test.The test now verifies that the groupingHash parameter is correctly set to the same value as the customMessage when reporting breadcrumb logging failures.
105-107: LGTM: Consistent update to all error handling test cases.All test expectations across various scenarios have been consistently updated to use the new object parameter format, maintaining the same test coverage while adapting to the interface changes.
Also applies to: 118-124, 135-137, 147-153, 164-170, 178-178, 186-186, 199-202, 226-229, 252-256, 281-285
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (6)
9-9: Added appropriate type import for updated error handling pattern.The import of the
SDKErrortype is necessary for the updated callback signature used in the test cases, reflecting the standardized error handling approach being implemented throughout the codebase.
19-19: Constructor usage updated to match refactored dependency pattern.The
ExternalSrcLoaderis now correctly instantiated with only the logger parameter, reflecting the removal of theerrorHandlerdependency from its constructor. This aligns with the refactoring effort to simplify error handling across components.
49-52: Test description and callback signature properly updated.The test description and callback signature have been appropriately updated to match the new error handling approach. The callback now accepts both the script ID and an error object, reflecting the standardized error propagation pattern.
57-61: Error message expectations correctly updated.The assertion has been updated to check for the specific error message structure, which now includes "Unable to load (error)" rather than the previous message format. This ensures tests properly validate the updated error formatting standard.
73-75: Second test callback signature properly aligned with new pattern.Consistent with the first test case, this callback signature was correctly updated to match the refactored error handling approach, accepting both the script ID and a structured error object.
80-85: Error expectations properly updated in duplicate script test.The assertions have been updated to verify the specific error message for the duplicate script case, maintaining consistency with the new error message format throughout the error handling system.
packages/analytics-js/src/components/core/Analytics.ts (2)
99-99: Constructor invocation properly updated to match refactored dependency pattern.The
ExternalSrcLoaderis now correctly instantiated with only the logger parameter, aligning with the refactored constructor signature where the error handler dependency was removed. This matches the updated implementation in the ExternalSrcLoader service.
189-194: Error handling standardized with proper grouping hash.The error handling has been updated to use a structured error object with named properties (
error,context,customMessage, andgroupingHash) instead of multiple positional arguments. This standardizes error reporting format and adds proper error grouping for Bugsnag, which was the core objective of this PR.The use of the issue message as the grouping hash is appropriate here, as it will group similar lifecycle failures together in error reporting tools.
packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts (2)
10-10: Import updated to use consistent terminology.The import was changed from
DESTINATION_EVENT_FORWARDING_ERRORtoINTEGRATION_EVENT_FORWARDING_ERROR, reflecting a standardization of terminology in error message constants.
73-78: Error handling standardized with improved grouping.The error handling call was updated to use a structured object with named properties instead of multiple positional arguments, consistent with the standardization across the codebase. This change:
- Maintains the original error and context
- Uses the user-friendly ID in the custom message for human readability
- Adds a specific grouping hash using the display name, which will improve error grouping in Bugsnag
This implementation aligns perfectly with the PR's objective of properly setting grouping hash for all errors.
packages/analytics-js/src/services/ErrorHandler/utils.ts (3)
34-36: Added necessary imports for error grouping utility.Added imports for utility functions (
isDefined,isString) and theILoggertype to support the new error grouping hash function.
212-236: Added robust utility for consistent error grouping hash generation.This new utility function properly normalizes and determines error grouping hash with careful handling of different input types and edge cases:
- Only applies grouping hash for CDN installations
- Handles non-string inputs by normalizing them and extracting the message
- Falls back to the default hash when normalization fails or input is empty
- Preserves valid string inputs
This centralized implementation ensures consistent error grouping behavior across the codebase, which directly supports the PR's objective of improving error grouping in Bugsnag.
250-250: Added export for new utility function.The new
getErrorGroupingHashfunction is properly exported, making it available for use throughout the codebase.packages/analytics-js/src/components/pluginsManager/PluginsManager.ts (3)
11-11: Correctly imported SDKError type alongside IErrorHandler.The import has been properly updated to include the SDKError type, which is needed for the updated error handling implementation.
276-276: Nice improvement using the error as groupingHash for remote plugin failures.Using the error object itself as the groupingHash helps ensure that similar plugin loading failures will be grouped together in error tracking systems like Bugsnag, while still differentiating between different types of failures.
336-343: Great implementation of the updated onError method.The updated method signature and implementation correctly adapts to the new error handling pattern, which uses a structured object instead of positional parameters. This makes error handling more consistent and extensible.
packages/analytics-js/src/services/StoreManager/Store.ts (3)
116-118: Well-structured error handling with proper grouping hash.The updated error handling correctly uses the error message as both the custom message and the grouping hash, which will help ensure similar storage errors are grouped together in error tracking tools.
143-145: Good error handling for data fetch errors with proper grouping.The consistent approach of using the same string for both custom message and grouping hash ensures that all data fetch errors for a particular key will be grouped together in Bugsnag.
208-215: Correctly implemented the new onError method signature.The updated method properly follows the new error handling pattern by passing a structured object to the error handler. This improves consistency across the codebase.
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (6)
4-4: Correctly updated imports for the new error handling approach.The additional imports of ErrorInfo type and getErrorGroupingHash utility are necessary for the updated error handling implementation.
Also applies to: 26-27
63-68: Well-structured error event listeners using the new object-based format.The global error listeners have been properly updated to use the new object-based format for error handling. This ensures consistent error reporting for both handled and unhandled errors.
Also applies to: 73-78
91-95: Good refactoring of onError method to use structured error info.The method signature has been correctly updated to accept a single ErrorInfo object instead of multiple parameters, making the interface more flexible and consistent.
127-139: Excellent implementation of grouping hash normalization.The comment and implementation properly explain and handle the grouping hash normalization for CDN installations. This will improve error grouping in Bugsnag by focusing on the message content rather than surrounding code, which is especially helpful for CDN installations where the context varies.
141-147: Correctly updated getBugsnagErrorEvent call with the normalized hash.The call has been updated to include the normalized grouping hash, ensuring that errors are properly grouped in Bugsnag.
183-189: Good consistency in using the new error handling format for breadcrumbs.The breadcrumb error handling correctly uses the new object-based onError format and includes a proper grouping hash.
packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts (3)
1-2: Improved error messages with better formatting and terminology.The updated error message functions now use "integration" instead of "destination" for consistency, and properly quote the destination names for better readability in error logs.
Also applies to: 4-5, 7-8, 10-11
13-17: Good addition of new error message functions for timeout and ready check scenarios.These new error message functions will help provide more specific and actionable error messages for timeout and ready check failures in integrations.
19-26: Properly updated exports with the new function names.The exports list has been correctly updated to include the renamed and new error message functions.
packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts
Outdated
Show resolved
Hide resolved
size-limit report 📦
|
saikumarrs
force-pushed
the
feature/sdk-3304-fix-bugsnag-grouping-for-error-messages-with-ids
branch
from
63badf1 to
c77589f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2246 +/- ##
===========================================
+ Coverage 62.09% 63.22% +1.13%
===========================================
Files 479 479
Lines 16621 16635 +14
Branches 3284 3300 +16
===========================================
+ Hits 10321 10518 +197
+ Misses 5092 4880 -212
- Partials 1208 1237 +29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
saikumarrs
force-pushed
the
feature/sdk-3304-fix-bugsnag-grouping-for-error-messages-with-ids
branch
from
c77589f to
7909fd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (2)
392-394: Remove unnecessary constructorsSeveral empty constructors can be safely removed throughout this file, as they don't contain any logic and are not required.
- // eslint-disable-next-line @typescript-eslint/no-useless-constructor - constructor() {}This also applies to similar constructors at lines 285, 428, and 465.
🧰 Tools
🪛 Biome (1.9.4)
[error] 393-393: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
402-403: Consider using undefined assignment instead of deleteUsing the
deleteoperator can impact performance. Consider using undefined assignment instead.- delete (window as any)[destSDKIdentifier]; - delete (window as any).rudderanalytics; + (window as any)[destSDKIdentifier] = undefined; + (window as any).rudderanalytics = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 403-403: 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
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📥 Commits
Reviewing files that changed from the base of the PR and between 42470a3b90a97901688254e281ae054cf18c0148 and 94e71ca126c819377d041ce4beaa8d450a2e25c8.
📒 Files selected for processing (4)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts(3 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.ts(2 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts(4 hunks)packages/analytics-js/.size-limit.mjs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts
- packages/analytics-js/.size-limit.mjs
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (4)
packages/analytics-js-common/src/types/Destination.ts (1)
Destination(79-87)packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts (2)
getCumulativeIntegrationsConfig(284-284)initializeDestination(285-285)packages/analytics-js-common/__mocks__/ErrorHandler.ts (1)
defaultErrorHandler(16-16)packages/analytics-js-plugins/__mocks__/state.ts (1)
state(207-207)
🪛 Biome (1.9.4)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts
[error] 285-285: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 393-393: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 403-403: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 428-428: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 465-465: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Bundle size checks
🔇 Additional comments (4)
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.ts (1)
108-169: Well-structured tests for sendEventToDestinationThe new test suite for
sendEventToDestinationthoroughly covers its functionality, including successful event forwarding, graceful handling of unsupported methods, and proper error reporting.The error handler test case is particularly valuable, as it verifies the new structured error object format with a consistent
groupingHashproperty based on the display name rather than the ID, which should improve error grouping in monitoring tools like Bugsnag.packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (3)
143-143: Simplified timeout error messageThe test expectation for timeout errors has been simplified to a more generic error message without destination-specific information, which aligns with the standardized error handling approach.
291-374: Comprehensive tests for getCumulativeIntegrationsConfigThe test suite for
getCumulativeIntegrationsConfigcovers all expected scenarios, including successful config merging, fallback behavior when the destination lacks required methods, and error handling.The error handling test correctly verifies the use of the new structured error object format with the
groupingHashproperty that uses the display name ("Google Analytics 4 (GA4)") rather than the ID ("GA4___1234567890"), which should improve error grouping in Bugsnag.
376-507: Thorough tests for initializeDestinationThe test suite for
initializeDestinationis comprehensive, covering:
- Successful initialization
- Error handling during initialization
- Timeout scenarios when waiting for the integration to be ready
All error handling tests properly verify the new structured error object format, including both user-friendly error messages with IDs (for debugging) and consistent
groupingHashvalues using display names (for error grouping).🧰 Tools
🪛 Biome (1.9.4)
[error] 393-393: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 403-403: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 428-428: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 465-465: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
saikumarrs
force-pushed
the
develop
branch
2 times, most recently
from
1c3b89c to
21ae75d
Compare
saikumarrs
force-pushed
the
feature/sdk-3304-fix-bugsnag-grouping-for-error-messages-with-ids
branch
from
94e71ca to
ae0123b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/analytics-js-plugins/src/beaconQueue/logMessages.ts (1)
16-16: Minor formatting improvement aligns with error standardization.Removing the trailing period from the error message improves consistency with other error messages. This minor formatting change supports the broader effort to standardize error handling across the codebase.
Consider adding test coverage for this function as indicated by the static analysis to ensure message formatting is validated.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-16: packages/analytics-js-plugins/src/beaconQueue/logMessages.ts#L16
Added line #L16 was not covered by testspackages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (1)
284-285: Consider the useless constructor lint warnings.The static analyzer flags these constructors as unnecessary. While they're acceptable in test mocks, consider whether they add value or could be simplified.
For cleaner test code, you could simplify these:
-// eslint-disable-next-line @typescript-eslint/no-useless-constructor -constructor() {} +// No constructor needed for simple mock classesAlso applies to: 393-393, 428-428, 465-465
🧰 Tools
🪛 Biome (1.9.4)
[error] 285-285: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 94e71ca126c819377d041ce4beaa8d450a2e25c8 and ae0123b87c2e87e05a0200ce06cbec1ea89d4ec4.
⛔ Files ignored due to path filters (1)
nx.jsonis excluded by!**/*.json
📒 Files selected for processing (45)
packages/analytics-js-common/__fixtures__/msw.handlers.ts(1 hunks)packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts(6 hunks)packages/analytics-js-common/src/constants/logMessages.ts(1 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts(3 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts(3 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts(1 hunks)packages/analytics-js-common/src/types/ErrorHandler.ts(1 hunks)packages/analytics-js-plugins/__tests__/customConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts(3 hunks)packages/analytics-js-plugins/__tests__/iubendaConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/ketchConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.ts(2 hunks)packages/analytics-js-plugins/__tests__/oneTrustConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/storageMigrator/index.test.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/customConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/index.ts(5 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts(4 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts(1 hunks)packages/analytics-js-plugins/src/iubendaConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/ketchConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts(2 hunks)packages/analytics-js-plugins/src/oneTrustConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/storageMigrator/index.ts(1 hunks)packages/analytics-js-plugins/src/storageMigrator/logMessages.ts(1 hunks)packages/analytics-js/.size-limit.mjs(6 hunks)packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts(2 hunks)packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts(1 hunks)packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts(11 hunks)packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts(2 hunks)packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts(2 hunks)packages/analytics-js/src/components/configManager/ConfigManager.ts(1 hunks)packages/analytics-js/src/components/core/Analytics.ts(2 hunks)packages/analytics-js/src/components/pluginsManager/PluginsManager.ts(4 hunks)packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts(2 hunks)packages/analytics-js/src/constants/logMessages.ts(1 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(5 hunks)packages/analytics-js/src/services/ErrorHandler/utils.ts(3 hunks)packages/analytics-js/src/services/HttpClient/HttpClient.ts(1 hunks)packages/analytics-js/src/services/StoreManager/Store.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/analytics-js-plugins/tests/ketchConsentManager/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (33)
- packages/analytics-js-plugins/tests/customConsentManager/index.test.ts
- packages/analytics-js-plugins/src/ketchConsentManager/index.ts
- packages/analytics-js-plugins/tests/iubendaConsentManager/index.test.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts
- packages/analytics-js-common/src/constants/logMessages.ts
- packages/analytics-js-common/fixtures/msw.handlers.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts
- packages/analytics-js-plugins/src/oneTrustConsentManager/index.ts
- packages/analytics-js-plugins/src/beaconQueue/index.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts
- packages/analytics-js-plugins/src/storageMigrator/logMessages.ts
- packages/analytics-js/tests/services/HttpClient/HttpClient.test.ts
- packages/analytics-js-common/tests/services/ExternalSrcLoader/ExternalSrcLoader.test.ts
- packages/analytics-js/src/constants/logMessages.ts
- packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts
- packages/analytics-js-plugins/src/iubendaConsentManager/index.ts
- packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts
- packages/analytics-js-plugins/tests/storageMigrator/index.test.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts
- packages/analytics-js-common/src/types/ErrorHandler.ts
- packages/analytics-js/src/services/HttpClient/HttpClient.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts
- packages/analytics-js-plugins/tests/oneTrustConsentManager/index.test.ts
- packages/analytics-js-plugins/tests/nativeDestinationQueue/utilities.test.ts
- packages/analytics-js-plugins/src/storageMigrator/index.ts
- packages/analytics-js-plugins/src/customConsentManager/index.ts
- packages/analytics-js/src/components/configManager/ConfigManager.ts
- packages/analytics-js/tests/components/configManager/ConfigManager.test.ts
- packages/analytics-js/tests/services/ErrorHandler/ErrorHandler.test.ts
- packages/analytics-js/.size-limit.mjs
- packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts
- packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
🧰 Additional context used
🧠 Learnings (2)
packages/analytics-js-plugins/src/deviceModeDestinations/index.ts (2)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-10-28T08:03:12.163Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the issue regarding inconsistent error handling approaches in `getDMTDeliveryPayload` is no longer valid.
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts:10-11
Timestamp: 2024-10-08T15:52:59.819Z
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/components/core/Analytics.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js/src/components/core/Analytics.ts:125-127
Timestamp: 2024-10-28T08:19:43.438Z
Learning: In `packages/analytics-js/src/components/core/Analytics.ts`, inputs to the `clone` function are sanitized prior, so error handling for clone operations is not needed.
🧬 Code Graph Analysis (3)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (3)
packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts (2)
getCumulativeIntegrationsConfig(284-284)initializeDestination(285-285)packages/analytics-js-common/__mocks__/ErrorHandler.ts (1)
defaultErrorHandler(16-16)packages/analytics-js-plugins/__mocks__/state.ts (1)
state(207-207)
packages/analytics-js-plugins/src/beaconQueue/logMessages.ts (2)
packages/analytics-js-common/src/constants/logMessages.ts (1)
LOG_CONTEXT_SEPARATOR(25-25)packages/analytics-js-plugins/src/shared-chunks/common.ts (1)
LOG_CONTEXT_SEPARATOR(31-31)
packages/analytics-js/src/components/core/Analytics.ts (2)
packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts (1)
ExternalSrcLoader(41-41)packages/analytics-js-common/src/constants/loggerContexts.ts (1)
ANALYTICS_CORE(32-32)
🪛 Biome (1.9.4)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts
[error] 285-285: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 393-393: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 403-403: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 428-428: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 465-465: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🪛 GitHub Check: codecov/patch
packages/analytics-js-plugins/src/beaconQueue/logMessages.ts
[warning] 12-13: packages/analytics-js-plugins/src/beaconQueue/logMessages.ts#L12-L13
Added lines #L12 - L13 were not covered by tests
[warning] 16-16: packages/analytics-js-plugins/src/beaconQueue/logMessages.ts#L16
Added line #L16 was not covered by tests
packages/analytics-js-plugins/src/deviceModeDestinations/index.ts
[warning] 14-14: packages/analytics-js-plugins/src/deviceModeDestinations/index.ts#L14
Added line #L14 was not covered by tests
[warning] 44-44: packages/analytics-js-plugins/src/deviceModeDestinations/index.ts#L44
Added line #L44 was not covered by tests
[warning] 85-85: packages/analytics-js-plugins/src/deviceModeDestinations/index.ts#L85
Added line #L85 was not covered by tests
[warning] 98-98: packages/analytics-js-plugins/src/deviceModeDestinations/index.ts#L98
Added line #L98 was not covered by tests
[warning] 124-124: packages/analytics-js-plugins/src/deviceModeDestinations/index.ts#L124
Added line #L124 was not covered by tests
packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts
[warning] 2-2: packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts#L2
Added line #L2 was not covered by tests
[warning] 5-5: packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts#L5
Added line #L5 was not covered by tests
packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts
[warning] 20-20: packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts#L20
Added line #L20 was not covered by tests
packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts
[warning] 204-204: packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts#L204
Added line #L204 was not covered by tests
packages/analytics-js/src/components/core/Analytics.ts
[warning] 189-189: packages/analytics-js/src/components/core/Analytics.ts#L189
Added line #L189 was not covered by tests
packages/analytics-js/src/components/pluginsManager/PluginsManager.ts
[warning] 317-317: packages/analytics-js/src/components/pluginsManager/PluginsManager.ts#L317
Added line #L317 was not covered by tests
[warning] 328-328: packages/analytics-js/src/components/pluginsManager/PluginsManager.ts#L328
Added line #L328 was not covered by tests
packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts
[warning] 348-348: packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts#L348
Added line #L348 was not covered by tests
packages/analytics-js/src/services/ErrorHandler/utils.ts
[warning] 224-224: packages/analytics-js/src/services/ErrorHandler/utils.ts#L224
Added line #L224 was not covered by tests
[warning] 226-226: packages/analytics-js/src/services/ErrorHandler/utils.ts#L226
Added line #L226 was not covered by tests
[warning] 228-228: packages/analytics-js/src/services/ErrorHandler/utils.ts#L228
Added line #L228 was not covered by tests
[warning] 231-231: packages/analytics-js/src/services/ErrorHandler/utils.ts#L231
Added line #L231 was not covered by tests
[warning] 233-233: packages/analytics-js/src/services/ErrorHandler/utils.ts#L233
Added line #L233 was not covered by tests
[warning] 235-235: packages/analytics-js/src/services/ErrorHandler/utils.ts#L235
Added line #L235 was not covered by tests
packages/analytics-js/src/services/StoreManager/Store.ts
[warning] 116-117: packages/analytics-js/src/services/StoreManager/Store.ts#L116-L117
Added lines #L116 - L117 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Security and code quality checks
- GitHub Check: Bundle size checks
🔇 Additional comments (23)
packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts (1)
49-49: LGTM! Simplified dependency injection.Removing the
errorHandlerparameter fromExternalSrcLoaderconstructor reduces coupling and aligns with the broader refactoring effort.packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (1)
449-453:Details
✅ Verification successful
Verify intentional duplicate parameter usage.
The call passes
FAILED_SETTING_COOKIE_FROM_SERVER_GLOBAL_ERRORas both thecustomMessageandgroupingHash. This appears intentional for error grouping purposes, but please confirm this is the desired behavior.
🏁 Script executed:
#!/bin/bash # Search for similar patterns of using same value for customMessage and groupingHash rg -A 3 -B 3 "onError.*,.*,.*\1" --type tsLength of output: 244
I’ll locate the
onErrordefinition and scan for otherthis.onErrorcalls to see if duplicate parameters are a common pattern.
🏁 Script executed:
#!/bin/bash # 1. Show the onError method signature in UserSessionManager.ts rg -n "onError\s*\(" packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts # 2. List all occurrences of this.onError(...) across the repo for comparison rg "this\.onError\s*\(" -n --type tsLength of output: 2509
Intentional duplicate parameter usage for error grouping
The call tothis.onError( e, FAILED_SETTING_COOKIE_FROM_SERVER_GLOBAL_ERROR, FAILED_SETTING_COOKIE_FROM_SERVER_GLOBAL_ERROR );deliberately passes the same constant for both
customMessageandgroupingHash. This mirrors the existing pattern in:
services/StoreManager/Store.ts(lines 117 & 144):this.onError(err, customMessage, customMessage);No changes required here.
packages/analytics-js/src/services/ErrorHandler/utils.ts (1)
34-36: LGTM! Appropriate utility imports.The imported utilities (
isDefined,isString,normalizeError) are properly used in the newgetErrorGroupingHashfunction.packages/analytics-js/src/components/core/Analytics.ts (2)
99-99: LGTM: ExternalSrcLoader constructor simplificationThe removal of the error handler parameter from ExternalSrcLoader constructor aligns with the broader refactor where error handling is moved to callback patterns instead of direct dependencies.
189-194: LGTM: Improved error handling with structured objectThe updated error handling follows the new standardized pattern with proper context, custom message, and grouping hash. Using the same value for both
customMessageandgroupingHashensures consistent error grouping in Bugsnag.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 189-189: packages/analytics-js/src/components/core/Analytics.ts#L189
Added line #L189 was not covered by testspackages/analytics-js-plugins/src/deviceModeDestinations/index.ts (5)
10-10: LGTM: Updated imports for error handling refactorThe addition of
SDKErrorimport and update toINTEGRATION_*constants align with the standardized error handling and improved terminology throughout the codebase.Also applies to: 14-14
44-48: LGTM: Structured error handling implementationThe error handler call now uses the standardized object parameter pattern with proper context identification (
DEVICE_MODE_DESTINATIONS_PLUGIN), which will improve error categorization and debugging.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 44-44: packages/analytics-js-plugins/src/deviceModeDestinations/index.ts#L44
Added line #L44 was not covered by tests
85-85: LGTM: Safe type assertionThe explicit cast to string is safe since
destDisplayNamesToFileNamesMapvalues are guaranteed to be strings based on the mapping structure.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 85-85: packages/analytics-js-plugins/src/deviceModeDestinations/index.ts#L85
Added line #L85 was not covered by tests
96-104: LGTM: Enhanced callback error handlingThe updated callback signature with optional
SDKErrorparameter and structured error reporting improves error context and grouping. Using the custom message as both message and grouping hash ensures proper error deduplication in Bugsnag.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 98-98: packages/analytics-js-plugins/src/deviceModeDestinations/index.ts#L98
Added line #L98 was not covered by tests
124-124: LGTM: Simplified control flowThe removal of the redundant
else ifcondition simplifies the logic while maintaining the same behavior.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 124-124: packages/analytics-js-plugins/src/deviceModeDestinations/index.ts#L124
Added line #L124 was not covered by testspackages/analytics-js/src/services/StoreManager/Store.ts (2)
116-117: LGTM: Consistent error handling with descriptive messagesThe updated error handling calls use descriptive custom messages from constants and apply the same value for both
customMessageandgroupingHash, which ensures proper error deduplication while providing clear context about the operation that failed.Also applies to: 143-144
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 116-117: packages/analytics-js/src/services/StoreManager/Store.ts#L116-L117
Added lines #L116 - L117 were not covered by tests
208-215: LGTM: Standardized onError method signature and implementationThe updated method signature accepts optional
customMessageandgroupingHashparameters and properly implements the structured error object pattern. The context string correctly identifies the specific store instance for better debugging.packages/analytics-js/src/components/pluginsManager/PluginsManager.ts (4)
11-11: LGTM: Added SDKError import for enhanced error handlingThe import addition supports the updated error handling signature with proper typing.
276-276: LGTM: Enhanced remote plugin loading error handlingThe error handling includes a descriptive custom message with the plugin name and cleverly uses the error itself as the grouping hash, which allows similar plugin loading failures to be grouped together in Bugsnag while maintaining specific error details.
317-317: LGTM: Improved plugin registration/unregistration error contextThe error messages now include the specific plugin name that failed to register or unregister, providing better debugging context while following the standardized error handling pattern.
Also applies to: 328-328
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 317-317: packages/analytics-js/src/components/pluginsManager/PluginsManager.ts#L317
Added line #L317 was not covered by tests
336-342: LGTM: Standardized onError method implementationThe updated method signature supports optional
customMessageand flexiblegroupingHash(string or SDKError), and properly implements the structured error object pattern with appropriate context identification.packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (5)
4-4: LGTM: Proper import of error handler mock.The import of
defaultErrorHandleris correctly added to support testing the new error handling behavior with structured error objects.
10-11: LGTM: New utility function imports.The addition of
getCumulativeIntegrationsConfigandinitializeDestinationimports properly supports the new test coverage being added.
143-143: LGTM: Simplified timeout error message.The change from destination-specific to generic timeout error message aligns with the PR's goal of improving error grouping by removing dynamic identifiers that could cause fragmentation.
291-374: Excellent test coverage for getCumulativeIntegrationsConfig.The test suite comprehensively covers:
- Happy path with proper data merging
- Fallback behavior when method is missing
- Error handling with structured error objects including
groupingHashThe error assertion correctly verifies the new structured error format with both
customMessage(containing userFriendlyId) andgroupingHash(containing displayName) fields.
376-507: Comprehensive test coverage for initializeDestination with proper async handling.The test suite excellently covers:
- Successful initialization flow
- Error handling during initialization
- Timeout scenarios with proper timer mocking
The timeout test correctly uses
donecallback for async completion and properly manages fake timers. The structured error assertions verify the PR's objective of implementing proper grouping hash for error reporting.🧰 Tools
🪛 Biome (1.9.4)
[error] 393-393: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 403-403: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 428-428: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 465-465: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts (2)
1-26: Excellent terminology standardization and message structure improvements.The changes effectively support the PR's objectives:
- Consistent terminology: The shift from "destination" to "integration" provides better alignment with the domain language
- Enhanced error grouping: The new message functions like
INTEGRATION_READY_TIMEOUT_ERRORandINTEGRATION_READY_CHECK_ERRORenable better error categorization for Bugsnag grouping- Improved readability: Adding quotes around destination names makes error messages clearer
The separation of timeout errors from destination-specific errors is particularly valuable for the grouping hash functionality, as it allows timeouts to be grouped together regardless of the specific destination.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 2-2: packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts#L2
Added line #L2 was not covered by tests
[warning] 5-5: packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts#L5
Added line #L5 was not covered by tests
13-17: Well-designed separation of generic vs. destination-specific errors.The addition of
INTEGRATION_READY_TIMEOUT_ERROR(generic) andINTEGRATION_READY_CHECK_ERROR(destination-specific) provides the flexibility needed for proper error grouping:
- Timeout errors can be grouped together across all destinations
- Readiness check errors can be grouped by destination when needed
This aligns perfectly with the PR's goal of setting proper grouping hashes for different error scenarios.
packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (2)
216-230: Improve fire-and-forget test reliability.The test uses
setTimeoutwith a 100ms delay to check DOM updates, which could be flaky in different environments. Consider using a more reliable approach.- it('should handle fire-and-forget mode when no callback is provided and script loads successfully', () => { + it('should handle fire-and-forget mode when no callback is provided and script loads successfully', done => { // Test fire-and-forget mode - no callback provided externalSrcLoaderInstance.loadJSFile({ url: `${dummyDataplaneHost}/jsFileSample.js`, id: 'dummyScript', // No callback provided - should be fire-and-forget }); - // Script should still be loaded to DOM - setTimeout(() => { + // Use a more reliable approach to wait for script loading + const checkInterval = setInterval(() => { const newScriptElement = document.getElementById('dummyScript') as HTMLScriptElement; - expect(newScriptElement).toBeDefined(); - expect(newScriptElement.src).toStrictEqual(`${dummyDataplaneHost}/jsFileSample.js`); - }, 100); + if (newScriptElement) { + expect(newScriptElement).toBeDefined(); + expect(newScriptElement.src).toStrictEqual(`${dummyDataplaneHost}/jsFileSample.js`); + clearInterval(checkInterval); + done(); + } + }, 10); });
232-246: Same timing issue as previous test.This test has the same potential timing reliability issue as the previous fire-and-forget test.
Apply similar changes as suggested for the previous test to make this more reliable by using an interval check instead of a fixed timeout.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 58725b1dce90b3f4f58422260fb2d6d1966f34ab and c39ed3db4f8bd4c2a789b76bb1afb9cd7248702e.
📒 Files selected for processing (3)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts(6 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.ts(2 hunks)packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js-plugins/tests/nativeDestinationQueue/utilities.test.ts
- packages/analytics-js/tests/services/ErrorHandler/utils.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (2)
packages/analytics-js-common/src/types/ErrorHandler.ts (1)
SDKError(5-5)packages/analytics-js-common/__fixtures__/fixtures.ts (1)
dummyDataplaneHost(198-198)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Unit Tests and Lint
- GitHub Check: Security and code quality checks
- GitHub Check: Bundle size checks
🔇 Additional comments (6)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (6)
9-9: LGTM - Proper type import for error handling.The import of
SDKErrortype aligns with the refactored error handling approach mentioned in the AI summary.
19-19: LGTM - Constructor signature update aligns with refactoring.The constructor now only takes a logger parameter, which is consistent with the refactoring that removed the external error handler dependency from ExternalSrcLoader.
33-53: LGTM - Good test coverage for exception handling.This new test properly validates exception handling during script loading with the updated callback signature. The test correctly:
- Mocks
setTimeoutto trigger an exception- Uses the new callback signature
(loadedScript: string, err: SDKError)- Verifies the error message includes proper context (unknown event type, script ID, and URL)
- Restores the original
setTimeoutfunction
71-92: LGTM - Error callback signature updated correctly.The test properly uses the new callback signature
(loadedScript: string, err: SDKError)and validates the updated error message format that includes the event type context.
94-127: LGTM - Error handling test updated appropriately.The test correctly validates the duplicate script ID scenario with the new callback signature and updated error message format.
191-214: LGTM - Good timeout test coverage.This new test properly validates timeout handling:
- Uses fake timers to control timeout simulation
- Verifies the timeout error message includes proper context
- Correctly advances timers beyond the 10-second timeout threshold
- Properly restores real timers in the callback
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts
Outdated
Show resolved
Hide resolved
saikumarrs
force-pushed
the
feature/sdk-3304-fix-bugsnag-grouping-for-error-messages-with-ids
branch
from
c39ed3d to
c54fae5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (1)
56-56: Fix callback signature inconsistency (duplicate issue).The callback signature still uses an optional
loadedScriptparameter, which is inconsistent with the interface definitioncallback?(id: string, error?: SDKError): void. This was previously flagged but remains unresolved.
🧹 Nitpick comments (3)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (3)
287-287: Address the static analysis hint about unnecessary constructor.The ESLint disable comment addresses a valid static analysis warning, but consider removing the unnecessary constructor entirely for cleaner code.
Apply this diff to remove the unnecessary constructor:
- [sdkTypeName]: class { - // eslint-disable-next-line @typescript-eslint/no-useless-constructor - constructor() {} - }, + [sdkTypeName]: class {},
700-700: Remove unnecessary constructors for cleaner code.The static analysis correctly identifies unnecessary constructors in mock classes.
Apply these diffs to remove unnecessary constructors:
- [sdkTypeName]: class { - // eslint-disable-next-line @typescript-eslint/no-useless-constructor - constructor() {} - init = initMock; - }, + [sdkTypeName]: class { + init = initMock; + },- [sdkTypeName]: class { - // eslint-disable-next-line @typescript-eslint/no-useless-constructor - constructor() {} - init = () => { - throw new Error('Error'); - }; - }, + [sdkTypeName]: class { + init = () => { + throw new Error('Error'); + }; + },- [sdkTypeName]: class { - // eslint-disable-next-line @typescript-eslint/no-useless-constructor - constructor() {} - init = () => initMock(); - isLoaded = () => false; - }, + [sdkTypeName]: class { + init = () => initMock(); + isLoaded = () => false; + },Also applies to: 735-735, 772-772
🧰 Tools
🪛 Biome (1.9.4)
[error] 700-700: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
709-710: Optimize cleanup performance by avoiding delete operator.The static analysis correctly identifies a performance issue with the delete operator.
Apply this diff to use undefined assignment instead:
- delete (window as any)[destSDKIdentifier]; - delete (window as any).rudderanalytics; + (window as any)[destSDKIdentifier] = undefined; + (window as any).rudderanalytics = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 710-710: 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
📥 Commits
Reviewing files that changed from the base of the PR and between c39ed3db4f8bd4c2a789b76bb1afb9cd7248702e and c54fae5eb861a5b158d1cd35be9a5b322576503d.
⛔ Files ignored due to path filters (1)
nx.jsonis excluded by!**/*.json
📒 Files selected for processing (46)
packages/analytics-js-common/__fixtures__/msw.handlers.ts(1 hunks)packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts(6 hunks)packages/analytics-js-common/src/constants/logMessages.ts(1 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts(3 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts(3 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts(1 hunks)packages/analytics-js-common/src/types/ErrorHandler.ts(1 hunks)packages/analytics-js-plugins/__tests__/customConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts(4 hunks)packages/analytics-js-plugins/__tests__/iubendaConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/ketchConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.ts(2 hunks)packages/analytics-js-plugins/__tests__/oneTrustConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/storageMigrator/index.test.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/customConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/index.ts(5 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts(4 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts(1 hunks)packages/analytics-js-plugins/src/iubendaConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/ketchConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts(2 hunks)packages/analytics-js-plugins/src/oneTrustConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/storageMigrator/index.ts(1 hunks)packages/analytics-js-plugins/src/storageMigrator/logMessages.ts(1 hunks)packages/analytics-js/.size-limit.mjs(2 hunks)packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts(2 hunks)packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts(1 hunks)packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts(11 hunks)packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts(3 hunks)packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts(2 hunks)packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts(2 hunks)packages/analytics-js/src/components/configManager/ConfigManager.ts(1 hunks)packages/analytics-js/src/components/core/Analytics.ts(2 hunks)packages/analytics-js/src/components/pluginsManager/PluginsManager.ts(4 hunks)packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts(2 hunks)packages/analytics-js/src/constants/logMessages.ts(1 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(5 hunks)packages/analytics-js/src/services/ErrorHandler/utils.ts(3 hunks)packages/analytics-js/src/services/HttpClient/HttpClient.ts(1 hunks)packages/analytics-js/src/services/StoreManager/Store.ts(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/analytics-js/.size-limit.mjs
- packages/analytics-js-plugins/tests/oneTrustConsentManager/index.test.ts
- packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts
🚧 Files skipped from review as they are similar to previous changes (41)
- packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts
- packages/analytics-js-plugins/tests/customConsentManager/index.test.ts
- packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts
- packages/analytics-js-plugins/src/ketchConsentManager/index.ts
- packages/analytics-js/tests/services/HttpClient/HttpClient.test.ts
- packages/analytics-js-plugins/src/iubendaConsentManager/index.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts
- packages/analytics-js/src/constants/logMessages.ts
- packages/analytics-js-plugins/src/storageMigrator/logMessages.ts
- packages/analytics-js-plugins/src/storageMigrator/index.ts
- packages/analytics-js/src/services/HttpClient/HttpClient.ts
- packages/analytics-js-common/fixtures/msw.handlers.ts
- packages/analytics-js/src/components/configManager/ConfigManager.ts
- packages/analytics-js-plugins/src/customConsentManager/index.ts
- packages/analytics-js/tests/components/configManager/ConfigManager.test.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts
- packages/analytics-js-plugins/tests/storageMigrator/index.test.ts
- packages/analytics-js-plugins/src/beaconQueue/logMessages.ts
- packages/analytics-js-plugins/tests/ketchConsentManager/index.test.ts
- packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts
- packages/analytics-js-plugins/tests/iubendaConsentManager/index.test.ts
- packages/analytics-js-common/src/constants/logMessages.ts
- packages/analytics-js-common/src/types/ErrorHandler.ts
- packages/analytics-js/tests/services/ErrorHandler/ErrorHandler.test.ts
- packages/analytics-js-plugins/tests/nativeDestinationQueue/utilities.test.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts
- packages/analytics-js-plugins/src/beaconQueue/index.ts
- packages/analytics-js/src/components/pluginsManager/PluginsManager.ts
- packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts
- packages/analytics-js-plugins/src/oneTrustConsentManager/index.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/index.ts
- packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
- packages/analytics-js/src/components/core/Analytics.ts
- packages/analytics-js/src/services/StoreManager/Store.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts
- packages/analytics-js/tests/services/ErrorHandler/utils.test.ts
- packages/analytics-js/src/services/ErrorHandler/utils.ts
- packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (2)
packages/analytics-js-common/src/types/ErrorHandler.ts (1)
SDKError(5-5)packages/analytics-js-common/__fixtures__/fixtures.ts (1)
dummyDataplaneHost(202-202)
🪛 Biome (1.9.4)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts
[error] 700-700: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 710-710: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 735-735: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 772-772: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Security and code quality checks
- GitHub Check: Bundle size checks
🔇 Additional comments (12)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (8)
9-9: LGTM: Proper type import added.The addition of the
SDKErrortype import aligns with the updated callback signatures that now include error parameters.
19-19: LGTM: Constructor updated to reflect dependency changes.The constructor now only requires a
loggerparameter, which aligns with the refactoring mentioned in the AI summary whereerrorHandlerdependency was removed from theExternalSrcLoaderconstructor.
33-53: LGTM: Comprehensive exception handling test.This test effectively validates error handling when
globalThis.setTimeoutis unavailable. The error message format and callback signature are consistent with the updated error handling approach.
71-92: LGTM: Error handling test properly updated.The test correctly expects both
loadedScriptanderrparameters in the callback, and validates the updated error message format that includes event details.
94-127: LGTM: Duplicate script error test updated correctly.The test properly handles the new callback signature and validates the appropriate error message for script conflicts.
191-214: LGTM: Comprehensive timeout handling test.This test effectively validates timeout scenarios using Jest fake timers and properly tests the 10-second timeout threshold with appropriate error messaging.
216-230: LGTM: Fire-and-forget success scenario covered.This test validates that script loading works correctly when no callback is provided, ensuring the script is still added to the DOM.
232-246: LGTM: Fire-and-forget failure scenario covered.This test ensures that even when script loading fails without a callback, the script element is still added to the DOM, maintaining consistent behavior.
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (4)
4-4: LGTM! Proper imports for enhanced test coverage.The imports correctly add the necessary dependencies for testing error handling and the new utility functions.
Also applies to: 12-13
146-146: LGTM! Error message expectation updated correctly.The test expectation aligns with the timeout error message format.
598-681: Excellent comprehensive test coverage for getCumulativeIntegrationsConfig.The test suite thoroughly covers:
- Successful merging of integration configs
- Handling missing methods gracefully
- Proper error handling with structured error info objects
The error handler verification correctly validates the new structured format with
error,context,customMessage, andgroupingHashproperties.
683-814: Comprehensive test coverage for initializeDestination with proper error handling validation.The test suite effectively covers:
- Successful destination initialization
- Error handling during initialization with structured error info
- Timeout scenarios with appropriate error context and grouping
The structured error handler calls are correctly validated with proper context and grouping hash for enhanced error reporting.
🧰 Tools
🪛 Biome (1.9.4)
[error] 700-700: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 710-710: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 735-735: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 772-772: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts (1)
265-269: Use optional chaining for cleaner code.Apply this diff to improve readability:
- const retryQueueCall = MockRetryQueue.mock.calls[0]; - if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + const retryQueueCall = MockRetryQueue.mock.calls[0]; + queueProcessCallback = retryQueueCall?.[2];🧰 Tools
🪛 Biome (1.9.4)
[error] 267-267: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (3)
244-249: Use optional chaining for cleaner code.Apply this diff:
- // Get the callback that was passed to RetryQueue - const retryQueueCall = MockRetryQueue.mock.calls[0]; - if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + // Get the callback that was passed to RetryQueue + const retryQueueCall = MockRetryQueue.mock.calls[0]; + queueProcessCallback = retryQueueCall?.[2];🧰 Tools
🪛 Biome (1.9.4)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
340-343: Replace delete operator with undefined assignment.While performance isn't critical in tests, using undefined assignment is a better practice.
Apply this diff:
const eventToEnqueue = { ...sampleEvent }; - delete eventToEnqueue.sentAt; + eventToEnqueue.sentAt = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 342-342: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
400-403: Use optional chaining for consistency.Apply this diff:
const retryQueueCall = MockRetryQueue.mock.calls[0]; - const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const sizeCalculator = retryQueueCall?.[6];🧰 Tools
🪛 Biome (1.9.4)
[error] 401-401: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts (2)
188-193: Use optional chaining consistently throughout the file.Multiple locations in this file could benefit from optional chaining for cleaner and more consistent code.
Apply these diffs:
Line 190:
- if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + queueProcessCallback = retryQueueCall?.[2];Line 367:
- const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const sizeCalculator = retryQueueCall?.[6];Line 389:
- const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const sizeCalculator = retryQueueCall?.[6];Lines 487 and 513: Apply the same pattern for consistency.
Also applies to: 365-369, 387-391, 485-489, 511-515
🧰 Tools
🪛 Biome (1.9.4)
[error] 190-190: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
286-289: Replace delete operator with undefined assignment.Apply this diff:
const eventWithoutSentAt = { ...sampleEvent }; - delete eventWithoutSentAt.sentAt; + eventWithoutSentAt.sentAt = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 288-288: 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
📥 Commits
Reviewing files that changed from the base of the PR and between 43df3e7f0479b373bff2a80a6bcb9cea9f4580ff and 5631bd7e5ab10393e7235af571a91e48f58f421b.
📒 Files selected for processing (6)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/logMessages.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js-plugins/src/beaconQueue/index.ts
- packages/analytics-js-plugins/src/beaconQueue/logMessages.ts
🧰 Additional context used
🧠 Learnings (3)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/utilities/retryQueue/utilities.ts:0-0
Timestamp: 2024-11-08T13:17:51.356Z
Learning: The issue regarding missing test coverage for the `findOtherQueues` function in `packages/analytics-js-common/src/utilities/retryQueue/utilities.ts` is no longer applicable.
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/utilities/retryQueue/utilities.ts:0-0
Timestamp: 2024-11-08T13:17:51.356Z
Learning: The issue regarding missing test coverage for the `findOtherQueues` function in `packages/analytics-js-common/src/utilities/retryQueue/utilities.ts` is no longer applicable.
packages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.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-10-08T15:52:59.819Z
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.
🧬 Code Graph Analysis (1)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (8)
packages/analytics-js-plugins/src/beaconQueue/utilities.ts (1)
getNormalizedBeaconQueueOptions(71-71)packages/analytics-js-plugins/src/utilities/retryQueue/RetryQueue.ts (1)
RetryQueue(885-885)packages/analytics-js-common/src/types/ApplicationState.ts (1)
ApplicationState(189-206)packages/analytics-js-common/src/types/HttpClient.ts (1)
IHttpClient(57-68)packages/analytics-js-common/src/types/Store.ts (1)
IStoreManager(20-29)packages/analytics-js-common/src/types/ErrorHandler.ts (1)
IErrorHandler(15-21)packages/analytics-js-common/src/types/Event.ts (1)
RudderEvent(46-63)packages/analytics-js-plugins/src/beaconQueue/index.ts (1)
BeaconQueue(152-152)
🪛 Biome (1.9.4)
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts
[error] 267-267: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts
[error] 190-190: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 288-288: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 367-367: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 389-389: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 487-487: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 513-513: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 342-342: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 401-401: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Unit Tests and Lint
- GitHub Check: Bundle size checks
- GitHub Check: Security and code quality checks
🔇 Additional comments (4)
packages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts (1)
15-461: Excellent comprehensive test coverage!The test suite provides thorough coverage of both utility functions with well-structured scenarios including:
- Successful transformations with proper event enqueueing
- Various failure modes (410, 404, 500 status codes)
- Proper handling of
propagateEventsUntransformedOnErrorflags- Multiple destination processing
- Edge cases like missing destinations and empty events
- Exception handling during JSON parsing
The mock setup is realistic and the assertions are detailed and specific.
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts (1)
361-368: Good implementation of structured error handling!The test correctly verifies that errors are passed using the new structured error object format with
errorandcontextproperties. This aligns perfectly with the PR objective of standardizing error handling across the codebase.packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (1)
307-313: Excellent implementation of structured error handling!The test properly verifies the new error object structure with
error,context, andcustomMessageproperties. This comprehensive error information will help with better error grouping in Bugsnag.packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts (1)
234-241: Comprehensive error handling test coverage!Excellent job testing edge cases including:
- Structured error objects with all required properties
- Graceful handling when logger or errorHandler are undefined
- Various failure scenarios
This thorough testing ensures the new error handling approach is robust.
Also applies to: 476-532
packages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts (1)
283-285:⚠️ Potential issueFix: Same issue - don't reassign imported constants.
This has the same problem as mentioned earlier.
Use the same solution proposed above with
global.__POLYFILL_URL__:- // @ts-expect-error needed for the test - POLYFILL_URL = 'invalid-url'; + global.__POLYFILL_URL__ = 'invalid-url';🧰 Tools
🪛 Biome (1.9.4)
[error] 284-284: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 5631bd7e5ab10393e7235af571a91e48f58f421b and 99d77744991c2c3cab852b8963638a25bb4e337c.
📒 Files selected for processing (3)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts(6 hunks)packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts(6 hunks)packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/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-10-08T15:52:59.819Z
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.
🧬 Code Graph Analysis (2)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (6)
packages/analytics-js-common/src/types/Destination.ts (1)
Destination(79-89)packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts (6)
createDestinationInstance(407-407)isDestinationSDKMounted(405-405)applySourceConfigurationOverrides(411-411)applyOverrideToDestination(412-412)getCumulativeIntegrationsConfig(409-409)initializeDestination(410-410)packages/analytics-js-plugins/__mocks__/state.ts (1)
state(207-207)packages/analytics-js-common/__mocks__/Logger.ts (1)
defaultLogger(18-18)packages/analytics-js-common/src/types/LoadOptions.ts (2)
SourceConfigurationOverride(142-144)SourceConfigurationOverrideDestination(136-140)packages/analytics-js-common/__mocks__/ErrorHandler.ts (1)
defaultErrorHandler(16-16)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts (1)
packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts (3)
applySourceConfigurationOverrides(411-411)isDestinationSDKMounted(405-405)initializeDestination(410-410)
🪛 Biome (1.9.4)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts
[error] 267-267: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 311-311: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 950-950: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 960-960: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 985-985: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1022-1022: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts
[error] 128-128: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 284-284: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Bundle size checks
🔇 Additional comments (12)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts (3)
1-19: LGTM! Comprehensive test setup with proper mocking.The imports and initial setup are well-organized, bringing in all necessary types and mocks for comprehensive testing.
20-86: LGTM! Excellent mock setup for comprehensive testing.The mock setup is thorough and well-structured:
- Detection functions are properly mocked with sensible defaults
- External dependencies (ad blockers, page utilities, timezone) are appropriately mocked
- Mock function references make it easy to override behaviors in specific tests
131-403: LGTM! Comprehensive test coverage with well-structured test suites.The test suites provide excellent coverage:
- Initialization tests: Verify proper setup and initialization flow
- Browser Capability Detection: Thoroughly tests capability detection and state updates
- Polyfill Handling: Covers various scenarios including custom URLs and error cases
- Window Event Listeners: Tests event attachment and state updates
- Lifecycle Management: Verifies status transitions
The tests are well-organized, use proper mocking, and cover both happy paths and edge cases.
🧰 Tools
🪛 Biome (1.9.4)
[error] 284-284: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts (4)
27-48: Excellent test infrastructure setup for the new utility functions.The addition of mocks for
isDestinationSDKMountedandinitializeDestinationprovides good control over test scenarios and aligns with the expanded functionality.
155-159: Correctly updated to use the new structured error object format.The error handler assertion now uses a single object parameter with
contextanderrorproperties, which aligns with the PR's objective of standardizing error handling.
377-428: Excellent edge case coverage forsetActiveDestinations.The new test cases effectively cover:
- Empty configured destinations array
- Missing displayName property
- Malformed postConsent integrations object
These tests ensure robust error handling in edge scenarios.
430-806: Comprehensive test suite for theloadmethod with proper error grouping.The test suite excellently covers:
- SDK mounting scenarios
- Error handling with structured error objects containing
groupingHash- Timeout handling
- Edge cases with invalid configurations
All error handler calls correctly use the new format with
error,context,customMessage, andgroupingHashproperties, which directly addresses the PR's objective of proper error grouping for Bugsnag.packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (5)
150-150: Correctly updated to match the simplified error message format.The timeout error expectation no longer includes the destination identifier, aligning with the updated error message constants mentioned in the AI summary.
265-282: Good defensive test for missing global object.This test ensures proper error handling when the
rudderanalyticsglobal is missing, which is a valid edge case to cover.🧰 Tools
🪛 Biome (1.9.4)
[error] 267-267: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
766-931: Excellent test coverage forgetCumulativeIntegrationsConfig.The test suite thoroughly covers:
- Normal merging scenarios
- Error handling with structured error objects
- Edge cases with null/undefined values
- Behavior without error handler
The error handler assertions correctly use the new format with
groupingHashfor proper Bugsnag grouping.
933-1063: Comprehensive test suite forinitializeDestinationwith proper error grouping.The tests effectively cover:
- Successful initialization
- Error handling during initialization
- Timeout scenarios
All error handler calls use the structured format with distinct
customMessageandgroupingHashproperties, ensuring proper error grouping in Bugsnag as per the PR objectives.🧰 Tools
🪛 Biome (1.9.4)
[error] 950-950: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 960-960: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 985-985: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1022-1022: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
267-267: Static analysis hints can be safely ignored.The Biome warnings about
deleteoperators and unnecessary constructors are false positives in this test context:
- The
deleteoperators are used to clean up global mocks between tests- The "unnecessary" constructors are intentionally empty for mock class definitions
These patterns are standard practice in Jest test files.
Also applies to: 311-311, 950-950, 960-960, 985-985, 1022-1022
🧰 Tools
🪛 Biome (1.9.4)
[error] 267-267: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts
Outdated
Show resolved
Hide resolved
saikumarrs
force-pushed
the
feature/sdk-3304-fix-bugsnag-grouping-for-error-messages-with-ids
branch
from
99d7774 to
2b4a34a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts (2)
127-129:⚠️ Potential issueCritical: Still reassigning imported constant - address previous feedback.
This issue was already flagged in previous reviews but remains unaddressed. Reassigning the imported
POLYFILL_URLconstant violates module semantics and can cause test isolation issues.As mentioned in the previous review, please implement the suggested module mocking approach instead of direct reassignment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
283-285:⚠️ Potential issueCritical: Another instance of the same imported constant reassignment.
This is the same issue as lines 127-129 - reassigning an imported constant. Both instances need to be fixed using proper module mocking.
Apply the same module mocking solution suggested for lines 127-129 to maintain consistency and fix both violations.
🧰 Tools
🪛 Biome (1.9.4)
[error] 284-284: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (1)
56-56: Fix callback signature to match interface requirements.The callback signature is still inconsistent with the interface definition. Based on the past review comments and the updated interface, this should include both required
loadedScriptand optionalerrparameters.Apply this fix to match the interface:
- const cb = (loadedScript?: string) => { + const cb = (loadedScript: string, err?: SDKError) => {
🧹 Nitpick comments (7)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts (3)
190-192: Use optional chaining for safer property access.The conditional check can be simplified using optional chaining for better readability and safety.
- if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + queueProcessCallback = retryQueueCall?.[2];🧰 Tools
🪛 Biome (1.9.4)
[error] 190-190: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
288-288: Avoid delete operator for better performance.The
deleteoperator can impact performance. Consider using undefined assignment instead.- delete eventWithoutSentAt.sentAt; + eventWithoutSentAt.sentAt = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 288-288: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
367-367: Consistently apply optional chaining pattern.Similar to the earlier suggestion, these conditional checks can be simplified using optional chaining.
- const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const sizeCalculator = retryQueueCall?.[6]; - const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const sizeCalculator = retryQueueCall?.[6]; - const queueProcessCallback = retryQueueCall && retryQueueCall[2]; + const queueProcessCallback = retryQueueCall?.[2]; - const queueProcessCallback = retryQueueCall && retryQueueCall[2]; + const queueProcessCallback = retryQueueCall?.[2];Also applies to: 389-389, 487-487, 513-513
🧰 Tools
🪛 Biome (1.9.4)
[error] 367-367: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (3)
246-248: Use optional chaining for safer property access.Consistent with the pattern in the additional test file, this conditional check can be simplified.
- if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + queueProcessCallback = retryQueueCall?.[2];🧰 Tools
🪛 Biome (1.9.4)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
342-342: Avoid delete operator for better performance.Use undefined assignment instead of the delete operator for better performance.
- delete eventToEnqueue.sentAt; + eventToEnqueue.sentAt = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 342-342: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
401-401: Apply optional chaining consistently.Same pattern as the other conditional checks in both test files.
- const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const sizeCalculator = retryQueueCall?.[6];🧰 Tools
🪛 Biome (1.9.4)
[error] 401-401: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts (1)
267-267: Apply the optional chain operator for safer property access.The static analysis tool correctly identified that this can be simplified using optional chaining.
Apply this diff to use optional chaining:
- if (retryQueueCall && retryQueueCall[2]) { + if (retryQueueCall?.[2]) {🧰 Tools
🪛 Biome (1.9.4)
[error] 267-267: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 99d77744991c2c3cab852b8963638a25bb4e337c and 2b4a34affdad8838f930b5932c18b13c44e90b2e.
⛔ Files ignored due to path filters (1)
nx.jsonis excluded by!**/*.json
📒 Files selected for processing (52)
packages/analytics-js-common/__fixtures__/msw.handlers.ts(1 hunks)packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts(6 hunks)packages/analytics-js-common/src/constants/logMessages.ts(1 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts(3 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts(3 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts(1 hunks)packages/analytics-js-common/src/types/ErrorHandler.ts(1 hunks)packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/customConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts(6 hunks)packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts(6 hunks)packages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/iubendaConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/ketchConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.ts(2 hunks)packages/analytics-js-plugins/__tests__/oneTrustConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/storageMigrator/index.test.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/customConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/index.ts(5 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts(4 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts(1 hunks)packages/analytics-js-plugins/src/iubendaConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/ketchConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts(2 hunks)packages/analytics-js-plugins/src/oneTrustConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/storageMigrator/index.ts(1 hunks)packages/analytics-js-plugins/src/storageMigrator/logMessages.ts(1 hunks)packages/analytics-js/.size-limit.mjs(2 hunks)packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts(7 hunks)packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts(2 hunks)packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts(1 hunks)packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts(11 hunks)packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts(3 hunks)packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts(2 hunks)packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts(2 hunks)packages/analytics-js/src/components/configManager/ConfigManager.ts(1 hunks)packages/analytics-js/src/components/core/Analytics.ts(2 hunks)packages/analytics-js/src/components/pluginsManager/PluginsManager.ts(4 hunks)packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts(2 hunks)packages/analytics-js/src/constants/logMessages.ts(1 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(5 hunks)packages/analytics-js/src/services/ErrorHandler/utils.ts(3 hunks)packages/analytics-js/src/services/HttpClient/HttpClient.ts(1 hunks)packages/analytics-js/src/services/StoreManager/Store.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/analytics-js-plugins/tests/ketchConsentManager/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (43)
- packages/analytics-js-plugins/tests/customConsentManager/index.test.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts
- packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts
- packages/analytics-js/tests/services/HttpClient/HttpClient.test.ts
- packages/analytics-js-plugins/src/customConsentManager/index.ts
- packages/analytics-js-plugins/src/ketchConsentManager/index.ts
- packages/analytics-js-plugins/src/beaconQueue/index.ts
- packages/analytics-js-plugins/src/oneTrustConsentManager/index.ts
- packages/analytics-js-plugins/tests/iubendaConsentManager/index.test.ts
- packages/analytics-js-plugins/src/storageMigrator/index.ts
- packages/analytics-js-plugins/tests/oneTrustConsentManager/index.test.ts
- packages/analytics-js-common/fixtures/msw.handlers.ts
- packages/analytics-js/.size-limit.mjs
- packages/analytics-js-plugins/src/storageMigrator/logMessages.ts
- packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts
- packages/analytics-js-plugins/src/beaconQueue/logMessages.ts
- packages/analytics-js/tests/components/configManager/ConfigManager.test.ts
- packages/analytics-js-plugins/src/iubendaConsentManager/index.ts
- packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts
- packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts
- packages/analytics-js/src/components/configManager/ConfigManager.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts
- packages/analytics-js-plugins/tests/nativeDestinationQueue/utilities.test.ts
- packages/analytics-js-plugins/tests/storageMigrator/index.test.ts
- packages/analytics-js-common/src/constants/logMessages.ts
- packages/analytics-js/src/components/pluginsManager/PluginsManager.ts
- packages/analytics-js/tests/services/ErrorHandler/ErrorHandler.test.ts
- packages/analytics-js/src/services/ErrorHandler/utils.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts
- packages/analytics-js-common/src/types/ErrorHandler.ts
- packages/analytics-js/src/services/HttpClient/HttpClient.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts
- packages/analytics-js/src/services/StoreManager/Store.ts
- packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/index.ts
- packages/analytics-js/src/constants/logMessages.ts
- packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
- packages/analytics-js/src/components/core/Analytics.ts
- packages/analytics-js/tests/services/ErrorHandler/utils.test.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts
🧰 Additional context used
🧠 Learnings (3)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/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-10-08T15:52:59.819Z
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-plugins/__tests__/deviceModeTransformation/utilities.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-10-08T15:52:59.819Z
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-plugins/__tests__/beaconQueue/index.test.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/utilities/retryQueue/utilities.ts:0-0
Timestamp: 2024-11-08T13:17:51.356Z
Learning: The issue regarding missing test coverage for the `findOtherQueues` function in `packages/analytics-js-common/src/utilities/retryQueue/utilities.ts` is no longer applicable.
🧬 Code Graph Analysis (1)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (2)
packages/analytics-js-common/src/types/ErrorHandler.ts (1)
SDKError(5-5)packages/analytics-js-common/__fixtures__/fixtures.ts (1)
dummyDataplaneHost(202-202)
🪛 Biome (1.9.4)
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts
[error] 267-267: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts
[error] 267-267: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 311-311: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1109-1109: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1119-1119: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1144-1144: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1181-1181: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts
[error] 190-190: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 288-288: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 367-367: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 389-389: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 487-487: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 513-513: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 342-342: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 401-401: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts
[error] 128-128: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 284-284: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Bundle size checks
🔇 Additional comments (26)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts (4)
20-85: Excellent comprehensive mocking setup.The extensive mocking of detection functions, utilities, and external dependencies provides excellent test isolation. The mock function references and default return values create a robust testing foundation.
93-126: Well-structured test setup with proper dependency injection.The beforeEach setup properly initializes mocked dependencies and sets consistent default values, ensuring test isolation and predictable behavior.
131-216: Comprehensive test coverage for initialization and capability detection.The new test suites provide excellent coverage of the CapabilitiesManager's core functionality, including browser capability detection, ad blocker detection conditional logic, and proper state management.
299-401: Thorough testing of polyfill handling and event management.The expanded test coverage for polyfill loading scenarios, window event listeners, and lifecycle management significantly improves the test suite's comprehensiveness. The tests properly verify both positive and negative scenarios.
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (7)
9-9: LGTM: Import addition aligns with error handling standardization.The import of
SDKErrortype properly supports the standardized error handling approach across the codebase.
19-19: LGTM: Constructor simplification aligns with architecture changes.The simplified constructor that only accepts a logger is consistent with removing the external error handler dependency, which aligns with the PR's goal of standardizing error handling.
33-53: Excellent exception handling test with proper error verification.This test comprehensively covers the edge case where
setTimeoutis unavailable, properly simulating the exception scenario and verifying the error message format. The test setup and cleanup are handled correctly.
71-92: LGTM: Proper error callback implementation for failed script loading.The test correctly implements the updated callback signature with both
loadedScriptanderrparameters, and properly verifies the error message format including the event type context.
94-127: LGTM: Comprehensive duplicate script error handling test.The test properly validates the scenario where a script with the same ID already exists, correctly implementing the updated callback signature and verifying appropriate error messaging.
191-214: Well-implemented timeout test with proper Jest timer usage.The test effectively uses Jest fake timers to simulate the timeout scenario and properly verifies the timeout error message. The cleanup with
useRealTimers()ensures no side effects on other tests.
216-246: Thorough fire-and-forget mode test coverage.These tests properly validate the behavior when no callback is provided, ensuring scripts are still loaded to the DOM regardless of success or failure. The use of
setTimeoutfor DOM verification is appropriate for asynchronous script loading scenarios.packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts (2)
234-241: Excellent error handling test structure.The test correctly validates the new structured error handling pattern with
error,context, andcustomMessageproperties, which aligns perfectly with the PR objectives for improving error grouping.
176-274: Comprehensive edge case coverage.The test suite provides excellent coverage of edge cases including empty arrays, undefined payloads, beacon API errors, and multiple event processing. This thoroughness will help ensure the BeaconQueue plugin is robust in production.
🧰 Tools
🪛 Biome (1.9.4)
[error] 190-190: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (2)
307-314: Proper error handling structure validation.The test correctly validates the new structured error handling pattern, ensuring that error reporting includes proper context and custom messages for better error grouping in Bugsnag.
156-421: Excellent test coverage and structure.This test suite provides comprehensive coverage of the BeaconQueue plugin's functionality including:
- Plugin initialization and configuration
- Queue processing with success/failure scenarios
- Event enqueuing with timestamp management
- Size calculation functionality
- Error handling edge cases
The mock setup is thorough and the test assertions validate both the happy path and error scenarios effectively.
🧰 Tools
🪛 Biome (1.9.4)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 342-342: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 401-401: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts (2)
363-367: LGTM! Error handler correctly uses structured object.The error handling test properly validates the new structured error object interface with
error,contextproperties, which aligns perfectly with the PR's objective to standardize error reporting.
1-435: Excellent comprehensive test coverage for NativeDestinationQueue plugin.This new test suite provides thorough coverage of all plugin functionality including:
- Plugin initialization and configuration
- Queue processing with various scenarios (filtering, transformation, errors)
- Event enqueuing with optional parameters
- Edge cases and error handling
The test structure follows best practices with proper mocking and clear test scenarios.
🧰 Tools
🪛 Biome (1.9.4)
[error] 267-267: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts (3)
155-158: LGTM! Error handler properly uses structured object interface.The error handling call correctly uses the new structured object pattern with
contextanderrorproperties, which aligns with the PR's goal of standardizing error reporting for better Bugsnag grouping.
378-427: Excellent addition of comprehensive edge case testing.These new test cases provide valuable coverage for:
- Empty configured destinations array
- Missing displayName handling
- Malformed consent integrations
This thorough edge case coverage improves the robustness of the test suite.
430-807: Outstanding comprehensive test coverage for the load method.The new test suite for the
loadmethod provides extensive coverage including:
- SDK mounting state scenarios
- Mixed SDK mounting conditions
- Custom callback handling
- Error scenarios with proper structured error reporting
- Edge cases with malformed data
The tests follow excellent patterns with proper mocking and detailed scenario validation.
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (3)
265-282: Good addition of error handling test for missing global object.This test case properly validates the error handling when the
rudderanalyticsglobal object is missing, which is an important edge case for robustness.🧰 Tools
🪛 Biome (1.9.4)
[error] 267-267: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
925-1089: Excellent comprehensive test coverage for getCumulativeIntegrationsConfig.The new test suite provides thorough coverage including:
- Normal configuration merging
- Missing method handling
- Error scenarios with proper structured error reporting
- Null/undefined edge cases
- Error handler parameter validation
The structured error handler calls correctly use the new pattern with
error,context,customMessage, andgroupingHashproperties.
1092-1222: Outstanding test coverage for initializeDestination function.The test suite comprehensively covers:
- Successful initialization
- Error handling during initialization with proper structured error reporting
- Timeout scenarios with detailed error context
The error handler calls properly follow the new structured object pattern, contributing to the PR's goal of improved error grouping.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1109-1109: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1119-1119: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1144-1144: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1181-1181: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts (3)
92-132: Excellent test coverage for createPayload function.The tests comprehensively cover:
- Correct payload structure validation
- Null token handling
- Unique orderNo generation based on timestamp
The test assertions properly validate the expected payload structure and behavior.
134-353: Outstanding comprehensive test coverage for sendTransformedEventToDestinations.The test suite provides thorough coverage of all scenarios including:
- Successful transformation (200 status)
- Transformation failures with different propagation settings
- Various HTTP error statuses (404, 500)
- Exception handling with structured error reporting
- Multiple destinations handling
The error handler test correctly uses the structured object pattern, though the groupingHash validation was intentionally omitted as previously clarified.
356-459: Comprehensive edge case testing with excellent coverage.The additional test cases provide valuable coverage for:
- Multiple destination scenarios
- Empty event handling
- Missing destination validation
- Complex transformation response processing
This thorough edge case coverage significantly improves the robustness of the test suite.
saikumarrs
force-pushed
the
feature/sdk-3304-fix-bugsnag-grouping-for-error-messages-with-ids
branch
from
2b4a34a to
24a2576
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts (2)
127-129:⚠️ Potential issueFix: Don't reassign imported constants - mock the module instead.
This is a duplicate of a previous review comment. Reassigning an imported constant violates module semantics and can cause test isolation issues.
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
283-284:⚠️ Potential issueFix: Don't reassign imported constants - mock the module instead.
Same issue as line 128 - reassigning imported constants violates module semantics. Use the module mocking approach suggested in the previous review comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 284-284: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (1)
56-56:⚠️ Potential issueFix inconsistent callback signature.
This callback signature is inconsistent with the updated interface and other tests in the file. Based on the past review comments, this should have been updated to match the new signature.
Apply this diff to fix the callback signature:
- const cb = (loadedScript?: string) => { + const cb = (loadedScript: string, err?: SDKError) => {
🧹 Nitpick comments (8)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts (3)
190-192: Consider using optional chaining for better readability.The conditional check can be simplified using optional chaining as suggested by the static analysis tool.
- if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + queueProcessCallback = retryQueueCall?.[2];🧰 Tools
🪛 Biome (1.9.4)
[error] 190-190: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
288-288: Consider using undefined assignment instead of delete for better performance.The delete operator can impact performance. Consider using undefined assignment instead.
- delete eventWithoutSentAt.sentAt; + eventWithoutSentAt.sentAt = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 288-288: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
367-367: Simplify conditional checks with optional chaining.Multiple locations can benefit from optional chaining for better readability and consistency.
- const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const sizeCalculator = retryQueueCall?.[6];Apply similar changes to lines 389, 487, and 513 for consistency.
Also applies to: 389-389, 487-487, 513-513
🧰 Tools
🪛 Biome (1.9.4)
[error] 367-367: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (3)
246-248: Simplify conditional access with optional chaining.The conditional check can be streamlined using optional chaining for better readability.
- if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + queueProcessCallback = retryQueueCall?.[2];🧰 Tools
🪛 Biome (1.9.4)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
342-342: Optimize property removal for better performance.Use undefined assignment instead of the delete operator to avoid performance impact.
- delete eventToEnqueue.sentAt; + eventToEnqueue.sentAt = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 342-342: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
401-401: Apply optional chaining for consistency.Simplify the conditional access pattern for better readability.
- const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const sizeCalculator = retryQueueCall?.[6];🧰 Tools
🪛 Biome (1.9.4)
[error] 401-401: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (1)
216-246: Good coverage for fire-and-forget mode.The tests effectively verify that scripts are loaded to the DOM even when no callback is provided, covering both success and failure scenarios.
Consider wrapping the DOM checks in the
setTimeoutcallbacks with proper assertions to ensure they're actually executed during the test run, or use a more robust async testing pattern likewaitForif available.packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts (1)
266-269: Use optional chaining for safer property access.The static analysis tool correctly identifies that optional chaining would be safer here.
Apply this diff to use optional chaining:
- const retryQueueCall = MockRetryQueue.mock.calls[0]; - if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + const retryQueueCall = MockRetryQueue.mock.calls[0]; + if (retryQueueCall?.[2]) { + queueProcessCallback = retryQueueCall[2]; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 267-267: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 2b4a34affdad8838f930b5932c18b13c44e90b2e and 24a257630c2bfb89507348d54ff6f92d3e989eb3.
⛔ Files ignored due to path filters (1)
nx.jsonis excluded by!**/*.json
📒 Files selected for processing (52)
packages/analytics-js-common/__fixtures__/msw.handlers.ts(1 hunks)packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts(6 hunks)packages/analytics-js-common/src/constants/logMessages.ts(1 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts(3 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts(3 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts(1 hunks)packages/analytics-js-common/src/types/ErrorHandler.ts(1 hunks)packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/customConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts(6 hunks)packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts(6 hunks)packages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/iubendaConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/ketchConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.ts(2 hunks)packages/analytics-js-plugins/__tests__/oneTrustConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/storageMigrator/index.test.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/customConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/index.ts(5 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts(4 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts(1 hunks)packages/analytics-js-plugins/src/iubendaConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/ketchConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts(2 hunks)packages/analytics-js-plugins/src/oneTrustConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/storageMigrator/index.ts(1 hunks)packages/analytics-js-plugins/src/storageMigrator/logMessages.ts(1 hunks)packages/analytics-js/.size-limit.mjs(2 hunks)packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts(7 hunks)packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts(2 hunks)packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts(1 hunks)packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts(11 hunks)packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts(3 hunks)packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts(2 hunks)packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts(2 hunks)packages/analytics-js/src/components/configManager/ConfigManager.ts(1 hunks)packages/analytics-js/src/components/core/Analytics.ts(2 hunks)packages/analytics-js/src/components/pluginsManager/PluginsManager.ts(4 hunks)packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts(2 hunks)packages/analytics-js/src/constants/logMessages.ts(1 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(5 hunks)packages/analytics-js/src/services/ErrorHandler/utils.ts(3 hunks)packages/analytics-js/src/services/HttpClient/HttpClient.ts(1 hunks)packages/analytics-js/src/services/StoreManager/Store.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/analytics-js-plugins/tests/iubendaConsentManager/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (43)
- packages/analytics-js-plugins/src/ketchConsentManager/index.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts
- packages/analytics-js-plugins/src/beaconQueue/index.ts
- packages/analytics-js/src/constants/logMessages.ts
- packages/analytics-js-plugins/tests/oneTrustConsentManager/index.test.ts
- packages/analytics-js-plugins/tests/ketchConsentManager/index.test.ts
- packages/analytics-js/.size-limit.mjs
- packages/analytics-js-plugins/tests/customConsentManager/index.test.ts
- packages/analytics-js-plugins/src/storageMigrator/logMessages.ts
- packages/analytics-js-plugins/src/iubendaConsentManager/index.ts
- packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts
- packages/analytics-js-plugins/src/oneTrustConsentManager/index.ts
- packages/analytics-js-plugins/src/customConsentManager/index.ts
- packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts
- packages/analytics-js-plugins/src/storageMigrator/index.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts
- packages/analytics-js-plugins/tests/storageMigrator/index.test.ts
- packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts
- packages/analytics-js/tests/services/HttpClient/HttpClient.test.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts
- packages/analytics-js-common/src/types/ErrorHandler.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts
- packages/analytics-js/tests/components/configManager/ConfigManager.test.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/index.ts
- packages/analytics-js/src/services/StoreManager/Store.ts
- packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts
- packages/analytics-js-plugins/tests/nativeDestinationQueue/utilities.test.ts
- packages/analytics-js/tests/services/ErrorHandler/ErrorHandler.test.ts
- packages/analytics-js/src/services/HttpClient/HttpClient.ts
- packages/analytics-js/src/services/ErrorHandler/utils.ts
- packages/analytics-js/src/components/core/Analytics.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts
- packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
- packages/analytics-js-common/fixtures/msw.handlers.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts
- packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts
- packages/analytics-js/src/components/configManager/ConfigManager.ts
- packages/analytics-js-plugins/src/beaconQueue/logMessages.ts
- packages/analytics-js/src/components/pluginsManager/PluginsManager.ts
- packages/analytics-js/tests/services/ErrorHandler/utils.test.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts
- packages/analytics-js-common/src/constants/logMessages.ts
🧰 Additional context used
🧠 Learnings (4)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/utilities/retryQueue/utilities.ts:0-0
Timestamp: 2024-11-08T13:17:51.356Z
Learning: The issue regarding missing test coverage for the `findOtherQueues` function in `packages/analytics-js-common/src/utilities/retryQueue/utilities.ts` is no longer applicable.
packages/analytics-js-plugins/__tests__/deviceModeDestinations/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-10-08T15:52:59.819Z
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-plugins/__tests__/deviceModeTransformation/utilities.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-10-08T15:52:59.819Z
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-plugins/__tests__/deviceModeDestinations/utils.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-10-08T15:52:59.819Z
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.
🧬 Code Graph Analysis (2)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (2)
packages/analytics-js-common/src/types/ErrorHandler.ts (1)
SDKError(5-5)packages/analytics-js-common/__fixtures__/fixtures.ts (1)
dummyDataplaneHost(202-202)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (6)
packages/analytics-js-common/src/types/Destination.ts (1)
Destination(79-89)packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts (6)
createDestinationInstance(420-420)isDestinationSDKMounted(418-418)applySourceConfigurationOverrides(424-424)applyOverrideToDestination(425-425)getCumulativeIntegrationsConfig(422-422)initializeDestination(423-423)packages/analytics-js-plugins/__mocks__/state.ts (1)
state(207-207)packages/analytics-js-common/__mocks__/Logger.ts (1)
defaultLogger(18-18)packages/analytics-js-common/src/types/LoadOptions.ts (2)
SourceConfigurationOverride(142-144)SourceConfigurationOverrideDestination(136-140)packages/analytics-js-common/__mocks__/ErrorHandler.ts (1)
defaultErrorHandler(16-16)
🪛 Biome (1.9.4)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts
[error] 190-190: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 288-288: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 367-367: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 389-389: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 487-487: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 513-513: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 342-342: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 401-401: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts
[error] 267-267: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 311-311: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1109-1109: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1119-1119: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1144-1144: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1181-1181: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts
[error] 267-267: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts
[error] 128-128: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 284-284: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Bundle size checks
🔇 Additional comments (38)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts (6)
74-85: Well-structured mock function references.Good practice to extract the mocked functions into typed references for better type safety and readability in tests.
93-129: Comprehensive test setup with proper state management.The beforeEach setup properly resets state, initializes mocks, and sets default return values. This ensures test isolation and predictable behavior.
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
131-148: Good initialization testing coverage.These tests verify that the CapabilitiesManager properly initializes with its dependencies and calls the expected methods during initialization.
150-216: Excellent browser capability detection test coverage.The test suite comprehensively covers:
- All capability detection functions
- State value setting
- Context value setting
- Different capability combinations
- Ad blocker detection scenarios
This provides robust verification of the capability detection functionality.
299-402: Well-organized additional test coverage.The new test sections for polyfill handling, window event listeners, and lifecycle management provide excellent coverage of the CapabilitiesManager's functionality. The tests are well-structured and cover both positive and negative scenarios.
105-105:Details
✅ Verification successful
Verify error handler integration with new interface.
Based on the AI summary, error handling has been refactored to use a single object parameter instead of multiple arguments. Ensure that the CapabilitiesManager properly integrates with this new error handling interface.
🏁 Script executed:
#!/bin/bash # Check how CapabilitiesManager calls the error handler to verify it matches the new interface rg -A 5 -B 2 "errorHandler.*\(" packages/analytics-js/src/components/capabilitiesManager/Length of output: 908
Error handler integration matches the new interface
CapabilitiesManager’s onError method correctly invokes
this.errorHandler.onErrorwith a single object parameter—includingerror,context: CAPABILITIES_MANAGER, andgroupingHash—in line with the refactored error handler signature. No further changes are necessary.packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts (4)
1-11: LGTM! Comprehensive imports and type safety.The import structure is well-organized with proper TypeScript typing for all dependencies and test utilities.
13-68: Excellent mocking strategy for isolated testing.The comprehensive mocking setup properly isolates the BeaconQueue plugin from its dependencies, ensuring reliable unit tests. The mock implementations provide realistic return values for testing different scenarios.
234-241: Perfect alignment with PR objectives for error handling standardization.The error handler invocation correctly uses the new structured format with
error,context, andcustomMessageproperties, which aligns perfectly with the PR's goal of improving error grouping in Bugsnag.
476-532: Excellent error handling robustness testing.The tests for undefined logger and errorHandler scenarios ensure the plugin gracefully handles missing dependencies without throwing errors. This contributes to the overall reliability of the error handling system.
🧰 Tools
🪛 Biome (1.9.4)
[error] 487-487: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 513-513: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (5)
1-11: Well-structured imports with proper type definitions.The imports are cleanly organized and include all necessary types and utilities for comprehensive testing.
12-87: Comprehensive mocking setup enables reliable unit testing.The mocking strategy effectively isolates the plugin under test while providing realistic mock behaviors for all dependencies. The setup ensures consistent test execution across different scenarios.
156-176: Thorough plugin structure validation.The tests properly verify the plugin's interface and ensure it exposes the correct extension points and dependencies.
307-314: Excellent implementation of standardized error handling.The error handler call correctly uses the new structured object format with
error,context, andcustomMessageproperties. This perfectly aligns with the PR's objective of improving error grouping in Bugsnag by providing consistent error information structure.
388-421: Comprehensive size calculation testing.The size calculator tests ensure proper batch size computation, which is critical for managing payload limits in the beacon queue.
🧰 Tools
🪛 Biome (1.9.4)
[error] 401-401: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (6)
9-9: Good addition for standardized error handling.The import of
SDKErrortype aligns with the refactoring to standardize error handling across the codebase.
19-19: Constructor update aligns with error handling refactoring.The removal of the error handler parameter from the constructor is consistent with the shift to callback-based error propagation.
33-53: Excellent test coverage for exception handling.This test effectively covers the scenario where script loading fails due to missing global functions. The callback signature is correct and the test properly cleans up by restoring
globalThis.setTimeout.
71-92: Proper callback signature update for error handling.The test correctly uses the new callback signature and verifies error handling through the callback parameter, aligning with the refactored error propagation approach.
94-127: Well-updated test for duplicate script handling.The test properly uses the new callback signature and provides comprehensive verification of both the script loading behavior and error handling for duplicate script IDs.
191-214: Excellent timeout handling test implementation.The test effectively uses Jest fake timers to simulate timeout scenarios and properly verifies the timeout error handling. The cleanup with
jest.useRealTimers()is appropriate.packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts (2)
1-434: Excellent comprehensive test coverage for NativeDestinationQueue plugin.This new test file provides thorough coverage of the plugin's functionality including initialization, queue setup, event processing, and error handling scenarios. The test structure is well-organized with proper mocking and realistic test data.
🧰 Tools
🪛 Biome (1.9.4)
[error] 267-267: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
363-367: Error handling aligns perfectly with PR objectives.The error handler test correctly uses the new structured object format, which supports the PR's goal of setting proper grouping hashes for error message grouping in Bugsnag.
packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts (5)
27-28: Good addition of utility function mocks.Adding mocks for
isDestinationSDKMountedandinitializeDestinationenables more comprehensive testing of the plugin's behavior.Also applies to: 34-36
155-158: Error handling correctly uses structured object format.The error handler calls properly use the new structured object interface with
error,context, and other properties, aligning with the PR's goal of standardizing error handling.Also applies to: 407-410
378-427: Excellent edge case coverage.These new tests handle important edge cases like empty configured destinations, missing displayName, and malformed postConsent integrations. This improves the robustness of the test suite.
430-806: Comprehensive load method testing.The extensive test suite for the
loadmethod covers multiple scenarios including SDK mounting states, callback behaviors, error handling, and edge cases. This significantly improves test coverage.
647-652: Proper error handling with grouping hash.The error handler calls include both
customMessageandgroupingHashproperties, which directly supports the PR objective of setting proper grouping hashes for Bugsnag error grouping.Also applies to: 795-800
packages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts (2)
1-461: Excellent test coverage for device mode transformation utilities.This new test file provides comprehensive coverage of the
createPayloadandsendTransformedEventToDestinationsfunctions, including success scenarios, various HTTP error responses, exception handling, and edge cases.
349-353: Error handling follows standardized object format.The error handler test correctly validates the new structured object interface. As noted in previous reviews, the intentional exclusion of
groupingHashvalidation here maintains focus on core error handling behavior.packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (8)
4-8: Good additions of necessary imports.The new imports support the expanded test coverage for additional utility functions like
getCumulativeIntegrationsConfigandinitializeDestination.Also applies to: 16-17
150-150: Simplified error message expectation.The updated error message expectation is more concise and focuses on the essential timeout information without destination-specific details.
265-282: Good error handling test for missing global.This test ensures proper error handling when the global
rudderanalyticsobject is missing, improving robustness.🧰 Tools
🪛 Biome (1.9.4)
[error] 267-267: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
318-337: Comprehensive edge case testing for SDK mounting.These new tests cover important edge cases like missing SDK types or prototypes, improving the reliability of the
isDestinationSDKMountedfunction.
553-622: Extensive test coverage for configuration overrides.The new tests thoroughly cover various scenarios for
applySourceConfigurationOverridesincluding multiple overrides, edge cases, and error conditions.
873-922: Good testing of cloning behavior.These tests ensure that destination cloning with
cloneIdworks correctly and handles complex scenarios properly.
1001-1006: Proper error handling with grouping hashes.The error handler calls correctly include both
customMessageandgroupingHashproperties, which directly supports the PR objective of improving Bugsnag error grouping by providing consistent grouping hashes.Also applies to: 1164-1170, 1206-1213
925-1223: Excellent new test suites for utility functions.The comprehensive test coverage for
getCumulativeIntegrationsConfigandinitializeDestinationfunctions covers normal operation, error scenarios, and edge cases with proper error handling.🧰 Tools
🪛 Biome (1.9.4)
[error] 1109-1109: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1119-1119: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1144-1144: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1181-1181: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts (1)
127-129: Fix: Don't reassign imported constants - this issue was already flagged.The reassignment of
POLYFILL_URLon lines 128 and 284 violates module semantics and causes test isolation issues. This is the same issue that was flagged in previous reviews.Also applies to: 283-284
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
🧹 Nitpick comments (11)
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts (1)
269-271: Simplify conditional check with optional chaining.The conditional check can be simplified using optional chaining for better readability and safety.
- if (retryQueueCall && retryQueueCall[2]) { + if (retryQueueCall?.[2]) {🧰 Tools
🪛 Biome (1.9.4)
[error] 269-269: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js/__tests__/services/StoreManager/Store.test.ts (1)
121-138: Consider verifying circular reference handling more explicitly.While the test correctly validates that circular references don't cause errors, it only checks that non-circular properties are preserved. Consider adding an assertion to verify how the circular reference itself is handled.
const retrieved = store.get(QueueStatuses.QUEUE); expect(retrieved.name).toBe('test'); expect(retrieved.nested.value).toBe(42); - // Circular reference should be handled (removed or replaced) + // Verify circular reference is handled (should be undefined or replaced) + expect(retrieved.self).toBeUndefined();packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts (1)
161-338: Comprehensive test coverage for theunregisterLocalPluginsmethod.The tests properly validate error handling with the new structured error format and include appropriate edge cases. The dynamic module mocking approach is valid for these test scenarios.
Consider using
jest.mock()withmockImplementationfor cleaner module mocking:// At the top of the file jest.mock('../../../src/components/pluginsManager/pluginsInventory', () => ({ pluginsInventory: {} })); // In each test const { pluginsInventory } = require('../../../src/components/pluginsManager/pluginsInventory'); Object.assign(pluginsInventory, mockPluginsInventory);This approach would eliminate the need to manually restore the original inventory after each test.
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts (5)
189-195: Consider using optional chaining for cleaner code.The callback extraction can be simplified.
- // Get the callback that was passed to RetryQueue - // Use the last call instead of assuming it's the first call - const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + // Get the callback that was passed to RetryQueue + // Use the last call instead of assuming it's the first call + const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; + queueProcessCallback = retryQueueCall?.[2];🧰 Tools
🪛 Biome (1.9.4)
[error] 192-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
288-291: Replace delete operator with undefined assignment for better performance.it('should handle event without sentAt property', () => { const eventWithoutSentAt = { ...sampleEvent }; - delete eventWithoutSentAt.sentAt; + eventWithoutSentAt.sentAt = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 290-290: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
367-373: Use optional chaining for consistency.it('should handle empty item data array in size calculator', () => { const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const sizeCalculator = retryQueueCall?.[6];🧰 Tools
🪛 Biome (1.9.4)
[error] 369-369: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
389-393: Apply optional chaining pattern here as well.it('should handle large payload size', () => { const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const sizeCalculator = retryQueueCall?.[6];🧰 Tools
🪛 Biome (1.9.4)
[error] 391-391: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
487-491: Maintain consistent optional chaining usage.const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - const queueProcessCallback = retryQueueCall && retryQueueCall[2]; + const queueProcessCallback = retryQueueCall?.[2];🧰 Tools
🪛 Biome (1.9.4)
[error] 489-489: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (3)
255-260: Use optional chaining for cleaner code.// Get the callback that was passed to RetryQueue const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + queueProcessCallback = retryQueueCall?.[2];🧰 Tools
🪛 Biome (1.9.4)
[error] 257-257: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
351-354: Replace delete operator with undefined assignment.it('should enqueue event with sentAt timestamp', () => { const eventToEnqueue = { ...sampleEvent }; - delete eventToEnqueue.sentAt; + eventToEnqueue.sentAt = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 353-353: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
410-414: Apply optional chaining pattern consistently.it('should calculate batch size correctly', () => { const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const sizeCalculator = retryQueueCall?.[6];🧰 Tools
🪛 Biome (1.9.4)
[error] 412-412: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 24a257630c2bfb89507348d54ff6f92d3e989eb3 and 40e1b06662354b591e7f1171c099c7f2e4b9998f.
📒 Files selected for processing (10)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/logMessages.ts(1 hunks)packages/analytics-js/.size-limit.mjs(4 hunks)packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts(7 hunks)packages/analytics-js/__tests__/components/core/Analytics.test.ts(1 hunks)packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts(1 hunks)packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts(3 hunks)packages/analytics-js/__tests__/services/StoreManager/Store.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/analytics-js-plugins/src/beaconQueue/logMessages.ts
- packages/analytics-js/.size-limit.mjs
- packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts
🧰 Additional context used
🧠 Learnings (1)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/utilities/retryQueue/utilities.ts:0-0
Timestamp: 2024-11-08T13:17:51.356Z
Learning: The issue regarding missing test coverage for the `findOtherQueues` function in `packages/analytics-js-common/src/utilities/retryQueue/utilities.ts` is no longer applicable.
🧬 Code Graph Analysis (1)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (9)
packages/analytics-js-plugins/src/beaconQueue/utilities.ts (1)
getNormalizedBeaconQueueOptions(71-71)packages/analytics-js-plugins/src/utilities/retryQueue/RetryQueue.ts (1)
RetryQueue(886-886)packages/analytics-js-plugins/src/types/plugins.ts (1)
QueueProcessCallbackInfo(38-46)packages/analytics-js-common/src/types/ApplicationState.ts (1)
ApplicationState(189-206)packages/analytics-js-common/src/types/HttpClient.ts (1)
IHttpClient(57-68)packages/analytics-js-common/src/types/Store.ts (1)
IStoreManager(20-29)packages/analytics-js-common/src/types/ErrorHandler.ts (1)
IErrorHandler(15-21)packages/analytics-js-common/src/types/Event.ts (1)
RudderEvent(46-63)packages/analytics-js-plugins/src/beaconQueue/index.ts (1)
BeaconQueue(170-170)
🪛 Biome (1.9.4)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts
[error] 192-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 290-290: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 369-369: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 391-391: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 489-489: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts
[error] 128-128: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 284-284: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts
[error] 257-257: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 353-353: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 412-412: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts
[error] 269-269: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Unit Tests and Lint
- GitHub Check: Bundle size checks
- GitHub Check: Security and code quality checks
🔇 Additional comments (25)
packages/analytics-js/__tests__/components/core/Analytics.test.ts (1)
118-144: LGTM! Excellent test coverage for the new error handling structure.This test case effectively validates the new error handler interface that uses a structured object with
error,context,customMessage, andgroupingHashproperties. The test properly:
- Mocks the lifecycle method to simulate an error condition
- Verifies the exact error object structure expected by the error handler
- Sets the
groupingHashto match thecustomMessage, which aligns with the PR objective of proper error grouping- Follows good testing practices by restoring the original method
The test ensures that errors during lifecycle execution are properly caught and reported with the correct context and grouping information for Bugsnag.
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts (6)
1-65: Well-structured test setup with comprehensive mocking.The test file demonstrates excellent organization with proper mocking of dependencies. The mocking strategy using
jest.requireActualpreserves original functionality while allowing targeted function mocking, which is a best practice for maintaining test reliability.
76-98: Comprehensive sample event structure for testing.The sample event object includes all necessary properties for thorough testing of the NativeDestinationQueue functionality, covering various event types and contexts.
355-370: Excellent alignment with PR objectives for structured error handling.This test correctly validates the new structured error handling format that's central to this PR's objectives. The error handler is called with an object containing
errorandcontextproperties, which standardizes error reporting across the codebase and supports proper grouping hash functionality for Bugsnag.
274-296: Thorough testing of the main event processing flow.The test comprehensively validates the queue processing callback, including destination filtering, deny-list checking, and direct event sending. The assertion structure properly verifies all expected function calls with correct parameters.
317-339: Good coverage of mixed destination scenarios.This test effectively validates the handling of destinations that require different processing paths (with and without transformation), ensuring the plugin correctly routes events based on destination configuration.
373-435: Complete test coverage for enqueuing functionality.The tests cover both standard event enqueuing and direct event-to-destination enqueuing, including scenarios with and without optional parameters. This ensures robust validation of the plugin's queuing capabilities.
packages/analytics-js/__tests__/services/StoreManager/Store.test.ts (5)
27-37: LGTM! Test infrastructure improvements look good.The addition of the
keys()method andisEnabledflag to the localStorage proxy enhances test coverage and better simulates real localStorage behavior.
87-108: Well-structured test for invalid key validation.This test correctly validates that the Store class handles invalid keys defensively by preventing unnecessary storage operations.
110-119: Good coverage of falsy value handling.The test correctly validates that
undefinedandnullare normalized tonull, while other falsy values are preserved as-is. This behavior is appropriate for a storage system.
140-164: Excellent test for new structured error handling interface.This test correctly validates the new error handler interface with the structured object format, including the
groupingHashproperty that aligns perfectly with the PR objectives. The test properly verifies all expected error information is passed.
166-186: Comprehensive quota exceeded error handling test.This test correctly validates the special case handling for quota exceeded errors, ensuring the Store falls back to in-memory storage rather than just reporting the error. The test properly mocks the DOMException and verifies the expected behavior.
packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts (1)
15-159: Well-structured and comprehensive test coverage for theregistermethod.The tests properly validate the new error handler interface with structured error objects and cover various scenarios including success paths, error handling, and edge cases.
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts (3)
1-81: Test setup and mocking look good.The imports, mock configurations, and type assertions are properly structured. The mocking strategy effectively isolates the plugin behavior for testing.
223-243: Excellent test coverage for error handling with the new structured format.The test correctly validates that the error handler is called with the new structured error object containing
error,context, andcustomMessageproperties, which aligns perfectly with the PR objective of standardizing error handling for proper Bugsnag grouping.
161-509: Comprehensive edge case test coverage!This additional test suite excellently complements the main test file by thoroughly covering edge cases such as:
- Empty data arrays
- Undefined payloads
- Exception handling
- Missing optional parameters
- Configuration edge cases
The tests properly validate the new structured error handling format throughout, which is crucial for the PR's objective of improving Bugsnag error grouping.
🧰 Tools
🪛 Biome (1.9.4)
[error] 192-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 290-290: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 369-369: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 391-391: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 489-489: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (2)
318-324: Great validation of structured error handling.The test properly verifies that errors are reported using the new structured format with
error,context, andcustomMessagefields, supporting the PR's goal of improving error grouping.
167-435: Well-structured test suite with good coverage.This test file provides solid coverage of the BeaconQueue plugin's core functionality including:
- Plugin initialization and structure
- Queue configuration with various options
- Event processing callbacks
- Event enqueuing with timestamp handling
- Batch size calculations
The tests correctly validate the new error handling structure, which aligns with the PR objectives.
🧰 Tools
🪛 Biome (1.9.4)
[error] 257-257: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 353-353: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 412-412: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts (7)
1-86: Well-structured mocking setup with comprehensive coverage.The mocking setup is thorough and well-organized, covering all the necessary detection functions, utilities, and external dependencies. The modular approach with separate mock blocks for different functionalities improves maintainability.
93-129: Excellent test setup with proper state management.The
beforeEachsetup properly resets state, initializes mocked dependencies, and sets default return values. This ensures test isolation and consistent baseline behavior across all tests.🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
131-148: Good initialization testing coverage.These tests properly verify that the CapabilitiesManager initializes with correct dependencies and calls the expected methods during initialization.
150-216: Comprehensive browser capability detection tests.The test suite covers various scenarios including different capability combinations, ad blocker detection configuration, and proper state updates. The tests verify both positive and negative cases effectively.
396-427: Excellent window event listener testing.The tests properly verify that event listeners are attached and that the online/offline status updates correctly when events are dispatched. This ensures the component handles network status changes appropriately.
429-438: Good lifecycle management testing.The test verifies that the
onReadymethod properly callsdetectBrowserCapabilitiesand updates the lifecycle status, ensuring proper component lifecycle handling.
359-393:Details
✅ Verification successful
Verify the error handling test logic for polyfill loading failures.
The test simulates polyfill loading failure by calling the callback without a
scriptId, but the test expects the error handler to be called with a structured error object includinggroupingHash: undefined.Please verify that the actual CapabilitiesManager implementation calls
onErrorwith this exact structure when polyfill loading fails:
🏁 Script executed:
#!/bin/bash # Description: Verify the onError call signature in CapabilitiesManager polyfill loading # Expected: Find onError calls with structured error objects containing groupingHash ast-grep --pattern $'this.onError($_, { $$$ })' # Also check for direct errorHandler.onError calls ast-grep --pattern $'this.errorHandler.onError({ error: $_, context: $_, groupingHash: $_ })'Length of output: 152
I’ll dig into the actual implementation to confirm how
onErroranderrorHandler.onErrorare used around polyfill loading:This will show us whether the implementation indeed calls
this.onError(...)with the matching structure and whetherthis.errorHandler.onError({ error, context, groupingHash })is used when the polyfill load callback fires with noscriptId.
🏁 Script executed:
#!/bin/bash # 1. Locate the CapabilitiesManager implementation echo "Locating CapabilitiesManager implementation..." FILE=$(rg -l "export class CapabilitiesManager" -g "*.ts") echo "Found file: $FILE" # 2. Inspect the onError method definition echo -e "\n-- onError method signature --" rg -n "onError(" "$FILE" # 3. Check where errorHandler.onError is called echo -e "\n-- errorHandler.onError invocations --" rg -n "errorHandler\.onError" "$FILE" # 4. Find the polyfill loading logic to see how failures are handled echo -e "\n-- Polyfill loading logic --" rg -n "polyfill" "$FILE"Length of output: 668
I’ll locate the actual
CapabilitiesManagersource and inspect how polyfill load failures are routed throughonErroranderrorHandler.onError:
🏁 Script executed:
#!/bin/bash # 1. Find the implementation file echo "Searching for CapabilitiesManager.ts…" FILE=$(find packages/analytics-js -type f -name "CapabilitiesManager.ts" | head -n1) echo "Found: $FILE" # 2. Show the prepareBrowserCapabilities method where loadJSFile is invoked echo -e "\n-- prepareBrowserCapabilities snippet --" rg -C3 "prepareBrowserCapabilities" "$FILE" # 3. Show loadJSFile callback and error handling logic echo -e "\n-- loadJSFile usage and callback --" rg -C3 "loadJSFile" "$FILE" # 4. Inspect onError and errorHandler.onError definitions/invocations echo -e "\n-- onError and errorHandler.onError in this file --" rg -nE "onError|errorHandler\.onError" "$FILE"Length of output: 1728
I’ll pull more context around the polyfill loader callback and any
onErrorusages:
🏁 Script executed:
#!/bin/bash FILE="packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts" echo "— Context around loadJSFile call —" rg -C25 "externalSrcLoader.loadJSFile" "$FILE" echo -e "\n— Callback registration lines —" rg -n "callback" "$FILE" echo -e "\n— All onError invocations in this file —" rg -n "onError" "$FILE"Length of output: 2945
Gathering constant definitions to confirm the error message format:
🏁 Script executed:
#!/bin/bash # 1. Find where POLYFILL_SCRIPT_ID is defined echo "-- POLYFILL_SCRIPT_ID definition --" rg -n "POLYFILL_SCRIPT_ID" -g "*.ts" # 2. Find where POLYFILL_SCRIPT_LOAD_ERROR is defined echo -e "\n-- POLYFILL_SCRIPT_LOAD_ERROR definition --" rg -n "POLYFILL_SCRIPT_LOAD_ERROR" -g "*.ts"Length of output: 1519
✅ Polyfill error handling aligns with test expectations
The
prepareBrowserCapabilitiesmethod invokesthis.onError(new Error(POLYFILL_SCRIPT_LOAD_ERROR(...)))when the polyfill load callback fires without ascriptId. TheonError(error, groupingHash?)implementation then calls:this.errorHandler.onError({ error, context: 'CapabilitiesManager', groupingHash, });– The
POLYFILL_SCRIPT_LOAD_ERRORconstant produces
Failed to load the polyfill script with ID "rudderstackPolyfill" from URL https://test.polyfill.url.
–onErroris called once with theErrorinstance.
–errorHandler.onErroris called once with
{ error: Error, context: 'CapabilitiesManager', groupingHash: undefined }.No changes are required.
|
BugBot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Merge Strategy Change Causes Configuration Overwrites
The merge strategy for destination integration configurations in getCumulativeIntegrationsConfig was changed from deep merge (mergeDeepRight) to shallow merge (object spread). This change causes nested properties within integration configurations to be overwritten instead of merged, potentially leading to data loss or incorrect destination setup.
packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts#L211-L215
BugBot free trial expires on June 9, 2025
You have used $0.00 of your $50.00 spend limit so far. Manage your spend limit in the Cursor dashboard.
Was this report helpful? Give feedback by reacting with 👍 or 👎
saikumarrs
force-pushed
the
feature/sdk-3304-fix-bugsnag-grouping-for-error-messages-with-ids
branch
from
5862b46 to
1f5143f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts (2)
127-128:⚠️ Potential issueFix: Don't reassign imported constants - use proper mocking instead.
This is the same issue flagged in previous reviews. Reassigning an imported constant violates module semantics and can cause test isolation issues.
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
283-284:⚠️ Potential issueFix: Don't reassign imported constants - use proper mocking instead.
This is the same POLYFILL_URL reassignment issue that appears elsewhere in the test. Use proper module mocking as suggested in previous reviews.
🧰 Tools
🪛 Biome (1.9.4)
[error] 284-284: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (1)
56-62: Update callback signature for consistency.This test still uses the old callback signature pattern
(loadedScript?: string)while other tests in the file use the standardized pattern(loadedScript: string, err: SDKError). This inconsistency was noted in past review comments and should be addressed.Apply this diff to maintain consistency:
- const cb = (loadedScript?: string) => { + const cb = (loadedScript: string, err?: SDKError) => { expect(loadedScript).toStrictEqual('dummyScript'); const newScriptElement = document.getElementById('dummyScript') as HTMLScriptElement; expect(newScriptElement).toBeDefined(); expect(newScriptElement.src).toStrictEqual(`${dummyDataplaneHost}/jsFileSample.js`); done(); };
🧹 Nitpick comments (11)
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts (1)
267-271: Apply optional chaining for safer property access.The static analysis tool correctly identifies that optional chaining should be used for safer property access.
Apply this diff to use optional chaining:
- const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; + queueProcessCallback = retryQueueCall?.[2];🧰 Tools
🪛 Biome (1.9.4)
[error] 269-269: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts (3)
190-195: Apply optional chaining for safer property access.Multiple instances in this file use non-safe property access that could be improved with optional chaining.
Apply this diff:
- const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; + queueProcessCallback = retryQueueCall?.[2];🧰 Tools
🪛 Biome (1.9.4)
[error] 192-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
288-291: Replace delete operator with undefined assignment for better performance.The delete operator can impact performance. Use undefined assignment instead.
Apply this diff:
- const eventWithoutSentAt = { ...sampleEvent }; - delete eventWithoutSentAt.sentAt; + const eventWithoutSentAt = { ...sampleEvent, sentAt: undefined };🧰 Tools
🪛 Biome (1.9.4)
[error] 290-290: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
367-387: Apply optional chaining consistently.Similar to other instances in this file, use optional chaining for safer property access.
Apply this diff:
- const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; + const sizeCalculator = retryQueueCall?.[6];🧰 Tools
🪛 Biome (1.9.4)
[error] 369-369: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (3)
257-257: Apply optional chaining for better safety.Use optional chaining to improve code safety and readability.
- if (retryQueueCall && retryQueueCall[2]) { + if (retryQueueCall?.[2]) {🧰 Tools
🪛 Biome (1.9.4)
[error] 257-257: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
353-353: Replace delete operator with undefined assignment.The delete operator can impact performance. Use undefined assignment instead.
- delete eventToEnqueue.sentAt; + eventToEnqueue.sentAt = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 353-353: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
412-412: Apply optional chaining for consistency.Use optional chaining to match the pattern used elsewhere in the codebase.
- const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const sizeCalculator = retryQueueCall?.[6];🧰 Tools
🪛 Biome (1.9.4)
[error] 412-412: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (4)
267-267: Replace delete operator with undefined assignment.The delete operator can impact performance. Use undefined assignment instead.
- delete (globalThis as any).rudderanalytics; + (globalThis as any).rudderanalytics = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 267-267: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
310-311: Remove unnecessary constructor.The constructor is empty and unnecessary.
- // eslint-disable-next-line @typescript-eslint/no-useless-constructor - constructor() {}🧰 Tools
🪛 Biome (1.9.4)
[error] 311-311: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
1109-1109: Remove unnecessary constructors.These empty constructors are unnecessary and should be removed for cleaner code.
- // eslint-disable-next-line @typescript-eslint/no-useless-constructor - constructor() {}Also applies to: 1144-1144, 1181-1181
🧰 Tools
🪛 Biome (1.9.4)
[error] 1109-1109: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
1119-1119: Replace delete operator with undefined assignment.The delete operator can impact performance. Use undefined assignment instead.
- delete (window as any)[destSDKIdentifier]; - delete (window as any).rudderanalytics; + (window as any)[destSDKIdentifier] = undefined; + (window as any).rudderanalytics = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 1119-1119: 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
📥 Commits
Reviewing files that changed from the base of the PR and between 98309faa5c37099771a0c7a9e478bba8b3696e57 and 1f5143f8e1cbba18eeee5b736ec9fac47e9843ee.
⛔ Files ignored due to path filters (1)
nx.jsonis excluded by!**/*.json
📒 Files selected for processing (55)
packages/analytics-js-common/__fixtures__/msw.handlers.ts(1 hunks)packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts(6 hunks)packages/analytics-js-common/src/constants/logMessages.ts(1 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts(3 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts(3 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts(1 hunks)packages/analytics-js-common/src/types/ErrorHandler.ts(1 hunks)packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/customConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts(6 hunks)packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts(6 hunks)packages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/iubendaConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/ketchConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.ts(2 hunks)packages/analytics-js-plugins/__tests__/oneTrustConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/storageMigrator/index.test.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/customConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/index.ts(5 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts(4 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts(1 hunks)packages/analytics-js-plugins/src/iubendaConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/ketchConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts(2 hunks)packages/analytics-js-plugins/src/oneTrustConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/storageMigrator/index.ts(1 hunks)packages/analytics-js-plugins/src/storageMigrator/logMessages.ts(1 hunks)packages/analytics-js/.size-limit.mjs(4 hunks)packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts(7 hunks)packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts(2 hunks)packages/analytics-js/__tests__/components/core/Analytics.test.ts(1 hunks)packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts(1 hunks)packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts(4 hunks)packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts(11 hunks)packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts(3 hunks)packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts(2 hunks)packages/analytics-js/__tests__/services/StoreManager/Store.test.ts(2 hunks)packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts(2 hunks)packages/analytics-js/src/components/configManager/ConfigManager.ts(1 hunks)packages/analytics-js/src/components/core/Analytics.ts(2 hunks)packages/analytics-js/src/components/pluginsManager/PluginsManager.ts(4 hunks)packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts(2 hunks)packages/analytics-js/src/constants/logMessages.ts(1 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(5 hunks)packages/analytics-js/src/services/ErrorHandler/utils.ts(3 hunks)packages/analytics-js/src/services/HttpClient/HttpClient.ts(1 hunks)packages/analytics-js/src/services/StoreManager/Store.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/analytics-js-plugins/tests/customConsentManager/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (47)
- packages/analytics-js-plugins/tests/ketchConsentManager/index.test.ts
- packages/analytics-js-plugins/tests/storageMigrator/index.test.ts
- packages/analytics-js-plugins/src/oneTrustConsentManager/index.ts
- packages/analytics-js-plugins/src/beaconQueue/logMessages.ts
- packages/analytics-js/tests/services/HttpClient/HttpClient.test.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts
- packages/analytics-js-plugins/src/storageMigrator/logMessages.ts
- packages/analytics-js-plugins/src/iubendaConsentManager/index.ts
- packages/analytics-js-plugins/tests/iubendaConsentManager/index.test.ts
- packages/analytics-js-plugins/tests/nativeDestinationQueue/utilities.test.ts
- packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts
- packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts
- packages/analytics-js-plugins/src/ketchConsentManager/index.ts
- packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts
- packages/analytics-js/src/components/configManager/ConfigManager.ts
- packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts
- packages/analytics-js/src/services/HttpClient/HttpClient.ts
- packages/analytics-js-plugins/src/customConsentManager/index.ts
- packages/analytics-js/src/constants/logMessages.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts
- packages/analytics-js-common/src/constants/logMessages.ts
- packages/analytics-js-plugins/tests/oneTrustConsentManager/index.test.ts
- packages/analytics-js-common/fixtures/msw.handlers.ts
- packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts
- packages/analytics-js-plugins/src/beaconQueue/index.ts
- packages/analytics-js/src/components/core/Analytics.ts
- packages/analytics-js-common/src/types/ErrorHandler.ts
- packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
- packages/analytics-js-plugins/src/storageMigrator/index.ts
- packages/analytics-js/tests/services/ErrorHandler/ErrorHandler.test.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/index.ts
- packages/analytics-js/tests/components/configManager/ConfigManager.test.ts
- packages/analytics-js/tests/components/core/Analytics.test.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts
- packages/analytics-js/src/components/pluginsManager/PluginsManager.ts
- packages/analytics-js/src/services/ErrorHandler/utils.ts
- packages/analytics-js/tests/services/StoreManager/Store.test.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts
- packages/analytics-js/.size-limit.mjs
- packages/analytics-js/tests/components/pluginsManager/PluginsManager.test.ts
- packages/analytics-js/src/services/StoreManager/Store.ts
- packages/analytics-js/tests/services/ErrorHandler/utils.test.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts
- packages/analytics-js-plugins/tests/deviceModeTransformation/utilities.test.ts
🧰 Additional context used
🧠 Learnings (3)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.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-10-08T15:52:59.819Z
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-plugins/__tests__/deviceModeDestinations/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-10-08T15:52:59.819Z
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-plugins/__tests__/beaconQueue/index.test.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/utilities/retryQueue/utilities.ts:0-0
Timestamp: 2024-11-08T13:17:51.356Z
Learning: The issue regarding missing test coverage for the `findOtherQueues` function in `packages/analytics-js-common/src/utilities/retryQueue/utilities.ts` is no longer applicable.
🪛 Biome (1.9.4)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts
[error] 192-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 290-290: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 369-369: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 391-391: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 489-489: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts
[error] 267-267: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 311-311: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1109-1109: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1119-1119: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1144-1144: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1181-1181: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts
[error] 257-257: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 353-353: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 412-412: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts
[error] 269-269: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts
[error] 128-128: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 284-284: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Security and code quality checks
- GitHub Check: Bundle size checks
- GitHub Check: Unit Tests and Lint
🔇 Additional comments (23)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts (7)
2-3: LGTM: Improved imports for comprehensive testing setup.The additional imports for
IHttpClient,IErrorHandler, and various detection functions support the expanded test coverage and proper mocking strategy.Also applies to: 5-13, 18-18
20-86: LGTM: Comprehensive mock setup for isolated testing.The extensive mocking setup properly isolates the CapabilitiesManager from its dependencies, enabling reliable unit testing of browser capability detection, utilities, and external services.
131-148: LGTM: Well-structured initialization tests.The initialization tests properly verify dependency injection and method invocation during the init process.
150-216: LGTM: Comprehensive browser capability detection tests.Excellent coverage of capability detection scenarios including different combinations, ad blocker detection, and conditional logic based on configuration.
359-394: LGTM: Validates new standardized error handling pattern.The test correctly verifies the new error handling structure with
error,context, andgroupingHashproperties, aligning with the PR's objective to standardize error handling across the codebase.
396-427: LGTM: Thorough event listener testing.Good coverage of window event listener attachment and state updates for online/offline and resize events.
429-438: LGTM: Lifecycle management validation.The lifecycle test properly verifies the ready state transition and capability detection invocation.
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts (1)
365-370: LGTM! Proper error handling structure.The error handler assertions correctly follow the new structured error object pattern with
error,context, and other properties. This aligns with the PR's goal of standardizing error handling across the codebase.packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts (1)
191-214: LGTM! Good timeout error handling test.The timeout test correctly validates the error handling with the new structured approach and proper timeout simulation using Jest fake timers.
packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts (2)
647-658: LGTM! Excellent error handling structure with grouping hash.The error handling test correctly uses the new structured error object pattern including the
groupingHashproperty, which aligns perfectly with the PR's goal of setting proper grouping hash for all errors to improve Bugsnag error grouping.
155-159: LGTM! Consistent error handling pattern.The error handler assertion properly uses the structured object pattern with
contextanderrorproperties, maintaining consistency across the codebase.packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts (1)
236-242: LGTM! Proper error handling with structured object.The error handling correctly uses the new structured pattern with
error,context, andcustomMessageproperties, which aligns with the PR's standardization goals.packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (2)
318-325: Excellent structured error handling implementation.The error handler invocation correctly uses the new structured object format with error, context, and customMessage properties, which aligns perfectly with the PR's objective to standardize error handling.
1-435: Comprehensive test coverage with excellent mocking strategy.This test suite provides thorough coverage of the BeaconQueue plugin functionality, including proper mocking of dependencies, error scenarios, and edge cases. The implementation follows testing best practices and validates the plugin's integration points effectively.
🧰 Tools
🪛 Biome (1.9.4)
[error] 257-257: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 353-353: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 412-412: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (9)
4-8: Good addition of imports for expanded test coverage.The new imports support the expanded test coverage for additional utility functions and error handling patterns.
150-150: Simplified error message improves consistency.The updated error message expectation removes destination-specific identifiers, making error messages more consistent across different destinations.
265-282: Excellent edge case testing for missing global.This test case properly handles the scenario when the
rudderanalyticsglobal is missing, ensuring robust error handling.🧰 Tools
🪛 Biome (1.9.4)
[error] 267-267: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
553-622: Comprehensive test coverage for source configuration overrides.These additional test cases provide excellent coverage for edge cases, including multiple overrides, missing loggers, empty destinations, and complex override scenarios. The testing approach is thorough and well-structured.
873-922: Excellent coverage for destination cloning functionality.These test cases thoroughly validate the cloning behavior with
cloneIdparameter and ensure deep cloning of destination configs works correctly.
1001-1006: Perfect implementation of structured error handling with grouping hash.The error handler invocation correctly uses the new structured format with
error,context,customMessage, andgroupingHashproperties. This precisely implements the PR's objective to set proper grouping hash for all errors.
1164-1170: Structured error handling with proper grouping hash.The error handler call correctly implements the new structured format with both
customMessageandgroupingHashproperties, perfectly aligning with the PR's goal to improve error message grouping.
925-1090: Outstanding test coverage for cumulative integrations config.This comprehensive test suite covers all scenarios including normal operation, missing methods, error handling, null/undefined instances, and edge cases. The tests validate both functionality and error handling patterns effectively.
1092-1223: Thorough testing of destination initialization with proper error handling.These tests cover successful initialization, error scenarios, and timeout handling. The structured error handling with grouping hash is implemented correctly throughout, supporting the PR's objective for improved error grouping.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1109-1109: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1119-1119: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1144-1144: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1181-1181: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/analytics-js-plugins/src/deviceModeDestinations/index.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js/src/components/pluginsManager/PluginsManager.ts
Outdated
Show resolved
Hide resolved
saikumarrs
force-pushed
the
feature/sdk-3304-fix-bugsnag-grouping-for-error-messages-with-ids
branch
from
1f5143f to
4efa24e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts (2)
127-128:⚠️ Potential issueFix: Don't reassign imported constants - use proper module mocking instead.
This is the same issue flagged in previous reviews. Direct reassignment of imported constants violates module semantics and can cause test isolation issues.
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
283-284:⚠️ Potential issueFix: Same import reassignment issue as lines 127-128.
This is another instance of the same import reassignment violation. Apply the same module mocking solution referenced in the previous comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 284-284: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
🧹 Nitpick comments (11)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts (5)
190-194: Use optional chaining for better code safety.The conditional access pattern can be simplified using optional chaining.
- const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; + queueProcessCallback = retryQueueCall?.[2];🧰 Tools
🪛 Biome (1.9.4)
[error] 192-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
288-305: Avoid the delete operator for better performance.Using the delete operator can impact performance. Consider using undefined assignment instead.
- const eventWithoutSentAt = { ...sampleEvent }; - delete eventWithoutSentAt.sentAt; + const eventWithoutSentAt = { ...sampleEvent, sentAt: undefined };🧰 Tools
🪛 Biome (1.9.4)
[error] 290-290: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
367-386: Use optional chaining consistently.Similar to the earlier suggestion, this conditional access can be simplified.
- const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; + const sizeCalculator = retryQueueCall?.[6];🧰 Tools
🪛 Biome (1.9.4)
[error] 369-369: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
389-404: Use optional chaining for consistency.Apply the same optional chaining pattern here as well.
- const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; + const sizeCalculator = retryQueueCall?.[6];🧰 Tools
🪛 Biome (1.9.4)
[error] 391-391: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
478-508: Excellent error handling coverage.This test validates that the plugin gracefully handles undefined error handlers without throwing exceptions, which is crucial for robustness. The use of optional chaining suggestion applies here too.
- const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - const queueProcessCallback = retryQueueCall && retryQueueCall[2]; + const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; + const queueProcessCallback = retryQueueCall?.[2];🧰 Tools
🪛 Biome (1.9.4)
[error] 489-489: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (3)
255-260: Use optional chaining for better code safety.The conditional access pattern should use optional chaining for consistency and safety.
- const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; + queueProcessCallback = retryQueueCall?.[2];🧰 Tools
🪛 Biome (1.9.4)
[error] 257-257: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
351-368: Avoid the delete operator for performance.Using the delete operator can impact performance. Consider using undefined assignment instead.
- const eventToEnqueue = { ...sampleEvent }; - delete eventToEnqueue.sentAt; + const eventToEnqueue = { ...sampleEvent, sentAt: undefined };🧰 Tools
🪛 Biome (1.9.4)
[error] 353-353: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
410-433: Use optional chaining consistently.Apply optional chaining for the conditional access pattern.
- const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - const sizeCalculator = retryQueueCall && retryQueueCall[6]; + const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; + const sizeCalculator = retryQueueCall?.[6];🧰 Tools
🪛 Biome (1.9.4)
[error] 412-412: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts (1)
267-271: Use optional chaining for safer array accessThe current array access pattern can be simplified and made safer using optional chaining.
Apply this diff to improve the code:
- const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; - if (retryQueueCall && retryQueueCall[2]) { - queueProcessCallback = retryQueueCall[2]; - } + const retryQueueCall = MockRetryQueue.mock.calls[MockRetryQueue.mock.calls.length - 1]; + if (retryQueueCall?.[2]) { + queueProcessCallback = retryQueueCall[2]; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 269-269: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts (1)
499-499: Remove redundant mock clear.This mock clear is unnecessary since
beforeEachon line 124 already callsjest.clearAllMocks().- // Clear mocks - mockIsDestinationSDKMounted.mockClear();packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts (1)
176-187: Consider using jest.mock for more robust module mocking.The current approach of directly modifying
pluginsInventoryModule.pluginsInventoryworks but is fragile and requires manual restoration. Consider usingjest.mock()withjest.doMock()for cleaner test isolation.- // Mock the pluginsInventory import - const pluginsInventoryModule = require('../../../src/components/pluginsManager/pluginsInventory'); - const originalInventory = pluginsInventoryModule.pluginsInventory; - pluginsInventoryModule.pluginsInventory = mockPluginsInventory; + // Mock the pluginsInventory module + jest.doMock('../../../src/components/pluginsManager/pluginsInventory', () => ({ + pluginsInventory: mockPluginsInventory, + })); - // Restore original inventory - pluginsInventoryModule.pluginsInventory = originalInventory; + // Clear the mock after test + jest.dontMock('../../../src/components/pluginsManager/pluginsInventory');Also applies to: 198-200, 225-227, 238-242, 270-272, 279-282, 291-293, 303-306, 334-336
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 1f5143f8e1cbba18eeee5b736ec9fac47e9843ee and 4efa24e.
⛔ Files ignored due to path filters (1)
nx.jsonis excluded by!**/*.json
📒 Files selected for processing (55)
packages/analytics-js-common/__fixtures__/msw.handlers.ts(1 hunks)packages/analytics-js-common/__tests__/services/ExternalSrcLoader/ExternalSrcLoader.test.ts(6 hunks)packages/analytics-js-common/src/constants/logMessages.ts(1 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts(3 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts(3 hunks)packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts(1 hunks)packages/analytics-js-common/src/types/ErrorHandler.ts(1 hunks)packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/customConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts(6 hunks)packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts(8 hunks)packages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/iubendaConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/ketchConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.ts(2 hunks)packages/analytics-js-plugins/__tests__/oneTrustConsentManager/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/storageMigrator/index.test.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/beaconQueue/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/customConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/index.ts(5 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts(4 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts(1 hunks)packages/analytics-js-plugins/src/iubendaConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/ketchConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts(2 hunks)packages/analytics-js-plugins/src/oneTrustConsentManager/index.ts(1 hunks)packages/analytics-js-plugins/src/storageMigrator/index.ts(1 hunks)packages/analytics-js-plugins/src/storageMigrator/logMessages.ts(1 hunks)packages/analytics-js/.size-limit.mjs(1 hunks)packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts(7 hunks)packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts(2 hunks)packages/analytics-js/__tests__/components/core/Analytics.test.ts(1 hunks)packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts(1 hunks)packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts(4 hunks)packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts(11 hunks)packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts(3 hunks)packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts(2 hunks)packages/analytics-js/__tests__/services/StoreManager/Store.test.ts(2 hunks)packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts(2 hunks)packages/analytics-js/src/components/configManager/ConfigManager.ts(1 hunks)packages/analytics-js/src/components/core/Analytics.ts(2 hunks)packages/analytics-js/src/components/pluginsManager/PluginsManager.ts(4 hunks)packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts(2 hunks)packages/analytics-js/src/constants/logMessages.ts(1 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(5 hunks)packages/analytics-js/src/services/ErrorHandler/utils.ts(3 hunks)packages/analytics-js/src/services/HttpClient/HttpClient.ts(1 hunks)packages/analytics-js/src/services/StoreManager/Store.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/analytics-js-plugins/tests/ketchConsentManager/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (45)
- packages/analytics-js-plugins/src/oneTrustConsentManager/index.ts
- packages/analytics-js-plugins/src/beaconQueue/logMessages.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts
- packages/analytics-js/.size-limit.mjs
- packages/analytics-js/tests/services/HttpClient/HttpClient.test.ts
- packages/analytics-js-plugins/src/iubendaConsentManager/index.ts
- packages/analytics-js-plugins/src/customConsentManager/index.ts
- packages/analytics-js-plugins/src/ketchConsentManager/index.ts
- packages/analytics-js-common/fixtures/msw.handlers.ts
- packages/analytics-js-plugins/tests/iubendaConsentManager/index.test.ts
- packages/analytics-js-plugins/src/beaconQueue/index.ts
- packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts
- packages/analytics-js-plugins/tests/storageMigrator/index.test.ts
- packages/analytics-js/tests/components/core/Analytics.test.ts
- packages/analytics-js/tests/components/configManager/ConfigManager.test.ts
- packages/analytics-js-plugins/src/storageMigrator/index.ts
- packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts
- packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts
- packages/analytics-js/src/components/configManager/ConfigManager.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/logMessages.ts
- packages/analytics-js-common/src/constants/logMessages.ts
- packages/analytics-js/src/constants/logMessages.ts
- packages/analytics-js/tests/services/ErrorHandler/ErrorHandler.test.ts
- packages/analytics-js-plugins/src/storageMigrator/logMessages.ts
- packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/jsFileLoader.ts
- packages/analytics-js/src/services/HttpClient/HttpClient.ts
- packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts
- packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
- packages/analytics-js/src/services/ErrorHandler/utils.ts
- packages/analytics-js-plugins/tests/customConsentManager/index.test.ts
- packages/analytics-js-common/src/types/ErrorHandler.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/types.ts
- packages/analytics-js/src/services/StoreManager/Store.ts
- packages/analytics-js-common/src/services/ExternalSrcLoader/ExternalSrcLoader.ts
- packages/analytics-js/src/components/pluginsManager/PluginsManager.ts
- packages/analytics-js-common/tests/services/ExternalSrcLoader/ExternalSrcLoader.test.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/index.ts
- packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts
- packages/analytics-js-plugins/src/deviceModeDestinations/logMessages.ts
- packages/analytics-js/tests/services/ErrorHandler/utils.test.ts
- packages/analytics-js/src/components/core/Analytics.ts
- packages/analytics-js-plugins/tests/oneTrustConsentManager/index.test.ts
- packages/analytics-js-plugins/tests/nativeDestinationQueue/utilities.test.ts
🧰 Additional context used
🧠 Learnings (4)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/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-10-08T15:52:59.819Z
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-plugins/__tests__/deviceModeDestinations/utils.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-10-08T15:52:59.819Z
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-plugins/__tests__/deviceModeTransformation/utilities.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-10-08T15:52:59.819Z
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-plugins/__tests__/beaconQueue/index.test.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/utilities/retryQueue/utilities.ts:0-0
Timestamp: 2024-11-08T13:17:51.356Z
Learning: The issue regarding missing test coverage for the `findOtherQueues` function in `packages/analytics-js-common/src/utilities/retryQueue/utilities.ts` is no longer applicable.
🧬 Code Graph Analysis (2)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts (2)
packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts (3)
applySourceConfigurationOverrides(421-421)isDestinationSDKMounted(415-415)initializeDestination(420-420)packages/analytics-js-common/src/types/LoadOptions.ts (1)
SourceConfigurationOverride(142-144)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts (8)
packages/analytics-js-plugins/src/beaconQueue/utilities.ts (1)
getNormalizedBeaconQueueOptions(71-71)packages/analytics-js-plugins/src/utilities/retryQueue/RetryQueue.ts (1)
RetryQueue(886-886)packages/analytics-js-common/src/types/ApplicationState.ts (1)
ApplicationState(189-206)packages/analytics-js-common/src/types/Store.ts (1)
IStoreManager(20-29)packages/analytics-js-common/src/types/ErrorHandler.ts (1)
IErrorHandler(15-21)packages/analytics-js-plugins/src/types/plugins.ts (2)
QueueProcessCallbackInfo(38-46)QueueProcessCallback(61-65)packages/analytics-js-common/src/types/Event.ts (1)
RudderEvent(46-63)packages/analytics-js-plugins/src/beaconQueue/index.ts (1)
BeaconQueue(170-170)
🪛 Biome (1.9.4)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts
[error] 192-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 290-290: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 369-369: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 391-391: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 489-489: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts
[error] 268-268: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 312-312: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1299-1299: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1309-1309: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1334-1334: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1371-1371: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts
[error] 257-257: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 353-353: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 412-412: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts
[error] 269-269: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts
[error] 128-128: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 284-284: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Bundle size checks
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
🔇 Additional comments (29)
packages/analytics-js/__tests__/services/StoreManager/Store.test.ts (6)
27-37: Good addition to complete the localStorage proxy interface.The
keys()method andisEnabledflag are useful extensions to the localStorage proxy for testing purposes. The implementation correctly iterates through localStorage entries and handles null keys safely.
87-108: Well-structured test for invalid key validation.This test properly verifies that the Store's
setmethod doesn't attempt to store values when provided with invalid keys. The use of spies to verify thatsetItemis not called is an appropriate testing approach.
110-119: Comprehensive test coverage for falsy values.The test correctly validates how different falsy values are handled during storage and retrieval. The expected behavior (undefined/null → null, other falsy values preserved) appears logical and well-tested.
121-138: Good test for circular reference handling.This test ensures the Store can handle objects with circular references without throwing errors, which is important for robust error handling. The test structure is sound and validates the essential properties while acknowledging that circular references are handled safely.
140-164: Excellent alignment with the PR's error grouping objectives.This test validates the new structured error handling approach where
onErroris called with an object containingerror,context,customMessage, andgroupingHash. The use ofgroupingHashset to the same value ascustomMessagedirectly supports the PR objective of "set proper grouping hash for all errors" for improved Bugsnag error grouping.
166-186: Proper test coverage for quota exceeded scenarios.This test correctly validates the specific behavior for quota exceeded errors (switching to in-memory storage) versus generic storage errors. The distinction between the two error types and their different handling paths is well-tested.
packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts (2)
20-129: Excellent comprehensive test setup improvements.The extensive mocking setup and beforeEach configuration significantly improve test coverage and isolation. The systematic approach to mocking browser detection functions, utilities, and proper state management demonstrates good testing practices.
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
131-438: Outstanding comprehensive test coverage improvements.The new test suites provide excellent coverage across all CapabilitiesManager functionality:
- Initialization tests verify proper dependency injection and setup
- Browser capability detection tests cover various capability combinations and ad blocker detection scenarios
- Polyfill handling tests comprehensively cover success/failure cases, custom URLs, and error scenarios
- Window event listener tests verify proper event attachment and state updates
- Lifecycle management tests ensure proper status transitions
The error handling tests (lines 359-394) particularly demonstrate the new structured error object format with
groupingHashsupport, directly supporting the PR objectives for improved error grouping.🧰 Tools
🪛 Biome (1.9.4)
[error] 284-284: The imported variable POLYFILL_URL is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
packages/analytics-js-plugins/__tests__/beaconQueue/index.additional.test.ts (3)
1-12: LGTM! Comprehensive import structure.The imports are well-organized and include all necessary types and mocks for comprehensive testing.
13-82: LGTM! Excellent mocking setup.The mocking strategy is comprehensive and properly isolates the BeaconQueue plugin behavior by mocking all external dependencies including utilities, shared chunks, RetryQueue, and browser APIs.
236-242: Verify the structured error handling pattern.The test correctly validates the new structured error handling format with
error,context, andcustomMessageproperties, which aligns with the PR objective of standardizing error reporting.packages/analytics-js-plugins/__tests__/beaconQueue/index.test.ts (5)
1-12: LGTM! Well-structured imports.The import structure is clean and includes all necessary dependencies for comprehensive plugin testing.
13-87: LGTM! Comprehensive mocking strategy.The mocking setup effectively isolates the plugin behavior and provides predictable test conditions for all dependencies.
167-187: LGTM! Thorough plugin initialization testing.The tests properly validate plugin structure, dependencies, and extension point exposure.
318-325: Excellent validation of structured error handling.The test correctly validates the new error handling pattern with structured error objects containing
error,context, andcustomMessage, which aligns perfectly with the PR objective of standardizing error reporting across the codebase.
99-435: Excellent test coverage and structure.The test suite provides comprehensive coverage of the BeaconQueue plugin's core functionality including initialization, queue processing, event enqueuing, and size calculation. The tests properly validate both success and failure scenarios.
🧰 Tools
🪛 Biome (1.9.4)
[error] 257-257: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 353-353: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 412-412: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/index.test.ts (7)
1-11: LGTM: Clean import structureThe imports are well-organized and properly typed, including all necessary types for the test suite.
12-45: LGTM: Comprehensive mocking setupThe mocking strategy is thorough and properly isolates the unit under test. The mock implementations provide realistic behavior while maintaining test predictability.
76-129: LGTM: Realistic test dataThe sample event and destination data are comprehensive and realistic, providing good coverage for testing various scenarios.
365-368: LGTM: Proper error handling pattern alignmentThe test correctly validates the new structured error handling format that aligns with the PR objectives. The error handler is called with a structured object containing
errorandcontextproperties, which supports the goal of improving error grouping in Bugsnag.
273-339: LGTM: Comprehensive queue processing testsThe test cases thoroughly cover different queue processing scenarios including:
- Events without transformation
- Events requiring transformation
- Mixed destination handling
- Proper mock verification and callback validation
This ensures the queue processing logic works correctly across all use cases.
341-370: LGTM: Excellent error handling and edge case coverageThe tests properly cover:
- Event filtering for deny-listed events with appropriate warning logs
- Error handling during destination processing with proper error handler invocation
- Proper cleanup and callback execution in all scenarios
This ensures robust error handling aligned with the PR's error grouping improvements.
373-435: LGTM: Complete enqueuing functionality testsThe enqueuing tests comprehensively cover both standard event enqueuing and direct event-to-destination enqueuing, including scenarios with and without optional parameters. This ensures the plugin's enqueuing functionality works correctly in all configurations.
packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts (1)
27-803: Excellent test coverage improvements!The addition of comprehensive tests for device mode destinations, including edge cases, SDK mounting scenarios, and error handling, significantly improves the robustness of the codebase. The structured error handling assertions align perfectly with the PR objectives.
packages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts (1)
1-462: Well-structured and comprehensive test coverage!This new test file provides excellent coverage for the device mode transformation utilities. The tests thoroughly cover success scenarios, various error conditions (404, 410, 500 status codes), JSON parsing exceptions, and edge cases. The structured error handling assertions align with the PR objectives.
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (2)
266-1413: Excellent expansion of test coverage!The additions significantly improve test coverage for device mode destination utilities:
- Proper edge case handling for missing
rudderanalyticsglobal- Comprehensive tests for configuration overrides and cloning
- Thorough coverage of
getCumulativeIntegrationsConfigwith error scenarios- Complete test suite for
initializeDestinationincluding timeout handlingAll error assertions correctly use the structured error object format.
🧰 Tools
🪛 Biome (1.9.4)
[error] 268-268: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 312-312: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1299-1299: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1309-1309: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1334-1334: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1371-1371: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
151-151:Details
✅ Verification successful
Verify the timeout error message format.
The error message was changed from "timeout of 11000 ms occurred" to "A timeout of 11000 ms occurred". Please ensure this matches the actual implementation in the source code.
🏁 Script executed:
#!/bin/bash # Description: Verify the timeout error message format in the source implementation # Search for the timeout error message in the utils implementation rg -A 2 -B 2 "timeout.*occurred" packages/analytics-js-plugins/src/deviceModeDestinations/Length of output: 626
Timeout error message matches source implementation
The test’s expectation (
new Error("A timeout of 11000 ms occurred")) now exactly matches theINTEGRATION_READY_TIMEOUT_ERRORmessage inlogMessages.ts. No further changes needed.packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts (2)
15-159: Excellent comprehensive test coverage for plugin registration.The test suite thoroughly covers various scenarios including successful registration, error handling, multiple failures, and different error types. The error handler integration follows the new object-based signature consistently.
161-338: Comprehensive test coverage for plugin unregistration with proper error handling.The unregistration tests mirror the registration tests effectively, covering all the necessary scenarios including error handling, multiple failures, and edge cases. The consistent use of the new error handler object signature aligns well with the broader refactoring effort.
packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (2)
268-268: Address static analysis hint: Replace delete with undefined assignmentWhile the delete operator works correctly here, consider using undefined assignment for better performance as suggested by the linter.
- delete (globalThis as any).rudderanalytics; + (globalThis as any).rudderanalytics = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 268-268: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
312-312: Consider removing useless constructorsThe static analysis tool correctly identifies several empty constructors that serve no purpose. While these don't affect functionality, removing them would improve code cleanliness.
- // eslint-disable-next-line @typescript-eslint/no-useless-constructor - constructor() {}Also applies to: 1297-1297, 1332-1332, 1369-1369
🧰 Tools
🪛 Biome (1.9.4)
[error] 312-312: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts(5 hunks)packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts(8 hunks)packages/analytics-js/.size-limit.mjs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js/.size-limit.mjs
🧰 Additional context used
🧠 Learnings (2)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/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-10-08T15:52:59.819Z
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-plugins/__tests__/deviceModeDestinations/utils.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-10-08T15:52:59.819Z
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.
🪛 Biome (1.9.4)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts
[error] 268-268: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 312-312: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1297-1297: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1307-1307: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1332-1332: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 1369-1369: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (13)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/index.test.ts (6)
27-28: LGTM: Proper mock additions for utility functionsThe new mocks for
isDestinationSDKMountedandinitializeDestinationprovide good control over SDK mounting states and initialization behavior for comprehensive testing.
155-158: Excellent: Updated to use structured error objectThe error assertion correctly uses the new structured format with
contextanderrorproperties, aligning with the PR's goal of standardizing error handling with proper grouping.
221-221: Good: Precise control with mockReturnValueOnceUsing
mockReturnValueOnceinstead ofmockReturnValueprovides better test isolation and precision in override-related scenarios.Also applies to: 311-313
334-383: Excellent: Comprehensive edge case coverageThe new tests for
setActiveDestinationscover important edge cases including empty destinations, missing displayName, and malformed postConsent integrations. This enhances test robustness significantly.
603-608: Excellent: Proper structured error handling with groupingHashThe error assertion correctly uses the new structured format including the
groupingHashproperty for proper Bugsnag grouping, which aligns perfectly with the PR objectives.
751-756: LGTM: Consistent error handling patternThe timeout error handling test consistently uses the structured error object format with proper context and grouping hash, maintaining the standardized error reporting pattern.
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (7)
151-151: LGTM: Simplified error message formatThe updated timeout rejection test correctly expects a cleaner error message without destination identifiers, improving error message consistency.
266-283: Excellent: Important error case coverageThe new test for handling missing
rudderanalyticsglobal object ensures proper error handling when the SDK is not properly initialized, which is a critical edge case.🧰 Tools
🪛 Biome (1.9.4)
[error] 268-268: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
319-338: Good: Enhanced SDK mounting detection testsThe additional tests for missing SDK type, missing prototype, and logger parameter provide comprehensive coverage of the
isDestinationSDKMountedfunction edge cases.
699-766: Excellent: Comprehensive override testingThe new tests for
applySourceConfigurationOverridescover multiple scenarios including edge cases with unmatched IDs, missing logger, and complex override scenarios. This significantly improves test coverage.
1189-1194: Perfect: Structured error handling with groupingHashThe error handling test correctly validates the new structured error format with proper context, custom message, and groupingHash properties. This demonstrates excellent alignment with the PR's error standardization goals.
1352-1358: Excellent: Consistent error structure in initialization testsThe initialization error test properly uses the structured error object format with context, custom message, and groupingHash, maintaining consistency with the broader error handling refactoring.
1394-1401: LGTM: Timeout error handling validationThe timeout scenario test correctly validates the structured error format for SDK readiness timeouts, ensuring proper error grouping and context information.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts (1)
166-348: Well-structured tests for plugin unregistration with one minor suggestion.The unregistration tests are comprehensive and properly handle the pluginsInventory mocking. However, the repeated pattern of mocking and restoring the pluginsInventory module could be extracted to a helper function to reduce code duplication.
Consider extracting the pluginsInventory mocking pattern into a helper function:
+ const mockPluginsInventory = (inventory: Record<string, any>) => { + const pluginsInventoryModule = require('../../../src/components/pluginsManager/pluginsInventory'); + const originalInventory = pluginsInventoryModule.pluginsInventory; + pluginsInventoryModule.pluginsInventory = inventory; + return () => { + pluginsInventoryModule.pluginsInventory = originalInventory; + }; + };Then use it in tests:
- const pluginsInventoryModule = require('../../../src/components/pluginsManager/pluginsInventory'); - const originalInventory = pluginsInventoryModule.pluginsInventory; - pluginsInventoryModule.pluginsInventory = mockPluginsInventory; + const restore = mockPluginsInventory({ + TestPlugin1: () => ({ name: 'TestPlugin1' }), + TestPlugin2: () => ({ name: 'TestPlugin2' }), + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Security and code quality checks
- GitHub Check: Bundle size checks
- GitHub Check: Unit Tests and Lint
🔇 Additional comments (2)
packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts (2)
15-164: Excellent comprehensive test coverage for plugin registration.The test suite provides thorough coverage of the
registermethod including successful registration, error handling, empty arrays, and various error types. The error handler assertions correctly include thegroupingHash: undefinedparameter as required.
411-424: Enhanced mock destination objects improve test realism.The additions of detailed properties like
displayName,userFriendlyId,shouldApplyDeviceModeTransformation,enabled,propagateEventsUntransformedOnError,blacklistedEvents,whitelistedEvents, andeventFilteringOptionmake the mock objects more representative of real destination configurations.Also applies to: 440-453, 469-482, 501-528, 545-558
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/analytics-js/__tests__/services/StoreManager/Store.test.ts (1)
140-157: Consider adding explicit verification of circular reference handling.While the test correctly verifies that no error is thrown, it could be more explicit about how circular references are handled.
Consider adding a more specific assertion about the circular reference handling:
// Circular reference should be handled (removed or replaced) + expect(retrieved.self).toBeUndefined(); // or whatever the expected behavior is
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts(6 hunks)packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts(3 hunks)packages/analytics-js/__tests__/services/StoreManager/Store.test.ts(2 hunks)packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts(5 hunks)packages/analytics-js/src/services/ErrorHandler/utils.ts(3 hunks)packages/analytics-js/src/services/StoreManager/Store.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/analytics-js/src/services/StoreManager/Store.ts
- packages/analytics-js/src/services/ErrorHandler/utils.ts
- packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts
- packages/analytics-js/tests/services/ErrorHandler/utils.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts (5)
packages/analytics-js/__fixtures__/fixtures.ts (1)
entriesWithOnlyCookieStorage(744-744)packages/analytics-js/src/services/HttpClient/HttpClient.ts (1)
defaultHttpClient(110-110)packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (1)
UserSessionManager(916-916)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (1)
defaultErrorHandler(196-196)examples/chrome-extension/content-script-v3/foreground.js (1)
COOKIE_KEYS(940-940)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Bundle size checks
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
🔇 Additional comments (12)
packages/analytics-js/__tests__/services/StoreManager/Store.test.ts (6)
27-37: LGTM: Well-implemented localStorage proxy enhancements.The
keys()method correctly iterates through localStorage and filters out null keys, and theisEnabledflag provides a useful capability flag for the proxy.
87-108: Excellent test coverage for invalid key validation.This test properly verifies that the Store doesn't attempt storage operations for invalid keys, which is important for preventing unnecessary storage calls and maintaining data integrity.
110-119: Good coverage of falsy value handling with clear expectations.The test appropriately maps different falsy values:
undefinedandnull→null(normalized)'',0,false→ preserved as-isThis behavior makes sense for a storage system where undefined values should be treated as null.
121-138: Thorough edge case testing for empty string decryption.This test ensures that when decryption returns an empty string, the store correctly treats it as a null value, which is good defensive programming.
159-183: Excellent alignment with standardized error handling objectives.This test perfectly validates the new structured error reporting format with
error,context,customMessage, andgroupingHashfields, directly supporting the PR's goal of standardizing error handling across the codebase.
185-205: Comprehensive quota exceeded error handling validation.This test thoroughly covers the quota exceeded scenario by verifying both the warning log and the engine swap to in-memory storage, ensuring graceful degradation when storage is unavailable.
packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts (6)
1143-1147: LGTM: Test correctly verifies migration with specific keys.The test assertion properly verifies that
migrateStorageIfNeededis called with the expected parameters when retrieving session info, ensuring the migration functionality works correctly for specific keys.
1174-1197: Good test coverage for specific key migration functionality.This test case effectively validates that the
getEntryValuemethod correctly callsmigrateStorageIfNeededwith specific keys, providing good coverage for the enhanced migration functionality.
2139-2172: Well-structured test for comprehensive error handling.This test effectively validates the complete error handling flow, including the fallback behavior when
getEncryptedCookieDatafails. The test properly verifies that all cookies are processed via the callback when the server-side approach fails.
2188-2239: LGTM: Comprehensive API request validation.The test correctly validates that
makeRequestToSetCookieconstructs the proper HTTP request with all required parameters, including the correct URL, method, payload structure, and options.
2241-2309: Excellent test coverage for migration functionality.These test cases thoroughly validate the
migrateStorageIfNeededmethod behavior:
- Tests migration with specific keys vs. all keys
- Properly mocks the plugins manager to verify invocation counts
- Validates the correct parameters are passed to the migration function
The tests ensure the new optional
keysparameter works as intended.
2108-2137:Details
❌ Incorrect review comment
Verify error handler interface consistency with the refactoring.
The test calls
onErrorwith three separate parameters (error, message, grouping hash), but according to the AI summary, the error handling refactoring standardized the interface to use a single structured object. Please confirm this is the correct interface.
🏁 Script executed:
#!/bin/bash # Search for the onError method definition and usage patterns to verify the interface echo "Searching for onError method definitions..." ast-grep --pattern 'onError($$$) { $$$ }' echo -e "\nSearching for onError method calls in UserSessionManager..." rg -A 3 -B 1 'onError\(' --include="*UserSessionManager*" echo -e "\nSearching for error handler interface definitions..." ast-grep --pattern 'interface $_ErrorHandler { $$$ onError: $$$ $$$ }'Length of output: 979
🏁 Script executed:
#!/bin/bash # Locate the UserSessionManager source file echo "Locating UserSessionManager source file..." USER_SESSION_FILE=$(rg -l "class UserSessionManager" -g "*.ts") echo "Found: $USER_SESSION_FILE" # Show the onError method definition with surrounding context echo -e "\n-- onError definition in UserSessionManager --" rg -n "onError" -C3 "$USER_SESSION_FILE" # Search for any global ErrorHandler interface or type that declares onError echo -e "\n-- Searching for ErrorHandler interface/type definitions --" rg -n "interface ErrorHandler" -C3 -g "*.ts" rg -n "type ErrorHandler" -C3 -g "*.ts" # Search for any standalone onError property/type signatures echo -e "\n-- Searching for onError property/type declarations --" rg -n "onError:" -C3 -g "*.ts"Length of output: 3923
No changes needed—test correctly targets the wrapper’s three-parameter signature
The
onErrormethod onUserSessionManagerstill accepts(error, customMessage?, groupingHash?)and internally forwards a single object to the injectederrorHandler. The test spies the class method (not the handler’s interface), so it’s valid as written.Likely an incorrect or invalid review comment.
|
saikumarrs
deleted the
feature/sdk-3304-fix-bugsnag-grouping-for-error-messages-with-ids
branch
PR Description
Please include a summary of the change along with the relevant motivation and context.
Linear task (optional)
https://linear.app/rudderstack/issue/SDK-3304/fix-bugsnag-grouping-for-error-messages-with-ids
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit