Add multi currency support #43
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds multi-currency support with live FX rates from frankfurter.app, affecting both the FastAPI backend (new /fx/rates endpoint with 3-tier caching) and React frontend (CurrencyContext, formatting throughout components).
Critical Issue: There's a field name mismatch between frontend and backend - the frontend expects timestamp but the backend returns fetched_at, which will cause runtime failures.
Architecture Concern: The FxService in-memory cache is ineffective because a new instance is created per request. The three-tier caching strategy (memory → DB → fallback) only provides DB and fallback layers in practice.
The implementation shows good defensive programming with graceful degradation when the FX provider is unavailable. Consider adding integration tests that verify the complete flow from API endpoint to frontend formatting to catch type mismatches like the timestamp/fetched_at issue.
| if (!response.ok) { | ||
| throw new Error(`HTTP error! status: ${response.status}`) | ||
| } | ||
|
|
There was a problem hiding this comment.
Critical bug: The backend returns fetched_at in the RatesResponse schema, but this code tries to access data.timestamp. This will cause the currency system to fail at runtime. Change to data.fetched_at.
| if (!context) { | ||
| throw new Error('useCurrency must be used within CurrencyProvider') | ||
| } | ||
| return context |
There was a problem hiding this comment.
Code duplication: The useCurrency hook is defined here and also in frontend/src/hooks/useCurrency.ts. Remove this duplicate definition and import from the hooks file instead.
| ) | ||
|
|
||
| @app.get("/fx/rates", response_model=RatesResponse) | ||
| def get_fx_rates(session: Session = Depends(get_session)): |
There was a problem hiding this comment.
Performance issue: Creating a new FxService instance on every request defeats the purpose of the in-memory cache (_cache and _cache_fetched_at). The cache will never be reused across requests. Consider making FxService a singleton or using FastAPI's dependency injection with a cached instance.
|
|
||
| id: Optional[int] = Field(default=None, primary_key=True) | ||
| base: str = Field(default="USD") | ||
| rates: dict = Field(sa_column=Column(JSON)) |
There was a problem hiding this comment.
Potential bug: Using default_factory for fetched_at can cause issues when loading existing records from the database. SQLModel might re-evaluate this factory when hydrating objects, potentially overwriting the actual stored timestamp. Consider removing the default and setting this explicitly when creating new records, or use SQLAlchemy's server_default if you want database-level defaults.
|
|
||
| const detectCurrencyFromLocale = (): Currency => { | ||
| const locale = typeof navigator !== 'undefined' ? navigator.language : 'en-US' // eslint-disable-line no-undef | ||
|
|
There was a problem hiding this comment.
Code quality: The eslint-disable no-undef comment suggests improper handling. The navigator global is available in browser contexts. Consider using a proper TypeScript declaration or checking typeof navigator !== 'undefined' without disabling the linter.
|
|
||
| fetchRates() | ||
| }, []) | ||
|
|
There was a problem hiding this comment.
Logic issue: This effect attempts to refetch when stale becomes true, but it has stale and fetchedAt as dependencies. The effect won't trigger when rates become stale due to time passing - it only runs when these values change. The stale detection interval (lines 154-162) sets setStale(true) but this effect may not reliably trigger the refetch. Consider calling fetchRates() directly from the interval callback when rates are detected as stale.
| mock_datetime.UTC = UTC | ||
|
|
||
| response1 = client.get("/fx/rates") | ||
| assert response1.status_code == 200 |
There was a problem hiding this comment.
Test data mismatch: The mock provider response structure doesn't match what the FxService expects. The backend's fetch_from_provider method accesses data["rates"] directly (line 89 in currency.py), and the mock provides this correctly. However, the actual frankfurter.app API returns fetched_at not timestamp. Verify the test accurately reflects the real API response structure.
| "from": "USD", | ||
| "to": "GBP,EUR,AUD,MXN,JPY" | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing error handling: response.raise_for_status() will raise an exception for HTTP errors, but response.json() can also fail if the response body is not valid JSON. Consider wrapping the JSON parsing in a try-except block or validating the response structure before accessing nested fields.
Multi-Currency Support Implementation
Adds international currency localization supporting USD, GBP, EUR, AUD, MXN, JPY with live FX rates.
Features
Architecture
Backend (FastAPI):
FxServicewith intelligent cachingGET /fx/ratesendpointFxRatesSQLModel table for persistenceFrontend (React + TypeScript):
CurrencyContextfor global state and FX ratesuseCurrency()hook with convert/format functionsCurrencySelectorcomponent in headerKey Files
Backend (new):
backend/app/currency.py- FX service, models, schemasbackend/tests/api/test_currency.py- 7 comprehensive testsbackend/alembic/versions/50f63877083c_add_fx_rates_table.py- MigrationFrontend (new):
frontend/src/context/CurrencyContext.tsx- Currency state managementfrontend/src/hooks/useCurrency.ts- Custom hookfrontend/src/components/CurrencySelector.tsx+ testsfrontend/src/utils/currency.test.ts- 13 utility testsfrontend/e2e/tests/currency.spec.ts- 6 E2E testsUpdated (8 components):
All components now use
useCurrency().format():Cart.tsx,ProductCard.tsx,CartItem.tsx,CartItemMobile.tsx,Product.tsx,FeaturedBanner.tsx,DeliveryOptionsSelector.tsx,DeliveryOptionsSummary.tsxDocumentation:
README.md,AGENTS.md,backend/AGENTS.md,frontend/AGENTS.mdTesting
Implementation Notes