refactor: rewrite PersistenceManager with centralized data management #1254

thkruz · 2025-11-17T10:44:38Z

Add new EventBusEvent types: buildDataUpdate, buildPreferencesUpdate, debouncedSync
Implement centralized preferences and userData objects
Add EventBus listeners for automatic data/preferences updates
Implement debounced sync with 3-second delay
Add isUseLocalStorage and isUseRemoteStorage flags
Sync loop runs when either storage flag is enabled
Save to localStorage only when isUseLocalStorage is true
Emit debouncedSync event for external sync (e.g., Supabase)
All data is properly serialized to JSON
Load from localStorage on initialization

Breaking Changes:

Removed old getItem(), saveItem(), checkIfEnabled() methods
Code using old API will need to be updated to use new preferences/userData objects

- Add new EventBusEvent types: buildDataUpdate, buildPreferencesUpdate, debouncedSync - Implement centralized preferences and userData objects - Add EventBus listeners for automatic data/preferences updates - Implement debounced sync with 3-second delay - Add isUseLocalStorage and isUseRemoteStorage flags - Sync loop runs when either storage flag is enabled - Save to localStorage only when isUseLocalStorage is true - Emit debouncedSync event for external sync (e.g., Supabase) - All data is properly serialized to JSON - Load from localStorage on initialization Breaking Changes: - Removed old getItem(), saveItem(), checkIfEnabled() methods - Code using old API will need to be updated to use new preferences/userData objects

CLAassistant · 2025-11-17T10:44:44Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ 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".

chatgpt-codex-connector · 2025-11-17T10:47:55Z

src/engine/utils/persistence-manager.ts

 export class PersistenceManager {
  private readonly storage_: Storage;
-
  private static instance_: PersistenceManager;

+  private preferences_: Preferences = {};


Restore removed persistence API to satisfy existing callers

The refactor removes the previous getItem/saveItem/removeItem API entirely, but the rest of the codebase still calls those methods (e.g., sensorManager.ts:443, settings.ts:404–429). TypeScript will now fail to compile because PersistenceManager no longer exposes the referenced methods. Either restore the old API or update all callers in the same change so the project builds.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2025-11-17T10:47:55Z

src/engine/utils/persistence-manager.ts

+    // Only run sync loop if at least one storage method is enabled
+    if (this.isUseLocalStorage_ || this.isUseRemoteStorage_) {
+      this.debouncedSync_();
    }


Enable sync or persistence never runs

The new sync path only runs when isUseLocalStorage_ or isUseRemoteStorage_ is true, but both flags default to false and are never set anywhere else in the repository (search for isUseLocalStorage/isUseRemoteStorage returns only this file). As a result markChanged_ exits without scheduling debouncedSync_, so no preferences or user data are persisted or emitted even when mutated.

Useful? React with 👍 / 👎.

Copilot

Pull Request Overview

This PR refactors the PersistenceManager to use centralized data management with event-driven architecture. The old methods (getItem(), saveItem(), checkIfEnabled()) are replaced with a new pattern using centralized preferences and userData objects, automatic EventBus-driven updates, and debounced synchronization.

Key changes:

Centralized data storage using preferences and userData objects with dedicated interfaces
Event-driven updates via EventBus listeners for automatic sync triggering
Debounced synchronization with 3-second delay to reduce storage write frequency
Support for both local and remote storage with conditional sync logic

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.

File	Description
src/engine/utils/persistence-manager.ts	Complete rewrite with centralized preferences/userData management, EventBus integration, and debounced sync mechanism
src/engine/events/event-bus.ts	Added type definitions for new events: buildDataUpdate, buildPreferencesUpdate, debouncedSync
src/engine/events/event-bus-events.ts	Added three new EventBusEvent enum values with documentation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-17T10:49:34Z

src/engine/utils/persistence-manager.ts

+  private buildUserData_(): void {
+    // This will be populated by plugins/managers via the userData setter
+    // or by directly accessing the userData object and modifying it
+    this.markChanged_();
+  }


The buildUserData_() method is called by event listeners but doesn't actually build or update any data - it only marks changes. This is misleading. Consider renaming to onUserDataUpdated_() or removing this method and calling markChanged_() directly from the event listeners, since the method provides no additional logic.

Copilot · 2025-11-17T10:49:34Z

src/engine/utils/persistence-manager.ts

+  private buildPreferences_(): void {
+    // This will be populated by settings manager via the preferences setter
+    // or by directly accessing the preferences object and modifying it
+    this.markChanged_();
  }


The buildPreferences_() method is called by event listeners but doesn't actually build or update any data - it only marks changes. This is misleading. Consider renaming to onPreferencesUpdated_() or removing this method and calling markChanged_() directly from the event listeners, since the method provides no additional logic.

Copilot · 2025-11-17T10:49:34Z

src/engine/utils/persistence-manager.ts

+  private loadFromLocalStorage_(): void {
+    try {
+      // Load preferences
+      const prefsData = this.storage_.getItem(StorageKey.PREFERENCES);
+
+      if (prefsData) {
+        this.preferences_ = JSON.parse(prefsData);
+      }

-    const value = this.storage_.getItem(key);
+      // Load user data
+      const userDataStr = this.storage_.getItem(StorageKey.USER_DATA);

-    if (value === null) {
-      return fallback;
+      if (userDataStr) {
+        this.userData_ = JSON.parse(userDataStr);
+      }
+    } catch (error) {
+      errorManagerInstance.error(error as Error, 'PersistenceManager.loadFromLocalStorage_', 'Failed to load from localStorage');
    }


Loading from localStorage occurs unconditionally during initialization, even when isUseLocalStorage_ is false (it defaults to false and is only set later). This could load stale data when the application intends to use only remote storage. Consider checking the isUseLocalStorage_ flag before loading, or add a mechanism to configure storage preferences before initialization.

Copilot · 2025-11-17T10:49:35Z

src/engine/utils/persistence-manager.ts

+    eventBus.on(EventBusEvent.buildDataUpdate, () => {
+      this.buildUserData_();
+    });
+
+    // Listen for preferences updates (settings changes)
+    eventBus.on(EventBusEvent.buildPreferencesUpdate, () => {
+      this.buildPreferences_();
+    });
+
+    // Listen to specific events that trigger data updates
+    eventBus.on(EventBusEvent.onWatchlistAdd, () => {
+      this.buildUserData_();
+    });
+
+    eventBus.on(EventBusEvent.onWatchlistRemove, () => {
+      this.buildUserData_();
+    });
+
+    eventBus.on(EventBusEvent.setSensor, () => {
+      this.buildUserData_();
+    });
+
+    // Listen to settings save event
+    eventBus.on(EventBusEvent.saveSettings, () => {
+      this.buildPreferences_();
+    });


Event listeners are registered but never cleaned up. This creates a memory leak if PersistenceManager instances are ever destroyed and recreated. Consider implementing a destroy() or cleanup() method that removes all event listeners, or document that this is a singleton that should never be destroyed.

Copilot · 2025-11-17T10:49:35Z

src/engine/utils/persistence-manager.ts

+  }
+
+  set preferences(value: Preferences) {
+    this.preferences_ = value;


The preferences setter replaces the entire preferences object, which could unintentionally discard existing preferences if callers pass partial updates. Consider documenting that this requires the complete preferences object, or implement a merge strategy (e.g., Object.assign(this.preferences_, value)) to preserve existing values not included in the update.

Suggested change

this.preferences_ = value;

Object.assign(this.preferences_, value);

Copilot · 2025-11-17T10:49:35Z

src/engine/utils/persistence-manager.ts

+  }
+
+  set userData(value: UserData) {
+    this.userData_ = value;


The userData setter replaces the entire userData object, which could unintentionally discard existing data if callers pass partial updates. Consider documenting that this requires the complete userData object, or implement a merge strategy to preserve existing values not included in the update.

Suggested change

this.userData_ = value;

this.userData_ = { ...this.userData_, ...value };

Copilot · 2025-11-17T10:49:36Z

src/engine/utils/persistence-manager.ts

+      const prefsData = this.storage_.getItem(StorageKey.PREFERENCES);
+
+      if (prefsData) {
+        this.preferences_ = JSON.parse(prefsData);


JSON.parse without validation could load malformed or malicious data into preferences. Consider validating the parsed object structure matches the Preferences interface before assignment, or implement a schema validation mechanism.

Copilot · 2025-11-17T10:49:36Z

src/engine/utils/persistence-manager.ts

-    if (value === null) {
-      return fallback;
+      if (userDataStr) {
+        this.userData_ = JSON.parse(userDataStr);


JSON.parse without validation could load malformed or malicious data into userData. Consider validating the parsed object structure matches the UserData interface before assignment, or implement a schema validation mechanism.

- Replace StorageKey enum with STORAGE_KEYS constant object - Add key registration system with duplicate detection - Plugins can register keys via registerKey(key, type, pluginId) - Duplicate key registration throws error in debug mode, warns otherwise - Make Preferences and UserData interfaces extensible via module augmentation - Add utility methods: registerKey, unregisterKey, isKeyRegistered, getRegisteredKeys - Add comprehensive plugin integration guide with examples - Support both compile-time (module augmentation) and runtime (registration) safety Breaking Changes: - StorageKey enum removed (replaced with STORAGE_KEYS constant) - Plugins must register keys instead of modifying enum - UserData fields now optional to support dynamic plugin additions Plugin Migration: - Use pm.registerKey('myKey', 'preference', 'PluginId') to register - Access via pm.preferences.myKey or pm.userData.myKey - Optionally extend interfaces via module augmentation for type safety

sonarqubecloud · 2025-11-17T11:04:22Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Copilot AI review requested due to automatic review settings November 17, 2025 10:44

Copilot started reviewing on behalf of thkruz November 17, 2025 10:45 View session

Copilot finished reviewing on behalf of thkruz November 17, 2025 10:45

chatgpt-codex-connector bot reviewed Nov 17, 2025

View reviewed changes

Copilot AI reviewed Nov 17, 2025

View reviewed changes

	this.preferences_ = value;
	Object.assign(this.preferences_, value);

	this.userData_ = value;
	this.userData_ = { ...this.userData_, ...value };

refactor: rewrite PersistenceManager with centralized data management #1254

Are you sure you want to change the base?

refactor: rewrite PersistenceManager with centralized data management #1254

Conversation

thkruz commented Nov 17, 2025

Uh oh!

CLAassistant commented Nov 17, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Nov 17, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants