From 14c19ea538578655c3ff78cc9b9b3439bec46e29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Mon, 11 May 2026 16:55:58 +0100 Subject: [PATCH] feat(web): add CI hover popover on PR top-bar button (#846) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(github): plumb CI popover fields + branch protection cache Adds RequiredReviews/UnresolvedReviewThreads/ChecksTotal/ChecksPassing to TaskPR + PRStatus. Extends the GraphQL PR query with reviewThreads to populate UnresolvedReviewThreads on every poll. New branch_protection.go holds an in-memory TTL cache + BranchProtectionFetcher interface; PATClient/GHClient implement REST/CLI lookups so SyncTaskPR can resolve required_approving_review_count for the PR's base branch. SyncTaskPR preserves existing counts when a lightweight (GraphQL) sync returns 0 so the popover doesn't flap during the polling cycle. * feat(github): mock controller hooks for CI popover e2e Extends associateTaskPR to accept review/check counts, required_reviews, and unresolved_review_threads, and switches to ReplaceTaskPR so re-POSTing the same task drives the popover live-refresh path. Adds POST /pr-feedback to seed PRFeedback (checks/reviews/comments) keyed by a deterministic synthetic head_sha, and PUT /auth-health to flip the mock client's authenticated flag for the "Reconnect GitHub" branch. Mock client gains ReplaceCheckRuns/ReplaceReviews/ReplaceComments helpers and an authenticated field that defaults to true. * feat(web): CI hover popover on PR top-bar button Hovering the PR top-bar button now reveals a popover with Passed/In-Progress/Failed counts, a workflow-rollup list under the non-empty buckets (with "open on GitHub" + "+ ctx" affordances on failed rows), a review-state row ("Approved 1 / 2 required"), and unresolved-comments count. Click on the button keeps the existing "open PR detail panel" behavior so shipped tests stay green. PRFeedback is lazy-fetched on hover-open via a new SWR-style hook backed by a 20-entry LRU cache slice; the popover refetches whenever the underlying TaskPR's updated_at changes (the WS handler already applies poll deltas). Bucket logic + workflow grouping live in a small util with vitest coverage. The aggregate-popover variant for multi-PR tasks ships but the multi-PR trigger preserves its existing DropdownMenu so per-row click → addPRPanel still works. * test(e2e): pr-topbar-popover scenarios A–F covering CI popover 11 specs covering: count rows + zero-bucket hiding, workflow rollup with (N/M passed) badge under Failed, review row + unresolved comments, hover open + leave close, failed-row open/+ctx affordances, header external link, "No checks have started" empty state, "Reconnect GitHub" auth-lost branch, and live count updates on a re-associate. Adds api-client helpers (mockGitHubSeedPRFeedback, mockGitHubSetAuthHealth, extended mockGitHubAssociateTaskPR) and SessionPage popover accessors. * fix(github): address bot review comments on CI popover - branch_protection.go: evict stale entries on read so the cache map cannot grow unbounded over a long-running deployment; rewrite the comment to drop the inaccurate singleflight claim (no dedup logic exists, concurrent misses race harmlessly to set()). - gh_client.go: replace fragile strings.Contains "HTTP 404" with an isNotFoundErr helper that tolerates the formats current and future gh CLI versions use ("HTTP 404", "404 Not Found", "status: 404"). - graphql.go: extend reviewThreads with totalCount and attribute the un-fetched tail to the unresolved bucket so PRs with >100 threads don't silently undercount in the popover. - pr-ci-popover.tsx: drop the dead "icon" initializer in PRReviewRow (every branch overwrites it). Memoize useAddCheckToContext so PRWorkflowRow's reference equality stays stable across renders. - pr-topbar-button.tsx: collapse the duplicate JSDoc on usePopoverInteractions. * fix(github): address remaining bot review comments + tests Real correctness fixes: - Replace the "0/0 means counts omitted" heuristic with an explicit PRStatus.ChecksPopulated flag so the rich REST sync can clear stale totals when checks are removed from a PR upstream. - Refresh tp.BaseBranch from status.PR on each SyncTaskPR so a retargeted PR's branch-protection lookup keys off the new branch. Persist base_branch in UpdateTaskPR. - Cache 403 from /branches/.../protection (token lacks Administration: Read scope) the same way as 404 — otherwise every poller tick burns rate-limit quota retrying. Both PATClient and GHClient. - usePRCIPopover: refetch on every popover-open transition (not just on updated_at change), via a wasEnabledRef gate. Restores the intended SWR semantics that the lastSyncedRef-only guard suppressed. - groupChecksByWorkflow: prefer the URL of a job matching the group's bucket so clicking a "failed" workflow row navigates to a failing job, not an earlier passing one. Tests: - branch_protection_test.go: cache hit/miss/TTL/eviction, error not cached, no-rule cached, client-without-fetcher path. - pat_client_test.go: TestPATClient_FetchBranchProtection table-test for 200/200-with-null/404/403/500. - service_test.go: ChecksPopulated preserve-on-zero, accept real zero, base-branch retarget, change-publish for the new fields. * refactor(github): trim associateTaskPR + drop unreachable PRAggregatePopover - mock_controller.go: extract associateTaskPRRequest struct, buildTaskPRFromRequest, and ensureMockPRForRequest helpers so the handler is back under the 80-line Go limit. - pr-ci-popover.tsx: remove unreachable PRAggregatePopover export. Multi-PR tasks currently use the existing DropdownMenu trigger (no hover popover), so the aggregate component was dead code. * fix(github): drop unused anchorRef + tests for is{NotFound,Forbidden}Err - pr-topbar-button.tsx: anchorRef was leftover from the dropped click-to-pin onInteractOutside guard. Removed. - gh_client_test.go: tests for the new isNotFoundErr / isForbiddenErr helpers covering each accepted format and a few non-matches so a future gh CLI wording change is caught. * fix(github): UnresolvedReviewThreadsPopulated guard + drop unreachable nil checks - service.go: mirror the ChecksPopulated/preserve dance for UnresolvedReviewThreads. The REST single-PR poller (getPRStatus) doesn't fetch review threads, so without this guard every REST sync would clobber the GraphQL-populated value to 0 and emit a spurious github.task_pr.updated event. The GraphQL path (convertBatchedPRResult) sets the new flag. - branch_protection.go: drop unreachable s == nil / fetcher == nil checks. fetchRequiredReviews is only invoked from SyncTaskPR on a service constructed via NewService, which always initialises the cache. After a successful type assertion, the interface value cannot be nil; the !ok branch already covers nil-interface inputs. - service_test.go: new TestSyncTaskPR_UnresolvedThreads_PreservesOnLightweightSync covers the regression. Existing NewFieldsPublishEvent threads-change case updated to set UnresolvedReviewThreadsPopulated. * refactor(web): drop CI popover progress bar — last-updated is enough The animated progress bar was visual noise; the "updated Ns ago" timestamp tells the user the same thing without the indicator. Footer also hides entirely until the first fetch lands so an empty popover doesn't show a useless "" line. * fix(web): background-sync PRFeedback so popover is fresh on first hover The on-hover SWR fetch was the only refresh path, so opening the popover showed the previously-cached value while a new fetch raced behind it — visibly stale until the refetch landed. The button icon, by contrast, updates the moment a github.task_pr.updated WS push lands. Add a usePRFeedbackBackgroundSync hook mounted at the PRTopbarButton that subscribes to the same pr.updated_at change signal and refetches PRFeedback into the cache in the background. The popover hook still fires an on-open refetch as a safety net for the very first hover (before any sync). No extra polling — fetches are gated on real TaskPR changes, matching the icon's cadence. * feat(web): CI status chip in chat status bar + tweak elapsed label - New renders a compact colored circle in the chat status bar (left of the TodoIndicator, just above the input). Hover opens the same PRCIPopover anchored to the chip's top edge so the card expands upward — matching the user's "open bottom-up" ask. The chip mounts the background-sync hook so the popover is fresh on first hover even when the top-bar PR button isn't visible. - elapsedLabel: "just now" → "updated just now" so the footer reads the same regardless of how recently the cache was refreshed. * fix(web): chip color was missing — use static class map for Tailwind JIT \`getPRStatusColor().replace("text-", "bg-")\` produced class strings at runtime, which Tailwind's content scanner doesn't pick up — the chip rendered with no background. Replace with a static lookup so each literal `bg-*` class appears in the source and gets compiled into the bundle. * fix(web): chip uses currentColor SVG so the dot actually renders The previous bg-* approach relied on Tailwind compiling the literal class strings. Even with a static map some palette entries (notably bg-muted-foreground) didn't surface as a usable utility, so the chip remained transparent. Apply getPRStatusColor's text-* class to the button wrapper and fill an SVG circle with currentColor — the text-* utilities are already compiled (PRTaskIcon uses them) so the dot is guaranteed to render in the right color. * fix(web): redesign PR status chip — clipboard icon + status glyph - Move chip to right of TodoIndicator (was left). - Render as "[clipboard icon] [status glyph]" instead of a bare circle: passed → green CircleCheck, failed → red CircleX, in_progress → yellow Loader2 (spinning), merged/neutral → muted dot. Status icons reuse already-compiled text-* utilities so colors render reliably. * fix(github): ReviewCount = approved-reviewer count + ReviewCountsPopulated guard Two bugs feeding the popover's "Approved (0)" misreport: - Semantic: REST path set ReviewCount to len(reviews), which double-counts authors who left multiple reviews and includes COMMENTED rows. Replace with countApprovedReviewers — distinct authors whose latest review state is APPROVED. GraphQL converter gets the parallel countApprovedReviewerNodes helper and also populates PendingReviewCount from ReviewRequests.TotalCount (was unset, so the chip's "(K requested)" suffix was always missing). - Clobber: GraphQL polls used to write ReviewCount/PendingReviewCount=0 on every cycle because the converter never set them. Add ReviewCountsPopulated to PRStatus and gate SyncTaskPR's writes on it the same way ChecksPopulated already does, so a partial sync path preserves the persisted values instead of resetting them to zero. * fix(web): popover nitpicks (icon, review row, indent, badge) - Chip icon: ClipboardCheck → ListCheck. - PRReviewRow always renders (drop the early-return). Fresh PRs with no approvals + no protection rule now show "Awaiting review" instead of vanishing. - Indent workflow rows under the In Progress / Failed group headers (pl-5) so the hierarchy reads more clearly. - In-progress workflow badge: "0/1 ran" → "1 running". The old phrasing hid whether the job had even started. * feat(web): stacked pass-rate progress bar in CI popover Mirrors TodoIndicator's progress bar at the top of the CI section, but split into three coloured segments: green (passed) | yellow (in_progress) | red (failed) sized proportionally to each bucket's count. The label reads "Pass rate · K/N (P%)" so the user gets the same at-a-glance summary TODOs shows. Skipped/cancelled-but-not-failed jobs aren't counted — the bar always reflects the actionable picture. * fix(web): right-align review counts + drop chip hover bg - Move approval/required counts to the right edge of the review row ("Approved" on the left, "1 / 2" on the right), mirroring the bucket-row layout. The "(K requested)" suffix becomes a "· K requested" trailing fragment on the right side. - PRStatusChip: drop hover:bg-accent/50 so the trigger doesn't grow a background plate when the popover is open. * fix(web): switch CI chip icon to IconChecklist * fix(github): clear bot review-comment backlog Real correctness fixes: - pr-status-chip: pending checks / pending review now beat checks_state === "success" in chipStatus precedence — a PR with CI green but reviewers outstanding is no longer rendered as "passed". - branch_protection: drop the unreachable Reset() method. - pr-topbar-button: stop double-mounting usePRFeedbackBackgroundSync; the chip already covers it. - use-pr-ci-popover: dedupe key combines prFeedbackKey + updated_at so switching tasks with same-timestamp PRs still refetches. - use-pr-ci-popover: drop stale comment about the removed progress bar. Refactor: - client_helpers + graphql: extract reviewSample + latestReviewStateByAuthor helper so the three near-identical per-reviewer dedup loops share one implementation. Tests: - countApprovedReviewers + reduceReviewSummary unit tests. - TestSyncTaskPR_ReviewCounts_PreservesOnLightweightSync + AcceptsRealZero mirror the existing ChecksPopulated guards. - groupChecksByWorkflow distinct-URL test verifies the failed-job URL-selection fix. - branch_protection TTL tests now use testing/synctest — instant virtual time, no real-sleep flake on CI. * fix(web): clicking the CI chip no longer closes the popover HoverCard treats the trigger button as outside the content's bounding box, so a click on the chip fired Radix's onPointerDownOutside and auto-closed the popover. Add a triggerRef + onPointerDownOutside guard on HoverCardContent that filters out clicks landing on the trigger — clicking the chip is now a no-op while the popover stays open via hover. --- .../internal/github/branch_protection.go | 119 ++++ .../internal/github/branch_protection_test.go | 138 +++++ .../backend/internal/github/client_helpers.go | 96 ++- .../internal/github/client_helpers_test.go | 120 ++++ apps/backend/internal/github/gh_client.go | 59 ++ .../backend/internal/github/gh_client_test.go | 47 ++ apps/backend/internal/github/graphql.go | 103 ++-- apps/backend/internal/github/mock_client.go | 67 ++- .../internal/github/mock_controller.go | 197 +++++- apps/backend/internal/github/models.go | 77 ++- apps/backend/internal/github/pat_client.go | 32 + .../internal/github/pat_client_test.go | 74 +++ apps/backend/internal/github/service.go | 94 ++- apps/backend/internal/github/service_test.go | 247 ++++++++ apps/backend/internal/github/store.go | 41 +- apps/web/components/github/pr-ci-popover.tsx | 569 ++++++++++++++++++ apps/web/components/github/pr-status-chip.tsx | 118 ++++ .../components/github/pr-task-icon.test.ts | 3 + .../components/github/pr-topbar-button.tsx | 145 ++++- .../components/task/chat/chat-input-area.tsx | 2 + apps/web/e2e/helpers/api-client.ts | 48 ++ apps/web/e2e/pages/session-page.ts | 82 +++ .../e2e/tests/pr/pr-topbar-popover.spec.ts | 450 ++++++++++++++ .../hooks/domains/github/use-pr-ci-popover.ts | 104 ++++ apps/web/lib/github/check-buckets.test.ts | 147 +++++ apps/web/lib/github/check-buckets.ts | 126 ++++ .../lib/state/slices/github/github-slice.ts | 29 + apps/web/lib/state/slices/github/types.ts | 14 + apps/web/lib/state/store.ts | 4 + apps/web/lib/types/github.ts | 9 + 30 files changed, 3202 insertions(+), 159 deletions(-) create mode 100644 apps/backend/internal/github/branch_protection.go create mode 100644 apps/backend/internal/github/branch_protection_test.go create mode 100644 apps/web/components/github/pr-ci-popover.tsx create mode 100644 apps/web/components/github/pr-status-chip.tsx create mode 100644 apps/web/e2e/tests/pr/pr-topbar-popover.spec.ts create mode 100644 apps/web/hooks/domains/github/use-pr-ci-popover.ts create mode 100644 apps/web/lib/github/check-buckets.test.ts create mode 100644 apps/web/lib/github/check-buckets.ts diff --git a/apps/backend/internal/github/branch_protection.go b/apps/backend/internal/github/branch_protection.go new file mode 100644 index 0000000000..7abfa70ff6 --- /dev/null +++ b/apps/backend/internal/github/branch_protection.go @@ -0,0 +1,119 @@ +package github + +import ( + "context" + "sync" + "time" +) + +// BranchProtection captures the subset of a GitHub branch protection rule that +// the CI popover surfaces ("N approvals required to merge"). Stored values +// have a fixed TTL so changes to the protection rule on GitHub propagate +// without restarting the backend. +type BranchProtection struct { + RequiredApprovingReviewCount int + HasRule bool // false when GitHub returned 404 (no rule) + FetchedAt time.Time +} + +// branchProtectionCacheTTL is how long a cached protection lookup stays +// authoritative before a refetch is allowed. One hour balances API quota +// (4-PRs-per-cycle GraphQL polling) against rule-change latency (rare). +const branchProtectionCacheTTL = time.Hour + +// branchProtectionCache is an in-memory TTL cache keyed by +// "owner/repo@base_branch". Concurrent reads share the RLock; concurrent +// cache misses for the same key may each issue their own GitHub fetch and +// race to set() the result — values are deterministic so the loser's +// write is harmless, and the extra round-trips are bounded by the 1h TTL. +// Stale entries are evicted on read so the map cannot grow unbounded over +// a long-running process: callers always observe an active TTL window. +type branchProtectionCache struct { + mu sync.RWMutex + m map[string]BranchProtection + ttl time.Duration +} + +func newBranchProtectionCache() *branchProtectionCache { + return &branchProtectionCache{ + m: make(map[string]BranchProtection), + ttl: branchProtectionCacheTTL, + } +} + +func (c *branchProtectionCache) get(key string) (BranchProtection, bool) { + c.mu.RLock() + bp, ok := c.m[key] + c.mu.RUnlock() + if !ok { + return BranchProtection{}, false + } + if time.Since(bp.FetchedAt) <= c.ttl { + return bp, true + } + // Evict the stale entry so the map doesn't grow unbounded across + // long-running deployments with many distinct (owner, repo, branch) + // keys. Recheck under the write lock so a concurrent set() that just + // landed isn't clobbered. + c.mu.Lock() + if cur, present := c.m[key]; present && time.Since(cur.FetchedAt) > c.ttl { + delete(c.m, key) + } + c.mu.Unlock() + return BranchProtection{}, false +} + +func (c *branchProtectionCache) set(key string, bp BranchProtection) { + c.mu.Lock() + defer c.mu.Unlock() + c.m[key] = bp +} + +func branchProtectionKey(owner, repo, branch string) string { + return owner + "/" + repo + "@" + branch +} + +// BranchProtectionFetcher is implemented by the live GitHub clients (PAT, gh +// CLI). The mock client returns a sentinel "no rule" so e2e tests get +// deterministic results unless they seed via the mock controller. +type BranchProtectionFetcher interface { + FetchBranchProtection(ctx context.Context, owner, repo, branch string) (BranchProtection, error) +} + +// fetchRequiredReviews returns the branch-protection-derived +// RequiredApprovingReviewCount for (owner, repo, branch), or nil if: +// - the active client doesn't implement BranchProtectionFetcher, +// - the upstream call errors (token lacks scope, network down, etc.), +// - the repo has no protection rule on the base branch. +// +// Result is cached for branchProtectionCacheTTL. nil never gets cached so a +// transient error is retried on the next sync. +func (s *Service) fetchRequiredReviews(ctx context.Context, owner, repo, branch string) *int { + if owner == "" || repo == "" || branch == "" { + return nil + } + key := branchProtectionKey(owner, repo, branch) + if bp, ok := s.protectionCache.get(key); ok { + if !bp.HasRule { + return nil + } + n := bp.RequiredApprovingReviewCount + return &n + } + fetcher, ok := s.client.(BranchProtectionFetcher) + if !ok { + return nil + } + bp, err := fetcher.FetchBranchProtection(ctx, owner, repo, branch) + if err != nil { + // Don't cache errors — next sync retries. + return nil + } + bp.FetchedAt = time.Now().UTC() + s.protectionCache.set(key, bp) + if !bp.HasRule { + return nil + } + n := bp.RequiredApprovingReviewCount + return &n +} diff --git a/apps/backend/internal/github/branch_protection_test.go b/apps/backend/internal/github/branch_protection_test.go new file mode 100644 index 0000000000..475adf365a --- /dev/null +++ b/apps/backend/internal/github/branch_protection_test.go @@ -0,0 +1,138 @@ +package github + +import ( + "context" + "errors" + "sync/atomic" + "testing" + "testing/synctest" + "time" +) + +// fakeBPFetcher is a minimal BranchProtectionFetcher for testing the +// service-level cache wiring. It records call counts and returns canned +// values per (owner, repo, branch) key. +type fakeBPFetcher struct { + calls atomic.Int64 + bp BranchProtection + err error +} + +func (f *fakeBPFetcher) FetchBranchProtection(_ context.Context, _, _, _ string) (BranchProtection, error) { + f.calls.Add(1) + return f.bp, f.err +} + +// minimal Client surface — only the bits fetchRequiredReviews needs (it +// asserts the client implements BranchProtectionFetcher; everything else +// can be nil-valued). +type fakeBPClient struct { + *fakeBPFetcher + Client +} + +func newServiceWithBPFetcher(f *fakeBPFetcher) *Service { + return &Service{ + client: fakeBPClient{fakeBPFetcher: f}, + protectionCache: newBranchProtectionCache(), + } +} + +func TestBranchProtectionCache_GetSet_TTL(t *testing.T) { + // synctest.Test advances time in virtual ticks so a 60ms sleep is + // instantaneous wall-clock — keeps the suite fast and deterministic + // (no real sleeps that flake on heavily loaded CI runners). + synctest.Test(t, func(t *testing.T) { + c := newBranchProtectionCache() + c.ttl = 50 * time.Millisecond + key := "o/r@main" + c.set(key, BranchProtection{HasRule: true, RequiredApprovingReviewCount: 2, FetchedAt: time.Now()}) + got, ok := c.get(key) + if !ok || got.RequiredApprovingReviewCount != 2 { + t.Fatalf("fresh entry: got=%+v ok=%v", got, ok) + } + time.Sleep(60 * time.Millisecond) + if _, ok := c.get(key); ok { + t.Fatalf("stale entry should not be returned") + } + }) +} + +func TestBranchProtectionCache_StaleEntryIsEvicted(t *testing.T) { + c := newBranchProtectionCache() + c.ttl = 1 * time.Millisecond + c.set("o/r@main", BranchProtection{HasRule: true, FetchedAt: time.Now().Add(-time.Hour)}) + if _, ok := c.get("o/r@main"); ok { + t.Fatalf("expected stale miss") + } + c.mu.RLock() + _, present := c.m["o/r@main"] + c.mu.RUnlock() + if present { + t.Fatalf("stale entry should have been deleted from map") + } +} + +func TestFetchRequiredReviews_CacheHit(t *testing.T) { + f := &fakeBPFetcher{bp: BranchProtection{HasRule: true, RequiredApprovingReviewCount: 3}} + s := newServiceWithBPFetcher(f) + if got := s.fetchRequiredReviews(context.Background(), "o", "r", "main"); got == nil || *got != 3 { + t.Fatalf("first call: %v", got) + } + if got := s.fetchRequiredReviews(context.Background(), "o", "r", "main"); got == nil || *got != 3 { + t.Fatalf("cached call: %v", got) + } + if calls := f.calls.Load(); calls != 1 { + t.Fatalf("fetcher called %d times, want 1 (cache miss + hit)", calls) + } +} + +func TestFetchRequiredReviews_NoRuleIsCached(t *testing.T) { + f := &fakeBPFetcher{bp: BranchProtection{HasRule: false}} + s := newServiceWithBPFetcher(f) + if got := s.fetchRequiredReviews(context.Background(), "o", "r", "main"); got != nil { + t.Fatalf("expected nil for no-rule, got %v", got) + } + // Second call must not hit the network — the negative result is cached + // so we don't re-query upstream every poll. + _ = s.fetchRequiredReviews(context.Background(), "o", "r", "main") + if calls := f.calls.Load(); calls != 1 { + t.Fatalf("fetcher called %d times, want 1", calls) + } +} + +func TestFetchRequiredReviews_ErrorIsNotCached(t *testing.T) { + f := &fakeBPFetcher{err: errors.New("boom")} + s := newServiceWithBPFetcher(f) + if got := s.fetchRequiredReviews(context.Background(), "o", "r", "main"); got != nil { + t.Fatalf("expected nil on error, got %v", got) + } + if got := s.fetchRequiredReviews(context.Background(), "o", "r", "main"); got != nil { + t.Fatalf("expected nil on retry, got %v", got) + } + if calls := f.calls.Load(); calls != 2 { + t.Fatalf("fetcher called %d times, want 2 (error not cached)", calls) + } +} + +func TestFetchRequiredReviews_ClientWithoutFetcherReturnsNil(t *testing.T) { + // noop client doesn't satisfy BranchProtectionFetcher. + s := &Service{client: nil, protectionCache: newBranchProtectionCache()} + if got := s.fetchRequiredReviews(context.Background(), "o", "r", "main"); got != nil { + t.Fatalf("expected nil when client lacks BranchProtectionFetcher, got %v", got) + } +} + +func TestFetchRequiredReviews_TTLRefetch(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + f := &fakeBPFetcher{bp: BranchProtection{HasRule: true, RequiredApprovingReviewCount: 1}} + s := newServiceWithBPFetcher(f) + s.protectionCache.ttl = 50 * time.Millisecond + _ = s.fetchRequiredReviews(context.Background(), "o", "r", "main") + time.Sleep(60 * time.Millisecond) + _ = s.fetchRequiredReviews(context.Background(), "o", "r", "main") + if calls := f.calls.Load(); calls != 2 { + t.Fatalf("fetcher called %d times, want 2 (refetch after TTL)", calls) + } + }) +} diff --git a/apps/backend/internal/github/client_helpers.go b/apps/backend/internal/github/client_helpers.go index ff16c2292e..7df629414a 100644 --- a/apps/backend/internal/github/client_helpers.go +++ b/apps/backend/internal/github/client_helpers.go @@ -91,17 +91,103 @@ func getPRStatus(ctx context.Context, c Client, owner, repo string, number int) reviewState, pendingReviewCount := deriveReviewSyncState(pr, reviews) total, passing := countCheckResults(checks) return &PRStatus{ - PR: pr, - ReviewState: reviewState, - ChecksState: computeOverallCheckStatus(checks), - MergeableState: pr.MergeableState, - ReviewCount: len(reviews), + PR: pr, + ReviewState: reviewState, + ChecksState: computeOverallCheckStatus(checks), + MergeableState: pr.MergeableState, + // ReviewCount is the number of distinct reviewers whose latest review + // state is APPROVED — it's the value the popover renders as + // "Approved (N)" / "Approved N / M required". Counting raw review + // entries (`len(reviews)`) double-counts authors who left multiple + // reviews and also includes COMMENTED / CHANGES_REQUESTED rows. + ReviewCount: countApprovedReviewers(reviews), PendingReviewCount: pendingReviewCount, ChecksTotal: total, ChecksPassing: passing, + // REST path actually counted check runs + reviews — distinguish + // from the batched-GraphQL path that only carries rollup state. + ChecksPopulated: true, + ReviewCountsPopulated: true, }, nil } +// reviewSample is a normalized review row consumed by latestReviewStateByAuthor. +// Adapter functions below convert PRReview (REST) and reviewNode (GraphQL) +// to this shape so the per-reviewer dedup rule is only spelled out once. +type reviewSample struct { + author string + state string + at time.Time +} + +// latestReviewStateByAuthor reduces a list of reviews to "one entry per +// reviewer, latest binding state wins". COMMENTED and PENDING never +// override a prior APPROVED / CHANGES_REQUESTED for the same author so a +// reviewer who approved-then-commented still counts as approved. +// Anonymous reviewers (deleted users) get a unique synthetic key so each +// review still counts independently. +func latestReviewStateByAuthor(samples []reviewSample) map[string]reviewSample { + byAuthor := make(map[string]reviewSample, len(samples)) + for _, s := range samples { + key := s.author + if key == "" { + key = ":" + s.at.UTC().Format(time.RFC3339Nano) + } + state := strings.ToUpper(s.state) + if state != reviewStateApproved && state != reviewStateChangesRequested { + if _, ok := byAuthor[key]; ok { + continue + } + } + prev, ok := byAuthor[key] + if !ok || !s.at.Before(prev.at) { + byAuthor[key] = reviewSample{author: s.author, state: state, at: s.at} + } + } + return byAuthor +} + +// countApprovedAuthors counts distinct reviewers whose latest binding state is +// APPROVED. Shared between the REST and GraphQL count helpers. +func countApprovedAuthors(samples []reviewSample) int { + count := 0 + for _, s := range latestReviewStateByAuthor(samples) { + if s.state == reviewStateApproved { + count++ + } + } + return count +} + +// reduceReviewSummary collapses the per-author map into a single overall +// review state. CHANGES_REQUESTED wins across authors; APPROVED otherwise; +// "" when nothing is binding. Shared between REST and GraphQL paths. +func reduceReviewSummary(samples []reviewSample) string { + hasApproval := false + for _, l := range latestReviewStateByAuthor(samples) { + switch l.state { + case reviewStateChangesRequested: + return computedReviewStateChangesRequested + case reviewStateApproved: + hasApproval = true + } + } + if hasApproval { + return computedReviewStateApproved + } + return "" +} + +// countApprovedReviewers returns the number of distinct authors whose latest +// review state is APPROVED. REST-path adapter over the shared helper. +func countApprovedReviewers(reviews []PRReview) int { + samples := make([]reviewSample, len(reviews)) + for i, r := range reviews { + samples[i] = reviewSample{author: r.Author, state: r.State, at: r.CreatedAt} + } + return countApprovedAuthors(samples) +} + // countCheckResults returns (total, passing) counts using the same logic as // computeOverallCheckStatus: skipped/neutral and still-running checks are // excluded from the total so the X/Y badge communicates pass rate rather diff --git a/apps/backend/internal/github/client_helpers_test.go b/apps/backend/internal/github/client_helpers_test.go index 6500215993..5a869c4d92 100644 --- a/apps/backend/internal/github/client_helpers_test.go +++ b/apps/backend/internal/github/client_helpers_test.go @@ -471,3 +471,123 @@ func TestHasChangesRequested(t *testing.T) { }) } } + +// --- Reviewer dedup helpers (countApprovedReviewers + reduceReviewSummary) --- + +func TestCountApprovedReviewers(t *testing.T) { + t0 := time.Date(2025, 5, 1, 12, 0, 0, 0, time.UTC) + mk := func(author, state string, offset time.Duration) PRReview { + return PRReview{Author: author, State: state, CreatedAt: t0.Add(offset)} + } + cases := []struct { + name string + reviews []PRReview + want int + }{ + {name: "empty", reviews: []PRReview{}, want: 0}, + { + name: "single approval", + reviews: []PRReview{mk("alice", reviewStateApproved, 0)}, + want: 1, + }, + { + name: "dedup same author latest wins", + reviews: []PRReview{ + mk("alice", reviewStateChangesRequested, 0), + mk("alice", reviewStateApproved, time.Hour), + }, + want: 1, + }, + { + name: "COMMENTED does not replace prior APPROVED", + reviews: []PRReview{ + mk("alice", reviewStateApproved, 0), + mk("alice", "COMMENTED", time.Hour), + }, + want: 1, + }, + { + name: "PENDING never counts even alone", + reviews: []PRReview{mk("alice", "PENDING", 0)}, + want: 0, + }, + { + name: "two distinct approvers", + reviews: []PRReview{ + mk("alice", reviewStateApproved, 0), + mk("bob", reviewStateApproved, time.Hour), + }, + want: 2, + }, + { + name: "approve then change-request reverts", + reviews: []PRReview{ + mk("alice", reviewStateApproved, 0), + mk("alice", reviewStateChangesRequested, time.Hour), + }, + want: 0, + }, + { + name: "anonymous reviewers counted separately", + reviews: []PRReview{ + mk("", reviewStateApproved, 0), + mk("", reviewStateApproved, time.Hour), + }, + want: 2, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := countApprovedReviewers(tc.reviews); got != tc.want { + t.Fatalf("countApprovedReviewers = %d, want %d", got, tc.want) + } + }) + } +} + +func TestReduceReviewSummary(t *testing.T) { + t0 := time.Date(2025, 5, 1, 12, 0, 0, 0, time.UTC) + mk := func(author, state string, offset time.Duration) reviewSample { + return reviewSample{author: author, state: state, at: t0.Add(offset)} + } + cases := []struct { + name string + in []reviewSample + want string + }{ + {name: "empty", in: nil, want: ""}, + { + name: "single approval", + in: []reviewSample{mk("a", reviewStateApproved, 0)}, + want: computedReviewStateApproved, + }, + { + name: "changes requested wins across authors", + in: []reviewSample{ + mk("a", reviewStateApproved, 0), + mk("b", reviewStateChangesRequested, time.Hour), + }, + want: computedReviewStateChangesRequested, + }, + { + name: "later approval supersedes earlier changes-requested from same author", + in: []reviewSample{ + mk("a", reviewStateChangesRequested, 0), + mk("a", reviewStateApproved, time.Hour), + }, + want: computedReviewStateApproved, + }, + { + name: "only comments returns empty", + in: []reviewSample{mk("a", "COMMENTED", 0)}, + want: "", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := reduceReviewSummary(tc.in); got != tc.want { + t.Fatalf("reduceReviewSummary = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/apps/backend/internal/github/gh_client.go b/apps/backend/internal/github/gh_client.go index 73c9ba7ebb..17ac5156b7 100644 --- a/apps/backend/internal/github/gh_client.go +++ b/apps/backend/internal/github/gh_client.go @@ -505,6 +505,65 @@ func (c *GHClient) GetPRStatus(ctx context.Context, owner, repo string, number i return getPRStatus(ctx, c, owner, repo, number) } +// FetchBranchProtection looks up branch protection via `gh api`. A 404 from +// `gh` (no rule) and a 403 (token lacks Administration: Read scope) are both +// treated as "no rule we can see" so callers cache the negative result and +// don't burn rate-limit quota on every poll. The string match is permissive +// across formats so a future `gh` CLI release that tweaks the wording +// doesn't silently break the cache. +func (c *GHClient) FetchBranchProtection(ctx context.Context, owner, repo, branch string) (BranchProtection, error) { + out, err := c.run(ctx, "api", + fmt.Sprintf("repos/%s/%s/branches/%s/protection", owner, repo, branch)) + if err != nil { + if isNotFoundErr(err) || isForbiddenErr(err) { + return BranchProtection{HasRule: false}, nil + } + return BranchProtection{}, err + } + var raw struct { + RequiredPullRequestReviews *struct { + RequiredApprovingReviewCount int `json:"required_approving_review_count"` + } `json:"required_pull_request_reviews"` + } + if err := json.Unmarshal([]byte(out), &raw); err != nil { + return BranchProtection{}, fmt.Errorf("decode branch protection: %w", err) + } + if raw.RequiredPullRequestReviews == nil { + return BranchProtection{HasRule: true}, nil + } + return BranchProtection{ + HasRule: true, + RequiredApprovingReviewCount: raw.RequiredPullRequestReviews.RequiredApprovingReviewCount, + }, nil +} + +// isNotFoundErr matches the formats the `gh` CLI uses to report a 404 across +// recent versions: "HTTP 404", "HTTP 404: ...", and "404 Not Found". +// Restrictive enough that an unrelated 404 substring (e.g. inside a JSON +// payload accidentally rendered into stderr) is unlikely to false-match. +func isNotFoundErr(err error) bool { + if err == nil { + return false + } + s := err.Error() + return strings.Contains(s, "HTTP 404") || + strings.Contains(s, "404 Not Found") || + strings.Contains(s, "status: 404") +} + +// isForbiddenErr matches the formats the `gh` CLI uses to report a 403, used +// by the branch-protection lookup to silently fall back to "no rule" when +// the token lacks Administration: Read scope. +func isForbiddenErr(err error) bool { + if err == nil { + return false + } + s := err.Error() + return strings.Contains(s, "HTTP 403") || + strings.Contains(s, "403 Forbidden") || + strings.Contains(s, "status: 403") +} + func (c *GHClient) ListPRFiles(ctx context.Context, owner, repo string, number int) ([]PRFile, error) { out, err := c.run(ctx, "api", fmt.Sprintf("repos/%s/%s/pulls/%d/files", owner, repo, number), diff --git a/apps/backend/internal/github/gh_client_test.go b/apps/backend/internal/github/gh_client_test.go index dac69d689b..4836ecc459 100644 --- a/apps/backend/internal/github/gh_client_test.go +++ b/apps/backend/internal/github/gh_client_test.go @@ -1,10 +1,57 @@ package github import ( + "errors" "testing" "time" ) +func TestIsNotFoundErr(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"HTTP 404", errors.New("gh api: HTTP 404"), true}, + {"HTTP 404 with suffix", errors.New("gh: HTTP 404: Not Found (404)"), true}, + {"404 Not Found", errors.New("404 Not Found"), true}, + {"status: 404", errors.New("request failed (status: 404)"), true}, + {"unrelated 500", errors.New("HTTP 500: server error"), false}, + {"unrelated text", errors.New("connection refused"), false}, + {"403 not 404", errors.New("HTTP 403: Forbidden"), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isNotFoundErr(tc.err); got != tc.want { + t.Fatalf("isNotFoundErr(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + +func TestIsForbiddenErr(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"HTTP 403", errors.New("gh api: HTTP 403"), true}, + {"403 Forbidden", errors.New("403 Forbidden"), true}, + {"status: 403", errors.New("request failed (status: 403)"), true}, + {"404 not 403", errors.New("HTTP 404"), false}, + {"500 not 403", errors.New("HTTP 500"), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isForbiddenErr(tc.err); got != tc.want { + t.Fatalf("isForbiddenErr(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + func TestParseTimePtr(t *testing.T) { tests := []struct { name string diff --git a/apps/backend/internal/github/graphql.go b/apps/backend/internal/github/graphql.go index 5cd5fe4e19..a4fed0dc1e 100644 --- a/apps/backend/internal/github/graphql.go +++ b/apps/backend/internal/github/graphql.go @@ -94,6 +94,12 @@ type batchedPRResult struct { ReviewRequests struct { TotalCount int `json:"totalCount"` } `json:"reviewRequests"` + ReviewThreads struct { + TotalCount int `json:"totalCount"` + Nodes []struct { + IsResolved bool `json:"isResolved"` + } `json:"nodes"` + } `json:"reviewThreads"` Commits struct { Nodes []struct { Commit struct { @@ -184,6 +190,7 @@ func prFieldsBlock() string { `author { login } createdAt updatedAt mergedAt closedAt ` + `reviews(last: 100) { nodes { state author { login } submittedAt } } ` + `reviewRequests(first: 0) { totalCount } ` + + `reviewThreads(first: 100) { totalCount nodes { isResolved } } ` + `commits(last: 1) { nodes { commit { statusCheckRollup { state } } } }` } @@ -236,59 +243,61 @@ func convertBatchedPRResult(raw *batchedPRResult, owner, repo string, number int if len(raw.Commits.Nodes) > 0 && raw.Commits.Nodes[0].Commit.StatusCheckRollup != nil { checksState = strings.ToLower(raw.Commits.Nodes[0].Commit.StatusCheckRollup.State) } + // Count unresolved threads from the fetched nodes. When the page is + // capped (totalCount > nodes), fall back to totalCount for the + // resolved-vs-unresolved estimate so the popover doesn't undercount on + // busy PRs. The fallback is conservative: we have no way to tell from + // the truncated page how many of the un-fetched threads are resolved, + // so attribute the unseen tail to "unresolved" — the popover's value + // is meant to be actionable, not exact, and over-reporting is safer + // than silently hiding open feedback. + unresolved := 0 + for _, t := range raw.ReviewThreads.Nodes { + if !t.IsResolved { + unresolved++ + } + } + if total := raw.ReviewThreads.TotalCount; total > len(raw.ReviewThreads.Nodes) { + unresolved += total - len(raw.ReviewThreads.Nodes) + } return &PRStatus{ - PR: pr, - ReviewState: reviewState, - ChecksState: checksState, - MergeableState: pr.MergeableState, + PR: pr, + ReviewState: reviewState, + ChecksState: checksState, + MergeableState: pr.MergeableState, + ReviewCount: countApprovedReviewerNodes(raw.Reviews.Nodes), + PendingReviewCount: raw.ReviewRequests.TotalCount, + ReviewCountsPopulated: true, + UnresolvedReviewThreads: unresolved, + UnresolvedReviewThreadsPopulated: true, } } +// reviewNodesToSamples converts the GraphQL reviewNode shape to the shared +// reviewSample slice used by the dedup helpers in client_helpers.go. +func reviewNodesToSamples(nodes []reviewNode) []reviewSample { + samples := make([]reviewSample, len(nodes)) + for i, n := range nodes { + samples[i] = reviewSample{author: n.Author.Login, state: n.State, at: n.SubmittedAt} + } + return samples +} + +// countApprovedReviewerNodes returns the number of distinct authors whose +// latest review state is APPROVED, on the GraphQL reviewNode shape. Thin +// adapter over countApprovedAuthors so REST and GraphQL agree on what +// "Approved (N)" means. +func countApprovedReviewerNodes(nodes []reviewNode) int { + return countApprovedAuthors(reviewNodesToSamples(nodes)) +} + // summarizeReviewState collapses the review history to a single -// "approved"/"changes_requested"/"" value matching the existing -// service-side summary. Per-reviewer dedup: each reviewer's most-recent -// review wins, so a CHANGES_REQUESTED followed by an APPROVED from the -// same author resolves to APPROVED. CHANGES_REQUESTED beats APPROVED across -// distinct reviewers. +// "approved"/"changes_requested"/"" value. Per-reviewer dedup: each +// reviewer's most-recent binding review wins, so a CHANGES_REQUESTED +// followed by APPROVED from the same author resolves to APPROVED. +// CHANGES_REQUESTED beats APPROVED across distinct reviewers. func summarizeReviewState(nodes []reviewNode) string { - type latest struct { - state string - at time.Time - } - byAuthor := map[string]latest{} - for _, n := range nodes { - // Anonymous reviewers (deleted users) get a unique bucket so each - // review still counts independently. - key := n.Author.Login - if key == "" { - key = ":" + n.SubmittedAt.UTC().Format(time.RFC3339Nano) - } - state := strings.ToUpper(n.State) - // COMMENTED and PENDING are non-binding states and shouldn't replace - // a prior APPROVED / CHANGES_REQUESTED for the same reviewer. - if state != reviewStateApproved && state != reviewStateChangesRequested { - if _, ok := byAuthor[key]; ok { - continue - } - } - prev, ok := byAuthor[key] - if !ok || !n.SubmittedAt.Before(prev.at) { - byAuthor[key] = latest{state: state, at: n.SubmittedAt} - } - } - hasApproval := false - for _, l := range byAuthor { - switch l.state { - case reviewStateChangesRequested: - return computedReviewStateChangesRequested - case reviewStateApproved: - hasApproval = true - } - } - if hasApproval { - return computedReviewStateApproved - } - return "" + return reduceReviewSummary(reviewNodesToSamples(nodes)) } // PATClient.ExecuteGraphQL satisfies GraphQLExecutor by POSTing to /graphql. diff --git a/apps/backend/internal/github/mock_client.go b/apps/backend/internal/github/mock_client.go index 6c1a707341..74db6dcbf6 100644 --- a/apps/backend/internal/github/mock_client.go +++ b/apps/backend/internal/github/mock_client.go @@ -51,6 +51,8 @@ type repoKey struct { type MockClient struct { mu sync.RWMutex user string + authenticated bool + authError string prs map[prKey]*PR prsByBranch map[branchKey]*PR orgs []GitHubOrg @@ -67,23 +69,26 @@ type MockClient struct { // NewMockClient creates a new MockClient with default values. func NewMockClient() *MockClient { return &MockClient{ - user: mockDefaultUser, - prs: make(map[prKey]*PR), - prsByBranch: make(map[branchKey]*PR), - repos: make(map[string][]GitHubRepo), - branches: make(map[repoKey][]RepoBranch), - reviews: make(map[prKey][]PRReview), - comments: make(map[prKey][]PRComment), - checks: make(map[checkKey][]CheckRun), - files: make(map[prKey][]PRFile), - commits: make(map[prKey][]PRCommitInfo), + user: mockDefaultUser, + authenticated: true, + prs: make(map[prKey]*PR), + prsByBranch: make(map[branchKey]*PR), + repos: make(map[string][]GitHubRepo), + branches: make(map[repoKey][]RepoBranch), + reviews: make(map[prKey][]PRReview), + comments: make(map[prKey][]PRComment), + checks: make(map[checkKey][]CheckRun), + files: make(map[prKey][]PRFile), + commits: make(map[prKey][]PRCommitInfo), } } // --- Client interface implementation --- func (m *MockClient) IsAuthenticated(context.Context) (bool, error) { - return true, nil + m.mu.RLock() + defer m.mu.RUnlock() + return m.authenticated, nil } func (m *MockClient) GetAuthenticatedUser(context.Context) (string, error) { @@ -270,6 +275,16 @@ func (m *MockClient) SetUser(username string) { m.user = username } +// SetAuthHealth toggles the authenticated state for the e2e auth-lost test. +// When authenticated=false the popover renders the "Reconnect GitHub" branch. +// authError is an opaque message preserved for diagnostics. +func (m *MockClient) SetAuthHealth(authenticated bool, authError string) { + m.mu.Lock() + defer m.mu.Unlock() + m.authenticated = authenticated + m.authError = authError +} + // AddPR adds a PR to the mock data store, indexed by owner/repo/number and branch. func (m *MockClient) AddPR(pr *PR) { m.mu.Lock() @@ -327,6 +342,34 @@ func (m *MockClient) AddCheckRuns(owner, repo, ref string, checks []CheckRun) { m.checks[k] = append(m.checks[k], checks...) } +// ReplaceCheckRuns overwrites the check runs for a ref. Used by the e2e mock +// controller so a follow-up seed call yields deterministic state. +func (m *MockClient) ReplaceCheckRuns(owner, repo, ref string, checks []CheckRun) { + m.mu.Lock() + defer m.mu.Unlock() + cp := make([]CheckRun, len(checks)) + copy(cp, checks) + m.checks[checkKey{owner, repo, ref}] = cp +} + +// ReplaceReviews overwrites the reviews for a PR. +func (m *MockClient) ReplaceReviews(owner, repo string, number int, reviews []PRReview) { + m.mu.Lock() + defer m.mu.Unlock() + cp := make([]PRReview, len(reviews)) + copy(cp, reviews) + m.reviews[prKey{owner, repo, number}] = cp +} + +// ReplaceComments overwrites the comments for a PR. +func (m *MockClient) ReplaceComments(owner, repo string, number int, comments []PRComment) { + m.mu.Lock() + defer m.mu.Unlock() + cp := make([]PRComment, len(comments)) + copy(cp, comments) + m.comments[prKey{owner, repo, number}] = cp +} + // AddPRFiles appends files for a PR. func (m *MockClient) AddPRFiles(owner, repo string, number int, files []PRFile) { m.mu.Lock() @@ -348,6 +391,8 @@ func (m *MockClient) Reset() { m.mu.Lock() defer m.mu.Unlock() m.user = mockDefaultUser + m.authenticated = true + m.authError = "" m.prs = make(map[prKey]*PR) m.prsByBranch = make(map[branchKey]*PR) m.orgs = nil diff --git a/apps/backend/internal/github/mock_controller.go b/apps/backend/internal/github/mock_controller.go index 18ebb41bfb..eee21bda92 100644 --- a/apps/backend/internal/github/mock_controller.go +++ b/apps/backend/internal/github/mock_controller.go @@ -1,6 +1,8 @@ package github import ( + "context" + "fmt" "net/http" "time" @@ -41,6 +43,8 @@ func (c *MockController) RegisterRoutes(router *gin.Engine) { api.POST("/commits", c.addPRCommits) api.POST("/branches", c.addBranches) api.POST("/task-prs", c.associateTaskPR) + api.POST("/pr-feedback", c.seedPRFeedback) + api.PUT("/auth-health", c.setAuthHealth) api.DELETE("/reset", c.reset) } @@ -188,25 +192,39 @@ func (c *MockController) addBranches(ctx *gin.Context) { ctx.JSON(http.StatusOK, gin.H{"added": len(req.Branches)}) } -// associateTaskPR directly creates a github_task_prs record for E2E testing. +// associateTaskPRRequest is the JSON body for the mock-controller's +// associateTaskPR endpoint. Pointer fields are optional — leave them nil +// to skip the corresponding TaskPR column update. +type associateTaskPRRequest struct { + TaskID string `json:"task_id"` + Owner string `json:"owner"` + Repo string `json:"repo"` + PRNumber int `json:"pr_number"` + PRURL string `json:"pr_url"` + PRTitle string `json:"pr_title"` + HeadBranch string `json:"head_branch"` + BaseBranch string `json:"base_branch"` + AuthorLogin string `json:"author_login"` + State string `json:"state"` + ReviewState string `json:"review_state"` + ChecksState string `json:"checks_state"` + MergeableState string `json:"mergeable_state"` + Additions int `json:"additions"` + Deletions int `json:"deletions"` + ReviewCount *int `json:"review_count,omitempty"` + PendingReviewCount *int `json:"pending_review_count,omitempty"` + RequiredReviews *int `json:"required_reviews,omitempty"` + ChecksTotal *int `json:"checks_total,omitempty"` + ChecksPassing *int `json:"checks_passing,omitempty"` + UnresolvedReviewThreads *int `json:"unresolved_review_threads,omitempty"` +} + +// associateTaskPR directly creates (or replaces) a github_task_prs record for +// E2E testing. Repeating the call with the same (task_id, owner, repo, +// pr_number) updates the row so tests can drive the live-refresh +// scenario by re-POSTing with new aggregate counts. func (c *MockController) associateTaskPR(ctx *gin.Context) { - var req struct { - TaskID string `json:"task_id"` - Owner string `json:"owner"` - Repo string `json:"repo"` - PRNumber int `json:"pr_number"` - PRURL string `json:"pr_url"` - PRTitle string `json:"pr_title"` - HeadBranch string `json:"head_branch"` - BaseBranch string `json:"base_branch"` - AuthorLogin string `json:"author_login"` - State string `json:"state"` - ReviewState string `json:"review_state"` - ChecksState string `json:"checks_state"` - MergeableState string `json:"mergeable_state"` - Additions int `json:"additions"` - Deletions int `json:"deletions"` - } + var req associateTaskPRRequest if err := ctx.ShouldBindJSON(&req); err != nil { ctx.JSON(http.StatusBadRequest, gin.H{"error": "invalid payload"}) return @@ -215,6 +233,26 @@ func (c *MockController) associateTaskPR(ctx *gin.Context) { req.State = defaultPRState } now := time.Now().UTC() + tp := buildTaskPRFromRequest(&req, now) + if err := c.store.ReplaceTaskPR(ctx.Request.Context(), tp); err != nil { + ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + return + } + c.ensureMockPRForRequest(ctx.Request.Context(), &req, now) + // Publish the event so the frontend Zustand store picks up the new PR + // without requiring a page reload — mirrors real AssociatePRWithTask. + if c.eventBus != nil { + event := bus.NewEvent(events.GitHubTaskPRUpdated, "github", tp) + if err := c.eventBus.Publish(ctx.Request.Context(), events.GitHubTaskPRUpdated, event); err != nil { + c.logger.Debug("mock: failed to publish task PR updated event", zap.Error(err)) + } + } + ctx.JSON(http.StatusCreated, tp) +} + +// buildTaskPRFromRequest copies the required fields from the JSON body into +// a TaskPR and applies the optional pointer fields when present. +func buildTaskPRFromRequest(req *associateTaskPRRequest, now time.Time) *TaskPR { tp := &TaskPR{ TaskID: req.TaskID, Owner: req.Owner, @@ -233,19 +271,122 @@ func (c *MockController) associateTaskPR(ctx *gin.Context) { Deletions: req.Deletions, CreatedAt: now, } - if err := c.store.CreateTaskPR(ctx.Request.Context(), tp); err != nil { - ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + if req.ReviewCount != nil { + tp.ReviewCount = *req.ReviewCount + } + if req.PendingReviewCount != nil { + tp.PendingReviewCount = *req.PendingReviewCount + } + if req.RequiredReviews != nil { + v := *req.RequiredReviews + tp.RequiredReviews = &v + } + if req.ChecksTotal != nil { + tp.ChecksTotal = *req.ChecksTotal + } + if req.ChecksPassing != nil { + tp.ChecksPassing = *req.ChecksPassing + } + if req.UnresolvedReviewThreads != nil { + tp.UnresolvedReviewThreads = *req.UnresolvedReviewThreads + } + return tp +} + +// ensureMockPRForRequest seeds a synthetic PR in the mock client so the lazy +// PRFeedback path can resolve check runs by HeadSHA. Skipped when the PR was +// already seeded explicitly via addPRs (tests that pin a head_sha for +// matching check_runs must not have it overwritten). +func (c *MockController) ensureMockPRForRequest(ctx context.Context, req *associateTaskPRRequest, now time.Time) { + if existing, _ := c.mock.GetPR(ctx, req.Owner, req.Repo, req.PRNumber); existing != nil { return } - // Publish the event so the frontend Zustand store picks up the new PR - // without requiring a page reload — mirrors real AssociatePRWithTask. - if c.eventBus != nil { - event := bus.NewEvent(events.GitHubTaskPRUpdated, "github", tp) - if err := c.eventBus.Publish(ctx.Request.Context(), events.GitHubTaskPRUpdated, event); err != nil { - c.logger.Debug("mock: failed to publish task PR updated event", zap.Error(err)) - } + c.mock.AddPR(&PR{ + Number: req.PRNumber, + Title: req.PRTitle, + URL: req.PRURL, + HTMLURL: req.PRURL, + State: req.State, + HeadBranch: req.HeadBranch, + HeadSHA: mockHeadSHA(req.Owner, req.Repo, req.PRNumber), + BaseBranch: req.BaseBranch, + AuthorLogin: req.AuthorLogin, + RepoOwner: req.Owner, + RepoName: req.Repo, + Additions: req.Additions, + Deletions: req.Deletions, + CreatedAt: now, + UpdatedAt: now, + }) +} + +// seedPRFeedback registers checks (and optionally reviews / comments) for a +// specific PR. The PR must already exist in the mock client (associateTaskPR +// synthesizes one). The check ref is the PR's HeadSHA so getPRFeedback +// resolves them via ListCheckRuns. +func (c *MockController) seedPRFeedback(ctx *gin.Context) { + var req struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + PRNumber int `json:"pr_number"` + Checks []CheckRun `json:"checks"` + Reviews []PRReview `json:"reviews"` + Comments []PRComment `json:"comments"` } - ctx.JSON(http.StatusCreated, tp) + if err := ctx.ShouldBindJSON(&req); err != nil { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "invalid payload"}) + return + } + if req.Owner == "" || req.Repo == "" || req.PRNumber == 0 { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "owner, repo, pr_number are required"}) + return + } + headSHA := mockHeadSHA(req.Owner, req.Repo, req.PRNumber) + // If associateTaskPR ran first, an underlying PR row exists with this + // HeadSHA. Otherwise synthesize a minimal one so getPRFeedback's GetPR + // call doesn't fail. + if pr, err := c.mock.GetPR(ctx.Request.Context(), req.Owner, req.Repo, req.PRNumber); err != nil || pr == nil { + c.mock.AddPR(&PR{ + Number: req.PRNumber, + RepoOwner: req.Owner, + RepoName: req.Repo, + HeadSHA: headSHA, + CreatedAt: time.Now().UTC(), + UpdatedAt: time.Now().UTC(), + }) + } + // Replace prior seeded checks/reviews/comments so a follow-up call gives + // deterministic state; helpful for tests that drive a "then a check + // finishes" transition. + c.mock.ReplaceCheckRuns(req.Owner, req.Repo, headSHA, req.Checks) + c.mock.ReplaceReviews(req.Owner, req.Repo, req.PRNumber, req.Reviews) + c.mock.ReplaceComments(req.Owner, req.Repo, req.PRNumber, req.Comments) + ctx.JSON(http.StatusOK, gin.H{ + "checks": len(req.Checks), + "reviews": len(req.Reviews), + "comments": len(req.Comments), + }) +} + +// setAuthHealth toggles the mock client's authenticated state so e2e tests +// can drive the "Reconnect GitHub" branch in the popover. +func (c *MockController) setAuthHealth(ctx *gin.Context) { + var req struct { + Authenticated bool `json:"authenticated"` + Error string `json:"error"` + } + if err := ctx.ShouldBindJSON(&req); err != nil { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "invalid payload"}) + return + } + c.mock.SetAuthHealth(req.Authenticated, req.Error) + ctx.JSON(http.StatusOK, gin.H{"authenticated": req.Authenticated}) +} + +// mockHeadSHA produces a deterministic synthetic head SHA for a (owner, repo, +// pr_number) tuple so test seed calls can compute the same key. +func mockHeadSHA(owner, repo string, prNumber int) string { + return fmt.Sprintf("mocksha-%s-%s-%d", owner, repo, prNumber) } func (c *MockController) reset(ctx *gin.Context) { diff --git a/apps/backend/internal/github/models.go b/apps/backend/internal/github/models.go index 433d65606b..5b86740698 100644 --- a/apps/backend/internal/github/models.go +++ b/apps/backend/internal/github/models.go @@ -92,8 +92,29 @@ type PRStatus struct { MergeableState string `json:"mergeable_state"` // "clean", "blocked", "behind", "dirty", "has_hooks", "unstable", "draft", "unknown", "" ReviewCount int `json:"review_count"` PendingReviewCount int `json:"pending_review_count"` + RequiredReviews *int `json:"required_reviews,omitempty"` // nil when no branch protection rule found ChecksTotal int `json:"checks_total"` ChecksPassing int `json:"checks_passing"` + // ChecksPopulated reports whether the sync path actually computed + // ChecksTotal / ChecksPassing. The batched GraphQL poller doesn't (it + // only carries the rollup state), so SyncTaskPR uses this flag to + // decide whether to overwrite the persisted counts. A value of true + // with both counts at 0 is a real "no checks" answer; a value of false + // means "I didn't look, keep what's there." + ChecksPopulated bool `json:"checks_populated,omitempty"` + UnresolvedReviewThreads int `json:"unresolved_review_threads"` + // UnresolvedReviewThreadsPopulated mirrors ChecksPopulated for the + // review-threads field. The REST path (getPRStatus) doesn't fetch + // review threads, so it leaves the field at zero; SyncTaskPR uses + // this flag to avoid clobbering a non-zero value set by the GraphQL + // path during a subsequent REST sync. + UnresolvedReviewThreadsPopulated bool `json:"unresolved_review_threads_populated,omitempty"` + // ReviewCountsPopulated covers ReviewCount + PendingReviewCount. Both + // REST and GraphQL paths compute these now, but historically only the + // REST path did, so without the guard a GraphQL poll would clobber + // the REST value back to 0 (the popover's "Approved (1)" turning + // into "Approved (0)" until a new REST call landed). + ReviewCountsPopulated bool `json:"review_counts_populated,omitempty"` } // PRSearchPage is a paginated slice of PR search results, with the total @@ -137,31 +158,37 @@ type PRWatch struct { // repository this PR belongs to (multi-repo tasks can have one PR per repo). // Empty for legacy rows persisted before multi-repo support. type TaskPR struct { - ID string `json:"id" db:"id"` - TaskID string `json:"task_id" db:"task_id"` - RepositoryID string `json:"repository_id,omitempty" db:"repository_id"` - Owner string `json:"owner" db:"owner"` - Repo string `json:"repo" db:"repo"` - PRNumber int `json:"pr_number" db:"pr_number"` - PRURL string `json:"pr_url" db:"pr_url"` - PRTitle string `json:"pr_title" db:"pr_title"` - HeadBranch string `json:"head_branch" db:"head_branch"` - BaseBranch string `json:"base_branch" db:"base_branch"` - AuthorLogin string `json:"author_login" db:"author_login"` - State string `json:"state" db:"state"` // open, closed, merged - ReviewState string `json:"review_state" db:"review_state"` // approved, changes_requested, pending, "" - ChecksState string `json:"checks_state" db:"checks_state"` // success, failure, pending, "" - MergeableState string `json:"mergeable_state" db:"mergeable_state"` // clean, blocked, behind, dirty, has_hooks, unstable, draft, unknown, "" - ReviewCount int `json:"review_count" db:"review_count"` - PendingReviewCount int `json:"pending_review_count" db:"pending_review_count"` - CommentCount int `json:"comment_count" db:"comment_count"` - Additions int `json:"additions" db:"additions"` - Deletions int `json:"deletions" db:"deletions"` - CreatedAt time.Time `json:"created_at" db:"created_at"` - MergedAt *time.Time `json:"merged_at,omitempty" db:"merged_at"` - ClosedAt *time.Time `json:"closed_at,omitempty" db:"closed_at"` - LastSyncedAt *time.Time `json:"last_synced_at,omitempty" db:"last_synced_at"` - UpdatedAt time.Time `json:"updated_at" db:"updated_at"` + ID string `json:"id" db:"id"` + TaskID string `json:"task_id" db:"task_id"` + RepositoryID string `json:"repository_id,omitempty" db:"repository_id"` + Owner string `json:"owner" db:"owner"` + Repo string `json:"repo" db:"repo"` + PRNumber int `json:"pr_number" db:"pr_number"` + PRURL string `json:"pr_url" db:"pr_url"` + PRTitle string `json:"pr_title" db:"pr_title"` + HeadBranch string `json:"head_branch" db:"head_branch"` + BaseBranch string `json:"base_branch" db:"base_branch"` + AuthorLogin string `json:"author_login" db:"author_login"` + State string `json:"state" db:"state"` // open, closed, merged + ReviewState string `json:"review_state" db:"review_state"` // approved, changes_requested, pending, "" + ChecksState string `json:"checks_state" db:"checks_state"` // success, failure, pending, "" + MergeableState string `json:"mergeable_state" db:"mergeable_state"` // clean, blocked, behind, dirty, has_hooks, unstable, draft, unknown, "" + ReviewCount int `json:"review_count" db:"review_count"` + PendingReviewCount int `json:"pending_review_count" db:"pending_review_count"` + // RequiredReviews is the branch protection's required_approving_review_count. + // Nil when no protection rule exists or the token lacks scope to read it. + RequiredReviews *int `json:"required_reviews,omitempty" db:"required_reviews"` + CommentCount int `json:"comment_count" db:"comment_count"` + UnresolvedReviewThreads int `json:"unresolved_review_threads" db:"unresolved_review_threads"` + ChecksTotal int `json:"checks_total" db:"checks_total"` + ChecksPassing int `json:"checks_passing" db:"checks_passing"` + Additions int `json:"additions" db:"additions"` + Deletions int `json:"deletions" db:"deletions"` + CreatedAt time.Time `json:"created_at" db:"created_at"` + MergedAt *time.Time `json:"merged_at,omitempty" db:"merged_at"` + ClosedAt *time.Time `json:"closed_at,omitempty" db:"closed_at"` + LastSyncedAt *time.Time `json:"last_synced_at,omitempty" db:"last_synced_at"` + UpdatedAt time.Time `json:"updated_at" db:"updated_at"` } // RepoFilter identifies a GitHub repository for review watch filtering. diff --git a/apps/backend/internal/github/pat_client.go b/apps/backend/internal/github/pat_client.go index 5383b21b38..be45ec0738 100644 --- a/apps/backend/internal/github/pat_client.go +++ b/apps/backend/internal/github/pat_client.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -551,6 +552,37 @@ func parseNextLink(header string) string { return "" } +// FetchBranchProtection looks up the branch protection rule on a repo's base +// branch via the REST API. A 404 is treated as "no rule configured" and a 403 +// is treated as "no rule we can see" (token lacks Administration: Read scope). +// Both return HasRule=false with no error so the cache stores the negative +// result and we don't burn rate-limit quota retrying every poll cycle. +// Other errors propagate so callers can decide whether to retry. +func (c *PATClient) FetchBranchProtection(ctx context.Context, owner, repo, branch string) (BranchProtection, error) { + endpoint := fmt.Sprintf("/repos/%s/%s/branches/%s/protection", owner, repo, branch) + var raw struct { + RequiredPullRequestReviews *struct { + RequiredApprovingReviewCount int `json:"required_approving_review_count"` + } `json:"required_pull_request_reviews"` + } + if err := c.get(ctx, endpoint, &raw); err != nil { + var apiErr *GitHubAPIError + if errors.As(err, &apiErr) { + if apiErr.StatusCode == http.StatusNotFound || apiErr.StatusCode == http.StatusForbidden { + return BranchProtection{HasRule: false}, nil + } + } + return BranchProtection{}, err + } + if raw.RequiredPullRequestReviews == nil { + return BranchProtection{HasRule: true}, nil + } + return BranchProtection{ + HasRule: true, + RequiredApprovingReviewCount: raw.RequiredPullRequestReviews.RequiredApprovingReviewCount, + }, nil +} + // patPR is the JSON shape from the GitHub REST API for PRs. type patPR struct { Number int `json:"number"` diff --git a/apps/backend/internal/github/pat_client_test.go b/apps/backend/internal/github/pat_client_test.go index 7902b8aa7a..a9e7605218 100644 --- a/apps/backend/internal/github/pat_client_test.go +++ b/apps/backend/internal/github/pat_client_test.go @@ -297,6 +297,80 @@ func TestPATClient_MarksExhaustedFromRateLimitBody(t *testing.T) { } } +func TestPATClient_FetchBranchProtection(t *testing.T) { + cases := []struct { + name string + status int + body string + wantHasRule bool + wantRequired int + wantErr bool + }{ + { + name: "200 with required reviews", + status: http.StatusOK, + body: `{"required_pull_request_reviews":{"required_approving_review_count":2}}`, + wantHasRule: true, + wantRequired: 2, + }, + { + name: "200 with rule but no required reviews block", + status: http.StatusOK, + body: `{"required_pull_request_reviews":null}`, + wantHasRule: true, + }, + { + name: "404 maps to no rule", + status: http.StatusNotFound, + body: `{"message":"Branch not protected"}`, + wantHasRule: false, + }, + { + name: "403 (no admin scope) maps to no rule", + status: http.StatusForbidden, + body: `{"message":"Resource not accessible by integration"}`, + wantHasRule: false, + }, + { + name: "500 propagates as error", + status: http.StatusInternalServerError, + body: `{"message":"server error"}`, + wantErr: true, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + wantPath := "/repos/o/r/branches/main/protection" + if r.URL.Path != wantPath { + t.Fatalf("path = %q, want %q", r.URL.Path, wantPath) + } + w.WriteHeader(tc.status) + _, _ = w.Write([]byte(tc.body)) + })) + t.Cleanup(srv.Close) + c := newPATClientPointingAt(t, srv.URL) + bp, err := c.FetchBranchProtection(context.Background(), "o", "r", "main") + if tc.wantErr { + if err == nil { + t.Fatal("want error, got nil") + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if bp.HasRule != tc.wantHasRule { + t.Fatalf("HasRule = %v, want %v", bp.HasRule, tc.wantHasRule) + } + if bp.RequiredApprovingReviewCount != tc.wantRequired { + t.Fatalf("RequiredApprovingReviewCount = %d, want %d", + bp.RequiredApprovingReviewCount, tc.wantRequired) + } + }) + } +} + // newPATClientPointingAt builds a PATClient whose underlying HTTP client // reroutes any github API URL to the given test server. func newPATClientPointingAt(t *testing.T, baseURL string) *PATClient { diff --git a/apps/backend/internal/github/service.go b/apps/backend/internal/github/service.go index 47e6a0b041..8a1cd638c9 100644 --- a/apps/backend/internal/github/service.go +++ b/apps/backend/internal/github/service.go @@ -71,21 +71,23 @@ type Service struct { taskEventSubs []bus.Subscription searchCache *ttlCache prStatusCache *ttlCache + protectionCache *branchProtectionCache rateTracker *RateTracker } // NewService creates a new GitHub service. func NewService(client Client, authMethod string, secrets SecretProvider, store *Store, eventBus bus.EventBus, log *logger.Logger) *Service { return &Service{ - client: client, - authMethod: authMethod, - secrets: secrets, - store: store, - eventBus: eventBus, - logger: log, - searchCache: newTTLCache(), - prStatusCache: newTTLCache(), - rateTracker: NewRateTracker(eventBus, log), + client: client, + authMethod: authMethod, + secrets: secrets, + store: store, + eventBus: eventBus, + logger: log, + searchCache: newTTLCache(), + prStatusCache: newTTLCache(), + protectionCache: newBranchProtectionCache(), + rateTracker: NewRateTracker(eventBus, log), } } @@ -753,6 +755,51 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu return err } + // Some sync paths (notably the batched GraphQL poller) don't populate + // ChecksTotal / ChecksPassing — they only carry the rollup state. The + // caller sets status.ChecksPopulated=true when it actually counted + // checks; otherwise we preserve the persisted values so the popover + // doesn't flap to "0/0" between a rich REST sync and a lightweight + // GraphQL one. When the populated counter says 0/0 it really is 0/0 + // (e.g. all workflows were removed from the PR), so we honor it. + nextChecksTotal, nextChecksPassing := tp.ChecksTotal, tp.ChecksPassing + if status.ChecksPopulated { + nextChecksTotal = status.ChecksTotal + nextChecksPassing = status.ChecksPassing + } + // Same Populated/preserve dance for unresolved review threads — the + // REST path doesn't fetch them, so blindly writing status.UnresolvedReviewThreads + // would clobber the non-zero value set by the GraphQL path on every poll. + nextUnresolved := tp.UnresolvedReviewThreads + if status.UnresolvedReviewThreadsPopulated { + nextUnresolved = status.UnresolvedReviewThreads + } + // Review counts: only overwrite when the caller actually computed them. + // Both REST and GraphQL paths now populate these, but a partial sync + // path that doesn't would otherwise reset the popover's "Approved (N)" + // to zero. + nextReviewCount, nextPendingReviewCount := tp.ReviewCount, tp.PendingReviewCount + if status.ReviewCountsPopulated { + nextReviewCount = status.ReviewCount + nextPendingReviewCount = status.PendingReviewCount + } + // PRs can be retargeted to a different base branch; pick up the new + // branch from status.PR before resolving branch-protection so we don't + // indefinitely surface the wrong rule. + nextBaseBranch := tp.BaseBranch + if status.PR.BaseBranch != "" && status.PR.BaseBranch != tp.BaseBranch { + nextBaseBranch = status.PR.BaseBranch + } + // RequiredReviews comes from branch protection, fetched separately. + // Treat nil as "unknown — don't touch"; only write when the caller has it + // or our cache resolves the rule for this base branch. + nextRequiredReviews := tp.RequiredReviews + if status.RequiredReviews != nil { + nextRequiredReviews = status.RequiredReviews + } else if fetched := s.fetchRequiredReviews(ctx, tp.Owner, tp.Repo, nextBaseBranch); fetched != nil { + nextRequiredReviews = fetched + } + changed := tp.State != status.PR.State || tp.PRTitle != status.PR.Title || tp.Additions != status.PR.Additions || @@ -760,8 +807,13 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu tp.ReviewState != status.ReviewState || tp.ChecksState != status.ChecksState || tp.MergeableState != status.MergeableState || - tp.ReviewCount != status.ReviewCount || - tp.PendingReviewCount != status.PendingReviewCount || + tp.ReviewCount != nextReviewCount || + tp.PendingReviewCount != nextPendingReviewCount || + !intPtrEqual(tp.RequiredReviews, nextRequiredReviews) || + tp.ChecksTotal != nextChecksTotal || + tp.ChecksPassing != nextChecksPassing || + tp.UnresolvedReviewThreads != nextUnresolved || + tp.BaseBranch != nextBaseBranch || !timeEqual(tp.MergedAt, status.PR.MergedAt) || !timeEqual(tp.ClosedAt, status.PR.ClosedAt) @@ -774,8 +826,13 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu tp.ReviewState = status.ReviewState tp.ChecksState = status.ChecksState tp.MergeableState = status.MergeableState - tp.ReviewCount = status.ReviewCount - tp.PendingReviewCount = status.PendingReviewCount + tp.ReviewCount = nextReviewCount + tp.PendingReviewCount = nextPendingReviewCount + tp.RequiredReviews = nextRequiredReviews + tp.ChecksTotal = nextChecksTotal + tp.ChecksPassing = nextChecksPassing + tp.UnresolvedReviewThreads = nextUnresolved + tp.BaseBranch = nextBaseBranch // CommentCount is no longer updated from polling -- only refreshed on-demand now := time.Now().UTC() tp.LastSyncedAt = &now @@ -804,6 +861,17 @@ func timeEqual(a, b *time.Time) bool { return a.Equal(*b) } +// intPtrEqual compares two nullable int pointers for equality. +func intPtrEqual(a, b *int) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + return *a == *b +} + // --- PR info and feedback (live) --- // GetPR fetches basic PR details from GitHub. diff --git a/apps/backend/internal/github/service_test.go b/apps/backend/internal/github/service_test.go index 4cc69bf4e3..6f32e8e078 100644 --- a/apps/backend/internal/github/service_test.go +++ b/apps/backend/internal/github/service_test.go @@ -910,6 +910,253 @@ func TestSyncTaskPR_PublishesEventOnMergeableStateChange(t *testing.T) { } } +// TestSyncTaskPR_ChecksPopulated_PreservesOnLightweightSync verifies the +// "preserve when batch sync didn't populate" path: a PRStatus with +// ChecksPopulated=false must NOT clobber the persisted ChecksTotal / +// ChecksPassing, even when status carries 0/0. +func TestSyncTaskPR_ChecksPopulated_PreservesOnLightweightSync(t *testing.T) { + svc, store, _ := 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", + HeadBranch: "feat", + BaseBranch: "main", + State: "open", + ChecksTotal: 10, + ChecksPassing: 7, + }); err != nil { + t.Fatalf("create: %v", err) + } + status := &PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "owner", RepoName: "repo"}, + ChecksTotal: 0, + ChecksPassing: 0, + ChecksPopulated: false, // batched GraphQL path: didn't count + } + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync: %v", err) + } + stored, err := store.GetTaskPR(ctx, "t1") + if err != nil { + t.Fatalf("get: %v", err) + } + if stored.ChecksTotal != 10 || stored.ChecksPassing != 7 { + t.Fatalf("counts clobbered: total=%d passing=%d (want 10/7)", stored.ChecksTotal, stored.ChecksPassing) + } +} + +// TestSyncTaskPR_ChecksPopulated_AcceptsRealZero verifies the design fix: +// when the rich sync path explicitly reports 0/0 (workflows removed from +// the PR), we DO honor the new value instead of clinging to stale totals. +func TestSyncTaskPR_ChecksPopulated_AcceptsRealZero(t *testing.T) { + svc, store, _ := setupSyncTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open", + ChecksTotal: 5, ChecksPassing: 5, + }); err != nil { + t.Fatalf("create: %v", err) + } + status := &PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "o", RepoName: "r"}, + ChecksTotal: 0, + ChecksPassing: 0, + ChecksPopulated: true, // REST path: counted to zero + } + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync: %v", err) + } + stored, _ := store.GetTaskPR(ctx, "t1") + if stored.ChecksTotal != 0 || stored.ChecksPassing != 0 { + t.Fatalf("real-zero not honored: total=%d passing=%d", stored.ChecksTotal, stored.ChecksPassing) + } +} + +// TestSyncTaskPR_ReviewCounts_PreservesOnLightweightSync covers the same +// preserve-on-not-populated dance for ReviewCount/PendingReviewCount as the +// checks/threads variants. A partial sync path (status.ReviewCountsPopulated +// = false) must not reset the popover's "Approved (N)" line to zero. +func TestSyncTaskPR_ReviewCounts_PreservesOnLightweightSync(t *testing.T) { + svc, store, eb := setupSyncTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open", + ReviewCount: 2, PendingReviewCount: 1, + }); err != nil { + t.Fatalf("create: %v", err) + } + status := &PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "o", RepoName: "r"}, + ReviewCount: 0, + PendingReviewCount: 0, + ReviewCountsPopulated: false, + } + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync: %v", err) + } + stored, _ := store.GetTaskPR(ctx, "t1") + if stored.ReviewCount != 2 || stored.PendingReviewCount != 1 { + t.Fatalf("review counts clobbered: review=%d pending=%d (want 2/1)", + stored.ReviewCount, stored.PendingReviewCount) + } + if got := eb.publishedCount(); got != 0 { + t.Fatalf("expected no event when nothing changed, got %d", got) + } +} + +// TestSyncTaskPR_ReviewCounts_AcceptsRealZero is the symmetric case: when the +// caller actually counted reviewers and reports zero, we honor it (e.g. all +// approvals dismissed) instead of clinging to the previous non-zero values. +func TestSyncTaskPR_ReviewCounts_AcceptsRealZero(t *testing.T) { + svc, store, _ := setupSyncTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open", + ReviewCount: 2, PendingReviewCount: 1, + }); err != nil { + t.Fatalf("create: %v", err) + } + status := &PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "o", RepoName: "r"}, + ReviewCount: 0, + PendingReviewCount: 0, + ReviewCountsPopulated: true, + } + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync: %v", err) + } + stored, _ := store.GetTaskPR(ctx, "t1") + if stored.ReviewCount != 0 || stored.PendingReviewCount != 0 { + t.Fatalf("real-zero not honored: review=%d pending=%d", + stored.ReviewCount, stored.PendingReviewCount) + } +} + +// TestSyncTaskPR_UnresolvedThreads_PreservesOnLightweightSync verifies the +// REST single-PR poller (which doesn't fetch review threads) does NOT +// clobber the value set by the GraphQL batched poller. Without the +// UnresolvedReviewThreadsPopulated guard, every REST sync would flip the +// count to 0 and emit a spurious github.task_pr.updated event. +func TestSyncTaskPR_UnresolvedThreads_PreservesOnLightweightSync(t *testing.T) { + svc, store, eb := setupSyncTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open", + UnresolvedReviewThreads: 3, + }); err != nil { + t.Fatalf("create: %v", err) + } + status := &PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "o", RepoName: "r"}, + UnresolvedReviewThreads: 0, + UnresolvedReviewThreadsPopulated: false, // REST path: didn't look + } + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync: %v", err) + } + stored, _ := store.GetTaskPR(ctx, "t1") + if stored.UnresolvedReviewThreads != 3 { + t.Fatalf("threads clobbered: got %d, want 3", stored.UnresolvedReviewThreads) + } + if got := eb.publishedCount(); got != 0 { + t.Fatalf("expected no event when nothing changed, got %d", got) + } +} + +// TestSyncTaskPR_BaseBranchRetarget covers the case where a PR is retargeted +// to a different base branch upstream — the new branch must be persisted so +// future branch-protection lookups key off the right value. +func TestSyncTaskPR_BaseBranchRetarget(t *testing.T) { + svc, store, _ := setupSyncTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open", + }); err != nil { + t.Fatalf("create: %v", err) + } + status := &PRStatus{ + PR: &PR{ + Number: 1, State: "open", RepoOwner: "o", RepoName: "r", + BaseBranch: "develop", + }, + } + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync: %v", err) + } + stored, _ := store.GetTaskPR(ctx, "t1") + if stored.BaseBranch != "develop" { + t.Fatalf("base branch not updated: got %q want develop", stored.BaseBranch) + } +} + +// TestSyncTaskPR_NewFieldsPublishEvent guards that a change to any of the +// CI-popover-relevant fields (UnresolvedReviewThreads, RequiredReviews, +// ChecksTotal, ChecksPassing) triggers a github.task_pr.updated event so the +// frontend popover refreshes via WS. +func TestSyncTaskPR_NewFieldsPublishEvent(t *testing.T) { + required2 := 2 + cases := []struct { + name string + init TaskPR + status PRStatus + }{ + { + name: "unresolved threads change", + init: TaskPR{TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open"}, + status: PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "o", RepoName: "r"}, + UnresolvedReviewThreads: 3, + UnresolvedReviewThreadsPopulated: true, + }, + }, + { + name: "required reviews change", + init: TaskPR{TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open"}, + status: PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "o", RepoName: "r"}, + RequiredReviews: &required2, + }, + }, + { + name: "checks total change", + init: TaskPR{TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open"}, + status: PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "o", RepoName: "r"}, + ChecksTotal: 5, + ChecksPassing: 3, + ChecksPopulated: true, + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + svc, store, eb := setupSyncTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &tc.init); err != nil { + t.Fatalf("create: %v", err) + } + if err := svc.SyncTaskPR(ctx, "t1", &tc.status); err != nil { + t.Fatalf("sync: %v", err) + } + if got := eb.publishedCount(); got != 1 { + t.Fatalf("expected 1 event, got %d", got) + } + }) + } +} + func TestTimeEqual(t *testing.T) { now := time.Now() later := now.Add(time.Second) diff --git a/apps/backend/internal/github/store.go b/apps/backend/internal/github/store.go index 151de13bfe..8991bf4afa 100644 --- a/apps/backend/internal/github/store.go +++ b/apps/backend/internal/github/store.go @@ -74,7 +74,11 @@ const createTablesSQL = ` mergeable_state TEXT NOT NULL DEFAULT '', review_count INTEGER DEFAULT 0, pending_review_count INTEGER DEFAULT 0, + required_reviews INTEGER, comment_count INTEGER DEFAULT 0, + unresolved_review_threads INTEGER DEFAULT 0, + checks_total INTEGER DEFAULT 0, + checks_passing INTEGER DEFAULT 0, additions INTEGER DEFAULT 0, deletions INTEGER DEFAULT 0, created_at DATETIME NOT NULL, @@ -163,6 +167,13 @@ func (s *Store) initSchema() error { // Phase 4 (multi-repo): per-repo PR association on github_task_prs. _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN repository_id TEXT NOT NULL DEFAULT ''`) _, _ = s.db.Exec(`ALTER TABLE github_pr_watches ADD COLUMN repository_id TEXT NOT NULL DEFAULT ''`) + // CI popover: aggregate counts + branch protection's required_approving_review_count + // + unresolved review-threads, surfaced in the PR top-bar hover popover so the + // frontend can render the counts row without a second round-trip. + _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN required_reviews INTEGER`) + _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN unresolved_review_threads INTEGER DEFAULT 0`) + _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN checks_total INTEGER DEFAULT 0`) + _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN checks_passing INTEGER DEFAULT 0`) if err := s.migratePRTablesForMultiRepo(); err != nil { return fmt.Errorf("migrate PR tables for multi-repo: %w", err) } @@ -360,7 +371,11 @@ func (s *Store) migratePRTablesForMultiRepo() error { mergeable_state TEXT NOT NULL DEFAULT '', review_count INTEGER DEFAULT 0, pending_review_count INTEGER DEFAULT 0, + required_reviews INTEGER, comment_count INTEGER DEFAULT 0, + unresolved_review_threads INTEGER DEFAULT 0, + checks_total INTEGER DEFAULT 0, + checks_passing INTEGER DEFAULT 0, additions INTEGER DEFAULT 0, deletions INTEGER DEFAULT 0, created_at DATETIME NOT NULL, @@ -580,11 +595,13 @@ func (s *Store) CreateTaskPR(ctx context.Context, tp *TaskPR) error { tp.UpdatedAt = now _, err := s.db.ExecContext(ctx, ` INSERT INTO github_task_prs (id, task_id, repository_id, owner, repo, pr_number, pr_url, pr_title, head_branch, base_branch, author_login, - state, review_state, checks_state, mergeable_state, review_count, pending_review_count, comment_count, additions, deletions, + state, review_state, checks_state, mergeable_state, review_count, pending_review_count, required_reviews, comment_count, + unresolved_review_threads, checks_total, checks_passing, additions, deletions, created_at, merged_at, closed_at, last_synced_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, tp.ID, tp.TaskID, tp.RepositoryID, tp.Owner, tp.Repo, tp.PRNumber, tp.PRURL, tp.PRTitle, tp.HeadBranch, tp.BaseBranch, tp.AuthorLogin, - tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, tp.ReviewCount, tp.PendingReviewCount, tp.CommentCount, tp.Additions, tp.Deletions, + tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, tp.ReviewCount, tp.PendingReviewCount, tp.RequiredReviews, tp.CommentCount, + tp.UnresolvedReviewThreads, tp.ChecksTotal, tp.ChecksPassing, tp.Additions, tp.Deletions, tp.CreatedAt, tp.MergedAt, tp.ClosedAt, tp.LastSyncedAt, tp.UpdatedAt) return err } @@ -713,11 +730,13 @@ func (s *Store) ReplaceTaskPR(ctx context.Context, tp *TaskPR) error { } if _, err := tx.ExecContext(ctx, ` INSERT INTO github_task_prs (id, task_id, repository_id, owner, repo, pr_number, pr_url, pr_title, head_branch, base_branch, author_login, - state, review_state, checks_state, mergeable_state, review_count, pending_review_count, comment_count, additions, deletions, + state, review_state, checks_state, mergeable_state, review_count, pending_review_count, required_reviews, comment_count, + unresolved_review_threads, checks_total, checks_passing, additions, deletions, created_at, merged_at, closed_at, last_synced_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, tp.ID, tp.TaskID, tp.RepositoryID, tp.Owner, tp.Repo, tp.PRNumber, tp.PRURL, tp.PRTitle, tp.HeadBranch, tp.BaseBranch, tp.AuthorLogin, - tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, tp.ReviewCount, tp.PendingReviewCount, tp.CommentCount, tp.Additions, tp.Deletions, + tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, tp.ReviewCount, tp.PendingReviewCount, tp.RequiredReviews, tp.CommentCount, + tp.UnresolvedReviewThreads, tp.ChecksTotal, tp.ChecksPassing, tp.Additions, tp.Deletions, tp.CreatedAt, tp.MergedAt, tp.ClosedAt, tp.LastSyncedAt, tp.UpdatedAt); err != nil { return err } @@ -729,13 +748,15 @@ func (s *Store) UpdateTaskPR(ctx context.Context, tp *TaskPR) error { tp.UpdatedAt = time.Now().UTC() _, err := s.db.ExecContext(ctx, ` UPDATE github_task_prs SET state = ?, review_state = ?, checks_state = ?, mergeable_state = ?, - review_count = ?, pending_review_count = ?, comment_count = ?, - additions = ?, deletions = ?, pr_title = ?, + review_count = ?, pending_review_count = ?, required_reviews = ?, comment_count = ?, + unresolved_review_threads = ?, checks_total = ?, checks_passing = ?, + additions = ?, deletions = ?, pr_title = ?, base_branch = ?, merged_at = ?, closed_at = ?, last_synced_at = ?, updated_at = ? WHERE id = ?`, tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, - tp.ReviewCount, tp.PendingReviewCount, tp.CommentCount, - tp.Additions, tp.Deletions, tp.PRTitle, + tp.ReviewCount, tp.PendingReviewCount, tp.RequiredReviews, tp.CommentCount, + tp.UnresolvedReviewThreads, tp.ChecksTotal, tp.ChecksPassing, + tp.Additions, tp.Deletions, tp.PRTitle, tp.BaseBranch, tp.MergedAt, tp.ClosedAt, tp.LastSyncedAt, tp.UpdatedAt, tp.ID) return err } diff --git a/apps/web/components/github/pr-ci-popover.tsx b/apps/web/components/github/pr-ci-popover.tsx new file mode 100644 index 0000000000..01c2640776 --- /dev/null +++ b/apps/web/components/github/pr-ci-popover.tsx @@ -0,0 +1,569 @@ +"use client"; + +import { useCallback, useEffect, useMemo, useState, type ReactNode } from "react"; +import { + IconCheck, + IconCircleCheck, + IconCircleDot, + IconCircleX, + IconExternalLink, + IconGitPullRequest, + IconMessageCircle, + IconPlus, +} from "@tabler/icons-react"; +import { Button } from "@kandev/ui/button"; +import { useToast } from "@/components/toast-provider"; +import { useAppStore } from "@/components/state-provider"; +import { useCommentsStore } from "@/lib/state/slices/comments"; +import type { PRFeedbackComment } from "@/lib/state/slices/comments"; +import { useGitHubStatus } from "@/hooks/domains/github/use-github-status"; +import { usePRCIPopover } from "@/hooks/domains/github/use-pr-ci-popover"; +import { + bucketCheck, + bucketCheckCounts, + groupChecksByWorkflow, + type CheckBucket, + type WorkflowGroup, +} from "@/lib/github/check-buckets"; +import type { CheckRun, TaskPR } from "@/lib/types/github"; + +type CountsView = { + passed: number; + inProgress: number; + failed: number; +}; + +const CHECK_GROUP_ORDER: CheckBucket[] = ["passed", "in_progress", "failed"]; + +function countForBucket(counts: CountsView, kind: CheckBucket): number { + if (kind === "passed") return counts.passed; + if (kind === "in_progress") return counts.inProgress; + return counts.failed; +} + +function deriveAggregateCounts(pr: TaskPR): CountsView { + // Pre-load: if checks_total is populated, derive a coarse split using + // checks_state to attribute the "non-passing" remainder. This is good + // enough for the instant render — the lazy PRFeedback fetch refines. + const total = pr.checks_total; + const passing = pr.checks_passing; + const remaining = Math.max(0, total - passing); + if (pr.checks_state === "failure") { + return { passed: passing, failed: remaining, inProgress: 0 }; + } + if (pr.checks_state === "pending") { + return { passed: passing, failed: 0, inProgress: remaining }; + } + if (pr.checks_state === "success") { + return { passed: total, failed: 0, inProgress: 0 }; + } + return { passed: passing, failed: 0, inProgress: remaining }; +} + +function PRCIPopoverHeader({ pr }: { pr: TaskPR }) { + const checksUrl = `${pr.pr_url}/checks`; + return ( +
+ CI status + e.stopPropagation()} + > + + +
+ ); +} + +function CheckGroupIcon({ kind }: { kind: CheckBucket }) { + if (kind === "passed") return ; + if (kind === "in_progress") + return ; + return ; +} + +function bucketLabel(kind: CheckBucket): string { + if (kind === "passed") return "Passed"; + if (kind === "in_progress") return "In progress"; + return "Failed"; +} + +function CheckGroupHeader({ kind, count }: { kind: CheckBucket; count: number }) { + const label = bucketLabel(kind); + return ( +
+
+ + {label} +
+ + {count} + +
+ ); +} + +function PRWorkflowRow({ + group, + onAddAsContext, +}: { + group: WorkflowGroup; + onAddAsContext: ((message: string) => void) | null; +}) { + // For an in-progress workflow, "0/1 ran" reads as "nothing finished" + // and confuses people: did it start? It's clearer to just report how + // many jobs are running. (Failed workflows go to a different bucket, + // so an in_progress group never has failed jobs.) + const badge = + group.bucket === "in_progress" + ? `${group.inProgress} running` + : `${group.passed}/${group.total} passed`; + return ( +
{ + if (group.htmlUrl) window.open(group.htmlUrl, "_blank", "noopener,noreferrer"); + }} + > + + {group.workflow} + + {badge} + {group.htmlUrl && ( + + )} + {group.bucket === "failed" && onAddAsContext && ( + + )} +
+ ); +} + +function buildWorkflowMessage(group: WorkflowGroup): string { + const failed = group.jobs.filter((j) => bucketCheck(j) === "failed"); + const lines: string[] = [ + `### Workflow **${group.workflow}** has ${failed.length} failing job${failed.length !== 1 ? "s" : ""}.`, + "", + ]; + for (const job of failed) { + lines.push(`- **${job.name}** — ${job.conclusion || job.status}`); + if (job.output) lines.push(` ${job.output}`); + if (job.html_url) lines.push(` ${job.html_url}`); + } + lines.push("", "Please investigate and fix."); + return lines.join("\n"); +} + +function PRCheckGroup({ + kind, + count, + workflows, + isLoading, + onAddAsContext, +}: { + kind: CheckBucket; + count: number; + workflows?: WorkflowGroup[]; + isLoading: boolean; + onAddAsContext: ((message: string) => void) | null; +}) { + if (count <= 0 && (!workflows || workflows.length === 0)) return null; + const showRows = kind !== "passed"; + const hasWorkflows = !!workflows && workflows.length > 0; + return ( +
+ + {showRows && ( +
+ {hasWorkflows && + workflows!.map((g) => ( + + ))} + {!hasWorkflows && isLoading + ? Array.from({ length: Math.min(2, count || 1) }).map((_, i) => ( +
+ )) + : null} +
+ )} +
+ ); +} + +/** + * Stacked progress bar mirroring TodoIndicator's bar but split into three + * segments — green (passed), yellow (in_progress), red (failed) — each + * sized proportionally to its count. The leading "pass rate" label gives + * the same at-a-glance number TodoIndicator shows ("6/10 (60%)"). + * + * Skipped/cancelled-but-not-failed jobs aren't counted toward the total + * (they never enter any bucket), so the bar always reflects the actionable + * picture: how close are we to "all green". + */ +function PRChecksProgressBar({ counts }: { counts: CountsView }) { + const total = counts.passed + counts.inProgress + counts.failed; + if (total === 0) return null; + const pct = (n: number) => (n / total) * 100; + const passedPct = pct(counts.passed); + const inProgressPct = pct(counts.inProgress); + const failedPct = pct(counts.failed); + const completePct = Math.round(passedPct); + return ( +
+
+ Pass rate + + {counts.passed}/{total} ({completePct}%) + +
+
+ {passedPct > 0 && ( +
+ )} + {inProgressPct > 0 && ( +
+ )} + {failedPct > 0 && ( +
+ )} +
+
+ ); +} + +function PRChecksSection({ + pr, + feedback, + isFetching, + onAddAsContext, +}: { + pr: TaskPR; + feedback: { checks?: CheckRun[] } | null; + isFetching: boolean; + onAddAsContext: ((message: string) => void) | null; +}) { + const aggregateCounts = useMemo(() => deriveAggregateCounts(pr), [pr]); + + const { precise, byBucket } = useMemo(() => { + // Treat empty `feedback.checks` the same as "feedback not loaded yet" so + // we keep showing the aggregate counts. Some mock paths return empty + // arrays without errors, and we don't want the popover to flash 0/0/0. + if (!feedback?.checks || feedback.checks.length === 0) { + return { precise: null as CountsView | null, byBucket: null }; + } + const counts = bucketCheckCounts(feedback.checks); + const precise: CountsView = { + passed: counts.passed, + inProgress: counts.inProgress, + failed: counts.failed, + }; + const groups = groupChecksByWorkflow(feedback.checks); + const byBucket: Record = { + passed: groups.filter((g) => g.bucket === "passed"), + in_progress: groups.filter((g) => g.bucket === "in_progress"), + failed: groups.filter((g) => g.bucket === "failed"), + }; + return { precise, byBucket }; + }, [feedback]); + + const counts = precise ?? aggregateCounts; + // "No checks at all": the lazy fetch has settled (not isFetching) and there + // are no checks anywhere — neither in PRFeedback nor in the aggregate. + const noChecksAtAll = + !isFetching && pr.checks_total === 0 && (!feedback || (feedback.checks?.length ?? 0) === 0); + if (noChecksAtAll) { + return ( +
+
+ No checks have started +
+
+ ); + } + return ( +
+ + {CHECK_GROUP_ORDER.map((kind) => { + const value = countForBucket(counts, kind); + return ( + + ); + })} +
+ ); +} + +function PRReviewRow({ pr }: { pr: TaskPR }) { + const required = pr.required_reviews ?? null; + const approved = pr.review_count; + const requested = pr.pending_review_count; + + let label: string; + let icon: ReactNode; + if (pr.review_state === "approved") { + icon = ; + label = "Approved"; + } else if (pr.review_state === "changes_requested") { + icon = ; + label = "Changes requested"; + } else { + // Default branch covers "no reviews yet", "review pending", and any + // unknown state. Always rendered (no early-return) so a fresh PR + // still surfaces its review status. + icon = ; + label = "Awaiting review"; + } + // Counts live on the right edge, mirroring the check-group rows. + // Use "K / M" when a required-minimum is known, otherwise just the + // approved count. The "(K requested)" suffix slides in when reviewers + // have been pinged but haven't submitted a review. + let countText: string; + if (required != null) countText = `${approved} / ${required}`; + else countText = `${approved}`; + if (requested > 0) countText += ` · ${requested} requested`; + return ( +
+
+ {icon} + {label} +
+ {countText} +
+ ); +} + +function PRCommentsRow({ pr }: { pr: TaskPR }) { + if (!pr.unresolved_review_threads || pr.unresolved_review_threads <= 0) return null; + return ( +
+ + + {pr.unresolved_review_threads} unresolved comment + {pr.unresolved_review_threads === 1 ? "" : "s"} + +
+ ); +} + +function elapsedLabel(elapsed: number | null): string { + if (elapsed == null) return ""; + if (elapsed === 0) return "updated just now"; + return `updated ${formatElapsedShort(elapsed)} ago`; +} + +function PRPopoverFooter({ lastUpdatedAt }: { lastUpdatedAt: number | null }) { + // Capture a "now" snapshot only inside the setInterval callback (an event + // handler — no impurity in render, no setState during commit). When no + // tick has fired yet, fall back to lastUpdatedAt so the rendered elapsed + // is 0 ("just now"). + const [now, setNow] = useState(null); + useEffect(() => { + if (lastUpdatedAt == null) return; + const id = setInterval(() => setNow(Date.now()), 10_000); + return () => clearInterval(id); + }, [lastUpdatedAt]); + const elapsed = + lastUpdatedAt == null + ? null + : Math.max(0, Math.floor(((now ?? lastUpdatedAt) - lastUpdatedAt) / 1000)); + if (lastUpdatedAt == null) return null; + return ( +
+ + {elapsedLabel(elapsed)} + +
+ ); +} + +function formatElapsedShort(seconds: number): string { + if (seconds < 60) return `${seconds}s`; + const minutes = Math.floor(seconds / 60); + if (minutes < 60) return `${minutes}m`; + const hours = Math.floor(minutes / 60); + return `${hours}h`; +} + +function ReconnectGitHubBlock() { + return ( +
+ GitHub authentication lost. + + Reconnect GitHub + +
+ ); +} + +export function PRCIPopover({ + pr, + enabled, + onOpenDetailPanel, +}: { + pr: TaskPR; + enabled: boolean; + onOpenDetailPanel?: () => void; +}) { + const ghStatus = useAppStore((s) => s.githubStatus.status); + const authLost = ghStatus !== null && !ghStatus.authenticated; + // Trigger an initial status load from the same hook the rest of the app uses. + useGitHubStatus(); + const { feedback, isFetching, lastUpdatedAt } = usePRCIPopover(pr, enabled && !authLost); + const onAddAsContext = useAddCheckToContext(pr); + + return ( +
e.stopPropagation()} + > + + {authLost ? ( + + ) : ( + <> + +
+ + +
+ + )} + {onOpenDetailPanel && ( + + )} + +
+ ); +} + +// --- Add-to-context wiring (mirrors pr-detail-panel.tsx for failed checks) --- +function useAddCheckToContext(pr: TaskPR): ((message: string) => void) | null { + const sessionId = useAppStore((s) => s.tasks.activeSessionId); + const addComment = useCommentsStore((s) => s.addComment); + const { toast } = useToast(); + const prNumber = pr.pr_number; + // Always call useCallback (rules-of-hooks) before bailing out, so the + // returned callback identity is stable across renders unless its inputs + // change. Without memoization the popover would create a new function + // every parent render, defeating PRWorkflowRow's cheap reference equality. + const handler = useCallback( + (message: string) => { + if (!sessionId) return; + const comment: PRFeedbackComment = { + id: `pr-feedback-${Date.now()}-${Math.random().toString(36).slice(2, 7)}`, + sessionId, + text: message, + createdAt: new Date().toISOString(), + status: "pending", + source: "pr-feedback", + prNumber, + feedbackType: "check", + content: message, + }; + addComment(comment); + toast({ description: "Added to chat context" }); + }, + [sessionId, prNumber, addComment, toast], + ); + return sessionId ? handler : null; +} + +// Multi-PR aggregate popover deferred — multi-PR tasks currently keep the +// existing DropdownMenu trigger (no hover popover). The aggregate component +// will land alongside that hover variant when it ships, see follow-up. diff --git a/apps/web/components/github/pr-status-chip.tsx b/apps/web/components/github/pr-status-chip.tsx new file mode 100644 index 0000000000..bf5403b716 --- /dev/null +++ b/apps/web/components/github/pr-status-chip.tsx @@ -0,0 +1,118 @@ +"use client"; + +import { useRef } from "react"; +import { + IconCircleCheckFilled, + IconCircleXFilled, + IconChecklist, + IconLoader2, + IconPointFilled, +} from "@tabler/icons-react"; +import { HoverCard, HoverCardContent, HoverCardTrigger } from "@kandev/ui/hover-card"; +import { useTaskPR } from "@/hooks/domains/github/use-task-pr"; +import { usePRFeedbackBackgroundSync } from "@/hooks/domains/github/use-pr-ci-popover"; +import { PRCIPopover } from "@/components/github/pr-ci-popover"; +import { isPRReadyToMerge } from "@/components/github/pr-task-icon"; +import type { TaskPR } from "@/lib/types/github"; + +const HOVER_OPEN_DELAY_MS = 150; +const HOVER_CLOSE_DELAY_MS = 150; + +type ChipStatus = "passed" | "failed" | "in_progress" | "merged" | "closed" | "neutral"; + +function chipStatus(pr: TaskPR): ChipStatus { + if (pr.state === "merged") return "merged"; + if (pr.state === "closed") return "closed"; + if (pr.review_state === "changes_requested" || pr.checks_state === "failure") return "failed"; + // Pending checks / pending review must beat checks_state === "success" so a + // PR with all checks green but reviewers still outstanding renders as + // in-progress, not passed. Without this order, the chip flips to green the + // moment CI finishes and ignores the human gate. + if (pr.checks_state === "pending" || pr.review_state === "pending") return "in_progress"; + if (pr.checks_state === "success") return "passed"; + return "neutral"; +} + +/** + * Compact CI indicator for the chat status bar — a "CI" prefix icon plus a + * status glyph that mirrors the popover's bucket colors: + * passed → green check + * failed → red X + * in progress → yellow spinner + * merged → purple dot + * neutral → muted dot + * + * Hovering opens the full PRCIPopover anchored to the top edge so the card + * expands upward (the chip sits just above the chat input). + * + * Returns null when the task has no PR yet. + */ +export function PRStatusChip({ taskId }: { taskId: string | null }) { + const { pr } = useTaskPR(taskId); + // Subscribe at the chip level so the cache warms even when the + // top-bar PR button isn't mounted (e.g. small viewport that hides it). + usePRFeedbackBackgroundSync(pr); + if (!pr) return null; + return ; +} + +function PRStatusChipInner({ pr }: { pr: TaskPR }) { + const status = chipStatus(pr); + const triggerRef = useRef(null); + return ( + + + + + { + // Radix HoverCard treats the trigger as outside the content's + // bounding box, so a click on the chip would auto-close the + // popover. Filter out trigger clicks so clicking the chip is + // a no-op while the popover stays open via hover. + if (triggerRef.current && triggerRef.current.contains(e.target as Node)) { + e.preventDefault(); + } + }} + > + + + + ); +} + +function ChipStatusGlyph({ status }: { status: ChipStatus }) { + switch (status) { + case "passed": + return