Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 4
🧹 Outside diff range and nitpick comments (3)
apps/demo-game/src/pages/admin/reports/[id].tsx (3)
127-157: Refactor to eliminate duplication in benchmark configurationsThe
absolutePerformanceConfigandaccReturnConfigobjects contain duplicated benchmark configurations. Extracting the common benchmark settings into a separate object will enhance maintainability and reduce potential inconsistencies.Apply this diff to refactor the benchmark configurations:
+const benchmarkConfig = { + bankBenchmark: { + label: 'Savings B', + color: colors[4], + }, + bondsBenchmark: { + label: 'Bonds B', + color: colors[5], + }, + stocksBenchmark: { + label: 'Stocks B', + color: colors[6], + }, + accBankBenchmarkReturn: { + label: 'Savings B', + color: colors[4], + }, + accBondsBenchmarkReturn: { + label: 'Bonds B', + color: colors[5], + }, + accStocksBenchmarkReturn: { + label: 'Stocks B', + color: colors[6], + }, +}; const absolutePerformanceConfig = { ...playerConfig, - bankBenchmark: { - label: 'Savings B', - color: colors[4], - }, - bondsBenchmark: { - label: 'Bonds B', - color: colors[5], - }, - stocksBenchmark: { - label: 'Stocks B', - color: colors[6], - }, + bankBenchmark: benchmarkConfig.bankBenchmark, + bondsBenchmark: benchmarkConfig.bondsBenchmark, + stocksBenchmark: benchmarkConfig.stocksBenchmark, }; const accReturnConfig = { ...playerConfig, - accBankBenchmarkReturn: { - label: 'Savings B', - color: colors[4], - }, - accBondsBenchmarkReturn: { - label: 'Bonds B', - color: colors[5], - }, - accStocksBenchmarkReturn: { - label: 'Stocks B', - color: colors[6], - }, + accBankBenchmarkReturn: benchmarkConfig.accBankBenchmarkReturn, + accBondsBenchmarkReturn: benchmarkConfig.accBondsBenchmarkReturn, + accStocksBenchmarkReturn: benchmarkConfig.accStocksBenchmarkReturn, };
221-228: Use.forEach()instead of.map()for side effectsThe
.map()method is used here without utilizing its returned array, which is intended for transformation. Since you're performing side effects, consider using.forEach()for better readability and adherence to JavaScript conventions.Apply this diff to replace
.map()with.forEach():-playerResPerPeriod.map((result) => { +playerResPerPeriod.forEach((result) => { const assetsWithReturns = result.facts.assetsWithReturns.filter( (_, ix) => ix > 0 ) - Object.keys(dataBenchmark).map((key) => { + Object.keys(dataBenchmark).forEach((key) => { dataBenchmark[key].push(...assetsWithReturns.map((a) => a[key])) }) })
659-680: Extract tooltip formatter into a reusable function to reduce duplicationThe tooltip formatter function is similar across multiple chart components. To enhance code maintainability and readability, consider extracting this formatter into a reusable function or component.
| const period = ~~(segmentIx / game.activePeriodIx) + 1 | ||
| const segment = (segmentIx % game.activePeriodIx) + 1 |
There was a problem hiding this comment.
Prevent division by zero when calculating period and segment
The calculation uses game.activePeriodIx as a divisor. If game.activePeriodIx is zero, this will cause a division by zero error. Ensure that game.activePeriodIx is not zero before performing the calculation.
Apply this diff to add a safety check:
const dataAvg = segmentResultPerSegmentAvg.map((values, segmentIx) => {
+ if (game.activePeriodIx === 0) {
+ console.error('game.activePeriodIx is zero, cannot divide by zero.')
+ return null;
+ }
const period = Math.floor(segmentIx / game.activePeriodIx) + 1
const segment = (segmentIx % game.activePeriodIx) + 1Committable suggestion skipped: line range outside the PR's diff.
| // TODO(JJ): Maybe add a random color generator, because we have unkown many teams | ||
| const colors = [ | ||
| 'hsl(var(--chart-1))', | ||
| 'hsl(var(--chart-2))', | ||
| 'hsl(var(--chart-3))', | ||
| 'hsl(var(--chart-4))', | ||
| 'hsl(var(--chart-5))', | ||
| '#2b463c', | ||
| '#688f4f', | ||
| '#b1d182', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement dynamic color generation for unknown team counts
The current implementation uses a static array of colors, which may not suffice when dealing with an unknown number of teams. Consider implementing a dynamic color generator to ensure that each team is assigned a unique color, regardless of the number of teams.
Apply this diff to implement dynamic color generation:
-// TODO(JJ): Maybe add a random color generator, because we have unkown many teams
const colors = [
'hsl(var(--chart-1))',
'hsl(var(--chart-2))',
'hsl(var(--chart-3))',
'hsl(var(--chart-4))',
'hsl(var(--chart-5))',
'#2b463c',
'#688f4f',
'#b1d182',
]
+// Generate colors dynamically for unknown number of teams
+const generateColors = (numColors) => {
+ return Array.from({ length: numColors }, (_, i) => `hsl(${(i * (360 / numColors))}, 70%, 50%)`);
+};
+const colors = generateColors(game.players.length);Committable suggestion skipped: line range outside the PR's diff.
| const benchmarksFlat = Object.keys(benchmarksPerPeriod[0]).reduce( | ||
| (acc, key) => { | ||
| acc[key] = benchmarksPerPeriod.flatMap((p) => p[key]) | ||
| return acc | ||
| }, | ||
| {} | ||
| ) |
There was a problem hiding this comment.
Ensure benchmarksPerPeriod is not empty before accessing index 0
The code assumes that benchmarksPerPeriod[0] exists. If benchmarksPerPeriod is empty, this will result in an error. Add a check to confirm that benchmarksPerPeriod contains data before proceeding.
Apply this diff to add a safety check:
const benchmarksPerPeriod = getBenchmarksPerPeriod()
+if (benchmarksPerPeriod.length === 0) {
+ console.error('Benchmarks per period is empty.')
+ return null;
+}
const benchmarksFlat = Object.keys(benchmarksPerPeriod[0]).reduce(
(acc, key) => {
acc[key] = benchmarksPerPeriod.flatMap((p) => p[key])
return acc
},
{}
)Committable suggestion skipped: line range outside the PR's diff.
rschlaefli
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
apps/demo-game/src/pages/admin/reports/[id].tsx (2)
144-174: Consider using a more maintainable approach for benchmark colorsThe configuration objects are well-structured, but they rely on hardcoded color indices (colors[4], colors[5], colors[6]). This makes the code fragile to changes in the colors array.
Consider creating named constants for benchmark colors or using a color generation function:
+const BENCHMARK_COLORS = { + SAVINGS: 'hsl(var(--chart-5))', + BONDS: '#2b463c', + STOCKS: '#688f4f', +} const absolutePerformanceConfig = { ...playerConfig, bankBenchmark: { label: 'Savings B', - color: colors[4], + color: BENCHMARK_COLORS.SAVINGS, }, // ... similar changes for other benchmarks }
271-288: Improve readability of benchmark data integrationThe nested reduce operations make the code harder to understand and maintain.
Consider extracting the benchmark key filtering and mapping logic into separate functions:
+const BENCHMARK_KEYS = ['bankBenchmark', 'bondsBenchmark', 'stocksBenchmark']; + +const getBenchmarkData = (benchmarksFlat, index) => + BENCHMARK_KEYS.reduce((acc, key) => { + acc[key] = benchmarksFlat[key][index]; + return acc; + }, {}); const dataTotalAssets = composeChartData(dataPerPeriod, 'totalAssets').map( (d, ix) => ({ ...d, - ...Object.keys(benchmarksFlat) - .filter(key => - key === 'bankBenchmark' || - key === 'bondsBenchmark' || - key === 'stocksBenchmark' - ) - .reduce((acc, key) => { - acc[key] = benchmarksFlat[key][ix] - return acc - }, {}), + ...getBenchmarkData(benchmarksFlat, ix) }) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/demo-game/src/pages/admin/reports/[id].tsx(14 hunks)
🔇 Additional comments (1)
apps/demo-game/src/pages/admin/reports/[id].tsx (1)
Line range hint 469-776: LGTM! Well-implemented chart updates
The chart updates maintain consistency across different visualizations and provide clear formatting for percentage values. The tooltip implementations are particularly well done.
| const getBenchmarksPerPeriod = () => { | ||
| if (!previousSegmentResults || previousSegmentResults.length === 0) | ||
| return [] | ||
| let output = [] | ||
| // The benchmarks are saved for all results, we only need it once for each segment | ||
| const player = game.players[0] | ||
| for (let i = 0; i < numPeriods; i++) { | ||
| const playerResPerPeriod = previousSegmentResults.filter( | ||
| (result) => | ||
| result.period.index === i && result.player.id === player.id | ||
| ) | ||
| const dataBenchmark = { | ||
| bankBenchmark: [], | ||
| bondsBenchmark: [], | ||
| stocksBenchmark: [], | ||
| accBankBenchmarkReturn: [], | ||
| accBondsBenchmarkReturn: [], | ||
| accStocksBenchmarkReturn: [], | ||
| } | ||
|
|
||
| playerResPerPeriod.map((result) => { | ||
| const assetsWithReturns = result.facts.assetsWithReturns.filter( | ||
| (_, ix) => ix > 0 | ||
| ) | ||
| Object.keys(dataBenchmark).map((key) => { | ||
| dataBenchmark[key].push(...assetsWithReturns.map((a) => a[key])) | ||
| }) | ||
| }) | ||
| output.push(dataBenchmark) | ||
| } | ||
| return output | ||
| } |
There was a problem hiding this comment.
Add null check for assetsWithReturns
The function assumes result.facts.assetsWithReturns always exists, which could lead to runtime errors.
Add a null check before accessing the property:
playerResPerPeriod.map((result) => {
+ if (!result.facts.assetsWithReturns) {
+ console.warn('Missing assetsWithReturns for result:', result);
+ return;
+ }
const assetsWithReturns = result.facts.assetsWithReturns.filter(
(_, ix) => ix > 0
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getBenchmarksPerPeriod = () => { | |
| if (!previousSegmentResults || previousSegmentResults.length === 0) | |
| return [] | |
| let output = [] | |
| // The benchmarks are saved for all results, we only need it once for each segment | |
| const player = game.players[0] | |
| for (let i = 0; i < numPeriods; i++) { | |
| const playerResPerPeriod = previousSegmentResults.filter( | |
| (result) => | |
| result.period.index === i && result.player.id === player.id | |
| ) | |
| const dataBenchmark = { | |
| bankBenchmark: [], | |
| bondsBenchmark: [], | |
| stocksBenchmark: [], | |
| accBankBenchmarkReturn: [], | |
| accBondsBenchmarkReturn: [], | |
| accStocksBenchmarkReturn: [], | |
| } | |
| playerResPerPeriod.map((result) => { | |
| const assetsWithReturns = result.facts.assetsWithReturns.filter( | |
| (_, ix) => ix > 0 | |
| ) | |
| Object.keys(dataBenchmark).map((key) => { | |
| dataBenchmark[key].push(...assetsWithReturns.map((a) => a[key])) | |
| }) | |
| }) | |
| output.push(dataBenchmark) | |
| } | |
| return output | |
| } | |
| const getBenchmarksPerPeriod = () => { | |
| if (!previousSegmentResults || previousSegmentResults.length === 0) | |
| return [] | |
| let output = [] | |
| // The benchmarks are saved for all results, we only need it once for each segment | |
| const player = game.players[0] | |
| for (let i = 0; i < numPeriods; i++) { | |
| const playerResPerPeriod = previousSegmentResults.filter( | |
| (result) => | |
| result.period.index === i && result.player.id === player.id | |
| ) | |
| const dataBenchmark = { | |
| bankBenchmark: [], | |
| bondsBenchmark: [], | |
| stocksBenchmark: [], | |
| accBankBenchmarkReturn: [], | |
| accBondsBenchmarkReturn: [], | |
| accStocksBenchmarkReturn: [], | |
| } | |
| playerResPerPeriod.map((result) => { | |
| if (!result.facts.assetsWithReturns) { | |
| console.warn('Missing assetsWithReturns for result:', result); | |
| return; | |
| } | |
| const assetsWithReturns = result.facts.assetsWithReturns.filter( | |
| (_, ix) => ix > 0 | |
| ) | |
| Object.keys(dataBenchmark).map((key) => { | |
| dataBenchmark[key].push(...assetsWithReturns.map((a) => a[key])) | |
| }) | |
| }) | |
| output.push(dataBenchmark) | |
| } | |
| return output | |
| } |
| const segmentResultPerSegmentAvg = segmentResultsPerPlayer[0].map( | ||
| (_, i) => { | ||
| const numSegments = segmentResultsPerPlayer[0][i].length | ||
| const sumPerSegment = segmentResultsPerPlayer | ||
| .reduce( | ||
| (sum, player) => sum.map((val, idx) => val + player[i][idx]), | ||
| Array(numSegments).fill(0) | ||
| ) | ||
| .map((sum) => sum / segmentResultsPerPlayer.length) | ||
|
|
||
| const computeTotalDecisionAvg = () => { | ||
| const result = Array(segmentResultPerPlayerAvg[0].length).fill(0) | ||
| segmentResultPerPlayerAvg.forEach((val) => { | ||
| val.forEach((v, i) => { | ||
| result[i] += v / segmentResultPerPlayerAvg.length | ||
| }) | ||
| }) | ||
| return result | ||
| } | ||
| const totalDecisionAvg = computeTotalDecisionAvg() | ||
| return sumPerSegment | ||
| } | ||
| ) |
There was a problem hiding this comment.
Add safety checks and simplify array operations
The average calculation could fail if there are no players, and the array operations could be simplified.
Consider adding safety checks and simplifying the code:
const segmentResultPerSegmentAvg = segmentResultsPerPlayer[0].map(
(_, i) => {
+ if (segmentResultsPerPlayer.length === 0) {
+ console.warn('No players available for average calculation');
+ return Array(numSegments).fill(0);
+ }
const numSegments = segmentResultsPerPlayer[0][i].length
- const sumPerSegment = segmentResultsPerPlayer
- .reduce(
- (sum, player) => sum.map((val, idx) => val + player[i][idx]),
- Array(numSegments).fill(0)
- )
- .map((sum) => sum / segmentResultsPerPlayer.length)
+ const sumPerSegment = Array(numSegments).fill(0).map((_, idx) =>
+ segmentResultsPerPlayer.reduce((sum, player) =>
+ sum + player[i][idx], 0) / segmentResultsPerPlayer.length
+ );
return sumPerSegment
}
)Committable suggestion skipped: line range outside the PR's diff.
rschlaefli
left a comment
rschlaefli
left a comment
There was a problem hiding this comment.
Some small things that we should still fix on this branch before merging:
- Average decision is indexed with P1 S0, P2 S1, ..., and not with correct segments
- Tooltips for benchmarks should have nice naming
- The admin reports seem to show results already for the running segment, which is a bit strange, as this should not have been computed yet (it is also the case on the dev branch, but we could fix it on this one)
- Some other small things
We can also discuss this in January before merging this PR :)
jajakob
force-pushed
the
jakob/report-improvements
branch
from
4c95401 to
7d6e886
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/demo-game/src/pages/admin/reports/[id].tsx (3)
144-174: Add TypeScript interfaces for configuration objectsConsider adding TypeScript interfaces to define the structure of these configuration objects. This would improve type safety and make the code more maintainable.
interface ChartConfig { label: string; color: string; } interface BenchmarkConfig extends Record<string, ChartConfig> { bankBenchmark: ChartConfig; bondsBenchmark: ChartConfig; stocksBenchmark: ChartConfig; }
271-288: Optimize data transformation for better readability and performanceThe nested map/reduce operations make the code hard to follow. Consider extracting the benchmark filtering logic into a separate function.
+const getBenchmarkKeys = (type: 'absolute' | 'return') => { + const keys = { + absolute: ['bankBenchmark', 'bondsBenchmark', 'stocksBenchmark'], + return: ['accBankBenchmarkReturn', 'accBondsBenchmarkReturn', 'accStocksBenchmarkReturn'] + } + return keys[type] +} const dataTotalAssets = composeChartData(dataPerPeriod, 'totalAssets').map( (d, ix) => ({ ...d, - ...Object.keys(benchmarksFlat) - .filter(key => - key === 'bankBenchmark' || - key === 'bondsBenchmark' || - key === 'stocksBenchmark' - ) + ...getBenchmarkKeys('absolute') .reduce((acc, key) => { acc[key] = benchmarksFlat[key][ix] return acc }, {}) }) )
756-777: Extract common tooltip configurationThe tooltip configuration is duplicated across multiple chart components. Consider extracting it into a reusable component or configuration object.
const createTooltipFormatter = (valueFormatter = (v: number) => v.toFixed(2)) => (value: number, name: string, item: any) => [ <div key={name} className="flex w-full items-center justify-between gap-x-2"> <div className="flex items-center gap-x-1"> <div className="h-[8px] w-[8px] rounded-sm" style={{ background: item.color }} /> <span className="text-xs text-gray-600"> {name === 'bank' ? 'savings' : name} </span> </div> <span className="font-bold text-black"> {valueFormatter(value)} </span> </div> ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/demo-game/src/pages/admin/reports/[id].tsx(12 hunks)
🔇 Additional comments (3)
apps/demo-game/src/pages/admin/reports/[id].tsx (3)
54-63: Consider implementing dynamic color generation
The current implementation uses a static array of colors, which may not be sufficient for an unknown number of teams.
448-459: Replace bitwise operation with explicit Math.floor
The use of ~~ for floor division is clever but reduces code readability. Consider using Math.floor instead.
- const period = ~~(segmentIx / game.activePeriodIx) + 1
+ const period = Math.floor(segmentIx / game.activePeriodIx) + 1262-268:
Add safety check for benchmarksPerPeriod array access
The code assumes benchmarksPerPeriod[0] exists. Add a check to prevent potential runtime errors.
const benchmarksPerPeriod = getBenchmarksPerPeriod()
+if (!benchmarksPerPeriod.length) {
+ console.warn('No benchmark data available')
+ return { game, playerConfig }
+}
const benchmarksFlat = Object.keys(benchmarksPerPeriod[0]).reduce(Likely invalid or redundant comment.
2 participants
Summary by CodeRabbit
New Features
Improvements