chore: html emails infrastructure #1987
Conversation
cyyynthia
force-pushed
the
cynthia/react-email
branch
2 times, most recently
from
0d73c0f to
3e1496b
Compare
cyyynthia
force-pushed
the
cynthia/react-email
branch
from
3e1496b to
8fb4d33
Compare
cyyynthia
force-pushed
the
cynthia/react-email
branch
from
8fb4d33 to
6a361e0
Compare
cyyynthia
force-pushed
the
cynthia/react-email
branch
from
6a361e0 to
91c5357
Compare
WalkthroughThis change introduces a comprehensive framework for generating, localizing, and sending HTML emails using React Email and Thymeleaf. It adds new backend Kotlin services and configuration for email templating, global variables, and localization, as well as a complete frontend email package with React components, localization utilities, and build scripts. Gradle and CI workflows are updated to support email build, test, and deployment processes. Changes
Sequence Diagram(s)sequenceDiagram
participant ReactEmail as React Email (Dev/Build)
participant Gradle as Gradle Build
participant Backend as Kotlin Backend
participant Thymeleaf as Thymeleaf Engine
participant MailSender as JavaMailSender
participant User as Email Recipient
ReactEmail->Gradle: Build & export email templates (HTML, i18n)
Gradle->Backend: Copy templates, i18n, resources to backend
Backend->Thymeleaf: Process email template with variables & locale
Thymeleaf->Backend: Rendered HTML email content
Backend->MailSender: Compose and send email (with subject, HTML, attachments)
MailSender->User: Deliver localized HTML email
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
cyyynthia
force-pushed
the
cynthia/react-email
branch
from
1d31033 to
1777d2c
Compare
cyyynthia
force-pushed
the
cynthia/react-email
branch
from
1777d2c to
bb08e1c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (34)
backend/data/src/main/kotlin/io/tolgee/pubSub/RedisPubSubReceiver.kt (1)
17-21: Reuse ObjectMapper to avoid repeated instantiation.Calling
jacksonObjectMapper()on every incoming message can impact performance and GC pressure. Consider definingprivate val objectMapper = jacksonObjectMapper()at the class level or injecting it as a Spring bean, then use
objectMapper.readValue(...).backend/security/build.gradle (1)
26-26: Add JitPack repository
Enables resolving dependencies from JitPack. Consider centralizing common repository definitions in the root build script to avoid duplication across modules.backend/testing/build.gradle (1)
44-44: Add JitPack repository
Ensures JitPack artifacts can be fetched. You may want to define shared repositories in the root Gradle config to keep module scripts DRY.backend/app/build.gradle (1)
51-51: Add JitPack repository
Allows usage of JitPack-hosted dependencies. It’s worth extracting this into the top-levelbuild.gradleto prevent repetition.backend/api/build.gradle (1)
30-30: Add JitPack repository
Required for modules that pull in JitPack packages. Consider moving repository declarations to the root project to streamline maintenance.DEVELOPMENT.md (1)
70-72: Verify link to HACKING.md and provide contextEnsure that
email/HACKING.mdexists at the specified path and the link renders correctly. Consider adding a brief summary of what readers will find in that file for immediate context.Run this script to confirm the file’s presence:
#!/bin/bash # Verify that the email HACKING guide is in place if [ ! -f email/HACKING.md ]; then echo "ERROR: Missing file email/HACKING.md" exit 1 else echo "OK: Found email/HACKING.md" fiemail/tsconfig.json (1)
1-6: Approve: Basic TypeScript configuration for email
Thetsconfig.jsoncorrectly preserves JSX and includes all relevant source paths.Consider enabling stricter compiler options (e.g.,
"strict": true,"target", or"moduleResolution") to improve type safety and catch errors early.ee/backend/app/build.gradle (1)
36-36: Duplicate repository declaration; consider centralizing. The JitPack repo is added across multiple backend modules. To avoid repetition and ensure consistency, centralize repository definitions in the rootsettings.gradleor a shared Gradle script.backend/development/build.gradle (1)
38-38: Duplicate repository declaration across modules; consider centralization. As with other modules, JitPack is repeatedly added. You could configure it once in the rootsettings.gradleor a common Gradle script to DRY up the build configuration.email/components/atoms/TolgeeLink.ts (2)
20-25: Consider using a class-merging utility
The manual string concatenation works, but adopting a library likeclsxorclassnamescan make conditional class compositions more robust and readable:-import { Link, LinkProps } from '@react-email/components'; +import { Link, LinkProps } from '@react-email/components'; +import clsx from 'clsx'; … -export default function TolgeeLink(props: LinkProps) { - const className = props.className - ? `${LINK_CLASSES} ${props.className}` - : LINK_CLASSES; +export default function TolgeeLink({ className, ...rest }: LinkProps) { + const merged = clsx(LINK_CLASSES, className);
27-27: Use JSX syntax for clarity
Instead ofReact.createElement, consider:return <Link {...props} className={className} />;This is more idiomatic in TSX and improves readability.
email/components/atoms/TolgeeButton.ts (2)
20-21: Extract shared brand classes if reused
If other components share parts of theBUTTON_CLASSESstring (e.g.,bg-brand text-white), consider extracting those to a shared constant for consistency.
23-28: Leverage destructuring and JSX
You can simplify the component and improve type inference by destructuring props and using JSX:-export default function TolgeeButton(props: ButtonProps) { - const className = props.className - ? `${BUTTON_CLASSES} ${props.className}` - : BUTTON_CLASSES; - - return React.createElement(Button, { ...props, className: className }); -} +export default function TolgeeButton({ className, ...rest }: ButtonProps) { + const merged = `${BUTTON_CLASSES}${className ? ` ${className}` : ''}`; + return <Button {...rest} className={merged} />; +}email/tailwind.config.ts (1)
14-18: Consider using numeric values for unitless line heights.
Switching from string'1'to numeric1in the larger font-size entries improves type clarity.backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateUtils.kt (1)
19-20: Utility class should be a Kotlinobject.
Convertingclass EmailTemplateUtilsto anobjectprevents unnecessary instantiation and aligns with Kotlin utility conventions.backend/data/src/test/kotlin/io/tolgee/email/EmailServiceTest.kt (2)
61-67: StubbingcreateMimeMessage()can be simplifiedThe current
let { … }indirection is a bit hard to follow and doesn’t handle multiple invocations.val msg = JavaMailSenderImpl().createMimeMessage() whenever(mailSender.createMimeMessage()).thenReturn(msg)This is clearer and resilient to additional stubbing.
140-146: Deep multipart traversal makes assertions brittleHard-coding three nested
getBodyPart(0)levels will break as soon as the message structure changes (e.g., text + HTML alternative).
Consider usingMimeMessageUtils.getBodyPartByMimeType(...)or walking the tree recursively to find the firsttext/htmlpart.email/components/layouts/LayoutCore.tsx (2)
31-38: Suppress emptyth:textattribute to avoid noise in the outputWhen
subjectis a plain string,thTextisnulland React will still renderth:text="", leaving an empty attribute in every compiled template.-<title {...{ 'th:text': thText }}>{convert(subjectPlain)}</title> +<title {...(thText ? { 'th:text': thText } : {})}> + {convert(subjectPlain)} +</title>
37-38:html-to-textdefaults strip line-breaks; preserve them for email subjectsIf the translated subject ever contains
<br>or paragraphs, the default converter will squash them. Pass explicit options:-{convert(subjectPlain)} +{convert(subjectPlain, { wordwrap: false, selectors: [{ selector: 'br', format: 'block' }] })}email/emails/zz-test-email.tsx (2)
56-60: Ambiguous then/else semantics in<If>blockRelying on child order (
<span>yes</span><span>no</span>) makes templates hard to read and error-prone.
If the component supports explicit props (then/else) consider switching; otherwise add a comment so future editors keep the order intact.
63-79: Nested<ul>elements render unreliably in many email clientsOutlook and Gmail often strip list styling or the entire nested list. Tables or padded
<p>tags are safer.-<ul className="text-sm"> +<table role="presentation" className="text-sm">Re-evaluate if bullet hierarchy is really needed in a demo template.
email/emails/registration-confirm.tsx (1)
80-82: Plain\nline-breaks will be collapsed – use<br/>for predictable rendering-defaultValue={'Happy translating,\nTolgee'} +defaultValue={'Happy translating,<br/>Tolgee'}Most webmail UIs strip single LF characters in
text/htmlparts.email/components/ImgResource.ts (2)
37-40: Avoiddelete– use rest destructuring for better perf & type-safety
delete newProps.resourceName/delete newProps.srctriggers hidden-class de-opts in V8 and is flagged by Biome.-const newProps = { ...props } as Props; -delete newProps.resourceName; -delete newProps.src; +const { resourceName, src: _ignored, ...newProps } = props;
41-49: Large binary read on every render in dev mode
readFileSync(file)runs each time the component function executes, which can slow down hot-reload.
Cache the base64 string in auseMemoor a module-level Map keyed byresourceName.email/.eslintrc.json (1)
6-11: Missing Prettier recommended configYou have the Prettier plugin enabled and the rule enforced, but the recommended config (
plugin:prettier/recommended) is not inextends. Adding it auto-disables rules Prettier already covers and prevents double reporting."extends": [ "eslint:recommended", "plugin:react/recommended", - "plugin:@typescript-eslint/recommended" + "plugin:@typescript-eslint/recommended", + "plugin:prettier/recommended" ],email/.config/extractor.ts (2)
45-56: Rename helper to avoid shadowing globaltoString.Declaring a local
function toStringshadows the globalObject.prototype.toStringwhich can be confusing and may trigger tooling lint-errors (Biome already flags this).
Rename the helper to something more specific, e.g.literalToString, to eliminate the collision.-function toString(node: Expression | JSXAttrValue): string | null { +function literalToString(node: Expression | JSXAttrValue): string | null { @@ - case 'JSXExpressionContainer': - return toString(node.expression); + case 'JSXExpressionContainer': + return literalToString(node.expression);
66-77: Minor: incorrect node type reported in default-value warning.The warning message for an un-extracted default value prints
keyNameExpr.typeinstead ofdefaultValueExpr.type, which can mislead debugging.- warning: `Failed to extract the default value. It may be dynamic or not yet recognized during parse. AST node was a \`${keyNameExpr.type}\``, + warning: `Failed to extract the default value. It may be dynamic or not yet recognized during parse. AST node was a \`${defaultValueExpr.type}\``,backend/data/src/test/kotlin/io/tolgee/email/EmailGlobalVariablesProviderTest.kt (1)
33-36:@Componenton the test class is unnecessary and pollutes the context.Marking the test class as a Spring bean serves no purpose and can slow test startup or introduce circular-dependency noise. Remove the annotation.
-@Component @ExtendWith(SpringExtension::class) @Import(EmailGlobalVariablesProvider::class) class EmailGlobalVariablesProviderTest {backend/data/src/main/kotlin/io/tolgee/email/EmailGlobalVariablesProvider.kt (1)
41-48: Simplify null-safe qualifier extraction withrunCatching.The current try/catch could be expressed more idiomatically:
private fun String?.intoQualifier(): String = this?.let { runCatching { URI(it).host }.getOrNull() } ?: SELF_HOSTED_DEFAULT_QUALIFIERNot required, but it improves readability.
backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateConfig.kt (1)
57-64: Enable template caching for production.
SpringTemplateEnginedefaultscacheabletotrue, butClassLoaderTemplateResolverdoes not set a TTL. To avoid class-path rescans in production you can set:templateResolver.isCacheable = true templateResolver.cacheTTLMs = 10 * 60 * 1000L // 10 min, tweak as neededNo change needed for tests, but worth considering.
gradle/email.gradle (1)
57-66: Consider adding inputs/outputs for incremental build support.While the conditional logic is good, this task lacks inputs/outputs declarations. Consider adding them to improve build caching and incremental build support.
For example, you could declare the translation files as outputs:
tasks.register('updateEmailTranslations', Exec) { onlyIf { System.getenv("SKIP_EMAIL_BUILD") != "true" && System.getenv("TOLGEE_API_URL") != null && System.getenv("TOLGEE_API_URL") != "" } workingDir = emailPath commandLine npmCommandName, "run", "tg:pull" + outputs.dir("${emailPath}/translations") + outputs.upToDateWhen { false } // Always run when conditions are met }email/HACKING.md (1)
174-174: Format the bare URL as a markdown link.-- Get SVG from https://simpleicons.org/ +- Get SVG from [Simple Icons](https://simpleicons.org/)email/components/layouts/ClassicLayout.tsx (2)
48-61: Decorative icon is still announced to screen-readersThe
ImgResourceinSocialLinkis markedaria-hidden={true}and has a meaningfulalttext.
Those two attributes contradict each other – an element that is hidden from assistive technology should not expose alternative text. Use an empty alt text when the image is decorative, or droparia-hiddenif the icon really conveys information.- <ImgResource - className="mx-auto" - resourceName={resourceName} - alt={social} - aria-hidden={true} - /> + <ImgResource + className="mx-auto" + resourceName={resourceName} + alt="" + aria-hidden + />
95-106: Unnecessary empty row whenisCloudis false
<Row>and<Column>are rendered unconditionally, but their content is wrapped in<If condition="${isCloud}">.
In a self-hosted deployment (isCloud= false) the email will still contain an empty table row, adding useless markup and vertical spacing. Wrap the whole row in the conditional to keep the output minimal.- <Row> - <Column> - <If condition="${isCloud}"> + <If condition="${isCloud}"> + <Row> + <Column> @@ - </If> - </Column> - </Row> + </Column> + </Row> + </If>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
email/package-lock.jsonis excluded by!**/package-lock.jsonemail/resources/facebook.pngis excluded by!**/*.pngemail/resources/github.pngis excluded by!**/*.pngemail/resources/linkedin.pngis excluded by!**/*.pngemail/resources/slack.pngis excluded by!**/*.pngemail/resources/tolgee_logo_text.pngis excluded by!**/*.pngemail/resources/twitter-x.pngis excluded by!**/*.pngemail/resources/twitter.pngis excluded by!**/*.png
📒 Files selected for processing (60)
.github/actions/download-backend-build/action.yaml(2 hunks).github/actions/upload-backend-build/action.yaml(3 hunks).github/workflows/reportIntermittentTests.yml(2 hunks).github/workflows/test.yml(2 hunks).gitmodules(0 hunks)DEVELOPMENT.md(1 hunks)backend/api/build.gradle(1 hunks)backend/app/build.gradle(1 hunks)backend/app/src/main/kotlin/io/tolgee/Application.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/activity/ActivityLogTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/PatControllerTest.kt(2 hunks)backend/data/build.gradle(4 hunks)backend/data/src/main/kotlin/io/tolgee/activity/EntityDescriptionProvider.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/activity/iterceptor/InterceptedEventsManager.kt(2 hunks)backend/data/src/main/kotlin/io/tolgee/configuration/HibernateConfig.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailGlobalVariablesProvider.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailMessageSource.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailService.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateConfig.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateUtils.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/pubSub/RedisPubSubReceiver.kt(1 hunks)backend/data/src/main/resources/email-i18n(1 hunks)backend/data/src/main/resources/email-templates(1 hunks)backend/data/src/test/kotlin/io/tolgee/email/EmailGlobalVariablesProviderTest.kt(1 hunks)backend/data/src/test/kotlin/io/tolgee/email/EmailServiceTest.kt(1 hunks)backend/data/src/test/kotlin/io/tolgee/service/machineTranslation/MtBatchTranslatorTest.kt(3 hunks)backend/data/src/test/resources/email-i18n-test/messages_en.properties(1 hunks)backend/development/build.gradle(1 hunks)backend/security/build.gradle(1 hunks)backend/testing/build.gradle(1 hunks)build.gradle(2 hunks)ee/backend/app/build.gradle(1 hunks)ee/backend/tests/build.gradle(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/selfHostedLimitsAndReporting/KeyCountLimitTest.kt(1 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/slack/SlackWithBatchOperationTest.kt(1 hunks)email/.config/extractor.ts(1 hunks)email/.config/tolgeerc.json(1 hunks)email/.eslintrc.json(1 hunks)email/.gitignore(1 hunks)email/.prettierrc.json(1 hunks)email/HACKING.md(1 hunks)email/components/For.ts(1 hunks)email/components/If.ts(1 hunks)email/components/ImgResource.ts(1 hunks)email/components/LocalizedText.ts(1 hunks)email/components/Var.ts(1 hunks)email/components/atoms/TolgeeButton.ts(1 hunks)email/components/atoms/TolgeeLink.ts(1 hunks)email/components/layouts/ClassicLayout.tsx(1 hunks)email/components/layouts/LayoutCore.tsx(1 hunks)email/components/translate.ts(1 hunks)email/emails/registration-confirm.tsx(1 hunks)email/emails/zz-test-email.tsx(1 hunks)email/env.d.ts(1 hunks)email/i18n/messages_en.properties(1 hunks)email/package.json(1 hunks)email/tailwind.config.ts(1 hunks)email/tsconfig.json(1 hunks)gradle/email.gradle(1 hunks)webapp/.tolgeerc.json(1 hunks)
💤 Files with no reviewable changes (1)
- .gitmodules
🧰 Additional context used
🧬 Code Graph Analysis (1)
email/components/layouts/LayoutCore.tsx (1)
email/components/translate.ts (1)
TranslatedText(24-28)
🪛 Biome (1.9.4)
email/.config/extractor.ts
[error] 45-45: Do not shadow the global "toString" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
email/components/ImgResource.ts
[error] 38-38: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 LanguageTool
email/HACKING.md
[uncategorized] ~13-~13: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...t.email/docs/introduction). If you need real world examples, they provide a bunch of great...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[typographical] ~41-~41: Except for inverted sentences, ‘Can I’ requires a question mark at the end of the sentence.
Context: ...n I Use](https://caniuse.com/) of emails. This also applies to the layout; alway...
(MD_PRP_QUESTION_MARK)
[uncategorized] ~53-~53: Possible missing comma found.
Context: ...a layout, or at least must use the core layout as it contains important building block...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~61-~61: Possible missing comma found.
Context: ...ed to receive output from the t.raw() function documented below. The core layout only ...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~65-~65: The plural noun “properties” cannot be used with the article “a”. Did you mean “a dynamic property” or “dynamic properties”?
Context: ... of a better section: whenever you need a dynamic properties (e.g. href that takes the value of a va...
(A_NNS)
[style] ~139-~139: This phrase is redundant (‘G’ stands for ‘graphic’). Use simply “SVGs”.
Context: ...ile you want to insert. Be careful, [SVG images are poorly supported](https://www.can...
(ACRONYM_TAUTOLOGY)
[style] ~142-~142: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...PG, and GIF should be good. It is also very important that files are never deleted, and p...
(EN_WEAK_ADJECTIVE)
[uncategorized] ~169-~169: Possible missing comma found.
Context: ...ed as demo values, except for localized strings as a default value is provided then. Th...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
email/HACKING.md
174-174: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (55)
ee/backend/tests/src/test/kotlin/io/tolgee/ee/selfHostedLimitsAndReporting/KeyCountLimitTest.kt (1)
34-40: Formatting-only change ininitMocks
The indentation and spacing were adjusted without any alteration to the mock setup logic.backend/data/src/main/kotlin/io/tolgee/pubSub/RedisPubSubReceiver.kt (1)
18-21: Indentation alignment inreceiveWebsocketMessagelooks good.The
data.message?.letblock is now correctly indented, improving readability without altering behavior.backend/data/src/main/kotlin/io/tolgee/activity/iterceptor/InterceptedEventsManager.kt (2)
239-239: Consistent indentation applied.The
ifcheck inside themapNotNulllambda has been re-indented to match the project's space-based style. No logic was altered.
249-249: Consistent indentation applied.The filter condition in
getEntityIgnoredMembershas been re-indented for alignment with surrounding code. This is purely stylistic.webapp/.tolgeerc.json (1)
2-2: Schema URL updated to official docs
The$schemareference now points to the documentation URL, aligning with Tolgee’s current CLI schema location.ee/backend/tests/build.gradle (1)
34-34: Added JitPack repository
Consistent addition ofhttps://jitpack.ioensures dependencies from JitPack resolve correctly. No issues found.backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/PatControllerTest.kt (2)
105-105: Formatting: Indentation normalized
Adjusted indentation for the null expiration assertion to align with surrounding code style.
132-132: Formatting: Assertion indentation fixed
Normalized indentation for the post-update description assertion to improve readability.backend/data/src/main/kotlin/io/tolgee/activity/EntityDescriptionProvider.kt (1)
57-57: Formatting: Lambda indentation fixed
Aligned thefilterlambda indentation for consistency with the project's Kotlin style guidelines.backend/data/src/test/resources/email-i18n-test/messages_en.properties (1)
1-2: Add ICU test keys
Introduced ICU-formatted test entries for email i18n; placeholders and naming follow established conventions.backend/data/src/test/kotlin/io/tolgee/service/machineTranslation/MtBatchTranslatorTest.kt (3)
158-158: Formatting: Mock setup indentation
Aligned themockQueryResultinvocation indentation within the application context setup for clarity.
205-208: Prevent NPE by mocking providers
Added mocks forGoogleTranslationProviderandLlmTranslationProviderto the application context to avoid bean lookup NPEs during tests.
248-252: Formatting: Extension function indentation
Corrected indentation of themockIntoAppContextextension function to match the surrounding code style.backend/app/src/test/kotlin/io/tolgee/activity/ActivityLogTest.kt (1)
240-244: Formatting only: Lambda indentation improved
Refactoring the lambda passed tosatisfiesfor better readability without altering behavior.backend/data/src/main/kotlin/io/tolgee/configuration/HibernateConfig.kt (2)
25-27: Correct use of Spring annotations
Annotations@Componentand@Profile("hibernate-trace")correctly scope the bean to thehibernate-traceprofile.
28-30: Enable SQL trace logging viaQueryStackTraceLogger
Thecustomizeimplementation properly sets theSTATEMENT_INSPECTORproperty for detailed SQL stack trace logging..github/workflows/test.yml (2)
73-75: Approve: Skip email build in backend tests
AddingSKIP_EMAIL_BUILD: trueto the backend-test job correctly bypasses the email build step during CI test runs, aligning with the new separate email build process.
153-156: Approve: Skip email build in E2E tests
IntroducingSKIP_EMAIL_BUILD: truein the e2e job prevents unnecessary email template builds during end-to-end testing, optimizing CI performance..github/workflows/reportIntermittentTests.yml (2)
60-63: Approve: Skip email build in intermittent backend tests
SettingSKIP_EMAIL_BUILD: truein the backend-test job for intermittent E2E reporting ensures the email build step is consistently skipped.
137-140: Approve: Skip email build in intermittent E2E tests
AddingSKIP_EMAIL_BUILD: truein the e2e job of this workflow correctly omits email builds for scheduled test runs.email/.gitignore (1)
1-3: Approve: Ignore email build artifacts and dependencies
The entries for.react-email,node_modules, anddistappropriately exclude generated assets and dependencies from version control in the email directory.email/.prettierrc.json (1)
1-5: Approve: Prettier formatting rules for email
The Prettier config enforces trailing commas, two-space indentation, and single quotes, ensuring consistent code style across the email frontend.backend/app/src/main/kotlin/io/tolgee/Application.kt (2)
8-8:
15-15: Verify Thymeleaf exclusion impact. ExcludingThymeleafAutoConfigurationdisables auto-configuration for all Thymeleaf templates. Confirm that this won’t break any existing web UI views or controllers relying on Thymeleaf rendering..github/actions/download-backend-build/action.yaml (2)
14-14: Approve switching to bash shell. Bash is required for the--zstdtar option and is available on GitHub Actions runners.
26-26: Add email artifact extraction. This new line unpacks the email build into./email/dist, matching the upload configuration. Ensure the artifactbackend-email.tar.zstis correctly published by the upload action and the path is accurate.ee/backend/tests/src/test/kotlin/io/tolgee/ee/slack/SlackWithBatchOperationTest.kt (1)
96-96: Whitespace-only change; no logic impact. The indentation adjustment aligns this assertion with surrounding code..github/actions/upload-backend-build/action.yaml (1)
8-8: Switching shell to bash is appropriate
Using bash enables advanced flags like--zstd. Composite actions support bash natively on GitHub-hosted runners.build.gradle (2)
64-64: Applied email.gradle after evaluation
Loading theemail.gradlescript here ensures email build tasks are registered on the server-app project.
74-74: Verify copyEmailResources task dependency chain
Ensure thatcopyEmailResourcesinemail.gradledepends onbuildEmailsandupdateEmailTranslationsso the latest compiled email assets and translations are always copied before packaging.email/env.d.ts (1)
19-23: Verify TypeScript module augmentation for Thymeleaf attributes
Confirm that this augmentation actually allowsth:…attributes in your JSX without type errors (tsconfig includes this file, and React’s JSX namespace is correctly extended).email/components/atoms/TolgeeLink.ts (1)
17-18: Confirm@react-email/componentsis declared in dependencies
Make sure thepackage.jsonincludes@react-email/componentsso this import resolves correctly.email/components/atoms/TolgeeButton.ts (1)
17-18: Confirm@react-email/componentsis declared in dependencies
Ensurepackage.jsonhas@react-email/componentsso theButtonimport resolves without errors.email/.config/tolgeerc.json (1)
1-14: Configuration file validated.
JSON structure and extraction patterns correctly align with the Tolgee CLI requirements for the email package.email/tailwind.config.ts (1)
1-63: Valid Tailwind configuration.
Font sizes, spacing scale, and custom brand color are well-defined and satisfy theTailwindConfiginterface.email/components/LocalizedText.ts (1)
17-27: LocalizedText component implemented correctly.
The props interface and_tinvocation correctly wire up default values and optional parameters for React Email-based templates.backend/data/build.gradle (3)
57-59: JitPack repository addition approved.
Required for thecom.github.transferwise:spring-icudependency and correctly placed underrepositories.
77-77: Email Gradle script applied correctly.
Includinggradle/email.gradleintegrates the email frontend build into the backend lifecycle as intended.
261-263: To be sure there’s no existing skip logic, let’s also look for any references to the expected env var or property checks ingradle/email.gradle:#!/bin/bash # Look for the CI flag and any getenv/property checks in the email build script grep -R "SKIP_EMAIL_BUILD" -n gradle/email.gradle grep -R "SKIP_EMAIL" -n gradle/email.gradle grep -R "System.getenv" -n gradle/email.gradle grep -R "hasProperty" -n gradle/email.gradleemail/package.json (1)
6-12: Script globs are inconsistentThe repo folder is
emails/(plural) while build tooling expectsemail/. Confirm that"prettier": "prettier --write ./emails ..."actually picks up templates; otherwise contributors will silently skip formatting.
email/i18n/messages_en.properties (1)
1-13: LGTM – keys & placeholders look consistentKeys follow a clear naming convention and placeholders align with ICU syntax. No issues spotted.
email/components/If.ts (1)
49-51: Undefined access when previewing with a single child.In dev-mode,
demoValue === falsereturnschildren[1], butchildren[1]isundefinedwhen only one child is supplied, producingReactwarnings.Add an explicit guard:
- if (demoValue === false) return children[1]; + if (demoValue === false) return children[1] ?? null;Consider documenting that two children are required when using
demoValue.gradle/email.gradle (3)
21-29: Task implementation looks good!The
copyEmailResourcestask is properly configured with inputs/outputs for incremental builds and correct task ordering.
31-40: Good use of npm ci for reproducible builds!The task properly declares inputs/outputs and respects the
SKIP_EMAIL_BUILDenvironment variable.
42-55: Well-structured build task with comprehensive inputs!The task correctly declares all necessary inputs for proper incremental build support and has appropriate task dependencies.
backend/data/src/main/kotlin/io/tolgee/email/EmailMessageSource.kt (3)
38-91: Clean delegation pattern implementation!The getMessage methods properly delegate to the wrapped provider and apply post-processing consistently.
93-105: Thread-safe counter with proper overflow handling!Good use of
@Synchronizedand overflow protection for long-running applications.
106-142: XML parsing implementation is suitable for the intended use case.The regex-based approach with stack tracking works well for the simple XML-like tags used in email templates. The implementation correctly handles nested tags and generates appropriate Thymeleaf fragments.
Note that the pattern only matches tags without attributes (e.g.,
<tag>but not<tag attr="value">), which appears to be by design based on the comment about being "specific to emails template structure."backend/data/src/main/kotlin/io/tolgee/email/EmailService.kt (2)
48-71: Clever two-pass template processing approach!The double template processing allows Thymeleaf expressions generated by the message source to be evaluated, which is necessary for the XML tag handling in
EmailMessageSource. Good documentation about theThymeleafEvaluationContextrequirement.
73-90: Standard email sending implementation with proper MIME handling!Good use of
MULTIPART_MODE_MIXED_RELATEDfor HTML emails with embedded resources.email/HACKING.md (1)
1-179: Excellent comprehensive documentation!This guide provides thorough coverage of the email templating system with clear examples and explanations. The sections on React Email, layouts, utility components, and localization are particularly well-structured.
email/components/translate.ts (3)
43-64: Elegant recursive implementation for line break handling!The function correctly handles strings, arrays, and React elements with proper recursion for nested children.
163-170: Document the double underscore convention for variable names.The code replaces
__with.in variable names (line 165), which could cause issues if variables legitimately contain double underscores.This convention should be clearly documented. Is this behavior mentioned in the documentation? Consider adding a comment explaining this transformation:
const stringArgsMap = stringArgs.map( - (a) => `${a}: #strings.escapeXml(${a.replace(/__/g, '.')})` + // Convert __ to . to access nested properties (e.g., item__name -> item.name) + (a) => `${a}: #strings.escapeXml(${a.replace(/__/g, '.')})` );
182-226: Well-designed translation API with proper error handling!The exported functions provide a clean API with appropriate error handling. The
t.rawfunction correctly prevents component usage where plain text is required.email/components/layouts/ClassicLayout.tsx (1)
82-85: Double rendering guard forsubject
subjectis supplied to both<LayoutCore>(line 70) and rendered again inside the header (line 83).
Ifsubjectcan contain HTML produced by ICU formatting, you risk duplicating markup and increasing the email size. Confirm thatLayoutCoreneeds the raw value only for the<title>tag and cannot reuse the already-rendered header, or consider memoising the rendered value to avoid evaluatingt.render()twice.
backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateUtils.kt
Outdated
Show resolved
Hide resolved
backend/data/src/test/kotlin/io/tolgee/email/EmailGlobalVariablesProviderTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
email/components/ImgResource.ts (1)
26-29: Refactor directory traversal for robustness
Inlining the assignment in the loop condition halts after one iteration and doesn’t reliably detect the real filesystem root (especially on Windows). Extract a helper to detect the root and perform thejoininside the loop body:- // noinspection StatementWithEmptyBodyJS - while ( - !readdirSync(root).includes('resources') && - root !== (root = join(root, '..')) - ); + const isFsRoot = (p: string) => join(p, '..') === p; + while (!readdirSync(root).includes('resources') && !isFsRoot(root)) { + root = join(root, '..'); + }
🧹 Nitpick comments (2)
gradle/email.gradle (2)
21-25: Clarify destination path and trailing/.
The doublestatic/staticis intentional for Spring’s resource mapping, but the trailing/.is unnecessary. Consider removing it or documenting:- into "${project.projectDir}/build/generated/resources/static/static/emails/." + into "${project.projectDir}/build/generated/resources/static/static/emails"
67-76: MakeupdateEmailTranslationsincremental
Sincetg:pullwrites files intoi18n, declare inputs/outputs for up-to-date checks:inputs.dir("${emailPath}/i18n") outputs.dir("${emailPath}/i18n")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/data/build.gradle(5 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailService.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateConfig.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateUtils.kt(1 hunks)backend/data/src/test/kotlin/io/tolgee/email/EmailGlobalVariablesProviderTest.kt(1 hunks)backend/data/src/test/kotlin/io/tolgee/email/EmailServiceTest.kt(1 hunks)build.gradle(1 hunks)email/components/ImgResource.ts(1 hunks)email/components/Var.ts(1 hunks)gradle/email.gradle(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- build.gradle
- backend/data/build.gradle
- backend/data/src/test/kotlin/io/tolgee/email/EmailServiceTest.kt
- backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateUtils.kt
- email/components/Var.ts
- backend/data/src/test/kotlin/io/tolgee/email/EmailGlobalVariablesProviderTest.kt
- backend/data/src/main/kotlin/io/tolgee/email/EmailService.kt
- backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateConfig.kt
🧰 Additional context used
🪛 Biome (1.9.4)
email/components/ImgResource.ts
[error] 42-42: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 43-43: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Frontend static check 🪲
- GitHub Check: Build backend 🏗️
- GitHub Check: Build frontend 🏗️
🔇 Additional comments (3)
email/components/ImgResource.ts (2)
45-49: Verify Thymeleaf expression syntax
You’re settingdata-th-srcto\${instanceUrl} + '/static/emails/${props.resourceName}'. Please confirm this concatenation is correctly evaluated by Thymeleaf at runtime. You might simplify to:newProps['data-th-src'] = `\${instanceUrl}/static/emails/${props.resourceName}`;if your setup supports it.
38-56: Production/dev logic and integration look solid
The split between inline base64 images in development anddata-th-srcfor production templates aligns well with React Email and Thymeleaf usage.gradle/email.gradle (1)
1-17: Gradle email build integration is comprehensive
The registration ofinstallEmailDeps,buildEmails, resource-copying, and translation update tasks—withonlyIfguards and declared inputs/outputs—fits well into the existing build lifecycle.
only do a single pass render (much safer wrt SSTI) properly deal with Fragments while making the final HTML isolate test email to test resources
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
email/components/ImgResource.ts (2)
26-29: Infinite loop on Windows root still unresolvedThis was flagged in a previous review. When
rootis the FS root (C:\),join(root,'..')resolves to the same path, thewhilecondition never changes, leading to 100 % CPU.Please integrate an explicit “is FS root?” guard:
-while ( - !readdirSync(root).includes('resources') && - root !== (root = join(root, '..')) -); +const isFsRoot = (p: string) => join(p, '..') === p; +while ( + !readdirSync(root).includes('resources') && + !isFsRoot(root) +) { + root = join(root, '..'); +}
42-44: Still using thedeleteoperator → performance hitInstead of cloning and deleting, destructure unwanted keys:
- const { resourceName: _a, ...newProps } = props as Props; - delete newProps.src; + const { resourceName: _a, src: _unusedSrc, ...newProps } = props;
🧹 Nitpick comments (5)
backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt (2)
33-35: TemplateEngine bean lookup happens for every JVM instance, but not every request ⇒ fine, yetlazystill incurs synchronized overhead
by lazyuses a JVM-level monitor. Under high email volume this resolver will be instantiated only once but can be hit by many concurrent threads; the very first call will synchronize.
Nothing is wrong functionally, but if you want zero-locking fast-path you could switch tolazy(LazyThreadSafetyMode.PUBLICATION)(double-checked locking) or inject the engine directly instead of run-time lookup.
54-95: Algorithmically correct but readability could improveThe manual
deltajuggling andreplaceRangemath make the loop tricky to audit.
Consider using aStringBuilderorMatcher.appendReplacement/appendTailto avoid manual index shifting; that removes thedeltabookkeeping and reduces off-by-one risk.gradle/email.gradle (1)
49-58:installEmailDepstask not wired into normal Gradle cache – consider declaringpackage-lock.jsonasinputs.fileonly onceCurrent declaration repeats
package-lock.jsonin bothinstallEmailDepsand the build tasks, which may trigger unnecessary re-runs. Move the installation outputs (node_modules) into aconfigurationCachefriendlyExecwrapper or a GradlenpmInstallplugin to shorten CI times.email/HACKING.md (2)
39-42: Minor typo & grammar – “real-world”, “always”Not blocking, but polishing contributor docs improves credibility. Search for:
- “real world examples” → “real-world examples”
- “always prefer” → “always prefer”
65-67: Article misuse“whenever you need a dynamic properties”
Should be “a dynamic property” or “dynamic properties”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
email/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
.github/actions/download-backend-build/action.yaml(2 hunks).github/actions/upload-backend-build/action.yaml(3 hunks).github/workflows/test.yml(3 hunks)backend/data/build.gradle(5 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailService.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateConfig.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateEngine.kt(1 hunks)backend/data/src/test/kotlin/io/tolgee/email/EmailServiceTest.kt(1 hunks)email/.gitignore(1 hunks)email/HACKING.md(1 hunks)email/components/For.ts(1 hunks)email/components/If.ts(1 hunks)email/components/ImgResource.ts(1 hunks)email/components/LocalizedText.ts(1 hunks)email/components/Var.ts(1 hunks)email/components/layouts/ClassicLayout.tsx(1 hunks)email/components/translate.ts(1 hunks)email/emails/__tests__/test-email.tsx(1 hunks)email/env.d.ts(1 hunks)email/package.json(1 hunks)email/tsconfig.json(1 hunks)gradle/email.gradle(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateEngine.kt
- backend/data/src/main/kotlin/io/tolgee/email/EmailService.kt
🚧 Files skipped from review as they are similar to previous changes (16)
- email/tsconfig.json
- email/.gitignore
- .github/actions/upload-backend-build/action.yaml
- .github/actions/download-backend-build/action.yaml
- backend/data/src/test/kotlin/io/tolgee/email/EmailServiceTest.kt
- email/components/LocalizedText.ts
- email/env.d.ts
- backend/data/build.gradle
- .github/workflows/test.yml
- email/components/layouts/ClassicLayout.tsx
- email/components/For.ts
- email/components/Var.ts
- backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateConfig.kt
- email/components/If.ts
- email/components/translate.ts
- email/package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt (1)
backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateEngine.kt (1)
provider(22-28)
🪛 LanguageTool
email/HACKING.md
[uncategorized] ~13-~13: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...t.email/docs/introduction). If you need real world examples, they provide a bunch of great...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~33-~33: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...worry > about building them yourself or copy files around. ### TailwindCSS For styl...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
[typographical] ~41-~41: Except for inverted sentences, ‘Can I’ requires a question mark at the end of the sentence.
Context: ...n I Use](https://caniuse.com/) of emails. This also applies to the layout; alway...
(MD_PRP_QUESTION_MARK)
[uncategorized] ~53-~53: Possible missing comma found.
Context: ...a layout, or at least must use the core layout as it contains important building block...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~61-~61: Possible missing comma found.
Context: ...ed to receive output from the t.raw() function documented below. The core layout only ...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~65-~65: The plural noun “properties” cannot be used with the article “a”. Did you mean “a dynamic property” or “dynamic properties”?
Context: ... of a better section: whenever you need a dynamic properties (e.g. href that takes the value of a va...
(A_NNS)
[style] ~139-~139: This phrase is redundant (‘G’ stands for ‘graphic’). Use simply “SVGs”.
Context: ...ile you want to insert. Be careful, [SVG images are poorly supported](https://www.can...
(ACRONYM_TAUTOLOGY)
[style] ~142-~142: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...PG, and GIF should be good. It is also very important that files are never deleted, and p...
(EN_WEAK_ADJECTIVE)
[uncategorized] ~158-~158: Possible missing comma found.
Context: ...e exactly one child; to render multiple nodes make sure to use a fragment. This comp...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~170-~170: Possible missing comma found.
Context: ...ed as demo values, except for localized strings as a default value is provided then. Th...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
email/HACKING.md
175-175: Bare URL used
null
(MD034, no-bare-urls)
🪛 Biome (1.9.4)
email/components/ImgResource.ts
[error] 43-43: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: E2E testing 🔎 (15, 11)
- GitHub Check: E2E testing 🔎 (15, 8)
- GitHub Check: E2E testing 🔎 (15, 12)
- GitHub Check: E2E testing 🔎 (15, 10)
- GitHub Check: E2E testing 🔎 (15, 2)
- GitHub Check: E2E testing 🔎 (15, 0)
- GitHub Check: E2E testing 🔎 (15, 6)
- GitHub Check: E2E testing 🔎 (15, 13)
- GitHub Check: E2E testing 🔎 (15, 14)
- GitHub Check: E2E testing 🔎 (15, 9)
- GitHub Check: E2E testing 🔎 (15, 7)
- GitHub Check: E2E testing 🔎 (15, 5)
- GitHub Check: E2E testing 🔎 (15, 1)
- GitHub Check: E2E testing 🔎 (15, 4)
- GitHub Check: BT 🔎 (server-app:runWithoutEeTests)
- GitHub Check: E2E testing 🔎 (15, 3)
- GitHub Check: BT 🔎 (ee-test:test)
- GitHub Check: BT 🔎 (server-app:runContextRecreatingTests)
- GitHub Check: BT 🔎 (server-app:runStandardTests)
- GitHub Check: BT 🔎 (data:test)
- GitHub Check: BT 🔎 (server-app:runWebsocketTests)
🔇 Additional comments (3)
gradle/email.gradle (1)
24-24: Double “static/static” path may be accidental
into "${project.projectDir}/build/generated/resources/main/static/static/emails"
If the duplication is not intentional, drop one “static”. Otherwise add a short comment explaining the requirement to prevent future clean-ups from “fixing” it.email/emails/__tests__/test-email.tsx (1)
67-83:demoIterationsreceives a number, docs say it expects an array
<For … demoIterations={3}>
If the component signature expectsunknown[], passing anumberwill break type safety and preview rendering. Replace with something likedemoIterations={[{}, {}, {}]}or adapt the prop type.email/components/ImgResource.ts (1)
45-53: String interpolation escapes${…}correctly, but mixes literal & evaluated templatesBack-ticked template literal double-interpolates:
`\${instanceUrl} + '/static/emails/${props.resourceName}'`•
\${instanceUrl}stays literal – OK
•${props.resourceName}is evaluated now.If someone ever passes an untrusted
resourceNamethis path could break out of/static. ConsiderencodeURIComponentor at minimumpath.posix.basename()before interpolation.
backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt (1)
33-35: Avoid brittle bean look-up by nameHard-coding the bean name (
"emailTemplateEngine") couples this resolver to the Spring context configuration and will blow up withNoSuchBeanDefinitionExceptionif the bean is renamed or the profile excludes it.Prefer one of:
- Inject the
TemplateEnginedirectly via constructor.- If laziness is required, request by type:
applicationContext.getBean(TemplateEngine::class.java).Either option removes the stringly-typed dependency and eases refactors.
email/HACKING.md (3)
40-42: Grammar nit: add question mark after “Can I”The sentence references “Can I Use”/“Can I Email” but omits the question mark that belongs to the phrase.
- anything that's cool in \[CURRENT_YEAR]. [Can I Email](https://www.caniemail.com/) is a good resource + anything that's cool in \[CURRENT_YEAR]. [Can I Email?](https://www.caniemail.com/) is a good resource
65-67: Misleading article “a” before plural noun“A dynamic properties” should be either “a dynamic property” or simply “dynamic properties”.
- whenever you need a dynamic properties (e.g. href that takes the value of a variable), + whenever you need dynamic properties (e.g. an href that takes the value of a variable),
142-144: Redundant phrase “SVG images”“SVG” already implies an image; consider trimming to “SVGs” for conciseness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
email/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.github/workflows/test.yml(4 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt(1 hunks)email/HACKING.md(1 hunks)email/components/translate.ts(1 hunks)email/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- email/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/test.yml
- email/components/translate.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt (1)
backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateEngine.kt (1)
provider(22-28)
🪛 LanguageTool
email/HACKING.md
[uncategorized] ~13-~13: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...t.email/docs/introduction). If you need real world examples, they provide a bunch of great...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~33-~33: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...worry > about building them yourself or copy files around. ### TailwindCSS For styl...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
[typographical] ~41-~41: Except for inverted sentences, ‘Can I’ requires a question mark at the end of the sentence.
Context: ...n I Use](https://caniuse.com/) of emails. This also applies to the layout; alway...
(MD_PRP_QUESTION_MARK)
[uncategorized] ~53-~53: Possible missing comma found.
Context: ...a layout, or at least must use the core layout as it contains important building block...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~61-~61: Possible missing comma found.
Context: ...ed to receive output from the t.raw() function documented below. The core layout only ...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~65-~65: The plural noun “properties” cannot be used with the article “a”. Did you mean “a dynamic property” or “dynamic properties”?
Context: ... of a better section: whenever you need a dynamic properties (e.g. href that takes the value of a va...
(A_NNS)
[style] ~142-~142: This phrase is redundant (‘G’ stands for ‘graphic’). Use simply “SVGs”.
Context: ...ile you want to insert. Be careful, [SVG images are poorly supported](https://www.can...
(ACRONYM_TAUTOLOGY)
[style] ~145-~145: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...PG, and GIF should be good. It is also very important that files are never deleted, and p...
(EN_WEAK_ADJECTIVE)
[uncategorized] ~161-~161: Possible missing comma found.
Context: ...e exactly one child; to render multiple nodes make sure to use a fragment. This comp...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~173-~173: Possible missing comma found.
Context: ...ed as demo values, except for localized strings as a default value is provided then. Th...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
email/HACKING.md
178-178: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build frontend 🏗️
- GitHub Check: Build backend 🏗️
- GitHub Check: Frontend static check 🪲
backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt
Outdated
Show resolved
Hide resolved
backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt
Outdated
Show resolved
Hide resolved
backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt (1)
60-62: Minor allocation hotspot – avoidjoinToStringin the hot loop
joinToString("--")runs on every opening tag, creating a new list iterator and string each time.
If many rich-text tags are present in one message this becomes avoidable churn.- val fragName = "intl-${stack.joinToString("--")}" + val fragName = buildString { + append("intl-") + stack.fastForEachIndexed { idx, part -> // inline extension or simple for loop + if (idx > 0) append("--") + append(part) + } + }Even a simple
StringBuilder/manual loop would cut allocations here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateConfig.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateEngine.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateEngine.kt
- backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateConfig.kt
🧰 Additional context used
🧠 Learnings (1)
backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt (1)
Learnt from: cyyynthia
PR: tolgee/tolgee-platform#1987
File: backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt:52-66
Timestamp: 2025-06-17T13:37:28.558Z
Learning: In EmailMessageResolver.postProcessMessage(), the fragment ID counter is locally scoped and creates ephemeral IDs that are used only within the immediate template processing context. The fragments are rendered immediately with templateEngine.process() and discarded, so the counter only needs to ensure local uniqueness within a single message processing invocation, not across different calls.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build frontend 🏗️
- GitHub Check: Frontend static check 🪲
- GitHub Check: Build backend 🏗️
🔇 Additional comments (2)
backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt (2)
55-59: Correct matcher reference – previous bug resolved 👍
stack.add(it.group(2))now correctly uses the lambda’sMatchResult(it) instead of the outerMatcher, fixing the wrong-tag issue reported in earlier reviews.
Looks good.
67-69: ```shell
#!/bin/bash
set -eecho "1) Occurrences of stack.removeLast with context:"
rg -C5 -n "stack.removeLast"echo -e "\n2) Declaration/initialization of 'stack' in EmailMessageResolver.kt:"
rg -C5 -n "stack =" -n backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt || trueecho -e "\n3) Any ArrayDeque usage in the file:"
rg -C5 -n "ArrayDeque" -n backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt || true</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
I assume Snyk is unhappy about my addition of JitPack repository. 😅 Requesting review from both Jan and Stepan, as there were some wishes expressed in #2710 😃 |
|
Hi ^^ |
|
Bleh, this explains that. I guess I'll just replace it with a custom impl, it seems there's barely any lib that does it, and it should be simple enough... Plus we already pull icu4j so it's just a matter of connecting the bits together... 😮💨 Thanks for the info 👍🏻 |
|
Yee, or we might be able to pin the |
might make builds stupid fast now, hehe
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
gradle.properties (2)
3-3: Consider a shared remote build cache for CI.
Enablingorg.gradle.caching=truelocally is great; pairing it with a remote cache (e.g., Gradle Enterprise or a custom cache server) can dramatically speed up builds across branches and contributors.
4-5: Review configuration-cache and on-demand settings.
The configuration-cache feature is commented out pending HHH-19283, andorg.gradle.configureondemand=trueis enabled. Verify that “configure on demand” is still supported in your Gradle version and plan to re-enable configuration cache once upstream fixes are available.settings.gradle (2)
82-103: Enforce strict repository mode and content filtering.
The configuration excludescom.github.transferwisefrom Maven Central and includes only that group from JitPack under strictFAIL_ON_PROJECT_REPOSmode. Ensure no other JitPack‐sourced dependencies are required beyond this group.Consider extracting the
jitpackDependencieslist into a shared Gradle property or version catalog constant to avoid duplication if more groups are added later.
78-78: Validate Vaadin JSON library coordinates.
ThevaadinJsonentry points to an old Android‐JSON artifact (0.0.20131108.vaadin1). Please confirm the artifact exists in Maven Central, review its license, and ensure compatibility with the rest of the JSON tooling in your project.To verify artifact availability, you can run:
#!/bin/bash # Check if vaadinJson POM is reachable on Maven Central curl -I https://repo1.maven.org/maven2/com/vaadin/external/google/android-json/0.0.20131108.vaadin1/android-json-0.0.20131108.vaadin1.pombackend/data/build.gradle (3)
173-176: Verify Mailjet JSON module exclusion.
Excludingorg.json:jsonavoids conflicts but verify no other JSON libs are pulled transitively. Also consider documenting the root cause in the comment for future maintainers.
177-177: Confirm Vaadin JSON dependency usage.
You’ve addedlibs.vaadinJsonhere—ensure code paths actually leverage this library. If it’s only for transitive need, consider consolidating to avoid extra dependencies.
259-265: Wire email tasks into resource processing.
MakingprocessResourcesandprocessTestResourcesdepend on the new email copy tasks correctly integrates the generated templates into the build. You might also group these under a singlebuildEmailstask for clarity, but functionally this is sound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/api/build.gradle(0 hunks)backend/app/build.gradle(0 hunks)backend/data/build.gradle(5 hunks)backend/development/build.gradle(0 hunks)backend/ktlint/build.gradle(1 hunks)backend/misc/build.gradle(0 hunks)backend/security/build.gradle(0 hunks)backend/testing/build.gradle(0 hunks)build.gradle(0 hunks)ee/backend/app/build.gradle(0 hunks)ee/backend/tests/build.gradle(1 hunks)gradle.properties(1 hunks)gradle/webapp.gradle(0 hunks)settings.gradle(1 hunks)
💤 Files with no reviewable changes (9)
- backend/testing/build.gradle
- backend/api/build.gradle
- backend/development/build.gradle
- ee/backend/app/build.gradle
- build.gradle
- backend/security/build.gradle
- gradle/webapp.gradle
- backend/app/build.gradle
- backend/misc/build.gradle
✅ Files skipped from review due to trivial changes (2)
- ee/backend/tests/build.gradle
- backend/ktlint/build.gradle
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Frontend static check 🪲
- GitHub Check: Build backend 🏗️
🔇 Additional comments (5)
gradle.properties (2)
1-2: Validate JVM heap settings for local and CI environments.
The increased max heap sizes for the Kotlin daemon (-Xmx2g) and Gradle (-Xmx4g) may cause OOM or memory pressure on CI agents or low-spec developer machines. Please confirm these values work reliably across all target environments.
6-8: Verify KAPT K2 and incremental processing compatibility.
Turning onkapt.use.k2, incremental APT, and excluding the compile classpath can speed up builds, but some annotation processors may not be K2-ready. Ensure your processors (e.g., MapStruct, Dagger, QueryDSL) compile without regressions under these flags.backend/data/build.gradle (3)
70-70: Apply new email build script.
Includingemail.gradleintegrates the email frontend build tasks into this module’s lifecycle. The placement before Liquibase script is fine.
104-104: Add Thymeleaf starter for email templates.
Bringing inspring-boot-starter-thymeleafis necessary to render the compiled HTML templates.
243-245: Include generated resource directories.
Addingbuild/generated/resources/{main,test}tosrcDirsensures the compiled email templates and locale files are on the classpath.
|
Well doing it manually implies copy/pasting an entire class from Spring, so here we are with some exclusions to get rid of the problems 😅 I also tweaked (quite a bit) the Gradle scripts, using lazy constructs and fiddling with some flags. It doesn't seem to have an impact on CI, but I assume it'll start doing some wonders as soon as cache gets warm (PR is read-only cache). It has significantly improved build times on my machine. |
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
build.gradle (1)
52-58: Undefined variableunpackinaddVersionFile
The callmustRunAfter unpackreferences no declared variable. You need to point at the task provider instead:-tasks.register('addVersionFile', Task) { - mustRunAfter unpack +tasks.register('addVersionFile', Task) { + mustRunAfter tasks.named('unpack') doLast { // … } }
🧹 Nitpick comments (1)
gradle/liquibase.gradle (1)
44-46: Prefer lazy task dependency references
Use theTaskProviderAPI instead of a string literal to wire upcompileKotlin. This ensures the dependency is resolved lazily and avoids potential ordering issues.-tasks.named('diff') { - dependsOn 'compileKotlin' -} +tasks.named('diff') { + dependsOn tasks.named('compileKotlin') +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/api/build.gradle(1 hunks)backend/app/build.gradle(3 hunks)backend/data/build.gradle(5 hunks)backend/development/build.gradle(1 hunks)backend/ktlint/build.gradle(1 hunks)backend/misc/build.gradle(1 hunks)backend/security/build.gradle(2 hunks)backend/testing/build.gradle(2 hunks)build.gradle(4 hunks)ee/backend/app/build.gradle(2 hunks)ee/backend/tests/build.gradle(2 hunks)gradle.properties(1 hunks)gradle/docker.gradle(4 hunks)gradle/liquibase.gradle(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- backend/testing/build.gradle
- backend/security/build.gradle
- backend/development/build.gradle
- backend/misc/build.gradle
- ee/backend/tests/build.gradle
- backend/api/build.gradle
- ee/backend/app/build.gradle
- backend/ktlint/build.gradle
- backend/app/build.gradle
- gradle.properties
- gradle/docker.gradle
- backend/data/build.gradle
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Frontend static check 🪲
- GitHub Check: Build backend 🏗️
- GitHub Check: Ktlint 🪲
- GitHub Check: Build frontend 🏗️
🔇 Additional comments (8)
build.gradle (8)
44-50: Rootunpacktask looks good
Registration of theunpacktask withCopytype and lazy reference tobootJaris correct.
68-76:packResourcestask configuration is solid
TheZiptask properly bundles dependencies with stored compression. Consider addinggroup/descriptionfor better discoverability, but functionality is correct.
88-90: CustombootJarproxy task is correct
Delegating the rootbootJarto:server-app:bootJaris clear and leverages lazy configuration.
92-100:startDbChangelogContainerorchestration is fine
Using a default task withdoLastandexecis valid. If preferred, you can switch the registration toExectype, but this works as-is.
101-106:stopDbChangelogContainerwith Exec type is properly defined
TheExectask cleanly removes the container.
137-141: SubprojectdiffChangelogordering is correct
ApplyingmustRunAfter ':server-app:bootRun'to alldiffChangelogtasks keeps the startup sequence in place.
145-146:allDepsregistration looks good
RegisteringDependencyReportTaskin subprojects is a useful shortcut for dependency inspection.
151-155: Test logging configuration is sound
ConfiguringtestLoggingviawithType(Test)ensures consistent output across all test tasks.
backend/data/src/main/kotlin/io/tolgee/email/EmailGlobalVariablesProvider.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/actions/download-backend-build/action.yaml(2 hunks).github/actions/setup-env/action.yaml(2 hunks).github/actions/upload-backend-build/action.yaml(2 hunks).github/workflows/inactive-issues.yml(1 hunks).github/workflows/prerelease-alpha.yml(0 hunks).github/workflows/preview.yml(0 hunks).github/workflows/release.yml(5 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/preview.yml
- .github/workflows/prerelease-alpha.yml
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/inactive-issues.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/actions/upload-backend-build/action.yaml
🧰 Additional context used
🪛 GitHub Actions: Test
.github/actions/setup-env/action.yaml
[error] 66-66: Unrecognized named-value: 'secrets'. Located at position 1 within expression: secrets.DOCKERHUB_USERNAME
🔇 Additional comments (15)
.github/actions/download-backend-build/action.yaml (2)
4-7: Add configurable extraction path withsource-directoryinput
Introducing thesource-directoryinput greatly improves flexibility, allowing the action to target arbitrary checkout locations rather than hardcoded paths.
18-23: Switch shell to Bash for multi-line extraction script
Usingbashas theshellensures the here-doc and environment variable expansion work correctly, which is necessary for$SRC_DIR..github/actions/setup-env/action.yaml (3)
11-13: Approve addition ofnpm-cacheinput
The newnpm-cacheflag lets callers skip or enable npm caching independently from Node setup.
14-22: Approve addition of Docker-related flags
Thedocker,docker-ghcr, anddocker-hubboolean inputs provide granular control over Docker environment setup and authentication.
54-58: Approve QEMU and Buildx setup steps
Conditionally installing QEMU and Buildx under thedockerflag is correct and follows best practices for cross-platform Docker builds..github/workflows/release.yml (10)
14-17: Apply least-privilege permissions
Granting onlycontents: read,id-token: write, andattestations: writealigns with GitHub Actions security best practices.
19-19: Upgradeactions/checkoutto v4
Switching to the latest checkout action ensures compatibility and performance improvements.
21-26: Leverage customsetup-envaction
Centralizing environment setup via the composite action reduces duplication and keeps the workflow DRY.
48-49: Inject API credentials intobootJarstep
PassingTOLGEE_API_KEYandTOLGEE_API_URLas environment variables is necessary for the Gradle tasks to configure cloud integrations.
56-57: Inject API credentials intodockerPreparestep
Consistent propagation of secrets to the Docker preparation task ensures templating and builds have the correct runtime context.
59-68: Integrate Docker metadata-action for tagging
Usingdocker/metadata-action@v5with semver patterns improves image tagging consistency and automation.
69-82: Streamline multi-platform build & SBOM generation
Thedocker/build-push-action@v6configuration with cache, SBOM, and provenance aligns with best practices for secure and efficient image publishing.
95-97: Propagate credentials topackResources
Ensuring thepackResourcesGradle task receives the same API context asbootJaranddockerPreparemaintains consistency across all build stages.
128-130: Pass PAT to downstream billing workflow
Using a dedicatedTOLGEE_MACHINE_PATtoken for thedispatchesAPI call adheres to the principle of least privilege.
134-134: Secure curl invocation with environment token
Referencing$TOKENin theAuthorizationheader ensures the request is authenticated without hardcoding secrets.
|
I adjusted GHA tasks here so they can be used in other repositories, and improved Docker image build (namely: better image tagging with support for |
This PR introduces tooling for writing HTML emails using React Email, that is then compiled to Thymeleaf templates the backend can consume.
Features
The email design isn't the one made in #2710, but rather the first draft I originally made when first prototyping with this more than a year ago. (quick note: the design has a different footer based on whether it's sent from Cloud or Self-Hosted). This is why I'm marking this as "chore" more than "feat"; it doesn't bring any feature to the product yet.
I added
exportsto the package.json as a potential way to have additional email template workspaces ineeandbilling: those would be able to import core parts like the layout and essential components/parts, while also defining their own component/parts separately, providing good isolation of them and keeping clear legal boundaries between Tolgee OSS, Tolgee EE, and proprietary Tolgee Cloud licenses.I included documentation about how to use it, in
email/HACKING.md. Gradle config has been updated to build emails as part of the backend build process.Closes #2707
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Style
Chores
Tests