ref(store): migrate stores to drift/pref async architecture#1061
ref(store): migrate stores to drift/pref async architecture#1061lollipopkit wants to merge 29 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThis PR migrates local persistence from Hive to an AppDb/Drift + SQLCipher stack and PrefStore: adds a Drift-based AppDb schema and SQLCipher bootstrap, a HiveToSqliteMigrator, and replaces many Hive-backed stores (servers, snippets, private keys, container, history, settings, connection_stats, etc.) with asynchronous AppDb/PrefStore implementations. It updates providers, call sites to async Futures, adds platform plugin registrations for sqlcipher, and updates pubspec/dev deps. Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
fix(snippet): avoid delete/put race on same name
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/data/model/app/bak/backup.dart (2)
146-161: Consider logging when container type parsing fails.When both
byNameand thetoStringfallback fail to parse the container type, the entry is silently skipped. Adding a log statement would help diagnose backup compatibility issues.♻️ Suggested improvement
type ??= ContainerType.values.firstWhereOrNull( (e) => e.toString() == raw, ); if (type != null) { await Stores.container.setType(type, id); + } else { + _logger.warning('Unknown container type: $raw for id: $id'); } continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/model/app/bak/backup.dart` around lines 146 - 161, The code that parses ContainerType inside the block that handles keys starting with 'providerConfig' silently skips entries when both ContainerType.values.byName(raw) and the toString() fallback fail; update this block (the parsing around ContainerType, the variable raw and id, and the subsequent Stores.container.setType call) to emit a warning or error log when type remains null so failed parses are visible — include the key/id and raw value in the log for debugging and use your project’s logger (e.g., processLogger or the existing logger utility) rather than printing to stdout.
82-89: Consider wrapping restore operations in error handling to prevent partial state.If any restore operation fails (e.g.,
_restoreSnippetsthrows after_restoreServerssucceeds), the database is left in an inconsistent partial state with some stores cleared/restored and others untouched. Consider wrapping the operations in a try-catch to either rollback or provide meaningful error feedback to the user.♻️ Suggested approach
+ try { await _restoreServers(spis); await _restoreSnippets(snippets); await _restoreKeys(keys); await _restoreHistory(history.cast<String, Object?>()); await _restoreContainer(container.cast<String, Object?>()); await _restoreSettings( settings?.cast<String, Object?>() ?? <String, Object?>{}, ); + } catch (e, trace) { + _logger.severe('Restore failed, data may be in partial state', e, trace); + rethrow; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/model/app/bak/backup.dart` around lines 82 - 89, The sequence of restore calls (_restoreServers, _restoreSnippets, _restoreKeys, _restoreHistory, _restoreContainer, _restoreSettings) can leave the DB in a partially restored state if one throws, so wrap the entire restore sequence in error handling: start a transaction or capture snapshots of affected stores before calling the restores, run all restores inside a try block, and on any catch rollback to the saved snapshots (or roll back the transaction) and rethrow or surface a clear error; ensure cleanup and an informative error message so callers/users know the restore failed and the DB was reverted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/data/model/app/bak/backup.dart`:
- Around line 146-161: The code that parses ContainerType inside the block that
handles keys starting with 'providerConfig' silently skips entries when both
ContainerType.values.byName(raw) and the toString() fallback fail; update this
block (the parsing around ContainerType, the variable raw and id, and the
subsequent Stores.container.setType call) to emit a warning or error log when
type remains null so failed parses are visible — include the key/id and raw
value in the log for debugging and use your project’s logger (e.g.,
processLogger or the existing logger utility) rather than printing to stdout.
- Around line 82-89: The sequence of restore calls (_restoreServers,
_restoreSnippets, _restoreKeys, _restoreHistory, _restoreContainer,
_restoreSettings) can leave the DB in a partially restored state if one throws,
so wrap the entire restore sequence in error handling: start a transaction or
capture snapshots of affected stores before calling the restores, run all
restores inside a try block, and on any catch rollback to the saved snapshots
(or roll back the transaction) and rethrow or surface a clear error; ensure
cleanup and an informative error message so callers/users know the restore
failed and the DB was reverted.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/data/model/app/bak/backup2.dart (1)
22-23:⚠️ Potential issue | 🟡 MinorRegenerate and commit the Freezed/JSON generated files after these model changes.
The generated files
backup2.freezed.dartandbackup2.g.dartmust be regenerated viadart run build_runner build --delete-conflicting-outputsand committed alongside these annotated model modifications, as required by the coding guidelines forlib/data/model/**/*.dart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/model/app/bak/backup2.dart` around lines 22 - 23, The Freezed model BackupV2 was modified but the generated artifacts are missing; run dart run build_runner build --delete-conflicting-outputs to regenerate backup2.freezed.dart and backup2.g.dart, verify there are no build errors, and commit those generated files alongside the BackupV2 changes so lib/data/model/**/*.dart rules are satisfied.
🧹 Nitpick comments (2)
lib/data/store/connection_stats.dart (2)
172-186: Performance: Fetches all history rows then filters in memory.The query fetches all
connectionStatsRecordsfor all servers, then filters to the top 20 per server in Dart code. For large datasets, this could consume significant memory and time.Consider using SQL window functions (
ROW_NUMBER() OVER (PARTITION BY server_id ORDER BY timestamp_ms DESC)) to limit at the database level, or batch the history queries per server with limits.For typical connection stats volumes, this may be acceptable. Flag for future optimization if data grows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/store/connection_stats.dart` around lines 172 - 186, The current implementation in connection_stats.dart reads all rows from connectionStatsRecords then keeps only the first 20 per server in memory (the loop that populates recentConnectionsByServer using _fromRow), which is inefficient for large datasets; change the data access to push the limit into the DB by either: (a) using a SQL window function (ROW_NUMBER() OVER (PARTITION BY server_id ORDER BY timestamp_ms DESC)) to select only rows where row_number <= 20, or (b) looping serverIds and issuing a parameterized query per server ordering by timestampMs DESC with LIMIT 20, then convert rows with _fromRow into recentConnectionsByServer — update the code that builds historyRows and the subsequent loop to use the new query approach.
68-85: Hardcoded table name in raw SQL.The table name
connection_stats_recordsis hardcoded in the SQL string. If the Drift table definition changes, this query will fail at runtime without compile-time detection. Consider extracting the table name as a constant or adding a comment referencing the Drift table definition.This is a common trade-off when using
customSelectfor complex aggregations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/store/connection_stats.dart` around lines 68 - 85, The SQL in the customSelect call hardcodes the table name "connection_stats_records", which will break if the Drift table is renamed; update the query to use a single constant for the table name (e.g., define a const like kConnectionStatsTable = 'connection_stats_records' and interpolate that constant into the customSelect SQL) or add a clear comment referencing the Drift table/class (e.g., ConnectionStatsRecords) so future renames are caught; keep the rest of the customSelect call and variables (successName, serverId) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/data/model/app/bak/backup2.dart`:
- Around line 137-139: Replace the unsafe logging that interpolates the raw
backup payload (the "$val" in the _loggerV2.warning call) so sensitive backup
contents are never written to logs; update the catch block around the decode
logic to log a safe message via _loggerV2.warning including the error (e) and
stack (s) plus non-sensitive metadata about val (e.g., its runtimeType,
length/size, or a truncated hash/fingerprint) and keep the existing descriptive
text ("Decode backup object map failed") instead of the raw payload.
In `@lib/data/model/app/bak/container_restore.dart`:
- Around line 17-23: When handling keys that start with
_containerProviderConfigKey in the loop (the block using
key.startsWith(_containerProviderConfigKey)), guard against an empty id produced
by key.substring(_containerProviderConfigKey.length) by checking that
id.isNotEmpty before calling _resolveContainerType(raw) and
Stores.container.setType(type, id); if id is empty simply skip processing that
entry so you don't persist an invalid mapping.
In `@lib/data/store/connection_stats.dart`:
- Around line 36-51: The cleanup guard can race because `_cleanupRunning` is
read and set across an `await`; fix by making the check-and-set synchronous
before any await: evaluate `shouldCleanupByCount`/`shouldCleanupByInterval`,
then immediately if not `_cleanupRunning` set `_cleanupRunning = true` in the
same synchronous block and only then perform `await _cleanOldRecords()`;
alternatively implement a simple mutual-exclusion Future/Completer lock around
`_cleanOldRecords()` so callers await the same in-flight cleanup instead of both
entering (use `_cleanupRunning` -> a `_cleanupFuture` that is set to the cleanup
Future and cleared when done). Ensure you update places that reset
`_cleanupRunning` (`finally` block) to instead complete/clear the
`_cleanupFuture` or reset `_cleanupRunning` consistently.
---
Outside diff comments:
In `@lib/data/model/app/bak/backup2.dart`:
- Around line 22-23: The Freezed model BackupV2 was modified but the generated
artifacts are missing; run dart run build_runner build
--delete-conflicting-outputs to regenerate backup2.freezed.dart and
backup2.g.dart, verify there are no build errors, and commit those generated
files alongside the BackupV2 changes so lib/data/model/**/*.dart rules are
satisfied.
---
Nitpick comments:
In `@lib/data/store/connection_stats.dart`:
- Around line 172-186: The current implementation in connection_stats.dart reads
all rows from connectionStatsRecords then keeps only the first 20 per server in
memory (the loop that populates recentConnectionsByServer using _fromRow), which
is inefficient for large datasets; change the data access to push the limit into
the DB by either: (a) using a SQL window function (ROW_NUMBER() OVER (PARTITION
BY server_id ORDER BY timestamp_ms DESC)) to select only rows where row_number
<= 20, or (b) looping serverIds and issuing a parameterized query per server
ordering by timestampMs DESC with LIMIT 20, then convert rows with _fromRow into
recentConnectionsByServer — update the code that builds historyRows and the
subsequent loop to use the new query approach.
- Around line 68-85: The SQL in the customSelect call hardcodes the table name
"connection_stats_records", which will break if the Drift table is renamed;
update the query to use a single constant for the table name (e.g., define a
const like kConnectionStatsTable = 'connection_stats_records' and interpolate
that constant into the customSelect SQL) or add a clear comment referencing the
Drift table/class (e.g., ConnectionStatsRecords) so future renames are caught;
keep the rest of the customSelect call and variables (successName, serverId)
unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lib/data/model/app/bak/backup.dartlib/data/model/app/bak/backup2.dartlib/data/model/app/bak/container_restore.dartlib/data/provider/container.dartlib/data/provider/private_key.dartlib/data/provider/snippet.dartlib/data/res/store.dartlib/data/store/connection_stats.dart
| } catch (e, s) { | ||
| _loggerV2.warning('Decode backup object map failed: $val', e, s); | ||
| } |
There was a problem hiding this comment.
Do not log raw backup payload values in decode failures.
At Line 138, interpolating $val can leak sensitive backup contents (including private-key material) to logs.
Suggested fix
- _loggerV2.warning('Decode backup object map failed: $val', e, s);
+ _loggerV2.warning(
+ 'Decode backup object map failed (valueType=${val.runtimeType})',
+ e,
+ s,
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/data/model/app/bak/backup2.dart` around lines 137 - 139, Replace the
unsafe logging that interpolates the raw backup payload (the "$val" in the
_loggerV2.warning call) so sensitive backup contents are never written to logs;
update the catch block around the decode logic to log a safe message via
_loggerV2.warning including the error (e) and stack (s) plus non-sensitive
metadata about val (e.g., its runtimeType, length/size, or a truncated
hash/fingerprint) and keep the existing descriptive text ("Decode backup object
map failed") instead of the raw payload.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/data/model/app/bak/backup2.dart (1)
17-18: Reminder: Run build_runner after modifications.This file uses
@freezedwith generated part files. After finalizing changes, ensure code generation is up to date.dart run build_runner build --delete-conflicting-outputsAs per coding guidelines: "After modifying annotated model files, run: dart run build_runner build --delete-conflicting-outputs"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/model/app/bak/backup2.dart` around lines 17 - 18, This file declares generated parts (part 'backup2.freezed.dart' and part 'backup2.g.dart') for a `@freezed` model—after you finish edits to the model (e.g., backup2.dart) run code generation to update those parts by executing the build_runner command; ensure you run: dart run build_runner build --delete-conflicting-outputs and verify that backup2.freezed.dart and backup2.g.dart are regenerated and checked into the branch so the `@freezed` annotations and JSON (de)serialization remain in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/data/model/app/bak/backup2.dart`:
- Around line 17-18: This file declares generated parts (part
'backup2.freezed.dart' and part 'backup2.g.dart') for a `@freezed` model—after you
finish edits to the model (e.g., backup2.dart) run code generation to update
those parts by executing the build_runner command; ensure you run: dart run
build_runner build --delete-conflicting-outputs and verify that
backup2.freezed.dart and backup2.g.dart are regenerated and checked into the
branch so the `@freezed` annotations and JSON (de)serialization remain in sync.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/data/model/app/bak/backup2.dartlib/data/model/app/bak/container_restore.dartlib/data/store/connection_stats.dartlib/data/store/server.dart
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/data/store/server.dart`:
- Around line 14-20: ServerStore currently uses Drift via adb.AppDb (see
ServerStore class and the _db getter) but the repo policy requires Hive-backed
persistence under lib/data/store; either convert ServerStore to a Hive-backed
implementation (replace adb.AppDb usage, persist state using Hive boxes and the
Hive API within ServerStore) so it complies with the lib/data/store/**/*.dart
guideline, or include an accompanying PR change to the repository rule to allow
Drift; ensure you update or replace references to adb.AppDb and the _db getter
accordingly and implement Hive box open/close, read, write, and migration logic
inside ServerStore to match existing store semantics.
- Around line 444-445: Replace the sensitive raw payload in the error log inside
the server custom map decode path: do not log the variable raw; instead log only
safe metadata (e.g., length/size and runtime type) and the error/stack. Locate
the dprint call that currently uses 'Failed to decode server custom map: $raw'
and change it to include metadata like raw length and type along with e and st,
or omit raw entirely, so no full payload or tokens are written to logs.
- Around line 122-126: The migration block currently removes the old mapping
before setting the new one which can lose data if setting fails; change the
sequence in the migration logic that references containerHostsToMigrate,
container.set, and container.remove so you perform await container.set(newId,
dockerHost) first and only after it succeeds call await container.remove(oldId);
ensure you keep the null check for dockerHost and consider wrapping the
set/remove pair in a try/catch (or rethrow) so failures during set do not leave
the mapping removed.
- Around line 44-47: The code currently deletes the record with delete(old.id)
before writing the new record, which can cause permanent data loss if
put(newInfo) fails; change the flow in the function handling old.id/newInfo.id
to first attempt to write newInfo (call put(newInfo)) while guarding against ID
collisions (e.g., check exists/get for newInfo.id or use an upsert/transaction
if available), only after a successful put then delete(old.id); ensure you
handle and propagate errors so that if put fails the old record remains intact
and if newInfo.id equals old.id simply perform an update instead of delete/put.
| class ServerStore { | ||
| ServerStore._(); | ||
|
|
||
| static final instance = ServerStore._(); | ||
|
|
||
| void put(Spi info) { | ||
| set(info.id, info); | ||
| adb.AppDb get _db => adb.AppDb.instance; | ||
|
|
There was a problem hiding this comment.
Store implementation conflicts with the current repository persistence rule.
ServerStore now persists through Drift in lib/data/store/, but the active repository rule for this path still requires Hive-backed stores. Please either align implementation with the rule or update the rule in the same PR to prevent policy drift.
As per coding guidelines lib/data/store/**/*.dart: Persist state using Hive stores placed under lib/data/store/.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/data/store/server.dart` around lines 14 - 20, ServerStore currently uses
Drift via adb.AppDb (see ServerStore class and the _db getter) but the repo
policy requires Hive-backed persistence under lib/data/store; either convert
ServerStore to a Hive-backed implementation (replace adb.AppDb usage, persist
state using Hive boxes and the Hive API within ServerStore) so it complies with
the lib/data/store/**/*.dart guideline, or include an accompanying PR change to
the repository rule to allow Drift; ensure you update or replace references to
adb.AppDb and the _db getter accordingly and implement Hive box open/close,
read, write, and migration logic inside ServerStore to match existing store
semantics.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai Why still is blocking? |
|
Tip For best results, initiate chat on the files or code changes.
From the PR changes, I can see that most operations have been converted to async (store fetches, puts, provider loads, etc.). If you can point me to a specific file or line, I can analyze whether there's a blocking operation that should be async. 🧠 Learnings used |
|
@coderabbitai Why still is blocking this PR? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 202731 🏁 Script executed: Length of output: 108 🏁 Script executed: Length of output: 1523
Current Status
What's BlockingThe review system shows I requested changes on your last commit (
How to UnblockSince you've been actively addressing issues, you have a few options:
Looking at your commit history, you've made many fixes. The review status just needs to be updated. Would you like me to do a fresh review of the current state? 🧠 Learnings used |
Looking at the whole timeline, it seems that 3 issues are still not resolved. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 80042 🏁 Script executed: Length of output: 29739 🏁 Script executed: Length of output: 674
🚫 Blocking Issues1. ServerStore policy mismatch (
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
lib/data/store/connection_stats.dart (1)
1-18:⚠️ Potential issue | 🟠 MajorInline exception comment is not enough to satisfy the current repository policy.
This file is still Drift-backed under
lib/data/store/, but the repository rule requires Hive-backed persistence in that path. Please codify the exception in the project policy (or move this store outside the scoped path) so implementation and policy are consistent.As per coding guidelines:
lib/data/store/**/*.dart: Persist state using Hive stores placed under lib/data/store/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/store/connection_stats.dart` around lines 1 - 18, The code comment notes a policy exception but the repository policy and file placement are inconsistent: ConnectionStatsStore (class ConnectionStatsStore in this file) remains Drift-backed under lib/data/store/, which violates the rule that files under lib/data/store/** must be Hive-backed; either codify this Drift exception in the project policy/config (e.g., add an explicit exception entry for ConnectionStatsStore/connection_stats.dart in the policy doc or CI rule) or move/rename the Drift-backed store out of the lib/data/store/ scope (e.g., to lib/data/sql_store/ or lib/data/legacy/) and update imports/usages accordingly; ensure the chosen action updates the repository policy file (or CI configuration) and the inline comment to reference the formal policy exception identifier so code placement and policy are consistent.
🧹 Nitpick comments (4)
docs/src/content/docs/fr/principles/ssh.md (1)
88-88: Clarify the storage backend to match the migration terminology.“local storage” is a bit ambiguous here. Consider naming the concrete backend used in this PR (e.g., AppDb/SQLite with SQLCipher via PrefStore) so the security model is explicit.
✏️ Suggested wording
-- Mot de passe stocké chiffré dans le stockage local +- Mot de passe stocké chiffré dans AppDb (SQLite/SQLCipher), via PrefStore🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/src/content/docs/fr/principles/ssh.md` at line 88, Replace the ambiguous phrase "Mot de passe stocké chiffré dans le stockage local" with a concrete backend description referencing the migration terminology used in this PR, e.g., state that the password is stored encrypted in AppDb (SQLite with SQLCipher) via PrefStore; update the sentence to read something like "Mot de passe stocké chiffré dans AppDb (SQLite + SQLCipher) via PrefStore" so the security model and backend are explicit.docs/src/content/docs/zh/principles/architecture.md (1)
84-88: Consider adding historical context about the Hive → Drift migration.For users familiar with the previous architecture, it would be helpful to briefly mention that this Store layer replaces the previous Hive-based persistence, and reference the migration strategy if applicable to understanding how the system works.
📝 Suggested addition
**为何采用这套 Store 层？** +- 此架构从 Hive 迁移而来，提供更强的数据一致性和安全性 - `AppDb` + Drift 负责关系型和查询密集型数据 - `PrefStore` 负责轻量级键值持久化 - `SQLCipher` 保护本地数据库中的敏感记录 - Store API 将持久化细节隔离在 UI 和业务逻辑之外(Translation: "This architecture migrated from Hive to provide stronger data consistency and security")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/src/content/docs/zh/principles/architecture.md` around lines 84 - 88, Add one short sentence after the Store layer bullet list noting that this Store design replaced the previous Hive-based persistence; reference the migration path and rationale by name (mention "Hive → Drift migration", and the key components "AppDb", "PrefStore", "SQLCipher", and "Store API") and point readers to the migration guide or docs page if available so readers familiar with the old architecture can find migration details and rationale.docs/src/content/docs/fr/principles/architecture.md (1)
88-88: Minor terminology consistency tweak in French text.Consider replacing “Store APIs” with “API de Store” (or fully French wording) to keep style consistent within the paragraph.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/src/content/docs/fr/principles/architecture.md` at line 88, Replace the English phrase "Store APIs" in the sentence "Les Store APIs gardent les détails de persistance hors de l'UI et de la logique métier" with a consistent French wording such as "API de Store" (or fully French alternative like "APIs de stockage") so the sentence reads e.g. "Les API de Store gardent les détails de persistance hors de l'UI et de la logique métier"; update the exact phrase "Store APIs" wherever it appears in this paragraph to maintain terminology consistency.docs/src/content/docs/fr/development/structure.md (1)
55-55: Consider a more idiomatic French phrasing for the storage sentence.“Stockage local basé sur …” is understandable, but “Stockage local avec …” (or “reposant sur …”) reads more naturally in this context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/src/content/docs/fr/development/structure.md` at line 55, Remplace la formulation actuelle "Stockage local basé sur AppDb/SQLCipher et PrefStore :" par une tournure plus idiomatique — par exemple "Stockage local avec AppDb/SQLCipher et PrefStore :" ou "Stockage local reposant sur AppDb/SQLCipher et PrefStore :" — en conservant le sens et la ponctuation; cherche l'occurrence exacte "Stockage local basé sur AppDb/SQLCipher et PrefStore" dans le fichier et remplace-la par l'une des alternatives proposées.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/src/content/docs/es/principles/architecture.md`:
- Line 82: Replace inconsistent capitalization of the term "stores" in this
section by choosing a single form (either "Capa de Stores" or "Capa de
stores"/"capa de stores") and applying it consistently throughout; update the
heading "Capa de Stores" and the inline occurrence "capa de stores" so both use
the same casing and terminology, and ensure subsequent references in the section
follow that chosen form.
In `@docs/src/content/docs/es/principles/ssh.md`:
- Line 88: Replace the awkward Spanish phrase "Contraseña almacenada cifrada en
el almacenamiento local" with the clearer wording "Contraseña almacenada de
forma cifrada en el almacenamiento local" to improve readability; locate the
exact line containing the original sentence and update it accordingly.
In `@docs/src/content/docs/zh/development/architecture.md`:
- Around line 47-52: The docs introduce AppDb/Drift/SQLCipher storage (AppDb,
Drift, SQLCipher, PrefStore, Store API) that conflicts with the current
repository guideline requiring Hive-backed stores under lib/data/store/; before
merging, either revert or update the documentation to match the repo policy:
confirm whether the repository policy has been updated to permit Drift/SQLCipher
stores, and if it has not, change this doc to reflect the existing Hive
requirement and coordinate a separate policy PR; if the policy is updated, amend
the documentation to reference the policy change, add a clear migration note
describing Hive→Drift steps for existing contributors, and update the quoted
guideline ("Persist state using Hive stores placed under lib/data/store/") to
the new policy text.
In `@docs/src/content/docs/zh/principles/state.md`:
- Around line 267-283: The documentation example (ServersNotifier class) shows
persistence via Stores.server (Drift/Store) which conflicts with the repo
guideline requiring Hive-backed stores under lib/data/store/; update the example
to use the Hive-backed store implementation (the same API surface used by
Stores.server.fetch/put/delete) or explicitly document this example as an
alternative architecture and request a policy change; also make the async state
updates use AsyncValue.guard in addServer and removeServer (replace direct
AsyncData(...) assignments with await AsyncValue.guard(() =>
Stores.server.fetch())) to match earlier patterns and add basic error
propagation.
---
Duplicate comments:
In `@lib/data/store/connection_stats.dart`:
- Around line 1-18: The code comment notes a policy exception but the repository
policy and file placement are inconsistent: ConnectionStatsStore (class
ConnectionStatsStore in this file) remains Drift-backed under lib/data/store/,
which violates the rule that files under lib/data/store/** must be Hive-backed;
either codify this Drift exception in the project policy/config (e.g., add an
explicit exception entry for ConnectionStatsStore/connection_stats.dart in the
policy doc or CI rule) or move/rename the Drift-backed store out of the
lib/data/store/ scope (e.g., to lib/data/sql_store/ or lib/data/legacy/) and
update imports/usages accordingly; ensure the chosen action updates the
repository policy file (or CI configuration) and the inline comment to reference
the formal policy exception identifier so code placement and policy are
consistent.
---
Nitpick comments:
In `@docs/src/content/docs/fr/development/structure.md`:
- Line 55: Remplace la formulation actuelle "Stockage local basé sur
AppDb/SQLCipher et PrefStore :" par une tournure plus idiomatique — par exemple
"Stockage local avec AppDb/SQLCipher et PrefStore :" ou "Stockage local reposant
sur AppDb/SQLCipher et PrefStore :" — en conservant le sens et la ponctuation;
cherche l'occurrence exacte "Stockage local basé sur AppDb/SQLCipher et
PrefStore" dans le fichier et remplace-la par l'une des alternatives proposées.
In `@docs/src/content/docs/fr/principles/architecture.md`:
- Line 88: Replace the English phrase "Store APIs" in the sentence "Les Store
APIs gardent les détails de persistance hors de l'UI et de la logique métier"
with a consistent French wording such as "API de Store" (or fully French
alternative like "APIs de stockage") so the sentence reads e.g. "Les API de
Store gardent les détails de persistance hors de l'UI et de la logique métier";
update the exact phrase "Store APIs" wherever it appears in this paragraph to
maintain terminology consistency.
In `@docs/src/content/docs/fr/principles/ssh.md`:
- Line 88: Replace the ambiguous phrase "Mot de passe stocké chiffré dans le
stockage local" with a concrete backend description referencing the migration
terminology used in this PR, e.g., state that the password is stored encrypted
in AppDb (SQLite with SQLCipher) via PrefStore; update the sentence to read
something like "Mot de passe stocké chiffré dans AppDb (SQLite + SQLCipher) via
PrefStore" so the security model and backend are explicit.
In `@docs/src/content/docs/zh/principles/architecture.md`:
- Around line 84-88: Add one short sentence after the Store layer bullet list
noting that this Store design replaced the previous Hive-based persistence;
reference the migration path and rationale by name (mention "Hive → Drift
migration", and the key components "AppDb", "PrefStore", "SQLCipher", and "Store
API") and point readers to the migration guide or docs page if available so
readers familiar with the old architecture can find migration details and
rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36e5436e-1a02-4a6b-8651-528f96e33f9d
📒 Files selected for processing (27)
docs/src/content/docs/de/development/architecture.mddocs/src/content/docs/de/development/structure.mddocs/src/content/docs/de/principles/architecture.mddocs/src/content/docs/de/principles/ssh.mddocs/src/content/docs/development/architecture.mddocs/src/content/docs/development/structure.mddocs/src/content/docs/es/development/architecture.mddocs/src/content/docs/es/development/structure.mddocs/src/content/docs/es/principles/architecture.mddocs/src/content/docs/es/principles/ssh.mddocs/src/content/docs/fr/development/architecture.mddocs/src/content/docs/fr/development/structure.mddocs/src/content/docs/fr/principles/architecture.mddocs/src/content/docs/fr/principles/ssh.mddocs/src/content/docs/ja/development/architecture.mddocs/src/content/docs/ja/development/structure.mddocs/src/content/docs/ja/principles/architecture.mddocs/src/content/docs/ja/principles/ssh.mddocs/src/content/docs/principles/architecture.mddocs/src/content/docs/principles/ssh.mddocs/src/content/docs/principles/state.mddocs/src/content/docs/zh/development/architecture.mddocs/src/content/docs/zh/development/structure.mddocs/src/content/docs/zh/principles/architecture.mddocs/src/content/docs/zh/principles/ssh.mddocs/src/content/docs/zh/principles/state.mdlib/data/store/connection_stats.dart
✅ Files skipped from review due to trivial changes (1)
- docs/src/content/docs/de/principles/ssh.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/src/content/docs/development/structure.md
- docs/src/content/docs/zh/development/structure.md
| class ServersNotifier extends _$ServersNotifier { | ||
| @override | ||
| List<Server> build() { | ||
| // 从 Hive 加载 | ||
| return Hive.box<Server>('servers').values.toList(); | ||
| Future<List<Spi>> build() async { | ||
| // 从 Store 层加载 | ||
| return Stores.server.fetch(); | ||
| } | ||
|
|
||
| void addServer(Server server) { | ||
| state = [...state, server]; | ||
| // 持久化到 Hive | ||
| Hive.box<Server>('servers').put(server.id, server); | ||
| Future<void> addServer(Spi server) async { | ||
| await Stores.server.put(server); | ||
| state = AsyncData(await Stores.server.fetch()); | ||
| } | ||
|
|
||
| void removeServer(String id) { | ||
| state = state.where((s) => s.id != id).toList(); | ||
| // 从 Hive 中删除 | ||
| Hive.box<Server>('servers').delete(id); | ||
| Future<void> removeServer(String id) async { | ||
| await Stores.server.delete(id); | ||
| state = AsyncData(await Stores.server.fetch()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Documentation example conflicts with repository persistence guidelines.
This code example accurately reflects the Store-based async architecture, but it conflicts with the repository guideline requiring Hive-backed stores under lib/data/store/. Based on learnings: "Persist state using Hive stores placed under lib/data/store/".
This architectural conflict has already been flagged as a blocking issue in the PR review. The resolution requires either:
- Reverting to Hive-based persistence, or
- Updating the repository policy to officially support Drift/Store architecture
Optional enhancement: Consider adding error handling to the example for consistency with the AsyncValue.guard pattern shown in earlier sections (lines 86-88). This would make the example more complete:
Future<void> addServer(Spi server) async {
await Stores.server.put(server);
state = await AsyncValue.guard(() => Stores.server.fetch());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/src/content/docs/zh/principles/state.md` around lines 267 - 283, The
documentation example (ServersNotifier class) shows persistence via
Stores.server (Drift/Store) which conflicts with the repo guideline requiring
Hive-backed stores under lib/data/store/; update the example to use the
Hive-backed store implementation (the same API surface used by
Stores.server.fetch/put/delete) or explicitly document this example as an
alternative architecture and request a policy change; also make the async state
updates use AsyncValue.guard in addServer and removeServer (replace direct
AsyncData(...) assignments with await AsyncValue.guard(() =>
Stores.server.fetch())) to match earlier patterns and add basic error
propagation.
Deploying serverbox with
|
| Latest commit: |
fdcdce3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://18d2ad34.serverbox.pages.dev |
| Branch Preview URL: | https://fix-sqlite-migration-safety.serverbox.pages.dev |
| throw SSHErr( | ||
| type: SSHErrType.connect, | ||
| message: | ||
| 'Jump server `$jumpId` not found for ${spi.name} (${_hostIdentifier(spi)}) ' | ||
| 'while resolving in $context', | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 _resolveJumpSpi throws exception for missing jump servers instead of falling back to direct connection
The new _resolveJumpSpi helper (lib/core/utils/server.dart:399-421) throws an SSHErr when a server's jumpId references a server that no longer exists. The old code in genClient returned null in this case, causing the connection to fall back to a direct socket connection. Now, any server with a stale jumpId (e.g., pointing to a deleted jump server) will fail to connect entirely with an error. This also affects ensureKnownHostKey at lib/core/utils/server.dart:367-371 which calls the same helper. Users who previously deleted a jump server but kept the reference in another server's config could connect fine; after this change they cannot connect at all.
Old vs new behavior
Old code (returned null, fell through to direct connection):
final jumpSpi_ = () {
if (jumpSpi != null) return jumpSpi;
final jumpId = spi.jumpId;
if (jumpId != null) {
return jumpSpisById?[jumpId] ?? Stores.server.box.get(jumpId);
}
}();
if (jumpSpi_ != null) { /* use jump */ } else { /* direct */ }New code (throws):
final resolved = await Stores.server.fetchOne(jumpId);
if (resolved != null) return resolved;
throw SSHErr(...);| throw SSHErr( | |
| type: SSHErrType.connect, | |
| message: | |
| 'Jump server `$jumpId` not found for ${spi.name} (${_hostIdentifier(spi)}) ' | |
| 'while resolving in $context', | |
| ); | |
| } | |
| final resolved = await Stores.server.fetchOne(jumpId); | |
| return resolved; |
Was this helpful? React with 👍 or 👎 to provide feedback.
… fix/sqlite-migration-safety
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
lib/data/store/connection_stats.dart (1)
1-20:⚠️ Potential issue | 🟠 MajorStore backend in this path still conflicts with repository storage policy.
ConnectionStatsStore(Line 8) usesAppDb/Drift (Line 19) underlib/data/store/**, which does not match the current Hive-only rule for this directory. Please align by either migrating this store back to Hive or updating the repository policy/exception in the same PR.As per coding guidelines:
lib/data/store/**/*.dart: Persist state using Hive stores placed under lib/data/store/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/store/connection_stats.dart` around lines 1 - 20, ConnectionStatsStore currently depends on drift/adb.AppDb (imports: package:drift/drift.dart and adb.AppDb) which violates the lib/data/store Hive-only policy; either convert this store to a Hive-backed implementation or add a repository policy exception in this PR. To fix, replace ConnectionStatsStore (and any references) with a Hive-backed store (e.g., ConnectionStatsHiveStore) using Hive type adapters and Hive box names instead of adb.AppDb and drift types, remove the drift and adb imports from connection_stats.dart, and update callers to use the new Hive store; alternatively, if database-backed storage is intended, add/update the repository policy/exception documentation and tests in this PR to permit adb.AppDb usage for connection stats.docs/src/content/docs/zh/development/architecture.md (1)
47-55:⚠️ Potential issue | 🟠 MajorDocumentation states a storage policy change that is not reflected in current repo guidelines.
Line 54 says
lib/data/store/is no longer Hive-only, but the active repository guideline still requires Hive-backed stores there. Please either update the formal guideline in this PR or adjust this statement to avoid policy/doc drift.As per coding guidelines:
lib/data/store/**/*.dart: Persist state using Hive stores placed under lib/data/store/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/src/content/docs/zh/development/architecture.md` around lines 47 - 55, The doc line claiming lib/data/store/ is no longer Hive-only conflicts with the active guideline that requires Hive-backed stores; either (A) update the repository guideline entry that matches the pattern `lib/data/store/**/*.dart` to reflect migration to AppDb/SQLCipher and remove the Hive-only requirement, and adjust any enforcement rules/tests, or (B) revert the sentence in the architecture doc to state that lib/data/store/ remains Hive-backed and mention that HiveToSqliteMigrator migrates legacy Hive data to AppDb/SQLCipher; modify the architecture.md text around "AppDb/SQLCipher + PrefStore" and the `lib/data/store/` sentence accordingly so docs and policy align.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/src/content/docs/fr/development/structure.md`:
- Line 16: The docs line under the store section references "AppDb/SQLCipher +
PrefStore" which conflicts with the repo policy requiring Hive-backed stores
under lib/data/store/**/*.dart; update the documentation to reflect the
repository canonical policy or mark this doc as transitional/pending: either
replace "AppDb/SQLCipher + PrefStore" with the current canonical "Hive-backed
stores (lib/data/store/)" text or add a clear note next to the store/ entry that
the repository currently enforces Hive persists for lib/data/store/**/*.dart and
that AppDb/SQLCipher + PrefStore is a proposed/transition option until the
policy is changed.
In `@docs/src/content/docs/principles/state.md`:
- Around line 263-282: The docs example conflicts with the repository
persistence policy: replace the Drift/PrefStore usage in the ServersNotifier
example (methods build, addServer, removeServer referencing Stores.server) with
the canonical Hive-backed store calls used by our lib/data/store/* convention
(e.g., use the Hive-backed ServerStore.put/delete/fetch methods and import the
Hive store singleton instead of Stores.server), or alternatively update the
repository policy to explicitly allow Drift/PrefStore and note that change in
the docs; ensure the ServersNotifier example and its use of
build/addServer/removeServer reference the approved store API consistently.
---
Duplicate comments:
In `@docs/src/content/docs/zh/development/architecture.md`:
- Around line 47-55: The doc line claiming lib/data/store/ is no longer
Hive-only conflicts with the active guideline that requires Hive-backed stores;
either (A) update the repository guideline entry that matches the pattern
`lib/data/store/**/*.dart` to reflect migration to AppDb/SQLCipher and remove
the Hive-only requirement, and adjust any enforcement rules/tests, or (B) revert
the sentence in the architecture doc to state that lib/data/store/ remains
Hive-backed and mention that HiveToSqliteMigrator migrates legacy Hive data to
AppDb/SQLCipher; modify the architecture.md text around "AppDb/SQLCipher +
PrefStore" and the `lib/data/store/` sentence accordingly so docs and policy
align.
In `@lib/data/store/connection_stats.dart`:
- Around line 1-20: ConnectionStatsStore currently depends on drift/adb.AppDb
(imports: package:drift/drift.dart and adb.AppDb) which violates the
lib/data/store Hive-only policy; either convert this store to a Hive-backed
implementation or add a repository policy exception in this PR. To fix, replace
ConnectionStatsStore (and any references) with a Hive-backed store (e.g.,
ConnectionStatsHiveStore) using Hive type adapters and Hive box names instead of
adb.AppDb and drift types, remove the drift and adb imports from
connection_stats.dart, and update callers to use the new Hive store;
alternatively, if database-backed storage is intended, add/update the repository
policy/exception documentation and tests in this PR to permit adb.AppDb usage
for connection stats.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 722f445d-1929-4916-93ab-12c6ce738aea
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
docs/src/content/docs/es/principles/architecture.mddocs/src/content/docs/es/principles/ssh.mddocs/src/content/docs/fr/development/structure.mddocs/src/content/docs/fr/principles/architecture.mddocs/src/content/docs/fr/principles/ssh.mddocs/src/content/docs/principles/state.mddocs/src/content/docs/zh/development/architecture.mddocs/src/content/docs/zh/principles/architecture.mddocs/src/content/docs/zh/principles/state.mdios/Flutter/AppFrameworkInfo.plistlib/data/store/connection_stats.dartpubspec.yaml
💤 Files with no reviewable changes (1)
- ios/Flutter/AppFrameworkInfo.plist
🚧 Files skipped from review as they are similar to previous changes (1)
- pubspec.yaml
| │ ├── model/ # Modèles de données par fonctionnalité | ||
| │ ├── provider/ # Providers Riverpod | ||
| │ └── store/ # Stockage local (Hive) | ||
| │ └── store/ # Abstractions de stockage local (AppDb/SQLCipher + PrefStore) |
There was a problem hiding this comment.
Resolve policy/docs mismatch for storage architecture before merging.
These lines document AppDb/SQLCipher + PrefStore as the store standard, but the current repository policy still requires Hive-backed stores under lib/data/store/. Please either update the canonical policy source in this PR or adjust this doc to clearly mark it as a pending/transition state.
Based on learnings: "Applies to lib/data/store/**/*.dart : Persist state using Hive stores placed under lib/data/store/".
Also applies to: 55-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/src/content/docs/fr/development/structure.md` at line 16, The docs line
under the store section references "AppDb/SQLCipher + PrefStore" which conflicts
with the repo policy requiring Hive-backed stores under
lib/data/store/**/*.dart; update the documentation to reflect the repository
canonical policy or mark this doc as transitional/pending: either replace
"AppDb/SQLCipher + PrefStore" with the current canonical "Hive-backed stores
(lib/data/store/)" text or add a clear note next to the store/ entry that the
repository currently enforces Hive persists for lib/data/store/**/*.dart and
that AppDb/SQLCipher + PrefStore is a proposed/transition option until the
policy is changed.
| ### Store Integration | ||
|
|
||
| ```dart | ||
| @riverpod | ||
| class ServerStoreNotifier extends _$ServerStoreNotifier { | ||
| class ServersNotifier extends _$ServersNotifier { | ||
| @override | ||
| List<Server> build() { | ||
| // Load from Hive | ||
| return Hive.box<Server>('servers').values.toList(); | ||
| Future<List<Spi>> build() async { | ||
| // Load from the store layer | ||
| return Stores.server.fetch(); | ||
| } | ||
|
|
||
| void addServer(Server server) { | ||
| state = [...state, server]; | ||
| // Persist to Hive | ||
| Hive.box<Server>('servers').put(server.id, server); | ||
| Future<void> addServer(Spi server) async { | ||
| await Stores.server.put(server); | ||
| state = await AsyncValue.guard(() => Stores.server.fetch()); | ||
| } | ||
|
|
||
| void removeServer(String id) { | ||
| state = state.where((s) => s.id != id).toList(); | ||
| // Remove from Hive | ||
| Hive.box<Server>('servers').delete(id); | ||
| Future<void> removeServer(String id) async { | ||
| await Stores.server.delete(id); | ||
| state = await AsyncValue.guard(() => Stores.server.fetch()); | ||
| } |
There was a problem hiding this comment.
Resolve the persistence-policy mismatch before documenting this as canonical.
This section codifies async Stores.server usage aligned with Drift/AppDb migration, but it conflicts with the repository persistence rule that stores under lib/data/store/ must be Hive-backed. Please either update the repository policy to permit Drift/PrefStore or keep the docs aligned with the current policy until that change is approved.
Based on learnings: Applies to lib/data/store/**/*.dart : Persist state using Hive stores placed under lib/data/store/.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/src/content/docs/principles/state.md` around lines 263 - 282, The docs
example conflicts with the repository persistence policy: replace the
Drift/PrefStore usage in the ServersNotifier example (methods build, addServer,
removeServer referencing Stores.server) with the canonical Hive-backed store
calls used by our lib/data/store/* convention (e.g., use the Hive-backed
ServerStore.put/delete/fetch methods and import the Hive store singleton instead
of Stores.server), or alternatively update the repository policy to explicitly
allow Drift/PrefStore and note that change in the docs; ensure the
ServersNotifier example and its use of build/addServer/removeServer reference
the approved store API consistently.
| Stores.connectionStats.recordConnection(ConnectionStat( | ||
| serverId: spi.id, | ||
| serverName: spi.name, | ||
| timestamp: time1, | ||
| result: ConnectionResult.success, | ||
| durationMs: spentTime, | ||
| )); |
There was a problem hiding this comment.
🟡 Unawaited recordConnection calls can produce unhandled async errors
Stores.connectionStats.recordConnection(...) is an async method performing a database insert, but it's called without await or unawaited() at lines 145 and 178 in lib/data/provider/server/single.dart. If the database write fails (e.g., disk full, DB locked), the resulting unhandled Future error will propagate to the zone error handler. While the app's runZonedGuarded will catch it, it bypasses any local error handling and may mask persistent database issues.
| Stores.connectionStats.recordConnection(ConnectionStat( | |
| serverId: spi.id, | |
| serverName: spi.name, | |
| timestamp: time1, | |
| result: ConnectionResult.success, | |
| durationMs: spentTime, | |
| )); | |
| // Record successful connection | |
| unawaited(Stores.connectionStats.recordConnection(ConnectionStat( | |
| serverId: spi.id, | |
| serverName: spi.name, | |
| timestamp: time1, | |
| result: ConnectionResult.success, | |
| durationMs: spentTime, | |
| )).catchError((e, s) { | |
| Loggers.app.warning('Record connection stat failed', e, s); | |
| })); |
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary
Verification
Closes #1037
Summary by CodeRabbit