Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: set server side cookies #1649

Merged

Conversation

MoumitaM
Copy link
Contributor

@MoumitaM MoumitaM commented Mar 18, 2024

PR Description

  • added new load option to enable setting cookie from server side
  • added new load option to take the base URL to set the cookie to if not same as dataplane url
  • If the option is enabled SDK will make a POST request to the API gateway with the cookie name value and settings.
  • Whenever a cookie is modified cookie setter method will be called

Linear task (optional)

https://linear.app/rudderstack/issue/SDK-1301/create-a-cookie-setter-provider-for-service

Cross Browser Tests

Please confirm you have tested for the following browsers:

  • Chrome
  • Firefox
  • IE11

Sanity Suite

  • All sanity suite test cases pass locally

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

Summary by CodeRabbit

  • New Features

    • Added support for server-side cookies management.
    • Introduced new utility functions for URL validation and data service URL construction.
  • Enhancements

    • Updated the UserSessionManager with new properties and methods for improved cookie handling and HTTP requests.
    • Enhanced configuration with a new property to enable server-side cookies.
  • Bug Fixes

    • Improved error handling in UserSessionManager with an additional custom message parameter.
  • Documentation

    • Updated configuration examples to include server-side cookies settings.

Copy link
Contributor

coderabbitai bot commented Mar 18, 2024

Walkthrough

Walkthrough

The recent updates to the analytics-js package bring enhancements in URL validation, server-side cookie management, and HTTP client integration. Notable changes include the addition of new utility functions for URL handling, improvements to UserSessionManager for better cookie management, and updates in configuration files to support server-side cookies. Test coverage has also been expanded to accommodate these new functionalities.

Changes

File(s) Change Summary
.../components/configManager/validate.test.ts
.../components/configManager/util/validate.ts
Added getTopDomainUrl and getDataServiceUrl functions for URL handling.
.../components/configManager/ConfigManager.ts Updated exports to include isValidUrl and getDataServiceUrl, removed isValidSourceConfig, and added DEFAULT_DATA_SERVICE_ENDPOINT.
.../components/userSessionManager/UserSessionManager.ts
.../components/userSessionManager/UserSessionManager.test.ts
Introduced new methods, HTTP client integration, error handling enhancements, and import updates for testing.
.../state/slices/serverCookies.ts Added serverSideCookiesState for managing server-side cookies state.
.../public/index.html Updated configuration to include useServerSideCookies: true.

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 86.36364% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 54.81%. Comparing base (e7e27be) to head (b0e1484).

Files Patch % Lines
...s-js/src/components/configManager/ConfigManager.ts 46.15% 6 Missing and 1 partial ⚠️
...alytics-js/src/components/utilities/loadOptions.ts 25.00% 1 Missing and 2 partials ⚠️
...omponents/userSessionManager/UserSessionManager.ts 96.15% 1 Missing and 1 partial ⚠️
...mon/src/constants/integrations/Criteo/constants.ts 0.00% 1 Missing ⚠️
...js-integrations/src/integrations/Criteo/browser.js 0.00% 1 Missing ⚠️
packages/analytics-js/src/constants/logMessages.ts 92.85% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1649      +/-   ##
===========================================
+ Coverage    54.60%   54.81%   +0.20%     
===========================================
  Files          463      464       +1     
  Lines        15701    15797      +96     
  Branches      3109     3155      +46     
===========================================
+ Hits          8574     8659      +85     
+ Misses        5854     5825      -29     
- Partials      1273     1313      +40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 14797b5 and 6a681c1.
Files selected for processing (9)
  • packages/analytics-js-common/src/types/LoadOptions.ts (1 hunks)
  • packages/analytics-js/fixtures/msw.handlers.ts (1 hunks)
  • packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts (3 hunks)
  • packages/analytics-js/public/index.html (1 hunks)
  • packages/analytics-js/src/components/core/Analytics.ts (1 hunks)
  • packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (4 hunks)
  • packages/analytics-js/src/components/utilities/loadOptions.ts (1 hunks)
  • packages/analytics-js/src/services/StoreManager/storages/CookieStorage.ts (2 hunks)
  • packages/analytics-js/src/state/slices/loadOptions.ts (1 hunks)
Additional comments: 12
packages/analytics-js/src/state/slices/loadOptions.ts (1)
  • 40-40: The addition of the useServerSideCookie property with a default value of false is correctly implemented. This allows for an opt-in approach to using server-side cookie setting capabilities, aligning with the PR's objectives.
packages/analytics-js/__fixtures__/msw.handlers.ts (1)
  • 80-87: The addition of the new HTTP POST handler for setting a cookie at ${dummyDataplaneHost}/setCookie is correctly implemented. It simulates server-side cookie setting in a testing environment with appropriate response configuration.
packages/analytics-js/src/services/StoreManager/storages/CookieStorage.ts (2)
  • 7-7: The addition of the state import from @rudderstack/analytics-js/state is correctly implemented, facilitating access to the global state within the CookieStorage class for managing cookie settings.
  • 43-44: The modifications to the configure method in the CookieStorage class, including the deletion of the enabled property and the assignment of this.options to state.storage.cookie.value, are correctly implemented. These changes enhance cookie management by aligning with the application's state management strategy.
packages/analytics-js-common/src/types/LoadOptions.ts (1)
  • 150-151: The addition of the useServerSideCookie and cookieServerUrl fields to the LoadOptions type is correctly implemented. These fields enhance cookie management capabilities by enabling server-side cookie setting and specifying a custom base URL for the cookie.
packages/analytics-js/src/components/utilities/loadOptions.ts (1)
  • 60-61: The modification to the normalizeLoadOptions function, adding a check to normalize the useServerSideCookie option value to a boolean, is correctly implemented. This ensures that the option is correctly interpreted and used within the application.
packages/analytics-js/public/index.html (2)
  • 82-82: The pluginsSDKBaseURL declaration is commented out in the loadOptions object. Could you clarify the reasoning behind this change? It's important to ensure that example configurations in public-facing files are clear and intentional.
  • 83-83: The useServerSideCookie declaration is added but commented out in the loadOptions object. It would be helpful to include a comment explaining the purpose and usage of this option, especially since it's a significant feature introduced by this PR.
packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (3)
  • 65-79: The addition of the httpClient property and its initialization in the constructor are correctly implemented. This change enables the UserSessionManager class to make HTTP requests, which is necessary for the new server-side cookie setting functionality.
  • 270-288: The setServerSideCookie method is correctly implemented to make an external request for setting cookies from the server side. However, it's important to ensure that the server endpoint (${baseUrl}/setCookie) properly handles the POST request and securely processes the cookie data.
  • 300-318: The modifications to the syncValueToStorage method to support server-side cookie setting are correctly implemented. When useServerSideCookie is true and the storage type is cookieStorage, the method now correctly calls setServerSideCookie with the encrypted cookie value. This change aligns with the PR objectives for enhanced cookie management.
packages/analytics-js/src/components/core/Analytics.ts (1)
  • 245-245: The addition of the httpClient dependency to the Analytics class constructor is correctly implemented. This change enhances the class's functionality by introducing a new component for handling HTTP requests, which is necessary for the new server-side cookie setting functionality and other HTTP-based interactions.

Copy link

github-actions bot commented Mar 18, 2024

size-limit report 📦

Name Size (Base) Size (Current) Size Limit Status
Common Code - No bundling 15.82 KB 15.82 KB (0%) 16.5 KB
Remote Module Federation Mapping - CDN 330 B 330 B (0%) 512 B
Remote Module Federated Plugins - CDN 5.69 KB 5.69 KB (0%) 6 KB
Core ESM - NPM 7.61 KB 7.64 KB (+0.47% ▲) 8 KB
Core CJS - NPM 22.79 KB 23.42 KB (+2.78% ▲) 23.7 KB
Core - NPM 22.9 KB 23.55 KB (+2.85% ▲) 23.7 KB
Core Legacy - CDN 43.33 KB 44.1 KB (+1.77% ▲) 44.2 KB
Core - CDN 23.1 KB 23.74 KB (+2.77% ▲) 24 KB
Core (legacy build) - CDN - v1.1 31.45 KB 31.45 KB (0%) 32 KB
Core - NPM - v1.1 31.48 KB 31.48 KB (0%) 32 KB
Service Worker Module 23.15 KB 23.15 KB (0%) 24 KB
All Integrations (legacy build) - CDN 98.88 KB 98.88 KB (0%) 105 KB

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 6a681c1 and 7b90beb.
Files selected for processing (11)
  • packages/analytics-js-common/src/types/LoadOptions.ts (1 hunks)
  • packages/analytics-js/.size-limit.js (1 hunks)
  • packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts (3 hunks)
  • packages/analytics-js/public/index.html (1 hunks)
  • packages/analytics-js/src/components/configManager/ConfigManager.ts (2 hunks)
  • packages/analytics-js/src/components/configManager/util/validate.ts (2 hunks)
  • packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (6 hunks)
  • packages/analytics-js/src/components/utilities/loadOptions.ts (1 hunks)
  • packages/analytics-js/src/constants/logMessages.ts (2 hunks)
  • packages/analytics-js/src/services/StoreManager/storages/storageEngine.ts (2 hunks)
  • packages/analytics-js/src/state/slices/loadOptions.ts (1 hunks)
Files skipped from review as they are similar to previous changes (6)
  • packages/analytics-js-common/src/types/LoadOptions.ts
  • packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts
  • packages/analytics-js/public/index.html
  • packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts
  • packages/analytics-js/src/components/utilities/loadOptions.ts
  • packages/analytics-js/src/state/slices/loadOptions.ts
Additional comments: 4
packages/analytics-js/.size-limit.js (1)
  • 29-29: The increase in the size limit for the 'Core - CDN' module to '23.5 KiB' seems justified given the new features introduced. However, it's important to continue monitoring the overall size and performance impact of these changes.
packages/analytics-js/src/services/StoreManager/storages/storageEngine.ts (1)
  • 46-54: The modifications to configureCookieStorageEngine align with the objectives of introducing server-side cookie setting functionality. It's important to ensure that the options passed to CookieStorage are validated and sanitized upstream to prevent potential security issues. Consider adding a comment or documentation to highlight this requirement.
packages/analytics-js/src/components/configManager/ConfigManager.ts (1)
  • 98-104: The usage of validateAndReturnCookieServerUrl within the init method is appropriate for validating the cookieServerUrl. Ensure that the calling code is prepared to handle null or errors as a result of the validation, especially if changes are made to validateAndReturnCookieServerUrl based on previous suggestions.
packages/analytics-js/src/constants/logMessages.ts (1)
  • 94-102: The new error message constants related to server-side cookies functionality are clear and meaningful. Ensure consistent usage of these constants throughout the codebase for effective debugging and error handling.

@MoumitaM MoumitaM requested a review from a team as a code owner March 21, 2024 19:01
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 7b90beb and 91789ef.
Files selected for processing (1)
  • packages/analytics-js/public/index.html (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • packages/analytics-js/public/index.html

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 91789ef and ffa79ee.
Files selected for processing (2)
  • packages/analytics-js-common/src/types/LoadOptions.ts (1 hunks)
  • packages/analytics-js/public/index.html (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • packages/analytics-js-common/src/types/LoadOptions.ts
  • packages/analytics-js/public/index.html

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ffa79ee and a59c8d0.
Files selected for processing (2)
  • packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts (3 hunks)
  • packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (6 hunks)
Files skipped from review as they are similar to previous changes (2)
  • packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts
  • packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts

Copy link

Hello! This PR has been open for 20 days without any activity. Therefore, it's considered as stale and is scheduled to be closed in 10 days. If you're still working on this, please remove the 'Stale' label or add a comment to keep it open. Thanks for your contribution!

@github-actions github-actions bot added Stale and removed Stale labels Apr 17, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

@MoumitaM
Copy link
Contributor Author

All the previous comments are addressed

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Out of diff range and nitpick comments (1)
packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (1)

283-338: Validate the server response status code more robustly in setServerSideCookie.

The method checks if details?.xhr?.status === 200 to determine if the request was successful. However, other 2xx status codes could also indicate success. Consider checking for all successful 2xx responses or use a utility function to check for HTTP success status codes.

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 0c65220 and 0f49281.
Files selected for processing (10)
  • packages/analytics-js-common/src/types/ApplicationState.ts (2 hunks)
  • packages/analytics-js/.size-limit.js (2 hunks)
  • packages/analytics-js/fixtures/msw.handlers.ts (1 hunks)
  • packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts (3 hunks)
  • packages/analytics-js/src/components/configManager/ConfigManager.ts (4 hunks)
  • packages/analytics-js/src/components/configManager/util/validate.ts (2 hunks)
  • packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (5 hunks)
  • packages/analytics-js/src/constants/logMessages.ts (2 hunks)
  • packages/analytics-js/src/state/index.ts (3 hunks)
  • packages/analytics-js/src/state/slices/serverCookies.ts (1 hunks)
Files skipped from review due to trivial changes (1)
  • packages/analytics-js/.size-limit.js
Additional Context Used
Learnings (2)
packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (1)
User: MoumitaM
PR: rudderlabs/rudder-sdk-js#1625
File: packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts:1374-1385
Timestamp: 2024-02-22T10:46:36.393Z
Learning: Multiple cookies with the same name are not possible in web browsers; a newer cookie will overwrite an older one if they share the same name.
packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts (1)
User: MoumitaM
PR: rudderlabs/rudder-sdk-js#1625
File: packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts:1374-1385
Timestamp: 2024-02-22T10:46:36.393Z
Learning: Multiple cookies with the same name are not possible in web browsers; a newer cookie will overwrite an older one if they share the same name.
Additional comments not posted (6)
packages/analytics-js/src/state/index.ts (1)

16-16: Integration of serverSideCookiesState into the main application state is correctly implemented.

packages/analytics-js/__fixtures__/msw.handlers.ts (1)

80-105: The simulation of the server-side cookie setting feature is correctly implemented. Ensure that the cookie settings such as Path, Domain, and Expires are correctly configured as per the application requirements.

Verification successful

The server-side cookie setting feature in the file packages/analytics-js/__fixtures__/msw.handlers.ts is correctly implemented. The cookie settings such as Path, Domain, and Expires are configured as per the application requirements.

  • File: packages/analytics-js/__fixtures__/msw.handlers.ts
Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the cookie settings in the simulated server response.

# Test: Search for the cookie settings in the handler. Expect: Correct configuration of Path, Domain, and Expires.
rg --type js $'Set-Cookie'

Length of output: 25



Script:

#!/bin/bash
# Description: Verify the cookie settings in the specific file `msw.handlers.ts`.

# Test: Search for the cookie settings in the handler within `msw.handlers.ts`. Expect: Correct configuration of Path, Domain, and Expires.
rg --type js $'Set-Cookie' -- 'msw.handlers.ts'

Length of output: 100



Script:

#!/bin/bash
# Description: Locate the file `msw.handlers.ts` in the repository.

# Test: Use fd to find the file `msw.handlers.ts` and print its path.
fd 'msw.handlers.ts'

Length of output: 129



Script:

#!/bin/bash
# Description: Verify the cookie settings in the file located at `packages/analytics-js/__fixtures__/msw.handlers.ts`.

# Test: Search for the cookie settings in the handler within the specified file. Expect: Correct configuration of Path, Domain, and Expires.
rg --type js $'Set-Cookie' -- 'packages/analytics-js/__fixtures__/msw.handlers.ts'

Length of output: 101

packages/analytics-js-common/src/types/ApplicationState.ts (1)

156-159: The definition of ServerCookiesState is correctly implemented, ensuring that the state can handle both the enablement status and the server URL for cookies.

packages/analytics-js/src/components/configManager/ConfigManager.ts (1)

97-105: The handling of the useServerSideCookies and dataServerUrl options in the init method is correctly implemented. It ensures that the server-side cookies feature is enabled only if the URL is valid.

packages/analytics-js/src/constants/logMessages.ts (1)

94-102: The log messages related to server-side cookie errors are correctly defined, providing clear and actionable error information.

packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts (1)

1391-1421: Review the logic for server-side cookie synchronization.

The tests for syncValueToStorage correctly handle the scenarios where server-side cookies are enabled and disabled. However, it's crucial to verify that the setServerSideCookie method behaves as expected under various network conditions and server responses. Consider adding tests that simulate network failures and server errors to ensure the system's resilience.

@MoumitaM MoumitaM requested a review from saikumarrs May 20, 2024 11:08
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 5eaadf1 and 40523f5.
Files selected for processing (7)
  • packages/analytics-js-common/src/types/ApplicationState.ts (2 hunks)
  • packages/analytics-js/tests/components/configManager/validate.test.ts (2 hunks)
  • packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts (3 hunks)
  • packages/analytics-js/src/components/configManager/ConfigManager.ts (5 hunks)
  • packages/analytics-js/src/components/configManager/util/validate.ts (1 hunks)
  • packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (5 hunks)
  • packages/analytics-js/src/state/slices/serverCookies.ts (1 hunks)
Files skipped from review as they are similar to previous changes (5)
  • packages/analytics-js-common/src/types/ApplicationState.ts
  • packages/analytics-js/src/components/configManager/ConfigManager.ts
  • packages/analytics-js/src/components/configManager/util/validate.ts
  • packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts
  • packages/analytics-js/src/state/slices/serverCookies.ts
Additional Context Used
Learnings (1)
packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts (2)
User: MoumitaM
PR: rudderlabs/rudder-sdk-js#1625
File: packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts:1374-1385
Timestamp: 2024-02-22T10:46:36.393Z
Learning: Multiple cookies with the same name are not possible in web browsers; a newer cookie will overwrite an older one if they share the same name.
User: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts:10-11
Timestamp: 2024-05-06T08:40:17.868Z
Learning: The misuse of `IHttpClient` in type assertions within the file `packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts` has been corrected by the user.
Additional comments not posted (6)
packages/analytics-js/__tests__/components/configManager/validate.test.ts (2)

Line range hint 5-34: The test cases for validateLoadArgs are comprehensive and well-structured.


35-46: The parameterized tests for getTopDomainUrl effectively cover a variety of URL formats.

packages/analytics-js/__tests__/components/userSessionManager/UserSessionManager.test.ts (4)

23-24: Mocking of external dependencies is correctly implemented.

This ensures that the tests do not make actual network requests, which is crucial for reliable and isolated unit tests.


89-89: Ensure defaultHttpClient is correctly injected into UserSessionManager.

This change is crucial for enabling the manager to make network requests, which is part of the new functionality to handle server-side cookies.


1391-1407: Properly handle the state condition for server-side cookie setting.

The conditional logic to check isEnabledServerSideCookies before invoking setServerSideCookie is a good practice to ensure that cookies are only set when explicitly enabled.


1425-1571: Ensure robust error handling and fallback mechanisms for server-side cookie setting.

The comprehensive tests for different scenarios, including network failures and server unavailability, ensure that the system gracefully handles errors and falls back to client-side cookie management when necessary.

coderabbitai[bot]
coderabbitai bot previously approved these changes May 20, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 40523f5 and 8f8e48f.
Files selected for processing (4)
  • packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts (3 hunks)
  • packages/analytics-js/public/index.html (1 hunks)
  • packages/analytics-js/src/components/configManager/ConfigManager.ts (5 hunks)
  • packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts (5 hunks)
Files skipped from review as they are similar to previous changes (3)
  • packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts
  • packages/analytics-js/public/index.html
  • packages/analytics-js/src/components/userSessionManager/UserSessionManager.ts
Additional comments not posted (2)
packages/analytics-js/src/components/configManager/ConfigManager.ts (2)

34-34: The introduction of DEFAULT_DATA_SERVICE_ENDPOINT is a good practice for default configurations.


21-21: Ensure consistent use of utility functions across the codebase.

coderabbitai[bot]
coderabbitai bot previously approved these changes May 20, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 8f8e48f and 3229627.
Files selected for processing (3)
  • packages/analytics-js/tests/components/configManager/validate.test.ts (2 hunks)
  • packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts (3 hunks)
  • packages/analytics-js/src/components/configManager/util/validate.ts (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • packages/analytics-js/tests/components/configManager/validate.test.ts
  • packages/analytics-js/tests/components/userSessionManager/UserSessionManager.test.ts
  • packages/analytics-js/src/components/configManager/util/validate.ts

Copy link

sonarcloud bot commented May 21, 2024

@MoumitaM MoumitaM merged commit 8b8ac8f into develop May 22, 2024
9 checks passed
@MoumitaM MoumitaM deleted the feature/sdk-1301-create-a-cookie-setter-provider-for-service branch May 22, 2024 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants