feat: add session replay for mixpanel #1899
Conversation
|
Warning Rate limit exceeded@shrouti1507 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 28 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the Mixpanel integration within the analytics-js library. A new property, Changes
Possibly related PRs
Suggested reviewers
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 (4)
packages/analytics-js-integrations/src/integrations/Mixpanel/util.js (1)
276-285: LGTM: Well-implementedgetDestinationOptionsfunction with clear documentation.The new
getDestinationOptionsfunction is well-implemented and documented. It provides a clean way to retrieve destination-specific options with backward compatibility.Consider adding null safety to the return statement for extra robustness:
-const getDestinationOptions = integrationsOptions => - integrationsOptions && (integrationsOptions[DISPLAY_NAME] || integrationsOptions[NAME]); +const getDestinationOptions = integrationsOptions => + integrationsOptions ? (integrationsOptions[DISPLAY_NAME] || integrationsOptions[NAME]) : undefined;This change ensures that
undefinedis explicitly returned whenintegrationsOptionsis falsy, maintaining consistent behavior across all scenarios.packages/analytics-js-integrations/src/integrations/Mixpanel/browser.js (3)
70-70: LGTM: New property for session replay configuration.The
sessionReplayPercentageproperty is correctly added to support the new session replay feature. Consider adding a default value or type checking for robustness.Consider adding a default value or type checking:
- this.sessionReplayPercentage = config.sessionReplayPercentage; + this.sessionReplayPercentage = config.sessionReplayPercentage || 0; // Default to 0% if not specified
Line range hint
77-114: LGTM with suggestions: Session replay configuration added.The changes correctly implement the session replay configuration. However, consider the following improvements:
- Instead of changing
optionstolet, consider creating a separate object for session replay config and then merging it withoptions.- The
sessionReplayConfigobject could be moved to a separate method for better readability.- Consider adding null checks for
mixpanelIntgConfigproperties.Here's a suggested refactoring:
init() { // ... existing code ... const options = { cross_subdomain_cookie: this.crossSubdomainCookie || false, secure_cookie: this.secureCookie || false, // ... other existing options ... }; const mixpanelIntgConfig = getDestinationOptions(this.analytics.loadOnlyIntegrations); if (mixpanelIntgConfig) { const sessionReplayConfig = this.getSessionReplayConfig(mixpanelIntgConfig); Object.assign(options, removeUndefinedAndNullValues(sessionReplayConfig)); } // ... rest of the method ... } getSessionReplayConfig(mixpanelIntgConfig) { return { record_sessions_percent: this.sessionReplayPercentage, record_block_class: mixpanelIntgConfig?.recordBlockClass, record_collect_fonts: mixpanelIntgConfig?.recordCollectFonts, record_idle_timeout_ms: mixpanelIntgConfig?.recordIdleTimeout, record_mask_text_class: mixpanelIntgConfig?.recordMaskTextClass, record_mask_text_selector: mixpanelIntgConfig?.recordMaskTextSelector, record_max_ms: mixpanelIntgConfig?.recordMaxMs, record_min_ms: mixpanelIntgConfig?.recordMinMs }; }This refactoring improves readability and maintainability of the code.
Line range hint
1-424: Suggestion: Add tests and documentation for session replay feature.While the implementation looks correct, it's important to ensure that the new session replay functionality is properly tested and documented. Consider the following suggestions:
- Add unit tests for the new
getSessionReplayConfigmethod (if implemented as suggested in the previous comment).- Update existing tests for the
initmethod to cover the new session replay configuration.- Add documentation comments explaining the purpose and usage of the new
sessionReplayPercentageproperty and how it affects the Mixpanel integration.- Consider adding a section in the class-level documentation about the session replay feature and how to configure it.
Would you like assistance in drafting test cases or documentation for the new session replay feature?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/analytics-js-integrations/src/integrations/Mixpanel/browser.js (3 hunks)
- packages/analytics-js-integrations/src/integrations/Mixpanel/util.js (3 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/analytics-js-integrations/src/integrations/Mixpanel/util.js (3)
5-5: LGTM: Import statement updated correctly.The addition of the
NAMEconstant to the import statement is consistent with its usage in the newgetDestinationOptionsfunction. This change maintains good module organization.
300-300: LGTM: Export statement updated correctly.The addition of
getDestinationOptionsto the export statement is appropriate, making the new utility function available for use in other parts of the application.
Line range hint
1-301: Overall assessment: Well-implemented changes enhancing Mixpanel integration options retrieval.The changes to this file are well-structured and purposeful. They introduce a new utility function
getDestinationOptionsthat enhances the flexibility of retrieving Mixpanel integration options. The implementation is backward-compatible and well-documented. The minor suggestion for null safety in thegetDestinationOptionsfunction is the only point for potential improvement.To ensure the changes don't introduce any unintended side effects, please run the following verification script:
This script will help identify any areas in the codebase that might need to be updated to use the new
getDestinationOptionsfunction, as well as any existing error handling that might need to be adjusted.packages/analytics-js-integrations/src/integrations/Mixpanel/browser.js (1)
19-19: LGTM: New import for session replay functionality.The addition of
getDestinationOptionsimport is consistent with the new session replay feature being implemented.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1899 +/- ##
===========================================
+ Coverage 57.34% 57.37% +0.03%
===========================================
Files 482 482
Lines 16417 16430 +13
Branches 3294 3274 -20
===========================================
+ Hits 9414 9427 +13
- Misses 5725 5767 +42
+ Partials 1278 1236 -42 ☔ View full report in Codecov by Sentry. |
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/analytics-js-integrations/src/integrations/Mixpanel/browser.js (2)
75-75: LGTM: NewsessionReplayPercentageproperty added.The
sessionReplayPercentageproperty is correctly initialized from the config object. Consider adding a comment or updating the class documentation to explain the purpose and expected values of this new property.
Line range hint
82-129: LGTM: Session replay configuration added with proper validation.The implementation of session replay configuration is well-structured and includes appropriate validation for the
sessionReplayPercentage. The use ofremoveUndefinedAndNullValuesensures clean configuration objects.Consider extracting the session replay configuration logic into a separate method for improved readability and maintainability.
packages/analytics-js-integrations/__tests__/integrations/Mixpanel/browser.test.js (1)
182-215: EnsureconsoleSpyis properly restored even if the test failsCurrently,
consoleSpy.mockRestore()is called at the end of the test. If an assertion fails before this call, the mock may not be restored, potentially affecting other tests. To prevent this, wrap the test code in atry...finallyblock to ensureconsoleSpyis restored regardless of test outcomes.Apply this diff to adjust the test:
test('Session replay configuration with invalid percentage', () => { const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + try { // Existing test code... mixpanel.init(); expect(window.mixpanel._i[0][1]).toEqual({ cross_subdomain_cookie: false, secure_cookie: false, persistence: 'localStorage', persistence_name: 'test', loaded: expect.any(Function), }); expect(consoleSpy).toHaveBeenCalledWith( '%c RS SDK - Mixpanel %c Invalid sessionReplayPercentage: 101. It should be a string matching the pattern "^(100|[1-9]?[0-9])$"', 'font-weight: bold; background: black; color: white;', 'font-weight: normal;', ); } + finally { consoleSpy.mockRestore(); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/analytics-js-integrations/tests/integrations/Mixpanel/browser.test.js (1 hunks)
- packages/analytics-js-integrations/src/integrations/Mixpanel/browser.js (4 hunks)
🧰 Additional context used
🔇 Additional comments (5)
packages/analytics-js-integrations/src/integrations/Mixpanel/browser.js (2)
8-13: LGTM: New utility imports added.The additional imports from
commonUtilsare appropriate for the new functionality being implemented.
Line range hint
1-424: Overall: Well-implemented session replay functionality.The changes to add session replay for Mixpanel are well-structured and properly integrated into the existing
Mixpanelclass. The implementation includes appropriate configuration handling and validation.To ensure the reliability of these changes:
- Add unit tests for the new session replay configuration logic.
- Perform integration tests to verify the correct interaction with the Mixpanel SDK.
- Conduct cross-browser testing as mentioned in the PR objectives.
Run the following script to check for existing tests related to the new functionality:
packages/analytics-js-integrations/__tests__/integrations/Mixpanel/browser.test.js (3)
80-122: Tests for session replay configuration are comprehensive and well-structuredThe added test thoroughly covers the session replay configuration with all options provided, ensuring that the integration behaves as expected.
124-154: Partial session replay options are correctly validatedThe test accurately checks the behavior when only partial session replay options are provided, confirming that defaults and optional parameters are handled appropriately.
156-181: Handling of missingloadOnlyIntegrationsis properly testedThis test ensures that the integration correctly processes the session replay configuration even when
loadOnlyIntegrationsis not specified, enhancing the robustness of the code.
packages/analytics-js-integrations/__tests__/integrations/Mixpanel/browser.test.js
Outdated
Show resolved
Hide resolved
packages/analytics-js-integrations/__tests__/integrations/Mixpanel/browser.test.js
Show resolved
Hide resolved
packages/analytics-js-integrations/src/integrations/Mixpanel/browser.js
Outdated
Show resolved
Hide resolved
|
PR Description
Adding session replay feature for mixpanel.
Ref: https://docs.mixpanel.com/docs/tracking-methods/sdks/javascript#session-replay
Linear task (optional)
Linear task link
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
sessionReplayPercentageproperty to enhance session replay capabilities in the Mixpanel integration.getDestinationOptions, for improved retrieval of destination-specific options, ensuring backward compatibility.Tests