feat(genai): multi-bank chat context and summary - #128
Closed
azzabaatout wants to merge 9 commits into
Closed
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds end-to-end multi-bank dashboard support: connection and transaction synchronization, aggregated analytics, expanded API and GenAI context models, demo/live account switching, filtered bank selection, richer dashboard views, database migrations, and a local Compose development stack. ChangesMulti-bank dashboard
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 10
🧹 Nitpick comments (2)
server/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardController.java (2)
68-145: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftParallelize independent downstream requests to reduce latency.
The dashboard endpoint fetches data from multiple downstream services using sequential blocking calls (
account,trend,expenses,allTx,summary, andconnections). Since these requests do not depend on each other, waiting for each to complete sequentially significantly increases the total response time and degrades the dashboard's load speed.Consider leveraging WebFlux to fetch these independent resources concurrently (e.g., using
Mono.zip()) before returning the aggregated response.🤖 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 68 - 145, The dashboard aggregation flow should issue the independent account, trend, expenses, allTx, summary, and banking connection requests concurrently instead of blocking after each request. Refactor the request orchestration around the visible WebClient calls in DashboardController to compose them with WebFlux operators such as Mono.zip, then extract their results while preserving the existing null handling and optional genai fallback behavior.
103-104: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse
Arrays.stream(allTx)here to avoid creating an intermediate immutable list just to stream and limit it.🤖 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 103 - 104, Update the transaction conversion in DashboardController to use Arrays.stream(allTx).limit(RECENT_TX_LIMIT).toList() when allTx is non-null, while preserving the existing empty-list result for null values. Remove the intermediate List.of(allTx) creation.
🤖 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 50-59: Update formatBankBalance to validate the selected currency
code before constructing Intl.NumberFormat, falling back to EUR for malformed or
unsupported non-empty values. Preserve the existing em-dash behavior for null
balances and normal currency formatting for valid codes, ensuring invalid bank
payloads cannot throw during Dashboard rendering.
- Around line 847-868: Update bankShare in the App component to avoid
calculating percentages from mixed-currency values: either convert each bank
balance and data.account.totalBalance to the same currency before division, or
return null when a common-currency conversion is unavailable. Preserve the
existing null and non-positive-total guards and only display a share when the
values are comparable.
In `@docker-compose.dev.yml`:
- Around line 79-82: Add an extra_hosts entry to the genai-service configuration
so host.docker.internal resolves to the Docker host gateway on Linux, while
preserving the existing Ollama environment variables and URL.
- Around line 105-113: Update the client service configuration to reflect that
the prebuilt Nginx image cannot consume VITE_API_BASE_URL or VITE_ACCOUNT_ID at
runtime: either add the appropriate build context so these values are applied
during the client build, or remove the unused VITE_* environment entries if this
service is not intended for local UI development.
- Around line 11-13: Update the PostgreSQL service volumes in
docker-compose.dev.yml to mount infra/docker/migrate-multibank.sql into
/docker-entrypoint-initdb.d alongside init.sql, preserving read-only access so
fresh postgres-data volumes apply the multi-bank schema and seed updates.
In
`@server/banking-service/src/main/java/com/team/bank/banking/controller/BankingController.java`:
- Around line 223-235: Update ensureAnchorAccount to make anchor creation
atomic: replace the existsById-then-save sequence with an insert-on-conflict
repository operation, or catch and safely ignore the duplicate-key exception
from a concurrent insert. Preserve the existing account field values and
successful no-op behavior when the anchor already exists.
- Around line 184-190: Update the sync flow in BankingController around
active.forEach(syncService::syncAccount) to process each BankingConnection
independently, catching failures per connection so later banks still
synchronize. Track whether any connection failed and return an explicit
partial-failure response after attempting all syncs, while preserving the
existing empty-active response and successful response behavior.
In
`@server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java`:
- Around line 140-164: Define a single reporting-currency contract across all
affected sites: in
server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java:140-164,
retain each transaction’s source currency or convert its amount to EUR before
persistence; if retaining it, add the transaction currency column in
infra/docker/init.sql:13-19 and map that field in
server/banking-service/src/main/java/com/team/bank/banking/model/Transaction.java:29-37.
In
server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java:243-265,
convert non-EUR balances to EUR or preserve their aggregate currency instead of
labeling raw values as euros.
- Around line 146-151: Update the direction classification in BankingSyncService
to recognize supported credit and debit indicator values explicitly; do not use
DEBIT as the fallback. When credit_debit_indicator is missing or unsupported,
skip or reject the transaction according to the service’s existing handling
conventions, while preserving known CREDIT and DEBIT behavior.
- Around line 134-138: Make syncAccount() transactional so
deleteByConnectionId(), transaction saves, and the aggregate recompute execute
within one transaction. Ensure any failure rolls back the entire
replace-by-connection operation, preserving the existing synchronization
behavior while preventing empty or partially reinserted connection data.
---
Nitpick comments:
In
`@server/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardController.java`:
- Around line 68-145: The dashboard aggregation flow should issue the
independent account, trend, expenses, allTx, summary, and banking connection
requests concurrently instead of blocking after each request. Refactor the
request orchestration around the visible WebClient calls in DashboardController
to compose them with WebFlux operators such as Mono.zip, then extract their
results while preserving the existing null handling and optional genai fallback
behavior.
- Around line 103-104: Update the transaction conversion in DashboardController
to use Arrays.stream(allTx).limit(RECENT_TX_LIMIT).toList() when allTx is
non-null, while preserving the existing empty-list result for null values.
Remove the intermediate List.of(allTx) creation.
🪄 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: 6a505ad4-2a0b-488a-bb55-aac3fc950df9
📒 Files selected for processing (23)
client/src/App.test.tsxclient/src/App.tsxclient/src/api.tsclient/src/styles/app.cssdocker-compose.dev.ymlgenai/main.pygenai/tests/test_main.pyinfra/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
Comment on lines
+50
to
+59
| function formatBankBalance(value: number | null, currency: string | null): string { | ||
| if (value === null) { | ||
| return "—"; | ||
| } | ||
| return new Intl.NumberFormat("en-US", { | ||
| style: "currency", | ||
| currency: currency || "EUR", | ||
| maximumFractionDigits: 0, | ||
| }).format(value); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== App.tsx around formatBankBalance =="
ast-grep outline client/src/App.tsx --view expanded || true
echo
sed -n '1,120p' client/src/App.tsx | cat -n
echo
echo "== Search for error boundaries / Dashboard render =="
rg -n "ErrorBoundary|error boundary|Dashboard" client/src/App.tsx client/src -g '!**/*.map' || true
echo
echo "== Node runtime probe for Intl.NumberFormat invalid currency =="
node - <<'JS'
for (const currency of ["EUR", "USD", "", null, "not-a-code", "123", "€"]) {
try {
const opts = { style: "currency", currency: currency || "EUR", maximumFractionDigits: 0 };
const out = new Intl.NumberFormat("en-US", opts).format(1234);
console.log(JSON.stringify({ currency, ok: true, out }));
} catch (e) {
console.log(JSON.stringify({ currency, ok: false, name: e.name, message: e.message }));
}
}
JSRepository: AET-DevOps26/team-team
Length of output: 11345
Guard formatBankBalance against invalid currency codes
currency || "EUR" only covers falsy values; malformed non-empty codes still make Intl.NumberFormat throw a RangeError. Since this runs during Dashboard render, one bad bank payload can take down the whole view. Validate or fall back before constructing the formatter.
🧰 Tools
🪛 GitHub Check: build-test
[warning] 54-54:
Expected blank line before this statement
🤖 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 `@client/src/App.tsx` around lines 50 - 59, Update formatBankBalance to
validate the selected currency code before constructing Intl.NumberFormat,
falling back to EUR for malformed or unsupported non-empty values. Preserve the
existing em-dash behavior for null balances and normal currency formatting for
valid codes, ensuring invalid bank payloads cannot throw during Dashboard
rendering.
Comment on lines
+847
to
+868
| const bankCount = data.connections.length; | ||
| const bankLabel = bankCount === 1 ? "bank" : "banks"; | ||
| const total = data.account.totalBalance; | ||
| const flow = data.monthlyFlow; | ||
| const maxBankSpend = data.spendByBank.reduce((m, b) => Math.max(m, b.spending), 0); | ||
|
|
||
| const bankShare = (balance: number | null): string | null => { | ||
| if (balance === null || total <= 0) { | ||
| return null; | ||
| } | ||
| return `${Math.round((balance / total) * 100)}%`; | ||
| }; | ||
|
|
||
| return ( | ||
| <main className="term"> | ||
| <header className="statusbar"> | ||
| <span className="live"> | ||
| <span className="dot" />1 bank linked | ||
| <span className="dot" /> | ||
| {bankCount} {bankLabel} linked | ||
| </span> | ||
| <span className="right"> | ||
| <ModeToggle mode={mode} onChange={onModeChange} /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby symbols
ast-grep outline client/src/App.tsx --view expanded > /tmp/app_outline.txt
sed -n '1,220p' /tmp/app_outline.txt
printf '\n--- App.tsx excerpt around the cited lines ---\n'
sed -n '820,960p' client/src/App.tsx | cat -n
printf '\n--- Search for bankShare usages and currency fields ---\n'
rg -n "bankShare\\(|currency|totalBalance|monthlyFlow|spendByBank|balance" client/src/App.tsx client/src -g '!**/*.map'
printf '\n--- Relevant type/data definitions ---\n'
git ls-files | rg 'client/src/.*\\.(ts|tsx)$' | xargs -r rg -n "type .*currency|interface .*currency|totalBalance|spendByBank|connections|monthlyFlow|balance:"Repository: AET-DevOps26/team-team
Length of output: 14357
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '840,940p' client/src/App.tsx | cat -nRepository: AET-DevOps26/team-team
Length of output: 4149
bankShare should not compare mixed-currency balances
bankShare divides a per-bank balance by data.account.totalBalance, but the UI already formats individual banks in their own currencies. For mixed-currency accounts, that percentage is not meaningful; convert both values to a common currency first or omit the share.
🤖 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 `@client/src/App.tsx` around lines 847 - 868, Update bankShare in the App
component to avoid calculating percentages from mixed-currency values: either
convert each bank balance and data.account.totalBalance to the same currency
before division, or return null when a common-currency conversion is
unavailable. Preserve the existing null and non-positive-total guards and only
display a share when the values are comparable.
Comment on lines
+11
to
+13
| volumes: | ||
| - postgres-data:/var/lib/postgresql/data | ||
| - ./infra/docker/init.sql:/docker-entrypoint-initdb.d/init.sql:ro |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if migrate-multibank.sql is automatically sourced by init.sql
grep -i "migrate" infra/docker/init.sql || echo "Migration script is not sourced internally. It needs to be mounted."Repository: AET-DevOps26/team-team
Length of output: 228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Files\n'
git ls-files 'docker-compose.dev.yml' 'infra/docker/init.sql' 'infra/docker/migrate-multibank.sql' || true
printf '\n## docker-compose.dev.yml excerpt\n'
cat -n docker-compose.dev.yml | sed -n '1,40p'
printf '\n## infra/docker/init.sql excerpt\n'
cat -n infra/docker/init.sql | sed -n '1,200p'
printf '\n## migrate-multibank.sql presence\n'
if [ -f infra/docker/migrate-multibank.sql ]; then
wc -l infra/docker/migrate-multibank.sql
cat -n infra/docker/migrate-multibank.sql | sed -n '1,80p'
else
echo 'infra/docker/migrate-multibank.sql not found'
fiRepository: AET-DevOps26/team-team
Length of output: 7176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## docker-compose.dev.yml around postgres service\n'
sed -n '1,30p' docker-compose.dev.yml
printf '\n## search for migrate-multibank references\n'
rg -n "migrate-multibank|docker-entrypoint-initdb.d|init.sql" -S .Repository: AET-DevOps26/team-team
Length of output: 2614
Mount infra/docker/migrate-multibank.sql with the init scripts. infra/docker/init.sql doesn’t source it, so fresh postgres-data volumes will miss the multi-bank schema changes and seed updates.
Suggested fix
volumes:
- postgres-data:/var/lib/postgresql/data
- ./infra/docker/init.sql:/docker-entrypoint-initdb.d/init.sql:ro
+ - ./infra/docker/migrate-multibank.sql:/docker-entrypoint-initdb.d/migrate-multibank.sql:ro📝 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
| volumes: | |
| - postgres-data:/var/lib/postgresql/data | |
| - ./infra/docker/init.sql:/docker-entrypoint-initdb.d/init.sql:ro | |
| volumes: | |
| - postgres-data:/var/lib/postgresql/data | |
| - ./infra/docker/init.sql:/docker-entrypoint-initdb.d/init.sql:ro | |
| - ./infra/docker/migrate-multibank.sql:/docker-entrypoint-initdb.d/migrate-multibank.sql:ro |
🤖 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 `@docker-compose.dev.yml` around lines 11 - 13, Update the PostgreSQL service
volumes in docker-compose.dev.yml to mount infra/docker/migrate-multibank.sql
into /docker-entrypoint-initdb.d alongside init.sql, preserving read-only access
so fresh postgres-data volumes apply the multi-bank schema and seed updates.
Comment on lines
+79
to
+82
| environment: | ||
| MODEL_PROVIDER: local | ||
| OLLAMA_BASE_URL: http://host.docker.internal:11434 | ||
| OLLAMA_MODEL: llama3.1:8b |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Ensure host.docker.internal resolves on Linux.
The genai-service connects to a local Ollama instance using http://host.docker.internal:11434. While this works natively on Docker Desktop (Mac/Windows), it often fails to resolve on Linux Docker Engine. Add an extra_hosts configuration to the service to ensure consistent cross-platform functionality for all developers.
💻 Proposed fix
environment:
MODEL_PROVIDER: local
OLLAMA_BASE_URL: http://host.docker.internal:11434
OLLAMA_MODEL: llama3.1:8b
+ extra_hosts:
+ - "host.docker.internal:host-gateway"📝 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
| environment: | |
| MODEL_PROVIDER: local | |
| OLLAMA_BASE_URL: http://host.docker.internal:11434 | |
| OLLAMA_MODEL: llama3.1:8b | |
| environment: | |
| MODEL_PROVIDER: local | |
| OLLAMA_BASE_URL: http://host.docker.internal:11434 | |
| OLLAMA_MODEL: llama3.1:8b | |
| extra_hosts: | |
| - "host.docker.internal:host-gateway" |
🤖 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 `@docker-compose.dev.yml` around lines 79 - 82, Add an extra_hosts entry to the
genai-service configuration so host.docker.internal resolves to the Docker host
gateway on Linux, while preserving the existing Ollama environment variables and
URL.
Comment on lines
+105
to
+113
| client: | ||
| image: ghcr.io/aet-devops26/client:latest | ||
| environment: | ||
| VITE_API_BASE_URL: http://localhost:8083 | ||
| VITE_ACCOUNT_ID: 111-222 | ||
| depends_on: | ||
| - orchestrator-service | ||
| ports: | ||
| - "3000:80" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the client Dockerfile performs runtime env substitution, or if it needs to be built locally
cat client/DockerfileRepository: AET-DevOps26/team-team
Length of output: 509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docker-compose.dev.yml: client service =="
sed -n '95,125p' docker-compose.dev.yml
echo
echo "== repository files related to client images/builds =="
git ls-files | rg '(^|/)(client|docker-compose.*|Dockerfile|nginx\.conf)$'
echo
echo "== client Dockerfile =="
cat client/DockerfileRepository: AET-DevOps26/team-team
Length of output: 1621
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docker-compose.yml: client service =="
sed -n '1,140p' docker-compose.yml
echo
echo "== client/nginx.conf =="
cat client/nginx.confRepository: AET-DevOps26/team-team
Length of output: 4333
VITE_* env vars won’t affect this client image
This service runs a prebuilt Nginx image, so environment values won’t be injected into the Vite bundle at runtime. If this compose file is meant for local UI work, add a build context or use a dev server; otherwise remove the unused VITE_* vars.
🤖 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 `@docker-compose.dev.yml` around lines 105 - 113, Update the client service
configuration to reflect that the prebuilt Nginx image cannot consume
VITE_API_BASE_URL or VITE_ACCOUNT_ID at runtime: either add the appropriate
build context so these values are applied during the client build, or remove the
unused VITE_* environment entries if this service is not intended for local UI
development.
Comment on lines
+184
to
+190
| List<BankingConnection> active = | ||
| connectionRepository.findByAccountIdAndStatus(accountId, "ACTIVE"); | ||
| if (optConnection.isEmpty()) { | ||
| if (active.isEmpty()) { | ||
| return ResponseEntity.badRequest().build(); | ||
| } | ||
| BankingConnection connection = optConnection.get(); | ||
| syncService.syncAccount(connection); | ||
| // Re-sync every linked bank; each syncAccount recomputes the shared aggregate. | ||
| active.forEach(syncService::syncAccount); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Isolate failures between linked banks.
An exception from one bank aborts the loop, prevents later connections from syncing, and returns a blanket 500 after partial work. Handle each connection independently and report partial failure explicitly.
🤖 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/controller/BankingController.java`
around lines 184 - 190, Update the sync flow in BankingController around
active.forEach(syncService::syncAccount) to process each BankingConnection
independently, catching failures per connection so later banks still
synchronize. Track whether any connection failed and return an explicit
partial-failure response after attempting all syncs, while preserving the
existing empty-active response and successful response behavior.
Comment on lines
+223
to
+235
| /** Creates the zeroed profile/anchor account row if it does not already exist. */ | ||
| private void ensureAnchorAccount(UUID accountId, LocalDateTime now) { | ||
| if (accountRepository.existsById(accountId)) { | ||
| return; | ||
| } | ||
| Account account = new Account(); | ||
| account.setId(accountId); | ||
| account.setCustomerName("My accounts"); | ||
| account.setAccountType("AGGREGATE"); | ||
| account.setBalance(BigDecimal.ZERO); | ||
| account.setCreditLimit(BigDecimal.ZERO); | ||
| account.setUpdatedAt(now); | ||
| accountRepository.save(account); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make anchor creation atomic.
Concurrent first-connect requests can both pass existsById() and race to insert the same account ID, causing one request to fail. Use an insert-on-conflict operation or safely handle the duplicate-key race.
🤖 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/controller/BankingController.java`
around lines 223 - 235, Update ensureAnchorAccount to make anchor creation
atomic: replace the existsById-then-save sequence with an insert-on-conflict
repository operation, or catch and safely ignore the duplicate-key exception
from a concurrent insert. Preserve the existing account field values and
successful no-op behavior when the anchor already exists.
Comment on lines
+134
to
+138
| // Replace-by-connection: Enable Banking returns the full recent window on every call, so | ||
| // clearing this connection's rows and re-inserting keeps repeated syncs idempotent without a | ||
| // stable external transaction id (and never drops genuinely repeated payments). | ||
| transactionRepository.deleteByConnectionId(connection.getId()); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 '`@Transactional`|deleteByConnectionId|void syncAccount|transactionRepository\.save' \
server/banking-service/src/main/javaRepository: AET-DevOps26/team-team
Length of output: 4004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== BankingSyncService annotations and method body ==\n'
sed -n '1,220p' server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java | cat -n
printf '\n== TransactionRepository ==\n'
sed -n '1,120p' server/banking-service/src/main/java/com/team/bank/banking/model/TransactionRepository.java | cat -nRepository: AET-DevOps26/team-team
Length of output: 11386
Make the replace-by-connection sync atomic.
deleteByConnectionId() and the later save() calls each run in their own transactions, so a failure mid-sync can leave this connection empty or only partially reinserted. Wrap syncAccount() in one transaction, including the aggregate recompute, so the replacement is all-or-nothing.
🤖 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 134 - 138, Make syncAccount() transactional so
deleteByConnectionId(), transaction saves, and the aggregate recompute execute
within one transaction. Ensure any failure rolls back the entire
replace-by-connection operation, preserving the existing synchronization
behavior while preventing empty or partially reinserted connection data.
Comment on lines
+140
to
+164
| // transaction_amount is a nested object ({amount, currency}) just like balance_amount, so | ||
| // read the inner "amount" rather than stringifying the whole map. | ||
| BigDecimal txAmount = parseAmount(field(tx, "transaction_amount")); | ||
| if (txAmount == null) { | ||
| continue; | ||
| } | ||
| String creditDebitIndicator = String.valueOf(field(tx, "credit_debit_indicator")); | ||
| String direction = | ||
| "CRDT".equalsIgnoreCase(creditDebitIndicator) | ||
| || "CREDIT".equalsIgnoreCase(creditDebitIndicator) | ||
| ? "CREDIT" | ||
| : "DEBIT"; | ||
|
|
||
| String counterparty = counterpartyName(tx, direction); | ||
|
|
||
| Transaction newTx = new Transaction(); | ||
| newTx.setId(UUID.randomUUID()); | ||
| newTx.setAccountId(connection.getAccountId()); | ||
| newTx.setConnectionId(connection.getId()); | ||
| newTx.setBankName(connection.getBankName()); | ||
| newTx.setCounterparty(counterparty); | ||
| newTx.setCategory(describe(tx, counterparty)); | ||
| newTx.setAmount(txAmount); | ||
| newTx.setDirection(direction); | ||
| newTx.setCreatedAt(parseTransactionDate(tx)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define one end-to-end reporting-currency contract.
Source currencies are discarded or raw non-EUR values are presented as euros, making multi-currency balances and analytics numerically incorrect.
server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java#L140-L164: retain transaction currency or convert amounts to EUR before persistence.infra/docker/init.sql#L13-L19: add a transaction currency column if source amounts remain unconverted.server/banking-service/src/main/java/com/team/bank/banking/model/Transaction.java#L29-L37: map the currency field through the JPA entity.server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java#L243-L265: convert non-EUR balances or preserve aggregate currency rather than labeling raw values as euros.
📍 Affects 3 files
server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java#L140-L164(this comment)infra/docker/init.sql#L13-L19server/banking-service/src/main/java/com/team/bank/banking/model/Transaction.java#L29-L37server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java#L243-L265
🤖 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 140 - 164, Define a single reporting-currency contract across all
affected sites: in
server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java:140-164,
retain each transaction’s source currency or convert its amount to EUR before
persistence; if retaining it, add the transaction currency column in
infra/docker/init.sql:13-19 and map that field in
server/banking-service/src/main/java/com/team/bank/banking/model/Transaction.java:29-37.
In
server/banking-service/src/main/java/com/team/bank/banking/service/BankingSyncService.java:243-265,
convert non-EUR balances to EUR or preserve their aggregate currency instead of
labeling raw values as euros.
Comment on lines
+146
to
+151
| String creditDebitIndicator = String.valueOf(field(tx, "credit_debit_indicator")); | ||
| String direction = | ||
| "CRDT".equalsIgnoreCase(creditDebitIndicator) | ||
| || "CREDIT".equalsIgnoreCase(creditDebitIndicator) | ||
| ? "CREDIT" | ||
| : "DEBIT"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not classify unknown indicators as debits.
A missing or unsupported credit_debit_indicator currently becomes DEBIT, incorrectly inflating spending. Accept known debit values explicitly and skip or reject unknown values.
Proposed fix
String direction =
"CRDT".equalsIgnoreCase(creditDebitIndicator)
|| "CREDIT".equalsIgnoreCase(creditDebitIndicator)
? "CREDIT"
- : "DEBIT";
+ : "DBIT".equalsIgnoreCase(creditDebitIndicator)
+ || "DEBIT".equalsIgnoreCase(creditDebitIndicator)
+ ? "DEBIT"
+ : null;
+ if (direction == null) {
+ log.warn("Skipping transaction with unknown direction '{}'", creditDebitIndicator);
+ continue;
+ }📝 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
| String creditDebitIndicator = String.valueOf(field(tx, "credit_debit_indicator")); | |
| String direction = | |
| "CRDT".equalsIgnoreCase(creditDebitIndicator) | |
| || "CREDIT".equalsIgnoreCase(creditDebitIndicator) | |
| ? "CREDIT" | |
| : "DEBIT"; | |
| String creditDebitIndicator = String.valueOf(field(tx, "credit_debit_indicator")); | |
| String direction = | |
| "CRDT".equalsIgnoreCase(creditDebitIndicator) | |
| || "CREDIT".equalsIgnoreCase(creditDebitIndicator) | |
| ? "CREDIT" | |
| : "DBIT".equalsIgnoreCase(creditDebitIndicator) | |
| || "DEBIT".equalsIgnoreCase(creditDebitIndicator) | |
| ? "DEBIT" | |
| : null; | |
| if (direction == null) { | |
| log.warn("Skipping transaction with unknown direction '{}'", creditDebitIndicator); | |
| continue; | |
| } |
🤖 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 146 - 151, Update the direction classification in
BankingSyncService to recognize supported credit and debit indicator values
explicitly; do not use DEBIT as the fallback. When credit_debit_indicator is
missing or unsupported, skip or reject the transaction according to the
service’s existing handling conventions, while preserving known CREDIT and DEBIT
behavior.
This was referenced Jul 14, 2026
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
1 participant
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.
Closes #127
Aligns the genai service with the multi-bank dashboard (#126) and grounds it in the aggregated data.
connection→connectionslist (per-bank account name/balance/currency); newtransactions,monthlyFlowandspendByBankmodels;AccountSummarydrops the removed credit fields — fixes/summarizerejecting the new orchestrator payload/summarizeaccepts optionalconnections+monthlyFlow; the summary now mentions the linked-bank count and this month's in/out/netnull, which previously caused a 422 and a canned replyspendByBankis forwarded in the chat context (client sends it too)docker-compose.dev.yml:MODEL_PROVIDER/LOGOS_*pass through from the gitignored.envso the Logos gateway can be tested locallyapp_versionPrometheus metricVerified locally against the dev stack with a Logos key: the dashboard summary shows the linked-bank count and monthly flow, and chat replies are grounded in real dashboard data (with model reasoning shown in the UI).
Merge after #126 — this branch includes a merge of
feature/real-multibank-dashboard, so the extra commits in the diff disappear once #126 lands.