Skip to content

feat(presupuestos): monthly budget management + semantic HTML enforcement#7

Open
DataDave-Dev wants to merge 4 commits into
mainfrom
feat/budget-management
Open

feat(presupuestos): monthly budget management + semantic HTML enforcement#7
DataDave-Dev wants to merge 4 commits into
mainfrom
feat/budget-management

Conversation

@DataDave-Dev

@DataDave-Dev DataDave-Dev commented Jul 22, 2026

Copy link
Copy Markdown
Owner

Summary

  • Budget management: monthly spending limits per category — budget_limits table (migration 0007), budgets service (list/upsert/remove, spent aggregation, copy from previous month), API routes under /api/presupuestos, and the presupuestos view with month navigation, consumption bars, limit editor and copy dialog. Wizard budget step now materializes limits.
  • Semantic HTML: repo-wide markup convention (openspec adopt-semantic-html) — jsx-a11y rules as errors, single h1 owned by shell header, corrected heading hierarchy in wizard and views, ul/li movement feed, accessible modal backdrops, keyboard-focus sidebar expansion.
  • Lint cleanup: ignore .agents/** templates, allow _-prefixed unused bindings, drop dead OAuth helper.

Test plan

  • pnpm lint — 0 errors, 0 warnings
  • pnpm typecheck — clean
  • pnpm test:unit — 191/191 passing (includes budget domain/service/handler tests)
  • Manual: create/edit/remove limits, copy previous month, month navigation
  • Manual: verify wizard budget step seeds limits

Summary by CodeRabbit

  • New Features
    • Added monthly budget management with month navigation, category spending limits, progress tracking, and over-budget indicators.
    • Added options to copy limits from the previous month, create or update limits, and remove limits.
    • Added authenticated budget data loading and API support.
  • Bug Fixes
    • Improved budget period handling and prevented duplicate budgets for the same month.
    • Corrected budget calculations across currencies, cancelled transactions, and date boundaries.
  • Accessibility
    • Improved heading hierarchy, screen-reader landmarks, semantic lists, keyboard navigation, and dialog interactions.
  • Tests
    • Added comprehensive coverage for budget calculations, validation, services, APIs, and UI behavior.

- Add jsx-a11y semantic rules as errors in eslint config
- Fix heading hierarchy: shell header owns the single h1, views start
  at h2, wizard steps shift h3/h4 to h2/h3
- Add accessible names to account/budget list sections (sr-only h2)
- Restructure modal backdrops: click-to-close on aria-hidden backdrop,
  dialog element keeps Escape handling
- Movements feed renders as ul/li cards instead of div stacks
- Sidebar expands on keyboard focus, not only hover
- Ignore .agents/** plugin template files in eslint
- Allow underscore-prefixed unused args/vars/catch bindings
- Drop dead startGoogleOAuth helper from login form
- budget domain: monthly limits per category with validation
- budgets service: list/upsert/remove limits, spent aggregation,
  copy limits from previous month
- API routes: GET/PUT /api/presupuestos, limit and copy handlers
- presupuestos view: month navigation, limit list with consumption,
  limit editor and copy-from-previous dialogs
- schema: budget_limits table (migration 0007)
- wizard materialize: seed budget limits from wizard budget step
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This change introduces monthly budget periods, database persistence, budget services, authenticated APIs, and a client budgeting interface. It also updates accessibility semantics and linting, adds a development launch configuration, and removes an unused OAuth helper.

Changes

Monthly budget management

Layer / File(s) Summary
Budget periods and persistence
src/lib/finance/domain/*, src/lib/db/*, src/lib/finance/services/wizard-materialize*
Adds UTC month parsing and validation, backfills budget periods, enforces one budget per user and month, and materializes wizard budgets for the current month.
Budget service operations
src/lib/finance/services/budgets*
Adds monthly budget retrieval, spending aggregation, limit mutation, previous-month copying, category listing, and service-level tests.
Authenticated budget APIs
src/lib/finance/handlers/*, src/app/api/presupuestos/*
Adds authenticated GET, POST, copy, and DELETE handlers with standardized validation, failure, and internal-error responses.
Budget page and interaction UI
src/app/(app)/presupuestos/*
Replaces the placeholder page with month-aware loading, limit displays, progress indicators, dialogs, copy actions, notices, and client rendering tests.

Accessibility and interface semantics

Layer / File(s) Summary
Semantic markup and keyboard interaction
src/app/(app)/configuracion/perfil/*, src/app/(app)/cuentas/*, src/app/(app)/movimientos/*, src/app/wizard/*, src/components/app-shell/*
Updates heading levels, list markup, dialog backdrop semantics, accounts landmark labeling, and keyboard-driven sidebar focus behavior.
Accessibility lint configuration
eslint.config.mjs
Adds .agents/** to global ignores and enables selected JSX accessibility rules with underscore-prefixed unused-variable exceptions.

Development and authentication cleanup

Layer / File(s) Summary
Development launch and login cleanup
.claude/launch.json, src/app/(auth)/login/login-form.tsx
Adds a fixed-port pnpm dev launch configuration and removes the unused Google OAuth redirect helper.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 34.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the two main changes: monthly budget management and semantic HTML/accessibility enforcement.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/budget-management

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Checkov (3.3.8)
.claude/launch.json

Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'

src/lib/db/migrations/meta/0007_snapshot.json

Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'

src/lib/db/migrations/meta/_journal.json

Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (2)
src/lib/finance/services/budgets.ts (1)

366-381: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Redundant full budget load to compute available categories.

listBudgetableCategories calls getBudgetForMonth, which runs the limit join plus both spending-aggregation queries, only to build the taken set of category ids. Callers (page.tsx Lines 32-36 and budgets-route.ts GET Lines 33-37) already call getBudgetForMonth for the same month, so the spending aggregation executes twice per request. Consider passing the already-loaded budget in, or selecting just the taken categoryIds directly from budget_category_amounts.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/finance/services/budgets.ts` around lines 366 - 381, Update
listBudgetableCategories to avoid calling getBudgetForMonth solely to build the
taken set; either accept and reuse the already-loaded budget from callers, or
query only categoryId values from budget_category_amounts for the user and
month. Preserve the existing category filtering and alphabetical ordering while
eliminating duplicate spending-aggregation queries.
src/app/(app)/presupuestos/presupuestos-client.tsx (1)

36-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Comments in src/app must be written in English. UI copy stays Spanish, but code comments should be English per the repo guideline; both files carry Spanish comments.

  • src/app/(app)/presupuestos/presupuestos-client.tsx#L36-L42: translate this and the other Spanish comments (Lines 58-61, 210, 226-227, 367, 607-608) to English.
  • src/app/(app)/presupuestos/page.tsx#L26-L31: translate the Spanish comments at Lines 26-27, 31, and 40 to English.

As per coding guidelines: "Routes, UI copy, and user-facing strings must be Spanish; code identifiers, comments, and commits must be English."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/`(app)/presupuestos/presupuestos-client.tsx around lines 36 - 42,
Translate all Spanish code comments to English without changing UI copy: update
the comments at src/app/(app)/presupuestos/presupuestos-client.tsx lines 36-42,
58-61, 210, 226-227, 367, and 607-608, plus src/app/(app)/presupuestos/page.tsx
lines 26-27, 31, and 40. Preserve the existing explanations and behavior while
keeping routes, identifiers, and user-facing strings Spanish.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/app/`(app)/configuracion/perfil/profile-form-container.tsx:
- Around line 162-167: Replace the clickable backdrop div in the profile form
container with a non-focusable button that retains the onCancel behavior and
includes an accessible label, while preserving its existing full-screen backdrop
styling and aria-hidden semantics as appropriate.

In `@src/app/`(app)/presupuestos/presupuestos-client.tsx:
- Around line 399-403: Clamp the progressbar’s aria-valuenow in the component
rendering around percent so it never exceeds the fixed aria-valuemax of 100,
while preserving percent for the visual over-budget display. Use the existing
percent value as the source and ensure the ARIA value remains within the 0–100
range.

In `@src/components/app-shell/authenticated-shell.tsx`:
- Around line 39-40: Update the sidebar focus handling around onBlurCapture so
setIsSidebarHovered(false) runs only when focus moves outside the sidebar, using
the blur event’s relatedTarget to detect whether the newly focused element
remains within the sidebar. Preserve the existing focus-in behavior and avoid
collapsing while navigating between sidebar items.

In `@src/lib/finance/domain/budget.ts`:
- Around line 136-139: Update the amount validation in the budget record
validation flow to accept only values whose original type is number; remove the
Number(rec.amount) coercion so booleans, arrays, strings, and other JSON values
are rejected. Preserve the existing finite and greater-than-zero checks and
error message for invalid amounts.
- Around line 62-72: The date construction in budgetMonthFromKey must preserve
years 0000–0099 instead of allowing Date.UTC to remap them to 1900–1999. Update
periodStart and periodEnd creation to use a year-safe approach such as
setUTCFullYear, or reject those years explicitly if that matches the intended
contract; keep the existing key parsing and month-boundary behavior unchanged.

In `@src/lib/finance/services/budgets.ts`:
- Around line 335-356: Wrap the budget insert and the budgetCategoryAmounts
insert in a single synchronous db.transaction((tx) => { ... }) callback, using
tx for both writes. Preserve the existing values and selected mapping so the
budget row and its limits commit or roll back together.

---

Nitpick comments:
In `@src/app/`(app)/presupuestos/presupuestos-client.tsx:
- Around line 36-42: Translate all Spanish code comments to English without
changing UI copy: update the comments at
src/app/(app)/presupuestos/presupuestos-client.tsx lines 36-42, 58-61, 210,
226-227, 367, and 607-608, plus src/app/(app)/presupuestos/page.tsx lines 26-27,
31, and 40. Preserve the existing explanations and behavior while keeping
routes, identifiers, and user-facing strings Spanish.

In `@src/lib/finance/services/budgets.ts`:
- Around line 366-381: Update listBudgetableCategories to avoid calling
getBudgetForMonth solely to build the taken set; either accept and reuse the
already-loaded budget from callers, or query only categoryId values from
budget_category_amounts for the user and month. Preserve the existing category
filtering and alphabetical ordering while eliminating duplicate
spending-aggregation queries.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 79c242c3-d4ba-42e7-aa76-a4ef0048f82a

📥 Commits

Reviewing files that changed from the base of the PR and between 84c3cd4 and addd798.

📒 Files selected for processing (34)
  • .claude/launch.json
  • eslint.config.mjs
  • src/app/(app)/configuracion/perfil/profile-form-container.tsx
  • src/app/(app)/configuracion/perfil/profile-form.tsx
  • src/app/(app)/cuentas/cuentas-client.tsx
  • src/app/(app)/movimientos/movimientos-client.tsx
  • src/app/(app)/presupuestos/page.tsx
  • src/app/(app)/presupuestos/presupuestos-client.test.ts
  • src/app/(app)/presupuestos/presupuestos-client.tsx
  • src/app/(auth)/login/login-form.tsx
  • src/app/api/presupuestos/copiar/route.ts
  • src/app/api/presupuestos/route.ts
  • src/app/api/presupuestos/topes/[categoryId]/route.ts
  • src/app/wizard/components/step-header.tsx
  • src/app/wizard/steps/step-budget-goals.tsx
  • src/app/wizard/steps/step-catalog.tsx
  • src/app/wizard/steps/step-summary.tsx
  • src/components/app-shell/authenticated-shell.tsx
  • src/components/app-shell/placeholder-view.tsx
  • src/lib/db/migrations/0007_busy_luke_cage.sql
  • src/lib/db/migrations/meta/0007_snapshot.json
  • src/lib/db/migrations/meta/_journal.json
  • src/lib/db/schema.ts
  • src/lib/finance/domain/budget.test.ts
  • src/lib/finance/domain/budget.ts
  • src/lib/finance/handlers/budget-copy-route.ts
  • src/lib/finance/handlers/budget-limit-route.ts
  • src/lib/finance/handlers/budgets-response.ts
  • src/lib/finance/handlers/budgets-route.test.ts
  • src/lib/finance/handlers/budgets-route.ts
  • src/lib/finance/services/budgets.test.ts
  • src/lib/finance/services/budgets.ts
  • src/lib/finance/services/wizard-materialize.test.ts
  • src/lib/finance/services/wizard-materialize.ts
💤 Files with no reviewable changes (1)
  • src/app/(auth)/login/login-form.tsx

Comment on lines +162 to +167
<div className="fixed inset-0 z-50 flex items-center justify-center p-4">
<div
className="w-full max-w-md rounded-lg border border-border bg-card p-6 shadow-xl"
onClick={(e) => e.stopPropagation()}
aria-hidden="true"
onClick={onCancel}
className="absolute inset-0 bg-black/50"
/>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use an interactive backdrop element.

This div has a click handler but remains non-interactive, and aria-hidden does not exempt it. The new eslint.config.mjs rules will make this file fail lint. Use a non-focusable <button> backdrop with an accessible label, or add keyboard-equivalent behavior with a narrow suppression.

Proposed fix
-      <div
-        aria-hidden="true"
-        onClick={onCancel}
-        className="absolute inset-0 bg-black/50"
-      />
+      <button
+        type="button"
+        tabIndex={-1}
+        aria-label="Cerrar diálogo"
+        onClick={onCancel}
+        className="absolute inset-0 bg-black/50"
+      />
📝 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
<div className="fixed inset-0 z-50 flex items-center justify-center p-4">
<div
className="w-full max-w-md rounded-lg border border-border bg-card p-6 shadow-xl"
onClick={(e) => e.stopPropagation()}
aria-hidden="true"
onClick={onCancel}
className="absolute inset-0 bg-black/50"
/>
<div className="fixed inset-0 z-50 flex items-center justify-center p-4">
<button
type="button"
tabIndex={-1}
aria-label="Cerrar diálogo"
onClick={onCancel}
className="absolute inset-0 bg-black/50"
/>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/`(app)/configuracion/perfil/profile-form-container.tsx around lines
162 - 167, Replace the clickable backdrop div in the profile form container with
a non-focusable button that retains the onCancel behavior and includes an
accessible label, while preserving its existing full-screen backdrop styling and
aria-hidden semantics as appropriate.

Comment on lines +399 to +403
<div
role="progressbar"
aria-valuenow={percent}
aria-valuemin={0}
aria-valuemax={100}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

aria-valuenow can exceed aria-valuemax on overruns.

When a category is over budget, percent is capped at 999 (Line 64) and passed to aria-valuenow, but aria-valuemax is fixed at 100. An out-of-range aria-valuenow is invalid per ARIA and assistive tech may report it inconsistently. Clamp the value or expose the overrun via aria-valuetext.

♿ Proposed fix
       <div
         role="progressbar"
-        aria-valuenow={percent}
+        aria-valuenow={Math.min(100, percent)}
         aria-valuemin={0}
         aria-valuemax={100}
+        aria-valuetext={`${percent}%`}
         aria-label={`Consumo de ${limit.categoryName}`}
📝 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
<div
role="progressbar"
aria-valuenow={percent}
aria-valuemin={0}
aria-valuemax={100}
<div
role="progressbar"
aria-valuenow={Math.min(100, percent)}
aria-valuemin={0}
aria-valuemax={100}
aria-valuetext={`${percent}%`}
aria-label={`Consumo de ${limit.categoryName}`}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/`(app)/presupuestos/presupuestos-client.tsx around lines 399 - 403,
Clamp the progressbar’s aria-valuenow in the component rendering around percent
so it never exceeds the fixed aria-valuemax of 100, while preserving percent for
the visual over-budget display. Use the existing percent value as the source and
ensure the ARIA value remains within the 0–100 range.

Comment on lines +39 to +40
onFocusCapture={() => setIsSidebarHovered(true)}
onBlurCapture={() => setIsSidebarHovered(false)}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Only collapse when focus leaves the sidebar.

onBlurCapture also runs when focus moves from one sidebar item to another. Clearing the state unconditionally can start the collapse transition while keyboard users tab within the navigation, causing flicker and unstable focus behavior. Check relatedTarget before collapsing.

Proposed fix
-          onBlurCapture={() => setIsSidebarHovered(false)}
+          onBlurCapture={(event) => {
+            const next = event.relatedTarget;
+            if (!(next instanceof Node) || !event.currentTarget.contains(next)) {
+              setIsSidebarHovered(false);
+            }
+          }}
📝 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
onFocusCapture={() => setIsSidebarHovered(true)}
onBlurCapture={() => setIsSidebarHovered(false)}
onFocusCapture={() => setIsSidebarHovered(true)}
onBlurCapture={(event) => {
const next = event.relatedTarget;
if (!(next instanceof Node) || !event.currentTarget.contains(next)) {
setIsSidebarHovered(false);
}
}}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/app-shell/authenticated-shell.tsx` around lines 39 - 40,
Update the sidebar focus handling around onBlurCapture so
setIsSidebarHovered(false) runs only when focus moves outside the sidebar, using
the blur event’s relatedTarget to detect whether the newly focused element
remains within the sidebar. Preserve the existing focus-in behavior and avoid
collapsing while navigating between sidebar items.

Comment on lines +62 to +72
export function budgetMonthFromKey(key: string): BudgetMonth | null {
const match = MONTH_PATTERN.exec(key);
if (!match) return null;
const year = Number(match[1]);
const month = Number(match[2]);
return {
key,
periodStart: new Date(Date.UTC(year, month - 1, 1, 0, 0, 0, 0)),
// Último instante del mes: el primer instante del siguiente, menos un milisegundo.
periodEnd: new Date(Date.UTC(year, month, 1, 0, 0, 0, 0) - 1),
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== File outline ==\n'
ast-grep outline src/lib/finance/domain/budget.ts --view expanded || true

printf '\n== Relevant excerpt ==\n'
sed -n '1,140p' src/lib/finance/domain/budget.ts

printf '\n== JS Date.UTC probe ==\n'
node - <<'JS'
for (const y of [0, 1, 42, 99, 100, 1900]) {
  const d = new Date(Date.UTC(y, 0, 1));
  console.log(y, d.toISOString());
}
JS

Repository: DataDave-Dev/valora-official

Length of output: 6152


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== Search for budgetMonthFromKey usages and tests ==\n'
rg -n "budgetMonthFromKey\(" src/lib/finance -g '!**/node_modules/**' || true

printf '\n== Search for month key format/pattern ==\n'
rg -n "MONTH_PATTERN|BudgetMonth|periodStart|periodEnd|0000|0099" src/lib/finance -g '!**/node_modules/**' || true

Repository: DataDave-Dev/valora-official

Length of output: 8739


Handle years 00000099 without Date.UTC remapping. MONTH_PATTERN accepts YYYY-MM, but Date.UTC() turns 099 into 19001999, so 0001-01 keeps the 0001-01 key while producing 1901 dates. Reject that range or build the dates with setUTCFullYear.

🧰 Tools
🪛 OpenGrep (1.25.0)

[ERROR] 63-63: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.

(coderabbit.command-injection.exec-js)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/finance/domain/budget.ts` around lines 62 - 72, The date construction
in budgetMonthFromKey must preserve years 0000–0099 instead of allowing Date.UTC
to remap them to 1900–1999. Update periodStart and periodEnd creation to use a
year-safe approach such as setUTCFullYear, or reject those years explicitly if
that matches the intended contract; keep the existing key parsing and
month-boundary behavior unchanged.

Comment on lines +136 to +139
const amount = typeof rec.amount === "number" ? rec.amount : Number(rec.amount);
if (!Number.isFinite(amount) || amount <= 0) {
errors.push({ field: "amount", message: "El monto debe ser mayor a cero." });
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Do not coerce arbitrary JSON values into amounts.

Number(true) and Number([100]) are valid positive numbers, so malformed payloads can persist unintended limits. Require a JSON number at the API boundary.

Proposed fix
-const amount = typeof rec.amount === "number" ? rec.amount : Number(rec.amount);
-if (!Number.isFinite(amount) || amount <= 0) {
+const amount = rec.amount;
+if (typeof amount !== "number" || !Number.isFinite(amount) || amount <= 0) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const amount = typeof rec.amount === "number" ? rec.amount : Number(rec.amount);
if (!Number.isFinite(amount) || amount <= 0) {
errors.push({ field: "amount", message: "El monto debe ser mayor a cero." });
}
const amount = rec.amount;
if (typeof amount !== "number" || !Number.isFinite(amount) || amount <= 0) {
errors.push({ field: "amount", message: "El monto debe ser mayor a cero." });
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/finance/domain/budget.ts` around lines 136 - 139, Update the amount
validation in the budget record validation flow to accept only values whose
original type is number; remove the Number(rec.amount) coercion so booleans,
arrays, strings, and other JSON values are rejected. Preserve the existing
finite and greater-than-zero checks and error message for invalid amounts.

Comment on lines +335 to +356
await db.insert(budgets).values({
id: budgetId,
userId,
scope: source.scope,
monthlyTotal: source.monthlyTotal,
currency: source.currency,
periodStart: month.periodStart,
periodEnd: month.periodEnd,
createdAt: now,
updatedAt: now,
});

await db.insert(budgetCategoryAmounts).values(
selected.map((limit) => ({
id: randomUUID(),
budgetId,
categoryId: limit.categoryId,
amount: limit.amount,
createdAt: now,
updatedAt: now,
})),
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

drizzle-orm 0.45 better-sqlite3 synchronous db.transaction API

💡 Result:

In Drizzle ORM 0.45.x, when using the better-sqlite3 driver, the db.transaction API is synchronous [1][2]. Because better-sqlite3 is a synchronous library, it does not support asynchronous callbacks within its transaction method [1][2][3]. Key points regarding this behavior: 1. Sync vs. Async: Unlike drivers for PostgreSQL or MySQL, better-sqlite3 handles database operations synchronously [1][2]. Passing an async function to db.transaction is unsupported and may throw an error (e.g., "Transaction function cannot return a promise") [1]. 2. Correct Usage: You must use a synchronous callback for db.transaction and ensure that all database operations inside the transaction use synchronous query methods (such as.run,.get,.all, or.values) [1][4]. Do not use await inside the transaction block [1][4]. Example of the correct synchronous pattern: db.transaction((tx) => { tx.insert(users).values({ name: 'Alice' }).run; tx.insert(posts).values({ userId: 1, title: 'Hello' }).run; }); Using await or returning a promise inside this block will interfere with the synchronous execution flow of the underlying better-sqlite3 driver, potentially causing transaction failures or improper rollback behavior [1][2][3].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the target file around the cited lines
if [ -f src/lib/finance/services/budgets.ts ]; then
  nl -ba src/lib/finance/services/budgets.ts | sed -n '280,390p'
else
  echo "Missing src/lib/finance/services/budgets.ts" >&2
  exit 1
fi

echo
echo "---- transaction usage in finance services ----"
rg -n "transaction\\(" src/lib/finance/services -g '*.ts' -g '*.tsx' || true

Repository: DataDave-Dev/valora-official

Length of output: 206


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "---- target excerpt ----"
sed -n '280,390p' src/lib/finance/services/budgets.ts | cat -n

echo
echo "---- transaction usage in finance services ----"
rg -n "transaction\\(" src/lib/finance/services -g '*.ts' -g '*.tsx' || true

Repository: DataDave-Dev/valora-official

Length of output: 4611


Wrap the budget and limit inserts in a transaction. If the second insert fails, the budget row survives on its own and the next copy hits the “Este mes ya tiene un presupuesto” conflict with no limits attached. Use a single synchronous db.transaction((tx) => { ... }) so both writes commit or roll back together.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/finance/services/budgets.ts` around lines 335 - 356, Wrap the budget
insert and the budgetCategoryAmounts insert in a single synchronous
db.transaction((tx) => { ... }) callback, using tx for both writes. Preserve the
existing values and selected mapping so the budget row and its limits commit or
roll back together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant