Conversation
WalkthroughThe pull request introduces new type definitions, Changes
Suggested reviewers
Possibly related PRs
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between ec3d0faaa407a61a748e55d973db5d90c209df22 and d5ae688. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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: 0
🧹 Nitpick comments (4)
frontend/server/api/collection/index.ts (4)
44-48: Good addition of the separate count query.The implementation correctly adds a separate query to get the total count of collections from TinyBird, which aligns with the PR objective to fix the collection count issue.
However, consider making this second API call conditional on the
countparameter to avoid unnecessary requests when the total count isn't needed.type CollectionCount = {'count(id)': number}; - const collectionCountResult = await fetchFromTinybird<CollectionCount[]>('/v0/pipes/collections_list.json', { - count: true, - }); + let collectionCount = 0; + if (count) { + const collectionCountResult = await fetchFromTinybird<CollectionCount[]>('/v0/pipes/collections_list.json', { + count: true, + }); + collectionCount = collectionCountResult.data[0]?.['count(id)'] || 0; + }
52-52: Update the total property to use the conditional count.If you implement the conditional fetching suggested above, update this line accordingly.
- total: collectionCountResult.data[0]?.['count(id)'] || 0, + total: count ? collectionCount : 0,
58-58: Consider enhancing error handling for multiple API calls.The error message has been improved to be more specific, but it doesn't differentiate between errors from the two API calls. Consider capturing and logging both types of errors distinctly.
- console.error('Error fetching collection list from TinyBird:', error); + console.error('Error in collection API:', error instanceof Error ? error.message : String(error)); + console.error('Error details:', error);
44-52: Consider a more performant approach in the future.While the current implementation correctly addresses the immediate issue, making two separate API calls to the same endpoint may impact performance. In the future, consider working with the TinyBird team to modify the endpoint to return both paginated data and total count in a single response.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 98c9440 and 60a1aadcef50dc0e5f4c10d3488eff4d6d2f60de.
📒 Files selected for processing (1)
frontend/server/api/collection/index.ts(1 hunks)
borfast
force-pushed
the
bugfix/ins-225-collections-total-number-returned-in-the-api-is-not-correct
branch
from
60a1aad to
d2400d6
Compare
borfast
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/server/api/collection/index.ts (1)
44-48: Use a more descriptive alias for'count(id)'.If allowed by the Tinybird pipeline, consider aliasing
'count(id)'to a clearer property name, such astotal_count, to improve readability in this code.frontend/server/api/project/index.ts (1)
59-63: Use a more descriptive alias for'count(id)'.Similarly, consider aliasing
'count(id)'to something liketotal_countin the Tinybird query for better clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 60a1aadcef50dc0e5f4c10d3488eff4d6d2f60de and d2400d6562c63448b70e108337ccfa83641ef65d.
📒 Files selected for processing (2)
frontend/server/api/collection/index.ts(2 hunks)frontend/server/api/project/index.ts(1 hunks)
🔇 Additional comments (4)
frontend/server/api/collection/index.ts (2)
52-52: Good use of a fallback for empty data.Providing a default value of
0ensures the endpoint avoids crashes whencollectionCountResult.datais empty or malformed.
58-58: More descriptive error message.Logging the specific source (
TinyBird) makes troubleshooting easier.frontend/server/api/project/index.ts (2)
67-67: Nice fallback for missing data.Defaulting
totalto0prevents runtime errors in case of an empty result set.
73-73: Clearer error message aids debugging.Specifying that the error occurred while fetching from Tinybird is helpful for pinpointing issues quickly.
borfast
force-pushed
the
bugfix/ins-225-collections-total-number-returned-in-the-api-is-not-correct
branch
from
d2400d6 to
ec3d0fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/server/api/project/index.ts (2)
59-67: Consider potential performance impact of multiple API calls.While the implementation correctly solves the issue of getting accurate counts, be aware that it introduces an additional API call. For high-traffic scenarios, you might want to consider:
- Implementing caching for the count results
- Only making the count request when the
countparameter is true- Exploring if TinyBird can return both the paginated results and total count in a single request
59-63: Consider extracting common patterns if similar code exists elsewhere.According to the AI summary, similar changes were made in the collection API. If these patterns are repeated across multiple files, consider extracting this functionality into a reusable utility function to promote DRY principles.
// Example utility function async function fetchWithTotalCount<T, CountT extends {'count(id)': number}>( endpoint: string, params: Record<string, any> ): Promise<{items: T[], totalCount: number}> { const itemsResult = await fetchFromTinybird<T[]>(endpoint, params); const countResult = await fetchFromTinybird<CountT[]>(endpoint, { count: true }); return { items: itemsResult.data, totalCount: countResult.data[0]?.['count(id)'] || 0 }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d2400d6562c63448b70e108337ccfa83641ef65d and ec3d0faaa407a61a748e55d973db5d90c209df22.
📒 Files selected for processing (2)
frontend/server/api/collection/index.ts(1 hunks)frontend/server/api/project/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/server/api/collection/index.ts
🔇 Additional comments (3)
frontend/server/api/project/index.ts (3)
59-63: Good implementation of a separate count query.The introduction of a specific type for the count result and making a separate API call to get the total project count addresses the issue mentioned in the PR objectives. This ensures the frontend receives the accurate total count regardless of pagination parameters.
67-67: LGTM! Robust handling of the count result.The implementation properly handles potential edge cases by using the optional chaining operator (
?.) and providing a fallback value of 0. This ensures the application doesn't break if the count result is unexpected.
73-73: Improved error logging.The updated error message now clearly indicates that the error occurred while fetching the project list from TinyBird, making debugging easier.
Signed-off-by: Raúl Santos <4837+borfast@users.noreply.github.com>
borfast
force-pushed
the
bugfix/ins-225-collections-total-number-returned-in-the-api-is-not-correct
branch
from
ec3d0fa to
d5ae688
Compare
borfast
deleted the
bugfix/ins-225-collections-total-number-returned-in-the-api-is-not-correct
branch
2 participants
The TinyBird endpoint for the collections and projects list require a separate query to get the total count but our backend API is only making one request for the data, so the total it returns to the frontend is the total number of values in the query result, not the total available in TinyBird.
The ticket in Linear is here.
Summary by CodeRabbit