Skip to content

fix(ui): stop false-positive conflict dialog for pages with no local edit - #321

Merged
tstapler merged 2 commits into
mainfrom
fix/false-positive-conflict-no-local-edit
Sep 10, 2026
Merged

fix(ui): stop false-positive conflict dialog for pages with no local edit#321
tstapler merged 2 commits into
mainfrom
fix/false-positive-conflict-no-local-edit

Conversation

@tstapler

Copy link
Copy Markdown
Owner

Summary

  • checkAndShowPendingConflict only auto-resolved when previousContent == diskContent (the write-loopback case). It missed the case where previousContent is blank because the page never existed locally before the change (e.g. a host-directory import of a brand-new page) — there's no local edit to protect, so the conflict dialog shouldn't appear.
  • Fixed by also auto-resolving when previousContent.isBlank().

Reported by the user: pages like a newly-created "SPIFFE" page were surfacing "Compare versions" dialogs showing "(no local edit to compare)".

Test plan

  • Added checkAndShowPendingConflict_autoResolves_When_PageNeverExistedLocallyBeforeTheChange to DiskConflictResolutionTest
  • ./gradlew :kmp:jvmTest --tests "dev.stapler.stelekit.ui.DiskConflictResolutionTest" — 23/23 pass, 0 failures

checkAndShowPendingConflict only skipped the dialog when previousContent
exactly matched diskContent. When an off-page host-directory change
introduces a brand-new page the browser never had, previousContent is
blank instead — there's no local edit to protect, but the check didn't
catch that case and surfaced a spurious conflict dialog.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

JVM Load Benchmark (Desktop)

Synthetic in-memory benchmark measuring load performance for the desktop (JVM) app.
Comparing 7e85e65b (this PR) vs cdc56208 (baseline)
Graph config: xlarge — 230 pages

Metric This PR Baseline Delta
Phase 1 TTI ↓ 0ms 1ms -1ms (-100%) ✅
Phase 2 background ↓ 0ms 0ms 0 (0%)
Phase 3 index ↓ 1ms 1ms 0 (0%)
Total ↓ 2ms 2ms 0 (0%)
Write p95 (baseline) ↓ 18ms 16ms +2ms (+13%) ⚠️
Write p95 (under load) ↓ 1ms n/a
Jank factor ↓ 0.06x n/a
↓ lower is better
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) `38.7%` byte[]_[k] `6.8%` java.lang.String_[k] `6.1%` int[]_[k] `4.9%` java.lang.Object[]_[k] `4.9%` java.util.LinkedHashMap$Entry_[k]
Top CPU hotspots (this PR) `96.5%` /usr/lib/x86_64-linux-gnu/libc.so.6 `1.2%` /tmp/sqlite-3.51.3.0-f52abb5b-428c-4d39-bd60-f6debcf7c371-libsqlitejdbc.so `0.5%` __libc_pwrite `0.3%` fsync `0.2%` SR_handler

…block

previousContent.isBlank() only inspects the page's first block. A page
with a blank root block (e.g. a spacer bullet) but real content in
later blocks would look blank and get silently auto-resolved on a disk
overwrite, discarding the local edit with no dialog and no way to undo.

Added PendingConflict.pageExistedLocally, computed from whether any
block on the page has non-blank content, and switched the auto-resolve
check to use that instead.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@tstapler

Copy link
Copy Markdown
Owner Author

Adversarial review caught a real gap in the first version: the auto-resolve check used previousContent.isBlank(), but previousContent only captures the page's first block. A page with a blank root block but real content in later blocks would falsely look like "no local edit" and get silently auto-resolved on a disk overwrite — a data-loss regression.

Pushed a fix (5b5db7b): added PendingConflict.pageExistedLocally, computed from whether any block on the page has non-blank content, and switched the check to that. Added a regression test proving the dialog still surfaces for that scenario. Full DiskConflictResolutionTest suite: 24/24 pass.

@tstapler
tstapler marked this pull request as ready for review September 10, 2026 17:44
@tstapler
tstapler merged commit 6e5b29d into main Sep 10, 2026
0 of 2 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Android Load Benchmark

Instrumented benchmark on an API 30 x86_64 emulator — 500-page synthetic graph.

Comparing 7e85e65b (this PR) vs cdc56208 (baseline)
Device: API 30 x86_64 emulator — 530 pages loaded

Graph Load

Metric This PR Baseline Delta
Phase 1 TTI ↓ 56ms 38ms +18ms (+47%) ⚠️
Phase 3 index ↓ 4404ms 4644ms -240ms (-5%) ✅

Interactive Write Latency (during Phase 3)

Metric This PR Baseline Delta
Write p95 (baseline) ↓ 10ms 8ms +2ms (+25%) ⚠️
Write p95 (during phase 3) ↓ 23ms 10ms +13ms (+130%) ⚠️
Jank factor ↓ 2.3x 1.25x +1.05x (+84%) ⚠️
Concurrent writes ↑ 21 22 -1ms (-5%) ⚠️

SAF I/O Overhead (ContentProvider vs direct File read)

Measures Binder IPC cost added by ContentResolver per readFile() call.
Real SAF via ExternalStorageProvider will be higher on device; this is a lower bound.

Metric This PR Baseline Delta
Direct read / file ↓ 0.0ms 0.0ms 0 (0%)
Provider read / file ↓ 0.2ms 0.2ms 0 (0%)
IPC overhead ratio ↓ 6x 6x 0 (0%)
↓ lower is better · ↑ higher is better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant