feat: add /query-cache #288
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer's Guide by SourceryThis pull request implements the 'Query Cache' feature by adding a new menu item, creating a new chart component to display query cache metrics, and defining a new query configuration to fetch and display query cache details from the 'system.query_cache' table. File-Level Changes
Tips
|
There was a problem hiding this comment.
Hey @duyet - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Check for division by zero. (link)
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 5 other issues
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| { | ||
| title: 'Query Cache', | ||
| href: '/query-cache', | ||
| description: 'Query cache usage', | ||
| icon: DatabaseZapIcon, | ||
| }, |
There was a problem hiding this comment.
suggestion: Consider adding a unique identifier for the new menu item.
Adding a unique identifier (e.g., id: 'query-cache') to each menu item can help with tracking and managing menu items programmatically.
| { | |
| title: 'Query Cache', | |
| href: '/query-cache', | |
| description: 'Query cache usage', | |
| icon: DatabaseZapIcon, | |
| }, | |
| { | |
| id: 'query-cache', | |
| title: 'Query Cache', | |
| href: '/query-cache', | |
| description: 'Query cache usage', | |
| icon: DatabaseZapIcon, | |
| }, |
There was a problem hiding this comment.
This one can respect the type when suggesting adding an id
There was a problem hiding this comment.
Thank you for pointing that out, @duyet. You're correct that the suggestion should respect the existing type definitions. If the MenuItem type does not currently include an id field, we should update the type definition accordingly. Here's an example of how you might adjust the type and the menu item:
-
Update the
MenuItemtype to include anidfield:interface MenuItem { id: string; title: string; href: string; description: string; icon: React.ComponentType; }
-
Add the
idfield to the new menu item:{ id: 'query-cache', title: 'Query Cache', href: '/query-cache', description: 'Query cache usage', icon: DatabaseZapIcon, }
Would you like me to update the type definition in the PR, or do you prefer to handle it?
| const { data } = await fetchData< | ||
| { | ||
| total_result_size: number | ||
| total_staled_result_size: number | ||
| readable_total_result_size: string | ||
| readable_total_staled_result_size: string | ||
| }[] | ||
| >({ query }) |
There was a problem hiding this comment.
suggestion (bug_risk): Handle potential errors from fetchData.
Consider adding error handling for the fetchData call to manage potential network or query issues gracefully.
| >({ query }) | ||
| const first = data?.[0] | ||
|
|
||
| if (!data || !first) return null |
There was a problem hiding this comment.
suggestion: Provide user feedback when data is not available.
Instead of returning null, consider rendering a message or a placeholder component to inform the user that no data is available.
| if (!data || !first) return null | |
| if (!data || !first) { | |
| return <div>No data available</div> | |
| } |
| query, | ||
| result_size, | ||
| formatReadableSize(result_size) AS readable_result_size, | ||
| round(100 * result_size / max(result_size) OVER ()) AS pct_result_size, |
There was a problem hiding this comment.
issue (bug_risk): Check for division by zero.
Ensure that max(result_size) is never zero to avoid potential division by zero errors.
| expires_in: ColumnFormat.Duration, | ||
| }, | ||
|
|
||
| relatedCharts: [['query-cache', {}]], |
There was a problem hiding this comment.
question: Clarify the purpose of the empty object in relatedCharts.
If the empty object is intended for future use, consider adding a placeholder comment or a TODO to clarify its purpose.
| >({ query }) | ||
| const first = data?.[0] | ||
|
|
||
| if (!data || !first) return null |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!data || !first) return null | |
| if (!data || !first) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Signed-off-by: Duyet Le <5009534+duyet@users.noreply.github.com>
Close #287
Summary by Sourcery
This pull request introduces a new 'Query Cache' feature, adding it to the menu, implementing a chart component to display query cache metrics, and creating a query configuration to fetch and display query cache data.