Conversation
WalkthroughThis pull request updates the organization leaderboard data retrieval mechanism. The endpoint is modified to parse query parameters (such as Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant E as API Endpoint
participant DS as DataSource
participant TS as Tinybird Source
participant TA as Tinybird API
C->>E: GET /organization-leaderboard?metric=...&repository=...&startDate=...&endDate=...
E->>E: Parse query parameters and construct filter object
E->>DS: createDataSource() and call fetchOrganizationsLeaderboard(filter)
DS->>TS: fetchOrganizationsLeaderboard(filter)
TS->>TA: fetchFromTinybird(query)
TA-->>TS: Return raw leaderboard data
TS->>TS: Process and map data to defined types
TS-->>DS: Return processed leaderboard response
DS-->>E: Return fetched leaderboard data
E-->>C: Return data payload in 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)
87-97: 🛠️ Refactor suggestionConsider boundary checks for arrays.
Here, you directly access
currentSummary.data[0]andpreviousSummary.data[0]. An empty array could cause an out-of-bounds error. Implement checks or an early return for safer, more defensive coding.
🧹 Nitpick comments (13)
frontend/server/data/tinybird/tinybird.ts (1)
41-41: Enhanced query parameter filteringGood improvement to filter out empty strings and null values in addition to undefined values, making the API request more robust.
However, the comment on line 38 only mentions filtering undefined values and should be updated to reflect this expanded filtering criteria.
- // We don't want o send undefined values to TinyBird, so we remove those from the query. We also format DateTime objects for TinyBird + // We don't want to send undefined, empty strings, or null values to TinyBird, so we remove those from the query. We also format DateTime objects for TinyBirdfrontend/server/api/project/[slug]/contributors/contributor-leaderboard.get.ts (2)
43-48: Add validation for potential invalid date strings.
IfDateTime.fromISOreceives an invalid date string, it may result in unexpected behavior. Consider adding fallback logic or user feedback to handle invalid dates gracefully.
50-51: Consider error handling around data fetching.
Wrapping the data source call in a try/catch block can improve resilience by handling potential errors from the data source.frontend/server/api/project/[slug]/contributors/organization-leaderboard.get.ts (2)
43-48: Add validation for potential invalid date strings.
UsingDateTime.fromISOwith invalid input might lead to unexpected results. Consider adding error handling or fallback values.
50-51: Encapsulate data fetching in try/catch to handle runtime errors.
Graceful error handling will ensure the endpoint doesn't break due to data source failures.frontend/server/data/tinybird/organizations-leaderboard-source.ts (2)
4-10: Consider making thelogoandwebsitefields optional.
Currently,logoandwebsiteare declared as required fields even though they're often undefined or empty. Converting them to optional properties (e.g.,logo?: string) could communicate intent more clearly and help prevent undefined references.
29-30: Validate or sanitize query parameters before passing them to Tinybird.
The TODO comment raises a valid security concern. To mitigate risks of injection or malformed requests, consider validating or sanitizing incoming parameters before they are included in the query object.Would you like me to suggest a sanitization approach or integrate a validation library?
frontend/server/api/project/[slug]/contributors/active-organizations.get.ts (1)
32-32: Clarify or remove the TODO regarding project visibility checks.
If the application must verify whether a project is publicly visible, this step should be implemented or removed from the code to avoid confusion.frontend/server/data/tinybird/contributors-leaderboard-source.ts (1)
28-30: Sanitize or validate the filter object before passing it to Tinybird.
Similar to the organizations leaderboard, the code warns about passing unchecked query parameters directly. Consider client-side sanitization or server-side validation to prevent malicious or malformed requests.frontend/server/data/tinybird/active-organizations-data-source.ts (2)
1-7: Consider extracting or refining the API-facing transformation.These lines highlight a potential concern: the file returns data in a specific format that the API expects. For reusability and maintainability, consider abstracting or relocating this transformation layer so that the data source could be more generic and self-contained, facilitating use cases beyond a single endpoint.
70-74: Evaluate combining parallel requests or reusing intermediate data.You're calling the same endpoint thrice with different parameters. While this is perfectly valid, consider if there's a more efficient approach (e.g., partial queries or unified queries) to reduce round-trip times.
frontend/server/data/types.ts (2)
23-28: Ensure naming consistency with “repo” vs. “repository”.In other parts of the codebase, the property name for repositories varies between “repo” and “repository”. Maintaining a consistent naming convention throughout prevents confusion and reduces the chance of errors.
31-40: Evaluate potential overlap between “ContributorsFilterGranularity” and the new “*_LeaderboardFilter” structures.If the granularity concept applies to both contributors and organizations in a similar manner, consider unifying or sharing an abstract enum/type to reduce duplication. This can help with code simplicity and uniform updates in the future.
📜 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 c4d74d0ad7f84cdd872f098d6c6ac77157f2f670.
📒 Files selected for processing (14)
frontend/app/components/modules/project/components/contributors/active-organizations.vue(1 hunks)frontend/server/api/project/[slug]/contributors/active-contributors.get.ts(2 hunks)frontend/server/api/project/[slug]/contributors/active-organizations.get.ts(2 hunks)frontend/server/api/project/[slug]/contributors/contributor-leaderboard.get.ts(2 hunks)frontend/server/api/project/[slug]/contributors/organization-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/active-organizations-data-source.ts(1 hunks)frontend/server/data/tinybird/contributors-leaderboard-source.ts(1 hunks)frontend/server/data/tinybird/organizations-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 (16)
frontend/app/components/modules/project/components/contributors/active-organizations.vue (1)
78-78: Parameter naming consistency improvementThe change from
intervaltogranularityaligns with the newActiveOrganizationsFilterGranularityenum definition, creating a more consistent API across the application.frontend/server/api/project/[slug]/contributors/active-contributors.get.ts (1)
28-28: Refactored to use centralized data source factoryGood architectural improvement to use the generic
createDataSource()function instead of a specific data source creator. This change promotes better maintainability and flexibility.Also applies to: 59-59
frontend/server/data/data-sources.ts (1)
1-34: Well-designed data source abstractionExcellent implementation of the Strategy pattern for data sources:
- The interface clearly defines the contract for different data operations
- The factory function provides a single point for creating data sources
- The design allows for easy substitution of different data source implementations in the future
This change significantly improves the architecture and maintainability of the codebase.
frontend/server/data/tinybird/active-contributors-data-source.test.ts (2)
10-10: Use updated import path consistently across codebase.
The new import path is correct. Please ensure all references to the old file path are removed to avoid confusion.
15-15: Rename clarifies the test suite's purpose.
Changing the test suite name to match the data source is good for clarity.frontend/server/api/project/[slug]/contributors/contributor-leaderboard.get.ts (2)
1-4: Refactored import statements look correct.
All newly introduced imports align with the updated data source approach.
55-55: Returning result data aligns with new dynamic data approach.
This is consistent with the rest of the refactor.frontend/server/api/project/[slug]/contributors/organization-leaderboard.get.ts (2)
1-4: Refactored import statements reflect the new approach.
The new imports align with the updated data source architecture.
55-55: Consistent with the dynamic data pattern.
Returning the retrieved data is in line with the new design.frontend/server/data/tinybird/organizations-leaderboard-source.ts (1)
43-49: Verify whethercontributionValueshould remain at 0.
Currently, the code sets each organization'scontributionValueto 0. If intended for future usage, ensure there's a plan to implement actual calculation logic, or explicitly document that this field is a placeholder.frontend/server/data/tinybird/contributors-leaderboard-source.ts (1)
39-49: Confirm ifcontributionValueis intended to remain zero.
ThecontributionValueproperty is consistently set to 0. If future logic is planned, add a TODO specifying how and when to compute an actual value. Otherwise, clarify its purpose in documentation.frontend/server/data/tinybird/active-organizations-data-source.ts (2)
87-88: Handle empty data arrays gracefully.The code assumes
.data[0]exists oncurrentSummaryandpreviousSummary. If these arrays ever come back empty, the code will throw an error. Consider adding a fallback check or a default value for more robust error handling.
99-109: Overall summary structure looks good.The response format is clearly arranged with a
summaryblock and adataarray. This well-structured approach should be easy for clients to consume and fosters consistency across similar data sources.frontend/server/data/types.ts (1)
42-51: Keep an eye on future additions to filtering logic.The new leaderboard filters (organizations vs contributors) share common fields and logic. Should they gain more specialized fields, you may refactor them into separate domain-specific types or classes. For now, they look well-organized.
frontend/server/data/tinybird/active-contributors-data-source.ts (2)
78-84: Check for data emptiness prior to mapping.Similar to the active organizations data source, ensure that
data?.dataisn’t empty or undefined. Add a guard clause to handle unexpected responses gracefully, preventing runtime errors if no data is returned.
99-109: Enforcing consistent response structure is beneficial.Your
ActiveContributorsResponseobject’s summary plus data array format matches the approach inactive-organizations-data-source. This consistency is helpful for uniform client consumption and streamlines any future improvements or refactoring.
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential undefined data before accessing data.rows.
Since data is conditionally checked only once, referencing data.rows outside that check may cause a runtime error if data is undefined. Consider returning early or providing a safe fallback.
if (!data) {
+ return {
+ meta: { offset: 0, limit: 10, total: 0 },
+ data: []
+ };
}
return {
meta: {
offset: 0,
limit: 10,
total: data.rows
},
data: processedData
};📝 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.
| total: data.rows | |
| if (!data) { | |
| return { | |
| meta: { offset: 0, limit: 10, total: 0 }, | |
| data: [] | |
| }; | |
| } | |
| return { | |
| meta: { | |
| offset: 0, | |
| limit: 10, | |
| total: data.rows | |
| }, | |
| data: processedData | |
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate date inputs to handle invalid or partial ISO date strings.
Currently, if the user provides an invalid date (e.g., malformed ISO string), it silently fails. Consider returning an error or fallback date when parsing fails.
There was a problem hiding this comment.
🛠️ Refactor suggestion
Safeguard against undefined response objects when setting total.
If data is undefined, data.rows would throw a runtime error. Ensure you handle undefined at this point or remove reliance on data.rows if the previous check fails.
if (!data) {
+ return {
+ meta: { offset: 0, limit: 10, total: 0 },
+ data: []
+ };
}
return {
meta: {
offset: 0,
limit: 10,
total: data.rows
},
data: processedData
};📝 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.
| total: data.rows | |
| if (!data) { | |
| return { | |
| meta: { offset: 0, limit: 10, total: 0 }, | |
| data: [] | |
| }; | |
| } | |
| return { | |
| meta: { | |
| offset: 0, | |
| limit: 10, | |
| total: data.rows | |
| }, | |
| data: processedData | |
| }; |
There was a problem hiding this comment.
Validate user-provided parameters to mitigate potential injection threats.
The comment acknowledges passing unchecked query parameters directly to Tinybird. To avoid security risks (e.g., injection attacks), consider validating or sanitizing these parameters on the server side before constructing the queries.
There was a problem hiding this comment.
Validate query parameters before passing them to Tinybird.
This TODO is crucial for preventing possible injection or malicious usage of your API. Consider implementing server-side validation or sanitization of all parameters to ensure you’re not exposing Tinybird to arbitrary or harmful queries.
borfast
force-pushed
the
organizations-leaderboard-widget
branch
from
c4d74d0 to
5bced60
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/server/api/project/[slug]/contributors/active-organizations.get.ts (1)
50-56: 🛠️ Refactor suggestionValidate date inputs to handle invalid or partial ISO date strings.
Currently, if the user provides an invalid date (e.g., malformed ISO string), it silently fails. Consider returning an error or fallback date when parsing fails.
if (query.startDate && (query.startDate as string).trim() !== '') { - filter.startDate = DateTime.fromISO(query.startDate as string); + const startDate = DateTime.fromISO(query.startDate as string); + if (!startDate.isValid) { + throw createError({ + statusCode: 400, + statusMessage: `Invalid startDate: ${query.startDate}` + }); + } + filter.startDate = startDate; } if (query.endDate && (query.endDate as string).trim() !== '') { - filter.endDate = DateTime.fromISO(query.endDate as string); + const endDate = DateTime.fromISO(query.endDate as string); + if (!endDate.isValid) { + throw createError({ + statusCode: 400, + statusMessage: `Invalid endDate: ${query.endDate}` + }); + } + filter.endDate = endDate; }
🧹 Nitpick comments (4)
frontend/server/api/project/[slug]/contributors/contributor-leaderboard.get.ts (1)
43-48: Consider adding validation for the filter parametersThe current implementation constructs the filter without validating the query parameters. Consider adding validation to ensure that the
metricis a valid value from the enum, and thatstartDateandendDateare valid ISO date strings. Invalid values could lead to unexpected behavior.const filter: ContributorsLeaderboardFilter = { metric: (query.metric as ContributorsLeaderboardFilterMetric) || ContributorsLeaderboardFilterMetric.ALL, repository: query.repository as string, startDate: query.startDate ? DateTime.fromISO(query.startDate as string) : undefined, endDate: query.endDate ? DateTime.fromISO(query.endDate as string) : undefined, }; + +// Validate dates are valid after parsing +if (query.startDate && filter.startDate && !filter.startDate.isValid) { + throw createError({ + statusCode: 400, + statusMessage: `Invalid startDate: ${query.startDate}` + }); +} + +if (query.endDate && filter.endDate && !filter.endDate.isValid) { + throw createError({ + statusCode: 400, + statusMessage: `Invalid endDate: ${query.endDate}` + }); +}frontend/server/api/project/[slug]/contributors/active-organizations.get.ts (1)
38-44: Add validation for the granularity parameterThe code doesn't validate that the granularity parameter is a valid value from the
ActiveOrganizationsFilterGranularityenum. Consider adding validation to ensure that the granularity value is valid.+const validGranularities = Object.values(ActiveOrganizationsFilterGranularity); +const providedGranularity = query.granularity as string; + +if (providedGranularity && !validGranularities.includes(providedGranularity as ActiveOrganizationsFilterGranularity)) { + throw createError({ + statusCode: 400, + statusMessage: `Invalid granularity: ${providedGranularity}. Valid values are: ${validGranularities.join(', ')}` + }); +} const filter: ActiveOrganizationsFilter = { granularity: (query.granularity as ActiveOrganizationsFilterGranularity) || ActiveOrganizationsFilterGranularity.QUARTERLY, project, repository: undefined, startDate: undefined, endDate: undefined };frontend/server/data/tinybird/active-organizations-data-source.ts (1)
87-97: Consider refactoring the percentage change calculation for better readabilityThe percentage change calculation logic could be refactored into a separate helper function to improve readability and maintain the Single Responsibility Principle. This would also make it easier to test.
-const currentOrganizationCount = currentSummary.data[0].organizationCount; -const previousOrganizationCount = previousSummary.data[0].organizationCount; -const changeValue = currentOrganizationCount - previousOrganizationCount; -let percentageChange = 0; -if (previousOrganizationCount === 0 && currentOrganizationCount > 0) { - percentageChange = 100; -} else if (previousOrganizationCount === 0 && currentOrganizationCount === 0) { - percentageChange = 0; -} else if (previousOrganizationCount !== 0) { - percentageChange = ((currentOrganizationCount - previousOrganizationCount) / previousOrganizationCount) * 100; -} +const currentOrganizationCount = currentSummary.data[0].organizationCount; +const previousOrganizationCount = previousSummary.data[0].organizationCount; +const changeValue = currentOrganizationCount - previousOrganizationCount; +const percentageChange = calculatePercentageChange(currentOrganizationCount, previousOrganizationCount); +/** + * Calculate percentage change between two values, handling edge cases + * + * @param current Current value + * @param previous Previous value + * @returns Percentage change + */ +function calculatePercentageChange(current: number, previous: number): number { + if (previous === 0 && current > 0) { + return 100; + } + + if (previous === 0 && current === 0) { + return 0; + } + + if (previous !== 0) { + return ((current - previous) / previous) * 100; + } + + return 0; +}frontend/server/data/types.ts (1)
18-29: Consider unifying the granularity enums to avoid duplicationThe
ActiveOrganizationsFilterGranularityenum duplicates the values inContributorsFilterGranularity. Consider using a single enum for both filters to avoid duplication and ensure consistency.-export enum ActiveOrganizationsFilterGranularity { - WEEKLY = 'weekly', - MONTHLY = 'monthly', - QUARTERLY = 'quarterly' -} +// Use the existing ContributorsFilterGranularity enum export type ActiveOrganizationsFilter = { project: string; repository?: string; - granularity?: ActiveOrganizationsFilterGranularity; + granularity?: ContributorsFilterGranularity; startDate?: DateTime; endDate?: DateTime; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between c4d74d0ad7f84cdd872f098d6c6ac77157f2f670 and 5bced60f5e330b3f47d0e6b0a8b66be78ec6800e.
📒 Files selected for processing (15)
.git-branches.toml(1 hunks)frontend/app/components/modules/project/components/contributors/active-organizations.vue(1 hunks)frontend/server/api/project/[slug]/contributors/active-contributors.get.ts(2 hunks)frontend/server/api/project/[slug]/contributors/active-organizations.get.ts(2 hunks)frontend/server/api/project/[slug]/contributors/contributor-leaderboard.get.ts(2 hunks)frontend/server/api/project/[slug]/contributors/organization-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/active-organizations-data-source.ts(1 hunks)frontend/server/data/tinybird/contributors-leaderboard-source.ts(1 hunks)frontend/server/data/tinybird/organizations-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
✅ Files skipped from review due to trivial changes (1)
- .git-branches.toml
🚧 Files skipped from review as they are similar to previous changes (9)
- frontend/app/components/modules/project/components/contributors/active-organizations.vue
- frontend/server/data/tinybird/tinybird.ts
- frontend/server/api/project/[slug]/contributors/active-contributors.get.ts
- frontend/server/data/tinybird/active-contributors-data-source.test.ts
- frontend/server/api/project/[slug]/contributors/organization-leaderboard.get.ts
- frontend/server/data/tinybird/contributors-leaderboard-source.ts
- frontend/server/data/tinybird/organizations-leaderboard-source.ts
- frontend/server/data/data-sources.ts
- frontend/server/data/tinybird/active-contributors-data-source.ts
🧰 Additional context used
🧠 Learnings (1)
frontend/server/data/tinybird/active-organizations-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 (3)
frontend/server/api/project/[slug]/contributors/active-organizations.get.ts (1)
62-68: Good error handling with detailed error informationThe error handling implementation is well done. It logs the error for debugging purposes and provides a clear error message to the client.
frontend/server/data/tinybird/active-organizations-data-source.ts (1)
46-47: Validate user-provided parameters to mitigate potential injection threats.The comment acknowledges passing unchecked query parameters directly to Tinybird. To avoid security risks (e.g., injection attacks), consider validating or sanitizing these parameters on the server side before constructing the queries.
Based on the retrieved learning, I see that security concerns about unchecked query parameters are being tracked in Linear ticket INS-137 (https://linear.app/lfx/issue/INS-137/validate-user-input) and will be addressed separately. Please ensure this issue is properly addressed in that ticket.
frontend/server/data/types.ts (1)
31-51: Good practice: Consistent type definitions for similar entitiesThe structure of
ContributorsLeaderboardFilterandOrganizationsLeaderboardFilteris consistent, which promotes maintainability and makes the code easier to understand.
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to handle API failures gracefully
The current implementation doesn't handle errors that might occur during the data fetching process. Consider adding try-catch blocks to handle errors gracefully, similar to the implementation in the active-organizations.get.ts file.
-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',
+ data: { message: error.message }
+ });
+}
-
-return {
- meta,
- data: result.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); | |
| return { | |
| meta, | |
| data: commits | |
| data: result.data | |
| 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', | |
| data: { message: error.message } | |
| }); | |
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential errors from Tinybird API requests
The code currently doesn't handle potential errors from individual Tinybird API requests. If one of the requests fails, the entire Promise.all will reject, but there's no specific error handling for each API call.
-const [currentSummary, previousSummary, data] = await Promise.all([
- fetchFromTinybird<TinybirdActiveOrganizationsSummary>('/v0/pipes/active_organizations.json', currentSummaryQuery),
- fetchFromTinybird<TinybirdActiveOrganizationsSummary>('/v0/pipes/active_organizations.json', previousSummaryQuery),
- fetchFromTinybird<TinybirdActiveOrganizationsData>('/v0/pipes/active_organizations.json', dataQuery)
-]);
+try {
+ const [currentSummary, previousSummary, data] = await Promise.all([
+ fetchFromTinybird<TinybirdActiveOrganizationsSummary>('/v0/pipes/active_organizations.json', currentSummaryQuery),
+ fetchFromTinybird<TinybirdActiveOrganizationsSummary>('/v0/pipes/active_organizations.json', previousSummaryQuery),
+ fetchFromTinybird<TinybirdActiveOrganizationsData>('/v0/pipes/active_organizations.json', dataQuery)
+ ]);
+} catch (error) {
+ console.error('Error fetching data from Tinybird:', error);
+ throw new Error(`Failed to fetch data from Tinybird: ${error.message}`);
+}📝 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 [currentSummary, previousSummary, data] = await Promise.all([ | |
| fetchFromTinybird<TinybirdActiveOrganizationsSummary>('/v0/pipes/active_organizations.json', currentSummaryQuery), | |
| fetchFromTinybird<TinybirdActiveOrganizationsSummary>('/v0/pipes/active_organizations.json', previousSummaryQuery), | |
| fetchFromTinybird<TinybirdActiveOrganizationsData>('/v0/pipes/active_organizations.json', dataQuery) | |
| ]); | |
| try { | |
| const [currentSummary, previousSummary, data] = await Promise.all([ | |
| fetchFromTinybird<TinybirdActiveOrganizationsSummary>('/v0/pipes/active_organizations.json', currentSummaryQuery), | |
| fetchFromTinybird<TinybirdActiveOrganizationsSummary>('/v0/pipes/active_organizations.json', previousSummaryQuery), | |
| fetchFromTinybird<TinybirdActiveOrganizationsData>('/v0/pipes/active_organizations.json', dataQuery) | |
| ]); | |
| } catch (error) { | |
| console.error('Error fetching data from Tinybird:', error); | |
| throw new Error(`Failed to fetch data from Tinybird: ${error.message}`); | |
| } |
borfast
force-pushed
the
organizations-leaderboard-widget
branch
4 times, most recently
from
e3b1b0c to
262c3d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/server/data/tinybird/contributors-leaderboard-source.ts (1)
51-58:⚠️ Potential issueSafeguard against
undefinedresponse objects when settingtotal.If
dataisundefined,data.rowswould throw a runtime error. Ensure you handleundefinedat this point or remove reliance ondata.rowsif the previous check fails.if (!data) { + return { + meta: { offset: 0, limit: 10, total: 0 }, + data: [] + }; } return { meta: { offset: 0, limit: 10, - total: data.rows + total: data.rows || 0 }, data: processedData };
🧹 Nitpick comments (6)
frontend/server/api/project/[slug]/contributors/organization-leaderboard.get.ts (2)
43-48: Consider handling undefined repository parameter.The filter directly casts
query.repositoryto string without checking if it exists, which could lead to unexpected behavior if the repository parameter is not provided.const filter: OrganizationsLeaderboardFilter = { metric: (query.metric as OrganizationsLeaderboardFilterMetric) || OrganizationsLeaderboardFilterMetric.ALL, - repository: query.repository as string, + repository: query.repository ? String(query.repository) : '', startDate: query.startDate ? DateTime.fromISO(query.startDate as string) : undefined, endDate: query.endDate ? DateTime.fromISO(query.endDate as string) : undefined, };
53-56: Consider updating meta with dynamic values from result.The meta object contains static values for offset, limit, and total, but it would be better to use values from the fetched result if available.
return { - meta, + meta: { + offset: meta.offset, + limit: meta.limit, + total: result.meta?.total || meta.total + }, data: result.data };frontend/server/data/types.ts (2)
18-22: Consider reusing ContributorsFilterGranularity.
ActiveOrganizationsFilterGranularityis identical to the existingContributorsFilterGranularity. Consider using a single, generic enum to reduce duplication.-export enum ActiveOrganizationsFilterGranularity { - WEEKLY = 'weekly', - MONTHLY = 'monthly', - QUARTERLY = 'quarterly' -} +// Reuse the existing enum +export type ActiveOrganizationsFilterGranularity = ContributorsFilterGranularity;
31-40: Consider creating a base type for leaderboard filters.
ContributorsLeaderboardFilterandOrganizationsLeaderboardFiltershare the same structure. Consider creating a base type to reduce duplication.+// Base type for leaderboard filters +export type BaseLeaderboardFilter = { + repository: string; + startDate?: DateTime; + endDate?: DateTime; +}; export enum ContributorsLeaderboardFilterMetric { ALL = 'all', COMMITS = 'commits' } -export type ContributorsLeaderboardFilter = { - metric: ContributorsLeaderboardFilterMetric; - repository: string; - startDate?: DateTime; - endDate?: DateTime; -} +export type ContributorsLeaderboardFilter = BaseLeaderboardFilter & { + metric: ContributorsLeaderboardFilterMetric; +}; export enum OrganizationsLeaderboardFilterMetric { ALL = 'all', COMMITS = 'commits' } -export type OrganizationsLeaderboardFilter = { - metric: OrganizationsLeaderboardFilterMetric; - repository: string; - startDate?: DateTime; - endDate?: DateTime; -} +export type OrganizationsLeaderboardFilter = BaseLeaderboardFilter & { + metric: OrganizationsLeaderboardFilterMetric; +};Also applies to: 42-51
frontend/server/api/project/[slug]/contributors/active-organizations.get.ts (2)
29-30: Document a mismatch between the comment and implementation.The code comment mentions a 'time-period' query parameter, but this parameter isn't being used in the implementation. Either implement support for this parameter or update the comment to reflect the actual implementation.
38-44: Add validation for the granularity parameter.The code sets a default granularity but doesn't validate if the provided value is one of the allowed enum values. This could lead to errors if an invalid value is passed.
const filter: ActiveOrganizationsFilter = { - granularity: (query.granularity as ActiveOrganizationsFilterGranularity) || ActiveOrganizationsFilterGranularity.QUARTERLY, + granularity: isValidGranularity(query.granularity) + ? (query.granularity as ActiveOrganizationsFilterGranularity) + : ActiveOrganizationsFilterGranularity.QUARTERLY, project, repository: undefined, startDate: undefined, endDate: undefined }; // Add this helper function function isValidGranularity(value: unknown): boolean { return Object.values(ActiveOrganizationsFilterGranularity).includes(value as ActiveOrganizationsFilterGranularity); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 5bced60f5e330b3f47d0e6b0a8b66be78ec6800e and 7c3dfce83e7ac390b184e04c84994dcff7806395.
📒 Files selected for processing (11)
frontend/app/components/modules/project/components/contributors/active-organizations.vue(1 hunks)frontend/server/api/project/[slug]/contributors/active-organizations.get.ts(2 hunks)frontend/server/api/project/[slug]/contributors/contributor-leaderboard.get.ts(0 hunks)frontend/server/api/project/[slug]/contributors/organization-leaderboard.get.ts(2 hunks)frontend/server/data/data-sources.ts(1 hunks)frontend/server/data/tinybird/active-contributors-data-source.ts(1 hunks)frontend/server/data/tinybird/active-organizations-data-source.ts(1 hunks)frontend/server/data/tinybird/contributors-leaderboard-source.ts(1 hunks)frontend/server/data/tinybird/organizations-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 (5)
- frontend/app/components/modules/project/components/contributors/active-organizations.vue
- frontend/server/data/tinybird/tinybird.ts
- frontend/server/data/tinybird/active-contributors-data-source.ts
- frontend/server/data/tinybird/organizations-leaderboard-source.ts
- frontend/server/data/tinybird/active-organizations-data-source.ts
🧰 Additional context used
🧠 Learnings (1)
frontend/server/data/tinybird/contributors-leaderboard-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 (16)
frontend/server/data/data-sources.ts (5)
4-9: Import organization looks good.The imports are well-organized, grouping filter types and response types together.
11-13: Response type imports correctly added.These new response type imports properly complement the filter types and match the data sources being added.
15-17: Function imports correctly added.The fetch function imports align with the interface methods being added below.
21-23: Interface extension looks good.The DataSource interface is properly extended with the new methods, each with the correct parameter and return types.
28-31: Implementation correctly includes new methods.The implementation in createDataSource() properly includes all the newly imported functions.
frontend/server/api/project/[slug]/contributors/organization-leaderboard.get.ts (2)
1-4: Import statements look good.The import statements correctly include necessary dependencies for date handling, data source creation, and type definitions.
50-51: Data source creation and fetching looks good.The data source is properly created and used to fetch organizations leaderboard data.
frontend/server/data/types.ts (1)
23-29: ActiveOrganizationsFilter type looks good.The filter type is well-structured with appropriate optional fields.
frontend/server/data/tinybird/contributors-leaderboard-source.ts (4)
4-18: Well-defined response types.The types for the leaderboard data are clear and well-documented, with appropriate comments describing each field.
31-38: Query construction looks good.The query construction correctly uses the filter parameters for repository, startDate, and endDate.
39-49: Data processing looks good.The code properly handles the case where data might be undefined and correctly maps the TinyBird response to the expected format.
27-30:Details
✅ Verification successful
Acknowledge the security concern in the TODO comment.
The TODO comment about unchecked query parameters being passed to TinyBird is an important security consideration. From your past learnings, I see this is being tracked in Linear ticket INS-137 and will be addressed separately.
I recommend confirming that this concern is still being tracked:
🏁 Script executed:
#!/bin/bash # Search for any mentions of validation or security concerns rg -i "validate.*input|unchecked.*query|security.*risk"Length of output: 1185
Security Concern Acknowledged & Tracked
The TODO comment highlighting unchecked query parameters is valid and consistently appears across similar TinyBird data source files. As verified by the search output, this potential security risk is uniformly marked in multiple files and is already being tracked via Linear ticket INS-137. No immediate code change is necessary under this PR; please ensure alignment with the ongoing tracking and eventual remediation in INS-137.frontend/server/api/project/[slug]/contributors/active-organizations.get.ts (4)
50-56: Validate date inputs to handle invalid or partial ISO date strings.Currently, if the user provides an invalid date (e.g., malformed ISO string), it silently fails. Consider checking the validity of the DateTime object after parsing and providing appropriate error handling.
if (query.startDate && (query.startDate as string).trim() !== '') { - filter.startDate = DateTime.fromISO(query.startDate as string); + const parsedDate = DateTime.fromISO(query.startDate as string); + if (!parsedDate.isValid) { + throw createError({ + statusCode: 400, + statusMessage: `Invalid startDate: ${parsedDate.invalidReason}`, + }); + } + filter.startDate = parsedDate; }
32-33: Address the TODO comments before merging.There are two TODO comments related to validating query parameters and checking project configuration. Consider implementing these checks before finalizing this PR or create separate issues to track them.
Also applies to: 37-38
60-69: The error handling is well implemented.Good job implementing proper error handling with detailed error messages. This will help with debugging and provide clear feedback to API consumers.
19-22: Overall architectural improvement.The transition from hardcoded mock data to a dynamic filtering approach with proper typing is a significant improvement. The code is more maintainable, type-safe, and follows a clean separation of concerns by delegating data fetching to a dedicated data source.
Also applies to: 38-44, 58-69
frontend/server/api/project/[slug]/contributors/active-organizations.get.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Raúl Santos <4837+borfast@users.noreply.github.com>
borfast
force-pushed
the
organizations-leaderboard-widget
branch
from
262c3d2 to
7d8ec0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/server/api/project/[slug]/contributors/organization-leaderboard.get.ts (4)
43-48: Consider validating the metric value before using it.The code currently casts
query.metrictoOrganizationsLeaderboardFilterMetricwithout validation, which may lead to runtime errors if an invalid metric value is provided. Consider implementing validation or using a safer approach to handle potentially invalid input.const filter: OrganizationsLeaderboardFilter = { - metric: (query.metric as OrganizationsLeaderboardFilterMetric) || OrganizationsLeaderboardFilterMetric.ALL, + metric: Object.values(OrganizationsLeaderboardFilterMetric).includes(query.metric as OrganizationsLeaderboardFilterMetric) + ? (query.metric as OrganizationsLeaderboardFilterMetric) + : OrganizationsLeaderboardFilterMetric.ALL, repository: query.repository as string, startDate: query.startDate ? DateTime.fromISO(query.startDate as string) : undefined, endDate: query.endDate ? DateTime.fromISO(query.endDate as string) : undefined, };
46-47: Validate date string format before parsing.The current implementation doesn't validate if the provided date strings are valid ISO format. Invalid dates could lead to unexpected behavior. Consider adding validation or handling potential parsing errors.
metric: (query.metric as OrganizationsLeaderboardFilterMetric) || OrganizationsLeaderboardFilterMetric.ALL, repository: query.repository as string, - startDate: query.startDate ? DateTime.fromISO(query.startDate as string) : undefined, - endDate: query.endDate ? DateTime.fromISO(query.endDate as string) : undefined, + startDate: query.startDate ? (DateTime.fromISO(query.startDate as string).isValid ? DateTime.fromISO(query.startDate as string) : undefined) : undefined, + endDate: query.endDate ? (DateTime.fromISO(query.endDate as string).isValid ? DateTime.fromISO(query.endDate as string) : undefined) : undefined,
37-41: Consider using dynamic pagination metadata.The
metaobject is currently hardcoded with static values. Consider deriving these values from the actual result or from query parameters to ensure accurate pagination information.- const meta = { - offset: 0, - limit: 10, - total: 100 - }; + const offset = parseInt(query.offset as string) || 0; + const limit = parseInt(query.limit as string) || 10; + + // Later, after fetching data: + const meta = { + offset, + limit, + total: result.total || result.data.length + };
46-47: Consider using nullish coalescing for potentially undefined values.When checking for the existence of query parameters, using the nullish coalescing operator (
??) can provide cleaner code and handle bothundefinedandnullvalues.metric: (query.metric as OrganizationsLeaderboardFilterMetric) || OrganizationsLeaderboardFilterMetric.ALL, repository: query.repository as string, - startDate: query.startDate ? DateTime.fromISO(query.startDate as string) : undefined, - endDate: query.endDate ? DateTime.fromISO(query.endDate as string) : undefined, + startDate: (query.startDate as string) ? DateTime.fromISO(query.startDate as string) : undefined, + endDate: (query.endDate as string) ? DateTime.fromISO(query.endDate as string) : undefined,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 7c3dfce83e7ac390b184e04c84994dcff7806395 and 7d8ec0d.
📒 Files selected for processing (4)
frontend/server/api/project/[slug]/contributors/organization-leaderboard.get.ts(2 hunks)frontend/server/data/data-sources.ts(1 hunks)frontend/server/data/tinybird/organizations-leaderboard-source.ts(1 hunks)frontend/server/data/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/server/data/types.ts
- frontend/server/data/data-sources.ts
- frontend/server/data/tinybird/organizations-leaderboard-source.ts
| const dataSource = createDataSource(); | ||
| const result = await dataSource.fetchOrganizationsLeaderboard(filter); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the data fetch operation.
The API call lacks error handling. If fetchOrganizationsLeaderboard fails, the error will propagate to the caller without any specific error message or handling. Consider implementing try/catch to provide better error responses.
const dataSource = createDataSource();
- const result = await dataSource.fetchOrganizationsLeaderboard(filter);
+ try {
+ const result = await dataSource.fetchOrganizationsLeaderboard(filter);
+
+ return {
+ meta,
+ data: result.data
+ };
+ } catch (error) {
+ console.error("Error fetching organizations leaderboard:", error);
+ throw createError({
+ statusCode: 500,
+ statusMessage: "Failed to fetch organizations 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.fetchOrganizationsLeaderboard(filter); | |
| const dataSource = createDataSource(); | |
| try { | |
| const result = await dataSource.fetchOrganizationsLeaderboard(filter); | |
| return { | |
| meta, | |
| data: result.data | |
| }; | |
| } catch (error) { | |
| console.error("Error fetching organizations leaderboard:", error); | |
| throw createError({ | |
| statusCode: 500, | |
| statusMessage: "Failed to fetch organizations leaderboard data", | |
| }); | |
| } |
3 participants
This adds the necessary endpoint for the organizations leaderboard widget.
This PR is built on the changes from #78, 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