refactor: move /v1/provider-graph-data to modules#1324
refactor: move /v1/provider-graph-data to modules#1324baktun14 merged 1 commit intoakash-network:mainfrom
Conversation
WalkthroughThis update introduces a new modular architecture for provider graph data APIs in the application. It adds a dedicated controller, service, schemas, and router for provider graph data, replacing the previous monolithic route and service implementation. Test suites and seeders are refactored to support the new structure and enhance testability. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant Controller
participant Service
participant DB
Client->>Router: GET /v1/provider-graph-data/{dataName}
Router->>Controller: getProviderGraphData(dataName)
Controller->>Service: getProviderGraphData(dataName)
Service->>DB: Query provider stats for dataName
DB-->>Service: Aggregated stats data
Service-->>Controller: Structured response
Controller-->>Router: Structured response
Router-->>Client: 200 OK (JSON) or 404 Not Found
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use ✨ 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 (
|
8731ed9 to
5eab4d2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1324 +/- ##
==========================================
+ Coverage 34.24% 34.66% +0.41%
==========================================
Files 799 802 +3
Lines 19451 19467 +16
Branches 3596 3594 -2
==========================================
+ Hits 6661 6748 +87
+ Misses 12205 12134 -71
Partials 585 585
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
apps/api/src/provider/routes/provider-graph-data/provider-graph-data.router.ts (1)
37-40: Consider removing or replacing console.log statement.The console.log statement appears to be for debugging purposes and may not be appropriate for production code. Consider using proper logging with appropriate log levels.
- console.log("Rejected graph request: " + dataName); + // Use a proper logger instead + logger.debug(`Rejected graph request: ${dataName}`);apps/api/test/functional/dashboard-data.spec.ts (1)
20-195: Comprehensive test setup for dashboard data.The test setup creates a complete environment with days, providers, provider snapshots, and blocks across multiple dates. This provides good coverage for testing the dashboard data endpoint.
Consider extracting some of the repeated provider snapshot creation logic into helper functions to reduce verbosity.
You could simplify the test setup with a helper function:
function createProviderSnapshot(owner: string, date: Date, baseValue: number) { return ProviderSnapshotSeeder.createInDatabase({ owner, checkDate: date, isOnline: true, isLastSuccessOfDay: true, activeCPU: baseValue + 1, activeGPU: baseValue + 2, activeMemory: baseValue + 3, activePersistentStorage: baseValue + 4, activeEphemeralStorage: baseValue + 5, // Add other properties following the same pattern }); } // Then use it like: providerSnapshots = await Promise.all([ createProviderSnapshot(providers[0].owner, twoDaysAgo, 100), createProviderSnapshot(providers[0].owner, yesterday, 200), // etc. ]);apps/api/test/seeders/provider-snapshot.seeder.ts (2)
6-31: Consider extracting the repeated Faker ranges into reusable helpers.Almost every numeric field uses the same
faker.number.int({ min: 0, max: 1000000000 })(or similar) call. Consolidating these default ranges in a helper (e.g.randomResourceValue()) will:
- eliminate duplication,
- make future range adjustments less error-prone, and
- improve readability.
- activeMemory: overrides.activeMemory ?? faker.number.int({ min: 0, max: 1000000000 }), - ... - availablePersistentStorage: overrides.availablePersistentStorage ?? faker.number.int({ min: 0, max: 1000000000 }) + activeMemory: overrides.activeMemory ?? randomResourceValue(), + ... + availablePersistentStorage: overrides.availablePersistentStorage ?? randomResourceValue()Where
randomResourceValueis implemented once at the top of the file.
34-37: Await the update to guarantee DB persistence before the caller continues.
ProviderSnapshot.createis asynchronous; returning the promise withoutawaitinsidecreateInDatabaseworks, but it leaves the caller responsible for awaiting it.
Wrapping it inawait(as you already do) is correct—just make sure callers don’t forget to await their invocation ofcreateInDatabase. A short JSDoc note could help avoid accidental misuse.apps/api/test/functional/provider-graph-data.spec.ts (1)
15-17: Use a single “now” reference to avoid accidental clock-skew.
subDays(new Date(), n)is evaluated three times at module load.
Although the skew is usually milliseconds, it can bite when the code runs very close to midnight in CI, causing “today” and “yesterday” to cross a date boundary.-const yesterday = subDays(new Date(), 1); -const twoDaysAgo = subDays(new Date(), 2); -const threeDaysAgo = subDays(new Date(), 3); +const now = new Date(); +const yesterday = subDays(now, 1); +const twoDaysAgo = subDays(now, 2); +const threeDaysAgo = subDays(now, 3);apps/api/src/provider/services/provider-graph-data/provider-graph-data.service.ts (4)
15-16:parseInt(...) || 0silently hides NaN – log or throw instead?If the column contains a non-numeric string, the current logic converts it to
0, which can silently mask data issues.
Consider explicit validation or at least logging a warning whenNumber.isNaN(parseInt(...))is true.
19-38: Minor SQL optimisation – avoid double summation.You already aggregate CPU/GPU/… in the sub-query; summing again in the outer query forces an extra pass.
Aggregating once in the sub-query and selecting the summed columns in the outer query (SUM(...) AS cpu→ justcpu) will reduce work, especially on large tables.
94-97: Replace console.error with the application logger.Using
console.*inside a service bypasses the structured logging mechanism and makes log aggregation harder.-console.error( - `removeLastAroundMidnight: lastItem.date is not a string: ` + - ... -); +appLogger.error( + { lastItem }, + 'removeLastAroundMidnight(): Expected lastItem.date to be a string' +);
90-91: Typo:isFirstFifteenMinuesOfDayRename the variable to
isFirstFifteenMinutesOfDayfor clarity.apps/api/src/provider/http-schemas/provider-graph-data.schema.ts (1)
3-4: Useas constforauthorizedDataNamesto preserve literal types
By appendingas constyou lock down the array to its exact string literals, which enables stronger typing and safer downstream use (e.g., when inferring enums). You can also export a union type from it for clarity.-export const authorizedDataNames = ["count", "cpu", "gpu", "memory", "storage"]; +export const authorizedDataNames = ["count", "cpu", "gpu", "memory", "storage"] as const; +export type AuthorizedDataName = typeof authorizedDataNames[number];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (19)
apps/api/src/app.ts(2 hunks)apps/api/src/provider/controllers/provider-graph-data/provider-graph-data.controller.ts(1 hunks)apps/api/src/provider/http-schemas/provider-graph-data.schema.ts(1 hunks)apps/api/src/provider/routes/index.ts(1 hunks)apps/api/src/provider/routes/provider-graph-data/provider-graph-data.router.ts(1 hunks)apps/api/src/provider/services/provider-graph-data/provider-graph-data.service.ts(1 hunks)apps/api/src/provider/services/provider-stats/provider-stats.service.ts(0 hunks)apps/api/src/routes/v1/dashboardData.ts(2 hunks)apps/api/src/routes/v1/index.ts(0 hunks)apps/api/src/routes/v1/providerGraphData.ts(0 hunks)apps/api/src/services/db/statsService.ts(1 hunks)apps/api/test/functional/dashboard-data.spec.ts(1 hunks)apps/api/test/functional/provider-graph-data.spec.ts(1 hunks)apps/api/test/functional/provider-versions.spec.ts(2 hunks)apps/api/test/functional/providers.spec.ts(2 hunks)apps/api/test/seeders/block.seeder.ts(1 hunks)apps/api/test/seeders/day.seeder.ts(1 hunks)apps/api/test/seeders/provider-snapshot.seeder.ts(1 hunks)apps/api/test/seeders/provider.seeder.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/api/src/provider/services/provider-stats/provider-stats.service.ts
- apps/api/src/routes/v1/index.ts
- apps/api/src/routes/v1/providerGraphData.ts
🧰 Additional context used
🧬 Code Graph Analysis (5)
apps/api/src/app.ts (1)
apps/api/src/provider/routes/provider-graph-data/provider-graph-data.router.ts (1)
providerGraphDataRouter(32-32)
apps/api/test/functional/provider-versions.spec.ts (1)
apps/api/test/seeders/provider-snapshot.seeder.ts (1)
ProviderSnapshotSeeder(5-38)
apps/api/src/routes/v1/dashboardData.ts (1)
apps/api/src/provider/http-schemas/provider-graph-data.schema.ts (1)
ProviderGraphDataResponse(40-40)
apps/api/test/functional/dashboard-data.spec.ts (5)
apps/api/src/app.ts (1)
initDb(167-179)apps/api/test/seeders/day.seeder.ts (1)
DaySeeder(5-22)apps/api/test/seeders/provider.seeder.ts (1)
ProviderSeeder(5-42)apps/api/test/seeders/provider-snapshot.seeder.ts (1)
ProviderSnapshotSeeder(5-38)apps/api/test/seeders/block.seeder.ts (1)
BlockSeeder(8-36)
apps/api/test/functional/provider-graph-data.spec.ts (5)
apps/api/src/app.ts (1)
initDb(167-179)apps/api/test/seeders/day.seeder.ts (1)
DaySeeder(5-22)apps/api/test/seeders/provider.seeder.ts (1)
ProviderSeeder(5-42)apps/api/test/seeders/provider-snapshot.seeder.ts (1)
ProviderSnapshotSeeder(5-38)apps/api/test/seeders/block.seeder.ts (1)
BlockSeeder(8-36)
🔇 Additional comments (23)
apps/api/test/seeders/provider.seeder.ts (1)
39-39: Improved static method invocationThe change from using
this.create()to an explicitProviderSeeder.create()enhances code clarity by clearly indicating that a static method is being called. This modification aligns with best practices for static method invocation and maintains consistency with similar refactorings in other seeder classes.apps/api/src/app.ts (2)
49-49: Added import for new provider graph data routerThe addition of
providerGraphDataRouterto the imports properly integrates the new modular provider graph data API component.
115-115: Registered new provider graph data routerThe new router registration follows the same pattern as other provider-related routers in the application, maintaining consistency in the routing structure.
apps/api/test/seeders/day.seeder.ts (1)
19-19: Improved static method invocationSimilar to the change in
provider.seeder.ts, replacingthis.create()withDaySeeder.create()improves clarity when calling static methods. This ensures consistency across all seeder classes in the codebase.apps/api/src/provider/routes/index.ts (1)
7-7: Added export for provider graph data routerThe new export statement properly integrates the provider graph data router into the central provider routes module. This follows the established pattern for exposing route handlers and enables the router to be imported wherever provider routes are used.
apps/api/src/services/db/statsService.ts (1)
1-295: Code organization improvement through modularization.The removal of the
getProviderGraphDatafunction and its helperremoveLastAroundMidnightfrom this file is a positive change that aligns with the goal of moving provider graph data functionality to a dedicated module. This refactoring improves code organization by:
- Reducing the size and complexity of the statsService file
- Following the single responsibility principle by moving provider-specific functionality to a dedicated provider module
- Making the codebase more maintainable through better separation of concerns
The remaining statistics functions are logically grouped and focused on their specific domains.
apps/api/test/functional/provider-versions.spec.ts (3)
1-1: Import optimization by removing unused types.Good refactoring to remove the unused
ProviderSnapshottype import and only keep theProvidertype.
6-6: Improved test data generation through dedicated seeder.Good addition of the
ProviderSnapshotSeederimport to replace the previous standalone function.
37-58: Enhanced test data creation pattern.The refactoring to use
ProviderSnapshotSeeder.createInDatabasedirectly improves the code by:
- Simplifying the test setup by removing intermediate steps
- Using a consistent pattern for test data creation
- Leveraging the encapsulated creation logic in the seeder class
- Making the tests more maintainable through standardized entity creation
This change aligns well with the seeder pattern used for providers earlier in the file.
apps/api/src/routes/v1/dashboardData.ts (4)
3-3: Dependency injection implementation.Good addition of the
containerimport fromtsyringeto support dependency injection for the provider graph data service.
5-6: Improved type safety with service refactoring.Good addition of the type import and service class to replace the previous direct function import, enhancing type safety and maintaining the contract with the service.
9-9: Clean import after refactoring.Good update to import only the
getDashboardDatafunction after thegetProviderGraphDatafunction was removed from the stats service.
165-165: Dependency injection implementation for service access.The change to use
container.resolve(ProviderGraphDataService).getProviderGraphData("count")properly implements dependency injection, improving:
- Testability by allowing service mocking in tests
- Maintainability by reducing direct dependencies
- Consistency with the new modular architecture
The type assertion ensures type safety for the fallback value in case of errors.
apps/api/src/provider/controllers/provider-graph-data/provider-graph-data.controller.ts (1)
1-15: Well-structured controller with proper separation of concerns.This new controller follows good design practices:
- Uses
@singleton()decorator for proper dependency injection registration- Implements constructor injection for the service dependency
- Provides a clean API that delegates to the service layer
- Maintains proper typing with
ProviderGraphDataResponsereturn type- Follows the single responsibility principle
The controller acts as a thin layer between the routes and the service, adhering to clean architecture principles.
apps/api/test/seeders/block.seeder.ts (1)
19-29: Good enhancement to the block seeder for testing provider metrics.The additional block properties align well with the refactoring of the provider graph data endpoints. These properties support the more detailed testing of dashboard data and provider graph data metrics in the new functional tests.
apps/api/src/provider/routes/provider-graph-data/provider-graph-data.router.ts (2)
10-30: Well-structured OpenAPI route definition.The route definition is comprehensive with appropriate tags, request parameter validation, and response schemas. This follows good API documentation practices.
42-43: Good use of dependency injection.Using container.resolve to obtain the controller instance follows good dependency injection practices and allows for better testability.
apps/api/test/functional/providers.spec.ts (2)
1-2: Good practice using type-only imports.Changing to type-only imports for database models helps avoid importing the actual implementation when only the type is needed.
21-64: Well-implemented refactoring of test data setup.Replacing manual creation with seeder methods improves code maintainability and consistency. This change aligns with the modular approach taken throughout the codebase and reduces duplication.
apps/api/test/functional/dashboard-data.spec.ts (1)
201-246: Thorough assertions for dashboard data.The test assertions comprehensively verify all the expected properties in both the "now" and "compare" sections of the response. This ensures that the API correctly calculates and returns all metrics.
apps/api/test/functional/provider-graph-data.spec.ts (1)
212-215: Date format in expectation may not match Postgres DATE parsing.
DATEcolumns returned bypgare, by default, plain strings likeYYYY-MM-DD.
The test expectsYYYY-MM-DDT00:00:00.000Z. If the code runs against a default Postgres driver, this assertion will fail.Confirm that you have a custom
pgtype parser (or that Sequelize mapsdate→Date) producing the ISO-with-time string; otherwise adjust the expectation to the actual format.apps/api/src/provider/http-schemas/provider-graph-data.schema.ts (2)
1-2: 👍 Import of Zod is correct
No issues with the import statement here.
38-40: ✅ Type inference from schemas is spot-on
InferringProviderGraphDataParamsandProviderGraphDataResponseviaz.inferensures consistency between runtime validation and TypeScript types.
| const isFirstFifteenMinuesOfDay = now.getHours() === 0 && now.getMinutes() <= 15; | ||
| const lastItem = stats.length > 0 ? stats[stats.length - 1] : null; | ||
| if (lastItem && typeof lastItem.date !== "string") { | ||
| console.error( | ||
| `removeLastAroundMidnight: lastItem.date is not a string: ` + | ||
| `type = ${typeof lastItem.date} value = ${lastItem.date} constructor = ${(lastItem.date as any)?.constructor?.name}` + | ||
| JSON.stringify(lastItem) | ||
| ); | ||
| } | ||
|
|
||
| const lastItemIsForToday = lastItem && isSameDay(lastItem.date, now); | ||
| if (isFirstFifteenMinuesOfDay && lastItemIsForToday) { | ||
| return stats.slice(0, stats.length - 1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
isSameDay receives a string – convert to Date for robust comparison.
date-fns will coerce the string, but relying on implicit coercion can introduce subtle TZ bugs.
Also, the log message above assumes date should be a string, while the comparison below assumes Date.
-const lastItemIsForToday = lastItem && isSameDay(lastItem.date, now);
+const lastItemIsForToday =
+ lastItem && isSameDay(new Date(lastItem.date as unknown as string), now);(Adjust the earlier typeof check accordingly.)
📝 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.
| const isFirstFifteenMinuesOfDay = now.getHours() === 0 && now.getMinutes() <= 15; | |
| const lastItem = stats.length > 0 ? stats[stats.length - 1] : null; | |
| if (lastItem && typeof lastItem.date !== "string") { | |
| console.error( | |
| `removeLastAroundMidnight: lastItem.date is not a string: ` + | |
| `type = ${typeof lastItem.date} value = ${lastItem.date} constructor = ${(lastItem.date as any)?.constructor?.name}` + | |
| JSON.stringify(lastItem) | |
| ); | |
| } | |
| const lastItemIsForToday = lastItem && isSameDay(lastItem.date, now); | |
| if (isFirstFifteenMinuesOfDay && lastItemIsForToday) { | |
| return stats.slice(0, stats.length - 1); | |
| } | |
| const isFirstFifteenMinuesOfDay = now.getHours() === 0 && now.getMinutes() <= 15; | |
| const lastItem = stats.length > 0 ? stats[stats.length - 1] : null; | |
| if (lastItem && typeof lastItem.date !== "string") { | |
| console.error( | |
| `removeLastAroundMidnight: lastItem.date is not a string: ` + | |
| `type = ${typeof lastItem.date} value = ${lastItem.date} constructor = ${(lastItem.date as any)?.constructor?.name}` + | |
| JSON.stringify(lastItem) | |
| ); | |
| } | |
| - const lastItemIsForToday = lastItem && isSameDay(lastItem.date, now); | |
| + const lastItemIsForToday = | |
| + lastItem && isSameDay(new Date(lastItem.date as unknown as string), now); | |
| if (isFirstFifteenMinuesOfDay && lastItemIsForToday) { | |
| return stats.slice(0, stats.length - 1); | |
| } |
🤖 Prompt for AI Agents
In
apps/api/src/provider/services/provider-graph-data/provider-graph-data.service.ts
around lines 90 to 103, the isSameDay function is called with lastItem.date
which may be a string, but it should be a Date object for reliable comparison.
Update the code to explicitly convert lastItem.date to a Date instance before
passing it to isSameDay. Also, adjust the earlier typeof check to expect a Date
instead of a string and update the error log message accordingly to reflect this
type expectation.
There was a problem hiding this comment.
But lastItem.date is a Date there, isn't it?
There was a problem hiding this comment.
@jzsfkzm Looking at the code closely, lastItem.date is actually expected to be a string, not a Date object. The error check specifically looks for string type with typeof lastItem.date !== "string" and the error message explicitly says "lastItem.date is not a string".
This seems to be because the SQL query returns date fields as strings from the database. While date-fns can sometimes handle string inputs by coercing them, the explicit conversion recommended in the suggestion would make the type handling more consistent and avoid potential timezone issues.
There was a problem hiding this comment.
I just checked, and typeof stats[0].date is "object". We get dates there in the date field of ProviderStats items indeed.
There was a problem hiding this comment.
@jzsfkzm Thank you for checking that! You're right - if typeof stats[0].date is "object", then the dates are already being returned as Date objects from the database query.
In this case, the error check is actually incorrect. It's looking for strings when it should be expecting Date objects. The isSameDay function is being used correctly since it's receiving a Date object already.
I recommend updating the error check to match the actual type:
- if (lastItem && typeof lastItem.date !== "string") {
+ if (lastItem && !(lastItem.date instanceof Date)) {
console.error(
- `removeLastAroundMidnight: lastItem.date is not a string: ` +
+ `removeLastAroundMidnight: lastItem.date is not a Date object: ` +
`type = ${typeof lastItem.date} value = ${lastItem.date} constructor = ${(lastItem.date as any)?.constructor?.name}` +
JSON.stringify(lastItem)
);
}This would better align with how the data is actually structured.
There was a problem hiding this comment.
Interesting part, @baktun14 @ygrishajev what do you think about this? .date there is always a Date, but we log if it's not a string (which is never, since it's always a date). Not sure why is this check in the code, Rabbit suggest to change it to check for Date, or maybe we should just remove the whole check & log?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@jzsfkzm I'm not sure, I think there was an issue with the type, but I don't recall why this log was added
There was a problem hiding this comment.
Finally, I removed this check. I believe date is a Date in every case.
| export const ProviderGraphDataParamsSchema = z.object({ | ||
| dataName: z.string().openapi({ example: "cpu", enum: authorizedDataNames }) | ||
| }); |
There was a problem hiding this comment.
❗ Enforce validation of dataName against authorized values
Using z.string().openapi({ enum: [...] }) only supplies OpenAPI metadata—it does not reject invalid inputs at runtime. Switch to z.enum (or add a .refine) to enforce that dataName truly must be one of authorizedDataNames.
-export const ProviderGraphDataParamsSchema = z.object({
- dataName: z.string().openapi({ example: "cpu", enum: authorizedDataNames })
-});
+export const ProviderGraphDataParamsSchema = z.object({
+ dataName: z.enum(authorizedDataNames).openapi({ example: "cpu" })
+});🤖 Prompt for AI Agents
In apps/api/src/provider/http-schemas/provider-graph-data.schema.ts around lines
5 to 7, the schema uses z.string() with openapi enum metadata which does not
enforce runtime validation of dataName against authorizedDataNames. Replace
z.string().openapi({ enum: authorizedDataNames }) with
z.enum(authorizedDataNames) to ensure dataName is validated at runtime against
the authorized values.
There was a problem hiding this comment.
Applying this fix would break the 404, but should we?
There was a problem hiding this comment.
No description provided.
5eab4d2 to
e57a8d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/api/src/provider/services/provider-graph-data/provider-graph-data.service.ts (1)
90-96: 🛠️ Refactor suggestionTypo & repeated TZ-safety concern around
lastItem.date.
isFirstFifteenMinuesOfDay→isFirstFifteenMinutesOfDay- Prior feedback still applies: pass a
DatetoisSameDayto avoid implicit string coercion and TZ bugs.apps/api/test/functional/provider-graph-data.spec.ts (1)
172-177: 🛠️ Refactor suggestionAwait the
updatecalls to avoid race conditions in CI.The providers might be fetched before their
lastSuccessfulSnapshotIdfields are persisted, causing flaky tests on slower runners.
🧹 Nitpick comments (1)
apps/api/test/functional/provider-graph-data.spec.ts (1)
212-215: Snapshot date assertion is timezone-sensitive.
format(..., "yyyy-MM-dd") + "T00:00:00.000Z"assumes the DB always returns midnight UTC.
On machines whose default TZ ≠ UTC the insertedDATEliteral may resolve to a different ISO string, causing false negatives.Consider normalising both expected and actual dates with
utcToZonedTime/startOfDay(UTC)or compare viaisSameDayinstead of string equality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (19)
apps/api/src/app.ts(2 hunks)apps/api/src/provider/controllers/provider-graph-data/provider-graph-data.controller.ts(1 hunks)apps/api/src/provider/http-schemas/provider-graph-data.schema.ts(1 hunks)apps/api/src/provider/routes/index.ts(1 hunks)apps/api/src/provider/routes/provider-graph-data/provider-graph-data.router.ts(1 hunks)apps/api/src/provider/services/provider-graph-data/provider-graph-data.service.ts(1 hunks)apps/api/src/provider/services/provider-stats/provider-stats.service.ts(0 hunks)apps/api/src/routes/v1/dashboardData.ts(2 hunks)apps/api/src/routes/v1/index.ts(0 hunks)apps/api/src/routes/v1/providerGraphData.ts(0 hunks)apps/api/src/services/db/statsService.ts(1 hunks)apps/api/test/functional/dashboard-data.spec.ts(1 hunks)apps/api/test/functional/provider-graph-data.spec.ts(1 hunks)apps/api/test/functional/provider-versions.spec.ts(2 hunks)apps/api/test/functional/providers.spec.ts(2 hunks)apps/api/test/seeders/block.seeder.ts(1 hunks)apps/api/test/seeders/day.seeder.ts(1 hunks)apps/api/test/seeders/provider-snapshot.seeder.ts(1 hunks)apps/api/test/seeders/provider.seeder.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/api/src/provider/services/provider-stats/provider-stats.service.ts
- apps/api/src/routes/v1/index.ts
- apps/api/src/routes/v1/providerGraphData.ts
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/app.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/api/src/provider/routes/index.ts
- apps/api/test/seeders/provider.seeder.ts
- apps/api/test/functional/provider-versions.spec.ts
- apps/api/src/routes/v1/dashboardData.ts
- apps/api/src/services/db/statsService.ts
- apps/api/test/seeders/block.seeder.ts
- apps/api/test/functional/providers.spec.ts
- apps/api/src/provider/controllers/provider-graph-data/provider-graph-data.controller.ts
- apps/api/src/provider/routes/provider-graph-data/provider-graph-data.router.ts
- apps/api/test/seeders/provider-snapshot.seeder.ts
- apps/api/test/functional/dashboard-data.spec.ts
- apps/api/test/seeders/day.seeder.ts
- apps/api/src/provider/http-schemas/provider-graph-data.schema.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/api/test/functional/provider-graph-data.spec.ts (5)
apps/api/src/app.ts (1)
initDb(167-179)apps/api/test/seeders/day.seeder.ts (1)
DaySeeder(5-22)apps/api/test/seeders/provider.seeder.ts (1)
ProviderSeeder(5-42)apps/api/test/seeders/provider-snapshot.seeder.ts (1)
ProviderSnapshotSeeder(5-38)apps/api/test/seeders/block.seeder.ts (1)
BlockSeeder(8-36)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: validate-api
- GitHub Check: test-api-build
| currentValue: (typeof currentValue[dataName] === "number" ? currentValue[dataName] : parseInt(currentValue[dataName] as string)) as number, | ||
| compareValue: (typeof compareValue[dataName] === "number" ? compareValue[dataName] : parseInt(compareValue[dataName] as string)) as number, | ||
| snapshots: stats, | ||
| now: { | ||
| count: currentValue.count, | ||
| cpu: parseInt(currentValue.cpu), | ||
| gpu: parseInt(currentValue.gpu), | ||
| memory: parseInt(currentValue.memory), | ||
| storage: parseInt(currentValue.storage) | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Convert all numeric strings consistently and specify radix in parseInt.
currentValue.count/compareValue.count are left as raw strings while the other fields are coerced to numbers.
This creates an inconsistent response shape (count can be a string) and surprise-casts on the consumer side.
Additionally, parseInt is called without a radix, which can lead to silent octal parsing when the value starts with 0.
- now: {
- count: currentValue.count,
- cpu: parseInt(currentValue.cpu),
- gpu: parseInt(currentValue.gpu),
- memory: parseInt(currentValue.memory),
- storage: parseInt(currentValue.storage)
- },
+ now: {
+ count: Number.parseInt(currentValue.count as unknown as string, 10),
+ cpu: Number.parseInt(currentValue.cpu as string, 10),
+ gpu: Number.parseInt(currentValue.gpu as string, 10),
+ memory: Number.parseInt(currentValue.memory as string, 10),
+ storage: Number.parseInt(currentValue.storage as string, 10)
+ },
compare: {
- count: compareValue.count,
- cpu: parseInt(compareValue.cpu),
- gpu: parseInt(compareValue.gpu),
- memory: parseInt(compareValue.memory),
- storage: parseInt(compareValue.storage)
+ count: Number.parseInt(compareValue.count as unknown as string, 10),
+ cpu: Number.parseInt(compareValue.cpu as string, 10),
+ gpu: Number.parseInt(compareValue.gpu as string, 10),
+ memory: Number.parseInt(compareValue.memory as string, 10),
+ storage: Number.parseInt(compareValue.storage as string, 10)
}📝 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.
| currentValue: (typeof currentValue[dataName] === "number" ? currentValue[dataName] : parseInt(currentValue[dataName] as string)) as number, | |
| compareValue: (typeof compareValue[dataName] === "number" ? compareValue[dataName] : parseInt(compareValue[dataName] as string)) as number, | |
| snapshots: stats, | |
| now: { | |
| count: currentValue.count, | |
| cpu: parseInt(currentValue.cpu), | |
| gpu: parseInt(currentValue.gpu), | |
| memory: parseInt(currentValue.memory), | |
| storage: parseInt(currentValue.storage) | |
| }, | |
| currentValue: (typeof currentValue[dataName] === "number" | |
| ? currentValue[dataName] | |
| : parseInt(currentValue[dataName] as string)) as number, | |
| compareValue: (typeof compareValue[dataName] === "number" | |
| ? compareValue[dataName] | |
| : parseInt(compareValue[dataName] as string)) as number, | |
| snapshots: stats, | |
| now: { | |
| count: Number.parseInt(currentValue.count as unknown as string, 10), | |
| cpu: Number.parseInt(currentValue.cpu as string, 10), | |
| gpu: Number.parseInt(currentValue.gpu as string, 10), | |
| memory: Number.parseInt(currentValue.memory as string, 10), | |
| storage: Number.parseInt(currentValue.storage as string, 10) | |
| }, | |
| compare: { | |
| count: Number.parseInt(compareValue.count as unknown as string, 10), | |
| cpu: Number.parseInt(compareValue.cpu as string, 10), | |
| gpu: Number.parseInt(compareValue.gpu as string, 10), | |
| memory: Number.parseInt(compareValue.memory as string, 10), | |
| storage: Number.parseInt(compareValue.storage as string, 10) | |
| } |
🤖 Prompt for AI Agents
In
apps/api/src/provider/services/provider-graph-data/provider-graph-data.service.ts
around lines 68 to 77, ensure all numeric string fields including count are
consistently converted to numbers to avoid inconsistent response shapes. Update
all parseInt calls to include a radix of 10 to prevent unintended octal parsing.
Replace raw string usage of count with parseInt(count, 10) or equivalent numeric
conversion for consistency.
e57a8d7 to
a310b44
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/api/src/provider/services/provider-graph-data/provider-graph-data.service.ts (2)
71-84: Convert all numeric strings consistently and specify radix in parseIntThe code doesn't convert
countfields to numbers while other fields are converted, leading to inconsistent response types. Additionally, allparseIntcalls should include a radix parameter.now: { - count: currentValue.count, - cpu: parseInt(currentValue.cpu), - gpu: parseInt(currentValue.gpu), - memory: parseInt(currentValue.memory), - storage: parseInt(currentValue.storage) + count: Number(currentValue.count), + cpu: Number.parseInt(currentValue.cpu as string, 10), + gpu: Number.parseInt(currentValue.gpu as string, 10), + memory: Number.parseInt(currentValue.memory as string, 10), + storage: Number.parseInt(currentValue.storage as string, 10) }, compare: { - count: compareValue.count, - cpu: parseInt(compareValue.cpu), - gpu: parseInt(compareValue.gpu), - memory: parseInt(compareValue.memory), - storage: parseInt(compareValue.storage) + count: Number(compareValue.count), + cpu: Number.parseInt(compareValue.cpu as string, 10), + gpu: Number.parseInt(compareValue.gpu as string, 10), + memory: Number.parseInt(compareValue.memory as string, 10), + storage: Number.parseInt(compareValue.storage as string, 10) }
88-98: Fix type checking for lastItem.dateBased on previous discussions in the PR,
lastItem.dateis actually a Date object, not a string. The type check should be updated to verify it's a Date instance instead of checking if it's a string.private removeLastAroundMidnight(stats: ProviderStats[]) { const now = new Date(); const isFirstFifteenMinuesOfDay = now.getHours() === 0 && now.getMinutes() <= 15; const lastItem = stats.length > 0 ? stats[stats.length - 1] : null; - if (lastItem && typeof lastItem.date !== "string") { + if (lastItem && !(lastItem.date instanceof Date)) { console.error( - `removeLastAroundMidnight: lastItem.date is not a string: ` + + `removeLastAroundMidnight: lastItem.date is not a Date object: ` + `type = ${typeof lastItem.date} value = ${lastItem.date} constructor = ${(lastItem.date as any)?.constructor?.name}` + JSON.stringify(lastItem) ); }
🧹 Nitpick comments (2)
apps/api/src/provider/services/provider-graph-data/provider-graph-data.service.ts (2)
90-90: Fix typo in variable nameThere's a typo in the variable name "isFirstFifteenMinuesOfDay" (should be "Minutes" not "Minues").
- const isFirstFifteenMinuesOfDay = now.getHours() === 0 && now.getMinutes() <= 15; + const isFirstFifteenMinutesOfDay = now.getHours() === 0 && now.getMinutes() <= 15;
101-101: Update variable name referenceIf you fix the typo in the variable name as suggested above, don't forget to update the reference here as well.
- if (isFirstFifteenMinuesOfDay && lastItemIsForToday) { + if (isFirstFifteenMinutesOfDay && lastItemIsForToday) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/api/src/app.ts(2 hunks)apps/api/src/provider/controllers/provider-graph-data/provider-graph-data.controller.ts(1 hunks)apps/api/src/provider/http-schemas/provider-graph-data.schema.ts(1 hunks)apps/api/src/provider/routes/index.ts(1 hunks)apps/api/src/provider/routes/provider-graph-data/provider-graph-data.router.ts(1 hunks)apps/api/src/provider/services/provider-graph-data/provider-graph-data.service.ts(1 hunks)apps/api/src/provider/services/provider-stats/provider-stats.service.ts(0 hunks)apps/api/src/routes/v1/dashboardData.ts(2 hunks)apps/api/src/routes/v1/index.ts(0 hunks)apps/api/src/routes/v1/providerGraphData.ts(0 hunks)apps/api/src/services/db/statsService.ts(1 hunks)apps/api/test/functional/dashboard-data.spec.ts(1 hunks)apps/api/test/functional/provider-graph-data.spec.ts(1 hunks)apps/api/test/functional/provider-versions.spec.ts(2 hunks)apps/api/test/functional/providers.spec.ts(2 hunks)apps/api/test/seeders/block.seeder.ts(1 hunks)apps/api/test/seeders/day.seeder.ts(1 hunks)apps/api/test/seeders/provider-snapshot.seeder.ts(1 hunks)apps/api/test/seeders/provider.seeder.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/api/src/routes/v1/index.ts
- apps/api/src/provider/services/provider-stats/provider-stats.service.ts
- apps/api/src/routes/v1/providerGraphData.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- apps/api/src/app.ts
- apps/api/test/seeders/provider.seeder.ts
- apps/api/src/provider/controllers/provider-graph-data/provider-graph-data.controller.ts
- apps/api/test/functional/providers.spec.ts
- apps/api/test/functional/provider-versions.spec.ts
- apps/api/test/seeders/day.seeder.ts
- apps/api/src/provider/routes/index.ts
- apps/api/src/services/db/statsService.ts
- apps/api/src/provider/routes/provider-graph-data/provider-graph-data.router.ts
- apps/api/test/functional/provider-graph-data.spec.ts
- apps/api/test/seeders/provider-snapshot.seeder.ts
- apps/api/test/seeders/block.seeder.ts
- apps/api/src/routes/v1/dashboardData.ts
- apps/api/src/provider/http-schemas/provider-graph-data.schema.ts
- apps/api/test/functional/dashboard-data.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test-api-build
- GitHub Check: validate-api
| export class ProviderGraphDataService { | ||
| @Memoize({ ttlInSeconds: minutesToSeconds(5) }) | ||
| async getProviderGraphData(dataName: ProviderStatsKey): Promise<ProviderGraphDataResponse> { | ||
| const getter = (block: ProviderStats) => (typeof block[dataName] === "number" ? (block[dataName] as number) : parseInt(block[dataName] as string) || 0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add radix parameter to parseInt for safer numeric conversion
The parseInt function should include a radix parameter to avoid unintended parsing behavior, especially when the string may start with '0'.
- const getter = (block: ProviderStats) => (typeof block[dataName] === "number" ? (block[dataName] as number) : parseInt(block[dataName] as string) || 0);
+ const getter = (block: ProviderStats) => (typeof block[dataName] === "number" ? (block[dataName] as number) : parseInt(block[dataName] as string, 10) || 0);📝 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.
| const getter = (block: ProviderStats) => (typeof block[dataName] === "number" ? (block[dataName] as number) : parseInt(block[dataName] as string) || 0); | |
| const getter = (block: ProviderStats) => | |
| typeof block[dataName] === "number" | |
| ? (block[dataName] as number) | |
| : parseInt(block[dataName] as string, 10) || 0; |
🤖 Prompt for AI Agents
In
apps/api/src/provider/services/provider-graph-data/provider-graph-data.service.ts
at line 15, the parseInt call lacks a radix parameter, which can cause incorrect
parsing of strings starting with '0'. Fix this by adding the radix parameter 10
to the parseInt call to ensure decimal parsing, changing
parseInt(block[dataName] as string) to parseInt(block[dataName] as string, 10).
| currentValue: (typeof currentValue[dataName] === "number" ? currentValue[dataName] : parseInt(currentValue[dataName] as string)) as number, | ||
| compareValue: (typeof compareValue[dataName] === "number" ? compareValue[dataName] : parseInt(compareValue[dataName] as string)) as number, | ||
| snapshots: stats, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Number.parseInt with radix parameter for safe numeric conversion
Current code is missing radix parameter in parseInt calls which can lead to unexpected parsing behavior with strings that have leading zeros.
- currentValue: (typeof currentValue[dataName] === "number" ? currentValue[dataName] : parseInt(currentValue[dataName] as string)) as number,
- compareValue: (typeof compareValue[dataName] === "number" ? compareValue[dataName] : parseInt(compareValue[dataName] as string)) as number,
+ currentValue: (typeof currentValue[dataName] === "number" ? currentValue[dataName] : Number.parseInt(currentValue[dataName] as string, 10)) as number,
+ compareValue: (typeof compareValue[dataName] === "number" ? compareValue[dataName] : Number.parseInt(compareValue[dataName] as string, 10)) as number,📝 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.
| currentValue: (typeof currentValue[dataName] === "number" ? currentValue[dataName] : parseInt(currentValue[dataName] as string)) as number, | |
| compareValue: (typeof compareValue[dataName] === "number" ? compareValue[dataName] : parseInt(compareValue[dataName] as string)) as number, | |
| snapshots: stats, | |
| currentValue: (typeof currentValue[dataName] === "number" | |
| ? currentValue[dataName] | |
| : Number.parseInt(currentValue[dataName] as string, 10)) as number, | |
| compareValue: (typeof compareValue[dataName] === "number" | |
| ? compareValue[dataName] | |
| : Number.parseInt(compareValue[dataName] as string, 10)) as number, | |
| snapshots: stats, |
🤖 Prompt for AI Agents
In
apps/api/src/provider/services/provider-graph-data/provider-graph-data.service.ts
around lines 68 to 70, the parseInt calls lack the radix parameter, which can
cause incorrect parsing of strings with leading zeros. Update both parseInt
calls to include the radix parameter 10 to ensure decimal parsing, e.g., use
Number.parseInt(value, 10).
a310b44 to
1d94762
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/api/test/functional/provider-graph-data.spec.ts (1)
174-179:⚠️ Potential issueThe unawaited provider updates issue still exists.
The calls to
providers[0].updateandproviders[1].updateare not awaited, which can cause race conditions in tests as previously identified.Apply this diff to fix the race condition:
- providers[0].update({ - lastSuccessfulSnapshotId: providerSnapshots[0].id - }); - providers[1].update({ - lastSuccessfulSnapshotId: providerSnapshots[2].id - }); + await Promise.all([ + providers[0].update({ + lastSuccessfulSnapshotId: providerSnapshots[0].id + }), + providers[1].update({ + lastSuccessfulSnapshotId: providerSnapshots[2].id + }) + ]);
🧹 Nitpick comments (1)
apps/api/test/functional/provider-graph-data.spec.ts (1)
24-43: Consider extracting magic numbers to constants.The block height ranges and date setup contain hardcoded values that could benefit from named constants for better maintainability and readability.
+const BLOCK_HEIGHTS = { + THREE_DAYS_AGO: { first: 1, last: 100 }, + TWO_DAYS_AGO: { first: 101, last: 200 }, + YESTERDAY: { first: 201, last: 300 } +} as const; await Promise.all([ DaySeeder.createInDatabase({ date: format(threeDaysAgo, "yyyy-MM-dd"), - firstBlockHeight: 1, - lastBlockHeight: 100, - lastBlockHeightYet: 100 + firstBlockHeight: BLOCK_HEIGHTS.THREE_DAYS_AGO.first, + lastBlockHeight: BLOCK_HEIGHTS.THREE_DAYS_AGO.last, + lastBlockHeightYet: BLOCK_HEIGHTS.THREE_DAYS_AGO.last }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/api/src/app.ts(2 hunks)apps/api/src/provider/controllers/provider-graph-data/provider-graph-data.controller.ts(1 hunks)apps/api/src/provider/http-schemas/provider-graph-data.schema.ts(1 hunks)apps/api/src/provider/routes/index.ts(1 hunks)apps/api/src/provider/routes/provider-graph-data/provider-graph-data.router.ts(1 hunks)apps/api/src/provider/services/provider-graph-data/provider-graph-data.service.ts(1 hunks)apps/api/src/provider/services/provider-stats/provider-stats.service.ts(0 hunks)apps/api/src/routes/v1/dashboardData.ts(2 hunks)apps/api/src/routes/v1/index.ts(0 hunks)apps/api/src/routes/v1/providerGraphData.ts(0 hunks)apps/api/src/services/db/statsService.ts(1 hunks)apps/api/test/functional/dashboard-data.spec.ts(1 hunks)apps/api/test/functional/provider-graph-data.spec.ts(1 hunks)apps/api/test/functional/provider-versions.spec.ts(2 hunks)apps/api/test/functional/providers.spec.ts(2 hunks)apps/api/test/seeders/block.seeder.ts(1 hunks)apps/api/test/seeders/day.seeder.ts(1 hunks)apps/api/test/seeders/provider-snapshot.seeder.ts(1 hunks)apps/api/test/seeders/provider.seeder.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/api/src/provider/services/provider-stats/provider-stats.service.ts
- apps/api/src/routes/v1/index.ts
- apps/api/src/routes/v1/providerGraphData.ts
✅ Files skipped from review due to trivial changes (1)
- apps/api/test/seeders/day.seeder.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/api/test/seeders/provider.seeder.ts
- apps/api/src/app.ts
- apps/api/src/provider/routes/index.ts
- apps/api/test/functional/provider-versions.spec.ts
- apps/api/src/services/db/statsService.ts
- apps/api/src/routes/v1/dashboardData.ts
- apps/api/src/provider/controllers/provider-graph-data/provider-graph-data.controller.ts
- apps/api/test/seeders/block.seeder.ts
- apps/api/src/provider/routes/provider-graph-data/provider-graph-data.router.ts
- apps/api/test/seeders/provider-snapshot.seeder.ts
- apps/api/test/functional/providers.spec.ts
- apps/api/test/functional/dashboard-data.spec.ts
- apps/api/src/provider/services/provider-graph-data/provider-graph-data.service.ts
- apps/api/src/provider/http-schemas/provider-graph-data.schema.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/api/test/functional/provider-graph-data.spec.ts (5)
apps/api/src/app.ts (1)
initDb(170-182)apps/api/test/seeders/day.seeder.ts (1)
DaySeeder(5-22)apps/api/test/seeders/provider.seeder.ts (1)
ProviderSeeder(5-42)apps/api/test/seeders/provider-snapshot.seeder.ts (1)
ProviderSnapshotSeeder(5-38)apps/api/test/seeders/block.seeder.ts (1)
BlockSeeder(8-36)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test-api-build
- GitHub Check: validate-api
🔇 Additional comments (3)
apps/api/test/functional/provider-graph-data.spec.ts (3)
210-221: LGTM! Test assertions and expected values are mathematically correct.The expected values properly sum all resource types (active + pending + available) across both providers:
- CPU "now": (301+306+311) + (501+506+511) = 2436 ✓
- Storage "now": (304+305+309+310+314+315) + (504+505+509+510+514+515) = 4914 ✓
The test structure with dynamic snapshot validation is well-designed.
230-274: Excellent edge case testing for data exclusion.The test correctly verifies that snapshots from the first 15 minutes of the current day are excluded from graph data. The setup with
today.setHours(0, 14, 59, 999)precisely targets the boundary condition, and the assertion confirms the latest snapshot remains from yesterday.
1-11: Well-structured imports and dependencies.The imports are properly organized and include all necessary components for comprehensive testing. The use of date-fns for date manipulation and the seeder classes follows established patterns in the codebase.
490cbe0 to
d9667fc
Compare
d9667fc to
3c55faf
Compare
refs #1272
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Chores