diff --git a/.cursor/pages/Dashboard.md b/.cursor/pages/Dashboard.md index e867176..7ca143d 100644 --- a/.cursor/pages/Dashboard.md +++ b/.cursor/pages/Dashboard.md @@ -15,44 +15,61 @@ The Dashboard is the app's home screen, providing an at-a-glance summary of the **File:** `Blackbook/Views/Dashboard/DashboardView.swift` **Data sources:** -- `@Query(sort: \Contact.relationshipScore, order: .reverse)` — all contacts, filtered to exclude hidden -- `@Query(sort: \Reminder.dueDate)` — all reminders +- `@State private var allContacts: [Contact]` — fetched on demand via `FetchDescriptor(sortBy: \.relationshipScore, .reverse)`, filtered to exclude hidden/merged-away via the `contacts` computed property +- `@State private var reminders: [Reminder]` — fetched on demand via `FetchDescriptor(sortBy: \.dueDate)` - `DashboardViewModel` — `@State private var viewModel` -**Lifecycle:** Calls `viewModel.recalculateScores(context:)` on appear. +> **Why not `@Query`?** As of PR #43, `DashboardView` no longer uses `@Query`. `@Query` auto-refreshed on every `ModelContext` save — including the intermediate saves a sync-pull makes — and the `_SwiftData_SwiftUI` machinery faulted the `Contact.interactions` inverse relationship while observing that partial state, crashing the app at launch (`EXC_BAD_ACCESS`). The dashboard now snapshots the store explicitly so it only ever renders settled state. -**Layout:** `NavigationStack` > `ScrollView` > `VStack(spacing: 20)` with `.padding()`. Navigation title: "Dashboard". +**Lifecycle:** +- `.task` — on first appearance waits ~500ms (so any in-flight pull-apply commits first), calls `refreshFromStore()`, sets `hasLoadedOnce = true`, then schedules `viewModel.recalculateScoresIfNeeded(context:)` ~1s later. +- `.onReceive(NotificationCenter … .blackbookSyncDidComplete)` — re-runs `refreshFromStore()` after each successful sync so newly pulled data appears without a relaunch. +- `refreshFromStore()` (`@MainActor`) — fetches contacts + reminders and recomputes `weeklyStats`. + +**Layout:** `NavigationStack` > `SwiftUI.Group` gated on `hasLoadedOnce`: +- **Loading:** centered `ProgressView` until the first fetch lands. +- **Loaded:** `ScrollView` > `VStack(spacing: 20)` with `.padding()`. + +Navigation title: **"Overview"**. **Cards (top to bottom):** 1. **Weekly Stats Card** (`weeklyStatsCard`) - Title: "This Week", icon: `chart.bar.fill`, accent gold - Shows `StatBubble` pair: total interactions count + unique people count - - Data from `viewModel.weeklyStats(from:)` + - Data from `weeklyStats` (computed by `viewModel.computeWeeklyStats(context:)`) -2. **Fading Relationships Card** (`fadingCard`) +2. **Prioritize Card** (`prioritizeCard`) + - Title: "Prioritize", icon: `pin.fill`, accent gold + - Empty: an `AddContactChip` button → `showingPrioritizePicker` sheet (`PrioritizeContactPicker`) + - Populated: `PriorityChipFlowLayout` of `PriorityContactChip`s (each a `NavigationLink(value: contact.id)`), plus a trailing `AddContactChip` + - The chip's ✕ button clears `isPriority` (via `markLocallyEdited()`) + +3. **Fading Relationships Card** (`fadingCard`) - Title: "Fading Relationships", icon: `arrow.down.right.circle.fill`, color: `fadingRed` - Empty state: "All relationships are healthy" - - Populated: `VStack(spacing: 8)` of contact rows — `HStack(spacing: 12)` with `ContactAvatarView(size: 36)`, name `.body.weight(.medium)`, last interaction `.caption`, `ScoreBadgeView`, `.padding(.vertical, 2)` + - Populated: `PriorityChipFlowLayout` of `ContactChip`s, each a `NavigationLink(value: contact.id)` - Data from `viewModel.fadingContacts(from:)` -3. **Upcoming Reminders Card** (`remindersCard`) +4. **Upcoming Reminders Card** (`remindersCard`) - Title: "Upcoming Reminders", icon: `bell.fill`, accent gold - Shows up to 5 incomplete reminders - Empty state: "No upcoming reminders" - Each row: title (line limit 1), contact name, due date (red if overdue) -4. **AI Assistant Card** (`aiCard`) +5. **AI Assistant Card** (`aiCard`) - Title: "AI Assistant", icon: `sparkles`, color: purple - Contains a `NavigationLink` to `AIInsightsView` - Label: "Get AI-powered outreach suggestions" with chevron -5. **Strongest Relationships Card** (`topContactsCard`) +6. **Strongest Relationships Card** (`topContactsCard`) - Title: "Strongest Relationships", icon: `star.fill`, color: `strongGreen` - Empty state: "Add contacts to see top relationships" - Populated: ranked list (#1, #2, etc.) in `HStack(spacing: 12)` with rank label (`.caption.weight(.bold)`, 24pt frame), `ContactAvatarView(size: 36)`, name `.body.weight(.medium)`, `ScoreBadgeView`, `.padding(.vertical, 2)` - Data from `viewModel.topContacts(from:)` +**Navigation:** `.navigationDestination(for: UUID.self)` resolves a contact id to `ContactDetailView` via the `contactsByID` map. + ### DashboardCard (Reusable Component) **File:** `Blackbook/Views/Dashboard/DashboardView.swift` (same file) diff --git a/Blackbook.xcodeproj/project.pbxproj b/Blackbook.xcodeproj/project.pbxproj index 6fc178e..da2c8a4 100644 --- a/Blackbook.xcodeproj/project.pbxproj +++ b/Blackbook.xcodeproj/project.pbxproj @@ -21,6 +21,7 @@ 1838B918B2D0E5BA722A3EF0 /* ServerMenuView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 107F0516F43606F715A04CAC /* ServerMenuView.swift */; }; 1D1FA4DA1805F09735832F43 /* Group.swift in Sources */ = {isa = PBXBuildFile; fileRef = 59D5A4C8F3FE7AFE76757690 /* Group.swift */; }; 1D5E32E9EE0B56EA56D2D7EC /* FeatureGating.swift in Sources */ = {isa = PBXBuildFile; fileRef = C0BF1D07C08681789AF0CC48 /* FeatureGating.swift */; }; + 1D99B1A030BE9CA3B7CF9713 /* ContactDeduplicationServiceTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = BBCD54BD3218CA370A819170 /* ContactDeduplicationServiceTests.swift */; }; 1E261EB5A02886029E363553 /* SyncTypesTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6169F3E4455CDCD77042D958 /* SyncTypesTests.swift */; }; 1F15A9907F62206EA97F4555 /* NetworkGraphEngine.swift in Sources */ = {isa = PBXBuildFile; fileRef = 682862BCC7F0F257DF35CEB8 /* NetworkGraphEngine.swift */; }; 25A80D3929B5841C1D224E29 /* TagModelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 12AE2904F18728288E8C2218 /* TagModelTests.swift */; }; @@ -102,6 +103,7 @@ AFC41BDFEE097478165A2D0D /* ContactDetailViewModelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 75F47D5E806EFA1DA2D5935A /* ContactDetailViewModelTests.swift */; }; B164427D17DE85E450AEA4C1 /* ActivityListView.swift in Sources */ = {isa = PBXBuildFile; fileRef = ACC2D7FC775E7A455AA78ABC /* ActivityListView.swift */; }; B383DE37B4755963A3BBD2FE /* SmartGroupsView.swift in Sources */ = {isa = PBXBuildFile; fileRef = A6DFE9C279FB07636F49D37A /* SmartGroupsView.swift */; }; + B3D765199ED7FF84739621F0 /* NetworkGraphEngineTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F8CBD51D0C167CD7EB595D1F /* NetworkGraphEngineTests.swift */; }; B7A06A425624014C337FFB6E /* BiometricServiceTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2AA93BA20CE88C3BC9C9317A /* BiometricServiceTests.swift */; }; B8795D426E8414EDC3028C87 /* ContactFormView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 98155954680EC712B26CB05D /* ContactFormView.swift */; }; B99B1B94442BAC565FEDAAA2 /* AIAssistantViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = A36AE5C06322D7E805B52341 /* AIAssistantViewModel.swift */; }; @@ -270,6 +272,7 @@ B388096991E8F365A624F046 /* BackupDetailView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BackupDetailView.swift; sourceTree = ""; }; B56B474D15D62192EC7F6305 /* ModelSyncApply.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ModelSyncApply.swift; sourceTree = ""; }; BB300BB5DF41935C07BC4210 /* ActivityFormView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ActivityFormView.swift; sourceTree = ""; }; + BBCD54BD3218CA370A819170 /* ContactDeduplicationServiceTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ContactDeduplicationServiceTests.swift; sourceTree = ""; }; BBD49AF1F502FF3C26EC5CF1 /* CalendarPickerView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CalendarPickerView.swift; sourceTree = ""; }; BEE4CD8BE91D0E8284B87F31 /* TagListView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TagListView.swift; sourceTree = ""; }; C0BF1D07C08681789AF0CC48 /* FeatureGating.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FeatureGating.swift; sourceTree = ""; }; @@ -296,6 +299,7 @@ F49D100318268DC2B37475CA /* SyncTypes.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SyncTypes.swift; sourceTree = ""; }; F767094E24F344D3643565AA /* LocationIconSuggestionView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LocationIconSuggestionView.swift; sourceTree = ""; }; F8A766A33F57F94C9BDF7ACF /* ConstantsTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ConstantsTests.swift; sourceTree = ""; }; + F8CBD51D0C167CD7EB595D1F /* NetworkGraphEngineTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NetworkGraphEngineTests.swift; sourceTree = ""; }; F8F368EF8553F3DD542C10AE /* LocationDetailView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LocationDetailView.swift; sourceTree = ""; }; FA40028F884B4B1161FA87B0 /* ContactMergeServiceTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ContactMergeServiceTests.swift; sourceTree = ""; }; FA9E2FA5C2EA9DC5CC7C31F9 /* Constants.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Constants.swift; sourceTree = ""; }; @@ -509,6 +513,7 @@ 2AA93BA20CE88C3BC9C9317A /* BiometricServiceTests.swift */, EA38C3DA0A5DB92E7C98C0F0 /* ClaudeAPIServiceTests.swift */, F8A766A33F57F94C9BDF7ACF /* ConstantsTests.swift */, + BBCD54BD3218CA370A819170 /* ContactDeduplicationServiceTests.swift */, 75F47D5E806EFA1DA2D5935A /* ContactDetailViewModelTests.swift */, ECBCBC491C2BFAA0AABA68B3 /* ContactListViewModelTests.swift */, FA40028F884B4B1161FA87B0 /* ContactMergeServiceTests.swift */, @@ -522,6 +527,7 @@ 367B4A2B6BD57C86106A9103 /* InteractionViewModelTests.swift */, 7DB7F634E85EAB2589E6F041 /* KeychainServiceTests.swift */, E47D954BD6E7D82202705588 /* LocationModelTests.swift */, + F8CBD51D0C167CD7EB595D1F /* NetworkGraphEngineTests.swift */, 9111F21061CF4BB14E6FE866 /* NetworkGraphViewModelTests.swift */, AD6C2B5FA5933483BFCEC3B4 /* NoteModelTests.swift */, AB7B07D8F95B7197892AB8D6 /* RelationshipScoreEngineTests.swift */, @@ -880,6 +886,7 @@ B7A06A425624014C337FFB6E /* BiometricServiceTests.swift in Sources */, F7DD6583F920E520F4EEBE9D /* ClaudeAPIServiceTests.swift in Sources */, E71E2B55D28997F49015FA8D /* ConstantsTests.swift in Sources */, + 1D99B1A030BE9CA3B7CF9713 /* ContactDeduplicationServiceTests.swift in Sources */, AFC41BDFEE097478165A2D0D /* ContactDetailViewModelTests.swift in Sources */, F9C8E6C6BBBDE1468720591E /* ContactListViewModelTests.swift in Sources */, 28092BCCCBD27C677F28E65F /* ContactMergeServiceTests.swift in Sources */, @@ -893,6 +900,7 @@ 819102BFD7CD40272EA76C97 /* InteractionViewModelTests.swift in Sources */, A303CE901664DED4348494BD /* KeychainServiceTests.swift in Sources */, 79377F20F80D93BE4ED30678 /* LocationModelTests.swift in Sources */, + B3D765199ED7FF84739621F0 /* NetworkGraphEngineTests.swift in Sources */, FD484B077257640144869BCF /* NetworkGraphViewModelTests.swift in Sources */, 17FC135DAFA6AACBD52DFD7D /* NoteModelTests.swift in Sources */, FD356F0646511FD58F59CCEF /* RelationshipScoreEngineTests.swift in Sources */, diff --git a/Blackbook/ViewModels/ContactListViewModel.swift b/Blackbook/ViewModels/ContactListViewModel.swift index 20855ca..92ad362 100644 --- a/Blackbook/ViewModels/ContactListViewModel.swift +++ b/Blackbook/ViewModels/ContactListViewModel.swift @@ -1,6 +1,5 @@ import Foundation import SwiftData -import SwiftUI @Observable final class ContactListViewModel { diff --git a/BlackbookTests/ContactDeduplicationServiceTests.swift b/BlackbookTests/ContactDeduplicationServiceTests.swift new file mode 100644 index 0000000..bd25288 --- /dev/null +++ b/BlackbookTests/ContactDeduplicationServiceTests.swift @@ -0,0 +1,192 @@ +import XCTest +import SwiftData +@testable import Blackbook + +/// Tests for `ContactDeduplicationService` — the union-find duplicate detector that auto-merges +/// contacts. This service is safety-critical (it mutates the contact graph via +/// `ContactMergeService`) and was previously untested. These tests exercise the pure `findGroups` +/// scan and the `mergeAll` mutation path. +@MainActor +final class ContactDeduplicationServiceTests: XCTestCase { + + private var container: ModelContainer! + private var context: ModelContext! + private let dedup = ContactDeduplicationService() + private let merger = ContactMergeService() + + override func setUpWithError() throws { + container = try TestHelpers.makeContainer() + context = ModelContext(container) + } + + override func tearDown() { + context = nil + container = nil + } + + // MARK: - Helpers + + @discardableResult + private func makeContact( + _ first: String, + _ last: String, + emails: [String] = [], + phones: [String] = [] + ) -> Contact { + let c = Contact(firstName: first, lastName: last) + c.emails = emails + c.phones = phones + context.insert(c) + return c + } + + // MARK: - findGroups: negative cases + + func testNoContactsYieldsNoGroups() throws { + XCTAssertTrue(try dedup.findGroups(in: context).isEmpty) + } + + func testSingleContactYieldsNoGroups() throws { + makeContact("Ada", "Lovelace", emails: ["ada@x.com"]) + try context.save() + XCTAssertTrue(try dedup.findGroups(in: context).isEmpty) + } + + func testDistinctContactsAreNotGrouped() throws { + makeContact("Ada", "Lovelace", emails: ["ada@x.com"], phones: ["111-111-1111"]) + makeContact("Alan", "Turing", emails: ["alan@x.com"], phones: ["222-222-2222"]) + try context.save() + XCTAssertTrue(try dedup.findGroups(in: context).isEmpty) + } + + // MARK: - findGroups: linking by each key + + func testLinkBySameName() throws { + // nameKey folds case and diacritics (but does NOT trim per-component whitespace — + // see testNameKeyDoesNotTrimInnerWhitespace below). + makeContact("José", "García") + makeContact("JOSE", "garcia") + try context.save() + let groups = try dedup.findGroups(in: context) + XCTAssertEqual(groups.count, 1) + guard groups.count == 1 else { return } + XCTAssertEqual(groups[0].duplicates.count, 1) + XCTAssertTrue(groups[0].matchReason.contains("Same name")) + } + + /// Documents a known limitation: `ContactSyncService.nameKey` trims only the *combined* + /// string's outer whitespace, not each component, so a stray inner space prevents a name + /// match. Captured as a regression guard; see review report for the proposed hardening. + func testNameKeyDoesNotTrimInnerWhitespace() throws { + makeContact("Grace", "Hopper") + makeContact("Grace", " Hopper") // leading space on last name → different key today + try context.save() + XCTAssertTrue(try dedup.findGroups(in: context).isEmpty, + "current behavior: inner whitespace is not normalized away") + } + + func testLinkBySharedEmailDespiteDifferentNames() throws { + makeContact("Robert", "Smith", emails: ["shared@x.com"]) + makeContact("Bob", "Smithe", emails: ["SHARED@x.com"]) // case-insensitive email match + try context.save() + let groups = try dedup.findGroups(in: context) + XCTAssertEqual(groups.count, 1) + XCTAssertTrue(groups[0].matchReason.contains("Shared email")) + } + + func testLinkBySharedPhoneIgnoresFormatting() throws { + makeContact("Jenny", "A", phones: ["(415) 867-5309"]) + makeContact("Jen", "B", phones: ["4158675309"]) // formatting stripped to same digits + try context.save() + let groups = try dedup.findGroups(in: context) + XCTAssertEqual(groups.count, 1) + XCTAssertTrue(groups[0].matchReason.contains("Shared phone")) + } + + func testEmptyNameKeyDoesNotLink() throws { + // Two contacts with blank names must NOT be grouped on an empty name key. + makeContact("", "", emails: ["a@x.com"]) + makeContact("", "", emails: ["b@x.com"]) + try context.save() + XCTAssertTrue(try dedup.findGroups(in: context).isEmpty) + } + + // MARK: - findGroups: transitive union-find correctness + + func testTransitiveLinkingFormsSingleComponent() throws { + // A~B by name, B~C by email, C~D by phone → all four collapse into one group. + let a = makeContact("Sam", "Vimes") + let b = makeContact("Sam", "Vimes", emails: ["sv@watch.gov"]) + let c = makeContact("Samuel", "V", emails: ["sv@watch.gov"], phones: ["555-0100"]) + let d = makeContact("S", "Vimes2", phones: ["5550100"]) + _ = (a, b, c, d) + try context.save() + let groups = try dedup.findGroups(in: context) + XCTAssertEqual(groups.count, 1) + XCTAssertEqual(groups[0].duplicates.count + 1, 4, "all four contacts should be one component") + } + + func testTwoSeparateDuplicatePairsYieldTwoGroups() throws { + makeContact("Pair", "One") + makeContact("Pair", "One") + makeContact("Pair", "Two") + makeContact("Pair", "Two") + try context.save() + let groups = try dedup.findGroups(in: context) + XCTAssertEqual(groups.count, 2) + XCTAssertTrue(groups.allSatisfy { $0.duplicates.count == 1 }) + } + + // MARK: - findGroups: exclusions & primary selection + + func testMergedAwayContactsAreExcludedFromScan() throws { + makeContact("Dup", "Erson") + let gone = makeContact("Dup", "Erson") + gone.isMergedAway = true + try context.save() + // Only one live contact with that name remains → no group. + XCTAssertTrue(try dedup.findGroups(in: context).isEmpty) + } + + func testPrimaryIsRichestContact() throws { + let sparse = makeContact("Rich", "Card") + let rich = makeContact("Rich", "Card") + // Give `rich` more data so its dataRichness wins. + TestHelpers.makeInteraction(contact: rich, in: context) + TestHelpers.makeInteraction(contact: rich, in: context) + TestHelpers.makeNote(contact: rich, in: context) + rich.emails = ["rich@x.com"] + try context.save() + let groups = try dedup.findGroups(in: context) + XCTAssertEqual(groups.count, 1) + XCTAssertEqual(groups[0].primary.id, rich.id, "the data-richer contact must be primary") + XCTAssertEqual(groups[0].duplicates.first?.id, sparse.id) + } + + // MARK: - mergeAll mutation path + + func testMergeAllSuppressesSecondariesAndReturnsCount() throws { + let keep = makeContact("Merge", "Target") + TestHelpers.makeNote(contact: keep, in: context) // make `keep` the richer primary + let dupe = makeContact("Merge", "Target") + try context.save() + + let merged = try dedup.mergeAll(using: merger, in: context) + + XCTAssertEqual(merged, 1) + XCTAssertEqual(dedup.lastMergeCount, 1) + XCTAssertEqual(dedup.lastGroupCount, 1) + XCTAssertTrue(dupe.isMergedAway, "secondary should be suppressed, not deleted") + XCTAssertEqual(dupe.mergedIntoContact?.id, keep.id) + // A second pass finds nothing because the duplicate is now merged away. + XCTAssertTrue(try dedup.findGroups(in: context).isEmpty) + } + + func testMergeAllOnCleanStoreIsNoOp() throws { + makeContact("Solo", "Contact", emails: ["solo@x.com"]) + try context.save() + let merged = try dedup.mergeAll(using: merger, in: context) + XCTAssertEqual(merged, 0) + XCTAssertEqual(dedup.lastGroupCount, 0) + } +} diff --git a/BlackbookTests/NetworkGraphEngineTests.swift b/BlackbookTests/NetworkGraphEngineTests.swift new file mode 100644 index 0000000..6b95667 --- /dev/null +++ b/BlackbookTests/NetworkGraphEngineTests.swift @@ -0,0 +1,138 @@ +import XCTest +import SwiftData +@testable import Blackbook + +/// Tests for `NetworkGraphEngine` — the force-directed layout engine behind the Network tab. +/// Covers graph construction (node/edge mapping, dangling-edge rejection), tag filtering, and +/// the convergence contract of `simulateStep`. Previously untested. +@MainActor +final class NetworkGraphEngineTests: XCTestCase { + + private var container: ModelContainer! + private var context: ModelContext! + private let engine = NetworkGraphEngine() + private let canvas = CGSize(width: 400, height: 400) + + override func setUpWithError() throws { + container = try TestHelpers.makeContainer() + context = ModelContext(container) + } + + override func tearDown() { + context = nil + container = nil + } + + // MARK: - Helpers + + @discardableResult + private func makeContact(_ first: String, score: Double = 50) -> Contact { + let c = TestHelpers.makeContact(firstName: first, lastName: "X", score: score, in: context) + return c + } + + @discardableResult + private func relate(_ a: Contact, _ b: Contact, label: String = "friend") -> ContactRelationship { + let rel = ContactRelationship(from: a, to: b, label: label) + context.insert(rel) + return rel + } + + // MARK: - buildGraph + + func testBuildGraphCreatesNodePerContact() { + let a = makeContact("A"); let b = makeContact("B"); let c = makeContact("C") + engine.buildGraph(contacts: [a, b, c], relationships: [], canvasSize: canvas) + XCTAssertEqual(engine.nodes.count, 3) + XCTAssertEqual(engine.edges.count, 0) + XCTAssertEqual(Set(engine.nodes.map(\.id)), Set([a.id, b.id, c.id])) + } + + func testBuildGraphMapsValidEdges() { + let a = makeContact("A"); let b = makeContact("B") + let rel = relate(a, b) + engine.buildGraph(contacts: [a, b], relationships: [rel], canvasSize: canvas) + XCTAssertEqual(engine.edges.count, 1) + XCTAssertEqual(engine.edges[0].fromId, a.id) + XCTAssertEqual(engine.edges[0].toId, b.id) + } + + func testBuildGraphDropsEdgeToContactNotInSet() { + // A relationship pointing at a contact that isn't part of the rendered set must be skipped, + // otherwise simulateStep would dereference a missing node index. + let a = makeContact("A"); let b = makeContact("B"); let outsider = makeContact("Z") + let danglingRel = relate(a, outsider) + engine.buildGraph(contacts: [a, b], relationships: [danglingRel], canvasSize: canvas) + XCTAssertEqual(engine.edges.count, 0, "edge to a non-rendered contact must be filtered out") + } + + func testBuildGraphPlacesNodesWithinCanvas() { + let a = makeContact("A") + engine.buildGraph(contacts: [a], relationships: [], canvasSize: canvas) + let p = engine.nodes[0].position + XCTAssertGreaterThanOrEqual(p.x, 0) + XCTAssertLessThanOrEqual(p.x, canvas.width) + XCTAssertGreaterThanOrEqual(p.y, 0) + XCTAssertLessThanOrEqual(p.y, canvas.height) + } + + func testRebuildReplacesPreviousGraph() { + let a = makeContact("A"); let b = makeContact("B") + engine.buildGraph(contacts: [a, b], relationships: [relate(a, b)], canvasSize: canvas) + XCTAssertEqual(engine.nodes.count, 2) + let c = makeContact("C") + engine.buildGraph(contacts: [c], relationships: [], canvasSize: canvas) + XCTAssertEqual(engine.nodes.count, 1) + XCTAssertEqual(engine.edges.count, 0) + XCTAssertEqual(engine.nodes[0].id, c.id) + } + + // MARK: - filteredNodes(byTagIds:) + + func testFilteredNodesEmptyFilterReturnsAll() { + let a = makeContact("A"); let b = makeContact("B") + engine.buildGraph(contacts: [a, b], relationships: [], canvasSize: canvas) + XCTAssertEqual(engine.filteredNodes(byTagIds: []).count, 2) + } + + func testFilteredNodesMatchesTaggedContactsOnly() throws { + let tag = Tag(name: "VIP", colorHex: "FF0000") + context.insert(tag) + let tagged = makeContact("Tagged") + tagged.tags = [tag] + let untagged = makeContact("Untagged") + engine.buildGraph(contacts: [tagged, untagged], relationships: [], canvasSize: canvas) + + let filtered = engine.filteredNodes(byTagIds: [tag.id]) + XCTAssertEqual(filtered.count, 1) + XCTAssertEqual(filtered.first?.id, tagged.id) + } + + func testFilteredNodesUnmatchedTagReturnsNone() { + let a = makeContact("A") + engine.buildGraph(contacts: [a], relationships: [], canvasSize: canvas) + XCTAssertTrue(engine.filteredNodes(byTagIds: [UUID()]).isEmpty) + } + + // MARK: - simulateStep + + func testSimulateStepEventuallyConverges() { + let a = makeContact("A"); let b = makeContact("B"); let c = makeContact("C") + engine.buildGraph(contacts: [a, b, c], relationships: [relate(a, b)], canvasSize: canvas) + var converged = false + // The engine damps velocity each step; within a generous bound it should report convergence. + for _ in 0..<2000 where !converged { + converged = engine.simulateStep(canvasSize: canvas) + } + XCTAssertTrue(converged, "force simulation should converge on a small graph") + // Positions must stay finite (no NaN/inf blow-up). + for node in engine.nodes { + XCTAssertTrue(node.position.x.isFinite && node.position.y.isFinite) + } + } + + func testSimulateStepOnEmptyGraphConvergesImmediately() { + engine.buildGraph(contacts: [], relationships: [], canvasSize: canvas) + XCTAssertTrue(engine.simulateStep(canvasSize: canvas)) + } +} diff --git a/docs/CODE_REVIEW_2026-06-02.md b/docs/CODE_REVIEW_2026-06-02.md new file mode 100644 index 0000000..a771beb --- /dev/null +++ b/docs/CODE_REVIEW_2026-06-02.md @@ -0,0 +1,74 @@ +# Blackbook — End-to-End Code & Design Review + +**Date:** 2026-06-02 +**Scope:** Full codebase (17 KLOC: 45 Views, 6 ViewModels, 11 Models, 19 Services, 9 Utilities), all design/standards docs (`CLAUDE.md`, `.cursor/rules/rules.md`, `.cursor/pages/*.md`), and the test suite. +**Baseline:** 182 XCTest + 13 Swift Testing = **195 tests, all green** (iOS + macOS) at start. +**After this review:** **219 tests, all green** (+24), both platforms build clean. + +Severity: **P1** correctness/data · **P2** standards/consistency · **P3** style/nits. + +--- + +## 1. Overall health — strong + +The codebase is in good shape and adheres to its own (unusually thorough) standards in the areas that matter most: + +- **MVVM/@Observable** applied consistently; ViewModels are logic-only. +- **NavigationStack discipline is correct.** Every detail/pushed view body (`ContactDetailView`, `TagDetailView`, `GroupDetailView`, `LocationDetailView`, `ActivityDetailView`) is free of a self-owned `NavigationStack`; the only `NavigationStack`s in those files belong to sheet sub-views (`AddContactsToX`, `MetViaPickerView`, …), which is exactly per spec. **No double-back-chevron regressions.** +- **Hidden-contact filtering** (`!isHidden && !isMergedAway`) is present in every view that `@Query`s `Contact`. +- **No off-actor SwiftData access.** The only `Task.detached` is `SubscriptionManager`'s StoreKit `Transaction.updates` listener — it never touches a `ModelContext`. The class of bug behind the recent crashes is absent elsewhere. +- **No bare `Group {}`**, no `print()` (uses `os.Logger`), design tokens centralized in `AppConstants.UI`. + +--- + +## 2. Fixed in this PR + +| # | Sev | Fix | +|---|-----|-----| +| 1 | P2 | **`ContactListViewModel` removed dead `import SwiftUI`.** It imported SwiftUI but used zero SwiftUI symbols — a direct violation of "ViewModels must never import SwiftUI." | +| 2 | P2 | **`.cursor/pages/Dashboard.md` resynced with `DashboardView`.** PR #43 changed the view (`@Query`→`@State` manual fetch, title "Dashboard"→"Overview", added `ProgressView` gate + `.blackbookSyncDidComplete` refresh + the `prioritizeCard`) but did not update the page doc, violating the mandatory "Page Documentation Sync" rule. Doc now matches code. | +| 3 | P1 | **+24 unit tests for two untested, pure-logic services.** `ContactDeduplicationService` (14 tests) is *safety-critical* — it auto-merges contacts — and had zero coverage. `NetworkGraphEngine` (10 tests) covers graph build, dangling-edge rejection, tag filtering, and simulation convergence. | +| 4 | P2 | **Regression guard** `testNameKeyDoesNotTrimInnerWhitespace` documents finding #5 below. | + +--- + +## 3. Findings to address (not changed here — need their own PR or a product decision) + +### P1 / P2 — correctness & test coverage + +**5. `ContactSyncService.nameKey` does not trim per-component whitespace.** +It trims only the *combined* `"first|last"` string's outer edges, so `" Hopper"` ≠ `"Hopper"` and a stray inner space defeats name-based dedup. Low incidence but real. **Proposed:** trim each component before joining. This touches a safety-critical merge path → do it in a dedicated PR with the new dedup tests as the safety net (the regression guard added here will flip when fixed). + +**6. The service layer that caused the recent production bugs has ZERO unit coverage.** +`ContactSyncService` (reattach-vs-insert matching), `LocalServerSyncService` (push `syncStatus` filter, epoch bootstrap, background-context apply) — the exact code behind the sync-drift (#44) and launch-crash (#43) incidents — are untested. They're I/O-bound, so this needs light protocol seams (inject `URLSession`/a transport protocol; split the pure apply/merge logic from the network call). **Highest-value next testing investment.** Also untested: `AuthenticationService`, `GoogleCalendarService`, `SocialEnrichmentService`, `UserActionLogger`, `SubscriptionManager`, `PhotoStorageService`, `BonjourBrowser`, and `AIAssistantViewModel` (its prompt construction is pure and mockable behind a `ClaudeAPIService` protocol). + +### P2 — design-standard consistency + +**7. `rules.md` "Icon 1 = 48×48" contradicts the code and every other doc.** +`.cursor/rules/rules.md` describes Icon 1 as `icon1Size` **48×48**, cornerRadius 10, `.title3`, and prescribes a larger "Location Row / Location Detail" layout for prominence (lines ~236, 286, 299, 363). But `Constants.swift` defines `icon1Size = 36` ("use 36pt consistently"), and `CLAUDE.md` + `Locations.md` + the actual `LocationRowView`/`LocationDetailView` all use **36×36 / cornerRadius 8 / `.font(.body)`**. The code converged on 36; the rules.md Icon-1/Location-prominence subsections are stale. **Proposed:** delete those subsections from `rules.md` (or, if 48pt prominence is actually wanted for Locations, change the code — but the consistent direction is 36). + +**8. `EntityListRow` adoption is incomplete.** +`CLAUDE.md`: "Data list rows use the shared `EntityListRow` component." Only `ActivityListView` and `MoreView` use it. `TagRowView`, `GroupRowView`, `LocationRowView` hand-roll a byte-for-byte-equivalent layout. Visually correct, but a DRY/standardization gap. **Proposed:** migrate the three collection rows to `EntityListRow` (pixel-identical, low risk). + +**9. `.caption2` used in 24 places vs. "never use `.caption2` for user-facing content."** +Breakdown: `ContactListView` (9 — table pills + last-interaction date), `ActivityListView` (4), `Subscription`/`BackupRestore` (6), others (5). Some are legitimately micro (network-graph node labels, `#1` rank badges); but the **contact-table last-interaction date and the column pills are user-facing content** in violation of the "minimum readable font is `.subheadline`, floor `.caption`" rule. This is a real tension between the rule and the dense desktop-table layout. **Decision needed:** either bump these to `.caption`/`.subheadline` and let the table reflow, *or* add an explicit documented exception for "compact multi-column table metadata." Pick one and write it down so the rule and the code agree. + +### P3 — style & hygiene + +**10. 17 files exceed the 300-line guideline.** Worst: `SettingsView.swift` (1043 — it contains `ScoringSettingsView`, `HiddenContactsView`, `HideContactsView`, `APIKeyEntryView`, `WeightSlider`, `SettingsRow`/`SettingsIcon` all inline), `BackupService` (823), `LocalSyncServer` (718 — see #12), `ModelSyncApply` (697), `ContactListView` (684), `ContactDetailView` (649). **Proposed:** split `SettingsView`'s four sheet/sub-screens into their own files (update `project.yml`/pbxproj via `xcodegen`). Non-urgent. + +**11. `Button(action: closure) { label }` in 6 places** (`InteractionLogView`, `ContactListView` ×3, `DashboardView`). These pass a stored `() -> Void` and resolve correctly to `init(action:label:)` — they compile and are not bugs — but the CLAUDE.md style rule prefers `Button { } label: { }`. Cosmetic; normalize opportunistically. + +**12. `Blackbook/Services/LocalSyncServer.swift` (718 lines) is dead code.** Confirmed: the `final class LocalSyncServer` is never instantiated anywhere. The live sync handlers are in `BlackbookServer/App/BackupServer.swift`. Keeping it invites editing the wrong file (this exact confusion is recorded in the work log for PR #36). **Proposed:** delete in a dedicated cleanup PR. + +--- + +## 4. Recommended next actions (priority order) + +1. **Add service-layer sync tests** behind protocol seams (#6) — the code most likely to regress, least covered. +2. **Resolve the `.caption2` rule vs. reality** (#9) — make the standard and the code agree. +3. **Fix `nameKey` inner-whitespace** (#5) with the dedup tests as the safety net. +4. **Delete dead `LocalSyncServer.swift`** (#12) and **purge the stale Icon-1/48×48 sections from `rules.md`** (#7). +5. **Migrate the 3 collection rows to `EntityListRow`** (#8) and **split `SettingsView`** (#10). + +Items 1–5 are independent and can land as separate small PRs.