Open
Conversation
📝 WalkthroughWalkthroughAdds .gitignore entry for build error logs, removes multiple backend build error artifacts and CI/CD workflow files, introduces ESLint configs and lint tooling, updates backend package.json scripts/dependencies, refactors and updates several backend services/tests and small parser/analytics changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/package.json`:
- Around line 94-96: Remove the redundant individual TypeScript-ESLint packages
from devDependencies: delete the "@typescript-eslint/eslint-plugin" and
"@typescript-eslint/parser" entries in package.json (they are the lines
currently listing those exact package names and versions) because the project
uses the "typescript-eslint" meta package in eslint.config.mjs; keep only the
meta package to avoid duplication and maintain a single dependency strategy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3126a9b-f7f7-467a-b7cd-a5949cff0e8b
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
backend/eslint.config.mjsbackend/package.jsonfrontend/eslint.config.js
Comment on lines
+94
to
96
| "@typescript-eslint/eslint-plugin": "^8.56.1", | ||
| "@typescript-eslint/parser": "^8.56.1", | ||
| "dotenv": "^17.2.3", |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cd backend && head -120 package.json | tail -40 && echo "---" && wc -l package.jsonRepository: OlufunbiIK/StellarSplit
Length of output: 1186
🏁 Script executed:
cat -n backend/package.json | sed -n '90,110p'Repository: OlufunbiIK/StellarSplit
Length of output: 718
🏁 Script executed:
find backend -maxdepth 2 -type f \( -name ".eslintrc*" -o -name "eslint.config.*" \) | head -20Repository: OlufunbiIK/StellarSplit
Length of output: 92
🏁 Script executed:
rg -n "@typescript-eslint/(eslint-plugin|parser)" backend --type js --type ts --type json | grep -v "package.json" | head -30Repository: OlufunbiIK/StellarSplit
Length of output: 49
🏁 Script executed:
rg -n "from.*typescript-eslint|import.*typescript-eslint|require.*typescript-eslint" backend --type js --type ts | head -30Repository: OlufunbiIK/StellarSplit
Length of output: 49
🏁 Script executed:
cat -n backend/eslint.config.mjsRepository: OlufunbiIK/StellarSplit
Length of output: 1602
Remove redundant @typescript-eslint individual packages from devDependencies.
The ESLint configuration (backend/eslint.config.mjs) imports and uses the typescript-eslint meta package exclusively (line 3-4, 25). Having both the meta package and individual packages (@typescript-eslint/eslint-plugin and @typescript-eslint/parser) in package.json is redundant—the meta package includes these automatically. Remove lines 94-95 to keep a single, consistent TypeScript-ESLint dependency strategy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/package.json` around lines 94 - 96, Remove the redundant individual
TypeScript-ESLint packages from devDependencies: delete the
"@typescript-eslint/eslint-plugin" and "@typescript-eslint/parser" entries in
package.json (they are the lines currently listing those exact package names and
versions) because the project uses the "typescript-eslint" meta package in
eslint.config.mjs; keep only the meta package to avoid duplication and maintain
a single dependency strategy.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
backend/src/friendship/friendship.controller.spec.ts (1)
11-18: Define explicit FriendshipService mock methods instead of an empty object.The empty
useValueworks for the current smoke test, but it makes future controller-method tests fragile and less clear. Predefine the service method keys used byFriendshipControllerto keep the test contract explicit.Proposed refactor
providers: [ { provide: FriendshipService, useValue: { - /* mock methods */ + sendRequest: jest.fn(), + acceptRequest: jest.fn(), + block: jest.fn(), + getFriends: jest.fn(), }, }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/friendship/friendship.controller.spec.ts` around lines 11 - 18, The provider's useValue currently is an empty object which hides the FriendshipService contract; replace it with an explicit mock object implementing the methods FriendshipController calls (e.g., the service methods such as list/findAll, createFriendship/create, acceptFriendRequest, removeFriend/remove, and any getMutualFriends/getById used by the controller) as jest.fn() stubs that return realistic fixtures or Promises; update the provider for FriendshipService to supply that mock object so controller tests can call and spy on those methods reliably.backend/src/modules/archiving/archiving.service.ts (1)
330-330: Avoid hardcoded app URL in reminder payloads.Line 330 hardcodes
https://stellarsplit.com. Prefer a config-driven base URL so staging/dev/custom domains don’t generate broken payment links.🔧 Suggested fix
- paymentLink: `https://stellarsplit.com/splits/${split.id}`, // Example link + paymentLink: `${this.appBaseUrl}/splits/${split.id}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/modules/archiving/archiving.service.ts` at line 330, The paymentLink currently hardcodes the app URL; change it to use a configurable base URL from environment/config (e.g., an AppConfig or ConfigService value like appBaseUrl / APP_BASE_URL) instead of "https://stellarsplit.com" when building the reminder payload in archiving.service.ts (the line that sets paymentLink: `.../splits/${split.id}`). Locate the construction of paymentLink in the method that references split.id and replace the literal with `${config.appBaseUrl}/splits/${split.id}` (or call a helper like getAppUrl()) and ensure the config value is validated/fallbacked so staging/dev domains generate correct links.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/modules/archiving/archiving.service.ts`:
- Around line 305-353: The current single try/catch around push and email (the
block calling this.pushService.sendNotification and
this.emailService.sendPaymentReminder) couples channels so one failure can
prevent the other or prevent logging; split them into independent try/catch
blocks: wrap the push call and its reminderLogRepo.create/save for
ReminderChannel.PUSH in its own try/catch and wrap the user lookup,
emailService.sendPaymentReminder call, and its reminderLogRepo.create/save for
ReminderChannel.EMAIL in a separate try/catch, and ensure each catch logs the
error and writes a DeliveryStatus.FAILED log via reminderLogRepo.save so
failures for one channel don’t suppress the other (reference
this.pushService.sendNotification, this.userRepo.findOne,
this.emailService.sendPaymentReminder, and reminderLogRepo.create/save).
In `@backend/src/ocr/parsers/receipt-parser.ts`:
- Around line 146-150: The subtotal lines are being misclassified because
matchesPattern(upperLine, ["TOTAL", "AMOUNT DUE", "AMOUNT"]) checks "TOTAL"
before "SUBTOTAL" and uses a broad "AMOUNT" token; update the matching order and
tokens so subtotal is tested first and "AMOUNT" is narrowed (e.g., use "AMOUNT
DUE" but remove plain "AMOUNT" or make it exact), i.e., call
matchesPattern(upperLine, ["SUBTOTAL","SUB-TOTAL","SUB TOTAL"]) before checking
totals and change the totals check to avoid the plain "AMOUNT" token; apply the
same reorder/token fix to the other similar matchesPattern call that checks
totals elsewhere in the file.
- Around line 111-113: The parser currently overwrites ReceiptItem.price with a
unit price (dividing by quantity), breaking downstream logic that expects price
to be the line total; revert this by keeping price as the original line total
when calling items.push (i.e., use price unchanged for ReceiptItem.price) and,
if you need the per-unit value, add a new field (e.g., unitPrice or unit_price)
to the object instead; update the items.push call that constructs { name:
itemName, quantity, price: ... } so price remains the line total and unit price
is provided separately.
In `@backend/src/realtime-analytics/analytics.aggregate.ts`:
- Around line 19-22: The code uses clickhouse.query and immediately returns
rows[0] as PlatformMetrics without guarding for empty results; update the logic
around the rows variable (the result of clickhouse.query) to check for an empty
array (e.g., rows.length === 0) and handle that case instead of returning
undefined — either throw a descriptive error like "No platform metrics found" or
return a well-formed fallback PlatformMetrics object so the function's declared
return contract is preserved; modify the block containing clickhouse.query and
the return of rows[0] as PlatformMetrics accordingly.
In `@backend/src/realtime-analytics/analytics.test.ts`:
- Around line 16-25: Stop mocking the entire "./analytics.aggregate" module so
the real aggregateMetrics and getAggregatedMetrics implementations run; instead
replace the mock with a Jest mock for the external ClickHouse client (module
"@apla/clickhouse") so database calls are stubbed but aggregation logic is
exercised. Concretely, remove or change the jest.mock("./analytics.aggregate",
...) block and add a jest.mock("@apla/clickhouse", ...) that returns a fake
client exposing the same query/insert methods used by
aggregateMetrics/getAggregatedMetrics; keep the test assertions the same so
failures in SQL/aggregation surface while tests remain isolated from a real DB.
In `@backend/src/receipts/receipts.controller.spec.ts`:
- Around line 1-5: Replace the meaningless aws-sdk check with a real unit test
for the receipts controller: import ReceiptsController and its dependent
ReceiptsService, build a Nest Test.createTestingModule providing a mock provider
for ReceiptsService, retrieve the ReceiptsController from the module, assert the
controller is defined, and add at least one behavioral assertion (e.g., call a
public method like findAll/getReceipts/createReceipt with a mocked return value
and assert the result). Remove the import of Service from "aws-sdk" and the
trivial test that asserts Service is defined.
In `@backend/src/tests/payment-integration.test.ts`:
- Around line 67-113: The tests currently only assert service is defined and
don't exercise payment processing logic; replace the final
expect(service).toBeDefined() in each test with calls to the payment handler on
service (e.g., service.processPayment or the actual method that handles verified
transactions) and assert real outcomes: after mocking
stellarService.verifyTransaction, assert repository/save methods were called
with expected payment records, assert domain state changes (paid vs partial) and
that the event/gateway emitter was invoked, and for duplicates assert the
repository lookup prevented a second save and an error/ignore behavior; add
spies/mocks for the repository (save/findByTxId) and event emitter to verify
these side effects. Ensure each test uses the specific mocked response from
stellarService.verifyTransaction and verifies the corresponding handling path
(complete, partial, invalid, duplicate).
- Around line 52-58: The test's EventsGateway mock doesn't match the real
contract (real gateway uses server.to(...).emit(...) and public methods like
emitPaymentReceived), so update the mock for EventsGateway to implement a server
object whose to() returns an object with emit as jest.fn(), and also include the
gateway's public methods (e.g., emitPaymentReceived) as jest.fn() or delegate to
server.to(...).emit(...); ensure any existing references to emit use the new
methods so the mock shape mirrors the real EventsGateway API.
---
Nitpick comments:
In `@backend/src/friendship/friendship.controller.spec.ts`:
- Around line 11-18: The provider's useValue currently is an empty object which
hides the FriendshipService contract; replace it with an explicit mock object
implementing the methods FriendshipController calls (e.g., the service methods
such as list/findAll, createFriendship/create, acceptFriendRequest,
removeFriend/remove, and any getMutualFriends/getById used by the controller) as
jest.fn() stubs that return realistic fixtures or Promises; update the provider
for FriendshipService to supply that mock object so controller tests can call
and spy on those methods reliably.
In `@backend/src/modules/archiving/archiving.service.ts`:
- Line 330: The paymentLink currently hardcodes the app URL; change it to use a
configurable base URL from environment/config (e.g., an AppConfig or
ConfigService value like appBaseUrl / APP_BASE_URL) instead of
"https://stellarsplit.com" when building the reminder payload in
archiving.service.ts (the line that sets paymentLink: `.../splits/${split.id}`).
Locate the construction of paymentLink in the method that references split.id
and replace the literal with `${config.appBaseUrl}/splits/${split.id}` (or call
a helper like getAppUrl()) and ensure the config value is validated/fallbacked
so staging/dev domains generate correct links.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8032f93f-90af-4ca7-bfdf-1ccb7fa6a984
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
backend/package.jsonbackend/src/disputes/disputes.service.spec.tsbackend/src/friendship/friendship.controller.spec.tsbackend/src/governance/governance.service.spec.tsbackend/src/modules/archiving/archiving.service.tsbackend/src/ocr/parsers/receipt-parser.tsbackend/src/realtime-analytics/analytics.aggregate.tsbackend/src/realtime-analytics/analytics.test.tsbackend/src/receipts/receipts.controller.spec.tsbackend/src/recurring-splits/recurring-splits.service.spec.tsbackend/src/reputation/tests/reputation.service.spec.tsbackend/src/reputation/tests/score-calculator.service.spec.tsbackend/src/search/search.controller.spec.tsbackend/src/search/search.service.spec.tsbackend/src/tests/payment-integration.test.tsbackend/src/tests/receipts/receipts.service.spec.ts
💤 Files with no reviewable changes (6)
- backend/src/governance/governance.service.spec.ts
- backend/src/reputation/tests/score-calculator.service.spec.ts
- backend/src/search/search.controller.spec.ts
- backend/src/tests/receipts/receipts.service.spec.ts
- backend/src/reputation/tests/reputation.service.spec.ts
- backend/src/search/search.service.spec.ts
✅ Files skipped from review due to trivial changes (1)
- backend/src/disputes/disputes.service.spec.ts
Comment on lines
+305
to
+353
| try { | ||
| // Send Push | ||
| await this.pushService.sendNotification( | ||
| participant.userId, | ||
| NotificationEventType.PAYMENT_REMINDER, | ||
| titleMap[type], | ||
| bodyMap[type], | ||
| ); | ||
|
|
||
| // Log Push | ||
| const logPush = this.reminderLogRepo.create({ | ||
| splitId: split.id, | ||
| participantId: participant.id, | ||
| reminderType: type, | ||
| channel: ReminderChannel.PUSH, | ||
| deliveryStatus: DeliveryStatus.SENT, | ||
| }); | ||
| await this.reminderLogRepo.save(logPush); | ||
|
|
||
| } catch (error) { | ||
| this.logger.error(`Failed to send reminder to ${participant.userId}`, error); | ||
| // Send Email | ||
| // Need to fetch user to get email | ||
| // Participant userId is uuid of User entity? Or wallet address? | ||
| // Looking at Participant entity: userId: string. | ||
| // Looking at User entity: id: uuid. | ||
| // Assuming participant.userId matches User.id (UUID). | ||
|
|
||
| const user = await this.userRepo.findOne({ | ||
| where: { id: participant.userId }, | ||
| }); | ||
|
|
||
| if (user && user.email && user.emailPreferences.reminders) { | ||
| await this.emailService.sendPaymentReminder(user.email, { | ||
| participantName: "User", // We don't have name | ||
| splitDescription: split.description || "Split", | ||
| amountDue: Number(participant.amountOwed), | ||
| paymentLink: `https://stellarsplit.com/splits/${split.id}`, // Example link | ||
| }); | ||
|
|
||
| // Log Email | ||
| const logEmail = this.reminderLogRepo.create({ | ||
| splitId: split.id, | ||
| participantId: participant.id, | ||
| reminderType: type, | ||
| channel: ReminderChannel.EMAIL, | ||
| deliveryStatus: DeliveryStatus.SENT, | ||
| }); | ||
| await this.reminderLogRepo.save(logEmail); | ||
| } | ||
|
|
||
| // Log Push | ||
| const logPush = this.reminderLogRepo.create({ | ||
| splitId: split.id, | ||
| participantId: participant.id, | ||
| reminderType: type, | ||
| channel: ReminderChannel.PUSH, | ||
| deliveryStatus: DeliveryStatus.SENT, | ||
| }); | ||
| await this.reminderLogRepo.save(logPush); | ||
| } catch (error) { |
There was a problem hiding this comment.
Single try/catch couples channels and can drop reminders.
From Line 305, push and email are in one try. If push fails, email is never attempted. If email fails after push succeeds, push success may not be logged. This creates silent delivery gaps.
🔧 Suggested fix
- try {
- // Send Push
- await this.pushService.sendNotification(
- participant.userId,
- NotificationEventType.PAYMENT_REMINDER,
- titleMap[type],
- bodyMap[type],
- );
+ // Push channel
+ try {
+ await this.pushService.sendNotification(
+ participant.userId,
+ NotificationEventType.PAYMENT_REMINDER,
+ titleMap[type],
+ bodyMap[type],
+ );
+ await this.reminderLogRepo.save(
+ this.reminderLogRepo.create({
+ splitId: split.id,
+ participantId: participant.id,
+ reminderType: type,
+ channel: ReminderChannel.PUSH,
+ deliveryStatus: DeliveryStatus.SENT,
+ }),
+ );
+ } catch (error) {
+ this.logger.error(`Push reminder failed for ${participant.userId}`, error);
+ await this.reminderLogRepo.save(
+ this.reminderLogRepo.create({
+ splitId: split.id,
+ participantId: participant.id,
+ reminderType: type,
+ channel: ReminderChannel.PUSH,
+ deliveryStatus: DeliveryStatus.FAILED,
+ }),
+ );
+ }
- // Send Email
+ // Email channel
+ try {
const user = await this.userRepo.findOne({
where: { id: participant.userId },
});
if (user && user.email && user.emailPreferences.reminders) {
await this.emailService.sendPaymentReminder(user.email, {
participantName: "User",
splitDescription: split.description || "Split",
amountDue: Number(participant.amountOwed),
paymentLink: `https://stellarsplit.com/splits/${split.id}`,
});
-
- // Log Email
- const logEmail = this.reminderLogRepo.create({
- splitId: split.id,
- participantId: participant.id,
- reminderType: type,
- channel: ReminderChannel.EMAIL,
- deliveryStatus: DeliveryStatus.SENT,
- });
- await this.reminderLogRepo.save(logEmail);
+ await this.reminderLogRepo.save(
+ this.reminderLogRepo.create({
+ splitId: split.id,
+ participantId: participant.id,
+ reminderType: type,
+ channel: ReminderChannel.EMAIL,
+ deliveryStatus: DeliveryStatus.SENT,
+ }),
+ );
}
-
- // Log Push
- const logPush = this.reminderLogRepo.create({
- splitId: split.id,
- participantId: participant.id,
- reminderType: type,
- channel: ReminderChannel.PUSH,
- deliveryStatus: DeliveryStatus.SENT,
- });
- await this.reminderLogRepo.save(logPush);
} catch (error) {
- this.logger.error(
- `Failed to send reminder to ${participant.userId}`,
- error,
- );
+ this.logger.error(`Email reminder failed for ${participant.userId}`, error);
+ await this.reminderLogRepo.save(
+ this.reminderLogRepo.create({
+ splitId: split.id,
+ participantId: participant.id,
+ reminderType: type,
+ channel: ReminderChannel.EMAIL,
+ deliveryStatus: DeliveryStatus.FAILED,
+ }),
+ );
}📝 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
| try { | |
| // Send Push | |
| await this.pushService.sendNotification( | |
| participant.userId, | |
| NotificationEventType.PAYMENT_REMINDER, | |
| titleMap[type], | |
| bodyMap[type], | |
| ); | |
| // Log Push | |
| const logPush = this.reminderLogRepo.create({ | |
| splitId: split.id, | |
| participantId: participant.id, | |
| reminderType: type, | |
| channel: ReminderChannel.PUSH, | |
| deliveryStatus: DeliveryStatus.SENT, | |
| }); | |
| await this.reminderLogRepo.save(logPush); | |
| } catch (error) { | |
| this.logger.error(`Failed to send reminder to ${participant.userId}`, error); | |
| // Send Email | |
| // Need to fetch user to get email | |
| // Participant userId is uuid of User entity? Or wallet address? | |
| // Looking at Participant entity: userId: string. | |
| // Looking at User entity: id: uuid. | |
| // Assuming participant.userId matches User.id (UUID). | |
| const user = await this.userRepo.findOne({ | |
| where: { id: participant.userId }, | |
| }); | |
| if (user && user.email && user.emailPreferences.reminders) { | |
| await this.emailService.sendPaymentReminder(user.email, { | |
| participantName: "User", // We don't have name | |
| splitDescription: split.description || "Split", | |
| amountDue: Number(participant.amountOwed), | |
| paymentLink: `https://stellarsplit.com/splits/${split.id}`, // Example link | |
| }); | |
| // Log Email | |
| const logEmail = this.reminderLogRepo.create({ | |
| splitId: split.id, | |
| participantId: participant.id, | |
| reminderType: type, | |
| channel: ReminderChannel.EMAIL, | |
| deliveryStatus: DeliveryStatus.SENT, | |
| }); | |
| await this.reminderLogRepo.save(logEmail); | |
| } | |
| // Log Push | |
| const logPush = this.reminderLogRepo.create({ | |
| splitId: split.id, | |
| participantId: participant.id, | |
| reminderType: type, | |
| channel: ReminderChannel.PUSH, | |
| deliveryStatus: DeliveryStatus.SENT, | |
| }); | |
| await this.reminderLogRepo.save(logPush); | |
| } catch (error) { | |
| // Push channel | |
| try { | |
| await this.pushService.sendNotification( | |
| participant.userId, | |
| NotificationEventType.PAYMENT_REMINDER, | |
| titleMap[type], | |
| bodyMap[type], | |
| ); | |
| await this.reminderLogRepo.save( | |
| this.reminderLogRepo.create({ | |
| splitId: split.id, | |
| participantId: participant.id, | |
| reminderType: type, | |
| channel: ReminderChannel.PUSH, | |
| deliveryStatus: DeliveryStatus.SENT, | |
| }), | |
| ); | |
| } catch (error) { | |
| this.logger.error(`Push reminder failed for ${participant.userId}`, error); | |
| await this.reminderLogRepo.save( | |
| this.reminderLogRepo.create({ | |
| splitId: split.id, | |
| participantId: participant.id, | |
| reminderType: type, | |
| channel: ReminderChannel.PUSH, | |
| deliveryStatus: DeliveryStatus.FAILED, | |
| }), | |
| ); | |
| } | |
| // Email channel | |
| try { | |
| const user = await this.userRepo.findOne({ | |
| where: { id: participant.userId }, | |
| }); | |
| if (user && user.email && user.emailPreferences.reminders) { | |
| await this.emailService.sendPaymentReminder(user.email, { | |
| participantName: "User", | |
| splitDescription: split.description || "Split", | |
| amountDue: Number(participant.amountOwed), | |
| paymentLink: `https://stellarsplit.com/splits/${split.id}`, | |
| }); | |
| await this.reminderLogRepo.save( | |
| this.reminderLogRepo.create({ | |
| splitId: split.id, | |
| participantId: participant.id, | |
| reminderType: type, | |
| channel: ReminderChannel.EMAIL, | |
| deliveryStatus: DeliveryStatus.SENT, | |
| }), | |
| ); | |
| } | |
| } catch (error) { | |
| this.logger.error(`Email reminder failed for ${participant.userId}`, error); | |
| await this.reminderLogRepo.save( | |
| this.reminderLogRepo.create({ | |
| splitId: split.id, | |
| participantId: participant.id, | |
| reminderType: type, | |
| channel: ReminderChannel.EMAIL, | |
| deliveryStatus: DeliveryStatus.FAILED, | |
| }), | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/modules/archiving/archiving.service.ts` around lines 305 - 353,
The current single try/catch around push and email (the block calling
this.pushService.sendNotification and this.emailService.sendPaymentReminder)
couples channels so one failure can prevent the other or prevent logging; split
them into independent try/catch blocks: wrap the push call and its
reminderLogRepo.create/save for ReminderChannel.PUSH in its own try/catch and
wrap the user lookup, emailService.sendPaymentReminder call, and its
reminderLogRepo.create/save for ReminderChannel.EMAIL in a separate try/catch,
and ensure each catch logs the error and writes a DeliveryStatus.FAILED log via
reminderLogRepo.save so failures for one channel don’t suppress the other
(reference this.pushService.sendNotification, this.userRepo.findOne,
this.emailService.sendPaymentReminder, and reminderLogRepo.create/save).
Comment on lines
+111
to
+113
| const unitPrice = | ||
| quantity > 1 ? parseFloat((price / quantity).toFixed(2)) : price; | ||
| items.push({ name: itemName, quantity, price: unitPrice }); |
There was a problem hiding this comment.
ReceiptItem.price was changed to unit price, which breaks downstream monetary logic.
item.price is consumed as a line total in backend/src/modules/splits/validators/split.validator.ts:143-150 and backend/src/modules/splits/split-calculation.service.ts:115-127. On Line [111]-Line [113], dividing by quantity causes undercounted totals and participant shares for multi-quantity items.
💡 Proposed fix (preserve existing `price` contract)
- const unitPrice =
- quantity > 1 ? parseFloat((price / quantity).toFixed(2)) : price;
- items.push({ name: itemName, quantity, price: unitPrice });
+ items.push({ name: itemName, quantity, price });📝 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
| const unitPrice = | |
| quantity > 1 ? parseFloat((price / quantity).toFixed(2)) : price; | |
| items.push({ name: itemName, quantity, price: unitPrice }); | |
| items.push({ name: itemName, quantity, price }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/ocr/parsers/receipt-parser.ts` around lines 111 - 113, The parser
currently overwrites ReceiptItem.price with a unit price (dividing by quantity),
breaking downstream logic that expects price to be the line total; revert this
by keeping price as the original line total when calling items.push (i.e., use
price unchanged for ReceiptItem.price) and, if you need the per-unit value, add
a new field (e.g., unitPrice or unit_price) to the object instead; update the
items.push call that constructs { name: itemName, quantity, price: ... } so
price remains the line total and unit price is provided separately.
Comment on lines
+146
to
+150
| if (this.matchesPattern(upperLine, ["TOTAL", "AMOUNT DUE", "AMOUNT"])) { | ||
| totals.total = price; | ||
| } else if (this.matchesPattern(upperLine, ['SUBTOTAL', 'SUB-TOTAL', 'SUB TOTAL'])) { | ||
| } else if ( | ||
| this.matchesPattern(upperLine, ["SUBTOTAL", "SUB-TOTAL", "SUB TOTAL"]) | ||
| ) { |
There was a problem hiding this comment.
SUBTOTAL can be misclassified as TOTAL with current matching order.
Because matching is substring-based (includes) and TOTAL is checked first, "SUBTOTAL" lines can hit the total branch before subtotal extraction.
💡 Proposed fix (check subtotal first, avoid broad `AMOUNT` token)
- if (this.matchesPattern(upperLine, ["TOTAL", "AMOUNT DUE", "AMOUNT"])) {
- totals.total = price;
- } else if (
- this.matchesPattern(upperLine, ["SUBTOTAL", "SUB-TOTAL", "SUB TOTAL"])
- ) {
+ if (
+ this.matchesPattern(upperLine, ["SUBTOTAL", "SUB-TOTAL", "SUB TOTAL"])
+ ) {
totals.subtotal = price;
+ } else if (this.matchesPattern(upperLine, ["TOTAL", "AMOUNT DUE"])) {
+ totals.total = price;
} else if (
this.matchesPattern(upperLine, ["TAX", "SALES TAX", "GST", "HST"])
) {Also applies to: 226-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/ocr/parsers/receipt-parser.ts` around lines 146 - 150, The
subtotal lines are being misclassified because matchesPattern(upperLine,
["TOTAL", "AMOUNT DUE", "AMOUNT"]) checks "TOTAL" before "SUBTOTAL" and uses a
broad "AMOUNT" token; update the matching order and tokens so subtotal is tested
first and "AMOUNT" is narrowed (e.g., use "AMOUNT DUE" but remove plain "AMOUNT"
or make it exact), i.e., call matchesPattern(upperLine,
["SUBTOTAL","SUB-TOTAL","SUB TOTAL"]) before checking totals and change the
totals check to avoid the plain "AMOUNT" token; apply the same reorder/token fix
to the other similar matchesPattern call that checks totals elsewhere in the
file.
Comment on lines
+19
to
22
| const rows = await clickhouse.query( | ||
| "SELECT splitsPerSecond, activeUsers, paymentSuccessRate, averageSettlementTime, geographicDistribution FROM platform_metrics ORDER BY timestamp DESC LIMIT 1", | ||
| ); | ||
| return rows[0] as PlatformMetrics; |
There was a problem hiding this comment.
Guard empty query results before returning PlatformMetrics.
If no rows exist, rows[0] is undefined, which breaks the declared return contract and can fail at runtime.
Suggested change
export async function getAggregatedMetrics(): Promise<PlatformMetrics> {
// Query ClickHouse for latest metrics, optimized for performance
const rows = await clickhouse.query(
"SELECT splitsPerSecond, activeUsers, paymentSuccessRate, averageSettlementTime, geographicDistribution FROM platform_metrics ORDER BY timestamp DESC LIMIT 1",
);
- return rows[0] as PlatformMetrics;
+ if (!rows.length) {
+ throw new Error("No platform metrics found");
+ }
+ return rows[0] as PlatformMetrics;
}📝 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
| const rows = await clickhouse.query( | |
| "SELECT splitsPerSecond, activeUsers, paymentSuccessRate, averageSettlementTime, geographicDistribution FROM platform_metrics ORDER BY timestamp DESC LIMIT 1", | |
| ); | |
| return rows[0] as PlatformMetrics; | |
| export async function getAggregatedMetrics(): Promise<PlatformMetrics> { | |
| // Query ClickHouse for latest metrics, optimized for performance | |
| const rows = await clickhouse.query( | |
| "SELECT splitsPerSecond, activeUsers, paymentSuccessRate, averageSettlementTime, geographicDistribution FROM platform_metrics ORDER BY timestamp DESC LIMIT 1", | |
| ); | |
| if (!rows.length) { | |
| throw new Error("No platform metrics found"); | |
| } | |
| return rows[0] as PlatformMetrics; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/realtime-analytics/analytics.aggregate.ts` around lines 19 - 22,
The code uses clickhouse.query and immediately returns rows[0] as
PlatformMetrics without guarding for empty results; update the logic around the
rows variable (the result of clickhouse.query) to check for an empty array
(e.g., rows.length === 0) and handle that case instead of returning undefined —
either throw a descriptive error like "No platform metrics found" or return a
well-formed fallback PlatformMetrics object so the function's declared return
contract is preserved; modify the block containing clickhouse.query and the
return of rows[0] as PlatformMetrics accordingly.
Comment on lines
+16
to
+25
| jest.mock("./analytics.aggregate", () => ({ | ||
| aggregateMetrics: jest.fn().mockResolvedValue(undefined), | ||
| getAggregatedMetrics: jest.fn().mockResolvedValue({ | ||
| splitsPerSecond: 10, | ||
| activeUsers: 100, | ||
| paymentSuccessRate: 0.98, | ||
| averageSettlementTime: 2.5, | ||
| geographicDistribution: { US: 50, UK: 30, IN: 20 }, | ||
| }), | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the test file exists and examine the mock and test
ls -la backend/src/realtime-analytics/analytics.test.ts
# Verify the mock usage in the test file
rg -n 'jest\.mock' backend/src/realtime-analytics/analytics.test.ts
# Look at lines around 16-25 to see the current mock
sed -n '10,30p' backend/src/realtime-analytics/analytics.test.ts
# Check the test at lines 47-51
sed -n '40,60p' backend/src/realtime-analytics/analytics.test.ts
# Find references to aggregateMetrics and getAggregatedMetrics in the test
rg -n 'aggregateMetrics|getAggregatedMetrics' backend/src/realtime-analytics/analytics.test.tsRepository: OlufunbiIK/StellarSplit
Length of output: 1665
🏁 Script executed:
# Check the analytics.aggregate module to understand its exports and dependencies
cat backend/src/realtime-analytics/analytics.aggregate.tsRepository: OlufunbiIK/StellarSplit
Length of output: 1005
🏁 Script executed:
# Verify the existence of ClickHouse dependency
rg -n '@apla/clickhouse|ClickHouse' backend/src/realtime-analytics/analytics.aggregate.ts
# Check if analytics.aggregate has any real logic that should be tested
wc -l backend/src/realtime-analytics/analytics.aggregate.tsRepository: OlufunbiIK/StellarSplit
Length of output: 277
🏁 Script executed:
# Read the entire test file to understand the full context
cat -n backend/src/realtime-analytics/analytics.test.tsRepository: OlufunbiIK/StellarSplit
Length of output: 2105
🏁 Script executed:
# Check if there are other mocks for ClickHouse
rg -n 'jest\.mock.*[Cc]lick|@apla' backend/src/realtime-analytics/analytics.test.tsRepository: OlufunbiIK/StellarSplit
Length of output: 49
Mocking ./analytics.aggregate prevents this test from validating real aggregation logic.
The test "should aggregate metrics in ClickHouse" (lines 47-51) calls only mocked functions that return hardcoded data. If the SQL query or insert logic in the real aggregateMetrics() or getAggregatedMetrics() breaks, this test would still pass undetected.
Instead, mock the actual external dependency (@apla/clickhouse) to allow the real aggregation logic to execute while keeping the test isolated from the database:
Suggested change
-jest.mock("./analytics.aggregate", () => ({
- aggregateMetrics: jest.fn().mockResolvedValue(undefined),
- getAggregatedMetrics: jest.fn().mockResolvedValue({
- splitsPerSecond: 10,
- activeUsers: 100,
- paymentSuccessRate: 0.98,
- averageSettlementTime: 2.5,
- geographicDistribution: { US: 50, UK: 30, IN: 20 },
- }),
-}));
+jest.mock("@apla/clickhouse", () =>
+ jest.fn().mockImplementation(() => ({
+ insert: jest.fn().mockResolvedValue(undefined),
+ query: jest.fn().mockResolvedValue([
+ {
+ splitsPerSecond: 10,
+ activeUsers: 100,
+ paymentSuccessRate: 0.98,
+ averageSettlementTime: 2.5,
+ geographicDistribution: { US: 50, UK: 30, IN: 20 },
+ },
+ ]),
+ })),
+);📝 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
| jest.mock("./analytics.aggregate", () => ({ | |
| aggregateMetrics: jest.fn().mockResolvedValue(undefined), | |
| getAggregatedMetrics: jest.fn().mockResolvedValue({ | |
| splitsPerSecond: 10, | |
| activeUsers: 100, | |
| paymentSuccessRate: 0.98, | |
| averageSettlementTime: 2.5, | |
| geographicDistribution: { US: 50, UK: 30, IN: 20 }, | |
| }), | |
| })); | |
| jest.mock("@apla/clickhouse", () => | |
| jest.fn().mockImplementation(() => ({ | |
| insert: jest.fn().mockResolvedValue(undefined), | |
| query: jest.fn().mockResolvedValue([ | |
| { | |
| splitsPerSecond: 10, | |
| activeUsers: 100, | |
| paymentSuccessRate: 0.98, | |
| averageSettlementTime: 2.5, | |
| geographicDistribution: { US: 50, UK: 30, IN: 20 }, | |
| }, | |
| ]), | |
| })), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/realtime-analytics/analytics.test.ts` around lines 16 - 25, Stop
mocking the entire "./analytics.aggregate" module so the real aggregateMetrics
and getAggregatedMetrics implementations run; instead replace the mock with a
Jest mock for the external ClickHouse client (module "@apla/clickhouse") so
database calls are stubbed but aggregation logic is exercised. Concretely,
remove or change the jest.mock("./analytics.aggregate", ...) block and add a
jest.mock("@apla/clickhouse", ...) that returns a fake client exposing the same
query/insert methods used by aggregateMetrics/getAggregatedMetrics; keep the
test assertions the same so failures in SQL/aggregation surface while tests
remain isolated from a real DB.
Comment on lines
+1
to
+5
| import { Service } from "aws-sdk"; | ||
|
|
||
| it("should be defined", () => { | ||
| expect(Service).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: confirm this spec does not exercise local receipts code.
set -euo pipefail
echo "=== backend/src/receipts/receipts.controller.spec.ts ==="
nl -ba backend/src/receipts/receipts.controller.spec.ts
echo
echo "=== Match placeholder pattern ==="
rg -n -C2 'import\s+\{\s*Service\s*\}\s+from\s+"aws-sdk"|expect\(\s*Service\s*\)\.toBeDefined\(\)' \
backend/src/receipts/receipts.controller.spec.ts
echo
echo "=== Check for imports of module-under-test in this file ==="
rg -n -C2 'from\s+["'\'']\./|from\s+["'\'']\.\./' \
backend/src/receipts/receipts.controller.spec.ts || trueRepository: OlufunbiIK/StellarSplit
Length of output: 183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== backend/src/receipts/receipts.controller.spec.ts ==="
cat -n backend/src/receipts/receipts.controller.spec.ts
echo
echo "=== Check for placeholder test pattern ==="
rg -n 'Service|toBeDefined' backend/src/receipts/receipts.controller.spec.ts || echo "No matches found"
echo
echo "=== Check for imports of module-under-test ==="
rg -n 'from.*\./|from.*\.\./' backend/src/receipts/receipts.controller.spec.ts || echo "No local imports found"Repository: OlufunbiIK/StellarSplit
Length of output: 450
Replace this placeholder test with a real receipts-controller assertion.
This test only verifies that an external AWS SDK symbol exists—it doesn't exercise the receipts controller at all. The spec file has no imports of the module under test, so this test provides zero regression protection and can pass even when the controller is broken.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/receipts/receipts.controller.spec.ts` around lines 1 - 5, Replace
the meaningless aws-sdk check with a real unit test for the receipts controller:
import ReceiptsController and its dependent ReceiptsService, build a Nest
Test.createTestingModule providing a mock provider for ReceiptsService, retrieve
the ReceiptsController from the module, assert the controller is defined, and
add at least one behavioral assertion (e.g., call a public method like
findAll/getReceipts/createReceipt with a mocked return value and assert the
result). Remove the import of Service from "aws-sdk" and the trivial test that
asserts Service is defined.
Comment on lines
+52
to
+58
| { | ||
| provide: EventsGateway, | ||
| useValue: { | ||
| server: { emit: jest.fn() }, | ||
| emit: jest.fn(), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Mock EventsGateway using its real contract.
On Lines 52-58, the mock exposes emit and server.emit, but the real gateway emits via server.to(...).emit(...) and public methods like emitPaymentReceived. This can let incorrect integration behavior pass unnoticed.
Proposed mock shape fix
{
provide: EventsGateway,
useValue: {
- server: { emit: jest.fn() },
- emit: jest.fn(),
+ server: {
+ to: jest.fn().mockReturnValue({ emit: jest.fn() }),
+ },
+ emitPaymentReceived: jest.fn(),
+ emitSplitUpdated: jest.fn(),
+ emitParticipantJoined: jest.fn(),
},
},📝 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
| { | |
| provide: EventsGateway, | |
| useValue: { | |
| server: { emit: jest.fn() }, | |
| emit: jest.fn(), | |
| }, | |
| }, | |
| { | |
| provide: EventsGateway, | |
| useValue: { | |
| server: { | |
| to: jest.fn().mockReturnValue({ emit: jest.fn() }), | |
| }, | |
| emitPaymentReceived: jest.fn(), | |
| emitSplitUpdated: jest.fn(), | |
| emitParticipantJoined: jest.fn(), | |
| }, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/tests/payment-integration.test.ts` around lines 52 - 58, The
test's EventsGateway mock doesn't match the real contract (real gateway uses
server.to(...).emit(...) and public methods like emitPaymentReceived), so update
the mock for EventsGateway to implement a server object whose to() returns an
object with emit as jest.fn(), and also include the gateway's public methods
(e.g., emitPaymentReceived) as jest.fn() or delegate to
server.to(...).emit(...); ensure any existing references to emit use the new
methods so the mock shape mirrors the real EventsGateway API.
Comment on lines
+67
to
113
| it("should process a complete payment", async () => { | ||
| // Mock the Stellar service to return a valid transaction | ||
| jest.spyOn(stellarService, 'verifyTransaction').mockResolvedValue({ | ||
| jest.spyOn(stellarService, "verifyTransaction").mockResolvedValue({ | ||
| valid: true, | ||
| amount: 50.00, | ||
| asset: 'XLM-USDC', | ||
| sender: 'GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN', | ||
| receiver: 'GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H', | ||
| timestamp: '2023-01-01T00:00:00Z', | ||
| amount: 50.0, | ||
| asset: "XLM-USDC", | ||
| sender: "GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN", | ||
| receiver: "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", | ||
| timestamp: "2023-01-01T00:00:00Z", | ||
| }); | ||
|
|
||
| // In a real test, we would mock repository methods and test the full flow | ||
| expect(service).toBeDefined(); | ||
| }); | ||
|
|
||
| it('should handle partial payments', async () => { | ||
| it("should handle partial payments", async () => { | ||
| // Mock a transaction with amount less than owed | ||
| jest.spyOn(stellarService, 'verifyTransaction').mockResolvedValue({ | ||
| jest.spyOn(stellarService, "verifyTransaction").mockResolvedValue({ | ||
| valid: true, | ||
| amount: 25.00, // Less than owed amount | ||
| asset: 'XLM-USDC', | ||
| sender: 'GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN', | ||
| receiver: 'GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H', | ||
| timestamp: '2023-01-01T00:00:00Z', | ||
| amount: 25.0, // Less than owed amount | ||
| asset: "XLM-USDC", | ||
| sender: "GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN", | ||
| receiver: "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", | ||
| timestamp: "2023-01-01T00:00:00Z", | ||
| }); | ||
|
|
||
| expect(service).toBeDefined(); | ||
| }); | ||
|
|
||
| it('should reject invalid transactions', async () => { | ||
| it("should reject invalid transactions", async () => { | ||
| // Mock an invalid transaction | ||
| jest.spyOn(stellarService, 'verifyTransaction').mockResolvedValue({ | ||
| jest.spyOn(stellarService, "verifyTransaction").mockResolvedValue({ | ||
| valid: false, | ||
| amount: 0, | ||
| asset: '', | ||
| sender: '', | ||
| receiver: '', | ||
| timestamp: '2023-01-01T00:00:00Z', | ||
| asset: "", | ||
| sender: "", | ||
| receiver: "", | ||
| timestamp: "2023-01-01T00:00:00Z", | ||
| }); | ||
|
|
||
| expect(service).toBeDefined(); | ||
| }); | ||
|
|
||
| it('should prevent duplicate transactions', async () => { | ||
| it("should prevent duplicate transactions", async () => { | ||
| // This test would verify that the service prevents duplicate submissions | ||
| expect(service).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
Tests don’t exercise payment-processing behavior yet.
On Lines 67-113, all four tests end with expect(service).toBeDefined(). They currently pass even if transaction validation, persistence, duplicate prevention, or gateway emissions are broken.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/tests/payment-integration.test.ts` around lines 67 - 113, The
tests currently only assert service is defined and don't exercise payment
processing logic; replace the final expect(service).toBeDefined() in each test
with calls to the payment handler on service (e.g., service.processPayment or
the actual method that handles verified transactions) and assert real outcomes:
after mocking stellarService.verifyTransaction, assert repository/save methods
were called with expected payment records, assert domain state changes (paid vs
partial) and that the event/gateway emitter was invoked, and for duplicates
assert the repository lookup prevented a second save and an error/ignore
behavior; add spies/mocks for the repository (save/findByTxId) and event emitter
to verify these side effects. Ensure each test uses the specific mocked response
from stellarService.verifyTransaction and verifies the corresponding handling
path (complete, partial, invalid, duplicate).
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.
Summary by CodeRabbit