Feature/duration bottomtime rename sac fix#95
Merged
Conversation
Pure mechanical rename with zero behavior change. Renames Dive.duration -> Dive.bottomTime and DiveSummary.duration -> DiveSummary.bottomTime across domain entities, database SQL, repositories, import/export services, presentation layer, and all tests. All 3001 tests pass identically.
Add edge case tests for effectiveRuntime (negative/zero entry-exit, missing exit, empty profile, single profile point). Add comprehensive sac/sacPressure tests covering all null-return branches, tank edge cases (missing volume/pressures, zero usage), multi-tank aggregation, and issue regression tests for #72 and #87. 13 effectiveRuntime tests + 25 SAC tests = 38 total (was 11).
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes inflated SAC/SAC-pressure calculations by using runtime (via a new Dive.effectiveRuntime fallback chain) instead of bottom time, and mechanically renames Dive.duration → Dive.bottomTime across the codebase (including a DB column rename migration).
Changes:
- Rename
Dive.durationtoDive.bottomTimeacross domain/data/presentation/tests and update DB schema + SQL queries accordingly. - Add
Dive.effectiveRuntimeand update SAC/SAC-pressure (and related gas analysis/statistics) to use runtime semantics. - Add new unit tests for
effectiveRuntimeand SAC regressions.
Reviewed changes
Copilot reviewed 78 out of 78 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/uddf_test_importer.dart | Update test UDDF importer to populate bottomTime and related profile fallback. |
| test/helpers/performance_data_generator.dart | Update generated test data to write bottomTime column. |
| test/features/universal_import/data/services/import_duplicate_checker_test.dart | Update duplicate-checker tests to use bottomTime. |
| test/features/media/data/services/trip_media_scanner_test.dart | Update scanner tests to use bottomTime. |
| test/features/import_wizard/data/adapters/healthkit_adapter_test.dart | Update HealthKit adapter tests to use bottomTime. |
| test/features/import_wizard/data/adapters/fit_adapter_test.dart | Update FIT adapter tests to use bottomTime. |
| test/features/dive_log/presentation/widgets/merge_dive_dialog_test.dart | Update merge dialog widget tests to use bottomTime. |
| test/features/dive_log/integration/multi_computer_integration_test.dart | Update integration seeding to write bottomTime. |
| test/features/dive_log/domain/services/field_attribution_service_test.dart | Update attribution test keys from duration to bottomTime. |
| test/features/dive_log/domain/models/dive_filter_state_test.dart | Rename filter fields to min/maxBottomTimeMinutes and update tests. |
| test/features/dive_log/domain/entities/dive_sac_fix_test.dart | New SAC/SAC-pressure regression tests using runtime semantics. |
| test/features/dive_log/domain/entities/dive_effective_runtime_test.dart | New unit tests validating effectiveRuntime fallback chain and edge cases. |
| test/features/dive_log/data/repositories/dive_repository_test.dart | Update repository tests to assert bottomTime. |
| test/features/dive_log/data/repositories/dive_repository_new_methods_test.dart | Update repository method tests to use bottomTime DB field. |
| test/features/dive_log/data/repositories/dive_consolidation_test.dart | Update consolidation tests to write/verify bottomTime. |
| test/features/dive_log/data/repositories/dive_computer_repository_impl_test.dart | Update computer repo tests to write bottomTime. |
| test/features/dive_log/data/repositories/dive_computer_data_repository_test.dart | Update computer data repo tests to write bottomTime. |
| test/features/dive_import/presentation/providers/dive_import_notifier_test.dart | Update import notifier test dive construction to use bottomTime. |
| test/features/dive_import/domain/services/imported_dive_converter_test.dart | Update converter tests to assert bottomTime behavior. |
| test/features/dive_import/data/services/uddf_duplicate_checker_test.dart | Update UDDF duplicate-checker tests to use bottomTime. |
| test/core/domain/models/dive_comparison_result_test.dart | Update comparison result test dive construction to use bottomTime. |
| test/core/constants/card_color_test.dart | Update card color test to use bottomTime. |
| lib/features/universal_import/data/services/import_duplicate_checker.dart | Use bottomTime for duration fallback when runtime/entry/exit aren’t available. |
| lib/features/trips/presentation/widgets/trip_overview_tab.dart | Display bottomTime instead of duration in trip overview UI. |
| lib/features/trips/presentation/widgets/trip_daily_breakdown.dart | Aggregate trip total bottom time using bottomTime. |
| lib/features/trips/presentation/pages/trip_detail_page.dart | Display bottomTime instead of duration in trip detail UI. |
| lib/features/trips/data/repositories/trip_repository.dart | Update trip summary SQL to sum bottom_time. |
| lib/features/statistics/data/repositories/statistics_repository.dart | Update SAC-related SQL to use COALESCE(runtime, bottom_time); update avg duration to bottom_time. |
| lib/features/settings/presentation/providers/export_providers.dart | Update export provider test/fixture mapping to bottomTime. |
| lib/features/media/presentation/helpers/photo_import_helper.dart | Switch photo matching window to effectiveRuntime (replacing calculatedDuration). |
| lib/features/media/data/services/trip_media_scanner.dart | Update exit-time fallback to use bottomTime. |
| lib/features/maps/presentation/pages/dive_activity_map_page.dart | Update map UI chips to show bottomTime. |
| lib/features/import_wizard/data/adapters/healthkit_adapter.dart | Update duration fallback to use bottomTime. |
| lib/features/import_wizard/data/adapters/fit_adapter.dart | Update duration fallback to use bottomTime. |
| lib/features/divers/data/repositories/diver_repository.dart | Update total-time SQL to sum bottom_time. |
| lib/features/dive_planner/presentation/providers/dive_planner_providers.dart | Create planned dives with bottomTime rather than duration. |
| lib/features/dive_log/presentation/widgets/merge_dive_dialog.dart | Display bottomTime in merge candidate UI. |
| lib/features/dive_log/presentation/widgets/dive_map_content.dart | Update map content to show bottomTime. |
| lib/features/dive_log/presentation/widgets/dive_list_content.dart | Prefer runtime ?? bottomTime when displaying “duration” in list tiles. |
| lib/features/dive_log/presentation/providers/dive_providers.dart | Sort by DiveSortField.bottomTime using bottomTime. |
| lib/features/dive_log/presentation/pages/dive_search_page.dart | Wire UI state to min/maxBottomTimeMinutes filter fields. |
| lib/features/dive_log/presentation/pages/dive_list_page.dart | Wire filter sheet to min/maxBottomTimeMinutes and list tiles to bottomTime. |
| lib/features/dive_log/presentation/pages/dive_edit_page.dart | Update edit form initialization/save paths to use bottomTime; runtime calc uses entry/exit. |
| lib/features/dive_log/presentation/pages/dive_detail_page.dart | Display bottomTime + attribution key; use effectiveRuntime for profile chart duration. |
| lib/features/dive_log/domain/services/field_attribution_service.dart | Attribute bottomTime field key from multi-source DiveDataSource.duration. |
| lib/features/dive_log/domain/models/dive_filter_state.dart | Rename filter fields to min/maxBottomTimeMinutes and apply to bottomTime. |
| lib/features/dive_log/domain/entities/dive_summary.dart | Rename summary field to bottomTime. |
| lib/features/dive_log/domain/entities/dive.dart | Rename duration → bottomTime; add effectiveRuntime; update sac/sacPressure to use runtime semantics. |
| lib/features/dive_log/data/services/gas_analysis_service.dart | Use effectiveRuntime for analysis range end time. |
| lib/features/dive_log/data/repositories/dive_repository_impl.dart | Persist/load bottom_time; update sort/filter SQL; update records queries and data-source backfill to map bottom time. |
| lib/features/dive_log/data/repositories/dive_computer_repository_impl.dart | Update queries/inserts to use bottom_time and persist bottom time seconds. |
| lib/features/dive_import/presentation/providers/dive_import_providers.dart | Use runtime ?? bottomTime for duplicate matching scoring inputs. |
| lib/features/dive_import/domain/services/imported_dive_converter.dart | Store computed bottom time via copyWith(bottomTime: ...). |
| lib/features/dive_import/data/services/uddf_entity_importer.dart | Map UDDF duration to bottomTime, and persist to DB correctly. |
| lib/features/dive_import/data/services/uddf_duplicate_checker.dart | Update duration fallback to use bottomTime. |
| lib/features/dashboard/presentation/widgets/recent_dives_card.dart | Prefer runtime ?? bottomTime for dashboard “duration” display. |
| lib/features/dashboard/presentation/widgets/personal_records_card.dart | Display longest dive based on bottomTime. |
| lib/features/dashboard/presentation/providers/dashboard_providers.dart | Compute longest dive using bottomTime. |
| lib/features/buddies/presentation/pages/buddy_detail_page.dart | Display bottomTime in buddy detail dive list items. |
| lib/core/services/sync/sync_data_serializer.dart | Serialize bottomTime under the existing duration key for sync payloads. |
| lib/core/services/pdf_templates/pdf_template_simple.dart | PDF output uses bottomTime for duration field. |
| lib/core/services/pdf_templates/pdf_template_professional.dart | PDF output uses bottomTime for duration field. |
| lib/core/services/pdf_templates/pdf_template_padi.dart | PADI PDF output uses bottomTime. |
| lib/core/services/pdf_templates/pdf_template_naui.dart | NAUI PDF output totals/fields use bottomTime. |
| lib/core/services/pdf_templates/pdf_template_detailed.dart | Detailed PDF output uses bottomTime. |
| lib/core/services/pdf_templates/pdf_shared_components.dart | Shared PDF totals use bottomTime. |
| lib/core/services/export/uddf/uddf_export_service.dart | UDDF export uses bottomTime and generated profile duration uses bottom time seconds. |
| lib/core/services/export/uddf/uddf_export_builders.dart | UDDF builders use bottomTime. |
| lib/core/services/export/pdf/pdf_export_service.dart | PDF export uses bottomTime for duration fields and totals. |
| lib/core/services/export/pdf/pdf_course_export_service.dart | Course PDF totals/fields use bottomTime. |
| lib/core/services/export/kml/kml_export_service.dart | KML export uses bottomTime for duration string. |
| lib/core/services/export/excel/excel_export_service.dart | Excel export uses bottomTime for duration + summary stats. |
| lib/core/services/export/csv/csv_export_service.dart | CSV export uses bottomTime for duration column. |
| lib/core/database/database.dart | DB schema version bump + dives.duration column rename migration to bottom_time; drift column renamed to bottomTime. |
| lib/core/constants/sort_options.dart | Rename sort field enum value to bottomTime. |
| lib/core/constants/card_color.dart | Card color attribute “duration” now uses bottomTime minutes. |
| integration_test/helpers/uddf_screenshot_helper.dart | Update screenshot importer seed data to use bottomTime. |
| integration_test/helpers/screenshot_test_data.dart | Update screenshot seeding to write bottomTime. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code Review - Stale attribution key for runtime statFile: lib/features/dive_log/presentation/pages/dive_detail_page.dart, line 712 This PR correctly updates field_attribution_service.dart to write attribution[bottomTime] instead of attribution[duration], and updates the bottom time stat item lookup. However, the runtime stat item (line 712) was not updated and still reads attribution?[duration], which is no longer set by the attribution service. Since attribution[duration] is no longer populated, the source attribution badge will silently never appear for the runtime stat on multi-source dives. See: submersion/lib/features/dive_log/presentation/pages/dive_detail_page.dart Lines 707 to 714 in 898bc08 Fix: Change line 712 to sourceName: attribution?[bottomTime], |
Member
Author
Code reviewFound 2 issues:
submersion/lib/features/dive_log/presentation/pages/dive_detail_page.dart Lines 711 to 713 in 898bc08
submersion/lib/features/dive_log/presentation/widgets/dive_list_content.dart Lines 1207 to 1209 in 898bc08 |
- Fix attribution key mismatch: dive_detail_page.dart:712 now reads attribution['bottomTime'] instead of stale 'duration' key - Fix dive planner: totalTime (all segments) stored as runtime instead of bottomTime, since it includes descent/ascent/deco phases - Rename DiveRecord.duration to DiveRecord.bottomTime for consistency - Fix trip_media_scanner to use effectiveRuntime for dive bounds - Clarify issue #87 regression test description and comments - Update records_page and dive_summary_widget for DiveRecord rename
Add 82 new tests across 21 test files covering the duration-to-bottomTime rename and SAC calculation fixes. Tests span all layers: - Entity tests: DiveSummary with bottomTime field - Service tests: GasAnalysisService.calculateCylinderSac with effectiveRuntime - Repository tests: DiveRepository sort/filter/mapping, StatisticsRepository SAC queries with COALESCE(runtime, bottom_time) - Export tests: Excel, PDF, KML, CSV exports with bottomTime data, all 5 PDF templates (simple, detailed, professional, padi, naui) - Provider tests: dive sort provider, dashboard personal records - Widget tests: DiveDetailPage, DiveEditPage, DiveListPage, DiveSearchPage, DiveSummaryWidget, PersonalRecordsCard, RecentDivesCard, BuddyDetailPage, TripOverviewTab, TripDetailPage Also adds shared mock_providers.dart test helper with MockSettingsNotifier, MockCurrentDiverIdNotifier, and common test utilities.
Tests that the filter sheet initializes minBottomTimeMinutes and maxBottomTimeMinutes fields from the pre-set DiveFilterState.
- Add surface interval test exercising effectiveRuntime fallback - Add gas analysis test for profile-timestamp diveEnd fallback - Add UDDF export builder test for synthetic profile from bottomTime - Add dive detail page test with attribution badges and data sources - Improve dive edit page profile test with realistic depth data
- Add TripDailyBreakdown test exercising _totalBottomTimeMinutes - Add liveaboard trip detail test covering _buildDivesTab bottomTime
ericgriffin
added a commit
that referenced
this pull request
Mar 28, 2026
PR #95 (duration->bottomTime rename) claimed migration v56. Bump the dive_detail_sections column addition to v57 and update schemaVersion.
ericgriffin
added a commit
that referenced
this pull request
Mar 28, 2026
- Bump migration from v56 to v57 (PR #95 claimed v56 for the duration->bottom_time column rename) - Regenerate Drift code against updated schema - Add missing setDiveDetailSections/resetDiveDetailSections stubs to MockSettingsNotifier
ericgriffin
added a commit
that referenced
this pull request
Mar 28, 2026
* feat: add DiveDetailSectionId enum and DiveDetailSectionConfig model Define the 17 configurable section IDs, their display metadata, and JSON serialization for database storage. Includes ensureAllSections for forward-compatible new-section detection. * feat: add diveDetailSections column to diver_settings (v56) Nullable TEXT column stores JSON section config. Null means use defaults. No data backfill needed for existing divers. * feat: add diveDetailSections to AppSettings and SettingsNotifier New field with copyWith support and clear-to-defaults flag. SettingsNotifier gets set and reset methods for the section config. * feat: wire diveDetailSections through DiverSettingsRepository JSON serialization via DiveDetailSectionConfig.sectionsFromJson/toJson. Stores null when config matches defaults to avoid unnecessary writes. * feat: add DiveDetailSectionsPage with drag-and-drop reordering ReorderableListView with visibility toggle switches per section. Reset to Default in overflow menu. Dimmed styling for hidden sections. * feat: add Dive Details section to Appearance page with route Navigation tile in Appearance links to /settings/dive-detail-sections. New GoRoute wires the DiveDetailSectionsPage into the router. * feat: config-driven section rendering on dive detail page Replace hardcoded section order with a registry map and loop over the user's diveDetailSections config. Fixed sections (header, profile) remain at the top. All data-driven auto-hide conditions preserved. * style: fix prefer_const lint hints in tests * fix: always serialize diveDetailSections to JSON in repository The previous code used List == (reference equality) to detect defaults and write null. This always evaluates to false for loaded lists, breaking the reset-to-defaults flow. Simplify by always writing JSON. * fix: add Dive Details section to desktop appearance settings The settings page uses a separate _AppearanceSectionContent widget for the master-detail layout on desktop. The new section was only added to the mobile AppearancePage, not the desktop inline version. * test: increase coverage for dive detail section config Add 17 new tests (32 -> 49 total) covering: - Model: fromJson missing visible field, fromJson unknown ID throws, copyWith no-args, round-trip serialization, ensureAllSections with empty list and custom visibility preservation - Widget: toggle switch updates visibility, hidden section has reduced opacity, reset-to-default menu, initial switch state, app bar title, section descriptions - AppSettings: copyWith preserves when unset, defaults match static const, clearDiveDetailSections takes precedence * test: expand coverage for dive detail section config Add 20 more tests (49 -> 69 total) covering: - Model: fromJson explicit false, single-item round-trip, tryFromJson success/missing-id/non-string-id, sectionsFromJson with map-JSON/ non-map-list-items/all-unknown-IDs/mixed-types, ensureAllSections preserves custom visibility, every displayName/description value, unique displayName constraint - Widget: toggle back on restores opacity, multiple independent toggles, overflow menu present, subheading text, custom initial section order via provider override - AppSettings: sequential copyWith independence, empty list, custom constructor * feat: localize dive detail section names and descriptions Add localizedDisplayName() and localizedDescription() to DiveDetailSectionId backed by ARB strings for all supported locales. Update settings and appearance pages to use localized section metadata. Add DiveDetailPage widget tests verifying section config visibility, ordering, and content rendering. * test: expand dive detail page section config coverage Add widget tests for dataSources, buddies, sightings, signatures, details, and all-sections-together rendering. Extract _buildTestWidget extraOverrides parameter and _alwaysRenderOverrides helper for sections that require additional provider stubs. * test: add appearance page tests and expand dive detail section config coverage Add widget tests for AppearancePage verifying the Dive Details section header, tile content, and reorder icon. Extend section config tests with dataSources, buddies, sightings, signatures, details, and all-sections rendering using extraOverrides for provider-dependent sections. * fix: resolve merge conflicts with main after PR #95 merge - Bump migration from v56 to v57 (PR #95 claimed v56 for the duration->bottom_time column rename) - Regenerate Drift code against updated schema - Add missing setDiveDetailSections/resetDiveDetailSections stubs to MockSettingsNotifier
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
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.
Summary
Fix SAC (Surface Air Consumption) calculations that were inflated by 1.5-2x because they used bottom time instead of runtime. Also rename
Dive.durationtoDive.bottomTimeto eliminate the naming confusion that caused this bug.Fixes #72, fixes #87 (SAC portion).
Changes
Dive.durationtoDive.bottomTimeacross 74 files (domain entities, database column, repository SQL, import/export services, presentation, tests). Pure mechanical refactor with zero behavior change.ALTER TABLE dives RENAME COLUMN duration TO bottom_timeDive.effectiveRuntimegetter with fallback chain:runtime->exitTime - entryTime-> profile-based ->bottomTimeDive.sacandDive.sacPressureto useeffectiveRuntimeinstead ofbottomTimeGasAnalysisService.calculateCylinderSacto useeffectiveRuntimefor tank usage rangeCOALESCE(d.runtime, d.bottom_time)for SAC calculationscalculatedDurationgetter witheffectiveRuntime— the old getter conflated runtime and bottom time; all 4 callers wanted runtimeDiveSortField.durationtoDiveSortField.bottomTime,minDurationMinutes/maxDurationMinutestominBottomTimeMinutes/maxBottomTimeMinutesTest Plan
flutter testpasses (3039 tests, 0 failures)flutter analyzepasses (0 issues)effectiveRuntime(13 tests) andsac/sacPressure(25 tests)DiveDataSource.duration,ImportedDive.duration,IncomingDiveData.durationSeconds,ComparisonFieldType.durationDive.durationorcalculatedDurationreferences