Files
multica/server/internal/handler/comment_resolve_test.go
Naiyuan Qing 34c68e1e4c fix(comments): enforce single resolution per thread (#3984)
A thread could hold multiple resolved comments at once: ResolveComment
was a plain per-row setter that never cleared the prior resolution, and
"replacing" one was a display-only illusion (deriveThreadResolution
picks the max resolved_at). The stale rows stayed resolved in the DB and
the optimistic update flashed the new resolution, then reverted.

Make single-resolution-per-thread a write invariant:

- ClearOtherThreadResolutions: thread-scoped clear via a RECURSIVE CTE
  (root + descendants of the target, id <> target), returns each cleared
  row.
- ResolveComment handler runs the clear + set in one tx so the replace
  is atomic. It emits comment:unresolved per cleared sibling (granular
  realtime consumers patch a single comment in place and would otherwise
  keep showing the stale resolution). Target keeps its COALESCE
  idempotency and the re-resolve event suppression.
- Frontend optimistic update mirrors the invariant: resolving clears
  every other resolution in the same thread, so the cache never shows
  two at once. Unresolve still only clears its own row.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: multica-agent <github@multica.ai>
2026-06-10 14:20:39 +08:00

255 lines
8.0 KiB
Go

package handler
import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"sync"
"testing"
"time"
"github.com/multica-ai/multica/server/internal/events"
"github.com/multica-ai/multica/server/pkg/protocol"
)
// resolveCommentHTTP drives the POST /api/comments/{id}/resolve handler and
// returns the decoded response. Mirrors the resolve path the web/desktop client
// hits.
func resolveCommentHTTP(t *testing.T, commentID string) CommentResponse {
t.Helper()
w := httptest.NewRecorder()
r := newRequest("POST", "/api/comments/"+commentID+"/resolve", nil)
r = withURLParam(r, "commentId", commentID)
testHandler.ResolveComment(w, r)
if w.Code != http.StatusOK {
t.Fatalf("resolve %s: status %d: %s", commentID, w.Code, w.Body.String())
}
var resp CommentResponse
if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
t.Fatalf("decode resolve response: %v", err)
}
return resp
}
// commentResolved reports whether a comment currently has resolved_at set,
// read straight from the row so the assertion sees committed state.
func commentResolved(t *testing.T, id string) bool {
t.Helper()
var resolvedAt *time.Time
if err := testPool.QueryRow(context.Background(),
`SELECT resolved_at FROM comment WHERE id = $1`, id,
).Scan(&resolvedAt); err != nil {
t.Fatalf("query resolved_at for %s: %v", id, err)
}
return resolvedAt != nil
}
// commentResolvedAt returns the raw resolved_at timestamp (nil when cleared).
func commentResolvedAt(t *testing.T, id string) *time.Time {
t.Helper()
var resolvedAt *time.Time
if err := testPool.QueryRow(context.Background(),
`SELECT resolved_at FROM comment WHERE id = $1`, id,
).Scan(&resolvedAt); err != nil {
t.Fatalf("query resolved_at for %s: %v", id, err)
}
return resolvedAt
}
// commentEventCapture records comment:resolved / comment:unresolved events for a
// single issue. The handler bus has no Unsubscribe, so the closure filters by
// issue id; events for other tests' issues are ignored.
type commentEventCapture struct {
mu sync.Mutex
events []struct {
Type string
CommentID string
}
}
func captureCommentEvents(t *testing.T, issueID string) *commentEventCapture {
t.Helper()
cap := &commentEventCapture{}
record := func(e events.Event) {
m, ok := e.Payload.(map[string]any)
if !ok {
return
}
c, ok := m["comment"].(CommentResponse)
if !ok || c.IssueID != issueID {
return
}
cap.mu.Lock()
cap.events = append(cap.events, struct {
Type string
CommentID string
}{e.Type, c.ID})
cap.mu.Unlock()
}
testHandler.Bus.Subscribe(protocol.EventCommentResolved, record)
testHandler.Bus.Subscribe(protocol.EventCommentUnresolved, record)
return cap
}
func (c *commentEventCapture) countFor(eventType, commentID string) int {
c.mu.Lock()
defer c.mu.Unlock()
n := 0
for _, e := range c.events {
if e.Type == eventType && e.CommentID == commentID {
n++
}
}
return n
}
// resolveTestFixture seeds an issue with two independent threads so the tests
// can prove the single-resolution invariant is thread-scoped, not issue-scoped:
//
// root1
// ├── a1
// └── b1
// root2 (separate thread)
// └── a2
type resolveTestFixture struct {
IssueID string
Root1 string
A1 string
B1 string
Root2 string
A2 string
}
func newResolveTestFixture(t *testing.T) resolveTestFixture {
t.Helper()
ctx := context.Background()
var issueID string
if err := testPool.QueryRow(ctx, `
INSERT INTO issue (workspace_id, creator_type, creator_id, title)
VALUES ($1, 'member', $2, $3)
RETURNING id
`, testWorkspaceID, testUserID, "resolve fixture").Scan(&issueID); err != nil {
t.Fatalf("create issue: %v", err)
}
t.Cleanup(func() {
testPool.Exec(context.Background(), `DELETE FROM issue WHERE id = $1`, issueID)
})
base := time.Now().UTC().Add(-1 * time.Hour).Truncate(time.Second)
insert := func(parent *string, offset time.Duration, body string) string {
t.Helper()
var id string
if err := testPool.QueryRow(ctx, `
INSERT INTO comment (issue_id, workspace_id, author_type, author_id, content, type, parent_id, created_at)
VALUES ($1, $2, 'member', $3, $4, 'comment', $5, $6)
RETURNING id
`, issueID, testWorkspaceID, testUserID, body, parent, base.Add(offset)).Scan(&id); err != nil {
t.Fatalf("insert comment %q: %v", body, err)
}
return id
}
root1 := insert(nil, 0, "root1")
a1 := insert(&root1, 1*time.Minute, "a1")
b1 := insert(&root1, 2*time.Minute, "b1")
root2 := insert(nil, 10*time.Minute, "root2")
a2 := insert(&root2, 11*time.Minute, "a2")
return resolveTestFixture{IssueID: issueID, Root1: root1, A1: a1, B1: b1, Root2: root2, A2: a2}
}
// TestResolveComment_ReplacesPriorThreadResolution is the core regression for
// MUL-3180: a thread must have at most one resolved comment, and resolving a new
// one atomically clears the previous resolution (instead of leaving two resolved
// rows that the UI only papered over).
func TestResolveComment_ReplacesPriorThreadResolution(t *testing.T) {
if testHandler == nil || testPool == nil {
t.Skip("database not available")
}
fx := newResolveTestFixture(t)
cap := captureCommentEvents(t, fx.IssueID)
// Resolve a1 → it is the resolution.
resolveCommentHTTP(t, fx.A1)
if !commentResolved(t, fx.A1) {
t.Fatalf("a1 should be resolved after first resolve")
}
// Resolve b1 → b1 becomes the resolution and a1 is cleared in the same write.
resolveCommentHTTP(t, fx.B1)
if !commentResolved(t, fx.B1) {
t.Fatalf("b1 should be resolved")
}
if commentResolved(t, fx.A1) {
t.Fatalf("a1 should have been cleared when b1 was resolved (single-resolution invariant)")
}
// The cleared sibling must broadcast comment:unresolved so granular realtime
// consumers drop the stale resolution; b1 must broadcast comment:resolved.
if got := cap.countFor(protocol.EventCommentUnresolved, fx.A1); got != 1 {
t.Fatalf("expected exactly 1 comment:unresolved for a1, got %d", got)
}
if got := cap.countFor(protocol.EventCommentResolved, fx.B1); got != 1 {
t.Fatalf("expected exactly 1 comment:resolved for b1, got %d", got)
}
}
// TestResolveComment_ScopedToThread proves the clear never reaches across into a
// sibling thread on the same issue.
func TestResolveComment_ScopedToThread(t *testing.T) {
if testHandler == nil || testPool == nil {
t.Skip("database not available")
}
fx := newResolveTestFixture(t)
resolveCommentHTTP(t, fx.B1) // thread 1 resolution
resolveCommentHTTP(t, fx.A2) // thread 2 resolution — must NOT touch thread 1
if !commentResolved(t, fx.B1) {
t.Fatalf("b1 (thread 1) must stay resolved when a separate thread is resolved")
}
if !commentResolved(t, fx.A2) {
t.Fatalf("a2 (thread 2) should be resolved")
}
// Resolving the root of thread 1 overrides the reply resolution (override
// works in both directions: reply→reply and reply→root).
resolveCommentHTTP(t, fx.Root1)
if !commentResolved(t, fx.Root1) {
t.Fatalf("root1 should be resolved")
}
if commentResolved(t, fx.B1) {
t.Fatalf("b1 should be cleared when root1 becomes the resolution")
}
if !commentResolved(t, fx.A2) {
t.Fatalf("a2 (other thread) must remain resolved throughout")
}
}
// TestResolveComment_ReResolveIsIdempotent pins the COALESCE idempotency +
// event suppression: re-resolving the current resolution keeps its original
// timestamp and emits no second comment:resolved event.
func TestResolveComment_ReResolveIsIdempotent(t *testing.T) {
if testHandler == nil || testPool == nil {
t.Skip("database not available")
}
fx := newResolveTestFixture(t)
cap := captureCommentEvents(t, fx.IssueID)
resolveCommentHTTP(t, fx.A1)
first := commentResolvedAt(t, fx.A1)
if first == nil {
t.Fatalf("a1 should be resolved")
}
resolveCommentHTTP(t, fx.A1) // re-resolve same comment
second := commentResolvedAt(t, fx.A1)
if second == nil || !second.Equal(*first) {
t.Fatalf("re-resolve must keep the original resolved_at (got %v, want %v)", second, first)
}
if got := cap.countFor(protocol.EventCommentResolved, fx.A1); got != 1 {
t.Fatalf("re-resolve no-op must not emit a second comment:resolved (got %d)", got)
}
}