From 8390d278dfc95ace4adcac8d612173433fd658ba Mon Sep 17 00:00:00 2001 From: David Andrews Date: Fri, 26 Jun 2026 15:27:57 -0400 Subject: [PATCH] fix(stats/gain): address review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Parse minimizer JSONL once per request; derive records/unparsed/projects sets in a single pass instead of three separate reads+parse loops - Use getTimeRangeConfig from aggregator.ts (export it) instead of a local RANGE_MS table that would drift on new range additions - Fix startsWith project filter to be separator-aware (=== project || startsWith(project+'/')) via shared matchesProject helper — prevents false matches on path prefix collisions across all three call sites - Remove author-specific paths (PycharmProjects, .herdr) from normalizeProjectPath; replace with generic worktree-suffix regex covering .wt/, -wt/, .worktrees/, -worktrees/, and /.*/worktrees/ layout patterns, keeping /.omp/wt/ drop - Exclude snapcompact from project-scoped responses (no cwd field means no project filter is possible — mixing all-projects savings into a per-project view is misleading); global view still shows snapcompact - Fix overall.reductionPercent to only divide minimizer+distill saved bytes by minimizer+distill original bytes; snapcompact has no originalBytes so including it in the numerator overstated the ratio - Key unparsed command map on the full command string; command field in GainUnparsedCommand now stores the full string (callers truncate for display) to avoid merging distinct long commands sharing a 120-char prefix - Fix api.ts getGainDashboardStats arg order: project before signal to match sibling convention; update GainRoute.tsx call site accordingly - Add CHANGELOG entry under [Unreleased] --- packages/stats/CHANGELOG.md | 4 + packages/stats/src/aggregator.ts | 2 +- packages/stats/src/client/api.ts | 2 +- packages/stats/src/client/app/routes.ts | 10 +- .../stats/src/client/routes/GainRoute.tsx | 44 ++-- packages/stats/src/gain-aggregator.ts | 216 +++++++++--------- packages/stats/src/index.ts | 10 +- packages/stats/src/server.ts | 2 +- 8 files changed, 156 insertions(+), 134 deletions(-) diff --git a/packages/stats/CHANGELOG.md b/packages/stats/CHANGELOG.md index 2b93e84a6..2a1a678a0 100644 --- a/packages/stats/CHANGELOG.md +++ b/packages/stats/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Added + +- Gain tab in `omp stats` dashboard (`/#/gain`) — surfaces bash minimizer, snapcompact, and pi-distill token-savings with project scoping and an Unparsed Commands panel listing unhandled bash commands as filter-tuning candidates ([#3543](https://github.com/can1357/oh-my-pi/pull/3543)). + ## [16.1.17] - 2026-06-24 ### Fixed diff --git a/packages/stats/src/aggregator.ts b/packages/stats/src/aggregator.ts index aad16f956..7c1b337fa 100644 --- a/packages/stats/src/aggregator.ts +++ b/packages/stats/src/aggregator.ts @@ -330,7 +330,7 @@ const TIME_RANGE_TO_CONFIG: Record> = }, }; -function getTimeRangeConfig(range?: string | null): TimeRangeConfig { +export function getTimeRangeConfig(range?: string | null): TimeRangeConfig { const normalized = range?.trim().toLowerCase() ?? DEFAULT_TIME_RANGE; const config = TIME_RANGE_TO_CONFIG[normalized as TimeRange]; if (config) { diff --git a/packages/stats/src/client/api.ts b/packages/stats/src/client/api.ts index 8aa458ee6..07caed2ba 100644 --- a/packages/stats/src/client/api.ts +++ b/packages/stats/src/client/api.ts @@ -85,8 +85,8 @@ export async function getFolderStats(range: TimeRange = "24h", signal?: AbortSig export async function getGainDashboardStats( range: TimeRange = "24h", - signal?: AbortSignal, project?: string | null, + signal?: AbortSignal, ): Promise { const params = new URLSearchParams({ range }); if (project) params.set("project", project); diff --git a/packages/stats/src/client/app/routes.ts b/packages/stats/src/client/app/routes.ts index ca531d957..909a8d591 100644 --- a/packages/stats/src/client/app/routes.ts +++ b/packages/stats/src/client/app/routes.ts @@ -1,7 +1,15 @@ import { Activity, AlertCircle, Coins, Cpu, Folder, LayoutDashboard, Smile, TrendingUp } from "lucide-react"; import type React from "react"; -export type DashboardSection = "overview" | "requests" | "errors" | "models" | "costs" | "behavior" | "projects" | "gain"; +export type DashboardSection = + | "overview" + | "requests" + | "errors" + | "models" + | "costs" + | "behavior" + | "projects" + | "gain"; export interface DashboardRoute { id: DashboardSection; diff --git a/packages/stats/src/client/routes/GainRoute.tsx b/packages/stats/src/client/routes/GainRoute.tsx index b87bfbd1b..289ae936f 100644 --- a/packages/stats/src/client/routes/GainRoute.tsx +++ b/packages/stats/src/client/routes/GainRoute.tsx @@ -1,16 +1,18 @@ import { format } from "date-fns"; -import { useState, useMemo } from "react"; +import { useMemo, useState } from "react"; import { Line } from "react-chartjs-2"; import { getGainDashboardStats } from "../api"; -import { - buildSharedPlugins, - buildSharedScales, - CHART_THEMES, - lineDatasetStyle, -} from "../components/chart-shared"; +import { buildSharedPlugins, buildSharedScales, CHART_THEMES, lineDatasetStyle } from "../components/chart-shared"; import { formatBytes, formatCompact, formatInteger, formatPercent } from "../data/formatters"; import { useResource } from "../data/useResource"; -import type { GainDashboardStats, GainUnparsedCommand, GainSourceTotals, GainTimeSeriesPoint, GainTopFilter, TimeRange } from "../types"; +import type { + GainDashboardStats, + GainSourceTotals, + GainTimeSeriesPoint, + GainTopFilter, + GainUnparsedCommand, + TimeRange, +} from "../types"; import { AsyncBoundary, DataTable, Panel } from "../ui"; import type { DataTableColumn } from "../ui/DataTable"; import { useSystemTheme } from "../useSystemTheme"; @@ -28,27 +30,22 @@ export function GainRoute({ active, range, refreshTrigger }: GainRouteProps) { data: stats, error, loading, - } = useResource( - ["gain", range, refreshTrigger, project], - signal => getGainDashboardStats(range, signal, project), - { pollMs: 30_000, enabled: active }, - ); + } = useResource(["gain", range, refreshTrigger, project], signal => getGainDashboardStats(range, project, signal), { + pollMs: 30_000, + enabled: active, + }); return (
{stats && ( <> - + - + )} @@ -83,14 +80,15 @@ function GainProjectSelector({ > {projects.map(p => ( - + ))}
); } - // --------------------------------------------------------------------------- // Overall metrics panel // --------------------------------------------------------------------------- @@ -299,9 +297,7 @@ const UNPARSED_COLUMNS: DataTableColumn[] = [ key: "command", header: "Command (unparsed)", render: item => ( - - {item.command} - + {item.command} ), }, { diff --git a/packages/stats/src/gain-aggregator.ts b/packages/stats/src/gain-aggregator.ts index acc5dca0f..40700ff09 100644 --- a/packages/stats/src/gain-aggregator.ts +++ b/packages/stats/src/gain-aggregator.ts @@ -10,15 +10,15 @@ * Missing files are treated as zero records — never an error. */ -import * as os from "node:os"; import * as path from "node:path"; import { getAgentDir, getStatsDbPath, isEnoent, logger } from "@oh-my-pi/pi-utils"; +import { getTimeRangeConfig } from "./aggregator"; import type { GainDashboardStats, - GainUnparsedCommand, GainSourceTotals, GainTimeSeriesPoint, GainTopFilter, + GainUnparsedCommand, } from "./shared-types"; const BYTES_PER_TOKEN_ESTIMATE = 4; @@ -40,9 +40,28 @@ interface MinimizerRecord { cwd: string; } -// Structural skip filters — these mean the parser didn't attempt compression, not a coverage gap. -const SKIP_FILTERS = new Set(["missed", "compound", "chain-noop", "unsupported"]); +// Paths that carry no tuning signal — temp/internal locations. +const TEMP_PATH_RE = /\/T\/|\/tmp\/|\/pi-bash-exec|\/omp-bash-exec|\/pi-bash-detach|\/var\/folders\//; +// --------------------------------------------------------------------------- +// Project-match helper +// --------------------------------------------------------------------------- + +/** True when `cwd` is exactly `project` or is a sub-path of it. */ +function matchesProject(cwd: string | undefined, project: string): boolean { + if (!cwd) return false; + return cwd === project || cwd.startsWith(`${project}/`); +} + +// --------------------------------------------------------------------------- +// Minimizer JSONL — single read, three derived result sets +// --------------------------------------------------------------------------- + +interface MinimizerSets { + records: MinimizerRecord[]; + unparsed: MinimizerRecord[]; + projects: Set; +} async function readMinimizerFile(): Promise { const filePath = path.join(getAgentDir(), "minimizer-gain.jsonl"); @@ -54,80 +73,69 @@ async function readMinimizerFile(): Promise { } } -async function readMinimizerRecords(cutoff: number | null, project: string | null): Promise { +/** + * Parse the minimizer JSONL exactly once and derive all three result sets in + * a single pass. Avoids re-reading and re-parsing the file three times per + * dashboard request. + */ +async function readMinimizerSets(cutoff: number | null, project: string | null): Promise { const text = await readMinimizerFile(); - if (!text) return []; - const records: MinimizerRecord[] = []; - for (const line of text.split("\n")) { - if (!line.trim()) continue; - try { - const rec = JSON.parse(line) as MinimizerRecord; - if (rec.kind === "missed") continue; - const ts = new Date(rec.timestamp).getTime(); - if (cutoff !== null && ts < cutoff) continue; - if (project !== null && !rec.cwd?.startsWith(project)) continue; - records.push(rec); - } catch { /* skip malformed */ } - } - return records; -} + const sets: MinimizerSets = { records: [], unparsed: [], projects: new Set() }; + if (!text) return sets; -/** Records where NO filter matched (filter==="missed", kind==="missed"), excluding temp/internal paths. */ -async function readMinimizerUnparsedRecords(cutoff: number | null, project: string | null): Promise { - const text = await readMinimizerFile(); - if (!text) return []; - const records: MinimizerRecord[] = []; for (const line of text.split("\n")) { if (!line.trim()) continue; try { const rec = JSON.parse(line) as MinimizerRecord; - // Only "no filter matched" records - if (rec.kind !== "missed" || rec.filter !== "missed") continue; - // Skip internal/temp cwds (no tuning signal) - if (TEMP_PATH_RE.test(rec.cwd ?? "")) continue; + + // Always collect project cwds (unfiltered). + if (rec.cwd) sets.projects.add(rec.cwd); + const ts = new Date(rec.timestamp).getTime(); if (cutoff !== null && ts < cutoff) continue; - if (project !== null && !rec.cwd?.startsWith(project)) continue; - records.push(rec); - } catch { /* skip malformed */ } + if (project !== null && !matchesProject(rec.cwd, project)) continue; + + if (rec.kind === "missed") { + // Unparsed: only "no filter matched" records from meaningful cwds. + if (rec.filter === "missed" && !TEMP_PATH_RE.test(rec.cwd ?? "")) { + sets.unparsed.push(rec); + } + } else { + sets.records.push(rec); + } + } catch { + /* skip malformed */ + } } - return records; + return sets; } // --------------------------------------------------------------------------- // Project normalization & deduplication // --------------------------------------------------------------------------- -const TEMP_PATH_RE = /\/T\/|\/tmp\/|\/pi-bash-exec|\/omp-bash-exec|\/pi-bash-detach|\/var\/folders\//; - -/** Collapse worktree/ephemeral sub-paths to their logical project root. Returns null to drop. */ +/** + * Collapse worktree sub-paths to their logical project root. + * + * Rules are generic: omp internal wt paths are dropped; conventional worktree + * suffixes (`.wt/`, `-wt/`, `.worktrees/`, `-worktrees/`) are stripped. No + * author-specific IDE or tool paths are baked in. + * + * Returns null to drop temp/internal paths entirely. + */ function normalizeProjectPath(p: string): string | null { if (TEMP_PATH_RE.test(p)) return null; - // omp internal worktrees — not meaningful + // omp internal worktrees — not meaningful project roots if (/\/\.omp\/wt\//.test(p)) return null; - // herdr worktrees → /.herdr/worktrees/ - const herdr = p.match(/(\/\.herdr\/worktrees\/[^/]+)/); - if (herdr) return herdr[1]; - // /PycharmProjects/-factory-worktrees/... → /PycharmProjects/ - const factory = p.match(/(\/PycharmProjects\/[^/]+)-factory-worktrees\/.*/); - if (factory) return p.slice(0, factory.index!) + factory[1]; - // /PycharmProjects/-worktrees/... → /PycharmProjects/ - const wt = p.match(/(\/PycharmProjects\/[^/]+)-worktrees\/.*/); - if (wt) return p.slice(0, wt.index!) + wt[1]; - // /PycharmProjects/.wt//... → /PycharmProjects/ - const dotWt = p.match(/(\/PycharmProjects\/[^/]+)\.wt\/.*/); - if (dotWt) return p.slice(0, dotWt.index!) + dotWt[1]; - // /PycharmProjects/-wt//... → /PycharmProjects/ - const dashWt = p.match(/(\/PycharmProjects\/[^/]+)-wt\/.*/); - if (dashWt) return p.slice(0, dashWt.index!) + dashWt[1]; - // PycharmProjects/.worktrees///... → PycharmProjects/ - const hiddenWt = p.match(/^(.+\/PycharmProjects)\/\.worktrees\/([^/]+)\/[^/]+.*/); - if (hiddenWt) return `${hiddenWt[1]}/${hiddenWt[2]}`; - return p; -} -function pathDepth(p: string): number { - return p.split("/").filter(Boolean).length; + // Generic worktree layouts — strip the worktree suffix/subpath. + // Matches: /.wt//..., -wt//..., + // .wt//..., /.worktrees//..., + // -worktrees//..., /.herdr/worktrees//... + const m = p.match(/^(.+?)(?:\/\.wt\/|\/\.worktrees\/|-worktrees\/|-wt\/|\.wt\/|\/.+\/worktrees\/)[^/]+(\/.*)?$/); + if (m) return m[1]; + + return p; } /** @@ -144,33 +152,17 @@ function dedupeProjects(rawPaths: Set): string[] { const sorted = Array.from(normalized).sort(); return sorted.filter(p => { // Drop p if a shorter path is a proper prefix of it AND that parent is deep enough - // to be a meaningful scope boundary (depth ≥ 4), not a catch-all like /Users/davidandrews. + // to be a meaningful scope boundary (depth ≥ 4), not a catch-all like /Users/x. return !sorted.some( other => other !== p && other.length < p.length && - p.startsWith(other.endsWith("/") ? other : other + "/") && - pathDepth(other) >= 4, + p.startsWith(other.endsWith("/") ? other : `${other}/`) && + other.split("/").filter(Boolean).length >= 4, ); }); } -async function readMinimizerProjects(): Promise> { - const text = await readMinimizerFile(); - const projects = new Set(); - if (!text) return projects; - for (const line of text.split("\n")) { - if (!line.trim()) continue; - try { - const rec = JSON.parse(line) as MinimizerRecord; - if (rec.cwd) projects.add(rec.cwd); - } catch { /* skip */ } - } - return projects; -} - - - // --------------------------------------------------------------------------- // Snapcompact record schema // --------------------------------------------------------------------------- @@ -184,8 +176,16 @@ interface SnapcompactRecord { savedTokens: number; } -async function readSnapcompactRecords(cutoff: number | null, _project: string | null): Promise { - // Snapcompact records have no cwd/project field — project filter not applicable. +/** + * Snapcompact records carry no cwd/project field — project filter cannot be + * applied. When a project is selected the snapcompact totals are omitted from + * project-scoped responses to avoid mixing unrelated savings into the + * per-project view. + */ +async function readSnapcompactRecords(cutoff: number | null, project: string | null): Promise { + // No project field → skip entirely for project-scoped requests. + if (project !== null) return []; + const filePath = path.join(path.dirname(getStatsDbPath()), "snapcompact-savings.jsonl"); let text: string; try { @@ -206,7 +206,9 @@ async function readSnapcompactRecords(cutoff: number | null, _project: string | if (seen.has(key)) continue; seen.add(key); records.push(rec); - } catch { /* skip malformed line */ } + } catch { + /* skip malformed line */ + } } return records; } @@ -232,7 +234,7 @@ interface PiDistillStats { } async function readPiDistillRecords(cutoff: number | null, project: string | null): Promise { - const filePath = path.join(os.homedir(), ".omp", "agent", "pi-distill", "stats.json"); + const filePath = path.join(getAgentDir(), "pi-distill", "stats.json"); let raw: string; try { raw = await Bun.file(filePath).text(); @@ -245,7 +247,7 @@ async function readPiDistillRecords(cutoff: number | null, project: string | nul const stats = JSON.parse(raw) as PiDistillStats; let sessions = Object.values(stats.sessions ?? {}); if (cutoff !== null) sessions = sessions.filter(s => s.lastTs >= cutoff); - if (project !== null) sessions = sessions.filter(s => s.project?.startsWith(project)); + if (project !== null) sessions = sessions.filter(s => matchesProject(s.project, project)); return sessions; } catch (err) { logger.debug("gain-aggregator: failed to parse pi-distill stats.json", { err: String(err) }); @@ -255,7 +257,7 @@ async function readPiDistillRecords(cutoff: number | null, project: string | nul /** Collect all distinct project values from pi-distill stats (unfiltered). */ async function readDistillProjects(): Promise> { - const filePath = path.join(os.homedir(), ".omp", "agent", "pi-distill", "stats.json"); + const filePath = path.join(getAgentDir(), "pi-distill", "stats.json"); const projects = new Set(); try { const raw = await Bun.file(filePath).text(); @@ -263,7 +265,9 @@ async function readDistillProjects(): Promise> { for (const s of Object.values(stats.sessions ?? {})) { if (s.project) projects.add(s.project); } - } catch { /* ignore */ } + } catch { + /* ignore */ + } return projects; } @@ -302,21 +306,17 @@ export async function getGainDashboardStats( range?: string | null, project?: string | null, ): Promise { - const normalized = range?.trim().toLowerCase() ?? "24h"; - const RANGE_MS: Record = { "1h": 3600_000, "24h": 86400_000, "7d": 604800_000, "30d": 2592000_000, "90d": 7776000_000 }; - const effectiveCutoff: number | null = - normalized === "all" ? null : Date.now() - (RANGE_MS[normalized] ?? 86400_000); + const { cutoff: effectiveCutoff } = getTimeRangeConfig(range); const effectiveProject: string | null = project?.trim() || null; - const [minimizerRecords, unparsedRecords, snapcompactRecords, distillRecords, minimizerProjects, distillProjects] = - await Promise.all([ - readMinimizerRecords(effectiveCutoff, effectiveProject), - readMinimizerUnparsedRecords(effectiveCutoff, effectiveProject), - readSnapcompactRecords(effectiveCutoff, effectiveProject), - readPiDistillRecords(effectiveCutoff, effectiveProject), - readMinimizerProjects(), - readDistillProjects(), - ]); + const [minimizerSets, snapcompactRecords, distillRecords, distillProjects] = await Promise.all([ + readMinimizerSets(effectiveCutoff, effectiveProject), + readSnapcompactRecords(effectiveCutoff, effectiveProject), + readPiDistillRecords(effectiveCutoff, effectiveProject), + readDistillProjects(), + ]); + + const { records: minimizerRecords, unparsed: unparsedRecords, projects: minimizerProjects } = minimizerSets; // --- Minimizer totals --- const minimizerTotals = emptyTotals(); @@ -354,22 +354,23 @@ export async function getGainDashboardStats( finalizeReductionPercent(minimizerTotals); // --- Unparsed commands (no filter matched — tuning targets) --- + // Key on the full command string to avoid collision; truncate only at display time. const cmdMap = new Map(); for (const rec of unparsedRecords) { - const key = (rec.command ?? "").slice(0, 120); - const existing = cmdMap.get(key); + const fullKey = rec.command ?? ""; + const existing = cmdMap.get(fullKey); if (existing) { existing.hits += 1; existing.inputBytes += rec.inputBytes ?? 0; } else { - cmdMap.set(key, { command: key, hits: 1, inputBytes: rec.inputBytes ?? 0 }); + // Store the full command; callers may truncate for display. + cmdMap.set(fullKey, { command: fullKey, hits: 1, inputBytes: rec.inputBytes ?? 0 }); } } const unparsedCommands: GainUnparsedCommand[] = Array.from(cmdMap.values()) .sort((a, b) => b.hits - a.hits) .slice(0, 25); - // --- Snapcompact totals --- const snapcompactTotals = emptyTotals(); @@ -408,6 +409,10 @@ export async function getGainDashboardStats( finalizeReductionPercent(distillTotals); // --- Overall totals --- + // reductionPercent is computed only from sources that have originalBytes + // (minimizer + distill). Snapcompact has no originalBytes, so including its + // savedBytes in the numerator with nothing in the denominator would overstate + // the ratio. The per-source cards each report their own correct ratio. const overall: GainSourceTotals = { savedTokens: minimizerTotals.savedTokens + snapcompactTotals.savedTokens + distillTotals.savedTokens, savedBytes: minimizerTotals.savedBytes + snapcompactTotals.savedBytes + distillTotals.savedBytes, @@ -416,8 +421,11 @@ export async function getGainDashboardStats( originalBytes: minimizerTotals.originalBytes + distillTotals.originalBytes, reductionPercent: null, }; - if (overall.originalBytes > 0) { - overall.reductionPercent = overall.savedBytes / overall.originalBytes; + // Use only sources with originalBytes for the ratio to avoid inflated percentages. + const ratioNumerator = minimizerTotals.savedBytes + distillTotals.savedBytes; + const ratioDenominator = overall.originalBytes; + if (ratioDenominator > 0) { + overall.reductionPercent = ratioNumerator / ratioDenominator; } // --- Time series (sorted ascending by date) --- diff --git a/packages/stats/src/index.ts b/packages/stats/src/index.ts index a9f6a27b6..bb897d541 100755 --- a/packages/stats/src/index.ts +++ b/packages/stats/src/index.ts @@ -15,9 +15,15 @@ export { syncAllSessions, } from "./aggregator"; export { closeDb } from "./db"; -export { startServer } from "./server"; export { getGainDashboardStats } from "./gain-aggregator"; -export type { GainDashboardStats, GainSource, GainSourceTotals, GainTimeSeriesPoint, GainTopFilter } from "./shared-types"; +export { startServer } from "./server"; +export type { + GainDashboardStats, + GainSource, + GainSourceTotals, + GainTimeSeriesPoint, + GainTopFilter, +} from "./shared-types"; export type { AggregatedStats, DashboardStats, diff --git a/packages/stats/src/server.ts b/packages/stats/src/server.ts index 50e5cbbaf..8af71ffdb 100644 --- a/packages/stats/src/server.ts +++ b/packages/stats/src/server.ts @@ -16,9 +16,9 @@ import { getTotalMessageCount, syncAllSessions, } from "./aggregator"; -import { getGainDashboardStats } from "./gain-aggregator"; import { decodeEmbeddedClientArchive } from "./embedded-client"; import embeddedClientArchiveTxt from "./embedded-client.generated.txt"; +import { getGainDashboardStats } from "./gain-aggregator"; const EMBEDDED_CLIENT_ARCHIVE = decodeEmbeddedClientArchive(embeddedClientArchiveTxt);