Skip to content

Conversation

witoszekdev
Copy link
Member

@witoszekdev witoszekdev commented Aug 18, 2025

Scope of the PR

This PR fixes errors when configuring SMTP app, by using correct Handlebars instance for parsing settings.
Previously it uses Handlebars , which did not include all handlers.

It also introduces proper validation for email templates before saving them. We will validate templates when rendering preview and display error messages with helpful tips. Validation is also done on save to prevent corrupted templates.

For existing incorrect configurations we will no longer log them as errors, and instead will use logger.warn. Note we already send HTTP 400 for user errors to Saleor.

Preview

CleanShot 2025-10-02 at 19 10 20@2x CleanShot 2025-10-02 at 19 10 45@2x CleanShot 2025-10-02 at 19 10 51@2x CleanShot 2025-10-02 at 19 11 12@2x

@witoszekdev witoszekdev requested a review from a team as a code owner August 18, 2025 14:19
Copy link

changeset-bot bot commented Aug 18, 2025

🦋 Changeset detected

Latest commit: a13881f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
saleor-app-smtp Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Aug 18, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
saleor-app-search Ready Ready Preview Oct 3, 2025 4:19pm
8 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
saleor-app-avatax Ignored Ignored Preview Oct 3, 2025 4:19pm
saleor-app-cms Ignored Ignored Preview Oct 3, 2025 4:19pm
saleor-app-klaviyo Ignored Ignored Preview Oct 3, 2025 4:19pm
saleor-app-payment-np-atobarai Ignored Ignored Preview Comment Oct 3, 2025 4:19pm
saleor-app-payment-stripe Ignored Ignored Preview Oct 3, 2025 4:19pm
saleor-app-products-feed Ignored Ignored Preview Oct 3, 2025 4:19pm
saleor-app-segment Ignored Ignored Preview Oct 3, 2025 4:19pm
saleor-app-smtp Ignored Ignored Preview Comment Oct 3, 2025 4:19pm

@witoszekdev witoszekdev marked this pull request as draft August 18, 2025 14:22
Copy link

codecov bot commented Aug 18, 2025

Codecov Report

❌ Patch coverage is 37.39377% with 221 lines in your changes missing coverage. Please review.
✅ Project coverage is 35.10%. Comparing base (abcb3cf) to head (a13881f).

Files with missing lines Patch % Lines
...mtp/src/modules/smtp/ui/template-error-display.tsx 0.00% 155 Missing and 1 partial ⚠️
apps/smtp/src/modules/smtp/ui/event-form.tsx 0.00% 28 Missing ⚠️
apps/smtp/src/modules/smtp/ui/template-preview.tsx 0.00% 23 Missing and 1 partial ⚠️
apps/smtp/src/modules/trpc/trpc-server.ts 11.11% 8 Missing ⚠️
...es/smtp/configuration/smtp-configuration.router.ts 87.80% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2065      +/-   ##
==========================================
+ Coverage   34.64%   35.10%   +0.46%     
==========================================
  Files         924      926       +2     
  Lines       59383    59659     +276     
  Branches     2818     2852      +34     
==========================================
+ Hits        20574    20946     +372     
+ Misses      38440    38346      -94     
+ Partials      369      367       -2     
Flag Coverage Δ
avatax 56.75% <ø> (ø)
cms 15.83% <ø> (ø)
domain 100.00% <ø> (ø)
dynamo-config-repository 79.29% <ø> (ø)
errors 91.66% <ø> (ø)
logger 28.81% <ø> (ø)
np-atobarai 71.75% <ø> (ø)
products-feed 3.87% <ø> (ø)
search 21.65% <ø> (ø)
segment 30.61% <ø> (ø)
shared 35.20% <ø> (ø)
smtp 34.29% <37.39%> (+5.38%) ⬆️
stripe 70.43% <ø> (ø)
webhook-utils 11.02% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@witoszekdev
Copy link
Member Author

For some reason we are detecting changes in other apps (not touched in this PR)

M	apps/np-atobarai/generated/app-webhooks-types/transaction-initialize-session.ts
M	apps/np-atobarai/generated/app-webhooks-types/transaction-process-session.ts
M	apps/np-atobarai/generated/app-webhooks-types/transaction-refund-requested.ts
M	apps/stripe/generated/app-webhooks-types/transaction-cancelation-requested.ts
M	apps/stripe/generated/app-webhooks-types/transaction-charge-requested.ts
M	apps/stripe/generated/app-webhooks-types/transaction-initialize-session.ts
M	apps/stripe/generated/app-webhooks-types/transaction-process-session.ts
M	apps/stripe/generated/app-webhooks-types/transaction-refund-requested.ts

I didn't have time to properly test this yet as well

@witoszekdev witoszekdev marked this pull request as ready for review August 19, 2025 14:44
krzysztofzuraw
krzysztofzuraw previously approved these changes Aug 25, 2025
@Copilot Copilot AI review requested due to automatic review settings October 2, 2025 17:09
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes errors when configuring SMTP app by replacing direct Handlebars usage with a custom HandlebarsTemplateCompiler that includes all handlers. The fix prevents template compilation failures when using advanced Handlebars helpers.

Key changes:

  • Replace direct Handlebars usage in tRPC routes with EmailCompiler validation
  • Add comprehensive error handling and user-friendly error display components
  • Implement template validation before saving configurations

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
apps/smtp/src/modules/trpc/trpc-server.ts Add error context extraction for better error formatting in tRPC responses
apps/smtp/src/modules/smtp/ui/template-preview.tsx New component for displaying rendered email templates with loading states
apps/smtp/src/modules/smtp/ui/template-error-display.tsx New component for displaying template compilation errors with helpful hints
apps/smtp/src/modules/smtp/ui/event-form.tsx Update form to use new error display and preview components with improved UX
apps/smtp/src/modules/smtp/services/mjml-compiler.ts Improve error message handling to pass through original error messages
apps/smtp/src/modules/smtp/services/handlebars-template-compiler.ts Improve error message handling to pass through original error messages
apps/smtp/src/modules/smtp/services/email-compiler.ts Add error context types, validation method, and enhanced error handling
apps/smtp/src/modules/smtp/configuration/smtp-configuration.service.ts Add template validation before saving configurations
apps/smtp/src/modules/smtp/configuration/smtp-configuration.router.ts Replace direct Handlebars usage with EmailCompiler for proper handler support
.changeset/giant-dolls-jump.md Add changeset documenting the fix

lkostrowski
lkostrowski previously approved these changes Oct 3, 2025
krzysztofzuraw
krzysztofzuraw previously approved these changes Oct 3, 2025
@witoszekdev witoszekdev dismissed stale reviews from krzysztofzuraw and lkostrowski via 47e86eb October 3, 2025 10:16
@Copilot Copilot AI review requested due to automatic review settings October 3, 2025 10:23
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

apps/smtp/src/modules/smtp/services/email-compiler.ts:1

  • Using an empty object for validation may not catch all potential template errors that depend on specific payload structure. Consider using a more representative mock payload or documenting this limitation.
import { err, ok, Result } from "neverthrow";

@Copilot Copilot AI review requested due to automatic review settings October 3, 2025 11:21
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

lkostrowski
lkostrowski previously approved these changes Oct 3, 2025
@Copilot Copilot AI review requested due to automatic review settings October 3, 2025 12:01
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

<mj-section>
<mj-column>
<mj-text >{{injectedValue}}</mj-text>
<mj-text >${{ injectedValue }}</mj-text>
Copy link
Preview

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalid template syntax. The ${{ injectedValue }} syntax is incorrect - it should be {{ injectedValue }} for Handlebars templating.

Suggested change
<mj-text >${{ injectedValue }}</mj-text>
<mj-text >{{ injectedValue }}</mj-text>

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants