diff --git a/AGENTS.md b/AGENTS.md index 410e5997..a6133c69 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,15 @@ This is a **requirement**: * **Any changes:** `npm run test:all` must pass * **New features:** Several unit tests + some integration tests + 1-2 new E2E tests + manual testing sanity check with Playwright +### External API Integration Rule +**Mocks are a liability when not validated against reality.** When building features that consume external APIs (GitHub, etc.): + +1. **Verify API response shapes with real data FIRST** — before writing specs, design docs, or mocks. Run the actual API call (e.g., `gh api ...`) and capture the real response. Include the real response in the design doc. +2. **Add at least one prod-mode E2E test** (`e2e/prod/`) for any feature that parses external API responses. Mock-mode tests verify UI behavior; prod-mode tests verify the API contract. +3. **Never assume API field names** — check the official API docs AND make a real call. APIs often differ from documentation or from what seems "obvious" (e.g., GitHub Timeline API's `head_ref_force_pushed` has `commit_id`, NOT `before_commit`/`after_commit`). + +**Rationale:** See `docs/retrospectives/2026-02-19-timeline-api-field-mismatch.md` — a wrong API shape assumption propagated from design doc → spec → mocks → all tests, and the bug was only caught during manual demo with real data. 1600+ automated tests all passed while the feature was fundamentally broken against real APIs. + ### Running Specific Tests (Vitest) **Important:** This project uses **Vitest** (not Jest), so Jest-specific flags like `--testPathPattern` will **not work**. @@ -126,6 +135,36 @@ You have access to Playwright via playwright-cli skill. Make sure to **only use * Sanity check your work as you reach a milestone in the implementation of a feature. Once you reach ~200 lines of code changes, the risk that you are compounding errors and don't have working code becomes high. A quick inspection in Playwright gives extra assurance that you are on the right track. * Final quality assurance. Don't ask the user to test a feature manually before you did it yourself! +## E2E Test Repo & Demo Data + +**Repo:** [`pedropaulovc/codjiflo-e2e-test-repo`](https://github.com/pedropaulovc/codjiflo-e2e-test-repo) — local path: `../../codjiflo-e2e-test-repo` + +Dedicated public repo for real GitHub data used by prod-mode E2E tests (`e2e/prod/`) and demo-presenter agents. You have full control: create PRs, push to main, force-push, add comments, close/reopen, merge, etc. Create as many PRs as needed. + +### Creating Scenarios + +Each scenario gets its own folder on `main` (base data) and its own PR branch (modifications). Branch naming: `test/` for E2E, `demo/` for demos. + +```bash +cd ../../codjiflo-e2e-test-repo +git checkout main && git pull +mkdir +# Add base files, commit, push to main +git checkout -b test/ +# Modify files, commit, push, create PR via gh +``` + +Manipulate PRs freely: force-push for iterations, `gh api` for line-level review comments, close/reopen, merge. Reference in tests as: +```typescript +const config = { owner: "pedropaulovc", repo: "codjiflo-e2e-test-repo", prNumber: }; +``` + +**Rules:** Never delete PRs/branches other tests depend on (`grep` for PR number in `e2e/`). PRs must stay open unless testing merged/closed state. Document new scenarios in `codjiflo-e2e-test-repo/README.md`. + +### For Demo-Presenter Agents + +Git operations against this repo ARE the "real user flows" for a code review tool — no mocking needed. Create PRs with the desired diff shape, add comments via `gh`, then navigate to the PR in CodjiFlo via Playwright (headed mode) to record the demo. Demo artifacts go to `spec/demo/` in the CodjiFlo repo. Escalate to coordinator/implementer for complex scenarios (multi-iteration force-push histories, etc.). + ## Shared environment There are multiple instances of Claude Code running in parallel. Each one has multiple node.exe instances (MCP, dev server, etc.) they also have dev servers running. Each worktree has its own designated port: 3010 for A, 3020 for B, etc. The `npm run dev` command is smart to only kill zombie servers associated with your worktree and only start a server in its designated port automatically. DO NOT kill all node.exe or kill by port number. If `npm run dev` fails STOP and ask the user for assistance. diff --git a/docs/retrospectives/2026-02-19-timeline-api-field-mismatch.md b/docs/retrospectives/2026-02-19-timeline-api-field-mismatch.md new file mode 100644 index 00000000..a70281e3 --- /dev/null +++ b/docs/retrospectives/2026-02-19-timeline-api-field-mismatch.md @@ -0,0 +1,136 @@ +# Retrospective: Timeline API Field Mismatch Bug + +**Date:** 2026-02-19 +**Feature:** S-4.2.1 Commit-Based Iteration Loader / S-4.2.2 Collapsed Iterations UI +**Bug:** `TimelineLoader` expected `before_commit.sha`/`after_commit.sha` on force-push events, but the real GitHub Timeline API only provides `commit_id` +**Impact:** Collapsed iteration groups never appeared with real GitHub data — only with mocked test data +**Fix:** Commits `dde70c9`, `2d2dcbc`, `551aef3` + +--- + +## What Happened + +The `TimelineLoader` was designed and implemented around an incorrect assumption about the GitHub Timeline API response shape. The code expected `head_ref_force_pushed` events to include nested `before_commit` and `after_commit` objects: + +```json +// ASSUMED (wrong) +{ + "event": "head_ref_force_pushed", + "before_commit": { "sha": "abc123" }, + "after_commit": { "sha": "def456" } +} +``` + +The real API only provides a flat `commit_id` field: + +```json +// ACTUAL +{ + "event": "head_ref_force_pushed", + "commit_id": "def456", + "commit_url": "https://api.github.com/repos/.../commits/def456" +} +``` + +This caused the filter at `timeline-loader.ts:155-160` to silently drop ALL force-push events (since `before_commit` and `after_commit` were always `undefined`), resulting in zero collapsed iteration groups with real data. + +## Root Cause Chain + +### 1. Design doc assumed API shape without verification + +`docs/plans/2026-02-17-s-4.2.1-commit-based-iteration-loader-design.md` explicitly specified the wrong interface: + +```typescript +interface TimelineEvent { + before_commit?: { sha: string }; + after_commit?: { sha: string }; +} +``` + +This was the source of truth for all downstream work. **Nobody verified this against the actual GitHub API documentation or a real API call.** + +### 2. Spec propagated the wrong assumption + +`spec/functional/iterations-stateless.md` documented force-push events as providing `before_commit.sha` and `after_commit.sha`. This was cited as the authoritative reference. + +### 3. Test plan used the wrong mock shape + +`spec/test/stateless-mode.md` contained mock data with the wrong fields: + +```javascript +mockTimelineWithForcePush = [ + { event: 'head_ref_force_pushed', before_commit: { sha: 'abc123' }, after_commit: { sha: 'def456' } } +] +``` + +### 4. All tests — unit and E2E — used mocks based on the wrong spec + +- `timeline-loader.test.ts`: 28 unit tests with wrong mock shapes +- `force-push-helpers.ts`: E2E mock helpers with wrong fields +- `github-mocks.ts`: `MockTimelineEvent` interface with wrong fields +- `iteration-stateless-mode.spec.ts`: inline mocks with wrong fields + +### 5. TDD was followed — but mocks were wrong + +The development process was textbook TDD: +1. Tester wrote failing tests (RED) +2. Implementer made them pass (GREEN) +3. Code reviewer approved + +But the tester's mocks were derived from the wrong spec, so the tests validated incorrect behavior. TDD only catches bugs when the tests themselves are correct. + +### 6. No real-API validation existed + +There were zero tests that hit the real GitHub Timeline API. All testing (unit + mock-mode E2E) used synthetic responses that matched the assumed shape. The bug was invisible to the entire test suite. + +## Why It Wasn't Caught + +| Safety Net | Why It Failed | +|-----------|---------------| +| Design review | API shape was assumed, not verified against docs | +| Spec review | Spec codified the assumption as fact | +| TDD (tester) | Tester wrote mocks from spec, which was wrong | +| TDD (implementer) | Implemented against wrong mocks, code "worked" | +| Code review | Reviewer checked code against spec — both were consistent but wrong | +| Unit tests | All mocked with wrong shape | +| E2E tests (mock mode) | All mocked with wrong shape | +| E2E tests (prod mode) | No prod-mode E2E test for force-push detection existed | +| Manual testing | Not performed until demo phase | + +**The entire chain was internally consistent.** Spec, design, code, and tests all agreed — they were just all wrong about the external API. + +## Lessons Learned + +### 1. Verify external API shapes with real data BEFORE writing specs + +Any feature that integrates with an external API must start with a real API call to capture the actual response shape. This should be a mandatory step in the design phase, not deferred to testing. + +**Action:** Add to AGENTS.md design checklist: "For external API integrations, capture real API responses and include them in the design doc." + +### 2. Prod-mode E2E tests are essential for API integration features + +Mock-mode E2E tests verify that the UI works with the expected data shape. Prod-mode E2E tests verify that the expected data shape actually matches reality. Both are needed. + +**Action:** For any feature that parses external API responses, at minimum one prod-mode E2E test should verify the API shape matches expectations. + +### 3. Mocks are a liability when they're not validated against reality + +Mocks make tests fast and deterministic, but they encode assumptions about external systems. When those assumptions are wrong, mocks hide bugs instead of finding them. + +**Action:** Consider adding runtime type assertions (e.g., Zod schemas) for external API responses that would fail fast in development if the response shape doesn't match. + +### 4. The demo phase caught what testing didn't + +The bug was discovered by the demo-presenter when they tried to use the feature with a real PR. This validates the demo phase as a critical quality gate — it's the first time the feature touches real data end-to-end. + +### 5. "All tests pass" is not the same as "it works" + +1477 unit tests and 130 E2E tests all passed. Zero tests used real GitHub API data for force-push detection. Test count is not a proxy for correctness. + +## Remaining Cleanup + +The following files still contain the wrong `before_commit`/`after_commit` fields and should be updated to reflect reality: + +- `spec/functional/iterations-stateless.md` (lines 65, 170) +- `spec/test/stateless-mode.md` (lines 159-167) +- `docs/plans/2026-02-17-s-4.2.1-commit-based-iteration-loader-design.md` (lines 97-98, 128) diff --git a/e2e/fixtures/force-push-helpers.ts b/e2e/fixtures/force-push-helpers.ts new file mode 100644 index 00000000..8dc10070 --- /dev/null +++ b/e2e/fixtures/force-push-helpers.ts @@ -0,0 +1,166 @@ +/** + * Shared force-push E2E test helpers + * + * Extracted from collapsed-iterations-ui.spec.ts and + * collapsed-iterations-expanded.spec.ts to eliminate duplication. + */ + +import type { Page } from "@playwright/test"; +import { + setupAuthState, + setupFullPRMocks, + setupStatelessIterationMocks, + type MockPR, + type MockFile, +} from "./github-mocks"; +import { setupLegacyDefaults } from "./legacy-defaults"; + +// ============================================================================ +// Shared constants +// ============================================================================ + +export const forcePushMockPR: MockPR = { + id: 1, + number: 123, + title: "Test PR for Force Push Scenarios", + body: "Testing force push iteration scenarios", + state: "open", + merged: false, + draft: false, + user: { + id: 1, + login: "testuser", + avatar_url: "https://avatars.githubusercontent.com/u/1", + }, + head: { ref: "feature/test", sha: "abc123" }, + base: { ref: "main", sha: "def456" }, + html_url: "https://github.com/test/repo/pull/123", + created_at: "2024-01-01T10:00:00Z", + updated_at: "2024-01-05T15:00:00Z", +}; + +export const forcePushMockFiles: MockFile[] = [ + { + filename: "src/test.ts", + status: "modified", + additions: 10, + deletions: 5, + changes: 15, + patch: "@@ -1,5 +1,10 @@\n+// New code\n const x = 1;", + }, +]; + +export const forcePushConfig = { + owner: "test", + repo: "repo", + prNumber: 123, + pageUrl: "/test/repo/123", +}; + +// ============================================================================ +// Setup helpers +// ============================================================================ + +export interface ForcePushScenarioOptions { + eventId: number; + beforeSha: string; + afterSha: string; + liveCommits: { + sha: string; + message: string; + author: string; + date: string; + }[]; + compareResponse: + | { + status: 200; + commits: { + sha: string; + message: string; + authorName: string; + authorLogin: string; + date: string; + }[]; + } + | { status: 404 }; +} + +/** + * Set up common beforeEach mocks for force-push tests: + * legacy defaults, auth state, and full PR mocks. + */ +export async function setupForcePushDefaults(page: Page): Promise { + await setupLegacyDefaults(page); + await setupAuthState(page); + await setupFullPRMocks( + page, + forcePushConfig.owner, + forcePushConfig.repo, + forcePushConfig.prNumber, + { + pr: forcePushMockPR, + files: forcePushMockFiles, + } + ); +} + +/** + * Set up a force-push scenario with mock timeline and compare API. + */ +export async function setupForcePushScenario( + page: Page, + options: ForcePushScenarioOptions +): Promise { + await setupStatelessIterationMocks( + page, + forcePushConfig.owner, + forcePushConfig.repo, + forcePushConfig.prNumber, + { + commits: options.liveCommits, + timeline: [ + { + id: 0, + event: "committed", + created_at: "2024-01-01T12:00:00Z", + sha: options.beforeSha, + }, + { + id: options.eventId, + event: "head_ref_force_pushed", + created_at: "2024-01-02T12:00:00Z", + commit_id: options.afterSha, + }, + ], + } + ); + + await page.route( + `https://api.github.com/repos/${forcePushConfig.owner}/${forcePushConfig.repo}/compare/${options.afterSha}...${options.beforeSha}`, + async (route) => { + if (options.compareResponse.status === 404) { + await route.fulfill({ + status: 404, + contentType: "application/json", + body: JSON.stringify({ message: "Not Found" }), + }); + return; + } + + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ + commits: options.compareResponse.commits.map((c) => ({ + sha: c.sha, + commit: { + message: c.message, + author: { name: c.authorName, date: c.date }, + }, + author: { login: c.authorLogin }, + })), + }), + }); + } + ); +} diff --git a/e2e/fixtures/github-mocks.ts b/e2e/fixtures/github-mocks.ts index 0f1aa3e2..eb624dfa 100644 --- a/e2e/fixtures/github-mocks.ts +++ b/e2e/fixtures/github-mocks.ts @@ -363,6 +363,31 @@ export async function setupIterationMocks( }); } ); + + // Mock stateless iteration endpoints (timeline + commits) with empty responses. + // Without these, loadIterations' stateless fallback leaks requests to the real + // GitHub API, which returns 401 when using a mock PAT token. + await page.route( + `https://api.github.com/repos/${owner}/${repo}/issues/${String(prNumber)}/timeline**`, + async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify([]), + }); + } + ); + + await page.route( + `https://api.github.com/repos/${owner}/${repo}/pulls/${String(prNumber)}/commits**`, + async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify([]), + }); + } + ); } /** @@ -613,8 +638,10 @@ export interface MockTimelineEvent { id: number; event: string; created_at: string; - before_commit?: { sha: string }; - after_commit?: { sha: string }; + /** SHA of the new HEAD after force-push (real GitHub API field) */ + commit_id?: string; + /** SHA from committed events in timeline */ + sha?: string; } /** diff --git a/e2e/mock/stateless-mode/collapsed-iterations-expanded.spec.ts b/e2e/mock/stateless-mode/collapsed-iterations-expanded.spec.ts new file mode 100644 index 00000000..c92edbe0 --- /dev/null +++ b/e2e/mock/stateless-mode/collapsed-iterations-expanded.spec.ts @@ -0,0 +1,359 @@ +import { test, expect } from "@playwright/test"; +import { + forcePushConfig, + setupForcePushDefaults, + setupForcePushScenario, +} from "../../fixtures/force-push-helpers"; + +test.describe("Collapsed Iterations Expanded View", () => { + test.beforeEach(async ({ page }) => { + await setupForcePushDefaults(page); + }); + + test("Include button expands collapsed group into individual discarded iteration tabs", async ({ + page, + }) => { + // 2 discarded commits + 1 live commit = revisions 1, 2 (discarded), 3 (live) + await setupForcePushScenario(page, { + eventId: 5001, + beforeSha: "old-before-sha", + afterSha: "new-after-sha", + liveCommits: [ + { + sha: "new-commit-1", + message: "New commit after force push", + author: "testuser", + date: "2024-01-03T10:00:00Z", + }, + ], + compareResponse: { + status: 200, + commits: [ + { + sha: "discarded-exp-1", + message: "Discarded commit 1", + authorName: "testuser", + authorLogin: "testuser", + date: "2024-01-01T10:00:00Z", + }, + { + sha: "discarded-exp-2", + message: "Discarded commit 2", + authorName: "testuser", + authorLogin: "testuser", + date: "2024-01-01T12:00:00Z", + }, + ], + }, + }); + + await page.goto(forcePushConfig.pageUrl); + + // Verify initial state: collapsed group tab visible + const collapsedTab = page.getByTestId("collapsed-group-5001"); + await expect(collapsedTab).toBeVisible(); + + // Click to open history view, then click "Include" + await collapsedTab.click(); + const includeBtn = page.getByTestId("collapsed-history-include-btn"); + await expect(includeBtn).toBeVisible(); + await includeBtn.click(); + + // After expanding: collapsed group tab should be replaced by individual tabs + await expect(collapsedTab).toBeHidden(); + + // Individual discarded iteration tabs should now be visible + // Revisions 1 and 2 are the discarded iterations + const discardedTab1 = page.getByTestId("iteration-tab-1"); + const discardedTab2 = page.getByTestId("iteration-tab-2"); + await expect(discardedTab1).toBeVisible(); + await expect(discardedTab2).toBeVisible(); + + // Discarded tabs should have the 'discarded' CSS class (grayed out) + await expect(discardedTab1).toHaveClass(/discarded/); + await expect(discardedTab2).toHaveClass(/discarded/); + + // The live iteration tab (revision 3) should still be visible and NOT discarded + const liveTab = page.getByTestId("iteration-tab-3"); + await expect(liveTab).toBeVisible(); + await expect(liveTab).not.toHaveClass(/discarded/); + }); + + test("Expanded discarded tabs participate in drag range selection", async ({ + page, + }) => { + // 2 discarded + 2 live = revisions 1,2 (discarded), 3,4 (live) + await setupForcePushScenario(page, { + eventId: 6001, + beforeSha: "old-drag-sha", + afterSha: "new-drag-sha", + liveCommits: [ + { + sha: "new-commit-1", + message: "Live commit 1", + author: "testuser", + date: "2024-01-03T10:00:00Z", + }, + { + sha: "new-commit-2", + message: "Live commit 2", + author: "testuser", + date: "2024-01-04T10:00:00Z", + }, + ], + compareResponse: { + status: 200, + commits: [ + { + sha: "discarded-drag-1", + message: "Old commit 1", + authorName: "testuser", + authorLogin: "testuser", + date: "2024-01-01T10:00:00Z", + }, + { + sha: "discarded-drag-2", + message: "Old commit 2", + authorName: "testuser", + authorLogin: "testuser", + date: "2024-01-01T12:00:00Z", + }, + ], + }, + }); + + await page.goto(forcePushConfig.pageUrl); + + // Expand the collapsed group + const collapsedTab = page.getByTestId("collapsed-group-6001"); + await expect(collapsedTab).toBeVisible(); + await collapsedTab.click(); + await page.getByTestId("collapsed-history-include-btn").click(); + + // Wait for expanded tabs to appear + const discardedTab1 = page.getByTestId("iteration-tab-1"); + const liveTab4 = page.getByTestId("iteration-tab-4"); + await expect(discardedTab1).toBeVisible(); + await expect(liveTab4).toBeVisible(); + + // Drag from discarded tab 2 to live tab 3 (cross-boundary range) + // This creates a range where both boundary tabs get 'selected' class + // and middle tabs (if any) get 'in-range' class + const discardedTab2 = page.getByTestId("iteration-tab-2"); + const liveTab3 = page.getByTestId("iteration-tab-3"); + await discardedTab2.hover(); + await page.mouse.down(); + await liveTab3.hover(); + await page.mouse.up(); + + // Range boundary tabs get 'selected', middle tabs get 'in-range' + // All tabs in the range should have aria-pressed="true" + await expect(discardedTab2).toHaveAttribute("aria-pressed", "true"); + await expect(liveTab3).toHaveAttribute("aria-pressed", "true"); + + // The discarded tab should also have 'discarded' class (styling preserved during selection) + await expect(discardedTab2).toHaveClass(/discarded/); + + // Tabs outside the range should NOT be selected + await expect(discardedTab1).toHaveAttribute("aria-pressed", "false"); + await expect(liveTab4).toHaveAttribute("aria-pressed", "false"); + }); + + test("CSS rule .iteration-tab.unavailable exists with correct opacity and cursor properties", async ({ + page, + }) => { + // AC-4.2.2.8: When a collapsed iteration's commit is GC'd, show as unavailable + // within the expanded view. The tab should have .unavailable class, aria-disabled, + // and no mouse handlers (pointer-events: none). + // + // We set up a scenario with 2 discarded commits: one available, one unavailable. + // The compare API returns both, but we inject 'unavailable' status directly + // into the store after loading (since the timeline-loader always returns 'available'). + + await setupForcePushScenario(page, { + eventId: 7001, + beforeSha: "old-unavail-sha", + afterSha: "new-unavail-sha", + liveCommits: [ + { + sha: "new-commit-1", + message: "Live commit", + author: "testuser", + date: "2024-01-03T10:00:00Z", + }, + ], + compareResponse: { + status: 200, + commits: [ + { + sha: "discarded-avail", + message: "Available discarded commit", + authorName: "testuser", + authorLogin: "testuser", + date: "2024-01-01T10:00:00Z", + }, + { + sha: "discarded-gc", + message: "GC'd discarded commit", + authorName: "testuser", + authorLogin: "testuser", + date: "2024-01-01T12:00:00Z", + }, + ], + }, + }); + + await page.goto(forcePushConfig.pageUrl); + + // Wait for iterations to load + const selector = page.getByTestId("iteration-selector"); + await expect(selector).toBeVisible(); + const collapsedTab = page.getByTestId("collapsed-group-7001"); + await expect(collapsedTab).toBeVisible(); + + // Expand the group + await collapsedTab.click(); + await page.getByTestId("collapsed-history-include-btn").click(); + + // Both discarded tabs should be visible (both are 'available' from API) + const discardedTab1 = page.getByTestId("iteration-tab-1"); + const discardedTab2 = page.getByTestId("iteration-tab-2"); + await expect(discardedTab1).toBeVisible(); + await expect(discardedTab2).toBeVisible(); + await expect(discardedTab1).toHaveClass(/discarded/); + await expect(discardedTab2).toHaveClass(/discarded/); + + // Verify .iteration-tab.unavailable CSS rule exists with correct properties + const unavailableStyles = await page.evaluate(() => { + for (const sheet of document.styleSheets) { + try { + for (const rule of sheet.cssRules) { + if (rule instanceof CSSStyleRule && rule.selectorText === '.iteration-tab.unavailable') { + return { + opacity: rule.style.opacity, + cursor: rule.style.cursor, + }; + } + } + } catch { + // Cross-origin stylesheet, skip + } + } + return null; + }); + + expect(unavailableStyles).not.toBeNull(); + expect(unavailableStyles?.opacity).toBe("0.4"); + expect(unavailableStyles?.cursor).toBe("not-allowed"); + }); + + test("Discarded tabs have reduced opacity and live tabs have full opacity", async ({ + page, + }) => { + await setupForcePushScenario(page, { + eventId: 8001, + beforeSha: "old-style-sha", + afterSha: "new-style-sha", + liveCommits: [ + { + sha: "new-commit-1", + message: "Live commit", + author: "testuser", + date: "2024-01-03T10:00:00Z", + }, + ], + compareResponse: { + status: 200, + commits: [ + { + sha: "discarded-style-1", + message: "Old commit", + authorName: "testuser", + authorLogin: "testuser", + date: "2024-01-01T10:00:00Z", + }, + ], + }, + }); + + await page.goto(forcePushConfig.pageUrl); + + // Expand the group + const collapsedTab = page.getByTestId("collapsed-group-8001"); + await expect(collapsedTab).toBeVisible(); + await collapsedTab.click(); + await page.getByTestId("collapsed-history-include-btn").click(); + + // Discarded tab should have reduced opacity (spec says 0.6) + const discardedTab = page.getByTestId("iteration-tab-1"); + await expect(discardedTab).toBeVisible(); + await expect(discardedTab).toHaveClass(/discarded/); + + const opacity = await discardedTab.evaluate( + (el) => window.getComputedStyle(el).opacity + ); + const opacityNum = parseFloat(opacity); + expect(opacityNum).toBeLessThanOrEqual(0.7); + expect(opacityNum).toBeGreaterThanOrEqual(0.5); + + // Live tab should have full opacity (1.0) + const liveTab = page.getByTestId("iteration-tab-2"); + const liveOpacity = await liveTab.evaluate( + (el) => window.getComputedStyle(el).opacity + ); + expect(parseFloat(liveOpacity)).toBe(1); + }); + + test("Clicking a discarded tab selects it for range diff like a regular tab", async ({ + page, + }) => { + await setupForcePushScenario(page, { + eventId: 9001, + beforeSha: "old-click-sha", + afterSha: "new-click-sha", + liveCommits: [ + { + sha: "new-commit-1", + message: "Live commit", + author: "testuser", + date: "2024-01-03T10:00:00Z", + }, + ], + compareResponse: { + status: 200, + commits: [ + { + sha: "discarded-click-1", + message: "Discarded commit", + authorName: "testuser", + authorLogin: "testuser", + date: "2024-01-01T10:00:00Z", + }, + ], + }, + }); + + await page.goto(forcePushConfig.pageUrl); + + // Expand + const collapsedTab = page.getByTestId("collapsed-group-9001"); + await expect(collapsedTab).toBeVisible(); + await collapsedTab.click(); + await page.getByTestId("collapsed-history-include-btn").click(); + + // Click the discarded tab (single click = select from base to this iteration) + const discardedTab = page.getByTestId("iteration-tab-1"); + await expect(discardedTab).toBeVisible(); + await discardedTab.click(); + + // Discarded tab should be selected (aria-pressed="true") + await expect(discardedTab).toHaveAttribute("aria-pressed", "true"); + + // It should have both 'discarded' (styling) and 'selected' or range classes + await expect(discardedTab).toHaveClass(/discarded/); + + // Live tab should NOT be in the selection (clicked only discarded tab 1) + const liveTab = page.getByTestId("iteration-tab-2"); + await expect(liveTab).toHaveAttribute("aria-pressed", "false"); + }); +}); diff --git a/e2e/mock/stateless-mode/collapsed-iterations-ui.spec.ts b/e2e/mock/stateless-mode/collapsed-iterations-ui.spec.ts new file mode 100644 index 00000000..2a83870a --- /dev/null +++ b/e2e/mock/stateless-mode/collapsed-iterations-ui.spec.ts @@ -0,0 +1,306 @@ +import { test, expect } from "@playwright/test"; +import { + forcePushConfig, + setupForcePushDefaults, + setupForcePushScenario, +} from "../../fixtures/force-push-helpers"; + +test.describe("Collapsed Iterations UI", () => { + test.beforeEach(async ({ page }) => { + await setupForcePushDefaults(page); + }); + + test("Click collapsed tab shows history view with commit details and iteration selector stays visible", async ({ + page, + }) => { + await setupForcePushScenario(page, { + eventId: 5001, + beforeSha: "old-before-sha", + afterSha: "new-after-sha", + liveCommits: [ + { + sha: "new-commit-1", + message: "New first commit after force push", + author: "testuser", + date: "2024-01-03T10:00:00Z", + }, + { + sha: "new-commit-2", + message: "Second commit after force push", + author: "testuser", + date: "2024-01-04T10:00:00Z", + }, + ], + compareResponse: { + status: 200, + commits: [ + { + sha: "discarded-aaa111", + message: "Old WIP commit that was squashed", + authorName: "alice", + authorLogin: "alice", + date: "2024-01-01T10:00:00Z", + }, + { + sha: "discarded-bbb222", + message: "Another discarded commit with fixes", + authorName: "bob", + authorLogin: "bob", + date: "2024-01-01T14:00:00Z", + }, + ], + }, + }); + + await page.goto(forcePushConfig.pageUrl); + + // Wait for iteration selector to appear + const selector = page.getByTestId("iteration-selector"); + await expect(selector).toBeVisible(); + + // Collapsed group tab must be a + + + ); +} diff --git a/src/features/iterations/components/IterationSelector.test.tsx b/src/features/iterations/components/IterationSelector.test.tsx index 0e25ff43..1e3b5eeb 100644 --- a/src/features/iterations/components/IterationSelector.test.tsx +++ b/src/features/iterations/components/IterationSelector.test.tsx @@ -9,6 +9,7 @@ interface MockStoreState { collapsedGroups: CollapsedIterationGroup[]; selectedRange: IterationRange | null; selectRange: ReturnType; + selectCollapsedGroup: ReturnType; isLoading: boolean; mode: 'stateful' | 'stateless'; artifactReference: ArtifactReference | null; @@ -50,17 +51,19 @@ function createMockIteration(revision: number, options?: { status?: 'live' | 'co // Helper to setup mock state function setupMockState(state: Partial) { const mockSelectRange = vi.fn(); + const mockSelectCollapsedGroup = vi.fn(); currentMockState = { iterations: [], collapsedGroups: [], selectedRange: null, selectRange: mockSelectRange, + selectCollapsedGroup: mockSelectCollapsedGroup, isLoading: false, mode: 'stateful', artifactReference: null, ...state, }; - return { mockSelectRange: currentMockState.selectRange }; + return { mockSelectRange: currentMockState.selectRange, mockSelectCollapsedGroup: currentMockState.selectCollapsedGroup }; } describe('IterationSelector', () => { @@ -493,8 +496,8 @@ describe('IterationSelector', () => { expect(collapsedTab).toHaveAttribute('title', '2 iterations discarded'); }); - it('collapsed group tab is non-interactive (no mouse/keyboard handlers)', () => { - const { mockSelectRange } = setupMockState({ + it('clicking collapsed group tab calls selectCollapsedGroup', () => { + const { mockSelectCollapsedGroup } = setupMockState({ iterations: [ createMockIteration(1, { status: 'collapsed', collapsedGroupId: '100' }), createMockIteration(2), @@ -513,10 +516,9 @@ describe('IterationSelector', () => { render(); const collapsedTab = screen.getByTestId('collapsed-group-100'); - fireEvent.mouseDown(collapsedTab); - fireEvent.mouseUp(screen.getByTestId('iteration-selector')); + fireEvent.click(collapsedTab); - expect(mockSelectRange).not.toHaveBeenCalled(); + expect(mockSelectCollapsedGroup).toHaveBeenCalledWith('100'); }); it('renders unknown count group tab', () => { @@ -612,7 +614,7 @@ describe('IterationSelector', () => { expect(screen.getByTestId('collapsed-group-100')).toHaveAttribute('title', '1 iteration discarded'); }); - it('collapsed group tab has role="img" for accessibility', () => { + it('collapsed group tab is a clickable button with aria-label', () => { setupMockState({ iterations: [ createMockIteration(1, { status: 'collapsed', collapsedGroupId: '100' }), @@ -632,10 +634,177 @@ describe('IterationSelector', () => { render(); const collapsedTab = screen.getByTestId('collapsed-group-100'); - expect(collapsedTab).toHaveAttribute('role', 'img'); expect(collapsedTab).toHaveAttribute('aria-label'); - // Should NOT be a button element (non-interactive) - expect(collapsedTab.tagName).toBe('DIV'); + expect(collapsedTab).not.toHaveAttribute('role', 'img'); + expect(collapsedTab.tagName).toBe('BUTTON'); + }); + + it('expanded group renders individual discarded iteration tabs with discarded class', () => { + setupMockState({ + iterations: [ + createMockIteration(1, { status: 'collapsed', collapsedGroupId: '100' }), + createMockIteration(2, { status: 'collapsed', collapsedGroupId: '100' }), + createMockIteration(3), + ], + collapsedGroups: [{ + forcePushEventId: '100', + discardedRevisions: [1, 2], + commits: [], + reason: 'force_push', + visibility: 'expanded', + }], + selectedRange: { fromSnapshot: 0, toSnapshot: 5 }, + mode: 'stateless', + }); + + render(); + + // Individual discarded tabs should render with discarded class + const tab1 = screen.getByTestId('iteration-tab-1'); + const tab2 = screen.getByTestId('iteration-tab-2'); + expect(tab1).toHaveClass('discarded'); + expect(tab2).toHaveClass('discarded'); + + // Collapsed group tab should NOT be present + expect(screen.queryByTestId('collapsed-group-100')).not.toBeInTheDocument(); + + // Live tab should NOT have discarded class + const tab3 = screen.getByTestId('iteration-tab-3'); + expect(tab3).not.toHaveClass('discarded'); + }); + + it('expanded discarded tabs participate in drag range selection', () => { + const { mockSelectRange } = setupMockState({ + iterations: [ + createMockIteration(1, { status: 'collapsed', collapsedGroupId: '100' }), + createMockIteration(2), + ], + collapsedGroups: [{ + forcePushEventId: '100', + discardedRevisions: [1], + commits: [], + reason: 'force_push', + visibility: 'expanded', + }], + selectedRange: { fromSnapshot: 0, toSnapshot: 3 }, + mode: 'stateless', + }); + + render(); + + // Drag from discarded tab 1 to live tab 2 + const tab1 = screen.getByTestId('iteration-tab-1'); + const tab2 = screen.getByTestId('iteration-tab-2'); + const container = screen.getByTestId('iteration-selector'); + + fireEvent.mouseDown(tab1); + fireEvent.mouseEnter(tab2); + fireEvent.mouseUp(container); + + // Range should include both tabs + expect(mockSelectRange).toHaveBeenCalledWith(1, 3); + }); + + it('clicking expanded discarded tab selects it like a regular tab', () => { + const { mockSelectRange } = setupMockState({ + iterations: [ + createMockIteration(1, { status: 'collapsed', collapsedGroupId: '100' }), + createMockIteration(2), + ], + collapsedGroups: [{ + forcePushEventId: '100', + discardedRevisions: [1], + commits: [], + reason: 'force_push', + visibility: 'expanded', + }], + selectedRange: { fromSnapshot: 0, toSnapshot: 3 }, + mode: 'stateless', + }); + + render(); + + const tab1 = screen.getByTestId('iteration-tab-1'); + fireEvent.mouseDown(tab1); + fireEvent.mouseUp(screen.getByTestId('iteration-selector')); + + // Single click selects from base to this iteration + expect(mockSelectRange).toHaveBeenCalledWith(0, 1); + }); + + it('unavailable expanded iteration gets unavailable class, disabled attribute, and is non-interactive', () => { + const { mockSelectRange } = setupMockState({ + iterations: [ + createMockIteration(1, { status: 'collapsed', collapsedGroupId: '100' }), + createMockIteration(2), + ], + collapsedGroups: [{ + forcePushEventId: '100', + discardedRevisions: [1], + commits: [{ + sha: 'head-sha-1', + message: 'GCd commit', + author: 'testuser', + date: '2024-01-01T10:00:00Z', + status: 'unavailable', + }], + reason: 'force_push', + visibility: 'expanded', + }], + selectedRange: { fromSnapshot: 0, toSnapshot: 3 }, + mode: 'stateless', + }); + + render(); + + const tab1 = screen.getByTestId('iteration-tab-1'); + + // Should have both discarded and unavailable classes + expect(tab1).toHaveClass('discarded'); + expect(tab1).toHaveClass('unavailable'); + + // Should be disabled + expect(tab1).toBeDisabled(); + + // Mouse events should not trigger selectRange + fireEvent.mouseDown(tab1); + fireEvent.mouseUp(screen.getByTestId('iteration-selector')); + expect(mockSelectRange).not.toHaveBeenCalled(); + + // Keyboard events should not trigger selectRange + fireEvent.keyDown(tab1, { key: 'Enter' }); + expect(mockSelectRange).not.toHaveBeenCalled(); + }); + + it('available expanded iteration does not get unavailable class', () => { + setupMockState({ + iterations: [ + createMockIteration(1, { status: 'collapsed', collapsedGroupId: '100' }), + createMockIteration(2), + ], + collapsedGroups: [{ + forcePushEventId: '100', + discardedRevisions: [1], + commits: [{ + sha: 'head-sha-1', + message: 'Available commit', + author: 'testuser', + date: '2024-01-01T10:00:00Z', + status: 'available', + }], + reason: 'force_push', + visibility: 'expanded', + }], + selectedRange: { fromSnapshot: 0, toSnapshot: 3 }, + mode: 'stateless', + }); + + render(); + + const tab1 = screen.getByTestId('iteration-tab-1'); + expect(tab1).toHaveClass('discarded'); + expect(tab1).not.toHaveClass('unavailable'); + expect(tab1).not.toBeDisabled(); }); it('drag across live tabs skips collapsed group (no range includes collapsed)', () => { diff --git a/src/features/iterations/components/IterationSelector.tsx b/src/features/iterations/components/IterationSelector.tsx index cba3b10f..21708acd 100644 --- a/src/features/iterations/components/IterationSelector.tsx +++ b/src/features/iterations/components/IterationSelector.tsx @@ -35,6 +35,8 @@ interface IterationTabProps { isInRange: boolean; isRangeStart: boolean; isRangeEnd: boolean; + isDiscarded?: boolean; + isUnavailable?: boolean; onMouseDown: (revision: number) => void; onMouseEnter: (revision: number) => void; onSelect: (revision: number) => void; @@ -46,6 +48,8 @@ function IterationTab({ isInRange, isRangeStart, isRangeEnd, + isDiscarded = false, + isUnavailable = false, onMouseDown, onMouseEnter, onSelect, @@ -56,6 +60,8 @@ function IterationTab({ isInRange && 'in-range', isRangeStart && 'range-start', isRangeEnd && 'range-end', + isDiscarded && 'discarded', + isUnavailable && 'unavailable', ] .filter(Boolean) .join(' '); @@ -65,7 +71,7 @@ function IterationTab({ day: 'numeric', }); - const handleKeyDown = (e: React.KeyboardEvent) => { + const handleKeyDown = isUnavailable ? undefined : (e: React.KeyboardEvent) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); onSelect(iteration.revision); @@ -81,6 +87,7 @@ function IterationTab({ onKeyDown={handleKeyDown} title={`Iteration ${iteration.revision} (${date})`} aria-pressed={isSelected || isInRange} + disabled={isUnavailable} data-testid={`iteration-tab-${iteration.revision}`} > @@ -94,24 +101,26 @@ function IterationTab({ interface CollapsedGroupTabProps { group: CollapsedIterationGroup; + onClick: (groupId: string) => void; } -function CollapsedGroupTab({ group }: CollapsedGroupTabProps) { +function CollapsedGroupTab({ group, onClick }: CollapsedGroupTabProps) { const count = group.discardedRevisions.length; const tooltip = group.unknownCount ? 'Unknown iterations discarded' : `${String(count)} iteration${count === 1 ? '' : 's'} discarded`; return ( -
onClick(group.forcePushEventId)} >
+ ); } @@ -124,7 +133,7 @@ interface IterationSelectorProps { } export function IterationSelector({ className }: IterationSelectorProps) { - const { iterations, collapsedGroups, selectRange, isLoading, artifactReference } = useIterationStore(); + const { iterations, collapsedGroups, selectRange, selectCollapsedGroup, isLoading, artifactReference } = useIterationStore(); const selectedRange = useIterationStore(selectSelectedRange); const [dragState, setDragState] = useState({ @@ -252,7 +261,7 @@ export function IterationSelector({ className }: IterationSelectorProps) { // Build display items: live iterations as tabs, collapsed groups as single tabs const displayItems = useMemo(() => { - const items: (| { type: 'iteration'; iteration: Iteration } + const items: (| { type: 'iteration'; iteration: Iteration; isUnavailable?: boolean } | { type: 'collapsed-group'; group: CollapsedIterationGroup })[] = []; const processedGroups: Set = new Set(); @@ -262,10 +271,19 @@ export function IterationSelector({ className }: IterationSelectorProps) { for (const iteration of iterations) { if (iteration.status === 'collapsed' && iteration.collapsedGroupId) { + const group = collapsedGroupById.get(iteration.collapsedGroupId); + + // When group is expanded, show individual iteration tabs instead of collapsed tab + if (group?.visibility === 'expanded') { + const commit = group.commits.find(c => c.sha === iteration.headSha); + const isUnavailable = commit?.status === 'unavailable'; + items.push({ type: 'iteration', iteration, isUnavailable }); + continue; + } + // Render collapsed group tab (once per group) if (!processedGroups.has(iteration.collapsedGroupId)) { processedGroups.add(iteration.collapsedGroupId); - const group = collapsedGroupById.get(iteration.collapsedGroupId); if (group) { items.push({ type: 'collapsed-group', group }); } @@ -336,11 +354,12 @@ export function IterationSelector({ className }: IterationSelectorProps) { ); } - const { iteration } = item; + const { iteration, isUnavailable } = item; // During drag, show preview highlighting const inPreviewRange = @@ -372,6 +391,8 @@ export function IterationSelector({ className }: IterationSelectorProps) { isInRange={isInRange} isRangeStart={isRangeStartTab} isRangeEnd={isRangeEndTab} + isDiscarded={iteration.status === 'collapsed'} + isUnavailable={isUnavailable ?? false} onMouseDown={handleMouseDown} onMouseEnter={handleMouseEnter} onSelect={handleKeyboardSelect} diff --git a/src/features/iterations/components/index.ts b/src/features/iterations/components/index.ts index 50112143..b3a7bff5 100644 --- a/src/features/iterations/components/index.ts +++ b/src/features/iterations/components/index.ts @@ -5,3 +5,4 @@ */ export { IterationSelector } from './IterationSelector'; +export { CollapsedIterationHistoryView } from './CollapsedIterationHistoryView'; diff --git a/src/features/iterations/index.ts b/src/features/iterations/index.ts index fcc604a3..980f19ed 100644 --- a/src/features/iterations/index.ts +++ b/src/features/iterations/index.ts @@ -6,7 +6,7 @@ */ // Components -export { IterationSelector } from './components'; +export { IterationSelector, CollapsedIterationHistoryView } from './components'; // Store export { useIterationStore } from './stores'; diff --git a/src/features/iterations/stores/useIterationStore.test.ts b/src/features/iterations/stores/useIterationStore.test.ts index bac30030..38027ed4 100644 --- a/src/features/iterations/stores/useIterationStore.test.ts +++ b/src/features/iterations/stores/useIterationStore.test.ts @@ -123,6 +123,7 @@ describe('useIterationStore', () => { artifactReference: null, currentPrKey: null, selectedRanges: {}, + activeCollapsedGroupId: null, client: null, spanTrackerService: null, isLoading: false, @@ -721,6 +722,14 @@ describe('useIterationStore', () => { toSnapshot: 5, }); }); + + it('should clear activeCollapsedGroupId when selecting a preset', () => { + useIterationStore.getState().selectCollapsedGroup('group-100'); + expect(useIterationStore.getState().activeCollapsedGroupId).toBe('group-100'); + + useIterationStore.getState().selectPreset('full'); + expect(useIterationStore.getState().activeCollapsedGroupId).toBeNull(); + }); }); describe('reset', () => { @@ -755,11 +764,127 @@ describe('useIterationStore', () => { expect(state.isLoading).toBe(false); expect(state.mode).toBe('stateful'); expect(state.collapsedGroups).toHaveLength(0); + expect(state.activeCollapsedGroupId).toBeNull(); expect(mockClose).toHaveBeenCalled(); expect(mockClearCache).toHaveBeenCalled(); }); }); + describe('selectCollapsedGroup', () => { + it('should set activeCollapsedGroupId', () => { + useIterationStore.getState().selectCollapsedGroup('group-100'); + + expect(useIterationStore.getState().activeCollapsedGroupId).toBe('group-100'); + }); + + it('should replace previous activeCollapsedGroupId', () => { + useIterationStore.getState().selectCollapsedGroup('group-100'); + useIterationStore.getState().selectCollapsedGroup('group-200'); + + expect(useIterationStore.getState().activeCollapsedGroupId).toBe('group-200'); + }); + }); + + describe('clearCollapsedGroup', () => { + it('should reset activeCollapsedGroupId to null', () => { + useIterationStore.getState().selectCollapsedGroup('group-100'); + expect(useIterationStore.getState().activeCollapsedGroupId).toBe('group-100'); + + useIterationStore.getState().clearCollapsedGroup(); + expect(useIterationStore.getState().activeCollapsedGroupId).toBeNull(); + }); + }); + + describe('selectRange clears activeCollapsedGroupId', () => { + beforeEach(async () => { + const mockIterations = createMockIterations(3); + const mockArtifacts = createMockArtifacts(); + const mockDb = {}; + + mockLoad.mockResolvedValue({ + db: mockDb, + reference: createMockReference(), + }); + mockGetIterations.mockReturnValue(mockIterations); + mockGetAllArtifacts.mockReturnValue(mockArtifacts); + + await useIterationStore.getState().loadIterations('owner', 'repo', 1); + }); + + it('should clear activeCollapsedGroupId when selecting a range', () => { + useIterationStore.getState().selectCollapsedGroup('group-100'); + expect(useIterationStore.getState().activeCollapsedGroupId).toBe('group-100'); + + useIterationStore.getState().selectRange(0, 5); + expect(useIterationStore.getState().activeCollapsedGroupId).toBeNull(); + }); + }); + + describe('reset clears activeCollapsedGroupId', () => { + it('should clear activeCollapsedGroupId on reset', () => { + useIterationStore.getState().selectCollapsedGroup('group-100'); + expect(useIterationStore.getState().activeCollapsedGroupId).toBe('group-100'); + + useIterationStore.getState().reset(); + expect(useIterationStore.getState().activeCollapsedGroupId).toBeNull(); + }); + }); + + describe('toggleCollapsedGroupVisibility', () => { + it('should toggle visibility from collapsed to expanded', () => { + useIterationStore.setState({ + collapsedGroups: [{ + forcePushEventId: '100', + discardedRevisions: [1], + commits: [], + reason: 'force_push', + visibility: 'collapsed', + }], + }); + + useIterationStore.getState().toggleCollapsedGroupVisibility('100'); + + const group = useIterationStore.getState().collapsedGroups[0]; + expect(group).toBeDefined(); + expect(group?.visibility).toBe('expanded'); + }); + + it('should toggle visibility from expanded to collapsed', () => { + useIterationStore.setState({ + collapsedGroups: [{ + forcePushEventId: '100', + discardedRevisions: [1], + commits: [], + reason: 'force_push', + visibility: 'expanded', + }], + }); + + useIterationStore.getState().toggleCollapsedGroupVisibility('100'); + + const group = useIterationStore.getState().collapsedGroups[0]; + expect(group).toBeDefined(); + expect(group?.visibility).toBe('collapsed'); + }); + + it('should clear activeCollapsedGroupId when toggling', () => { + useIterationStore.setState({ + activeCollapsedGroupId: '100', + collapsedGroups: [{ + forcePushEventId: '100', + discardedRevisions: [1], + commits: [], + reason: 'force_push', + visibility: 'collapsed', + }], + }); + + useIterationStore.getState().toggleCollapsedGroupVisibility('100'); + + expect(useIterationStore.getState().activeCollapsedGroupId).toBeNull(); + }); + }); + describe('selectSelectedRange selector', () => { it('should return null when no PR is loaded', () => { const state = useIterationStore.getState(); diff --git a/src/features/iterations/stores/useIterationStore.ts b/src/features/iterations/stores/useIterationStore.ts index fc14a1a3..add4c1dd 100644 --- a/src/features/iterations/stores/useIterationStore.ts +++ b/src/features/iterations/stores/useIterationStore.ts @@ -74,6 +74,9 @@ interface IterationState { currentPrKey: string | null; selectedRanges: { [key: string]: IterationRange }; + // Collapsed group history view + activeCollapsedGroupId: string | null; + // Services (not persisted) client: IterationClient | null; spanTrackerService: SpanTrackerService | null; @@ -90,6 +93,10 @@ interface IterationState { loadIterations: (owner: string, repo: string, prNumber: number) => Promise; selectRange: (fromSnapshot: number, toSnapshot: number) => void; selectPreset: (preset: IterationPreset) => void; + selectCollapsedGroup: (groupId: string) => void; + /** Available for future "dismiss without expanding" UX */ + clearCollapsedGroup: () => void; + toggleCollapsedGroupVisibility: (groupId: string) => void; getSpanTrackerService: () => SpanTrackerService | null; reset: () => void; } @@ -113,6 +120,7 @@ const initialState = { artifactReference: null, currentPrKey: null, selectedRanges: {}, + activeCollapsedGroupId: null, client: null, spanTrackerService: null, isLoading: false, @@ -136,22 +144,98 @@ export const useIterationStore = create()( set({ isLoading: true, error: null, currentPrKey: prKey }); try { - const loader = new ArtifactLoader(owner, repo, prNumber); + // Check for ?mode=stateless query parameter to bypass artifact loading + const forceStateless = typeof window !== 'undefined' + && new URLSearchParams(window.location.search).get('mode') === 'stateless'; + + // Try loading artifact (unless forced stateless) + if (!forceStateless) { + const loader = new ArtifactLoader(owner, repo, prNumber); + + // Get artifact reference early (from PR comment) so UI can show iteration count + const earlyReference = await loader.findArtifactReference(); + if (earlyReference) { + set({ artifactReference: earlyReference }); + } - // Get artifact reference early (from PR comment) so UI can show iteration count - const earlyReference = await loader.findArtifactReference(); - if (earlyReference) { - set({ artifactReference: earlyReference }); - } + const result = await loader.load(earlyReference ?? undefined); + + if (result) { + const { db, reference } = result; + + // Create iteration client + const client = new IterationClient(db); - const result = await loader.load(earlyReference ?? undefined); + // Load iterations and artifacts + const iterations = client.getIterations(); + const artifacts = client.getAllArtifacts(); - if (!result) { - // Determine reason for stateless mode - const hasArtifactReference = earlyReference !== null; + // Create SpanTracker service + const spanTrackerReader = new SQLiteSpanTrackerReader(db); + const spanTrackerService = new SpanTrackerService(spanTrackerReader); + + console.info( + `[CodjiFlo] Loaded ${iterations.length} iteration(s) and ${artifacts.length} artifact(s) for ${prKey}` + ); + + // Set default selection (full diff: base to latest) + const latestIteration = iterations[iterations.length - 1]; + const defaultRange: IterationRange | null = latestIteration + ? { + fromSnapshot: iterationToLeftSnapshot(latestIteration.revision), + toSnapshot: iterationToRightSnapshot(latestIteration.revision), + } + : null; + + const { selectedRanges } = get(); + const cachedRange = selectedRanges[prKey]; + + const maxValidSnapshot = latestIteration + ? iterationToRightSnapshot(latestIteration.revision) + : 0; + const latestLeftSnapshot = latestIteration + ? iterationToLeftSnapshot(latestIteration.revision) + : 0; + const isCachedRangeValid = + cachedRange !== undefined && + cachedRange.fromSnapshot >= 0 && + cachedRange.toSnapshot <= maxValidSnapshot && + cachedRange.fromSnapshot < cachedRange.toSnapshot && + !(cachedRange.fromSnapshot === 0 && latestLeftSnapshot > 0); + + const rangeToUse = isCachedRangeValid ? cachedRange : defaultRange; + + const newSelectedRanges = rangeToUse + ? updateLRUCache(selectedRanges, prKey, rangeToUse) + : selectedRanges; + + set({ + iterations, + collapsedGroups: [], + artifacts, + artifactTimestamp: reference.timestamp, + artifactReference: reference, + selectedRanges: newSelectedRanges, + client, + spanTrackerService, + isLoading: false, + mode: 'stateful', + statelessReason: null, + error: null, + }); + return; + } + } + + // Stateless mode: no artifact available or forced via query param + let reason: string; + if (forceStateless) { + reason = 'Stateless mode forced via ?mode=stateless query parameter.'; + console.info(`[CodjiFlo] Entering stateless mode: forced via query param for ${prKey}`); + } else { + const hasArtifactReference = get().artifactReference !== null; const isAuthenticated = useAuthStore.getState().token !== null; - let reason: string; if (hasArtifactReference && !isAuthenticated) { reason = 'Sign in to enable iteration tracking. CodjiFlo data is available for this PR.'; console.info(`[CodjiFlo] Entering stateless mode: not authenticated for ${prKey}`); @@ -159,136 +243,61 @@ export const useIterationStore = create()( reason = 'No CodjiFlo artifact found. The repository may not have the CodjiFlo GitHub Action installed.'; console.info(`[CodjiFlo] Entering stateless mode: no artifact found for ${prKey}`); } + } - // Load iterations from GitHub Commits + Timeline APIs - try { - const prData = await githubClient.fetch<{ base: { sha: string } }>( - `/repos/${owner}/${repo}/pulls/${String(prNumber)}` + // Load iterations from GitHub Commits + Timeline APIs + try { + const prData = await githubClient.fetch<{ base: { sha: string } }>( + `/repos/${owner}/${repo}/pulls/${String(prNumber)}` + ); + const timelineLoader = new TimelineLoader(owner, repo, prNumber, prData.base.sha); + const timelineResult = await timelineLoader.load(); + + if (timelineResult.iterations.length > 0) { + const latestLiveIteration = [...timelineResult.iterations] + .reverse() + .find(i => i.status === 'live'); + const latestIteration = latestLiveIteration ?? timelineResult.iterations[timelineResult.iterations.length - 1]; + + const defaultRange = latestIteration + ? { + fromSnapshot: iterationToLeftSnapshot(latestIteration.revision), + toSnapshot: iterationToRightSnapshot(latestIteration.revision), + } + : null; + + const { selectedRanges } = get(); + const newSelectedRanges = defaultRange + ? updateLRUCache(selectedRanges, prKey, defaultRange) + : selectedRanges; + + console.info( + `[CodjiFlo] Stateless mode: loaded ${String(timelineResult.iterations.length)} iteration(s) ` + + `(${String(timelineResult.collapsedGroups.length)} collapsed group(s)) for ${prKey}` ); - const timelineLoader = new TimelineLoader(owner, repo, prNumber, prData.base.sha); - const timelineResult = await timelineLoader.load(); - - if (timelineResult.iterations.length > 0) { - const latestLiveIteration = [...timelineResult.iterations] - .reverse() - .find(i => i.status === 'live'); - const latestIteration = latestLiveIteration ?? timelineResult.iterations[timelineResult.iterations.length - 1]; - - const defaultRange = latestIteration - ? { - fromSnapshot: iterationToLeftSnapshot(latestIteration.revision), - toSnapshot: iterationToRightSnapshot(latestIteration.revision), - } - : null; - - const { selectedRanges } = get(); - const newSelectedRanges = defaultRange - ? updateLRUCache(selectedRanges, prKey, defaultRange) - : selectedRanges; - - console.info( - `[CodjiFlo] Stateless mode: loaded ${String(timelineResult.iterations.length)} iteration(s) ` + - `(${String(timelineResult.collapsedGroups.length)} collapsed group(s)) for ${prKey}` - ); - - set({ - isLoading: false, - mode: 'stateless', - statelessReason: reason, - iterations: timelineResult.iterations, - collapsedGroups: timelineResult.collapsedGroups, - artifacts: [], - selectedRanges: newSelectedRanges, - }); - return; - } - } catch (timelineError) { - console.warn('[CodjiFlo] Failed to load stateless iterations:', timelineError); - // Fall through to empty stateless mode - } - set({ - isLoading: false, - mode: 'stateless', - statelessReason: reason, - iterations: [], - collapsedGroups: [], - artifacts: [], - }); - return; + set({ + isLoading: false, + mode: 'stateless', + statelessReason: reason, + iterations: timelineResult.iterations, + collapsedGroups: timelineResult.collapsedGroups, + artifacts: [], + selectedRanges: newSelectedRanges, + }); + return; + } + } catch (timelineError) { + console.warn('[CodjiFlo] Failed to load stateless iterations:', timelineError); } - const { db, reference } = result; - - // Create iteration client - const client = new IterationClient(db); - - // Load iterations and artifacts - const iterations = client.getIterations(); - const artifacts = client.getAllArtifacts(); - - // Create SpanTracker service - const spanTrackerReader = new SQLiteSpanTrackerReader(db); - const spanTrackerService = new SpanTrackerService(spanTrackerReader); - - console.info( - `[CodjiFlo] Loaded ${iterations.length} iteration(s) and ${artifacts.length} artifact(s) for ${prKey}` - ); - - // Set default selection (full diff: base to latest) - // Uses the base snapshot of the latest iteration to handle rebases correctly. - // After a rebase, the latest iteration's left snapshot contains the new base, - // while snapshot 0 would contain the stale old base. - const latestIteration = iterations[iterations.length - 1]; - const defaultRange: IterationRange | null = latestIteration - ? { - fromSnapshot: iterationToLeftSnapshot(latestIteration.revision), - toSnapshot: iterationToRightSnapshot(latestIteration.revision), - } - : null; - - // Check if we have a cached range for this specific PR - const { selectedRanges } = get(); - const cachedRange = selectedRanges[prKey]; - - // Validate cached range against current PR's iterations - const maxValidSnapshot = latestIteration - ? iterationToRightSnapshot(latestIteration.revision) - : 0; - const latestLeftSnapshot = latestIteration - ? iterationToLeftSnapshot(latestIteration.revision) - : 0; - const isCachedRangeValid = - cachedRange !== undefined && - cachedRange.fromSnapshot >= 0 && - cachedRange.toSnapshot <= maxValidSnapshot && - cachedRange.fromSnapshot < cachedRange.toSnapshot && - // Invalidate cached "full diff" if a rebase occurred: - // If cached fromSnapshot is 0 but latest iteration's left snapshot is different, - // the base has changed (rebase) and we should use the new base instead. - !(cachedRange.fromSnapshot === 0 && latestLeftSnapshot > 0); - - // Use cached range if valid, otherwise use default - const rangeToUse = isCachedRangeValid ? cachedRange : defaultRange; - - // Update selectedRanges with LRU eviction - const newSelectedRanges = rangeToUse - ? updateLRUCache(selectedRanges, prKey, rangeToUse) - : selectedRanges; - set({ - iterations, - collapsedGroups: [], - artifacts, - artifactTimestamp: reference.timestamp, - artifactReference: reference, - selectedRanges: newSelectedRanges, - client, - spanTrackerService, isLoading: false, - mode: 'stateful', - statelessReason: null, - error: null, + mode: 'stateless', + statelessReason: reason, + iterations: [], + collapsedGroups: [], + artifacts: [], }); } catch (err) { const message = err instanceof Error ? err.message : 'Failed to load iterations'; @@ -313,6 +322,7 @@ export const useIterationStore = create()( set({ selectedRanges: updateLRUCache(selectedRanges, currentPrKey, { fromSnapshot, toSnapshot }), + activeCollapsedGroupId: null, }); }, @@ -378,6 +388,27 @@ export const useIterationStore = create()( set({ selectedRanges: updateLRUCache(selectedRanges, currentPrKey, newRange), + activeCollapsedGroupId: null, + }); + }, + + selectCollapsedGroup: (groupId: string) => { + set({ activeCollapsedGroupId: groupId }); + }, + + clearCollapsedGroup: () => { + set({ activeCollapsedGroupId: null }); + }, + + toggleCollapsedGroupVisibility: (groupId: string) => { + const { collapsedGroups } = get(); + set({ + collapsedGroups: collapsedGroups.map(g => + g.forcePushEventId === groupId + ? { ...g, visibility: g.visibility === 'collapsed' ? 'expanded' as const : 'collapsed' as const } + : g + ), + activeCollapsedGroupId: null, }); }, diff --git a/src/features/iterations/timeline-loader.test.ts b/src/features/iterations/timeline-loader.test.ts index 49fa1feb..e85ab6f0 100644 --- a/src/features/iterations/timeline-loader.test.ts +++ b/src/features/iterations/timeline-loader.test.ts @@ -47,13 +47,24 @@ function makeCommit(sha: string, date: string, author = 'testuser', message = `c }; } -function makeForcePushEvent(id: number, beforeSha: string, afterSha: string, createdAt: string) { +function makeForcePushEvent(id: number, _beforeSha: string, afterSha: string, createdAt: string) { + // Real GitHub API only provides commit_id (the new HEAD after force-push). + // The beforeSha is inferred at runtime by walking committed events in the timeline. return { id, event: 'head_ref_force_pushed', created_at: createdAt, - before_commit: { sha: beforeSha }, - after_commit: { sha: afterSha }, + commit_id: afterSha, + }; +} + +/** Create a committed event (used to track current HEAD for force-push before-SHA inference) */ +function makeCommittedEvent(sha: string, createdAt: string) { + return { + id: 0, + event: 'committed', + created_at: createdAt, + sha, }; } @@ -215,13 +226,14 @@ describe('TimelineLoader', () => { describe('PR with force-push (discoverable discarded commits)', () => { it('builds collapsed iterations from discarded commits', async () => { - // Timeline: force-push replaced old-sha with new-sha + // Timeline: committed event establishes HEAD, then force-push replaces it // Compare API discovers 2 discarded commits mockFetch .mockResolvedValueOnce([ // commits (current) makeCommit('new111', '2024-01-03T10:00:00Z'), ]) .mockResolvedValueOnce([ // timeline + makeCommittedEvent('old-sha', '2024-01-01T12:00:00Z'), makeForcePushEvent(1001, 'old-sha', 'new-sha', '2024-01-02T12:00:00Z'), ]) .mockResolvedValueOnce( // compare: afterSha...beforeSha -> discarded commits @@ -273,7 +285,10 @@ describe('TimelineLoader', () => { it('collapsed group unknownCount is NOT set for discoverable commits', async () => { mockFetch .mockResolvedValueOnce([makeCommit('new1', '2024-01-02T10:00:00Z')]) - .mockResolvedValueOnce([makeForcePushEvent(100, 'old', 'new', '2024-01-01T12:00:00Z')]) + .mockResolvedValueOnce([ + makeCommittedEvent('old', '2024-01-01T10:00:00Z'), + makeForcePushEvent(100, 'old', 'new', '2024-01-01T12:00:00Z'), + ]) .mockResolvedValueOnce(makeCompareResult([ { sha: 'disc', date: '2024-01-01T10:00:00Z' }, ])); @@ -292,6 +307,7 @@ describe('TimelineLoader', () => { makeCommit('new111', '2024-01-03T10:00:00Z'), ]) .mockResolvedValueOnce([ // timeline + makeCommittedEvent('gc-sha', '2024-01-01T12:00:00Z'), makeForcePushEvent(2001, 'gc-sha', 'new-sha', '2024-01-02T12:00:00Z'), ]) .mockRejectedValueOnce( // compare returns 404 @@ -321,6 +337,7 @@ describe('TimelineLoader', () => { mockFetch .mockResolvedValueOnce([makeCommit('new111', '2024-01-03T10:00:00Z')]) .mockResolvedValueOnce([ + makeCommittedEvent('deleted-sha', '2024-01-01T12:00:00Z'), makeForcePushEvent(3001, 'deleted-sha', 'new-sha', '2024-01-02T12:00:00Z'), ]) .mockRejectedValueOnce( @@ -343,8 +360,10 @@ describe('TimelineLoader', () => { makeCommit('final-1', '2024-01-05T10:00:00Z'), makeCommit('final-2', '2024-01-06T10:00:00Z'), ]) - .mockResolvedValueOnce([ // timeline: two force-pushes + .mockResolvedValueOnce([ // timeline: committed events + two force-pushes + makeCommittedEvent('old-before-1', '2024-01-01T10:00:00Z'), makeForcePushEvent(100, 'old-before-1', 'after-1', '2024-01-02T10:00:00Z'), + makeCommittedEvent('gc-before', '2024-01-03T10:00:00Z'), makeForcePushEvent(200, 'gc-before', 'after-2', '2024-01-04T10:00:00Z'), ]) .mockResolvedValueOnce( // first compare: discoverable @@ -380,8 +399,10 @@ describe('TimelineLoader', () => { .mockResolvedValueOnce([ // commits (current) makeCommit('latest', '2024-01-10T10:00:00Z'), ]) - .mockResolvedValueOnce([ // timeline: two force-pushes in chronological order + .mockResolvedValueOnce([ // timeline: committed events + two force-pushes + makeCommittedEvent('old-1', '2024-01-02T10:00:00Z'), makeForcePushEvent(100, 'old-1', 'after-1', '2024-01-03T10:00:00Z'), + makeCommittedEvent('old-2', '2024-01-05T10:00:00Z'), makeForcePushEvent(200, 'old-2', 'after-2', '2024-01-06T10:00:00Z'), ]) .mockResolvedValueOnce( // first compare @@ -431,6 +452,7 @@ describe('TimelineLoader', () => { mockFetch .mockResolvedValueOnce([makeCommit('curr', '2024-01-02T10:00:00Z')]) .mockResolvedValueOnce([ + makeCommittedEvent('before-abc', '2024-01-01T10:00:00Z'), makeForcePushEvent(1, 'before-abc', 'after-def', '2024-01-01T12:00:00Z'), ]) .mockResolvedValueOnce(makeCompareResult([])); @@ -446,6 +468,7 @@ describe('TimelineLoader', () => { mockFetch .mockResolvedValueOnce([makeCommit('c1', '2024-01-01T10:00:00Z')]) .mockResolvedValueOnce([ + makeCommittedEvent('b', '2024-01-01T10:00:00Z'), makeForcePushEvent(1, 'b', 'a', '2024-01-01T12:00:00Z'), ]) .mockResolvedValueOnce(makeCompareResult([])); @@ -483,16 +506,14 @@ describe('TimelineLoader', () => { expect(at(result.iterations, 0).status).toBe('live'); }); - it('ignores force-push events missing before_commit or after_commit', async () => { + it('ignores force-push events missing commit_id', async () => { mockFetch .mockResolvedValueOnce([makeCommit('aaa', '2024-01-01T10:00:00Z')]) .mockResolvedValueOnce([ - // Missing before_commit - { id: 1, event: 'head_ref_force_pushed', created_at: '2024-01-01T10:00:00Z', after_commit: { sha: 'x' } }, - // Missing after_commit - { id: 2, event: 'head_ref_force_pushed', created_at: '2024-01-01T11:00:00Z', before_commit: { sha: 'y' } }, - // Missing both - { id: 3, event: 'head_ref_force_pushed', created_at: '2024-01-01T12:00:00Z' }, + // Force-push event without commit_id — should be ignored + { id: 1, event: 'head_ref_force_pushed', created_at: '2024-01-01T10:00:00Z' }, + // Another without commit_id + { id: 2, event: 'head_ref_force_pushed', created_at: '2024-01-01T11:00:00Z' }, ]); const loader = new TimelineLoader('o', 'r', 1, 'base'); @@ -502,6 +523,23 @@ describe('TimelineLoader', () => { expect(mockFetch).toHaveBeenCalledTimes(2); expect(result.collapsedGroups).toHaveLength(0); }); + + it('creates unknown-count group when no committed event precedes force-push', async () => { + // Force-push with commit_id but no prior committed event to infer before SHA + mockFetch + .mockResolvedValueOnce([makeCommit('aaa', '2024-01-02T10:00:00Z')]) + .mockResolvedValueOnce([ + makeForcePushEvent(99, '', 'new-head', '2024-01-01T10:00:00Z'), + ]); + + const loader = new TimelineLoader('o', 'r', 1, 'base'); + const result = await loader.load(); + + // No compare API call since before SHA is unknown + expect(mockFetch).toHaveBeenCalledTimes(2); + expect(result.collapsedGroups).toHaveLength(1); + expect(at(result.collapsedGroups, 0).unknownCount).toBe(true); + }); }); describe('error propagation', () => { @@ -509,6 +547,7 @@ describe('TimelineLoader', () => { mockFetch .mockResolvedValueOnce([makeCommit('aaa', '2024-01-01T10:00:00Z')]) .mockResolvedValueOnce([ + makeCommittedEvent('before', '2024-01-01T08:00:00Z'), makeForcePushEvent(1, 'before', 'after', '2024-01-01T10:00:00Z'), ]) .mockRejectedValueOnce( @@ -523,6 +562,7 @@ describe('TimelineLoader', () => { mockFetch .mockResolvedValueOnce([makeCommit('aaa', '2024-01-01T10:00:00Z')]) .mockResolvedValueOnce([ + makeCommittedEvent('before', '2024-01-01T08:00:00Z'), makeForcePushEvent(1, 'before', 'after', '2024-01-01T10:00:00Z'), ]) .mockRejectedValueOnce( @@ -557,6 +597,7 @@ describe('TimelineLoader', () => { mockFetch .mockResolvedValueOnce([makeCommit('aaa', '2024-01-01T10:00:00Z')]) .mockResolvedValueOnce([ + makeCommittedEvent('before', '2024-01-01T08:00:00Z'), makeForcePushEvent(1, 'before', 'after', '2024-01-01T10:00:00Z'), ]) .mockRejectedValueOnce(new Error('Network failed')); @@ -572,6 +613,7 @@ describe('TimelineLoader', () => { mockFetch .mockResolvedValueOnce([makeCommit('new1', '2024-01-02T10:00:00Z')]) .mockResolvedValueOnce([ + makeCommittedEvent('old', '2024-01-01T10:00:00Z'), makeForcePushEvent(100, 'old', 'new', '2024-01-01T12:00:00Z'), ]) .mockResolvedValueOnce(makeCompareResult([])); // empty commits @@ -611,6 +653,7 @@ describe('TimelineLoader', () => { mockFetch .mockResolvedValueOnce([makeCommit('live1', '2024-01-05T10:00:00Z')]) .mockResolvedValueOnce([ + makeCommittedEvent('old', '2024-01-02T10:00:00Z'), makeForcePushEvent(777, 'old', 'new', '2024-01-03T10:00:00Z'), ]) .mockResolvedValueOnce(makeCompareResult([ @@ -647,7 +690,10 @@ describe('TimelineLoader', () => { it('collapsed group forcePushEventId is string even when event id is numeric', async () => { mockFetch .mockResolvedValueOnce([makeCommit('live', '2024-01-02T10:00:00Z')]) - .mockResolvedValueOnce([makeForcePushEvent(42, 'old', 'new', '2024-01-01T10:00:00Z')]) + .mockResolvedValueOnce([ + makeCommittedEvent('old', '2024-01-01T08:00:00Z'), + makeForcePushEvent(42, 'old', 'new', '2024-01-01T10:00:00Z'), + ]) .mockResolvedValueOnce(makeCompareResult([ { sha: 'disc', date: '2024-01-01T08:00:00Z' }, ])); diff --git a/src/features/iterations/timeline-loader.ts b/src/features/iterations/timeline-loader.ts index 9d79f3c9..9cd687e7 100644 --- a/src/features/iterations/timeline-loader.ts +++ b/src/features/iterations/timeline-loader.ts @@ -33,8 +33,10 @@ interface TimelineEvent { id: number; event: string; created_at: string; - before_commit?: { sha: string }; - after_commit?: { sha: string }; + /** SHA of the new HEAD after force-push (real GitHub API field) */ + commit_id?: string; + /** SHA from committed events in timeline */ + sha?: string; } interface CompareResult { @@ -140,6 +142,48 @@ export class TimelineLoader { } } + // -------------------------------------------------------------------------- + // Force-Push SHA Inference + // -------------------------------------------------------------------------- + + /** + * Infer before/after SHAs for force-push events by walking the timeline + * chronologically. The real GitHub Timeline API's `head_ref_force_pushed` + * events only provide `commit_id` (the new HEAD after the force-push). + * We infer the "before" SHA by tracking the current HEAD via `committed` events. + * + * Events without a determinable before SHA are treated as GC'd (unknownCount). + */ + private inferForcePushSHAs( + timeline: TimelineEvent[] + ): { event: TimelineEvent; beforeSha: string | null; afterSha: string }[] { + const sorted = [...timeline].sort( + (a, b) => new Date(a.created_at).getTime() - new Date(b.created_at).getTime() + ); + + const result: { event: TimelineEvent; beforeSha: string | null; afterSha: string }[] = []; + let currentHead: string | null = null; + + for (const event of sorted) { + if (event.event === 'committed' && event.sha) { + currentHead = event.sha; + continue; + } + + if (event.event !== 'head_ref_force_pushed' || !event.commit_id) { + continue; + } + + const afterSha = event.commit_id; + result.push({ event, beforeSha: currentHead, afterSha }); + + // After force-push, the new HEAD is the after SHA + currentHead = afterSha; + } + + return result; + } + // -------------------------------------------------------------------------- // Iteration Building // -------------------------------------------------------------------------- @@ -151,24 +195,31 @@ export class TimelineLoader { const iterations: Iteration[] = []; const collapsedGroups: CollapsedIterationGroup[] = []; - // Step 1: Extract force-push events, sorted chronologically - const forcePushes = timeline - .filter((e): e is TimelineEvent & { before_commit: { sha: string }; after_commit: { sha: string } } => - e.event === 'head_ref_force_pushed' && - e.before_commit !== undefined && - e.after_commit !== undefined - ) - .sort((a, b) => new Date(a.created_at).getTime() - new Date(b.created_at).getTime()); + // Step 1: Infer before/after SHAs for force-push events by walking timeline + // chronologically. The real GitHub API only provides commit_id (the new HEAD + // after force-push). We track the current HEAD via committed events to infer + // what the HEAD was before each force-push. + const forcePushes = this.inferForcePushSHAs(timeline); // Step 2: Discover discarded commits for each force-push (sequential -- each needs API call) - for (const event of forcePushes) { - const discovery = await this.discoverDiscardedCommits( - event.after_commit.sha, - event.before_commit.sha - ); - + for (const { event, beforeSha, afterSha } of forcePushes) { const eventId = String(event.id); + // If we couldn't infer the before SHA, treat as unknown/GC'd + if (!beforeSha) { + collapsedGroups.push({ + forcePushEventId: eventId, + discardedRevisions: [], + commits: [], + reason: 'force_push', + visibility: 'collapsed', + unknownCount: true, + }); + continue; + } + + const discovery = await this.discoverDiscardedCommits(afterSha, beforeSha); + if (discovery.status === 'discovered') { const group: CollapsedIterationGroup = { forcePushEventId: eventId, @@ -184,7 +235,7 @@ export class TimelineLoader { revision: 0, // placeholder, assigned after sorting headSha: commit.sha, baseSha: this.baseSha, - beforeSha: event.before_commit.sha, + beforeSha, author: commit.author, createdAt: new Date(commit.date), status: 'collapsed', diff --git a/src/features/iterations/types.ts b/src/features/iterations/types.ts index cf0b60a5..11ee3da1 100644 --- a/src/features/iterations/types.ts +++ b/src/features/iterations/types.ts @@ -187,6 +187,9 @@ export interface IterationState { /** Selected ranges partitioned by PR key */ selectedRanges: { [key: string]: IterationRange }; + /** Active collapsed group ID for history view (not persisted) */ + activeCollapsedGroupId: string | null; + /** All file artifacts */ artifacts: ReviewFileArtifact[]; @@ -209,6 +212,10 @@ export interface IterationState { loadIterations: (owner: string, repo: string, prNumber: number) => Promise; selectRange: (fromSnapshot: number, toSnapshot: number) => void; selectPreset: (preset: IterationPreset) => void; + selectCollapsedGroup: (groupId: string) => void; + /** Available for future "dismiss without expanding" UX */ + clearCollapsedGroup: () => void; + toggleCollapsedGroupVisibility: (groupId: string) => void; reset: () => void; } diff --git a/src/styles/shared/features.css b/src/styles/shared/features.css index fd9c1c68..08c9e7a2 100644 --- a/src/styles/shared/features.css +++ b/src/styles/shared/features.css @@ -614,13 +614,75 @@ background-color: var(--tab-inactive); color: var(--watermark-text); opacity: 0.6; - cursor: default; + cursor: pointer; } .iteration-tab.collapsed svg { stroke: var(--watermark-text); } +/* Discarded iteration tab (expanded from collapsed group) */ +.iteration-tab.discarded { + opacity: 0.6; +} + +.iteration-tab.discarded:hover { + opacity: 0.8; +} + +/* Unavailable iteration tab (GC'd commit) */ +.iteration-tab.unavailable { + opacity: 0.4; + cursor: not-allowed; +} + +/* Collapsed iteration history view */ +.collapsed-history-view { + padding: 16px; + background-color: var(--control-bg); + border: 1px solid var(--combobox-border); + border-radius: 4px; + margin: 8px 0; +} + +.collapsed-history-header { + display: flex; + align-items: center; + gap: 8px; + margin-bottom: 12px; + font-weight: 600; + color: var(--fg); +} + +.collapsed-history-list { + display: flex; + flex-direction: column; + gap: 4px; + max-height: 300px; + overflow-y: auto; + margin-bottom: 12px; +} + +.collapsed-history-item { + display: flex; + align-items: center; + gap: 8px; + padding: 4px 8px; + font-size: 13px; + color: var(--fg); + border-radius: 3px; +} + +.collapsed-history-item.unavailable { + color: var(--watermark-text); + font-style: italic; +} + +.collapsed-history-actions { + display: flex; + gap: 8px; +} + /* Loading indicator */ .iteration-loading { display: flex;