feat: track time spent on a page #1876
Conversation
📝 WalkthroughWalkthroughThis pull request introduces modifications to the size limits in the Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1876 +/- ##
===========================================
+ Coverage 57.24% 57.34% +0.09%
===========================================
Files 480 482 +2
Lines 16363 16417 +54
Branches 3282 3285 +3
===========================================
+ Hits 9367 9414 +47
- Misses 5719 5735 +16
+ Partials 1277 1268 -9 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
packages/analytics-js/src/state/slices/loadOptions.ts (1)
45-48: LGTM: NewtrackPageLifecycleproperty is well-structured.The addition of the
trackPageLifecycleproperty aligns well with the PR objective of introducing page lifecycle event tracking. The structure withenabledset tofalseby default is a good practice, allowing users to opt-in to the feature.Consider adding a brief comment above the
trackPageLifecycleproperty to explain its purpose and how to enable it. This would improve the code's self-documentation. For example:// Configuration for tracking page lifecycle events (load and unload). // Set 'enabled' to true to activate this feature. trackPageLifecycle: { enabled: false, events: [PageLifecycleEvents.PAGELOADED, PageLifecycleEvents.PAGEUNLOADED], },packages/analytics-js/.size-limit.mjs (1)
39-39: Summary of size limit changes.The size limit increases (0.5 KiB for each build) appear to be related to the new feature for tracking page lifecycle events mentioned in the PR objectives. These changes are consistent across different build configurations (NPM, CDN, and Content Script).
While the increases seem reasonable, it's important to:
- Verify that these increases are indeed necessary and minimal for the new feature.
- Ensure that the feature is optimized for size, especially considering its impact on the Content Script version.
- Document the reason for these increases in the PR description or commit messages for future reference.
Consider implementing a strategy for monitoring and managing bundle size growth over time, such as setting up automated checks or alerts when size limits are approached or exceeded.
Also applies to: 50-50, 116-116
packages/analytics-js-common/src/types/LoadOptions.ts (2)
116-120: LGTM! Consider adding a comment for default values.The
TrackPageLifecycleOptionstype is well-structured and provides a clear way to configure page lifecycle tracking. The use of thePageLifecycleEventsenum for theeventsproperty ensures type safety.Consider adding a comment to clarify the default behavior when
eventsis not provided. For example:export type TrackPageLifecycleOptions = { enabled: boolean; // default false events?: PageLifecycleEvents[]; // If not provided, both PAGELOADED and PAGEUNLOADED events will be tracked options?: ApiOptions; };This addition would improve the clarity of the API for developers using this type.
164-164: LGTM! Consider adding JSDoc comment.The addition of the
trackPageLifecycleproperty to theLoadOptionstype is well-implemented and consistent with the existing structure.Consider adding a JSDoc comment to provide more context about this option. For example:
/** * Configuration for tracking page lifecycle events. * When enabled, it allows tracking of page load and unload events. */ trackPageLifecycle?: TrackPageLifecycleOptions;This addition would improve the self-documentation of the
LoadOptionstype and provide helpful information for developers using this SDK.packages/analytics-js/public/index.html (1)
153-161: Review theloadOptionsconfiguration and document changesThe addition of the
trackPageLifecycleproperty aligns well with the PR objective of tracking page lifecycle events. However, I have a few observations and suggestions:
The
trackPageLifecycleproperty is currently commented out. Consider uncommenting it to enable the feature, or provide a comment explaining why it's disabled by default.Several other properties in the
loadOptionsobject have been commented out. It's unclear whether this is intentional or if these are meant to be configurable options. Consider:
- Adding comments to explain why these options are commented out.
- If these are meant to be example configurations, consider moving them to a separate documentation file or adding a clear comment indicating they are example options.
To improve maintainability and clarity, consider adding a brief comment above the
loadOptionsobject explaining its purpose and how developers should use or modify it.Would you like assistance in drafting documentation for these configuration options?
packages/analytics-js/src/constants/logMessages.ts (1)
260-261: LGTM! Consider adding a test for the new constant.The new constant
PAGE_UNLOAD_ON_BEACON_DISABLED_WARNINGis well-defined and follows the existing patterns in the file. The message is clear and informative.To improve test coverage and address the codecov warning, consider adding a unit test for this new constant. This will ensure that any future changes to the message are caught by the test suite.
Also applies to: 327-327
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 261-261: packages/analytics-js/src/constants/logMessages.ts#L261
Added line #L261 was not covered by testspackages/analytics-js/src/app/RudderAnalytics.ts (1)
172-173: Clarify the inline comment regarding beacon usage.The comment
// throw warning if beacon is disabledmay not accurately reflect the code behavior. Since the warning is logged whenuseBeaconisfalse, consider rephrasing the comment to:// Log warning if beacon is disabledThis adjustment ensures the comment accurately describes the subsequent code block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- packages/analytics-js-common/src/types/LoadOptions.ts (2 hunks)
- packages/analytics-js/.size-limit.mjs (3 hunks)
- packages/analytics-js/public/index.html (1 hunks)
- packages/analytics-js/src/app/RudderAnalytics.ts (5 hunks)
- packages/analytics-js/src/components/utilities/loadOptions.ts (1 hunks)
- packages/analytics-js/src/constants/logMessages.ts (2 hunks)
- packages/analytics-js/src/state/slices/loadOptions.ts (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/analytics-js-common/src/types/LoadOptions.ts
[warning] 112-113: packages/analytics-js-common/src/types/LoadOptions.ts#L112-L113
Added lines #L112 - L113 were not covered by testspackages/analytics-js/src/app/RudderAnalytics.ts
[warning] 158-159: packages/analytics-js/src/app/RudderAnalytics.ts#L158-L159
Added lines #L158 - L159 were not covered by tests
[warning] 161-161: packages/analytics-js/src/app/RudderAnalytics.ts#L161
Added line #L161 was not covered by tests
[warning] 175-175: packages/analytics-js/src/app/RudderAnalytics.ts#L175
Added line #L175 was not covered by tests
[warning] 177-177: packages/analytics-js/src/app/RudderAnalytics.ts#L177
Added line #L177 was not covered by tests
[warning] 179-179: packages/analytics-js/src/app/RudderAnalytics.ts#L179
Added line #L179 was not covered by tests
[warning] 191-191: packages/analytics-js/src/app/RudderAnalytics.ts#L191
Added line #L191 was not covered by tests
[warning] 205-205: packages/analytics-js/src/app/RudderAnalytics.ts#L205
Added line #L205 was not covered by tests
[warning] 208-208: packages/analytics-js/src/app/RudderAnalytics.ts#L208
Added line #L208 was not covered by testspackages/analytics-js/src/components/utilities/loadOptions.ts
[warning] 127-127: packages/analytics-js/src/components/utilities/loadOptions.ts#L127
Added line #L127 was not covered by testspackages/analytics-js/src/constants/logMessages.ts
[warning] 261-261: packages/analytics-js/src/constants/logMessages.ts#L261
Added line #L261 was not covered by tests
🪛 Biome
packages/analytics-js/src/components/utilities/loadOptions.ts
[error] 125-125: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (10)
packages/analytics-js/src/state/slices/loadOptions.ts (2)
3-6: LGTM: Import statement changes are appropriate.The addition of
PageLifecycleEventsimport and the change to a type import forLoadOptionsare consistent with their usage and follow TypeScript best practices.
Line range hint
1-48: Overall, the changes align well with the PR objectives.The modifications to
loadOptions.tssuccessfully introduce the groundwork for tracking page lifecycle events, as outlined in the PR objectives. The implementation is conservative (disabled by default) and flexible (allows specifying which events to track), which is a good approach for introducing new features.The changes are well-structured and maintain the existing code organization. They provide the necessary configuration options for the new feature while ensuring backward compatibility by keeping the feature disabled by default.
To ensure that these changes are properly utilized, let's verify the usage of
trackPageLifecyclein other parts of the codebase:This will help us confirm that the new configuration is being properly utilized and that
PageLifecycleEventsis imported where necessary.packages/analytics-js/.size-limit.mjs (3)
50-50: Approve consistent size limit increase.The increase in size limit from 24.5 KiB to 25 KiB for "Core - Modern - CDN" is consistent with the previous change for the NPM build. This maintains parity between the NPM and CDN distributions, which is good practice.
116-116: Approve size limit increase with verification for Content Script.The increase in size limit from 38.5 KiB to 39 KiB for "Core (Content Script) - Modern - NPM (CJS)" is approved. However, it's important to verify that this increase is necessary for the Content Script version and that the new feature is indeed applicable in this context.
Please run the following script to verify the actual size of the Content Script build and compare it with the regular build:
#!/bin/bash # Verify the actual size of the Core (Content Script) - Modern - NPM (CJS) build content_script_size=$(wc -c < dist/npm/modern/content-script/cjs/index.cjs) content_script_size_kib=$(echo "scale=2; $content_script_size / 1024" | bc) echo "Actual size of Core (Content Script) - Modern - NPM (CJS): $content_script_size_kib KiB" # Compare with the regular build regular_size=$(wc -c < dist/npm/modern/cjs/index.cjs) regular_size_kib=$(echo "scale=2; $regular_size / 1024" | bc) echo "Actual size of Core - Modern - NPM (CJS): $regular_size_kib KiB" # Calculate the difference size_diff=$(echo "$content_script_size_kib - $regular_size_kib" | bc) echo "Size difference: $size_diff KiB"This will help ensure that the new limit is appropriate for the Content Script version and that the size difference between the regular and Content Script builds is justified.
39-39: Approve size limit increase with verification.The increase in size limit from 24.5 KiB to 25 KiB for "Core - Modern - NPM (CJS)" seems reasonable, given the new feature for tracking page lifecycle events. However, it's important to verify that this increase is indeed necessary and minimal.
Please run the following script to verify the actual size of the build:
This will help ensure that the new limit is appropriate and not overly permissive.
packages/analytics-js/src/components/utilities/loadOptions.ts (1)
124-130: LGTM! The changes align with the PR objectives and maintain code consistency.The new block for handling the
trackPageLifecycleproperty is implemented correctly and consistently with other properties in thenormalizeLoadOptionsfunction. It properly validates the property and removes undefined and null values.Regarding the static analysis hint about using the
deleteoperator:
- The use of
deleteis consistent with the rest of the function and codebase.- While assigning
undefinedcould be an alternative, it might not have the same effect in all cases, especially for non-configurable properties.To verify the impact and usage of
deletevsundefinedassignment, you can run the following script:This will help determine if the current usage is a established pattern in the codebase.
Regarding the code coverage warning:
The uncovered line is part of a utility function call, which is likely covered by other tests. To improve coverage, consider adding a test case specifically for thetrackPageLifecycleproperty normalization.✅ Verification successful
LGTM! The use of the
deleteoperator is consistent with the existing codebase practices.The
deleteoperation fortrackPageLifecyclealigns with how other properties are handled in thenormalizeLoadOptionsfunction and throughout the codebase. The static analysis warning appears to be a general recommendation and does not pose an issue in this context.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the usage of delete operator vs undefined assignment in the codebase echo "Occurrences of 'delete' operator:" rg '\bdelete\s+\w+(\.\w+)+' --type ts echo "\nOccurrences of undefined assignment for property removal:" rg '\w+(\.\w+)+ = undefined' --type tsLength of output: 8767
🧰 Tools
🪛 Biome
[error] 125-125: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 GitHub Check: codecov/patch
[warning] 127-127: packages/analytics-js/src/components/utilities/loadOptions.ts#L127
Added line #L127 was not covered by testspackages/analytics-js-common/src/types/LoadOptions.ts (1)
Line range hint
1-164: Overall, the changes look good and align with the PR objectives.The additions to
LoadOptions.tssuccessfully introduce the functionality to track page lifecycle events, as outlined in the PR objectives. The newPageLifecycleEventsenum,TrackPageLifecycleOptionstype, and the addition to theLoadOptionstype provide a clear and type-safe way to configure this feature.A few minor suggestions have been made to improve documentation and clarity:
- Consider adding test coverage for the
PageLifecycleEventsenum.- Add a comment to clarify the default behavior of the
eventsproperty inTrackPageLifecycleOptions.- Include a JSDoc comment for the
trackPageLifecycleproperty inLoadOptions.These changes effectively support the new feature for tracking page lifecycle events with configurable load options, as described in the PR summary.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 112-113: packages/analytics-js-common/src/types/LoadOptions.ts#L112-L113
Added lines #L112 - L113 were not covered by testspackages/analytics-js/src/app/RudderAnalytics.ts (3)
91-91: Confirm the impact of setting the minimum log level to 'WARN'.Setting the logger's minimum log level to
'WARN'in the constructor may suppress informational and debug logs that could be useful during development or troubleshooting. Ensure that this change aligns with the desired logging strategy and does not hinder the visibility of important log messages.
173-204: Verify the conditional registration of theonPageLeavehandler.Currently, the
onPageLeaveevent handler for tracking thePAGEUNLOADEDevent is only registered whenuseBeaconistrue. This means that if beacon usage is disabled, thePAGEUNLOADEDevent will not be tracked.Is this the intended behavior? If you want to track page unload events regardless of beacon usage, consider adjusting the condition to ensure the event handler is always registered, or provide alternative handling when
useBeaconisfalse.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 175-175: packages/analytics-js/src/app/RudderAnalytics.ts#L175
Added line #L175 was not covered by tests
[warning] 177-177: packages/analytics-js/src/app/RudderAnalytics.ts#L177
Added line #L177 was not covered by tests
[warning] 179-179: packages/analytics-js/src/app/RudderAnalytics.ts#L179
Added line #L179 was not covered by tests
[warning] 191-191: packages/analytics-js/src/app/RudderAnalytics.ts#L191
Added line #L191 was not covered by tests
235-235: Ensure correct integration oftrackPageLifecycleEventsafter the load event.The call to
this.trackPageLifecycleEvents(preloadedEventsArray, loadEvent[3]);integrates page lifecycle event tracking when a load event is triggered. Please verify thatloadEvent[3]correctly corresponds to theloadOptionsparameter expected by the method and that all necessary data is being passed for accurate tracking.
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/analytics-js/src/app/RudderAnalytics.ts (5 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/analytics-js/src/app/RudderAnalytics.ts (2)
12-16: LGTM: Import statements updated appropriately.The new imports (
PageLifecycleEvents,generateUUID,onPageLeave,PAGE_UNLOAD_ON_BEACON_DISABLED_WARNING) and the modified import ofLoadOptionsas a type are consistent with the new functionality being added to track page lifecycle events.Also applies to: 23-24, 35-39
Line range hint
1-458: Overall assessment: Good addition with room for improvementThe introduction of page lifecycle event tracking is a valuable feature that enhances the analytics capabilities of the SDK. The implementation is generally well-structured and handles various scenarios appropriately. However, there are several areas where the code could be improved:
- The
trackPageLifecycleEventsmethod could benefit from refactoring to improve readability and maintainability.- Consider making the log level configurable to balance between production and development needs.
- The
triggerBufferedLoadEventmethod could be more explicit about the options passed totrackPageLifecycleEvents.These improvements would enhance the overall quality and maintainability of the code. Please review the suggested changes and consider implementing them where appropriate.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
packages/analytics-js-common/src/types/LoadOptions.ts (3)
111-114: LGTM! Consider simplifying enum values.The
PageLifecycleEventsenum is well-defined and provides a clear representation of page lifecycle events. However, as suggested in a previous review, we can simplify the enum values by removing the extra context.Consider applying this change:
export enum PageLifecycleEvents { - LOADED = 'Page Loaded', - UNLOADED = 'Page Unloaded', + LOADED = 'Loaded', + UNLOADED = 'Unloaded', }This simplification maintains clarity while reducing redundancy.
116-120: LGTM! Consider adding JSDoc comments.The
TrackPageLifecycleOptionstype is well-structured and provides clear options for tracking page lifecycle events. The use of optional properties allows for flexible configuration.To improve documentation, consider adding JSDoc comments for each property:
/** * Options for tracking page lifecycle events. */ export type TrackPageLifecycleOptions = { /** Whether to enable page lifecycle tracking. Defaults to false. */ enabled: boolean; /** Specific page lifecycle events to track. If not specified, all events are tracked. */ events?: PageLifecycleEvents[]; /** Additional options for the API call when tracking events. */ options?: ApiOptions; };This addition will provide more context for developers using this type.
164-164: LGTM! Consider adding a JSDoc comment.The addition of
trackPageLifecycleto theLoadOptionstype is well-implemented. It's correctly marked as optional and uses the newly definedTrackPageLifecycleOptionstype.To improve documentation, consider adding a JSDoc comment for the new property:
/** * Options for tracking page lifecycle events. If not provided, page lifecycle tracking is disabled. */ trackPageLifecycle?: TrackPageLifecycleOptions;This comment will provide context for developers using the
LoadOptionstype.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/analytics-js-common/src/types/LoadOptions.ts (2 hunks)
- packages/analytics-js/src/app/RudderAnalytics.ts (5 hunks)
- packages/analytics-js/src/constants/logMessages.ts (2 hunks)
- packages/analytics-js/src/state/slices/loadOptions.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js/src/constants/logMessages.ts
- packages/analytics-js/src/state/slices/loadOptions.ts
🧰 Additional context used
🔇 Additional comments (2)
packages/analytics-js-common/src/types/LoadOptions.ts (1)
Line range hint
1-164: Overall, great implementation of page lifecycle tracking!The changes in this file successfully implement the new feature for tracking page lifecycle events. The additions are well-structured, type-safe, and maintain backward compatibility. The code is clean and follows good practices.
Key points:
- The
PageLifecycleEventsenum clearly defines the tracked events.- The
TrackPageLifecycleOptionstype provides a flexible configuration for the feature.- The
LoadOptionstype is updated correctly to include the new tracking option.Minor suggestions have been made to improve documentation with JSDoc comments, which will enhance the developer experience when using these types.
packages/analytics-js/src/app/RudderAnalytics.ts (1)
12-16: LGTM: Import changes are appropriate for the new feature.The new imports (PageLifecycleEvents, generateUUID, onPageLeave, PAGE_UNLOAD_ON_BEACON_DISABLED_WARNING) and the modified import of LoadOptions as a type are well-aligned with the new page lifecycle tracking feature being implemented.
Also applies to: 23-24, 35-39
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/analytics-js-common/src/types/IRudderAnalytics.ts (1)
79-80: LGTM! Consider adding JSDoc comments for clarity.The addition of
visitIdandpageLoadedTimestampproperties aligns well with the PR objectives of tracking page lifecycle events and time spent on a page. These properties will enhance the analytics capabilities of the SDK.Consider adding JSDoc comments to provide more context about these properties. For example:
/** Unique identifier for the current visit */ visitId: string; /** Timestamp when the page was loaded */ pageLoadedTimestamp: number;This will improve code readability and make it easier for other developers to understand the purpose of these properties.
packages/analytics-js/src/app/RudderAnalytics.ts (2)
93-93: LGTM: Setting minimum log level, but consider environment-based configuration.Setting the minimum log level to 'WARN' is a good practice for production environments as it reduces log verbosity. However, this might hinder debugging in development environments.
Consider making this log level configurable based on the environment. For example:
this.logger?.setMinLogLevel(process.env.NODE_ENV === 'production' ? 'WARN' : 'DEBUG');This would allow for more verbose logging in development while maintaining the 'WARN' level for production.
146-208: LGTM: NewtrackPageLifecycleEventsmethod, but consider refactoring for improved readability.The new
trackPageLifecycleEventsmethod successfully implements the tracking of page loaded and unloaded events. It appropriately uses thevisitIdandpageLoadedTimestampproperties and includes logic for using the Beacon API for the unload event.Consider refactoring this method to improve readability and maintainability:
- Extract the logic for creating page loaded and unloaded events into separate methods.
- Create a separate method for handling the page unload event listener.
Here's a suggested refactoring:
private createPageLoadedEvent(options: object) { return [ 'track', PageLifecycleEvents.LOADED, { visitId: this.visitId }, { ...options, originalTimestamp: new Date(this.pageLoadedTimestamp).toISOString(), }, ]; } private createPageUnloadedEvent(options: object) { const pageUnloadedTimestamp = Date.now(); const visitDuration = pageUnloadedTimestamp - this.pageLoadedTimestamp; return [ 'track', PageLifecycleEvents.UNLOADED, { visitId: this.visitId, visitDuration, }, { ...options, originalTimestamp: new Date(pageUnloadedTimestamp).toISOString(), }, ]; } private handlePageUnload(options: object) { onPageLeave((isAccessible: boolean) => { if (isAccessible === false && state.lifecycle.loaded.value) { this.track(...this.createPageUnloadedEvent(options)); } }); } trackPageLifecycleEvents( preloadedEventsArray: PreloadedEventCall[], loadOptions?: Partial<LoadOptions>, ) { const { trackPageLifecycle, useBeacon } = loadOptions ?? {}; const { events = [PageLifecycleEvents.LOADED, PageLifecycleEvents.UNLOADED], enabled = false, options = {}, } = trackPageLifecycle ?? {}; if (!enabled) { return; } if (events.length === 0 || events.includes(PageLifecycleEvents.LOADED)) { preloadedEventsArray.unshift(this.createPageLoadedEvent(options)); } if (events.length === 0 || events.includes(PageLifecycleEvents.UNLOADED)) { if (useBeacon === true) { this.handlePageUnload(options); } else { this.logger.warn(PAGE_UNLOAD_ON_BEACON_DISABLED_WARNING(RS_APP)); } } setExposedGlobal(GLOBAL_PRELOAD_BUFFER, clone(preloadedEventsArray)); }This refactoring improves readability and makes the code more modular and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/analytics-js-common/src/types/IRudderAnalytics.ts (1 hunks)
- packages/analytics-js/src/app/RudderAnalytics.ts (6 hunks)
- packages/analytics-js/src/constants/logMessages.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js/src/constants/logMessages.ts
🧰 Additional context used
📓 Learnings (1)
packages/analytics-js/src/app/RudderAnalytics.ts (1)
Learnt from: MoumitaM PR: rudderlabs/rudder-sdk-js#1876 File: packages/analytics-js/src/app/RudderAnalytics.ts:91-91 Timestamp: 2024-10-09T05:50:07.839Z Learning: In the `RudderAnalytics` class, `loadOptions` are determined at a later stage and are not available in the constructor when setting the logger's minimum log level.
🔇 Additional comments (5)
packages/analytics-js-common/src/types/IRudderAnalytics.ts (2)
79-80: Summary: Changes align with PR objectives, implementation verification needed.The additions to the
IRudderAnalyticsinterface are concise and align well with the PR objectives of tracking page lifecycle events and time spent on a page. ThevisitIdandpageLoadedTimestampproperties provide the necessary foundation for these features.However, to ensure the completeness of this feature:
- Verify the actual implementation of these properties in the class that implements this interface.
- Check the implementation of the configurable load options mentioned in the PR objectives.
- Ensure that these new properties are utilized correctly in the page tracking and time calculation logic.
Overall, the changes to this file are approved, pending the verification of their implementation and usage in the broader context of the SDK.
79-80: Verify implementation and configurable options.While the addition of
visitIdandpageLoadedTimestampto the interface is correct, please ensure the following:
- These properties are properly implemented in the class that implements the
IRudderAnalyticsinterface.- The PR objectives mention configurable load options. Although not visible in this file, make sure these options are implemented and connected to the new properties where appropriate.
To verify the implementation and usage of these new properties, you can run the following script:
✅ Verification successful
Implementation and Configurable Options Verified Successfully.
The
visitIdandpageLoadedTimestampproperties are properly implemented in theRudderAnalyticsclass. Additionally, configurable load options are correctly integrated and utilized across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the implementation of new properties and related configurable options # Test 1: Search for the implementation of visitId and pageLoadedTimestamp echo "Searching for visitId and pageLoadedTimestamp implementation:" rg --type typescript 'visitId|pageLoadedTimestamp' -g '!**/IRudderAnalytics.ts' # Test 2: Search for updates to LoadOptions or related configuration echo "Searching for LoadOptions or related configuration updates:" rg --type typescript 'LoadOptions|configurable.*options' -g '!**/IRudderAnalytics.ts' # Test 3: Search for usage of these properties in tracking functions echo "Searching for usage in tracking functions:" rg --type typescript '(page|track|identify).*\b(visitId|pageLoadedTimestamp)\b'Length of output: 713
Script:
#!/bin/bash # Description: Check for the implementation of new properties and related configurable options with corrected type # Test 1: Search for the implementation of visitId and pageLoadedTimestamp echo "Searching for visitId and pageLoadedTimestamp implementation:" rg --type ts 'visitId|pageLoadedTimestamp' -g '!**/IRudderAnalytics.ts' # Test 2: Search for updates to LoadOptions or related configuration echo "Searching for LoadOptions or related configuration updates:" rg --type ts 'LoadOptions|configurable.*options' -g '!**/IRudderAnalytics.ts' # Test 3: Search for usage of these properties in tracking functions echo "Searching for usage in tracking functions:" rg --type ts '(page|track|identify).*\b(visitId|pageLoadedTimestamp)\b'Length of output: 14957
packages/analytics-js/src/app/RudderAnalytics.ts (3)
12-16: LGTM: Import changes are appropriate for the new functionality.The new imports and modifications to existing imports are consistent with the introduction of page lifecycle event tracking. The addition of
PageLifecycleEvents,generateUUID,onPageLeave, andstateimports, along with the new warning constant, aligns well with the new features being implemented.Also applies to: 23-24, 35-39, 41-41
56-57: LGTM: New properties are well-defined for page lifecycle tracking.The addition of
visitIdandpageLoadedTimestampproperties is appropriate for implementing page lifecycle event tracking. ThevisitIdis initialized with a generated UUID, andpageLoadedTimestampis set to the current timestamp, which aligns well with their intended purposes.
232-233: LGTM: Integration of page lifecycle tracking intriggerBufferedLoadEvent.The addition of the
trackPageLifecycleEventscall in thetriggerBufferedLoadEventmethod successfully integrates the new page lifecycle tracking functionality with the existing load event handling. This ensures that page lifecycle events are tracked when a load event is triggered from the buffered events.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
packages/analytics-js/src/state/slices/pageLifecycle.ts (1)
4-7: LGTM: Well-structured page lifecycle state. Consider enhancing type safety.The
pageLifecycleStateconstant is well-defined:
- It correctly implements the
PageLifecycleStatetype.- Using
signalfor both properties enables reactive updates, which is ideal for tracking dynamic page lifecycle events.- Initializing with
undefinedis appropriate, as these values will be set when the page loads.Consider enhancing type safety by explicitly defining the signal types:
const pageLifecycleState: PageLifecycleState = { visitId: signal<string | undefined>(undefined), pageLoadedTimestamp: signal<number | undefined>(undefined), };This change would make the expected types of
visitIdandpageLoadedTimestampmore explicit, potentially preventing type-related issues in the future.packages/analytics-js/src/state/index.ts (1)
36-36: LGTM: New state slice integrated correctly.The
pageLifecyclestate slice has been properly added to bothdefaultStateValuesand theresetStatefunction, maintaining consistency with the existing state management pattern. This addition aligns well with the PR objective of tracking page lifecycle events.Consider adding a brief comment above the
pageLifecycleadditions in both locations to explain its purpose, enhancing code readability. For example:// Track page lifecycle events (e.g., load, unload) pageLifecycle: pageLifecycleState,Also applies to: 59-59
packages/analytics-js-common/src/types/ApplicationState.ts (1)
199-199: LGTM: Appropriate integration ofPageLifecycleStateThe addition of
pageLifecycle: PageLifecycleState;to theApplicationStateinterface is appropriate and aligns with the existing structure. This integration allows for managing page lifecycle state at the top level of the application state, which is consistent with the PR's objective.Consider adding a brief comment above the
pageLifecycleproperty to describe its purpose, maintaining consistency with other properties in the interface. For example:// Stores information about the current page's lifecycle pageLifecycle: PageLifecycleState;packages/analytics-js/src/app/RudderAnalytics.ts (1)
189-213: Consider HandlingUNLOADEDEvents WhenuseBeaconIs DisabledCurrently, if
useBeaconis set tofalse, the SDK logs a warning and does not track theUNLOADEDevent. This could lead to incomplete data on user sessions, as the duration and unload actions won't be captured.Consider implementing a fallback mechanism to track the
UNLOADEDevent even whenuseBeaconis disabled. Potential approaches include:
- Synchronous Requests: Using synchronous
XMLHttpRequestcalls to send theUNLOADEDevent before the page unloads. However, be cautious as synchronous requests on the main thread can negatively impact user experience.- Local Storage Queueing: Storing the
UNLOADEDevent in local storage to be sent on the next page load. This ensures the event isn't lost but may not capture the exact unload timing.- Navigator SendBeacon Alternative: Exploring other APIs or strategies that allow for background data transmission without relying on the beacon API.
Let me know if you'd like assistance in implementing one of these alternatives or discussing other potential solutions.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 192-192: packages/analytics-js/src/app/RudderAnalytics.ts#L192
Added line #L192 was not covered by tests
[warning] 194-196: packages/analytics-js/src/app/RudderAnalytics.ts#L194-L196
Added lines #L194 - L196 were not covered by tests
[warning] 211-211: packages/analytics-js/src/app/RudderAnalytics.ts#L211
Added line #L211 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- packages/analytics-js-common/src/types/ApplicationState.ts (2 hunks)
- packages/analytics-js/tests/services/Logger/Logger.test.ts (2 hunks)
- packages/analytics-js/src/app/RudderAnalytics.ts (4 hunks)
- packages/analytics-js/src/components/core/Analytics.ts (1 hunks)
- packages/analytics-js/src/services/Logger/Logger.ts (1 hunks)
- packages/analytics-js/src/state/index.ts (3 hunks)
- packages/analytics-js/src/state/slices/pageLifecycle.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
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-10-09T06:41:05.073Z 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/src/app/RudderAnalytics.ts
[warning] 177-177: packages/analytics-js/src/app/RudderAnalytics.ts#L177
Added line #L177 was not covered by tests
[warning] 192-192: packages/analytics-js/src/app/RudderAnalytics.ts#L192
Added line #L192 was not covered by tests
[warning] 194-196: packages/analytics-js/src/app/RudderAnalytics.ts#L194-L196
Added lines #L194 - L196 were not covered by tests
[warning] 211-211: packages/analytics-js/src/app/RudderAnalytics.ts#L211
Added line #L211 was not covered by tests
[warning] 214-214: packages/analytics-js/src/app/RudderAnalytics.ts#L214
Added line #L214 was not covered by tests
🔇 Additional comments (15)
packages/analytics-js/src/state/slices/pageLifecycle.ts (3)
1-2: LGTM: Appropriate imports for state management.The import statements are well-chosen:
- Using
signalfrom '@preact/signals-core' indicates a reactive approach to state management, which is beneficial for handling dynamic page lifecycle data.- Importing
PageLifecycleStatetype from a common package promotes code reuse and ensures consistency across the application.
9-9: LGTM: Appropriate export of the page lifecycle state.Exporting
pageLifecycleStateis correct:
- It allows other parts of the application to access and use this shared state.
- This approach promotes centralized state management, which is crucial for tracking page lifecycle events across the application.
1-9: LGTM: Well-implemented page lifecycle state management.This new file successfully introduces page lifecycle state management, aligning well with the PR objectives:
- It establishes a foundation for tracking page lifecycle events, specifically the
visitIdand page load timestamp.- The use of reactive programming (via
signal) enables efficient updates and subscriptions to these state values.- The modular approach, with this state defined in a separate slice, promotes good code organization and reusability.
The implementation provides a solid base for the feature to track time spent on a page. As development progresses, ensure to implement the logic for setting the
visitIdandpageLoadedTimestampvalues when the page loads, and calculate the time spent using these values when the page unloads.To ensure this new state is properly integrated, let's verify its usage:
packages/analytics-js/src/state/index.ts (2)
18-18: LGTM: New state slice import added correctly.The import of
pageLifecycleStatefollows the existing pattern and naming convention. It's properly organized within the project structure.
Line range hint
1-62: Overall, the changes look good and align with the PR objectives.The integration of the
pageLifecyclestate slice is well-implemented and consistent with the existing state management pattern. This addition supports the new feature for tracking page lifecycle events.To ensure the new state slice is properly utilized, let's verify its usage in the codebase:
This script will help us confirm that the new state slice is being properly imported and used throughout the codebase, and identify any remaining tasks related to its implementation.
✅ Verification successful
Verification of
pageLifecyclestate slice integration is successful.The
pageLifecycleStateis properly imported and utilized within the codebase, ensuring that the new page lifecycle tracking feature is fully integrated and functioning as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new pageLifecycle state slice # Test 1: Check for imports of pageLifecycleState echo "Checking for imports of pageLifecycleState:" rg --type typescript "import.*pageLifecycleState.*from.*slices/pageLifecycle" # Test 2: Check for usage of pageLifecycle in the state echo "Checking for usage of pageLifecycle in the state:" rg --type typescript "state\.pageLifecycle" # Test 3: Check for any TODO comments related to pageLifecycle echo "Checking for any TODO comments related to pageLifecycle:" rg --type typescript "TODO.*pageLifecycle"Length of output: 604
Script:
#!/bin/bash # Description: Verify the usage of the new pageLifecycle state slice by searching TypeScript files # Test 1: Check for imports of pageLifecycleState echo "Checking for imports of pageLifecycleState in .ts and .tsx files:" rg 'import.*pageLifecycleState.*from.*slices/pageLifecycle' --glob '*.ts' --glob '*.tsx' # Test 2: Check for usage of pageLifecycle in the state echo "Checking for usage of pageLifecycle in the state in .ts and .tsx files:" rg 'state\.pageLifecycle' --glob '*.ts' --glob '*.tsx' # Test 3: Check for any TODO comments related to pageLifecycle echo "Checking for any TODO comments related to pageLifecycle in .ts and .tsx files:" rg 'TODO.*pageLifecycle' --glob '*.ts' --glob '*.tsx'Length of output: 1332
packages/analytics-js-common/src/types/ApplicationState.ts (2)
86-89: LGTM: Well-definedPageLifecycleStatetypeThe
PageLifecycleStatetype is well-defined and aligns with the PR objective of tracking page lifecycle events. The use ofSignalfrom '@preact/signals-core' for both properties suggests a reactive approach to state management, which is beneficial for handling state changes efficiently.The types for
visitIdandpageLoadedTimestampare appropriate:
visitId: Signal<string | undefined>allows for cases where the visit ID might not be set.pageLoadedTimestamp: Signal<number | undefined>is suitable for storing a timestamp.This structure will facilitate tracking when a page is loaded and potentially calculating time spent on a page.
86-89: Summary: Effective implementation of page lifecycle tracking typesThe changes in this file successfully implement the necessary types for tracking page lifecycle events, which aligns with the PR objectives. The new
PageLifecycleStatetype and its integration into theApplicationStateinterface are well-structured and consistent with the existing codebase.Key points:
- The
PageLifecycleStatetype provides a clear structure for storing visit ID and page load timestamp.- The use of
Signalfrom '@preact/signals-core' suggests a reactive approach to state management.- The integration into
ApplicationStateis straightforward and maintains the existing structure.These changes lay a solid foundation for implementing the feature to track time spent on a page and manage page lifecycle events.
To ensure these changes are properly utilized, please verify that the corresponding logic for managing these new state properties is implemented in the relevant components or services. You may want to check files related to page tracking or analytics in the codebase.
Also applies to: 199-199
packages/analytics-js/__tests__/services/Logger/Logger.test.ts (1)
Line range hint
191-202: Behavior change: Default log level is now 'log' instead of 'error'The test case has been updated to reflect a change in the Logger's default behavior when an invalid log level is set. This is a significant change that affects the verbosity of logs in error scenarios.
- The new behavior allows for more verbose logging by default, which can be beneficial for debugging but might also increase log noise.
- The test correctly verifies that both 'error' and 'warn' levels are logged with the new default.
To ensure complete coverage of the new behavior:
- Add test cases for 'info' and 'log' levels:
loggerInstance.info('dummy info msg'); expect(console.info).toHaveBeenCalled(); loggerInstance.log('dummy log msg'); expect(console.log).toHaveBeenCalled();
Verify that the Logger class implementation has been updated to match this new behavior:
Update the documentation to reflect this change in default behavior:
Please ensure these additional steps are taken to maintain consistency across the codebase and documentation.
packages/analytics-js/src/app/RudderAnalytics.ts (7)
12-16: Updated Imports for Page Lifecycle TrackingThe inclusion of
PageLifecycleEvents,AnonymousIdOptions,ConsentOptions, andLoadOptionsin the imports ensures that all necessary types and constants are available for implementing the page lifecycle tracking feature.
23-24: Imports for Utilities Supporting UUID Generation and Page Leave DetectionImporting
generateUUIDandonPageLeaveutilities is appropriate as they are essential for generating unique visit IDs and detecting when a user leaves the page, respectively. This supports accurate tracking of user sessions and page lifecycle events.
35-39: Addition of New Log Message ConstantAdding
PAGE_UNLOAD_ON_BEACON_DISABLED_WARNINGto the imported log messages allows the SDK to warn users when page unload events cannot be tracked due to beacon usage being disabled. This improves transparency and aids in debugging.
41-41: Importing State Management ModuleImporting
{ state }from'../state'provides access to the application’s shared state, which is necessary for storing and retrieving thevisitIdandpageLoadedTimestampused in page lifecycle tracking.
92-94: Initialization ofvisitIdandpageLoadedTimestampin ConstructorInitializing
state.pageLifecycle.visitId.valuewith a generated UUID andstate.pageLifecycle.pageLoadedTimestamp.valuewith the current timestamp ensures that these values are captured as early as possible. This is crucial for accurately tracking the duration of a user's visit and associating events with the correct session.
144-145: Integrating Page Lifecycle Event Tracking in the Load ProcessCalling
this.trackPageLifecycleEvents(loadOptions);within theloadmethod appropriately integrates the page lifecycle event tracking into the SDK's initialization process. This ensures that the tracking configuration provided inloadOptionsis applied when the SDK loads.
156-215: Implementation oftrackPageLifecycleEventsMethodThe
trackPageLifecycleEventsmethod effectively handles the tracking of page loaded and unloaded events based on user-configurable options. Key aspects include:
- Configurable Tracking: Respecting the
enabledflag andeventsarray allows users to opt-in to specific lifecycle events.- Event Queuing: Preloading the
LOADEDevent into thepreloadedEventsArrayensures the event is captured even if the SDK isn't fully initialized.- Page Unload Handling: Using
onPageLeavewith theuseBeaconcheck ensures that theUNLOADEDevent is tracked reliably when the beacon API is available, improving data accuracy.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 177-177: packages/analytics-js/src/app/RudderAnalytics.ts#L177
Added line #L177 was not covered by tests
[warning] 192-192: packages/analytics-js/src/app/RudderAnalytics.ts#L192
Added line #L192 was not covered by tests
[warning] 194-196: packages/analytics-js/src/app/RudderAnalytics.ts#L194-L196
Added lines #L194 - L196 were not covered by tests
[warning] 211-211: packages/analytics-js/src/app/RudderAnalytics.ts#L211
Added line #L211 was not covered by tests
[warning] 214-214: packages/analytics-js/src/app/RudderAnalytics.ts#L214
Added line #L214 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
packages/analytics-js/src/services/Logger/index.ts (1)
1-7: LGTM. Consider updating documentation.The addition of
POST_LOAD_LOG_LEVELto the exports is consistent with the PR objectives and likely supports the new feature for tracking page lifecycle events. This change expands the public API of the Logger module in a non-breaking manner.Consider updating the module's documentation to reflect this new export and explain its purpose and usage, especially in relation to the new page lifecycle tracking feature.
packages/analytics-js/src/services/Logger/Logger.ts (1)
124-124: Approved: Addition of POST_LOAD_LOG_LEVEL to exportsThe addition of
POST_LOAD_LOG_LEVELto the export statement is consistent with its introduction earlier in the file. This makes the new constant available for use in other parts of the codebase.Consider adding a brief comment or documentation for the
POST_LOAD_LOG_LEVELconstant to explain its purpose and intended usage. This will help other developers understand when and how to use this new log level.packages/analytics-js/src/app/RudderAnalytics.ts (3)
12-16: New imports look good, but consider organizing them.The new imports for page lifecycle events, UUID generation, and page utilities are appropriate for the added functionality. However, the import statements could be better organized for improved readability.
Consider grouping related imports together. For example:
import { PageLifecycleEvents, type AnonymousIdOptions, type ConsentOptions, type LoadOptions, } from '@rudderstack/analytics-js-common/types/LoadOptions'; import { generateUUID } from '@rudderstack/analytics-js-common/utilities/uuId'; import { onPageLeave } from '@rudderstack/analytics-js-common/utilities/page'; // ... other imports ... import { EMPTY_GROUP_CALL_ERROR, PAGE_UNLOAD_ON_BEACON_DISABLED_WARNING, WRITE_KEY_NOT_A_STRING_ERROR, } from '../constants/logMessages'; import { state } from '../state';Also applies to: 23-24, 35-39
143-144: Consider moving page lifecycle tracking to a separate method.The addition of page lifecycle event tracking in the
loadmethod is good, but it might be cleaner to extract this into a separate method for better separation of concerns.Consider refactoring like this:
load(writeKey: string, dataPlaneUrl: string, loadOptions?: Partial<LoadOptions>) { // ... existing code ... this.setDefaultInstanceKey(writeKey); this.initializePageLifecycleTracking(loadOptions); // ... rest of the method ... } private initializePageLifecycleTracking(loadOptions?: Partial<LoadOptions>) { this.trackPageLifecycleEvents(loadOptions); }This change would improve the readability and maintainability of the
loadmethod.
150-159: Good implementation ofgetPreloadedEvents, but consider error handling.The
getPreloadedEventsmethod is a good addition for retrieving preloaded events. However, it might benefit from some additional error handling.Consider adding a try-catch block to handle potential errors when accessing the global object:
getPreloadedEvents(): PreloadedEventCall[] { try { const globalEvents = (globalThis as typeof window).rudderanalytics; return Array.isArray(globalEvents) ? globalEvents as PreloadedEventCall[] : []; } catch (error) { this.logger.error('Error accessing preloaded events:', error); return []; } }This change would make the method more robust against unexpected runtime errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/analytics-js/.size-limit.mjs (4 hunks)
- packages/analytics-js/src/app/RudderAnalytics.ts (4 hunks)
- packages/analytics-js/src/components/core/Analytics.ts (2 hunks)
- packages/analytics-js/src/services/Logger/Logger.ts (2 hunks)
- packages/analytics-js/src/services/Logger/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js/.size-limit.mjs
- packages/analytics-js/src/components/core/Analytics.ts
🧰 Additional context used
📓 Learnings (1)
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-10-09T06:41:05.073Z 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/src/app/RudderAnalytics.ts
[warning] 182-182: packages/analytics-js/src/app/RudderAnalytics.ts#L182
Added line #L182 was not covered by tests
[warning] 186-186: packages/analytics-js/src/app/RudderAnalytics.ts#L186
Added line #L186 was not covered by tests
[warning] 201-201: packages/analytics-js/src/app/RudderAnalytics.ts#L201
Added line #L201 was not covered by tests
[warning] 203-205: packages/analytics-js/src/app/RudderAnalytics.ts#L203-L205
Added lines #L203 - L205 were not covered by tests
[warning] 220-220: packages/analytics-js/src/app/RudderAnalytics.ts#L220
Added line #L220 was not covered by tests
[warning] 223-223: packages/analytics-js/src/app/RudderAnalytics.ts#L223
Added line #L223 was not covered by tests
🔇 Additional comments (2)
packages/analytics-js/src/app/RudderAnalytics.ts (2)
92-93: Good initialization of page lifecycle state.The addition of
visitIdandpageLoadedTimestampinitialization in the constructor is a good practice. It ensures that these values are set as early as possible in the lifecycle of theRudderAnalyticsinstance.
231-231: Good use ofgetPreloadedEventsmethod.The use of the newly added
getPreloadedEventsmethod intriggerBufferedLoadEventis appropriate and consistent with the new implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/analytics-js/src/state/slices/autoTrack.ts (1)
4-9: LGTM: Well-structured state management for page lifecycle tracking.The
autoTrackStateconstant is well-designed for tracking page lifecycle events. The use of signals forvisitIdandpageLoadedTimestampallows for reactive updates, which aligns with the PR objectives.Consider adding a brief comment explaining the purpose of
autoTrackStateand its properties for improved code readability:/** * Manages the state for auto-tracking analytics. * - visitId: Unique identifier for each page visit. * - pageLoadedTimestamp: Timestamp when the page was loaded. */ const autoTrackState: AutoTrackState = { // ... (rest of the code remains the same) };packages/analytics-js-common/src/types/LoadOptions.ts (2)
116-120: LGTM! Consider adding type forenableddefault.The
PageLifecycleOptionstype is well-structured and provides clear options for page lifecycle tracking. For consistency with other parts of the codebase, consider adding a type annotation for the default value of theenabledproperty.export type PageLifecycleOptions = { - enabled: boolean; // default false + enabled: boolean; // default: false events?: PageLifecycleEvents[]; options?: ApiOptions; };
122-126: LGTM! Consider adding type forenableddefault.The
AutoTrackOptionstype is well-structured and provides clear options for auto-tracking. For consistency with other parts of the codebase, consider adding a type annotation for the default value of theenabledproperty.export type AutoTrackOptions = { - enabled?: boolean; // default false + enabled?: boolean; // default: false options?: ApiOptions; pageLifecycle?: PageLifecycleOptions; };packages/analytics-js-common/src/types/ApplicationState.ts (1)
90-93: LGTM:PageLifecycleStatetype is well-defined.The
PageLifecycleStatetype is correctly implemented and aligns with the PR objectives. It uses theSignaltype consistently with other state definitions.Consider adding a brief comment explaining the purpose of
visitIdandpageLoadedTimestampfor better documentation. For example:export type PageLifecycleState = { // Unique identifier for the current page visit visitId: Signal<string | undefined>; // Timestamp when the page was loaded pageLoadedTimestamp: Signal<number | undefined>; };packages/analytics-js/src/app/RudderAnalytics.ts (1)
161-229: LGTM: Comprehensive page lifecycle tracking implemented.The
trackPageLifecycleEventsmethod is a solid implementation of page lifecycle tracking. It handles both page loaded and unloaded events, respects theenabledflag, and correctly uses theuseBeaconoption for unload tracking. The warning log for disabled beacon is a good addition for debugging purposes.However, consider refactoring this method into smaller, more focused methods to improve readability and maintainability. For example:
- A method for handling page loaded events
- A method for handling page unloaded events
- A method for setting up the page unload listener
This refactoring would make the code easier to understand and maintain in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/analytics-js-common/src/types/ApplicationState.ts (2 hunks)
- packages/analytics-js-common/src/types/LoadOptions.ts (2 hunks)
- packages/analytics-js/src/app/RudderAnalytics.ts (4 hunks)
- packages/analytics-js/src/state/index.ts (3 hunks)
- packages/analytics-js/src/state/slices/autoTrack.ts (1 hunks)
- packages/analytics-js/src/state/slices/loadOptions.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js/src/state/index.ts
- packages/analytics-js/src/state/slices/loadOptions.ts
🧰 Additional context used
📓 Learnings (1)
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-10-09T06:41:05.073Z Learning: The `trackPageLifecycleEvents` method in `packages/analytics-js/src/app/RudderAnalytics.ts` does not require refactoring as the current implementation is preferred.
🔇 Additional comments (13)
packages/analytics-js/src/state/slices/autoTrack.ts (3)
1-2: LGTM: Appropriate imports for reactive state management.The import statements are well-chosen for implementing reactive state management for auto-tracking analytics. The use of
@preact/signals-corealigns with the PR objective of tracking page lifecycle events, and importing theAutoTrackStatetype ensures type safety.
11-11: LGTM: Proper export of the autoTrackState constant.The named export of
autoTrackStateis correct and follows good practices. This allows for easy and clear importing in other parts of the application where page lifecycle tracking is needed.
1-11: Overall assessment: Well-implemented state management for auto-tracking analytics.This new file introduces a solid foundation for managing the state of auto-tracking analytics, particularly for page lifecycle events. The implementation aligns well with the PR objectives and follows good practices:
- Utilizes reactive programming with
@preact/signals-corefor efficient state updates.- Ensures type safety with proper TypeScript typing.
- Provides a clear and concise structure for managing page visit IDs and load timestamps.
The code is well-organized and sets up the necessary infrastructure for tracking time spent on a page, as outlined in the PR objectives. It's a good starting point for implementing the feature, and future additions can build upon this structure.
To ensure this new file is properly integrated into the project, let's verify its usage:
This script will help us confirm that the new
autoTrackStateis being imported and used correctly in other parts of the application, ensuring it's properly integrated into the auto-tracking feature.packages/analytics-js-common/src/types/LoadOptions.ts (3)
111-114: LGTM! Suggestion addressed.The
PageLifecycleEventsenum is well-defined and provides a clear representation of page lifecycle events. The naming is consistent and follows common conventions. The suggestion to avoid extra context in the enum values has been addressed.
170-170: LGTM!autoTrackproperty added.The addition of the
autoTrackproperty to theLoadOptionstype is consistent with the newly definedAutoTrackOptionstype. The property is correctly marked as optional, which is appropriate for a configuration option.
Line range hint
111-170: Summary: New feature for tracking page lifecycle events implemented successfully.The changes in this file successfully implement the new feature for tracking page lifecycle events, as described in the PR objectives. The additions include:
- A new
PageLifecycleEventsenum- A new
PageLifecycleOptionstype- A new
AutoTrackOptionstype- A new
autoTrackproperty in theLoadOptionstypeThese changes enhance the functionality of the SDK by allowing for configurable tracking of page lifecycle events. The implementation is well-structured, consistent with the existing codebase, and doesn't introduce breaking changes to existing code.
A few minor suggestions have been made for improving consistency in comments, but overall, the implementation looks good and achieves the stated objectives of the PR.
packages/analytics-js-common/src/types/ApplicationState.ts (2)
86-88: LGTM: NewAutoTrackStatetype looks good.The
AutoTrackStatetype is well-defined and aligns with the PR objectives of tracking page lifecycle events. It follows the existing naming conventions and structure in the file.
203-203: LGTM:ApplicationStateinterface updated correctly.The addition of
autoTrack: AutoTrackStateto theApplicationStateinterface is correct and integrates the new auto-tracking functionality into the main application state.To ensure that this change is properly utilized, let's verify its usage across the codebase:
This will help ensure that the new state is being properly utilized and initialized in the application.
✅ Verification successful
Apologies for the issues with the previous verification scripts. Let's re-run the verification with corrected commands to ensure proper usage of the
autoTrackstate:
Verified:
autoTrackis properly integrated throughout the codebase.The addition of
autoTrack: AutoTrackStateto theApplicationStateinterface has been thoroughly verified. TheautoTrackstate is correctly utilized and initialized across relevant modules, including state slices, user session management, and corresponding tests.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of the new autoTrack state in the codebase # Search for autoTrack usage in TypeScript files echo "Searching for autoTrack usage:" rg --type typescript 'autoTrack' -C 3 # Search for PageLifecycleState usage echo "\nSearching for PageLifecycleState usage:" rg --type typescript 'PageLifecycleState' -C 3 # Search for potential initialization of autoTrack echo "\nSearching for potential initialization of autoTrack:" rg --type typescript 'autoTrack\s*:\s*{' -C 3Length of output: 583
Script:
#!/bin/bash # Description: Check for usage of the new autoTrack state in the codebase # Search for autoTrack usage in TypeScript (.ts) files echo "Searching for autoTrack usage:" rg --type ts 'autoTrack' -C 3 # Search for PageLifecycleState usage in TypeScript (.ts) files echo "\nSearching for PageLifecycleState usage:" rg --type ts 'PageLifecycleState' -C 3 # Search for potential initialization of autoTrack with corrected regex echo "\nSearching for potential initialization of autoTrack:" rg --type ts 'autoTrack\s*:\s*\{' -C 3Length of output: 47699
packages/analytics-js/src/app/RudderAnalytics.ts (5)
12-16: LGTM: Import statements updated appropriately.The new import statements are correctly added to support the new page lifecycle tracking functionality. The imports include necessary types, utilities, and constants required for the implementation.
Also applies to: 23-24, 35-39, 41-41
92-94: LGTM: Constructor updated to initialize page lifecycle tracking.The constructor now initializes
visitIdandpageLoadedTimestampin the state. This is a good practice as it ensures these values are set as soon as the RudderAnalytics instance is created, providing accurate data for page lifecycle tracking.
143-145: LGTM: Page lifecycle tracking initiated in load method.The
trackPageLifecycleEventsmethod is now called within theloadmethod. This ensures that page lifecycle tracking is initiated when the SDK is loaded, which is the appropriate time to start this tracking.
150-159: LGTM: New method added to safely retrieve preloaded events.The
getPreloadedEventsmethod is a good addition. It safely retrieves the preloaded events array from the global object, with a fallback to an empty array. This encapsulation improves code organization and reusability.
236-236: LGTM: Consistent use ofgetPreloadedEventsmethod.The
triggerBufferedLoadEventmethod now uses the newly addedgetPreloadedEvents()method instead of directly accessing the global object. This change improves code consistency and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/analytics-js/__tests__/app/RudderAnalytics.test.ts (2)
50-58: Ensure 'RudderStackGlobals' is defined or use optional chainingIn the assertion at line 51, you're accessing
RudderStackGlobals.app.preloadedEventsBufferwithout verifying ifRudderStackGlobalsorappis defined. This could lead to a runtime error ifRudderStackGlobalsis undefined. Consider using optional chaining or ensuringRudderStackGlobalsis properly initialized in the test setup.Suggested fix:
- expect((window as any).RudderStackGlobals.app.preloadedEventsBuffer).toEqual([ + expect(window.RudderStackGlobals?.app?.preloadedEventsBuffer).toEqual([
381-401: Review necessity of skipping the 'Page Unloaded' event testThe test
'should track Page Unloaded event if useBeacon is set to true and trackPageLifecycle feature is enabled'is currently skipped usingit.skip. If this functionality is important, consider enabling the test or documenting the reason for skipping, to ensure the 'Page Unloaded' event is properly tested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/analytics-js/.size-limit.mjs (5 hunks)
- packages/analytics-js/tests/app/RudderAnalytics.test.ts (3 hunks)
- packages/analytics-js/src/app/RudderAnalytics.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js/.size-limit.mjs
🧰 Additional context used
📓 Learnings (2)
packages/analytics-js/__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-10-09T06:41:05.073Z 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/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-10-09T06:41:05.073Z 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/src/app/RudderAnalytics.ts
[warning] 232-232: packages/analytics-js/src/app/RudderAnalytics.ts#L232
Added line #L232 was not covered by tests
[warning] 245-245: packages/analytics-js/src/app/RudderAnalytics.ts#L245
Added line #L245 was not covered by tests
[warning] 247-247: packages/analytics-js/src/app/RudderAnalytics.ts#L247
Added line #L247 was not covered by tests
[warning] 249-249: packages/analytics-js/src/app/RudderAnalytics.ts#L249
Added line #L249 was not covered by tests
[warning] 251-251: packages/analytics-js/src/app/RudderAnalytics.ts#L251
Added line #L251 was not covered by tests
🔇 Additional comments (7)
packages/analytics-js/src/app/RudderAnalytics.ts (7)
12-16: LGTM: Import statements updated appropriately.The new import statements are consistent with the added functionality for page lifecycle tracking. They provide necessary types, utilities, and constants required for the new features.
Also applies to: 23-24, 35-39, 41-41
92-93: LGTM: Constructor updated to initialize page lifecycle state.The additions to the constructor are appropriate for the new page lifecycle tracking feature. Initializing
visitIdwith a UUID andpageLoadedTimestampwith the current time ensures that each page load is uniquely identifiable from the start of the SDK's lifecycle.
143-144: LGTM: Page lifecycle tracking initiated inloadmethod.The addition of
this.trackPageLifecycleEvents(loadOptions)in theloadmethod is appropriate. It ensures that page lifecycle events are set up for tracking when the SDK is loaded, before the analytics instance is created and loaded.
151-159: LGTM:getPreloadedEventsmethod added.The
getPreloadedEventsmethod is a good addition. It provides a safe way to access preloaded events from the global object, handling cases where the expected data might not be present. Encapsulating this logic in a separate method improves code organization and reusability.
161-191: LGTM:trackPageLifecycleEventsmethod implemented correctly.The
trackPageLifecycleEventsmethod is well-implemented. It effectively orchestrates the page lifecycle tracking feature by:
- Extracting relevant options from
loadOptionswith appropriate default values.- Calling
trackPageLoadedEventandsetupPageUnloadTrackingwhen enabled.- Updating the global preload buffer.
The use of optional chaining and default values is good for handling potentially undefined options.
193-217: LGTM:trackPageLoadedEventmethod implemented correctly.The
trackPageLoadedEventmethod is well-implemented. It correctly:
- Checks if the LOADED event should be tracked based on the
eventsarray.- Adds the event to the preloaded events array with the correct structure.
- Includes relevant data such as
visitIdandoriginalTimestamp.The use of
unshiftensures that the page loaded event is added to the beginning of the array, which is appropriate for maintaining the correct order of events.
219-264: LGTM: Page unload tracking implemented correctly, but needs test coverage.The
setupPageUnloadTrackingandregisterPageUnloadListenermethods are well-implemented:
- They correctly check if the unload event should be tracked and if the beacon API is enabled.
- A warning is logged if beacon is disabled, which is good for debugging.
- The unload event is tracked with the correct data, including visit duration.
However, the static analysis hints indicate that these methods lack test coverage.
Please add unit tests to cover the following scenarios:
- Setting up page unload tracking with beacon enabled and disabled.
- Registering the page unload listener and verifying that it's called correctly.
- Tracking the unload event with various page states (accessible/not accessible, SDK loaded/not loaded).
Here's a script to verify the current test coverage:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 232-232: packages/analytics-js/src/app/RudderAnalytics.ts#L232
Added line #L232 was not covered by tests
[warning] 245-245: packages/analytics-js/src/app/RudderAnalytics.ts#L245
Added line #L245 was not covered by tests
[warning] 247-247: packages/analytics-js/src/app/RudderAnalytics.ts#L247
Added line #L247 was not covered by tests
[warning] 249-249: packages/analytics-js/src/app/RudderAnalytics.ts#L249
Added line #L249 was not covered by tests
[warning] 251-251: packages/analytics-js/src/app/RudderAnalytics.ts#L251
Added line #L251 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
packages/analytics-js/src/state/slices/autoTrack.ts (1)
4-11: LGTM: Well-structuredautoTrackStateobject with room for improvement.The
autoTrackStateobject is well-defined:
- It uses
signal()for reactive state management.- Initial values are appropriate.
- The structure allows for easy expansion of auto-tracking features.
Consider adding JSDoc comments to describe the purpose of each property, especially
visitIdandpageLoadedTimestamp. This would improve code readability and maintainability. For example:const autoTrackState: AutoTrackState = { enabled: signal(false), pageLifecycle: { enabled: signal(false), /** Unique identifier for the current page visit */ visitId: signal(undefined), /** Timestamp when the page was loaded */ pageLoadedTimestamp: signal(undefined), }, };packages/analytics-js-common/src/types/ApplicationState.ts (2)
86-89: LGTM! Consider adding a comment for clarity.The
AutoTrackStatetype definition looks good. It appropriately uses theSignaltype for reactive state management and includes the necessary properties for auto-tracking functionality.Consider adding a brief comment above the type definition to explain its purpose and usage:
/** * Represents the state of auto-tracking features, including page lifecycle tracking. */ export type AutoTrackState = { // ... (existing code) };
91-95: LGTM! Consider adding a comment for clarity.The
PageLifecycleStatetype definition is well-structured and uses appropriate types for each property. The use ofSignaltypes ensures reactive state management, which is consistent with the rest of the file.Consider adding a brief comment above the type definition to explain its purpose and usage:
/** * Represents the state of page lifecycle tracking, including visit ID and page load timestamp. */ export type PageLifecycleState = { // ... (existing code) };packages/analytics-js/__tests__/app/RudderAnalytics.test.ts (1)
325-408: LGTM: Comprehensive tests for trackPageLifecycleEventsThese tests provide excellent coverage for the
trackPageLifecycleEventsfunctionality. They effectively verify:
- Behavior when tracking is not enabled
- Inheritance of options from autoTrack
- Overriding of autoTrack options
- Tracking of Page Loaded event regardless of useBeacon option
- Tracking of Page Unloaded event with useBeacon true
The use of
jest.fn()for mocking and the simulation of thebeforeunloadevent are good practices.Consider adding a test case for when
useBeaconis false to ensure the Page Unloaded event is not tracked in this scenario. This would provide complete coverage of theuseBeaconoption's impact.packages/analytics-js/src/app/RudderAnalytics.ts (4)
143-151: LGTM with a suggestion: Consider using immutable operations.The integration of page lifecycle tracking in the
loadmethod is well-implemented. However, directly mutating thepreloadedEventsArrayinpromotePreloadedConsentEventsToTopmight lead to unexpected side effects. Consider using immutable operations to create a new array instead of modifying the existing one.const updatedEventsArray = promotePreloadedConsentEventsToTop([...preloadedEventsArray]); setExposedGlobal(GLOBAL_PRELOAD_BUFFER, clone(updatedEventsArray));This approach ensures that the original array remains unchanged, which can prevent potential bugs and improve code predictability.
174-198: LGTM with a minor suggestion: Consider destructuring for improved readability.The
trackPageLifecycleEventsmethod is well-implemented and correctly handles both page load and unload events based on the provided options. To improve readability, consider destructuring thepageLifecycleobject directly in the parameter list:trackPageLifecycleEvents( preloadedEventsArray: PreloadedEventCall[], { autoTrack, useBeacon }: Partial<LoadOptions> = {}, ) { const { enabled: autoTrackEnabled = false, options: autoTrackOptions = {}, pageLifecycle: { events = [PageLifecycleEvents.LOADED, PageLifecycleEvents.UNLOADED], enabled = autoTrackEnabled, options = autoTrackOptions, } = {}, } = autoTrack ?? {}; // ... rest of the method }This approach reduces nesting and makes the default values more explicit.
233-246: LGTM with a suggestion: Consider renaming the method for clarity.The
setupPageUnloadTrackingmethod is well-implemented. It correctly sets up the page unload tracking based on the provided configuration and logs a warning if beacon is disabled.However, to improve clarity, consider renaming this method to something more descriptive of its actual functionality, such as
configurePageUnloadTrackingorinitializePageUnloadTracking. This would better reflect that the method is not just setting up tracking, but also handling configuration checks and warnings.
252-272: LGTM with a suggestion: Add error handling for the tracking call.The
registerPageUnloadListenermethod is well-implemented. It correctly sets up the listener for page unload events, calculates the visit duration, and tracks the unload event using the necessary information from the global state.However, to improve robustness, consider adding error handling for the
this.trackcall. This will ensure that any exceptions during the tracking process don't break the page unload flow. For example:try { this.track( PageLifecycleEvents.UNLOADED, { visitId: state.autoTrack.pageLifecycle.visitId.value, visitDuration, }, { ...options, originalTimestamp: new Date(pageUnloadedTimestamp).toISOString(), }, ); } catch (error) { this.logger.error('Failed to track page unload event', error); }This addition will help catch and log any errors that occur during the tracking process, improving the overall reliability of the feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/analytics-js-common/src/types/ApplicationState.ts (2 hunks)
- packages/analytics-js/tests/app/RudderAnalytics.test.ts (4 hunks)
- packages/analytics-js/public/index.html (1 hunks)
- packages/analytics-js/src/app/RudderAnalytics.ts (4 hunks)
- packages/analytics-js/src/state/slices/autoTrack.ts (1 hunks)
- packages/analytics-js/src/state/slices/loadOptions.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js/public/index.html
- packages/analytics-js/src/state/slices/loadOptions.ts
🧰 Additional context used
📓 Learnings (2)
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-10-09T06:41:05.073Z 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/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-10-09T06:41:05.073Z Learning: The `trackPageLifecycleEvents` method in `packages/analytics-js/src/app/RudderAnalytics.ts` does not require refactoring as the current implementation is preferred.
🔇 Additional comments (15)
packages/analytics-js/src/state/slices/autoTrack.ts (3)
1-2: LGTM: Imports are appropriate for the file's purpose.The imports are well-chosen:
signalfrom '@preact/signals-core' enables reactive state management.AutoTrackStatetype from the common types ensures type safety.These imports align well with the file's purpose of managing auto-tracking state.
13-13: LGTM: Appropriate export ofautoTrackState.The named export of
autoTrackStateis correct and allows for easy import in other parts of the application.
1-13: Overall implementation looks good, with a suggestion for verification.The
autoTrack.tsfile introduces a well-structured state management system for auto-tracking, aligning with the PR objectives. The use of Preact signals for reactive state management is a good choice for efficient updates.To ensure complete implementation of the page lifecycle tracking feature:
- Verify that there's logic elsewhere in the codebase to update these signals, especially
visitIdandpageLoadedTimestamp.- Check for the presence of functions that calculate time spent on a page using these signals.
- Confirm the existence of event listeners for page load and unload events that interact with this state.
You can use the following script to search for related implementations:
This will help ensure that the auto-tracking state is properly utilized and the feature is fully implemented.
packages/analytics-js-common/src/types/ApplicationState.ts (2)
205-205: LGTM! The ApplicationState interface is correctly updated.The addition of the
autoTrackproperty to theApplicationStateinterface is appropriate and consistent with the newly addedAutoTrackStatetype. This update allows the application to manage the state of auto-tracking features, including page lifecycle tracking.
86-95: Past review suggestion has been addressed.The suggestion from the previous review to add the
enabledstatus for auto tracking and page lifecycle has been successfully implemented in the current code. BothAutoTrackStateandPageLifecycleStateincludeenabledproperties of typeSignal<boolean>, which satisfies the requirement.packages/analytics-js/__tests__/app/RudderAnalytics.test.ts (6)
2-2: LGTM: Import and mock setup look goodThe new import for
PreloadedEventCalland the modification in the mockrudderanalyticsarray are appropriate for the new tests being added. These changes set up the necessary context for testing the new functionality.Also applies to: 24-24
52-60: LGTM: New test for preloaded events retrievalThis test is a valuable addition. It verifies that the
RudderStackGlobals.app.preloadedEventsBufferis correctly populated with the expected preloaded events, which is crucial for the initialization process.
289-294: LGTM: Edge case handling for non-array rudderanalyticsThis test is a valuable addition. It verifies that the
getPreloadedEventsmethod returns an empty array whenglobalThis.rudderanalyticsis not an array. This is good defensive programming and helps prevent potential errors.
296-308: LGTM: Test for correct buffered events retrievalThis test effectively verifies that when
globalThis.rudderanalyticsis an array, thegetPreloadedEventsmethod correctly returns it. This test, along with the previous one, provides good coverage for thegetPreloadedEventsmethod behavior.
311-323: LGTM: Well-structured test suite for trackPageLifecycleEventsThe new describe block for
trackPageLifecycleEventsis well-structured. It includes appropriate setup and teardown methods, ensuring a clean state for each test. This provides a solid foundation for testing this new functionality.
410-422: LGTM: Test for load API integration with trackPageLifecycleEventsThis test effectively verifies that the
trackPageLifecycleEventsmethod is correctly invoked when theloadAPI is called. It's a crucial test to ensure proper initialization of the page lifecycle tracking feature.packages/analytics-js/src/app/RudderAnalytics.ts (4)
12-16: LGTM: Import statements updated appropriately.The new import statements are correctly added to support the new page lifecycle tracking functionality. The imports include necessary types, utilities, and constants required for the implementation.
Also applies to: 23-24, 35-39, 41-41
92-93: LGTM: Constructor updated with necessary initializations.The addition of
visitIdandpageLoadedTimestampinitializations in the constructor is appropriate for the new page lifecycle tracking feature. These values will be used to uniquely identify page visits and record load times.
161-166: LGTM: Well-implemented preloaded events retrieval.The
getPreloadedEventsmethod is correctly implemented. It safely retrieves the preloaded events array from the global object, includes a type check, and provides a fallback to an empty array. This approach ensures robust handling of the preloaded events.
206-225: LGTM: Page load event tracking implemented correctly.The
trackPageLoadedEventmethod is well-implemented. It correctly adds the page loaded event to the beginning of the preloaded events array, using thevisitIdandpageLoadedTimestampfrom the global state. The method also respects theeventsconfiguration, ensuring the event is only added when appropriate.
MoumitaM
dismissed stale reviews from saikumarrs and coderabbitai[bot]
via
6c9a94d
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
README.md (1)
Line range hint
1-1: Consider highlighting the removal of implicitpagecall more prominently.The AI-generated summary mentions the removal of information about the implicit
pagecall at the end of the SDK snippet. This is a significant change that affects how users implement the SDK, especially those upgrading from previous versions. Consider adding a prominent note or callout box near the beginning of the README to ensure users are aware of this change and understand they need to explicitly callrudderanalytics.page()if required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- README.md (2 hunks)
- packages/analytics-js-common/src/utilities/page.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/analytics-js-common/src/utilities/page.ts
[warning] 19-19: packages/analytics-js-common/src/utilities/page.ts#L19
Added line #L19 was not covered by tests
🔇 Additional comments (3)
README.md (3)
81-84: LGTM: Improved naming consistency in ESM example.The change from
rudderanalyticstorudderAnalyticsin the ECMAScript modules example enhances consistency across the documentation. This update aligns with modern JavaScript naming conventions and improves code readability.
95-95: LGTM: Consistent naming in CJS example.The update of the exported variable name from
rudderanalyticstorudderAnalyticsin the CommonJS example maintains consistency with the ESM example. This change ensures uniform naming conventions across different module systems, enhancing the overall documentation clarity.
Line range hint
1-1: Overall documentation improvements with a crucial update.The changes in this README file primarily focus on improving naming consistency across different module systems (ESM and CJS) and aligning with modern JavaScript conventions. These updates enhance the overall clarity and readability of the documentation.
Additionally, the removal of information about the implicit
pagecall is a significant change that affects SDK implementation. While this update is mentioned in the AI-generated summary, it's recommended to make it more prominent in the documentation to ensure users, especially those upgrading from previous versions, are well-informed about this change.These updates collectively contribute to a more consistent and informative README for the RudderStack JavaScript SDK.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/analytics-js-common/__tests__/utilities/page.test.ts (1)
186-201: Approve the new test case with suggestions for improvement.The new test case effectively covers an important edge case: the behavior of
onPageLeavewhen the tab is inactive during a page unload. It's a valuable addition to the test suite.However, consider the following improvements:
- Use
jest.useFakeTimers()andjest.runAllTimers()instead ofsetTimeoutfor more reliable and synchronous testing.- Explicitly check that the callback is called exactly twice to ensure no unexpected calls occur.
Here's a suggested refactor:
it('should fire the callback on beforeunload event on the next tick even when the tab is inactive', () => { jest.useFakeTimers(); const evCallback = jest.fn(); onPageLeave(evCallback); setVisibilityState('hidden'); dispatchDocumentEvent('visibilitychange'); dispatchWindowEvent('beforeunload'); expect(evCallback).toHaveBeenCalledTimes(1); expect(evCallback).toHaveBeenNthCalledWith(1, true); jest.runAllTimers(); expect(evCallback).toHaveBeenCalledTimes(2); expect(evCallback).toHaveBeenNthCalledWith(2, false); jest.useRealTimers(); });This refactored version provides more reliable timing control and explicitly checks for the exact number of callback invocations.
…hub.com/rudderlabs/rudder-sdk-js into feat/SDK-2478-track-timespent-on-a-page
|
PR Description
New feature:
Linear task (optional)
Linear task link
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
onPageLeavefunction to ensure correct behavior under various conditions.