feat: added finance-visualizer community demo, closes #1761 #1831
Conversation
HelloSniperMonkey
force-pushed
the
main
branch
from
6c1a61e to
407e2fa
Compare
|
hi @HelloSniperMonkey you need to sign your commits |
|
@sumitsaurabh927 do i need to sign all my commits ? because i just signed the last commit. |
📝 WalkthroughWalkthroughAdds a complete Personal Finance Visualizer demo under Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser as Client (React)
participant API as Next.js API
participant DB as MongoDB
User->>Browser: Open "Add Transaction" modal
Browser->>Browser: Validate form (zod/react-hook-form)
Browser->>API: POST /api/transactions (payload)
API->>API: dbConnect()
API->>DB: Transaction.create(payload)
DB-->>API: created document
API-->>Browser: 201 + created transaction
Browser->>API: GET /api/transactions
API->>DB: Transaction.find().sort({date:-1})
DB-->>API: transactions list
API-->>Browser: transactions
Browser->>Browser: Update state & re-render dashboard
sequenceDiagram
participant User
participant Browser as Client (React)
participant API as Next.js API
participant DB as MongoDB
User->>Browser: Select Month/Year and open BudgetForm
Browser->>API: GET /api/budgets?month=YYYY-MM&year=YYYY
API->>API: dbConnect()
API->>DB: Budget.find({ month: "YYYY-MM" })
DB-->>API: budgets
API-->>Browser: budgets
Browser->>Browser: Fill BudgetForm and submit
Browser->>API: POST /api/budgets (category, amount, month, year)
API->>API: dbConnect()
API->>DB: Budget.findOne({ category, month })
alt exists
API->>DB: update and save
else
API->>DB: Budget.create(...)
end
DB-->>API: updated/created budget
API-->>Browser: 200/201 + budget
Browser->>API: GET /api/budgets?month=YYYY-MM&year=YYYY
API->>DB: Budget.find(...)
DB-->>API: updated budgets list
API-->>Browser: budgets
Browser->>Browser: Recompute charts and UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![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
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@community/finance-visualizer/src/app/api/budgets/route.ts`:
- Around line 24-34: The POST handler uses month.padStart(2, '0') and can throw
if month/year are missing or not strings; add input validation at the start of
POST (before using month/year and before Budget.findOne) to ensure month and
year are present, convert month and year to strings or numbers, verify month is
an integer between 1 and 12 and year is a valid year (numeric), then produce a
400 response for invalid/missing inputs; when valid, format month safely via
String(month).padStart(2,'0') or by zero-padding a numeric month and continue to
use Budget.findOne and the rest of the logic.
In `@community/finance-visualizer/src/components/ui/button.tsx`:
- Around line 38-55: The Button component currently leaves the HTML button type
unspecified which causes buttons inside forms to default to type="submit";
modify the render so when using the native "button" element (Comp === "button")
you ensure a default type of "button" if no type prop was provided—i.e., when
rendering the native element include type={props.type ?? "button"} (but do not
force type when asChild/Slot is used), keeping all other props and className
logic intact in the Button function.
In `@community/finance-visualizer/src/components/ui/label.tsx`:
- Around line 8-21: The Label wrapper currently defines a plain function but
must forward refs to the Radix component: convert the Label function to use
React.forwardRef, accept (props: React.ComponentPropsWithoutRef<typeof
LabelPrimitive.Root>, ref: React.Ref<HTMLElement> or the appropriate element
type), pass the ref through to LabelPrimitive.Root (ref={ref}), and keep
existing className merging with cn and {...props}; ensure the exported name
remains Label and types mirror other Radix-wrapped components in the directory
for consistency with focus/measuring/ref-based integrations.
In `@community/finance-visualizer/src/lib/dbConnect.ts`:
- Around line 28-37: The cached promise from mongoose.connect is not cleared on
rejection, so update the dbConnect logic (the cached, cached.promise,
mongoose.connect and cached.conn usage) to reset cached.promise when the
connection attempt fails: attach a .catch handler to the
mongoose.connect(MONGODB_URI, opts) promise that sets cached.promise = null (or
deletes it) and rethrows the error so callers still receive the failure; ensure
cached.conn is only assigned after await cached.promise resolves successfully so
subsequent calls can retry connecting on transient failures.
In `@community/finance-visualizer/src/models/Budget.ts`:
- Around line 12-23: BudgetSchema currently stores month ("YYYY-MM") and year
separately which can diverge; add strict month-format validation and derive year
from month in a schema pre-validate hook: validate month matches /^\d{4}-\d{2}$/
(or use a validator function on the month path) and in
BudgetSchema.pre('validate') parse the year from month (e.g.,
parseInt(month.split('-')[0])) and assign it to the year field or throw a
ValidationError if month is malformed; keep the existing compound unique index
BudgetSchema.index({ category: 1, month: 1 }, ...) so the derived year stays
consistent with month for queries and reporting.
♻️ Duplicate comments (2)
community/finance-visualizer/src/app/api/transactions/route.ts (1)
5-6: Duplicate:dbConnect()should be inside thetryblock.Same concern as in the id-scoped transactions route.
Also applies to: 15-16
community/finance-visualizer/src/app/api/budgets/[id]/route.ts (1)
5-10: Duplicate:dbConnect()should be inside thetryblock.Same concern as in the transactions id-scoped route.
Also applies to: 26-31
🟡 Minor comments (16)
community/finance-visualizer/README.md-5-5 (1)
5-5: Clarify the "zero code changes" claim.The statement that Lingo.dev enables translation "with zero code changes to the application logic or UI components" is misleading. According to the PR objectives, this implementation adds a LanguageSwitcher component (a UI component) and middleware for locale routing (application logic).
Consider rephrasing to clarify that Lingo.dev eliminates the need for manual
t()function calls in existing components, while acknowledging that locale routing infrastructure (middleware, language switcher) still needs to be implemented.📝 Suggested clarification
-This demo showcases how **Lingo.dev** can translate an existing application into multiple languages (Spanish, French, German, Chinese, and Japanese) with **zero code changes** to the application logic or UI components. +This demo showcases how **Lingo.dev** can translate an existing application into multiple languages (Spanish, French, German, Chinese, and Japanese) without requiring manual `t()` function calls or modifications to existing UI components—only locale routing infrastructure needs to be added.community/finance-visualizer/src/components/CategoryPieChart.tsx-20-47 (1)
20-47: Guard against NaN percentages when totals are zero.If all category values are 0, the percentage calculation divides by 0 and renders
NaN%. Compute the total once and guard against zero.🛠️ Proposed fix
const categoryData: CategoryData[] = data.reduce((acc: CategoryData[], transaction: ITransaction) => { const existing = acc.find((item) => item.name === transaction.category); if (existing) { existing.value += transaction.amount; } else { acc.push({ name: transaction.category, value: transaction.amount, color: getCategoryColor(transaction.category), icon: getCategoryIcon(transaction.category), }); } return acc; }, []); + const total = categoryData.reduce((sum, item) => sum + item.value, 0); const CustomTooltip = ({ active, payload }: { active?: boolean; payload?: { payload: CategoryData }[] }) => { if (active && payload && payload.length) { const data = payload[0].payload; return ( <div className="bg-white dark:bg-gray-800 p-3 border rounded-lg shadow-lg"> <p className="flex items-center gap-2"> <span>{data.icon}</span> <span className="font-medium">{data.name}</span> </p> <p className="text-sm text-gray-600 dark:text-gray-400"> - {formatCurrency(data.value)} ({((data.value / categoryData.reduce((sum, item) => sum + item.value, 0)) * 100).toFixed(1)}%) + {formatCurrency(data.value)} ({total > 0 ? ((data.value / total) * 100).toFixed(1) : '0.0'}%) </p> </div> ); } return null; };community/finance-visualizer/src/app/api/transactions/route.ts-21-22 (1)
21-22: Avoid returning raw error objects to clients.Serializing and returning the full error can leak internal details. Prefer a sanitized message.
🛠️ Proposed fix
} catch (error) { - return NextResponse.json({ success: false, error }, { status: 400 }); + const message = error instanceof Error ? error.message : 'Unexpected error'; + return NextResponse.json({ success: false, error: message }, { status: 400 }); }community/finance-visualizer/src/app/api/budgets/route.ts-19-20 (1)
19-20: Avoid exposing raw error objects in API responses.Returning the raw
errorobject (Lines 20, 52) may leak sensitive information such as stack traces or database details to clients. Return a generic error message instead.Proposed fix
} catch (error) { - return NextResponse.json({ success: false, error }, { status: 400 }); + console.error('Budget operation failed:', error); + return NextResponse.json({ success: false, error: 'Operation failed' }, { status: 400 }); }Also applies to: 51-52
community/finance-visualizer/src/components/SpendingInsights.tsx-80-84 (1)
80-84: Potential runtime issue when no transactions exist for the selected month.When
currentSpendingis empty,Object.keys(currentSpending)[0]returnsundefined, causing the reduce to accesscurrentSpending[undefined]. This won't crash but will produce incorrect results.Proposed fix
// Find highest spending category - const highestSpendingCategory = Object.keys(currentSpending).reduce((max, category) => - currentSpending[category] > currentSpending[max] ? category : max, - Object.keys(currentSpending)[0] - ); + const spendingCategories = Object.keys(currentSpending); + const highestSpendingCategory = spendingCategories.length > 0 + ? spendingCategories.reduce((max, category) => + currentSpending[category] > currentSpending[max] ? category : max, + spendingCategories[0] + ) + : null;community/finance-visualizer/src/components/TransactionForm.tsx-102-102 (1)
102-102: Usevalueinstead ofdefaultValuefor controlled Select behavior.Using
defaultValueon the Select means it won't update if the form is reset or iftransactionprop changes. For proper controlled behavior with react-hook-form, usevalue:- <Select onValueChange={field.onChange} defaultValue={field.value} disabled={isLoading}> + <Select onValueChange={field.onChange} value={field.value} disabled={isLoading}>community/finance-visualizer/src/components/MonthlyExpensesChart.tsx-29-34 (1)
29-34: Fragile date parsing for sorting may produce incorrect order.Parsing the formatted month string (e.g., "Jan 2025") with
new Date()is unreliable across browsers and locales. Some environments may fail to parse this format, resulting inNaNand incorrect sorting.Consider storing the raw date or a sortable key alongside the formatted label:
Suggested approach
interface MonthlyData { month: string; total: number; + sortKey: number; // e.g., year * 12 + monthIndex } - const monthlyData: MonthlyData[] = data.reduce((acc: MonthlyData[], transaction: ITransaction) => { - const month = new Date(transaction.date).toLocaleString('default', { month: 'short', year: 'numeric' }); + const monthlyData: MonthlyData[] = data.reduce((acc: MonthlyData[], transaction: ITransaction) => { + const d = new Date(transaction.date); + const month = d.toLocaleString('default', { month: 'short', year: 'numeric' }); + const sortKey = d.getFullYear() * 12 + d.getMonth(); const existing = acc.find((item) => item.month === month); if (existing) { existing.total += transaction.amount; } else { - acc.push({ month, total: transaction.amount }); + acc.push({ month, total: transaction.amount, sortKey }); } return acc; }, []); const sortedMonthlyData = monthlyData.sort((a, b) => { - const dateA = new Date(a.month); - const dateB = new Date(b.month); - return dateA.getTime() - dateB.getTime(); + return a.sortKey - b.sortKey; });community/finance-visualizer/src/components/ui/form.tsx-45-54 (1)
45-54: Context validation check is ineffective.The check
if (!fieldContext)will never throw because the context default value is{}(line 28-30), which is truthy. The error will never be thrown even whenuseFormFieldis called outside a<FormField>.Suggested fix
const useFormField = () => { const fieldContext = React.useContext(FormFieldContext) const itemContext = React.useContext(FormItemContext) const { getFieldState } = useFormContext() const formState = useFormState({ name: fieldContext.name }) const fieldState = getFieldState(fieldContext.name, formState) - if (!fieldContext) { + if (!fieldContext.name) { throw new Error("useFormField should be used within <FormField>") }community/finance-visualizer/src/components/DashboardSummary.tsx-47-50 (1)
47-50: Icon spans are empty and won't display any content.The spans intended for icons have a className from
iconsprop but no content inside them. If these are meant to use Bootstrap Icons or similar icon fonts, consider adding an<i>tag or the icon component:- <span className={`text-2xl ${icons?.totalExpenses || ''}`}></span> + <span className={`text-2xl ${icons?.totalExpenses || ''}`}> + {icons?.totalExpenses && <i className={icons.totalExpenses} aria-hidden="true" />} + </span>Alternatively, if the
iconsprop is intended to pass icon class names directly to be applied, the current pattern won't render visible icons since there's no icon element or content.community/finance-visualizer/src/components/TransactionList.tsx-21-24 (1)
21-24: Handle DELETE failures to avoid stale UI state.Right now the list refreshes even if the DELETE fails, and failures are silent. Consider checking
res.okand handling errors before refreshing.💡 Suggested fix
- const handleDelete = async (id: string) => { - await fetch(`/api/transactions/${id}`, { method: 'DELETE' }); - fetchTransactions(); - }; + const handleDelete = async (id: string) => { + try { + const res = await fetch(`/api/transactions/${id}`, { method: 'DELETE' }); + if (!res.ok) throw new Error('Failed to delete transaction'); + fetchTransactions(); + } catch (err) { + console.error(err); + } + };community/finance-visualizer/src/components/BudgetForm.tsx-238-246 (1)
238-246: Render category icons instead of the raw class name.
category?.iconis a class string (e.g.,bi-fork-knife), so the UI will display the text instead of an icon.💡 Suggested fix
- <span className="text-xl">{category?.icon}</span> + <i className={`bi ${category?.icon ?? ''} text-xl`} aria-hidden="true" />community/finance-visualizer/src/app/lingo/cache/es.json-9-10 (1)
9-10: Fix Spanish pluralization for “transacción”.The current pattern produces “transacciónes”. Use full ICU plural forms instead.
💡 Suggested fix
- "9eb2defc9ced": "{expression0} transacción{expression1, plural, one {} other {es}}", + "9eb2defc9ced": "{expression0} {expression1, plural, one {transacción} other {transacciones}}",community/finance-visualizer/src/app/lingo/cache/en.json-8-10 (1)
8-10: Fix English pluralization for “transaction”.The ICU pattern currently outputs “transactionone”. Use full plural forms for clarity.
💡 Suggested fix
- "9eb2defc9ced": "{expression0} transaction{expression1, plural, one {one} other {s}}", + "9eb2defc9ced": "{expression0} {expression1, plural, one {transaction} other {transactions}}",community/finance-visualizer/src/components/LanguageSwitcher.tsx-13-20 (1)
13-20: Add missing translations for de/zh/ja or remove unsupported languages from the UI.The UI offers
de/zh/ja, and the middleware/config correctly support these locales. However, the lingo translation cache only containsen.json,es.json, andfr.json. Users selecting German, Chinese, or Japanese will see untranslated content. Either add translation files for these languages tosrc/app/lingo/cache/or removede,zh, andjafrom the language list.community/finance-visualizer/src/app/layout.tsx-28-36 (1)
28-36: Use dynamiclangattribute derived from locale cookie set by middleware.The app routes requests to
/es,/fr, and other locales, with middleware setting thelocalecookie. However,<html lang="en">remains hardcoded, causing accessibility and SEO issues. Sincelayout.tsxis a Server Component, use thecookies()helper fromnext/serverto read the locale and update thelangattribute dynamically.community/finance-visualizer/src/app/page.tsx-83-85 (1)
83-85: Use the Lingo i18n system to localize UI text strings.The app has a fully configured Lingo i18n system with
LingoProviderand locale files (en/es/fr) containing translations for all UI strings, butpage.tsxhardcodes English text instead of using the localization system. This breaks the language-switching demo: theLanguageSwitchercomponent will exist but won't affect the UI text. Wrap hardcoded strings like "Personal Finance Visualizer" (line 83), "Track your expenses..." (line 118), "Budget Management" (line 144), and others in i18n calls to use the existing translations.Also applies to: 118-120, 144-146
🧹 Nitpick comments (23)
community/finance-visualizer/README.md (3)
9-9: Use precise language count.The phrase "5+ languages" is vague. The application supports exactly 6 languages (English source + 5 target locales: ES, FR, DE, ZH, JA), as shown in line 10 and the configuration example.
✏️ Suggested clarification
-- **Multi-language Support**: Automatically translated into 5+ languages using Lingo.dev. +- **Multi-language Support**: Automatically translated into 6 languages using Lingo.dev.
64-67: Add language specifier to fenced code block.The fenced code block for environment variables should specify a language for proper syntax highlighting.
As per static analysis hints, add language specifier.
🔧 Suggested fix
- ``` + ```env MONGODB_URI=mongodb+srv://<username>:<password>@<cluster-url>/personal-finance?retryWrites=true LINGO_API_KEY=your_lingo_api_key_here ```
80-109: Add language specifier to fenced code block.The fenced code block for the project structure should specify a language (e.g.,
textorplaintext) for proper syntax highlighting.As per static analysis hints, add language specifier.
🔧 Suggested fix
-``` +```text src/ ├── app/ │ ├── api/community/finance-visualizer/src/lib/currency.ts (1)
1-18: Parameterize locale/currency to align with locale switching.Line 3 hard-codes
en-IN/INR. Since the app supports multiple locales, consider passing locale/currency (with defaults) so UI formatting can align with the active locale and any future currency preferences.♻️ Suggested refactor (backward compatible defaults)
-export const formatCurrency = (amount: number): string => { - return new Intl.NumberFormat('en-IN', { +export const formatCurrency = ( + amount: number, + locale: string = 'en-IN', + currency: string = 'INR' +): string => { + return new Intl.NumberFormat(locale, { style: 'currency', - currency: 'INR', + currency, minimumFractionDigits: 0, maximumFractionDigits: 0, }).format(amount); }; // Format currency for tooltips and displays where decimals might be needed -export const formatCurrencyDetailed = (amount: number): string => { - return new Intl.NumberFormat('en-IN', { +export const formatCurrencyDetailed = ( + amount: number, + locale: string = 'en-IN', + currency: string = 'INR' +): string => { + return new Intl.NumberFormat(locale, { style: 'currency', - currency: 'INR', + currency, minimumFractionDigits: 2, maximumFractionDigits: 2, }).format(amount); };community/finance-visualizer/src/lib/categories.ts (1)
10-21: Make category labels localizable (avoid hardcoded English).If
labelis rendered directly,/esand/frviews will still show English. Consider using translation keys (or a stable slugvalueplus a localized label) so categories can be translated consistently.community/finance-visualizer/src/app/api/transactions/[id]/route.ts (1)
5-10: MovedbConnect()inside thetryblock for consistent JSON error handling.If the connection fails, it currently bypasses the
catchand returns a framework-level error instead of your JSON payload.♻️ Suggested change
export async function PUT( request: Request, { params }: { params: Promise<{ id: string }> } ) { - await dbConnect(); try { + await dbConnect(); const body = await request.json(); const { id } = await params; const transaction = await Transaction.findByIdAndUpdate(id, body, { new: true, runValidators: true, }); @@ export async function DELETE( request: Request, { params }: { params: Promise<{ id: string }> } ) { - await dbConnect(); try { + await dbConnect(); const { id } = await params; const deletedTransaction = await Transaction.deleteOne({ _id: id }); if (deletedTransaction.deletedCount === 0) { return NextResponse.json({ success: false }, { status: 404 }); }Also applies to: 26-31
community/finance-visualizer/src/app/api/budgets/route.ts (1)
5-7: Consider wrappingdbConnect()in try-catch for consistent error handling.If
dbConnect()throws, the error won't be caught and will result in an unhandled rejection. This matches the pattern in the transactions route, but moving it inside the try block would provide more robust error handling.Also applies to: 24-26
community/finance-visualizer/src/components/SpendingInsights.tsx (1)
29-45: Consider extracting transaction filtering logic to a shared utility.This filtering pattern (by month/year) is duplicated in
BudgetComparisonChart.tsx. For maintainability, consider extracting to a shared helper function.community/finance-visualizer/src/components/BudgetComparisonChart.tsx (2)
75-87: CustomTooltip is recreated on every render and could display more useful information.The tooltip component is defined inside the render function, causing unnecessary re-creation. Additionally, it only shows the category name - consider showing budget and actual values for better UX.
Proposed enhancement
+// Move outside component or memoize +const CustomTooltip = ({ active, payload }: { active?: boolean; payload?: { payload: BudgetComparisonData }[]; label?: string }) => { + if (active && payload && payload.length) { + const data = payload[0].payload; + return ( + <div className="bg-white dark:bg-gray-800 p-3 border rounded-lg shadow-lg"> + <p className="font-medium mb-2">{data.category}</p> + <p className="text-sm">Budget: {formatCurrency(data.budget)}</p> + <p className="text-sm">Actual: {formatCurrency(data.actual)}</p> + <p className="text-sm">Usage: {data.percentage.toFixed(1)}%</p> + </div> + ); + } + return null; +};
89-92:monthNamesarray is duplicated across components.This constant is also defined in
SpendingInsights.tsx. Consider extracting to a shared constants file for better maintainability.community/finance-visualizer/src/components/MonthlyExpensesChart.tsx (1)
47-47: Tooltip formatter type may not match Recharts signature.The Recharts
Tooltipformatter callback receives(value, name, props)wherevaluecan benumber | string | Array. The current type annotationnumber | undefinedis narrow. Consider using the library's expected types or a broader type to avoid potential runtime issues.community/finance-visualizer/src/components/DashboardSummary.tsx (2)
138-138: Avoid double type casting for_id.The
transaction._id as unknown as stringcast is a type workaround. SinceITransactionextends MongooseDocument, the_idfield is typicallyTypes.ObjectId. UsingString()or.toString()is cleaner:- <div key={transaction._id as unknown as string} className="flex items-center justify-between"> + <div key={String(transaction._id)} className="flex items-center justify-between">
34-40: Locale-dependent month comparison may be inconsistent.Using
toLocaleString('default', ...)for string comparison can produce different results across environments. For reliable filtering, compare year and month numbers directly:- const currentMonth = new Date().toLocaleString('default', { month: 'long', year: 'numeric' }); - const currentMonthExpenses = transactions - .filter(transaction => { - const transactionMonth = new Date(transaction.date).toLocaleString('default', { month: 'long', year: 'numeric' }); - return transactionMonth === currentMonth; - }) - .reduce((sum, transaction) => sum + transaction.amount, 0); + const now = new Date(); + const currentYear = now.getFullYear(); + const currentMonthNum = now.getMonth(); + const currentMonthExpenses = transactions + .filter(transaction => { + const d = new Date(transaction.date); + return d.getFullYear() === currentYear && d.getMonth() === currentMonthNum; + }) + .reduce((sum, transaction) => sum + transaction.amount, 0); + const currentMonth = now.toLocaleString('default', { month: 'long', year: 'numeric' });community/finance-visualizer/src/components/TransactionForm.tsx (2)
73-81: Consider using react-hook-form's built-in types for render props.The explicit field type annotations are verbose. You can omit them and let TypeScript infer the types from
FormField's generic parameters, or useControllerRenderProps:- render={({ field }: { field: { onChange: (event: React.ChangeEvent<HTMLInputElement>) => void; value: string } }) => ( + render={({ field }) => (TypeScript will infer the correct field type from the
nameprop and the form schema.
49-51: Consider parsing error details from the API response.Currently, any non-ok response throws a generic "Failed to save transaction" error. If the API returns validation errors or specific messages, parsing them would provide better user feedback:
if (!res.ok) { - throw new Error('Failed to save transaction'); + const errorData = await res.json().catch(() => ({})); + throw new Error(errorData.message || 'Failed to save transaction'); }community/finance-visualizer/src/components/LanguageSwitcher.tsx (1)
33-60: Prefer Next.js router navigation over full reloads.Using
window.location.hrefforces a full page reload. ConsideruseRouter().push()for a smoother client transition.♻️ Suggested refactor
import * as React from "react" +import { useRouter } from "next/navigation" @@ export function LanguageSwitcher() { const [value, setValue] = React.useState("en") + const router = useRouter() @@ - window.location.href = newPath + router.push(newPath) }community/finance-visualizer/src/middleware.ts (1)
36-46: Redundant condition check.At line 42, if execution reaches this block,
localeis already falsy (undefined), meaning no locale prefix was matched in lines 10-12. The check!locales.some(l => pathname.startsWith(/${l}))will therefore always be true here, making it redundant alongsidepathname === '/'.Suggested simplification
- // For default locale (root path), ensure locale is set to 'en' (or default) - // to avoid stuck cookies from previous visits - const response = NextResponse.next(); - // Optional: Force 'en' cookie if at root? - // If we don't, visiting / after /es might still show Spanish if cookie persists. - // Our LanguageSwitcher assumes / is English. - if (pathname === '/' || !locales.some(l => pathname.startsWith(`/${l}`))) { - response.cookies.set("locale", "en"); - } - - return response; + // For paths without locale prefix, default to 'en' + // This prevents stale cookies from previous locale visits + const response = NextResponse.next(); + response.cookies.set("locale", "en"); + return response;community/finance-visualizer/src/app/page.tsx (6)
69-75: Redundant dependencies in useEffect.Since
fetchBudgetsis memoized withuseCallbackdepending on[selectedMonth, selectedYear], including all three in the dependency array at line 75 is redundant. WhenselectedMonthorselectedYearchanges,fetchBudgetsreference updates automatically, triggering the effect.Suggested simplification
useEffect(() => { fetchBudgets(); - }, [selectedMonth, selectedYear, fetchBudgets]); + }, [fetchBudgets]);
154-163: Minor: Hardcoded year in month name generation.The hardcoded
2024doesn't affect month name output, but using a constant or the current year would be cleaner.Suggested fix
- const monthName = new Date(2024, i).toLocaleString('default', { month: 'long' }); + const monthName = new Date(2000, i).toLocaleString('default', { month: 'long' });
165-165: Add radix to parseInt.While the value from the controlled select is safe, specifying the radix is a best practice to avoid edge cases.
Suggested fix
- <Select value={selectedYear.toString()} onValueChange={(value) => setSelectedYear(parseInt(value))}> + <Select value={selectedYear.toString()} onValueChange={(value) => setSelectedYear(parseInt(value, 10))}>
97-115: Duplicate Add Transaction Dialog.The Add Transaction dialog is duplicated in both the header (lines 97-115) and the transactions tab (lines 239-254). Consider extracting this into a reusable component or rendering it once at a shared location.
Also applies to: 239-254
38-67: Inconsistent error handling between fetch functions.
fetchTransactionssets error state for UI display (line 50), whilefetchBudgetsonly logs to console (line 65). Consider consistent error handling for both, or clarify why budgets errors should be silent.
28-36: Consider adding loading state.There's no loading indicator while transactions or budgets are being fetched. Users may not know data is loading, especially on slower connections.
Example addition
const [activeTab, setActiveTab] = useState("overview"); + const [isLoading, setIsLoading] = useState(true);Then wrap fetch calls with
setIsLoading(true/false)and show a loading indicator in the UI.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@community/finance-visualizer/src/app/api/budgets/route.ts`:
- Around line 5-22: The GET handler currently allows partial filters and returns
all budgets when only month or year is supplied; update the GET function to
validate the query params: after extracting month and year from searchParams, if
exactly one of month or year is present (i.e., one truthy and the other falsy)
return a 400 NextResponse.json with a clear error message, otherwise proceed as
before (no change when both are absent, and build the month string with
year-month.padStart(2,'0') when both are present) so Budget.find(query) only
runs for valid inputs.
♻️ Duplicate comments (2)
community/finance-visualizer/src/components/ui/button.tsx (1)
50-55: Defaulttype="button"is still overridden by the later spread.Because
{...props}comes after the default,props.type(oftenundefined) overwrites the default and the element falls back to the HTML default"submit". This keeps the original form‑submit risk.🛠️ Proposed fix
function Button({ className, variant, size, asChild = false, ...props }: React.ComponentProps<"button"> & VariantProps<typeof buttonVariants> & { asChild?: boolean }) { const Comp = asChild ? Slot : "button" + const { type, ...rest } = props return ( <Comp data-slot="button" className={cn(buttonVariants({ variant, size, className }))} - {...(Comp === "button" ? { type: props.type ?? "button" } : {})} - {...props} + type={asChild ? undefined : (type ?? "button")} + {...rest} /> ) }community/finance-visualizer/src/models/Budget.ts (1)
15-23: Tighten month validation to reject invalid months (00/13).The current regex allows values like
2024-00and2024-13. Consider constraining to01–12and mirror the same pattern in the pre-validate check (Line 34) to keep them consistent.🛠️ Suggested fix
- return /^\d{4}-\d{2}$/.test(v); + return /^\d{4}-(0[1-9]|1[0-2])$/.test(v);Please verify locally that
2024-13is rejected after tightening the regex.
🧹 Nitpick comments (1)
community/finance-visualizer/src/app/api/budgets/route.ts (1)
24-81: Consider atomic upsert to avoid race conditions on the unique index.
findOne+createcan race under concurrent requests and surface duplicate-key errors. AfindOneAndUpdateupsert keeps this atomic.♻️ Suggested refactor
- // Check if budget already exists for this category and month - const existingBudget = await Budget.findOne({ - category, - month: formattedMonth - }); - - if (existingBudget) { - // Update existing budget - existingBudget.amount = amount; - await existingBudget.save(); - return NextResponse.json({ success: true, data: existingBudget }); - } else { - // Create new budget - const budget = await Budget.create({ - category, - amount, - month: formattedMonth, - year: yearNum, - }); - return NextResponse.json({ success: true, data: budget }, { status: 201 }); - } + const budget = await Budget.findOneAndUpdate( + { category, month: formattedMonth }, + { + $set: { amount }, + $setOnInsert: { category, month: formattedMonth, year: yearNum }, + }, + { new: true, upsert: true, runValidators: true } + ); + return NextResponse.json({ success: true, data: budget });
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@community/finance-visualizer/src/app/api/budgets/`[id]/route.ts:
- Around line 11-30: The PUT handler currently passes the request body directly
into Budget.findByIdAndUpdate without validating fields; extract the validation
used in the budgets POST handler (month, year, amount checks) into a shared
validator (e.g., validateBudgetPayload) and call it at the top of the exported
PUT function before calling Budget.findByIdAndUpdate; if validation fails return
a 400 JSON response with the validation errors, otherwise proceed to update
(using the validated/sanitized payload) and keep existing error handling around
dbConnect/Budget.findByIdAndUpdate.
In `@community/finance-visualizer/src/app/api/budgets/route.ts`:
- Around line 49-106: The POST handler in function POST is missing validation
for category and amount; add checks after parsing body and before
formattedMonth: ensure category is present and a non-empty string (trim and
reject empty) and ensure amount is present and a finite number > 0 (if amount is
a string parseFloat and validate with isFinite and > 0); on validation failure
return NextResponse.json with a 400 status and a clear error message; use the
validated/parsed amount variable when assigning existingBudget.amount and when
calling Budget.create so you never store an invalid value (references: POST,
Budget.findOne, existingBudget, Budget.create).
In `@community/finance-visualizer/src/app/api/transactions/`[id]/route.ts:
- Around line 11-30: The PUT handler (function PUT) currently passes the raw
request body into Transaction.findByIdAndUpdate, allowing missing required
fields and mass-assignment; validate and sanitize the input first: parse the
body, run it through a schema validator (e.g., Zod or a Joi schema) or at
minimum pick/whitelist only allowed fields (e.g., const allowed =
['amount','date','description','category']; const payload = pick(body,
allowed)), check required fields and types, return a 422/400 with validation
errors if invalid, then call Transaction.findByIdAndUpdate(id, payload, { new:
true, runValidators: true }) so Mongoose validation and mass-assignment are
prevented.
In `@community/finance-visualizer/src/app/api/transactions/route.ts`:
- Around line 25-33: The POST handler currently forwards request.json() directly
into Transaction.create, allowing malformed or unexpected fields; add explicit
server-side validation before calling Transaction.create: implement a
validateTransactionBody(body) (or inline checks in POST) that ensures required
fields amount, date, description, and category exist, amount is a positive
number, date is a valid ISO/date parseable value, and description/category are
non-empty strings; sanitize the body to only allowed properties (whitelist) to
prevent mass assignment, and return a clear NextResponse.json({ success: false,
error: "..." }, { status: 400 }) when validation fails, otherwise proceed to
dbConnect() and Transaction.create with the sanitized object.
In `@community/finance-visualizer/src/components/BudgetForm.tsx`:
- Around line 48-71: fetchBudgets is being called twice on mount because one
useEffect calls it unconditionally and the other calls it when
watchMonth/watchYear are truthy (they are on mount); also fetchBudgets parses
JSON without checking res.ok so data may be undefined on HTTP errors. Fix by
removing the unconditional mount useEffect and only calling fetchBudgets from
the effect that watches watchMonth and watchYear (or add an initialized flag to
that effect to avoid double-run), and update fetchBudgets to check res.ok before
calling res.json()—on non-ok, log the status/error and setExistingBudgets([]) or
return early to avoid parsing undefined data. Ensure you modify the
functions/useEffects named fetchBudgets, the two useEffect blocks, and the
setExistingBudgets usage accordingly.
In `@community/finance-visualizer/src/components/TransactionList.tsx`:
- Around line 27-30: The handleDelete function lacks error handling and always
calls fetchTransactions; wrap the deletion in a try/catch, await the
fetch(`/api/transactions/${id}`, { method: 'DELETE' }) response, check
response.ok (and optionally parse error body) and only call fetchTransactions()
on success; on failure (non-ok status or caught exception) report the error to
the user (e.g., via an alert or error state) and avoid refreshing the list, and
ensure handleDelete returns or rethrows appropriately so calling code can react.
In `@community/finance-visualizer/src/components/ui/form.tsx`:
- Around line 28-30: The contexts FormFieldContext and FormItemContext are
created with {} defaults so truthy checks like if (!fieldContext) never fire;
change both context types to allow null (e.g., FormFieldContextValue | null and
FormItemContextValue | null), initialize them with React.createContext(...,
null), and update consumer guards in hooks/components that reference
FormFieldContext and FormItemContext (e.g., any useFormField/useFormItem or
consumers of formFieldId/formItemId) to explicitly check for null (fieldContext
=== null) and throw the existing "must be used within ..." errors to restore the
guard and avoid undefined IDs.
In `@community/finance-visualizer/src/middleware.ts`:
- Around line 16-50: The cookie-reset branch uses a weaker check locales.some(l
=> pathname.startsWith(`/${l}`)) that creates false positives like "/enroll";
change it to reuse the exact locale-detection logic used earlier (the matcher
used in locales.find: (l) => pathname.startsWith(`/${l}/`) || pathname ===
`/${l`) ), or extract that logic into a helper (e.g., isLocalePath(pathname, l))
and call it from both places so the response.cookies.set("locale", "en") only
runs when the path is truly non-locale (or when pathname === '/').
🧹 Nitpick comments (9)
community/finance-visualizer/src/components/DashboardSummary.tsx (1)
40-46: Locale-dependent date comparison may cause inconsistent filtering.Using
toLocaleString('default', ...)for date comparison is fragile because the output format can vary across different environments/runtimes. This could cause transactions to be incorrectly included or excluded from the current month calculation.♻️ Suggested fix using numeric comparison
- const currentMonth = new Date().toLocaleString('default', { month: 'long', year: 'numeric' }); - const currentMonthExpenses = transactions - .filter(transaction => { - const transactionMonth = new Date(transaction.date).toLocaleString('default', { month: 'long', year: 'numeric' }); - return transactionMonth === currentMonth; - }) - .reduce((sum, transaction) => sum + transaction.amount, 0); + const now = new Date(); + const currentMonthExpenses = transactions + .filter(transaction => { + const d = new Date(transaction.date); + return d.getMonth() === now.getMonth() && d.getFullYear() === now.getFullYear(); + }) + .reduce((sum, transaction) => sum + transaction.amount, 0); + const currentMonth = now.toLocaleString('default', { month: 'long', year: 'numeric' });community/finance-visualizer/src/app/api/transactions/route.ts (1)
10-18: Inconsistent error handling in GET handler.The catch block returns
{ success: false }without the error details, while the POST handler includeserrorin the response. Consider including error information for debugging consistency, or at minimum log the error server-side.♻️ Suggested fix
- } catch { - return NextResponse.json({ success: false }, { status: 400 }); + } catch (error) { + return NextResponse.json({ success: false, error }, { status: 400 }); }community/finance-visualizer/src/components/TransactionList.tsx (1)
86-93: Non-null assertion onselectedTransactionis safe but could be clearer.The
selectedTransaction!assertion on line 87 is protected by theopencondition on line 69, so it won't be null when rendered. However, consider adding a guard or restructuring to avoid the assertion.♻️ Optional: Add explicit guard
<DialogContent> <DialogHeader> <DialogTitle>Edit Transaction</DialogTitle> </DialogHeader> + {selectedTransaction && ( <TransactionForm - transaction={selectedTransaction!} + transaction={selectedTransaction} onSuccess={() => { fetchTransactions(); setIsEditDialogOpen(false); setSelectedTransaction(null); }} /> + )} </DialogContent>community/finance-visualizer/src/app/api/budgets/route.ts (1)
37-38: Avoid returning raw error objects in API responses.Returning
{ success: false, error }can serialize to{}forErrorinstances (sinceErrorproperties aren't enumerable) or leak internal details. Extract a message string instead.Proposed fix
} catch (error) { - return NextResponse.json({ success: false, error }, { status: 400 }); + const message = error instanceof Error ? error.message : 'An error occurred'; + return NextResponse.json({ success: false, error: message }, { status: 400 }); }Apply this pattern to both the GET handler (line 38) and POST handler (line 104).
Also applies to: 103-104
community/finance-visualizer/src/components/BudgetForm.tsx (1)
36-36: Consider a client-specific type instead ofIBudgetfrom Mongoose model.
IBudgetextends Mongoose'sDocument, which includes server-side methods and_idasObjectId. On the client,_idis serialized as a string, leading to awkward casts likebudget._id as unknown as string. A lean client type would be cleaner.Example client type
// In a shared types file, e.g., `@/types/budget.ts` export interface BudgetData { _id: string; category: string; amount: number; month: string; year: number; }Then use
BudgetData[]instead ofIBudget[]for client-side state.Also applies to: 243-263
community/finance-visualizer/src/lib/dbConnect.ts (1)
1-21: Minor: Non-null assertion on line 3 is safe but could be cleaner.The
!onprocess.env.MONGODB_URI!is technically fine since the runtime check on lines 5-9 will throw before it's used. However, declaring without assertion and letting the check guard usage would be more idiomatic.Alternative pattern
-const MONGODB_URI = process.env.MONGODB_URI!; +const MONGODB_URI = process.env.MONGODB_URI; if (!MONGODB_URI) { throw new Error( 'Please define the MONGODB_URI environment variable inside .env.local' ); }TypeScript will still narrow the type after the check in the function body.
community/finance-visualizer/src/app/page.tsx (3)
43-57: Consider memoizingfetchTransactionswithuseCallback.
fetchTransactionsis passed toTransactionList(line 275) as a prop. Since it's recreated on every render, this could cause unnecessary re-renders of child components. Wrapping it inuseCallbackwould provide a stable reference.Proposed change
- const fetchTransactions = async () => { + const fetchTransactions = useCallback(async () => { try { setError(null); const res = await fetch("/api/transactions"); if (!res.ok) { throw new Error('Failed to fetch transactions'); } const { data } = await res.json(); setTransactions(data); } catch (err) { setError(err instanceof Error ? err.message : 'An error occurred'); } - }; + }, []);
78-80: Redundant dependencies in useEffect.
fetchBudgetsalready hasselectedMonthandselectedYearin its dependency array (line 72). Including them again in this useEffect's deps (line 80) is redundant—onlyfetchBudgetsis needed.useEffect(() => { fetchBudgets(); - }, [selectedMonth, selectedYear, fetchBudgets]); + }, [fetchBudgets]);
102-120: Duplicate Dialog definitions share state—consider extracting to a component.The "Add Transaction" dialog is defined twice (header and transactions tab), both bound to
isDialogOpen. This works but duplicates JSX. Extracting to a reusable component or rendering once at a higher level would reduce duplication.Also applies to: 244-259
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@community/finance-visualizer/src/app/api/budgets/route.ts`:
- Around line 38-40: The catch blocks currently return the raw error object via
NextResponse.json({ success: false, error }, { status: 400 }) which can
mis-serialize and leak internals; change those responses (both occurrences
around the existing catch and the one at 89-91) to return a safe payload such as
{ success: false, error: error instanceof Error ? error.message : String(error)
} (optionally include a code field if available) and ensure you do not include
error.stack or the whole error object when calling NextResponse.json.
🧹 Nitpick comments (3)
community/finance-visualizer/src/lib/validators.ts (1)
207-224: Utility function appears unused.
pickAllowedFieldsis exported but not utilized in the route handlers sincevalidateTransactionPayloadalready handles field whitelisting internally. Consider removing it if not needed elsewhere, or document its intended use case.community/finance-visualizer/src/app/api/transactions/[id]/route.ts (1)
50-51: Consider logging errors for debugging.The catch blocks discard error details, which can make debugging production issues difficult. Consider logging the error before returning the response.
♻️ Suggested improvement
} catch (error) { + console.error('Transaction update failed:', error); return NextResponse.json({ success: false }, { status: 400 }); }Also applies to: 73-74
community/finance-visualizer/src/app/api/budgets/[id]/route.ts (1)
38-41: Partial month/year updates are silently ignored.If a user provides only
monthor onlyyearin the update payload, the value passes validation but is silently excluded from the update (lines 38-41 require both). This could confuse API consumers expecting partial updates.Consider either rejecting requests with only one of month/year, or documenting this behavior clearly.
♻️ Suggested fix to reject partial month/year
// Include month/year if they were validated - if (validation.data!.month !== undefined && validation.data!.year !== undefined) { + const hasMonth = validation.data!.month !== undefined; + const hasYear = validation.data!.year !== undefined; + + if (hasMonth !== hasYear) { + return NextResponse.json( + { success: false, error: 'Both month and year must be provided together' }, + { status: 400 } + ); + } + + if (hasMonth && hasYear) { updatePayload.month = `${validation.data!.year}-${String(validation.data!.month).padStart(2, '0')}`; updatePayload.year = validation.data!.year; }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@community/finance-visualizer/src/app/api/budgets/route.ts`:
- Around line 16-33: Validate the month and year query params before building
the query: ensure month and year are parsed as integers (use the existing month
and year variables), check month is between 1 and 12 and year is a reasonable
numeric year (e.g., four digits or a min/max range), and if validation fails
return the same 400 NextResponse used for missing pairs; only when both values
are present and valid build the query using `${year}-${month.padStart(2,'0')}`.
Reference the month/year vars, hasMonth/hasYear checks, the query variable, and
the padStart usage when locating where to add parsing and range checks.
- Around line 69-88: Replace the non-atomic findOne → save/create flow for
Budget with a single atomic findOneAndUpdate({ category, month: formattedMonth
}, { $set: { amount, year: yearNum, category, month: formattedMonth } }, {
upsert: true, new: true, rawResult: true }) call so concurrent requests won't
race; locate the block that uses Budget.findOne, existingBudget.save(), and
Budget.create(...) and swap it for the upsert call, then inspect the rawResult
to decide whether to return status 200 or 201 and return NextResponse.json({
success: true, data: doc }, { status }) accordingly, and keep handling
E11000/other DB errors in your catch path.
|
Hey @HelloSniperMonkey! Just checking in - are you still working on this PR? We noticed there are some comments that may need addressing. If you need more time, no problem! Just let us know. If we don't hear back within a week, we'll close this to keep the repo tidy, but you can always reopen when ready. |
2 participants
Summary
Introduces the finance-visualizer community example, a full-stack Next.js application for tracking finances with lingo.dev internationalization integration, enabling multi-language support (English, Spanish, French).
Changes
Testing
Business logic tests added:
Visuals
Previously:
Required for UI/UX changes:
Checklist
Closes #1761
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.