diff --git a/app/renderer/sections/PullRequests.test.tsx b/app/renderer/sections/PullRequests.test.tsx index 90b2911..da6b2b4 100644 --- a/app/renderer/sections/PullRequests.test.tsx +++ b/app/renderer/sections/PullRequests.test.tsx @@ -1,5 +1,5 @@ // @vitest-environment jsdom -import { render, screen } from '@testing-library/react' +import { render, screen, waitFor } from '@testing-library/react' import userEvent from '@testing-library/user-event' import { beforeEach, describe, expect, it, vi } from 'vitest' @@ -125,6 +125,23 @@ describe('PullRequests', () => { expect(screen.queryByText('Feature work')).toBeNull() }) + it('closes an open expansion when the period changes the PR set', async () => { + const changed: PrPayload = { ...SAMPLE, rows: [SAMPLE.rows[0]!] } // #781 dropped + getOverview.mockImplementation((period: string) => Promise.resolve(makePayload(period === 'lifetime' ? SAMPLE : changed))) + const { rerender } = render() + + const link = await screen.findByRole('link', { name: 'getagentseal/codeburn#780' }) + await userEvent.click(rowForLink(link)) + expect(rowForLink(link)).toHaveAttribute('aria-expanded', 'true') + + rerender() + // The new period drops #781, so the PR set changes and the stale expansion + // resets once the new data lands (wait for the breakdown to disappear). + await waitFor(() => expect(screen.queryByText('Feature work')).toBeNull()) + const link2 = await screen.findByRole('link', { name: 'getagentseal/codeburn#780' }) + expect(rowForLink(link2)).toHaveAttribute('aria-expanded', 'false') + }) + it('toggles expansion from the keyboard with Enter', async () => { getOverview.mockResolvedValue(makePayload(SAMPLE)) render() diff --git a/app/renderer/sections/PullRequests.tsx b/app/renderer/sections/PullRequests.tsx index 5ac54c5..74b64f4 100644 --- a/app/renderer/sections/PullRequests.tsx +++ b/app/renderer/sections/PullRequests.tsx @@ -1,5 +1,5 @@ import type { KeyboardEvent, MouseEvent } from 'react' -import { useState } from 'react' +import { useEffect, useState } from 'react' import { CliErrorPanel } from '../components/CliErrorPanel' import { EmptyNote } from '../components/EmptyState' @@ -83,6 +83,11 @@ function PullRequestsPage({ pullRequests, staleError }: { pullRequests?: PullReq function PrTable({ pullRequests }: { pullRequests: PullRequests }) { const { rows, distinctCost, distinctSessions, attributedCost, unattributedCost, otherPrCount, otherPrCost } = pullRequests const [expandedUrl, setExpandedUrl] = useState(null) + // Reset any open expansion when the PR set changes (a period/provider switch or + // a refresh that alters the list): a stale expandedUrl would otherwise linger + // pointing at a row that is no longer present. + const rowKey = rows.map(row => row.url).join('|') + useEffect(() => { setExpandedUrl(null) }, [rowKey]) // A new-attribution payload carries `attributedCost`; an older by-reference // payload omits it, so the rows are not summable and the footer must differ. @@ -118,7 +123,11 @@ function PrTable({ pullRequests }: { pullRequests: PullRequests }) { onToggle={() => setExpandedUrl(current => current === pr.url ? null : pr.url)} /> ))} - {otherCount > 0 && ( + + {otherCount > 0 && ( + // A muted summary line, kept out of the sorted rows: its cost is an + // aggregate of the capped-away PRs and can exceed a visible row. + Other ({otherCount.toLocaleString('en-US')} more PRs) @@ -128,8 +137,8 @@ function PrTable({ pullRequests }: { pullRequests: PullRequests }) { - )} - + + )} {summable ? ( diff --git a/app/renderer/styles/plain.css b/app/renderer/styles/plain.css index 5d813ff..ed45b6b 100644 --- a/app/renderer/styles/plain.css +++ b/app/renderer/styles/plain.css @@ -601,7 +601,8 @@ td:first-child { font-size: var(--fs-body); font-weight: var(--fw-body); } .pr-cat-name { overflow: hidden; color: var(--ink); font-weight: 560; text-overflow: ellipsis; white-space: nowrap; } .pr-cat-main strong { flex: 0 0 auto; font-family: var(--mono); font-size: 11.5px; font-variant-numeric: tabular-nums; } .pr-cat-empty { margin: 0; color: var(--mut2); font-size: var(--fs-meta); } -.pr-table td.pr-other-label { color: var(--mut2); font-weight: var(--fw-medium); text-align: left; } +.pr-table tfoot .pr-other-row td { border-top: 1px solid var(--line); color: var(--mut2); font-style: italic; } +.pr-table td.pr-other-label { color: var(--mut2); font-weight: var(--fw-medium); font-style: italic; text-align: left; } .pr-footnote { margin: 12px 2px 2px; color: var(--mut); font-size: var(--fs-meta); line-height: 1.55; } .pr-unattributed { margin: 4px 2px 2px; color: var(--mut2); font-size: var(--fs-meta); font-variant-numeric: tabular-nums; } .opt-waste { min-width: 0; } diff --git a/src/session-cache.ts b/src/session-cache.ts index 5372442..644458e 100644 --- a/src/session-cache.ts +++ b/src/session-cache.ts @@ -369,15 +369,14 @@ function validateCache(raw: unknown): raw is SessionCache { // file itself is never written or deleted (old binaries still own it). const V5_CACHE_FILE = 'session-cache.v5.json' -// v5-shaped validation: identical to validateCache but pinned to version 5. The -// per-turn schema is a superset (prRefs is optional), so a v5 file passes the -// section/file/turn validators unchanged. -function validateV5Cache(raw: unknown): raw is SessionCache { +// Lightweight top-level check: a version-5 cache with a providers object. The +// individual files are validated per-entry in adoptV5Cache so one corrupt entry +// cannot drop every valid expired-transcript PR session along with it. +function isV5CacheEnvelope(raw: unknown): raw is { version: number; providers: Record } { if (!raw || typeof raw !== 'object') return false const o = raw as Record - if (o['version'] !== 5) return false - if (!o['providers'] || typeof o['providers'] !== 'object' || Array.isArray(o['providers'])) return false - return Object.values(o['providers'] as Record).every(validateProviderSection) + return o['version'] === 5 + && !!o['providers'] && typeof o['providers'] === 'object' && !Array.isArray(o['providers']) } // One-time migration for the 5 -> 6 bump (per-turn prRefs capture). A fresh v6 @@ -386,24 +385,31 @@ function validateV5Cache(raw: unknown): raw is SessionCache { // Carry forward exactly the v5 entries whose source no longer exists AND that // carry prLinks (they can never re-parse, but they hold attributable PR spend); // present sources are intentionally dropped so they re-parse fresh under v6 and -// gain per-turn refs. Each carried section takes the CURRENT envFingerprint so -// the scan reuses it and appends the freshly-parsed present sources. The daily -// cache, which owns durable cost history, is not touched. +// gain per-turn refs. Each file is validated individually, so a single corrupt +// entry is skipped rather than discarding the whole cache. Each carried section +// takes the CURRENT envFingerprint so the scan reuses it and appends the +// freshly-parsed present sources. The daily cache (durable cost history) is not +// touched. async function adoptV5Cache(): Promise { try { const raw = await readFile(join(getCacheDir(), V5_CACHE_FILE), 'utf-8') const parsed = JSON.parse(raw) - if (!validateV5Cache(parsed)) return null + if (!isV5CacheEnvelope(parsed)) return null const migrated: SessionCache = { version: CACHE_VERSION, providers: {}, complete: false } for (const [provider, section] of Object.entries(parsed.providers)) { + if (!section || typeof section !== 'object') continue + const rawFiles = (section as Record)['files'] const files: Record = {} - for (const [path, file] of Object.entries(section.files)) { - if (!existsSync(path) && file.prLinks?.length) files[path] = file + if (rawFiles && typeof rawFiles === 'object' && !Array.isArray(rawFiles)) { + for (const [path, file] of Object.entries(rawFiles as Record)) { + if (!validateCachedFile(file)) continue + if (!existsSync(path) && file.prLinks?.length) files[path] = file + } } migrated.providers[provider] = { envFingerprint: computeEnvFingerprint(provider), files, - ...(section.durable ? { durable: true } : {}), + ...((section as Record)['durable'] ? { durable: true } : {}), } } return migrated diff --git a/src/sessions-report.ts b/src/sessions-report.ts index 783e2a4..90f707f 100644 --- a/src/sessions-report.ts +++ b/src/sessions-report.ts @@ -255,6 +255,7 @@ export function attributeSessionPrSpend(session: AttributableSession): SessionPr export function aggregateByPr(projects: ProjectSummary[]): PrRow[] { const byUrl = new Map; firstStarted: string; lastEnded: string models: Map; categories: Map }>() @@ -268,7 +269,7 @@ export function aggregateByPr(projects: ProjectSummary[]): PrRow[] { for (const [url, c] of perUrl) { if (c.cost === 0 && c.calls === 0 && c.savingsUSD === 0) continue const row = byUrl.get(url) ?? { - cost: 0, savingsUSD: 0, calls: 0, approx: false, + cost: 0, savingsUSD: 0, calls: 0, approx: false, legacyCost: 0, sessions: new Set(), firstStarted: session.firstTimestamp, lastEnded: session.lastTimestamp, models: new Map(), categories: new Map(), } @@ -276,7 +277,9 @@ export function aggregateByPr(projects: ProjectSummary[]): PrRow[] { row.savingsUSD += c.savingsUSD row.calls += c.calls row.sessions.add(sessionKey) - if (c.approx) row.approx = true + // A legacy (approx) contribution carries no per-turn categories; track its + // cost so a mixed row can reconcile its category breakdown to the total. + if (c.approx) { row.approx = true; row.legacyCost += c.cost } for (const [m, mc] of c.models) addToMap(row.models, m, mc) for (const [cat, cc] of c.categories) addToMap(row.categories, cat, cc) if (session.firstTimestamp < row.firstStarted) row.firstStarted = session.firstTimestamp @@ -288,13 +291,25 @@ export function aggregateByPr(projects: ProjectSummary[]): PrRow[] { return [...byUrl.entries()] .map(([url, r]) => { // Collapse raw model names to short display names, summing costs that map - // to the same short name, then order by attributed cost. + // to the same short name, then order by attributed cost (name asc breaks + // ties for a stable order) and cap at the top 4 to bound the payload. const shortCosts = new Map() for (const [raw, mc] of r.models) addToMap(shortCosts, getShortModelName(raw), mc) - const models = [...shortCosts.entries()].sort((a, b) => b[1] - a[1]).map(([name]) => name) + const models = [...shortCosts.entries()] + .sort((a, b) => b[1] - a[1] || a[0].localeCompare(b[0])) + .slice(0, 4) + .map(([name]) => name) const categories = [...r.categories.entries()] - .sort((a, b) => b[1] - a[1]) .map(([cat, cost]) => ({ name: CATEGORY_LABELS[cat as TaskCategory] ?? cat, cost })) + // Mixed row: live per-turn categories exist AND part of the row came from a + // legacy even-split (no turn data). Add a synthetic line for the legacy + // share so the expansion reconciles with the row cost instead of silently + // dropping it. A legacy-only row keeps no categories (it surfaces as "no + // per-turn detail"), so there is nothing to reconcile there. + if (categories.length > 0 && r.legacyCost > 0) { + categories.push({ name: 'Legacy estimate (no per-turn detail)', cost: r.legacyCost }) + } + categories.sort((a, b) => b.cost - a.cost || a.name.localeCompare(b.name)) return { url, label: shortenPrUrl(url), cost: r.cost, savingsUSD: r.savingsUSD, diff --git a/tests/session-cache-v5-adoption.test.ts b/tests/session-cache-v5-adoption.test.ts index 3d97c19..23807d3 100644 --- a/tests/session-cache-v5-adoption.test.ts +++ b/tests/session-cache-v5-adoption.test.ts @@ -93,4 +93,44 @@ describe('v5 -> v6 cache adoption of expired PR sessions', () => { // Model union is still surfaced on legacy rows. expect(rows[0]!.models.length).toBeGreaterThan(0) }) + + it('skips a corrupt v5 entry but still adopts a valid expired PR entry', async () => { + await loadPricing() + const goodPath = join(configDir, 'projects', 'good-proj', 'good.jsonl') + const badPath = join(configDir, 'projects', 'bad-proj', 'bad.jsonl') + const v5 = { + version: 5, + complete: true, + providers: { + claude: { + envFingerprint: 'stale-v5', + files: { + // Corrupt: malformed turns fail validateCachedFile and are skipped. + [badPath]: { + fingerprint: { dev: 9, ino: 9, mtimeMs: 9, sizeBytes: 9 }, + mcpInventory: [], + prLinks: ['https://github.com/o/r/pull/9'], + turns: [{ garbage: true }], + }, + // Valid expired PR entry that must survive alongside the corrupt one. + [goodPath]: { + fingerprint: { dev: 1, ino: 2, mtimeMs: 3, sizeBytes: 4 }, + mcpInventory: [], + canonicalCwd: '/good/proj', + canonicalProjectName: 'good-proj', + prLinks: ['https://github.com/o/r/pull/1'], + turns: [{ timestamp: '2026-07-20T10:00:00.000Z', sessionId: 'good', userMessage: 'work', calls: [cachedCall('gk1', 40)] }], + }, + }, + }, + }, + } + await writeFile(join(cacheDir, 'session-cache.v5.json'), JSON.stringify(v5)) + + const range = { start: new Date('2026-07-20T00:00:00Z'), end: new Date('2026-07-20T23:59:59Z') } + const rows = aggregateByPr(await parseAllSessions(range, 'claude')) + const urls = rows.map(r => r.url) + expect(urls).toContain('https://github.com/o/r/pull/1') // valid entry survives + expect(urls).not.toContain('https://github.com/o/r/pull/9') // corrupt entry skipped, not fatal + }) }) diff --git a/tests/sessions-by-pr.test.ts b/tests/sessions-by-pr.test.ts index 082d1de..e882ee7 100644 --- a/tests/sessions-by-pr.test.ts +++ b/tests/sessions-by-pr.test.ts @@ -55,6 +55,15 @@ function sessionWithTurns(id: string, prLinks: string[], turns: ClassifiedTurn[] return { ...session(id, 0, 0, prLinks, first, last), turns } } +// A turn referencing `prRefs` whose calls each carry a [model, cost] pair. +function turnModels(prRefs: string[], calls: Array<[string, number]>): ClassifiedTurn { + return { + userMessage: '', timestamp: '2026-07-01T10:00:00Z', sessionId: 's', + category: 'coding', retries: 0, hasEdits: false, prRefs, + assistantCalls: calls.map(([model, cost]) => ({ ...call(cost), model })), + } +} + function project(sessions: SessionSummary[]): ProjectSummary { return { project: 'p', projectPath: '/p', sessions, totalCostUSD: 0, totalSavingsUSD: 0, totalApiCalls: 0, totalProxiedCostUSD: 0 } } @@ -304,6 +313,43 @@ describe('models + categories attribution', () => { }) }) +describe('mixed live + legacy category reconciliation (round-3 finding 1)', () => { + it('adds a synthetic legacy line so a mixed row reconciles to its cost', () => { + const rows = aggregateByPr([project([ + sessionWithTurns('live', [A], [cturn(10, 1, [A])]), // live: Coding $10 + session('legacy', 90, 5, [A]), // legacy: $90, no turn data + ])]) + const row = rows.find(r => r.url === A)! + expect(row.cost).toBeCloseTo(100, 6) + expect(row.approx).toBe(true) + const cats = row.categories! + expect(cats.reduce((s, c) => s + c.cost, 0)).toBeCloseTo(row.cost, 6) // reconciles + expect(cats.find(c => c.name === 'Coding')!.cost).toBeCloseTo(10, 6) + expect(cats.find(c => c.name === 'Legacy estimate (no per-turn detail)')!.cost).toBeCloseTo(90, 6) + }) + + it('a legacy-only row still omits categories (surfaces the no-detail note)', () => { + const rows = aggregateByPr([project([session('legacy', 90, 5, [A])])]) + expect(rows[0]!.categories).toBeUndefined() + }) +}) + +describe('model list bounds (round-3 finding 5)', () => { + it('caps a row to the top 4 models by attributed cost', () => { + const rows = aggregateByPr([project([ + sessionWithTurns('s', [A], [turnModels([A], [['m-f', 60], ['m-e', 50], ['m-d', 40], ['m-c', 30], ['m-b', 20], ['m-a', 10]])]), + ])]) + expect(rows[0]!.models).toEqual(['m-f', 'm-e', 'm-d', 'm-c']) + }) + + it('breaks model ties by name ascending for a stable order', () => { + const rows = aggregateByPr([project([ + sessionWithTurns('s', [A], [turnModels([A], [['m-d', 10], ['m-a', 10], ['m-c', 10], ['m-b', 10]])]), + ])]) + expect(rows[0]!.models).toEqual(['m-a', 'm-b', 'm-c', 'm-d']) + }) +}) + describe('distinct-session keying (finding 7)', () => { it('counts two same-sessionId sessions in different projects as two', () => { const s1 = sessionWithTurns('same-id', [A], [cturn(10, 1, [A])])