fix: prevent cookie storage engine options init by default #2077
Conversation
📝 WalkthroughWalkthroughThis pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant CS as CookieStorage
participant GO as getDefaultCookieOptions
C->>CS: new CookieStorage() (options not provided)
C->>CS: configure()
CS->>CS: Check if options is undefined
alt options undefined
CS->>GO: getDefaultCookieOptions()
GO-->>CS: return default options
CS->>CS: Set options property with default options
else options defined
CS->>CS: Use provided options
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## hotfix/prevent-import-execution #2077 +/- ##
================================================================
Coverage 61.55% 61.55%
================================================================
Files 478 478
Lines 16447 16448 +1
Branches 3264 3256 -8
================================================================
+ Hits 10124 10125 +1
+ Misses 5117 5089 -28
- Partials 1206 1234 +28 ☔ 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: 0
🧹 Nitpick comments (2)
packages/analytics-js/src/services/StoreManager/storages/CookieStorage.ts (2)
26-38: Consider adding safeguard in other methods that usethis.options.While the
configuremethod now safely initializesoptionswhen needed, other methods likesetItemthat usethis.optionsdirectly could still be called beforeconfigure. Consider adding a safeguard mechanism to ensureoptionsis always initialized before use.setItem(key: string, value: Nullable<string>): boolean { + // Ensure options are initialized + if (!this.options) { + this.configure(); + } cookie(key, value, this.options, this.logger); this.length = Object.keys(cookie()).length; return true; }🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
22-24: Update constructor documentation to clarify options initialization behavior.Since the initialization behavior of
optionshas changed from immediate to deferred, it would be helpful to update the constructor's JSDoc to explain thatoptionswill be lazily initialized whenconfigureis called./** * A storage utility to persist values in cookies via Storage interface */ class CookieStorage implements IStorage { logger?: ILogger; options?: ICookieStorageOptions; isSupportAvailable = true; isEnabled = true; length = 0; + /** + * Creates a new CookieStorage instance + * @param logger Optional logger instance + * Note: Cookie options are not initialized in the constructor but are lazily + * initialized when configure() is called. + */ constructor(logger?: ILogger) { this.logger = logger; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/analytics-js/src/services/StoreManager/storages/CookieStorage.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Bundle size checks
🔇 Additional comments (3)
packages/analytics-js/src/services/StoreManager/storages/CookieStorage.ts (3)
17-17: Appropriate type improvement: Makingoptionsoptional aligns with deferred initialization.The change from required to optional type properly reflects that
optionsmay not be initialized immediately upon class instantiation.
27-29: Good implementation of lazy initialization for cookie options.The added conditional ensures that default options are only set when needed, which aligns with the PR objective to prevent cookie storage engine options initialization by default.
41-41:❓ Verification inconclusive
Verify that
optionsis available before use insetItemmethod.The
setItemmethod usesthis.optionsdirectly without checking if it's defined. IfsetItemis called beforeconfigure, this could potentially lead to runtime errors.
🏁 Script executed:
#!/bin/bash # Search for any direct usages of the CookieStorage.setItem method without prior configure calls # Find places where CookieStorage is instantiated echo "Searching for instantiations of CookieStorage..." rg "new CookieStorage" -A 5 --type ts # Find places where setItem is called directly echo "Searching for direct calls to setItem..." rg "\.setItem\(" --type ts # Check if there are any test cases testing the behavior echo "Checking for related test cases..." rg "CookieStorage" --type testLength of output: 5638
Action Required: Ensure
this.optionsis initialized before usage insetItem.The current implementation calls:
cookie(key, value, this.options, this.logger);without verifying if
this.optionshas been set. IfsetItemis invoked before theconfiguremethod (or any initialization logic that assignsthis.options), it may lead to runtime errors. Please add a safeguard to check for (or initialize)this.options—or enforce that configuration always happens before callingsetItem.
- Location:
packages/analytics-js/src/services/StoreManager/storages/CookieStorage.ts(around line 41)- Recommendation: Either add a conditional guard (e.g., an early-return or error throw when
this.optionsis absent) or ensure thatthis.optionsis assigned during construction/configuration.
|
saikumarrs
changed the base branch from
develop
to
hotfix/prevent-import-execution
saikumarrs
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
size-limit report 📦
|
saikumarrs
merged commit cf0e4d9
into
hotfix/prevent-import-execution
PR Description
I have fixed an issue with the cookie storage engine where options are initialised by default causing crashes on Next.js server-side imports.
The default cookie storage options are set once it is configured in
RudderAnalyticsclass's constructor.Linear task (optional)
https://linear.app/rudderstack/issue/SDK-3071/prevent-initialising-cookie-module-by-default
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security