fix(github): break infinite PR sync loop and improve diff panel targeting (#503)

apps/backend/internal/github/service.go

@@ -400,6 +400,8 @@ func (s *Service) ListTaskPRs(ctx context.Context, taskIDs []string) (map[string
 }
 
 // SyncTaskPR updates a TaskPR record with the latest PR status.
+// It only publishes a github.task_pr.updated event when data actually changed,
+// preventing feedback loops with frontend sync handlers.
 func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatus) error {
 	if status == nil || status.PR == nil {
 		return fmt.Errorf("sync task PR: missing PR data for task %s", taskID)
@@ -409,6 +411,17 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu
 		return err
 	}
 
+	changed := tp.State != status.PR.State ||
+		tp.PRTitle != status.PR.Title ||
+		tp.Additions != status.PR.Additions ||
+		tp.Deletions != status.PR.Deletions ||
+		tp.ReviewState != status.ReviewState ||
+		tp.ChecksState != status.ChecksState ||
+		tp.ReviewCount != status.ReviewCount ||
+		tp.PendingReviewCount != status.PendingReviewCount ||
+		!timeEqual(tp.MergedAt, status.PR.MergedAt) ||
+		!timeEqual(tp.ClosedAt, status.PR.ClosedAt)
+
 	tp.State = status.PR.State
 	tp.PRTitle = status.PR.Title
 	tp.Additions = status.PR.Additions
@@ -427,7 +440,7 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu
 		return fmt.Errorf("update task PR: %w", err)
 	}
 
-	if s.eventBus != nil {
+	if changed && s.eventBus != nil {
 		event := bus.NewEvent(events.GitHubTaskPRUpdated, "github", tp)
 		if err := s.eventBus.Publish(ctx, events.GitHubTaskPRUpdated, event); err != nil {
 			s.logger.Debug("failed to publish task PR updated event", zap.Error(err))
@@ -436,6 +449,17 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu
 	return nil
 }
 
+// timeEqual compares two nullable time pointers for equality.
+func timeEqual(a, b *time.Time) bool {
+	if a == nil && b == nil {
+		return true
+	}
+	if a == nil || b == nil {
+		return false
+	}
+	return a.Equal(*b)
+}
+
 // --- PR info and feedback (live) ---
 
 // GetPR fetches basic PR details from GitHub.

apps/backend/internal/github/service_test.go

@@ -2,9 +2,17 @@ package github
 
 import (
 	"context"
+	"database/sql"
 	"strings"
+	"sync"
 	"testing"
 	"time"
+
+	"github.com/jmoiron/sqlx"
+	_ "github.com/mattn/go-sqlite3"
+
+	"github.com/kandev/kandev/internal/common/logger"
+	"github.com/kandev/kandev/internal/events/bus"
 )
 
 func TestGetPR_NilClient(t *testing.T) {
@@ -420,6 +428,221 @@ func TestFindLatestCommentTime(t *testing.T) {
 	}
 }
 
+// --- mockEventBus for SyncTaskPR tests ---
+
+type mockEventBus struct {
+	mu     sync.Mutex
+	events []*bus.Event
+}
+
+func (m *mockEventBus) Publish(_ context.Context, _ string, event *bus.Event) error {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	m.events = append(m.events, event)
+	return nil
+}
+
+func (m *mockEventBus) Subscribe(string, bus.EventHandler) (bus.Subscription, error) {
+	return nil, nil
+}
+
+func (m *mockEventBus) QueueSubscribe(string, string, bus.EventHandler) (bus.Subscription, error) {
+	return nil, nil
+}
+
+func (m *mockEventBus) Request(context.Context, string, *bus.Event, time.Duration) (*bus.Event, error) {
+	return nil, nil
+}
+
+func (m *mockEventBus) Close() {}
+
+func (m *mockEventBus) IsConnected() bool { return true }
+
+func (m *mockEventBus) publishedCount() int {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	return len(m.events)
+}
+
+// setupSyncTest creates a Service backed by in-memory SQLite and a mock event bus.
+func setupSyncTest(t *testing.T) (*Service, *Store, *mockEventBus) {
+	t.Helper()
+
+	rawDB, err := sql.Open("sqlite3", ":memory:")
+	if err != nil {
+		t.Fatalf("open sqlite: %v", err)
+	}
+	rawDB.SetMaxOpenConns(1)
+	rawDB.SetMaxIdleConns(1)
+	sqlxDB := sqlx.NewDb(rawDB, "sqlite3")
+	t.Cleanup(func() { _ = sqlxDB.Close() })
+
+	store, err := NewStore(sqlxDB, sqlxDB)
+	if err != nil {
+		t.Fatalf("new store: %v", err)
+	}
+
+	log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"})
+	eb := &mockEventBus{}
+	svc := NewService(nil, "pat", nil, store, eb, log)
+
+	return svc, store, eb
+}
+
+func TestSyncTaskPR_PublishesEventOnChange(t *testing.T) {
+	svc, store, eb := setupSyncTest(t)
+	ctx := context.Background()
+
+	if err := store.CreateTaskPR(ctx, &TaskPR{
+		TaskID:     "t1",
+		Owner:      "owner",
+		Repo:       "repo",
+		PRNumber:   1,
+		PRURL:      "https://github.com/owner/repo/pull/1",
+		PRTitle:    "Initial",
+		HeadBranch: "feat",
+		BaseBranch: "main",
+		State:      "open",
+	}); err != nil {
+		t.Fatalf("create task PR: %v", err)
+	}
+
+	status := &PRStatus{
+		PR: &PR{
+			Number:    1,
+			Title:     "Updated title",
+			State:     "open",
+			RepoOwner: "owner",
+			RepoName:  "repo",
+		},
+	}
+
+	if err := svc.SyncTaskPR(ctx, "t1", status); err != nil {
+		t.Fatalf("sync task PR: %v", err)
+	}
+
+	if got := eb.publishedCount(); got != 1 {
+		t.Errorf("expected 1 event after change, got %d", got)
+	}
+}
+
+func TestSyncTaskPR_NoEventWhenUnchanged(t *testing.T) {
+	svc, store, eb := setupSyncTest(t)
+	ctx := context.Background()
+
+	if err := store.CreateTaskPR(ctx, &TaskPR{
+		TaskID:      "t1",
+		Owner:       "owner",
+		Repo:        "repo",
+		PRNumber:    1,
+		PRURL:       "https://github.com/owner/repo/pull/1",
+		PRTitle:     "Same title",
+		HeadBranch:  "feat",
+		BaseBranch:  "main",
+		State:       "open",
+		Additions:   10,
+		Deletions:   5,
+		ReviewState: "approved",
+		ChecksState: "success",
+		ReviewCount: 2,
+	}); err != nil {
+		t.Fatalf("create task PR: %v", err)
+	}
+
+	status := &PRStatus{
+		PR: &PR{
+			Number:    1,
+			Title:     "Same title",
+			State:     "open",
+			Additions: 10,
+			Deletions: 5,
+			RepoOwner: "owner",
+			RepoName:  "repo",
+		},
+		ReviewState: "approved",
+		ChecksState: "success",
+		ReviewCount: 2,
+	}
+
+	if err := svc.SyncTaskPR(ctx, "t1", status); err != nil {
+		t.Fatalf("sync task PR: %v", err)
+	}
+
+	if got := eb.publishedCount(); got != 0 {
+		t.Errorf("expected 0 events when unchanged, got %d", got)
+	}
+}
+
+func TestSyncTaskPR_SecondIdenticalSyncNoEvent(t *testing.T) {
+	svc, store, eb := setupSyncTest(t)
+	ctx := context.Background()
+
+	if err := store.CreateTaskPR(ctx, &TaskPR{
+		TaskID:     "t1",
+		Owner:      "owner",
+		Repo:       "repo",
+		PRNumber:   1,
+		PRURL:      "https://github.com/owner/repo/pull/1",
+		PRTitle:    "Original",
+		HeadBranch: "feat",
+		BaseBranch: "main",
+		State:      "open",
+	}); err != nil {
+		t.Fatalf("create task PR: %v", err)
+	}
+
+	status := &PRStatus{
+		PR: &PR{
+			Number:    1,
+			Title:     "Changed",
+			State:     "open",
+			RepoOwner: "owner",
+			RepoName:  "repo",
+		},
+	}
+
+	// First sync: data changed -> event.
+	if err := svc.SyncTaskPR(ctx, "t1", status); err != nil {
+		t.Fatalf("first sync: %v", err)
+	}
+	if got := eb.publishedCount(); got != 1 {
+		t.Fatalf("expected 1 event after first sync, got %d", got)
+	}
+
+	// Second sync with same data -> no additional event.
+	if err := svc.SyncTaskPR(ctx, "t1", status); err != nil {
+		t.Fatalf("second sync: %v", err)
+	}
+	if got := eb.publishedCount(); got != 1 {
+		t.Errorf("expected still 1 event after identical second sync, got %d", got)
+	}
+}
+
+func TestTimeEqual(t *testing.T) {
+	now := time.Now()
+	later := now.Add(time.Second)
+
+	tests := []struct {
+		name string
+		a, b *time.Time
+		want bool
+	}{
+		{"both nil", nil, nil, true},
+		{"a nil", nil, &now, false},
+		{"b nil", &now, nil, false},
+		{"equal", &now, &now, true},
+		{"not equal", &now, &later, false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := timeEqual(tt.a, tt.b); got != tt.want {
+				t.Errorf("timeEqual() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
 func TestTriggerPRSync_SyncsExistingWatch(t *testing.T) {
 	_, svc, mockClient, store := setupPollerTest(t)
 	ctx := context.Background()

apps/web/hooks/domains/github/use-task-pr.ts

@@ -64,14 +64,16 @@ export function useTaskPR(taskId: string | null) {
     retryRef.current = 0;
   }, [taskId]);
 
+  // Sync once when the task becomes active (freshness check).
+  // Intentionally excludes `pr` so WS-driven store updates don't re-trigger.
   useEffect(() => {
     if (!taskId) return;
-
-    // Always sync once when the task becomes active (freshness)
     refresh();
+  }, [taskId, refresh]);
 
-    // If no PR in store yet, retry periodically until found
-    if (pr) return;
+  // Retry polling when no PR is in the store yet.
+  useEffect(() => {
+    if (!taskId || pr) return;
 
     const interval = setInterval(() => {
       if (retryRef.current >= SYNC_MAX_RETRIES) {

apps/web/lib/state/dockview-layout-builders.ts

@@ -1,6 +1,8 @@
 import type { DockviewApi, AddPanelOptions } from "dockview-react";
 import {
   SIDEBAR_LOCK,
+  SIDEBAR_GROUP,
+  CENTER_GROUP,
   LAYOUT_SIDEBAR_MAX_PX,
   LAYOUT_RIGHT_MAX_PX,
   RIGHT_TOP_GROUP,
@@ -50,15 +52,21 @@ export function focusOrAddPanel(
     return;
   }
   // Guard: if the referenced group no longer exists (stale ID after layout
-  // transition), fall back to the active panel's group or the first group.
+  // transition), try the well-known center group, then the first non-sidebar
+  // group. Avoid falling back to the active panel's group because the user
+  // may have just clicked in the sidebar, which would place the new panel there.
   const pos = options.position;
   if (pos && "referenceGroup" in pos) {
     const groupExists = api.groups.some((g) => g.id === pos.referenceGroup);
     if (!groupExists) {
-      if (api.activePanel) {
-        options = { ...options, position: { referencePanel: api.activePanel.id } };
+      const centerGroup = api.groups.find((g) => g.id === CENTER_GROUP);
+      const nonSidebarGroup = api.groups.find((g) => g.id !== SIDEBAR_GROUP);
+      if (centerGroup) {
+        options = { ...options, position: { ...pos, referenceGroup: centerGroup.id } };
+      } else if (nonSidebarGroup) {
+        options = { ...options, position: { ...pos, referenceGroup: nonSidebarGroup.id } };
       } else if (api.groups.length > 0) {
-        options = { ...options, position: { referenceGroup: api.groups[0].id } };
+        options = { ...options, position: { ...pos, referenceGroup: api.groups[0].id } };
       } else {
         options = Object.fromEntries(
           Object.entries(options).filter(([k]) => k !== "position"),