Fix duplicate mock property and lint issues from email audit trail PR - #4495
Fix duplicate mock property and lint issues from email audit trail PR#4495builder-io-integration[bot] wants to merge 8 commits into
Conversation
… payload sizes, stable pagination sort
… payload sizes, stable pagination sort
…a4e758630' into ai_main_4a3db45f528a4e758630 # Conflicts: # packages/core/src/email-catalog/log.ts # packages/core/src/email-catalog/schema.ts # packages/core/src/localization/default-messages.ts # packages/dispatch/src/components/transactional-email-send-log.tsx
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Code Review Summary
PR #4495 completes the transactional-email audit trail from #4357: it adds additive log columns and indexes, captures provider request/response metadata with body and attachment redaction, tags Better Auth magic-link sends, exposes richer filters through list-email-log, and adds the Dispatch send-log UI. The implementation preserves organization/app scoping, distinguishes provider HTTP failures from pre-provider exceptions, bounds query sizes, and uses React-rendered text for logged values. The changeset correctly covers both affected packages.
Risk assessment: Standard, because this changes shared email delivery, persisted audit data, authorization-sensitive log access, and an authentication email path.
Key Findings
- 🟡 MEDIUM — Offset pagination can duplicate or skip rows when new sends arrive between page requests, despite the code comment promising stable paging. A cursor/keyset boundary or fixed snapshot cutoff is needed for a live audit log.
- The recipient/sender
LIKEfilters also do not escape%and_, so wildcard characters do not behave as the documented literal substring match; this is a lower-severity correctness issue.
The redaction strategy is a strong pattern: email bodies, magic-link tokens, and attachment bytes are omitted before persistence, while provider status/body remain available for diagnostics. Focused review agents found no additional confirmed authorization, migration, or UI correctness issues.
🧪 Browser testing: Will run after this review (PR touches UI code)
| sql: `SELECT ${LOG_COLUMNS} FROM email_log | ||
| WHERE ${where.join(" AND ")} | ||
| ORDER BY created_at DESC, id DESC | ||
| LIMIT ? OFFSET ?`, |
There was a problem hiding this comment.
🟡 Offset pagination is not stable while new sends arrive
The UI requests separate pages with OFFSET, so a new send inserted between page requests shifts every later row: the next page can duplicate a row already shown or skip a row. The id tie-breaker only stabilizes equal timestamps; use a cursor/keyset boundary based on the last (created_at, id) or a fixed snapshot cutoff for the paging session.
Additional Info
Found by 1 of 3 review agents; confirmed against the live-send paging flow.
There was a problem hiding this comment.
Reviewed. The offset-based pagination pattern flagged here (LIMIT ? OFFSET ? with created_at DESC, id DESC tie-break) is pre-existing — it was introduced by the already-merged #4357 and is unchanged by this PR, which is scoped only to the TS1117 duplicate-property fix in the log.spec.ts mock and oxfmt formatting. I also confirmed the same offset pattern is already live on main today (including in the newer, further-evolved implementation merged via #4496/#4504), so this isn't a regression this branch introduces. Agreed it's a real limitation for high-volume concurrent sends, but a keyset/cursor rework is a scope change for an admin log's pagination, not a lint fix — leaving it for a separate follow-up rather than bundling it here.
|
@builderio-bot look at the latest PR feedback and fix anything you agree with. Be skeptical. Reply on each comment thread whether you fixed it and why. Get CI green and keep the branch mergeable. |
2 similar comments
|
@builderio-bot look at the latest PR feedback and fix anything you agree with. Be skeptical. Reply on each comment thread whether you fixed it and why. Get CI green and keep the branch mergeable. |
|
@builderio-bot look at the latest PR feedback and fix anything you agree with. Be skeptical. Reply on each comment thread whether you fixed it and why. Get CI green and keep the branch mergeable. |
|
For some reason automation has pinged this PR 3 times, investigating... |
|
Confirmed clean — the merge was aborted successfully and HEAD is back at the original PR commit Here's the fuller picture after digging deeper: The one thing this PR actually still fixes is real and still needed. I checked But the branch itself is far too stale to reconcile safely. Doing the merge for real (not Recommendation stands, sharpened: rather than reconciling this branch, someone should fix the still-live duplicate-property bug directly against current I'll leave the branch untouched rather than force a risky reconciliation, since this needs your call. ⠀ |
4 participants
Follow-up to #4357 (already merged).
ensureColumnExistsproperty inpackages/core/src/email-catalog/log.spec.tsmock (TS1117 duplicate object-literal property), flagged in post-merge PR feedback on Add filterable audit trail for transactional email sends #4357.oxfmt) in the three files touched by Add filterable audit trail for transactional email sends #4357.@agent-native/core: patch,@agent-native/dispatch: minor).mainthat had reintroduced the duplicate mock property and stale schema types.