Skip to content

localStorage strategy for remote configuration#4071

Open
mormubis wants to merge 11 commits intomainfrom
adlrb/remote-config-localstorage
Open

localStorage strategy for remote configuration#4071
mormubis wants to merge 11 commits intomainfrom
adlrb/remote-config-localstorage

Conversation

@mormubis
Copy link
Contributor

@mormubis mormubis commented Jan 9, 2026

Motivation

Remote configuration does not support localStorage, which is beneficial for some customers.

Changes

This PR adds the localStorage strategy for remote configuration.

Test instructions

No backend ready for it yet.

Checklist

  • Tested locally
  • Tested on staging
  • Added unit tests for this change.
  • Added e2e/integration tests for this change.

@cit-pr-commenter
Copy link

cit-pr-commenter bot commented Jan 9, 2026

Bundles Sizes Evolution

📦 Bundle Name Base Size Local Size 𝚫 𝚫% Status
Rum 169.43 KiB 169.68 KiB +251 B +0.14%
Rum Profiler 4.29 KiB 4.29 KiB 0 B 0.00%
Rum Recorder 24.54 KiB 24.54 KiB 0 B 0.00%
Logs 56.72 KiB 56.72 KiB 0 B 0.00%
Flagging 944 B 944 B 0 B 0.00%
Rum Slim 126.26 KiB 126.51 KiB +251 B +0.19%
Worker 23.63 KiB 23.63 KiB 0 B 0.00%
🚀 CPU Performance

Pending...

🧠 Memory Performance

Pending...

🔗 RealWorld

@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Jan 9, 2026

⚠️ Tests

Fix all issues with Cursor

⚠️ Warnings

❄️ 2 New flaky tests detected

cookie getCurrentSite caches the result from Safari 12.1.2 (Mac OS 10.14.6) (Datadog) (Fix with Cursor)
Expected spy cookie to have been called 2 times. It was called 0 times.
<Jasmine>
webpack:///packages/core/src/browser/cookie.spec.ts:46:43 <- /tmp/_karma_webpack_649369/commons.js:37890:49
<Jasmine>
cookie getCurrentSite returns the eTLD+1 for foo.bar.baz.example.com from Safari 12.1.2 (Mac OS 10.14.6) (Datadog) (Fix with Cursor)
Expected 'foo.bar.baz.example.com' to be 'example.com'.
<Jasmine>
webpack:///packages/core/src/browser/cookie.spec.ts:20:61 <- /tmp/_karma_webpack_649369/commons.js:37870:108
<Jasmine>

ℹ️ Info

🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 54.55%
Overall Coverage: 77.24% (-0.02%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: d8a54ae | Docs | Datadog PR Page | Was this helpful? Give us feedback!

@mormubis mormubis force-pushed the adlrb/remote-config-localstorage branch from 9e8e512 to 94d9a59 Compare January 30, 2026 09:10
@mormubis
Copy link
Contributor Author

@codex review

@DataDog DataDog deleted a comment from chatgpt-codex-connector bot Jan 30, 2026
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 94d9a591b0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@mormubis mormubis changed the title feat: add localStorage strategy to remote config localStorage strategy for remote configuration Feb 2, 2026
@mormubis mormubis force-pushed the adlrb/remote-config-localstorage branch from c812560 to a8d262d Compare February 4, 2026 10:01
mormubis and others added 11 commits February 12, 2026 16:02
Add 4 new E2E tests for the localStorage strategy:
1. Basic localStorage retrieval - reads simple string values
2. localStorage with regex extractor - parses extracted values
3. Missing key handling - gracefully falls back when key doesn't exist
4. localStorage unavailable - handles access errors and falls back

Tests follow the same pattern as the js strategy tests (PR #3766) and validate:
- Synchronous value resolution during SDK initialization
- Regex extraction functionality
- Fallback behavior for missing or inaccessible keys
- Integration with both CDN and npm setups

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
When a remote configuration value fails to resolve (e.g., localStorage key
is missing or access fails), the system now preserves the initial configuration
fallback value instead of overwriting it with undefined.

This fixes the issue where the SDK would lose the fallback version when:
- localStorage key is not found
- localStorage access throws an error
- Any other dynamic resolution returns undefined

The fix adds an undefined check before assigning resolved values, ensuring
that undefined values do not override the initial configuration.

Fixes E2E tests:
- should resolve to undefined when localStorage key is missing
- should handle localStorage access failure gracefully

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…ng comment

Restore 'electron' union members removed by the submodule bump (unrelated
to the localStorage feature) and add a comment explaining the undefined
preservation logic in applyRemoteConfiguration.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mormubis mormubis force-pushed the adlrb/remote-config-localstorage branch from 961a387 to d8a54ae Compare February 12, 2026 15:02
@mormubis mormubis marked this pull request as ready for review February 13, 2026 14:45
@mormubis mormubis requested a review from a team as a code owner February 13, 2026 14:45
Comment on lines +96 to +97
// Skip undefined values to preserve init config fallbacks when any dynamic strategy
// (cookie, dom, js, localStorage) fails to resolve.
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓ question: ‏do we have a property for which we want to have this behavior? I may miss something but for now I don't think we can configure a property in init that we can override by a dynamic config.
If we can't really achieve that with current system, I'd be in favor of not adding this fallback and the related tests.

expect(initConfiguration.version).toBe('fallback-version')
})

createTest('should handle localStorage access failure gracefully')
Copy link
Collaborator

Choose a reason for hiding this comment

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

👏 praise: ‏nice catch 👍

Comment on lines 248 to +251
it('should resolve to undefined if the cookie is missing', () => {
expectAppliedRemoteConfigurationToBe(
{ version: { rcSerializedType: 'dynamic', strategy: 'cookie', name: COOKIE_NAME } },
{ version: undefined }
{} // version should not be set when resolution fails
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 suggestion: ‏Seems that the title could be updated instead of adding a comment

it('should not be set if the cookie is missing', () => {

).toEqual({
...initConfigWithVersion,
applicationId: 'yyy',
// version should remain 'init-version', not undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 suggestion: ‏what about making that explicit then, even though it's already implied by ...initConfigWithVersion,:

Suggested change
// version should remain 'init-version', not undefined
version: 'init-version'

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.

3 participants