fix: keep draft PR icons muted (#3424)

2026-09-06 11:36:41 +01:00
parent 188fb09082
commit fed90594d3
7 changed files with 261 additions and 4 deletions
@@ -41,6 +41,20 @@ describe("draft PR task status", () => {
    expect(getPRStatusColor(draftPR())).toBe("text-muted-foreground");
  });

+  // @covers AC-UI-PR-TASK-STATUS-SUMMARY-001.20
+  it("stays muted when checks fail", () => {
+    expect(getPRStatusColor({ ...draftPR(), checks_state: "failure" })).toBe(
+      "text-muted-foreground",
+    );
+  });
+
+  // @covers AC-UI-PR-TASK-STATUS-SUMMARY-001.20
+  it("stays muted when changes are requested", () => {
+    expect(getPRStatusColor({ ...draftPR(), review_state: "changes_requested" })).toBe(
+      "text-muted-foreground",
+    );
+  });
+
  it("identifies the draft without claiming it is ready to merge", () => {
    const summary = derivePRTaskStatusSummary(draftPR(), false);
    expect(summary.rows.at(-1)).toEqual({ kind: "merge", status: "draft", tone: "muted" });
@@ -156,12 +156,12 @@ export function getPRStatusColor(pr: TaskPR): string {
  // membership must remain visible while provider checks or mergeability
  // fields hydrate, even when those fields still describe an earlier state.
  if (isPRQueued(pr)) return QUEUED;
-  if (pr.review_state === "changes_requested" || pr.checks_state === "failure") {
-    return RED_500;
-  }
  if (isPRDraft(pr)) {
    return MUTED_FOREGROUND;
  }
+  if (pr.review_state === "changes_requested" || pr.checks_state === "failure") {
+    return RED_500;
+  }
  const blockerColor = openMergeBlockerColor(pr);
  if (blockerColor) return blockerColor;
  if (isPRReadyToMerge(pr)) {
@@ -731,4 +731,48 @@ test.describe("PR status badge", () => {
      { timeout: 15_000 },
    );
  });
+
+  // @covers AC-UI-PR-TASK-STATUS-SUMMARY-001.20
+  test("keeps a draft PR icon muted when CI fails", async ({ testPage, apiClient, seedData }) => {
+    test.setTimeout(120_000);
+
+    const taskTitle = "Draft PR with failing CI";
+    const { task } = await seedBadgeTest(
+      apiClient,
+      seedData.workspaceId,
+      seedData.agentProfileId,
+      seedData.repositoryId,
+      taskTitle,
+    );
+    const kanban = new KanbanPage(testPage);
+    await kanban.goto();
+
+    await apiClient.mockGitHubAssociateTaskPR({
+      task_id: task.id,
+      owner: "testorg",
+      repo: "testrepo",
+      pr_number: 2968,
+      pr_url: "https://github.com/testorg/testrepo/pull/2968",
+      pr_title: taskTitle,
+      head_branch: "feat/draft-with-failing-ci",
+      base_branch: "main",
+      author_login: "test-user",
+      state: "open",
+      review_state: "approved",
+      checks_state: "failure",
+      mergeable_state: "draft",
+    });
+    await waitForTaskPRFields(apiClient, task.id, {
+      state: "open",
+      checks_state: "failure",
+      mergeable_state: "draft",
+    });
+
+    const taskRow = testPage.getByTestId("sidebar-task-item").filter({ hasText: taskTitle });
+    await expect(taskRow).toBeVisible({ timeout: 15_000 });
+    const icon = taskRow.getByTestId(`pr-task-icon-${task.id}`);
+    await expect(icon).toBeVisible({ timeout: 15_000 });
+    await expect(icon).toHaveClass(/text-muted-foreground/);
+    await expect(icon).not.toHaveClass(/text-red-500/);
+  });
 });
@@ -0,0 +1,92 @@
+---
+created: 2026-09-05
+status: implemented
+requirements:
+  - REQ-UI-PR-TASK-STATUS-SUMMARY-001
+system_design:
+  - ../../specs/ui/system-design/pr-task-status-summary.md
+legacy_specs: []
+---
+
+# Implementation Plan: Draft PR icon color
+
+## Overview
+
+Move the GitHub task-row draft check ahead of non-terminal review and CI failure
+checks in the existing `getPRStatusColor` precedence. Add a focused unit
+regression and a rendered desktop scenario, then run the existing mobile task
+status scenario to confirm that the passive mobile indicator remains intact.
+
+The bug is localized to the shared frontend display helper. No backend state,
+API payload, PR status chip semantics, queue behavior, or mobile composition
+changes are required.
+
+## Scope
+
+### In scope
+
+- Keep an open draft PR's task-row icon muted when CI reports failure.
+- Preserve terminal-state and active-merge-queue precedence.
+- Preserve red coloring for a non-draft PR with failing checks.
+- Cover the pure helper and the rendered sidebar icon.
+
+### Out of scope
+
+- Changing the CI-specific `PRStatusChip` status or its failure messaging.
+- Changing merge readiness, merge actions, queue state, backend projections, or
+  provider synchronization.
+- Adding a new mobile interaction or changing the existing task-row geometry.
+
+## Technical approach
+
+`getPRStatusColor` already owns the task-row icon color and `isPRDraft` already
+recognizes an open draft through `mergeable_state === "draft"`. Keep terminal
+states and `isPRQueued` ahead of the draft branch, then evaluate draft status
+before the combined `changes_requested`/failed-CI branch. This is the minimum
+precedence correction and leaves all later mergeability and readiness branches
+unchanged.
+
+The mobile design contract is unchanged because this is content-only styling:
+the task-switcher row remains the primary touch target, the existing PR status
+drawer remains the detailed mobile surface, and no new hit area or scroll owner
+is introduced. The existing `mobile-task-status-summary.spec.ts` is the nearest
+mobile proof for the passive indicator and no-overflow behavior.
+
+## Tests
+
+- `apps/web/components/github/pr-task-icon-draft.test.ts` will prove that an
+  open draft with `checks_state: "failure"` returns
+  `text-muted-foreground`, while the existing non-draft failure coverage keeps
+  the red behavior.
+- `apps/web/e2e/tests/pr/pr-status-badge.spec.ts` will prove the actual sidebar
+  icon class for a seeded draft PR with failing CI.
+
+## E2E tests
+
+- Desktop Chromium: extend the PR status badge suite with the draft/failing-CI
+  scenario, mapped to `AC-UI-PR-TASK-STATUS-SUMMARY-001.20`.
+- Mobile `mobile-chrome`: run the existing task-status-summary scenario. It
+  already covers the shared PR indicator inside the task-switcher row, normal
+  task navigation, and document horizontal containment; no new mobile spec is
+  needed for this content-only color change.
+
+## Work orders
+
+- [x] [Task 01: Restore muted draft PR icon precedence](task-01-draft-pr-icon-color.md)
+
+## Verification results
+
+- Focused frontend unit tests passed: 2 files, 72 tests.
+- Desktop Chromium E2E passed: 1 draft PR icon regression test.
+- Mobile Chromium E2E passed: 1 existing task-status-summary test.
+- Frontend TypeScript typecheck passed.
+- Specification lint and `git diff --check` passed before implementation.
+
+## Risks
+
+- Moving the draft branch must not move it ahead of terminal states or an
+  active queue entry, which have stronger lifecycle meaning.
+- `PRTaskIcon` is shared by sidebar, Kanban, and rich task-list surfaces, so the
+  helper regression must remain independent of a single mounting surface.
+- The CI status chip intentionally remains failure-oriented; changing it would
+  hide information outside this request.
@@ -0,0 +1,97 @@
+---
+id: "01-draft-pr-icon-color"
+title: "Restore muted draft PR icon precedence"
+status: done
+wave: 1
+depends_on: []
+plan: "plan.md"
+requirements:
+  - REQ-UI-PR-TASK-STATUS-SUMMARY-001
+acceptance_criteria:
+  - AC-UI-PR-TASK-STATUS-SUMMARY-001.20
+system_design:
+  - ../../specs/ui/system-design/pr-task-status-summary.md
+---
+
+# Task 01: Restore muted draft PR icon precedence
+
+## Summary
+
+Correct the shared GitHub task-row icon color precedence so an open draft pull
+request remains muted when CI reports failure. Prove the correction with a
+pure-helper regression and a rendered sidebar scenario while preserving the
+existing mobile passive-indicator path.
+
+## In scope
+
+- Add the failing draft-plus-failing-CI regression before production changes.
+- Move only the draft branch in `getPRStatusColor`.
+- Add the focused desktop rendered assertion and run the existing mobile check.
+
+## Out of scope
+
+- `PRStatusChip` CI status derivation.
+- GitHub synchronization, API contracts, merge actions, and queue behavior.
+- New mobile controls, layout changes, or translations.
+
+## Acceptance
+
+- An open draft PR with failing checks renders the task-row icon with
+  `text-muted-foreground`.
+- A non-draft PR with failing checks remains red, and terminal/queued precedence
+  remains covered by the existing tests.
+- The shared desktop icon and existing mobile task-row flow pass their focused
+  checks without document overflow or changed navigation behavior.
+
+## Verification
+
+```bash
+(cd apps && pnpm --filter @kandev/web test -- --run \
+  components/github/pr-task-icon-draft.test.ts \
+  components/github/pr-task-icon.test.ts)
+(cd apps/web && pnpm e2e:run tests/pr/pr-status-badge.spec.ts \
+  -- --grep "draft PR icon"
+)
+(cd apps/web && pnpm e2e:run --project mobile-chrome \
+  tests/task/mobile-task-status-summary.spec.ts
+)
+(cd apps/web && pnpm run typecheck)
+```
+
+## Files likely touched
+
+- `apps/web/components/github/pr-task-icon.tsx`
+- `apps/web/components/github/pr-task-icon-draft.test.ts`
+- `apps/web/e2e/tests/pr/pr-status-badge.spec.ts`
+
+## Dependencies
+
+None.
+
+## Risks
+
+- Draft must remain below terminal and active-queue checks but above failure
+  coloring for non-terminal task-icon presentation.
+
+## Parallelism
+
+`sequential`
+
+## Inputs
+
+- `docs/specs/ui/requirements/pr-task-status-summary.md`, especially
+  `AC-UI-PR-TASK-STATUS-SUMMARY-001.5` and `.20`.
+- `docs/specs/ui/system-design/pr-task-status-summary.md`, especially the
+  status-color precedence section and mobile behavior.
+- Existing `getPRStatusColor` and draft/PR status tests.
+
+## Results
+
+- TDD RED unit run reproduced the bug: the draft plus failing-CI case returned
+  `text-red-500`.
+- TDD RED desktop E2E run reproduced the rendered sidebar failure: the icon
+  received `text-red-500` instead of `text-muted-foreground`.
+- Focused frontend unit tests passed: 2 files, 72 tests.
+- Desktop Chromium E2E passed: 1 draft PR icon regression test.
+- Mobile Chromium E2E passed: 1 existing task-status-summary test.
+- Frontend TypeScript typecheck passed.
@@ -2,7 +2,7 @@
 status: active
 system: ui
 created: 2026-08-06
-updated: 2026-08-26
+updated: 2026-09-05
 owners:
  - kandev
 ---
@@ -41,6 +41,7 @@ the bounded task-status projection and not the full pull-request record.
 - **AC-UI-PR-TASK-STATUS-SUMMARY-001.17:** Each GitHub PR summary shall show the non-empty PR author login below the PR title. A missing author login shall not create an empty label.
 - **AC-UI-PR-TASK-STATUS-SUMMARY-001.18:** On a coarse pointer, the task row shall remain the primary touch target. After task navigation, the existing PR-status drawer shall show the same author login.
 - **AC-UI-PR-TASK-STATUS-SUMMARY-001.19:** TaskPR API and WebSocket payloads shall carry the owning workspace ID. The backend shall route typed PR events by that ID, and the frontend shall ignore missing or mismatched updates before changing the active workspace cache.
+- **AC-UI-PR-TASK-STATUS-SUMMARY-001.20:** When an open GitHub pull request is a draft and has no active merge-queue entry, the task pull-request icon shall use the muted color even when its review or CI fields report a failure. Terminal and active-queue precedence shall remain unchanged.

 ## Migrated source detail

@@ -55,6 +55,15 @@ inconsistent behavior.
 - `PRCIPopover` and `PRStatusChipDrawer` provide the existing detailed touch
  path after the user opens a task.

+### Status-color precedence
+
+`getPRStatusColor` in `apps/web/components/github/pr-task-icon.tsx` is the
+single source for the task-row GitHub pull-request icon color. It preserves
+terminal states first and active merge-queue membership next. For an open draft
+without an active queue entry, it returns the muted color before non-terminal
+review or check failures. The separate `PRStatusChip` keeps its CI-specific
+status derivation and is not a source for the task-row icon color.
+
 ## Data and contracts

 The design uses the existing `GET /api/v1/github/task-prs?task_ids=<id>`