Migrate legacy storage keys to scoped keys in various store classes#289
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to complete the migration from legacy (unscoped) SharedPreferences keys to scoped keys (scoped by companion public key), ensuring stores still load data when migration doesn’t run.
Changes:
- Adds additional fallback reads for scoped keys in multiple storage
load*()methods. - Updates channel message/settings stores to use a legacy per-channel key during migration.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/storage/unread_store.dart | Adds fallback read intended to load scoped unread-count data. |
| lib/storage/message_store.dart | Adds additional fallback read when loading per-contact messages. |
| lib/storage/contact_store.dart | Adds additional fallback read when loading contacts. |
| lib/storage/contact_group_store.dart | Adds fallback read intended to load scoped contact groups. |
| lib/storage/community_store.dart | Adds fallback read intended to load scoped communities. |
| lib/storage/channel_store.dart | Adds fallback read intended to load scoped channels. |
| lib/storage/channel_settings_store.dart | Changes which key is read first for channel settings/migration. |
| lib/storage/channel_message_store.dart | Adjusts key selection/migration for per-channel message storage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (jsonString == null || jsonString.isEmpty) { | ||
| jsonString = prefs.getString(keyFor); | ||
| } |
There was a problem hiding this comment.
loadCommunities() currently reads the legacy key (_keyPrefix) first and only migrates when that legacy value is empty. When legacy data exists, it will never be migrated and can mask newer writes to the scoped key (keyFor). The load order should be: try keyFor first; if missing, read _keyPrefix, persist to keyFor, then remove _keyPrefix.
| final key = '$keyFor$channelIndex'; | ||
| final oldKey = '$_keyPrefix$channelIndex'; | ||
| bool? enabled = prefs.getBool(key); | ||
| bool? enabled = prefs.getBool(oldKey); | ||
| if (enabled == null) { | ||
| // Attempt migration from legacy unscoped key on first load |
There was a problem hiding this comment.
enabled is being loaded from oldKey rather than the scoped key. This breaks migration: when only the legacy key exists, enabled is non-null so the migration block is skipped, but the function still returns prefs.getBool(key) ?? false (i.e., false), effectively dropping the user’s legacy setting and never writing it to the scoped key. Load should check key first; if null, then read oldKey, migrate to key, and remove oldKey.
| final key = '$keyFor$channelIndex'; | ||
| final oldKey = '$_keyPrefix$channelIndex'; | ||
|
|
||
| String? jsonString = prefs.getString(_keyPrefix); | ||
| String? jsonString = prefs.getString(oldKey); | ||
| if (jsonString == null || jsonString.isEmpty) { |
There was a problem hiding this comment.
loadChannelMessages() is reading from oldKey first (prefs.getString(oldKey)). If the legacy key contains data, the migration block will never run and the scoped key (key) will never be read, so newer writes (which use key) can be ignored and the legacy key never cleaned up. This should read key first; if missing, read oldKey, migrate it to key, then remove oldKey.
| } | ||
| } | ||
| if (jsonString == null || jsonString.isEmpty) { | ||
| jsonString = prefs.getString(keyFor); |
There was a problem hiding this comment.
This fallback reads prefs.getString(keyFor) (the prefix) instead of the full per-channel key (key / '$keyFor$channelIndex'). That key does not match what saveChannelMessages() writes and can cause incorrect behavior. This should read the same full key used elsewhere in the method.
| jsonString = prefs.getString(keyFor); | |
| jsonString = prefs.getString(key); |
| if (jsonString == null || jsonString.isEmpty) { | ||
| jsonString = prefs.getString(keyFor); | ||
| } |
There was a problem hiding this comment.
loadContactUnreadCount() still starts by reading the legacy key (_keyPrefix) and only attempts “migration” when that legacy value is empty. If the legacy key actually contains data, this function will keep returning the legacy value forever and will ignore newer writes to the scoped key (keyFor), since saveContactUnreadCount() writes to keyFor. Load should read keyFor first, and only if missing fall back to _keyPrefix and migrate that value to keyFor (then remove the legacy key).
| jsonString = prefs.getString(keyFor); | ||
| } | ||
| if (jsonString == null || jsonString.isEmpty) { |
There was a problem hiding this comment.
In loadMessages(), this fallback reads prefs.getString(keyFor) (the prefix) rather than the per-contact key (key / '$keyFor$contactKeyHex'). That key is not used by saveMessages() and could cause incorrect reads if anything else ever stores data under the prefix key. This block should be removed or changed to read the same per-contact key used elsewhere in the function.
| jsonString = prefs.getString(keyFor); | |
| } | |
| if (jsonString == null || jsonString.isEmpty) { |
| jsonString = prefs.getString(keyFor); | ||
| } | ||
| if (jsonString == null || jsonString.isEmpty) { |
There was a problem hiding this comment.
This extra prefs.getString(keyFor) is redundant: jsonString was already loaded from keyFor at the top of the method and keyFor does not change within the function. Consider removing this block to avoid duplicate reads and keep the migration flow easier to follow.
| jsonString = prefs.getString(keyFor); | |
| } | |
| if (jsonString == null || jsonString.isEmpty) { |
| if (jsonString == null || jsonString.isEmpty) { | ||
| jsonString = prefs.getString(keyFor); | ||
| } |
There was a problem hiding this comment.
loadGroups() reads from the legacy key (_keyPrefix) first and only “migrates” when that legacy value is empty. If legacy data exists, this will never migrate and will keep returning the legacy value while saveGroups() writes to keyFor, leading to stale reads. Load should read keyFor first; if missing, read _keyPrefix, write it to keyFor, then remove _keyPrefix.
| if (jsonString == null || jsonString.isEmpty) { | ||
| jsonString = prefs.getString(keyFor); | ||
| } |
There was a problem hiding this comment.
loadChannels() reads the legacy key (_keyPrefix) first and only migrates when that legacy value is empty. If legacy data exists, it will never migrate and will keep returning the legacy value, while saveChannels() writes to keyFor—leading to stale reads. Load should read keyFor first, then fall back to _keyPrefix for one-time migration (and remove _keyPrefix after successful migration).
Forgot to load stores when miration was not available.