diff --git a/mutation_queue.go b/mutation_queue.go index f32e162..273539b 100644 --- a/mutation_queue.go +++ b/mutation_queue.go @@ -35,6 +35,7 @@ type mutationOperation struct { PRID string `json:"pull_request_id"` ThreadID string `json:"thread_id,omitempty"` Body string `json:"body,omitempty"` + ReplyID string `json:"reply_id,omitempty"` Resolved bool `json:"resolved,omitempty"` Viewer string `json:"viewer,omitempty"` Original PullRequestMetadata `json:"original,omitempty"` @@ -372,9 +373,11 @@ func executeQueuedMutation( reason: "could not checkpoint the reply attempt", err: err, } } - if _, err := writer.ReplyToThread(ctx, operation.ThreadID, operation.Body); err != nil { + comment, err := writer.ReplyToThread(ctx, operation.ThreadID, operation.Body) + if err != nil { return mutationReplayMsg{operation: operation, state: mutationReplayWaiting, details: details, err: err} } + operation.ReplyID = comment.ID return verifying() case mutationResolution: if thread == nil { @@ -477,6 +480,9 @@ func queuedReplyPresent(details PRDetails, operation mutationOperation) bool { } matches := 0 for _, comment := range thread.Comments { + if operation.ReplyID != "" && comment.ID == operation.ReplyID { + return true + } if comment.Body == operation.Body && strings.EqualFold(comment.Author, operation.Viewer) && !comment.CreatedAt.Before(operation.EnqueuedAt.Add(-time.Minute)) { matches++ @@ -546,6 +552,10 @@ func (m App) projectQueuedMutations(details PRDetails) PRDetails { pendingID := "pending:" + operation.ID found := false for index := range thread.Comments { + if queuedReplyMatchesComment(thread.Comments[index], operation) { + found = true + continue + } if thread.Comments[index].ID == pendingID { thread.Comments[index].Pending = mutationNeedsAttention(operation) found = true @@ -573,6 +583,15 @@ func (m App) projectQueuedMutations(details PRDetails) PRDetails { return details } +func queuedReplyMatchesComment(comment ReviewComment, operation mutationOperation) bool { + if operation.ReplyID != "" && comment.ID == operation.ReplyID { + return true + } + return comment.Body == operation.Body && + strings.EqualFold(comment.Author, operation.Viewer) && + !comment.CreatedAt.Before(operation.EnqueuedAt.Add(-time.Minute)) +} + func mutationNeedsAttention(operation mutationOperation) bool { return operation.Blocked || operation.Unverified } diff --git a/mutation_queue_test.go b/mutation_queue_test.go index efd9ea8..8200760 100644 --- a/mutation_queue_test.go +++ b/mutation_queue_test.go @@ -303,6 +303,56 @@ func TestReplayReconcilesReplyBeforeAskingAboutAmbiguousDelivery(t *testing.T) { } } +func TestReplyProjectionUsesRemoteCommentWithoutTemporaryDuplicate(t *testing.T) { + store := loadMutationQueue("") + enqueued := time.Now().Add(-time.Second) + operation := mutationOperation{ + ID: "reply", Kind: mutationReply, Owner: "o", Repository: "r", Number: 1, + ThreadID: "thread", Body: "body", Viewer: "me", EnqueuedAt: enqueued, + } + if err := store.add(operation); err != nil { + t.Fatal(err) + } + m := NewApp(nil, "o", "r", false, 50, time.Minute) + m.mutations = store + details := PRDetails{ + PullRequest: PullRequest{Owner: "o", Repository: "r", Number: 1}, + Threads: []ReviewThread{{ID: "thread", Comments: []ReviewComment{ + {ID: "remote", Author: "me", Body: "body", CreatedAt: enqueued.Add(time.Second)}, + }}}, + } + + projected := m.projectQueuedMutations(details) + if len(projected.Threads[0].Comments) != 1 || projected.Threads[0].Comments[0].ID != "remote" { + t.Fatalf("single remote reply was duplicated: %#v", projected.Threads[0].Comments) + } + + details.Threads[0].Comments = append(details.Threads[0].Comments, ReviewComment{ + ID: "actual-duplicate", Author: "me", Body: "body", CreatedAt: enqueued.Add(2 * time.Second), + }) + projected = m.projectQueuedMutations(details) + if len(projected.Threads[0].Comments) != 2 { + t.Fatalf("remote duplicates were not preserved exactly: %#v", projected.Threads[0].Comments) + } +} + +func TestSuccessfulReplyCheckpointsRemoteCommentID(t *testing.T) { + store := loadMutationQueue("") + operation := mutationOperation{ + ID: "reply", Kind: mutationReply, Owner: "o", Repository: "r", Number: 1, + ThreadID: "thread", Body: "body", Viewer: "me", EnqueuedAt: time.Now(), + } + if err := store.add(operation); err != nil { + t.Fatal(err) + } + details := PRDetails{Threads: []ReviewThread{{ID: "thread", ViewerCanReply: true}}} + result := executeQueuedMutation(context.Background(), &recordingService{}, store, operation, details) + stored, ok := store.front() + if result.state != mutationReplayVerifying || !ok || stored.ReplyID != "new-comment" { + t.Fatalf("reply verification identity was not checkpointed: result=%#v stored=%#v", result, stored) + } +} + func TestSuccessfulResolutionWaitsForLiveVerification(t *testing.T) { store := loadMutationQueue("") operation := mutationOperation{ diff --git a/tui.go b/tui.go index d95d8b6..93483c6 100644 --- a/tui.go +++ b/tui.go @@ -1638,6 +1638,8 @@ func (m *App) trackThreadUpdates(details PRDetails) { state := m.readState.Data[prID] if state.Threads == nil { state.Threads = make(map[string]bool) + } + if state.Comments == nil { state.Comments = make(map[string]bool) } if !state.Initialized { @@ -1654,25 +1656,47 @@ func (m *App) trackThreadUpdates(details PRDetails) { } return } + readStateChanged := false for _, thread := range details.Threads { threadIsNew := !state.Threads[thread.ID] updated := threadIsNew - if threadIsNew { - m.newThreads[thread.ID] = true - } + viewerAuthoredOnly := len(thread.Comments) > 0 for _, comment := range thread.Comments { + viewerAuthored := details.ViewerLogin != "" && + strings.EqualFold(comment.Author, details.ViewerLogin) + if viewerAuthored { + if !state.Comments[comment.ID] { + state.Comments[comment.ID] = true + readStateChanged = true + } + } else { + viewerAuthoredOnly = false + } if !state.Comments[comment.ID] { updated = true m.unreadComments[comment.ID] = true } m.knownComments[comment.ID] = true } + if threadIsNew && !viewerAuthoredOnly { + m.newThreads[thread.ID] = true + } else if threadIsNew { + state.Threads[thread.ID] = true + readStateChanged = true + updated = false + } m.knownThreads[thread.ID] = true if updated { m.unreadThreads[thread.ID] = true m.updatedThreads[thread.ID] = true } } + if readStateChanged { + m.readState.Data[prID] = state + if err := m.readState.save(); err != nil { + m.recordHealth("read state", healthWarning, err.Error()) + } + } m.initializedPRs[prID] = true } diff --git a/tui_test.go b/tui_test.go index 70da7a7..0310ae7 100644 --- a/tui_test.go +++ b/tui_test.go @@ -1752,6 +1752,39 @@ func TestPollingMarksNewThreadCommentsUnread(t *testing.T) { } } +func TestPollingDoesNotMarkViewerReplyUnread(t *testing.T) { + m := NewApp(nil, "o", "r", false, 50, 10*time.Second) + m.screen = threadScreen + initial := PRDetails{ + PullRequest: PullRequest{ID: "pr", Owner: "o", Repository: "r", Number: 1}, + ViewerLogin: "me", + Threads: []ReviewThread{{ + ID: "thread", Comments: []ReviewComment{{ID: "original", Author: "reviewer"}}, + }}, + } + updated, _ := m.Update(detailsLoadedMsg{owner: "o", repo: "r", number: 1, details: initial}) + m = updated.(App) + + refreshed := initial + refreshed.Threads = []ReviewThread{{ + ID: "thread", Comments: []ReviewComment{ + {ID: "original", Author: "reviewer"}, + {ID: "own-reply", Author: "ME", Body: "sent by me"}, + }, + }} + updated, _ = m.Update(detailsLoadedMsg{owner: "o", repo: "r", number: 1, details: refreshed}) + m = updated.(App) + + if m.unreadThreads["thread"] || m.unreadComments["own-reply"] || len(m.updatedThreads) != 0 { + t.Fatalf("viewer reply was marked new: threads=%v comments=%v updated=%v", + m.unreadThreads, m.unreadComments, m.updatedThreads) + } + state := m.readState.Data["pr"] + if !state.Comments["own-reply"] { + t.Fatal("viewer reply was not persisted as read") + } +} + func TestResolvingThreadMarksItRead(t *testing.T) { service := &recordingService{} m := NewApp(service, "o", "r", false, 50, time.Second)