fix: error filtering of non-errors #1811
Conversation
WalkthroughWalkthroughThe recent changes significantly enhance the error reporting capabilities within the analytics JavaScript plugins. A new Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1811 +/- ##
===========================================
+ Coverage 56.61% 56.63% +0.01%
===========================================
Files 473 473
Lines 16093 16108 +15
Branches 3219 3226 +7
===========================================
+ Hits 9111 9122 +11
+ Misses 5738 5737 -1
- Partials 1244 1249 +5 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/analytics-js-plugins/tests/errorReporting/utils.test.ts (1 hunks)
- packages/analytics-js-plugins/src/errorReporting/utils.ts (1 hunks)
- packages/analytics-js/tests/components/configManager/commonUtil.test.ts (2 hunks)
- packages/analytics-js/src/components/configManager/util/commonUtil.ts (1 hunks)
Additional comments not posted (5)
packages/analytics-js-plugins/src/errorReporting/utils.ts (1)
147-150: LGTM! But verify the function usage in the codebase.The new conditional check to filter out non-error messages is a good addition. The code changes are approved.
However, ensure that all function calls to
isRudderSDKErrorare tested to verify the new behavior.Verification successful
LGTM! The new conditional check to filter out non-error messages is a good addition. The code changes are approved.
The function
isRudderSDKErroris used and tested in multiple places within the codebase, confirming that the new behavior is verified.
- Verified usage in:
packages/analytics-js-plugins/src/errorReporting/utils.tspackages/analytics-js-plugins/src/errorReporting/index.tspackages/analytics-js-plugins/src/bugsnag/utils.tspackages/analytics-js-plugins/__tests__/errorReporting/utils.test.tspackages/analytics-js-plugins/__tests__/bugsnag/utils.test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `isRudderSDKError` match the new behavior. # Test: Search for the function usage. Expect: Only occurrences of the new behavior. rg --type python -A 5 $'isRudderSDKError'Length of output: 71
Script:
#!/bin/bash # Description: Verify all function calls to `isRudderSDKError` match the new behavior. # Test: Search for the function usage. Expect: Only occurrences of the new behavior. rg --type typescript -A 5 'isRudderSDKError'Length of output: 79
Script:
#!/bin/bash # Description: Verify all function calls to `isRudderSDKError` match the new behavior. # Test: Search for the function usage in `.ts` files. Expect: Only occurrences of the new behavior. rg 'isRudderSDKError' --glob '*.ts' -A 5Length of output: 7454
packages/analytics-js/src/components/configManager/util/commonUtil.ts (1)
76-77: LGTM! But verify the function usage in the codebase.The changes to dynamically enable or disable error reporting based on the current configuration are a good addition. The code changes are approved.
However, ensure that all function calls to
updateReportingStateare tested to verify the new behavior.Verification successful
Let's correct the file type and search for the function
updateReportingStatein the TypeScript codebase.
Function usage verified and adequately tested.
The function
updateReportingStateis used in the main codebase and is covered by tests incommonUtil.test.ts. The new behavior is verified to be in place.
Code usage:
packages/analytics-js/src/components/configManager/ConfigManager.tsTest coverage:
packages/analytics-js/__tests__/components/configManager/commonUtil.test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `updateReportingState` match the new behavior. # Test: Search for the function usage. Expect: Only occurrences of the new behavior. rg --type python -A 5 $'updateReportingState'Length of output: 75
Script:
#!/bin/bash # Description: Verify all function calls to `updateReportingState` match the new behavior. # Test: Search for the function usage. Expect: Only occurrences of the new behavior. rg --type ts -A 5 'updateReportingState'Length of output: 5425
packages/analytics-js-plugins/__tests__/errorReporting/utils.test.ts (1)
119-125: LGTM!The new test case for the
isRudderSDKErrorfunction correctly identifies non-error messages. The code changes are approved.packages/analytics-js/__tests__/components/configManager/commonUtil.test.ts (2)
113-115: Confirm the uncommenting of assertions for error and metrics reporting.The assertions for
state.reporting.isErrorReportingEnabled.valueandstate.reporting.isMetricsReportingEnabled.valuebeingtrueandmockLogger.warnnot being called are now active. Ensure that these conditions are met in the codebase.
136-138: Confirm the uncommenting of assertions for error and metrics reporting.The assertions for
state.reporting.isErrorReportingEnabled.valueandstate.reporting.isMetricsReportingEnabled.valuebeingtrueandmockLogger.warnnot being called are now active. Ensure that these conditions are met in the codebase.
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/analytics-js-plugins/src/errorReporting/event/event.ts (6 hunks)
- packages/analytics-js-plugins/src/errorReporting/index.ts (1 hunks)
- packages/analytics-js/tests/services/ErrorHandler/processError.test.ts (1 hunks)
- packages/analytics-js/src/services/ErrorHandler/processError.ts (1 hunks)
Additional comments not posted (5)
packages/analytics-js/src/services/ErrorHandler/processError.ts (1)
42-60: Reintroduced script loading error handling: Verify correctnessThe reintroduced code block correctly handles script loading errors by checking if the error is an instance of
Eventand performing specific checks on the event target. Ensure that this logic aligns with the broader error handling strategy and does not introduce any regressions.Verification successful
Reintroduced script loading error handling: Verified correctness
The reintroduced code block correctly handles script loading errors by checking if the error is an instance of
Eventand performing specific checks on the event target. The logic aligns with the broader error handling strategy and is well-verified by existing tests.
packages/analytics-js/__tests__/services/ErrorHandler/processError.test.ts: Contains multiple tests verifying the behavior ofgetNormalizedErrorForUnhandledError.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the reintroduced script loading error handling logic. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type python -A 5 $'getNormalizedErrorForUnhandledError'Length of output: 90
Script:
#!/bin/bash # Description: Verify the correctness of the reintroduced script loading error handling logic. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'getNormalizedErrorForUnhandledError'Length of output: 98
Script:
#!/bin/bash # Description: Verify the correctness of the reintroduced script loading error handling logic. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type ts -A 5 $'getNormalizedErrorForUnhandledError'Length of output: 8969
packages/analytics-js-plugins/src/errorReporting/index.ts (1)
87-87: Added check forerrorPayload: Verify correctnessThe added check for
errorPayloadbefore verifying if the first error inerrorPayload.errorsis a Rudder SDK error ensures that the function does not attempt to access properties oferrorPayloadwhen it is undefined or null. This change improves the robustness of the error handling logic.packages/analytics-js/__tests__/services/ErrorHandler/processError.test.ts (1)
98-98: Re-enabled test case: Verify correctnessThe re-enabled test case "should return error instance for Event argument value with SDK script target" ensures that the function correctly returns an error instance for an Event argument value with an SDK script target. This change suggests that the developers have resolved previous issues or determined that the test is now relevant for the current implementation.
packages/analytics-js-plugins/src/errorReporting/event/event.ts (2)
76-79: LGTM! But verify the function usage in the codebase.The changes to
logInputErrorare approved. The function now logs a warning and returnsundefined, which simplifies the error handling process.However, ensure that all function calls to
logInputErrorare correctly handled in the codebase.Verification successful
Verified: Function usage is correctly handled.
The
logInputErrorfunction is used appropriately within the filepackages/analytics-js-plugins/src/errorReporting/event/event.ts, and all instances are correctly integrated.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `logInputError` are correctly handled in the codebase. # Test: Search for the function usage. Expect: Only occurrences of the new implementation. rg --type ts -A 5 $'logInputError'Length of output: 3240
Line range hint
94-119:
Verify the function's behavior with various inputs.The changes simplify the handling of non-error inputs. Ensure that the function correctly handles all input types and scenarios.
Verification successful
Verify the function's behavior with various inputs.
The
normaliseErrorfunction correctly handles various input types, including strings, numbers, booleans, functions, objects, and null values. The implementation ensures that unsupported input types are logged appropriately.
- The function is defined in
packages/analytics-js-plugins/src/errorReporting/event/event.tsat lines 67-139.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the behavior of `normaliseError` with various inputs. # Test: Create a script to test the function with different inputs. Expect: Correct handling of all input types. cat << 'EOF' > test_normaliseError.ts import { normaliseError } from './packages/analytics-js-plugins/src/errorReporting/event/event'; const logger = { warn: console.warn, }; const testCases = [ { input: 'string', tolerateNonErrors: false, component: 'test', expected: [new Error('string'), 1] }, { input: 42, tolerateNonErrors: false, component: 'test', expected: [new Error('42'), 1] }, { input: true, tolerateNonErrors: false, component: 'test', expected: [new Error('true'), 1] }, { input: () => {}, tolerateNonErrors: false, component: 'test', expected: [undefined, 0] }, { input: null, tolerateNonErrors: false, component: 'test', expected: [undefined, 0] }, { input: { name: 'Error', message: 'message' }, tolerateNonErrors: false, component: 'test', expected: [new Error('message'), 1] }, { input: new Error('error'), tolerateNonErrors: false, component: 'test', expected: [new Error('error'), 0] }, ]; testCases.forEach(({ input, tolerateNonErrors, component, expected }) => { const result = normaliseError(input, tolerateNonErrors, component, logger); console.assert(JSON.stringify(result) === JSON.stringify(expected), `Test failed for input: ${input}`); }); console.log('All tests passed.'); EOFLength of output: 5
Script:
#!/bin/bash # Verify the existence and implementation of the `normaliseError` function in the specified file. # Search for the definition of the `normaliseError` function. ast-grep --lang typescript --pattern 'function normaliseError($_, $_, $_, $_) { $$$ }' # Search for the usage of `normaliseError` function in the codebase to understand its context and ensure it is used correctly. rg 'normaliseError'Length of output: 684
Script:
#!/bin/bash # Extract the implementation of the `normaliseError` function for review. ast-grep --lang typescript --pattern 'const normaliseError = ($_,$_, $_, $_) => { $$$ }'Length of output: 7230
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- packages/analytics-js-plugins/tests/errorReporting/utils.test.ts (3 hunks)
- packages/analytics-js-plugins/src/errorReporting/utils.ts (3 hunks)
- packages/analytics-js/tests/services/ErrorHandler/processError.test.ts (3 hunks)
- packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (2 hunks)
- packages/analytics-js/src/services/ErrorHandler/processError.ts (2 hunks)
- packages/analytics-js/src/services/HttpClient/xhr/xhrRequestHandler.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/analytics-js/src/services/HttpClient/xhr/xhrRequestHandler.ts
Files skipped from review as they are similar to previous changes (3)
- packages/analytics-js-plugins/tests/errorReporting/utils.test.ts
- packages/analytics-js-plugins/src/errorReporting/utils.ts
- packages/analytics-js/tests/services/ErrorHandler/processError.test.ts
Additional comments not posted (2)
packages/analytics-js/src/services/ErrorHandler/processError.ts (1)
41-59: Ensure theEventerror handling logic is necessary.The added code segment handles errors that are instances of
Event, specifically focusing on script loading errors. This logic seems to enhance error reporting for third-party script loading failures. Verify that this logic is necessary and beneficial for the overall error handling strategy.packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (1)
203-203: Verify the impact of removingisAllowedToBeNotifiedcondition.The removal of the
isAllowedToBeNotified(error)condition simplifies the control flow but may increase the number of errors reported. Ensure that this change aligns with the overall error reporting strategy and does not introduce noise in the error logs.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- packages/analytics-js-common/src/constants/errors.ts (1 hunks)
- packages/analytics-js-plugins/tests/errorReporting/utils.test.ts (2 hunks)
- packages/analytics-js-plugins/src/errorReporting/event/event.ts (3 hunks)
- packages/analytics-js-plugins/src/errorReporting/index.ts (3 hunks)
- packages/analytics-js-plugins/src/errorReporting/utils.ts (3 hunks)
Files skipped from review due to trivial changes (1)
- packages/analytics-js-common/src/constants/errors.ts
Files skipped from review as they are similar to previous changes (4)
- packages/analytics-js-plugins/tests/errorReporting/utils.test.ts
- packages/analytics-js-plugins/src/errorReporting/event/event.ts
- packages/analytics-js-plugins/src/errorReporting/index.ts
- packages/analytics-js-plugins/src/errorReporting/utils.ts
packages/analytics-js-plugins/src/errorReporting/event/event.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js-plugins/src/errorReporting/event/event.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js-plugins/src/errorReporting/event/event.ts
Outdated
Show resolved
Hide resolved
| } | ||
| if (maybeError !== null && isError(maybeError)) { | ||
| error = maybeError; | ||
| } else if (maybeError !== null && hasNecessaryFields(maybeError)) { |
There was a problem hiding this comment.
Shouldn't we check if it is an object first?
packages/analytics-js-plugins/src/errorReporting/event/event.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- packages/analytics-js-plugins/tests/errorReporting/utils.test.ts (2 hunks)
- packages/analytics-js-plugins/src/errorReporting/constants.ts (2 hunks)
- packages/analytics-js-plugins/src/errorReporting/event/event.ts (3 hunks)
- packages/analytics-js-plugins/src/errorReporting/index.ts (2 hunks)
- packages/analytics-js-plugins/src/errorReporting/utils.ts (3 hunks)
Files skipped from review due to trivial changes (1)
- packages/analytics-js-plugins/src/errorReporting/constants.ts
Files skipped from review as they are similar to previous changes (3)
- packages/analytics-js-plugins/tests/errorReporting/utils.test.ts
- packages/analytics-js-plugins/src/errorReporting/event/event.ts
- packages/analytics-js-plugins/src/errorReporting/utils.ts
Additional comments not posted (2)
packages/analytics-js-plugins/src/errorReporting/index.ts (2)
87-89: Improved robustness: Updated check forerrorPayload.The updated check for
errorPayloadensures it is defined, reducing the risk of runtime errors and enhancing the stability of the error reporting functionality.
87-89: Good addition: Added validation for error payloads.The
isAllowedToBeNotified(errorPayload.errors[0])check ensures that only valid errors are processed, enhancing the robustness of the error handling logic.However, verify the usage of the
isAllowedToBeNotifiedfunction to ensure it is correctly implemented and tested.
packages/analytics-js-plugins/src/errorReporting/event/event.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js-plugins/src/errorReporting/event/event.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/analytics-js-plugins/src/errorReporting/event/event.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js-plugins/src/errorReporting/event/event.ts
|
PR Description
Linear task (optional)
Linear task link
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
Bug Fixes
Tests