Bridge remaining period gap and fix data-loss in merge (follow-up to #166)

LESSONS.md

@@ -97,11 +97,15 @@ A prediction window (e.g. a 5-day expected period) should remain visible as long
 **Don't double-count with offset when SQLite immediately reflects inserts in aggregate queries**
 In a Room migration loop that inserts rows one-by-one, calling `MAX(displayOrder)+1` in a subquery correctly reflects all previously-inserted rows in the same transaction — SQLite is not a snapshot. Adding a separate `offset` counter on top of that result double-counts and produces gaps (e.g., displayOrder 7, 9, 11 instead of 7, 8, 9). Remove the offset variable and let the `MAX+1` subquery self-increment across the loop.
 
-**Date-range entities with an "ongoing" (null end) state need an existence check before quick-log entry points create a new record**
-When an entity represents a date range and a null end date means "ongoing, extends through today" (e.g. an active period), any UI shortcut that creates a *new* record for a tapped date (calendar tap, FAB, speed dial) must first check whether that date already falls within an existing record's range. Otherwise the user ends up with two overlapping "ongoing" records for what they consider a single continuous span. Add a shared `recordForDate(records, date)` helper that treats a null end as `today`, and route quick-log entry points through it: if a match exists, navigate to edit it; otherwise create new.
-
-**"Ongoing" should mean unbounded, not "through today" — and adjacency needs its own check, separate from containment**
-A `recordForDate` helper that treats a null end date as `LocalDate.now()` only works while "today" is on or after the date being checked, and only reads clean while a session stays open past midnight without new writes. Reopen the app on a later date, or check a future date, and the same ongoing record silently stops matching — a fresh "new record" branch fires instead of "extend existing," fragmenting what the user considers one continuous span. Treat the ongoing case as having no upper bound at all (any date `>= start` matches) instead of substituting `now()`. Separately, containment (`start <= date <= end`) only catches dates *inside* a closed record's range — it does nothing for the day immediately *after* a closed record's end, which is the single most common way these entities fragment in practice (a user who closes a date-range record explicitly each day, rather than leaving it open, and logs again the next day). Add `date == end.plusDays(1)` as an explicit adjacency match alongside containment; don't assume containment implies "continues."
+**Date-range entities with an "ongoing" (null end) state need containment, adjacency, and gap-bridging checks — not just containment — before a quick-log entry point creates a new record**
+When an entity represents a date range and a null end date means "ongoing" (e.g. an active period), any UI shortcut that creates a *new* record for a tapped date (calendar tap, FAB, speed dial) must check more than plain containment (`start <= date <= end`), or the same fragmentation bug resurfaces in layers:
+1. Treat "ongoing" as unbounded (any date `>= start` matches), not as extending only through `LocalDate.now()`. A `now()` substitute stops matching the moment "today" moves past the last check — reopening the app on a later date, or checking a future date, silently fires "create new" instead of "extend existing."
+2. Containment alone still misses the day immediately *after* a closed record's end — the single most common real-world fragmentation path (a user who sets an explicit end date each day rather than leaving it open, then logs again tomorrow). Add `date == end.plusDays(1)` as an explicit adjacency match.
+3. Adjacency in isolation can just shift the fragmentation by one row: extending the earlier record to include the day-after-end is a no-op if a *second*, separate record already starts the day after that — the two are now merely touching, not merged. Detect this (a tapped date bridging a one-day gap between two distinct records) and merge them immediately rather than leaving a second manual step.
+4. Ship a one-time migration that finds already-fragmented records at these same distances (touching, or a single day apart) and merges them, guarded by a preference flag — the entry-point fix alone never touches data that's already broken on existing installs.
+5. Give the user a manual "merge with adjacent record" action in the list/history view too, as a backstop for gaps the auto-detection can't infer (larger gaps, ambiguous cases).
+
+When implementing the merge itself: don't reach for a `deleteLogsForPeriod`-style helper that was written for actual record *deletion*. Merging is not deleting — the absorbed record's own day-specific data (flow, symptoms, any per-day mirror log) is still valid history and must be preserved, not wiped out on the assumption that it would otherwise "linger orphaned."
 
 
 **Insert/upsert flags need a separate edit-by-ID path**

app/src/main/java/com/mapgie/goflo/GoFloApplication.kt

@@ -196,14 +196,14 @@ class GoFloApplication : Application() {
     }
 
     /**
-     * One-time data fixup: merges period entries that sit exactly one day apart
-     * (e.g. a period closed with an explicit end date, then a new period logged
-     * for the very next day, before the entry-point fix existed) into a single
-     * entry.
+     * One-time data fixup: merges period entries that are touching or sit a
+     * single unlogged day apart (e.g. a period closed with an explicit end
+     * date, then a new period logged for the next day or the day after,
+     * before the entry-point fix existed) into a single entry.
      *
-     * Each absorbed period's own tracking-log entries (flow, symptoms, pinned
-     * categories) are removed too, mirroring the cleanup performed on delete —
-     * otherwise they'd linger, orphaned, under a date no longer tied to any period.
+     * The absorbed periods' own tracking-log entries (flow, symptoms, pinned
+     * categories) are deliberately left in place — they're still valid, dated
+     * records of what was logged, not orphaned data to discard.
      *
      * Only runs once — guarded by the [periodAdjacencyMergeDone] preference flag.
      */
@@ -212,12 +212,6 @@ class GoFloApplication : Application() {
         if (prefs.periodAdjacencyMergeDone) return
 
         val absorbed = repository.mergeAdjacentPeriods()
-        for (period in absorbed) {
-            trackingRepository.deleteLogsForPeriod(
-                LocalDate.parse(period.startDate),
-                period.endDate?.let { LocalDate.parse(it) }
-            )
-        }
         if (absorbed.isNotEmpty()) {
             GoFloWidget.updateAllWidgets(this)
         }

app/src/main/java/com/mapgie/goflo/data/repository/PeriodRepository.kt

@@ -70,6 +70,28 @@ class PeriodRepository(
         periodDao.updatePeriod(period.copy(flowLevel = flowLevel))
     }
 
+    /**
+     * If [date] sits in the single-day gap between one period's end and the
+     * next period's start, merges the two into one continuous period.
+     *
+     * `periodForDate` already treats the day right after an explicit end date
+     * as covered by that period (see its doc), so tapping that day alone
+     * opens the earlier period for a deferred end-date extension. But when a
+     * *second*, separate period already starts the following day, extending
+     * the first one in isolation just leaves it newly touching the second —
+     * still two rows, now requiring a manual History merge to finish the
+     * job. Detecting and merging both immediately closes the gap in one step.
+     *
+     * @return the merged period if [date] bridged a gap, else null.
+     */
+    suspend fun mergeGapAt(date: LocalDate): PeriodEntry? {
+        val periods = periodDao.getAllPeriodsOnce()
+        val before = periods.firstOrNull { it.endDate == date.minusDays(1).toString() }
+        val after = periods.firstOrNull { it.startDate == date.plusDays(1).toString() }
+        if (before == null || after == null) return null
+        return mergePeriods(before, after)
+    }
+
     /**
      * One-time data fixup: merges period entries whose date ranges overlap into a
      * single entry. An ongoing period (endDate == null) is treated as extending
@@ -157,15 +179,20 @@ class PeriodRepository(
     }
 
     /**
-     * One-time data fixup: merges period entries that are adjacent — the day right
-     * after one period's explicit end date is exactly the start of the next — into
-     * a single entry.
+     * One-time data fixup: merges period entries that are at most one day
+     * apart — either touching (the day right after one period's explicit end
+     * date is exactly the start of the next) or separated by a single
+     * unlogged day (the next period starts two days after the previous one's
+     * end) — into a single entry.
      *
-     * This repairs periods fragmented by the entry-point bug where logging a day
-     * right after a period was closed with an explicit end date (rather than left
-     * ongoing) created a new, disconnected period instead of continuing the
-     * existing one. Only a one-day gap is merged; larger gaps are left alone since
-     * they represent a genuinely new cycle, not a fragmented one.
+     * This repairs periods fragmented by the entry-point bug where logging a
+     * day right after a period was closed with an explicit end date (rather
+     * than left ongoing) created a new, disconnected period instead of
+     * continuing the existing one — including the case where a third day was
+     * then logged before the gap day ever was, leaving that day orphaned
+     * between two separate entries. Only gaps of zero or one day are merged;
+     * larger gaps are left alone since they represent a genuinely new cycle,
+     * not a fragmented one.
      *
      * Periods are processed in start-date order, reusing [mergePeriods] for each
      * adjacent pair found.
@@ -183,7 +210,8 @@ class PeriodRepository(
         for (next in periods.drop(1)) {
             val currentEnd = current.endDate?.let { LocalDate.parse(it) }
             val nextStart = LocalDate.parse(next.startDate)
-            if (currentEnd != null && nextStart == currentEnd.plusDays(1)) {
+            val gapDays = currentEnd?.let { ChronoUnit.DAYS.between(it, nextStart) }
+            if (gapDays != null && gapDays in 1..2) {
                 absorbed.add(next)
                 current = mergePeriods(current, next)
             } else {

app/src/main/java/com/mapgie/goflo/ui/screens/history/HistoryViewModel.kt

@@ -105,19 +105,17 @@ class HistoryViewModel(
 
     /**
      * Merges [first] and [second] into a single continuous period, combining their
-     * notes and symptoms. The tracking-log entries (flow, symptoms, pinned
-     * categories) belonging to whichever period is absorbed are also removed,
-     * mirroring the cleanup performed on delete — otherwise its flow/symptom
-     * entry would linger, orphaned, under a date no longer tied to any period.
+     * notes and symptoms. The absorbed period's own tracking-log entries (flow,
+     * symptoms, pinned categories) are deliberately left alone — they're still
+     * valid, dated records of what was logged that day, not orphaned data. They
+     * were previously deleted here on the assumption that they'd otherwise linger
+     * unattached, but `deleteLogsForPeriod` was written for actual period
+     * *deletion* and silently discarded real history that the user asked to
+     * combine, not remove.
      */
     fun mergePeriods(first: PeriodEntry, second: PeriodEntry) {
         viewModelScope.launch {
-            val absorbed = if (first.startDate <= second.startDate) second else first
             repository.mergePeriods(first, second)
-            trackingRepository?.deleteLogsForPeriod(
-                LocalDate.parse(absorbed.startDate),
-                absorbed.endDate?.let { LocalDate.parse(it) }
-            )
             application?.let { GoFloWidget.updateAllWidgets(it) }
         }
     }

app/src/main/java/com/mapgie/goflo/ui/screens/home/HomeScreen.kt

@@ -113,11 +113,16 @@ fun HomeScreen(
 
     // If [date] already falls within an existing period's range (including an
     // ongoing period, which extends through today), edit that period instead of
-    // creating a new, overlapping entry.
+    // creating a new, overlapping entry. If [date] is also the single unlogged
+    // day between that period's end and a separate, later period's start,
+    // merge the two immediately so the gap doesn't need a second, manual
+    // History merge to close.
     fun navigateToLogPeriod(date: LocalDate) {
         val existing = PeriodRepository.periodForDate(state.periods, date)
         if (existing != null) {
-            onNavigate(Screen.LogPeriod.withId(existing.id, date))
+            viewModel.mergeGapAt(date, fallbackId = existing.id) { targetId ->
+                onNavigate(Screen.LogPeriod.withId(targetId, date))
+            }
         } else {
             onNavigate(Screen.LogPeriod.newEntryForDate(date))
         }

app/src/main/java/com/mapgie/goflo/ui/screens/home/HomeViewModel.kt

@@ -187,6 +187,17 @@ class HomeViewModel(
     fun selectDay(date: LocalDate) { _selectedDay.value = date }
     fun clearSelectedDay() { _selectedDay.value = null }
 
+    /**
+     * If [date] bridges a one-day gap between two existing periods, merges
+     * them (see [PeriodRepository.mergeGapAt]) and hands the merged id to
+     * [onResolved]; otherwise hands back [fallbackId] unchanged.
+     */
+    fun mergeGapAt(date: LocalDate, fallbackId: Long, onResolved: (Long) -> Unit) {
+        viewModelScope.launch {
+            onResolved(repository.mergeGapAt(date)?.id ?: fallbackId)
+        }
+    }
+
     // ── Quick increment (Plus One categories) ───────────────────────────────────
 
     /** Transient confirmation message after an instant increment; null when none pending. */

app/src/test/java/com/mapgie/goflo/data/repository/PeriodRepositoryTest.kt

@@ -1,11 +1,72 @@
 package com.mapgie.goflo.data.repository
 
+import com.mapgie.goflo.data.database.dao.PeriodDao
+import com.mapgie.goflo.data.database.dao.SymptomDao
 import com.mapgie.goflo.data.database.entities.PeriodEntry
+import com.mapgie.goflo.data.database.entities.SymptomEntry
+import kotlinx.coroutines.flow.Flow
+import kotlinx.coroutines.flow.flowOf
+import kotlinx.coroutines.runBlocking
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertNull
+import org.junit.Assert.assertTrue
 import org.junit.Test
 import java.time.LocalDate
 
+/** In-memory [PeriodDao] fake for exercising [PeriodRepository]'s suspend write paths. */
+private class FakePeriodDao : PeriodDao {
+    val periods = mutableListOf<PeriodEntry>()
+    private var nextId = 1L
+
+    override fun getAllPeriods(): Flow<List<PeriodEntry>> = flowOf(periods.sortedByDescending { it.startDate })
+    override suspend fun getAllPeriodsOnce(): List<PeriodEntry> = periods.sortedBy { it.startDate }
+    override fun getPeriodById(id: Long): Flow<PeriodEntry?> = flowOf(periods.firstOrNull { it.id == id })
+
+    override suspend fun insertPeriod(period: PeriodEntry): Long {
+        val id = if (period.id != 0L) period.id else nextId++
+        periods.removeAll { it.id == id }
+        periods.add(period.copy(id = id))
+        return id
+    }
+
+    override suspend fun updatePeriod(period: PeriodEntry) {
+        val index = periods.indexOfFirst { it.id == period.id }
+        if (index >= 0) periods[index] = period
+    }
+
+    override suspend fun deletePeriod(period: PeriodEntry) {
+        periods.removeAll { it.id == period.id }
+    }
+
+    override suspend fun deleteAllPeriods() { periods.clear() }
+    override suspend fun countPeriods(): Int = periods.size
+}
+
+/** In-memory [SymptomDao] fake for exercising [PeriodRepository]'s suspend write paths. */
+private class FakeSymptomDao : SymptomDao {
+    val symptoms = mutableListOf<SymptomEntry>()
+    private var nextId = 1L
+
+    override fun getSymptomsForPeriod(periodId: Long): Flow<List<SymptomEntry>> =
+        flowOf(symptoms.filter { it.periodId == periodId })
+    override suspend fun getSymptomsForPeriodOnce(periodId: Long): List<SymptomEntry> =
+        symptoms.filter { it.periodId == periodId }
+    override suspend fun insertSymptom(symptom: SymptomEntry) {
+        symptoms.add(symptom.copy(id = nextId++))
+    }
+    override suspend fun deleteSymptomsByPeriodId(periodId: Long) {
+        symptoms.removeAll { it.periodId == periodId }
+    }
+    override suspend fun deleteAllSymptoms() { symptoms.clear() }
+    override suspend fun getAllSymptoms(): List<SymptomEntry> = symptoms.toList()
+    override fun getAllSymptomsFlow(): Flow<List<SymptomEntry>> = flowOf(symptoms.toList())
+    override suspend fun bulkRenameSymptoms(oldLabel: String, newLabel: String) {
+        val renamed = symptoms.map { if (it.symptomType == oldLabel) it.copy(symptomType = newLabel) else it }
+        symptoms.clear()
+        symptoms.addAll(renamed)
+    }
+}
+
 class PeriodRepositoryTest {
 
     private fun entry(startDate: String, endDate: String? = null, id: Long = 0L) =
@@ -177,4 +238,76 @@ class PeriodRepositoryTest {
         val periods = listOf(entry("2024-01-01", "2024-01-05", id = 1))
         assertNull(PeriodRepository.periodForDate(periods, LocalDate.of(2024, 1, 10)))
     }
+
+    // ── mergeGapAt / mergeAdjacentPeriods ─────────────────────────────────────
+
+    private fun buildRepository(vararg seed: PeriodEntry): Pair<PeriodRepository, FakePeriodDao> {
+        val periodDao = FakePeriodDao()
+        seed.forEach { periodDao.periods.add(it) }
+        return PeriodRepository(periodDao, FakeSymptomDao()) to periodDao
+    }
+
+    @Test
+    fun `mergeGapAt merges the two periods flanking a one-day gap`() = runBlocking {
+        // Reproduces the reported bug: a period ending 28 June and another starting
+        // 30 June leave the 29th orphaned. Logging the 29th should join them.
+        val (repo, dao) = buildRepository(
+            entry("2024-06-26", "2024-06-28", id = 1),
+            entry("2024-06-30", null, id = 2),
+        )
+        val merged = repo.mergeGapAt(LocalDate.of(2024, 6, 29))
+        assertEquals(1L, merged?.id)
+        assertEquals(1, dao.periods.size)
+        val survivor = dao.periods.single()
+        assertEquals("2024-06-26", survivor.startDate)
+        assertNull(survivor.endDate) // the later (ongoing) period's null end date wins
+    }
+
+    @Test
+    fun `mergeGapAt returns null when the date only touches one period`() = runBlocking {
+        val (repo, dao) = buildRepository(entry("2024-01-01", "2024-01-05", id = 1))
+        assertNull(repo.mergeGapAt(LocalDate.of(2024, 1, 6)))
+        assertEquals(1, dao.periods.size) // untouched — no second period to bridge to
+    }
+
+    @Test
+    fun `mergeGapAt returns null when no period precedes the date`() = runBlocking {
+        val (repo, _) = buildRepository(entry("2024-06-30", null, id = 2))
+        assertNull(repo.mergeGapAt(LocalDate.of(2024, 6, 29)))
+    }
+
+    @Test
+    fun `mergeAdjacentPeriods merges periods that are touching`() = runBlocking {
+        val (repo, dao) = buildRepository(
+            entry("2024-06-01", "2024-06-05", id = 1),
+            entry("2024-06-06", "2024-06-08", id = 2),
+        )
+        val absorbed = repo.mergeAdjacentPeriods()
+        assertEquals(listOf(2L), absorbed.map { it.id })
+        assertEquals(1, dao.periods.size)
+        assertEquals("2024-06-08", dao.periods.single().endDate)
+    }
+
+    @Test
+    fun `mergeAdjacentPeriods merges periods separated by a single unlogged day`() = runBlocking {
+        val (repo, dao) = buildRepository(
+            entry("2024-06-26", "2024-06-28", id = 1),
+            entry("2024-06-30", "2024-07-02", id = 2),
+        )
+        val absorbed = repo.mergeAdjacentPeriods()
+        assertEquals(listOf(2L), absorbed.map { it.id })
+        assertEquals(1, dao.periods.size)
+        assertEquals("2024-07-02", dao.periods.single().endDate)
+    }
+
+    @Test
+    fun `mergeAdjacentPeriods leaves periods with a gap of more than one day alone`() = runBlocking {
+        val (repo, dao) = buildRepository(
+            entry("2024-06-01", "2024-06-05", id = 1),
+            entry("2024-06-08", "2024-06-12", id = 2), // 2-day gap — a genuinely new cycle
+        )
+        val absorbed = repo.mergeAdjacentPeriods()
+        assertTrue(absorbed.isEmpty())
+        assertEquals(2, dao.periods.size)
+    }
 }