feat: move data plane events processing to core #1999
Conversation
…porting-to-core-sdk-2826
📝 WalkthroughWalkthroughThis pull request introduces significant architectural changes to the RudderStack JavaScript SDK, primarily focusing on removing beacon and XHR queue-related functionalities and introducing a new Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant ER as EventRepository
participant DPQ as DataPlaneEventsQueue
participant HC as HttpClient
App->>ER: Initialize
ER->>DPQ: Create New Queue
App->>ER: Enqueue Event
ER->>DPQ: Enqueue Event
DPQ->>HC: Send Event
alt Event Successful
HC-->>DPQ: Success
DPQ-->>ER: Event Processed
else Event Failed
HC-->>DPQ: Error
DPQ->>DPQ: Retry or Drop Event
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…porting-to-core-sdk-2826
…e-sdk-2826' into feat.move-event-handling-to-core-sdk-2823
…ndling-to-core-sdk-2823
saikumarrs
changed the base branch from
feat.move-error-reporting-to-core-sdk-2826
to
feat.move-error-reporting-to-core-sdk-2826-new
…ndling-to-core-sdk-2823
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1999 +/- ##
===========================================
+ Coverage 61.36% 61.54% +0.18%
===========================================
Files 478 473 -5
Lines 16393 16317 -76
Branches 3239 3229 -10
===========================================
- Hits 10060 10043 -17
+ Misses 5118 5046 -72
- Partials 1215 1228 +13 ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts
Dismissed
Show dismissed
Hide dismissed
packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts
Dismissed
Show dismissed
Hide dismissed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (32)
packages/analytics-js-common/src/component-cookie/index.ts (1)
25-25: Consider at least logging errors in development builds for easier debugging.Currently, errors are silently ignored in the catch block, which can impede troubleshooting unexpected decoding issues. If the goal is to avoid polluting logs for non-URI-encoded cookies, consider adding conditional or debug-only logging rather than entirely suppressing it.
packages/analytics-js-common/__mocks__/StoreManager.ts (1)
30-31: Consider exposing configuration options.Introducing these properties in the mock is beneficial for testing error handling and logging, but consider allowing an override in different test setups or production code to ensure custom error handling and logging can be injected when needed.
packages/analytics-js/rollup.config.mjs (1)
182-182: Consider providing a fallback when referencingpkg.repository.url
Although referencingpkg.repository.urlis typically safe, consider verifying thatpkg.repositoryis defined (or provide a fallback) to avoid build-time issues in scenarios where therepositoryfield is missing frompackage.json.packages/analytics-js/src/components/core/Analytics.ts (1)
215-218: Add handling for changes to thewriteKeyafter initialization.Currently, the SDK sets the Auth header only once if
state.lifecycle.writeKey.valueis available during the mount phase. If thewriteKeychanges after mounting, the header may not be updated accordingly. If your use case may require changing thewriteKeyat runtime, consider re-invokingsetAuthHeaderin response to state changes. Otherwise, this approach is fine if thewriteKeyis fixed after loading.packages/analytics-js/__tests__/app/RudderAnalytics.test.ts (1)
Line range hint
786-806: Consider resetting the globalstate.lifecycle.loaded.valueafter this test.You're mutating a shared global state (
state.lifecycle.loaded.value) for test verification but never revert it back to its default. This might cause unpredictable behavior or side effects in subsequent tests. A quick fix is to resetstate.lifecycle.loaded.value(e.g., tofalse) in theafterEachblock.packages/analytics-js-common/src/types/HttpClient.ts (2)
39-40: Add usage notes forIBaseRequestOptionsandIRequestOptions.
useAuthis optional, but confirm default behaviors (i.e., ifuseAuthisn’t specified, do we skip or include auth?). EnsureIFetchRequestOptionsis well-documented, especially for cross-fetch usage.Also applies to: 43-44
50-60: DeprecategetAsyncDatagradually.The doc comment calls it deprecated; ensure you remove references across the code. Provide transitional guidance or timeline if external plugins depend on it.
packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts (3)
23-23: Avoid duplicating helper utilities.
isDefinedAndNotNullandisNonEmptyObjectare reused in multiple places. Confirm they’re centralized to prevent version drift if used frequently.
68-88: Refactor repeated error handling.
reqFailedHandleris valuable but repeated logic from lines 82–87 might exist in other flows. Consider centralizing shared patterns to reduce duplication.
95-147: Increase test coverage around conditional flows.Multiple lines here (e.g., 97, 102, 106, 108, 110, 113, 115–116, 137, 143, 146–147) lack coverage per codecov warnings. Add tests verifying 404, partial transformations, or no
responsescenarios to ensure robust coverage.Would you like help drafting additional test cases or opening a follow-up to track improved coverage?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 97-97: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L97
Added line #L97 was not covered by tests
[warning] 102-102: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L102
Added line #L102 was not covered by tests
[warning] 106-106: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L106
Added line #L106 was not covered by tests
[warning] 108-108: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L108
Added line #L108 was not covered by tests
[warning] 110-110: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L110
Added line #L110 was not covered by tests
[warning] 113-113: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L113
Added line #L113 was not covered by tests
[warning] 115-116: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L115-L116
Added lines #L115 - L116 were not covered by tests
[warning] 137-137: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L137
Added line #L137 was not covered by tests
[warning] 143-143: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L143
Added line #L143 was not covered by tests
[warning] 146-147: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L146-L147
Added lines #L146 - L147 were not covered by testspackages/analytics-js-common/__mocks__/HttpClient.ts (1)
8-8: Adopt “request” method consistently and simplify mocks.Renaming
getDatatorequestaligns with the broader refactoring. Verify whethergetAsyncDatais still required; consider removing or renaming it for consistency if it is no longer used.packages/analytics-js-common/src/services/HttpClientError.ts (2)
8-10: Add unit tests to increase coverageThe constructor and its logic for handling
originalError(lines #9, #14-15, #18-20) are not currently covered by unit tests according to static analysis. Consider writing additional tests to verify:
- The class correctly adopts fields such as
status,statusText, andresponseBody.- The error message is properly appended when
originalErroris present.Below is a skeleton test snippet you might consider using (in a new or existing test file, e.g.,
HttpClientError.test.ts):+ import { HttpClientError } from '../services/HttpClientError'; + describe('HttpClientError', () => { + it('should assign properties and handle originalError correctly', () => { + const originalErr = new Error('Original error message'); + const error = new HttpClientError('Client error occurred', { + status: 404, + statusText: 'Not Found', + responseBody: '{"error":"Resource not found"}', + originalError: originalErr, + }); + + expect(error.status).toBe(404); + expect(error.statusText).toBe('Not Found'); + expect(error.responseBody).toBe('{"error":"Resource not found"}'); + expect(error.message).toBe('Client error occurred: Original error message'); + }); + });Also applies to: 11-11, 13-17, 18-21
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 9-9: packages/analytics-js-common/src/services/HttpClientError.ts#L9
Added line #L9 was not covered by tests
24-24: Export coverage testLine #24 (the export statement) is also flagged as uncovered by tests. Although an export line typically requires no direct test, static analysis may flag uncovered lines. If you’d like to address it, ensure that at least one test file imports and uses
HttpClientErrordirectly to confirm that this export path is exercised.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 24-24: packages/analytics-js-common/src/services/HttpClientError.ts#L24
Added line #L24 was not covered by testspackages/analytics-js-common/src/utilities/retryQueue/constants.ts (1)
23-33: Add descriptive comments for new queue statuses
Each of these queue statuses is meaningful to the retry logic. Consider adding short doc comments explaining when and how each status is used.packages/analytics-js/__tests__/components/capabilitiesManager/detection/adBlockers.test.ts (2)
10-11: Explicit test initialization
InitializingsourceConfigUrlhelps ensure each test starts with a known configuration. Keep watch for tests that might accidentally depend on this URL.
18-18: Consider adding in-code documentation
responseObjis a pivotal variable in these tests. A brief comment on its structure and intent would increase maintainability.packages/analytics-js-common/src/types/Source.ts (3)
1-3: Focus on consistent naming and placement for imports
Importing your type definitions from separate files is helpful for maintainability, but ensure that each file has a clearly defined purpose (e.g., grouping related types together). Keep it consistent to avoid confusion as the codebase grows.
11-21: Validate destination item structure at runtime
This type definition is comprehensive for each destination item. In the future, ensure there's a runtime check that matches these fields to detect partial or malformed data, especially before you attempt to read nested properties in your logic.
33-44: Consider restricting 'any' usage
category,config,configSchema,options, anduiConfiguseNullable<any>. If possible, refine these to more specific types to prevent unexpected data structures.packages/analytics-js/__fixtures__/msw.handlers.ts (1)
84-90: Potential test flakiness with 11-second delay.
Consider lowering the timeout or mocking time to avoid slow tests.-}, 11000); // 1 second more than the default timeout value +}, 2000); // reduce delay to minimize test runtimepackages/analytics-js-common/src/types/LoadOptions.ts (1)
139-141: Double-check handling of deprecated properties.
PropertiesuseBeaconandbeaconQueueOptionsare marked as deprecated. Ensure that any internal uses or backward compatibility logic remains functional, and that new code relies onqueueOptionsor the relevant replacements.Also applies to: 143-145
packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts (1)
23-23: Add test coverage for RetryQueue usage.
Line 23 references and initializesRetryQueuebut is uncovered by tests. Consider adding a test to ensure proper instantiation and behavior of the queue.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 23-23: packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts#L23
Added line #L23 was not covered by testspackages/analytics-js/__tests__/browser.test.ts (2)
1-1: Clarify necessity of compat rule disabling.
Ensure that disablingcompat/compatis truly needed for broader browser support (including IE11 testing). If possible, re-enable it to catch any browser compatibility issues early.
98-98: Skipping tests might reduce coverage.
The.skipmodifier on these tests prevents code coverage from improving. Consider enabling or revisiting them to confirm that the tested functionalities still behave as intended.Also applies to: 122-122
packages/analytics-js/__tests__/services/StoreManager/Store.test.ts (1)
67-75: Avoid duplicated test blocks
The test name"should log a warning if the underlying cookie value is a legacy encrypted value"appears twice with similar logic. Consider merging them or clarifying functional differences to reduce duplication.packages/analytics-js/src/app/RudderAnalytics.ts (1)
252-269: Validate page unload logic.
TrackingUNLOADEDwhenisAccessible === falseandstate.lifecycle.loaded.valueis straightforward. However, consider edge cases (e.g., single-page apps or real-time dynamic unload events). Also, verify that negative or zerovisitDurationis handled gracefully.packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts (3)
258-258: Batch queue additions and flush logic.
Appending items toBATCH_QUEUEand moving them to the main queue when criteria are met is correct. Keep an eye on concurrency issues if events arrive in quick succession. Testing for race conditions would be beneficial.Also applies to: 267-267, 333-333, 366-366, 371-371, 377-377
589-594: Reclaim flow merges queue data.
The approach of collecting remote queue items into the local queue is practical, but be mindful of potential stale data or in-progress collisions. Also confirm that removing stale entries cleans them effectively.Also applies to: 658-658, 675-675
709-713: Check reclaim timers.
UsingRECLAIM_START/RECLAIM_ENDwith timed intervals requires consistent scheduling. Synchronize the approach across tabs to avoid inadvertently overwriting data or reclaiming a queue in active use.Also applies to: 720-724, 733-734, 766-766, 785-785
packages/analytics-js/__tests__/components/core/Analytics.test.ts (1)
261-263: Clearer error reporting message.
Explicit “LoadAPI:: The provided callback parameter is not a function” helps with debugging. The updated text is more user-friendly.packages/analytics-js-common/__tests__/utilities/retryQueue/RetryQueue.test.ts (1)
315-316: Consider consistently typing 'ack' store key.
The store is defined withvalidKeys: QueueStatuses, but'ack' as stringmay become error-prone. Consider introducing a typed key for'ack'if it’s a stable part of the store interface.- foundQueue.set('ack' as string, 0); + // Example: typed approach if "ack" is a recognized property of the store + foundQueue.set('ack', 0);Also applies to: 319-319, 321-321
packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts (1)
52-54: Avoidanyif possible.
DefiningclientDataStoreCookie,clientDataStoreLS, andclientDataStoreSessionasanysacrifices type safety. Consider providing a minimal interface to improve clarity.- let clientDataStoreCookie: any; - let clientDataStoreLS: any; - let clientDataStoreSession: any; + import type { Store } from '../../../src/services/StoreManager/Store'; + let clientDataStoreCookie: Store; + let clientDataStoreLS: Store; + let clientDataStoreSession: Store;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.eslintrc.jsonis excluded by!**/*.jsonpackages/analytics-js-plugins/tsconfig.jsonis excluded by!**/*.json
📒 Files selected for processing (82)
jest/jest.polyfills.js(0 hunks)packages/analytics-js-common/__mocks__/HttpClient.ts(1 hunks)packages/analytics-js-common/__mocks__/Store.ts(4 hunks)packages/analytics-js-common/__mocks__/StoreManager.ts(2 hunks)packages/analytics-js-common/__tests__/utilities/retryQueue/RetryQueue.test.ts(18 hunks)packages/analytics-js-common/__tests__/utilities/url.test.ts(1 hunks)packages/analytics-js-common/src/component-cookie/index.ts(1 hunks)packages/analytics-js-common/src/constants/QueueStatuses.ts(0 hunks)packages/analytics-js-common/src/constants/index.ts(0 hunks)packages/analytics-js-common/src/services/HttpClientError.ts(1 hunks)packages/analytics-js-common/src/types/ApplicationState.ts(0 hunks)packages/analytics-js-common/src/types/HttpClient.ts(1 hunks)packages/analytics-js-common/src/types/LoadOptions.ts(3 hunks)packages/analytics-js-common/src/types/PluginsManager.ts(1 hunks)packages/analytics-js-common/src/types/Source.ts(1 hunks)packages/analytics-js-common/src/types/Store.ts(2 hunks)packages/analytics-js-common/src/utilities/http.ts(1 hunks)packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts(19 hunks)packages/analytics-js-common/src/utilities/retryQueue/constants.ts(2 hunks)packages/analytics-js-common/src/utilities/retryQueue/logMessages.ts(1 hunks)packages/analytics-js-common/src/utilities/retryQueue/types.ts(1 hunks)packages/analytics-js-integrations/__tests__/integrations/Braze/browser.test.js(0 hunks)packages/analytics-js-integrations/src/integrations/Braze/utils.js(1 hunks)packages/analytics-js-plugins/__mocks__/state.ts(1 hunks)packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts(4 hunks)packages/analytics-js-plugins/__tests__/xhrQueue/index.test.ts(0 hunks)packages/analytics-js-plugins/__tests__/xhrQueue/utilities.test.ts(0 hunks)packages/analytics-js-plugins/rollup.config.mjs(0 hunks)packages/analytics-js-plugins/src/beaconQueue/constants.ts(0 hunks)packages/analytics-js-plugins/src/beaconQueue/index.ts(0 hunks)packages/analytics-js-plugins/src/beaconQueue/logMessages.ts(0 hunks)packages/analytics-js-plugins/src/beaconQueue/types.ts(0 hunks)packages/analytics-js-plugins/src/beaconQueue/utilities.ts(0 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/index.ts(2 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts(5 hunks)packages/analytics-js-plugins/src/index.ts(0 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/shared-chunks/common.ts(2 hunks)packages/analytics-js-plugins/src/types/plugins.ts(0 hunks)packages/analytics-js-plugins/src/utilities/retryQueue/types.ts(0 hunks)packages/analytics-js-plugins/src/xhrQueue/constants.ts(0 hunks)packages/analytics-js-plugins/src/xhrQueue/index.ts(0 hunks)packages/analytics-js-plugins/src/xhrQueue/logMessages.ts(0 hunks)packages/analytics-js-plugins/src/xhrQueue/types.ts(0 hunks)packages/analytics-js-plugins/src/xhrQueue/utilities.ts(0 hunks)packages/analytics-js/__fixtures__/fixtures.ts(13 hunks)packages/analytics-js/__fixtures__/msw.handlers.ts(2 hunks)packages/analytics-js/__mocks__/remotePlugins/BeaconQueue.ts(0 hunks)packages/analytics-js/__mocks__/remotePlugins/XhrQueue.ts(0 hunks)packages/analytics-js/__tests__/app/RudderAnalytics.test.ts(1 hunks)packages/analytics-js/__tests__/browser.test.ts(6 hunks)packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts(1 hunks)packages/analytics-js/__tests__/components/capabilitiesManager/detection/adBlockers.test.ts(3 hunks)packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts(4 hunks)packages/analytics-js/__tests__/components/configManager/commonUtil.test.ts(0 hunks)packages/analytics-js/__tests__/components/core/Analytics.test.ts(5 hunks)packages/analytics-js/__tests__/components/eventManager/EventManager.test.ts(1 hunks)packages/analytics-js/__tests__/components/eventRepository/EventRepository.test.ts(1 hunks)packages/analytics-js/__tests__/components/pluginsManager/PluginsManager.test.ts(0 hunks)packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts(4 hunks)packages/analytics-js/__tests__/components/utilities/event.test.ts(0 hunks)packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts(3 hunks)packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts(1 hunks)packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts(3 hunks)packages/analytics-js/__tests__/services/StoreManager/Store.test.ts(4 hunks)packages/analytics-js/__tests__/services/StoreManager/StoreManager.test.ts(1 hunks)packages/analytics-js/public/index.html(0 hunks)packages/analytics-js/rollup.config.mjs(1 hunks)packages/analytics-js/src/app/RudderAnalytics.ts(3 hunks)packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts(0 hunks)packages/analytics-js/src/components/capabilitiesManager/detection/adBlockers.ts(2 hunks)packages/analytics-js/src/components/capabilitiesManager/detection/browser.ts(1 hunks)packages/analytics-js/src/components/capabilitiesManager/detection/dom.ts(0 hunks)packages/analytics-js/src/components/capabilitiesManager/detection/index.ts(1 hunks)packages/analytics-js/src/components/configManager/ConfigManager.ts(6 hunks)packages/analytics-js/src/components/configManager/constants.ts(0 hunks)packages/analytics-js/src/components/configManager/types.ts(2 hunks)packages/analytics-js/src/components/configManager/util/commonUtil.ts(1 hunks)packages/analytics-js/src/components/configManager/util/validate.ts(1 hunks)packages/analytics-js/src/components/core/Analytics.ts(1 hunks)packages/analytics-js/src/components/pluginsManager/PluginsManager.ts(0 hunks)packages/analytics-js/src/components/pluginsManager/bundledBuildPluginImports.ts(0 hunks)
⛔ Files not processed due to max files limit (28)
- packages/analytics-js/src/components/pluginsManager/defaultPluginsList.ts
- packages/analytics-js/src/components/pluginsManager/federatedModulesBuildPluginImports.ts
- packages/analytics-js/src/components/pluginsManager/pluginNames.ts
- packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts
- packages/analytics-js/src/components/utilities/destinations.ts
- packages/analytics-js/src/components/utilities/loadOptions.ts
- packages/analytics-js/src/constants/logMessages.ts
- packages/analytics-js/src/constants/timeouts.ts
- packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
- packages/analytics-js/src/services/HttpClient/HttpClient.ts
- packages/analytics-js/src/services/HttpClient/fetch/index.ts
- packages/analytics-js/src/services/HttpClient/index.ts
- packages/analytics-js/src/services/HttpClient/xhr/xhrRequestHandler.ts
- packages/analytics-js/src/services/HttpClient/xhr/xhrResponseHandler.ts
- packages/analytics-js/src/services/StoreManager/Store.ts
- packages/analytics-js/src/state/slices/capabilities.ts
- packages/analytics-js/src/state/slices/dataPlaneEvents.ts
- packages/analytics-js/src/state/slices/loadOptions.ts
- packages/analytics-js/src/types/remote-plugins.d.ts
- packages/sanity-suite/public/v1.1/index-cdn.html
- packages/sanity-suite/public/v1.1/index-local.html
- packages/sanity-suite/public/v1.1/index-npm.html
- packages/sanity-suite/public/v1.1/integrations/index-cdn.html
- packages/sanity-suite/public/v1.1/integrations/index-local.html
- packages/sanity-suite/public/v1.1/integrations/index-npm.html
- packages/sanity-suite/public/v1.1/manualLoadCall/index-cdn.html
- packages/sanity-suite/public/v1.1/manualLoadCall/index-local.html
- packages/sanity-suite/public/v1.1/manualLoadCall/index-npm.html
💤 Files with no reviewable changes (32)
- packages/analytics-js-common/src/constants/index.ts
- packages/analytics-js/src/components/configManager/constants.ts
- packages/analytics-js-plugins/src/index.ts
- packages/analytics-js-common/src/constants/QueueStatuses.ts
- jest/jest.polyfills.js
- packages/analytics-js/src/components/capabilitiesManager/detection/dom.ts
- packages/analytics-js-plugins/rollup.config.mjs
- packages/analytics-js/tests/components/configManager/commonUtil.test.ts
- packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts
- packages/analytics-js/tests/components/pluginsManager/PluginsManager.test.ts
- packages/analytics-js/mocks/remotePlugins/XhrQueue.ts
- packages/analytics-js/public/index.html
- packages/analytics-js-integrations/tests/integrations/Braze/browser.test.js
- packages/analytics-js-plugins/src/xhrQueue/logMessages.ts
- packages/analytics-js/mocks/remotePlugins/BeaconQueue.ts
- packages/analytics-js-common/src/types/ApplicationState.ts
- packages/analytics-js-plugins/src/xhrQueue/constants.ts
- packages/analytics-js-plugins/src/utilities/retryQueue/types.ts
- packages/analytics-js/tests/components/utilities/event.test.ts
- packages/analytics-js-plugins/tests/xhrQueue/index.test.ts
- packages/analytics-js-plugins/src/beaconQueue/types.ts
- packages/analytics-js-plugins/src/beaconQueue/constants.ts
- packages/analytics-js-plugins/src/xhrQueue/types.ts
- packages/analytics-js-plugins/tests/xhrQueue/utilities.test.ts
- packages/analytics-js-plugins/src/types/plugins.ts
- packages/analytics-js-plugins/src/xhrQueue/index.ts
- packages/analytics-js/src/components/pluginsManager/bundledBuildPluginImports.ts
- packages/analytics-js-plugins/src/beaconQueue/logMessages.ts
- packages/analytics-js-plugins/src/beaconQueue/utilities.ts
- packages/analytics-js-plugins/src/xhrQueue/utilities.ts
- packages/analytics-js/src/components/pluginsManager/PluginsManager.ts
- packages/analytics-js-plugins/src/beaconQueue/index.ts
✅ Files skipped from review due to trivial changes (4)
- packages/analytics-js-common/tests/utilities/url.test.ts
- packages/analytics-js-common/src/utilities/retryQueue/logMessages.ts
- packages/analytics-js-integrations/src/integrations/Braze/utils.js
- packages/analytics-js/tests/services/StoreManager/StoreManager.test.ts
🧰 Additional context used
📓 Learnings (12)
packages/analytics-js-common/src/types/PluginsManager.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-plugins/src/xhrQueue/index.ts:34-34
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The deprecated plugins have been removed from the `PluginName` type, so the explicit `PluginName` type annotation is no longer necessary in `packages/analytics-js-plugins/src/xhrQueue/index.ts`.
packages/analytics-js/__fixtures__/msw.handlers.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1867
File: packages/analytics-js/__fixtures__/msw.handlers.ts:86-86
Timestamp: 2024-11-12T15:14:33.334Z
Learning: When updating test files to replace 'example.com' with 'dummy.dataplane.host.com', note that the change is only needed for test suites that actually make network requests.
packages/analytics-js/src/components/configManager/util/commonUtil.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/types/LoadOptions.ts:0-0
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In the `packages/analytics-js-common/src/types/LoadOptions.ts` file, the `dataplanes` property within the `SourceConfigResponse` type has been removed as it is no longer necessary.
packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts (3)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts:72-72
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In `UserSessionManager.test.ts`, when necessary, it's acceptable to directly set raw cookie values by accessing the original storage engine with `getOriginalEngine()`, instead of using the `Store` interface methods.
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1867
File: packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts:1692-1692
Timestamp: 2024-11-12T15:14:33.334Z
Learning: When updating test suites, changes to the domain (e.g., from 'example.com' to 'dummy.dataplane.host.com') are only necessary for tests that actually make network requests.
Learnt from: MoumitaM
PR: rudderlabs/rudder-sdk-js#1625
File: packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts:1374-1385
Timestamp: 2024-11-12T15:14:33.334Z
Learning: Multiple cookies with the same name are not possible in web browsers; a newer cookie will overwrite an older one if they share the same name.
packages/analytics-js/__tests__/app/RudderAnalytics.test.ts (1)
Learnt from: MoumitaM
PR: rudderlabs/rudder-sdk-js#1876
File: packages/analytics-js/src/app/RudderAnalytics.ts:0-0
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The `trackPageLifecycleEvents` method in `packages/analytics-js/src/app/RudderAnalytics.ts` does not require refactoring as the current implementation is preferred.
packages/analytics-js-common/__tests__/utilities/retryQueue/RetryQueue.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-12T15:14:23.319Z
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/src/app/RudderAnalytics.ts (1)
Learnt from: MoumitaM
PR: rudderlabs/rudder-sdk-js#1876
File: packages/analytics-js/src/app/RudderAnalytics.ts:0-0
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The `trackPageLifecycleEvents` method in `packages/analytics-js/src/app/RudderAnalytics.ts` does not require refactoring as the current implementation is preferred.
packages/analytics-js-plugins/src/deviceModeTransformation/index.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts:10-11
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The misuse of `IHttpClient` in type assertions within the file `packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts` has been corrected by the user.
packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts:10-11
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The misuse of `IHttpClient` in type assertions within the file `packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts` has been corrected by the user.
packages/analytics-js-common/src/types/LoadOptions.ts (3)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-plugins/src/beaconQueue/index.ts:63-63
Timestamp: 2024-11-12T15:14:23.319Z
Learning: The `beaconQueueOptions` property in `state.loadOptions.value` is deprecated for end users but is still used internally for now.
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/types/LoadOptions.ts:0-0
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In the `packages/analytics-js-common/src/types/LoadOptions.ts` file, the `dataplanes` property within the `SourceConfigResponse` type has been removed as it is no longer necessary.
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/types/LoadOptions.ts:175-192
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In this project, deprecated properties may still be used internally to provide backward compatibility for users, as the same types are used for both public-facing APIs and the internal codebase.
packages/analytics-js-common/src/types/HttpClient.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/types/HttpClient.ts:17-21
Timestamp: 2024-11-12T15:14:33.334Z
Learning: In the `ResponseDetails` type, either `response` or `error` must be present, and both can be present simultaneously.
packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-plugins/src/xhrQueue/index.ts:34-34
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The deprecated plugins have been removed from the `PluginName` type, so the explicit `PluginName` type annotation is no longer necessary in `packages/analytics-js-plugins/src/xhrQueue/index.ts`.
🪛 GitHub Check: codecov/patch
packages/analytics-js-common/src/utilities/http.ts
[warning] 3-3: packages/analytics-js-common/src/utilities/http.ts#L3
Added line #L3 was not covered by tests
packages/analytics-js-common/src/services/HttpClientError.ts
[warning] 9-9: packages/analytics-js-common/src/services/HttpClientError.ts#L9
Added line #L9 was not covered by tests
[warning] 14-15: packages/analytics-js-common/src/services/HttpClientError.ts#L14-L15
Added lines #L14 - L15 were not covered by tests
[warning] 18-20: packages/analytics-js-common/src/services/HttpClientError.ts#L18-L20
Added lines #L18 - L20 were not covered by tests
[warning] 24-24: packages/analytics-js-common/src/services/HttpClientError.ts#L24
Added line #L24 was not covered by tests
packages/analytics-js-plugins/src/deviceModeTransformation/index.ts
[warning] 81-82: packages/analytics-js-plugins/src/deviceModeTransformation/index.ts#L81-L82
Added lines #L81 - L82 were not covered by tests
packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts
[warning] 568-568: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L568
Added line #L568 was not covered by tests
[warning] 573-573: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L573
Added line #L573 was not covered by tests
packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts
[warning] 97-97: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L97
Added line #L97 was not covered by tests
[warning] 102-102: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L102
Added line #L102 was not covered by tests
[warning] 106-106: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L106
Added line #L106 was not covered by tests
[warning] 108-108: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L108
Added line #L108 was not covered by tests
[warning] 110-110: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L110
Added line #L110 was not covered by tests
[warning] 113-113: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L113
Added line #L113 was not covered by tests
[warning] 115-116: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L115-L116
Added lines #L115 - L116 were not covered by tests
[warning] 137-137: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L137
Added line #L137 was not covered by tests
[warning] 143-143: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L143
Added line #L143 was not covered by tests
[warning] 146-147: packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts#L146-L147
Added lines #L146 - L147 were not covered by tests
packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts
[warning] 23-23: packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts#L23
Added line #L23 was not covered by tests
🪛 GitHub Check: CodeQL
packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts
[failure] 18-18: Hard-coded credentials
The hard-coded value "dummyWriteKey" is used as authorization header.
[failure] 65-65: Hard-coded credentials
The hard-coded value "rawHeaderValue" is used as authorization header.
⏰ 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 (144)
packages/analytics-js-common/__mocks__/StoreManager.ts (1)
2-3: Use consistent naming and ensure usage across codebase.The addition of these imports for defaultErrorHandler and defaultLogger is clear. However, confirm that consistent naming is maintained in files referencing these mocks, and verify whether a custom or flexible approach is needed for downstream testing scenarios.
packages/analytics-js-common/src/types/HttpClient.ts (5)
3-11: Validate referencing the original error inHttpClientErrorProperties.These optional fields are useful for debugging. Ensure that code handling these properties can handle cases when
originalErroris not set (e.g., logging combined stack traces).
13-16: Confirm union logic consistency forerrorandresponseusage.
IHttpClientErrorextendsErrorwith optional fields. Verify all code correctly treatserrorfields as optional at runtime, especially around logging and fallback flows.
20-24: Reconcile union type with usage in the codebase.
ResponseDetailsuses a union type to enforce eitherresponseorerrormust exist. If there are scenarios where both exist simultaneously, consider clarifying the type definition or usage logic.
27-28: Ensure callback parameters remain consistent.The callback now expects both
dataanddetails. Confirm all call sites were updated accordingly to avoid undefined or mismatched parameters.
[approve]Also applies to: 31-35
61-63: ClarifyIFetchRequestOptionsusage.
timeoutis appended here. Ensure consistency with other abort/timeout logic. Document behavior on timeouts to avoid confusion with different request libraries.packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts (3)
8-8: Confirm alignment with newResponseDetailstype import.Validate that both
errorandresponsedata fields are used consistently with the updated type fromHttpClient.ts.
55-56: Maintain clear function signature changes.Switching from
(result, status)to(response, details)is correct but be sure all internal or external callers are refactored accordingly.
90-91: Double-check fallback status assignment.Merging
details.error?.status ?? details.response?.statusensures capturing both success and error statuses. Confirm it won’t mask partial error data that might reveal deeper issues.packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts (2)
1-1: Confirm import usage ofHttpClientError.This ensures consistent error instantiation across tests. No issues found at first glance.
Also applies to: 4-4
30-35: Excellent test coverage for the newrequestmethod.These lines verify diverse scenarios (e.g., no auth, raw response, 4xx/5xx errors, JSON parse failures, timeouts). The coverage is thorough. Continue to keep them updated if the
HttpClientinterface evolves.Also applies to: 44-47, 51-60, 64-80, 84-101, 104-116, 119-132, 136-156, 160-179, 184-205, 210-235, 239-257, 260-283
packages/analytics-js-common/src/utilities/http.ts (1)
3-5: Add test coverage to increase reliability.Currently, the
isErrRetryablefunction lacks test coverage (per the codecov warning). This function is a key part of retry logic, so ensure it’s thoroughly tested with variousstatusvalues to confirm that it handles retryable and non-retryable errors correctly.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3-3: packages/analytics-js-common/src/utilities/http.ts#L3
Added line #L3 was not covered by testspackages/analytics-js/src/components/capabilitiesManager/detection/index.ts (1)
2-2: Confirm removal of formerly exportedhasBeaconusage.Removing
hasBeaconfrom the exports is consistent with the decommissioning of beacon-related code. Verify that no other locations rely on it and remove any references to avoid undefined behavior.packages/analytics-js/src/components/configManager/types.ts (2)
1-1: Good import practice forSourceConfigResponse.Switching to a single import source clarifies the type origin and ensures consistency across the codebase. No issues noted here.
16-16: Validate null-handling logic inprocessConfig.Allowing
nullfor theresponseparameter broadens the function’s acceptable inputs. Confirm that the implementation robustly handlesnull, and update tests to account for this scenario.packages/analytics-js-common/src/services/HttpClientError.ts (1)
3-7: Good use of typed error fieldsDeclaring explicit optional fields (
status,statusText,responseBody) on the class provides clarity and helps ensure that any code handling these fields can account for their possible absence.packages/analytics-js-common/src/types/PluginsManager.ts (1)
26-26: Removal of deprecated plugin references confirmedEliminating
'BeaconQueue'and'XhrQueue'fromPluginNamealigns with the PR objectives to remove beacon-related functionality and unify event handling. This change is consistent with the ongoing architectural refactor and appears correct.packages/analytics-js/src/components/capabilitiesManager/detection/browser.ts (1)
21-21: Beacon functionality removedWith
hasBeaconno longer exported, this complements the decision to discontinue beacon-based transport. The remaining exports (isBrowser,isNode,hasCrypto,hasUAClientHints, andisIE11) adequately cover the current feature set.packages/analytics-js/src/components/capabilitiesManager/detection/adBlockers.ts (1)
15-27: Verify authentication usage in HEAD requestSwitching to the
HEADmethod withuseAuth: truemight introduce overhead or unexpected behavior if the server enforces authentication on HEAD requests. To ensure this change is fully compatible with your servers, consider verifying that the server accepts HEAD calls with auth credentials, or at least confirm there’s a suitable error or fallback path in case the HEAD request fails.Here’s a script to search for references to this
detectAdBlockersusage and see if fallback logic is present:✅ Verification successful
HEAD request with authentication is correctly implemented
The current implementation is well-designed:
- Uses HEAD method intentionally to minimize data transfer
- Handles both success and error cases appropriately
- Authentication is required as part of the detection mechanism
- Feature is opt-in through the
sendAdblockPageoption🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Attempt to locate any usage of detectAdBlockers or fallback code in the codebase rg -A 5 "detectAdBlockers"Length of output: 9453
packages/analytics-js-plugins/src/shared-chunks/common.ts (2)
11-11: Helpful addition for safer checks
AddingisDefinedAndNotNullprovides a concise way to ensure variables are neither null nor undefined, reducing downstream errors.
39-39: Verify no cyclic dependencies
ExportingRetryQueuehere is convenient, but please confirm it doesn't create any circular references with other imports.✅ Verification successful
No cyclic dependencies found with RetryQueue
The re-export of RetryQueue from analytics-js-common is safe as it maintains a clean one-way dependency from plugins to common utilities without any circular references.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for circular imports referencing `RetryQueue`. ast-grep --pattern $'import { RetryQueue } from $_'Length of output: 52
Script:
#!/bin/bash # Find RetryQueue.ts file fd RetryQueue.ts # Search for any imports from analytics-js-plugins rg -l "from ['|\"]@rudderstack/analytics-js-plugins" -g '*.ts' # Search for RetryQueue imports with different patterns rg -l "RetryQueue" -g '*.ts'Length of output: 681
Script:
#!/bin/bash # Check RetryQueue.ts contents for any analytics-js-plugins imports cat packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.tsLength of output: 24575
packages/analytics-js-common/src/utilities/retryQueue/constants.ts (1)
53-59: Confirm references to old queue statuses
You're now exporting a new set of constants andQueueStatuses. Ensure no part of the codebase references the removedQueueStatusesfrom the old location.✅ Verification successful
No references to removed queue statuses found
All usages ofQueueStatusesin the codebase are consistent with the current set of statuses being exported.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash rg "QueueStatuses" -A 5Length of output: 36478
packages/analytics-js/__tests__/components/capabilitiesManager/detection/adBlockers.test.ts (5)
2-3: Better error handling
Switching from a generic error toHttpClientErrorclarifies the error type and improves debugging.
21-24: Check for callback side effects
Mockingrequestwith a callback covers the asynchronous aspect well. Keep an eye on potential side effects if future tests become more complex.
29-32: Good coverage for blocked request scenario
ThrowingHttpClientErrorensuresisAdBlockedis set. This effectively tests the ad blocker detection logic.
58-60: All scenarios tested
The no-block scenario is well covered. Testing blocked, redirected, and normal flow provides confidence in the adBlockers detection.
44-46: Clarify internal redirection
Markingredirected: trueas an ad-block condition might be correct for your use case. Double-check logic to confirm it’s always treated as blocked.✅ Verification successful
Redirection check is a valid ad blocker detection technique
The implementation correctly treats redirected responses as blocked because ad blockers commonly redirect requests to internal URLs instead of outright blocking them. This behavior is well-documented in the codebase and consistently implemented across components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash rg "redirected" -A 5Length of output: 4866
packages/analytics-js-common/src/types/Store.ts (2)
13-13: Changing validKeys to an array
Switching tostring[]is simpler, but verify no existing code relies on the previous key-value approach for lookups or merges.
35-35: Consistent interface usage
DefiningvalidKeysas an array here matches the config interface. Ensure that any code referencingvalidKeysis updated accordingly.packages/analytics-js/src/components/configManager/util/validate.ts (2)
7-7: Use of explicit type import enhances type safety
Great job importingSourceConfigResponseexplicitly! This approach makes the code more self-documenting and ensures clarity when others read or maintain it.
9-9: Refactor to a type predicate improves downstream type narrowing
ChangingisValidSourceConfigto a type predicate (res is SourceConfigResponse) is highly beneficial. It lets you avoid manual type casts in the call sites and takes advantage of TypeScript’s control flow analysis for improved type safety.packages/analytics-js-common/src/types/Source.ts (3)
5-9: DestinationDefinition looks solid
The interface neatly captures the destination’s core identify fields (name,displayName,updatedAt). Consider makingupdatedAttyped asDateif possible, or at least clarifying the date format in the documentation to avoid confusion.
23-31: Connection type is straightforward
These fields appear consistent across the codebase. Just be sure that the external references (e.g.,destinationId,sourceId) exist before you rely on them.
46-72: SourceConfigResponse type is well-defined
Defining deeply nested fields in a single block improves transparency about the shape of the server’s response. Keep validating the shape of the server data upon fetch to avoid runtime errors if a field goes missing.packages/analytics-js-common/__mocks__/Store.ts (3)
7-7: Nullable import is aligned with partial returns
IncludingNullableensures that you properly handle undefined return flows, which is a best practice in TypeScript.
26-26: Explicit string[] definition
DeclaringvalidKeysasstring[]clarifies the type for future contributors, preventing confusion about the intended data structure.
47-47: Leverage generic return type in get method
Using<T>in thegetmethod is a great way to handle various item types. This design supports more flexible, type-safe usage across different calls.packages/analytics-js/__tests__/components/eventManager/EventManager.test.ts (1)
4-4: Swapping to a mock HttpClient clarifies test dependencies
ImportingdefaultHttpClientfrom the mocks directory is a good step for test isolation. Confirm that this matches the updated request flow in production to avoid inconsistencies between test and production logic.packages/analytics-js-common/src/utilities/retryQueue/types.ts (11)
1-2: Good separation of concerns for type imports.
Congrats on enforcing strict, minimal imports for these queue-related types.
4-4: Appropriate union types for queue item categorization.
Defining'Single' | 'Batch'clarifies usage scenarios.
6-12: Clear structure for queue items.
Provides a distinct shape for each queued item, facilitating better type safety.
14-20: Flexible data type for queue items.
Allowing multiple data shapes ensures the queue can handle diverse payloads.
22-36: Robust callback signature for processing queue items.
All relevant parameters, including retry attempts, are accounted for.
38-38: Straightforward callback for batch size calculation.
Extensible if batch logic evolves.
40-46: Well-defined completion callback.
Properly typed parameters help standardize error handling.
47-65: Comprehensive queue interface.
Covers all essential methods and storage interactions.
67-72: Clear definition of backoff parameters.
Ensures controlling retry intervals is straightforward.
74-79: Timeout settings well-defined.
Makes queue scheduling more transparent.
81-85: Effective structure for tracking in-progress items.
Helps in concurrent or async queue processing.packages/analytics-js/__fixtures__/msw.handlers.ts (2)
13-28: Auth header test handler logic is solid.
Returning a 401 for invalid headers is consistent with typical auth flows.
29-44: Raw auth header test mirrors the previous pattern effectively.
This duplication helps test different header formats.packages/analytics-js/__tests__/components/capabilitiesManager/CapabilitiesManager.test.ts (1)
124-129: Effective migration to the new request method.
Good job integrating the newrequestcall to spot adblockers. Tests remain readable and clear.packages/analytics-js-plugins/src/deviceModeTransformation/index.ts (9)
15-19: Neat import of queue-related types.
Centralizing these types fosters consistency across modules.
23-24: Imports for local transformation types are well-organized.
Keeps references coherent by grouping transformation logic.
51-54: Transparent signature for processing queued item.
Solid approach to pass retry attempts and handle them in the callback.
56-57: Explicit type casting to specialized data.
Helps maintain clarity when dealing with your custom transformation structure.
59-59: Migration tohttpClient.request
Aligns with the new standard approach instead of usinggetAsyncData.
63-68: Properly specifying headers and body in request options.
Ensures consistent JSON content handling in transformation flow.
74-85: Error handling with retry logic is well-handled.
Separating retryable from non-retryable helps maintain resilience.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 81-82: packages/analytics-js-plugins/src/deviceModeTransformation/index.ts#L81-L82
Added lines #L81 - L82 were not covered by tests
80-82: Coverage for lines 81-82 is missing.
Please add test cases to confirm correct behavior when max retry attempts are reached.Do you want help in writing an additional test to ensure coverage here?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 81-82: packages/analytics-js-plugins/src/deviceModeTransformation/index.ts#L81-L82
Added lines #L81 - L82 were not covered by tests
87-95: Conditional event dispatch upon success or final attempt.
Prevents sending partial transformations if a retry is still viable.packages/analytics-js-common/src/types/LoadOptions.ts (2)
8-8: Confirm usage consistency of new import.
The new importSourceConfigResponselooks correct, but please verify all references in the codebase to ensure the removal of any legacy or deprecated imports related to the oldApiObject.
[approve]✅ Verification successful
Import verified as correct and consistent
The new importSourceConfigResponseis properly used for source configuration handling and is not related to or replacingApiObject. Both types serve different purposes and coexist in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify all references to the removed ApiObject or replaced usage rg -A 2 "ApiObject"Length of output: 42131
Script:
#!/bin/bash # Search for SourceConfigResponse references and Source type imports rg "SourceConfigResponse" rg "from './Source'" rg "from '.*Source'" # Find and show content of Source.ts file fd -t f "Source.ts" -x cat {}Length of output: 11104
156-156: Verify alignment of getSourceConfig signature changes.
The updated signature now strictly returnsSourceConfigResponse | Promise<SourceConfigResponse>. Make sure all callers ofgetSourceConfigexpect or awaitSourceConfigResponserather than any older union types.✅ Verification successful
Signature change for getSourceConfig is properly aligned across the codebase
The implementation in ConfigManager.ts and associated tests verify that the stricter return typeSourceConfigResponse | Promise<SourceConfigResponse>is properly handled, with appropriate validation and error handling in place.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Identify function calls or references to getSourceConfig across the repo rg -A 3 "getSourceConfig"Length of output: 79105
Script:
#!/bin/bash # Find SourceConfigResponse type definition rg -A 3 "type SourceConfigResponse" # Find implementation that processes the response from getSourceConfig ast-grep --pattern 'getSourceConfig().then'Length of output: 436
Script:
#!/bin/bash # Find implementation that processes config response rg -A 5 "processConfig|processResponse" # Find all getSourceConfig usages and implementations ast-grep --pattern 'getSourceConfig = $_'Length of output: 79058
packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts (1)
16-19: Confirm the new import paths.
The updated import paths forDoneCallbackandIQueuefrom@rudderstack/analytics-js-common/utilities/retryQueue/typesappear valid. Verify that no stale import references remain in the codebase.packages/analytics-js-plugins/__mocks__/state.ts (1)
154-154: New source name property looks good.
Adding thename: 'dummy-source-name'property is consistent with the new source info usage. No issues found.packages/analytics-js/__tests__/browser.test.ts (3)
44-50: Good switch to fetch-based mocking.
Replacing XHR with a fetch-based mock is a solid approach. Confirm that all test assertions correctly handle both the JSON and text operations for consistency.
82-82: Proper mocking/unmocking of fetch.
Mockingwindow.fetchinbeforeEachand restoring it inafterEachis a clean approach. This ensures isolation of test cases.Also applies to: 85-86, 95-95
111-111: Validate fetch call counts.
The expectations forfetchMockcall counts (3 and 6) appear correct. Just ensure that any future additional requests do not inadvertently break this assertion.Also applies to: 130-130
packages/analytics-js/__tests__/services/StoreManager/Store.test.ts (3)
64-65: Nicely tested scenario for invalid JSON
This test ensures that non-JSON data returnsnull, matching the store's expected behavior.
132-132: ConsistentvalidKeysdefinition
SwitchingvalidKeysto an array is aligned with the updated store interface.
192-193: Good test coverage for QuotaExceeded scenario
Verifying that the store switches to the in-memory engine and successfully sets/retrieves the new key-value is appropriate.packages/analytics-js/src/components/configManager/ConfigManager.ts (11)
7-7: Revised import usage
No issues noted.
14-14: ImportingSourceConfigResponse
This change looks consistent with the revised type usage.
27-27: AddedIConfigManagerimport
No concerns—enhances clarity by referencing the correct interface type.
155-155: Simplified source config validation
UsingisValidSourceConfigdirectly is more readable and robust.
161-161: Explicit check for disabled source
Clean approach to stopping further processing ifsource.enabled === false.
167-167: Refined lifecycle update
updateReportingState(response)is properly placed to reflect the new config changes.
170-172: Ternary for destination filtering
Simple and concise approach to handle empty array vs. filter call.
178-181: Storing source fields in global state
Looks correct and consistent for the source object shape.
190-190: Consents update
updateConsentsState(response)is well-placed, ensuring any new consent-related configuration is captured.
224-224: Replacing legacy fetch withrequest
This aligns with the updated HTTP client API.
227-228: HTTP method and auth usage
UsingGETwithuseAuth: trueis consistent with secured retrieval.packages/analytics-js/__tests__/services/ErrorHandler/ErrorHandler.test.ts (5)
160-160: Skipped error notification
Ensures no HTTP call is made when error reporting is disabled.
168-168: Handling disallowed error messages
Correctly prevents notifications for known messages like"The request failed...".
184-185: Verifying request call
Using.requestinstead of the old.getAsyncDataensures consistent error reporting.
189-192: Request body and headers
Passing JSON viabodyand specifying theContent-Typeheader is correct for the new HTTP client signature.
203-203: Logging internal notification failures
This gracefully recovers from unexpected issues when attempting to send error data.packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts (8)
8-13: Centralizing retry queue types
Adopting the shared types (RetryQueue,QueueItem,QueueItemData, etc.) enhances consistency across the codebase.
16-16: Introduction ofHttpClientError
This clarifies error handling, making it more structured.
136-137: Mocking.request
Adapting the test to the new HTTP client method ensures coverage for success scenarios.
189-189: Proper restoration
CallingmockRestore()is a good practice to avoid side effects in subsequent tests.
193-197: Simulating a successful transformation
Mocking.requestto return a 200 response verifies that the event is processed and removed from the queue.
239-241: Verifying parameter injection
Theresponseobject is properly passed tosendTransformedEventToDestinationsalongside the transformed payload.
248-248: Cleanup after each test block
mockRestore()helps maintain test isolation.
251-257: Ensuring retry on error
The test verifies that a retryable error requeues the event instead of dropping it.packages/analytics-js/__tests__/components/eventRepository/EventRepository.test.ts (1)
Line range hint
344-353: Good test coverage for callback error handling.
This test accurately verifies that exceptions thrown within the callback function are properly caught and logged.packages/analytics-js/src/components/configManager/util/commonUtil.ts (1)
26-26: Properly typed import.
Introducing theSourceConfigResponseimport aligns well with the rest of the file’s usage of strongly typed objects. No issues spotted.packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts (4)
4-7: Consistent type usage.
Refactoring these type imports from@rudderstack/analytics-js-common/types/Sourcecentralizes the source configuration logic and maintains consistency.
167-167: Potential duplication of thenameproperty.
Thename: dummySourceConfigResponse.source.nameline appears twice. Confirm if this is intentional or remove the duplicate if not needed.
229-229: Casting withas unknown as SourceConfigResponse.
Casting toSourceConfigResponseis acceptable for testing partial or mock responses. Ensure that the test input remains valid for all required fields.
243-243: Robust negative test coverage.
Verifying behavior with invalid stringified configuration strengthens error-handling coverage.packages/analytics-js/__fixtures__/fixtures.ts (13)
2-3: Additional type imports.
Bringing inStorageEntriesandSourceConfigResponseensures further type consistency throughout the fixtures.
270-270: Type conformance withsatisfies StorageEntries.
Explicitly declaringentriesWithOnlyCookieStoragehelps catch type mismatches at compile time.
309-309: Consistent validation usingsatisfies.
This pattern ensuresentriesWithOnlyLocalStoragestrictly adheres to theStorageEntriescontract.
348-348: Maintaining correctness.
entriesWithoutCookieStorageusessatisfies StorageEntriesto prevent silent incorrect storage type usage.
387-387: Clear storage type enforcement.
entriesWithoutCookieAndLocalStoragealso benefits from compile-time checks throughsatisfies.
426-426: Ensuring consistent usage.
entriesWithOnlyNoStorageproperly usessatisfies StorageEntriesto validate storage type definitions.
465-465: Accurate type specification.
entriesWithMixStorageexemplifies multiple storage types, verifying correctness before runtime.
504-504: Mixed storage approach.
entriesWithMixStorageButWithoutNonefurther showcases flexible storage assignments with a compile-time safeguard.
543-543: Enhancing clarity.
TheloadOptionWithEntryobject is distinct from theStorageEntriesusage but remains consistent with the surrounding patterns.
612-612: Type safety for fallback storage.
entriesWithInMemoryFallbackhelps ensure each entry remains typed even with the fallback logic.
663-663: Well-defined session-only entries.
entriesWithStorageOnlyForSessionclarifies that only the session info is persisted, checked viasatisfies.
702-702: Strictnonetype usage for anonymousId.
anonymousIdWithNoStorageEntriesintensifies type checking for ephemeral user ID configurations.
741-741: Valid specialized storage scenario.
entriesWithStorageOnlyForAnonymousIddemonstrates partial storage usage, staying consistent with the overall storage model.packages/analytics-js/src/app/RudderAnalytics.ts (3)
191-191: Destructure loadOptions safely.
Good use of optional chaining to avoid errors ifloadOptionsis undefined. Ensure that any subsequent usage also checks for null/undefined to avoid potential runtime errors.
215-215: Confirm initialization order.
CallingsetupPageUnloadTrackingright after configuring page lifecycle events is logical. Just make sure there’s no race condition if the user triggers a navigation event before the rest of the setup completes.
250-250: Method name matches usage.
ThesetupPageUnloadTrackingname clearly conveys its purpose. No immediate concerns, but ensure cross-browser tests confirm consistent behavior.packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts (1)
92-92: Replace references to Beacon with StorageMigrator.
This updated test datum aligns with removing beacon-related logic. It remains consistent withisSDKErrorensuring coverage of the new plugin name.packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts (6)
1-5: Refined import paths.
Imports from relative paths and type modules look correct. Ensure that external references are updated everywhere to avoid import resolution conflicts.
9-18: Updated type definitions in queue logic.
IntroducingQueueTimeouts,QueueBackoff, andInProgressQueueItemhelps clarify callback signatures. This improves type safety in the queue’s processing pipeline.
37-49: Centralize queue status constants.
UsingACK,BATCH_QUEUE,IN_PROGRESS,QUEUEfrom a shared constants file is a clean approach. This consolidation helps maintain consistent naming and usage for queue statuses.
161-163: Initialize queue storage early.
Setting defaults forIN_PROGRESS,QUEUE, andBATCH_QUEUEprevents runtime surprises. Be sure all internal references to these keys expect valid array/object semantics.
478-478: Concurrency in processHead.
Carefully removing items fromQUEUEand updatingIN_PROGRESShelps avoid data duplication. Confirm that large bursts of events don’t introduce timing conflicts.
Also consider a final re-check after processing to handle rapid re-queues.Also applies to: 484-487, 535-536
564-564: Enhance test coverage for ack logic.
Lines 568 and 573 (clearingRECLAIM_START/RECLAIM_END) are not covered by tests per static analysis. Providing direct test scenarios for these code paths will improve reliability.Also applies to: 568-568, 573-573
packages/analytics-js/__tests__/components/core/Analytics.test.ts (3)
80-103: Lifecycle events re-labeled.
Switching to'readyExecuted'ensures a consistent final state. The incremented call counts in the test confirm more granular event triggers. Verify that these transitions match real-world usage (edge cases, multiple calls).
292-292: Check no re-invocation.
EnsuringonDestinationsReadydoes not re-trigger if already in a'ready'state prevents double-calling dependent logic. Looks good.
381-383: Improved error logging in callbacks.
The enhanced “ReadyAPI:: The provided callback parameter is not a function” clarifies the source and context of the error. This aligns with the load API changes above.packages/analytics-js-common/__tests__/utilities/retryQueue/RetryQueue.test.ts (16)
2-3: Imports look consistent.
The newly introduced import ofQueueStatusesand the existing mocks appear logically consistent with the revised constants structure.
8-8: Type import usage is consistent.
TheQueueItemandQueueItemDatatypes are used in the file without issues.
302-302: Retrieving stored queue looks good.
Usingqueue.store.get('queue')is consistent with the code’s logic to inspect the stored queue array.
397-398: Similar duplication of 'ack' usage.
This resembles the pattern noted above. Please see the previous refactor suggestion.Also applies to: 401-401, 403-403
486-487: Repeated 'ack' store usage.
Refer to the earlier suggestion regarding typed store keys.Also applies to: 490-490, 492-492
533-534: Repeated 'ack' store usage.
Refer to the earlier suggestion regarding typed store keys.Also applies to: 537-537, 539-539
587-588: Repeated 'queue' store usage.
Consider strongly typing'queue'if the store interface is known.Also applies to: 591-591, 593-593, 594-594
628-629: Repeated 'ack' and 'inProgress' store usage.
Refer to the previous suggestion on typed store keys.Also applies to: 632-632, 634-634, 635-635
669-670: Repeated typed store key suggestion.
Please see above.Also applies to: 673-673, 675-675, 676-676
710-711: Multiple typed store keys.
Same comment—consider strongly typing or centralizing these keys.Also applies to: 714-714, 716-716, 717-717, 732-732, 747-747
783-784: Store usage pattern repeated.
See prior typed key suggestions.Also applies to: 787-787, 789-789, 790-790, 803-803
835-836: Store usage pattern repeated.
Same reference to typed keys.Also applies to: 839-839, 841-841, 842-842, 849-849, 856-856
890-891: Repeated 'ack' key usage.
See above.Also applies to: 894-894, 896-896, 897-897
930-931: Typification of keys.
Identical to prior suggestions.Also applies to: 934-934, 936-936, 937-937
970-971: Consider typed store keys.
Matches the same usage pattern.Also applies to: 974-974, 976-976, 977-977
Line range hint
1845-1850: Confirm security with credentials: 'include'.
Including credentials may pose CSRF risks if not carefully handled. Ensure the server side enforces the correct CORS or anti-CSRF checks to avoid vulnerabilities.Also applies to: 1854-1854, 1873-1877
✅ Verification successful
Credentials usage is properly secured
The implementation uses credentials: 'include' securely within a first-party context, with proper CORS configuration, authentication headers, and request validation in place. The cross-origin requests are restricted to RudderStack's own analytics services with appropriate security controls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify secure usage of cross-site requests with credentials. # Search for CSRF or CORS logic references in the codebase. rg -A 5 "csrf|cors|Origin|Access-Control-Allow"Length of output: 119563
Script:
#!/bin/bash # Search for fetch/XHR requests with credentials and security related configurations rg -A 5 "credentials:|withCredentials|fetch\(|XMLHttpRequest|xsrfCookie|csrf|headers" --type ts --type jsLength of output: 97815
packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts (1)
1171-1173: Spying on cookie set calls is good.
The approach of spying, validating calls, and restoring is well-structured and makes the test predictable.Also applies to: 1177-1179
| @@ -12,7 +13,7 @@ class Store implements IStore { | |||
| this.id = config.id; | |||
| this.name = config.name; | |||
| this.isEncrypted = config.isEncrypted ?? false; | |||
| this.validKeys = config.validKeys ?? {}; | |||
| this.validKeys = config.validKeys ?? []; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use an array for validKeys
Switching validKeys to an array is simpler and more predictable for iteration. Ensure you update any prior code that might rely on a key-value record structure.
| @@ -146,7 +140,7 @@ class ConfigManager implements IConfigManager { | |||
| * A callback function that is executed once we fetch the source config response. | |||
| * Use to construct and store information that are dependent on the sourceConfig. | |||
| */ | |||
| processConfig(response: SourceConfigResponse | string | undefined, details?: ResponseDetails) { | |||
| processConfig(response: SourceConfigResponse | undefined | null, details?: ResponseDetails) { | |||
There was a problem hiding this comment.
💡 Codebase verification
Breaking change detected in processConfig signature
The removal of string type support affects existing code:
examples/chrome-extension/content-script-v3/foreground.jscontains logic to handle string input that needs to be updated- Test case in
packages/analytics-js/__tests__/components/configManager/ConfigManager.test.tspassing stringified JSON needs revision
🔗 Analysis chain
Validate external usage of the new signature
The method signature now excludes the string type. Ensure no external code passes a string to processConfig.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify no external callers use `processConfig` with a string.
rg -A 3 "processConfig("
Length of output: 105
Script:
#!/bin/bash
# Search for processConfig method calls using ripgrep
echo "=== Ripgrep search ==="
rg -A 3 "processConfig\("
# Search for processConfig method calls using ast-grep
echo -e "\n=== AST-grep search ==="
ast-grep --pattern 'processConfig($_)'
Length of output: 5945
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/analytics-js/src/app/RudderAnalytics.ts (1)
249-269: Consider adding error boundary for visit duration calculation.While the implementation looks good, the visit duration calculation could benefit from additional error handling to ensure robustness.
Consider applying this diff:
setupPageUnloadTracking(events: PageLifecycleEvents[], options: ApiOptions) { if (events.length === 0 || events.includes(PageLifecycleEvents.UNLOADED)) { onPageLeave((isAccessible: boolean) => { if (isAccessible === false && state.lifecycle.loaded.value) { const pageUnloadedTimestamp = Date.now(); - const visitDuration = - pageUnloadedTimestamp - - (state.autoTrack.pageLifecycle.pageLoadedTimestamp.value as number); + const pageLoadedTimestamp = state.autoTrack.pageLifecycle.pageLoadedTimestamp.value; + const visitDuration = typeof pageLoadedTimestamp === 'number' + ? pageUnloadedTimestamp - pageLoadedTimestamp + : 0; this.track( PageLifecycleEvents.UNLOADED, { visitDuration, }, { ...options, originalTimestamp: getFormattedTimestamp(new Date(pageUnloadedTimestamp)), }, ); } }); } }packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts (3)
161-163: Consider adding error handling for storage operations.The
setStorageEntrycalls could fail due to storage quota exceeded or other storage-related errors.private setDefaultQueueEntries() { + try { this.setStorageEntry(IN_PROGRESS, {}); this.setStorageEntry(QUEUE, []); this.setStorageEntry(BATCH_QUEUE, []); + } catch (err) { + this.logger?.error('Failed to set default queue entries', err); + } }
Line range hint
333-366: Consider adding batch size validation.The batch queue manipulation logic should validate the maximum size to prevent memory issues.
handleNewItemForBatch(entry: QueueItem<QueueItemData>) { let curEntry: QueueItem<QueueItemData> | undefined; let batchQueue = (this.getStorageEntry(BATCH_QUEUE) as Nullable<QueueItem[]>) ?? []; + + // Prevent excessive memory usage + const MAX_BATCH_QUEUE_LENGTH = 1000; + if (batchQueue.length >= MAX_BATCH_QUEUE_LENGTH) { + this.logger?.warn(`Batch queue exceeded ${MAX_BATCH_QUEUE_LENGTH} items, oldest items will be dropped`); + batchQueue = batchQueue.slice(-MAX_BATCH_QUEUE_LENGTH); + }
Line range hint
477-535: Consider adding timeout handling for processQueueCb.There's a TODO comment about handling
processQueueCbtimeout that should be addressed to prevent queue processing from hanging.Would you like me to propose an implementation for the timeout handling in
processQueueCb?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts(20 hunks)packages/analytics-js/src/app/RudderAnalytics.ts(3 hunks)packages/analytics-js/src/state/slices/dataPlaneEvents.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/analytics-js/src/state/slices/dataPlaneEvents.ts
🧰 Additional context used
📓 Learnings (1)
packages/analytics-js/src/app/RudderAnalytics.ts (2)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/types/LoadOptions.ts:175-192
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In this project, deprecated properties may still be used internally to provide backward compatibility for users, as the same types are used for both public-facing APIs and the internal codebase.
Learnt from: MoumitaM
PR: rudderlabs/rudder-sdk-js#1876
File: packages/analytics-js/src/app/RudderAnalytics.ts:0-0
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The `trackPageLifecycleEvents` method in `packages/analytics-js/src/app/RudderAnalytics.ts` does not require refactoring as the current implementation is preferred.
🪛 GitHub Check: codecov/patch
packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts
[warning] 567-567: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L567
Added line #L567 was not covered by tests
[warning] 572-572: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L572
Added line #L572 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Bundle size checks
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/analytics-js/src/app/RudderAnalytics.ts (1)
Line range hint
190-214: LGTM! Clean refactoring of page lifecycle tracking.The changes simplify the method by removing the beacon-related logic while maintaining the core functionality of page lifecycle tracking.
packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts (3)
Line range hint
1-44: LGTM! Import paths and constant usage look good.The changes to use relative imports and string constants for queue statuses improve maintainability and reduce coupling.
Line range hint
258-267: LGTM! Batch queue processing logic is correct.The implementation properly handles batch queue processing and cleanup.
Line range hint
416-489: Verify error handling in requeue method.The removal of the error parameter from
requeuemethod might affect error tracking and debugging capabilities.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/analytics-js-common/src/types/LoadOptions.ts(3 hunks)packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts(4 hunks)packages/analytics-js/__tests__/components/configManager/commonUtil.test.ts(0 hunks)packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts(1 hunks)packages/analytics-js/__tests__/services/StoreManager/StoreManager.test.ts(1 hunks)packages/analytics-js/public/index.html(0 hunks)packages/analytics-js/rollup.config.mjs(1 hunks)packages/analytics-js/src/components/configManager/ConfigManager.ts(6 hunks)packages/analytics-js/src/components/configManager/util/commonUtil.ts(1 hunks)packages/analytics-js/src/state/slices/loadOptions.ts(0 hunks)packages/sanity-suite/public/v1.1/index-local.html(1 hunks)
💤 Files with no reviewable changes (3)
- packages/analytics-js/tests/components/configManager/commonUtil.test.ts
- packages/analytics-js/src/state/slices/loadOptions.ts
- packages/analytics-js/public/index.html
✅ Files skipped from review due to trivial changes (1)
- packages/sanity-suite/public/v1.1/index-local.html
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/analytics-js/tests/services/StoreManager/StoreManager.test.ts
- packages/analytics-js/rollup.config.mjs
- packages/analytics-js/tests/services/ErrorHandler/utils.test.ts
- packages/analytics-js-common/src/types/LoadOptions.ts
- packages/analytics-js/src/components/configManager/ConfigManager.ts
🧰 Additional context used
📓 Learnings (1)
packages/analytics-js/src/components/configManager/util/commonUtil.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/types/LoadOptions.ts:0-0
Timestamp: 2024-11-12T15:14:23.319Z
Learning: In the `packages/analytics-js-common/src/types/LoadOptions.ts` file, the `dataplanes` property within the `SourceConfigResponse` type has been removed as it is no longer necessary.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Security and code quality checks
- GitHub Check: Bundle size checks
🔇 Additional comments (7)
packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts (4)
5-8: LGTM! Type imports are now centralized.The relocation of type imports to '@rudderstack/analytics-js-common/types/Source' improves type organization and maintainability.
230-231: LGTM! Proper type assertion for testing.The type assertion pattern (
as unknown as SourceConfigResponse) is correctly used to test error handling with invalid source configurations.
244-246: LGTM! Good error handling test coverage.The test case properly validates error handling for malformed input using appropriate TypeScript annotations.
Line range hint
1-386: Verify test coverage for data plane events processing.Since this PR moves data plane events processing to core, let's verify that all relevant test cases are covered.
Run the following script to check for any missing test coverage:
✅ Verification successful
Data plane events processing test coverage is comprehensive ✅
The codebase has thorough test coverage for data plane functionality across all relevant components, including configuration management, event repository, error handling, and service worker integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for data plane events processing # Search for data plane related tests echo "Searching for existing data plane event tests..." rg -l "dataPlane|DataPlane" --type ts --glob "**/*.test.ts" # Search for potentially missing test cases echo "Checking for potential data plane event handling without tests..." ast-grep --pattern 'function $FNAME($$$) { $$$ dataPlane $$$ }'Length of output: 940
packages/analytics-js/src/components/configManager/util/commonUtil.ts (3)
26-26: LGTM! Import changes align with architectural changes.The addition of
SourceConfigResponsetype import aligns with the PR objectives of moving data plane events processing to core.
Line range hint
449-484: LGTM! Clean implementation of consent state management.The
updateConsentsStatefunction:
- Properly handles the updated
SourceConfigResponsetype- Maintains good practices with batch updates for state changes
- Has proper validation of inputs
Line range hint
1-1: Verify the removal of data plane events state management.The removal of
updateDataPlaneEventsStateFromLoadOptionsfunction aligns with moving data plane events processing to core. However, we should verify that all references to this function have been properly handled.✅ Verification successful
Function removal is safe and properly handled ✅
The removal of
updateDataPlaneEventsStateFromLoadOptionsis verified to be safe. Data plane events state management has been properly moved to core and is now handled through the centralized state management system inanalytics-js/src/state/slices/dataPlaneEvents.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the removed function rg "updateDataPlaneEventsStateFromLoadOptions" --type tsLength of output: 56
Script:
#!/bin/bash # Check git history for the function and related patterns git log -G"updateDataPlaneEventsStateFromLoadOptions" --oneline # Search for similar data plane events state patterns rg "dataPlaneEvents.*State" --type ts # Look for remaining data plane configuration code rg "dataPlane.*Config" --type tsLength of output: 792
| @@ -165,6 +165,7 @@ describe('ConfigManager', () => { | |||
| config: dummySourceConfigResponse.source.config, | |||
| name: dummySourceConfigResponse.source.name, | |||
| workspaceId: dummySourceConfigResponse.source.workspaceId, | |||
| name: dummySourceConfigResponse.source.name, | |||
There was a problem hiding this comment.
Remove duplicate 'name' property.
The name property is duplicated in the expectedSourceState object, which could lead to confusion or potential issues in the test.
Apply this diff to remove the duplicate:
- name: dummySourceConfigResponse.source.name,Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (18)
packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts (2)
Line range hint
136-190: Consider enhancing the mock implementation with response details.While the mock implementation works, it could be more explicit about the response structure to match the actual HTTP client behavior.
Consider updating the mock implementation to include response details:
- defaultHttpClient.request.mockImplementation(({ callback }) => { - callback(true); + defaultHttpClient.request.mockImplementation(({ callback }) => { + callback(true, { + response: { status: 200 } as Response, + } as ResponseDetails); });
252-258: Consider adding more error scenarios.While the current error case tests the retry mechanism with a 502 status, consider adding test cases for:
- Non-retryable errors (4xx)
- Network timeouts
- Malformed responses
Example additional test case:
it('should handle non-retryable errors appropriately', () => { defaultHttpClient.request.mockImplementation(({ callback }) => { callback?.(null, { error: new HttpClientError('client error', { status: 400, }), } as ResponseDetails); }); // ... test implementation });packages/analytics-js/src/components/dataPlaneEventsQueue/DataPlaneEventsQueue.ts (2)
28-33: Consider concurrency constraints when creating multiple queue instances.While appending the write key to the queue name helps distinguish separate instances, ensure that simultaneously running multiple instances won’t lead to conflicts or shared state issues.
106-130: Consider avoiding deep clone of large events for performance.Cloning can be expensive for large payloads or high traffic. If immutability isn’t critical at this stage, consider a shallow copy or direct usage to minimize overhead.
packages/analytics-js/src/components/dataPlaneEventsQueue/utilities.ts (3)
1-23: Evaluate the overhead of multiple external utility libraries.This file imports
mergeDeepRightandclonefrom separate libraries. If performance or bundle size is a concern, consider consolidated or built-in utilities to reduce the dependency footprint.
24-39: Clarify fallback logic whenuseBeaconis set.Currently,
beaconQueueOptionsautomatically overridesqueueOptions. This might surprise users if both are set. A short comment or additional documentation clarifying which option takes precedence can avoid confusion.
102-153: Enhance error logs with IDs or correlation keys.
logErrorOnFailurelogs failures but doesn’t provide correlation info for debugging multi-event or multi-request scenarios. Adding a unique batch ID or event ID can greatly simplify error tracing in production.packages/analytics-js/src/components/dataPlaneEventsQueue/constants.ts (1)
7-10: Consider adding jitter to retry attempts.The retry queue configuration looks good, but adding a jitter factor could help prevent thundering herd problems when multiple clients retry simultaneously.
const DEFAULT_RETRY_QUEUE_OPTIONS: QueueOpts = { maxAttempts: 10, maxItems: 100, + jitter: true, };packages/analytics-js/src/components/dataPlaneEventsQueue/types.ts (1)
19-26: Consider adding queue status and metrics methods.The interface looks good but could benefit from additional methods for observability:
export interface IDataPlaneEventsQueue { logger: ILogger; enqueue(event: RudderEvent): void; start(): void; stop(): void; clear(): void; isRunning(): boolean; + getQueueSize(): number; + getQueueStatus(): string; + getMetrics(): { processed: number; failed: number; retried: number }; }packages/analytics-js/src/components/dataPlaneEventsQueue/logMessages.ts (1)
6-11: Consider adding size reduction suggestions in the warning message.The warning message is informative but could be more helpful by including common ways to reduce payload size.
const EVENT_PAYLOAD_SIZE_CHECK_FAIL_WARNING = ( context: string, payloadSize: number, sizeLimit: number, ): string => - `${context}${LOG_CONTEXT_SEPARATOR}The size of the event payload (${payloadSize} bytes) exceeds the maximum limit of ${sizeLimit} bytes. Events with large payloads may be dropped in the future. Please review your instrumentation to ensure that event payloads are within the size limit.`; + `${context}${LOG_CONTEXT_SEPARATOR}The size of the event payload (${payloadSize} bytes) exceeds the maximum limit of ${sizeLimit} bytes. Events with large payloads may be dropped in the future. Consider: 1) Removing unnecessary properties 2) Truncating long values 3) Splitting large events into smaller ones.`;packages/analytics-js/__tests__/components/dataPlaneEventsQueue/dataPlaneEventsQueue.test.ts (2)
Line range hint
277-278: Consider removing redundant stop call.The queue is already stopped in the
beforeEachblock (line 30). This additional stop call appears redundant.🧰 Tools
🪛 Biome (1.9.4)
[error] 112-112: The imported variable EVENT_PAYLOAD_SIZE_BYTES_LIMIT is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 121-121: The imported variable EVENT_PAYLOAD_SIZE_BYTES_LIMIT is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
Line range hint
287-291: Enhance test assertion for event properties.The test only verifies that the event was enqueued with basic properties. Consider adding assertions for all relevant event properties to ensure complete validation.
expect(dpEventsQueueEnqueueSpy).toHaveBeenNthCalledWith(1, { ...testEvent, integrations: { All: true }, + type: 'track', + event: 'test-event', + userId: 'test-user', + properties: { + test: 'test', + }, });🧰 Tools
🪛 Biome (1.9.4)
[error] 112-112: The imported variable EVENT_PAYLOAD_SIZE_BYTES_LIMIT is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 121-121: The imported variable EVENT_PAYLOAD_SIZE_BYTES_LIMIT is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
packages/analytics-js/__tests__/components/eventRepository/EventRepository.test.ts (1)
431-437: Consider using a test helper for pre-consent configuration.This pre-consent configuration appears multiple times in the test file. Consider extracting it to a test helper function to improve maintainability.
const setPreConsentConfig = (delivery: string = 'buffer', strategy: string = 'session') => { state.consents.preConsent.value = { enabled: true, events: { delivery }, storage: { strategy }, }; };packages/analytics-js/__tests__/components/dataPlaneEventsQueue/utilities.test.ts (2)
367-368: Use a constant for the test payload size limit.Instead of directly assigning a magic number, consider using a named constant for better readability and maintainability.
- EVENT_PAYLOAD_SIZE_BYTES_LIMIT = 32; + const TEST_PAYLOAD_SIZE_LIMIT = 32; + EVENT_PAYLOAD_SIZE_BYTES_LIMIT = TEST_PAYLOAD_SIZE_LIMIT;🧰 Tools
🪛 Biome (1.9.4)
[error] 367-367: The imported variable EVENT_PAYLOAD_SIZE_BYTES_LIMIT is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
347-349: Consider using beforeEach for consistent test state.The
afterEachblock resets the payload size limit. Consider also setting it in abeforeEachblock to ensure a consistent starting state for each test.🧰 Tools
🪛 Biome (1.9.4)
[error] 348-348: The imported variable EVENT_PAYLOAD_SIZE_BYTES_LIMIT is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
packages/analytics-js-common/src/utilities/retryQueue/constants.ts (1)
23-23: Consider using binary notation for better readability.For byte sizes, consider using binary notation to make the size more explicit.
-const MAX_PAGE_UNLOAD_BATCH_SIZE_BYTES = 64 * 1024; // 64 KB +const MAX_PAGE_UNLOAD_BATCH_SIZE_BYTES = 64 << 10; // 64 KBpackages/analytics-js-common/src/utilities/retryQueue/types.ts (1)
14-21: Consider tightening the type definition for better type safety.The
QueueItemDatatype usesRecord<string, any>which is very permissive and could lead to type safety issues. Consider using a more specific type or interface that better represents the expected data structure.packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts (1)
284-320: Consider splitting complex methods for better maintainability.The
flushForPageLeaveandhandleNewItemForBatchmethods are quite complex with multiple responsibilities. Consider extracting some logic into helper methods, such as:
calculateBatchSizeprepareBatchItemsupdateBatchQueueThis would improve readability and testability.
Would you like me to propose a specific refactoring approach?
Also applies to: 377-410
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 287-287: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L287
Added line #L287 was not covered by tests
[warning] 289-289: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L289
Added line #L289 was not covered by tests
[warning] 294-294: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L294
Added line #L294 was not covered by tests
[warning] 297-297: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L297
Added line #L297 was not covered by tests
[warning] 300-300: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L300
Added line #L300 was not covered by tests
[warning] 303-303: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L303
Added line #L303 was not covered by tests
[warning] 306-306: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L306
Added line #L306 was not covered by tests
[warning] 309-309: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L309
Added line #L309 was not covered by tests
[warning] 312-313: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L312-L313
Added lines #L312 - L313 were not covered by tests
[warning] 317-317: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L317
Added line #L317 was not covered by tests
[warning] 319-319: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L319
Added line #L319 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/analytics-js-common/__tests__/utilities/retryQueue/RetryQueue.test.ts(41 hunks)packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts(21 hunks)packages/analytics-js-common/src/utilities/retryQueue/constants.ts(2 hunks)packages/analytics-js-common/src/utilities/retryQueue/types.ts(1 hunks)packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts(4 hunks)packages/analytics-js/.size-limit.mjs(1 hunks)packages/analytics-js/__tests__/components/core/Analytics.test.ts(7 hunks)packages/analytics-js/__tests__/components/dataPlaneEventsQueue/dataPlaneEventsQueue.test.ts(1 hunks)packages/analytics-js/__tests__/components/dataPlaneEventsQueue/utilities.test.ts(1 hunks)packages/analytics-js/__tests__/components/eventRepository/EventRepository.test.ts(14 hunks)packages/analytics-js/__tests__/components/utilities/loadOptions.test.ts(0 hunks)packages/analytics-js/src/components/dataPlaneEventsQueue/DataPlaneEventsQueue.ts(1 hunks)packages/analytics-js/src/components/dataPlaneEventsQueue/constants.ts(1 hunks)packages/analytics-js/src/components/dataPlaneEventsQueue/logMessages.ts(1 hunks)packages/analytics-js/src/components/dataPlaneEventsQueue/types.ts(1 hunks)packages/analytics-js/src/components/dataPlaneEventsQueue/utilities.ts(1 hunks)packages/analytics-js/src/components/eventRepository/EventRepository.ts(6 hunks)packages/analytics-js/src/components/eventRepository/constants.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/analytics-js/tests/components/utilities/loadOptions.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js/tests/components/core/Analytics.test.ts
- packages/analytics-js-common/tests/utilities/retryQueue/RetryQueue.test.ts
🧰 Additional context used
📓 Learnings (1)
packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts:10-11
Timestamp: 2024-11-12T15:14:33.334Z
Learning: The misuse of `IHttpClient` in type assertions within the file `packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts` has been corrected by the user.
🪛 Biome (1.9.4)
packages/analytics-js/__tests__/components/dataPlaneEventsQueue/utilities.test.ts
[error] 348-348: The imported variable EVENT_PAYLOAD_SIZE_BYTES_LIMIT is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 367-367: The imported variable EVENT_PAYLOAD_SIZE_BYTES_LIMIT is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
packages/analytics-js/__tests__/components/dataPlaneEventsQueue/dataPlaneEventsQueue.test.ts
[error] 112-112: The imported variable EVENT_PAYLOAD_SIZE_BYTES_LIMIT is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
[error] 121-121: The imported variable EVENT_PAYLOAD_SIZE_BYTES_LIMIT is read-only
The variable is imported here
Use a local variable instead of reassigning an import.
(lint/suspicious/noImportAssign)
🪛 GitHub Check: codecov/patch
packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts
[warning] 192-193: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L192-L193
Added lines #L192 - L193 were not covered by tests
[warning] 287-287: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L287
Added line #L287 was not covered by tests
[warning] 289-289: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L289
Added line #L289 was not covered by tests
[warning] 294-294: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L294
Added line #L294 was not covered by tests
[warning] 297-297: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L297
Added line #L297 was not covered by tests
[warning] 300-300: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L300
Added line #L300 was not covered by tests
[warning] 303-303: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L303
Added line #L303 was not covered by tests
[warning] 306-306: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L306
Added line #L306 was not covered by tests
[warning] 309-309: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L309
Added line #L309 was not covered by tests
[warning] 312-313: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L312-L313
Added lines #L312 - L313 were not covered by tests
[warning] 317-317: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L317
Added line #L317 was not covered by tests
[warning] 319-319: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L319
Added line #L319 was not covered by tests
[warning] 610-610: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L610
Added line #L610 was not covered by tests
[warning] 615-615: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L615
Added line #L615 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Bundle size checks
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (23)
packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts (2)
8-16: LGTM! Well-structured type imports.The migration of type imports to
@rudderstack/analytics-js-commonaligns with the PR objective of centralizing functionality in the core SDK. The imports are properly organized and maintain clear type safety.
Line range hint
194-249: LGTM! Comprehensive success case testing.The test case properly validates the transformation success path with appropriate response structure and cleanup.
packages/analytics-js/src/components/dataPlaneEventsQueue/DataPlaneEventsQueue.ts (2)
34-52: Verify default queue config values to prevent misconfiguration.
getNormalizedQueueOptions(state.loadOptions.value)merges user-defined and default queue settings. Make sure to validate these settings (e.g., timeouts, flush intervals) to reduce the risk of unintended behavior if a user or environment variable provides invalid values.
132-151: Convenient control of the queue.The
start(),stop(),clear(), andisRunning()methods provide a clean interface for queue management. This clear separation of concerns is a good design choice that promotes maintainability.packages/analytics-js/src/components/dataPlaneEventsQueue/utilities.ts (2)
41-62: Handle potential invalid or malformed URLs.
new URL(dataplaneUrl)may throw ifdataplaneUrlis malformed. Consider adding error handling or fallback logic to avoid runtime exceptions in less controlled environments.
64-100: Check behavior whenitemDatais an empty array.
getRequestInfotreatsitemDataas an array for batch requests. IfitemDatais empty, the logic attempts to clone the headers fromitemData[0]and uses first item references. Ensure this edge case is handled or is guaranteed never to occur.packages/analytics-js/src/components/eventRepository/constants.ts (1)
4-4: Confirm references to the removedDATA_PLANE_QUEUE_EXT_POINT_PREFIX.Ensure that the removal of
DATA_PLANE_QUEUE_EXT_POINT_PREFIXis intentional and that no other files rely on it. Double-check upstream references or dynamic usage.packages/analytics-js/src/components/dataPlaneEventsQueue/constants.ts (1)
12-12: Verify timeout and payload size limits across browsers.The timeout (10s) and payload size (32KB) limits look reasonable, but we should verify they work consistently across different browsers, especially IE11 which was mentioned in the PR objectives.
Also applies to: 16-16
✅ Verification successful
Timeout and payload size limits are verified and safe across browsers
The codebase implements consistent and safe limits:
- 10-second timeout is well within browser limits
- 32KB payload size limit is conservatively set
- Both limits are enforced uniformly across browsers including IE11
- Proper fallback mechanisms are in place for legacy browsers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any browser-specific handling of timeouts or payload sizes rg -l "payload|timeout|size.*limit" | xargs rg -l "browser|chrome|firefox|ie|edge"Length of output: 10340
Script:
#!/bin/bash # Search for network request implementations and browser detection echo "=== Network Request Implementations ===" ast-grep --pattern 'fetch($$$)' ast-grep --pattern 'new XMLHttpRequest($$$)' echo -e "\n=== Timeout and Payload Size Handling ===" rg -A 5 "REQUEST_TIMEOUT|PAYLOAD.*SIZE" echo -e "\n=== Browser Detection and IE11 Handling ===" rg -A 5 "(?i)(browser|ie11|internet.*explorer).*detection"Length of output: 44561
packages/analytics-js/src/components/dataPlaneEventsQueue/types.ts (1)
14-17: Add validation for sentAt timestamp format.The BatchPayload type should ensure the sentAt timestamp follows a consistent format.
packages/analytics-js/src/components/eventRepository/EventRepository.ts (2)
53-57: LGTM! Good improvement in initialization.Moving the DataPlaneEventsQueue initialization to the constructor improves code organization and makes the dependencies clearer.
109-114: Verify race condition handling in queue start logic.The queue start logic involves multiple conditions and a timeout. Ensure there are no race conditions between the effect callback and the timeout.
✅ Verification successful
Race conditions are properly handled in the queue start logic
The implementation has proper safeguards against race conditions:
- The
scheduleTimeoutActiveflag prevents multiple concurrent queue startsclearTimeoutis called before starting the queue to prevent timeout callback racing- Both the effect callback and timeout callback check
scheduleTimeoutActivebefore starting the queue🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related race condition tests or similar patterns rg -A 5 "clearTimeout.*start"Length of output: 70354
Script:
#!/bin/bash # Search for queue tests and implementation rg -A 10 "class.*Queue|dataplaneEventsQueue" # Search for queue start/stop patterns rg -A 5 "queue.*start|start.*queue" # Search for scheduleTimeoutActive usage rg -A 5 "scheduleTimeoutActive"Length of output: 107859
packages/analytics-js/__tests__/components/dataPlaneEventsQueue/dataPlaneEventsQueue.test.ts (1)
1-11: LGTM! The test setup and imports look good.The test file is well-organized with proper imports and mocks.
packages/analytics-js/__tests__/components/eventRepository/EventRepository.test.ts (1)
84-85: LGTM! Good test setup with proper state management.The test properly sets up the initial state and includes cleanup in
afterEach.Also applies to: 87-88
packages/analytics-js-common/src/utilities/retryQueue/constants.ts (1)
33-35: LGTM! Well-documented array organization.The comment explaining the intentional ordering of the ACK entry in the QueueStatuses array is helpful for maintaining the code.
packages/analytics-js-common/src/utilities/retryQueue/types.ts (3)
4-12: Well-structured type definitions!The
QueueItemTypeandQueueItemtypes are well-defined with clear semantics and good type safety. The generic type parameter with a default type is a good practice.
24-49: Excellent callback type definitions and documentation!The callback types are well-defined with comprehensive JSDoc comments explaining each parameter. The error handling in
DoneCallbackis properly typed.
50-88: Well-designed interface and supporting types!The
IQueueinterface provides a comprehensive contract for queue operations, and the supporting types (QueueBackoff,QueueTimeouts,InProgressQueueItem) are well-structured.packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts (4)
284-320: Review edge cases in batch flushing logic.The batch flushing logic handles size limits well, but consider these edge cases:
- What happens if
batchSizeCalcCbthrows an error?- Should we log when items are dropped due to size constraints?
Also, this critical functionality lacks test coverage.
Would you like me to help generate test cases covering these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 287-287: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L287
Added line #L287 was not covered by tests
[warning] 289-289: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L289
Added line #L289 was not covered by tests
[warning] 294-294: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L294
Added line #L294 was not covered by tests
[warning] 297-297: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L297
Added line #L297 was not covered by tests
[warning] 300-300: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L300
Added line #L300 was not covered by tests
[warning] 303-303: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L303
Added line #L303 was not covered by tests
[warning] 306-306: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L306
Added line #L306 was not covered by tests
[warning] 309-309: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L309
Added line #L309 was not covered by tests
[warning] 312-313: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L312-L313
Added lines #L312 - L313 were not covered by tests
[warning] 317-317: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L317
Added line #L317 was not covered by tests
[warning] 319-319: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L319
Added line #L319 was not covered by tests
Line range hint
460-474: Good simplification of requeue logic!The removal of the unused error parameter and simplified logic improves code maintainability while preserving the core functionality.
577-584: Good addition of page accessibility state!The
isPageAccessibleparameter allows queue processors to adapt their behavior based on page state, which is important for handling page unload scenarios.
190-194: Add test coverage for page leave handling.The page leave handling logic is not covered by tests. This is critical functionality that ensures data is not lost when the page is unloaded.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 192-193: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L192-L193
Added lines #L192 - L193 were not covered by testspackages/analytics-js/.size-limit.mjs (2)
33-33:⚠️ Potential issueCODEOWNERS approval required for size limit changes.
The file header explicitly states: "DO NOT EDIT the size limits configured in this file. Should you update them, please contact CODEOWNERS." Please confirm if CODEOWNERS have approved these changes.
Also applies to: 39-39, 45-45, 50-50
33-33: Justify the ~3 KiB size increase across modern variants.While moving data plane events to core explains the size increase, the removal of BeaconQueue and XhrQueue plugins should offset some of this. Please provide a breakdown of:
- Size added by moving data plane events to core
- Size reduced by removing plugins
- Net impact on bundle size
Run this script to analyze the size changes:
Also applies to: 39-39, 45-45, 50-50
✅ Verification successful
Size increase is justified by architectural improvements
The ~3 KiB increase is a reasonable trade-off for:
- Moving data plane events dispatch to core, improving maintainability
- Removing BeaconQueue and XhrQueue plugins, simplifying the architecture
- Enhanced type safety and streamlined configuration management
- New HTTP client implementation with better error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare file sizes between base and PR branch # Get the base branch name base_branch=$(gh pr view 1999 --json baseRefName -q .baseRefName) # Checkout base branch and build git checkout $base_branch npm run build # Store base sizes echo "Base branch sizes:" ls -lh dist/npm/modern/*/index.* dist/cdn/modern/iife/rsa.min.js # Checkout PR branch and build git checkout - npm run build # Compare with PR sizes echo "PR branch sizes:" ls -lh dist/npm/modern/*/index.* dist/cdn/modern/iife/rsa.min.jsLength of output: 986
Script:
#!/bin/bash # Search for size-related files and changes # Get the base branch name base_branch=$(gh pr view 1999 --json baseRefName -q .baseRefName) # Compare size limit configurations echo "=== Size limit changes ===" git diff $base_branch...HEAD packages/analytics-js/.size-limit.mjs # Look for removed plugin files echo -e "\n=== Removed plugin files ===" git diff --name-status $base_branch...HEAD | grep -E "packages/analytics-js/.*/(BeaconQueue|XhrQueue)" # Get PR description for context echo -e "\n=== PR description ===" gh pr view 1999 --json body -q .bodyLength of output: 3355
| handleRetryQueueItem( | ||
| itemData: QueueItemData, | ||
| done: DoneCallback, | ||
| attemptNumber?: number, | ||
| maxRetryAttempts?: number, | ||
| willBeRetried?: boolean, | ||
| isPageAccessible?: boolean, | ||
| ) { | ||
| const { data, url, headers } = getRequestInfo(itemData as EventsQueueItemData, state); | ||
|
|
||
| const keepalive = isPageAccessible === false; | ||
|
|
||
| this.httpClient.request({ | ||
| url, | ||
| options: { | ||
| method: 'POST', | ||
| headers, | ||
| body: data as string, | ||
| useAuth: true, | ||
| keepalive, | ||
| }, | ||
| isRawResponse: true, | ||
| timeout: REQUEST_TIMEOUT_MS, | ||
| callback: (result, details) => { | ||
| // null means item will not be requeued | ||
| let queueErrResp = null; | ||
| if (details.error) { | ||
| const isRetryableFailure = isErrRetryable(details); | ||
| if (isRetryableFailure) { | ||
| queueErrResp = details; | ||
| } | ||
|
|
||
| logErrorOnFailure( | ||
| isRetryableFailure, | ||
| details.error.message, | ||
| willBeRetried, | ||
| attemptNumber, | ||
| maxRetryAttempts, | ||
| this.logger, | ||
| ); | ||
| } | ||
| done(queueErrResp, result); | ||
| }, | ||
| }); | ||
|
|
||
| // For requests that happen on page leave, we cannot wait for fetch API to complete | ||
| // So, we assume that the request is successful and call done immediately | ||
| if (keepalive) { | ||
| done(null); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid calling done() immediately for unload requests to prevent silent data loss.
When isPageAccessible === false, the code sets keepalive and instantly calls done(null), marking the queue item successful. If the request fails after page leave, this event is lost. Consider deferring done() or recording the result asynchronously for better reliability.
Proposed fix (conceptual):
- if (keepalive) {
- done(null);
- }
+ if (keepalive) {
+ // Optionally set a timeout or a small buffer here if possible
+ // or rely on the request callback if the environment allows
+ }Committable suggestion skipped: line range outside the PR's diff.
| const finalEvent = getFinalEvent(event, state); | ||
| this.dataplaneEventsQueue.enqueue(finalEvent); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding event validation before enqueuing.
The enqueue method should validate the event structure before processing.
enqueue(event: RudderEvent, callback?: ApiCallback): void {
const finalEvent = getFinalEvent(event, state);
+ if (!this.validateEvent(finalEvent)) {
+ this.logger.error(`Invalid event structure: ${JSON.stringify(finalEvent)}`);
+ return;
+ }
this.dataplaneEventsQueue.enqueue(finalEvent);Also applies to: 157-157
| this.setStorageEntry(ACK, this.schedule.now()); | ||
|
|
||
| if (this.reclaimStartVal != null) { | ||
| this.reclaimStartVal = null; | ||
| this.setStorageEntry(QueueStatuses.RECLAIM_START, null); | ||
| this.setStorageEntry(RECLAIM_START, null); | ||
| } | ||
|
|
||
| if (this.reclaimEndVal != null) { | ||
| this.reclaimEndVal = null; | ||
| this.setStorageEntry(QueueStatuses.RECLAIM_END, null); | ||
| this.setStorageEntry(RECLAIM_END, null); |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for storage entry clearing.
The storage entry clearing operations in ack() method lack test coverage. These operations are critical for proper queue management across browser tabs. Specifically, add test cases for:
- Setting the ACK storage entry
- Clearing RECLAIM_START when reclaimStartVal is present
- Clearing RECLAIM_END when reclaimEndVal is present
🔗 Analysis chain
Add test coverage for storage entry clearing.
The storage entry clearing operations in ack() method lack test coverage. These operations are critical for proper queue management across browser tabs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing tests related to storage clearing
rg -l "reclaimStartVal|reclaimEndVal" --type ts --type test
Length of output: 90
Script:
#!/bin/bash
# Find test files related to RetryQueue and storage operations
rg -t ts "RetryQueue.*\.test|RetryQueue.*\.spec" --files-with-matches
# Search for test cases related to storage operations
rg -t ts "describe.*storage|test.*storage|it.*storage" -C 2 packages/analytics-js-common/src
Length of output: 166
Script:
#!/bin/bash
# Find all test files in the package
fd -e test.ts -e spec.ts . packages/analytics-js-common/
# Search for RetryQueue tests and storage operations
rg "RetryQueue" -A 5 -B 5 packages/analytics-js-common/src/**/*.test.ts packages/analytics-js-common/src/**/*.spec.ts
# Search in the implementation file for storage method signatures
rg "setStorageEntry|getStorageEntry" packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts
Length of output: 3339
Script:
#!/bin/bash
# Check test coverage for storage clearing operations
rg -A 10 -B 10 "ack\(|RECLAIM_" packages/analytics-js-common/__tests__/utilities/retryQueue/RetryQueue.test.ts
Length of output: 112
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 610-610: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L610
Added line #L610 was not covered by tests
[warning] 615-615: packages/analytics-js-common/src/utilities/retryQueue/RetryQueue.ts#L615
Added line #L615 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/analytics-js/src/components/capabilitiesManager/detection/dom.ts (1)
Line range hint
8-42: Consider organizing capabilities by category.The
legacyJSEngineRequiredPolyfillsobject mixes different types of capabilities (DOM, network, language features). Consider organizing these checks into separate categories for better maintainability and clarity:const languageFeaturePolyfills = { 'Number.isNaN': () => !isFunction(globalThis.Number.isNaN), // ... other language feature checks }; const networkFeaturePolyfills = { fetch: () => !isFunction(globalThis.fetch), AbortController: () => !isFunction(globalThis.AbortController), // ... other network-related checks }; const domFeaturePolyfills = { 'Element.prototype.dataset': () => !isDatasetAvailable(), CustomEvent: () => !isFunction(globalThis.CustomEvent), // ... other DOM-related checks }; const legacyJSEngineRequiredPolyfills = { ...languageFeaturePolyfills, ...networkFeaturePolyfills, ...domFeaturePolyfills, };This organization would:
- Make the code more maintainable
- Make it easier to understand which features belong to which category
- Allow for more granular polyfill loading if needed in the future
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.eslintrc.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts(3 hunks)packages/analytics-js/src/components/capabilitiesManager/detection/dom.ts(2 hunks)
⏰ 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 (5)
packages/analytics-js/__tests__/components/configManager/ConfigManager.test.ts (3)
5-8: LGTM! Good type organization.Consolidating type imports and moving them to the common package improves code organization and reusability.
229-229: LGTM! Improved type safety.Explicit type casting to
SourceConfigResponseenhances type checking and makes the test's intentions clearer.
243-244: LGTM! Good error handling test.Testing with stringified JSON input helps ensure robust error handling for invalid configurations.
packages/analytics-js/src/components/capabilitiesManager/detection/dom.ts (2)
24-24: Review the necessity of theString.prototype.trimcheck.The addition of
fetchandAbortControllerchecks aligns well with the PR's objective of moving data plane events processing to core. However, theString.prototype.trimcheck seems unrelated to this objective and is widely supported in modern browsers.Could you clarify why the
String.prototype.trimcheck was added? If it's not directly related to the data plane events processing, consider handling it in a separate PR.Also applies to: 40-41
44-47: Great refactoring ofisLegacyJSEngine!The refactoring to use
Array.prototype.some()improves code readability while maintaining the same functionality. The type safety is well preserved.
…ndling-to-core-sdk-2823
…ndling-to-core-sdk-2823
…ndling-to-core-sdk-2823
|
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-2823/move-dataplane-events-dispatch-functionality-to-core-sdk
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
Release Notes
Overview
This release introduces significant architectural changes to the RudderStack Analytics JavaScript SDK, focusing on simplifying event delivery mechanisms and improving type safety.
New Features
DataPlaneEventsQueueclass for managing event deliveryBreaking Changes
BeaconQueueandXhrQueuepluginsuseBeaconandbeaconQueueOptionsload optionsisBeaconAvailablecapability detectionImprovements
Bug Fixes
Deprecations