journal entries that carry a number, not just a tag

[abdulsaheel]

User description

The most-requested journal changes — mood, water, caffeine with a dose and a time, a sleep-quality rating, custom entries — all need the same thing the journal did not have: a number. A tag can only say whether something happened, and one coffee versus five is usually the actual question.

Nine built-in fields ship: mood, sleep quality, energy, stress, soreness, water, caffeine, alcohol, screens before bed. Beyond those you can define your own — name it, say whether it is a 1–5 rating, an amount, or minutes, and it behaves exactly like a built-in everywhere downstream. Caffeine and alcohol also ask when the last one was, because a 200 mg morning coffee and a 200 mg evening one are the same dose and a completely different night, and that is the half that predicts anything.

Absent and zero are different answers, and every control here is shaped around it. "No caffeine today" is a measurement; "I didn't fill this in" is not. Reading the second as the first invents a data point at the very bottom of the dose range, which is exactly where a correlation is most sensitive. So: a field starts unset, one tap on minus reaches a real zero rather than making you step down to it, any value can be cleared back to unset, and clearing one deletes the row instead of leaving a stale reading nobody made. Saving a day replaces that day outright rather than merging into it, for the same reason.

Ratings are 1–5, not 1–10 — a ten-point self-report is not ten distinguishable states, and the extra resolution is noise a rank correlation then has to see through.

Deleting a custom field forgets its label, not its history. Those readings were still real, and they render unlabelled until the field is defined again.

Schema goes to 28. Two new tables, purely additive — an upgrade touches no existing journal row, and a day that only ever had tags keeps them and simply has no numeric rows. There is a migration test from v27 pinning that.

Correlating these fields is deliberately not in this PR. That needs a rank statistic that does not exist in the analytics package yet (OpenStrap/analytics#41), and holding storage and UI hostage to that merge would just make both harder to review. Values are recorded and shown here; the insight wiring follows once the pin moves.

One drive-by: the journal screen kept its own private copy of the preset tag list, which is how it would have drifted from the compose screen the first time either changed.

---

PR Type

Enhancement, tests

---

Description

• Adds numeric journal fields (mood, sleep quality, energy, stress, soreness, water, caffeine, alcohol, screens) with absent-vs-zero semantics

• Schema bumped to v28: two new tables (journal_metric, journal_field_def), purely additive; existing tag/note rows untouched

• Users can define custom fields (name, kind, unit, ceiling, step, optional time); deleting a definition preserves its history

• Journal screen gains a NUMBERS section with rating dots and steppers; tag list de-duplicated from kJournalPresetTags

---

Diagram Walkthrough

flowchart LR
  A["journal_fields.dart\n(JournalFieldSpec, kJournalFields,\nJournalMetricValue)"]
  B["db.dart\nschema v28\njournal_metric +\njournal_field_def tables"]
  C["local_repository_impl.dart\ngetJournalMetrics /\npostJournalMetrics /\ngetJournalFields"]
  D["journal_screen.dart\nNUMBERS section +\ncustom field management"]
  E["journal_metric_editor.dart\n_RatingDots / _Stepper /\n_FieldRow widgets"]
  F["custom_journal_field_sheet.dart\ndefine user-invented field"]
  G["test/journal_fields_test.dart\ntest/journal_metric_store_test.dart\ntest/db_migration_ladder_test.dart"]

  A -- "types" --> B
  A -- "types" --> C
  B -- "CRUD" --> C
  C -- "API" --> D
  D -- "embeds" --> E
  D -- "opens" --> F
  B -- "migration pinned by" --> G
  C -- "round-trip pinned by" --> G
  A -- "vocab pinned by" --> G

Loading

File Walkthrough

	Relevant files
Enhancement	7 files journal_fields.dart New file: numeric field vocabulary, specs, and value types +236/-0  db.dart Schema v28: journal_metric and journal_field_def tables + CRUD +211/-1  local_repository.dart Abstract interface for numeric journal field operations    +22/-0    local_repository_impl.dart Implement numeric journal field read/write with value clamping +47/-0    journal_metric_editor.dart New widget: rating dots and stepper rows for numeric fields +385/-0  custom_journal_field_sheet.dart New sheet: define a user-invented numeric journal field    +255/-0  journal_screen.dart Add NUMBERS section; deduplicate tag list; load/save metrics +84/-7
Tests	3 files journal_fields_test.dart New tests: field vocabulary, key uniqueness, formatting, value equality +163/-0  journal_metric_store_test.dart New tests: round-trip, zero storage, destructive replace, custom defs +195/-0  db_migration_ladder_test.dart Pin v27→v28 migration: tags/notes preserved, new tables usable immediately +51/-0

---

[coderabbitai]

Warning

Review limit reached

@abdulsaheel, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 48 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0237462c-8306-41e5-92d0-125c4417b194

📥 Commits

Reviewing files that changed from the base of the PR and between 3b4a44b and 1e3c3af.

📒 Files selected for processing (10)

• lib/data/db.dart
• lib/data/journal_fields.dart
• lib/data/local_repository.dart
• lib/data/local_repository_impl.dart
• lib/ui/journal/custom_journal_field_sheet.dart
• lib/ui/journal/journal_metric_editor.dart
• lib/ui/journal/journal_screen.dart
• test/db_migration_ladder_test.dart
• test/journal_fields_test.dart
• test/journal_metric_store_test.dart

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 1e3c3af)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Missing mounted guard In _addCustomField, showCustomJournalFieldSheet is awaited and then api.postCustomJournalField(spec) is awaited before the mounted check. If the widget is disposed between the two awaits, the setState call is guarded, but postCustomJournalField still runs against a potentially stale _api reference. More critically, the context captured in the showCustomJournalFieldSheet call is the widget's own context — if the widget is disposed while the sheet is open, Navigator.pop inside _save (in the sheet) uses a stale context. This matches the recurring §4.5 pattern. The mounted check should come immediately after the first await. Future<void> _addCustomField() async { final api = _api; if (api == null) return; final spec = await showCustomJournalFieldSheet( context, existingKeys: _fieldSpecs.map((f) => f.key).toSet(), ); if (spec == null) return; await api.postCustomJournalField(spec); if (!mounted) return; setState(() => _fieldSpecs = [..._fieldSpecs, spec]); } Absent-input fabrication on unknown field In postJournalMetrics, when spec == null (the field key has no definition — e.g. a custom field whose definition was deleted), the value is passed through unclamped with e.value.value. This is fine for the value itself, but the comment says "A zero is a real answer" — the real concern is that a value from an unknown field is stored without any bounds check. More importantly, the clamping path calls getJournalFields() on every save, which issues two DB queries (one for custom defs, one for built-ins). If a field is unknown, the code silently stores whatever value arrived. This is a minor honesty concern: a field with no spec and an out-of-range value will be stored and later read back as a real data point with no ceiling enforcement. The fix is to either reject unknown-key fields or apply a conservative default clamp. final v = spec == null ? e.value.value : e.value.value.clamp(0.0, spec.max).toDouble(); Export missing journal_field_def schema exportDaysDb calls copyRows('journal_field_def') (no WHERE clause, copies all definitions), but the export database is opened via _openExportDb which calls _createUserTables to set up the schema. _createUserTables now calls _createJournalMetric and _createJournalFieldDef, so the tables exist. However, copyRows uses INSERT-OR-REPLACE by default — if the export DB already has a journal_field_def row with the same key (e.g. from a previous export of overlapping days), it will be silently replaced. This is likely intentional and correct, but worth confirming that copyRows uses REPLACE semantics rather than IGNORE, since a stale definition in the export would otherwise survive. // Custom journal field definitions are not day-scoped, so they ride along // whole. Without them an exported day carries numbers under keys like // `custom_magnesium` with no label, no unit and no idea what scale they // are on — the values survive the export and their meaning does not. await copyRows('journal_field_def');

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 1e3c3af
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Reset saving latch on failure path _saving is set to true on entry but there is no finally block to reset it on the failure path. If either postJournal or postJournalMetrics throws, _saving stays true and the Save button is permanently disabled until the user force-quits — the same sticky-boolean-latch pattern identified as recurring in AGENTS.md §4.3. Wrap the body in a try/finally that calls setState(() => _saving = false). lib/ui/journal/journal_screen.dart [183-196] Future<void> _save() async { if (api == null || _saving) return; setState(() => _saving = true); try { - await api.postJournal(...); + await api.postJournal( + _editingDate, + _selectedTags.toList(), + _noteCtrl.text.trim(), + ); await api.postJournalMetrics(_editingDate, _metrics); if (!mounted) return; - ... + ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text('Saved ${_isToday ? 'today' : _editingDate}')), + ); + } catch (e) { + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar(content: Text('Save failed — please try again')), + ); + } finally { + if (mounted) setState(() => _saving = false); + } +} Suggestion importance[1-10]: 8 __ Why: The _saving flag is set to true but never reset if postJournal or postJournalMetrics throws, permanently disabling the Save button. This is a real bug that would require a force-quit to recover from, making it a high-impact fix.	Medium
General	Guard context use after await in field creation context is used to open the modal sheet before the await api.postCustomJournalField(spec) call, which is fine, but the showCustomJournalFieldSheet call itself captures context across an await boundary inside showModalBottomSheet. More critically, if postCustomJournalField throws, the field is never appended to _fieldSpecs but the DB write also failed, leaving the UI and DB in sync — however there is no error feedback to the user and no retry path. The missing mounted check before showCustomJournalFieldSheet is the real issue: if the widget unmounts while the sheet is open and the user confirms, Navigator.pop inside _save of the sheet will operate on a detached navigator. Wrap the post-await section in a try/catch with user feedback via ScaffoldMessenger. lib/ui/journal/journal_screen.dart [155-166] Future<void> _addCustomField() async { final api = _api; if (api == null) return; final spec = await showCustomJournalFieldSheet( context, existingKeys: _fieldSpecs.map((f) => f.key).toSet(), ); - if (spec == null) return; - await api.postCustomJournalField(spec); - if (!mounted) return; - setState(() => _fieldSpecs = [..._fieldSpecs, spec]); + if (spec == null || !mounted) return; + try { + await api.postCustomJournalField(spec); + if (!mounted) return; + setState(() => _fieldSpecs = [..._fieldSpecs, spec]); + } catch (_) { + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar(content: Text('Could not save field — please try again')), + ); + } } Suggestion importance[1-10]: 6 __ Why: The missing mounted check after showCustomJournalFieldSheet and the lack of error handling for postCustomJournalField failures are valid concerns. Adding if (!mounted) return before the API call and wrapping in try/catch improves robustness, though the risk is relatively low in practice.	Low
	Skip unvalidatable fields instead of storing unclamped values When spec == null (a field whose custom definition was deleted while its readings remain), the value is passed through unclamped. This means a corrupt or out-of-range value from a future build can be stored without any bound. Per AGENTS.md §4.1, absent-input handling that fabricates or silently accepts a bad value is a bug; the safe choice is to skip storing the field entirely when no spec is available to validate it, rather than persisting an unvalidated number. lib/data/local_repository_impl.dart [2722-2738] final specs = await getJournalFields(); final clamped = <String, JournalMetricValue>{}; for (final e in fields.entries) { final spec = journalFieldSpec( e.key, custom: specs.where((s) => s.custom).toList(), ); - final v = spec == null - ? e.value.value - : e.value.value.clamp(0.0, spec.max).toDouble(); - // A zero is a real answer ("no caffeine today") and is stored as one. - // Absence is expressed by leaving the field out of the map entirely. + // Skip fields with no definition — no spec means no known ceiling, so + // we cannot validate the value and must not store it. + if (spec == null) continue; + final v = e.value.value.clamp(0.0, spec.max).toDouble(); clamped[e.key] = JournalMetricValue( v, atMinuteOfDay: e.value.atMinuteOfDay, ); } Suggestion importance[1-10]: 5 __ Why: When spec == null, the value is stored unclamped, which could allow out-of-range values into the database. However, this scenario (a field with no definition) is an edge case that the codebase already documents as intentional — orphaned readings are preserved — so skipping them entirely would change the intended behavior of preserving history for deleted custom fields.	Low

---

Previous suggestions

Suggestions up to commit 8a9805d

Category	Suggestion	Impact
Possible issue	Reset boolean latch on save failure path _saving is set to true on entry but there is no finally block to reset it on the failure path. If either postJournal or postJournalMetrics throws, _saving stays true and the Save button is permanently disabled until the app is force-closed — the exact sticky-boolean-latch pattern flagged in §4.3 of AGENTS.md. Add a finally { if (mounted) setState(() => _saving = false); } block. lib/ui/journal/journal_screen.dart [183-196] Future<void> _save() async { if (api == null || _saving) return; setState(() => _saving = true); try { - await api.postJournal(...); + await api.postJournal( + _editingDate, + _selectedTags.toList(), + _noteCtrl.text.trim(), + ); await api.postJournalMetrics(_editingDate, _metrics); if (!mounted) return; - ... + ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text('Saved ${_isToday ? 'today' : _editingDate}')), + ); + } catch (e) { + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar(content: Text('Save failed')), + ); + } finally { + if (mounted) setState(() => _saving = false); + } +} Suggestion importance[1-10]: 8 __ Why: The _saving flag is set to true but never reset on the error path, permanently disabling the Save button after any failure. This is a real usability bug that requires a finally block to fix.	Medium
	Absent-spec fields fabricate a value instead of abstaining When spec == null (a field whose custom definition was deleted while its history remained), the value is passed through unclamped. This is fine for the value itself, but the code still stores the entry in clamped and therefore persists it. A field with no definition has no max, so there is no bound to enforce — but more importantly, a field the caller included with spec == null is an orphaned key that the UI cannot have rendered a control for, meaning it arrived from stale in-memory state. Skipping orphaned keys here prevents fabricating a reading for a field the user could not have interacted with. lib/data/local_repository_impl.dart [2729-2738] final v = spec == null - ? e.value.value + ? null : e.value.value.clamp(0.0, spec.max).toDouble(); +if (v == null) continue; // no definition → skip rather than store unclamped // A zero is a real answer ("no caffeine today") and is stored as one. // Absence is expressed by leaving the field out of the map entirely. clamped[e.key] = JournalMetricValue( v, atMinuteOfDay: e.value.atMinuteOfDay, ); Suggestion importance[1-10]: 4 __ Why: When spec == null, the value is stored unclamped, but the scenario (orphaned key from stale in-memory state) is an edge case. The suggestion's improved_code uses continue inside a for loop which is valid, but the practical impact is limited since the UI shouldn't normally produce orphaned keys.	Low
General	Custom field definitions omitted from day export The day-export copy loop now copies journal_metric rows but never copies journal_field_def rows. A custom field's definition lives in journal_field_def, and without it the exported database contains numeric readings whose unit, label, and ceiling are unknown — the values render as bare numbers and the entry ceiling is gone, breaking the clamping invariant on re-import. journal_field_def should be copied once (it is not day-scoped), analogous to how baselines or sync_cursor are handled in the full-reset list. lib/data/db.dart [3468-3473] await copyRows( 'journal_metric', where: 'date = ?', whereArgs: [dayId], ); await copyRows('cycle_log', where: 'date = ?', whereArgs: [dayId]); +// journal_field_def is not day-scoped; copy it once after the day loop +// (guard with INSERT OR IGNORE so repeated days don't duplicate rows). Suggestion importance[1-10]: 6 __ Why: The day-export loop copies journal_metric rows but omits journal_field_def, meaning exported databases lack the metadata needed to interpret custom field values. However, the improved_code doesn't actually show the fix implementation, only a comment placeholder, reducing confidence in the suggestion's completeness.	Low

[abdulsaheel]

One of the three was real.

Custom field definitions were missing from the day export. They are not day-scoped, so the per-day copy loop skipped them, and an exported day ended up holding values under keys like custom_magnesium with no label, no unit and no scale — the numbers survived and their meaning did not. They ride along whole now, with a test that opens the exported file and checks both halves are there.

The _saving latch already has its finally — it is right there in _save, unchanged by this PR, resetting the flag on both paths.

The unclamped write for a field with no definition is deliberate and, as far as I can tell, unreachable from the UI: removing a custom field drops it from the editor's value map in the same setState, so an orphaned key never reaches the save. If one somehow did, storing the number the user actually entered is the honest option — there is no ceiling to clamp against, and dropping it would discard a real reading to satisfy a definition that no longer exists.

[github-actions]

Persistent review updated to latest commit 1e3c3af

[abdulsaheel]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.