Feature: Dives / Dive xy / Add buddy picker #638 - #1237
Conversation
Buddies in the "Add buddy" sheet can now be sorted by number of shared dives (descending by default) instead of just alphabetically, and can be marked as favorites with a star toggle that pins them to the top of the list regardless of sort. Adds buddies.is_favorite (schema v161) with the usual onUpgrade/beforeOpen migration pair. Addresses submersion-app#638. Co-authored-by: alpheios-one <275321969+alpheios-one@users.noreply.github.com>
alpheios-one
changed the title
|
📦 Build artifacts for this PR · commit
Artifacts expire in 7 days. Downloading requires being signed in to GitHub. macOS needs two extractions: unzip the downloaded artifact, then unzip the Updated automatically on each push. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
CI's format check failed because these files weren't run through dart format before the initial push. Co-authored-by: alpheios-one <275321969+alpheios-one@users.noreply.github.com>
|
@ericgriffin Can you take a look at this and fix it, I can't get it right or the main path keeps running away from me. Thank you |
|
@claude Bitte:
Wichtig: Bitte NICHT den PR schliessen/wiederöffnen während die Erst nach erfolgreichem Push einen Abschlusskommentar posten: |
main took v161 for diver_settings.default_show_o2_cell_mv (issue submersion-app#1235) while this branch was open, so buddies.is_favorite moves to the next free rung. v165, v166 and v167 are claimed by PRs submersion-app#1290, submersion-app#1300 and submersion-app#1276.
There was a problem hiding this comment.
Pull request overview
Adds “buddy picker” enhancements to support issue #638: sorting buddies by shared dive count (default) with an in-sheet sort toggle, plus favoriting buddies to pin them to the top. This includes a schema bump to v161 (buddies.is_favorite), repository APIs to toggle/set favorites, UI updates to display dive counts and star controls, and new tests covering sorting, favorites, and the migration.
Changes:
- Add
buddies.is_favorite(schema v161) withonUpgrade+beforeOpenbackstop and a dedicated migration test. - Update the Add-buddy picker sheet to use dive-count-aware providers, add a sort toggle, and add favorite starring + “Favorites” section.
- Add/extend tests for provider sorting behavior, repository dive-count query behavior, and favorite toggling.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/features/buddies/presentation/widgets/buddy_picker_test.dart | Updates picker widget tests to use dive-count-aware providers and disambiguate the “Done” button. |
| test/features/buddies/presentation/widgets/buddy_picker_roles_test.dart | Adjusts role-related picker tests for dive-count-aware buddy fixtures. |
| test/features/buddies/presentation/widgets/buddy_picker_chip_interactions_test.dart | Updates chip interaction test overrides to provide BuddyWithDiveCount. |
| test/features/buddies/presentation/providers/buddy_providers_test.dart | Adds tests for sort defaults and applyBuddyWithDiveCountSorting, plus a notifier favorite toggle test. |
| test/features/buddies/data/repositories/buddy_repository_test.dart | Adds tests for getAllBuddiesWithDiveCount (counts/query/isFavorite) and favorite toggles. |
| test/core/database/migration_v161_buddy_favorite_test.dart | New migration coverage validating v161 column existence/defaults and upgrade/backstop paths. |
| lib/features/buddies/presentation/widgets/buddy_picker.dart | Adds sort toggle UI, favorites pinning/sectioning, dive-count display, and star favorite actions. |
| lib/features/buddies/presentation/providers/buddy_providers.dart | Adds search-with-count provider, sort state for picker, improves deterministic count sorting, and adds notifier favorite toggle. |
| lib/features/buddies/presentation/pages/buddy_edit_page.dart | Preserves isFavorite on edit saves. |
| lib/features/buddies/domain/entities/buddy.dart | Introduces isFavorite field with default + copyWith/props support. |
| lib/features/buddies/data/repositories/buddy_repository.dart | Plumbs is_favorite, adds query filtering for count query, and adds toggleFavorite/setFavorite. |
| lib/features/buddies/data/repositories/buddy_merge_repository.dart | Ensures merge paths carry isFavorite. |
| lib/core/database/database.dart | Adds isFavorite column definition, bumps schema version to 161, and adds/asserts migration helper + ladder entry. |
| .github/workflows/claude.yml | Adds a Claude Code workflow that runs build_runner before invoking the Claude action. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…claude.yml Addresses the three Copilot review findings on PR submersion-app#1237. Sort toggle direction: text fields invert direction throughout this codebase, so SortDirection.descending is what renders A to Z (buddySortProvider on the standalone buddy list already defaults to name + descending for that reason). The picker's new toggle asked for ascending, which landed on the inverted branch and rendered Z to A. Three widget tests now pin the rendered order for the default sort, the toggled sort, and the toggle back. setFavorite phantom sync records: the update wrote unconditionally and then marked the record pending even when no row matched, leaving a sync record pointing at a buddy that does not exist. Drift's write() returns the affected row count, so the method now returns early on zero. toggleFavorite already guarded this with its read-before-write; both paths now have a regression test asserting sync_records stays empty for an unknown id. Removed .github/workflows/claude.yml: an issue_comment-triggered job with contents: write and pull-requests: write, gated only on the comment body containing "@claude", lets any commenter drive privileged automation with the base repo's secrets.
|
Picked this up. Two things landed on this branch. Merge conflict against upstream Worth knowing for next time: renumbering a schema claim is not a one-line edit. It touches six places, and missing any one of them ships a broken ladder:
The migration test was renamed to Also worth flagging, because it is the trap behind this whole class of conflict: when two branches write the same schema number, git auto-merges them with no conflict marker at all. The conflict you hit was the visible, lucky case. The three Copilot findings are addressed, each answered in its own thread:
The branch has also been brought up to date with upstream Verified locally on the branch: The push itself used Thanks for the contribution, and for flagging that you were stuck rather than force-pushing at it. |
|
Addendum to the comment above, plus a correction to my own earlier wording. On the hand-patched generated code. You flagged in the description that What is actually verified, which is the reassurance that matters: and CI's Code Generation job passed on this branch from a clean checkout. So the committed sources regenerate cleanly and the generated layer is consistent with them. That caveat in your description is resolved, just for a different reason than I first said: there was nothing to hand-patch into the PR in the first place. Merge conflict, third round. Verified after the merge: |
v168 was already claimed and pushed by PR #1237 (issue #638, buddies.is_favorite). That claim was resolved locally and not yet pushed when this branch picked its number, so the open-PR diff scan this plan prescribes could not see it. 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. Moves all six coupled sites together plus the test filename: the scalar, the migrationVersions entry, the _assertTripDayWeatherSchema docstring, the onUpgrade guard, its reportProgress twin, and the beforeOpen backstop comment. The migration test is renamed to migration_v171_trip_day_weather_test.dart with its greaterThanOrEqualTo and contains assertions updated. Its stranded-database fixture moves from 168 to 171: 168 is now a real rung owned by #1237, so a database stamped there upgrades normally and no longer exercises the backstop. The design doc and plan are updated too, including the reasoning passages that were wrong rather than merely stale: the plan recorded #1237 as a stale v161 claim when it was a live v168 one. Both now say to scan every worktree's working-tree scalar alongside open PR diffs, and to re-run both immediately before pushing rather than only when picking the number. Ladder is left non-contiguous by design: 165 #1290, 166 #1300, 167 #1276, 168 #1237, 169 the dive-computer gear-twin branch, 170 #1322. Verified: ladder monotonic, unique, scalar == max, 168 absent; helper defined exactly once and referenced three times; guard and twin both at 171; flutter analyze clean; flutter test test/core/database/ 471 passed.
…6 + v170) Two conflicts, both from main moving under this branch: - database.dart: the ladder took v166 (place_name_language, #1187). All three hunks are additive, so both rungs stay, ordered by version, with main's comment text verbatim; the scalar takes the max. v170's comment now names only the rungs still outstanding (167 #1276, 168 #1237, 169 a parallel worktree). The floor stays at 170: a defaulted column does not raise it, but this branch's synced-column rename does. - dive_detail_page.dart: main extracted the Cylinders card into _cylindersCard so it can also render inside the Cylinders/Weights pair (#1306). Took main's structure and moved this branch's parameter change into the helper, so both call sites get it.
# Conflicts: # lib/core/database/database.dart
github-project-automation
Bot
moved this from In progress
to Done
in Submersion Release Tracker
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 submersion-app#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.
Summary
buddies.is_favorite(schema v168, renumbered from v161 by a maintainer: v161 was taken bydefault_show_o2_cell_mvon main) with the standardonUpgrade/beforeOpenmigration pair, plusBuddyRepository.toggleFavoriteandsetFavorite. Reuses the existing dive-count join that already powers the standalone buddy list page.Addresses #638.
This PR was authored in an automated session without local
build_runneraccess, so parts of the generated database code were hand-patched rather than regenerated directly:lib/core/database/database.g.dart— manually updated for the newis_favoritecolumn.mocks.dartfiles — manually updated for the newgetAllBuddiesWithDiveCountsignatureI'd especially appreciate a close look at:
onUpgrade/beforeOpenpair) -- reviewed and renumbered to v168build_runnerregenerates the branch with no diff, and CI's Code Generation job passesHappy to address any feedback.
Maintainer note (2026-08-26). Picked up and pushed to the contributor's branch: the three Copilot findings are fixed and answered in-thread, the merge conflict against current
mainis resolved, the migration is renumbered v161 to v168, and the generated layer is confirmed consistent (those files are gitignored, so nothing hand-patched reached the PR;build_runnerleaves no diff and CI's Code Generation job passes). Details in the comment thread below.