Display PR review comments inline.

src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs

@@ -125,11 +125,6 @@ public PullRequestDetailViewModelDesigner()
 
         public Task InitializeAsync(ILocalRepositoryModel localRepository, IConnection connection, string owner, string repo, int number) => Task.CompletedTask;
 
-        public Task<string> ExtractFile(IPullRequestFileNode file, bool head)
-        {
-            return null;
-        }
-
         public string GetLocalFilePath(IPullRequestFileNode file)
         {
             return null;

src/GitHub.App/Services/PullRequestEditorService.cs

@@ -71,9 +71,26 @@ public async Task OpenFile(
 
             try
             {
-                var file = await session.GetFile(relativePath);
-                var fullPath = GetAbsolutePath(session, file);
-                var fileName = workingDirectory ? fullPath : await ExtractFile(session, file, true);
+                var fullPath = Path.Combine(session.LocalRepository.LocalPath, relativePath);
+                string fileName;
+                string commitSha;
+
+                if (workingDirectory)
+                {
+                    fileName = fullPath;
+                    commitSha = null;
+                }
+                else
+                {
+                    var file = await session.GetFile(relativePath);
+                    fileName = await pullRequestService.ExtractToTempFile(
+                        session.LocalRepository,
+                        session.PullRequest,
+                        file.RelativePath,
+                        file.CommitSha,
+                        pullRequestService.GetEncoding(session.LocalRepository, file.RelativePath));
+                    commitSha = file.CommitSha;
+                }
 
                 using (workingDirectory ? null : OpenInProvisionalTab())
                 {
@@ -84,7 +101,7 @@ public async Task OpenFile(
 
                     if (!workingDirectory)
                     {
-                        AddBufferTag(buffer, session, fullPath, null);
+                        AddBufferTag(buffer, session, fullPath, commitSha, null);
                     }
                 }
 
@@ -100,21 +117,33 @@ public async Task OpenFile(
         }
 
         /// <inheritdoc/>
-        public async Task OpenDiff(
-            IPullRequestSession session,
-            string relativePath,
-            bool workingDirectory)
+        public async Task OpenDiff(IPullRequestSession session, string relativePath, string headSha)
         {
             Guard.ArgumentNotNull(session, nameof(session));
             Guard.ArgumentNotEmptyString(relativePath, nameof(relativePath));
 
             try
             {
-                var file = await session.GetFile(relativePath);
+                var workingDirectory = headSha == null;
+                var file = await session.GetFile(relativePath, headSha ?? "HEAD");
+                var mergeBase = await pullRequestService.GetMergeBase(session.LocalRepository, session.PullRequest);
+                var encoding = pullRequestService.GetEncoding(session.LocalRepository, file.RelativePath);
                 var rightPath = file.RelativePath;
                 var leftPath = await GetBaseFileName(session, file);
-                var rightFile = workingDirectory ? GetAbsolutePath(session, file) : await ExtractFile(session, file, true);
-                var leftFile = await ExtractFile(session, file, false);
+                var rightFile = workingDirectory ?
+                    Path.Combine(session.LocalRepository.LocalPath, relativePath) :
+                    await pullRequestService.ExtractToTempFile(
+                        session.LocalRepository,
+                        session.PullRequest,
+                        relativePath,
+                        file.CommitSha,
+                        encoding);
+                var leftFile = await pullRequestService.ExtractToTempFile(
+                    session.LocalRepository,
+                    session.PullRequest,
+                    relativePath,
+                    mergeBase,
+                    encoding);
                 var leftLabel = $"{leftPath};{session.GetBaseBranchDisplay()}";
                 var rightLabel = workingDirectory ? rightPath : $"{rightPath};PR {session.PullRequest.Number}";
                 var caption = $"Diff - {Path.GetFileName(file.RelativePath)}";
@@ -148,14 +177,11 @@ public async Task OpenDiff(
                 frame.GetProperty((int)__VSFPROPID.VSFPROPID_DocView, out docView);
                 var diffViewer = ((IVsDifferenceCodeWindow)docView).DifferenceViewer;
 
-                AddBufferTag(diffViewer.LeftView.TextBuffer, session, leftPath, DiffSide.Left);
+                AddBufferTag(diffViewer.LeftView.TextBuffer, session, leftPath, mergeBase, DiffSide.Left);
 
                 if (!workingDirectory)
                 {
-                    AddBufferTag(diffViewer.RightView.TextBuffer, session, rightPath, DiffSide.Right);
-                    EnableNavigateToEditor(diffViewer.LeftView, session, file);
-                    EnableNavigateToEditor(diffViewer.RightView, session, file);
-                    EnableNavigateToEditor(diffViewer.InlineView, session, file);
+                    AddBufferTag(diffViewer.RightView.TextBuffer, session, rightPath, file.CommitSha, DiffSide.Right);
                 }
 
                 if (workingDirectory)
@@ -179,7 +205,7 @@ public async Task OpenDiff(
             Guard.ArgumentNotEmptyString(relativePath, nameof(relativePath));
             Guard.ArgumentNotNull(thread, nameof(thread));
 
-            await OpenDiff(session, relativePath, false);
+            await OpenDiff(session, relativePath, thread.CommitSha);
 
             // HACK: We need to wait here for the diff view to set itself up and move its cursor
             // to the first changed line. There must be a better way of doing this.
@@ -365,19 +391,24 @@ void ShowErrorInStatusBar(string message, Exception e)
             statusBar.ShowMessage(message + ": " + e.Message);
         }
 
-        void AddBufferTag(ITextBuffer buffer, IPullRequestSession session, string path, DiffSide? side)
+        void AddBufferTag(
+            ITextBuffer buffer,
+            IPullRequestSession session,
+            string path,
+            string commitSha,
+            DiffSide? side)
         {
             buffer.Properties.GetOrCreateSingletonProperty(
                 typeof(PullRequestTextBufferInfo),
-                () => new PullRequestTextBufferInfo(session, path, side));
+                () => new PullRequestTextBufferInfo(session, path, commitSha, side));
 
             var projection = buffer as IProjectionBuffer;
 
             if (projection != null)
             {
                 foreach (var source in projection.SourceBuffers)
                 {
-                    AddBufferTag(source, session, path, side);
+                    AddBufferTag(source, session, path, commitSha, side);
                 }
             }
         }
@@ -430,19 +461,6 @@ async Task DoNavigateToEditor(IPullRequestSession session, IPullRequestSessionFi
             }
         }
 
-        async Task<string> ExtractFile(IPullRequestSession session, IPullRequestSessionFile file, bool head)
-        {
-            var encoding = pullRequestService.GetEncoding(session.LocalRepository, file.RelativePath);
-            var relativePath = head ? file.RelativePath : await GetBaseFileName(session, file);
-
-            return await pullRequestService.ExtractFile(
-                session.LocalRepository,
-                session.PullRequest,
-                relativePath,
-                head,
-                encoding).ToTask();
-        }
-
         ITextBuffer GetBufferAt(string filePath)
         {
             IVsUIHierarchy uiHierarchy;

src/GitHub.App/Services/PullRequestService.cs

@@ -326,6 +326,20 @@ public IObservable<BranchTrackingDetails> CalculateHistoryDivergence(ILocalRepos
             });
         }
 
+        public async Task<string> GetMergeBase(ILocalRepositoryModel repository, IPullRequestModel pullRequest)
+        {
+            using (var repo = gitService.GetRepository(repository.LocalPath))
+            {
+                return await gitClient.GetPullRequestMergeBase(
+                    repo,
+                    pullRequest.Base.RepositoryCloneUrl,
+                    pullRequest.Base.Sha,
+                    pullRequest.Head.Sha,
+                    pullRequest.Base.Ref,
+                    pullRequest.Number);
+            }
+        }
+
         public IObservable<TreeChanges> GetTreeChanges(ILocalRepositoryModel repository, IPullRequestModel pullRequest)
         {
             return Observable.Defer(async () =>
@@ -446,46 +460,18 @@ public IObservable<Tuple<string, int>> GetPullRequestForCurrentBranch(ILocalRepo
             });
         }
 
-        public IObservable<string> ExtractFile(
+        public async Task<string> ExtractToTempFile(
             ILocalRepositoryModel repository,
             IPullRequestModel pullRequest,
-            string fileName,
-            bool head,
+            string relativePath,
+            string commitSha,
             Encoding encoding)
         {
-            return Observable.Defer(async () =>
+            using (var repo = gitService.GetRepository(repository.LocalPath))
             {
-                using (var repo = gitService.GetRepository(repository.LocalPath))
-                {
-                    var remote = await gitClient.GetHttpRemote(repo, "origin");
-                    string sha;
-
-                    if (head)
-                    {
-                        sha = pullRequest.Head.Sha;
-                    }
-                    else
-                    {
-                        try
-                        {
-                            sha = await gitClient.GetPullRequestMergeBase(
-                                repo,
-                                pullRequest.Base.RepositoryCloneUrl,
-                                pullRequest.Base.Sha,
-                                pullRequest.Head.Sha,
-                                pullRequest.Base.Ref,
-                                pullRequest.Number);
-                        }
-                        catch (NotFoundException ex)
-                        {
-                            throw new NotFoundException($"The Pull Request file failed to load. Please check your network connection and click refresh to try again. If this issue persists, please let us know at [email protected]", ex);
-                        }
-                    }
-
-                    var file = await ExtractToTempFile(repo, pullRequest.Number, sha, fileName, encoding);
-                    return Observable.Return(file);
-                }
-            });
+                var remote = await gitClient.GetHttpRemote(repo, "origin");
+                return await ExtractToTempFile(repo, pullRequest.Number, commitSha, relativePath, encoding);
+            }
         }
 
         public Encoding GetEncoding(ILocalRepositoryModel repository, string relativePath)

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

@@ -486,27 +486,6 @@ public override async Task Refresh()
             }
         }
 
-        /// <summary>
-        /// Gets a file as it appears in the pull request.
-        /// </summary>
-        /// <param name="file">The changed file.</param>
-        /// <param name="head">
-        /// If true, gets the file at the PR head, otherwise gets the file at the PR merge base.
-        /// </param>
-        /// <returns>The path to a temporary file.</returns>
-        public Task<string> ExtractFile(IPullRequestFileNode file, bool head)
-        {
-            var path = file.RelativePath;
-            var encoding = pullRequestsService.GetEncoding(LocalRepository, path);
-
-            if (!head && file.OldPath != null)
-            {
-                path = file.OldPath;
-            }
-
-            return pullRequestsService.ExtractFile(LocalRepository, model, path, head, encoding).ToTask();
-        }
-
         /// <summary>
         /// Gets the full path to a file in the working directory.
         /// </summary>

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

@@ -44,12 +44,12 @@ public PullRequestFilesViewModel(
             this.service = service;
 
             DiffFile = ReactiveCommand.CreateAsyncTask(x =>
-                editorService.OpenDiff(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, false));
+                editorService.OpenDiff(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, "HEAD"));
             ViewFile = ReactiveCommand.CreateAsyncTask(x =>
                 editorService.OpenFile(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, false));
             DiffFileWithWorkingDirectory = ReactiveCommand.CreateAsyncTask(
                 isBranchCheckedOut,
-                x => editorService.OpenDiff(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, true));
+                x => editorService.OpenDiff(pullRequestSession, ((IPullRequestFileNode)x).RelativePath));
             OpenFileInWorkingDirectory = ReactiveCommand.CreateAsyncTask(
                 isBranchCheckedOut,
                 x => editorService.OpenFile(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, true));

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

@@ -1,6 +1,7 @@
 using System;
-using System.Diagnostics.CodeAnalysis;
+using System.Linq;
 using System.Reactive;
+using System.Threading.Tasks;
 using GitHub.Extensions;
 using GitHub.Models;
 using GitHub.Services;
@@ -13,11 +14,10 @@ namespace GitHub.ViewModels.GitHubPane
     /// </summary>
     public class PullRequestReviewFileCommentViewModel : IPullRequestReviewFileCommentViewModel
     {
-        [SuppressMessage("Microsoft.Performance", "CA1823:AvoidUnusedPrivateFields", Justification = "This will be used in a later PR")]
         readonly IPullRequestEditorService editorService;
-        [SuppressMessage("Microsoft.Performance", "CA1823:AvoidUnusedPrivateFields", Justification = "This will be used in a later PR")]
         readonly IPullRequestSession session;
         readonly IPullRequestReviewCommentModel model;
+        IInlineCommentThreadModel thread;
 
         public PullRequestReviewFileCommentViewModel(
             IPullRequestEditorService editorService,
@@ -31,6 +31,8 @@ public PullRequestReviewFileCommentViewModel(
             this.editorService = editorService;
             this.session = session;
             this.model = model;
+
+            Open = ReactiveCommand.CreateAsyncTask(DoOpen);
         }
 
         /// <inheritdoc/>
@@ -41,5 +43,27 @@ public PullRequestReviewFileCommentViewModel(
 
         /// <inheritdoc/>
         public ReactiveCommand<Unit> Open { get; }
+
+        async Task DoOpen(object o)
+        {
+            try
+            {
+                if (thread == null)
+                {
+                    var commit = model.Position.HasValue ? model.CommitId : model.OriginalCommitId;
+                    var file = await session.GetFile(RelativePath, commit);
+                    thread = file.InlineCommentThreads.FirstOrDefault(t => t.Comments.Any(c => c.Id == model.Id));
+                }
+
+                if (thread != null && thread.LineNumber != -1)
+                {
+                    await editorService.OpenDiff(session, RelativePath, thread);
+                }
+            }
+            catch (Exception)
+            {
+                // TODO: Show error.
+            }
+        }
     }
 }

src/GitHub.Exports.Reactive/Models/IInlineCommentThreadModel.cs

@@ -36,19 +36,14 @@ public interface IInlineCommentThreadModel
         bool IsStale { get; set; }
 
         /// <summary>
-        /// Gets or sets the 0-based line number of the comment.
+        /// Gets or sets the 0-based line number of the comment, or -1 of the thread is outdated.
         /// </summary>
         int LineNumber { get; set; }
 
         /// <summary>
-        /// Gets the SHA of the commit that the thread was left con.
+        /// Gets the SHA of the commit that the thread appears on.
         /// </summary>
-        string OriginalCommitSha { get; }
-
-        /// <summary>
-        /// Gets the 1-based line number in the original diff that the thread was left on.
-        /// </summary>
-        int OriginalPosition { get; }
+        string CommitSha { get; }
 
         /// <summary>
         /// Gets the relative path to the file that the thread is on.

src/GitHub.Exports.Reactive/Models/IPullRequestSessionFile.cs

@@ -33,6 +33,12 @@ public interface IPullRequestSessionFile : INotifyPropertyChanged
         /// </summary>
         string CommitSha { get; }
 
+        /// <summary>
+        /// Gets a value indicating whether <see cref="CommitSha"/> is tracking the related pull
+        /// request HEAD or whether it is pinned at a particular commit.
+        /// </summary>
+        bool IsTrackingHead { get; }
+
         /// <summary>
         /// Gets the path to the file relative to the repository.
         /// </summary>