feat: fix ecurrency bugs + UX improvments #619
Conversation
WalkthroughBackend validation layers for maxNegativeBalance are strengthened across CurrencyController and CurrencyService with normalization and range checks. LedgerController integrates GroupService to support group-scoped account initialization with admin authorization. Frontend modals are updated to accept and propagate account context (user or group), adjust balances queries and mutations accordingly, and provide UI input for maxNegativeBalance configuration. Changes
Sequence DiagramsequenceDiagram
actor Client
participant LedgerController
participant GroupService
participant LedgerService
Client->>LedgerController: POST /initialize-account<br/>{accountType, accountId, ...}
alt accountType is GROUP
LedgerController->>GroupService: isGroupAdmin(userId, groupId)
alt Admin Check Passes
GroupService-->>LedgerController: true
LedgerController->>LedgerService: initializeAccount(finalAccountId, GROUP)
LedgerService-->>LedgerController: Success
LedgerController-->>Client: 200 OK
else Admin Check Fails
GroupService-->>LedgerController: false
LedgerController-->>Client: 403 Forbidden
end
else accountType is USER (or default)
LedgerController->>LedgerService: initializeAccount(userId, USER)
LedgerService-->>LedgerController: Success
LedgerController-->>Client: 200 OK
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
platforms/eCurrency/client/src/components/currency/create-currency-modal.tsx (1)
25-47: Consider adding error handling for mutation failures.The mutation lacks an
onErrorhandler. If the server returns a validation error (e.g., duplicate currency name, invalid group), users won't see feedback. The localerrorstate is available but only used for client-side validation.const createMutation = useMutation({ mutationFn: async (data: { name: string; description?: string; groupId: string; allowNegative: boolean; maxNegativeBalance: number | null; }) => { const response = await apiClient.post("/api/currencies", data); return response.data; }, onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["currencies"] }); queryClient.invalidateQueries({ queryKey: ["balances"] }); setName(""); setDescription(""); setGroupId(""); setAllowNegative(false); setMaxNegativeInput(""); setError(null); onOpenChange(false); }, + onError: (error: any) => { + const message = error?.response?.data?.error || error?.message || "Failed to create currency"; + setError(message); + }, });platforms/eCurrency-api/src/controllers/LedgerController.ts (1)
109-113: Address the TODO: Missing group admin authorization for transfers.The TODO at line 110 indicates a security gap. Since
GroupServiceis now integrated, consider completing this authorization check to prevent unauthorized transfers from group accounts.// Verify user has permission to transfer from this account if (finalFromAccountType === AccountType.GROUP) { - // TODO: Verify user is admin of the group + const isAdmin = await this.groupService.isGroupAdmin(finalFromAccountId, req.user.id); + if (!isAdmin) { + return res.status(403).json({ error: "Only group admins can transfer from group accounts" }); + } } else if (finalFromAccountId !== req.user.id) { return res.status(403).json({ error: "You can only transfer from your own account" }); }platforms/eCurrency-api/src/controllers/CurrencyController.ts (2)
24-43: ClarifyallowNegativecoercion and possibly skipmaxNegativeBalancevalidation when negatives are disabledThe normalization/validation flow looks solid overall, but two details are worth tightening:
const allowNegativeFlag = Boolean(allowNegative);will treat any non-empty string (including"false") or non-zero number astrue. If the API is ever called by anything other than your current frontend, this can easily misinterpret intent. It’s often safer to require a real boolean and/or explicitly checkallowNegative === true(or a small whitelist of accepted values) instead of genericBoolean(...).- When
allowNegativeFlagisfalse, you eventually forcenormalizedMaxNegative = null, but only after fully parsing and validating the inboundmaxNegativeBalance. That means a client sendingallowNegative: falsewith a (say) positivemaxNegativeBalancegets a 400, even though the value will be ignored anyway. Consider short‑circuiting: if!allowNegativeFlag, ignore any providedmaxNegativeBalance(or at least skip the zero/negative/limit checks) so callers don’t get confusing validation errors for a field that’s effectively disabled.
188-195: Validation mirrors create flow; consider centralizing the limit constantThe added guards here (non‑positive, >= −1_000_000_000) are consistent with the
createCurrencyvalidation, which is good for behavior parity. Since-1_000_000_000is now a business limit used in multiple places (and likely in the service layer too), consider lifting it into a shared constant (e.g., in the service or a config/constants module) to avoid divergence if the limit ever changes.platforms/eCurrency/client/src/components/currency/add-currency-account-modal.tsx (2)
35-45: Group-aware balances query behavior is correct; consider optional early return when context is missingThe group-specific parameterization:
- Uses
params.accountType = "group"; params.accountId = accountContext.id;only whenisGroupContext && accountContext?.id, falling back to the authenticated user for the default path, which matches the likely backend contract.- Keeps
paramsempty for user context, so you don’t accidentally change existing semantics.If you ever expect the modal to be opened only with a valid context, you could optionally add a guard (e.g., returning an empty list or throwing) when
isGroupContextis true butaccountContext?.idis missing, so misconfiguration fails loudly instead of silently calling the user‑scoped endpoint.
64-76: Mutation closure pattern is fine; the extra!currencyIdguard is redundantThe mutation is correctly switched to a zero‑argument
mutationFnand invoked viainitializeMutation.mutate()in the submit handler, avoiding accidentally passing the submit event into the mutation.Given:
- The submit handler already checks
if (currencyId)before callingmutate().- The submit button is disabled when
!currencyIdor there are no available currencies.the inner guard:
if (!currencyId) return;is effectively unreachable and can be dropped to simplify the mutation function. The rest of the payload construction, including conditional
accountType/accountIdfor group context and the matchinginvalidateQueries({ queryKey: balancesQueryKey }), looks solid.Also applies to: 108-108
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
platforms/eCurrency-api/src/controllers/CurrencyController.ts(2 hunks)platforms/eCurrency-api/src/controllers/LedgerController.ts(2 hunks)platforms/eCurrency-api/src/services/CurrencyService.ts(2 hunks)platforms/eCurrency/client/src/components/currency/add-currency-account-modal.tsx(4 hunks)platforms/eCurrency/client/src/components/currency/create-currency-modal.tsx(5 hunks)platforms/eCurrency/client/src/pages/currency-detail.tsx(4 hunks)platforms/eCurrency/client/src/pages/dashboard.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
platforms/eCurrency/client/src/pages/dashboard.tsx (1)
platforms/eCurrency/client/src/components/currency/add-currency-account-modal.tsx (1)
AddCurrencyAccountModal(18-199)
platforms/eCurrency-api/src/controllers/LedgerController.ts (2)
platforms/eCurrency-api/src/services/LedgerService.ts (1)
LedgerService(9-334)platforms/eCurrency-api/src/services/GroupService.ts (1)
GroupService(6-237)
platforms/eCurrency/client/src/components/currency/add-currency-account-modal.tsx (1)
platforms/eCurrency/client/src/lib/apiClient.ts (1)
apiClient(5-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
platforms/eCurrency-api/src/services/CurrencyService.ts (2)
37-47: LGTM! Validation logic for maxNegativeBalance in createCurrency is well-structured.The validation correctly ensures:
allowNegativemust be true when setting a max negative balance- Value must be zero or negative
- Value respects the lower bound of -1 billion
This aligns with the client-side
MAX_NEGATIVE_LIMITconstant and provides proper server-side enforcement.
125-127: LGTM! Lower-bound guard completes the validation chain.The update path now has parity with the create path for the -1 billion limit.
platforms/eCurrency/client/src/pages/currency-detail.tsx (2)
25-25: LGTM! Client-side validation aligns with server-side constraints.The
MAX_NEGATIVE_LIMITof 1 billion matches the server-side limit inCurrencyService.ts. The magnitude-to-negative conversion logic is correct.Also applies to: 211-216
328-361: LGTM! UI improvements for max negative balance configuration.The numeric input approach is clearer than the previous slider-based UI. The responsive layout and button grouping are well-structured.
platforms/eCurrency/client/src/components/currency/create-currency-modal.tsx (2)
67-95: LGTM! Validation logic is consistent with currency-detail.tsx.The form submission correctly:
- Clears previous errors before validation
- Validates magnitude is non-negative and within limits
- Converts to negative value for the payload
- Resets
maxNegativeInputwhenallowNegativeis false
175-192: LGTM! Conditional UI for max negative balance input.The input is appropriately gated by
allowNegativeand provides clear guidance with the placeholder and helper text.platforms/eCurrency-api/src/controllers/LedgerController.ts (1)
298-327: LGTM! Group account initialization with proper authorization.The logic correctly:
- Derives
finalAccountTypewith a sensible default- Requires
accountIdfor GROUP accounts- Verifies admin permissions via
groupService.isGroupAdminThe
isGroupAdmin(finalAccountId, req.user.id)call at line 318 correctly passes the group ID as the first parameter and user ID as the second, matching the service signature shown in the relevant code snippets.
| interface AccountContext { | ||
| type: "user" | "group"; | ||
| id: string; | ||
| } | ||
|
|
||
| interface AddCurrencyAccountModalProps { | ||
| open: boolean; | ||
| onOpenChange: (open: boolean) => void; | ||
| accountContext?: AccountContext | null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for other "balances" query key usages
rg -n '["'"'"'"]balances' --type ts --type tsx -C 2Repository: MetaState-Prototype-Project/prototype
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Search for balancesQueryKey variable
rg -n 'balancesQueryKey' --type ts --type tsx -C 2Repository: MetaState-Prototype-Project/prototype
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Search for AccountContext type definition/usage
rg -n 'AccountContext' --type ts --type tsx -C 2Repository: MetaState-Prototype-Project/prototype
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Look for useQuery calls with balances
rg -n 'useQuery.*balances|balances.*useQuery' --type ts --type tsx -C 3Repository: MetaState-Prototype-Project/prototype
Length of output: 108
🏁 Script executed:
#!/bin/bash
# First, find the specific file mentioned in the review
fd -e tsx -e ts | grep -i "add-currency-account-modal"Repository: MetaState-Prototype-Project/prototype
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Search for balances without specifying tsx type
rg 'balancesQueryKey' -C 2Repository: MetaState-Prototype-Project/prototype
Length of output: 1840
🏁 Script executed:
#!/bin/bash
# Search for AccountContext usage
rg 'AccountContext' -C 2Repository: MetaState-Prototype-Project/prototype
Length of output: 10028
🏁 Script executed:
#!/bin/bash
# Broader search for balances query patterns
rg '"balances"' -C 2Repository: MetaState-Prototype-Project/prototype
Length of output: 3451
Fix query key structure mismatch and centralize AccountContext type
The query key inconsistency is a real issue. The balancesQueryKey in this modal uses a decomposed tuple structure ["balances", accountContext?.type, accountContext?.id], but other components (dashboard.tsx, transfer-modal.tsx) query balances with ["balances", accountContext] (the entire object as a single parameter). React Query won't match these keys, so:
- Invalidating with
balancesQueryKeyhere won't invalidate the balances query in the dashboard or transfer modal. - The modal's mutation success won't cascade cache updates to other screens showing balance data.
Additionally, when accountContext is undefined, the key becomes ["balances", undefined, undefined], creating yet another cache entry.
Also: The AccountContext type is defined locally and duplicated inline (as { type: "user" | "group"; id: string }) across at least 5 files (dashboard, currency-detail, user-menu-dropdown, account-context-switcher). Extract it to a shared types file to prevent divergence.
🤖 Prompt for AI Agents
In
platforms/eCurrency/client/src/components/currency/add-currency-account-modal.tsx
around lines 7 to 16, the modal constructs a mismatched React Query key and
duplicates the AccountContext type: change balancesQueryKey usage to match the
rest of the app by using the whole accountContext object as the second tuple
element (e.g. ["balances", accountContext]) instead of decomposing into type/id,
and ensure you normalize undefined to null (use accountContext ?? null) so you
don't create ["balances", undefined] entries; also extract the local
AccountContext interface into a shared types file (e.g.
platforms/eCurrency/client/src/types/account-context.ts), export it, and replace
local inline type definitions across dashboard.tsx, transfer-modal.tsx,
currency-detail, user-menu-dropdown, account-context-switcher and this modal
with an import of the centralized AccountContext type so all components use the
exact same type.
| <AddCurrencyAccountModal | ||
| open={addAccountOpen} | ||
| onOpenChange={setAddAccountOpen} | ||
| accountContext={accountContext} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'queryKey.*balances' --type=ts -C2Repository: MetaState-Prototype-Project/prototype
Length of output: 4044
🏁 Script executed:
cat -n platforms/eCurrency/client/src/components/currency/add-currency-account-modal.tsx | head -50Repository: MetaState-Prototype-Project/prototype
Length of output: 2228
🏁 Script executed:
sed -n '50,100p' platforms/eCurrency/client/src/pages/dashboard.tsxRepository: MetaState-Prototype-Project/prototype
Length of output: 2048
🏁 Script executed:
sed -n '70,80p' platforms/eCurrency/client/src/components/currency/add-currency-account-modal.tsxRepository: MetaState-Prototype-Project/prototype
Length of output: 404
Align query key structures between dashboard and AddCurrencyAccountModal to prevent stale data.
The accountContext prop is correctly passed to the modal. However, there is a confirmed cache key mismatch that will prevent proper query invalidation:
- Dashboard (line 96):
queryKey: ["balances", accountContext]where accountContext is an object - Modal (line 24):
balancesQueryKey = ["balances", accountContext?.type, accountContext?.id]spreads the object
React Query won't recognize these as the same key, so when the modal invalidates balances on success (line 75), the dashboard's query won't be refreshed, leaving stale balance data. Align the query key structure across both components—either use the object approach in the modal or use the spread approach in the dashboard.
🤖 Prompt for AI Agents
In platforms/eCurrency/client/src/pages/dashboard.tsx around lines 331 and
earlier at line ~96, the dashboard uses queryKey: ["balances", accountContext]
while AddCurrencyAccountModal (line ~24 and invalidation at ~75) uses
["balances", accountContext?.type, accountContext?.id]; update the dashboard to
construct the same key shape as the modal (e.g., ["balances",
accountContext?.type, accountContext?.id]) so React Query recognizes the same
cache entry and invalidations from the modal refresh the dashboard; ensure any
places that invalidate or refetch balances use the identical key structure.
Description of change
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.