Name the null reverse-path instead of showing bare angle brackets - #202
Conversation
A reporter that sees an empty envelope sender reports it as `<>`, the RFC 5321 null reverse-path, and the per-source panel printed it verbatim — a glyph that means nothing outside SMTP. It now reads "null sender <>", with a title explaining that it is what bounces, delivery status notifications and auto-replies look like. Not relabelled "none" or "empty" as the issue asked: that is the *other* case, a reporter omitting the element entirely, and conflating the two would report a source as having sent no envelope sender when what it actually sent was a bounce. Those omitted values turned out to be dropped in the wrong place — after the top-ten cap rather than before it. An omitted identifier is a large group in practice, often the largest, so it took a slot and was then discarded, leaving the panel showing nine values with no sign a tenth had been displaced. The filter moves into the query, and trims while it is there, because nothing trims these on the way in and a reporter that pretty-prints its XML stores the surrounding indentation as the value. Refs #196 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe API filters empty and whitespace-only source identifiers before top-ten aggregation. The web interface identifies RFC 5321 null reverse-path values as “null sender” while preserving the raw marker. Integration and component tests cover these cases. ChangesSource Identifier Handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to This PR makes null reverse-paths clearer and prevents omitted identifiers from consuming displayed slots. A bounded UI correctness risk remains because some header-from values may still be labeled as envelope senders, which could mislead users; merge is reasonable with explicit owner follow-up. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
src/web/src/pages/DomainDetailPage.tsx-216-216 (1)
216-216: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestrict the null-sender label to envelope-from values.
ValueListalso rendersdetail.headerFromsat Line 1632. A header-from value of<>therefore receives an RFC 5321 envelope-sender label. Pass an explicit value type or formatter, and enable this special rendering only fordetail.envelopeFroms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: fe0d7ccd-65d9-4e03-ad3b-21c9b0e42ba5
📒 Files selected for processing (4)
src/api.integration.tests/SourceIdentifierAnalyticsPostgresTests.cssrc/api/Application/Analytics/AnalyticsQueryService.cssrc/web/src/pages/DomainDetailPage.identifiers.test.tsxsrc/web/src/pages/DomainDetailPage.tsx
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
ValueList renders the header-from panel as well, and `<>` there is a malformed `From:` rather than a null reverse-path — that is an SMTP envelope concept. The label would have asserted the source sent bounces on the evidence of a broken reporter, which is the same conflation this change set out to avoid. It is now opt-in per list. Refs #196 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Labels RFC 5321 null reverse-paths clearly and prevents omitted identifiers from consuming top-ten analytics slots.
Changes:
- Renders
<>asnull sender <>with explanatory context. - Filters blank identifiers before grouping and limiting.
- Adds frontend and PostgreSQL integration coverage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/web/src/pages/DomainDetailPage.tsx |
Adds null-sender rendering. |
src/web/src/pages/DomainDetailPage.identifiers.test.tsx |
Tests identifier rendering states. |
src/api/Application/Analytics/AnalyticsQueryService.cs |
Filters blank identifiers before aggregation. |
src/api.integration.tests/SourceIdentifierAnalyticsPostgresTests.cs |
Verifies filtering and null-path preservation against PostgreSQL. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The per-source panel printed
<>verbatim in its Envelope from list. That isnot corrupt data and not an escaping bug: it is the RFC 5321 null reverse-path,
which a reporter sends when the source's envelope sender was empty — what
bounces, delivery status notifications and auto-replies use. It just reads as a
glyph nobody outside SMTP recognises.
It now renders as
null sender <>, with atitleexplaining what sends it.Not "none" or "empty"
The issue asked for that wording and this deliberately does not use it, because
the panel has two distinct states and those labels name the wrong one:
<envelope_from>""<>null sender <>Calling the second one "empty" would say a source sent no envelope sender when
what it actually sent was a bounce. The label loses a real signal about the
traffic.
The bug next door
Tracing it turned up a second problem in the same list.
GroupValuesAsynctookthe top ten values in the database and then dropped the blanks client-side:
An omitted identifier is a large group in practice — often the largest a source
has — so it won a place in the ten and was then discarded. The panel showed
nine values and gave no sign a tenth had been displaced. The filter moves into
the query, ahead of the cap.
It trims, because nothing trims these identifiers on ingest: a reporter that
pretty-prints its XML stores the surrounding newline and indentation as the
value, which is the same absence as an empty string. (The parser already trims
source_ipfor a related reason, with a comment saying so.)Verification
dotnet test src/api.tests— 802 passeddotnet test src/api.integration.tests— 78 passedcd src/web && npm run build && npm run lint && npm test— clean, 35 passedThe new backend tests are in the integration suite, not the fast one, and
have to be: the filter exists to stop an omitted value consuming one of ten
slots, which only means anything if the database applies the cap — and InMemory
would evaluate
Trim()in C# whether or not Npgsql can translate it, so thefast suite cannot tell a working query from one that 500s the panel at runtime.
Confirmed against PostgreSQL 18, and confirmed failing against the pre-fix code.
Not verified in a running browser — the change is a fifteen-line render branch
covered by a DOM test, and standing the authenticated app up with report data
was more than it warranted. Worth a glance on the panel before merge if you
disagree.
One thing left open
An omitted identifier now disappears from the list cleanly rather than
displacing a value, but it still disappears silently — its message volume is
simply absent from a panel that otherwise accounts for everything. Showing it
as a
(not reported)row is a product call rather than a bug fix, so it is notin here.
Refs #196