bug: fix createLikes notification logic and avoid mutating updateTransactionById edits - #129
devin-ai-integration[bot] wants to merge 2 commits into
Conversation
…Id edits Co-Authored-By: Parker Drew <parker.drew@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
| const sender = getUserById(senderId); | ||
| const receiver = getUserById(receiverId); | ||
|
|
||
| const finalEdits = { ...edits }; |
There was a problem hiding this comment.
📝 Info: Shallow copy of edits prevents mutation of caller's object
The change at backend/database.ts:584 creates finalEdits = { ...edits } so that when finalEdits.status is set to TransactionStatus.complete on line 590, the original edits object passed by the caller is not mutated. This matters because updateTransactionById is called from backend/transaction-routes.ts:179 with req.body, and mutating req.body could cause subtle issues if it's referenced again later. The fix is correct and well-scoped.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (userId !== senderId && userId !== receiverId) { | ||
| createLikeNotification(senderId, transactionId, like.id); | ||
| createLikeNotification(receiverId, transactionId, like.id); | ||
| } else if (userId === senderId) { | ||
| createLikeNotification(senderId, transactionId, like.id); | ||
| } else { | ||
| createLikeNotification(receiverId, transactionId, like.id); | ||
| } else { | ||
| createLikeNotification(senderId, transactionId, like.id); | ||
| } |
There was a problem hiding this comment.
📝 Info: Original createLikes condition was always true, making branches unreachable
The old condition userId !== senderId || userId !== receiverId at backend/database.ts:628 (LEFT) was a tautology whenever senderId !== receiverId (which is always true since a user cannot transact with themselves). This means the old code always entered the first branch, sending notifications to both parties regardless of who liked the transaction. The fix to && correctly makes the condition false when the liker is either the sender or receiver, allowing the sub-branches to execute. The swapped notification targets in lines 632-635 are also correct: when the liker is the sender, only the receiver should be notified, and vice versa.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-Authored-By: Parker Drew <parker.drew@cognition.ai>
| if (userId !== senderId && userId !== receiverId) { | ||
| createLikeNotification(senderId, transactionId, like.id); | ||
| createLikeNotification(receiverId, transactionId, like.id); | ||
| } else if (userId === senderId) { | ||
| createLikeNotification(senderId, transactionId, like.id); | ||
| } else { | ||
| createLikeNotification(receiverId, transactionId, like.id); | ||
| } else { | ||
| createLikeNotification(senderId, transactionId, like.id); | ||
| } |
There was a problem hiding this comment.
🚩 Condition fix changes notification behavior for all like/comment actions
The old condition userId !== senderId || userId !== receiverId was always true when sender ≠ receiver (which is the normal case for transactions), meaning both sender and receiver were always notified — even when one of them was the person performing the action. The else if / else branches were dead code (confirmed by /* istanbul ignore next */ markers that were removed).
The new && condition means the first branch only fires for third-party users. When the liker/commenter is the sender or receiver, only the OTHER party is now notified. This is a behavioral change: previously both parties always got notifications; now participants don't get self-notifications.
This is almost certainly the intended fix, but it's worth confirming there are no E2E tests or frontend logic that depend on self-notifications being generated (e.g., notification count assertions in Cypress tests at cypress/tests/).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed intended — this matches the behavior requested in the task. Checked for dependencies:
- CI (
.github/workflows/main.yml) only runsyarn test:unit:ci(vitest); the Cypress E2E suite isn't run in CI. All 44 unit tests pass, includinglikes.test.ts,comments.test.ts, andnotifications.test.ts. - The E2E notification specs (
cypress/tests/ui/notifications.spec.ts) assert on the other party's notification counts (third-party liker → both notified; participant liker → other party notified), not on self-notifications, so they're consistent with this change.
| if (userId !== senderId && userId !== receiverId) { | ||
| createLikeNotification(senderId, transactionId, like.id); | ||
| createLikeNotification(receiverId, transactionId, like.id); | ||
| } else if (userId === senderId) { | ||
| createLikeNotification(senderId, transactionId, like.id); | ||
| } else { | ||
| createLikeNotification(receiverId, transactionId, like.id); | ||
| } else { | ||
| createLikeNotification(senderId, transactionId, like.id); | ||
| } |
There was a problem hiding this comment.
📝 Info: Edge case: sender equals receiver is not guarded
If a transaction somehow has senderId === receiverId, the first branch (userId !== senderId && userId !== receiverId) would be false when the user is that person, falling to else if (userId === senderId) which would call createLikeNotification(receiverId, ...) — effectively a self-notification. This is the same behavior as before the change, so it's not a regression. In practice, createTransaction at backend/database.ts:528 takes separate userId and receiverId, so this edge case shouldn't occur in normal operation.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed — not a regression (same behavior as before for the senderId === receiverId case), and transactions are always created with distinct sender/receiver, so it won't occur in practice. Leaving as-is to keep the change scoped.
Summary
Bug fixes in
backend/database.ts.createLikes— always-true condition made branches dead code. The guarduserId !== senderId || userId !== receiverIdis alwaystrue(oneuserIdcan't equal both), so theelse if/elsenever ran. Changed||→&&and corrected which party each branch notifies:Removed the
/* istanbul ignore next */since the branches are now reachable.createComments— identical bug.createCommentshad the exact same always-true condition and self-notifying branches. Applied the same fix (flagged by Devin Review).updateTransactionById— mutated itseditsargument. It setstatusdirectly on the caller-suppliedPartial<Transaction>(which isreq.bodyat the call site). Now builds a local copy so the input is untouched; behavior (status set tocompleteonly inside theisRequestTransactionbranch) is preserved:yarn types(tsc --noEmit) and prettier check pass.Link to Devin session: https://app.devin.ai/sessions/b5be0ee641734dc091f3756e5d9656a3
Requested by: @parkerwindsurf
Devin Review