ENG-562: Forecast feature - #36
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces a comprehensive forecasting and budget tracking system. It adds database schema for forecasts, streams, items, and assumptions, deploys multiple API routes for CRUD and analytics operations, and implements frontend pages and components for forecast creation, editing, comparison, and tracking with CSV import support. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Page as ForecastEditorPage
participant API as API Routes
participant DB as Database
participant Lib as Business Logic
Client->>Page: Load forecast editor
Page->>API: GET /api/forecasts/[id]
API->>DB: Query forecast + streams + items
DB-->>API: Forecast data
API-->>Page: Return forecast details
Page->>Page: Initialize state, render grid
Client->>Page: Edit cell value
Page->>Page: Track pending changes
Client->>Page: Click Save All
Page->>API: PUT /api/forecasts/[id]/expense-periods/bulk
API->>DB: Upsert period values
DB-->>API: Confirm
API-->>Page: Success
Page->>Page: Clear pending changes
sequenceDiagram
participant Client
participant ImportPage as ForecastImportPage
participant API as API Routes
participant DB as Database
participant Lib as CSV Parser
Client->>ImportPage: Select CSV file
ImportPage->>ImportPage: Preview first 5 rows
Client->>ImportPage: Click Upload
ImportPage->>API: POST /api/forecasts/[id]/import (FormData)
API->>Lib: Parse CSV rows
Lib-->>API: Parsed row objects
API->>DB: Query/create streams & items
DB-->>API: Stream & item IDs
API->>DB: Upsert forecast period values
DB-->>API: Confirm counts
API-->>ImportPage: Success with stats
ImportPage->>ImportPage: Display result summary
sequenceDiagram
participant Client
participant API as API Routes
participant Lib as applyAssumptions
participant DB as Database
Client->>API: POST /api/forecasts/[id]/apply-assumptions
API->>API: Verify forecast ownership
API->>Lib: applyAssumptions(forecastId, companyId)
Lib->>DB: Load items, streams, period values
DB-->>Lib: All forecast data
Lib->>Lib: Compute base year & growth projections
Lib->>Lib: Apply seasonality weights per month
Lib->>DB: Bulk upsert computed period values
DB-->>Lib: Confirm
Lib-->>API: Complete
API-->>Client: Success message
sequenceDiagram
participant Client
participant Page as ForecastTrackingPage
participant API as API Routes
participant DB as Database
participant Lib as Business Logic
Client->>Page: Load tracking dashboard
Page->>API: GET /api/forecasts/[id]
API->>DB: Query forecast + streams + items
DB-->>API: Forecast data
API-->>Page: Forecast details
Page->>API: GET /api/forecasts/[id]/tracking
API->>Lib: getForecastTracking(forecastId, companyId)
Lib->>API: Fetch comparison data
API->>DB: Query period values & actuals
DB-->>API: Budget & actual amounts
API-->>Lib: Comparison rows
Lib->>Lib: Compute status (on_track/at_risk/off_track)
Lib-->>API: Tracking rows with status
API-->>Page: Tracking data
Page->>Page: Render dashboard with status badges
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Check Results✅ Tests PassedTest Output✅ Build PassedBuild Output |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (35)
.github/workflows/release.yml-78-78 (1)
78-78:⚠️ Potential issue | 🟠 MajorGuard
latesttag to stable releases only.Line 78 unconditionally publishes
latest, so prerelease tags (e.g.,v1.2.3-rc.1) will overwrite the stablelatestimage. Thecreate-releasejob correctly marks these as prerelease, but the Dockerlatesttag remains unguarded.Add a condition to prevent publishing
latestfor prerelease tags:Suggested fix
- type=raw,value=latest + type=raw,value=latest,enable=${{ !contains(github.ref_name, '-') }}Alternatively, remove the
type=raw,value=latestline entirely sincetype=semveris already configured and respectsflavor.latest=auto(default), which prevents implicitlatestfor prerelease versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml at line 78, The workflow unconditionally sets the Docker tag via the literal input "type=raw,value=latest", allowing prerelease tags to overwrite stable latest; update the release job to guard publishing "latest" by either removing the "type=raw,value=latest" input entirely (letting existing "type=semver" and its flavor.latest=auto behavior control latest) or add a condition that only emits the raw latest tag when the computed semver is not a prerelease (use the existing create-release logic to detect prerelease and skip adding "type=raw,value=latest"). Ensure you modify the step that currently contains "type=raw,value=latest" so it checks the prerelease flag (from the create-release output or semver result) before publishing the latest tag.docker-entrypoint.sh-12-14 (1)
12-14:⚠️ Potential issue | 🟠 MajorAvoid brittle
DATABASE_URLparsing for readiness checks.Lines 12–14 manually parse
DATABASE_URL; this is brittle and can produce wrong host/port for valid connection strings. Prefer probing with the URL directly.Suggested fix
- DB_HOST=$(echo $DATABASE_URL | sed -n 's|.*@\([^:/]*\).*|\1|p') - DB_PORT=$(echo $DATABASE_URL | sed -n 's|.*:\([0-9]*\)/.*|\1|p') - DB_PORT=${DB_PORT:-5432} + if [ -z "$DATABASE_URL" ]; then + echo "ERROR: DATABASE_URL is not set" + exit 1 + fi ... - if pg_isready -h "$DB_HOST" -p "$DB_PORT" > /dev/null 2>&1; then + if pg_isready -d "$DATABASE_URL" > /dev/null 2>&1; thenAlso applies to: 20-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-entrypoint.sh` around lines 12 - 14, The current manual sed parsing that assigns DB_HOST and DB_PORT from DATABASE_URL is brittle; instead modify the readiness check in docker-entrypoint.sh to avoid extracting host/port with DB_HOST/DB_PORT and use the DATABASE_URL directly (e.g., attempt a direct probe/connection using psql/pg_isready or a URL-aware parser) so the script tests connectivity against the actual connection string; update any references to DB_HOST/DB_PORT in the readiness logic to use DATABASE_URL or the parser's output (look for the DB_HOST, DB_PORT assignments and the readiness loop/health-check code that follows) and remove the fragile sed-based extraction.docker-entrypoint.sh-46-50 (1)
46-50:⚠️ Potential issue | 🟠 MajorDo not continue app startup after schema sync failure by default.
Lines 49–50 hide migration failures and continue boot, which can leave the app running against an incompatible schema.
Suggested fix
- if pnpm run push 2>&1; then + if pnpm run push 2>&1; then echo "Database schema synced successfully!" else - echo "WARNING: Schema sync failed, continuing anyway..." + echo "ERROR: Schema sync failed" + exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-entrypoint.sh` around lines 46 - 50, The current block running "pnpm run push" swallows failures and continues; change the else branch for the pnpm run push block so that on failure it logs an explicit error (including that schema sync failed) and exits non‑zero (e.g., exit 1) instead of continuing; locate the shell block that runs "pnpm run push" in docker-entrypoint.sh and replace the "WARNING: Schema sync failed, continuing anyway..." branch with an error log and an exit 1 to abort startup by default.src/components/forecasts/ForecastComparisonPage.tsx-34-42 (1)
34-42:⚠️ Potential issue | 🟠 MajorHandle fetch failures explicitly instead of silently showing empty-state.
Lines 34–42 lack
res.okchecks/catch handling, so API failures can be misreported as “No comparison data.”Suggested fix
+ const [error, setError] = useState<string | null>(null); + useEffect(() => { - Promise.all([ - fetch(`/api/forecasts/${forecastId}`).then((r) => r.json()), - fetch(`/api/forecasts/${forecastId}/comparison`).then((r) => r.json()), - ]).then(([forecast, comparison]) => { - setForecastName(forecast.name ?? ''); - setRows(comparison); - }).finally(() => setLoading(false)); + (async () => { + try { + const [forecastRes, comparisonRes] = await Promise.all([ + fetch(`/api/forecasts/${forecastId}`), + fetch(`/api/forecasts/${forecastId}/comparison`), + ]); + if (!forecastRes.ok || !comparisonRes.ok) throw new Error('Failed to load comparison data'); + const [forecast, comparison] = await Promise.all([forecastRes.json(), comparisonRes.json()]); + setForecastName(forecast.name ?? ''); + setRows(comparison); + } catch (e) { + setError(e instanceof Error ? e.message : 'Failed to load comparison data'); + } finally { + setLoading(false); + } + })(); }, [forecastId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forecasts/ForecastComparisonPage.tsx` around lines 34 - 42, The current useEffect fetches forecast and comparison without checking HTTP status or catching network errors, which can silently show the empty-state; update the Promise.all logic in the useEffect so each fetch checks res.ok and throws an Error with a descriptive message when not ok (for both `/api/forecasts/${forecastId}` and `/api/forecasts/${forecastId}/comparison`), add a .catch to handle any thrown errors, set an error state (e.g., setError) on failure, and only call setForecastName/setRows when the responses are valid; always call setLoading(false) in finally so the loading spinner stops.src/components/forecasts/VarianceCell.tsx-23-27 (1)
23-27:⚠️ Potential issue | 🟠 MajorGuard against invalid currency codes before formatting.
Lines 23–27 can throw a RangeError at runtime if
currencyreceives an invalid ISO-4217 code, breaking rendering. WhileVarianceCellis currently unused in the codebase, the component should be hardened before it enters production use. The default fallback to'IDR'mitigates some risk but doesn't prevent explicit invalid values from crashing the component.Suggested fix
+ const safeCurrency = /^[A-Z]{3}$/.test(currency) ? currency : 'USD'; const formatted = new Intl.NumberFormat('en-US', { style: 'currency', - currency, + currency: safeCurrency, maximumFractionDigits: 0, }).format(Math.abs(variance));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forecasts/VarianceCell.tsx` around lines 23 - 27, VarianceCell currently constructs an Intl.NumberFormat with the passed currency which can throw a RangeError for invalid ISO-4217 codes; wrap the formatter creation used to produce the formatted value (the new Intl.NumberFormat(...) that assigns to formatted) in a try/catch (or validate the currency) and fall back to a safe currency like 'IDR' when an error occurs so the component never throws at render time; update the code paths that use currency/formatted inside the VarianceCell component to use the safe fallback formatter.src/lib/db/migrations/0018_striped_machine_man.sql-3-12 (1)
3-12:⚠️ Potential issue | 🟠 MajorMissing unique index on
forecast_growth_rulesfor(forecast_id, scope_type, scope_id, year).Consistent with the schema.ts observation, this migration should include a unique index on
forecast_growth_rulesto prevent duplicate growth rules for the same scope and year.🔧 Proposed fix: Add migration line
Add this line at the end of the migration:
CREATE UNIQUE INDEX "forecast_growth_rules_unique_idx" ON "forecast_growth_rules" USING btree ("forecast_id","scope_type","scope_id","year");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/db/migrations/0018_striped_machine_man.sql` around lines 3 - 12, The migration for table forecast_growth_rules is missing a unique constraint for (forecast_id, scope_type, scope_id, year); add a CREATE UNIQUE INDEX statement named forecast_growth_rules_unique_idx on forecast_growth_rules using btree over the columns ("forecast_id","scope_type","scope_id","year") at the end of the migration to enforce uniqueness and prevent duplicate growth rules for the same scope and year.src/lib/db/schema.ts-692-702 (1)
692-702:⚠️ Potential issue | 🟠 MajorMissing unique constraint on
forecastGrowthRulesmay allow duplicate rules.Unlike
forecastSeasonalityWeights(which has a unique index onforecastId, scopeType, scopeId, month),forecastGrowthRuleslacks a unique constraint on(forecastId, scopeType, scopeId, year). This allows multiple conflicting growth rules for the same scope and year. TheapplyAssumptionslogic uses.find(), which returns only the first match, leading to non-deterministic behavior if duplicates exist.🔧 Proposed fix: Add unique index
export const forecastGrowthRules = pgTable('forecast_growth_rules', { id: serial('id').primaryKey(), forecastId: integer('forecast_id').notNull().references(() => forecasts.id), scopeType: forecastScopeTypeEnum('scope_type').notNull(), scopeId: integer('scope_id'), year: integer('year').notNull(), growthRate: decimal('growth_rate', { precision: 6, scale: 4 }).notNull(), createdAt: timestamp('created_at').defaultNow().notNull(), updatedAt: timestamp('updated_at').defaultNow().notNull(), -}); +}, (table) => ({ + uniqueRule: uniqueIndex('forecast_growth_rules_unique_idx').on( + table.forecastId, table.scopeType, table.scopeId, table.year + ), +}));A corresponding migration will be needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/db/schema.ts` around lines 692 - 702, Add a unique constraint/index on the forecast_growth_rules table to prevent duplicate rules for the same scope and year: enforce uniqueness across (forecast_id, scope_type, scope_id, year) in the pgTable definition (forecastGrowthRules) and add a corresponding DB migration to create the unique index; ensure the index semantics handle nullable scope_id as intended (or use a partial index if null scoping is required) so the applyAssumptions logic that uses .find() will always see at most one matching rule.src/app/api/forecasts/[id]/assumptions/route.ts-62-106 (1)
62-106:⚠️ Potential issue | 🟠 MajorDelete-then-insert operations are not atomic — risk of data loss.
The PUT handler deletes existing records before inserting new ones without a transaction. If an insert fails after a delete, data is permanently lost.
🔒 Proposed fix: Wrap in transaction
+ await db.transaction(async (tx) => { // Replace variables if (variables !== undefined) { - await db.delete(forecastVariables).where(eq(forecastVariables.forecastId, forecastId)); + await tx.delete(forecastVariables).where(eq(forecastVariables.forecastId, forecastId)); if (variables.length > 0) { - await db.insert(forecastVariables).values( + await tx.insert(forecastVariables).values( variables.map((v) => ({ forecastId, ...v, createdAt: now, updatedAt: now })) ); } } // Replace growth rules if (growthRules !== undefined) { - await db.delete(forecastGrowthRules).where(eq(forecastGrowthRules.forecastId, forecastId)); + await tx.delete(forecastGrowthRules).where(eq(forecastGrowthRules.forecastId, forecastId)); if (growthRules.length > 0) { - await db.insert(forecastGrowthRules).values( + await tx.insert(forecastGrowthRules).values( // ... mapping unchanged ); } } // Replace seasonality weights if (seasonalityWeights !== undefined) { - await db.delete(forecastSeasonalityWeights).where(eq(forecastSeasonalityWeights.forecastId, forecastId)); + await tx.delete(forecastSeasonalityWeights).where(eq(forecastSeasonalityWeights.forecastId, forecastId)); if (seasonalityWeights.length > 0) { - await db.insert(forecastSeasonalityWeights).values( + await tx.insert(forecastSeasonalityWeights).values( // ... mapping unchanged ); } } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/forecasts/`[id]/assumptions/route.ts around lines 62 - 106, The delete-then-insert sequences for variables, growthRules, and seasonalityWeights are not executed atomically and can cause data loss; wrap the replace logic for forecastVariables, forecastGrowthRules, and forecastSeasonalityWeights in a single database transaction so the deletes and subsequent inserts for a given forecastId either fully commit or fully roll back. Specifically, perform the operations currently using db.delete(...) and db.insert(...) inside a transactional callback (e.g., db.transaction or your ORM's unit-of-work API), using the transactional DB handle for the delete and insert calls and propagating errors to trigger rollback; apply this to the blocks that reference variables, growthRules, seasonalityWeights, forecastId, and now.src/lib/forecasts/assumptions.ts-163-171 (1)
163-171:⚠️ Potential issue | 🟠 MajorSequential upserts cause O(n) database round-trips.
Each row is upserted individually inside a loop. For a multi-year forecast with many items (e.g., 50 items × 3 years × 12 months = 1,800 rows), this results in 1,800 separate database calls, causing significant latency.
Drizzle ORM supports batch operations with
onConflictDoUpdate. Consider restructuring to perform a single bulk insert.⚡ Proposed fix: Batch upsert
- for (const row of upsertRows) { - await db - .insert(forecastPeriodValues) - .values({ ...row, createdAt: new Date(), updatedAt: new Date() }) - .onConflictDoUpdate({ - target: [forecastPeriodValues.itemId, forecastPeriodValues.period], - set: { amount: row.amount, updatedAt: new Date() }, - }); - } + const now = new Date(); + const rowsWithTimestamps = upsertRows.map(row => ({ + ...row, + createdAt: now, + updatedAt: now, + })); + + // Batch insert - Drizzle handles bulk onConflictDoUpdate + await db + .insert(forecastPeriodValues) + .values(rowsWithTimestamps) + .onConflictDoUpdate({ + target: [forecastPeriodValues.itemId, forecastPeriodValues.period], + set: { amount: sql`excluded.amount`, updatedAt: now }, + });Note: You'll need to import
sqlfromdrizzle-ormfor theexcluded.amountreference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/forecasts/assumptions.ts` around lines 163 - 171, The loop performs per-row upserts causing O(n) DB calls; replace the for-loop in assumptions.ts with a single bulk insert: build an array of rows where each row spreads original row and sets createdAt/updatedAt to new Date(), then call db.insert(forecastPeriodValues).values(bulkRows).onConflictDoUpdate({ target: [forecastPeriodValues.itemId, forecastPeriodValues.period], set: { amount: sql`excluded.amount`, updatedAt: new Date() } }); also import sql from 'drizzle-orm' and remove the per-row await loop to ensure one batch upsert instead of many calls.src/app/api/forecasts/[id]/expense-periods/bulk/route.ts-30-45 (1)
30-45:⚠️ Potential issue | 🟠 MajorMake the bulk upsert atomic.
A failure on the Nth row leaves rows
1..N-1committed, which is especially painful for CSV-sized imports. Wrap the write in a transaction, or send it as one multi-row upsert, so the request is all-or-nothing and avoids N round-trips.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/forecasts/`[id]/expense-periods/bulk/route.ts around lines 30 - 45, The loop that calls db.insert(forecastPeriodValues).values(...) for each v in parsed.data.values is performing N separate upserts so partial commits can occur; wrap the whole operation in a single atomic transaction or perform one multi-row upsert instead. Modify the code around the for (const v of parsed.data.values) loop to collect all value objects (including forecastId, itemId, period, amount, createdAt, updatedAt) and then call db.transaction(async (tx) => { await tx.insert(forecastPeriodValues).values(allRows).onConflictDoUpdate({ target: [forecastPeriodValues.itemId, forecastPeriodValues.period], set: { amount: /*excluded*/ , updatedAt: new Date() } }); }) or use the DB client's batch/multi-row upsert API so the entire import is committed or rolled back as one; ensure you use forecastId in the inserted rows and keep the existing onConflictDoUpdate behavior but execute it in one statement/transaction rather than inside the per-row loop.src/app/api/forecasts/[id]/metrics/route.ts-11-17 (1)
11-17:⚠️ Potential issue | 🟠 MajorReject malformed forecast ids before the DB lookup.
This has the same parsing issue as the tracking route: a bad path segment can become
NaNor a partially parsed integer and make the handler query with an unintended id instead of failing fast with 400.🛠️ Tighten the route-param validation
const { id } = await params; - const forecastId = parseInt(id); + const forecastId = Number(id); + if (!Number.isSafeInteger(forecastId) || forecastId <= 0) { + return NextResponse.json({ message: 'Invalid forecast id' }, { status: 400 }); + } const [forecast] = await db🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/forecasts/`[id]/metrics/route.ts around lines 11 - 17, The handler currently uses parseInt(id) and may accept malformed path segments; validate the route param before hitting the DB by ensuring params.id is a strictly numeric string (e.g., regex /^\d+$/ or Number.isInteger(Number(id)) after base-10 parse) and only then set forecastId via parseInt(id, 10); if the id is invalid, short-circuit and return a 400 response instead of running db.select or querying forecasts/forecastId.src/components/forecasts/AssumptionsPanel.tsx-81-85 (1)
81-85:⚠️ Potential issue | 🟠 Major
Apply Assumptionscurrently ignores the draft on screen.
onApplyreceives no form data, so clicking Apply after editing variables/rules/weights recomputes from the last saved assumptions, not what the user just changed.💡 Save the current draft before applying it
async function handleApply() { setApplying(true); try { + await onSave({ variables, growthRules, seasonalityWeights: weights }); await onApply(); } finally { setApplying(false); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forecasts/AssumptionsPanel.tsx` around lines 81 - 85, handleApply currently calls onApply() without persisting the in-memory form changes, so the apply uses the last saved assumptions; update handleApply to first persist the current draft (e.g., await the form save API or call the existing saveDraft/submitDraft function or formRef.submit()) before awaiting onApply(), ensuring you await the save operation and only then call onApply() while preserving setApplying state handling. Reference: handleApply and onApply.src/app/api/forecasts/[id]/tracking/route.ts-11-17 (1)
11-17:⚠️ Potential issue | 🟠 MajorReject malformed forecast ids before the DB lookup.
parseIntwill turn non-numeric segments intoNaNand partially numeric segments into a different id, so this route can either fall through to a bad query or resolve the wrong forecast id. Validate the segment first and return 400.🛠️ Tighten the route-param validation
const { id } = await params; - const forecastId = parseInt(id); + const forecastId = Number(id); + if (!Number.isSafeInteger(forecastId) || forecastId <= 0) { + return NextResponse.json({ message: 'Invalid forecast id' }, { status: 400 }); + } const [forecast] = await db🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/forecasts/`[id]/tracking/route.ts around lines 11 - 17, The route currently uses parseInt on params.id which can yield NaN or silently coerce malformed segments; before calling parseInt and querying the DB (see params, parseInt, forecastId, and the db.select on forecasts), validate that the id path segment is a strictly numeric integer (e.g. /^\d+$/ or Number.isInteger after Number(value)) and if it fails return a 400 Bad Request immediately; only then parse to an integer and proceed with the db lookup to ensure you never query with an invalid or partially parsed id.src/components/forecasts/AssumptionsPanel.tsx-65-70 (1)
65-70:⚠️ Potential issue | 🟠 MajorResync the draft state when the incoming assumptions change.
These
useState(initial...)calls only seed the form once. If the parent refetches assumptions or swaps to a different forecast, the dialog keeps editing the old arrays and the next save can overwrite fresher server data.🛠️ One way to reset the local draft from the latest props
-import { useState } from 'react'; +import { useEffect, useState } from 'react';const [weights, setWeights] = useState<SeasonalityWeight[]>(initialWeights); const [saving, setSaving] = useState(false); const [applying, setApplying] = useState(false); const [open, setOpen] = useState(false); + + useEffect(() => { + if (open) return; + setVariables(initialVars); + setGrowthRules(initialRules); + setWeights(initialWeights); + }, [open, initialVars, initialRules, initialWeights]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forecasts/AssumptionsPanel.tsx` around lines 65 - 70, Add a useEffect that watches the incoming prop arrays (initialVars, initialRules, initialWeights) and updates the local draft state variables (variables, growthRules, weights) when those props change; to avoid clobbering an active edit, only call setVariables(initialVars), setGrowthRules(initialRules) and setWeights(initialWeights) when the dialog is closed (open is false). Implement this by adding useEffect(() => { if (!open) { setVariables(initialVars); setGrowthRules(initialRules); setWeights(initialWeights); } }, [initialVars, initialRules, initialWeights, open]) so the local draft always resyncs to fresh assumptions from the parent except while the user is actively editing.src/app/api/forecasts/[id]/route.ts-58-65 (1)
58-65:⚠️ Potential issue | 🟠 MajorHandle malformed JSON explicitly in
PUT.Line 58 can throw on invalid JSON and currently bubbles to a 500 path. Return a structured
400instead.Proposed fix
- const body = await request.json(); + let body: unknown; + try { + body = await request.json(); + } catch { + return NextResponse.json({ message: 'Invalid JSON body' }, { status: 400 }); + } const parsed = forecastSchema.partial().safeParse(body);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/forecasts/`[id]/route.ts around lines 58 - 65, Wrap the await request.json() call in a try/catch to explicitly handle malformed JSON: catch JSON parsing errors around the line with const body = await request.json(), and return a 400 NextResponse.json with a clear message (e.g., "Malformed JSON" or "Invalid JSON") instead of letting it throw; keep the subsequent forecastSchema.partial().safeParse(body) validation as-is so schema errors still return the existing 400 payload.src/app/api/forecasts/[id]/import/route.ts-81-145 (1)
81-145:⚠️ Potential issue | 🟠 MajorMake import writes atomic to avoid partial data persistence.
All stream/item/value writes run outside a transaction. Any mid-import failure can leave partially imported data.
Proposed fix
- for (const row of rows) { + await db.transaction(async (tx) => { + for (const row of rows) { // Get or create stream - let streamId = streamCache.get(streamName); + let streamId = streamCache.get(streamName); if (!streamId) { @@ - const [newStream] = await db + const [newStream] = await tx .insert(forecastStreams) @@ - let itemId = itemCache.get(itemKey); + let itemId = itemCache.get(itemKey); if (!itemId) { - const [newItem] = await db + const [newItem] = await tx .insert(forecastItems) @@ - await db + await tx .insert(forecastPeriodValues) @@ - } - } + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/forecasts/`[id]/import/route.ts around lines 81 - 145, Wrap the entire import loop in a single database transaction so all inserts/updates for forecastStreams, forecastItems and forecastPeriodValues are committed atomically or rolled back on error; start a transaction (e.g., db.transaction or equivalent), replace calls to db.insert(...).returning() and db.insert(...).onConflictDoUpdate(...) with the transaction handle (tx) inside the transaction callback, and perform all cache updates (streamCache.set, itemCache.set) and upsertCount increments within that transaction scope; ensure errors propagate to abort the transaction and that you return/commit only when the full import completes successfully.src/components/forecasts/ForecastImportPage.tsx-102-116 (1)
102-116:⚠️ Potential issue | 🟠 MajorMake file selection keyboard-accessible and align UI behavior text.
Line 102 uses a clickable
div+hiddeninput, which is not keyboard-friendly. Also, Line 108 says “drag and drop” but no drop handlers are implemented.Proposed fix
- <div - className="border-2 border-dashed rounded-lg p-8 text-center cursor-pointer hover:bg-accent/20 transition-colors" - onClick={() => inputRef.current?.click()} - > + <label + htmlFor="forecast-import-file" + className="block border-2 border-dashed rounded-lg p-8 text-center cursor-pointer hover:bg-accent/20 transition-colors" + > <Upload className="h-10 w-10 mx-auto text-muted-foreground mb-2" /> <p className="text-sm font-medium">{file ? file.name : 'Click to select a CSV file'}</p> - <p className="text-xs text-muted-foreground mt-1">or drag and drop</p> - <input - ref={inputRef} - type="file" - accept=".csv,text/csv" - className="hidden" - onChange={handleFileChange} - /> - </div> + <p className="text-xs text-muted-foreground mt-1">CSV only</p> + </label> + <input + id="forecast-import-file" + ref={inputRef} + type="file" + accept=".csv,text/csv" + className="sr-only" + onChange={handleFileChange} + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forecasts/ForecastImportPage.tsx` around lines 102 - 116, The upload area currently uses a non-focusable div with a hidden input (inputRef) and lacks drag handlers; make it keyboard-accessible by replacing the clickable div with a semantic button (or add tabIndex, role="button" and keydown handling for Enter/Space) that forwards clicks to inputRef and calls handleFileChange on keyboard activation; either implement drag-and-drop handlers (onDragOver, onDrop) wired to the same file processing (reuse handleFileChange or a new handleDrop wrapper) or change the UI text to remove “drag and drop” to match behavior; ensure the visible element is reachable via keyboard and announces itself to assistive tech (aria-label or aria-describedby) so screen readers can identify the upload action.src/app/api/forecasts/[id]/route.ts-12-14 (1)
12-14:⚠️ Potential issue | 🟠 MajorValidate
idparameter in all three handlers before DB operations.Lines 13, 47, and 82 parse
idwithparseIntbut never validate the result.parseInt("abc")returnsNaNandparseInt("123abc")returns123. Invalid IDs should return400early instead of proceeding with potentially invalid database queries.Proposed fix pattern
- const forecastId = parseInt(id); + const forecastId = Number(id); + if (!Number.isInteger(forecastId) || forecastId <= 0) { + return NextResponse.json({ message: 'Invalid forecast id' }, { status: 400 }); + }Also applies to: 46-48, 81-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/forecasts/`[id]/route.ts around lines 12 - 14, The handlers in src/app/api/forecasts/[id]/route.ts parse the route param into forecastId using parseInt on the extracted id (variable names: params, id, forecastId) but never validate the result; update each handler (GET/PUT/DELETE) to validate the incoming id before any DB operation by ensuring the original id is a non-empty numeric string (e.g., /^\d+$/) or that Number(parsed) is an integer and not NaN, and if validation fails immediately return a 400 response with a clear error message; perform this check right after extracting id from params and before using forecastId in any repository/DB call.src/app/api/forecasts/[id]/import/route.ts-25-27 (1)
25-27:⚠️ Potential issue | 🟠 MajorValidate route ID parameters before database operations.
Line 26 uses
parseInt(id)without validation, allowing invalid values likeNaNto reach database queries. Invalid IDs should be rejected with a 400 response before any database operations.Proposed fix
const { companyId } = authInfo; const { id } = await params; - const forecastId = parseInt(id); + const forecastId = Number(id); + if (!Number.isInteger(forecastId) || forecastId <= 0) { + return NextResponse.json({ message: 'Invalid forecast id' }, { status: 400 }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/forecasts/`[id]/import/route.ts around lines 25 - 27, The code parses the route param with const { id } = await params; const forecastId = parseInt(id); without validating the result, so NaN can reach DB queries; update the handler in route.ts to validate the id after parsing (e.g., parseInt/id -> const forecastId = Number(id) or parseInt(id,10) and then check Number.isInteger(forecastId) && forecastId > 0 or isNaN(forecastId)), and if invalid return an early 400 response (Bad Request) before any database operations that use forecastId.src/app/api/forecasts/[id]/revenue-periods/bulk/route.ts-12-14 (1)
12-14:⚠️ Potential issue | 🟠 MajorReturn
400for invalididand malformed JSON payloads.Line 13 uses unchecked
parseIntwithout validation—parseInt("abc")returnsNaNandparseInt("123abc")returns123, both of which could cause database errors. Line 24 lacks a try-catch; malformed JSON in the request body will throw an unhandled exception instead of returning a client error.Proposed fix
const { companyId } = authInfo; const { id } = await params; - const forecastId = parseInt(id); + const forecastId = Number(id); + if (!Number.isInteger(forecastId) || forecastId <= 0) { + return NextResponse.json({ message: 'Invalid forecast id' }, { status: 400 }); + } @@ - const body = await request.json(); + let body: unknown; + try { + body = await request.json(); + } catch { + return NextResponse.json({ message: 'Invalid JSON body' }, { status: 400 }); + } const parsed = forecastBulkPeriodValuesSchema.safeParse(body);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/forecasts/`[id]/revenue-periods/bulk/route.ts around lines 12 - 14, Validate the forecast id and JSON payload: ensure the extracted params id is checked after parseInt (the forecastId variable) and return a 400 response when it is NaN or when the id contains non-digit characters, and wrap the request.json() call in a try/catch inside the route handler so malformed JSON results in a 400 response; update the code paths that use forecastId to bail out early on invalid id and return a descriptive 400 error, and catch JSON parsing errors around the request.json() call to return a 400 instead of letting an exception propagate.src/app/(authenticated)/forecasts/[id]/page.tsx-8-10 (1)
8-10:⚠️ Potential issue | 🟠 MajorValidate route
idbefore rendering the editor.Line 10 uses
parseInt(id)directly without validation. TheparseIntfunction accepts partially-numeric strings (e.g.,"12abc"→12) and returnsNaNonly for completely non-numeric input, causing requests to route to unintended forecasts or break downstream API calls throughout the component.Proposed fix
import { Metadata } from 'next'; +import { notFound } from 'next/navigation'; import ForecastEditorPage from '@/components/forecasts/ForecastEditorPage'; @@ export default async function Page({ params }: { params: Promise<{ id: string }> }) { const { id } = await params; - return <ForecastEditorPage forecastId={parseInt(id)} />; + const forecastId = Number(id); + if (!Number.isInteger(forecastId) || forecastId <= 0) { + notFound(); + } + return <ForecastEditorPage forecastId={forecastId} />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(authenticated)/forecasts/[id]/page.tsx around lines 8 - 10, The Page component currently does parseInt(id) and passes it to ForecastEditorPage without validation; update Page to validate params before rendering by ensuring the incoming id is a strictly numeric string (e.g., use /^\d+$/.test(id) or Number.isInteger(+id) combined with a strict string check) and only then call ForecastEditorPage with forecastId={parseInt(id, 10)}; if validation fails, handle it explicitly (return a 404/notFound response or render an error) instead of proceeding with a NaN/partially-parsed id.src/app/(authenticated)/forecasts/[id]/comparison/page.tsx-8-10 (1)
8-10:⚠️ Potential issue | 🟠 MajorAdd ID validation to prevent NaN propagation to API calls.
Line 10 uses
parseInt(id)without checking validity. Non-numeric route parameters (e.g.,/forecasts/abc/comparison) result inNaN, which propagates to the component and causes invalid API requests like/api/forecasts/NaN. This pattern appears in multiple forecast pages and should be guarded.Proposed fix
import { Metadata } from 'next'; +import { notFound } from 'next/navigation'; import ForecastComparisonPage from '@/components/forecasts/ForecastComparisonPage'; @@ export default async function Page({ params }: { params: Promise<{ id: string }> }) { const { id } = await params; - return <ForecastComparisonPage forecastId={parseInt(id)} />; + const forecastId = Number(id); + if (!Number.isInteger(forecastId) || forecastId <= 0) { + notFound(); + } + return <ForecastComparisonPage forecastId={forecastId} />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(authenticated)/forecasts/[id]/comparison/page.tsx around lines 8 - 10, The Page component extracts id via await params and passes parseInt(id) to ForecastComparisonPage without validation, which can produce NaN; update the Page function to validate the route param before calling parseInt (check that the awaited id is a finite integer string or Number.isInteger(Number(id))), and if invalid handle it early (e.g., call notFound()/redirect or render a safe fallback) instead of passing NaN into ForecastComparisonPage; apply the same validation pattern to other forecast page components that use parseInt on route params.src/app/api/forecasts/[id]/summary/route.ts-11-13 (1)
11-13:⚠️ Potential issue | 🟠 MajorAdd explicit validation for forecast ID before querying the database.
Line 12 uses
parseInt(id)without validation. Invalid input like non-numeric strings produceNaN, which bypasses the!forecastcheck on line 19 and can lead to unpredictable database behavior. Other routes in the codebase (e.g.,items/route.ts,streams/route.ts) validate withisNaN()checks for similar numeric parameters.Proposed fix
const { companyId } = authInfo; const { id } = await params; - const forecastId = parseInt(id); + const forecastId = Number(id); + if (!Number.isInteger(forecastId) || forecastId <= 0) { + return NextResponse.json({ message: 'Invalid forecast id' }, { status: 400 }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/forecasts/`[id]/summary/route.ts around lines 11 - 13, The code currently assigns forecastId via parseInt(id) without validating it; update the handler in route.ts to explicitly validate the parsed forecastId using isNaN(parsed) (or Number.isNaN) after the parseInt(id) call and return an appropriate client error (e.g., 400/Bad Request) if the value is not a valid number before proceeding to query the database (so that forecastId is never NaN when passed to the DB query that looks up the forecast). Ensure you reference the existing symbols params, id, forecastId and the parseInt call so the check is added immediately after that line and before the code that uses forecastId to fetch the forecast.src/app/api/forecasts/[id]/comparison/route.ts-11-13 (1)
11-13:⚠️ Potential issue | 🟠 MajorValidate
forecastIdbefore DB access.Line 12 parses with
parseInt(id)and proceeds directly to database query without validating the result. This can passNaN(from non-numeric input like "abc") or unexpected partial values into the query.Proposed fix
const { id } = await params; - const forecastId = parseInt(id); + const forecastId = Number(id); + if (!Number.isInteger(forecastId) || forecastId <= 0) { + return NextResponse.json({ message: 'Invalid forecast id' }, { status: 400 }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/forecasts/`[id]/comparison/route.ts around lines 11 - 13, The code parses params.id into forecastId using parseInt without validation, which can yield NaN or partial numbers and then hit the DB; update the route handler to validate the parsed value (use parseInt(id) result stored in forecastId), check Number.isInteger(forecastId) or !Number.isNaN(forecastId) and that it is positive, and if invalid return an early 400/Bad Request response (or throw a controlled HTTP error) instead of proceeding to the DB query; ensure the validation sits before any use of forecastId in the database lookup so only valid numeric IDs reach the query.src/components/forecasts/StreamTree.tsx-83-95 (1)
83-95:⚠️ Potential issue | 🟠 MajorAdd accessible names to the icon-only actions.
These buttons render only icons, so assistive tech gets unnamed controls. Add contextual
aria-labels such as “Add item to …”, “Delete stream …”, and “Delete item …” so the sidebar CRUD flow is usable with a screen reader.Also applies to: 117-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forecasts/StreamTree.tsx` around lines 83 - 95, The icon-only Buttons in StreamTree.tsx lack accessible names; update the Button elements that call onAddItem(stream.id), onDeleteStream(stream.id) (and the similar icon-only delete for items that calls onDeleteItem) to include descriptive aria-label props like `aria-label={`Add item to ${stream.title || stream.id}`}`, `aria-label={`Delete stream ${stream.title || stream.id}`}`, and `aria-label={`Delete item ${item.title || item.id} from ${stream.title || stream.id}`}` so screen readers receive context while preserving the existing onClick handlers.src/components/forecasts/ForecastEditorPage.tsx-82-116 (1)
82-116:⚠️ Potential issue | 🟠 MajorHandle bootstrap failures explicitly.
This block assumes all four requests succeed and return the expected shape. Any rejected fetch or error payload can leave the page stuck on the spinner or throw on
forecastData.startDate; wrap the load intry/catch/finally, checkres.ok, and keep an explicit error state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forecasts/ForecastEditorPage.tsx` around lines 82 - 116, Wrap the async load() body in a try/catch/finally inside the useEffect so any rejected fetch or JSON parse is caught; after each fetch (the calls fetching `/api/forecasts/${forecastId}`, `/api/forecasts/${forecastId}/assumptions`, `/api/income-categories`, `/api/expense-categories`) check res.ok and throw or set an explicit error when not ok before calling res.json(); in the catch set an explicit error state (e.g., setError) and avoid accessing nested fields like forecastData.startDate unless forecastData is valid (guard before parseInt), and in finally always call setLoading(false) so the spinner cannot get stuck; also guard the call to loadPeriodValues by confirming forecastData.items/startDate/endDate exist before awaiting it.src/lib/forecasts/metrics.ts-33-34 (1)
33-34:⚠️ Potential issue | 🟠 MajorUse the number of forecast months, not always 12.
These are labeled as average monthly rates, but Lines 33-34 divide by 12 even for partial years. A Jul–Dec forecast year should divide by 6, otherwise burn/run rate is materially understated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/forecasts/metrics.ts` around lines 33 - 34, The burnRate and runRate assignments currently divide by 12 regardless of partial years; change them to divide by the actual number of forecast months in that year. Compute monthsInYear for each year (e.g., from the forecast months array/range or a helper like getMonthsCountForYear(year, forecastStart, forecastEnd) or a precomputed monthsByYear map) then set burnRate[year] = exp / monthsInYear and runRate[year] = rev / monthsInYear, and guard against monthsInYear === 0 to avoid divide-by-zero.src/components/forecasts/ForecastEditorPage.tsx-183-193 (1)
183-193:⚠️ Potential issue | 🟠 MajorOnly mutate local state after delete success.
Both delete handlers remove rows from state without checking
res.ok. Any server rejection leaves the UI showing deleted data until the next refresh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forecasts/ForecastEditorPage.tsx` around lines 183 - 193, The delete handlers handleDeleteStream and handleDeleteItem currently mutate local state immediately; change them to first await the fetch into a response variable, check response.ok (and optionally parse/collect an error message), and only call setStreams/setItems/setPeriodValues when the response is successful; wrap the fetch in try/catch to handle network errors and surface or log failures instead of removing items from state on error.src/components/forecasts/ForecastTrackingPage.tsx-58-73 (1)
58-73:⚠️ Potential issue | 🟠 MajorGuard the bootstrap fetches before using their payloads.
Lines 60-68 treat both responses as success data. A 404/500 JSON body will make
trackingan object, and Lines 84, 86, and 95 will then fail on.map()/.filter(). Check bothokflags, validateArray.isArray(tracking), and fail into an explicit error state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forecasts/ForecastTrackingPage.tsx` around lines 58 - 73, The fetches inside the useEffect (the two fetch(`/api/forecasts/${forecastId}`) and fetch(`/api/forecasts/${forecastId}/tracking`) calls) must check each Response.ok and parse/throw on non-OK before using payloads; after Promise.all verify that tracking is an array with Array.isArray(tracking) before calling .map/.filter and fallback to an explicit error state (e.g., setAllRows([]) and set an error flag or message via existing state setter) so downstream code (setForecastName, setAllRows, setSelectedPeriod) never operates on an unexpected object; ensure setLoading(false) still runs in finally and propagate/handle thrown errors to update the error state.src/components/forecasts/StreamTree.tsx-104-123 (1)
104-123:⚠️ Potential issue | 🟠 MajorMake the item delete action visible.
Line 120 uses
group-hover:opacity-100, but the row never setsgroup. The button stays transparent, so item deletion is effectively hidden.Proposed fix
- className={cn( - 'flex items-center justify-between w-full px-6 py-2 text-sm text-left hover:bg-accent transition-colors cursor-pointer', + className={cn( + 'group flex items-center justify-between w-full px-6 py-2 text-sm text-left hover:bg-accent transition-colors cursor-pointer', selectedItemId === item.id && 'bg-accent font-medium' )} @@ - className="h-5 w-5 text-destructive opacity-0 group-hover:opacity-100" + className="h-5 w-5 text-destructive opacity-0 transition-opacity group-hover:opacity-100 group-focus-within:opacity-100"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forecasts/StreamTree.tsx` around lines 104 - 123, The delete Button stays transparent because the parent row never sets the "group" utility used by the Button's "group-hover:opacity-100"; update the row container (the div rendered for each item in StreamTree.tsx—the element using selectedItemId, onSelectItem and rendering the Button when onDeleteItem exists) to include the "group" class (or alternatively change the Button hover selector to use "hover:opacity-100"); add "group" to the container's className so the Button's group-hover rule becomes effective and the Trash2 icon becomes visible on hover.src/components/forecasts/ForecastsListPage.tsx-24-29 (1)
24-29:⚠️ Potential issue | 🟠 MajorValidate the
/api/forecastsresponse before storing it.Lines 25-27 assume every response body is the forecast array. A 401/500 JSON payload will be stored in
forecasts, and the next render will blow up onforecasts.map(...). Checkr.ok, validateArray.isArray(data), and render an error state instead of only logging.Proposed fix
useEffect(() => { - fetch('/api/forecasts') - .then((r) => r.json()) - .then(setForecasts) - .catch(console.error) - .finally(() => setLoading(false)); + let cancelled = false; + + (async () => { + try { + const r = await fetch('/api/forecasts'); + if (!r.ok) throw new Error('Failed to load forecasts'); + + const data = await r.json(); + if (!Array.isArray(data)) throw new Error('Invalid forecasts response'); + + if (!cancelled) setForecasts(data); + } catch (error) { + console.error(error); + if (!cancelled) setForecasts([]); + } finally { + if (!cancelled) setLoading(false); + } + })(); + + return () => { + cancelled = true; + }; }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forecasts/ForecastsListPage.tsx` around lines 24 - 29, The fetch in useEffect that hits '/api/forecasts' currently assumes the response body is always the forecasts array; update the logic to check response.ok and validate the parsed JSON before calling setForecasts: inside the then chain (or async function) inspect r.ok and if false call setError (or set a local error state) with a message, otherwise parse the JSON, verify Array.isArray(data) and only then call setForecasts(data); if validation fails call setError instead of setForecasts so the component can render an error state (and keep the existing finally to call setLoading(false)). Ensure you reference the existing useEffect/fetch('/api/forecasts'), setForecasts and setLoading, and that forecasts.map only runs when forecasts is a valid array (or guarded by the error/loading state).src/app/api/forecasts/[id]/streams/route.ts-91-93 (1)
91-93:⚠️ Potential issue | 🟠 MajorAdd cascade delete for stream child records.
This DELETE only removes
forecastStreamsrecords. Foreign key constraints are set toON DELETE no action(not cascade), so deleting a stream will leave orphanedforecastItemsand downstreamforecastPeriodValues. Either addON DELETE CASCADEto the constraints in the database schema, or delete all child records in a transaction before removing the stream:-- In migration, update constraints: ALTER TABLE "forecast_items" DROP CONSTRAINT "forecast_items_stream_id_forecast_streams_id_fk"; ALTER TABLE "forecast_items" ADD CONSTRAINT "forecast_items_stream_id_forecast_streams_id_fk" FOREIGN KEY ("stream_id") REFERENCES "public"."forecast_streams"("id") ON DELETE CASCADE ON UPDATE no action; ALTER TABLE "forecast_period_values" DROP CONSTRAINT "forecast_period_values_item_id_forecast_items_id_fk"; ALTER TABLE "forecast_period_values" ADD CONSTRAINT "forecast_period_values_item_id_forecast_items_id_fk" FOREIGN KEY ("item_id") REFERENCES "public"."forecast_items"("id") ON DELETE CASCADE ON UPDATE no action;Or in the route handler, delete children before the stream.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/forecasts/`[id]/streams/route.ts around lines 91 - 93, The DELETE currently only removes forecastStreams (query using forecastStreams, streamId, forecastId) leaving orphaned forecastItems and forecastPeriodValues because FKs are not cascading; either update the DB schema migrations to add ON DELETE CASCADE to the foreign key constraints for forecast_items.stream_id -> forecast_streams.id and forecast_period_values.item_id -> forecast_items.id, or modify this route handler to run a transaction that first deletes from forecast_period_values where item_id IN (select id from forecast_items where stream_id = streamId and forecast_id = forecastId), then deletes from forecast_items for that stream, and finally deletes the forecast_streams row (all using the same db connection/transaction to ensure atomicity).src/components/forecasts/ForecastGrid.tsx-73-81 (1)
73-81:⚠️ Potential issue | 🟠 MajorSurface bulk-save failures explicitly.
If
onBulkSaverejects, users get no feedback and only see saving stop. Add error handling with visible feedback so retry paths are clear.🔧 Suggested fix
+ const [saveError, setSaveError] = useState<string | null>(null); async function handleSave() { if (pendingChanges.length === 0) return; + setSaveError(null); setSaving(true); try { await onBulkSave(pendingChanges); setPendingChanges([]); + } catch { + setSaveError('Failed to save changes. Please try again.'); } finally { setSaving(false); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forecasts/ForecastGrid.tsx` around lines 73 - 81, handleSave currently awaits onBulkSave but never handles rejections, so failures silently stop saving; wrap the await in a try/catch that catches the error from onBulkSave, keeps pendingChanges intact (do not clear them), and setSaving(false) in finally; add a new state (e.g., [saveError, setSaveError]) and in the catch call setSaveError(error.message || String(error)) and also call any existing user-notification helper (or expose the error to the parent via a prop) so the UI can show a visible error and a clear retry path; update the component to render the saveError message and a Retry action that re-invokes handleSave.src/components/forecasts/ForecastGrid.tsx-163-170 (1)
163-170:⚠️ Potential issue | 🟠 MajorMake cell editing keyboard-accessible.
Using clickable
<td>cells blocks keyboard-only users from entering edit mode. Use a focusable control (e.g., button) for activation.♿ Suggested fix
- <td - key={p} - className={cn( - 'px-2 py-1 text-right cursor-pointer', - isPending && 'bg-yellow-50 dark:bg-yellow-900/20' - )} - onClick={() => !isEditing && startEdit(item.id, p)} - > + <td + key={p} + className={cn( + 'px-2 py-1 text-right', + isPending && 'bg-yellow-50 dark:bg-yellow-900/20' + )} + > {isEditing ? ( <input ... /> ) : ( - <span className={cn(val === 0 ? 'text-muted-foreground/40' : '')}> - {formatAmount(val, currency)} - </span> + <button + type="button" + className="w-full text-right" + onClick={() => startEdit(item.id, p)} + > + <span className={cn(val === 0 ? 'text-muted-foreground/40' : '')}> + {formatAmount(val, currency)} + </span> + </button> )} </td>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forecasts/ForecastGrid.tsx` around lines 163 - 170, The table cell currently uses an onClick on <td> (in ForecastGrid.tsx) which blocks keyboard users; wrap the cell content in a focusable control (e.g., a <button>) and move the click handler there: keep the existing logic (use isEditing to guard and call startEdit(item.id, p)), move the cn(...) classes from the <td> that affect interactive styling to the button (or split static cell styles on <td> and interactive styles on the button), add an accessible label (aria-label or visually hidden text) that describes the action and target, and ensure the button is keyboard operable and visually consistent with the table cell. This preserves item.id and p usage while making editing keyboard-accessible.src/components/forecasts/ForecastGrid.tsx-177-180 (1)
177-180:⚠️ Potential issue | 🟠 MajorPrevent duplicate commits when Tab or Enter exits the input field.
The input triggers both
onKeyDown(lines 179) andonBlur(line 177) for Tab and Enter, causingcommitEditto execute twice. This duplicates theonCellChangecallback and creates redundantsetPendingChangesupdates for the same edit.For Tab specifically: onKeyDown fires
commitEdit, then Tab moves focus away, triggeringonBlurwhich callscommitEditagain. For Enter: the same duplication occurs unless the input is in a form (where Enter would only fire onKeyDown).🔧 Suggested fix
- onBlur={() => commitEdit(item.id, p)} + onBlur={() => commitEdit(item.id, p)} onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === 'Tab') commitEdit(item.id, p); - if (e.key === 'Escape') setEditingCell(null); + if (e.key === 'Enter') { + e.preventDefault(); + commitEdit(item.id, p); + } + // Let Tab naturally blur; blur handler already commits once. + if (e.key === 'Escape') { + e.preventDefault(); + setEditingCell(null); + } }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/forecasts/ForecastGrid.tsx` around lines 177 - 180, The handlers call commitEdit twice because onKeyDown for Enter/Tab and onBlur both fire; fix by introducing a short-lived flag (e.g., skipNextBlurRef) and set it to true inside the onKeyDown branch that handles Enter/Tab before calling commitEdit, then update the onBlur handler to check that flag and if set clear it and skip calling commitEdit; keep the Escape branch (setEditingCell(null)) unchanged and ensure the flag is cleared after skipping so subsequent blurs behave normally; update references in the component where commitEdit, onKeyDown, onBlur, setEditingCell, onCellChange, and setPendingChanges are used.
Comment on lines
+202
to
+208
| async function handleBulkSave(changes: PeriodValue[]) { | ||
| const body = { values: changes.map((c) => ({ itemId: c.itemId, period: c.period, amount: c.amount.toFixed(2) })) }; | ||
| await fetch(`/api/forecasts/${forecastId}/revenue-periods/bulk`, { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(body), | ||
| }); |
There was a problem hiding this comment.
Split bulk saves by stream type.
The grid contains both revenue and expense items, but this handler always writes to /revenue-periods/bulk. Expense edits will hit the wrong endpoint and won't persist correctly.
Proposed fix
async function handleBulkSave(changes: PeriodValue[]) {
- const body = { values: changes.map((c) => ({ itemId: c.itemId, period: c.period, amount: c.amount.toFixed(2) })) };
- await fetch(`/api/forecasts/${forecastId}/revenue-periods/bulk`, {
- method: 'PUT',
- headers: { 'Content-Type': 'application/json' },
- body: JSON.stringify(body),
- });
+ const itemTypeById = new Map(gridItems.map((item) => [item.id, item.streamType]));
+ const toBody = (values: PeriodValue[]) => ({
+ values: values.map((c) => ({
+ itemId: c.itemId,
+ period: c.period,
+ amount: c.amount.toFixed(2),
+ })),
+ });
+
+ const revenueChanges = changes.filter((c) => itemTypeById.get(c.itemId) === 'revenue');
+ const expenseChanges = changes.filter((c) => itemTypeById.get(c.itemId) === 'expense');
+
+ const requests: Promise<Response>[] = [];
+
+ if (revenueChanges.length > 0) {
+ requests.push(
+ fetch(`/api/forecasts/${forecastId}/revenue-periods/bulk`, {
+ method: 'PUT',
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify(toBody(revenueChanges)),
+ })
+ );
+ }
+
+ if (expenseChanges.length > 0) {
+ requests.push(
+ fetch(`/api/forecasts/${forecastId}/expense-periods/bulk`, {
+ method: 'PUT',
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify(toBody(expenseChanges)),
+ })
+ );
+ }
+
+ const responses = await Promise.all(requests);
+ if (responses.some((r) => !r.ok)) {
+ throw new Error('Failed to save forecast period values');
+ }
}📝 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.
Suggested change
| async function handleBulkSave(changes: PeriodValue[]) { | |
| const body = { values: changes.map((c) => ({ itemId: c.itemId, period: c.period, amount: c.amount.toFixed(2) })) }; | |
| await fetch(`/api/forecasts/${forecastId}/revenue-periods/bulk`, { | |
| method: 'PUT', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify(body), | |
| }); | |
| async function handleBulkSave(changes: PeriodValue[]) { | |
| const itemTypeById = new Map(gridItems.map((item) => [item.id, item.streamType])); | |
| const toBody = (values: PeriodValue[]) => ({ | |
| values: values.map((c) => ({ | |
| itemId: c.itemId, | |
| period: c.period, | |
| amount: c.amount.toFixed(2), | |
| })), | |
| }); | |
| const revenueChanges = changes.filter((c) => itemTypeById.get(c.itemId) === 'revenue'); | |
| const expenseChanges = changes.filter((c) => itemTypeById.get(c.itemId) === 'expense'); | |
| const requests: Promise<Response>[] = []; | |
| if (revenueChanges.length > 0) { | |
| requests.push( | |
| fetch(`/api/forecasts/${forecastId}/revenue-periods/bulk`, { | |
| method: 'PUT', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify(toBody(revenueChanges)), | |
| }) | |
| ); | |
| } | |
| if (expenseChanges.length > 0) { | |
| requests.push( | |
| fetch(`/api/forecasts/${forecastId}/expense-periods/bulk`, { | |
| method: 'PUT', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify(toBody(expenseChanges)), | |
| }) | |
| ); | |
| } | |
| const responses = await Promise.all(requests); | |
| if (responses.some((r) => !r.ok)) { | |
| throw new Error('Failed to save forecast period values'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/forecasts/ForecastEditorPage.tsx` around lines 202 - 208,
handleBulkSave currently sends all changes to the revenue endpoint causing
expense edits to be lost; update handleBulkSave (PeriodValue[] changes) to
partition changes by stream type (e.g., revenue vs expense) — either using a
streamType property on PeriodValue (c.streamType) or by looking up the item type
from the existing item map — then build separate request bodies and issue PUT
requests to the correct endpoints (e.g.,
`/api/forecasts/${forecastId}/revenue-periods/bulk` for revenue and
`/api/forecasts/${forecastId}/expense-periods/bulk` for expenses), only calling
each endpoint when its group is non-empty and awaiting the requests (Promise.all
if parallel).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces a full Forecasting & Budget Tracking module to Summit Finance, giving users the ability to plan multi-year P&L forecasts, compare budgets against actuals, and track on-track/at-risk/off-track status per line item.
What's Changed
Database (7 new tables + 2 enums)
New Drizzle schema additions in
src/lib/db/schema.ts:forecastsforecast_streamsforecast_itemsforecast_period_valuesitem_id + period)forecast_variablesforecast_growth_rulesforecast_seasonality_weightsNew enums:
forecast_stream_type(revenue|expense),forecast_scope_type(global|stream|item)Migration:
0018_striped_machine_man.sqlAPI Routes (
src/app/api/forecasts/)/api/forecasts/api/forecasts/[id]/api/forecasts/[id]/streams/api/forecasts/[id]/items/api/forecasts/[id]/revenue-periods/bulk/api/forecasts/[id]/expense-periods/bulk/api/forecasts/[id]/assumptions/api/forecasts/[id]/apply-assumptions/api/forecasts/[id]/summary/api/forecasts/[id]/metrics/api/forecasts/[id]/comparison/api/forecasts/[id]/tracking/api/forecasts/[id]/importCalculation Engine (
src/lib/forecasts/)assumptions.ts— Applies YoY growth rules (global → stream → item precedence) and distributes annual totals using seasonality weights viaonConflictDoUpdateupsertsummary.ts— Aggregates period values into annual P&L totals per streammetrics.ts— Derives YoY growth %, profit margins, burn rate, run ratecomparison.ts— Joins forecast budgets against realincome/expensesrecords filtered by linked category IDtracking.ts— Computes on-track status using thresholds (≥90% = on-track, 75–90% = at-risk, <75% = off-track), with inverted logic for expense items (under-spend = good)UI Pages (
src/app/(authenticated)/forecasts/)/forecasts/forecasts/new/forecasts/[id]/forecasts/[id]/comparison/forecasts/[id]/tracking/forecasts/[id]/importUI Components (
src/components/forecasts/)ForecastGrid— Inline-editable spreadsheet grid with pending-change tracking and bulk saveStreamTree— Collapsible sidebar tree of streams and items; click any item to assign income/expense categoryAssumptionsPanel— Dialog with tabs for growth rules (with stream/item scope picker), seasonality monthly weights, and named variablesVarianceCell— Colour-coded variance display with on-track badgeForecastComparisonPage— Multi-period comparison table usingReact.Fragmentkeyed columnsForecastTrackingPage— Month navigator (prev/next + pill shortcuts), summary status cards, detail table per item, totals footer rowNavigation
TrendingUpicon)pathname.startsWith(item.href)) so all forecast sub-pages highlight correctlyDocker / CI
docker-entrypoint.sh— waits for Postgres readiness (pg_isready), runspnpm pushto sync schema, then starts the app. SupportsSKIP_DB_SETUP=trueoverrideDockerfile— addedpostgresql-clientto runner stage, copies and uses entrypointrelease.yml—latestDocker tag now always publishes on anyv*.*.*tag (removedenable={{is_default_branch}}gate)Bug Fixes
paramsawaited in all dynamic route pages (Next.js 15),<>fragments replaced withReact.Fragment key={}in grid and comparison tables,<button>nesting inStreamTree, missing<tbody>in CSV preview tableHeader.tsxtheme toggle hydration mismatch — entireDropdownMenusuppressed untilmounted, eliminating Radix ID mismatch between SSR and clientrevenuetype — now infersexpensefrom stream name via/expense/iregexTest Plan
/forecasts/new→ streams and items visible in editorexpensev*.*.*tag → GitHub Actions builds Docker image and pusheslatest+ versioned tags to Docker Hubpnpm push, schema created, app starts🤖 Generated with Claude Code
Summary by CodeRabbit
New Features