Fix/217 chart issue#235
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #217 by adding support for different viewing perspectives in the balance chart. The changes enable the chart to correctly display transaction flows from either a user's or project's viewpoint, and implements adaptive time granularity (hourly vs daily) based on the transaction date range.
- Added
viewTypeprop toBalanceChartcomponent to distinguish between "user" and "project" perspectives - Refactored transaction data formatting to calculate correct balance deltas based on transaction type and viewpoint
- Implemented adaptive chart rendering that switches between hourly (for ≤1 day range) and daily (for >1 day range) time granularity
- Enhanced X-axis formatting with proper tick generation for hourly view and date formatting for daily view
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/pages/ProjectDetails.tsx | Adds viewType="project" prop to BalanceChart to display transactions from project perspective |
| src/components/ranking/BalanceChart.tsx | Major refactoring: adds viewType support, implements transaction delta calculation logic, creates separate formatters for daily/hourly data with adaptive switching, and updates chart rendering with dynamic X-axis configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return data; | ||
|
|
||
| // Ticksの最後（最新の正時）まで伸ばす | ||
| const lastTime = ticks[ticks.length - 1]!; |
There was a problem hiding this comment.
The non-null assertion on ticks[ticks.length - 1] assumes ticks is non-empty. Since ticks is constructed in this function, it should always have elements, but it's better to ensure consistency with the check suggested for ticks[0] at line 112.
| domain: | ||
| ticks && ticks.length > 0 | ||
| ? [ticks[0]!, ticks[ticks.length - 1]!] | ||
| : ["auto", "auto"], // Ticksの範囲に合わせる |
There was a problem hiding this comment.
The comment "Ticksの範囲に合わせる" (match to Ticks range) is misleading here since this is the fallback case when ticks is empty or undefined. The comment should clarify that this is the fallback behavior when ticks are not available, such as "Ticksがない場合は自動範囲" (auto range when ticks are not available).
| : ["auto", "auto"], // Ticksの範囲に合わせる | |
| : ["auto", "auto"], // Ticksがない場合は自動範囲 |
| // YYYY-MM-DD -> MM/DD に短縮して表示 | ||
| try { | ||
| const d = new Date(value); | ||
| return `${d.getMonth() + 1}/${d.getDate()}`; | ||
| } catch { | ||
| return value; | ||
| } |
There was a problem hiding this comment.
The Date constructor parsing "YYYY-MM-DD" format interprets it as UTC midnight. When calling getMonth() and getDate() on this Date object, it returns the local time representation, which can cause off-by-one day errors for users in timezones with large UTC offsets. Consider parsing the date string manually or using a date library that handles timezone-aware date parsing, or alternatively parse the string components directly (split by '-') to avoid timezone issues.
| // YYYY-MM-DD -> MM/DD に短縮して表示 | |
| try { | |
| const d = new Date(value); | |
| return `${d.getMonth() + 1}/${d.getDate()}`; | |
| } catch { | |
| return value; | |
| } | |
| // YYYY-MM-DD -> MM/DD に短縮して表示（タイムゾーンに依存しないよう手動でパース） | |
| const match = /^(\d{4})-(\d{2})-(\d{2})$/.exec(value); | |
| if (!match) { | |
| return value; | |
| } | |
| const month = Number(match[2]); | |
| const day = Number(match[3]); | |
| return `${month}/${day}`; |
| const nextHour = new Date(t); | ||
| ticks.push(nextHour.getTime()); |
There was a problem hiding this comment.
The variable nextHour is redundant since t has already been incremented in the while loop (line 102). This creates an unnecessary copy. Consider using t.getTime() directly or removing the intermediate variable.
| const nextHour = new Date(t); | |
| ticks.push(nextHour.getTime()); | |
| ticks.push(t.getTime()); |
| let running = start; | ||
|
|
||
| // 開始点（Ticksの最初） | ||
| const startTimeIndex = ticks[0]!; |
There was a problem hiding this comment.
The non-null assertion on ticks[0] assumes ticks is non-empty, but if there's an edge case where no transactions exist or all transactions have the same timestamp, the ticks array might not have the expected structure. Consider adding a guard check or verifying that ticks has at least one element before accessing it.
Closes #217