fix: use single PersistenceManager to avoid undesired cascade mechanisms#40499
Open
gauthierpetetin wants to merge 40 commits intomainfrom
Open
fix: use single PersistenceManager to avoid undesired cascade mechanisms#40499gauthierpetetin wants to merge 40 commits intomainfrom
gauthierpetetin wants to merge 40 commits intomainfrom
Conversation
Keep sentryLocalStore as fallback for pre-init and UI. Add getPersistenceManager() that returns stateHooks._persistenceManager ?? sentryLocalStore. Register background's persistenceManager on stateHooks so hooks use it after init. Prevents get-failed cascade by sharing #dataRetrievalFailing across app and Sentry. Made-with: Cursor
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
Builds ready [e35c6ae]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚀 Bundle size reduced!]
|
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
Builds ready [38e8ec1]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
Builds ready [fe5f3c4]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
- setup-initial-state-hooks: add isBackgroundContext(), create one PersistenceManager per context with FixtureExtensionStore(initialize: isBackground); expose via stateHooks.persistenceManager - background: use stateHooks.persistenceManager instead of creating a second instance; remove unused ExtensionStore/FixtureExtensionStore and PersistenceManager imports - getSentryState: use globalThis.navigator for service worker safety Made-with: Cursor
gauthierpetetin
had a problem deploying
to
pr-comment
GitHub Actions
Error
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Gudahtt
reviewed
Mar 2, 2026
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
Builds ready [2578d11]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
gauthierpetetin
had a problem deploying
to
pr-comment
GitHub Actions
Error
Restore window.navigator.userAgent; the globalThis.navigator change was not part of the single PersistenceManager refactor. Made-with: Cursor
Export persistenceManager from setup-initial-state-hooks and import it in background.js; only consumer is background.js which already imports this module. Keeps state hooks on globalThis.stateHooks for Sentry and other callers that don't import this module. Made-with: Cursor
- MV3: detect service worker by undefined window or ServiceWorkerGlobalScope - MV2: detect background page by URL containing 'background' (e.g. background.html) - Remove MV3 Firefox reference (not applicable) - Logic verified in Chrome MV3 (SW + UI) and Firefox MV2 (background + UI) Made-with: Cursor
Contributor
Builds ready [ad592e3]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Define getEnvironmentTypeForHooks locally in setup-initial-state-hooks.js, using shared getEnvironmentType, and remove app/scripts/lib/get-environment-type.ts. Made-with: Cursor
gauthierpetetin
had a problem deploying
to
pr-comment
GitHub Actions
Error
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
Builds ready [656a270]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
gauthierpetetin
changed the title
gauthierpetetin
changed the title
Gudahtt
reviewed
Mar 9, 2026
Made-with: Cursor
gauthierpetetin
had a problem deploying
to
pr-comment
GitHub Actions
Error
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
Builds ready [a073850]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
This reverts commit bed1714.
…store, fix tests - Only check globalThis.self?.location?.href and compute isBackground when useFixtureStore is true; throw with clear error if href is missing - Add comment that globalThis.self is used for both UI and service worker - Tests: mock shared/lib/object.utils (not modules); expect throw when href empty or self undefined Made-with: Cursor
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
Builds ready [81d4159]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
gauthierpetetin
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Gudahtt
reviewed
Mar 16, 2026
| ? new FixtureExtensionStore() | ||
| : new ExtensionStore(), | ||
| }); | ||
| let isBackground = false; |
Member
There was a problem hiding this comment.
Nit: It could be confusing to leave this variable in the module scope where it might be re-used, when it's actually only set to the correct value in a narrow circumstance (when useFixtureStore is true). In all other cases it remains set to false even in the background process.
Member
There was a problem hiding this comment.
Maybe we could do something like this, and create a function to construct the local store:
function createLocalStore() {
const useFixtureStore =
process.env.IN_TEST &&
getManifestFlags().testing?.forceExtensionStore !== true;
if (!useFixtureStore) {
return new ExtensionStore();
}
// Use globalThis.self (not window) so this works in both the UI and the background/service worker, where window is undefined.
const locationHref = globalThis.self?.location?.href;
if (!locationHref) {
throw new Error(
'setup-initial-state-hooks: globalThis.self?.location?.href is not defined; expected to run in a document or service worker context.',
);
}
const isBackground =
getEnvironmentType(locationHref) === ENVIRONMENT_TYPE_BACKGROUND;
return new FixtureExtensionStore({ initialize: isBackground })
}
That keeps the intermediary variables out of the global scope, and never leaves it with an invalid/misleading default value
Contributor
Author
There was a problem hiding this comment.
Makes sense, it's now updated here: 4468792
Move useFixtureStore, locationHref, and isBackground logic into createLocalStore() so no misleading module-level isBackground default. Addresses PR review feedback. Made-with: Cursor
|
Contributor
Builds ready [1ec00b8]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The purpose of this PR is to use a single
PersistenceManagerinstance, while we previously had two:Why is it inconvenient to have 2 PersistenceManager instances?
In some cases, it can lead to undesired cascade mechanisms:
PersistenceManagerinstancegetMetaMetricsEnabled())PersistenceManagerinstanceWhat solution do we suggest in this PR?
The suggested solution is to create
PersistenceManagerinstance in 'setup-initial-state-hooks.js' and export it so that 'background.js' can use it as well.Changelog
CHANGELOG entry: null
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
NA
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Changes how the extension’s persistent storage layer is instantiated and shared during startup, which can affect initialization and error-reporting flows if misconfigured. Adds new context-detection logic for fixture stores that could impact test and MV2/MV3 behavior.
Overview
Uses a single exported
persistenceManagerfromsetup-initial-state-hooks.jsand reuses it inbackground.js, removing the separate background-createdPersistenceManagerto avoid duplicate/cascading storage read failures during early Sentry/metrics checks.Updates
setup-initial-state-hooks.jsto create the testFixtureExtensionStorewith context-awareinitializebehavior (background vs UI viaglobalThis.self.location.href) and adds a dedicated Jest test suite covering context detection plus thestateHookswiring (getPersistedState,getBackupState,getSentryState).Written by Cursor Bugbot for commit 1ec00b8. This will update automatically on new commits. Configure here.