Skip to content

DiffViewer: pin the ApplyResult CurrentHunkIndex=-1 reset contract with a test #31

@geevensingh

Description

@geevensingh

Problem

The auto-scroll-on-file-select feature shipped in c4ed6f0 (DiffViewer: auto-scroll first hunk on file selection) relies on an implicit invariant inside DiffPaneViewModel.ApplyResult (DiffViewer/ViewModels/DiffPaneViewModel.cs:483-513):

private void ApplyResult(
    string left, string right, string? placeholder,
    IReadOnlyList<DiffHunk> hunks,
    DiffHighlightMap highlightMap,
    InlineDiffBuilder.InlineDocument inline,
    bool whitespaceOnly)
{
    ...
    _currentHunks = hunks;
    CurrentHunkIndex = -1;             // ← load-bearing
    Viewport = null;
    ...
}

LoadAndScrollToFirstHunkAsync (DiffViewer/ViewModels/DiffPaneViewModel.cs:362-381) calls JumpToFirstHunk() after the load, which sets CurrentHunkIndex = 0 and raises HunkNavigationRequested. The contract being relied upon: between LoadAsync start and the post-load continuation, CurrentHunkIndex must be reset to -1 so the subsequent JumpToFirstHunk() produces a fresh PropertyChanged notification.

If ApplyResult ever stops resetting CurrentHunkIndex to -1 (or resets it to 0 for some optimization), the auto-scroll continuation will set it to 0 again — same value — and the [ObservableProperty] source-generated setter will skip the PropertyChanged raise. The view won't scroll, and no test will fail.

The same invariant matters for several other consumers:

  • The hunk overview bar's caret marker (re-renders on CurrentHunkIndex change)
  • F7/F8 navigation (relies on CurrentHunkIndex going through -1 → 0 → ...)

Suggested fix

Add a small unit test to DiffPaneViewModelTests.cs:

[Fact]
public async Task LoadAsync_ResetsCurrentHunkIndexToMinusOne()
{
    var repo = new FakeRepoForKeyboard();
    repo.SetChanges(ModifiedChange("a.cs"), ModifiedChange("b.cs"));
    repo.LeftText  = "alpha\n";
    repo.RightText = "ALPHA\n";

    var diff = new DiffService();
    var vm = new DiffPaneViewModel(repo, diff);

    await vm.LoadAsync(EntryFor("a.cs"));
    vm.JumpToFirstHunk();
    vm.CurrentHunkIndex.Should().Be(0);

    // Switch to a different file - the contract: CurrentHunkIndex
    // resets to -1 so subsequent JumpToFirstHunk() raises PropertyChanged.
    await vm.LoadAsync(EntryFor("b.cs"));
    vm.CurrentHunkIndex.Should().Be(-1,
        "loading a new file must reset CurrentHunkIndex so a subsequent " +
        "JumpToFirstHunk on a single-hunk file still raises PropertyChanged " +
        "(load-bearing for the auto-scroll-on-file-select feature)");
}

A second test could pin the broader contract by hooking INotifyPropertyChanged and asserting that CurrentHunkIndex did fire during the load (catching the case where someone "optimizes" away the reset).

Citations

  • DiffViewer/ViewModels/DiffPaneViewModel.cs:483-513ApplyResult (the invariant)
  • DiffViewer/ViewModels/DiffPaneViewModel.cs:362-381LoadAndScrollToFirstHunkAsync (the consumer)
  • DiffViewer/ViewModels/DiffPaneViewModel.cs:632-637JumpToFirstHunk (no-ops if index already matches because [ObservableProperty] skips same-value sets)
  • Commit c4ed6f0DiffViewer: auto-scroll first hunk on file selection

Severity

Low (no current bug). Hidden coupling that's one drive-by refactor away from a silent regression.

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions