Feature/budget management upstream#251
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR adds a complete budget management system for organizational units. It introduces a BudgetTransaction data model with compound indexing, backend business logic for allocating, spending, and refunding budgets with role-based access control, REST API endpoints for transaction submission and history retrieval, and a React frontend featuring unit selection, transaction forms, and paginated history display. ChangesBudget Management Feature
Sequence DiagramsequenceDiagram
participant User as User (Role)
participant UI as BudgetManagement
participant Hook as useBudgetManagement
participant API as Backend API
participant DB as Database
User->>UI: Select unit, enter amount
UI->>Hook: submitTransaction(endpoint, {unitId, amount, eventId, description})
Hook->>API: POST /api/budget/allocate|spend|refund
API->>DB: Validate unit, event, update budget_info
API->>DB: Create BudgetTransaction record
DB-->>API: Return transaction with balance_after
API-->>Hook: Transaction + updated budget
Hook->>Hook: Refresh page 1 transactions
Hook-->>UI: Updated budget, transactions, status
UI-->>User: Display success/error, update summary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@backend/controllers/budgetController.js`:
- Around line 146-151: The EXPENSE branch updates event.budget.spent without
checking that spent + amount <= allocated, allowing event-level overspend;
before assigning event.budget in the TRANSACTION_TYPES.EXPENSE branch (where
event.budget = Object.assign(... existingBudget ...)), add a validation that
computes newSpent = spent + amount and if newSpent > allocated return/throw the
same insufficient-budget error used at unit-level (or call the same helper),
otherwise set event.budget with spent: newSpent; reference
TRANSACTION_TYPES.EXPENSE, event.budget, existingBudget, allocated, spent, and
amount to locate the exact spot to add this check.
- Around line 340-341: The pagination parsing currently converts invalid strings
to NaN causing runtime errors; update the handler that defines page and limit
(the code reading req.query.page and req.query.limit and assigning to
page/limit) to robustly validate inputs: parse as integers (e.g., parseInt),
check Number.isFinite/Number.isInteger and that page >= 1 and limit between 1
and 100; if validation fails, return a 400 Bad Request (res.status(400).json({
error: 'Invalid pagination parameters' })) instead of using Math.max/Math.min on
NaN, otherwise use the validated parsed values as page and limit for
.skip()/.limit().
In `@frontend/src/Components/Budget/BudgetManagement.jsx`:
- Around line 61-84: The fetchEvents useEffect can have out-of-order responses
overwrite state when selectedUnitId changes; modify the effect to ignore stale
responses by capturing the current selectedUnitId (e.g., const currentId =
selectedUnitId) or using an AbortController before calling api.get inside
fetchEvents, and only call setEvents(filteredEvents) if currentId ===
selectedUnitId (or if the fetch was not aborted). Also ensure any created
AbortController is cleaned up in the effect's return handler to cancel in-flight
requests when selectedUnitId changes. Refer to the fetchEvents function and the
useEffect that depends on selectedUnitId to implement this guard.
In `@frontend/src/hooks/useBudgetManagement.js`:
- Around line 125-126: The normalization for nextPage in useBudgetManagement.js
can produce NaN because Number(nextPage || 1) will be NaN for non-numeric
strings; change the logic that computes normalized (the normalized variable) to
explicitly parse and validate nextPage (e.g., use Number.parseInt or
Number(nextPage) then check Number.isFinite/Number.isInteger or isNaN) and
default to 1 when parsing fails, then call fetchBudgetData(selectedUnitId,
normalized); ensure the code references nextPage, normalized, and
fetchBudgetData so invalid inputs never propagate to the API.
- Around line 74-76: In useBudgetManagement's fetch error path (the catch block
that currently calls setError), also clear any previous budget/unit state so
stale data doesn't stay visible after a failed fetch — e.g., call the
budget-related setters (replace with your actual names, e.g., setBudget(null),
setUnitData(null), setBudgetItems([]) or similar) before calling
setError(err?.response?.data?.message || "Failed to load budget data"); keep the
finally block as-is.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29207a15-6c9e-4d49-8da0-db16c1a617c9
📒 Files selected for processing (10)
backend/controllers/budgetController.jsbackend/index.jsbackend/models/schema.jsbackend/routes/budget.jsbackend/seed.jsfrontend/src/Components/Budget/BudgetManagement.jsxfrontend/src/config/dashboardComponents.jsfrontend/src/config/navbarConfig.jsfrontend/src/hooks/useBudgetManagement.jsfrontend/src/pages/budgetPage.jsx
Comment on lines
+146
to
+151
| if (type === TRANSACTION_TYPES.EXPENSE) { | ||
| event.budget = Object.assign({}, existingBudget, { | ||
| allocated, | ||
| spent: spent + amount, | ||
| }); | ||
| return; |
Contributor
There was a problem hiding this comment.
Add event-level insufficient-budget validation for expenses.
EXPENSE updates event.budget.spent without enforcing spent + amount <= allocated, so event budgets can be overspent even when unit-level budget passes.
Suggested fix
if (type === TRANSACTION_TYPES.EXPENSE) {
+ if (spent + amount > allocated) {
+ throw createHttpError(400, "Insufficient remaining event budget");
+ }
event.budget = Object.assign({}, existingBudget, {
allocated,
spent: spent + amount,
});
return;
}📝 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
| if (type === TRANSACTION_TYPES.EXPENSE) { | |
| event.budget = Object.assign({}, existingBudget, { | |
| allocated, | |
| spent: spent + amount, | |
| }); | |
| return; | |
| if (type === TRANSACTION_TYPES.EXPENSE) { | |
| if (spent + amount > allocated) { | |
| throw createHttpError(400, "Insufficient remaining event budget"); | |
| } | |
| event.budget = Object.assign({}, existingBudget, { | |
| allocated, | |
| spent: spent + amount, | |
| }); | |
| return; |
🤖 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 `@backend/controllers/budgetController.js` around lines 146 - 151, The EXPENSE
branch updates event.budget.spent without checking that spent + amount <=
allocated, allowing event-level overspend; before assigning event.budget in the
TRANSACTION_TYPES.EXPENSE branch (where event.budget = Object.assign(...
existingBudget ...)), add a validation that computes newSpent = spent + amount
and if newSpent > allocated return/throw the same insufficient-budget error used
at unit-level (or call the same helper), otherwise set event.budget with spent:
newSpent; reference TRANSACTION_TYPES.EXPENSE, event.budget, existingBudget,
allocated, spent, and amount to locate the exact spot to add this check.
Comment on lines
+340
to
+341
| const page = Math.max(1, Number(req.query.page || 1)); | ||
| const limit = Math.min(100, Math.max(1, Number(req.query.limit || 20))); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current pagination parsing pattern and absence/presence of integer guards.
rg -n -C2 'const page = Math.max\(1, Number\(req\.query\.page \|\| 1\)\);|const limit = Math.min\(100, Math.max\(1, Number\(req\.query\.limit \|\| 20\)\)\);|Number\.parseInt\(req\.query\.(page|limit)|Number\.isInteger\(.*(page|limit)' backend/controllers/budgetController.jsRepository: OpenLake/Student_Database_COSA
Length of output: 308
🏁 Script executed:
#!/bin/bash
# Search for other pagination/query parameter parsing patterns in the codebase
rg -n 'req\.query\.(page|limit|skip|offset)' backend/controllers/ --type js -A1Repository: OpenLake/Student_Database_COSA
Length of output: 343
🏁 Script executed:
#!/bin/bash
# Check if there's any error handling or validation middleware
rg -n 'try|catch|validation|validator|middleware' backend/controllers/budgetController.js | head -20Repository: OpenLake/Student_Database_COSA
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Look for how .skip() and .limit() are used with these variables
rg -n '\.skip\(|\.limit\(' backend/controllers/budgetController.js -B2 -A2Repository: OpenLake/Student_Database_COSA
Length of output: 319
🏁 Script executed:
#!/bin/bash
# Check what happens in the catch block for error handling
sed -n '337,410p' backend/controllers/budgetController.jsRepository: OpenLake/Student_Database_COSA
Length of output: 2412
🏁 Script executed:
#!/bin/bash
# Test Math.max behavior with NaN
node <<'EOF'
console.log("Math.max(1, NaN):", Math.max(1, NaN));
console.log("Math.min(100, Math.max(1, NaN)):", Math.min(100, Math.max(1, NaN)));
console.log("Number('invalid'):", Number('invalid'));
console.log("(NaN - 1) * 20:", (NaN - 1) * 20);
console.log("Calculation when Number('abc') is used:");
const page = Math.max(1, Number('abc' || 1));
const limit = Math.min(100, Math.max(1, Number('def' || 20)));
console.log("page:", page, "limit:", limit);
console.log("(page - 1) * limit:", (page - 1) * limit);
EOFRepository: OpenLake/Student_Database_COSA
Length of output: 261
🏁 Script executed:
#!/bin/bash
# Check the complete error handling in the catch block
sed -n '405,420p' backend/controllers/budgetController.jsRepository: OpenLake/Student_Database_COSA
Length of output: 517
🏁 Script executed:
#!/bin/bash
# Search for other instances of pagination/query parameter parsing to see if this is widespread
rg -n 'Number\(req\.query' backend/controllers/ --type jsRepository: OpenLake/Student_Database_COSA
Length of output: 298
🏁 Script executed:
#!/bin/bash
# Look for any input validation utilities or middleware
find backend -type f -name "*validation*" -o -name "*validator*" -o -name "*middleware*" | head -10Repository: OpenLake/Student_Database_COSA
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Check what's in the middlewares directory
ls -la backend/middlewares/Repository: OpenLake/Student_Database_COSA
Length of output: 446
🏁 Script executed:
#!/bin/bash
# Search for any pagination or query validation middleware
rg -n 'pagination|page|limit|query.*validat' backend/middlewares/ --type jsRepository: OpenLake/Student_Database_COSA
Length of output: 160
Harden pagination parsing against non-numeric query inputs.
Invalid page/limit query values become NaN due to Math.max(1, Number('invalid')) returning NaN rather than a fallback—this breaks .skip() and .limit(), surfacing as a 500 error instead of a proper 400 Bad Request.
Suggested fix
- const page = Math.max(1, Number(req.query.page || 1));
- const limit = Math.min(100, Math.max(1, Number(req.query.limit || 20)));
+ const pageInput = Number.parseInt(req.query.page, 10);
+ const limitInput = Number.parseInt(req.query.limit, 10);
+ const page = Number.isInteger(pageInput) && pageInput > 0 ? pageInput : 1;
+ const limitBase =
+ Number.isInteger(limitInput) && limitInput > 0 ? limitInput : 20;
+ const limit = Math.min(100, limitBase);🤖 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 `@backend/controllers/budgetController.js` around lines 340 - 341, The
pagination parsing currently converts invalid strings to NaN causing runtime
errors; update the handler that defines page and limit (the code reading
req.query.page and req.query.limit and assigning to page/limit) to robustly
validate inputs: parse as integers (e.g., parseInt), check
Number.isFinite/Number.isInteger and that page >= 1 and limit between 1 and 100;
if validation fails, return a 400 Bad Request (res.status(400).json({ error:
'Invalid pagination parameters' })) instead of using Math.max/Math.min on NaN,
otherwise use the validated parsed values as page and limit for
.skip()/.limit().
Comment on lines
+61
to
+84
| useEffect(() => { | ||
| const fetchEvents = async () => { | ||
| if (!selectedUnitId) { | ||
| setEvents([]); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const response = await api.get("/api/events/events"); | ||
| const filteredEvents = (response.data || []).filter( | ||
| (event) => | ||
| event.organizing_unit_id && | ||
| event.organizing_unit_id._id === selectedUnitId, | ||
| ); | ||
|
|
||
| setEvents(filteredEvents); | ||
| } catch (error) { | ||
| console.error("Failed to fetch events", error); | ||
| setEvents([]); | ||
| } | ||
| }; | ||
|
|
||
| fetchEvents(); | ||
| }, [selectedUnitId]); |
Contributor
There was a problem hiding this comment.
Prevent out-of-order event fetches from overwriting current unit events.
Lines 62–76 can race when users switch units quickly; an older response may overwrite events for the currently selected unit.
Proposed fix
useEffect(() => {
+ let isActive = true;
const fetchEvents = async () => {
if (!selectedUnitId) {
- setEvents([]);
+ if (isActive) setEvents([]);
return;
}
try {
const response = await api.get("/api/events/events");
const filteredEvents = (response.data || []).filter(
(event) =>
event.organizing_unit_id &&
event.organizing_unit_id._id === selectedUnitId,
);
-
- setEvents(filteredEvents);
+ if (isActive) setEvents(filteredEvents);
} catch (error) {
console.error("Failed to fetch events", error);
- setEvents([]);
+ if (isActive) setEvents([]);
}
};
fetchEvents();
+ return () => {
+ isActive = false;
+ };
}, [selectedUnitId]);📝 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
| useEffect(() => { | |
| const fetchEvents = async () => { | |
| if (!selectedUnitId) { | |
| setEvents([]); | |
| return; | |
| } | |
| try { | |
| const response = await api.get("/api/events/events"); | |
| const filteredEvents = (response.data || []).filter( | |
| (event) => | |
| event.organizing_unit_id && | |
| event.organizing_unit_id._id === selectedUnitId, | |
| ); | |
| setEvents(filteredEvents); | |
| } catch (error) { | |
| console.error("Failed to fetch events", error); | |
| setEvents([]); | |
| } | |
| }; | |
| fetchEvents(); | |
| }, [selectedUnitId]); | |
| useEffect(() => { | |
| let isActive = true; | |
| const fetchEvents = async () => { | |
| if (!selectedUnitId) { | |
| if (isActive) setEvents([]); | |
| return; | |
| } | |
| try { | |
| const response = await api.get("/api/events/events"); | |
| const filteredEvents = (response.data || []).filter( | |
| (event) => | |
| event.organizing_unit_id && | |
| event.organizing_unit_id._id === selectedUnitId, | |
| ); | |
| if (isActive) setEvents(filteredEvents); | |
| } catch (error) { | |
| console.error("Failed to fetch events", error); | |
| if (isActive) setEvents([]); | |
| } | |
| }; | |
| fetchEvents(); | |
| return () => { | |
| isActive = false; | |
| }; | |
| }, [selectedUnitId]); |
🤖 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 `@frontend/src/Components/Budget/BudgetManagement.jsx` around lines 61 - 84,
The fetchEvents useEffect can have out-of-order responses overwrite state when
selectedUnitId changes; modify the effect to ignore stale responses by capturing
the current selectedUnitId (e.g., const currentId = selectedUnitId) or using an
AbortController before calling api.get inside fetchEvents, and only call
setEvents(filteredEvents) if currentId === selectedUnitId (or if the fetch was
not aborted). Also ensure any created AbortController is cleaned up in the
effect's return handler to cancel in-flight requests when selectedUnitId
changes. Refer to the fetchEvents function and the useEffect that depends on
selectedUnitId to implement this guard.
Comment on lines
+74
to
+76
| } catch (err) { | ||
| setError(err?.response?.data?.message || "Failed to load budget data"); | ||
| } finally { |
Contributor
There was a problem hiding this comment.
Clear stale budget state on fetch failure.
On Line 74, the error path only sets error, so previous unit data can remain visible after a failed fetch for the newly selected unit.
Proposed fix
} catch (err) {
setError(err?.response?.data?.message || "Failed to load budget data");
+ setBudget(null);
+ setTransactions([]);
+ setPagination({ total: 0, page: 1, limit: DEFAULT_PAGE_SIZE });
} finally {
setIsLoadingData(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
| } catch (err) { | |
| setError(err?.response?.data?.message || "Failed to load budget data"); | |
| } finally { | |
| } catch (err) { | |
| setError(err?.response?.data?.message || "Failed to load budget data"); | |
| setBudget(null); | |
| setTransactions([]); | |
| setPagination({ total: 0, page: 1, limit: DEFAULT_PAGE_SIZE }); | |
| } finally { |
🤖 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 `@frontend/src/hooks/useBudgetManagement.js` around lines 74 - 76, In
useBudgetManagement's fetch error path (the catch block that currently calls
setError), also clear any previous budget/unit state so stale data doesn't stay
visible after a failed fetch — e.g., call the budget-related setters (replace
with your actual names, e.g., setBudget(null), setUnitData(null),
setBudgetItems([]) or similar) before calling
setError(err?.response?.data?.message || "Failed to load budget data"); keep the
finally block as-is.
Comment on lines
+125
to
+126
| const normalized = Math.max(1, Number(nextPage || 1)); | ||
| await fetchBudgetData(selectedUnitId, normalized); |
Contributor
There was a problem hiding this comment.
Harden page normalization against NaN.
Line 125 can produce NaN when nextPage is non-numeric, which then propagates to the API query.
Proposed fix
- const normalized = Math.max(1, Number(nextPage || 1));
+ const parsed = Number(nextPage);
+ const normalized = Number.isFinite(parsed)
+ ? Math.max(1, Math.trunc(parsed))
+ : 1;
await fetchBudgetData(selectedUnitId, normalized);🤖 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 `@frontend/src/hooks/useBudgetManagement.js` around lines 125 - 126, The
normalization for nextPage in useBudgetManagement.js can produce NaN because
Number(nextPage || 1) will be NaN for non-numeric strings; change the logic that
computes normalized (the normalized variable) to explicitly parse and validate
nextPage (e.g., use Number.parseInt or Number(nextPage) then check
Number.isFinite/Number.isInteger or isNaN) and default to 1 when parsing fails,
then call fetchBudgetData(selectedUnitId, normalized); ensure the code
references nextPage, normalized, and fetchBudgetData so invalid inputs never
propagate to the API.
Contributor
|
LGTM |
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
4 participants
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 contains the original budget management implementation from PR #240 along with stabilization fixes, workflow validation, and UI/backend improvements.
Fixes and Improvements
uuidv4import causing transaction failuresTested Locally
Additional Notes
This PR acts as a stabilization/follow-up PR for #240 and includes both the original contributor changes and maintainer fixes/review updates.
Summary by CodeRabbit