feat: real multi-bank dashboard with Demo/Live modes - #126
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds real multi-bank aggregation across banking, transaction, and dashboard services; expands the client with Demo/Live modes and multi-bank views; updates database schemas and seed data; and adds a local Docker Compose development stack. ChangesMulti-bank dashboard
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardController.java (1)
130-149: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPartial failure of the
connectionscall is silently swallowed and can misrepresent linked-bank state.
connectionStatusandconnectionsare fetched in the same try/catch. IfconnectionStatussucceeds but the subsequentconnectionscall throws, the catch discards both:connectionStatuswas already assigned (staysACTIVE) whileconnectionsstaysnull→[]. The client's empty-state gate usesconnectionStatus.status !== "ACTIVE", so this partial failure would render the full dashboard with "0 banks linked" instead of falling back to a degraded/retry state — and the log message ("continuing without connection status") is inaccurate for this case. Split into separate try/catch blocks so a failure on one call doesn't discard state already fetched successfully by the other, and log which specific call failed.🤖 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 `@server/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardController.java` around lines 130 - 149, Split the `connectionStatus` and `connections` WebClient requests in `DashboardController` into separate try/catch blocks, preserving any successfully fetched value when the other call fails. Update each warning to identify whether the status or connections request failed, and ensure the failed request’s fallback value drives the existing degraded/retry behavior rather than misrepresenting linked-bank state.
🧹 Nitpick comments (5)
server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java (1)
60-86: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBulk resync triggers N redundant
recomputeAggregatecalls.
syncAccountrecomputes the whole account aggregate on every call. WhenBankingController.sync()loopsactive.forEach(syncService::syncAccount)over N connections,recomputeAggregatere-queries and re-sums all active connections N times instead of once after the batch. Harmless at small N, but redundant DB work that scales quadratically with linked-bank count.🤖 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 `@server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java` around lines 60 - 86, Refactor the sync flow so bulk synchronization does not invoke recomputeAggregate once per connection. Remove the per-connection recomputeAggregate call from syncAccount, and have BankingController.sync trigger a single aggregate recomputation after all active connections have been synchronized, using the shared account identifier for the batch.infra/helm/banking-app/templates/init-sql-configmap.yaml (1)
20-41: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider indexing
connection_idgiven the new delete-by-connection hot path.
transactionRepository.deleteByConnectionId(...)and future connection-scoped reads will scantransactionsbyconnection_id, which has no index here. Fine at demo scale, but worth adding before real bank data grows this table.💡 Suggested index
); + CREATE INDEX IF NOT EXISTS idx_transactions_connection_id ON transactions (connection_id); + -- Zeroed anchor rows: hardcoded accountIds acting as user/profile aggregates🤖 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 `@infra/helm/banking-app/templates/init-sql-configmap.yaml` around lines 20 - 41, Add a database index on transactions.connection_id in the init SQL schema to support transactionRepository.deleteByConnectionId and future connection-scoped queries. Place the index definition alongside the transactions table creation, preserving the existing column definitions and seed data.server/banking-service/src/test/java/com/team/bank/banking/service/BankingSyncServiceTest.java (1)
119-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the mixed-currency aggregate branch.
Tests cover single-bank and multi-bank-same-currency aggregation, but not
recomputeAggregate'sbyCurrency.size() > 1branch (where the aggregate falls back to only theREPORTING_CURRENCYsubtotal, per the graph evidence inBankingSyncService.recomputeAggregate). That's the subtlest, most bug-prone path and currently untested — worth a dedicated case (e.g., one EUR bank + one USD bank) to lock in the documented behavior.🤖 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 `@server/banking-service/src/test/java/com/team/bank/banking/service/BankingSyncServiceTest.java` around lines 119 - 143, Add a dedicated test for the mixed-currency path in BankingSyncService.recomputeAggregate, using active banks with balances in different currencies such as EUR and USD. Assert that the aggregate balance falls back to only the REPORTING_CURRENCY subtotal, while preserving the existing same-currency test coverage.infra/docker/init.sql (1)
10-20: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMissing index on
transactions.connection_id. Both the fresh-install schema and the idempotent migration addconnection_idtotransactionswithout an index, yetdeleteByConnectionIdruns a scan-and-delete on this column on every single-connection sync (replace-by-connection). As transaction volume grows this becomes an increasingly expensive full-table scan on a hot path.
infra/docker/init.sql#L10-L20: addCREATE INDEX IF NOT EXISTS idx_transactions_connection_id ON transactions(connection_id);after thetransactionstable definition.infra/docker/migrate-multibank.sql#L21-L23: add the sameCREATE INDEX IF NOT EXISTS idx_transactions_connection_id ON transactions(connection_id);immediately after theconnection_idcolumn is added, so existing deployed databases get the index too.🤖 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 `@infra/docker/init.sql` around lines 10 - 20, Add the connection_id index in both schema paths: in infra/docker/init.sql after the transactions table definition, and in infra/docker/migrate-multibank.sql immediately after adding transactions.connection_id. Use CREATE INDEX IF NOT EXISTS with the shared idx_transactions_connection_id name so fresh installs and existing databases support deleteByConnectionId efficiently.server/banking-service/src/main/java/com/team/bank/banking/model/TransactionRepository.java (1)
3-17: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove
@Modifying, or switch to an explicit bulk delete.
deleteByConnectionId(...)is a derived delete, so@Modifyinghas no effect here. If the intent is a single SQLDELETEfor this sync path, replace it with@Query("delete from Transaction t where t.connectionId = :connectionId"); otherwise keep the derived method and drop@Modifying.🤖 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 `@server/banking-service/src/main/java/com/team/bank/banking/model/TransactionRepository.java` around lines 3 - 17, Remove the ineffective `@Modifying` annotation from deleteByConnectionId in TransactionRepository and keep the derived delete method with its existing transactional behavior.
🤖 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 `@client/src/App.tsx`:
- Around line 853-858: Update bankShare so it does not calculate percentages
against an EUR fallback when active connections contain mixed currencies: return
null unless the bank balance and total use the same currency, or use the
corresponding same-currency subtotal as the denominator. Preserve the existing
null and non-positive-total guards.
In `@docker-compose.dev.yml`:
- Around line 74-82: Add an extra_hosts mapping under the genai-service
definition so host.docker.internal resolves to host-gateway, preserving the
existing OLLAMA_BASE_URL and enabling host Ollama access on Linux.
- Around line 105-109: Update the client service configuration to provide
VITE_API_BASE_URL and VITE_ACCOUNT_ID during the image build via build.args,
matching the ARG names consumed by client/Dockerfile; do not rely solely on the
runtime environment block for these compiled frontend values.
In
`@server/banking-service/src/main/java/com/team/bank/banking/controller/BankingController.java`:
- Around line 182-197: Update the bulk resync loop in BankingController.sync so
each active BankingConnection invokes syncService.syncAccount independently
within its own try/catch. Isolate per-connection failures so one exception does
not prevent remaining banks from syncing or cause the request to fail, while
preserving the existing aggregate status response.
In
`@server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java`:
- Around line 196-212: Update the describe method’s remittance handling so any
List value, including an empty list, is processed as list content and cannot
fall through to the generic toString branch. Preserve the joined nonblank-line
result for nonempty lists, then fall back to counterparty or "Uncategorized"
when no usable remittance text exists.
- Around line 134-137: Update the sync flow around BankingSyncService and
EnableBankingClient.getTransactions to determine a stable lower bound before
deleting existing rows, using the oldest stored transaction or account opening
time, and pass it as date_from when fetching transactions. Ensure the lower
bound is established before
transactionRepository.deleteByConnectionId(connection.getId()) so aged
transactions are not omitted during replacement.
- Around line 122-167: Update syncTransactions to keep the Enable Banking fetch
and response validation outside a transaction, then delegate deletion and all
transaction saves to a dedicated `@Transactional` write method. Move transaction
mapping, deleteByConnectionId, and the save loop into that method so the delete
and reinserts commit or roll back atomically for each connection.
In
`@server/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardController.java`:
- Around line 96-107: Update the DashboardController transaction-loading flow
around the allTx WebClient call to avoid fetching and deserializing the
account’s unbounded history: use a transaction-service query with an appropriate
date range or pagination while preserving the data required by recent
transactions, monthlyFlow, and spendByBank. Also avoid adding another sequential
block on the dashboard path by composing this independent request with the
existing WebClient calls (for example, through Mono.zip) and adapting downstream
processing to the bounded result.
---
Outside diff comments:
In
`@server/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardController.java`:
- Around line 130-149: Split the `connectionStatus` and `connections` WebClient
requests in `DashboardController` into separate try/catch blocks, preserving any
successfully fetched value when the other call fails. Update each warning to
identify whether the status or connections request failed, and ensure the failed
request’s fallback value drives the existing degraded/retry behavior rather than
misrepresenting linked-bank state.
---
Nitpick comments:
In `@infra/docker/init.sql`:
- Around line 10-20: Add the connection_id index in both schema paths: in
infra/docker/init.sql after the transactions table definition, and in
infra/docker/migrate-multibank.sql immediately after adding
transactions.connection_id. Use CREATE INDEX IF NOT EXISTS with the shared
idx_transactions_connection_id name so fresh installs and existing databases
support deleteByConnectionId efficiently.
In `@infra/helm/banking-app/templates/init-sql-configmap.yaml`:
- Around line 20-41: Add a database index on transactions.connection_id in the
init SQL schema to support transactionRepository.deleteByConnectionId and future
connection-scoped queries. Place the index definition alongside the transactions
table creation, preserving the existing column definitions and seed data.
In
`@server/banking-service/src/main/java/com/team/bank/banking/model/TransactionRepository.java`:
- Around line 3-17: Remove the ineffective `@Modifying` annotation from
deleteByConnectionId in TransactionRepository and keep the derived delete method
with its existing transactional behavior.
In
`@server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java`:
- Around line 60-86: Refactor the sync flow so bulk synchronization does not
invoke recomputeAggregate once per connection. Remove the per-connection
recomputeAggregate call from syncAccount, and have BankingController.sync
trigger a single aggregate recomputation after all active connections have been
synchronized, using the shared account identifier for the batch.
In
`@server/banking-service/src/test/java/com/team/bank/banking/service/BankingSyncServiceTest.java`:
- Around line 119-143: Add a dedicated test for the mixed-currency path in
BankingSyncService.recomputeAggregate, using active banks with balances in
different currencies such as EUR and USD. Assert that the aggregate balance
falls back to only the REPORTING_CURRENCY subtotal, while preserving the
existing same-currency test coverage.
🪄 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: 85a4284d-19e6-4ce7-bd83-e9024b37460b
📒 Files selected for processing (21)
client/src/App.test.tsxclient/src/App.tsxclient/src/api.tsclient/src/styles/app.cssdocker-compose.dev.ymlinfra/docker/init.sqlinfra/docker/migrate-multibank.sqlinfra/helm/banking-app/templates/init-sql-configmap.yamlserver/banking-service/src/main/java/com/team/bank/banking/controller/BankingController.javaserver/banking-service/src/main/java/com/team/bank/banking/dto/ConnectionInfo.javaserver/banking-service/src/main/java/com/team/bank/banking/model/BankingConnection.javaserver/banking-service/src/main/java/com/team/bank/banking/model/BankingConnectionRepository.javaserver/banking-service/src/main/java/com/team/bank/banking/model/Transaction.javaserver/banking-service/src/main/java/com/team/bank/banking/model/TransactionRepository.javaserver/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.javaserver/banking-service/src/test/java/com/team/bank/banking/service/BankingSyncServiceTest.javaserver/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardController.javaserver/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardModels.javaserver/transaction-service/src/main/java/com/team/bank/transaction/Transaction.javaserver/transaction-service/src/main/java/com/team/bank/transaction/TransactionController.javaserver/transaction-service/src/main/java/com/team/bank/transaction/TransactionItem.java
There was a problem hiding this comment.
Pull request overview
This PR upgrades the dashboard from seeded/single-bank demo data to real multi-bank aggregation across banking-service, orchestrator-service, and the client, adding Demo/Live account switching and new per-bank/transaction metadata to support richer UI breakdowns.
Changes:
- Persist and expose per-connection bank details (account name, balance, currency) and per-transaction bank/counterparty metadata; migrate schema accordingly.
- Orchestrate and render a true multi-bank dashboard: linked-banks roster, monthly in/out/net, spending-by-bank, and recent transactions feed.
- Add client-side Demo/Live mode toggle (persisted) and improved “Find banks” UX (filterable + dismissible list).
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/transaction-service/src/main/java/com/team/bank/transaction/TransactionItem.java | Extends transaction DTO to include bankName and counterparty. |
| server/transaction-service/src/main/java/com/team/bank/transaction/TransactionController.java | Forwards bankName/counterparty in transaction list responses. |
| server/transaction-service/src/main/java/com/team/bank/transaction/Transaction.java | Adds bankName/counterparty columns to transaction entity. |
| server/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardModels.java | Expands dashboard payload models to include connection roster, transactions, monthly flow, and spend-by-bank. |
| server/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardController.java | Aggregates new dashboard sections by calling transaction-service and banking-service, computing monthly flow and spend-by-bank. |
| server/banking-service/src/test/java/com/team/bank/banking/service/BankingSyncServiceTest.java | Adds tests for balance selection, replace-by-connection tx sync, aggregate recompute, and snake_case/camelCase parsing. |
| server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java | Implements per-connection balance/currency sync, replace-by-connection transaction sync, and aggregate recomputation across ACTIVE connections. |
| server/banking-service/src/main/java/com/team/bank/banking/model/TransactionRepository.java | Replaces best-effort dedup query with delete-by-connection idempotency primitive. |
| server/banking-service/src/main/java/com/team/bank/banking/model/Transaction.java | Adds bank_name/connection_id/counterparty fields to synced transactions. |
| server/banking-service/src/main/java/com/team/bank/banking/model/BankingConnectionRepository.java | Changes ACTIVE lookup to return a list for multi-bank. |
| server/banking-service/src/main/java/com/team/bank/banking/model/BankingConnection.java | Adds per-connection accountName/balance/currency fields. |
| server/banking-service/src/main/java/com/team/bank/banking/dto/ConnectionInfo.java | Introduces DTO for linked-bank roster entries. |
| server/banking-service/src/main/java/com/team/bank/banking/controller/BankingController.java | Ensures anchor account exists pre-link, syncs all ACTIVE connections, and adds /connections/{accountId} roster endpoint. |
| infra/helm/banking-app/templates/init-sql-configmap.yaml | Updates schema + seed anchors for Demo/Live in helm init SQL. |
| infra/docker/migrate-multibank.sql | Adds idempotent migration for deployed DBs (new columns + seeds + cleanup). |
| infra/docker/init.sql | Updates local init schema/seed for Demo+Live anchors and new columns. |
| docker-compose.dev.yml | Adds a dev compose stack including postgres + services, wiring URLs and init.sql. |
| client/src/styles/app.css | Styles for Demo/Live toggle, bank list panel, monthly tiles, and transaction feed. |
| client/src/App.tsx | Implements Demo/Live mode, multi-bank roster rendering, spending-by-bank, recent transactions feed, and improved bank list UX. |
| client/src/App.test.tsx | Updates/extends tests for new payload shape, roster labeling, bank list filter/close, and Demo/Live switching. |
| client/src/api.ts | Updates TS models for new dashboard fields and adds backward-compatible defaults during rolling deploys. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Closes #125
Replaces the seeded dashboard data with real aggregated Enable Banking data.
1111…), Live the production account (2222…)init.sqland the helm configmap;infra/docker/migrate-multibank.sqlpatches existing DBsDeploy notes: run
migrate-multibank.sqlbefore rolling out banking-service (schema isddl-auto: none), then link real banks while in Live mode.GenAI chat-context changes ship in a separate PR.
Summary by CodeRabbit
New Features
Bug Fixes
Summary by CodeRabbit
New Features
Bug Fixes
Tests