-
Notifications
You must be signed in to change notification settings - Fork 6
feat: Added investment performance chart with benchmark comparison #2
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?
feat: Added investment performance chart with benchmark comparison #2
Conversation
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.
6 issues found across 4 files
Prompt for AI agents (all 6 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/routes/investments.ts">
<violation number="1" location="src/routes/investments.ts:1689">
P1: Date calculation bug: `startDate.setMonth(endDate.getMonth() - 1)` incorrectly modifies the month of `investment.invested_at` rather than calculating 1 month before `endDate`. This results in wrong date ranges. Initialize `startDate` from `endDate` instead, then subtract the period.</violation>
<violation number="2" location="src/routes/investments.ts:1750">
P1: Remove debug `console.log` statements before merging to production. These will clutter logs and may expose sensitive financial data like investment values and benchmark prices.</violation>
<violation number="3" location="src/routes/investments.ts:1772">
P2: Magic number `21000` as benchmark fallback is problematic. Different benchmarks (NIFTY BANK, NIFTY IT, NIFTY PHARMA) have vastly different price levels. Consider returning an error or omitting benchmark data when no benchmark records exist.</violation>
</file>
<file name="scripts/seed-final.sql">
<violation number="1" location="scripts/seed-final.sql:10">
P2: Use a clearly fake test email address instead of what appears to be a real personal email. Consider using a placeholder like `test@example.com` or `dev@localhost` to avoid privacy concerns and accidental email delivery.</violation>
<violation number="2" location="scripts/seed-final.sql:74">
P1: This `DELETE FROM investment_history;` unconditionally removes ALL records, which could be catastrophic if accidentally run against production. Consider either: (1) deleting only seeded test data by filtering on specific investment_ids, or (2) using `INSERT OR REPLACE` instead of DELETE+INSERT to maintain consistency with the `INSERT OR IGNORE` pattern used elsewhere.</violation>
</file>
<file name="public/static/app.js">
<violation number="1" location="public/static/app.js:2987">
P2: Loading state overlay is positioned outside the relative container. The `absolute inset-0` on `chartLoading` won't overlay the chart correctly because it's a sibling of the relative div, not a child. Move it inside the relative container.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
src/routes/investments.ts
Outdated
| ORDER BY recorded_date ASC | ||
| `).bind(benchmarkSymbol, startDateStr, endDateStr).all<BenchmarkData>(); | ||
|
|
||
| console.log('[DEBUG] Benchmark history count:', benchmarkHistory.results?.length); |
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.
P1: Remove debug console.log statements before merging to production. These will clutter logs and may expose sensitive financial data like investment values and benchmark prices.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/investments.ts, line 1750:
<comment>Remove debug `console.log` statements before merging to production. These will clutter logs and may expose sensitive financial data like investment values and benchmark prices.</comment>
<file context>
@@ -1635,4 +1636,185 @@ investments.get('/:id/rebalance-history', async (c) => {
+ ORDER BY recorded_date ASC
+`).bind(benchmarkSymbol, startDateStr, endDateStr).all<BenchmarkData>();
+
+console.log('[DEBUG] Benchmark history count:', benchmarkHistory.results?.length);
+console.log('[DEBUG] First benchmark record:', benchmarkHistory.results?.[0]);
+console.log('[DEBUG] Last benchmark record:', benchmarkHistory.results?.[benchmarkHistory.results?.length - 1]);
</file context>
✅ Addressed in afd3754
src/routes/investments.ts
Outdated
|
|
||
| switch (period) { | ||
| case '1M': | ||
| startDate.setMonth(endDate.getMonth() - 1); |
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.
P1: Date calculation bug: startDate.setMonth(endDate.getMonth() - 1) incorrectly modifies the month of investment.invested_at rather than calculating 1 month before endDate. This results in wrong date ranges. Initialize startDate from endDate instead, then subtract the period.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/investments.ts, line 1689:
<comment>Date calculation bug: `startDate.setMonth(endDate.getMonth() - 1)` incorrectly modifies the month of `investment.invested_at` rather than calculating 1 month before `endDate`. This results in wrong date ranges. Initialize `startDate` from `endDate` instead, then subtract the period.</comment>
<file context>
@@ -1635,4 +1636,185 @@ investments.get('/:id/rebalance-history', async (c) => {
+
+ switch (period) {
+ case '1M':
+ startDate.setMonth(endDate.getMonth() - 1);
+ break;
+ case '3M':
</file context>
✅ Addressed in afd3754
src/routes/investments.ts
Outdated
| const investmentValues: number[] = []; | ||
| const benchmarkValues: number[] = []; | ||
| let lastInvestmentValue = investmentHistory.results[0].current_value; | ||
| let lastBenchmarkValue = benchmarkHistory.results?.[0]?.close_price || 21000; // Use realistic default |
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.
P2: Magic number 21000 as benchmark fallback is problematic. Different benchmarks (NIFTY BANK, NIFTY IT, NIFTY PHARMA) have vastly different price levels. Consider returning an error or omitting benchmark data when no benchmark records exist.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/investments.ts, line 1772:
<comment>Magic number `21000` as benchmark fallback is problematic. Different benchmarks (NIFTY BANK, NIFTY IT, NIFTY PHARMA) have vastly different price levels. Consider returning an error or omitting benchmark data when no benchmark records exist.</comment>
<file context>
@@ -1635,4 +1636,185 @@ investments.get('/:id/rebalance-history', async (c) => {
+const investmentValues: number[] = [];
+const benchmarkValues: number[] = [];
+let lastInvestmentValue = investmentHistory.results[0].current_value;
+let lastBenchmarkValue = benchmarkHistory.results?.[0]?.close_price || 21000; // Use realistic default
+
+dates.forEach((date, index) => {
</file context>
✅ Addressed in afd3754
scripts/seed-final.sql
Outdated
| id, zerodha_user_id, name, email, broker_type, | ||
| is_primary, is_active, created_at, updated_at | ||
| ) VALUES ( | ||
| 1, 'AB1234', 'Prashant Trading', 'focusedprshnt@gmail.com', 'zerodha', |
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.
P2: Use a clearly fake test email address instead of what appears to be a real personal email. Consider using a placeholder like test@example.com or dev@localhost to avoid privacy concerns and accidental email delivery.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/seed-final.sql, line 10:
<comment>Use a clearly fake test email address instead of what appears to be a real personal email. Consider using a placeholder like `test@example.com` or `dev@localhost` to avoid privacy concerns and accidental email delivery.</comment>
<file context>
@@ -0,0 +1,106 @@
+ id, zerodha_user_id, name, email, broker_type,
+ is_primary, is_active, created_at, updated_at
+) VALUES (
+ 1, 'AB1234', 'Prashant Trading', 'focusedprshnt@gmail.com', 'zerodha',
+ 1, 1, datetime('now'), datetime('now')
+);
</file context>
✅ Addressed in afd3754
scripts/seed-final.sql
Outdated
| (3, 'DIVISLAB', 'NSE', 2, 4000.00, 4200.00, 20.0, 20.0, 400, 5.0, datetime('now')); | ||
|
|
||
| -- Step 7: Populate investment_history | ||
| DELETE FROM investment_history; |
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.
P1: This DELETE FROM investment_history; unconditionally removes ALL records, which could be catastrophic if accidentally run against production. Consider either: (1) deleting only seeded test data by filtering on specific investment_ids, or (2) using INSERT OR REPLACE instead of DELETE+INSERT to maintain consistency with the INSERT OR IGNORE pattern used elsewhere.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/seed-final.sql, line 74:
<comment>This `DELETE FROM investment_history;` unconditionally removes ALL records, which could be catastrophic if accidentally run against production. Consider either: (1) deleting only seeded test data by filtering on specific investment_ids, or (2) using `INSERT OR REPLACE` instead of DELETE+INSERT to maintain consistency with the `INSERT OR IGNORE` pattern used elsewhere.</comment>
<file context>
@@ -0,0 +1,106 @@
+(3, 'DIVISLAB', 'NSE', 2, 4000.00, 4200.00, 20.0, 20.0, 400, 5.0, datetime('now'));
+
+-- Step 7: Populate investment_history
+DELETE FROM investment_history;
+INSERT INTO investment_history (investment_id, recorded_date, invested_amount, current_value, day_change, day_change_percentage, total_pnl, total_pnl_percentage, created_at)
+SELECT i.id, date(i.invested_at, '+' || n.day || ' days'), i.invested_amount,
</file context>
✅ Addressed in afd3754
public/static/app.js
Outdated
| </div> | ||
| <!-- Loading State --> | ||
| <div id="chartLoading" class="hidden absolute inset-0 flex items-center justify-center bg-white bg-opacity-75"> |
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.
P2: Loading state overlay is positioned outside the relative container. The absolute inset-0 on chartLoading won't overlay the chart correctly because it's a sibling of the relative div, not a child. Move it inside the relative container.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At public/static/app.js, line 2987:
<comment>Loading state overlay is positioned outside the relative container. The `absolute inset-0` on `chartLoading` won't overlay the chart correctly because it's a sibling of the relative div, not a child. Move it inside the relative container.</comment>
<file context>
@@ -2787,122 +2818,207 @@ async function refreshBasketPrices(basketId) {
+ </div>
+
+ <!-- Loading State -->
+ <div id="chartLoading" class="hidden absolute inset-0 flex items-center justify-center bg-white bg-opacity-75">
+ <i class="fas fa-spinner fa-spin text-4xl text-indigo-600"></i>
+ </div>
</file context>
✅ Addressed in afd3754
Overview
This PR adds a historical performance chart to the Investment Detail page. Users can now see how their basket investment has performed over time and compare it against a relevant benchmark index (e.g., NIFTY IT, NIFTY BANK, NIFTY PHARMA). The implementation follows the existing SPA architecture, design system, and backend patterns used in OpenCase.
Problem
Previously, the Investment Detail view only showed point-in-time metrics:
Invested amount
Current value
Total P&L
There was no way to:
Visualize performance over time
Compare portfolio returns to a benchmark index
Analyze performance across different time ranges (1M, 3M, 6M, 1Y, ALL)
This limited a user’s ability to evaluate strategy quality and understand risk-adjusted performance.
Solution
Endpoint
GET /api/investments/:id/performance
Query parameters
period: 1M | 3M | 6M | 1Y | ALL (default: 1Y)
benchmark (optional): override benchmark symbol
Responsibilities
Validate that the investment belongs to the authenticated user.
Compute the date range based on period, clamped to invested_at.
Read daily snapshots from investment_history for the given investment_id.
Read benchmark prices from benchmark_data using:
baskets.benchmark_symbol for the investment’s basket, or
the optional benchmark query param.
Build a shared date axis and forward‑fill missing days for both series.
Normalize both portfolio and benchmark series to base 100 at the start date.
Return a compact time-series payload:
Uses investment_history and benchmark_data tables (no schema changes).
All queries are parameterized.
Handles “no data” cases explicitly with a NO_DATA error.
Normalization logic is in a small utility function for reuse and clarity.
Location
Investment Detail view (state.currentView === 'investment-detail').
UI/UX
New “Performance” card under the existing summary metrics.
Time range selector with buttons:
1M, 3M, 6M, 1Y, ALL
Chart.js line chart:
Portfolio line: solid indigo with a light fill.
Benchmark line: dashed gray line.
Y‑axis label: “Normalized to 100”.
Behavior
When an investment is opened, the chart auto‑loads with period=1Y.
Clicking a period button:
Updates the active button styling (indigo background, white text).
Refetches /api/investments/:id/performance?period=....
Destroys and recreates the Chart.js instance with new data.
Semi‑transparent overlay with spinner inside the chart card.
Centered message in the card if performance data is not available.
For easier local development and visual verification, a seed script populates:
investments with three sample investments (Tech Leaders, Banking Giants, Pharma Champions).
investment_history with 4–8 months of daily data per investment.
benchmark_data with ~1 year of daily prices for:
NIFTY 50
NIFTY IT
NIFTY BANK
NIFTY PHARMA
This script is intended for local/dev environments only and does not alter the schema.
Files Touched (high level)
Backend
src/routes/investments.ts
Add GET /api/investments/:id/performance route.
Query investment_history and benchmark_data.
Normalize and return time‑series response.
(If present) src/lib/utils.ts
Utility to normalize numeric series to base 100.
Utility to format dates (e.g., YYYY-MM-DD).
Frontend
public/static/app.js
Enhance renderInvestmentDetail() to include:
Performance card
Period selector buttons
Chart container
New helpers:
loadPerformanceChart(investmentId, period)
renderPerformanceChart(data) – Chart.js configuration
renderInvestmentHoldings(investment) if not already present / used here.
Impact and Risk
Schema changes: None.
Existing endpoints: Unchanged.
UI: Only the Investment Detail view is extended; other views are unaffected.
Risk level: Low
New endpoint is additive.
Frontend change is isolated to one view.
Seed data only affects local/dev environments.
Rollback is straightforward: remove the performance endpoint and the performance card/chart block from the Investment Detail view.
Summary by cubic
Adds a historical performance chart to the Investment Detail page with a benchmark comparison so users can see returns over time and compare against an index. Includes a new backend endpoint and a Chart.js UI with period filters.
New Features
Migration
Written for commit afd3754. Summary will update automatically on new commits.