Merge branch 'master' into feature/navigate-diff-to-editor

Author: jcansdale

src/GitHub.App/ViewModels/GitHubPane/NavigationViewModel.cs

@@ -25,8 +25,8 @@ public class NavigationViewModel : ViewModelBase, INavigationViewModel
         public NavigationViewModel()
         {
             history = new ReactiveList<IPanePageViewModel>();
-            history.BeforeItemsAdded.Subscribe(BeforeItemAdded);
-            history.CollectionChanged += CollectionChanged;
+            history.Changing.Subscribe(CollectionChanging);
+            history.Changed.Subscribe(CollectionChanged);
 
             var pos = this.WhenAnyValue(
                 x => x.Index,
@@ -107,7 +107,7 @@ public bool Forward()
         public void Clear()
         {
             Index = -1;
-            history.RemoveRange(0, history.Count);
+            history.Clear();
         }
 
         public int RemoveAll(IPanePageViewModel page)
@@ -139,27 +139,34 @@ void ItemRemoved(int removedIndex, IPanePageViewModel page)
             }
         }
 
-        void CollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
+        void CollectionChanging(NotifyCollectionChangedEventArgs e)
         {
-            using (DelayChangeNotifications())
+            if (e.Action == NotifyCollectionChangedAction.Add)
             {
-                switch (e.Action)
+                foreach (IPanePageViewModel page in e.NewItems)
                 {
-                    case NotifyCollectionChangedAction.Add:
-                        break;
-
-                    case NotifyCollectionChangedAction.Remove:
-                        for (var i = 0; i < e.OldItems.Count; ++i)
-                        {
-                            ItemRemoved(e.OldStartingIndex + i, (IPanePageViewModel)e.OldItems[i]);
-                        }
-                        break;
-
-                    case NotifyCollectionChangedAction.Reset:
-                        throw new NotSupportedException();
+                    BeforeItemAdded(page);
+                }
+            }
+            else if (e.Action == NotifyCollectionChangedAction.Reset)
+            {
+                foreach (var page in history)
+                {
+                    page.Dispose();
+                }
+            }
+        }
 
-                    default:
-                        throw new NotImplementedException();
+        void CollectionChanged(NotifyCollectionChangedEventArgs e)
+        {
+            using (DelayChangeNotifications())
+            {
+                if (e.Action == NotifyCollectionChangedAction.Remove)
+                {
+                    for (var i = 0; i < e.OldItems.Count; ++i)
+                    {
+                        ItemRemoved(e.OldStartingIndex + i, (IPanePageViewModel)e.OldItems[i]);
+                    }
                 }
             }
         }

src/GitHub.InlineReviews/Models/InlineCommentThreadModel.cs

@@ -39,6 +39,7 @@ public InlineCommentThreadModel(
 
             Comments = comments.ToList();
             DiffMatch = diffMatch;
+            DiffLineType = diffMatch[0].Type;
             OriginalCommitSha = originalCommitSha;
             OriginalPosition = originalPosition;
             RelativePath = relativePath;
@@ -51,7 +52,7 @@ public InlineCommentThreadModel(
         public IList<DiffLine> DiffMatch { get; }
 
         /// <inheritdoc/>
-        public DiffChangeType DiffLineType => DiffMatch.First().Type;
+        public DiffChangeType DiffLineType { get; }
 
         /// <inheritdoc/>
         public bool IsStale

src/GitHub.InlineReviews/Services/PullRequestSessionService.cs

@@ -11,11 +11,13 @@
 using GitHub.InlineReviews.Models;
 using GitHub.Models;
 using GitHub.Primitives;
+using GitHub.Logging;
 using GitHub.Services;
 using LibGit2Sharp;
 using Microsoft.VisualStudio.Text;
 using Microsoft.VisualStudio.Text.Projection;
 using ReactiveUI;
+using Serilog;
 
 namespace GitHub.InlineReviews.Services
 {
@@ -25,6 +27,8 @@ namespace GitHub.InlineReviews.Services
     [Export(typeof(IPullRequestSessionService))]
     public class PullRequestSessionService : IPullRequestSessionService
     {
+        static readonly ILogger log = LogManager.ForContext<PullRequestSessionService>();
+
         readonly IGitService gitService;
         readonly IGitClient gitClient;
         readonly IDiffService diffService;
@@ -88,6 +92,13 @@ public IReadOnlyList<IInlineCommentThreadModel> BuildCommentThreads(
                 var chunks = DiffUtilities.ParseFragment(hunk);
                 var chunk = chunks.Last();
                 var diffLines = chunk.Lines.Reverse().Take(5).ToList();
+                var firstLine = diffLines.FirstOrDefault();
+                if (firstLine == null)
+                {
+                    log.Warning("Ignoring in-line comment in {RelativePath} with no diff line context", relativePath);
+                    continue;
+                }
+
                 var thread = new InlineCommentThreadModel(
                     relativePath,
                     comments.Key.Item1,

test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionServiceTests.cs

@@ -53,6 +53,29 @@ Line 2
                 }
             }
 
+            [Test]
+            public async Task IgnoreCommentsWithNoDiffLineContext()
+            {
+                var baseContents = "Line 1";
+                var headContents = "Line 1";
+
+                var comment = CreateComment(@"@@ -10,7 +10,6 @@ class Program");
+
+                using (var diffService = new FakeDiffService(FilePath, baseContents))
+                {
+                    var diff = await diffService.Diff(FilePath, headContents);
+                    var pullRequest = CreatePullRequest(FilePath, comment);
+                    var target = CreateTarget(diffService);
+
+                    var result = target.BuildCommentThreads(
+                        pullRequest,
+                        FilePath,
+                        diff);
+
+                    Assert.That(result, Is.Empty);
+                }
+            }
+
             [Test]
             public async Task MatchesReviewCommentOnDifferentLine()
             {
@@ -212,7 +235,7 @@ Line 2
 
                     Assert.That(3, Is.EqualTo(threads[0].LineNumber));
                     Assert.That(
-                        new[] 
+                        new[]
                         {
                             Tuple.Create(2, DiffSide.Right),
                             Tuple.Create(3, DiffSide.Right)

test/UnitTests/GitHub.App/ViewModels/GitHubPane/NavigationViewModelTests.cs

@@ -1,4 +1,6 @@
 using System;
+using System.Collections.Generic;
+using System.Linq;
 using System.Reactive;
 using System.Reactive.Linq;
 using System.Reactive.Subjects;
@@ -309,6 +311,36 @@ public void CallsDeactivatedAndThenDisposedOnPages()
                 first.Dispose();
             });
         }
+
+        [Test]
+        public void DoesntThrowWhenHistoryHasMoreThan10Items()
+        {
+            var target = new NavigationViewModel();
+            var pages = new List<IPanePageViewModel>();
+
+            for (var i = 0; i < 11; ++i)
+            {
+                var page = CreatePage();
+                pages.Add(page);
+                target.NavigateTo(page);
+            }
+
+            foreach (var page in pages)
+            {
+                page.ClearReceivedCalls();
+            }
+
+            target.Clear();
+
+            pages.Last().Received().Deactivated();
+            pages.Last().Received().Dispose();
+
+            foreach (var page in pages.Take(pages.Count - 1))
+            {
+                page.DidNotReceive().Deactivated();
+                page.Received().Dispose();
+            }
+        }
     }
 
     public class TheRemoveMethod