feat: add expectations to student decision page #69
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several modifications across multiple components within the demo game application. Key changes include the simplification of the Changes
Possibly related PRs
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: 5
🧹 Outside diff range and nitpick comments (5)
apps/demo-game/src/components/DecisionsDisplay.tsx (1)
83-84: Consider adding responsive height adjustmentsThe fixed height of the scroll area might not be optimal for all screen sizes and content lengths.
Consider using dynamic height calculation or responsive classes:
-<ScrollArea className="h-96 rounded-md border p-4"> +<ScrollArea className="h-[calc(100vh-20rem)] min-h-96 max-h-[32rem] rounded-md border p-4">packages/ui/src/components/ProbabilityChart.tsx (1)
Line range hint
15-29: Consider architectural improvementsSeveral potential improvements could enhance maintainability and reusability:
- Move the probability distribution to a separate configuration file
- Add proper TypeScript types for the distribution data structure
- Consider implementing error boundaries for numerical calculations
- Extract the data transformation logic into a custom hook
Would you like me to help implement any of these improvements? I can provide specific code examples or create issues to track these tasks.
Also applies to: 107-116
apps/demo-game/src/pages/play/cockpit.tsx (3)
800-830: Improve decision switches implementationThe decision switches implementation has several areas for improvement:
- The percentage calculation is duplicated across all switches
- The switch id is not unique
- The refetchQueries array could be defined as a constant
+ const calculatePercentage = (decisions: Record<string, number>, state: boolean) => { + const totalDecisions = +decisions.bank + +decisions.bonds + +decisions.stocks; + return state && totalDecisions > 0 ? 1 / totalDecisions : 0; + }; + const REFETCH_QUERIES = [ResultDocument]; <div className="mt-8 flex flex-row gap-2"> {decisions.map((decision) => { return ( <div className="p-1" key={decision.name}> <Switch label={decision.label( - decision.state - ? 1 / - (+resultFactsDecisions.bank + - +resultFactsDecisions.bonds + - +resultFactsDecisions.stocks) - : 0 + calculatePercentage(resultFactsDecisions, decision.state) )} checked={decision.state} - id="switch" + id={`switch-${decision.name.toLowerCase()}`} onCheckedChange={async (checked) => { await performAction({ variables: { type: decision.action, payload: JSON.stringify({ decision: checked, }), }, - refetchQueries: [ResultDocument], + refetchQueries: REFETCH_QUERIES, }) }} /> </div> ) })} </div>
240-243: Simplify period state initializationThe condition for setting the period state can be simplified, and the effect dependency can be more specific.
- if (data?.result?.currentGame?.periods?.length > 0) { + const periodsLength = data?.result?.currentGame?.periods?.length; + if (periodsLength) { - setPeriod(data.result.currentGame.periods.length - 1) + setPeriod(periodsLength - 1) } - }, [data?.result?.currentGame?.periods?.length]) + }, [periodsLength])
832-837: Add null safety to interest rate display and improve text clarityThe interest rate access needs null safety checks, and the text can be made clearer.
<CardFooter className="text-slate-500"> - The assets put in savings yield a continuous return of{' '} - {currentGame.periods[period]?.facts.scenario.interestBank * 100} - % per month. The return of bonds and stocks is determined by the - market expectation and simulated by two dice. + {period !== null && currentGame.periods[period]?.facts.scenario && ( + <> + Savings account interest rate: {(currentGame.periods[period].facts.scenario.interestBank * 100).toFixed(1)}% per month. + Bond and stock returns are based on market expectations and simulated using dice rolls. + </> + )} </CardFooter>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/demo-game/src/components/DecisionsDisplay.tsx(1 hunks)apps/demo-game/src/pages/play/cockpit.tsx(4 hunks)packages/ui/src/components/ProbabilityChart.tsx(2 hunks)
🔇 Additional comments (4)
apps/demo-game/src/components/DecisionsDisplay.tsx (3)
75-82: LGTM! Clear and well-structured component header
The Card structure with header and description provides clear context about the decision history display.
85-89: LGTM! Clear column structure
The DecisionLayout implementation provides clear column headers for time and decision status.
123-125: LGTM! Clean component closure
The component structure is properly closed with clear hierarchy.
packages/ui/src/components/ProbabilityChart.tsx (1)
121-122: LGTM! Consistent number formatting
The use of .toFixed(2) improves readability and maintains consistency with other numerical displays in the component.
| <div className="flex flex-col justify-between"> | ||
| {segmentDecisions.map((e) => { | ||
| return ( | ||
| <div | ||
| className="flex flex-col justify-between py-2" | ||
| key={e.segment.id} | ||
| > | ||
| <div className="flex justify-between"> | ||
| <h4 className="flex gap-2"> | ||
| <div>P{e.period.index + 1}</div> | ||
| <div>S{e.segment.index + 1}</div> | ||
| </h4> | ||
|
|
||
| <div className="flex flex-col justify-between"> | ||
| {Object.keys(e.decisions).map((type) => { | ||
| return ( | ||
| <div | ||
| key={type} | ||
| className="flex min-w-36 items-center justify-between" | ||
| > | ||
| <div>{type}</div> | ||
| <OnOffIcon on={e.decisions[type]} /> | ||
| </div> | ||
| ) | ||
| })} | ||
| </div> | ||
| <div className="flex flex-col justify-between"> | ||
| {Object.keys(e.decisions).map((type) => { | ||
| return ( | ||
| <div | ||
| key={type} | ||
| className="flex min-w-36 items-center justify-between" | ||
| > | ||
| <div>{type}</div> | ||
| <OnOffIcon on={e.decisions[type]} /> | ||
| </div> | ||
| ) | ||
| })} | ||
| </div> | ||
| <Separator className="my-2" /> | ||
| </div> | ||
| ) | ||
| })} | ||
| </div> | ||
| </DecisionLayout> | ||
| </ScrollArea> | ||
| </CardContent> | ||
| </Card> | ||
| </> | ||
| <Separator className="my-2" /> | ||
| </div> | ||
| ) | ||
| })} | ||
| </div> | ||
| </DecisionLayout> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and decision rendering logic
The current implementation has some type safety concerns and potential improvements:
Consider applying these changes:
-{Object.keys(e.decisions).map((type) => {
+{(Object.keys(e.decisions) as Array<keyof typeof e.decisions>).map((type) => {
return (
<div
key={type}
className="flex min-w-36 items-center justify-between"
>
<div>{type}</div>
- <OnOffIcon on={e.decisions[type]} />
+ <OnOffIcon on={e.decisions[type] ?? false} />
</div>
)
})}Also, consider conditionally rendering the Separator to avoid an unnecessary divider after the last item:
-<Separator className="my-2" />
+{index < segmentDecisions.length - 1 && <Separator className="my-2" />}Committable suggestion skipped: line range outside the PR's diff.
| console.log(trendE, trendGap) | ||
|
|
There was a problem hiding this comment.
Remove debug console.log statement
Debug logging statements should not be committed to production code. Consider using proper logging utilities if this information is needed for debugging.
- console.log(trendE, trendGap)
-📝 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.
| console.log(trendE, trendGap) |
| @@ -96,14 +96,16 @@ function ProbabilityChart({ | |||
| }: { | |||
| trendE: number | |||
| trendGap: number | |||
| totalEyes: string | |||
| totalEyes?: string | |||
There was a problem hiding this comment.
Add null check for optional totalEyes prop
Making totalEyes optional could lead to undefined behavior in the Cell comparison. Consider adding proper null handling.
- <Cell
- fill={entry.eyes == totalEyes ? '#dc6027' : 'grey'}
- key={`cell-${index}`}
- ></Cell>
+ <Cell
+ fill={totalEyes && entry.eyes === totalEyes ? '#dc6027' : 'grey'}
+ key={`cell-${index}`}
+ ></Cell>📝 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.
| totalEyes?: string | |
| <Cell | |
| fill={totalEyes && entry.eyes === totalEyes ? '#dc6027' : 'grey'} | |
| key={`cell-${index}`} | |
| ></Cell> |
| <Card> | ||
| <CardHeader> | ||
| <CardTitle>Expectation Bonds</CardTitle> | ||
| <CardDescription> | ||
| The expected value of and possible fluctuations in the bond | ||
| price. | ||
| </CardDescription> | ||
| </CardHeader> | ||
| <CardContent> | ||
| <ProbabilityChart | ||
| trendE={ | ||
| currentGame.periods[period]?.facts.scenario.trendBonds | ||
| } | ||
| trendGap={ | ||
| currentGame.periods[period]?.facts.scenario.gapBonds | ||
| } | ||
| /> | ||
| </CardContent> | ||
| </Card> | ||
|
|
||
| <Card> | ||
| <CardHeader> | ||
| <CardTitle>Expectation Stocks</CardTitle> | ||
| <CardDescription> | ||
| The expected value of and possible fluctuations in the stock | ||
| price. | ||
| </CardDescription> | ||
| </CardHeader> | ||
| <CardContent> | ||
| <ProbabilityChart | ||
| trendE={ | ||
| currentGame.periods[period]?.facts.scenario.trendStocks | ||
| } | ||
| trendGap={ | ||
| currentGame.periods[period]?.facts.scenario.gapStocks | ||
| } | ||
| /> | ||
| </CardContent> | ||
| </Card> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce duplication in expectation cards and add null safety
The bonds and stocks expectation cards are nearly identical. Consider creating a reusable component and add null checks for safer access to period data.
+ interface ExpectationCardProps {
+ title: string;
+ trend: number;
+ gap: number;
+ }
+
+ const ExpectationCard: React.FC<ExpectationCardProps> = ({ title, trend, gap }) => (
+ <Card>
+ <CardHeader>
+ <CardTitle>Expectation {title}</CardTitle>
+ <CardDescription>
+ The expected value of and possible fluctuations in the {title.toLowerCase()} price.
+ </CardDescription>
+ </CardHeader>
+ <CardContent>
+ <ProbabilityChart trendE={trend} trendGap={gap} />
+ </CardContent>
+ </Card>
+ );
- <Card>
- <CardHeader>
- <CardTitle>Expectation Bonds</CardTitle>
- ...
- </CardHeader>
- <CardContent>
- <ProbabilityChart
- trendE={currentGame.periods[period]?.facts.scenario.trendBonds}
- trendGap={currentGame.periods[period]?.facts.scenario.gapBonds}
- />
- </CardContent>
- </Card>
+ {period !== null && currentGame.periods[period]?.facts.scenario && (
+ <>
+ <ExpectationCard
+ title="Bonds"
+ trend={currentGame.periods[period].facts.scenario.trendBonds}
+ gap={currentGame.periods[period].facts.scenario.gapBonds}
+ />
+ <ExpectationCard
+ title="Stocks"
+ trend={currentGame.periods[period].facts.scenario.trendStocks}
+ gap={currentGame.periods[period].facts.scenario.gapStocks}
+ />
+ </>
+ )}Committable suggestion skipped: line range outside the PR's diff.
| { label: 'Category', accessor: 'category', sortable: false }, | ||
| { | ||
| label: 'Value before decisions', | ||
| accessor: 'currentValue', | ||
| sortable: false, | ||
| }, | ||
| { | ||
| label: 'Value after decisions', | ||
| accessor: 'futureValue', | ||
| sortable: false, | ||
| }, | ||
| ] | ||
|
|
||
| const data_portfolio = [ | ||
| { | ||
| cat: 'Savings', | ||
| val: `CHF ${assets.bank.toFixed(2)}`, | ||
| category: 'Savings', | ||
| currentValue: `CHF ${assets.bank.toFixed(2)}`, | ||
| futureValue: `CHF ${( | ||
| assets.totalAssets * | ||
| (resultFactsDecisions.bank | ||
| ? 1 / | ||
| (+resultFactsDecisions.bank + | ||
| +resultFactsDecisions.bonds + | ||
| +resultFactsDecisions.stocks) | ||
| : 0) | ||
| ).toFixed(2)}`, | ||
| }, | ||
| { | ||
| cat: 'Bonds', | ||
| val: `CHF ${assets.bonds.toFixed(2)}`, | ||
| category: 'Bonds', | ||
| currentValue: `CHF ${assets.bonds.toFixed(2)}`, | ||
| futureValue: `CHF ${( | ||
| assets.totalAssets * | ||
| (resultFactsDecisions.bonds | ||
| ? 1 / | ||
| (+resultFactsDecisions.bank + | ||
| +resultFactsDecisions.bonds + | ||
| +resultFactsDecisions.stocks) | ||
| : 0) | ||
| ).toFixed(2)}`, | ||
| }, | ||
| { | ||
| cat: 'Stocks', | ||
| val: `CHF ${assets.stocks.toFixed(2)}`, | ||
| category: 'Stocks', | ||
| currentValue: `CHF ${assets.stocks.toFixed(2)}`, | ||
| futureValue: `CHF ${( | ||
| assets.totalAssets * | ||
| (resultFactsDecisions.stocks | ||
| ? 1 / | ||
| (+resultFactsDecisions.bank + | ||
| +resultFactsDecisions.bonds + | ||
| +resultFactsDecisions.stocks) | ||
| : 0) | ||
| ).toFixed(2)}`, | ||
| }, | ||
| { | ||
| cat: 'Total', | ||
| val: `CHF ${assets.totalAssets.toFixed(2)}`, | ||
| category: 'Total', | ||
| currentValue: `CHF ${assets.totalAssets.toFixed(2)}`, | ||
| futureValue: `CHF ${assets.totalAssets.toFixed(2)}`, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor portfolio calculations for better maintainability and safety
The portfolio value calculations contain duplicated logic and potential division by zero issues. Consider extracting the calculation logic into a helper function.
+ const calculateFutureValue = (totalAssets: number, decisions: Record<string, number>, category: string) => {
+ const totalDecisions = +decisions.bank + +decisions.bonds + +decisions.stocks;
+ if (totalDecisions === 0) return 0;
+ return totalAssets * (decisions[category] ? 1 / totalDecisions : 0);
+ };
const data_portfolio = [
{
category: 'Savings',
currentValue: `CHF ${assets.bank.toFixed(2)}`,
- futureValue: `CHF ${(
- assets.totalAssets *
- (resultFactsDecisions.bank
- ? 1 /
- (+resultFactsDecisions.bank +
- +resultFactsDecisions.bonds +
- +resultFactsDecisions.stocks)
- : 0)
- ).toFixed(2)}`,
+ futureValue: `CHF ${calculateFutureValue(assets.totalAssets, resultFactsDecisions, 'bank').toFixed(2)}`,
},
// Apply similar changes to bonds and stocks calculationsCommittable suggestion skipped: line range outside the PR's diff.
|
Summary by CodeRabbit
Release Notes
New Features
Cockpitcomponent layout with improved clarity for game state and player decisions.ProbabilityChartfor displaying expected values and fluctuations for bonds and stocks.Improvements
DecisionsDisplayCompactfor clearer decision overviews.ProbabilityChart.Bug Fixes
Cockpitcomponent.