feat: class based pricing-fetcher with using syntax#49
Conversation
There was a problem hiding this comment.
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!
Summary of Changes
Hello team, Gemini here with a summary of this pull request. This PR, authored by @ryoppippi, focuses on refactoring the cost calculation logic within the application. The primary goal is to enhance efficiency and maintainability by introducing a dedicated PricingFetcher class to handle pricing data and calculations. This change moves away from direct manipulation of a global pricing cache. Additionally, functions involved in cost calculation have been updated to be asynchronous, and the new using statement is leveraged for improved resource management, ensuring that PricingFetcher instances are properly cleaned up.
Highlights
- Refactor: PricingFetcher Class: Replaced direct usage of pricing cache functions (
clearPricingCache,fetchModelPricing,getModelPricing) with instances of the newPricingFetcherclass acrosssrc/data-loader.tsandsrc/debug.ts. This encapsulates pricing logic and simplifies its usage. - Async Cost Calculation: The
calculateCostForEntryfunction has been converted to an asynchronous function. It now accepts aPricingFetcherinstance and uses its methods to fetch pricing and calculate costs based on tokens, allowing for better handling of potential asynchronous operations related to pricing data. - Resource Management with
using: Theusingstatement is now integrated in functions likeloadDailyUsageData,loadSessionData, anddetectMismatchesto managePricingFetcherinstances. This ensures that the fetcher's resources (like the internal cache) are automatically cleaned up when the fetcher goes out of scope. - Test Updates: Associated test files (
src/data-loader.test.ts,src/debug.test.ts,src/pricing-fetcher.test.ts) have been updated to align with the new class-based, asynchronous approach. This includes removingbeforeEachblocks that cleared the old global cache and updating test cases to usePricingFetcherinstances.
Changelog
Click here to see the changelog
- src/data-loader.test.ts
- Removed
beforeEachhooks that cleared the global pricing cache (DIFF 3, DIFF 4, DIFF 5, DIFF 6). - Updated imports to use
PricingFetcherclass instead of standalone functions (DIFF 2). - Modified test cases for
calculateCostForEntryto be asynchronous and use aPricingFetcherinstance (DIFF 6, DIFF 7, DIFF 8, DIFF 9, DIFF 10).
- Removed
- src/data-loader.ts
- Updated imports to use
PricingFetcherclass (DIFF 1). - Changed
calculateCostForEntryfunction signature to be asynchronous and accept aPricingFetcherinstance (DIFF 2). - Updated logic within
calculateCostForEntryto usefetcher.calculateCostFromTokens(DIFF 3). - Introduced
using fetcher = ...for resource management inloadDailyUsageData(DIFF 4). - Updated cost calculation call in
loadDailyUsageDatato use thefetcherandawait(DIFF 5). - Introduced
using fetcher = ...for resource management inloadSessionData(DIFF 6). - Updated cost calculation call in
loadSessionDatato use thefetcherandawait(DIFF 7).
- Updated imports to use
- src/debug.test.ts
- Removed
beforeEachhook that cleared the global pricing cache (DIFF 1). - Updated imports to remove
clearPricingCache(DIFF 1).
- Removed
- src/debug.ts
- Updated imports to use
PricingFetcherclass instead of standalone functions (DIFF 1). - Introduced
using fetcher = new PricingFetcher()for resource management indetectMismatches(DIFF 2). - Updated cost calculation logic within
detectMismatchesto usefetcher.calculateCostFromTokensandawait(DIFF 3).
- Updated imports to use
- src/pricing-fetcher.test.ts
- Removed
beforeEachhook that cleared the global pricing cache (DIFF 1). - Updated imports to use
PricingFetcherclass andModelPricingtype (DIFF 1). - Added tests specifically for the
PricingFetcherclass, includingusingstatement support and direct cost calculation (DIFF 1). - Updated existing tests (
fetchModelPricing,getModelPricing,calculateCostFromTokens) to use methods of aPricingFetcherinstance instead of standalone functions (DIFF 1, DIFF 2, DIFF 3, DIFF 4, DIFF 5, DIFF 6, DIFF 7, DIFF 8, DIFF 9, DIFF 10).
- Removed
- src/pricing-fetcher.ts
- Refactored standalone pricing functions (
clearPricingCache,fetchModelPricing,getModelPricing,calculateCostFromTokens) into aPricingFetcherclass (DIFF 1). - Implemented the
Disposableinterface and[Symbol.dispose]method for automatic cleanup (DIFF 1). - Changed internal cache and
fetchModelPricingreturn type toMap<string, ModelPricing>for better handling (DIFF 1). - Updated
getModelPricingandcalculateCostFromTokensto be methods of thePricingFetcherclass and use the internal cache/logic (DIFF 1). - Added a new
calculateCostFromPricingmethod to perform the calculation given tokens and pricing data (DIFF 1).
- Refactored standalone pricing functions (
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 configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
commit: |
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the cost calculation logic by introducing the PricingFetcher class and leveraging the using statement for resource management. The changes significantly improve code modularity, encapsulation of pricing logic, and maintainability. The conversion of relevant functions to async and the updates to the test suite are well-implemented. The code quality is high, and the PR description is very clear and helpful.
Summary of Findings
- Code Structure in
calculateCostForEntry: Insrc/data-loader.ts, thecalculateCostForEntryfunction contains anif (mode === 'display')block. Due to the call-site logic inloadDailyUsageDataandloadSessionData(wherefetcherisnullifmode === 'display', andcalculateCostForEntryis not called in that case), this specific block is not reached by those primary callers. However, it's correctly maintained for direct calls tocalculateCostForEntry(e.g., from tests) and ensures the function adheres to its broader contract. This is a minor observation and doesn't require changes. - Test Coverage for
getModelPricingVariations: Insrc/pricing-fetcher.test.ts, a specific test for 'provider prefix' matching ingetModelPricingwas removed. The newgetModelPricingmethod within thePricingFetcherclass handles various model name variations (including prefixes) more broadly. This change is acceptable as the functionality is implicitly covered by other tests that rely on these variations.
Merge Readiness
The pull request is in excellent shape and significantly improves the codebase. I have not found any critical or high-severity issues. The minor observations noted in the findings_summary do not require changes. Based on this review, the code appears ready for merging. As always, ensure any CI checks pass and consider if further review by others is standard practice for your team. I am not authorized to approve pull requests.
src/data-loader.ts
Outdated
There was a problem hiding this comment.
It seems like the cost variable is being assigned a value based on whether fetcher is null or not. If fetcher is null, then the cost is assigned data.costUSD ?? 0. If fetcher is not null, then the cost is assigned await calculateCostForEntry(data, mode, fetcher). Is this the intended behavior? If so, consider adding a comment to explain this logic.
src/data-loader.ts
Outdated
There was a problem hiding this comment.
Since calculateCostForEntry is now asynchronous, it might be beneficial to handle potential errors (e.g., network issues when fetching pricing) more explicitly with a try-catch block to prevent unhandled rejections. What do you think about adding a try-catch block?
try {
if (mode === 'display') {
// Always use costUSD, even if undefined
return data.costUSD ?? 0;
}
...
} catch (error) {
console.error("Error calculating cost:", error);
return 0; // Or throw, depending on desired error handling
}There was a problem hiding this comment.
Pull Request Overview
This PR refactors cost calculation to use a new PricingFetcher class and the using statement for resource cleanup, converting synchronous cache calls into an async API and updating all callers and tests accordingly.
- Introduces
PricingFetcherclass with encapsulated caching and async methods. - Converts
calculateCostForEntryand related functions to asynchronous, usingusingfor automatic disposal. - Updates tests (
*.test.ts) to remove direct cache manipulation and leveragePricingFetcher.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/pricing-fetcher.ts | Added PricingFetcher class, replaced free functions with methods |
| src/pricing-fetcher.test.ts | Updated tests to instantiate and dispose PricingFetcher via using |
| src/debug.ts | Refactored to use PricingFetcher and using in detectMismatches |
| src/debug.test.ts | Removed cache-clear imports and adjusted imports for PricingFetcher |
| src/data-loader.ts | Made calculateCostForEntry async, applied using in loaders |
| src/data-loader.test.ts | Updated data-loader tests to use async calculateCostForEntry and PricingFetcher |
Comments suppressed due to low confidence (5)
src/pricing-fetcher.test.ts:70
- [nitpick] Relying on a
<5mstiming assertion can be flaky on CI or under load. Consider stubbing or mocking network calls, or using a spy to verify that the cache path is taken instead of measuring elapsed time directly.
expect(endTime - startTime).toBeLessThan(5);
src/pricing-fetcher.ts:16
- The
Disposableinterface is not imported or defined in this file, which will cause a compilation error. Consider importingDisposablefrom the appropriate module or removing theimplements Disposableclause.
export class PricingFetcher implements Disposable {
src/data-loader.ts:147
- Declaring
fetcherasnullin ausingstatement will invoke disposal onnull, causing a runtime error. Split the logic so thatusingis only applied whennew PricingFetcher()is used, or guard disposal whenfetcheris null.
using fetcher = mode === 'display' ? null : new PricingFetcher();
src/data-loader.ts:262
- As above, the
usingdeclaration will attempt to disposenullwhenmode === 'display'. Consider conditionally declaring theusingstatement only for non-null instances.
using fetcher = mode === 'display' ? null : new PricingFetcher();
src/debug.ts:108
- The comment indicates skipping comparison when pricing is unavailable, but there is no guard to skip if
calculatedCostis zero (pricing missing). Add a conditional (e.g.,if (calculatedCost > 0)) around the mismatch calculation to avoid false positives.
// Only compare if we could calculate a cost
dc5deed to
307b57c
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
feat: class based pricing-fetcher with using syntax
This pull request refactors the cost calculation logic in the
src/data-loader.ts,src/debug.ts, and associated test files to improve efficiency and maintainability. The key changes include replacing direct pricing cache manipulation with thePricingFetcherclass, converting synchronous cost calculation functions to asynchronous, and ensuring proper cleanup of resources using theusingstatement.Refactoring cost calculation logic:
Introduction of
PricingFetcherclass:clearPricingCache,fetchModelPricing,getModelPricing) with thePricingFetcherclass acrosssrc/data-loader.tsandsrc/debug.ts. This encapsulates pricing logic and simplifies resource management. [1] [2]Conversion to asynchronous cost calculation:
calculateCostForEntryto an asynchronous function that usesPricingFetcherfor token-based cost calculation. This ensures more flexibility and better handling of asynchronous operations. [1] [2] [3]Resource management improvements:
usingstatement to managePricingFetcherinstances in functions likeloadDailyUsageData,loadSessionData, anddetectMismatches. This ensures proper resource cleanup and avoids memory leaks. [1] [2] [3]Test updates:
beforeEachblocks that cleared the pricing cache and updated test cases to usePricingFetcherinstances for cost calculation. This ensures tests are consistent with the new implementation. [1] [2] [3]These changes enhance the codebase by improving modularity, reducing direct dependency on pricing cache functions, and ensuring better resource management.