Conversation
WalkthroughThe pull request removes a redundant comment from an API endpoint and extends the project’s data retrieval capabilities for contributor leaderboard data. It updates the DataSource interface to include a new method, adds a new Tinybird module for handling leaderboard data with defined types, refines query parameter filtering in the Tinybird fetch function, and introduces new types for filtering based on contributors leaderboard metrics. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Endpoint (/project/[slug]/contributors/contributor-leaderboard.get.ts)
participant DS as DataSource
participant TL as Tinybird Leaderboard Module
participant TB as Tinybird Service
Client->>API: Request contributor leaderboard
API->>DS: fetchContributorsLeaderboard(filter)
DS->>TL: fetchContributorsLeaderboard(filter)
TL->>TB: Execute query with filtered parameters
TB-->>TL: Return raw leaderboard data
TL-->>DS: Return processed leaderboard data
DS-->>API: Return leaderboard response
API-->>Client: Send leaderboard response
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
frontend/server/data/tinybird/active-contributors-data-source.ts (1)
43-112: 🛠️ Refactor suggestionMissing error handling for API calls.
The function doesn't include error handling for the Promise.all or any of the API calls.
export async function fetchActiveContributors(filter: ActiveContributorsFilter) { const dates = getPreviousDates(filter.startDate, filter.endDate); // TODO: We're passing unchecked query parameters to TinyBird directly from the frontend. // We need to ensure this doesn't pose a security risk. const currentSummaryQuery = { project: filter.project, repo: filter.repo, startDate: dates.current.from, endDate: dates.current.to }; // Rest of setup code... + try { const [currentSummary, previousSummary, data] = await Promise.all([ fetchFromTinybird<TinybirdActiveContributorsSummary>('/v0/pipes/active_contributors.json', currentSummaryQuery), fetchFromTinybird<TinybirdActiveContributorsSummary>('/v0/pipes/active_contributors.json', previousSummaryQuery), fetchFromTinybird<TinybirdActiveContributorsData>('/v0/pipes/active_contributors.json', dataQuery) ]); // Process data and build response... return response; + } catch (error) { + console.error('Error fetching active contributors:', error); + throw new Error('Failed to fetch active contributors data'); + } }
🧹 Nitpick comments (1)
frontend/server/api/project/[slug]/contributors/contributor-leaderboard.get.ts (1)
53-56: Consider adding pagination support from query parameters.The meta object uses hardcoded values for offset and limit rather than using the values from query parameters.
const query = getQuery(event); + const offset = Number(query.offset) || 0; + const limit = Number(query.limit) || 10; + const meta = { - offset: 0, - limit: 10, + offset, + limit, total: 100 };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 332b473 and 0ff84d25db218f2f054235ce0dc48e5fd98555e8.
📒 Files selected for processing (9)
frontend/server/api/project/[slug]/contributors/active-contributors.get.ts(2 hunks)frontend/server/api/project/[slug]/contributors/contributor-leaderboard.get.ts(2 hunks)frontend/server/data/active-contributors-data-source.ts(0 hunks)frontend/server/data/data-sources.ts(1 hunks)frontend/server/data/tinybird/active-contributors-data-source.test.ts(1 hunks)frontend/server/data/tinybird/active-contributors-data-source.ts(2 hunks)frontend/server/data/tinybird/contributors-leaderboard-source.ts(1 hunks)frontend/server/data/tinybird/tinybird.ts(1 hunks)frontend/server/data/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/server/data/active-contributors-data-source.ts
🔇 Additional comments (13)
frontend/server/api/project/[slug]/contributors/active-contributors.get.ts (2)
28-28: Refactoring data source import is a clean approach.The change to import
createDataSourcefrom the centralized data sources file aligns well with the refactoring goals and follows good software design principles by centralizing data source management.
59-59: Successfully updated the data source instantiation.The implementation correctly uses the new
createDataSource()factory function which ensures consistent data fetching behavior with the previous implementation.frontend/server/data/tinybird/tinybird.ts (1)
41-41: Good enhancement to query parameter filtering.Extending the filter to exclude empty strings and null values in addition to undefined values improves the robustness of the query processing. This prevents potential issues with Tinybird API calls by ensuring only meaningful values are sent.
frontend/server/data/tinybird/active-contributors-data-source.test.ts (1)
15-15: Test description updated appropriately.The renaming of the test suite from 'ContributorsDataSource' to 'ActiveContributorsDataSource' provides better specificity about what's being tested.
frontend/server/data/data-sources.ts (3)
1-10: Well-structured imports and documentation.The file has clear comments explaining its purpose and imports the necessary types and functions in a well-organized manner. The abstraction created here will make it easier to maintain and extend the data sources in the future.
11-14: Clean DataSource interface definition.The interface clearly defines the contract for data sources with the two required methods. This approach promotes loose coupling between the API endpoints and the specific data source implementations.
16-21: Factory function implementation is concise and effective.The
createDataSourcefunction adheres to the factory pattern and returns an object implementing theDataSourceinterface. This approach makes it easy to switch data source implementations in the future if needed.However, there appears to be a discrepancy with how this function is used in the test file. The test imports a
createActiveContributorsDataSourcefunction which is not exported by this file. Please ensure the tests are updated to work with this new structure.frontend/server/data/types.ts (1)
18-27: Well-structured type definitions for the contributors leaderboard.The new enum and type definitions are clearly organized and provide good type safety for the contributors leaderboard filtering functionality.
frontend/server/api/project/[slug]/contributors/contributor-leaderboard.get.ts (2)
1-4: Good imports with appropriate type imports.The imports are well organized, separating type imports appropriately.
43-48: Robust query parameter handling.The filter object is well constructed with appropriate fallbacks for optional parameters and proper type conversion for dates.
frontend/server/data/tinybird/contributors-leaderboard-source.ts (2)
4-18: Well-defined type definitions for leaderboard data.The type definitions are clear, with helpful comments for each property.
27-35:Details
❓ Verification inconclusive
Security concern flagged in TODO comment.
The code properly notes a security concern about passing unchecked query parameters to Tinybird.
This security concern should be addressed by implementing parameter validation before sending to Tinybird. Consider:
🏁 Script executed:
#!/bin/bash # Check if there are similar validation patterns elsewhere in the codebase rg -A 3 -B 3 "validateQuery|sanitizeQuery|validate.*Query" --type tsLength of output: 70
Security Concern – Unvalidated Query Parameters Remain
Our search for similar validation functions (using patterns like
validateQuery|sanitizeQuery|validate.*Query) did not yield any results elsewhere in the codebase. This suggests that no standard parameter sanitization is currently applied before passing these query parameters to Tinybird. To mitigate the identified risk, please ensure that the properties ofContributorsLeaderboardFilter(i.e.,repository,startDate, andendDate) are validated and sanitized before they are used to construct the query. Consider one of the following approaches:
- Implement explicit validation: Use type guards or a validation library (like Joi or Yup) to check the format and content of these inputs.
- Centralize sanitation logic: If similar queries are expected in other parts of the code, create a shared function to validate query parameters consistently.
Please review the changes carefully to confirm that proper input validation is implemented for Tinybird queries.
frontend/server/data/tinybird/active-contributors-data-source.ts (1)
15-31: Well-structured type definitions for active contributors.The exported types are clearly defined with good documentation in the form of comments for the summary properties.
There was a problem hiding this comment.
Potential import error.
The test file imports createActiveContributorsDataSource from ../data-sources, but the data-sources.ts file only exports createDataSource. This discrepancy might lead to runtime errors when running the tests.
Consider updating the import to use the new function:
-import {createActiveContributorsDataSource} from '../data-sources';
+import {createDataSource} from '../data-sources';And update the function call on line 27:
-const dataSource = createActiveContributorsDataSource(mockFetch);
+const dataSource = createDataSource();Note: You'll need to make additional changes to handle the mockFetch parameter correctly with the new function.
📝 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.
| import {createActiveContributorsDataSource} from '../data-sources'; | |
| // File: frontend/server/data/tinybird/active-contributors-data-source.test.ts | |
| // --- Before change (line 10) --- | |
| // import {createActiveContributorsDataSource} from '../data-sources'; | |
| // --- After change --- | |
| import {createDataSource} from '../data-sources'; | |
| // ... (other code remains unchanged) ... | |
| // Somewhere later in the file (line 27) | |
| // --- Before change --- | |
| // const dataSource = createActiveContributorsDataSource(mockFetch); | |
| // --- After change --- | |
| const dataSource = createDataSource(); | |
| // Note: Additional changes will be needed to properly adjust how mockFetch is handled with createDataSource. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing error handling for data fetching.
The code doesn't handle potential exceptions from the data source.
const dataSource = createDataSource();
- const result = await dataSource.fetchContributorsLeaderboard(filter);
+ try {
+ const result = await dataSource.fetchContributorsLeaderboard(filter);
+
+ return {
+ meta,
+ data: result.data
+ };
+ } catch (error) {
+ console.error('Error fetching contributors leaderboard:', error);
+ throw createError({
+ statusCode: 500,
+ statusMessage: 'Failed to fetch contributors leaderboard data'
+ });
+ }📝 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 dataSource = createDataSource(); | |
| const result = await dataSource.fetchContributorsLeaderboard(filter); | |
| const dataSource = createDataSource(); | |
| try { | |
| const result = await dataSource.fetchContributorsLeaderboard(filter); | |
| return { | |
| meta, | |
| data: result.data | |
| }; | |
| } catch (error) { | |
| console.error('Error fetching contributors leaderboard:', error); | |
| throw createError({ | |
| statusCode: 500, | |
| statusMessage: 'Failed to fetch contributors leaderboard data' | |
| }); | |
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fixed contributionValue in data mapping.
The contributionValue is hardcoded to 0 in the data mapping.
(item): ContributorsLeaderboardDataPoint => ({
avatar: item.avatar,
name: item.displayName,
contributions: item.contributionCount,
- contributionValue: 0
+ contributionValue: item.contributionPercentage
})📝 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.
| processedData = (data as TinybirdResponse<TinybirdContributorsLeaderboardData>)?.data.map( | |
| (item): ContributorsLeaderboardDataPoint => ({ | |
| avatar: item.avatar, | |
| name: item.displayName, | |
| contributions: item.contributionCount, | |
| contributionValue: 0 | |
| }) | |
| ); | |
| } | |
| processedData = (data as TinybirdResponse<TinybirdContributorsLeaderboardData>)?.data.map( | |
| (item): ContributorsLeaderboardDataPoint => ({ | |
| avatar: item.avatar, | |
| name: item.displayName, | |
| contributions: item.contributionCount, | |
| contributionValue: item.contributionPercentage | |
| }) | |
| ); | |
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoded pagination values.
The meta object uses hardcoded values for offset and limit, which won't support actual pagination.
return {
meta: {
- offset: 0,
- limit: 10,
+ offset: query?.offset || 0,
+ limit: query?.limit || 10,
total: data.rows
},
data: processedData
};You would need to add a query parameter to the function signature and pass it through from the API endpoint.
📝 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.
| return { | |
| meta: { | |
| offset: 0, | |
| limit: 10, | |
| total: data.rows | |
| }, | |
| data: processedData | |
| }; | |
| return { | |
| meta: { | |
| - offset: 0, | |
| - limit: 10, | |
| + offset: query?.offset || 0, | |
| + limit: query?.limit || 10, | |
| total: data.rows | |
| }, | |
| data: processedData | |
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Security concern flagged in TODO comment.
The code properly notes a security concern about passing unchecked query parameters to Tinybird.
This is a potential security risk that should be addressed. Consider implementing parameter validation before sending to Tinybird:
🏁 Script executed:
#!/bin/bash
# Check for existing validation patterns in the codebase
rg "sanitizeQueryParams|validateQuery" --type ts
# Look for Tinybird query execution patterns
rg -B 3 -A 3 "fetchFromTinybird" --type tsLength of output: 10578
Security Risk Detected: Implement Parameter Validation for Tinybird Queries
The current implementation (e.g., in frontend/server/data/tinybird/active-contributors-data-source.ts) passes query parameters directly to Tinybird without any sanitization or validation. A code search did not reveal any usage of functions like sanitizeQueryParams or validateQuery anywhere in the codebase. This leaves the system vulnerable to potential injection or other security risks.
- Action Required: Before calling
fetchFromTinybird, introduce a parameter validation/sanitization step. For example, validate or sanitize any incoming query parameters to ensure only safe, expected values are forwarded. - Reference: Consider a diff similar to:
- await fetchFromTinybird<TinybirdActiveContributorsData>('/v0/pipes/active_contributors.json', dataQuery) + const sanitizedQuery = validateQuery(dataQuery); // Ensure proper validation/sanitization here + await fetchFromTinybird<TinybirdActiveContributorsData>('/v0/pipes/active_contributors.json', sanitizedQuery)
Please address this security concern to prevent potential risks associated with unchecked query parameters.
Committable suggestion skipped: line range outside the PR's diff.
borfast
force-pushed
the
contributors-leaderboard-widget
branch
from
0ff84d2 to
64291b6
Compare
Signed-off-by: Raúl Santos <4837+borfast@users.noreply.github.com>
borfast
force-pushed
the
contributors-leaderboard-widget
branch
from
64291b6 to
4972e38
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/server/data/tinybird/active-contributors-data-source.ts (2)
70-74: Consider enhancing error handling for Promise.all()The current implementation uses Promise.all() without explicit error handling. If any of the three API requests fails, the entire operation will fail without graceful degradation.
- const [currentSummary, previousSummary, data] = await Promise.all([ - fetchFromTinybird<TinybirdActiveContributorsSummary>('/v0/pipes/active_contributors.json', currentSummaryQuery), - fetchFromTinybird<TinybirdActiveContributorsSummary>('/v0/pipes/active_contributors.json', previousSummaryQuery), - fetchFromTinybird<TinybirdActiveContributorsData>('/v0/pipes/active_contributors.json', dataQuery) - ]); + try { + const [currentSummary, previousSummary, data] = await Promise.all([ + fetchFromTinybird<TinybirdActiveContributorsSummary>('/v0/pipes/active_contributors.json', currentSummaryQuery), + fetchFromTinybird<TinybirdActiveContributorsSummary>('/v0/pipes/active_contributors.json', previousSummaryQuery), + fetchFromTinybird<TinybirdActiveContributorsData>('/v0/pipes/active_contributors.json', dataQuery) + ]); + + // Rest of the function continues here + } catch (error) { + // Log the error + console.error('Failed to fetch active contributors data:', error); + // Return a sensible default or re-throw based on your error handling strategy + throw new Error('Failed to fetch active contributors data'); + }
91-97: Extract percentage change calculation to a reusable utility function.The percentage change calculation logic is complex enough to warrant extraction into a separate utility function. This would improve readability and make the logic reusable across the codebase.
+ function calculatePercentageChange(current: number, previous: number): number { + if (previous === 0 && current > 0) { + return 100; + } else if (previous === 0 && current === 0) { + return 0; + } else if (previous !== 0) { + return ((current - previous) / previous) * 100; + } + return 0; + } const currentContributorCount = currentSummary.data[0].contributorCount; const previousContributorCount = previousSummary.data[0].contributorCount; const changeValue = currentContributorCount - previousContributorCount; - let percentageChange = 0; - if (previousContributorCount === 0 && currentContributorCount > 0) { - percentageChange = 100; - } else if (previousContributorCount === 0 && currentContributorCount === 0) { - percentageChange = 0; - } else if (previousContributorCount !== 0) { - percentageChange = ((currentContributorCount - previousContributorCount) / previousContributorCount) * 100; - } + const percentageChange = calculatePercentageChange(currentContributorCount, previousContributorCount);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 64291b656d13c1456a42d2ad07ba827f51bf4ee7 and 4972e38.
📒 Files selected for processing (6)
frontend/server/api/project/[slug]/contributors/contributor-leaderboard.get.ts(0 hunks)frontend/server/data/data-sources.ts(1 hunks)frontend/server/data/tinybird/active-contributors-data-source.ts(1 hunks)frontend/server/data/tinybird/contributors-leaderboard-source.ts(1 hunks)frontend/server/data/tinybird/tinybird.ts(1 hunks)frontend/server/data/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/server/api/project/[slug]/contributors/contributor-leaderboard.get.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/server/data/tinybird/tinybird.ts
- frontend/server/data/types.ts
- frontend/server/data/data-sources.ts
- frontend/server/data/tinybird/contributors-leaderboard-source.ts
🧰 Additional context used
🧠 Learnings (1)
frontend/server/data/tinybird/active-contributors-data-source.ts (1)
Learnt from: borfast
PR: LF-Engineering/insights#76
File: frontend/server/data/tinybird/contributors-leaderboard-source.ts:30-31
Timestamp: 2025-03-07T17:41:35.845Z
Learning: Security concerns about unchecked query parameters being passed to TinyBird are being tracked in Linear ticket INS-137 (https://linear.app/lfx/issue/INS-137/validate-user-input) and will be addressed separately.
🔇 Additional comments (2)
frontend/server/data/tinybird/active-contributors-data-source.ts (2)
79-79: Type annotation improvement enhances clarity.Changing the return type annotation from
ActiveContributorsResponseData[0]toActiveContributorsDataPointis a good improvement that makes the code more explicit and easier to understand. This change aligns with TypeScript best practices by using the most specific type available.
46-47: Security concern about unchecked query parameters.The TODO comment correctly identifies a security risk with passing unchecked parameters to Tinybird. I see from the retrieved learnings that this is being tracked in Linear ticket INS-137 and will be addressed separately.
3 participants
This adds the necessary endpoint for the contributors leaderboard widget.
This PR is built on the changes from #76, and it shouldn't be merged until after that one is merged and this one is rebase onto main. Even reviewing should be held back until the rebase, because right now this shows changes that belong to the previous PR in the chain.
Summary by CodeRabbit
New Features
Refactor