feat: add per-item currency support with server-side exchange rate co…

[BKSalman]

resolves #38

these changes were AI assisted, since I'm currently on a trip 😅, but I would really appreciate this feature being added

if there is anything you don't like or think is AI slop, I can manually change it

[mauriceboe]

Hey, nice work on this. The overall design is solid: per-item currency with server-side conversion, SQLite-cached rates with stale fallback, WebSocket propagation, and all aggregations correctly using COALESCE(converted_price, total_price).

Found one bug though: recalculateTrip in exchangeRates.ts fetches rates for the base currency (e.g. EUR) via fetchAndCacheRates(baseCurrency), which caches EUR→JPY, EUR→USD etc. But then it looks up getRate(itemCur, baseCurrency) which searches for e.g. JPY→EUR — that direction isn't in the cache, so it returns null. Individual item creation works fine because convertAmount correctly calls fetchAndCacheRates(from), but recalculateTrip (called on currency change and manual refresh) would set converted_price = null for all non-base-currency items.

Simplest fix: use convertAmount(item.total_price, itemCur, baseCurrency) per item inside recalculateTrip instead of the manual rate lookup, or fetch rates for each unique item currency rather than just the base currency.

Also the package-lock.json changes add "peer": true to a bunch of unrelated packages, probably from a different npm version. Would be good to clean that up to keep the diff focused.

[BKSalman]

ok, now it should be fixed, with other small bug fixes, and I also added the tables to the schema.ts file

please tell me if there is anything else wrong with it

[mauriceboe]

Hey, thanks for the update – the recalculateTrip bug is fixed correctly now, nice work.

Found a couple more things though:

1. Schema drift with paid_by_user_id: It's in schema.ts (CREATE TABLE) but not in the migration. Fresh databases will have the column, existing ones won't. Since it's unused right now, I'd just remove it from schema.ts until it's actually needed – otherwise it'll cause confusion down the line.

2. No validation on item_currency: The value from req.body goes straight into the DB and the API URL without any check. At minimum validate it's a 3-letter uppercase string, ideally check against a currency list.

3. No timeout on the external fetch: If exchangerate-api.com hangs, the request hangs forever. An AbortSignal.timeout(10000) or similar would be good.

These are pretty quick fixes. Once 1 and 2 are sorted I'm happy to merge.

[BKSalman]

@mauriceboe Thank you for your time, it should be good now

[BKSalman]

just resolved conflicts, and tested the basic functionality, and it seems like it's working

[mauriceboe]

Hey, nice work on this. The overall approach is solid and the earlier feedback was addressed well. Found a few more things though:

Bugs

1. item_currency not normalized on update
createBudgetItem correctly does .toUpperCase(), but updateBudgetItem writes data.item_currency straight into the DB without normalizing. If someone sends "eur" via PUT, it lands lowercase in the DB. The route validation (isValidCurrency) accepts it because it checks val.toUpperCase() but passes the original value through. Easy fix: normalize in updateBudgetItem before passing to the query.

2. Race condition in recalculateTrip – parallel API fetches
Promise.all over all items means if 10 items share the same foreign currency, all 10 hit the cache miss simultaneously and fire 10 parallel fetchAndCacheRates("JPY") calls before the first one finishes caching. Could trigger rate limits on the external API.

Fix: collect unique item currencies first, fetch them sequentially (or deduplicated), then convert locally.

Security

3. refresh-rates doesn't check budget_edit permission
The endpoint only checks verifyTripAccess – any trip member (even read-only) can spam rate refreshes and hammer the external API. Should either require budget_edit or have some rate limiting.

Cleanup

• formatConvertedAmount in formatters.ts is exported but never imported anywhere – dead code.
• The package-lock.json still has the unrelated "peer": true additions from the earlier review.

1–3 should be addressed before merge, the rest is nice-to-have.

[BKSalman]

@mauriceboe ok, done 👍

[BKSalman]

fixed failing tests

[BKSalman]

resolved conflicts, and removed "peer": true from package-lock.json