feat: add unit + integration tests across all services - #130
Conversation
- Add tests for account-service (controller, repository, utilization rate) - Add tests for banking-service (controller, repository, sync service, parse amount) - Add tests for orchestrator-service (dashboard controller) - Add tests for transaction-service (controller, repository, expense breakdown) - Add client-side API tests (api.test.ts) - Fix SpotBugs warnings (null safety, specific exceptions, format strings) - Add test dependencies (MockWebServer, SpotBugs, JUnit) to build configs - Add testing-plan.md and test-implementation-progress.md docs
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe pull request expands frontend and backend test coverage across account, banking, transaction, orchestration, and GenAI services. It adds H2 and MockWebServer test infrastructure, updates CI commands, extends client API typing, and adds guarded logging to banking session creation. ChangesApplication testing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (4)
docs/testing-plan.md (1)
176-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the documented orchestrator test strategy with the implementation.
The table describes
DashboardControllerTest.javaas using a mockedWebClient, while the supplied implementation context says it uses MockWebServer-backed downstream URLs. Update the wording so the plan does not prescribe a different mocking approach.Also applies to: 199-206
🤖 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 `@docs/testing-plan.md` around lines 176 - 178, Update the DashboardControllerTest.java testing-plan entry to describe the MockWebServer-backed downstream URL strategy used by the implementation, removing the prescription to mock a WebClient bean. Apply the same wording correction to the referenced related section so both documented test descriptions remain consistent.server/transaction-service/src/test/java/com/team/bank/transaction/ExpenseBreakdownTest.java (1)
27-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTest duplicates production algorithm instead of exercising it.
compute()re-implementsTransactionController#expenseBreakdownby hand rather than calling it. If the production algorithm changes, this test can keep passing against a stale copy while the real behavior silently diverges —TransactionControllerTestalready covers the same edge cases against the real controller via@WebMvcTest, making this duplication mostly redundant. Consider extracting the algorithm into a small, directly-testable (e.g., package-private static) method reused by both the controller and this test, if maintaining focused pure-unit coverage is still desired.🤖 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/transaction-service/src/test/java/com/team/bank/transaction/ExpenseBreakdownTest.java` around lines 27 - 57, Remove the duplicated algorithm from ExpenseBreakdownTest.compute and avoid testing a stale copy of TransactionController#expenseBreakdown. Either delete this redundant focused coverage because TransactionControllerTest already exercises the real controller, or extract the shared calculation into a directly testable package-private method and have both the controller and test call that method.server/banking-service/src/main/java/com/team/bank/banking/client/EnableBankingClient.java (1)
106-122: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBroaden diagnosability of the new catch block; add direct test coverage for it.
catch (Exception e)is very broad andlog.error("...: {}", e.getMessage())discards the stack trace, making failures hard to diagnose in production. Prefer catchingRestClientException(the actual exception typeRestTemplate.exchangethrows) and pass the throwable to the logger so the stack trace is preserved.Also, this exact catch/log/null-return path isn't exercised by any test in this batch —
BankingControllerTestonly mocksebClient.createSession(...), so the real try/catch in this file is untested.🪵 Proposed fix for exception handling
try { ResponseEntity<Map<String, Object>> response = restTemplate.exchange( url, HttpMethod.POST, entity, new ParameterizedTypeReference<>() {}); Map<String, Object> respBody = response.getBody(); log.info( "Enable Banking createSession — HTTP {} — body present: {}", response.getStatusCode().value(), respBody != null); if (respBody == null || respBody.isEmpty()) { log.warn("Enable Banking returned empty session body — code may be expired or invalid"); } return respBody; - } catch (Exception e) { - log.error("Enable Banking createSession failed: {}", e.getMessage()); + } catch (RestClientException e) { + log.error("Enable Banking createSession failed", e); return null; }Want me to add a unit test (e.g., mocking
RestTemplateto throw) that verifiescreateSessionlogs and returnsnullon exchange failure?🤖 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/client/EnableBankingClient.java` around lines 106 - 122, Update createSession in EnableBankingClient to catch RestClientException instead of Exception, and pass the exception itself to log.error so the full stack trace is preserved while retaining the null return behavior. Add focused unit coverage that mocks RestTemplate.exchange to throw RestClientException and verifies createSession returns null through this catch path.server/orchestrator-service/src/test/java/com/team/bank/orchestrator/DashboardControllerTest.java (1)
33-70: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winShared static
MockWebServerqueues across all tests — no per-test reset.The four
MockWebServerinstances persist for the whole class with FIFO response queues that aren't cleared between tests. This works today because every test's enqueue count matches its actual consumption, but it's a latent flakiness trap: JUnit 5's default method order is deterministic yet intentionally not source-order, so if any future test enqueues more/fewer responses than the controller actually consumes, a stray response can leak into a different test (run in an unpredictable order), producing a confusing failure far from the real cause.Consider resetting each server's dispatcher (or asserting via
takeRequest()/getRequestCount()) in a@BeforeEach, so each test's queue starts empty and any drift is caught immediately at the test that caused it rather than one that happens to run afterward.🤖 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/test/java/com/team/bank/orchestrator/DashboardControllerTest.java` around lines 33 - 70, Reset the response queues for accountServer, transactionServer, genaiServer, and bankingServer before each test so shared MockWebServer state cannot leak between test methods. Add a `@BeforeEach` setup method alongside startServers/stopServers that clears each server’s queued responses while preserving the existing per-test enqueue 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 642-646: Update the initializers in client/src/App.tsx at lines
642-646 and 945-947: wrap the localStorage.getItem(DOCK_WIDTH_STORAGE_KEY)
access in try/catch and fall back to null on failure, and wrap
sessionStorage.getItem("authed") similarly with a false fallback. Preserve the
existing parsing and authentication behavior when storage access succeeds.
In `@docs/test-implementation-progress.md`:
- Line 26: Update every fenced code block in
docs/test-implementation-progress.md, including the directory-tree blocks around
the referenced sections, to specify an appropriate language such as text. Ensure
no fenced block remains without a language identifier so markdownlint MD040
passes.
- Line 4: Correct the overall status in the document header and the Phase 2–6
entries so they accurately reflect the remaining unchecked work and
test/compilation instructions. Do not mark all phases complete until
verification and all required tasks are finished; update the completion date and
status only after those conditions are met.
In `@docs/testing-plan.md`:
- Line 39: Update the fenced diagram block in the testing plan by adding an
explicit language identifier, such as text, to its opening fence so it satisfies
markdownlint MD040.
- Around line 340-348: Synchronize the TrendChart test inventory across all
specified sites: in docs/testing-plan.md lines 340-348, keep the five
TrendChart.test.tsx tests aligned with the implementation; in
docs/test-implementation-progress.md lines 50-56, add TrendChart.test.tsx to the
created-file inventory when implemented; and in lines 201-207, update the Phase
6 entry with the verified file and test count.
- Around line 356-366: Update the CI documentation to match the actual
.github/workflows/ci.yml commands: remove npm run build from the documented
current-state frontend command, unless the workflow is explicitly updated to add
that build step. Ensure the statement about all tests executing in CI remains
accurate.
In
`@server/account-service/src/test/java/com/team/bank/account/AccountControllerTest.java`:
- Around line 62-69: Update the JSONPath numeric assertions in
AccountControllerTest.java at lines 62-69, 96-102, and 112-118, and
TransactionControllerTest.java at lines 51-66, replacing integer values such as
1200, 4000, and 500 with numeric-compatible assertions using 1200.0, 4000.0, and
500.0 or comparesEqualTo(BigDecimal). Leave non-numeric assertions unchanged.
In
`@server/banking-service/src/test/java/com/team/bank/banking/controller/BankingControllerTest.java`:
- Around line 297-311: Strengthen shouldFallbackToMostRecentWhenNoActive by
giving older and newer distinguishable response fields, such as different
bankName or country values, while keeping both statuses PENDING and their
timestamps ordered. Assert the selected field matches newer so the test verifies
the controller chooses the most recent connection rather than merely returning
any PENDING connection.
In
`@server/banking-service/src/test/java/com/team/bank/banking/service/BankingSyncServiceTest.java`:
- Around line 72-91: Strengthen shouldSkipSyncWhenExternalUidIsNull and
shouldSkipSyncWhenExternalUidIsBlank to verify bankingConnectionRepository is
never accessed or modified during the guard path. Also add a never()
verification for ebClient.getTransactions(anyString()) to the blank-UID test,
matching the null-UID test and preserving the early-return invariant.
In
`@server/banking-service/src/test/java/com/team/bank/banking/service/ParseAmountTest.java`:
- Around line 108-114: Rename the test method and its `@DisplayName` to describe
successful parsing of an Integer via toString(), matching the existing assertion
and inline comment; do not change the test logic.
In `@server/banking-service/src/test/resources/application-test.yml`:
- Around line 12-14: Ensure the enablebanking.private-key-path configuration
references an existing test resource by adding test-key.pem under the test
resources directory or updating the path to the correct existing key file. Keep
the app-id and configuration structure unchanged.
In
`@server/transaction-service/src/test/java/com/team/bank/transaction/TransactionRepositoryTest.java`:
- Around line 18-21: Update the DataJpaTest import used by
TransactionRepositoryTest and the other repository test classes to
org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest, removing the
incorrect org.springframework.boot.data.jpa.test.autoconfigure import so the
tests compile.
---
Nitpick comments:
In `@docs/testing-plan.md`:
- Around line 176-178: Update the DashboardControllerTest.java testing-plan
entry to describe the MockWebServer-backed downstream URL strategy used by the
implementation, removing the prescription to mock a WebClient bean. Apply the
same wording correction to the referenced related section so both documented
test descriptions remain consistent.
In
`@server/banking-service/src/main/java/com/team/bank/banking/client/EnableBankingClient.java`:
- Around line 106-122: Update createSession in EnableBankingClient to catch
RestClientException instead of Exception, and pass the exception itself to
log.error so the full stack trace is preserved while retaining the null return
behavior. Add focused unit coverage that mocks RestTemplate.exchange to throw
RestClientException and verifies createSession returns null through this catch
path.
In
`@server/orchestrator-service/src/test/java/com/team/bank/orchestrator/DashboardControllerTest.java`:
- Around line 33-70: Reset the response queues for accountServer,
transactionServer, genaiServer, and bankingServer before each test so shared
MockWebServer state cannot leak between test methods. Add a `@BeforeEach` setup
method alongside startServers/stopServers that clears each server’s queued
responses while preserving the existing per-test enqueue behavior.
In
`@server/transaction-service/src/test/java/com/team/bank/transaction/ExpenseBreakdownTest.java`:
- Around line 27-57: Remove the duplicated algorithm from
ExpenseBreakdownTest.compute and avoid testing a stale copy of
TransactionController#expenseBreakdown. Either delete this redundant focused
coverage because TransactionControllerTest already exercises the real
controller, or extract the shared calculation into a directly testable
package-private method and have both the controller and test call that method.
🪄 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: d50d409e-0265-46b4-8861-6f38345cd699
📒 Files selected for processing (26)
client/src/App.test.tsxclient/src/App.tsxclient/src/api.test.tsdocs/test-implementation-progress.mddocs/testing-plan.mdserver/account-service/build.gradle.ktsserver/account-service/src/test/java/com/team/bank/account/AccountControllerTest.javaserver/account-service/src/test/java/com/team/bank/account/AccountRepositoryTest.javaserver/account-service/src/test/java/com/team/bank/account/UtilizationRateTest.javaserver/account-service/src/test/resources/application-test.ymlserver/banking-service/build.gradle.ktsserver/banking-service/src/main/java/com/team/bank/banking/client/EnableBankingClient.javaserver/banking-service/src/test/java/com/team/bank/banking/controller/BankingControllerTest.javaserver/banking-service/src/test/java/com/team/bank/banking/model/BankingConnectionRepositoryTest.javaserver/banking-service/src/test/java/com/team/bank/banking/service/BankingSyncServiceTest.javaserver/banking-service/src/test/java/com/team/bank/banking/service/ParseAmountTest.javaserver/banking-service/src/test/resources/application-test.ymlserver/gradle/libs.versions.tomlserver/orchestrator-service/build.gradle.ktsserver/orchestrator-service/src/test/java/com/team/bank/orchestrator/DashboardControllerTest.javaserver/orchestrator-service/src/test/resources/application-test.ymlserver/transaction-service/build.gradle.ktsserver/transaction-service/src/test/java/com/team/bank/transaction/ExpenseBreakdownTest.javaserver/transaction-service/src/test/java/com/team/bank/transaction/TransactionControllerTest.javaserver/transaction-service/src/test/java/com/team/bank/transaction/TransactionRepositoryTest.javaserver/transaction-service/src/test/resources/application-test.yml
|
|
||
| ## Test Pyramid & Strategy | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify a language for the fenced diagram block.
Add a fence language such as text to satisfy markdownlint MD040.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/testing-plan.md` at line 39, Update the fenced diagram block in the
testing plan by adding an explicit language identifier, such as text, to its
opening fence so it satisfies markdownlint MD040.
Source: Linters/SAST tools
…ype, add missing repo method and mock
0ab8f58 to
b8566c6
Compare
…4g heap, runner JVM env vars
…--stacktrace for debugging
…anging service" This reverts commit 24769fd.
… tests only enqueued 5; restore --parallel
…lugin-react-hooks v5
|
I needed to push a small fix, since Selim's machine is quite restricted now! |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/banking-service/src/test/java/com/team/bank/banking/service/BankingSyncServiceTest.java (1)
412-414: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the timestamp assertion to ensure it was actually updated.
Since
activeConnection.setUpdatedAtis initialized in@BeforeEachto exactly one hour in the past (minusHours(1)), the updated timestamp should always be strictly afterbefore.Allowing
.equals(before)means this assertion would still evaluate totrueeven if the service completely failed to update the timestamp, defeating the purpose of the test.💚 Proposed fix
- assertTrue( - captor.getValue().getUpdatedAt().isAfter(before) - || captor.getValue().getUpdatedAt().equals(before)); + assertTrue(captor.getValue().getUpdatedAt().isAfter(before));🤖 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 412 - 414, Update the timestamp assertion in BankingSyncServiceTest to require captor.getValue().getUpdatedAt() to be strictly after before, removing the equals(before) alternative while preserving the existing captured-value check.
🧹 Nitpick comments (1)
genai/tests/test_main.py (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace ambiguous EN DASH with HYPHEN-MINUS in comments.
The section header comments use an EN DASH (
–), which is flagged by static analysis tools (RUF003) because it can be visually ambiguous and cause issues in some text-processing contexts. Consider replacing it with a standard HYPHEN-MINUS (-).
genai/tests/test_main.py#L27-L27: replaceChat – backwards-compatiblewithChat - backwards-compatiblegenai/tests/test_main.py#L55-L55: replaceChat – "messages" arraywithChat - "messages" arraygenai/tests/test_main.py#L98-L98: replaceChat – with dashboard contextwithChat - with dashboard context♻️ Proposed replacements
-# Chat – backwards-compatible "message" field +# Chat - backwards-compatible "message" field-# Chat – "messages" array (new shape) +# Chat - "messages" array (new shape)-# Chat – with dashboard context +# Chat - with dashboard context🤖 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 `@genai/tests/test_main.py` at line 27, Replace the EN DASH with a standard hyphen-minus in the section header comments at genai/tests/test_main.py lines 27, 55, and 98, preserving the rest of each comment text.Source: Linters/SAST tools
🤖 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 @.github/workflows/ci.yml:
- Around line 61-62: Remove the duplicate consecutive npm run build command from
the CI workflow, leaving exactly one build invocation in that step.
In `@client/src/App.test.tsx`:
- Around line 227-232: Update the fetchDashboard spy setup in the affected tests
to use mockResolvedValueOnce for each sequential response, preserving the
intended return order. Review newly added tests in App.test.tsx, including the
chat message test, and replace any consecutive mockResolvedValue calls on the
same spy with sequential mocks.
---
Outside diff comments:
In
`@server/banking-service/src/test/java/com/team/bank/banking/service/BankingSyncServiceTest.java`:
- Around line 412-414: Update the timestamp assertion in BankingSyncServiceTest
to require captor.getValue().getUpdatedAt() to be strictly after before,
removing the equals(before) alternative while preserving the existing
captured-value check.
---
Nitpick comments:
In `@genai/tests/test_main.py`:
- Line 27: Replace the EN DASH with a standard hyphen-minus in the section
header comments at genai/tests/test_main.py lines 27, 55, and 98, preserving the
rest of each comment text.
🪄 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: 6083901f-8096-44c4-a801-171f871ed635
⛔ Files ignored due to path filters (1)
server/banking-service/src/test/resources/test-key.pemis excluded by!**/*.pem
📒 Files selected for processing (14)
.github/workflows/ci.ymlclient/src/App.test.tsxclient/src/App.tsxclient/src/api.test.tsclient/src/api.tsgenai/tests/test_main.pyserver/banking-service/src/main/java/com/team/bank/banking/model/TransactionRepository.javaserver/banking-service/src/test/java/com/team/bank/banking/controller/BankingControllerTest.javaserver/banking-service/src/test/java/com/team/bank/banking/model/BankingConnectionRepositoryTest.javaserver/banking-service/src/test/java/com/team/bank/banking/service/BankingSyncServiceTest.javaserver/banking-service/src/test/java/com/team/bank/banking/service/ParseAmountTest.javaserver/build.gradle.ktsserver/gradle.propertiesserver/orchestrator-service/src/test/java/com/team/bank/orchestrator/DashboardControllerTest.java
💤 Files with no reviewable changes (1)
- server/build.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (6)
- server/banking-service/src/test/java/com/team/bank/banking/service/ParseAmountTest.java
- server/banking-service/src/test/java/com/team/bank/banking/model/BankingConnectionRepositoryTest.java
- client/src/api.test.ts
- server/banking-service/src/test/java/com/team/bank/banking/controller/BankingControllerTest.java
- server/orchestrator-service/src/test/java/com/team/bank/orchestrator/DashboardControllerTest.java
- client/src/App.tsx
| vi.spyOn(api, "fetchDashboard").mockResolvedValue( | ||
| dashboard({ connectionStatus: active }), | ||
| ); | ||
| vi.spyOn(api, "fetchDashboard").mockResolvedValue( | ||
| dashboard({ connectionStatus: active, connections: activeConnections }), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use .mockResolvedValueOnce for sequential returns.
Calling .mockResolvedValue() twice consecutively on the same spy immediately overwrites the first configuration before the test begins. To simulate sequential calls to fetchDashboard that return different values, use .mockResolvedValueOnce() for the first mock.
(Note: Please review other newly added tests in this file, such as the chat message test, that may have duplicated this pattern).
💚 Proposed fix to ensure both values are yielded
- vi.spyOn(api, "fetchDashboard").mockResolvedValue(
+ vi.spyOn(api, "fetchDashboard").mockResolvedValueOnce(
dashboard({ connectionStatus: active }),
);
vi.spyOn(api, "fetchDashboard").mockResolvedValue(
dashboard({ connectionStatus: active, connections: activeConnections }),
);📝 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.
| vi.spyOn(api, "fetchDashboard").mockResolvedValue( | |
| dashboard({ connectionStatus: active }), | |
| ); | |
| vi.spyOn(api, "fetchDashboard").mockResolvedValue( | |
| dashboard({ connectionStatus: active, connections: activeConnections }), | |
| ); | |
| vi.spyOn(api, "fetchDashboard").mockResolvedValueOnce( | |
| dashboard({ connectionStatus: active }), | |
| ); | |
| vi.spyOn(api, "fetchDashboard").mockResolvedValue( | |
| dashboard({ connectionStatus: active, connections: activeConnections }), | |
| ); |
🤖 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.test.tsx` around lines 227 - 232, Update the fetchDashboard
spy setup in the affected tests to use mockResolvedValueOnce for each sequential
response, preserving the intended return order. Review newly added tests in
App.test.tsx, including the chat message test, and replace any consecutive
mockResolvedValue calls on the same spy with sequential mocks.
azzabaatout
left a comment
There was a problem hiding this comment.
You can go through my comments @wardstoneX! let me know if anything is unclear
| npm run lint | ||
| npm run test | ||
| npm run build | ||
| npm run build |
There was a problem hiding this comment.
this needs to be deleted
There was a problem hiding this comment.
is this supposed to be here @wardstoneX ? i don t think this is supposed to be pushed in the repo
|
azza reviewed it, so i am merging. |
Summary by CodeRabbit