feat(api): format webhook email payload#955
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48f6d946-ae22-4440-b7a1-44e939419b11
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48f6d946-ae22-4440-b7a1-44e939419b11
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48f6d946-ae22-4440-b7a1-44e939419b11
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48f6d946-ae22-4440-b7a1-44e939419b11
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48f6d946-ae22-4440-b7a1-44e939419b11
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 1 critical |
| CodeStyle | 50 minor |
🟢 Metrics 46 complexity · 4 duplication
Metric Results Complexity 46 Duplication 4
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Greptile SummaryThis PR adds pretty-printed, syntax-highlighted JSON rendering to the webhook failure notification email's Event Payload field — HTML emails get a styled
Confidence Score: 4/5Safe to merge; the change is self-contained to the emails package with no interface or dependency modifications. The formatter and theme change are correct and well-tested. The two minor gaps are the silent fallback to the raw placeholder string in plain-text if hermes output ever stops containing it, and the missing NotContains(email.HTML, placeholder) assertion that would catch a future empty-UnsafeValue regression. api/pkg/emails/hermes_notification_email_factory.go and its test — the placeholder replacement pattern has no observable failure mode, and the test suite does not assert the placeholder is absent from the HTML output. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant WF as WebhookSendFailed
participant FEP as formatEventPayload
participant IJ as indentEventPayloadJSON
participant HJ as highlightEventPayloadJSON
participant HG as hermes.GenerateHTML
participant HT as hermes.GeneratePlainText
participant RPP as replaceWebhookSendFailedEventPayloadPlaceholder
WF->>FEP: payload.EventPayload (raw string)
FEP->>IJ: payload string
IJ-->>FEP: (formattedPayload, isJSON)
alt "isJSON == true"
FEP->>HJ: formattedPayload
HJ-->>FEP: HTML with escaped spans
else "isJSON == false"
FEP->>FEP: html.EscapeString(formattedPayload)
end
FEP-->>WF: (formattedPayload, template.HTML pre-block)
WF->>HG: hermes.Entry with placeholder Value and richHTML UnsafeValue
HG-->>WF: HTML email with UnsafeValue rendered
WF->>HT: hermes.Entry with placeholder Value
HT-->>WF: plain text containing placeholder
WF->>RPP: (plainText, formattedPayload)
RPP-->>WF: before + formattedPayload + after
WF-->>WF: "return Email{HTML, Text}"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant WF as WebhookSendFailed
participant FEP as formatEventPayload
participant IJ as indentEventPayloadJSON
participant HJ as highlightEventPayloadJSON
participant HG as hermes.GenerateHTML
participant HT as hermes.GeneratePlainText
participant RPP as replaceWebhookSendFailedEventPayloadPlaceholder
WF->>FEP: payload.EventPayload (raw string)
FEP->>IJ: payload string
IJ-->>FEP: (formattedPayload, isJSON)
alt "isJSON == true"
FEP->>HJ: formattedPayload
HJ-->>FEP: HTML with escaped spans
else "isJSON == false"
FEP->>FEP: html.EscapeString(formattedPayload)
end
FEP-->>WF: (formattedPayload, template.HTML pre-block)
WF->>HG: hermes.Entry with placeholder Value and richHTML UnsafeValue
HG-->>WF: HTML email with UnsafeValue rendered
WF->>HT: hermes.Entry with placeholder Value
HT-->>WF: plain text containing placeholder
WF->>RPP: (plainText, formattedPayload)
RPP-->>WF: before + formattedPayload + after
WF-->>WF: "return Email{HTML, Text}"
Reviews (1): Last reviewed commit: "fix(api): harden webhook payload formatt..." | Re-trigger Greptile |
| func replaceWebhookSendFailedEventPayloadPlaceholder(text string, formattedPayload string) string { | ||
| before, after, found := strings.Cut(text, webhookSendFailedEventPayloadPlaceholder) | ||
| if !found { | ||
| return text | ||
| } | ||
|
|
||
| return before + formattedPayload + after | ||
| } |
There was a problem hiding this comment.
Silent plain-text fallback exposes placeholder string
replaceWebhookSendFailedEventPayloadPlaceholder returns text unchanged when the placeholder is not found. At that point text is the hermes-generated plain-text output, which still contains __HTTPSMS_WEBHOOK_SEND_FAILED_EVENT_PAYLOAD__ as the literal Value of the dictionary entry. If hermes or the underlying html2text converter ever transforms or trims this token (unlikely today but possible on a dependency bump), users would receive a webhook failure email whose "Event Payload" line reads __HTTPSMS_WEBHOOK_SEND_FAILED_EVENT_PAYLOAD__ instead of the actual payload. An alternative would be to set Value to formattedPayload directly and accept that html2text may collapse its whitespace, or to emit an observable signal (log/metric) when !found.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 9fa5b76. If Hermes no longer preserves the sentinel, plain-text generation now retries with the actual formatted payload instead of exposing the internal token.
| assert.NotContains(t, email.Text, "<pre") | ||
| assert.NotContains(t, email.Text, webhookSendFailedEventPayloadPlaceholder) |
There was a problem hiding this comment.
Adding the mirrored assertion makes the test fail-fast if
UnsafeValue is ever accidentally empty and the template falls back to rendering the placeholder as the Value.
| assert.NotContains(t, email.Text, "<pre") | |
| assert.NotContains(t, email.Text, webhookSendFailedEventPayloadPlaceholder) | |
| assert.NotContains(t, email.Text, "<pre") | |
| assert.NotContains(t, email.Text, webhookSendFailedEventPayloadPlaceholder) | |
| assert.NotContains(t, email.HTML, webhookSendFailedEventPayloadPlaceholder) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 9fa5b76. The factory test now asserts the sentinel is absent from both HTML and plain-text output.
There was a problem hiding this comment.
Pull request overview
This PR improves readability of webhook failure notification emails in the API by pretty-printing the webhook Event Payload JSON and rendering it as a safe, whitespace-preserving code block in HTML (with lightweight syntax highlighting), while keeping plain-text emails readable and preserving existing rendering for all other dictionary entries.
Changes:
- Added an
Event Payloadformatter that pretty-prints valid JSON and produces safely-escaped highlighted HTML wrapped in an email-friendly<pre>block. - Updated the Hermes HTML theme dictionary rendering to optionally use
hermes.Entry.UnsafeValuewhen explicitly provided. - Wired the formatter into
WebhookSendFailedonly, including a placeholder-based restoration step to preserve payload whitespace in the generated plain-text output, plus targeted tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| docs/superpowers/specs/2026-07-18-webhook-email-payload-formatting-design.md | Captures design decisions and security constraints for formatting only the webhook Event Payload. |
| docs/superpowers/plans/2026-07-18-webhook-email-payload-formatting.md | Step-by-step implementation plan and intended test coverage for the formatter + Hermes theme + factory wiring. |
| api/pkg/emails/hermes_theme.go | Enables opt-in HTML dictionary rendering via Entry.UnsafeValue while keeping default escaped Value behavior. |
| api/pkg/emails/hermes_theme_test.go | Tests that only entries with UnsafeValue render unescaped in HTML and that plain text still uses Value. |
| api/pkg/emails/hermes_notification_email_factory.go | Formats webhook Event Payload and injects rich HTML safely; restores plain-text payload formatting via placeholder replacement. |
| api/pkg/emails/hermes_notification_email_factory_test.go | End-to-end tests verifying only Event Payload uses <pre>/highlighting and plain text does not leak HTML or placeholders. |
| api/pkg/emails/event_payload_formatter.go | Implements JSON indentation + safe token-level highlighting and HTML escaping inside a styled <pre> wrapper. |
| api/pkg/emails/event_payload_formatter_test.go | Covers valid/invalid JSON formatting, escaping, top-level JSON shapes, and ensures highlighting is applied only when appropriate. |
Integrate current origin/main (thread archive-view preservation #952, unarchive-on-receive #954, webhook email payload formatting #955) with the read-receipts feature set, preserving both. Semantic resolutions: - UpdateThread now resolves the per-phone unarchive decision and the inbound unread watermark in a single atomic repository.UpdateActivity call. Added an Unarchive flag to MessageThreadActivityUpdate so is_archived=false is applied in the same transaction as the read-state/last-message updates. - Missed calls and inbound SMS both use MessageStatusReceived + MarksUnread, so they share the same unarchive + mark-unread rules. - Combined the message thread service test suites (read-receipt stubs plus shouldCheckUnarchive coverage) and updated the service/handler test helpers to the 5-arg NewMessageThreadService signature (adds phoneRepository). - Merged tests/README.md coverage entries for both E2E suites. - Regenerated Swagger and web/shared/types/api.ts so both main's new fields and read-receipt fields are present. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf00a0ac-e11f-4015-b295-3cdd9b491229
Summary
Testing
go test ./pkg/emails -count=1go test -vet=offfor all API packages exceptpkg/handlersgo test ./...has pre-existing Go vet failures in stacktrace calls, and the handler package includes a localhost-dependent integration test.