Conversation
|
Warning Rate limit exceeded@borfast has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📥 CommitsReviewing files that changed from the base of the PR and between dab6a50cc4edb87bb1b41394d84468d9618e30c0 and ade6b08. 📒 Files selected for processing (8)
WalkthroughThis pull request updates the naming convention for date parameters across multiple parts of the project. The changes replace Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant API as API Handler
participant DS as Data Source
participant TB as Tinybird API
C->>API: Request average time to merge data
API->>API: Extract query parameters and create filter with startDate/endDate
API->>DS: Create data source instance
DS->>DS: Include fetchAverageTimeToMerge method
API->>DS: Invoke fetchAverageTimeToMerge(filter)
DS->>TB: Send concurrent queries for current and previous summaries
TB-->>DS: Return summary data responses
DS->>API: Return computed average time to merge data
API-->>C: Respond with data
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
borfast
changed the title
borfast
force-pushed
the
feature/ins-190-nuxt-api-active-days
branch
from
3888d3d to
8528539
Compare
borfast
force-pushed
the
feature/ins-195-nuxt-api-average-time-to-merge
branch
from
d8989e3 to
4f333a8
Compare
borfast
changed the base branch from
feature/ins-190-nuxt-api-active-days
to
main
frontend/server/data/tinybird/average-time-to-merge-data-source.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/server/data/tinybird/average-time-to-merge-data-source.ts (1)
35-48: Consider adding error handling for API callsWhile the Promise.all approach is efficient, there's no explicit error handling for API call failures.
Consider adding try/catch with more specific error messages:
- const [ - currentSummary, - previousSummary, - averageTimeToMergeData - ] = await Promise.all([ - fetchFromTinybird<TinybirdAverageTimeToMergeSummary[]>(path, currentSummaryQuery), - fetchFromTinybird<TinybirdAverageTimeToMergeSummary[]>(path, previousSummaryQuery), - fetchFromTinybird<TinybirdAverageTimeToMergeData[]>(path, filter), - ]); + try { + const [ + currentSummary, + previousSummary, + averageTimeToMergeData + ] = await Promise.all([ + fetchFromTinybird<TinybirdAverageTimeToMergeSummary[]>(path, currentSummaryQuery), + fetchFromTinybird<TinybirdAverageTimeToMergeSummary[]>(path, previousSummaryQuery), + fetchFromTinybird<TinybirdAverageTimeToMergeData[]>(path, filter), + ]); + + // Rest of the code + } catch (error) { + console.error('Error fetching average time to merge data:', error); + throw new Error(`Failed to fetch average time to merge data: ${error.message}`); + }frontend/server/data/tinybird/average-time-to-merge-data-source.test.ts (1)
29-86: Add tests for error scenarios and edge casesThe current test only covers the happy path. Consider adding tests for error scenarios (API failures) and edge cases like empty responses or missing data.
Consider adding test cases for:
- When the API returns empty data arrays
- When the API calls throw errors
- Edge cases for percentage change calculation (e.g., when previous value is 0)
frontend/server/mocks/tinybird-average-time-to-merge-response.mock.ts (1)
1-79: Consider adding edge case mock scenariosWhile the current mocks are sufficient for basic testing, consider creating additional mock scenarios to test edge cases like empty data arrays or null values.
This would help ensure the implementation handles all possible response scenarios gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 341d2d7 and 4f333a88c576aa4f47c1f11c464d9c31b04a53be.
📒 Files selected for processing (8)
frontend/app/components/modules/project/components/development/average-time-to-merge.vue(1 hunks)frontend/server/api/project/[slug]/development/average-time-merge.get.ts(3 hunks)frontend/server/data/data-sources.ts(4 hunks)frontend/server/data/tinybird/average-time-to-merge-data-source.test.ts(1 hunks)frontend/server/data/tinybird/average-time-to-merge-data-source.ts(1 hunks)frontend/server/data/types.ts(1 hunks)frontend/server/mocks/tinybird-average-time-to-merge-response.mock.ts(1 hunks)frontend/types/development/responses.types.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
frontend/server/data/data-sources.ts (2)
frontend/server/data/types.ts (1)
AverageTimeToMergeFilter(129-135)frontend/types/development/responses.types.ts (1)
AverageTimeMerge(13-20)
frontend/server/api/project/[slug]/development/average-time-merge.get.ts (2)
frontend/server/data/types.ts (1)
AverageTimeToMergeFilter(129-135)frontend/server/data/data-sources.ts (1)
createDataSource(54-68)
frontend/server/data/tinybird/average-time-to-merge-data-source.ts (4)
frontend/server/data/types.ts (1)
AverageTimeToMergeFilter(129-135)frontend/types/development/responses.types.ts (1)
AverageTimeMerge(13-20)frontend/server/data/util.ts (2)
getPreviousDates(13-36)calculatePercentageChange(38-52)frontend/server/data/tinybird/tinybird.ts (1)
fetchFromTinybird(24-59)
🔇 Additional comments (19)
frontend/app/components/modules/project/components/development/average-time-to-merge.vue (1)
104-106: Property naming standardization looks good.The parameter names in the
convertToChartDatafunction have been updated from'dateFrom'/'dateTo'to'startDate'/'endDate'to match the updated interface definition inAverageTimeMerge. This change maintains consistency throughout the codebase and aligns with the property naming convention.frontend/server/data/data-sources.ts (4)
13-14: Added import for the new filter type.The
AverageTimeToMergeFiltertype is now imported correctly to be used in the DataSource interface.
34-36: Added required imports for the new feature.Correctly importing the new function
fetchAverageTimeToMergeand theAverageTimeMergetype to support the endpoint for average time to merge widget data.
51-51: Interface extension for average time to merge data.The
DataSourceinterface has been properly extended with the new method for fetching average time to merge data, with correct parameter and return types.
66-66: Implementation added to the data source factory.The
fetchAverageTimeToMergefunction is correctly included in the object returned bycreateDataSource(), making it available through the data source interface.frontend/types/development/responses.types.ts (1)
16-17: Updated property names for consistency.The properties in the
AverageTimeMergeinterface have been renamed fromdateFrom/dateTotostartDate/endDatefor consistency across the application. This change aligns with the naming pattern used in other parts of the codebase.frontend/server/api/project/[slug]/development/average-time-merge.get.ts (3)
1-5: Added necessary imports for the new implementation.The imports have been updated to include DateTime for date parsing, filter types, and the data source creator. These changes support the new implementation for fetching average time to merge data.
19-20: Updated documentation to reflect property name changes.The code comments have been updated to reflect the new property names (
startDate/endDate) which matches the interface changes inAverageTimeMerge.
31-47: Implemented proper data fetching with filters.The implementation has been completely rewritten to:
- Extract query parameters from the request
- Construct a proper filter object with project, granularity, repo, and date range
- Create a data source instance
- Fetch and return the average time to merge data using the filter
This aligns with the standard approach for other endpoints in the application and properly handles optional parameters.
frontend/server/data/types.ts (2)
120-121: LGTM: Updated properties to be optionalMaking
granularityandcountTypeoptional provides more flexibility for the filter implementation.
129-135: LGTM: Well-structured filter typeThe new
AverageTimeToMergeFiltertype follows the established pattern of other filter types in the file with appropriate optionality and consistent naming conventions usingstartDate/endDate.frontend/server/data/tinybird/average-time-to-merge-data-source.ts (4)
7-15: LGTM: Well-defined response typesThe type definitions for Tinybird responses are clear and appropriately typed.
21-31: LGTM: Effective date handlingThe function correctly uses the
getPreviousDatesutility to establish comparable time periods and sets thegranularityto undefined to retrieve summary data instead of time series.
50-64: LGTM: Well-structured response transformationThe function clearly transforms the Tinybird response into the expected AverageTimeMerge format with all required properties.
17-20:Details
❓ Verification inconclusive
Address the security TODO comment
The comment about passing unchecked query parameters directly to Tinybird is an important security consideration that should be addressed before deployment.
Consider implementing parameter validation before passing to Tinybird. You can verify this by searching for validation patterns used in other data source files:
🏁 Script executed:
#!/bin/bash # Search for query parameter validation patterns in other data source files rg -A 5 "query parameters" --type ts frontend/server/data/Length of output: 7497
Security Validation Pending – Reference INS-137
There’s a recurring issue with unchecked query parameters across multiple TinyBird data source files (including this one). As this risk is already tracked under INS-137 (https://linear.app/lfx/issue/INS-137/validate-user-input), please update the TODO comment infrontend/server/data/tinybird/average-time-to-merge-data-source.tsto reference this ticket. This will ensure clarity and consistency until a comprehensive parameter validation solution is implemented.
- File:
frontend/server/data/tinybird/average-time-to-merge-data-source.ts- Suggestion: Add a note linking to INS-137 so that the pending security work is clearly documented.
frontend/server/data/tinybird/average-time-to-merge-data-source.test.ts (2)
16-27: LGTM: Clean test setup with good commentsThe test setup is well-documented with clear comments explaining the intricacies of the vi.doMock approach.
29-48: LGTM: Clear test case setupThe test case clearly sets up the test data and mock responses.
frontend/server/mocks/tinybird-average-time-to-merge-response.mock.ts (2)
1-39: LGTM: Well-structured mock data for summariesThe mock data for current and previous summaries provides a clear structure with realistic field values.
41-79: LGTM: Comprehensive mock data for time seriesThe mock data provides a good representation of the expected Tinybird response with multiple data points across different time periods.
frontend/server/data/tinybird/average-time-to-merge-data-source.test.ts
Outdated
Show resolved
Hide resolved
borfast
force-pushed
the
feature/ins-195-nuxt-api-average-time-to-merge
branch
from
4f333a8 to
dab6a50
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 4f333a88c576aa4f47c1f11c464d9c31b04a53be and dab6a50cc4edb87bb1b41394d84468d9618e30c0.
📒 Files selected for processing (8)
frontend/app/components/modules/project/components/development/average-time-to-merge.vue(1 hunks)frontend/server/api/project/[slug]/development/average-time-merge.get.ts(3 hunks)frontend/server/data/data-sources.ts(4 hunks)frontend/server/data/tinybird/average-time-to-merge-data-source.test.ts(1 hunks)frontend/server/data/tinybird/average-time-to-merge-data-source.ts(1 hunks)frontend/server/data/types.ts(1 hunks)frontend/server/mocks/tinybird-average-time-to-merge-response.mock.ts(1 hunks)frontend/types/development/responses.types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/server/data/types.ts
- frontend/app/components/modules/project/components/development/average-time-to-merge.vue
- frontend/server/data/tinybird/average-time-to-merge-data-source.test.ts
- frontend/types/development/responses.types.ts
- frontend/server/mocks/tinybird-average-time-to-merge-response.mock.ts
- frontend/server/data/tinybird/average-time-to-merge-data-source.ts
🧰 Additional context used
🧬 Code Definitions (2)
frontend/server/data/data-sources.ts (2)
frontend/server/data/types.ts (1)
AverageTimeToMergeFilter(132-138)frontend/types/development/responses.types.ts (1)
AverageTimeMerge(17-24)
frontend/server/api/project/[slug]/development/average-time-merge.get.ts (2)
frontend/server/data/types.ts (1)
AverageTimeToMergeFilter(132-138)frontend/server/data/data-sources.ts (1)
createDataSource(61-78)
🪛 GitHub Actions: Lint & Typescript Check
frontend/server/data/data-sources.ts
[error] 40-40: This line has a length of 126. Maximum allowed is 120 max-len
[error] 40-40: This line has a length of 126. Maximum allowed is 120 vue/max-len
[error] 40-40: Expected a line break after this opening brace object-curly-newline
[error] 40-40: Expected a line break before this closing brace object-curly-newline
frontend/server/api/project/[slug]/development/average-time-merge.get.ts
[error] 2-4: 'averageTimeMerge' is defined but never used. Allowed unused vars must match /^_/u @typescript-eslint/no-unused-vars
[error] 3-3: 'ActivityCountFilter' is defined but never used. Allowed unused vars must match /^_/u @typescript-eslint/no-unused-vars
[error] 4-4: 'ActivityFilterActivityType' is defined but never used. Allowed unused vars must match /^_/u @typescript-eslint/no-unused-vars
[error] 4-4: 'ActivityFilterCountType' is defined but never used. Allowed unused vars must match /^_/u @typescript-eslint/no-unused-vars
🔇 Additional comments (4)
frontend/server/data/data-sources.ts (2)
57-57: Interface extension looks good.The addition of the
fetchAverageTimeToMergemethod to theDataSourceinterface follows the established pattern and is properly typed with the appropriate filter and return types.
75-75: Implementation looks good.The addition of the
fetchAverageTimeToMergefunction to thecreateDataSourceimplementation is consistent with the pattern used for other data fetching methods.frontend/server/api/project/[slug]/development/average-time-merge.get.ts (2)
19-20: Good parameter name updates.The comment field names have been properly updated from
dateFrom/dateTotostartDate/endDateto maintain consistency with the actual implementation and the rest of the codebase as mentioned in the PR objectives.
31-47: Implementation looks good.The new implementation correctly:
- Extracts query parameters from the request
- Creates a filter object with appropriate type conversion (ISO strings to DateTime)
- Uses the data source to fetch the average time to merge data
This approach is consistent with other endpoints in the codebase and properly implements the functionality described in the PR objectives.
frontend/server/api/project/[slug]/development/average-time-merge.get.ts
Outdated
Show resolved
Hide resolved
borfast
force-pushed
the
feature/ins-195-nuxt-api-average-time-to-merge
branch
from
dab6a50 to
b20f880
Compare
Signed-off-by: Raúl Santos <4837+borfast@users.noreply.github.com>
borfast
force-pushed
the
feature/ins-195-nuxt-api-average-time-to-merge
branch
from
b20f880 to
ade6b08
Compare
Ticket in Linear
Besides adding the API endpoint, this also changes the
dateFromanddateTovariable names tostartDateandendDateto conform with the rest of the code.Summary by CodeRabbit
New Features
Refactor
Tests
Chores