End-to-end review: dedup + graph-engine tests, VM import fix, Dashboard doc resync - #45
Merged
Merged
Conversation
…ashboard doc End-to-end review pass. Findings documented in docs/CODE_REVIEW_2026-06-02.md. Changes (the safe, in-scope fixes): - ContactListViewModel: drop dead `import SwiftUI` (it used no SwiftUI symbols; violated the "ViewModels never import SwiftUI" rule). - .cursor/pages/Dashboard.md: resync with DashboardView after PR #43 (was stale — @query→@State manual fetch, title "Dashboard"→"Overview", ProgressView gate, .blackbookSyncDidComplete refresh, added the prioritizeCard). Satisfies the mandatory page-doc-sync rule that PR #43 missed. - +24 unit tests for two previously-untested, pure-logic services: - ContactDeduplicationServiceTests (14): union-find linking by name/email/phone, transitive components, merged-away exclusion, data-richness primary selection, and the mergeAll mutation path. This service auto-merges contacts and was safety-critical with zero coverage. - NetworkGraphEngineTests (10): graph build, dangling-edge rejection, tag filtering, simulation convergence + NaN-safety. - One test (testNameKeyDoesNotTrimInnerWhitespace) is a regression guard that documents a real nameKey limitation flagged in the report (#5). Test count 195 -> 219, all green on iOS + macOS. No production behavior change beyond the dead-import removal. The report's remaining findings (service-layer sync test coverage, .caption2 rule-vs-reality, nameKey whitespace, stale rules.md Icon-1 section, EntityListRow adoption, dead LocalSyncServer.swift) are left for follow-up PRs with rationale. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full end-to-end code & design review of Blackbook (17 KLOC, 45 Views, 19 Services). The complete findings report is committed at
docs/CODE_REVIEW_2026-06-02.md. This PR lands the safe, in-scope fixes the review surfaced; larger/riskier items are documented for follow-up PRs.Headline: the codebase is healthy
Group{}, noprint().Fixed here
ContactListViewModel: removed deadimport SwiftUI— it used zero SwiftUI symbols; violated "ViewModels never import SwiftUI."Dashboard.mdresynced withDashboardView— PR Fix iOS launch crash: decouple Dashboard from @Query + atomic pull apply #43 changed the view (@Query→@State, "Dashboard"→"Overview",ProgressViewgate, sync-completion refresh,prioritizeCard) without updating the page doc, breaking the mandatory page-doc-sync rule.ContactDeduplicationServiceTests(14) — safety-critical auto-merge logic: union-find linking by name/email/phone, transitive components, merged-away exclusion, data-richness primary selection,mergeAllmutation path.NetworkGraphEngineTests(10) — graph build, dangling-edge rejection, tag filtering, simulation convergence + NaN-safety.nameKeywhitespace limitation (report Add centralized backup storage, fix macOS crashes, add login gate #5).Tests: 195 → 219, all green on iOS + macOS. No production behavior change beyond the dead-import removal.
Documented for follow-up (NOT in this PR — see report)
ContactSyncService,LocalServerSyncService) — the code behind the Fix iOS launch crash: decouple Dashboard from @Query + atomic pull apply #43/Fix sync drift: mark every Contact edit as locally edited + filter merged-away from iMessage lookup #44 incidents — has zero unit coverage; needs protocol seams.nameKeydoesn't trim per-component whitespace.rules.md"Icon 1 = 48×48" contradicts code (icon1Size = 36) and all other docs;EntityListRowadoption incomplete (3 collection rows hand-roll it);.caption2used in 24 places vs. the "never caption2" rule (rule-vs-dense-table tension — needs a decision).SettingsView1043);Blackbook/Services/LocalSyncServer.swift(718 lines) is confirmed dead code.Test plan
xcodegen generateregistered both new test files🤖 Generated with Claude Code