Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request updates the active contributors functionality in several areas. The parameter used in API requests has been renamed from Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client (Vue Component)
participant A as API Endpoint
participant DS as Data Source
participant TB as Tinybird API
C->>A: Request active contributors (granularity, project, etc.)
A->>DS: Build filter object and invoke data source
DS->>TB: Send concurrent queries for current summary, previous summary, and timeseries data
TB-->>DS: Return fetched data
DS-->>A: Process results and calculate changes
A-->>C: Respond with active contributors data
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between 294ac173f2bf23882a915c4eeecae6c63e1bce3e and 2c6794f. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
✨ 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 review |
✅ Actions performedReview triggered.
|
borfast
force-pushed
the
feature/add-active-contributors-data-fetching
branch
from
96c3f96 to
d166cd8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 12
🧹 Nitpick comments (19)
frontend/server/data/tinybird/tinybird.test.ts (1)
1-58: Well-structured tests for the Tinybird integrationThe test cases effectively verify that:
- Requests to Tinybird include all expected parameters and format dates correctly
- Optional parameters are properly handled (excluded when not provided)
These tests will help ensure the reliability of the Tinybird integration.
One suggestion - consider adding a test case for error handling:
+ test('should handle API errors gracefully', async () => { + const mockFetch = vi.fn().mockImplementation(() => Promise.reject(new Error('API Error'))) as unknown as $Fetch; + const endpoint = '/v0/pipes/active_contributors.json'; + + const query: ActiveContributorsFilter = { + project: 'test-project', + granularity: ContributorsFilterGranularity.MONTHLY, + } + + await expect(fetchFromTinybird(endpoint, query, mockFetch)).rejects.toThrow('API Error'); + });frontend/server/data/util.ts (1)
12-31: Well-implemented date range calculation functionThis utility function correctly handles:
- Default values when dates aren't provided
- Proper calculation of previous time ranges based on current ranges
- Returning a clean, structured object with both current and previous ranges
The implementation using date-fns is clean and maintainable.
However, there's a potential edge case to handle:
export function getPreviousDates(currentFromDate?: Date, currentToDate?: Date) { const safeFromDate = currentFromDate || earliestPossibleFromDate; const safeToDate = currentToDate || new Date(); + // Ensure the from date is before the to date + if (safeFromDate > safeToDate) { + throw new Error('From date must be before or equal to the to date'); + } + const diffDays = differenceInDays(safeToDate, safeFromDate);frontend/server/data/active-contributors-data-source.ts (1)
4-5: Consider using relative imports for better maintainability.The imports use the
~~/server/data/format which can sometimes cause issues with IDE navigation and refactoring. Consider using relative paths for imports within the same directory structure.-import type {ActiveContributorsResponse, ActiveContributorsFilter} from "~~/server/data/types"; -import {createTinybirdDataSource} from "~~/server/data/tinybird/active-contributors-data-source"; +import type {ActiveContributorsResponse, ActiveContributorsFilter} from "./types"; +import {createTinybirdDataSource} from "./tinybird/active-contributors-data-source";frontend/server/api/project/[slug]/index.ts (2)
31-36: Hardcoded logo URL may lead to future maintenance issues.While not directly part of your changes, the hardcoded Kubernetes logo URL is a potential maintenance concern. If the external image becomes unavailable, it could affect the UI. Consider storing logo URLs in a configuration file or database.
- logo: 'https://c8.alamy.com/comp/2M8NCEE/kubernetes-logo-white-background-2M8NCEE.jpg', + logo: config.logoUrls[slug] || 'https://c8.alamy.com/comp/2M8NCEE/kubernetes-logo-white-background-2M8NCEE.jpg',You would need to implement a
configobject with alogoUrlsmapping.
32-32: Potential null reference in string manipulation.The expression
(slug?.at(0) || '').toUpperCase()has improved null safety overslug?.at(0)?.toUpperCase(), but it could be further clarified with a more explicit approach.- name: slug ? (slug?.at(0) || '').toUpperCase() + slug.slice(1) : '', + name: slug ? (slug.charAt(0).toUpperCase() + slug.slice(1)) : '',frontend/server/mocks/active-contributors.mock.ts (1)
16-90: Consider using data generators instead of manually commented sections.Having large commented-out blocks of data makes the codebase harder to maintain. Consider implementing a data generator function to create the weekly data points programmatically.
// Add this function at the bottom of the file function generateWeeklyData(startDate, weeks, baseContributors = 5000, variance = 2000) { const data = []; const start = new Date(startDate); for (let i = 0; i < weeks; i++) { const fromDate = new Date(start); fromDate.setDate(fromDate.getDate() + (i * 7)); const toDate = new Date(fromDate); toDate.setDate(toDate.getDate() + 7); data.push({ fromDate: fromDate.toISOString(), toDate: toDate.toISOString(), contributors: Math.floor(baseContributors + (Math.random() * variance * 2) - variance) }); } return data; } // Then replace the weekly.data assignment with: // export const weekly = { // summary: {...}, // data: generateWeeklyData('2024-11-11T00:00:00Z', 16, 7000, 3000) // };frontend/server/data/tinybird/active-contributors-data-source.test.ts (4)
16-24: Improve type safety in mock function implementation.The code uses a double type assertion (
as unknown as $Fetch) which is generally considered unsafe. It's better to create a properly typed mock function.- const mockFetch = vi.fn() - .mockResolvedValueOnce(mockWeeklyCurrentSummary) - .mockResolvedValueOnce(mockWeeklyPreviousSummary) - // The double type assertion is necessary because the mockFetch function needs to satisfy the type - // signature of the $Fetch type that createContributorsDataSource expects, even though they're structurally - // different types. - // Since we can't cast the mock function directly to $Fetch, we first cast it to unknown (which can be - // cast to anything) and then cast it to $Fetch. - .mockResolvedValueOnce(mockWeeklyTimeseries) as unknown as $Fetch; + // Create a properly typed mock function + const mockFetch: $Fetch = vi.fn().mockImplementation(async (url, options) => { + const urlString = url.toString(); + if (urlString.includes('current_summary')) { + return mockWeeklyCurrentSummary; + } else if (urlString.includes('previous_summary')) { + return mockWeeklyPreviousSummary; + } else if (urlString.includes('timeseries')) { + return mockWeeklyTimeseries; + } + throw new Error(`Unexpected URL: ${urlString}`); + });
39-45: Ensure cleanup of fake timers even if test fails.The code sets up fake timers but may not clean them up if an exception occurs before
vi.useRealTimers()is called. Consider using a try/finally pattern.- const fakeDate = new Date(2022, 11, 11) - vi.useFakeTimers(); - vi.setSystemTime(fakeDate); - - const result = await dataSource.fetchActiveContributors(filter); - - vi.useRealTimers(); + const fakeDate = new Date(2022, 11, 11); + vi.useFakeTimers(); + vi.setSystemTime(fakeDate); + + let result; + try { + result = await dataSource.fetchActiveContributors(filter); + } finally { + vi.useRealTimers(); + }
47-50: Consider adding unit tests for percentage calculation.The percentage change calculation is correct, but it might be worth testing this logic separately to ensure it remains correct if the business logic changes.
Consider adding a specific test case for the percentage calculation logic:
test('should calculate percentage change correctly', () => { const current = 110; const previous = 100; const expectedPercentage = 10; const calculatedPercentage = ((current - previous) / previous) * 100; expect(calculatedPercentage).toBe(expectedPercentage); // Edge case: previous is 0 const calculatedPercentageEdge = ((current - 0) / 1) * 100; expect(calculatedPercentageEdge).toBe(current * 100); });
52-66: Test validation could be more detailed.The test validates the structure of the response but doesn't verify that the specific values like dates are correctly formatted. Consider adding more detailed assertions.
Add more specific assertions to verify date formatting and other values:
// Add these assertions after the expect(result).toEqual(expectedResult) expect(result.summary.periodFrom).toBeInstanceOf(Date); expect(result.summary.periodTo).toBeInstanceOf(Date); expect(result.data[0].fromDate).toMatch(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$/);frontend/server/api/project/[slug]/contributors/active-contributors.get.ts (1)
49-51: Consider adding validation for repository formatWhile you check if the repository parameter is non-empty, you might want to validate its format as well, especially if certain repository name formats are expected by the Tinybird API.
if (query.repository && (query.repository as string).trim() !== '') { + // Consider validating repository format here + // e.g., check if it matches expected patterns for your data source filter.repo = query.repository as string; }frontend/server/data/tinybird/active-contributors-data-source.ts (3)
25-26: Remove commented-out codeThere's a commented-out line that appears to be leftover from development. Commented-out code can lead to confusion and should be removed before merging.
- // const testQuery = createTinyBirdQuery(filter, filter.project, filter.fromDate, filter.toDate);
27-47: Consider consolidating filter objects with a helper functionThe currentSummaryQuery, previousSummaryQuery, and dataQuery objects have similar structures with slight differences. Consider creating a helper function to reduce repetition.
+ const createQueryFilter = (base: Partial<ActiveContributorsFilter>): ActiveContributorsFilter => ({ + project: filter.project, + repo: filter.repo, + ...base + }); + - const currentSummaryQuery: ActiveContributorsFilter = { - project: filter.project, - repo: filter.repo, - fromDate: dates.current.from, - toDate: dates.current.to - } + const currentSummaryQuery = createQueryFilter({ + fromDate: dates.current.from, + toDate: dates.current.to + }); - const previousSummaryQuery: ActiveContributorsFilter = { - project: filter.project, - repo: filter.repo, - fromDate: dates.previous.from, - toDate: dates.previous.to - }; + const previousSummaryQuery = createQueryFilter({ + fromDate: dates.previous.from, + toDate: dates.previous.to + }); - const dataQuery: ActiveContributorsFilter = { - project: filter.project, - granularity: filter.granularity, - repo: filter.repo, - fromDate: dates.current.from, - toDate: dates.current.to - }; + const dataQuery = createQueryFilter({ + granularity: filter.granularity, + fromDate: dates.current.from, + toDate: dates.current.to + });
55-62: Add specific type annotation for map operationThe map operation could benefit from a more specific type annotation to ensure type safety throughout the transformation.
- processedData = (data as TinybirdContributorsDataResponse)?.data.map((item) => ({ + processedData = (data as TinybirdContributorsDataResponse)?.data.map((item): ActiveContributorsResponseData[0] => ({ fromDate: item.fromDate, toDate: item.toDate, contributors: item.contributor_count }));frontend/server/data/tinybird/tinybird.ts (3)
71-73: Consider adding timezone handling in date formattingWhen formatting dates for API requests, it's important to consider timezone issues. The current implementation might lead to inconsistent results across different server environments.
export function formatDateForTinyBird(date: Date): string { - return format(date, 'yyyy-MM-dd 00:00:00'); + // Force UTC timezone for consistency across environments + return format(new Date(date.toISOString().split('T')[0]), 'yyyy-MM-dd 00:00:00'); }
91-99: Improve parameter handling clarity with object destructuringThe code for constructing the Tinybird query parameters could be made more readable with object destructuring and conditional spreading.
- // We don't want to send parameters to Tinybird that are empty. Plus, we need to format the dates for Tinybird. - const tinybirdQuery: TinybirdActiveContributorsQueryParams = { - project: query.project, - token: config.tinybirdToken - }; - if (query.granularity) tinybirdQuery.granularity = query.granularity; - if (query.repo) tinybirdQuery.repo = query.repo; - if (query.fromDate) tinybirdQuery.fromDate = formatDateForTinyBird(query.fromDate); - if (query.toDate) tinybirdQuery.toDate = formatDateForTinyBird(query.toDate); + // Construct query parameters, formatting dates and omitting undefined values + const { project, granularity, repo, fromDate, toDate } = query; + const tinybirdQuery: TinybirdActiveContributorsQueryParams = { + project, + token: config.tinybirdToken, + ...(granularity && { granularity }), + ...(repo && { repo }), + ...(fromDate && { fromDate: formatDateForTinyBird(fromDate) }), + ...(toDate && { toDate: formatDateForTinyBird(toDate) }) + };
105-105: Remove console.log statement before productionConsole.log statements should generally be removed before code goes to production, especially those that might log sensitive data or large objects that could impact performance.
- console.log('Tinybird result', result); + // Consider using a proper logging system if debug info is neededfrontend/server/api/search.ts (2)
19-19: Great addition of the Gerrit projectAdding Gerrit as a new project is good, but consider including a logo URL for consistency with other projects like Kubernetes.
- {slug: 'gerrit', name: 'Gerrit', logo: ''}, + {slug: 'gerrit', name: 'Gerrit', logo: 'https://gerrit-review.googlesource.com/static/logo.png'},
49-51: Consider case-insensitive repository URL searchThe search function filters repositories by name (URL), but URLs are case-sensitive. If users type lowercase 'github' instead of 'GitHub', the search might not work as expected.
return { projects: projects.filter((project) => project.name.toLowerCase().includes(query.toLowerCase())), - repositories: repositories.filter((repository) => repository.name.toLowerCase().includes(query.toLowerCase())), + repositories: repositories.filter((repository) => { + // Extract domain and path for more user-friendly search + const url = repository.name; + const urlParts = url.replace(/https?:\/\//, '').split('/'); + const domain = urlParts[0]; + const path = urlParts.slice(1).join('/'); + + return domain.toLowerCase().includes(query.toLowerCase()) || + path.toLowerCase().includes(query.toLowerCase()); + }), collections: collections.filter((collection) => collection.name.toLowerCase().includes(query.toLowerCase())), };
🛑 Comments failed to post (12)
frontend/server/data/util.ts (1)
3-4: 💡 Verification agent
🧩 Analysis chain
TODO comment needs to be addressed
The comment "TODO: What should this date be?" indicates uncertainty about the appropriate earliest possible date for data retrieval. This should be resolved before final deployment.
Consider:
- Checking with your data team about the earliest date when data is available
- Adding documentation explaining the rationale once decided
🏁 Script executed:
#!/bin/bash # Look for any other references to earliest dates in the codebase rg -g "*.ts" -g "*.js" "earliest.*date" --ignore-caseLength of output: 568
Action: Resolve the ambiguous TODO on
earliestPossibleFromDateThe constant is still hard-coded as
new Date(2010, 0, 1)in bothfrontend/server/data/util.tsand its tests without addressing the uncertainty noted by the TODO comment. Please confirm with the data team whether 2010 is the intended starting date for data retrieval, and then either update the constant accordingly or replace the TODO comment with clear documentation explaining the rationale behind this date. This will ensure consistency across both production and test code.frontend/nuxt.config.ts (1)
31-39: 🛠️ Refactor suggestion
Empty Tinybird token in configuration could cause runtime errors.
The
tinybirdTokenis set to an empty string, which might cause authentication failures when making requests to the Tinybird API. While the comment indicates these values can be overridden by environment variables, there should be validation or error handling to prevent failures when the token is missing.Add validation in the code that uses this token to check if it's empty and provide a clear error message:
// Add this to the file that uses the tinybirdToken const config = useRuntimeConfig(); + if (!config.tinybirdToken) { + throw new Error('Tinybird token is missing. Please provide it via environment variable.'); + }Committable suggestion skipped: line range outside the PR's diff.
frontend/server/api/project/[slug]/index.ts (1)
3-3: 💡 Verification agent
🧩 Analysis chain
Repository URLs standardized to full paths: Ensure consistent implementation across the application.
The change from shorthand repository names to full URLs is a positive improvement for consistency. The addition of a Gerrit repository with a different URL format also expands system capabilities.
To ensure this change is consistently implemented across the codebase, let's check for other instances of repository references:
Also applies to: 7-7, 11-11, 15-15, 19-21
🏁 Script executed:
#!/bin/bash # Look for other instances of shorthand repository references that might need updating rg -l "projectSlug.*kubernetes|projectSlug.*crowdDotDev" --type tsLength of output: 142
Repository URL Standardization: Update All References
- The update in
frontend/server/api/project/[slug]/index.ts(line 3) correctly uses the full URL (https://github.com/kubernetes/kubernetes).- However, our search revealed an additional instance in
frontend/server/api/search.tsusing a shorthand repository reference.- Please update all such cases—including those at lines 7-7, 11-11, 15-15, and 19-21 if applicable—to ensure consistent use of full repository URLs across the codebase.
frontend/server/mocks/active-contributors.mock.ts (1)
105-106:
⚠️ Potential issueDate inconsistency in monthly mock data.
The first entry in the monthly data has
fromDate: '2024-11-11T00:00:00Z'andtoDate: '2024-01-31T00:00:00Z', where the toDate is earlier than the fromDate, which is logically inconsistent.- fromDate: '2024-11-11T00:00:00Z', - toDate: '2024-01-31T00:00:00Z', + fromDate: '2024-01-01T00:00:00Z', + toDate: '2024-01-31T00:00:00Z',📝 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.fromDate: '2024-01-01T00:00:00Z', toDate: '2024-01-31T00:00:00Z',frontend/server/data/types.ts (2)
1-1:
⚠️ Potential issueMissing import for $fetch type.
The code references
$fetchbut doesn't import it. This could lead to TypeScript compilation errors.+import type { $Fetch } from 'nitropack'; export type FetchFunction = typeof $fetch;Committable suggestion skipped: line range outside the PR's diff.
18-23: 🛠️ Refactor suggestion
Architectural concern noted in comment but not addressed.
The comment indicates that the
ActiveContributorsResponseDatatype shouldn't be defined in this file due to architectural concerns, but it remains here. Consider addressing this by moving the type to an appropriate location.Consider refactoring the architecture by:
- Create a separate directory/file structure that clearly separates API and data layer types
- Move this type to an appropriate API types file (e.g.,
frontend/server/api/types.ts)Additionally, the type definition is confusing as it defines both a type and an array:
-export type ActiveContributorsResponseData = { - fromDate: string; - toDate: string; - contributors: number; -}[]; +export type ActiveContributorsDataPoint = { + fromDate: string; + toDate: string; + contributors: number; +}; + +export type ActiveContributorsResponseData = ActiveContributorsDataPoint[];📝 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.// This shouldn't be here, it represents the format the API should return but the data layer shouldn't know about the API layer. export type ActiveContributorsDataPoint = { fromDate: string; toDate: string; contributors: number; }; export type ActiveContributorsResponseData = ActiveContributorsDataPoint[];frontend/server/api/project/[slug]/contributors/active-contributors.get.ts (2)
53-55: 🛠️ Refactor suggestion
Add date validation for fromDate and toDate parameters
When parsing ISO date strings, it's important to validate that they are valid dates. Currently, if an invalid date string is provided, parseISO will return an invalid date object that might cause issues downstream.
if (query.fromDate && (query.fromDate as string).trim() !== '') { - filter.fromDate = parseISO(query.fromDate as string); + const parsedFromDate = parseISO(query.fromDate as string); + if (!isNaN(parsedFromDate.getTime())) { + filter.fromDate = parsedFromDate; + } else { + // Consider throwing an error or setting a default date + console.warn(`Invalid fromDate: ${query.fromDate}`); + } } if (query.toDate && (query.toDate as string).trim() !== '') { - filter.toDate = parseISO(query.toDate as string); + const parsedToDate = parseISO(query.toDate as string); + if (!isNaN(parsedToDate.getTime())) { + filter.toDate = parsedToDate; + } else { + // Consider throwing an error or setting a default date + console.warn(`Invalid toDate: ${query.toDate}`); + } }Also applies to: 57-59
61-63: 🛠️ Refactor suggestion
Consider error handling for dataSource.fetchActiveContributors
The fetchActiveContributors method might fail due to network issues, invalid responses, or other reasons. Adding error handling would make the API more robust.
const dataSource = createActiveContributorsDataSource(); -return await dataSource.fetchActiveContributors(filter); +try { + return await dataSource.fetchActiveContributors(filter); +} catch (error) { + console.error('Error fetching active contributors:', error); + throw createError({ + statusCode: 500, + statusMessage: 'Failed to fetch active contributors data', + data: { message: 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 dataSource = createActiveContributorsDataSource(); try { return await dataSource.fetchActiveContributors(filter); } catch (error) { console.error('Error fetching active contributors:', error); throw createError({ statusCode: 500, statusMessage: 'Failed to fetch active contributors data', data: { message: error.message } }); }frontend/server/data/tinybird/active-contributors-data-source.ts (2)
64-67:
⚠️ Potential issuePrevent division by zero in percentage calculation
If
previousContributorCountis zero, the percentage calculation will result in Infinity. Add a check to handle this case.- const percentageChange = ((currentContributorCount - previousContributorCount) / previousContributorCount) * 100; + const percentageChange = previousContributorCount === 0 + ? currentContributorCount > 0 ? 100 : 0 // If previous is 0, and current > 0, it's a 100% increase; if both are 0, it's 0% + : ((currentContributorCount - previousContributorCount) / previousContributorCount) * 100;📝 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 currentContributorCount = currentSummary.data[0].contributor_count; const previousContributorCount = previousSummary.data[0].contributor_count; const percentageChange = previousContributorCount === 0 ? currentContributorCount > 0 ? 100 : 0 // If previous is 0, and current > 0, it's a 100% increase; if both are 0, it's 0% : ((currentContributorCount - previousContributorCount) / previousContributorCount) * 100; const changeValue = currentContributorCount - previousContributorCount;
49-53: 🛠️ Refactor suggestion
Handle potential Promise.all rejection and empty responses
Using Promise.all is efficient, but if any of the requests fail, the entire operation will fail. Consider adding proper error handling and checks for empty responses.
- const [currentSummary, previousSummary, data] = await Promise.all([ - fetchFromTinybird('/v0/pipes/active_contributors.json', currentSummaryQuery, fetchFn), - fetchFromTinybird('/v0/pipes/active_contributors.json', previousSummaryQuery, fetchFn), - fetchFromTinybird('/v0/pipes/active_contributors.json', dataQuery, fetchFn) - ]); + let currentSummary, previousSummary, data; + try { + [currentSummary, previousSummary, data] = await Promise.all([ + fetchFromTinybird('/v0/pipes/active_contributors.json', currentSummaryQuery, fetchFn), + fetchFromTinybird('/v0/pipes/active_contributors.json', previousSummaryQuery, fetchFn), + fetchFromTinybird('/v0/pipes/active_contributors.json', dataQuery, fetchFn) + ]); + } catch (error) { + console.error('Error fetching data from Tinybird:', error); + throw new Error(`Failed to fetch contributor data: ${error.message}`); + } + + // Validate that we have data + if (!currentSummary?.data?.length || !previousSummary?.data?.length) { + console.warn('Missing data in Tinybird response'); + // Consider returning a default response or throwing an error + }📝 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.let currentSummary, previousSummary, data; try { [currentSummary, previousSummary, data] = await Promise.all([ fetchFromTinybird('/v0/pipes/active_contributors.json', currentSummaryQuery, fetchFn), fetchFromTinybird('/v0/pipes/active_contributors.json', previousSummaryQuery, fetchFn), fetchFromTinybird('/v0/pipes/active_contributors.json', dataQuery, fetchFn) ]); } catch (error) { console.error('Error fetching data from Tinybird:', error); throw new Error(`Failed to fetch contributor data: ${error.message}`); } // Validate that we have data if (!currentSummary?.data?.length || !previousSummary?.data?.length) { console.warn('Missing data in Tinybird response'); // Consider returning a default response or throwing an error }frontend/server/data/tinybird/tinybird.ts (2)
107-113: 🛠️ Refactor suggestion
Implement the TODO for scalable response type handling
The TODO comment correctly identifies an issue with the current approach to response type handling. Consider implementing a generic response type parameter.
- // TODO: this isn't scalable, we will have many other request types, and thus many other response types. - // Could we specify the response type in the function signature? - if (query.granularity) { - return result as TinybirdContributorsDataResponse; - } - - return result as TinybirdContributorsSummaryResponse; + // Determine response type based on query parameters + return query.granularity + ? (result as TinybirdContributorsDataResponse) + : (result as TinybirdContributorsSummaryResponse);For a more scalable solution, consider refactoring the function signature to use generic types:
export async function fetchFromTinybird<T extends TinybirdContributorsResponse>( endpoint: string, query: ActiveContributorsFilter, fetchFn: FetchFunction = $fetch ): Promise<T> { // ... existing implementation ... return result as T; }
1-3:
⚠️ Potential issueFix the import statement for date-fns format
The import statement for the format function from date-fns is incorrect. The correct syntax is to import the function directly, not with the
{ format } from "date-fns/format"pattern.-import { format } from "date-fns/format"; +import { format } from "date-fns";📝 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 { format } from "date-fns"; import {useRuntimeConfig} from '#imports'; import type {ActiveContributorsFilter, ContributorsFilterGranularity} from "~~/server/data/types";
borfast
force-pushed
the
feature/add-active-contributors-data-fetching
branch
2 times, most recently
from
69fc3d5 to
a738512
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
frontend/server/api/project/[slug]/contributors/active-contributors.get.ts (1)
25-25: Consider clarifying the time-period parameter documentation.The comment mentions that time-period isn't defined yet, but includes some potential values. Since this PR implements date filtering with separate
fromDateandtoDateparameters, consider updating this comment to reflect the current implementation or add a TODO to revisit this when the time-period parameter is implemented.- * - time-period: string // This isn't defined yet, but we'll add '90d', '1y', '5y' for now + * - fromDate: string // ISO 8601 date string for filtering start date + * - toDate: string // ISO 8601 date string for filtering end date + * // TODO: Implement time-period parameter ('90d', '1y', '5y') as an alternative to fromDate/toDatefrontend/server/data/types.ts (1)
17-19: Valid concern about layer separation.The comment correctly identifies a potential architectural issue with mixing API response formats in the data layer. Consider addressing this by:
- Moving these types to a separate file (e.g.,
frontend/server/api/types.ts)- Creating separate interfaces for data layer and API layer with mappers between them
This would improve separation of concerns and make the codebase more maintainable.
frontend/server/api/search.ts (1)
49-51: Search functionality uses case-insensitive matching on full URLs.The search logic filters by performing a case-insensitive inclusion check on the full repository URL. This might affect user experience as:
- Users may not know or search for the full URL pattern
- Searching for "github" will now return all GitHub repositories
Consider enhancing the search to also match against repository shortnames or adding dedicated fields for more user-friendly searching.
- repositories: repositories.filter((repository) => repository.name.toLowerCase().includes(query.toLowerCase())), + repositories: repositories.filter((repository) => { + const lowerQuery = query.toLowerCase(); + // Extract shortname from URL for better matching + const shortName = repository.name.split('/').slice(-2).join('/').toLowerCase(); + return repository.name.toLowerCase().includes(lowerQuery) || + shortName.includes(lowerQuery) || + repository.projectName.toLowerCase().includes(lowerQuery); + }),frontend/server/data/tinybird/tinybird.test.ts (5)
10-14: Consider using a more realistic mock fetch implementationThe current mock implementation returns an empty object, which doesn't reflect a real API response structure. This might not fully test how your code handles actual API responses.
Consider enhancing your mock to return a more realistic response structure:
- const mockFetch = vi.fn().mockImplementation(() => Promise.resolve({})) as unknown as $Fetch; + const mockFetch = vi.fn().mockImplementation(() => Promise.resolve({ + data: { contributors: 10 }, + json: () => Promise.resolve({ data: { contributors: 10 } }) + })) as unknown as $Fetch;
34-36: Consider removing unnecessary type assertionsThe type assertions (
<Date>) might be redundant since the type is already defined in theActiveContributorsFilterinterface.- fromDate: formatDateForTinyBird(<Date>query.fromDate), - toDate: formatDateForTinyBird(<Date>query.toDate), + fromDate: formatDateForTinyBird(query.fromDate), + toDate: formatDateForTinyBird(query.toDate),
66-75: Fix indentation inconsistencies in the error testThe indentation in this test case is inconsistent with the rest of the file, which affects readability.
test('should handle API errors gracefully', async () => { const mockFetch = vi.fn().mockImplementation(() => Promise.reject(new Error('API Error'))) as unknown as $Fetch; const endpoint = '/v0/pipes/active_contributors.json'; - const query: ActiveContributorsFilter = { - project: 'test-project', - granularity: ContributorsFilterGranularity.MONTHLY, - } - await expect(fetchFromTinybird(endpoint, query, mockFetch)).rejects.toThrow('API Error'); -}); + const query: ActiveContributorsFilter = { + project: 'test-project', + granularity: ContributorsFilterGranularity.MONTHLY, + } + await expect(fetchFromTinybird(endpoint, query, mockFetch)).rejects.toThrow('API Error'); + });
66-75: Consider reusing the existing mock for the error testA new mock fetch function is created specifically for the error test. For consistency, consider reusing and reconfiguring the mock defined at the test suite level.
test('should handle API errors gracefully', async () => { - const mockFetch = vi.fn().mockImplementation(() => Promise.reject(new Error('API Error'))) as unknown as $Fetch; + // Reset the mock and configure it to reject + mockFetch.mockReset().mockImplementation(() => Promise.reject(new Error('API Error'))); const endpoint = '/v0/pipes/active_contributors.json'; const query: ActiveContributorsFilter = { project: 'test-project', granularity: ContributorsFilterGranularity.MONTHLY, } await expect(fetchFromTinybird(endpoint, query, mockFetch)).rejects.toThrow('API Error'); + // Reset the mock to its original implementation for subsequent tests + mockFetch.mockReset().mockImplementation(() => Promise.resolve({})); });
1-75: Consider adding additional test scenariosThe current tests cover basic request parameter validation and error handling, but you might want to add tests for other scenarios such as:
- Testing actual response parsing from Tinybird
- Testing different granularity values
- Testing with multiple projects or repos
- Testing edge cases like empty responses or malformed data
Comprehensive test coverage will help ensure the robustness of your Tinybird integration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 96c3f960a5d346b3242a115b93e38c64b1836a0c and a7385121550cac84a2ab759ea88b49bc1167c7f7.
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
frontend/app/components/modules/project/components/contributors/active-contributors.vue(1 hunks)frontend/nuxt.config.ts(1 hunks)frontend/package.json(3 hunks)frontend/server/api/project/[slug]/contributors/active-contributors.get.ts(1 hunks)frontend/server/api/project/[slug]/index.ts(1 hunks)frontend/server/api/project/index.ts(1 hunks)frontend/server/api/search.ts(1 hunks)frontend/server/data/active-contributors-data-source.ts(1 hunks)frontend/server/data/tinybird/active-contributors-data-source.test.ts(1 hunks)frontend/server/data/tinybird/active-contributors-data-source.ts(1 hunks)frontend/server/data/tinybird/tinybird.test.ts(1 hunks)frontend/server/data/tinybird/tinybird.ts(1 hunks)frontend/server/data/types.ts(1 hunks)frontend/server/data/util.test.ts(1 hunks)frontend/server/data/util.ts(1 hunks)frontend/server/mocks/active-contributors.mock.ts(3 hunks)frontend/server/mocks/tinybird-active-contributors-response.mock.ts(1 hunks)frontend/vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/server/mocks/tinybird-active-contributors-response.mock.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- frontend/vitest.config.ts
- frontend/app/components/modules/project/components/contributors/active-contributors.vue
- frontend/server/data/util.ts
- frontend/server/data/util.test.ts
- frontend/server/api/project/index.ts
- frontend/nuxt.config.ts
- frontend/server/data/tinybird/active-contributors-data-source.test.ts
- frontend/server/data/active-contributors-data-source.ts
- frontend/server/api/project/[slug]/index.ts
- frontend/server/mocks/active-contributors.mock.ts
- frontend/server/data/tinybird/tinybird.ts
- frontend/server/data/tinybird/active-contributors-data-source.ts
- frontend/package.json
🔇 Additional comments (19)
frontend/server/api/project/[slug]/contributors/active-contributors.get.ts (7)
21-21: Parameter name change looks good.The parameter rename from
intervaltogranularityis more descriptive and aligns with theContributorsFilterGranularityenum defined in the types file.
26-32: Imports look appropriate.The imports are aligned with the functionality being implemented, importing necessary date parsing functions and the data source interfaces.
35-35: Remember to address the TODO.The TODO comment about checking project configuration should be addressed in a follow-up PR if not part of this one.
Is this something that should be tracked with a separate issue?
40-47: Good filter structure with appropriate default values.The filter object is well-structured with proper typing and default values. Setting the default granularity to QUARTERLY is a good choice for reasonable default behavior.
49-51: Good handling of optional repository parameter.The code correctly handles the optional repository parameter, including checking for empty strings to avoid sending invalid filters.
53-59: Date parsing looks correct.The code properly uses
parseISOto parse the date strings from query parameters into Date objects for the filter. The empty string check is a good validation step.
61-72: Robust error handling implementation.The error handling is well-implemented with:
- Proper try-catch block
- Error logging with details
- Meaningful error response with appropriate status code and message
- Including the original error message in the data
This ensures that API consumers get useful error information while also logging details for debugging.
frontend/server/data/types.ts (5)
1-1: Good type definition for FetchFunction.Exporting the type of
$fetchasFetchFunctionmakes the code more maintainable and allows for easier mocking in tests.
3-7: Well-defined enum for granularity options.The
ContributorsFilterGranularityenum clearly defines the available options for time granularity and uses appropriate string values that match what would be expected in API requests.
9-15: Filter type definition is clear and concise.The
ActiveContributorsFiltertype correctly defines which fields are required (project) and which are optional. The types for each field are appropriate, usingDateobjects for date fields which allows for proper date manipulation.
19-24: Response type structure looks good.The
ActiveContributorsDataPointtype accurately represents the data structure for each point in the response, with appropriate types for each field.
25-35: Check date type consistency in API response.There's a potential type inconsistency:
- The API documentation at the top of the first file indicates
periodFromandperiodToshould be strings- However, they're defined as
Dateobjects hereThis might cause serialization issues when the API responds, as Date objects need to be converted to strings. Consider either:
- Updating these to be string types to match the API documentation, or
- Ensuring there's a serialization step before sending the response
Is there a serialization step that converts Date objects to strings in the API response? If not, you might need to update the types or add a conversion step.
frontend/server/api/search.ts (3)
19-19: New Gerrit project added.The addition of the Gerrit project is consistent with the PR objectives to add active contributors data fetching from additional sources.
34-39:Details
❓ Verification inconclusive
New Gerrit repository added with a different URL pattern.
The new Gerrit repository uses a different URL pattern compared to the GitHub repositories. Ensure that any code processing these URLs handles both formats correctly.
🏁 Script executed:
#!/bin/bash # Check how repository URLs are processed in the codebase # Look for code that might be parsing or processing repository URLs echo "Checking for URL processing patterns..." rg -A 3 "repository.name|repository.url" --type ts --type vue # Check if there's any code that specifically handles GitHub URLs which might need updating echo "Checking for GitHub-specific URL handling..." rg -A 3 "github.com" --type ts --type vueLength of output: 349
Action Required: Verify URL Processing for Mixed Repository Formats
The Gerrit repository entry (lines 34-39 in
frontend/server/api/search.ts) has been updated with a new URL format. However, our initial automated searches for URL processing logic did not yield conclusive results—specifically, searches including Vue files failed due to an unrecognized file type error. Please manually verify that all code responsible for processing repository URLs correctly handles both GitHub-style URLs and the updated Gerrit URL format. Consider checking for any assumptions around the URL pattern that might only cater to GitHub repositories.
- File:
frontend/server/api/search.ts(Lines: 34-39)- Action: Ensure URL parsing logic accommodates both GitHub and Gerrit URL structures.
24-24:Details
❓ Verification inconclusive
Repository URLs updated to use full paths instead of shorthand names.
The repository names have been standardized to use complete URLs rather than shorthand notations. This change enhances clarity and consistency across the application.
Also applies to: 30-30
🏁 Script executed:
#!/bin/bash # Check if the repository URLs are used consistently across the codebase # Look for any other instances of repository names that might need updating echo "Checking for other instances of repository names that might need updating..." rg -l "kubernetes/kubernetes|CrowdDotDev/crowd-kube" --type ts --type vueLength of output: 266
Below is an updated verification step to ensure that all shorthand repository URLs have been fully replaced—even in Vue files. The previous command failed to include Vue files properly (producing the error “unrecognized file type: vue”), so please run the following shell script to re-run the search:
#!/bin/bash echo "Searching for shorthand URLs in Vue files..." rg -l "kubernetes/kubernetes|CrowdDotDev/crowd-kube" -g "*.vue" echo "Searching for shorthand URLs in TypeScript files..." rg -l "kubernetes/kubernetes|CrowdDotDev/crowd-kube" --type tsRepository URLs standardization verification:
• Infrontend/server/api/search.ts(lines 24 and 30), the repository URL now uses the full path (e.g.,https://github.com/kubernetes/kubernetes).
• Please verify that no other files (including.vuefiles) still use shorthand repository names.Once you’ve confirmed that these changes are consistent across the codebase, you can mark this review as approved.
frontend/server/data/tinybird/tinybird.test.ts (4)
1-9: Good job on comprehensive importsThe imports are well-organized, importing both testing utilities and the necessary types/functions to be tested.
15-42: The first test case is well-structuredThis test effectively verifies that the correct parameters are sent to the Tinybird API, including the formatted dates and authorization token.
44-64: Good test for parameter exclusionThis test correctly verifies that undefined parameters aren't included in the request, which is an important edge case to cover.
10-14:Details
❓ Verification inconclusive
Verify runtime config availability in the test environment
The tests rely on
useRuntimeConfig()to access the Tinybird token. Make sure this configuration is properly set up in your test environment, or consider mocking it.
🏁 Script executed:
#!/bin/bash # Description: Check if runtime config is correctly set up for testing # Look for Vitest or Nuxt test configuration files that might setup the runtime config echo "Searching for test configuration files..." fd -e js -e ts "vitest.config|test.config|nuxt.test" --exec cat {} # Check if there's any mock for useRuntimeConfig in the codebase echo -e "\nChecking for useRuntimeConfig mocks..." rg -p "mock.*useRuntimeConfig|vi\.mock.*#imports" -A 5 -B 5Length of output: 467
Action: Verify Runtime Config Setup
The test configuration is using the Nuxt environment (via
defineVitestConfigfrom@nuxt/test-utils/config), which should normally provide a runtime configuration. However, no explicit mocks foruseRuntimeConfigwere found. Please ensure that a validtinybirdTokenis available during tests. If you encounter issues with missing or undefined tokens, consider adding a mock foruseRuntimeConfigdirectly in your test file. For example:vi.mock('#imports', () => ({ useRuntimeConfig: () => ({ tinybirdToken: 'fake-token' }) }));
- Verify that the Nuxt test environment instantiates runtime config correctly.
- If necessary, add an explicit mock to guarantee that
tinybirdTokenis provided during test execution.
emlimlf
left a comment
emlimlf
left a comment
There was a problem hiding this comment.
My only nitpick is the use of another date transformation plugin. We've decided to just use Luxon for any date transformation.
frontend/server/api/project/[slug]/contributors/active-contributors.get.ts
Outdated
Show resolved
Hide resolved
Then we seem to have three libraries already. I just realised there's also Day.js in there. 😅 I'll chage it to use Fluxon. Honestly, this seems to point to a bigger problem, which is scattered documentation and poor onboarding. I searched Confluence and found three different places where Luxon is referenced, none of them within the new-Insights section, and all of them pertaining to UI development, not backend - in fact, I didn't see an equivalent backend development section there. So the question here is: how are team members supposed to know about these decisions? It wasn't mentioned to me at any point, and expecting team members to scour Confluence doesn't seem reasonable either, because there's just too much in there and too scattered. I know this is not the place to discuss this and that's not what I intend, but right now I just want to leave it documented somewhere non-ephemeral where it doesn't end up forgotten (i.e. not Slack), because honestly, this lack or organised documentation and onboarding seems more and more like a big problem to me, and I want to circle back to this at some point once we have some more time. |
Signed-off-by: Raúl Santos <4837+borfast@users.noreply.github.com>
Signed-off-by: Raúl Santos <4837+borfast@users.noreply.github.com>
Signed-off-by: Raúl Santos <4837+borfast@users.noreply.github.com>
Signed-off-by: Raúl Santos <4837+borfast@users.noreply.github.com>
borfast
force-pushed
the
feature/add-active-contributors-data-fetching
branch
from
b1abc88 to
294ac17
Compare
Signed-off-by: Raúl Santos <4837+borfast@users.noreply.github.com>
borfast
force-pushed
the
feature/add-active-contributors-data-fetching
branch
from
294ac17 to
2c6794f
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
frontend/server/data/tinybird/tinybird.ts (1)
1-119:⚠️ Potential issueCommented-out code should be removed or implemented
The entire file is commented out. Commented-out code should not be committed to the repository as it creates confusion and makes the codebase harder to maintain. If this represents future work, it should be tracked in issues/tickets instead, or if it's experimental, it should be in a feature branch.
Either implement this code properly or remove it completely. If you need this for reference, consider adding it to documentation or a design document instead.
🧹 Nitpick comments (6)
frontend/server/utils/tinybird.ts (3)
19-22: Simplify date formatting functionThe null coalescing operator (
??) informatDateForTinyBirdis unnecessary astoFormat()will not return null/undefined when called on a valid DateTime object.- return date.toFormat('yyyy-MM-dd 00:00:00') ?? ''; + return date.toFormat('yyyy-MM-dd 00:00:00');
38-38: Fix typo in commentThere's a small typo in the comment.
- // 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 values to TinyBird, so we remove those from the query. We also format DateTime objects for TinyBird
50-57: Authentication redundancyThe token is being sent both as a query parameter and in the Authorization header. This is redundant and could potentially create confusion. Consider using only one method for authentication, preferably the Authorization header which is more secure.
return await $fetch(url, { query: { ...processedQuery, - token: config.tinybirdToken }, headers: { Authorization: `Bearer ${config.tinybirdToken}`, } });frontend/server/api/project/[slug]/contributors/active-contributors.get.ts (1)
63-72: Improve error handlingThe current error handling exposes internal error messages in the response. This can potentially leak sensitive information. Consider providing a generic error message to the client and only logging the detailed error internally.
try { return await dataSource.fetchActiveContributors(filter); } catch (error) { console.error('Error fetching active contributors:', error); throw createError({ statusCode: 500, statusMessage: 'Failed to fetch active contributors data', - data: { message: error.message } + data: { message: 'An internal server error occurred while fetching contributor data' } }); }frontend/server/data/tinybird/active-contributors-data-source.ts (2)
60-69: Improve null/undefined checking and type safety.The current implementation has a check for
data !== undefinedbut still uses optional chaining (?.) when accessingdata.map(). Also, the type assertion may be unnecessary if proper TypeScript typing is used.Consider refining this section:
let processedData: ActiveContributorsResponseData = []; if (data !== undefined) { - processedData = (data as TinybirdResponse<TinybirdContributorsData>)?.data.map( + processedData = data.data.map( (item): ActiveContributorsResponseData[0] => ({ startDate: item.startDate, endDate: item.endDate, contributors: item.contributorCount }) ); }If you need the type assertion for type safety, consider adding a more robust null check:
let processedData: ActiveContributorsResponseData = []; -if (data !== undefined) { +if (data && data.data && Array.isArray(data.data)) { processedData = (data as TinybirdResponse<TinybirdContributorsData>).data.map( (item): ActiveContributorsResponseData[0] => ({ startDate: item.startDate, endDate: item.endDate, contributors: item.contributorCount }) ); }
29-96: Add JSDoc comments to the fetchActiveContributors function.Adding proper JSDoc comments would improve maintainability and help other developers understand the function's purpose, parameters, and return value.
Consider adding JSDoc comments:
+/** + * Fetches active contributors data from Tinybird + * + * @param filter - Filtering criteria for the active contributors query + * @returns Promise that resolves to formatted active contributors response + */ async function fetchActiveContributors(filter: ActiveContributorsFilter) { // Function implementation... }
🛑 Comments failed to post (4)
frontend/server/data/tinybird/tinybird.test.ts (1)
1-76:
⚠️ Potential issueCommented out test file needs attention
This entire test file appears to be commented out. If these tests are intended to be part of the PR, they should be uncommented before merging.
If you're temporarily commenting them out to address issues, consider adding a TODO comment explaining why they're commented out and when they'll be re-enabled.
frontend/server/data/tinybird/tinybird.ts (1)
70-72: 💡 Verification agent
🧩 Analysis chain
Duplicated function with frontend/server/utils/tinybird.ts
The commented
formatDateForTinyBirdfunction and date processing logic duplicates functionality already implemented infrontend/server/utils/tinybird.ts. This would lead to maintenance issues if both were active.Also applies to: 91-100
🏁 Script executed:
#!/bin/bash # Check if there are other utility functions that might be duplicated echo "Checking for other date formatting utilities in the codebase..." rg -A 3 "formatDate.*TinyBird|date.*format.*TinyBird" --type tsLength of output: 1453
Action Required: Remove Duplicate Implementation of
formatDateForTinyBirdThe file
frontend/server/data/tinybird/tinybird.tscontains commented-out code forformatDateForTinyBird(lines 70–72 and 91–100) that duplicates the active implementation present infrontend/server/utils/tinybird.ts. To avoid confusion and potential maintenance issues in the future, please remove the commented duplicate code.
Affected Files & Lines:
frontend/server/data/tinybird/tinybird.ts(lines 70–72 and 91–100)Recommended Fix:
- Remove the commented-out
formatDateForTinyBirdfunction fromfrontend/server/data/tinybird/tinybird.ts.frontend/server/data/tinybird/active-contributors-data-source.ts (2)
71-81: 🛠️ Refactor suggestion
Add defensive null/undefined checking for API response data.
Currently, the code directly accesses properties from the API response without verifying if they exist. If the response structure is unexpected, this could lead to runtime errors.
Consider adding more defensive checks:
-const currentContributorCount = currentSummary.data[0].contributorCount; -const previousContributorCount = previousSummary.data[0].contributorCount; +const currentContributorCount = currentSummary?.data?.[0]?.contributorCount ?? 0; +const previousContributorCount = previousSummary?.data?.[0]?.contributorCount ?? 0; const changeValue = currentContributorCount - previousContributorCount;This ensures that even if the API response structure changes or returns unexpected values, the code won't crash with runtime errors.
📝 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 currentContributorCount = currentSummary?.data?.[0]?.contributorCount ?? 0; const previousContributorCount = previousSummary?.data?.[0]?.contributorCount ?? 0; 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; }
29-96: 🛠️ Refactor suggestion
Add error handling to the fetchActiveContributors function.
The function currently lacks explicit error handling for API request failures. If any of the Tinybird API calls fail, the error will propagate up the call stack without any specific handling or helpful error messages.
Consider adding try/catch blocks to handle potential errors and provide meaningful error information:
async function fetchActiveContributors(filter: ActiveContributorsFilter) { const dates = getPreviousDates(filter.startDate, filter.endDate); const currentSummaryQuery = { project: filter.project, repo: filter.repo, startDate: dates.current.from, endDate: dates.current.to }; const previousSummaryQuery = { project: filter.project, repo: filter.repo, startDate: dates.previous.from, endDate: dates.previous.to }; const dataQuery = { project: filter.project, granularity: filter.granularity, repo: filter.repo, startDate: dates.current.from, endDate: dates.current.to }; + try { const [currentSummary, previousSummary, data] = await Promise.all([ fetchTinybird<TinybirdContributorsSummary>('/v0/pipes/active_contributors.json', currentSummaryQuery), fetchTinybird<TinybirdContributorsSummary>('/v0/pipes/active_contributors.json', previousSummaryQuery), fetchTinybird<TinybirdContributorsData>('/v0/pipes/active_contributors.json', dataQuery) ]); let processedData: ActiveContributorsResponseData = []; if (data !== undefined) { processedData = (data as TinybirdResponse<TinybirdContributorsData>)?.data.map( (item): ActiveContributorsResponseData[0] => ({ startDate: item.startDate, endDate: item.endDate, contributors: item.contributorCount }) ); } 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 response: ActiveContributorsResponse = { summary: { current: currentContributorCount, previous: previousContributorCount, percentageChange, changeValue, periodFrom: dates.current.from, periodTo: dates.current.to, }, data: processedData, }; return response; + } catch (error) { + console.error('Error fetching active contributors data:', error); + throw new Error(`Failed to fetch active contributors data: ${error instanceof Error ? error.message : String(error)}`); + } }📝 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.async function fetchActiveContributors(filter: ActiveContributorsFilter) { const dates = getPreviousDates(filter.startDate, filter.endDate); const currentSummaryQuery = { project: filter.project, repo: filter.repo, startDate: dates.current.from, endDate: dates.current.to }; const previousSummaryQuery = { project: filter.project, repo: filter.repo, startDate: dates.previous.from, endDate: dates.previous.to }; const dataQuery = { project: filter.project, granularity: filter.granularity, repo: filter.repo, startDate: dates.current.from, endDate: dates.current.to }; try { const [currentSummary, previousSummary, data] = await Promise.all([ fetchTinybird<TinybirdContributorsSummary>('/v0/pipes/active_contributors.json', currentSummaryQuery), fetchTinybird<TinybirdContributorsSummary>('/v0/pipes/active_contributors.json', previousSummaryQuery), fetchTinybird<TinybirdContributorsData>('/v0/pipes/active_contributors.json', dataQuery) ]); let processedData: ActiveContributorsResponseData = []; if (data !== undefined) { processedData = (data as TinybirdResponse<TinybirdContributorsData>)?.data.map( (item): ActiveContributorsResponseData[0] => ({ startDate: item.startDate, endDate: item.endDate, contributors: item.contributorCount }) ); } 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 response: ActiveContributorsResponse = { summary: { current: currentContributorCount, previous: previousContributorCount, percentageChange, changeValue, periodFrom: dates.current.from, periodTo: dates.current.to, }, data: processedData, }; return response; } catch (error) { console.error('Error fetching active contributors data:', error); throw new Error(`Failed to fetch active contributors data: ${error instanceof Error ? error.message : String(error)}`); } }
3 participants
This adds the necessary backend code to serve requests from the frontend for the Active Contributors widget.
It was working a couple of days ago but for some reason now I can't get data from TinyBird, it always responds with 0 contributors. This is despite the fact that if I open the same URL directly in the browser with the same query parameters, I get the expected numbers.
Even with this issue, I decided to open this PR so that we can start more easily aligning the frontend and the backend.
Summary by CodeRabbit
New Features
Refactor