diff --git a/Strand/Data/JournalCatalog.swift b/Strand/Data/JournalCatalog.swift index eda4a37363..6304b2ef2d 100644 --- a/Strand/Data/JournalCatalog.swift +++ b/Strand/Data/JournalCatalog.swift @@ -62,7 +62,7 @@ final class JournalCatalogStore: ObservableObject { /// Persisted as a single JSON blob under `journal.catalog.v2`. @Published var items: [JournalCatalogItem] { didSet { persistItems() } } - private let d = UserDefaults.standard + private let d: UserDefaults private enum K { static let items = JournalCatalogBackupKeys.items // Legacy (v1) keys, read once for the one-time migration, never written again. @@ -70,7 +70,8 @@ final class JournalCatalogStore: ObservableObject { static let hidden = JournalCatalogBackupKeys.legacyHidden } - init() { + init(defaults: UserDefaults = .standard) { + d = defaults if let blob = d.data(forKey: K.items), let decoded = try? JSONDecoder().decode([JournalCatalogItem].self, from: blob) { items = decoded @@ -164,33 +165,13 @@ final class JournalCatalogStore: ObservableObject { .lowercased() } - /// imported > starter > custom; case-insensitive dedupe, first casing wins, with `hidden` - /// questions filtered out. Imported questions lead so the export's exact strings (which the - /// effects engine keys on) survive verbatim and pull the matching starter/custom out of the list. - nonisolated static func mergeCatalog(imported: [String], custom: [String], - hidden: [String] = []) -> [String] { - let hiddenSet = Set(hidden.map(norm)) - var seen = Set() - var out: [String] = [] - for q in imported + starterQuestions + custom { - // Display text trims surrounding whitespace/newlines; the dedup key normalises ALL - // whitespace (see `norm`) so an imported "…magnesium?\n" folds onto the starter (#224). - let t = q.trimmingCharacters(in: .whitespacesAndNewlines) - let key = norm(q) - if !t.isEmpty, !hiddenSet.contains(key), seen.insert(key).inserted { out.append(t) } - } - return out - } - /// The merged catalog resolved into full v2 items, grouped and ordered for display. Imported + /// starter + custom questions are folded onto one canonical key (norm dedupe, #224); each carries /// the user's saved displayName / kind / group / sortIndex (a starter with no saved item gets its /// default group and `.bool`). Hidden items are dropped unless `includeHidden`. The `custom` flag /// is preserved so the edit UI can offer "Delete" vs "Hide". /// - /// This is the display-side twin of `mergeCatalog(imported:custom:hidden:)`: same fold + dedupe, - /// but returning the typed items instead of bare strings. `canonical` is always the verbatim key - /// the engine joins on; `displayName ?? canonical` is what the UI renders (rename is display-only). + /// `canonical` is always the verbatim key the engine joins on; `displayName ?? canonical` is what the UI renders (rename is display-only). func resolvedItems(imported: [String], includeHidden: Bool = false) -> [JournalCatalogItem] { var byKey: [String: JournalCatalogItem] = [:] for it in items { byKey[Self.norm(it.canonical)] = it } diff --git a/Strand/Screens/InsightsView.swift b/Strand/Screens/InsightsView.swift index 7d455bdcce..e526e99bff 100644 --- a/Strand/Screens/InsightsView.swift +++ b/Strand/Screens/InsightsView.swift @@ -750,9 +750,8 @@ struct InsightsView: View { /// Behaviours the user actually has data for: distinct logged journal questions /// (`behaviours.keys`) ∪ imported-export questions, minus the catalog's hidden set. - /// Triage fix (a)/(b): we do NOT route this through `mergeCatalog`, which would inject - /// the whole starter catalog (and re-surface hidden behaviours) as eligible, so the - /// empty-state guard is real and only behaviours with history can be tested. + /// The full catalog resolver includes starter questions without history. Experiment eligibility + /// instead includes only behaviours with history and excludes hidden questions. private var experimentCandidates: [String] { let saved = experimentBehaviour.trimmingCharacters(in: .whitespacesAndNewlines) let hidden = Set(catalog.hiddenQuestions.map { $0.trimmingCharacters(in: .whitespaces).lowercased() }) diff --git a/StrandTests/JournalLogicTests.swift b/StrandTests/JournalLogicTests.swift index f0c1ce92d1..2eb60c8b9c 100644 --- a/StrandTests/JournalLogicTests.swift +++ b/StrandTests/JournalLogicTests.swift @@ -2,11 +2,25 @@ import XCTest import WhoopStore @testable import Strand -/// Pins the native-journal merge logic, mirroring the Android JournalLogTest value-for-value so the -/// two platforms merge catalogs and entries identically, question strings are opaque exact-match -/// keys to the effects engines on both sides. +/// Pins native journal-entry merging and the production typed catalog. Canonical question strings +/// remain exact-match keys to the effects engines on both platforms. final class JournalLogicTests: XCTestCase { + private func isolatedDefaults() -> UserDefaults { + let name = "JournalLogicTests.\(UUID().uuidString)" + let defaults = UserDefaults(suiteName: name)! + addTeardownBlock { UserDefaults(suiteName: name)?.removePersistentDomain(forName: name) } + return defaults + } + + @MainActor + private func makeCatalog(custom: [String] = [], hidden: [String] = []) -> JournalCatalogStore { + let defaults = isolatedDefaults() + defaults.set(custom, forKey: JournalCatalogBackupKeys.legacyCustom) + defaults.set(hidden, forKey: JournalCatalogBackupKeys.legacyHidden) + return JournalCatalogStore(defaults: defaults) + } + private func e(_ day: String, _ q: String, _ yes: Bool) -> JournalEntry { JournalEntry(day: day, question: q, answeredYes: yes, notes: nil) } @@ -31,7 +45,7 @@ final class JournalLogicTests: XCTestCase { @MainActor func testCatalogAdoptsImportedCasing() { - let cat = JournalCatalogStore.mergeCatalog(imported: ["DID YOU DRINK ANY ALCOHOL?"], custom: []) + let cat = makeCatalog().resolvedItems(imported: ["DID YOU DRINK ANY ALCOHOL?"]).map(\.canonical) XCTAssertEqual(cat.first, "DID YOU DRINK ANY ALCOHOL?") // The starter alcohol question deduped case-insensitively: 9 starters survive + 1 imported. XCTAssertEqual(cat.count, JournalCatalogStore.starterQuestions.count) @@ -39,8 +53,8 @@ final class JournalLogicTests: XCTestCase { @MainActor func testCustomsAppendAndBlanksDrop() { - let cat = JournalCatalogStore.mergeCatalog(imported: [], - custom: [" ", "Did you nap?", "did you NAP?"]) + let store = makeCatalog(custom: [" ", "Did you nap?", "did you NAP?"]) + let cat = store.resolvedItems(imported: []).map(\.canonical) XCTAssertEqual(Array(cat.prefix(JournalCatalogStore.starterQuestions.count)), JournalCatalogStore.starterQuestions) XCTAssertEqual(cat.last, "Did you nap?") @@ -51,9 +65,9 @@ final class JournalLogicTests: XCTestCase { func testImportedMagnesiumWithTrailingWhitespaceDoesNotDoublePrompt() { // #224: a WHOOP export leaves a trailing newline / non-breaking space on the cell, so the // imported "Did you take magnesium?\n" must fold onto the starter, NOT add a second row. - let cat = JournalCatalogStore.mergeCatalog( - imported: ["Did you take magnesium?\n", "Did you take magnesium?"], - custom: []) + let cat = makeCatalog().resolvedItems( + imported: ["Did you take magnesium?\n", "Did you take magnesium?", "Did you take magnesium?\u{00a0}"]) + .map(\.canonical) let magCount = cat.filter { $0.components(separatedBy: .whitespacesAndNewlines).filter { !$0.isEmpty }.joined(separator: " ") .caseInsensitiveCompare("Did you take magnesium?") == .orderedSame @@ -66,15 +80,57 @@ final class JournalLogicTests: XCTestCase { @MainActor func testHiddenQuestionsFilteredOutCaseInsensitively() { // Hide one starter (different casing) + one custom; both must drop from the merged catalog. - let cat = JournalCatalogStore.mergeCatalog( - imported: [], - custom: ["Did you nap?"], - hidden: ["did you drink any alcohol?", "DID YOU NAP?"]) + let store = makeCatalog(custom: ["Did you nap?"], + hidden: ["did you drink any alcohol?", "DID YOU NAP?"]) + let cat = store.resolvedItems(imported: []).map(\.canonical) XCTAssertFalse(cat.contains { $0.caseInsensitiveCompare("Did you drink any alcohol?") == .orderedSame }) XCTAssertFalse(cat.contains { $0.caseInsensitiveCompare("Did you nap?") == .orderedSame }) XCTAssertEqual(cat.count, JournalCatalogStore.starterQuestions.count - 1) } + @MainActor + func testSavedMetadataSurvivesNormalizedImportAndReload() throws { + let defaults = isolatedDefaults() + let store = JournalCatalogStore(defaults: defaults) + let canonical = "Did you take magnesium?" + store.rename(canonical, to: "Magnesium dose") + store.setKind(canonical, to: .numeric(unitLabel: "mg")) + store.setGroup(canonical, to: .supplements) + store.items[0].sortIndex = 7 + let saved = try XCTUnwrap(store.item(for: canonical)) + let imported = [" DID YOU TAKE MAGNESIUM?\n", canonical] + + let reloaded = JournalCatalogStore(defaults: defaults) + for catalog in [store, reloaded] { + let matches = catalog.resolvedItems(imported: imported).filter { + JournalCatalogStore.norm($0.canonical) == JournalCatalogStore.norm(canonical) + } + XCTAssertEqual(matches, [saved], "saved key, display, type, group and order survive imported aliases") + } + } + + @MainActor + func testHiddenImportedAliasCanBeRestoredWithoutDuplicatingItsSavedItem() throws { + let store = makeCatalog(hidden: ["did you drink any alcohol?"]) + let imported = [" DID YOU DRINK ANY ALCOHOL?\n", "Did you drink any alcohol?"] + let key = JournalCatalogStore.norm(imported[0]) + XCTAssertFalse(store.resolvedItems(imported: imported).contains { + JournalCatalogStore.norm($0.canonical) == key + }) + let hidden = store.resolvedItems(imported: imported, includeHidden: true).filter { + JournalCatalogStore.norm($0.canonical) == key + } + XCTAssertEqual(hidden.count, 1) + XCTAssertTrue(try XCTUnwrap(hidden.first).hidden) + store.restore(imported[0]) + let restored = store.resolvedItems(imported: imported).filter { + JournalCatalogStore.norm($0.canonical) == key + } + XCTAssertEqual(restored.count, 1) + XCTAssertFalse(try XCTUnwrap(restored.first).hidden) + XCTAssertEqual(restored.first?.canonical, hidden.first?.canonical) + } + // MARK: - v2 catalog (#322): legacy migration, rename key-stability, grouping, numeric type func testLegacyMigrationFoldsTwoArraysIntoItems() { @@ -103,7 +159,7 @@ final class JournalLogicTests: XCTestCase { // THE key-stability guarantee (#322): renaming an item changes only the display label; the // stored canonical (the DB/engine join key) is untouched, so all logged + imported history, // which is keyed on the canonical question string, still lines up after a rename. - let store = JournalCatalogStore() + let store = makeCatalog() store.items = [] // start from a clean catalog for a deterministic assertion let canonical = "Did you have caffeine late in the day?" @@ -136,7 +192,7 @@ final class JournalLogicTests: XCTestCase { @MainActor func testSetGroupAndKindPreserveCanonical() { - let store = JournalCatalogStore() + let store = makeCatalog() store.items = [] let canonical = "Did you take magnesium?" store.setGroup(canonical, to: .supplements) @@ -150,7 +206,7 @@ final class JournalLogicTests: XCTestCase { @MainActor func testResolvedItemsGroupStartersByDefaultAndDropHidden() { - let store = JournalCatalogStore() + let store = makeCatalog() store.items = [] let resolved = store.resolvedItems(imported: [], includeHidden: false) // Every starter is present with a default group and .bool kind. @@ -168,7 +224,7 @@ final class JournalLogicTests: XCTestCase { @MainActor func testAddCustomNumericItem() { - let store = JournalCatalogStore() + let store = makeCatalog() store.items = [] store.addCustom("Water (L)", kind: .numeric(unitLabel: "L"), group: .nutrition) XCTAssertTrue(store.isCustom("Water (L)")) diff --git a/android/app/src/main/java/com/noop/ui/InsightsScreen.kt b/android/app/src/main/java/com/noop/ui/InsightsScreen.kt index 590c2ad508..45022d6fa6 100644 --- a/android/app/src/main/java/com/noop/ui/InsightsScreen.kt +++ b/android/app/src/main/java/com/noop/ui/InsightsScreen.kt @@ -1281,9 +1281,8 @@ private fun ExperimentBehaviourPicker( /** * Behaviours the user actually has data for: distinct logged journal questions * (`behaviours.keys`) ∪ imported-export questions, minus the catalog's hidden set. - * Triage fix (a)/(b): we do NOT route this through `mergeJournalCatalog`, which would - * inject the whole starter catalog (and re-surface hidden behaviours), so only - * behaviours with real history are eligible, and the empty-state guard is real. + * The full catalog resolver includes starter questions without history. Experiment eligibility + * instead includes only behaviours with history and excludes hidden questions. */ private fun experimentCandidates( behaviours: Map>, diff --git a/android/app/src/main/java/com/noop/ui/JournalLog.kt b/android/app/src/main/java/com/noop/ui/JournalLog.kt index fe6c4cbec4..f11323a3e7 100644 --- a/android/app/src/main/java/com/noop/ui/JournalLog.kt +++ b/android/app/src/main/java/com/noop/ui/JournalLog.kt @@ -45,7 +45,7 @@ const val JOURNAL_DEVICE_ID = "noop-journal" /** Starter behaviour catalog (mirrors WHOOP's most popular journal questions, full-question * phrasing matching the export style). Question strings are opaque exact-match labels to the - * effects engine, so imported question strings always take precedence (mergeJournalCatalog). + * effects engine, so imported question strings lead the catalog resolution. * They are DATA, not UI literals, stored verbatim in the journal table and never localised. * Mirrors macOS JournalCatalogStore.starterQuestions value-for-value. */ val STARTER_JOURNAL_QUESTIONS: List = listOf( @@ -88,28 +88,6 @@ internal fun normJournalKey(s: String): String = } }.trim().lowercase() -/** Catalog = imported questions (exact strings → logged days join imported history), then starter - * defaults, then user customs. Case-insensitive dedupe, first casing wins, with `hidden` questions - * (starter/imported ones the user removed) filtered out. */ -internal fun mergeJournalCatalog( - imported: List, - custom: List, - hidden: List = emptyList(), - starter: List = STARTER_JOURNAL_QUESTIONS, -): List { - val hiddenSet = hidden.map { normJournalKey(it) }.toHashSet() - val out = ArrayList() - val seen = HashSet() - for (q in imported + starter + custom) { - // Display text trims surrounding whitespace; the dedup key normalises ALL whitespace (see - // normJournalKey) so an imported "…magnesium?\n" folds onto the starter (#224). - val t = q.trim() - val key = normJournalKey(q) - if (t.isNotEmpty() && key !in hiddenSet && seen.add(key)) out.add(t) - } - return out -} - /** Union of imported + native entries; on a (day, question) collision the NATIVE row wins (the * in-app answer is the user's most recent explicit action and stays editable). */ internal fun mergeJournalEntries( diff --git a/android/app/src/test/java/com/noop/ui/JournalCatalogTest.kt b/android/app/src/test/java/com/noop/ui/JournalCatalogTest.kt index b06611361e..f748c81432 100644 --- a/android/app/src/test/java/com/noop/ui/JournalCatalogTest.kt +++ b/android/app/src/test/java/com/noop/ui/JournalCatalogTest.kt @@ -114,6 +114,41 @@ class JournalCatalogTest { assertEquals(items.size, again.size) } + @Test + fun savedMetadataSurvivesNormalizedImportAndReload() { + val canonical = "Did you take magnesium?" + var saved = renameJournalItem(emptyList(), canonical, "Magnesium dose") + saved = setJournalItemKind(saved, canonical, JournalKind.Numeric("mg")) + saved = setJournalItemGroup(saved, canonical, JournalGroup.Supplements) + saved = saved.map { it.copy(sortIndex = 7) } + val imported = listOf(" DID YOU TAKE MAGNESIUM?\n", canonical) + val reloaded = decodeJournalCatalog(encodeJournalCatalog(saved)) + for (items in listOf(saved, reloaded)) { + val matches = resolveJournalItems(imported, items).filter { + normJournalKey(it.canonical) == normJournalKey(canonical) + } + assertEquals("saved key, display, type, group and order survive imported aliases", saved, matches) + } + } + + @Test + fun hiddenImportedAliasCanBeRestoredWithoutDuplicatingItsSavedItem() { + var saved = migrateLegacyJournalCatalog(emptyList(), listOf("did you drink any alcohol?")) + val imported = listOf(" DID YOU DRINK ANY ALCOHOL?\n", "Did you drink any alcohol?") + val key = normJournalKey(imported[0]) + assertFalse(resolveJournalItems(imported, saved).any { normJournalKey(it.canonical) == key }) + val hidden = resolveJournalItems(imported, saved, includeHidden = true).filter { + normJournalKey(it.canonical) == key + } + assertEquals(1, hidden.size) + assertTrue(hidden.single().hidden) + saved = restoreJournalItem(saved, imported[0]) + val restored = resolveJournalItems(imported, saved).filter { normJournalKey(it.canonical) == key } + assertEquals(1, restored.size) + assertFalse(restored.single().hidden) + assertEquals(hidden.single().canonical, restored.single().canonical) + } + // MARK: - JSON round-trip (persistence) @Test diff --git a/android/app/src/test/java/com/noop/ui/JournalLogTest.kt b/android/app/src/test/java/com/noop/ui/JournalLogTest.kt index ab75e27266..855f67e6c4 100644 --- a/android/app/src/test/java/com/noop/ui/JournalLogTest.kt +++ b/android/app/src/test/java/com/noop/ui/JournalLogTest.kt @@ -42,7 +42,7 @@ class JournalLogTest { @Test fun importedCasingWinsInCatalog() { - val cat = mergeJournalCatalog(listOf("DID YOU DRINK ANY ALCOHOL?"), emptyList()) + val cat = resolveJournalItems(listOf("DID YOU DRINK ANY ALCOHOL?"), emptyList()).map { it.canonical } assertEquals("DID YOU DRINK ANY ALCOHOL?", cat[0]) // The starter alcohol question deduped case-insensitively: 9 starters survive + 1 imported. assertEquals(STARTER_JOURNAL_QUESTIONS.size, cat.size) @@ -50,7 +50,8 @@ class JournalLogTest { @Test fun customsAppendAndBlanksDrop() { - val cat = mergeJournalCatalog(emptyList(), listOf(" ", "Did you nap?", "did you NAP?")) + val saved = migrateLegacyJournalCatalog(listOf(" ", "Did you nap?", "did you NAP?"), emptyList()) + val cat = resolveJournalItems(emptyList(), saved).map { it.canonical } assertEquals(STARTER_JOURNAL_QUESTIONS, cat.take(STARTER_JOURNAL_QUESTIONS.size)) assertEquals("Did you nap?", cat.last()) assertEquals(STARTER_JOURNAL_QUESTIONS.size + 1, cat.size) @@ -59,11 +60,11 @@ class JournalLogTest { @Test fun hiddenQuestionsAreFilteredOutCaseInsensitively() { // Hide one starter (different casing) + one custom; both must drop from the merged catalog. - val cat = mergeJournalCatalog( - imported = emptyList(), + val saved = migrateLegacyJournalCatalog( custom = listOf("Did you nap?"), hidden = listOf("did you drink any alcohol?", "DID YOU NAP?"), ) + val cat = resolveJournalItems(emptyList(), saved).map { it.canonical } assertFalse(cat.any { it.equals("Did you drink any alcohol?", ignoreCase = true) }) assertFalse(cat.any { it.equals("Did you nap?", ignoreCase = true) }) // The other 9 starters survive. @@ -74,10 +75,10 @@ class JournalLogTest { fun importedMagnesiumWithTrailingWhitespaceDoesNotDoublePrompt() { // #224: a WHOOP export leaves a trailing newline / non-breaking space on the cell, so the // imported "Did you take magnesium?\n" must fold onto the starter, NOT add a second row. - val cat = mergeJournalCatalog( - imported = listOf("Did you take magnesium?\n", "Did you take magnesium?"), - custom = emptyList(), - ) + val cat = resolveJournalItems( + imported = listOf("Did you take magnesium?\n", "Did you take magnesium?", "Did you take magnesium?\u00a0"), + savedItems = emptyList(), + ).map { it.canonical } assertEquals(1, cat.count { normJournalKey(it) == normJournalKey("Did you take magnesium?") }) // No net growth — both imported variants dedupe against the starter. assertEquals(STARTER_JOURNAL_QUESTIONS.size, cat.size)