blood work, and an export a spreadsheet can open - #219
Conversation
Two requests that turned out to share a shape: both are about data the band cannot produce. Labs. Twenty-five common markers with units and typical adult reference intervals, plus anything else you want to name. The intervals are there so a number means something — a bare ferritin is a spreadsheet with extra steps — but they are context, not a verdict. Every laboratory publishes its own interval and the one on your report is the one that applies to you, so a value outside ours reads as "outside the typical range" rather than "high", and nothing here suggests what to do about it. Several markers differ by sex and say so; without a sex in the profile they show no interval at all rather than picking one, because guessing would mark a large share of normal results as abnormal. A marker the app has never heard of gets no range invented for it. Each result carries the unit it was entered under, so a reading is never silently reinterpreted if the catalogue changes later, and a result is keyed on the marker and the date drawn, so fixing a typo corrects the value instead of stacking a second reading a chart would then average. CSV export sits beside the existing .db export, because opening your data in a spreadsheet and restoring it onto a new phone are different jobs and a SQLite file only does the second. Six files — daily metrics, workouts, sleep stages, metric history, journal, labs — all read through the same derived views the coach reads, so an export can never contain something the app itself would not show you and can never reach raw sensor rows or GPS. A metric that was never computed is an empty field, never a zero: nobody can recover the difference once it is in a spreadsheet, and a column of zeroes is something people average.
📝 WalkthroughWalkthroughThe change adds persistent lab-result storage, a marker catalogue with reference ranges, Labs entry and history screens, CSV exports for six datasets, profile actions, and migration and integration tests. ChangesLabs and CSV export
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant LabsScreen
participant LabEntrySheet
participant LabCatalogue
participant LocalDb
LabsScreen->>LabEntrySheet: open result form
LabEntrySheet->>LabCatalogue: resolve marker metadata
LabEntrySheet->>LocalDb: save or delete result
LocalDb-->>LabsScreen: return stored results
LabsScreen->>LabCatalogue: evaluate reference range
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Reviewer Guide 🔍(Review updated until commit 146ad8c)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 146ad8c Previous suggestionsSuggestions up to commit 6154a86Suggestions up to commit f81a1f0
Suggestions up to commit 189e262
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/data/csv_export.dart`:
- Around line 156-190: Update exportCsvFiles to use a dedicated export
subdirectory under the temporary directory, clear its existing contents at the
start of each export, and then write the generated CSV files there while
preserving the current skipped-set behavior and returned paths.
- Around line 172-189: Update the export flow around the loop over sets to skip
file creation and path insertion when a query returns no rows, while preserving
header/data output for non-empty results. Replace the discarded catch in the
export method with failure tracking that records each failed set name, including
query and write failures, and return both successful paths and failed set names
so the profile caller can distinguish empty data from total or partial export
failure.
- Around line 130-142: Update csvField to neutralize formula-like text values by
prefixing an apostrophe when a String begins with =, +, -, @, tab, or carriage
return. Apply this only to text inputs so numeric values, including negative
numbers, retain their existing export format, while preserving the current CSV
escaping and quoting behavior.
In `@lib/data/db.dart`:
- Around line 1315-1317: Clarify the lab-result deletion contract in the
documentation near lab_result.taken_on: explicitly state whether deleteDays
should preserve lab results. If preservation is intentional, update the
docstring to cover explicit day deletion as well as retention pruning;
otherwise, update deleteDays to delete matching lab_result rows.
- Around line 405-409: Update the migration ladder around the existing oldV < 29
block to add a sequential oldV < 28 migration step before it, invoking the
appropriate version-28 migration logic; ensure databases upgrading from version
27 execute version-28 migrations before the existing version-29 _createLabTables
step.
- Around line 1320-1333: Remove the CREATE INDEX statement for
idx_lab_result_marker from the lab_result schema setup, leaving the PRIMARY KEY
(marker, taken_on) definition unchanged so SQLite’s implicit index remains the
sole index for those columns.
In `@lib/data/lab_catalogue.dart`:
- Around line 386-394: Update _saveCustomMarker to load existing marker
definitions before writing and compare any definition using customLabMarkerKey’s
generated key against the incoming label and unit. Reject the new input or
require explicit user confirmation when either differs; only call
LocalDb.putLabMarkerDef for a non-colliding key or confirmed replacement.
In `@lib/ui/labs/lab_entry_sheet.dart`:
- Around line 104-114: Use the stored result unit as the authority: in
lib/ui/labs/lab_entry_sheet.dart lines 104-114, save widget.existing?['unit']
when labMarker(key, custom: widget.custom) is null, and render _valueFields for
edits even without a marker; in lib/ui/labs/labs_screen.dart lines 289-293,
display r['unit'] alongside m.format(...) instead of using
m.formatWithUnit(...).
- Around line 80-91: Update _pickDate so the initialDate passed to
showDatePicker is clamped between its firstDate (ten years before now) and
lastDate (now), while preserving _takenOn when it is already within that range
and continuing to update it only when a date is picked.
- Around line 76-78: Update the _dateLabel getter to use dayLabelOf from
data/day_label.dart instead of manually constructing the YYYY-MM-DD string,
adding the required import and preserving the existing _takenOn date value.
In `@lib/ui/labs/labs_screen.dart`:
- Around line 79-80: Update the _sex access used by build so it subscribes to
AppState changes via context.watch or context.select instead of context.read,
ensuring profile sex updates trigger recomputation of reference intervals. Pass
the subscribed value through the existing build flow rather than relying on the
non-reactive getter.
In `@test/csv_export_test.dart`:
- Around line 81-149: Add tests for exportCsvFiles covering the UTF-8 BOM,
timestamped filenames, returned paths, and skipping a set whose SQL query fails
while continuing with valid sets. Mock getTemporaryDirectory through the
plugins.flutter.io/path_provider channel, inspect the written files for the EF
BB BF prefix, and assert the invalid set is omitted while valid exports still
produce paths.
In `@test/lab_result_store_test.dart`:
- Around line 16-32: Add a migration regression test in the lab result store
test setup that creates and closes a database at a version below 29, then
reopens it through LocalDb to exercise the _createLabTables upgrade path. Assert
that lab_result and lab_marker_def exist and can accept writes, while preserving
the existing fresh-database tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5c5cdc0d-302a-47ae-9358-53b9befda7b4
📒 Files selected for processing (9)
lib/data/csv_export.dartlib/data/db.dartlib/data/lab_catalogue.dartlib/ui/labs/lab_entry_sheet.dartlib/ui/labs/labs_screen.dartlib/ui/profile/profile_screen.darttest/csv_export_test.darttest/lab_catalogue_test.darttest/lab_result_store_test.dart
| void main() { | ||
| setUpAll(() async { | ||
| sqfliteFfiInit(); | ||
| databaseFactory = databaseFactoryFfi; | ||
| LocalDb.dbName = 'openstrap_lab_result_test.db'; | ||
| await databaseFactory.deleteDatabase( | ||
| p.join(await databaseFactory.getDatabasesPath(), LocalDb.dbName), | ||
| ); | ||
| }); | ||
|
|
||
| tearDownAll(() async => LocalDb.close()); | ||
|
|
||
| setUp(() async { | ||
| final db = await LocalDb.instance; | ||
| await db.delete('lab_result'); | ||
| await db.delete('lab_marker_def'); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a migration regression test for the lab tables.
These tests only exercise a freshly created database, where _createUserTables builds the lab tables in onCreate. The upgrade path (oldV < 29 → _createLabTables) has no coverage, and that path is the one that can brick an existing install if the ladder is wrong. Open a database at an older version, close it, reopen through LocalDb, and assert that lab_result and lab_marker_def exist and accept a write.
The coding guidelines require regression tests for migrations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/lab_result_store_test.dart` around lines 16 - 32, Add a migration
regression test in the lab result store test setup that creates and closes a
database at a version below 29, then reopens it through LocalDb to exercise the
_createLabTables upgrade path. Assert that lab_result and lab_marker_def exist
and can accept writes, while preserving the existing fresh-database tests.
Source: Coding guidelines
Journal notes, tags and lab notes are free text, and a cell starting with =, +, -, @ or a control character is a formula in every mainstream spreadsheet. These files go through a share sheet, so the person who opens them executes it. Text cells are neutralised now; numbers are left alone, or every negative delta in the file would stop being a number. The files were also left in the temp directory forever, one more copy of your readiness, sleep, notes and blood work per export. They go in a directory that is wiped at the start of each run. An export could not tell "you have no data yet" from "the export broke": every set wrote a header-only file even with no rows, and failures were swallowed whole. Empty sets write nothing, failures come back by name, and the message says which happened. Labs: a row's own unit is the authority everywhere, not the marker definition — editing a reading keeps the unit it was measured in, and a reading whose marker was deleted stays readable and editable. Two custom names that slug to the same key no longer silently overwrite each other's definition. The date picker no longer throws on a draw older than its own window. The date label goes through the shared day-label seam. The reference range re-resolves when the profile's sex changes. And the redundant index is gone: the primary key already covers exactly those columns. Schema is 28 here rather than 29, so the ladder has no hole when this lands on its own.
|
All thirteen were real. The formula injection one is the one I am glad was caught — these files are handed straight to a share sheet, so a journal note beginning with The temp-file accumulation was the same class of problem in slow motion — plaintext readiness, sleep, journal notes and blood work, one more copy per export, nothing ever removing them. One export's worth exists at a time now. The return contract was genuinely broken and I had not noticed: every set wrote a header-only file even with zero rows, so the caller's "nothing to export" branch was only reachable when everything failed, which meant a total failure was reported to the user as an empty database. Empty sets write nothing, failures come back by name, and the two messages are now different sentences. On the day deletion question — the exemption is intentional and the docstring says so now. "Delete this day" in the data manager is about reclaiming space from sensor data; a blood panel is neither sensor data nor large, it was typed in by hand on a different screen, and it has its own delete there. Losing a year-old result because the band data from that date was cleared would be a surprising deletion, not a tidy one. The unit-authority finding was the most useful of the batch. The storage layer was already right — every row carries the unit it was entered under — but both UI sites read the unit from the resolved marker instead, which quietly undid that. Editing a row now keeps its own unit, and a reading whose custom marker was deleted stays fully readable and editable rather than becoming a number nobody can correct. Schema is 28 rather than 29 now, so the ladder has no hole if this lands before #218. |
|
Persistent review updated to latest commit f81a1f0 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/csv_export_test.dart (1)
101-111: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake each export test independent of sibling-group state.
Lines 101-111 configure
databaseFactoryandLocalDb.dbNamein a sibling group'ssetUpAll. A name-filteredexportCsvFilestest does not run that setup. Lines 237-245 also require the daily row inserted by a different test.Move database setup and cleanup to an ancestor scope. Use a local non-empty
CsvExportSetin the cleanup test so it does not depend on prior test data.Proposed test-local fixture
test('a run clears the previous run rather than piling up copies', () async { - final daily = kCsvExportSets.firstWhere((s) => s.name == 'daily'); - final first = await exportCsvFiles([daily], now: DateTime(2026, 7, 1)); + const nonEmpty = CsvExportSet( + name: 'cleanup', + title: 'Cleanup', + columns: ['value'], + sql: 'SELECT 1 AS value', + ); + final first = + await exportCsvFiles([nonEmpty], now: DateTime(2026, 7, 1)); expect(first.paths, hasLength(1)); - final second = await exportCsvFiles([daily], now: DateTime(2026, 7, 2)); + final second = + await exportCsvFiles([nonEmpty], now: DateTime(2026, 7, 2));As per coding guidelines: “Behavior changes, especially regressions involving readiness, abstention, idempotence, synchronization, migrations, and lifecycle safety, must include regression tests.”
Also applies to: 196-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/csv_export_test.dart` around lines 101 - 111, Move the sqflite initialization, databaseFactory assignment, LocalDb.dbName configuration, database deletion, and LocalDb.close cleanup from the sibling-group setUpAll/tearDownAll into an ancestor scope covering all export tests, including exportCsvFiles. Update the cleanup test around the CsvExportSet usage to create its own non-empty local fixture instead of relying on the daily row inserted by another test.Source: Coding guidelines
lib/data/db.dart (1)
3587-3588: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
exportDaysDbandimportFromDbFileconsistent for lab tables.
exportDaysDbcreateslab_resultandlab_marker_defvia schema setup but only sharesexportCopycontents. A selected-day file is supported for sharing/backup, andimportFromDbFilenow accepts these tables, so importing such a file can lose lab results and custom definitions. Copy the missing rows in the export path, or document that selected-day exports intentionally exclude them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/data/db.dart` around lines 3587 - 3588, Update exportDaysDb to copy lab_result and lab_marker_def rows alongside the existing exportCopy contents, matching the tables accepted by importFromDbFile so selected-day exports preserve lab results and custom definitions.Source: Coding guidelines
lib/ui/labs/lab_entry_sheet.dart (1)
106-109: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-finite lab values.
double.tryParsecan returnNaNorInfinityfor inputs such asNaN,Infinity, and-Infinity. Those values pass the current check and are stored as laboratory results. If the parsed value is not finite, show the user error instead of saving.Proposed fix
- if (value == null) { + if (value == null || !value.isFinite) { setState(() => _error = 'Enter the number from your report'); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/labs/lab_entry_sheet.dart` around lines 106 - 109, Update the value validation in the lab entry submission flow to reject parsed values that are not finite, including NaN and positive or negative Infinity. Extend the existing null check around double.tryParse in the relevant method so non-finite values set _error and return without saving, while finite values continue through the existing path.
♻️ Duplicate comments (1)
lib/ui/labs/lab_entry_sheet.dart (1)
111-121: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the existing result unit before the marker unit.
Line 121 currently prefers
marker?.unit, so editing a result after its marker unit changes overwrites the recorded unit._valueFieldsalso displays the marker unit instead of the recorded unit. Resolve one unit value that preferswidget.existing?['unit'], then use it for both persistence and the edit field.As per coding guidelines, use a single authoritative value/source for each displayed concern.
Proposed fix
+ String _resultUnit(LabMarker? marker) => + (widget.existing?['unit'] as String?) ?? marker?.unit ?? ''; + Future<void> _save() async { ... - unit: marker?.unit ?? (widget.existing?['unit'] as String?) ?? '', + unit: _resultUnit(marker), ... } List<Widget> _valueFields(LabMarker marker) => [ ... - Text(marker.unit, style: AppText.body.copyWith( + Text(_resultUnit(marker), style: AppText.body.copyWith(Also applies to: 315-337
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/labs/lab_entry_sheet.dart` around lines 111 - 121, The lab result unit resolution in the lab entry sheet must prefer the existing result’s recorded unit over the current marker unit. Define one authoritative unit value using widget.existing?['unit'] first, then marker?.unit and the empty fallback, and reuse it for LocalDb.putLabResult and the edit-field logic in _valueFields.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/data/csv_export.dart`:
- Around line 203-207: The shared temporary export directory is deleted by each
export, invalidating files still being consumed by Share.shareXFiles. Update
exportCsvFiles and its caller lifecycle so an export remains available until
sharing releases it, or serialize new exports through completion of the active
share rather than only the export method; cover all relevant export/session call
paths and add a regression test for overlapping export and share lifetimes.
In `@lib/ui/labs/lab_entry_sheet.dart`:
- Around line 91-97: Guard UI updates after asynchronous operations: in
lib/ui/labs/lab_entry_sheet.dart lines 91-97, check mounted before setState
updates _takenOn; in lib/ui/profile/profile_screen.dart lines 271-304, check
rowCtx.mounted before displaying the failure snackbar.
---
Outside diff comments:
In `@lib/data/db.dart`:
- Around line 3587-3588: Update exportDaysDb to copy lab_result and
lab_marker_def rows alongside the existing exportCopy contents, matching the
tables accepted by importFromDbFile so selected-day exports preserve lab results
and custom definitions.
In `@lib/ui/labs/lab_entry_sheet.dart`:
- Around line 106-109: Update the value validation in the lab entry submission
flow to reject parsed values that are not finite, including NaN and positive or
negative Infinity. Extend the existing null check around double.tryParse in the
relevant method so non-finite values set _error and return without saving, while
finite values continue through the existing path.
In `@test/csv_export_test.dart`:
- Around line 101-111: Move the sqflite initialization, databaseFactory
assignment, LocalDb.dbName configuration, database deletion, and LocalDb.close
cleanup from the sibling-group setUpAll/tearDownAll into an ancestor scope
covering all export tests, including exportCsvFiles. Update the cleanup test
around the CsvExportSet usage to create its own non-empty local fixture instead
of relying on the daily row inserted by another test.
---
Duplicate comments:
In `@lib/ui/labs/lab_entry_sheet.dart`:
- Around line 111-121: The lab result unit resolution in the lab entry sheet
must prefer the existing result’s recorded unit over the current marker unit.
Define one authoritative unit value using widget.existing?['unit'] first, then
marker?.unit and the empty fallback, and reuse it for LocalDb.putLabResult and
the edit-field logic in _valueFields.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 296e0360-9443-4ec7-90c0-d4a645a4da2c
📒 Files selected for processing (7)
lib/data/csv_export.dartlib/data/db.dartlib/ui/labs/lab_entry_sheet.dartlib/ui/labs/labs_screen.dartlib/ui/profile/profile_screen.darttest/csv_export_test.darttest/db_migration_ladder_test.dart
Clearing every previous run at the start of a new one was my own fix to the files piling up, and it introduced a worse problem: the export returns before the share sheet has finished reading, so a second export could delete the files the first share was still handing over. Each run gets its own directory now and the previous one survives, so a second export cannot destroy the first's files while still bounding how many copies of plaintext health data sit on disk. The export row also refuses to re-enter while a share is open. Plus two missing mounted guards after awaits.
|
Both real, and the first one was mine — the previous round's fix for files piling up introduced it. Clearing every earlier run at the start of a new export is fine right up until you notice that the export returns before the share sheet has finished reading, so a second export deletes the files the first share is still handing over. Each run has its own directory now and the previous one survives, which bounds the copies on disk without letting a new export destroy an open share. The row also refuses to re-enter while a share is in flight, since that await covers exactly the dangerous window. Worth noting the test I wrote for the earlier fix was asserting the wrong thing — that the previous run was gone — so it would have locked the bug in. It now asserts the opposite, alongside a second test that the retention is still bounded. The two mounted guards are in. |
|
Persistent review updated to latest commit 6154a86 |
Journal's numeric tables took schema 28 on main, so blood work is 29. Both rungs are separate and additive; an install on 27 gets the journal tables and then the lab tables, in that order.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/data/db.dart (1)
3517-3545: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude lab data in selected-day database exports.
exportDaysDbdoes not copylab_resultrows for each selectedtaken_ondate. It also does not copylab_marker_defrows. A Data history export can therefore omit laboratory results and lose custom marker labels and units.Copy matching
lab_resultrows inside the day loop. Copylab_marker_defonce after the loop. Add a regression test for a selected day with a custom laboratory marker.Proposed change
for (final dayId in sorted) { // ... + await copyRows( + 'lab_result', + where: 'taken_on = ?', + whereArgs: [dayId], + ); } await copyRows('journal_field_def'); + await copyRows('lab_marker_def');As per coding guidelines, “When adding or changing a capability, cover every call path, including all raw decode paths and all relevant export/session triggers.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/data/db.dart` around lines 3517 - 3545, Update exportDaysDb’s per-day export loop to copy lab_result rows matching each selected dayId via its taken_on date, then copy lab_marker_def once after the loop alongside journal_field_def. Add a regression test covering a selected day containing a custom laboratory marker, verifying both the result and marker definition are present in the exported database.Source: Coding guidelines
lib/ui/labs/lab_entry_sheet.dart (1)
111-123: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMove the existing result when its draw date changes.
putLabResultupserts using the new(marker, taken_on)key. When the user changes the draw date, the old row remains. The history then shows two results for one edited draw.Add a transactional update operation that deletes the original key and inserts the replacement row. Add a regression test for editing only
taken_on.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/labs/lab_entry_sheet.dart` around lines 111 - 123, Update the save flow around labMarker and LocalDb.putLabResult to detect when an existing result’s taken_on changes, then transactionally delete the original (marker, taken_on) row and insert the replacement under the new key. Preserve normal upsert behavior when the date is unchanged, and add a regression test covering an edit that changes only taken_on.
♻️ Duplicate comments (1)
lib/ui/labs/lab_entry_sheet.dart (1)
111-121: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the stored unit authoritative for existing results.
The fallback at Line 121 only uses the stored unit when the marker definition is absent. If the definition still exists but its canonical unit changed, saving a note, value, or date writes the new unit without converting
value. The value field at Line 335 also displays the current marker unit instead of the stored row unit.For an edit, use
widget.existing['unit']for both display and persistence. Use the catalogue unit only for a new result.Also applies to: 315-379
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/labs/lab_entry_sheet.dart` around lines 111 - 121, Update the existing-result paths in the lab entry sheet, including the save logic around labMarker and the value display near the value field, to treat widget.existing['unit'] as authoritative whenever editing. Use the catalogue marker unit only when creating a new result, and keep the entered value unchanged rather than replacing its unit without conversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/data/db.dart`:
- Around line 3517-3545: Update exportDaysDb’s per-day export loop to copy
lab_result rows matching each selected dayId via its taken_on date, then copy
lab_marker_def once after the loop alongside journal_field_def. Add a regression
test covering a selected day containing a custom laboratory marker, verifying
both the result and marker definition are present in the exported database.
In `@lib/ui/labs/lab_entry_sheet.dart`:
- Around line 111-123: Update the save flow around labMarker and
LocalDb.putLabResult to detect when an existing result’s taken_on changes, then
transactionally delete the original (marker, taken_on) row and insert the
replacement under the new key. Preserve normal upsert behavior when the date is
unchanged, and add a regression test covering an edit that changes only
taken_on.
---
Duplicate comments:
In `@lib/ui/labs/lab_entry_sheet.dart`:
- Around line 111-121: Update the existing-result paths in the lab entry sheet,
including the save logic around labMarker and the value display near the value
field, to treat widget.existing['unit'] as authoritative whenever editing. Use
the catalogue marker unit only when creating a new result, and keep the entered
value unchanged rather than replacing its unit without conversion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a838a74d-f56d-4c57-b3b9-876282bf8ced
📒 Files selected for processing (6)
lib/data/csv_export.dartlib/data/db.dartlib/ui/labs/lab_entry_sheet.dartlib/ui/profile/profile_screen.darttest/csv_export_test.darttest/db_migration_ladder_test.dart
|
Persistent review updated to latest commit 146ad8c |
1 participant
User description
Two asks from the discussions that turned out to share a shape — both are about data the band cannot produce.
Labs. Twenty-five common markers across blood count, iron, metabolic, lipids, hormones, vitamins, inflammation and organ panels, each with a fixed unit and typical adult reference intervals; anything missing you can add yourself.
The reference intervals are the part that can do harm, so the constraints are deliberate. They exist because a bare "ferritin 42" is a spreadsheet with extra steps — but they are context, never a verdict. Every laboratory publishes its own interval, derived from its own assay and population, and they differ enough that a value inside one lab's range sits outside another's, so the range on your own report is the one that applies to you. A value outside ours reads as "outside the typical range", not "high" or "abnormal", and nothing anywhere suggests what to do about it. Several markers genuinely differ by sex and carry both intervals; with no sex in the profile they show no interval at all rather than defaulting to one, because guessing would flag a large share of perfectly normal results. A custom marker gets no range invented for it.
Two storage decisions worth calling out. Each result stores the unit it was entered under rather than looking it up, so a reading can never be silently reinterpreted if the catalogue's canonical unit changes — turning 400 ng/mL into 400 nmol/L would be the worst fabrication available here. And a result is keyed on (marker, date drawn), so re-entering one corrects a typo instead of stacking a second reading that a trend would then average.
CSV export, beside the existing
.dbexport. Opening your data in a spreadsheet and restoring it onto a new phone are different jobs, and a SQLite file only does the second. Six files — daily metrics, workouts, sleep stages, metric history, journal, labs — all read through the same derived views the coach reads, so an export can never contain something the app would not show you and can never reach raw sensor rows or GPS. A metric that was never computed is written as an empty field, never a 0: the difference is unrecoverable once it is in a spreadsheet, and a column of zeroes is exactly the kind of thing someone averages. UTF-8 BOM so Excel on Windows does not mangle notes.Schema goes to 29, purely additive. Note it assumes #218 (schema 28) lands first — if that one is going to wait, tell me and I will rebase this to 28.
PR Type
Enhancement, tests
Description
Adds hand-entered blood-work (Labs) with 25 built-in markers, sex-aware reference intervals, and custom marker support
Adds CSV export (6 files: daily metrics, workouts, sleep stages, metric history, journal, labs) via the profile screen
Bumps SQLite schema from v27 to v29 with new
lab_resultandlab_marker_deftables; nokAlgoVersionbump (no analytics output changed)Adds tests for CSV escaping/absence semantics, catalogue invariants, and lab result storage idempotence
Diagram Walkthrough
File Walkthrough
6 files
New: 25 built-in blood markers with sex-aware reference intervalsSchema v29: add lab_result and lab_marker_def tables with CRUDNew: RFC 4180 CSV export for 6 data sets via coach viewsNew Labs screen: marker cards with range context and historyNew bottom sheet: add, edit, or delete a lab resultAdd Labs and CSV export entries to profile screen3 files
Tests: CSV escaping, null-as-empty, and schema query coverageTests: catalogue invariants, range selection, and custom keysTests: lab result upsert idempotence and unit preservationSummary by CodeRabbit
New Features
Bug Fixes