Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

apps/codex/src/weekly-report.ts (1)

46-57: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

formatBucket: formatDisplayDate keeps the locale-dependent week field from the prior review cycle.

The refactored delegation to buildBucketedReport with formatBucket: formatDisplayDate still produces a localized display string in every WeeklyReportRow.week, the same concern raised in the previous review round. Three concrete downstream effects remain:

1. JSON output is locale-fragile. commands/weekly.ts emits { weekly: rows, totals } where rows[n].week is an already-formatted locale string, making programmatic consumers locale-sensitive.
2. Table's dateFormatter receives a pre-formatted string. commands/weekly.ts passes dateFormatter: (dateStr) => formatDateCompact(dateStr) to ResponsiveTable (line 142), but row.week won't be an ISO date at that point — the cell will either be double-processed or silently pass through, depending on ResponsiveTable's detection logic.
3. Tests are forced to use weak assertions. Line 125 can only assert toContain('2025') because the exact formatted output is locale-dependent. With a raw ISO key, you could assert expect(first.week).toBe('2025-09-07').

💡 Suggested fix — propagate raw ISO key, format at render time

In buildWeeklyReport, omit the formatBucket field so buildBucketedReport stores the raw ISO key:

 	return buildBucketedReport({
 		bucketField: 'week',
 		events,
-		formatBucket: formatDisplayDate,
 		getBucketKey: (timestamp, timezone) => toWeekKey(timestamp, timezone, startDay),
 		getFilterDateKey: toDateKey,
 		locale: options.locale,
 		pricingSource: options.pricingSource,
 		since: options.since,
 		timezone: options.timezone,
 		until: options.until,
 	});

Then format at the table push site in commands/weekly.ts:

-			row.week,
+			formatDisplayDate(row.week, options.locale, options.timezone),

And tighten the test assertion:

-		expect(first.week).toContain('2025');
+		expect(first.week).toBe('2025-09-07');

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/codex/src/weekly-report.ts` around lines 46 - 57, buildBucketedReport is
being given formatBucket: formatDisplayDate which stores a locale-formatted
string in each WeeklyReportRow.week; remove the formatBucket option from the
buildBucketedReport(...) call in weekly-report.ts so toWeekKey produces and
stores the raw ISO week key, then update the presentation layer
(commands/weekly.ts) to format the ISO key at render time by using dateFormatter
or calling formatDisplayDate/formatDateCompact when pushing rows into
ResponsiveTable; finally tighten tests to assert the raw ISO key (e.g.,
'2025-09-07') instead of locale-dependent substrings.

apps/codex/src/bucketed-report.ts (2)

93-96: 💤 Low value

Consider parallelising pricing lookups with Promise.all.

Serial await inside a for…of loop is fine for in-memory lookups, but if getPricing ever involves I/O the requests will be queued. A Promise.all version is equally readable and future-proof.

♻️ Proposed refactor

-	const modelPricing = new Map<string, Awaited<ReturnType<PricingSource['getPricing']>>>();
-	for (const modelName of uniqueModels) {
-		modelPricing.set(modelName, await pricingSource.getPricing(modelName));
-	}
+	const modelPricing = new Map(
+		await Promise.all(
+			[...uniqueModels].map(async (modelName) =>
+				[modelName, await pricingSource.getPricing(modelName)] as const,
+			),
+		),
+	);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/codex/src/bucketed-report.ts` around lines 93 - 96, The current loop
awaits pricingSource.getPricing sequentially which will serialize I/O; instead
kick off all requests in parallel using Promise.all on uniqueModels.map(m =>
pricingSource.getPricing(m)), await the array of results, then populate
modelPricing (the Map named modelPricing) by pairing uniqueModels with the
returned pricing values; update references to PricingSource.getPricing and
ensure errors propagate/are handled as before.

---

67-83: ⚡ Quick win

Redundant double Map lookups — simplify to single get + conditional set.

The same get(…) ?? create(…) / if (!has(…)) set(…) pattern appears twice (lines 67–70 for summaries, lines 73–79 for summary.models). Each miss causes two separate Map operations when one is enough. The idiomatic pattern is a single .get() result checked for nullability.

♻️ Proposed refactor

-		const bucketKey = getBucketKey(event.timestamp, timezone);
-		const summary = summaries.get(bucketKey) ?? createSummary(bucketKey);
-		if (!summaries.has(bucketKey)) {
-			summaries.set(bucketKey, summary);
-		}
+		const bucketKey = getBucketKey(event.timestamp, timezone);
+		let summary = summaries.get(bucketKey);
+		if (summary == null) {
+			summary = createSummary(bucketKey);
+			summaries.set(bucketKey, summary);
+		}

 		addUsage(summary, event);
-		const modelUsage: ModelUsage = summary.models.get(modelName) ?? {
-			...createEmptyUsage(),
-			isFallback: false,
-		};
-		if (!summary.models.has(modelName)) {
-			summary.models.set(modelName, modelUsage);
-		}
+		let modelUsage = summary.models.get(modelName);
+		if (modelUsage == null) {
+			modelUsage = { ...createEmptyUsage(), isFallback: false };
+			summary.models.set(modelName, modelUsage);
+		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/codex/src/bucketed-report.ts` around lines 67 - 83, The code does two
Map lookups for both summaries and summary.models; replace each get/has/set pair
with a single get and a conditional set using that local variable: for
summaries, call summaries.get(bucketKey) once, if falsy
createSummary(bucketKey), set it into summaries and assign to summary, then call
addUsage(summary, event); for model usage, call summary.models.get(modelName)
once, if falsy create the ModelUsage via createEmptyUsage(), set it into
summary.models and assign to modelUsage, then call addUsage(modelUsage, event)
and preserve the existing isFallbackModel -> modelUsage.isFallback logic. Use
the existing identifiers summaries, createSummary, addUsage, summary.models,
createEmptyUsage, ModelUsage, modelName, event, and isFallbackModel to locate
and update the code.

apps/codex/src/bucketed-report.ts (1)

89-92: ⚡ Quick win

Parallelize independent pricing fetches with Promise.all.

The sequential await inside the for...of loop serializes pricing lookups that are entirely independent of each other. If each async call takes one second, four sequential calls take four seconds whereas running them in parallel takes approximately one second. Use Promise.all to fan them out concurrently:

⚡ Proposed refactor

-	const modelPricing = new Map<string, Awaited<ReturnType<PricingSource['getPricing']>>>();
-	for (const modelName of uniqueModels) {
-		modelPricing.set(modelName, await pricingSource.getPricing(modelName));
-	}
+	const modelPricing = new Map<string, Awaited<ReturnType<PricingSource['getPricing']>>>(
+		await Promise.all(
+			Array.from(uniqueModels).map(async (modelName) => [
+				modelName,
+				await pricingSource.getPricing(modelName),
+			] as const),
+		),
+	);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/codex/src/bucketed-report.ts` around lines 89 - 92, The current code
serially awaits pricingSource.getPricing for each model in the for...of loop
(using uniqueModels and modelPricing), causing unnecessary latency; instead,
fire all requests concurrently using Promise.all: map uniqueModels to promises
that call pricingSource.getPricing(modelName), await Promise.all to get results,
then populate modelPricing (the Map) by pairing each modelName with its
corresponding resolved pricing; this retains use of modelPricing and
pricingSource.getPricing but replaces the sequential await with parallelized
fetches.