feat: suspend table and related charts #236
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Hey @duyet - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| return ( | ||
| <div className="flex flex-col"> | ||
| <Suspense fallback={<ChartSkeleton />}> | ||
| <RelatedCharts relatedCharts={config.relatedCharts} /> | ||
| </Suspense> | ||
|
|
||
| <DataTable | ||
| <Suspense fallback={<TableSkeleton />}> | ||
| <Table | ||
| title={query.replaceAll('-', ' ')} | ||
| config={config} |
There was a problem hiding this comment.
suggestion: Consider handling errors within the new Page component.
The refactored Page component lacks error handling which was previously managed by a try-catch block. It's important to ensure that errors from the Table component or related processes are caught and handled appropriately to maintain robustness.
| </Suspense> | ||
|
|
||
| <DataTable | ||
| <Suspense fallback={<TableSkeleton />}> | ||
| <Table | ||
| title={query.replaceAll('-', ' ')} | ||
| config={config} | ||
| data={data} | ||
| searchParams={searchParams} | ||
| /> | ||
| </div> | ||
| ) | ||
| } catch (error) { | ||
| return <ErrorAlert title="ClickHouse Error" message={`${error}`} /> | ||
| } | ||
| </Suspense> | ||
| </div> |
There was a problem hiding this comment.
suggestion: Review the use of Suspense for error boundaries or alternative error handling strategies.
While Suspense is used for handling the loading state, consider implementing error boundaries or similar strategies to handle errors that might occur during the rendering of asynchronous components.
| @@ -0,0 +1,71 @@ | |||
| import { DataTable } from '@/components/data-table/data-table' | |||
There was a problem hiding this comment.
suggestion: Check for potential duplication of DataTable component usage.
As the DataTable component is now used within the Table component, ensure there's no redundant usage or similar components that could be consolidated to streamline the codebase.
| import { DataTable } from '@/components/data-table/data-table' | |
| import { Table } from '@/components/table' |
| } | ||
| } | ||
|
|
||
| let sql = config.sql |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
| let sql = config.sql | |
| let {sql} = config |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
No description provided.