-
Notifications
You must be signed in to change notification settings - Fork 82
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
refactor: reduce conginitive complexity of the function #1891
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1891 +/- ##
===========================================
+ Coverage 56.99% 57.01% +0.01%
===========================================
Files 476 476
Lines 16253 16260 +7
Branches 3237 3248 +11
===========================================
+ Hits 9264 9271 +7
+ Misses 5753 5744 -9
- Partials 1236 1245 +9 ☔ View full report in Codecov by Sentry. |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve significant updates to the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (1)
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 (
|
size-limit report 📦
|
There was a problem hiding this 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
🧹 Outside diff range and nitpick comments (1)
packages/analytics-js/src/components/configManager/util/commonUtil.ts (1)
161-163
: Refactor cookie options manipulation logic into a single locationThere's a TODO comment indicating that the cookie options manipulation logic should be refactored into one place. Consolidating this logic will improve maintainability and reduce potential inconsistencies.
Would you like assistance in refactoring this code? I can help by suggesting how to modularize the cookie options manipulation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/analytics-js/src/components/configManager/util/commonUtil.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/analytics-js/src/components/configManager/util/commonUtil.ts (1)
132-202
: Refactored function improves code maintainabilityThe
updateStorageStateFromLoadOptions
function has been successfully refactored to reduce cognitive complexity. The restructuring of logic related to server-side cookies and data service URL handling enhances readability and maintainability.
2798c4f
There was a problem hiding this 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
🧹 Outside diff range and nitpick comments (1)
packages/analytics-js/src/components/configManager/util/commonUtil.ts (1)
121-121
: Address the TODO: Centralize cookie options manipulationThe TODO comment indicates a need to refactor the cookie options manipulation logic into a single location. Consolidating this logic will improve maintainability and reduce potential errors due to code duplication.
Would you like assistance in refactoring this logic? I can help propose a solution or open a GitHub issue to track this task.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/analytics-js/src/components/configManager/util/commonUtil.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/analytics-js/src/components/configManager/util/commonUtil.ts (2)
81-158
: Refactoring server-side cookies logic improves modularityThe extraction of server-side cookies and data service URL handling into the
getServerSideCookiesStateData
helper function enhances code modularity and reduces cognitive complexity inupdateStorageStateFromLoadOptions
.
205-206
: Enhanced readability by integrating helper functionUpdating
updateStorageStateFromLoadOptions
to utilizegetServerSideCookiesStateData
simplifies the function, making it more readable and maintainable.
There was a problem hiding this 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
🧹 Outside diff range and nitpick comments (2)
packages/analytics-js/src/components/utilities/loadOptions.ts (1)
35-36
: LGTM! Consider adding a comment for clarity.The addition of the
sameDomainCookiesOnly
property normalization is implemented correctly and consistently with other boolean properties in the function. This change contributes to the PR's objective of reducing cognitive complexity.For improved clarity, consider adding a brief comment explaining the purpose of this property:
+ // Normalize sameDomainCookiesOnly to restrict cookies to the same domain if set to true normalizedLoadOpts.sameDomainCookiesOnly = normalizedLoadOpts.sameDomainCookiesOnly === true;
packages/analytics-js/src/components/configManager/util/commonUtil.ts (1)
84-161
: LGTM with suggestions: New helper function addedThe new
getServerSideCookiesStateData
function effectively centralizes the logic for managing server-side cookies and data service URL handling. It properly handles various scenarios and edge cases, which is commendable.However, consider the following suggestions to improve maintainability and readability:
Break down the function into smaller, more focused helper functions. For example, you could create separate functions for determining the use of exact domain, handling cross-site requests, and validating the data service URL.
Add more inline comments or JSDoc to explain the complex logic, especially around the conditions for enabling/disabling server-side cookies.
Consider using early returns to reduce nesting and improve readability.
Would you like assistance in refactoring this function into smaller, more focused helper functions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/analytics-js/src/components/configManager/util/commonUtil.ts (3 hunks)
- packages/analytics-js/src/components/utilities/loadOptions.ts (1 hunks)
- packages/analytics-js/src/state/slices/loadOptions.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/analytics-js/src/state/slices/loadOptions.ts (1)
40-40
: LGTM. Please provide more context on thecookie
property.The addition of the
cookie
property to thestorage
object indefaultLoadOptions
is structurally sound and follows the existing code patterns.However, to better understand the purpose and future usage of this property:
- Could you elaborate on the intended use of this
cookie
property?- Are there plans to add specific cookie configuration options in the future?
- How does this change relate to the PR's objective of reducing cognitive complexity?
To ensure this change doesn't introduce unintended side effects, let's verify its usage:
✅ Verification successful
Verified the
cookie
property usage.The addition of the
cookie
property has been successfully verified across the codebase. No unintended side effects were found, and the implementation aligns with existing usage patterns.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any existing uses of the new cookie property # Test 1: Search for direct access to storage.cookie echo "Checking for direct access to storage.cookie:" rg --type typescript "storage\.cookie" --glob '!**/loadOptions.ts' # Test 2: Search for spread operator usage with storage, which might inadvertently include the new cookie property echo "Checking for spread operator usage with storage:" rg --type typescript "\.\.\.storage" --glob '!**/loadOptions.ts'Length of output: 413
Script:
#!/bin/bash # Description: Check for any existing uses of the new cookie property # Test 1: Search for direct access to storage.cookie echo "Checking for direct access to storage.cookie:" rg "storage\.cookie" --glob '*.ts' --glob '*.tsx' --glob '!**/loadOptions.ts' # Test 2: Search for spread operator usage with storage echo "Checking for spread operator usage with storage:" rg "\.\.\.storage" --glob '*.ts' --glob '*.tsx' --glob '!**/loadOptions.ts'Length of output: 2929
packages/analytics-js/src/components/utilities/loadOptions.ts (1)
Line range hint
16-131
: Consider refactoring for improved maintainability and type safetyWhile the changes made are good, there are a few suggestions to further improve the code:
The
normalizeLoadOptions
function is quite long and handles many different properties. Consider splitting it into smaller, more focused functions for each group of related properties. This would improve readability and maintainability, further reducing cognitive complexity.There's a TODO comment at the beginning of the function suggesting the addition of warnings for invalid values. Consider implementing this to improve error handling and debugging.
Some properties are using type assertions. Try to improve type safety by using type guards or refactoring to avoid the need for assertions.
Here's an example of how you might start refactoring:
const normalizeCookieOptions = (opts: Partial<LoadOptions>): Partial<LoadOptions> => { const normalized = { ...opts }; if (!isString(normalized.setCookieDomain)) { delete normalized.setCookieDomain; } const cookieSameSiteValues = ['Strict', 'Lax', 'None']; if (!cookieSameSiteValues.includes(normalized.sameSiteCookie as CookieSameSite)) { delete normalized.sameSiteCookie; } normalized.secureCookie = normalized.secureCookie === true; normalized.sameDomainCookiesOnly = normalized.sameDomainCookiesOnly === true; return normalized; } // Use in main function const normalizeLoadOptions = ( loadOptionsFromState: LoadOptions, loadOptions: Partial<LoadOptions>, ): LoadOptions => { let normalizedLoadOpts = clone(loadOptions); normalizedLoadOpts = normalizeCookieOptions(normalizedLoadOpts); // ... other normalizations }To check for other potential SonarCloud issues, you can run the following command:
This will help identify long functions, type assertions, and TODO comments across the TypeScript files in the project.
🧰 Tools
🪛 Biome
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/analytics-js/src/components/configManager/util/commonUtil.ts (2)
6-9
: LGTM: Import statement updated correctlyThe addition of the
CookieOptions
import from theStorage
types is appropriate for the new functionality introduced in this file.
Line range hint
163-217
: LGTM: Effective integration of new helper functionThe changes to the
updateStorageStateFromLoadOptions
function successfully integrate the newgetServerSideCookiesStateData
helper function. The batch update at the end of the function has been appropriately modified to include the new state updates for server-side cookies and data service URL.These changes improve the modularity of the code and make it easier to manage server-side cookie related state updates.
Quality Gate passedIssues Measures |
PR Description
Refactored an internal function to reduce its cognitive complexity as pointed out in the SonarCloud analysis.
Linear task (optional)
https://linear.app/rudderstack/issue/SDK-2159/fix-sonar-detected-issues
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
sameDomainCookiesOnly
property to better control cookie handling based on domain requirements.cookie
property for improved state management.