feat: move data plane events processing to core #1999
Conversation
…porting-to-core-sdk-2826
📝 WalkthroughWalkthroughThis pull request introduces significant changes to the RudderStack JavaScript SDK, focusing on removing beacon-related functionality and streamlining event queue and HTTP request handling. The changes involve removing the Changes
Sequence DiagramsequenceDiagram
participant SDK as RudderStack SDK
participant ConfigManager as Configuration Manager
participant PluginsManager as Plugins Manager
participant HttpClient as HTTP Client
SDK->>ConfigManager: Initialize configuration
ConfigManager-->>SDK: Process source configuration
SDK->>PluginsManager: Load plugins
PluginsManager-->>SDK: Remove beacon and XHR queue plugins
SDK->>HttpClient: Set authentication header
HttpClient-->>SDK: Configure request method
SDK->>SDK: Track page lifecycle events
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.29% 61.23% -0.06%
===========================================
Files 478 469 -9
Lines 16394 16178 -216
Branches 3240 3221 -19
===========================================
- Hits 10048 9907 -141
+ Misses 5132 5063 -69
+ Partials 1214 1208 -6 ☔ 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
| @@ -14,7 +15,7 @@ | |||
|
|
|||
| beforeEach(() => { | |||
| clientInstance = new HttpClient(defaultLogger); | |||
| clientInstance.init(defaultErrorHandler); | |||
| clientInstance.setAuthHeader('dummyWriteKey'); | |||
There was a problem hiding this comment.
Avoid hard-coded credentials.
Per CodeQL warnings, dummyWriteKey and rawHeaderValue are hard-coded. Use environment variables or test config constants instead to reduce potential security exposure.
- clientInstance.setAuthHeader('dummyWriteKey');
+ // e.g., from process.env.TEST_AUTH_KEY or a test config
+ clientInstance.setAuthHeader(process.env.TEST_AUTH_KEY || 'dummyKeyValue');And similarly for rawHeaderValue.
Also applies to: 65-65
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 18-18: Hard-coded credentials
The hard-coded value "dummyWriteKey" is used as authorization header.
There was a problem hiding this comment.
These are not real sensitive values. So, we can ignore.
There was a problem hiding this comment.
@saikumarrs Understood, I'll note this for future reviews.
✏️ Learnings added
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1999
File: packages/analytics-js/__tests__/services/HttpClient/HttpClient.test.ts:18-18
Timestamp: 2025-01-24T10:29:23.632Z
Learning: Hard-coded dummy credentials (like 'dummyWriteKey', 'rawHeaderValue') in test files are acceptable as they are not real sensitive values and improve test readability.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| @@ -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.
| 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
Additional test coverage needed for reclaim value clearing
The test file has extensive coverage for queue operations, but lacks explicit verification of reclaim value clearing during acknowledgment. The suggested test case would improve coverage of this specific state management functionality.
🔗 Analysis chain
Add test coverage for reclaim value clearing.
The code that clears reclaimStartVal and reclaimEndVal (lines 567 and 572) lacks test coverage according to static analysis.
Consider adding test cases to cover these scenarios:
test('should clear reclaim values during acknowledgment', () => {
// Setup
queue.reclaimStartVal = 'test';
queue.reclaimEndVal = 'test';
// Execute
queue.ack();
// Verify
expect(queue.reclaimStartVal).toBeNull();
expect(queue.reclaimEndVal).toBeNull();
});🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find test files related to RetryQueue
echo "=== Looking for RetryQueue test files ==="
fd -t f "RetryQueue.*test" --full-path
echo -e "\n=== Looking for tests covering ack() method ==="
ast-grep --pattern 'test($$$, () => { $$$ queue.ack(); $$$ })'
echo -e "\n=== Looking for tests mentioning reclaimStartVal or reclaimEndVal ==="
rg "reclaimStartVal|reclaimEndVal" -A 5 -B 5
Length of output: 7574
Script:
#!/bin/bash
echo "=== Examining RetryQueue test file content ==="
cat "packages/analytics-js-common/__tests__/utilities/retryQueue/RetryQueue.test.ts"
Length of output: 32875
🧰 Tools
🪛 GitHub Check: codecov/patch
[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
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
Removed Features
useBeaconconfiguration optioneventsQueuePluginNamefrom data plane events stateType Safety Improvements
Configuration Changes
HTTP Client Updates
getAsyncDatamethod with newrequestmethod