feat: add data-aggregation module with helper functions #65
Conversation
Create a new module to house common data aggregation functions that will be used to reduce code duplication in data-loader.ts. This module includes: - aggregateByModel() for model-based aggregation - aggregateModelBreakdowns() for breakdown aggregation - createModelBreakdowns() for formatting - calculateTotals() for token/cost totals - filterByDateRange() for date filtering - isDuplicateEntry() and markAsProcessed() for deduplication - extractUniqueModels() for unique model extraction
Replace duplicate code in data-loader.ts with the new helper functions from data-aggregation module: - Use aggregateByModel() instead of inline model aggregation logic - Use createModelBreakdowns() for consistent breakdown formatting - Use calculateTotals() instead of inline reduce operations - Use filterByDateRange() for date filtering - Use isDuplicateEntry() and markAsProcessed() for deduplication - Use extractUniqueModels() for model list extraction - Use aggregateModelBreakdowns() in loadMonthlyUsageData This reduces code duplication by ~195 lines while maintaining the same functionality and improving maintainability.
WalkthroughThe changes refactor Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant DataLoader
participant Helpers
Caller->>DataLoader: loadDailyUsageData / loadSessionData / loadMonthlyUsageData
DataLoader->>Helpers: filterByDateRange
DataLoader->>Helpers: isDuplicateEntry / markAsProcessed
DataLoader->>Helpers: aggregateByModel / aggregateModelBreakdowns
DataLoader->>Helpers: createModelBreakdowns
DataLoader->>Helpers: calculateTotals
DataLoader->>Helpers: extractUniqueModels
DataLoader-->>Caller: Aggregated and filtered usage data
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ryoppippi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request aims to improve code organization and reduce duplication by extracting common data aggregation, filtering, and utility functions from data-loader.ts into a dedicated data-aggregation.ts module. The existing data loading functions (loadDailyUsageData, loadSessionData, loadMonthlyUsageData) were updated to utilize these new helper functions.
Highlights
- New Module: Introduced a new module
src/data-aggregation.tsto consolidate common data processing and aggregation logic. - Refactoring
data-loader.ts: Replaced duplicated and inline logic for aggregation, filtering, and utility functions withinloadDailyUsageData,loadSessionData, andloadMonthlyUsageDatawith calls to the new helper functions fromdata-aggregation.ts.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configureGemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new data-aggregation.ts module, refactoring common data aggregation logic out of data-loader.ts, improving readability and maintainability. The new helper functions are well-defined and effectively used. The review suggests enhancing clarity and type safety related to the calculateTotals return type, the filterByDateRange date format, and the spreading of the totals object in loadDailyUsageData and loadSessionData.
src/data-aggregation.ts
Outdated
| entries: T[], | ||
| getUsage: (entry: T) => UsageData['message']['usage'], | ||
| getCost: (entry: T) => number, | ||
| ): TokenStats & { totalCost: number } { |
There was a problem hiding this comment.
The return type TokenStats & { totalCost: number } suggests cost (from TokenStats) and totalCost might differ. However, they're initialized and accumulated identically, creating redundancy. Consider returning only one of these values to avoid confusion.
src/data-aggregation.ts
Outdated
| */ | ||
| export function filterByDateRange<T>( | ||
| items: T[], | ||
| getDate: (item: T) => string, |
There was a problem hiding this comment.
The filterByDateRange implementation assumes getDate returns a YYYY-MM-DD date string. Add a JSDoc comment to the getDate parameter specifying this expected format to ensure robustness.
Move all helper functions from data-aggregation.ts into data-loader.ts since they are only used in that file. This improves code organization by keeping related code together and reduces unnecessary file separation. - Moved aggregateByModel, aggregateModelBreakdowns, createModelBreakdowns, calculateTotals, filterByDateRange, isDuplicateEntry, markAsProcessed, and extractUniqueModels functions into data-loader.ts - Made all helper functions private (no export) since they are internal - Deleted data-aggregation.ts file - Removed import statement from data-loader.ts This maintains the same functionality and test coverage while simplifying the module structure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/data-aggregation.ts (2)
32-46: Avoid repeated object-literal allocation & tighten typingInside the hot loop you allocate a fresh “zero”
TokenStatsobject every time the model key is first seen.
Hoist this constant (or a factory) outside the loop, and/or use??=to keep the existing object to avoid the extra allocation on every first encounter.
Additionally,usageis currently inferred asany, which triggers all the ts/no-unsafe- warnings reported by ESLint. Constrain the generic to something that contains a strongly-typedusageproperty to silence those warnings.-const existing = modelAggregates.get(modelName) ?? { - inputTokens: 0, - ... -}; +const defaultStats: TokenStats = { + inputTokens: 0, + outputTokens: 0, + cacheCreationTokens: 0, + cacheReadTokens: 0, + cost: 0, +}; +const existing = modelAggregates.get(modelName) ?? defaultStats;If you introduce a generic constraint:
export function aggregateByModel<T extends { /* whatever shape */ }>(...)you’ll eliminate the unsafe-member-access noise.
🧰 Tools
🪛 ESLint
[error] 41-41: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 41-41: Unsafe member access .input_tokens on an
errortyped value.(ts/no-unsafe-member-access)
[error] 42-42: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 42-42: Unsafe member access .output_tokens on an
errortyped value.(ts/no-unsafe-member-access)
[error] 43-43: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 43-43: Unsafe member access .cache_creation_input_tokens on an
errortyped value.(ts/no-unsafe-member-access)
[error] 44-44: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 44-44: Unsafe member access .cache_read_input_tokens on an
errortyped value.(ts/no-unsafe-member-access)
146-155: Date filtering is fragile for non-YYYY-MM-DDinputs
replace(/-/g, '')assumes the date string has only hyphens.
IfgetDateever returns an ISO timestamp (2025-06-18T12:34:56Z) the compare logic silently breaks.Consider normalising with
new Date()and comparing epoch numbers, or slice the first 10 chars before stripping dashes:-const dateStr = getDate(item).replace(/-/g, ''); +const dateStr = getDate(item).substring(0, 10).replace(/-/g, '');This keeps the function tolerant to full ISO strings while preserving the cheap lexicographic compare trick.
src/data-loader.ts (1)
510-527: Duplicate-detection logic re-implemented instead of reusedIn
loadSessionDatayou’ve kept the old in-place deduplication (451-460) instead of using the new helpers, unlike the daily path above.Unify both call sites for consistency and to avoid divergence:
-const uniqueHash = createUniqueHash(data); -if (uniqueHash != null && processedHashes.has(uniqueHash)) … -… -if (uniqueHash != null) { processedHashes.add(uniqueHash); } +const uniqueHash = createUniqueHash(data); +if (isDuplicateEntry(uniqueHash, processedHashes)) { + continue; +} +markAsProcessed(uniqueHash, processedHashes);Reduces code and guarantees identical behaviour.
🧰 Tools
🪛 ESLint
[error] 513-513: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 513-513: Unsafe member access .message on an
errortyped value.(ts/no-unsafe-member-access)
[error] 523-523: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 523-523: Unsafe member access .message on an
errortyped value.(ts/no-unsafe-member-access)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/data-aggregation.ts(1 hunks)src/data-loader.ts(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/data-aggregation.ts (1)
src/data-loader.ts (2)
UsageData(46-46)ModelBreakdown(57-57)
src/data-loader.ts (2)
src/data-aggregation.ts (7)
isDuplicateEntry(161-169)markAsProcessed(174-181)aggregateByModel(14-50)createModelBreakdowns(89-98)extractUniqueModels(186-191)filterByDateRange(136-156)aggregateModelBreakdowns(55-84)src/calculate-cost.ts (1)
calculateTotals(18-37)
🪛 ESLint
src/data-aggregation.ts
[error] 29-29: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 41-41: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 41-41: Unsafe member access .input_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 42-42: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 42-42: Unsafe member access .output_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 43-43: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 43-43: Unsafe member access .cache_creation_input_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 44-44: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 44-44: Unsafe member access .cache_read_input_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 62-62: Unsafe member access .modelName on an error typed value.
(ts/no-unsafe-member-access)
[error] 66-66: Unsafe argument of type error typed assigned to a parameter of type string.
(ts/no-unsafe-argument)
[error] 66-66: Unsafe member access .modelName on an error typed value.
(ts/no-unsafe-member-access)
[error] 74-74: Unsafe argument of type error typed assigned to a parameter of type string.
(ts/no-unsafe-argument)
[error] 74-74: Unsafe member access .modelName on an error typed value.
(ts/no-unsafe-member-access)
[error] 75-75: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 75-75: Unsafe member access .inputTokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 76-76: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 76-76: Unsafe member access .outputTokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 77-77: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 77-77: Unsafe member access .cacheCreationTokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 78-78: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 78-78: Unsafe member access .cacheReadTokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 79-79: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 79-79: Unsafe member access .cost on an error typed value.
(ts/no-unsafe-member-access)
[error] 110-110: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 114-114: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 114-114: Unsafe member access .input_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 115-115: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 115-115: Unsafe member access .output_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 116-116: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 116-116: Unsafe member access .cache_creation_input_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 117-117: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 117-117: Unsafe member access .cache_read_input_tokens on an error typed value.
(ts/no-unsafe-member-access)
src/data-loader.ts
[error] 355-355: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 355-355: Unsafe member access .message on an error typed value.
(ts/no-unsafe-member-access)
[error] 365-365: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 365-365: Unsafe member access .message on an error typed value.
(ts/no-unsafe-member-access)
[error] 513-513: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 513-513: Unsafe member access .message on an error typed value.
(ts/no-unsafe-member-access)
[error] 523-523: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 523-523: Unsafe member access .message on an error typed value.
(ts/no-unsafe-member-access)
[error] 564-564: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 564-564: Unsafe member access .modelBreakdowns on an error typed value.
(ts/no-unsafe-member-access)
🔇 Additional comments (3)
src/data-loader.ts (3)
318-325: Nice reuse of the new dedup helpersSwitching to
isDuplicateEntry/markAsProcessedmakes the intent crystal-clear.
380-385: Elegant date-range filteringReplacing the inline logic with
filterByDateRangeimproves readability and removes duplication.
564-569: 👍 Aggregating monthly model data viaaggregateModelBreakdownsThe flat-map + helper combo is concise and avoids an O(n²) merge loop.
🧰 Tools
🪛 ESLint
[error] 564-564: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 564-564: Unsafe member access .modelBreakdowns on an
errortyped value.(ts/no-unsafe-member-access)
src/data-aggregation.ts
Outdated
| ): TokenStats & { totalCost: number } { | ||
| return entries.reduce( | ||
| (acc, entry) => { | ||
| const usage = getUsage(entry); | ||
| const cost = getCost(entry); | ||
|
|
||
| return { | ||
| inputTokens: acc.inputTokens + (usage.input_tokens ?? 0), | ||
| outputTokens: acc.outputTokens + (usage.output_tokens ?? 0), | ||
| cacheCreationTokens: acc.cacheCreationTokens + (usage.cache_creation_input_tokens ?? 0), | ||
| cacheReadTokens: acc.cacheReadTokens + (usage.cache_read_input_tokens ?? 0), | ||
| cost: acc.cost + cost, | ||
| totalCost: acc.totalCost + cost, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
cost and totalCost are the same – drop one
calculateTotals accumulates both cost and totalCost, but they end up holding identical values.
This duplication is confusing and leaks an unused cost field into every caller (e.g. DailyUsage doesn’t define it).
- cost: acc.cost + cost,
- totalCost: acc.totalCost + cost,
+ totalCost: acc.totalCost + cost,and remove cost from the accumulator’s seed & return type:
-export function calculateTotals<…>(): TokenStats & { totalCost: number }
+export function calculateTotals<…>(): Omit<TokenStats, 'cost'> & { totalCost: number }This keeps the public API minimal and avoids accidental misuse.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ): TokenStats & { totalCost: number } { | |
| return entries.reduce( | |
| (acc, entry) => { | |
| const usage = getUsage(entry); | |
| const cost = getCost(entry); | |
| return { | |
| inputTokens: acc.inputTokens + (usage.input_tokens ?? 0), | |
| outputTokens: acc.outputTokens + (usage.output_tokens ?? 0), | |
| cacheCreationTokens: acc.cacheCreationTokens + (usage.cache_creation_input_tokens ?? 0), | |
| cacheReadTokens: acc.cacheReadTokens + (usage.cache_read_input_tokens ?? 0), | |
| cost: acc.cost + cost, | |
| totalCost: acc.totalCost + cost, | |
| }; | |
| export function calculateTotals<…>(): Omit<TokenStats, 'cost'> & { totalCost: number } { | |
| return entries.reduce( | |
| (acc, entry) => { | |
| const usage = getUsage(entry); | |
| const cost = getCost(entry); | |
| return { | |
| inputTokens: acc.inputTokens + (usage.input_tokens ?? 0), | |
| outputTokens: acc.outputTokens + (usage.output_tokens ?? 0), | |
| cacheCreationTokens: acc.cacheCreationTokens + (usage.cache_creation_input_tokens ?? 0), | |
| cacheReadTokens: acc.cacheReadTokens + (usage.cache_read_input_tokens ?? 0), | |
| totalCost: acc.totalCost + cost, | |
| }; | |
| }, | |
| // …your initial seed here, minus the `cost` field… | |
| ); | |
| } |
🧰 Tools
🪛 ESLint
[error] 110-110: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 114-114: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 114-114: Unsafe member access .input_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 115-115: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 115-115: Unsafe member access .output_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 116-116: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 116-116: Unsafe member access .cache_creation_input_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 117-117: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 117-117: Unsafe member access .cache_read_input_tokens on an error typed value.
(ts/no-unsafe-member-access)
🤖 Prompt for AI Agents
In src/data-aggregation.ts around lines 107 to 120, the accumulator object in
the reduce function includes both 'cost' and 'totalCost' fields which hold
identical values, causing unnecessary duplication and confusion. Remove the
'cost' field entirely from the accumulator's initial seed, the reduce return
object, and the function's return type so that only 'totalCost' remains. This
will simplify the API and prevent leaking an unused 'cost' property to callers.
| const modelAggregates = aggregateByModel( | ||
| entries, | ||
| entry => entry.model, | ||
| entry => entry.data.message.usage, | ||
| entry => entry.cost, | ||
| ); | ||
|
|
||
| // Create model breakdowns | ||
| const modelBreakdowns: ModelBreakdown[] = Array.from(modelAggregates.entries()) | ||
| .map(([modelName, stats]) => ({ | ||
| modelName, | ||
| ...stats, | ||
| })) | ||
| .sort((a, b) => b.cost - a.cost); // Sort by cost descending | ||
| const modelBreakdowns = createModelBreakdowns(modelAggregates); | ||
|
|
||
| // Calculate totals | ||
| const totals = entries.reduce( | ||
| (acc, entry) => ({ | ||
| inputTokens: | ||
| acc.inputTokens + (entry.data.message.usage.input_tokens ?? 0), | ||
| outputTokens: | ||
| acc.outputTokens + (entry.data.message.usage.output_tokens ?? 0), | ||
| cacheCreationTokens: | ||
| acc.cacheCreationTokens | ||
| + (entry.data.message.usage.cache_creation_input_tokens ?? 0), | ||
| cacheReadTokens: | ||
| acc.cacheReadTokens | ||
| + (entry.data.message.usage.cache_read_input_tokens ?? 0), | ||
| totalCost: acc.totalCost + entry.cost, | ||
| }), | ||
| { | ||
| inputTokens: 0, | ||
| outputTokens: 0, | ||
| cacheCreationTokens: 0, | ||
| cacheReadTokens: 0, | ||
| totalCost: 0, | ||
| }, | ||
| const totals = calculateTotals( | ||
| entries, | ||
| entry => entry.data.message.usage, | ||
| entry => entry.cost, | ||
| ); | ||
|
|
||
| const modelsUsed = [...new Set(entries.map(e => e.model).filter((m): m is string => m != null && m !== '<synthetic>'))]; | ||
| const modelsUsed = extractUniqueModels(entries, e => e.model); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
totals carries an unused cost property
Because calculateTotals still returns cost, the spread here injects an extra field not defined in DailyUsageSchema.
Either drop cost from calculateTotals (preferred – see earlier comment) or destructure explicitly:
-...totals,
+const { cost: _unused, ...restTotals } = totals;
+...restTotals,Eliminates dead data and keeps runtime objects lean.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 355-355: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 355-355: Unsafe member access .message on an error typed value.
(ts/no-unsafe-member-access)
[error] 365-365: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 365-365: Unsafe member access .message on an error typed value.
(ts/no-unsafe-member-access)
🤖 Prompt for AI Agents
In src/data-loader.ts between lines 352 and 370, the object returned by
calculateTotals includes an unused cost property that is not part of
DailyUsageSchema. To fix this, modify the calculateTotals function to exclude
the cost property from its return value, ensuring only the necessary fields are
returned. This will prevent injecting extra unused fields and keep the runtime
objects lean and aligned with the schema.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/data-loader.ts (1)
199-227: Duplicate:calculateTotalsstill leaks an extracostfieldPrevious review already pointed out that the returned object should not expose
costbecause it is not part ofDailyUsageSchema/SessionUsageSchema.
Keeping it around still injects dead data when you later spread...totals, and TypeScript won’t catch this because the type alias explicitly contains it.-function calculateTotals<T>( - ... -): TokenStats & { totalCost: number } { +function calculateTotals<T>( + ... +): Omit<TokenStats, 'cost'> & { totalCost: number } { ... - cost: acc.cost + cost, totalCost: acc.totalCost + cost, ... - cost: 0, totalCost: 0, }After this change, remember to adjust the call sites (
loadDailyUsageData,loadSessionData) to remove the extra property if you rely on structural typing.🧰 Tools
🪛 ESLint
[error] 206-206: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 210-210: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 210-210: Unsafe member access .input_tokens on an
errortyped value.(ts/no-unsafe-member-access)
[error] 211-211: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 211-211: Unsafe member access .output_tokens on an
errortyped value.(ts/no-unsafe-member-access)
[error] 212-212: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 212-212: Unsafe member access .cache_creation_input_tokens on an
errortyped value.(ts/no-unsafe-member-access)
[error] 213-213: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 213-213: Unsafe member access .cache_read_input_tokens on an
errortyped value.(ts/no-unsafe-member-access)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 417e143 and 4a8d6762d2fb41b95cc683981751b722e18d05b2.
📒 Files selected for processing (1)
src/data-loader.ts(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/data-loader.ts (1)
src/calculate-cost.ts (1)
calculateTotals(18-37)
🪛 ESLint
src/data-loader.ts
[error] 130-130: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 136-136: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 136-136: Unsafe member access .input_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 137-137: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 137-137: Unsafe member access .output_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 138-138: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 138-138: Unsafe member access .cache_creation_input_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 139-139: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 139-139: Unsafe member access .cache_read_input_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 164-164: Unsafe member access .modelName on an error typed value.
(ts/no-unsafe-member-access)
[error] 168-168: Unsafe argument of type error typed assigned to a parameter of type string.
(ts/no-unsafe-argument)
[error] 168-168: Unsafe member access .modelName on an error typed value.
(ts/no-unsafe-member-access)
[error] 170-170: Unsafe argument of type error typed assigned to a parameter of type string.
(ts/no-unsafe-argument)
[error] 170-170: Unsafe member access .modelName on an error typed value.
(ts/no-unsafe-member-access)
[error] 171-171: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 171-171: Unsafe member access .inputTokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 172-172: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 172-172: Unsafe member access .outputTokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 173-173: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 173-173: Unsafe member access .cacheCreationTokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 174-174: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 174-174: Unsafe member access .cacheReadTokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 175-175: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 175-175: Unsafe member access .cost on an error typed value.
(ts/no-unsafe-member-access)
[error] 206-206: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 210-210: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 210-210: Unsafe member access .input_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 211-211: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 211-211: Unsafe member access .output_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 212-212: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 212-212: Unsafe member access .cache_creation_input_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 213-213: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 213-213: Unsafe member access .cache_read_input_tokens on an error typed value.
(ts/no-unsafe-member-access)
[error] 537-537: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 537-537: Unsafe member access .message on an error typed value.
(ts/no-unsafe-member-access)
[error] 547-547: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 547-547: Unsafe member access .message on an error typed value.
(ts/no-unsafe-member-access)
[error] 693-693: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 693-693: Unsafe member access .message on an error typed value.
(ts/no-unsafe-member-access)
[error] 703-703: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 703-703: Unsafe member access .message on an error typed value.
(ts/no-unsafe-member-access)
[error] 744-744: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 744-744: Unsafe member access .modelBreakdowns on an error typed value.
(ts/no-unsafe-member-access)
🔇 Additional comments (2)
src/data-loader.ts (2)
105-145: Helper extraction greatly improves readability 👍Pulling the model aggregation loop into
aggregateByModelremoves a lot of duplicated map-handling code from the three loaders and makes the intent crystal-clear. Nice separation of concerns and correct handling of the synthetic model.🧰 Tools
🪛 ESLint
[error] 130-130: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 136-136: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 136-136: Unsafe member access .input_tokens on an
errortyped value.(ts/no-unsafe-member-access)
[error] 137-137: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 137-137: Unsafe member access .output_tokens on an
errortyped value.(ts/no-unsafe-member-access)
[error] 138-138: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 138-138: Unsafe member access .cache_creation_input_tokens on an
errortyped value.(ts/no-unsafe-member-access)
[error] 139-139: Unsafe assignment of an error typed value.
(ts/no-unsafe-assignment)
[error] 139-139: Unsafe member access .cache_read_input_tokens on an
errortyped value.(ts/no-unsafe-member-access)
563-567: Date-range filter correctly applied – good touchFiltering after aggregation keeps the critical path lean and avoids unnecessary work when large files are involved. Implementation is straightforward and correct.
src/data-loader.ts
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Entries lacking IDs silently bypass deduplication
isDuplicateEntry returns false when either message ID or request ID is missing, so such entries are never tracked in processedHashes.
If a client repeatedly logs the same ID-less record, you’ll count it multiple times.
Consider a fallback—in the absence of a unique hash, derive a hash from timestamp + model + tokens (or simply always insert a placeholder string) so that duplicates are still suppressed.
🤖 Prompt for AI Agents
In src/data-loader.ts around lines 257 to 277, the isDuplicateEntry function
returns false when uniqueHash is null, causing entries without IDs to bypass
deduplication. To fix this, modify the logic to generate a fallback hash when
uniqueHash is null, for example by hashing a combination of timestamp, model,
and tokens or using a consistent placeholder string. Update both
isDuplicateEntry and markAsProcessed to use this fallback hash so that entries
lacking IDs are still tracked and duplicates suppressed.
- Fix TypeScript unsafe type issues in aggregation functions by improving generic constraints and hoisting defaultStats objects to avoid repeated allocation - Improve date filtering robustness by taking first 10 chars before removing hyphens to handle ISO timestamp formats properly - Unify duplicate detection logic in loadSessionData to use isDuplicateEntry and markAsProcessed helper functions consistently - Remove .ts extensions from import statements following project conventions Addresses feedback from PR #65 reviewers (CodeRabbit, Copilot, Gemini). All lint and type checks now pass.
ryoppippi
force-pushed
the
refactor-data-loader
branch
from
4a8d676 to
fbb507c
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors data aggregation logic by consolidating helper functions into data-loader.ts, improving maintainability, type safety, and performance. It also updates import statements for module resolution consistency.
- Extracted common data aggregation helpers and duplicate detection logic into consolidated functions.
- Enhanced type safety with improved generic constraints and updated import paths.
- Fixed linting and module resolution issues in several files.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/shared-args.internal.ts | Updated the import for getDefaultClaudePath to include an explicit .ts extension. |
| src/mcp.ts | Changed import statements for LoadOptions and data-loader to include explicit .ts. |
| src/calculate-cost.ts | Updated import for usage types from data-loader to include the .ts extension. |
Comments suppressed due to low confidence (4)
src/shared-args.internal.ts:4
- The import now explicitly includes the '.ts' extension. Confirm that using explicit extensions aligns with the project’s module resolution strategy and is consistent with other file imports.
import { getDefaultClaudePath } from './data-loader.ts';
src/mcp.ts:1
- Using the explicit .ts extension in the import statement may be against the project convention if extensions are meant to be omitted. Verify for consistency with the project’s import style.
import type { LoadOptions } from './data-loader.ts';
src/mcp.ts:11
- Ensure consistent use of the explicit '.ts' extension in inline import paths across the file in line with the project standards.
} from './data-loader.ts';
src/calculate-cost.ts:1
- This import now includes the '.ts' extension explicitly. Double-check that this approach is deliberate and consistent with the team’s conventions for module imports.
import type { DailyUsage, MonthlyUsage, SessionUsage } from './data-loader.ts';
- Fix TypeScript unsafe type issues in aggregation functions by improving generic constraints and hoisting defaultStats objects to avoid repeated allocation - Improve date filtering robustness by taking first 10 chars before removing hyphens to handle ISO timestamp formats properly - Unify duplicate detection logic in loadSessionData to use isDuplicateEntry and markAsProcessed helper functions consistently - Remove .ts extensions from import statements following project conventions Addresses feedback from PR #65 reviewers (CodeRabbit, Copilot, Gemini). All lint and type checks now pass.
feat: add data-aggregation module with helper functions
Summary
This PR refactors data aggregation logic by extracting common functionality into helper functions and then merging them back into
data-loader.tsfor better maintainability and performance.Key Changes
aggregateByModel,aggregateModelBreakdowns,calculateTotals,filterByDateRange, and duplicate detectionloadDailyUsageData,loadSessionData, andloadMonthlyUsageDatafunctionsisDuplicateEntryandmarkAsProcessedhelper functions across all data loading functionsTechnical Details
data-loader.tsfor optimal performanceReview Feedback Addressed
.tsextensions from importsTest Results
All 117 tests pass, including: