fix(subscriber): keep issue comment lookups on the owning save's connection - #3536
fallenbagel wants to merge 1 commit into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthrough
ChangesIssue comment notification flow
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: 🔵 Low · up to In the uncommon case that saving a comment fails at commit, users may receive a notification for a comment that does not exist. Dispatch notifications from a post-commit outbox to make delivery consistent with saved comments. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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. I’m a rabbit with comments to send, Comment |
Description
IssueCommentSubscriberlooked up the comment it had just inserted using the globalgetRepository(). That can use a different connection from the one running the insert transaction. On Postgres, the second connection cannot see the uncommitted row, sofindOneOrFailthrows and theISSUE_COMMENTnotification is dropped for every agent. SQLite effectively uses a single shared connection, so the lookup succeeds there, which is why this did not surface in SQLite.This changes the lookup to use
event.manager, which is tied to the connection running the insert transaction. This follows the same pattern already used byMediaSubscriberandMediaRequestSubscribersince #3365.How Has This Been Tested?
Tested manually by adding a comment on an existing issue, on both
SQLite and Postgres.
Screenshots / Logs (if applicable)
Before the fix on Postgres, every comment threw:
After the fix on Postgres, the notification sends correctly:
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit