Repository navigation
refactor(store): rename Store to KvStore - #42
Merged
Merged
Conversation
The interface is key-value: `get(key)`, `set(key, value)`, `remove(key)`, `keys()`. Calling it `Store` made that a coincidence of the API rather than a statement about what it is, and left no name for storage that is not key-addressed. That matters now the app is moving its entities into real tables. With both shapes present, `extends Store` reads as "the store base class" and hands whoever writes the next one a `set(key, value)` — which only fits by putting a JSON blob back in a value column, the thing the move exists to undo. `KvStore` states its own constraint, so an entity store visibly does not belong to it. `StoreProp.store` becomes `KvStore get store`, which is now a fact about properties rather than an accident: a property is addressed by key. Mechanical: the four subclasses (Sqlite, Hive, Pref, Mock) and the doc references. Nothing else changes.
`runBuild` was checked in returning `WhenComplete`, a type the resolved riverpod does not have, so `flutter analyze` failed and every widget test failed to compile — on main, before this branch. Two lines, from build_runner.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
Included review availability: 2 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-06-27T14:47:33.941ZApplied to files:
🔇 Additional comments (8)
📝 WalkthroughWalkthroughChangesThe public store interface is renamed from Store interface migration
App build return type
Possibly related PRs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This PR applies the Store-to-KvStore rename consistently and reports clean analysis and passing tests; no actionable merge-blocking risk remains. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
lollipopkit
added a commit
that referenced
this pull request
Aug 19, 2026
* fix: regenerate app.g.dart against the riverpod consumers resolve Reverts the second commit of #42, which was a misdiagnosis. `runBuild` returns `WhenComplete` in riverpod 3.3.x and `void` in 3.2.x. I regenerated against a stale local lockfile pinned to 3.2.1, so the checked-in file stopped matching what every consumer resolves — server_box pulls 3.3.2 and 75 of its test files then failed to compile against this package. There is no committed lockfile here, so a fresh resolve takes 3.3.2 and the original file was right all along. The analyze and test failures I reported on main were my own resolution, not this package's. flutter analyze: clean. flutter test: 253 passing. * fix: move the example lockfile to a riverpod that has WhenComplete Found by review on #43: `example/pubspec.lock` is tracked and pinned riverpod 3.2.1, where `runBuild` returns `void`, so the example could not compile against the generated file this PR restores. The lockfile is the side that moves, not the generated file. 3.2.1 is the only version in play that returns `void`; 3.3.2 — which server_box resolves — and 3.4.2 both declare `WhenComplete runBuild()`. Regenerating to `void` would compile the example and break every consumer, which is the mistake this PR is already undoing. Targeted upgrade of the three riverpod packages rather than all 51 the example would otherwise pull.
lollipopkit
added a commit
to lollipopkit/flutter_server_box
that referenced
this pull request
Aug 19, 2026
…he schema (#1322) * feat(store): the entity schema, with the rules in it Sixteen tables for the seven entities, replacing JSON blobs in `kv`. Two conventions the old layout could not express: A primary key is an id, never something the user typed. Snippets were keyed by name and private keys by a name-used-as-id, so renaming either broke every reference — `Spi.ssh.keyId` pointed at a private key's *name*. Names are ordinary `UNIQUE` columns now and a rename is one `UPDATE`. A list or map field is a child table. `server_tag`, `server_env`, `server_jump`, `server_disabled_cmd`, `server_custom_cmd`, `snippet_tag`, `snippet_auto_run_on`, and `known_host` — which was a JSON map in `setting` keyed `<serverId>::<keyType>`, so a deleted server left its fingerprints behind for ever. Rules that lived in one call site now live in the schema. SSH-or-monitor exclusivity was `Spix.validate()` alone, so a record could be written with both and fail later at connect time; it is a CHECK. Orphan cleanup was six hand-written calls in `delServer` that missed four more (agent conversations, port forwards, container hosts, known hosts); it is ON DELETE CASCADE. Deleting a private key sets its servers' key to null rather than deleting them. `setting` and `history` stay in `kv`: 103 unrelated preferences with no relations and nothing that queries by field, where a new one should stay a one-line change rather than a migration. `agent_conversation.data` stays JSON — an ordered log of heterogeneous items, only ever read whole. Columns would buy nothing and cost a migration per new item kind. Nothing writes to these yet; the stores and the m004 migration come next. * feat(store): carry the metadata an incremental sync needs Sync uploads the whole backup every time, so the cost grows with the data rather than with the change. Fixing that needs per-row change tracking, and adding it after the m004 migration has run would be a second migration — so the columns go in before anything writes to these tables. `updated_at` is what an incremental pull selects on; `rev` separates two edits inside one millisecond, which a clock cannot. Both live on the six sync roots only. A server and its tags, envs and jump hosts are one logical record: the children cascade with the parent and have no meaning without it, so syncing them separately would let a tag arrive before its server. `tombstone` makes a deletion a fact that can travel. Without one the peer that still holds the row reads its absence as an addition and puts it back, which is how a deleted server returns on the next sync. `sync_state` holds this device's id and its per-peer watermarks, and is never itself uploaded. The remote is a whole-file interface — `upload`/`download`/`list`, no range requests, no ETag — so the protocol on top of this has to be an immutable base plus append-only change files, named per device and sequence, with the peers pulling only what they have not seen. That comes next; this is what it will read. * feat(store): m004, entities out of kv and into tables One transaction: a migration that stops half way leaves the records in two shapes with nothing to say which is authoritative. Snippets and private keys get real ids. Both were keyed by a name the user typed — a private key's `id` *was* its name — so `Spi.ssh.keyId` pointed at a name and renaming a key detached every server using it. The old ids are mapped to generated ones and the references rewritten as they are copied. Rows that point at nothing are dropped rather than carried: `conn_stat` has a foreign key now, and the hand-written cleanup in `delServer` missed cases, so an upgrading install holds statistics for servers deleted long ago. Same for a jump host, a port forward or an auto-run target naming a server that no longer exists. Each is logged. A server that could be reached neither way, or both, cannot be represented under the new CHECK. It could not be connected to before either — `genClient` had nothing to dial — so it is dropped with a warning rather than failing the migration for every other record. `updated_at` is carried across instead of stamped as now, so the first sync after upgrading does not read as "everything changed today". `conn_stat`, `agent_conversation` and `agent_active` already exist under those names and `createAll` is `IF NOT EXISTS`, so they are renamed aside, recreated and copied through. Also fixes the ordering this exposed. `HiveImport` recorded `current`, which after adding v5 meant an upgrading install was marked done while its records sat in the v4 kv shape — `migrate` would have skipped m004 and stranded them. It now records `hiveImportProduces`, the layout it actually writes, and the two tests that asserted otherwise say so. * test(store): cover every released build as a migration source The SQLite layout has not shipped, so every install in the field is on Hive and 1466, 1480 and 1491 are all upgrade sources. The suite ran against one. `hive_adapters.g.dart` is byte-identical across the three, so they share one set of assertions — but that is a fact worth checking rather than assuming, so 1480 gets a fixture generated from its own tag even though the generator ran against it unchanged. 1491 shipped an `agent_conversation` box the others do not have, which is the case that needed its own data. The test is now parameterised over the three. `Paths.doc` is `late final` and cannot be set per group, so one temp directory is refilled from the fixture under test in `setUp`. Building the 1491 conversations by hand first is what caught the reason these fixtures exist: written as JSON with camelCase keys and a `type` discriminator they were silently dropped on import, because the release writes snake_case and `kind`. They are built through 1491's own model and serialiser now, and the README says why. * feat(store): the base an entity store sits on Deliberately not a `KvStore`: there is no key-addressed `get`/`set` here, because the records have columns, relations and constraints now. What it keeps is the shape the app already talks to — `fetch`, `fetchOneRaw`, `put`, `delete`, `watch` — so the call sites go on passing models around without learning which storage backs them, and this change stays in the storage layer instead of spreading through the app. `deleteById` is one statement: every child table declares ON DELETE CASCADE, replacing the six hand-written cleanups in `delServer` and the four it missed. It writes a tombstone as it goes, because a peer that still holds the row reads its absence as an addition and puts it back. `put` stamps `updated_at` and increments `rev` in the same transaction as the write, and `touch` does it for a parent whose child rows changed — an edit to a tag is a change to the server that owns it. Both clear any tombstone for the id: a record that comes back has stopped being deleted. * feat(store): ServerStore over the server tables A Spi is six tables now, read with six statements total rather than six per server: each child table is read once and grouped in Dart. `upsert` exists because the test caught `INSERT OR REPLACE` resetting `rev` to its default on every write — that statement deletes the row and inserts a new one, so every column absent from it goes back to the default, and `rev` is the one column that must not. It is an `ON CONFLICT DO UPDATE` naming only the data columns, leaving `updated_at` and `rev` to `_stamp`. Child rows are replaced wholesale rather than diffed: the record arrives as one object, so what it no longer carries is what was removed. A jump host that no longer exists is dropped instead of written as a dangling reference, which the old JSON array could hold and this cannot. Tags come back as a set rather than in the order the JSON array kept: the child table has no ordering column and the UI filters by membership. Noted in the test rather than left to be discovered. `idsWithTag` and `allTags` are what the server list used to get by decoding every record. * build: add drift, on the connection the app already opens The storage layer is hand-written SQL strings and hand-written row mapping, which is what an ORM generates. `INSERT OR REPLACE` silently resetting `rev` — caught by a test, not by the compiler — is the kind of thing typed queries prevent. Verified before committing to it, because encryption is the constraint that would have ruled it out: `NativeDatabase.opened` takes the `sqlite3` `Database` this app opens and keys itself, so sqlite3mc and the build-hook bundling are untouched. A spike confirmed the SSH-xor-monitor CHECK still fires and a dangling foreign key is still refused through Drift's executor — `foreign_keys` is per-connection, so that also confirms Drift is on the same connection rather than opening its own. drift_dev is pinned to 2.34.0 rather than 2.34.5: `hive_ce_generator` wants analyzer ^12 and 2.34.1+ wants ^13. That generator only exists to rebuild the frozen Hive adapters `HiveImport` reads, so it goes when that does. * feat(store): Drift owns the schema The 18 tables are Drift table definitions now, and `tables_schema_test.dart` was the acceptance gate: all 17 guarantees pass against what Drift creates — the SSH-xor-monitor CHECK, the cascades, ON DELETE SET NULL for a deleted private key, the unique names, the sync columns, the tag queries. With that shown, the hand-written DDL is a second source for one schema and is gone; `Tables` keeps only the name lists. `schemaVersion` is 1 and stays there. Version stays with `SchemaVersion`, because the steps that matter are outside what a Drift migration can express: m003 reads Hive boxes, m004 remaps ids and rewrites the references between them. Two mechanisms advancing one number is the ambiguity this change exists to remove. Drift cannot reference a column inside its own `check()`, which the analyzer caught as a recursive getter three times; those are table constraints instead. m003 no longer writes through the store objects. It produces the v4 key-value shape and those stores have moved on to tables — a migration that calls today's code changes meaning every time that code does. It writes into `kv` directly, with `updated_at` 0 so m004 can carry the real timestamps forward and the first sync after upgrading does not read as "everything changed". drift_dev is 2.34.0: `hive_ce_generator` wants analyzer ^12 and 2.34.1+ wants ^13. That generator goes when `HiveImport` does. The tree does not compile past the store layer yet — the six remaining stores and their call sites are the next step. * feat(store): the entity stores over their own tables Every store that holds records now reads and writes columns rather than a JSON blob in `kv`: private keys, snippets, port forwards and the container settings join the servers that moved first. Three things that were data loss, found while porting: - A private key's id *was* its name, and a snippet's key was its name. Both are generated ids with the name as an ordinary unique column now, so a rename is an UPDATE rather than a delete and an insert that leaves every reference behind. - m004 mapped `ssh.keyId` through the old-id table and wrote null when it matched nothing — which is exactly what an `IdentityFile` path put there by the ssh-config import looks like. It lands in `ssh_key_path` now, which is what `ServerStore.migrateIdentityFilePaths` used to recover it into. - The `docker` store's bare `<serverId>` keys, the Docker host from before per-runtime hosts, were dropped along with `providerConfig`. Both are carried across. `migrateIds` and `migrateIdentityFilePaths` are gone from the launch path. They scanned every server on every launch to repair a shape only an upgrading install can hold, and neither could have run after m004 anyway: an empty `Spi.id` has nowhere to live once the id is a primary key. m003 now writes one shape — rows in `kv` — instead of three. It no longer reaches through today's store objects for connection stats and agent conversations, so m004 owns every table and the rename-aside dance goes with it. The schema is created when the database is opened, which is what lets m004 stay one synchronous transaction. Also: container hosts and the chosen runtime are children of `server` rather than records of their own, so they cascade and travel with it; port forwards are in the backup for the first time; `conn_stat` rows get generated ids, so two attempts in the same millisecond no longer collide. The tests do not compile yet. * test(store): the whole upgrade path, and the adapters it broke `hive_release_migration_test` now runs HiveImport *and* KvToTablesMigration against each release fixture, so what it asserts is the shape the app reads rather than an intermediate no build ships. 42 assertions across 1466/1480/ 1491; it found five real bugs on the first run: - The generated Hive adapters no longer read any released box. Adding `id` to `Snippet` and `name` to `PrivateKeyInfo` made the generator emit `fields[n] as String` for a field those bytes do not carry, so both boxes failed to open and every snippet and key was silently left behind. They are frozen types in `lib/hive/legacy_adapters.dart` now, like `LegacySpiV2` already was, and out of `@GenerateAdapters`. - m004 read `ssh['keyId']`, but `SshCredential.toJson` writes `pubKeyId` — kept from the flat pre-v3 layout. Every server lost its key. - It read `key['key']`, but the released `PrivateKeyInfo.toJson` writes `private_key`. Every key was dropped. - It read `conversation['serverId']`, but `AgentConversation.toJson` is hand-written and snake_case. Every conversation was dropped. - `_toSpi` built a `ServerCustom` unconditionally, since the columns are NOT NULL with defaults, giving every server a non-null `custom` it never had. `hive_import_test` keeps its own scope — retry, idempotency, per-box progress — and asserts against `kv`, which is all the import produces now. It seeds through the released layouts rather than today's models. Two behaviour changes the tests pin down: deleting an agent conversation now cascades to the active row, so which one was active is read before the delete; and two connection attempts in the same millisecond are two rows, which is what generated ids were for. 1036/1036 tests pass. * feat(store): the call sites, and a unique name the user is told about A rename is an UPDATE of one column now, so the providers stop deleting and reinserting: that wrote a tombstone for a record that is still there and took its tags and auto-run targets with it by cascade. Renaming a snippet tag is one statement over `snippet_tag` rather than rewriting every snippet holding it, and the second copy of that loop in the provider is gone. Names are unique in the schema rather than in whichever dialog last checked, so a collision surfaces as `DuplicateNameException` and both editors turn it into a message and stay open on the field the user has to change. One new string, `nameAlreadyExistsFmt`, in en and zh. * docs: the storage layout as it is, not as Hive was `docs/development/architecture.md` still described hive_ce in both locales. Replaced with what is there: one encrypted SQLite file, two shapes in it and the rule for choosing between them, Drift owning the DDL and nothing else, ids that are not names, children that travel with their parent, and the two migration steps. CLAUDE.md gets the parts that steer future work — `INSERT OR REPLACE` being wrong on any row with sync columns or children, and that changing a model `lib/hive/` still has a generated adapter for makes every box written before it unreadable. * build(fl_lib): follow the KvStore rename onto current main The submodule pointer was a local commit based on fl_lib before #40, which made `clear` asynchronous and `SyncIface` non-const. Rebasing onto main brings both: - `BakSyncer` stops being a const singleton, since `SyncIface` no longer has a const constructor. - Three `store.clear()` calls did not await, so the settings page reported success before anything was cleared and two tests asserted on a store that had not been cleared yet. - `CachedSqliteStore` is deleted. Every store that extended it — server, private key, snippet — owns a table now, so it had no subclasses left. Blocked on lollipopkit/fl_lib#42; CI here cannot resolve the submodule until that lands. * fix(store): review follow-ups on #1322 Restore ordering, two silent losses, and a generated id shown to the user. - **A jump host is a server**, so during a restore the row it names may be written later. `write` inserted the link inline and dropped any forward reference; `writeLinks` is a second pass `replaceAll` and `merge` run once every row exists. - **The key picker rendered `item.id`.** With ids generated, that chip showed the user a `ShortId` instead of the name they typed. - **`_toEncodable` did not know `PortForwardConfig`**, so a backup carrying a typed one threw. Covered by a round trip through `fromJsonString`. - **`merge` stamped `updated_at = 0`** for a record the backup carried no timestamp for, leaving it older than anything — the next sync would take it straight back out. Absent means now. - **m004 collapsed duplicate snippet names.** Two snippets sharing a stored name produced two records but one `renamed` entry, so both `snippetOrder` entries resolved to whichever was de-duplicated last. - `SnippetNotifier.update` refuses a changed id, which `EntityStore.update` already did and going straight to `put` bypassed. - Deleting an agent conversation and promoting its replacement are one transaction: the delete cascades the active row away. - `resetTables()` for a caller that closes the handle itself, and the previous `AppDb.close` is best-effort. - The private key editor only touches `_loading` while mounted — `decryptPem` runs on another isolate. - Docs, the fixture README's destination path, an assertion no byte could satisfy, two unused temp dirs, and a key fixture whose id and name differ. Not taken: fl_lib's `set`/`setAll`/`remove` are synchronous — only `clear` returns a Future, and both call sites await it. `Stores.x` already resolves through GetIt. 1041/1041 passing. * fix(store): second review round, and the new string in every language l10n: `nameAlreadyExistsFmt` was only in en and zh. All 15 locales now, with each one's own quotation marks rather than a copy of the English. - **`merge` dropped every record from a backup that carries no timestamps.** It compared timestamps before asking whether the record exists here, so an addition read as `0 <= 0` — a tie — and was skipped. Every older envelope is like that. A record this device has never seen is an addition; only one the backup knew about and no longer holds is a delete. - **m004 left agent conversations pointing at a pre-migration server id.** A server whose id was regenerated took its conversations' scope with it now, while a scope that names no server — the global agent's — is kept as it is. - m004's `INSERT OR REPLACE` on `agent_conversation` would delete the active row by cascade; `ON CONFLICT DO UPDATE` instead. - `PrivateKeyNotifier.update` refuses a changed id, like the snippet one. - The private key editor's catch no longer clears `_loading` unguarded — the `finally` does it, and only while mounted. Three tests for the restore fixes: a jump host named before it arrives, a backup with no timestamps, and a record the backup never knew about. Not taken, all pre-existing on main and untouched here: the auto-refresh timer dropped without cancelling, two unawaited `connectionStats` clears, and `SandboxImport.run()` ordering — all from #1318. Reverted my own edit to `sshHostKeyFingerprintMd5Hex`: the key has no call site anywhere in `lib`, so whether it should say MD5 or SHA256 is not something this diff can answer. 1044/1044 passing. * fix(store): a migrated conversation kept the old server id in its payload m004 remapped `agent_conversation.server_id` when a server's id was regenerated, but re-encoded the original JSON beside it. The store rebuilds a conversation from `data` and then compares `conversation.serverId` against the server it was asked about — `fetchActive`, `setActive` and `deleteConversation` all do — so the column found the record and every one of those three rejected it. The conversation existed and nothing could reach it. `test/m004_id_remap_test.dart` covers the rule the bug broke: a server stored before 1155 has an empty id and lives under `user@ip:port`, so the migration generates one, and everything naming the old key has to follow in the same pass. Six cases — the id itself, agent conversations in both places, snippet auto-run targets, port forwards, container hosts and `serverOrder`. Checked against a reverted fix: the conversation case fails without it. 1050/1050 passing. * test(store): pin the hand-written m004 seed to what a release wrote The seed in `m004_id_remap_test` was a claim from memory about what `HiveImport` leaves for a pre-1155 server. Four such claims in this branch turned out wrong, each silently dropping a whole store, so it should not be one. Not by feeding that test release bytes: m004 does not consume any. Its input is what m003 leaves in `kv`, which is this repo's own intermediate — release bytes are m003's input, and `hive_release_migration_test` already runs all three fixtures through both steps. Instead the one fact the seed rests on is asserted there, against 1466/1480/1491: a server from before 1155 reaches `kv` with `id == ''` and an `ssh` map, and it is the only record that does. The seed cannot drift from a real upgrade without that failing. Verified by probe before writing it, rather than assumed again. 1053/1053 passing. * docs(test): say only what the fixture assertion covers The header claimed the `kv` key was "the form such an install used" and that the seed was "written the way 1466 wrote it". The release-backed assertion covers two things and neither is the key: `id == ''` and `ssh` nested under one key. So the comments now name those two, and say plainly that the key is an arbitrary legacy reference whose shape nothing asserts and nothing depends on. A comment claiming fixture backing it does not have is worse than none — it is the kind of thing a later reader trusts instead of checking.
AzadKuu
pushed a commit
to AzadKuu/azad_server_box
that referenced
this pull request
Aug 26, 2026
…he schema (lollipopkit#1322) * feat(store): the entity schema, with the rules in it Sixteen tables for the seven entities, replacing JSON blobs in `kv`. Two conventions the old layout could not express: A primary key is an id, never something the user typed. Snippets were keyed by name and private keys by a name-used-as-id, so renaming either broke every reference — `Spi.ssh.keyId` pointed at a private key's *name*. Names are ordinary `UNIQUE` columns now and a rename is one `UPDATE`. A list or map field is a child table. `server_tag`, `server_env`, `server_jump`, `server_disabled_cmd`, `server_custom_cmd`, `snippet_tag`, `snippet_auto_run_on`, and `known_host` — which was a JSON map in `setting` keyed `<serverId>::<keyType>`, so a deleted server left its fingerprints behind for ever. Rules that lived in one call site now live in the schema. SSH-or-monitor exclusivity was `Spix.validate()` alone, so a record could be written with both and fail later at connect time; it is a CHECK. Orphan cleanup was six hand-written calls in `delServer` that missed four more (agent conversations, port forwards, container hosts, known hosts); it is ON DELETE CASCADE. Deleting a private key sets its servers' key to null rather than deleting them. `setting` and `history` stay in `kv`: 103 unrelated preferences with no relations and nothing that queries by field, where a new one should stay a one-line change rather than a migration. `agent_conversation.data` stays JSON — an ordered log of heterogeneous items, only ever read whole. Columns would buy nothing and cost a migration per new item kind. Nothing writes to these yet; the stores and the m004 migration come next. * feat(store): carry the metadata an incremental sync needs Sync uploads the whole backup every time, so the cost grows with the data rather than with the change. Fixing that needs per-row change tracking, and adding it after the m004 migration has run would be a second migration — so the columns go in before anything writes to these tables. `updated_at` is what an incremental pull selects on; `rev` separates two edits inside one millisecond, which a clock cannot. Both live on the six sync roots only. A server and its tags, envs and jump hosts are one logical record: the children cascade with the parent and have no meaning without it, so syncing them separately would let a tag arrive before its server. `tombstone` makes a deletion a fact that can travel. Without one the peer that still holds the row reads its absence as an addition and puts it back, which is how a deleted server returns on the next sync. `sync_state` holds this device's id and its per-peer watermarks, and is never itself uploaded. The remote is a whole-file interface — `upload`/`download`/`list`, no range requests, no ETag — so the protocol on top of this has to be an immutable base plus append-only change files, named per device and sequence, with the peers pulling only what they have not seen. That comes next; this is what it will read. * feat(store): m004, entities out of kv and into tables One transaction: a migration that stops half way leaves the records in two shapes with nothing to say which is authoritative. Snippets and private keys get real ids. Both were keyed by a name the user typed — a private key's `id` *was* its name — so `Spi.ssh.keyId` pointed at a name and renaming a key detached every server using it. The old ids are mapped to generated ones and the references rewritten as they are copied. Rows that point at nothing are dropped rather than carried: `conn_stat` has a foreign key now, and the hand-written cleanup in `delServer` missed cases, so an upgrading install holds statistics for servers deleted long ago. Same for a jump host, a port forward or an auto-run target naming a server that no longer exists. Each is logged. A server that could be reached neither way, or both, cannot be represented under the new CHECK. It could not be connected to before either — `genClient` had nothing to dial — so it is dropped with a warning rather than failing the migration for every other record. `updated_at` is carried across instead of stamped as now, so the first sync after upgrading does not read as "everything changed today". `conn_stat`, `agent_conversation` and `agent_active` already exist under those names and `createAll` is `IF NOT EXISTS`, so they are renamed aside, recreated and copied through. Also fixes the ordering this exposed. `HiveImport` recorded `current`, which after adding v5 meant an upgrading install was marked done while its records sat in the v4 kv shape — `migrate` would have skipped m004 and stranded them. It now records `hiveImportProduces`, the layout it actually writes, and the two tests that asserted otherwise say so. * test(store): cover every released build as a migration source The SQLite layout has not shipped, so every install in the field is on Hive and 1466, 1480 and 1491 are all upgrade sources. The suite ran against one. `hive_adapters.g.dart` is byte-identical across the three, so they share one set of assertions — but that is a fact worth checking rather than assuming, so 1480 gets a fixture generated from its own tag even though the generator ran against it unchanged. 1491 shipped an `agent_conversation` box the others do not have, which is the case that needed its own data. The test is now parameterised over the three. `Paths.doc` is `late final` and cannot be set per group, so one temp directory is refilled from the fixture under test in `setUp`. Building the 1491 conversations by hand first is what caught the reason these fixtures exist: written as JSON with camelCase keys and a `type` discriminator they were silently dropped on import, because the release writes snake_case and `kind`. They are built through 1491's own model and serialiser now, and the README says why. * feat(store): the base an entity store sits on Deliberately not a `KvStore`: there is no key-addressed `get`/`set` here, because the records have columns, relations and constraints now. What it keeps is the shape the app already talks to — `fetch`, `fetchOneRaw`, `put`, `delete`, `watch` — so the call sites go on passing models around without learning which storage backs them, and this change stays in the storage layer instead of spreading through the app. `deleteById` is one statement: every child table declares ON DELETE CASCADE, replacing the six hand-written cleanups in `delServer` and the four it missed. It writes a tombstone as it goes, because a peer that still holds the row reads its absence as an addition and puts it back. `put` stamps `updated_at` and increments `rev` in the same transaction as the write, and `touch` does it for a parent whose child rows changed — an edit to a tag is a change to the server that owns it. Both clear any tombstone for the id: a record that comes back has stopped being deleted. * feat(store): ServerStore over the server tables A Spi is six tables now, read with six statements total rather than six per server: each child table is read once and grouped in Dart. `upsert` exists because the test caught `INSERT OR REPLACE` resetting `rev` to its default on every write — that statement deletes the row and inserts a new one, so every column absent from it goes back to the default, and `rev` is the one column that must not. It is an `ON CONFLICT DO UPDATE` naming only the data columns, leaving `updated_at` and `rev` to `_stamp`. Child rows are replaced wholesale rather than diffed: the record arrives as one object, so what it no longer carries is what was removed. A jump host that no longer exists is dropped instead of written as a dangling reference, which the old JSON array could hold and this cannot. Tags come back as a set rather than in the order the JSON array kept: the child table has no ordering column and the UI filters by membership. Noted in the test rather than left to be discovered. `idsWithTag` and `allTags` are what the server list used to get by decoding every record. * build: add drift, on the connection the app already opens The storage layer is hand-written SQL strings and hand-written row mapping, which is what an ORM generates. `INSERT OR REPLACE` silently resetting `rev` — caught by a test, not by the compiler — is the kind of thing typed queries prevent. Verified before committing to it, because encryption is the constraint that would have ruled it out: `NativeDatabase.opened` takes the `sqlite3` `Database` this app opens and keys itself, so sqlite3mc and the build-hook bundling are untouched. A spike confirmed the SSH-xor-monitor CHECK still fires and a dangling foreign key is still refused through Drift's executor — `foreign_keys` is per-connection, so that also confirms Drift is on the same connection rather than opening its own. drift_dev is pinned to 2.34.0 rather than 2.34.5: `hive_ce_generator` wants analyzer ^12 and 2.34.1+ wants ^13. That generator only exists to rebuild the frozen Hive adapters `HiveImport` reads, so it goes when that does. * feat(store): Drift owns the schema The 18 tables are Drift table definitions now, and `tables_schema_test.dart` was the acceptance gate: all 17 guarantees pass against what Drift creates — the SSH-xor-monitor CHECK, the cascades, ON DELETE SET NULL for a deleted private key, the unique names, the sync columns, the tag queries. With that shown, the hand-written DDL is a second source for one schema and is gone; `Tables` keeps only the name lists. `schemaVersion` is 1 and stays there. Version stays with `SchemaVersion`, because the steps that matter are outside what a Drift migration can express: m003 reads Hive boxes, m004 remaps ids and rewrites the references between them. Two mechanisms advancing one number is the ambiguity this change exists to remove. Drift cannot reference a column inside its own `check()`, which the analyzer caught as a recursive getter three times; those are table constraints instead. m003 no longer writes through the store objects. It produces the v4 key-value shape and those stores have moved on to tables — a migration that calls today's code changes meaning every time that code does. It writes into `kv` directly, with `updated_at` 0 so m004 can carry the real timestamps forward and the first sync after upgrading does not read as "everything changed". drift_dev is 2.34.0: `hive_ce_generator` wants analyzer ^12 and 2.34.1+ wants ^13. That generator goes when `HiveImport` does. The tree does not compile past the store layer yet — the six remaining stores and their call sites are the next step. * feat(store): the entity stores over their own tables Every store that holds records now reads and writes columns rather than a JSON blob in `kv`: private keys, snippets, port forwards and the container settings join the servers that moved first. Three things that were data loss, found while porting: - A private key's id *was* its name, and a snippet's key was its name. Both are generated ids with the name as an ordinary unique column now, so a rename is an UPDATE rather than a delete and an insert that leaves every reference behind. - m004 mapped `ssh.keyId` through the old-id table and wrote null when it matched nothing — which is exactly what an `IdentityFile` path put there by the ssh-config import looks like. It lands in `ssh_key_path` now, which is what `ServerStore.migrateIdentityFilePaths` used to recover it into. - The `docker` store's bare `<serverId>` keys, the Docker host from before per-runtime hosts, were dropped along with `providerConfig`. Both are carried across. `migrateIds` and `migrateIdentityFilePaths` are gone from the launch path. They scanned every server on every launch to repair a shape only an upgrading install can hold, and neither could have run after m004 anyway: an empty `Spi.id` has nowhere to live once the id is a primary key. m003 now writes one shape — rows in `kv` — instead of three. It no longer reaches through today's store objects for connection stats and agent conversations, so m004 owns every table and the rename-aside dance goes with it. The schema is created when the database is opened, which is what lets m004 stay one synchronous transaction. Also: container hosts and the chosen runtime are children of `server` rather than records of their own, so they cascade and travel with it; port forwards are in the backup for the first time; `conn_stat` rows get generated ids, so two attempts in the same millisecond no longer collide. The tests do not compile yet. * test(store): the whole upgrade path, and the adapters it broke `hive_release_migration_test` now runs HiveImport *and* KvToTablesMigration against each release fixture, so what it asserts is the shape the app reads rather than an intermediate no build ships. 42 assertions across 1466/1480/ 1491; it found five real bugs on the first run: - The generated Hive adapters no longer read any released box. Adding `id` to `Snippet` and `name` to `PrivateKeyInfo` made the generator emit `fields[n] as String` for a field those bytes do not carry, so both boxes failed to open and every snippet and key was silently left behind. They are frozen types in `lib/hive/legacy_adapters.dart` now, like `LegacySpiV2` already was, and out of `@GenerateAdapters`. - m004 read `ssh['keyId']`, but `SshCredential.toJson` writes `pubKeyId` — kept from the flat pre-v3 layout. Every server lost its key. - It read `key['key']`, but the released `PrivateKeyInfo.toJson` writes `private_key`. Every key was dropped. - It read `conversation['serverId']`, but `AgentConversation.toJson` is hand-written and snake_case. Every conversation was dropped. - `_toSpi` built a `ServerCustom` unconditionally, since the columns are NOT NULL with defaults, giving every server a non-null `custom` it never had. `hive_import_test` keeps its own scope — retry, idempotency, per-box progress — and asserts against `kv`, which is all the import produces now. It seeds through the released layouts rather than today's models. Two behaviour changes the tests pin down: deleting an agent conversation now cascades to the active row, so which one was active is read before the delete; and two connection attempts in the same millisecond are two rows, which is what generated ids were for. 1036/1036 tests pass. * feat(store): the call sites, and a unique name the user is told about A rename is an UPDATE of one column now, so the providers stop deleting and reinserting: that wrote a tombstone for a record that is still there and took its tags and auto-run targets with it by cascade. Renaming a snippet tag is one statement over `snippet_tag` rather than rewriting every snippet holding it, and the second copy of that loop in the provider is gone. Names are unique in the schema rather than in whichever dialog last checked, so a collision surfaces as `DuplicateNameException` and both editors turn it into a message and stay open on the field the user has to change. One new string, `nameAlreadyExistsFmt`, in en and zh. * docs: the storage layout as it is, not as Hive was `docs/development/architecture.md` still described hive_ce in both locales. Replaced with what is there: one encrypted SQLite file, two shapes in it and the rule for choosing between them, Drift owning the DDL and nothing else, ids that are not names, children that travel with their parent, and the two migration steps. CLAUDE.md gets the parts that steer future work — `INSERT OR REPLACE` being wrong on any row with sync columns or children, and that changing a model `lib/hive/` still has a generated adapter for makes every box written before it unreadable. * build(fl_lib): follow the KvStore rename onto current main The submodule pointer was a local commit based on fl_lib before lollipopkit#40, which made `clear` asynchronous and `SyncIface` non-const. Rebasing onto main brings both: - `BakSyncer` stops being a const singleton, since `SyncIface` no longer has a const constructor. - Three `store.clear()` calls did not await, so the settings page reported success before anything was cleared and two tests asserted on a store that had not been cleared yet. - `CachedSqliteStore` is deleted. Every store that extended it — server, private key, snippet — owns a table now, so it had no subclasses left. Blocked on lollipopkit/fl_lib#42; CI here cannot resolve the submodule until that lands. * fix(store): review follow-ups on lollipopkit#1322 Restore ordering, two silent losses, and a generated id shown to the user. - **A jump host is a server**, so during a restore the row it names may be written later. `write` inserted the link inline and dropped any forward reference; `writeLinks` is a second pass `replaceAll` and `merge` run once every row exists. - **The key picker rendered `item.id`.** With ids generated, that chip showed the user a `ShortId` instead of the name they typed. - **`_toEncodable` did not know `PortForwardConfig`**, so a backup carrying a typed one threw. Covered by a round trip through `fromJsonString`. - **`merge` stamped `updated_at = 0`** for a record the backup carried no timestamp for, leaving it older than anything — the next sync would take it straight back out. Absent means now. - **m004 collapsed duplicate snippet names.** Two snippets sharing a stored name produced two records but one `renamed` entry, so both `snippetOrder` entries resolved to whichever was de-duplicated last. - `SnippetNotifier.update` refuses a changed id, which `EntityStore.update` already did and going straight to `put` bypassed. - Deleting an agent conversation and promoting its replacement are one transaction: the delete cascades the active row away. - `resetTables()` for a caller that closes the handle itself, and the previous `AppDb.close` is best-effort. - The private key editor only touches `_loading` while mounted — `decryptPem` runs on another isolate. - Docs, the fixture README's destination path, an assertion no byte could satisfy, two unused temp dirs, and a key fixture whose id and name differ. Not taken: fl_lib's `set`/`setAll`/`remove` are synchronous — only `clear` returns a Future, and both call sites await it. `Stores.x` already resolves through GetIt. 1041/1041 passing. * fix(store): second review round, and the new string in every language l10n: `nameAlreadyExistsFmt` was only in en and zh. All 15 locales now, with each one's own quotation marks rather than a copy of the English. - **`merge` dropped every record from a backup that carries no timestamps.** It compared timestamps before asking whether the record exists here, so an addition read as `0 <= 0` — a tie — and was skipped. Every older envelope is like that. A record this device has never seen is an addition; only one the backup knew about and no longer holds is a delete. - **m004 left agent conversations pointing at a pre-migration server id.** A server whose id was regenerated took its conversations' scope with it now, while a scope that names no server — the global agent's — is kept as it is. - m004's `INSERT OR REPLACE` on `agent_conversation` would delete the active row by cascade; `ON CONFLICT DO UPDATE` instead. - `PrivateKeyNotifier.update` refuses a changed id, like the snippet one. - The private key editor's catch no longer clears `_loading` unguarded — the `finally` does it, and only while mounted. Three tests for the restore fixes: a jump host named before it arrives, a backup with no timestamps, and a record the backup never knew about. Not taken, all pre-existing on main and untouched here: the auto-refresh timer dropped without cancelling, two unawaited `connectionStats` clears, and `SandboxImport.run()` ordering — all from lollipopkit#1318. Reverted my own edit to `sshHostKeyFingerprintMd5Hex`: the key has no call site anywhere in `lib`, so whether it should say MD5 or SHA256 is not something this diff can answer. 1044/1044 passing. * fix(store): a migrated conversation kept the old server id in its payload m004 remapped `agent_conversation.server_id` when a server's id was regenerated, but re-encoded the original JSON beside it. The store rebuilds a conversation from `data` and then compares `conversation.serverId` against the server it was asked about — `fetchActive`, `setActive` and `deleteConversation` all do — so the column found the record and every one of those three rejected it. The conversation existed and nothing could reach it. `test/m004_id_remap_test.dart` covers the rule the bug broke: a server stored before 1155 has an empty id and lives under `user@ip:port`, so the migration generates one, and everything naming the old key has to follow in the same pass. Six cases — the id itself, agent conversations in both places, snippet auto-run targets, port forwards, container hosts and `serverOrder`. Checked against a reverted fix: the conversation case fails without it. 1050/1050 passing. * test(store): pin the hand-written m004 seed to what a release wrote The seed in `m004_id_remap_test` was a claim from memory about what `HiveImport` leaves for a pre-1155 server. Four such claims in this branch turned out wrong, each silently dropping a whole store, so it should not be one. Not by feeding that test release bytes: m004 does not consume any. Its input is what m003 leaves in `kv`, which is this repo's own intermediate — release bytes are m003's input, and `hive_release_migration_test` already runs all three fixtures through both steps. Instead the one fact the seed rests on is asserted there, against 1466/1480/1491: a server from before 1155 reaches `kv` with `id == ''` and an `ssh` map, and it is the only record that does. The seed cannot drift from a real upgrade without that failing. Verified by probe before writing it, rather than assumed again. 1053/1053 passing. * docs(test): say only what the fixture assertion covers The header claimed the `kv` key was "the form such an install used" and that the seed was "written the way 1466 wrote it". The release-backed assertion covers two things and neither is the key: `id == ''` and `ssh` nested under one key. So the comments now name those two, and say plainly that the key is an arbitrary legacy reference whose shape nothing asserts and nothing depends on. A comment claiming fixture backing it does not have is worse than none — it is the kind of thing a later reader trusts instead of checking.
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
1 participant
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.
What
sealed class Storebecomessealed class KvStore. The four subclasses(
SqliteStore,HiveStore,PrefStore,MockStore),extension StoreDefaults on KvStore, andStoreProp.storefollow. Nothing else changes.Why
The interface is key-value:
get(key),set(key, value),remove(key),keys(). Calling itStoremade that a coincidence of the API rather than astatement about what it is, and left no name for storage that is not
key-addressed.
That matters now, because server_box is moving its entities out of a JSON blob
in a value column and into real tables. With both shapes present,
extends Storereads as "the store base class" and hands whoever writes the next one aset(key, value)— which only fits by putting the JSON blob back, the thingthe move exists to undo.
KvStorestates its own constraint, so an entitystore visibly does not belong to it.
StoreProp.storebecomingKvStore get storeis the same point: a property isaddressed by key, and that is now a fact rather than an accident.
The second commit
lib/src/provider/app.g.dartwas checked in withrunBuildreturningWhenComplete, a type the riverpod that resolves here does not have. Soflutter analyzefails and every widget test fails to compile — onmain,before this branch. Regenerating is two lines and it is what makes CI able to
say anything about the first commit.
Verified
flutter analyze lib test— clean (was 1 error on main)flutter test— 253 passing (was 23 files failing to load on main)@coderabbitai review
Summary by CodeRabbit
KvStore.