fix(ui): resolve four UX bugs in DiskConflictDialog#203
Open
tstapler wants to merge 11 commits into
Open
Conversation
Requirements, research, plan, ADRs, and validation for fixing 4 UX bugs in DiskConflictDialog (preview granularity mismatch, hard truncation, git-literate-only manual resolve, no persistent conflict indicator), scoped from the docs/ux/journey-map.md "Disk conflict during active edit" journey. Plan went through two adversarial/architecture/UX review rounds plus a product-UX-engineering triad review before reaching READY TO BUILD. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fixes mismatched preview granularity (disk-side preview now matches the edited block, not the whole file), hard truncation with no escape hatch (adds a full-screen line-diff view), git-marker-only manual resolve (adds inline explanation and a post-write reminder), and no persistent indicator for deferred conflicts (adds a per-row sidebar icon plus an always-visible fallback badge for pages outside Favorites/Recent). Implements project_plans/disk-conflict-dialog-ux/implementation/plan.md, which went through two adversarial/architecture/UX review rounds and a product-UX-engineering triad review before implementation, plus a post-implementation 3-layer verify pass (idioms, architecture/refactor, correctness) that found and fixed a real parentUuid-scoping bug in the block-position matcher's plausibility check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the disk-conflict UX flow by (1) aligning “Disk version” to block-level scope where possible, (2) adding a full-screen line-diff escape hatch, (3) adding clearer manual-resolve guidance + snackbar feedback, and (4) making deferred conflicts visible via sidebar indicators—while also fixing the underlying pendingConflicts lifecycle bug and an adjacent stale-content race.
Changes:
- Add disk-side block matching (
DiskConflictBlockMatcher) and wireDiskConflict.diskBlockContentthrough conflict creation and manual-resolve. - Add “View full comparison” full-screen diff view with mutual-exclusion gating vs. the existing modal dialog.
- Fix deferred-conflict lifecycle (clear
pendingConflictsonly after resolution) and add sidebar per-row + global count indicators, with expanded regression tests.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| project_plans/disk-conflict-dialog-ux/research/stack.md | Research notes on current implementation and relevant reuse points. |
| project_plans/disk-conflict-dialog-ux/research/pitfalls.md | Identifies codebase-specific risks (parsing, identity, lifecycle, races). |
| project_plans/disk-conflict-dialog-ux/research/features.md | Surveys comparable UX patterns and maps them to SteleKit gaps. |
| project_plans/disk-conflict-dialog-ux/research/architecture.md | End-to-end watcher→VM→dialog trace and proposed integration points. |
| project_plans/disk-conflict-dialog-ux/requirements.md | Formalizes the four UX bugs and acceptance criteria. |
| project_plans/disk-conflict-dialog-ux/implementation/validation.md | Requirement→test mapping and coverage gap audit. |
| project_plans/disk-conflict-dialog-ux/implementation/plan.md | Full implementation plan/phasing for the UX + lifecycle fixes. |
| project_plans/disk-conflict-dialog-ux/implementation/architecture-review.md | Architecture review notes and follow-up concerns. |
| project_plans/disk-conflict-dialog-ux/implementation/adversarial-review.md | Adversarial review focusing on correctness/safety failure modes. |
| project_plans/disk-conflict-dialog-ux/design/ux-review.md | UX review of indicator surfaces, copy, and interaction details. |
| project_plans/disk-conflict-dialog-ux/decisions/ADR-001-diff-library-and-fullscreen-view-full.md | ADR selecting diff library + fullscreen approach and gating rationale. |
| project_plans/disk-conflict-dialog-ux/decisions/ADR-002-session-scoped-persistent-indicator.md | ADR scoping persistence to in-session indicators only. |
| project_plans/disk-conflict-dialog-ux/decisions/ADR-003-always-visible-conflict-count-badge.md | ADR adding a sidebar count surface as fallback visibility. |
| kmp/src/jvmTest/kotlin/dev/stapler/stelekit/ui/DiskConflictResolutionTest.kt | Expanded VM-level regression tests for lifecycle, wiring, and snackbar gating. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/StelekitViewModel.kt | Core lifecycle fix, disk block matching, manual-resolve improvements, full-view toggles. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/screens/DiskConflictFullScreen.kt | New fullscreen line-diff view + pure diff-state computation. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/GraphDialogLayer.kt | Enforces dialog/fullscreen mutual exclusivity and wires view-full entrypoint. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/components/Sidebar.kt | Adds per-row warning icon + always-visible conflict count banner. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/components/DiskConflictDialog.kt | Adds “View full comparison”, disk block preview fallback copy, marker explanation text. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/AppStateOptics.kt | Adds lens for pendingConflicts. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/AppState.kt | Adds diskConflictViewFullVisible, pendingConflictFilePaths, and diskBlockContent. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/App.kt | Threads pendingConflictFilePaths into LeftSidebar. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/db/DiskConflictBlockMatcher.kt | New pure matcher for block-scoped disk preview with plausibility check. |
| kmp/src/businessTest/kotlin/dev/stapler/stelekit/ui/screens/DiskConflictFullScreenStateTest.kt | Unit tests for diff-state computation edge cases. |
| kmp/src/businessTest/kotlin/dev/stapler/stelekit/db/DiskConflictBlockMatcherTest.kt | Unit tests for block matching behavior, including reorder plausibility check. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Maps the block editor experience (write/structure, link, reorganize, undo, rich content, disk-conflict resolution) into 10 journeys with Mermaid diagrams and identifies cross-cutting gaps. This is the source document that scoped the disk-conflict-dialog-ux fix in this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
JVM Load Benchmark (Desktop)Synthetic in-memory benchmark measuring load performance for the desktop (JVM) app.
Flamegraphs (this PR)**Allocation** — object allocation pressure (JDBC/SQLite churn)Alloc flamegraph not available CPU — method-level hotspots by on-CPU time CPU flamegraph not available Top allocation hotspots (this PR)`37.8%` byte[]_[k] `8%` java.lang.String_[k] `6.3%` java.util.LinkedHashMap$Entry_[k] `5.9%` int[]_[k] `3.8%` java.lang.Object[]_[k]Top CPU hotspots (this PR)`95.3%` /usr/lib/x86_64-linux-gnu/libc.so.6 `1.7%` /tmp/sqlite-3.51.3.0-cf4538fa-7685-455a-96fb-39f916d88a67-libsqlitejdbc.so `0.4%` __libc_pwrite `0.3%` SR_handler `0.2%` fsync |
Contributor
Android Load BenchmarkInstrumented benchmark on an API 30 x86_64 emulator — 500-page synthetic graph. Comparing Graph Load
Interactive Write Latency (during Phase 3)
SAF I/O Overhead (ContentProvider vs direct File read)Measures Binder IPC cost added by ContentResolver per readFile() call.
|
…view A 4-agent code review (testing, code quality, architecture, security) found that manualResolve()'s actual written conflict-marker content was never asserted, checkAndShowPendingConflict()'s diskBlockContent wiring (the off-page path) had zero coverage, buildDiffLines()'s cursor-based diff reconstruction was untested, and one existing test's pendingConflicts assertion was vacuous (true before the code under test even ran). Also fixes a stale KDoc on checkAndShowPendingConflict that described behavior the lifecycle fix intentionally removed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
manualResolve()'s newline check before the closing ">>>>>>> Disk" marker
checked conflict.diskContent.endsWith("\n"), but the text actually
appended is conflict.diskBlockContent (or a fallback excerpt) — a
different string. When the two disagreed on trailing-newline state,
this could produce a missing or extra blank line before the marker.
Found by GitHub Copilot's automated PR review.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Detekt's ModifierMissing rule flagged this composable for emitting content without accepting a Modifier — matches the existing convention on sibling screen composables like ConflictResolutionScreen. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
keepLocalChanges_dismisses_conflict only exercised the no-op path (no conflict set) and never tested actual dismiss behavior — real coverage already exists in keepLocalChanges_clears_the_pendingConflicts_entry. manualResolve_writes_conflict_markers never called manualResolve() at all — it only asserted the block's pre-existing content, making it dead code. Real coverage now exists in manualResolve_persists_conflict_markers_containing_matched_diskBlockContent and the sibling fallback-excerpt test. Both were independently flagged by three separate review passes (Gate 2 code review, this is-it-ready gate's test-quality reviewer). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ink ops insertTextAtCursor, insertLinkAtCursor, and replaceSelectionWithLink each fetched the block straight from the DB repository, bypassing BlockStateManager's in-memory optimistic state that holds the user's latest unsaved keystrokes (writes are debounced ~300-500ms before reaching the DB). If a user typed something and then inserted a tag or [[link]] within that debounce window, the insertion silently built on stale DB content instead of the fresh in-memory content — dropping the unsaved keystrokes. It also cost an unnecessary DB round-trip on every insertion, since the in-memory cache almost always already has the block during active editing. Extracted the fix into a shared findBlockOrNull() helper (in-memory first, DB fallback) and applied it consistently across all 7 call sites that previously duplicated this fetch pattern inconsistently, including the two pre-existing correct instances in updateBlockContent/applyContentChange. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…badges Both Screen.Journals and Screen.AllPages sourced their conflictFilePaths from appState.pendingConflicts.keys.toSet() directly, which only covers deferred conflicts — not the page whose DiskConflictDialog is currently open. The sidebar indicator (added earlier in this branch) already reads from AppState.pendingConflictFilePaths (pendingConflicts.keys + the open dialog's filePath) as its single source of truth; these two call sites were left on the older, narrower expression, so a page could show a conflict badge in the sidebar but not in Journals/AllPages (or vice versa) depending on whether its dialog was open vs. deferred. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Complexity x churn hotspot audit (temporal coupling from git history, line-count complexity proxy since this repo's Detekt complexity rules are intentionally disabled) identifies StelekitViewModel.kt as the top architectural risk: 139 functions across ~18 distinct bounded contexts, cross-referenced against 60+ existing planning docs/ADRs already touching it. Follow-up targeted architecture review (two independent parallel passes, SOLID/Clean Code + Clean Architecture/DDD) confirms the God ViewModel diagnosis and converges independently on disk-conflict resolution as the strongest extraction candidate, plus finds a systemic DatabaseWriteActor bypass across 15+ call sites and an inconsistent structural-operation layering within the class itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
manualResolve() and saveAsNewBlock() discarded the Either result from blockRepository.saveBlock(), clearing the pending-conflict entry even if the write failed. Now checks the result and surfaces a snackbar on failure instead of silently losing the user's merge. Also adds a hover tooltip to the sidebar's per-page conflict-warning icon for sighted mouse/touch users (it previously only had a screen-reader contentDescription).
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
Fixes four concrete UX bugs in
DiskConflictDialog— the modal users see when an actively-edited page's file changes on disk (git pull, sync, another device) while local edits are unconfirmed. A UX journey-mapping pass called this "the single most anxiety-inducing moment in the editor," and this PR fixes it without changing the underlying last-writer-wins resolution model.Context
Sourced from
docs/ux/journey-map.md's "Disk conflict during active edit" journey, which found the dialog behaving like a bug in four ways: the "Disk version" preview was scoped to the whole file while "Your edit" was scoped to one block (impossible to compare); both previews hard-truncated at 200 characters with no way to see more; "Manual resolve" injected raw git conflict markers with zero explanation; and a conflict on a page other than the one open was only ever shown as a one-shot, easy-to-miss snackbar.Full requirements/research/plan/ADRs are at
project_plans/disk-conflict-dialog-ux/.Changes
DiskConflictBlockMatcher.kt, new): parses the incoming disk content and matches it back to the locally-edited block by ordinal position, with a content-hash plausibility check that catches a same-count sibling reorder and falls back to "no match" rather than risk showing misattributed content.DiskConflictFullScreen.kt, new): a full-screen line-diff view using the already-declared-but-previously-unusedkotlin-multiplatform-diffdependency, reachable from the dialog without dismissing it. Handles the identical-content edge case (a known mtime-race that can produce a spurious conflict) and is WCAG-compliant (diff lines are marked with+/-/Sidebar.kt,App.kt): a per-row warning icon for pages in Favorites/Recent, plus an always-visible fallback badge (with an honest "cleared on app restart" label) for pages outside both lists, so no page with an unresolved conflict is ever left with zero signal.StelekitViewModel.kt): the deferred-conflict tracking map was being cleared the moment a user navigated to the affected page — before they'd actually resolved anything. Moved the clear to the tail of all four resolution paths, and closed an adjacent stale-content race where a second external change arriving mid-resolution could get silently dropped.DiskConflictResolutionTest.kt,DiskConflictBlockMatcherTest.kt, andDiskConflictFullScreenStateTest.kt— covering the lifecycle fix, block-matching correctness (including the reorder/false-positive cases), diff-state edge cases, and standing-collector resilience to malformed disk content.Impact
DiskConflictDialogand its supporting ViewModel/state — no change to the underlying conflict-resolution model (still last-writer-wins with four manual choices).DiskConflictgained a new optional field (diskBlockContent: String? = null); existing call sites/fixtures are unaffected.remember, all new DB reads are page-scoped (not whole-graph).kotlin-multiplatform-diff:1.3.0, which was already declared inbuild.gradle.ktsbut unused until now.Testing
jvmTestsuite green — 0 failures)./gradlew :kmp:compileKotlinJvm), no regressions in the broader test suitebazel run //kmp:desktop_apppass before merge to eyeball the "View full comparison" flowReviewer Notes
DiskConflictBlockMatcher.matchDiskBlockContent's plausibility check (content-hash collision detection against true siblings only — this was independently flagged and fixed during a 3-layer post-implementation verify pass); theGraphDialogLayer.ktgating that makesDiskConflictDialogandDiskConflictFullScreenmutually exclusive in composition.project_plans/disk-conflict-dialog-ux/implementation/plan.md's "Deliberately Deferred" section):SidecarManager's content-hash identity recovery wired into the ViewModel, out of proportion to this fix's scope.LlmSuggestionReviewScreenpattern) is a natural next project;buildDiffLines's line-reconstruction logic (the most failure-prone new code, per all three verify-pass reviewers) currently has no direct unit test — it'sprivate, only exercised via Compose rendering today.Related
Sourced from the UX journey-mapping pass at
docs/ux/journey-map.md(not yet merged separately).