feat: gainsight PX destination #1852
Conversation
|
Thank you for contributing this PR. |
WalkthroughWalkthroughThe pull request introduces the Gainsight PX integration by adding various constants, mappings, and a new class to facilitate user and event tracking. It includes the definition of essential constants in the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
packages/analytics-js-integrations/src/integrations/GainsightPX/browser.js (1)
51-51: Consider using an optional chain.The current code uses a logical AND operator
&&to check for the presence of thewindow.aptrinsicobject and itsinitproperty. Consider using an optional chain?.instead for a more concise and safer check.-return !!(window.aptrinsic && window.aptrinsic.init); +return !!window.aptrinsic?.init;Tools
Biome
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- packages/analytics-js-common/src/constants/integrations/GainsightPX/constants.ts (1 hunks)
- packages/analytics-js-common/src/constants/integrations/client_server_name.js (1 hunks)
- packages/analytics-js-common/src/constants/integrations/config_to_integration_names.js (1 hunks)
- packages/analytics-js-common/src/constants/integrations/destDisplayNamesToFileNamesMap.ts (2 hunks)
- packages/analytics-js-common/src/constants/integrations/destinationNames.ts (1 hunks)
- packages/analytics-js-common/src/constants/integrations/integration_cname.js (2 hunks)
- packages/analytics-js-integrations/src/integrations/GainsightPX/browser.js (1 hunks)
- packages/analytics-js-integrations/src/integrations/GainsightPX/index.js (1 hunks)
- packages/analytics-js-integrations/src/integrations/GainsightPX/nativeSdkLoader.js (1 hunks)
- packages/analytics-js-integrations/src/integrations/index.js (2 hunks)
Files skipped from review due to trivial changes (1)
- packages/analytics-js-integrations/src/integrations/GainsightPX/index.js
Additional context used
Biome
packages/analytics-js-integrations/src/integrations/GainsightPX/nativeSdkLoader.js
[error] 29-29: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/analytics-js-integrations/src/integrations/GainsightPX/browser.js
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Additional comments not posted (23)
packages/analytics-js-common/src/constants/integrations/GainsightPX/constants.ts (5)
1-1: LGTM!The constant is correctly defined and the value is appropriate for the directory name.
2-2: LGTM!The constant is correctly defined and the value is appropriate for the unique name identifier.
3-3: LGTM!The constant is correctly defined and the value is appropriate for the user-friendly display name.
5-5: LGTM!The constant is correctly defined as an object that maps the display name to the directory name. This mapping is useful for easier reference within the codebase.
6-10: LGTM!The constant is correctly defined as an object that maps various representations of the integration's name to the standardized name identifier. This mapping ensures consistency and helps avoid discrepancies when referring to the Gainsight PX integration across the application.
packages/analytics-js-integrations/src/integrations/GainsightPX/nativeSdkLoader.js (1)
7-38: LGTM!The
loadNativeSdkfunction is well-structured and follows best practices for dynamically loading the Gainsight PX native SDK. It correctly determines the appropriate hostname based on the data center, constructs the SDK URL, parses the optional JSON configuration, creates and configures the script element, and inserts it into the document to initiate the SDK loading process. The closure usage for defining the SDK's global namespace and queue is a valid approach.The code is clean, readable, and does not introduce any apparent issues or vulnerabilities. The suspicious assignment in an expression at line 29, as indicated by the static analysis hint, is a common pattern used in JavaScript for pushing arguments into an array and is not a cause for concern in this context.
Great job with the implementation!
Tools
Biome
[error] 29-29: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/analytics-js-common/src/constants/integrations/config_to_integration_names.js (1)
83-83: LGTM!The addition of the new entry
GAINSIGHT_PX_BROWSER: 'GainsightPX'to theconfigToIntNamesconstant is consistent with the existing pattern and expands the mapping to include the new Gainsight PX integration. The change is self-contained and does not introduce any apparent issues.packages/analytics-js-common/src/constants/integrations/client_server_name.js (1)
82-82: LGTM!The addition of the new entry
GAINSIGHT_PX_BROWSER: 'GainsightPX'to theclientToServerNamesobject is consistent with the existing pattern and expands the mapping of client identifiers to their corresponding server names. This change allows the analytics system to recognize and process data from the Gainsight PX browser client.packages/analytics-js-integrations/src/integrations/GainsightPX/browser.js (8)
12-27: LGTM!The
GainsightPXclass definition and constructor look good. The class properties are correctly initialized based on the provided configuration, and the logging setup is properly handled based on the analytics log level.
29-32: LGTM!The
initmethod correctly loads the native SDK using the provided configuration and calls theinitializeMemethod in the correct sequence.
34-48: LGTM!The
initializeMemethod correctly identifies the user if a user ID is present. It constructs the visitor and account objects using the appropriate IDs and traits, and calls thewindow.aptrinsicfunction with the correct arguments to send the identification information to the Gainsight PX service.
50-52: LGTM!The
isLoadedmethod correctly checks if the SDK is loaded and ready for use by verifying the presence of thewindow.aptrinsicobject and itsinitproperty. The double negation!!is used appropriately to convert the result to a boolean value.Tools
Biome
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
54-56: LGTM!The
isReadymethod correctly determines if the SDK is ready by calling theisLoadedmethod. This is a valid approach as the readiness of the SDK depends on whether it is loaded.
64-84: LGTM!The
identifymethod correctly constructs visitor and account objects from the provided data. It extracts the necessary information from therudderElementobject, such as user ID, group ID, and traits. The method appropriately checks for the presence of a user ID before calling thewindow.aptrinsicfunction to identify the user with the constructed visitor and account objects.
90-109: LGTM!The
groupmethod correctly constructs account and visitor objects from the provided data. It extracts the necessary information from therudderElementobject, such as user ID, group ID, anonymous ID, and traits. The method appropriately constructs the account object using the group ID or anonymous ID and traits, and the visitor object using the user ID and traits, if a user ID is present. Thewindow.aptrinsicfunction is called with the constructed objects to identify the user.
112-120: LGTM!The
trackmethod correctly sends custom events to the Gainsight PX service. It extracts the event name and properties from therudderElementobject and checks for the presence of an event name before proceeding. If the event name is missing, it appropriately logs an error and returns. Thewindow.aptrinsicfunction is called with the correct arguments to track the custom event.packages/analytics-js-integrations/src/integrations/index.js (2)
80-80: LGTM!The import statement is syntactically correct and necessary to use the
GainsightPXintegration in this file.
164-164: Looks good!The addition of the
GainsightPXintegration to theintegrationsobject is syntactically correct and follows the established naming convention. The key nameGAINSIGHT_PX_BROWSERis consistent with the PR summary's objective to prevent conflicts with the existing Gainsight PX integration.packages/analytics-js-common/src/constants/integrations/integration_cname.js (2)
81-81: LGTM!The import statement is syntactically correct and consistent with the existing import statements in the file. It is necessary to include the
GainsightPXintegration mapping in thecommonNamesobject.
166-166: LGTM!The spread syntax is syntactically correct and consistent with the existing spreads in the
commonNamesobject. It is necessary to include theGainsightPXintegration mapping in thecommonNamesobject.packages/analytics-js-common/src/constants/integrations/destDisplayNamesToFileNamesMap.ts (2)
161-162: LGTM!The addition of the
GainsightPXDisplayNameandGainsightPXDirectoryNameconstants is consistent with the existing pattern and enables the inclusion of the Gainsight PX integration in the mapping.
246-246: LGTM!The addition of the
GainsightPXDisplayNametoGainsightPXDirectoryNamemapping is consistent with the existing pattern and allows the Gainsight PX integration to be correctly referenced and linked.packages/analytics-js-common/src/constants/integrations/destinationNames.ts (1)
293-296: LGTM!The changes add new exports for the Gainsight PX integration, following the existing pattern in the file. The additions are consistent and expand the integration capabilities within the analytics framework.
PR Description
The intention is to allow customers to load the Gainsight PX javascript SDK in the browser and send events directly to Gainsight PX from the client side. This new integration will likely be used more than the existing home-grown Gainsight PX integration that uses the REST API to send events from server-to-server.
To avoid conflicts with the existing Gainsight PX integration, I used the name of "GAINSIGHT_PX_BROWSER".
There are corresponding PR's to the rudder-integrations-config and docs repos.
Docs: rudderlabs/rudderstack-docs#1193
UI: rudderlabs/rudder-integrations-config#1684
NOTE: I have not tested this in concert with the rudder-integrations-config change, so I am not sure about the "extraConfig" config field. In the integrations-config code, I marked it as type:"textareaInput" and subType: "JSON", and in the sdk-js code, I consumed that as a string. If it will come in as a javascript object instead, then I should change the code. Let me know how that subtype works. (or if that is not even a valid choice, let me know, I can get rid of the subType)
Linear task (optional)
n/a
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
Bug Fixes
Documentation