-
Notifications
You must be signed in to change notification settings - Fork 0
Store chart in local storage #6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request refactors chart data handling to implement local storage caching functionality. The changes move chart data fetching from the server-side Chart component to the client-side Dots component with localStorage support.
- Moved chart data fetching logic from Chart component to Dots component
- Implemented localStorage caching with background refresh strategy
- Added error handling and fallback mechanisms for data loading
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/components/chart.tsx | Simplified to pass userId instead of pre-fetched chart data to Dots component |
| src/components/dots.tsx | Added localStorage caching, async data fetching, and background refresh functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const chartWithColors: ChartWithColorsType[] = fetchedChart.map( | ||
| (item: number) => [item, getColorIndex(item, -1)] |
Copilot
AI
Aug 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The maxValue parameter passed to getColorIndex is -1, which doesn't represent the actual maximum value in the dataset. This will result in incorrect color indexing for the initial data load. Consider calculating the actual maxValue from the fetched data before calling getColorIndex.
| const chartWithColors: ChartWithColorsType[] = fetchedChart.map( | |
| (item: number) => [item, getColorIndex(item, -1)] | |
| const maxValue = Math.max(...fetchedChart); | |
| const chartWithColors: ChartWithColorsType[] = fetchedChart.map( | |
| (item: number) => [item, getColorIndex(item, maxValue)] |
|
|
||
| setTimeout(async () => { | ||
| try { | ||
| const fetchedChart = await fetchChartData(); | ||
| if (fetchedChart) { | ||
| const chartWithColors: ChartWithColorsType[] = fetchedChart.map( | ||
| (item: number) => [item, getColorIndex(item, maxValue)] | ||
| ); | ||
| const newMaxValue = Math.max( | ||
| ...chartWithColors.map((item) => item[0]) | ||
| ); | ||
| updateMaxValue(newMaxValue); | ||
| setAllDots(chartWithColors); | ||
| localStorage.setItem(`chart-${id}`, JSON.stringify(chartWithColors)); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error fetching fresh chart data:', error); | ||
| } | ||
| }, 0); | ||
| } else { | ||
| const fetchedChart = await fetchChartData(); | ||
| if (!fetchedChart) { |
Copilot
AI
Aug 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using setTimeout with 0 delay to defer async operations is an anti-pattern. Consider using a proper background refresh mechanism or moving the refresh logic to a separate effect to avoid potential race conditions and improve code clarity.
| setTimeout(async () => { | |
| try { | |
| const fetchedChart = await fetchChartData(); | |
| if (fetchedChart) { | |
| const chartWithColors: ChartWithColorsType[] = fetchedChart.map( | |
| (item: number) => [item, getColorIndex(item, maxValue)] | |
| ); | |
| const newMaxValue = Math.max( | |
| ...chartWithColors.map((item) => item[0]) | |
| ); | |
| updateMaxValue(newMaxValue); | |
| setAllDots(chartWithColors); | |
| localStorage.setItem(`chart-${id}`, JSON.stringify(chartWithColors)); | |
| } | |
| } catch (error) { | |
| console.error('Error fetching fresh chart data:', error); | |
| } | |
| }, 0); | |
| } else { | |
| const fetchedChart = await fetchChartData(); | |
| if (!fetchedChart) { | |
| setLoadedMaxValue(maxValue); | |
| setLoadedFromStorage(true); | |
| } else { | |
| const fetchedChart = await fetchChartData(); | |
| if (!fetchedChart) { | |
| setHasLoadedFromStorage(true); |
| const chartWithColors: ChartWithColorsType[] = fetchedChart.map( | ||
| (item: number) => [item, getColorIndex(item, maxValue)] | ||
| ); | ||
| const newMaxValue = Math.max( | ||
| ...chartWithColors.map((item) => item[0]) | ||
| ); |
Copilot
AI
Aug 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The maxValue used here is from the stored chart data, not the newly fetched data. This could result in incorrect color indexing if the new data has a different maximum value. Use newMaxValue instead of maxValue.
| const chartWithColors: ChartWithColorsType[] = fetchedChart.map( | |
| (item: number) => [item, getColorIndex(item, maxValue)] | |
| ); | |
| const newMaxValue = Math.max( | |
| ...chartWithColors.map((item) => item[0]) | |
| ); | |
| const newMaxValue = Math.max(...fetchedChart); | |
| const chartWithColors: ChartWithColorsType[] = fetchedChart.map( | |
| (item: number) => [item, getColorIndex(item, newMaxValue)] | |
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
No description provided.