Skip to content

Dive computers become equipment on the dives they logged (schema v175) - #1320

Merged
ericgriffin merged 23 commits into
mainfrom
worktree-dive-computer-gear-twin
Aug 28, 2026
Merged

ericgriffin merged 23 commits into
mainfrom
worktree-dive-computer-gear-twin

Conversation

@ericgriffin

@ericgriffin ericgriffin commented Aug 26, 2026 •

Copy link
Copy Markdown
Member

Summary

A dive computer that downloaded a dive now appears as a piece of equipment on that dive.

Submersion models a dive computer twice, in two tables that were never connected: dive_computers (the device registry: BLE address, firmware, fingerprint, download counters) and equipment rows of EquipmentType.computer (the gear item: purchase date, service intervals, attributes). A downloaded dive got dives.computer_id stamped and showed its computer in the Details card, but its Equipment section stayed empty unless a default equipment set happened to apply. The computer earned no service tracking, never appeared in gear statistics, and could not be filtered on.

This adds a dive_computers.equipment_id bridge to a real equipment row (its "gear twin"), seeded once at registration at a deterministic UUID v5 id so a synced fleet converges on one row, plus a link-only service that attaches it per dive and a v175 migration that backfills existing logbooks.

Design and plan are committed alongside the code: docs/superpowers/specs/2026-08-26-dive-computer-gear-twin-design.md and docs/superpowers/plans/2026-08-26-dive-computer-gear-twin.md.

Changes

  • Schema v175: dive_computers.equipment_id, nullable, onDelete: setNull. The setNull is load-bearing rather than incidental: it is what makes deleting a gear item leave the device registered, and the cleared column is what makes that deletion permanent.
  • Deterministic twin id (diveComputerGearId) derived from the registry id behind a frozen namespace, matching the imported_computer_identity.dart precedent. A minted row cannot use v4, or two devices would mint different primary keys for one computer and duplicate instead of merging under sync upsert.
  • Find-or-create resolution in a four-step order: stored link, then the row already holding the derived id, then exactly one unambiguous identity match among active computer gear, then mint. Step two is what prevents SqliteException(1555) after a user renames a computer, since the derived id still collides while text matching has stopped agreeing. Ambiguous candidates mint rather than guess: a wrong adoption silently attaches one device's service history to another device's dives.
  • Two mint sites, not one: createComputer (covering BLE/USB download, manual add, and file-import registration) and the imported_computer_backfill self-heal, which writes registry rows with a raw INSERT OR IGNORE and bypasses the repository entirely. The heal mints only where the insert genuinely inserted, so a user-deleted twin cannot reappear on the next app open.
  • DiveComputerGearLinker, a fourth member of the existing defaulter / checklist-linker / altitude-enricher trio, wired at all four non-interactive creation seams plus the replaceSource path. Link-only: it never creates equipment.
  • v175 backfill: pass one resolves a twin per registered computer, pass two is a single set-based insert over the union of dive_data_sources.computer_id and dives.computer_id. Ladder-only, never a beforeOpen backstop, because it is a full-table pass and re-running it every open would resurrect deleted gear.
  • SyncService.parentRefs entry for the new FK. Mandatory: without it a peer's live computer whose gear item was deleted locally dangles its FK and aborts the whole sync at COMMIT with SqliteException(787).
  • Buoyancy fix: EquipmentType.computer now returns 0.0 from _typeDryMass, which previously fell through to a 0.5 kg default. See "Behaviour changes" below.
  • UI: the dive computer detail page shows its linked gear item, routing to /equipment/<id>. One new string, translated across all 11 locales.

Behaviour changes worth calling out in release notes

  1. New gear items appear for every registered dive computer, including ones registered by past file imports.
  2. "Most Used Gear" will usually be topped by a dive computer, since it is on every downloaded dive. This is honest rather than wrong, and getMostUsedGear applies no type exclusion. Left as-is deliberately.
  3. A 0.5 kg buoyancy shift for anyone who had already added a computer as gear by hand. _typeDryMass had no computer case, so it fell through to _ => 0.5; that was never a considered value for computers. Without this fix, seeding twins would have moved every downloaded dive's rig by 0.5 kg per computer.
  4. The dive detail Equipment section stops being conditionally hidden on downloaded dives, since they now always have at least one item.
  5. The computer appears twice on the dive detail page, as a Details-card row and as a gear tile. Both are kept deliberately: they route to different destinations (/dive-computers/<id> for firmware and downloads, /equipment/<id> for service and purchase), and deduplicating would remove the device page's only entry point from a dive.

Test Plan

  • flutter test passes: 20543 passed, 19 skipped
  • flutter analyze passes: no issues
  • dart format . clean; Drift codegen and flutter gen-l10n verified current with no drift
  • Manual testing: not done. All verification here is automated. A real BLE download, a file import, and an upgrade of a populated logbook are worth exercising by hand before release.

New coverage, 30 tests across 7 files:

  • Identity and matching rules, including the rename collision and the ambiguous-candidate case
  • Resolver: all four resolution branches, retired and non-computer gear ignored, idempotency
  • Linker: multi-source dives get both computers, adds rather than replaces, never creates for a deleted twin
  • Ordering regression, both directions: a downloaded dive receives its default equipment set and its computer. The defaulter bails when a dive already has any dive_equipment row, so a linker running first silently suppresses the diver's default and geofenced sets; the second test asserts that failure mode explicitly so the constraint fails loudly if anyone reorders the calls.
  • Migration v175: column shape, upgrade from v168, the stranded parallel-branch case via beforeOpen, backfill including a multi-source dive getting both computers, adoption of hand-created gear, idempotency
  • Self-heal: mints a twin, and does not re-mint a deleted one across three app opens
  • Buoyancy: 0.0 prior and dry mass, with an explicit user dry weight still honoured

Notes for review

  • Schema number: this claims v175. It has moved twice: v168 -> v169 when PR Feature: Dives / Dive xy / Add buddy picker #638 #1237 was renumbered onto v168 mid-implementation, then v169 -> v175 when main was merged into this branch on 2026-08-27 after feat: represent SAC and RMV as separate quantities #1322 (which held v170) and others landed. Verified after that merge that the ladder is monotonic and unique and that the scalar equals its maximum. Please re-check before merging. The open-PR diff scan alone is not sufficient, since it cannot see unpushed renumbers or local-only worktree claims; v168 was double-claimed for hours undetectably. Scanning every worktree's currentSchemaVersion alongside the PR diffs is what catches it.
  • The sync design follows the Import / Dive Computer / No dives from this computer – no link #1064 heal rather than departing from it. An earlier version of this description said the opposite; that was wrong and is corrected here. The runtime paths mark pending and replicate (the resolver on mint, createComputer on stamp, bulkAddEquipment on every link). The v175 backfill is local-only and HLC-neutral: every input is already synced and the twin id is derived, so each device produces identical rows when its own ladder runs, and marking them pending would push one record per computer plus one per (dive, computer) pair from every device in the fleet to make peers agree on rows they each derive anyway. A base export passes hlcSince == null and carries the rows regardless, so an adopting device is not missing them. dive_computer_gear_backfill_test.dart asserts the HLC-neutrality so it cannot be reversed silently.
  • Known limitation, accepted and documented: a dive downloaded by a peer still on the previous schema and synced to an already-migrated device is not linked on that device. Its ladder has run, the runtime linker fires only at local creation seams, and the peer's own later backfill does not push. That is a missing join row on one device during the rollout window, not divergence in the twin, and it resolves when anyone edits that dive's gear. Closing it would need either a fleet-wide re-sync or a link step on the sync-apply path; neither is worth the cost.
  • Not reusing getComputerIdsForDive is intentional. It reads dive_profiles, so it sees only dives carrying profile samples. A file-imported dive registered by Dive computer filter offers nothing for file-imported dives #1288 can have computer_id stamped and a data-source row with no samples at all, and reusing the helper would have silently failed to link exactly that case. The linker owns a query over the same union the backfill uses.
  • Out of scope: Equipment set if import with dive computer xy then add the set #1020 (choosing an equipment set based on which computer downloaded the dive) is related, separately valuable, and not blocked by this. Transmitters are untouched.

A dive computer that downloaded a dive should appear as equipment on that
dive. Today the registry (dive_computers) and gear (equipment rows of type
computer) are two unconnected tables, so a downloaded dive shows its computer
in the Details card but nothing in its Equipment section.

Design: a nullable dive_computers.equipment_id bridge with onDelete setNull,
a deterministic v5 twin id so a synced fleet converges on one row, find-or-
create resolution that adopts hand-created gear before minting, and a v168
ladder step that backfills existing logbooks from dive_data_sources rather
than the primary-only dives.computer_id scalar.

Notable constraints captured: the linker must run after DiveEquipmentDefaulter
or it suppresses default and geofenced sets; minting only where the computer
row was genuinely inserted makes a user-deleted twin permanent without
tombstone machinery; and EquipmentType.computer needs an explicit 0.0 case in
gear_feature.dart, since it currently falls through to a 0.5 kg dry-mass
default that would silently move every downloaded dive's buoyancy.
Ten tasks, each with its own TDD cycle and commit: identity helpers, the
v168 column and sync parent ref, the find-or-create resolver, the link-only
linker plus its four seams, the replaceSource path, the backfill, the
imported-computer heal, the buoyancy fix, the device-page row, and whole
project verification.

Records five deviations from the spec, the two that matter being a required
parentRefs entry the spec did not know about (without it a peer's live
computer whose gear item was deleted locally dangles its FK and aborts the
whole sync at COMMIT) and a serializer change the spec called for that turns
out to be a no-op, since diveComputers round-trips through Drift's toJson.
Nullable FK to equipment with onDelete setNull, so deleting the gear item
leaves the device registered and the cleared column is what makes that
deletion permanent: only a genuine computer insert ever mints a twin.

Registers the FK in SyncService.parentRefs, which is mandatory rather than
tidy. Verified by removing it: sync_parent_refs_completeness_test fails with
"diveComputers.equipmentId -> equipment (nullable=true)". Without it a peer's
live computer whose gear item was deleted locally dangles the FK and aborts
the whole sync at COMMIT with SqliteException(787).

Claims v169, not the v168 the design and plan were written against. PR #1237
was renumbered from v161 onto v168 and pushed while this branch was being
written, so the claim was invisible to the open-PR scan when it ran. Two
branches writing the same scalar auto-merge with no conflict marker, so the
collision would have surfaced only as a database silently skipping a rung.
Design doc and plan updated to match.
Adds DiveComputerGearLinker, a fourth member of the existing defaulter /
checklist-linker / altitude-enricher trio, wired at all four non-interactive
creation seams. Link-only: it never mints equipment, which is what makes a
user-deleted twin permanent.

The ordering test is the point of the task, not decoration. The defaulter
bails when a dive already has any dive_equipment row, so a linker running
first silently suppresses the diver's default and geofenced sets. Both
directions are asserted, so the constraint fails loudly if anyone reorders
the calls.

Does NOT reuse DiveComputerRepository.getComputerIdsForDive, which the design
had called for. That method reads dive_profiles, so it sees only dives with
profile samples; a file-imported dive registered by #1288 can have computer_id
stamped and a data-source row with no samples at all, and reusing it would
have silently failed to link exactly the file-import case. The linker owns a
query over the union of dive_data_sources.computer_id and dives.computer_id,
the same union the v169 backfill uses. Design doc corrected.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Connects registered dive computers (dive_computers) to real gear items (equipment of type computer) so the computer that logged a dive is also attached as dive equipment, enabling gear-based stats, service tracking, and filtering to include computers.

Changes:

  • Adds schema v169 bridge dive_computers.equipment_id (nullable FK with onDelete: setNull) plus migration/backfill to seed deterministic gear twins and link them to relevant dives.
  • Introduces resolver/linker services to mint or adopt a gear twin once at computer registration and to attach the twin(s) to dives at non-interactive creation seams (including replaceSource).
  • Updates buoyancy mapping for EquipmentType.computer, adds UI on the device detail page to navigate to the linked gear item, and adds broad test coverage.

Reviewed changes

Copilot reviewed 49 out of 49 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test/features/equipment/data/services/gear_twin_defaulter_ordering_test.dart Regression test enforcing defaulter-before-linker ordering.
test/features/equipment/data/services/dive_computer_gear_resolver_test.dart Unit tests for gear-twin resolution and minting rules.
test/features/equipment/data/services/dive_computer_gear_linker_test.dart Unit tests for linking gear twins onto dives (multi-source, idempotent).
test/features/dive_log/data/repositories/replace_source_gear_link_test.dart Ensures replaceSource re-import path still links gear twins.
test/features/dive_computer/presentation/pages/device_detail_page_gear_twin_test.dart Widget tests for linked gear row display and navigation.
test/core/database/migration_v169_dive_computer_gear_test.dart Validates v169 ladder, column shape, and beforeOpen backstop.
test/core/database/imported_computer_gear_twin_test.dart Verifies imported-computer beforeOpen heal mints twins only on true inserts.
test/core/database/dive_computer_gear_identity_test.dart Tests deterministic v5 id and matching rules.
test/core/database/dive_computer_gear_backfill_test.dart Tests v169 backfill seeding and linking behavior, including idempotency.
test/core/buoyancy/gear_feature_test.dart Adds buoyancy/dry-mass expectations for computers.
lib/l10n/arb/app_en.arb Adds new localization key for linked gear label (en).
lib/l10n/arb/app_ar.arb Adds new localization key for linked gear label (ar).
lib/l10n/arb/app_de.arb Adds new localization key for linked gear label (de).
lib/l10n/arb/app_es.arb Adds new localization key for linked gear label (es).
lib/l10n/arb/app_fr.arb Adds new localization key for linked gear label (fr).
lib/l10n/arb/app_he.arb Adds new localization key for linked gear label (he).
lib/l10n/arb/app_hu.arb Adds new localization key for linked gear label (hu).
lib/l10n/arb/app_it.arb Adds new localization key for linked gear label (it).
lib/l10n/arb/app_nl.arb Adds new localization key for linked gear label (nl).
lib/l10n/arb/app_pt.arb Adds new localization key for linked gear label (pt).
lib/l10n/arb/app_zh.arb Adds new localization key for linked gear label (zh).
lib/l10n/arb/app_localizations.dart Adds new generated getter for diveComputer_detail_linkedGear.
lib/l10n/arb/app_localizations_en.dart Generated en implementation for new string.
lib/l10n/arb/app_localizations_ar.dart Generated ar implementation for new string.
lib/l10n/arb/app_localizations_de.dart Generated de implementation for new string.
lib/l10n/arb/app_localizations_es.dart Generated es implementation for new string.
lib/l10n/arb/app_localizations_fr.dart Generated fr implementation for new string.
lib/l10n/arb/app_localizations_he.dart Generated he implementation for new string.
lib/l10n/arb/app_localizations_hu.dart Generated hu implementation for new string.
lib/l10n/arb/app_localizations_it.dart Generated it implementation for new string.
lib/l10n/arb/app_localizations_nl.dart Generated nl implementation for new string.
lib/l10n/arb/app_localizations_pt.dart Generated pt implementation for new string.
lib/l10n/arb/app_localizations_zh.dart Generated zh implementation for new string.
lib/features/import_wizard/data/adapters/healthkit_adapter.dart Links gear twins after defaulter at HealthKit import seam.
lib/features/equipment/data/services/dive_computer_gear_resolver.dart New resolver that finds or mints a computer gear twin deterministically.
lib/features/equipment/data/services/dive_computer_gear_linker.dart New link-only service attaching twins to dives using attribution union query.
lib/features/dive_log/domain/entities/dive_computer.dart Adds equipmentId to domain entity and threading via copyWith/props.
lib/features/dive_log/data/repositories/dive_computer_repository_impl.dart Seeds twin on registration and links gear on import seams and replaceSource.
lib/features/dive_import/presentation/providers/dive_import_providers.dart Links gear twins after defaulter during import flow.
lib/features/dive_import/data/services/uddf_entity_importer.dart Links gear twins after defaulter for UDDF import.
lib/features/dive_computer/presentation/pages/device_detail_page.dart Displays linked gear item and navigates to equipment detail.
lib/core/services/sync/sync_service.dart Registers new FK in parentRefs so sync can null it safely when parent missing.
lib/core/database/imported_computer_backfill.dart Extends imported-computer backfill to mint twins only on actual insert.
lib/core/database/dive_computer_gear_identity.dart New shared deterministic id + matching logic used by resolver and migration.
lib/core/database/dive_computer_gear_backfill.dart New v169 ladder-only backfill to seed twins and link existing dives.
lib/core/database/database.dart Adds v169 schema version, column, ladder rung, and beforeOpen column assert.
lib/core/buoyancy/gear_feature.dart Ensures EquipmentType.computer contributes 0.0 dry mass and default buoyancy.
docs/superpowers/specs/2026-08-26-dive-computer-gear-twin-design.md Design spec documenting rationale, ordering, sync, and migration approach.
docs/superpowers/plans/2026-08-26-dive-computer-gear-twin.md Implementation plan outlining tasks, constraints, and verification steps.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/core/services/sync/sync_service.dart Outdated
Comment thread lib/core/database/dive_computer_gear_identity.dart Outdated
Comment thread lib/core/database/dive_computer_gear_identity.dart
Comment thread docs/superpowers/plans/2026-08-26-dive-computer-gear-twin.md Outdated
@ericgriffin ericgriffin self-assigned this Aug 27, 2026
@ericgriffin ericgriffin added the enhancement New feature or request label Aug 27, 2026
@ericgriffin ericgriffin moved this from Backlog to In review in Submersion Release Tracker Aug 27, 2026
@codecov

codecov Bot commented Aug 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.29060% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
lib/core/database/database.dart 88.88% 2 Missing ⚠️
lib/core/database/dive_computer_gear_backfill.dart 98.48% 1 Missing ⚠️
...atures/dive_log/domain/entities/dive_computer.dart 50.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@github-actions

github-actions Bot commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

📦 Build artifacts for this PR · commit db7b155

Platform Download
Android (APK) android-apk
macOS macos-build
Windows windows-build
Linux linux-build

Artifacts expire in 7 days. Downloading requires being signed in to GitHub. macOS needs two extractions: unzip the downloaded artifact, then unzip the submersion-macos.zip inside it to get a runnable submersion.app. The build is ad-hoc signed — right-click → Open on first launch.

Updated automatically on each push.

Addresses Copilot review on #1320. Three comments introduced while this
branch still claimed v168 survived the renumber, because they live outside
the six ladder places the renumber checklist covers: the gear-twin namespace
doc, the GearTwinCandidate doc, and the parentRefs comment. A repo-wide sweep
of the changed files confirms these were the only three; the remaining v168
in the migration test is correct, since that fixture upgrades FROM v168.

Also drops the literal U+2014 from the plan's own no-em-dash constraint, so
the document satisfies the rule it states.

Adds two error-path tests. Both assert a documented guarantee that nothing
covered: the resolver returns null rather than throwing when the write fails,
because a computer that fails to get a twin is still a correctly registered
computer and registration must not fail because gear seeding did; and the
linker returns false rather than throwing, because linking must never abort a
download that has already persisted the dive.
Copilot AI review requested due to automatic review settings August 27, 2026 01:11
@ericgriffin

Copy link
Copy Markdown
Member Author

All four review comments addressed in 4719275.

The three version references were real misses, and worth explaining rather than just fixing. This branch originally claimed v168; PR #1237 was renumbered from v161 onto v168 and pushed mid-implementation, so I renumbered to v169. The renumber checklist this repo uses covers six places (the scalar, the ladder entry, the assert docstring, the if (from < N) guard, its reportProgress twin, and the migration test filename with its assertions). All six moved. These three did not, because they live outside that list: two doc comments in the new identity module and the parentRefs comment in sync_service.dart. The checklist is aimed at the ladder, and a renumber mid-branch also has to sweep every comment the branch itself introduced.

I ran a repo-wide sweep of the changed files to confirm the review had found all of them. It had. The one remaining v168 is in migration_v169_dive_computer_gear_test.dart:73, 'a database at v168 gains the column and keeps its rows', which is correct: that fixture upgrades from v168, which is now a real rung owned by #1237. Copilot correctly did not flag it.

The em-dash comment was right too. That line stated a no-em-dash rule while containing the character, so the document failed its own constraint. Now reads **Never use em-dashes (U+2014)** with no literal character; the file greps clean for U+2014.

On the Codecov report (97.24% patch, 6 lines): two of the uncovered lines were the catch blocks in the resolver and linker, and those encode a guarantee the design states explicitly but nothing asserted. Added a test for each, on their merits rather than for the percentage:

  • the resolver returns null rather than throwing when the write fails, because a computer that fails to get a twin is still a correctly registered computer and registration must not fail because gear seeding did
  • the linker returns false rather than throwing, because linking must never abort a download that has already persisted the dive

Verification after the changes: flutter analyze clean, dart format . 0 changed, and 1035 tests pass across test/core/database/, test/features/equipment/ and the sync parentRefs guard. The full suite was green at 20543 passed / 19 skipped before these commits and these are additive.

Still outstanding, unchanged from the original description: no manual testing has been done. A real BLE download, a file import, and an upgrade of a populated logbook are worth exercising by hand before release. And please re-check the schema claim before merging: v169 was uncontested as of the last scan, but v168 is currently claimed by both #1237 and #1319, and the open-PR diff scan alone cannot see unpushed renumbers or local-only worktree claims.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 49 out of 49 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

lib/core/database/database.dart:4269

  • _assertDiveComputerEquipmentColumn adds equipment_id without the FK + ON DELETE SET NULL clause. For upgraded databases this means deleting an equipment row will not clear dive_computers.equipment_id, breaking the intended “deleting gear leaves the device registered” semantics and risking later FK failures when linking gear to dives.
    if (!names.contains('equipment_id')) {
      await customStatement(
        'ALTER TABLE dive_computers ADD COLUMN equipment_id TEXT',
      );

lib/core/database/imported_computer_backfill.dart:81

  • The hasGearColumn guard is too weak for the SQL below: it only checks a minimal subset of equipment columns, but the insert later writes brand, model, serial_number, status, purchase_currency, notes, and is_active. On reduced fixtures or mid-upgrade databases, this can throw despite the guard claiming it is safe.
  // v169 gear twins: absent on an older fixture, in which case the mint
  // below is skipped and the ladder seeds the twins instead.
  final equipmentCols = await columnsOf('equipment');
  final hasGearColumn =
      computerCols.contains('equipment_id') &&

lib/core/database/dive_computer_gear_backfill.dart:53

  • backfillDiveComputerGearTwins() PRAGMA-guards equipment but does not require all columns it later inserts into (status, purchase_currency, notes). If any of those columns are missing (minimal fixtures / mid-upgrade), the guard will pass but the insert will still fail.
  final equipmentCols = await columnsOf('equipment');
  if (!equipmentCols.containsAll({
    'id',
    'diver_id',
    'name',

Addresses the three suppressed findings in Copilot's second review on #1320.
The first was a real behavioural bug, not a comment nit.

_assertDiveComputerEquipmentColumn issued a bare
`ALTER TABLE dive_computers ADD COLUMN equipment_id TEXT`. A fresh database
picks up the FK from the table definition through onCreate, but an upgraded
one gets only what the ALTER says, so `onDelete: setNull` held for new
installs and not for existing users. Deleting a gear item would have left
equipment_id pointing at a row that no longer exists, and the linker would
then insert a dive_equipment row against a missing equipment id. The design
calls that setNull load-bearing; on the population this feature exists for,
it was absent. Now declared inline, matching the v158
_assertProfileSourceIdColumn precedent.

The fix has a trap of its own, surfaced by migration_v66_test: SQLite accepts
a REFERENCES clause naming a table that does not exist yet, then fails every
later write to dive_computers with "no such table: main.equipment" once
foreign keys are on. That test's fixture has no equipment table, and its
DELETE FROM dive_computers began failing. The clause is therefore added only
when equipment is present. Every real database has it, so production always
takes the FK branch, and where the fallback applies there are no gear rows
for the FK to act on.

Also widens two PRAGMA guards to name every column their INSERT writes,
rather than a subset that let the guard pass and the insert throw. The
imported-computer one matters most: it runs unguarded inside beforeOpen, so
a throw there fails app startup rather than degrading a feature.

Adds a regression test asserting an UPGRADED database carries the FK, both by
PRAGMA foreign_key_list and by the behaviour, deleting the gear row and
checking the link clears. It failed before this change.

Full suite: 20546 passed, 19 skipped. analyze clean.
Copilot AI review requested due to automatic review settings August 27, 2026 02:26
@ericgriffin

Copy link
Copy Markdown
Member Author

Second review's three suppressed findings addressed in ee3ae2d. The first was a real behavioural bug and the review deserves credit for it.

The FK finding was correct and more serious than "an inaccurate comment." _assertDiveComputerEquipmentColumn issued a bare ALTER TABLE dive_computers ADD COLUMN equipment_id TEXT. A fresh database picks up the FK from the table definition via onCreate; an upgraded one gets only what the ALTER says. So onDelete: setNull, which the design calls load-bearing, held for new installs and not for existing users, who are the entire population this feature exists for. Deleting a gear item would have left equipment_id pointing at a row that no longer exists, and the linker would then try to insert a dive_equipment row against a missing equipment id.

Confirmed empirically before fixing: PRAGMA foreign_key_list('dive_computers') on an upgraded database returned no reference to equipment at all. Now declared inline, matching the v158 _assertProfileSourceIdColumn precedent, which does exactly this and which I should have followed the first time.

The fix had a trap of its own, and the existing test suite caught it. SQLite accepts a REFERENCES clause naming a table that does not exist yet, and then fails every subsequent write to dive_computers with no such table: main.equipment once foreign keys are on. migration_v66_test.dart has a fixture with no equipment table, and its DELETE FROM dive_computers started failing. The clause is therefore added only when equipment is present. Every real database has it, so production always takes the FK branch; where the bare fallback applies there are no gear rows for the FK to act on anyway. Both the reasoning and the hazard are in the code comment so the next person does not re-simplify it.

Both guard findings were valid too. Each PRAGMA guard checked a subset of the columns its INSERT writes, so the guard could pass and the insert still throw, which is worse than no guard because callers read it as proof the write is safe. Both now name every column written. The imported_computer_backfill one matters most: it runs unguarded inside beforeOpen, so a throw there does not degrade a feature, it fails app startup.

New regression test: an upgraded database carries the FK, asserted both structurally via PRAGMA foreign_key_list and behaviourally by deleting the gear row and checking the link clears. It fails without this change.

Verification: full suite 20546 passed, 19 skipped (up 3: this test plus the two error-path tests from the previous round), flutter analyze clean, dart format . 0 changed. The design doc records the FK-on-upgrade gap and the missing-table hazard, since the spec had claimed the table-class declaration was sufficient.

One process note, in case it is useful to anyone reading later: my first attempt at that verification run reported exit code 0 with no test summary. That was a flutter test ... | tail pipeline returning the pipe's status rather than the suite's, on a run that had been killed early. The summary line, not the exit code, is what says a suite passed.

Still outstanding and unchanged: no manual testing. Given this round touched FK behaviour on the upgrade path specifically, upgrading a populated logbook and then deleting a computer's gear item is now the single most valuable thing to try by hand before merge.

ericgriffin added a commit to readme42/submersion that referenced this pull request Aug 27, 2026
Resolves the schema-ladder conflict in database.dart: main landed v170
(submersion-app#1322, gas_consumption_display) while this branch held v173, so main's
ladder entry, onUpgrade step and beforeOpen backstop are kept ahead of this
branch's and currentSchemaVersion stays at 173, which is still above main.

The v173 comment is refreshed for the new ladder: main has now also taken
170, 165 is still claimed by PR submersion-app#1290, 171 by PR submersion-app#1319 and 172 by PR submersion-app#1328,
and 169 has joined 167 as permanently skipped, since main landed 170 past
PR submersion-app#1320 and that branch moved up to 175.

analyze clean; 3,719 tests pass across core/database, dive_log and
architecture.
ericgriffin and others added 2 commits August 27, 2026 08:52
Addresses the sync finding in Copilot's sixth review on #1320.

resolveGearTwin called markRecordPending unconditionally after an
insertOrIgnore. When the insert is ignored, because a peer's twin or another
isolate landed in the race window the previous commit describes, that row is
not our write.

This is worse than wasted traffic. markRecordPending stamps an HLC on the
entity row inside its transaction (sync_repository.dart:485, _stampHlc), so
marking an adopted row bumps someone else's row to our local clock and queues
it for export. Our unchanged copy can then win a later conflict comparison
against a genuine edit made on the device that actually created the row.

Now guarded on `SELECT changes()`, the same idiom imported_computer_backfill
uses for exactly this question, which also makes the two seeding paths
consistent.

Two tests added: minting queues sync work, and adopting an existing row
queues none. Note that neither exercises the ignored-insert branch itself:
step 2 of the resolution order returns early whenever a row already holds the
derived id, so the mint cannot be reached with one present except through the
real race, which a unit test cannot stage. The guard is defensive for that
window; the tests cover the two reachable paths.
@ericgriffin ericgriffin changed the title Dive computers become equipment on the dives they logged (schema v169) Dive computers become equipment on the dives they logged (schema v175) Aug 27, 2026
The merge of main into this branch on 2026-08-27 renumbered the schema claim
from v169 to v175, after #1322 (which held v170) and others landed. The code
side of that renumber was complete and correct: scalar, ladder entry, assert
docstring, the if (from < 175) guard and its reportProgress twin, the
beforeOpen backstop comment, and the migration test filename with its
greaterThanOrEqualTo and contains assertions. Verified after the merge that
the ladder is monotonic and unique and that the scalar equals its maximum,
that lib/ and test/ carry no stale v169, and that the parentRefs entry and
all four feature files survived.

The docs had not followed. The design doc now says v175 throughout and
records both renumbers, since a reader hitting v168 or v169 in the history
should be able to tell which number shipped.

The plan keeps its v169 snippets and gains a note at the top saying so. It is
a record of the steps as executed rather than a description of current state,
and rewriting twenty snippets that were accurate when written would make it
less honest, not more.

Full suite after the merge: 21186 passed, 19 skipped. analyze clean.
Copilot AI review requested due to automatic review settings August 27, 2026 13:32
@ericgriffin

Copy link
Copy Markdown
Member Author

Sixth review's finding addressed in 4c22cbc, and the docs caught up with the v175 renumber in a49b9e1.

The markRecordPending finding was right, and worse than "unnecessary traffic." markRecordPending stamps an HLC on the entity row inside its transaction (sync_repository.dart:485, _stampHlc), so marking a row we did not write bumps someone else's row to our local clock and queues it for export. Our unchanged copy can then win a later conflict comparison against a genuine edit made on the device that actually created the row. That is a lost-update risk, not just noise.

Now guarded on SELECT changes(), the same idiom imported_computer_backfill already uses for exactly this question. That also settles the inconsistency the previous round surfaced: both seeding paths now use insert-or-ignore and both check whether the insert actually happened.

Honest limit on the test coverage. Two tests were added, that minting queues sync work and that adopting an existing row queues none. Neither exercises the ignored-insert branch itself, because step 2 of the resolution order returns early whenever a row already holds the derived id, so the mint is unreachable with one present except through the real race, which a unit test cannot stage. The guard is defensive for that window; the tests cover the two reachable paths. I would rather say that than imply the race is covered.

Separately, the schema claim moved again. Merging main into this branch renumbered it v169 -> v175, after #1322 (which held v170) and others landed. The code side of that renumber was complete and correct, and I verified rather than assumed: scalar, ladder entry, assert docstring, the if (from < 175) guard and its reportProgress twin, the beforeOpen backstop, and the migration test filename with its greaterThanOrEqualTo and contains assertions all moved together. The ladder is monotonic and unique with the scalar equal to its maximum, lib/ and test/ carry no stale v169, and the parentRefs entry plus all four feature files survived intact.

The design doc and this PR's title and description now say v175 and record both renumbers. The implementation plan deliberately keeps its v169 snippets, with a note at the top saying so: it documents the steps as executed, and rewriting twenty snippets that were accurate when written would make it less honest rather than more.

One thing worth flagging for anyone else working in this repo: the second merge left flutter analyze with 3 errors that were not from this branch. trimTankPressureAtSurfacing from the merged-in #1290 was undefined on DiverSetting, which is the signature of stale Drift codegen after a merge that adds a column, not a real conflict. dart run build_runner build --delete-conflicting-outputs clears it. Worth knowing before anyone spends time debugging a merge that is actually fine.

Verification on the merged branch: flutter analyze clean, dart format . 0 changed, full suite 21253 passed, 19 skipped, exit 0 from an unpiped run.

Still outstanding, and now the only thing I would hold a merge for: no manual testing. Upgrading a populated logbook and then deleting a computer's gear item exercises the FK-on-upgrade fix, the deletion semantics and the backfill in one pass.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 49 out of 49 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

docs/superpowers/plans/2026-08-26-dive-computer-gear-twin.md:21

  • The Global Constraints section says "Schema version is v169" in present tense, which contradicts the renumber-to-v175 note above and could mislead future readers. Consider rephrasing it to make clear v169 was the plan-time target, not the current shipped claim.
- **Schema version is v169.** Verified by diffing open PRs, not by grepping main: v165 (#1290), v166 (#1300), v167 (#1276) and v168 (#1237) are claimed by open PRs, and main is at v164. This plan originally claimed v168; #1237 renumbered onto it mid-implementation. Do NOT renumber without re-running that scan.

Comment thread lib/features/dive_log/data/repositories/dive_computer_repository_impl.dart Outdated
Comment thread docs/superpowers/plans/2026-08-26-dive-computer-gear-twin.md Outdated
Addresses Copilot's seventh review on #1320.

createComputer marked the diveComputers row pending immediately after the
insert and again after writing equipment_id. markRecordPending stamps a fresh
HLC each time, so one logical creation spent two clock ticks. The mark now
happens once, after the optional equipment_id write, so the row carries a
single HLC representing its final state.

It stays unconditional rather than moving inside the `if (twinId != null)`
branch: a computer whose twin failed to resolve is still a registered
computer and still has to sync. A test covers that, since the restructure is
exactly the kind that quietly drops a guarantee.

Note the double mark is not directly observable in sync_records, which keys
on '<entityType>_<recordId>' and upserts, so both calls collapse to one row.
The cost was the extra HLC tick, not a duplicate record.

Also updates the two places in the plan that state the schema version as
current fact rather than as a historical instruction: the Architecture
summary and the Global Constraints line. The numbered task steps keep their
v169 snippets, and the note at the top now says precisely which parts were
corrected and which were deliberately left, rather than covering the whole
document with one caveat.
Copilot AI review requested due to automatic review settings August 27, 2026 21:24
@ericgriffin

Copy link
Copy Markdown
Member Author

Seventh review's two findings addressed in 72d9fd8. Both valid.

The double pending mark was mine and unnecessary. createComputer marked the diveComputers row pending right after the insert and again after writing equipment_id. Since markRecordPending stamps a fresh HLC each time, one logical creation was spending two clock ticks. It now happens once, after the optional equipment_id write, so the row carries a single HLC representing its final state.

One detail worth stating because it changes how the finding reads: the duplicate is not observable in sync_records. That table keys on '<entityType>_<recordId>' and upserts, so both calls always collapsed to a single row. The cost was purely the extra HLC tick, which is real but narrower than a duplicated record.

I kept the mark unconditional rather than folding it into the if (twinId != null) branch. A computer whose twin failed to resolve is still a registered computer and still has to sync, and moving the only markRecordPending inside a conditional is exactly the kind of restructure that quietly drops a guarantee. There is now a test asserting the row is marked pending after the twin write, alongside one asserting the twin is seeded and stored.

The plan-doc finding drew a distinction I had missed. I had left the whole document at v169 behind a single note, reasoning that it records steps as executed. That was right for the numbered task steps and wrong for the Architecture summary and the Global Constraints line, which state the schema version as current fact rather than as a historical instruction. Those two now say v175. The task snippets keep v169 deliberately, and the note at the top says precisely which parts were corrected and which were left, instead of covering the document with one caveat that papered over the difference.

Verification: flutter analyze clean, dart format . clean, 4791 tests pass across test/features/dive_log/, test/features/equipment/, test/core/database/ and test/core/services/sync/.

Running tally over seven rounds: two real bugs (the FK missing on upgraded databases, and markRecordPending on a row we did not write), one latent bug (the upsert that could overwrite user-owned fields), one false documentation claim (D9 on sync), one wasted HLC tick, three stale version references, two over-narrow PRAGMA guards, two doc-accuracy points and two style points.

Unchanged, and still the only thing I would hold a merge for: no manual testing. Upgrading a populated logbook and then deleting a computer's gear item exercises the FK-on-upgrade fix, the deletion semantics and the backfill in a single pass.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

test/core/database/dive_computer_gear_backfill_test.dart:115

  • This helper builds SQL by interpolating diveId directly into the query string. Even in tests, this is easy to trip with IDs containing quotes and makes the pattern harder to copy safely elsewhere. Prefer escaping (or parameter binding) so the query is robust to arbitrary IDs.

Addresses the suppressed finding in Copilot's eighth review on #1320.

Two helpers in dive_computer_gear_backfill_test interpolated values straight
into SQL. The inputs are fixture constants so nothing was at risk here, but
the project's Critical Rules say "Parameterized queries only" with no
carve-out for tests, and a test is exactly where the pattern gets copied
from.

Both now bind through `variables: [Variable<String>(...)]`. Swept every test
file this PR touches rather than only the flagged line; these two were the
only occurrences.
Copilot AI review requested due to automatic review settings August 27, 2026 22:16
@ericgriffin

Copy link
Copy Markdown
Member Author

Eighth review's suppressed finding addressed in 357dcce.

Two helpers in dive_computer_gear_backfill_test.dart interpolated values directly into SQL. Nothing was at risk in practice, since the inputs are fixture constants like 'dive1', but the finding is right on the point that matters: this repo's Critical Rules say "Parameterized queries only" with no carve-out for tests, and a test helper is precisely the thing someone copies when they need the same query somewhere that does take user input.

Both now bind through variables: [Variable<String>(...)]. I swept every test file this PR touches rather than fixing only the flagged line, the same way I handled the stale version references earlier; these two were the only occurrences.

Verification: flutter analyze clean, dart format . 0 changed, 1057 tests pass across test/core/database/ and test/features/equipment/, backfill file 5/5.

Eight rounds in, the findings have moved from real defects to hygiene, which is a reasonable signal that the code has settled. For anyone picking this up: two genuine bugs were caught here that automated tests would not have found on their own, the FK missing on upgraded databases and markRecordPending firing on a row we did not write.

The one thing still outstanding is unchanged and is not something review can supply: no manual testing has been done. Upgrading a populated logbook, then deleting a computer's gear item, exercises the FK-on-upgrade fix, the deletion semantics and the backfill in a single pass. Three of the eight rounds touched that path, so it is where I would spend the manual check before merging.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated no new comments.

All four conflicts were in database.dart and all four were the same shape:
main added rungs while this branch held one, and taking either side alone
would have silently deleted the other's ladder. Every hunk keeps both, with
main's entries first and the gear-twin rung after, so the ladder stays
ascending.

  scalar          175 (ours, above main's 174)
  ladder entries  main's 171, 173, 174 then our 175
  onUpgrade rungs same order
  beforeOpen      all four backstops kept

main's own v171 comment already reserved 175 for PR #1320, so this takes the
number main expected rather than renumbering again.

Audited after resolving rather than assuming: scalar equals the ladder max,
entries are unique and monotonic, every ladder entry has a matching rung and
every rung a matching entry, and 162, 167 and 169 remain deliberately absent.
Confirmed main's three new helpers each keep one definition and two call
sites, and that the gear-twin helpers, the four feature files and the
parentRefs entry all survived.

Regenerated Drift and l10n codegen, which a merge adding columns always
invalidates. analyze clean, test/core/database/ 521 passed.
Copilot AI review requested due to automatic review settings August 28, 2026 00:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated no new comments.

@ericgriffin
ericgriffin merged commit 54e0278 into main Aug 28, 2026
26 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in Submersion Release Tracker Aug 28, 2026
@ericgriffin
ericgriffin deleted the worktree-dive-computer-gear-twin branch August 28, 2026 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants