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
WalkthroughAdds a complete HTML email subsystem: frontend React Email package, Gradle tasks to build/copy/email resources, Thymeleaf-based template engine and resolver, an EmailService for sending, global variables provider, CI changes to build/test/upload email artifacts, and related tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as React Email (dev/export)
participant NPM as npm build/export
participant Gradle as Gradle (build)
participant Backend as Kotlin Backend
participant Thymeleaf as Thymeleaf Engine
participant Mail as JavaMailSender
participant Recipient as Recipient (email)
Dev->>NPM: npm run export -> generates HTML templates & i18n
NPM->>Gradle: export output (email/out, i18n)
Gradle->>Backend: copyEmailTemplates / copyEmailResources / copyEmailLocaleData
Backend->>Thymeleaf: render template with EmailTemplateEngine + message resolver
Thymeleaf-->>Backend: rendered HTML + subject
Backend->>Mail: EmailService sends MIME message (subject, html, attachments)
Mail->>Recipient: deliver email
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. Comment |
cyyynthia
force-pushed
the
cynthia/react-email
branch
2 times, most recently
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.
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 🪲
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 |
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.
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
might make builds stupid fast now, hehe
dkrizan
force-pushed
the
cynthia/react-email
branch
from
954b9e4 to
add5c96
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
127-139: $VERSION is not expanded due to single-quoted JSON.The payload uses single quotes, so
$VERSIONis sent literally to the Billing workflow. Use double quotes (or a heredoc) for shell expansion and escape inner quotes.- curl \ + curl \ -X POST \ -H "Accept: application/vnd.github.v3+json" \ - -H "Authorization: token $TOKEN" \ + -H "Authorization: token $TOKEN" \ https://api.github.com/repos/tolgee/billing/actions/workflows/test.yml/dispatches \ - -d '{"ref":"main","inputs":{"release-version":"$VERSION"}}' + -d "{\"ref\":\"main\",\"inputs\":{\"release-version\":\"${VERSION}\"}}"Optionally switch to
gh workflow runwith--jsonto avoid quoting pitfalls.Also applies to: 129-131, 136-136, 138-138
♻️ Duplicate comments (5)
.github/actions/upload-backend-build/action.yaml (1)
29-45: Don’t fail upload when optional archives are missing; avoid tilde expansion pitfalls.actions/upload-artifact treats missing files poorly and may not expand ~. Prefer $HOME and ignore missing files.
Apply:
- - name: Upload backend build result + - name: Upload backend build result uses: actions/upload-artifact@v4 with: name: backend + if-no-files-found: ignore path: | - ~/backend-api.tar.zst - ~/backend-app.tar.zst - ~/backend-data.tar.zst - ~/backend-misc.tar.zst - ~/backend-security.tar.zst - ~/backend-testing.tar.zst - ~/backend-ktlint.tar.zst - ~/backend-development.tar.zst - ~/ee-backend-app.tar.zst - ~/ee-backend-tests.tar.zst - ~/backend-email.tar.zst + $HOME/backend-api.tar.zst + $HOME/backend-app.tar.zst + $HOME/backend-data.tar.zst + $HOME/backend-misc.tar.zst + $HOME/backend-security.tar.zst + $HOME/backend-testing.tar.zst + $HOME/backend-ktlint.tar.zst + $HOME/backend-development.tar.zst + $HOME/ee-backend-app.tar.zst + $HOME/ee-backend-tests.tar.zst + $HOME/backend-email.tar.zstbuild.gradle (2)
66-68: Use lazy task reference in mustRunAfter.Direct reference to copyDist can break at configuration time.
Apply:
- tasks.named("docker") { - mustRunAfter copyDist - } + tasks.named("docker") { + mustRunAfter tasks.named("copyDist") + }
85-88: ktlint task name is wrong.Plugin exposes ktlintCheck, not ktlint.
Apply:
- dependsOn ':server-app:check' - dependsOn ktlint + dependsOn ':server-app:check' + dependsOn rootProject.tasks.named('ktlintCheck')backend/data/src/test/kotlin/io/tolgee/email/EmailServiceTest.kt (2)
39-41: Mockito initialisation fixed — good call
54-56: Captor correctly initialised via MockitoExtension
🧹 Nitpick comments (17)
.github/workflows/release.yml (2)
50-51: Unify secrets interpolation style for env vars.Minor style nit: spaces around expressions are inconsistent (
${{ secrets.FOO }}vs${{secrets.FOO}}). Pick one style repo-wide.- TOLGEE_API_KEY: ${{secrets.TOLGEE_API_KEY}} - TOLGEE_API_URL: ${{secrets.TOLGEE_API_URL}} + TOLGEE_API_KEY: ${{ secrets.TOLGEE_API_KEY }} + TOLGEE_API_URL: ${{ secrets.TOLGEE_API_URL }}Also applies to: 58-59, 97-98, 115-115
14-17: Scope elevated permissions to the attestation step if possible.
id-token: writeandattestations: writeare only needed for the attestation step. Consider moving them to step-levelpermissionsto reduce the job’s attack surface.Example:
- permissions: - contents: read - id-token: write - attestations: write + permissions: + contents: read ... - - name: Publish attestation to GitHub + - name: Publish attestation to GitHub + permissions: + id-token: write + attestations: write.github/workflows/inactive-issues.yml (1)
13-13: Upgrade to actions/stale@v9 looks good.No breaking inputs here; permissions are correct. Consider adding operations-per-run to throttle if you hit rate limits, but optional.
.github/workflows/test.yml (1)
341-363: Speed up email checks with Node caching.Cache npm in the email job to cut CI time.
Apply:
- name: Setup environment uses: ./.github/actions/setup-env with: java: 'false' + - name: Setup Node + uses: actions/setup-node@v4 + with: + node-version: '20' + cache: 'npm' + cache-dependency-path: './email/package-lock.json' + # optional: pin architecture if needed + - name: Install dependencies run: npm ci working-directory: ./emailbuild.gradle (3)
107-110: Order-only constraints for optional modules should be conditional.mustRunAfter ':ee-app:diffChangelog' will reference a task that may not exist. Guard like you did below for finalizedBy.
Apply:
- tasks.register('stopDbChangelogContainer', Exec) { - mustRunAfter ':data:diffChangelog' - mustRunAfter ':ee-app:diffChangelog' + tasks.register('stopDbChangelogContainer', Exec) { + mustRunAfter ':data:diffChangelog' + if (gradle.ext.eeAppDirectoryExists) { + mustRunAfter ':ee-app:diffChangelog' + }
143-147: Wrong project scope: this block configures nothing.Inside server-app, subprojects is likely empty; also unqualified tasks refers to server-app, not each subproject. Use the actual subproject in the loop.
Apply:
- subprojects.forEach { - tasks.matching { name == 'diffChangelog' }.configureEach { - mustRunAfter ':server-app:bootRun' - } - } + gradle.rootProject.subprojects.forEach { p -> + p.tasks.matching { it.name == 'diffChangelog' }.configureEach { + mustRunAfter ':server-app:bootRun' + } + }
54-60: Minor: ensure unpack ordering via task providers.mustRunAfter unpack uses the task name directly; resolve lazily.
Apply:
- tasks.register('addVersionFile', Task) { - mustRunAfter unpack + tasks.register('addVersionFile', Task) { + mustRunAfter tasks.named('unpack')backend/app/src/test/kotlin/io/tolgee/activity/ActivityLogTest.kt (1)
239-245: Reduce flakiness by bounding wait timeConsider adding an explicit timeout to waitFor (or lowering pollTime) to keep tests snappy on CI.
- waitFor(pollTime = 2000) { + waitFor(timeout = 15000, pollTime = 1000) {backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateEngine.kt (2)
22-28: Call super in initializeSpringSpecific to preserve base initializationNot calling super can skip important SpringTemplateEngine setup.
class EmailTemplateEngine() : SpringTemplateEngine() { lateinit var emailMessageResolver: IMessageResolver override fun initializeSpringSpecific() { - setMessageResolver(emailMessageResolver) + super.initializeSpringSpecific() + setMessageResolver(emailMessageResolver) } }
22-22: Remove empty default constructor (detekt)-class EmailTemplateEngine() : SpringTemplateEngine() { +class EmailTemplateEngine : SpringTemplateEngine() {backend/data/src/test/kotlin/io/tolgee/email/EmailServiceTest.kt (2)
75-83: Add a negative test for missing <title>Assert that sendEmailTemplate fails with a clear error when template lacks a title, to lock in the contract.
@Test fun `it fails when template has no title`() { whenever(emailGlobalVariablesProvider.invoke()).thenReturn(emptyMap()) emailService.sendEmailTemplate("[email protected]", "no-title-email", Locale.ENGLISH, emptyMap()) // expect: IllegalStateException with message "Email template must have a title tag!" }
150-178: Consider testing attachments pathAdd a test that passes one EmailAttachment and verifies it appears in the sent message.
backend/data/src/main/kotlin/io/tolgee/email/EmailService.kt (3)
78-79: Use standard charset constant for clarity- val helper = MimeMessageHelper(message, MimeMessageHelper.MULTIPART_MODE_MIXED_RELATED, "UTF8") + val helper = MimeMessageHelper( + message, MimeMessageHelper.MULTIPART_MODE_MIXED_RELATED, java.nio.charset.StandardCharsets.UTF_8.name() + )
48-55: @async error handling: avoid swallowed exceptionsVoid-returning @async methods can drop exceptions. Either return CompletableFuture or configure an AsyncUncaughtExceptionHandler to log/alert.
- @Async - fun sendEmailTemplate(...): Unit { + @Async + fun sendEmailTemplate(...): java.util.concurrent.CompletableFuture<Unit> { ... - sendEmail(recipient, subject, html, attachments) + sendEmail(recipient, subject, html, attachments) + return java.util.concurrent.CompletableFuture.completedFuture(Unit) }Or add a central AsyncUncaughtExceptionHandler in your @configuration.
Also applies to: 70-87
89-96: Make title extraction robust (newlines, case) and explicit errorCurrent regex fails if title spans lines or uses uppercase tags.
- private fun extractEmailTitle(html: String): String { - return REGEX_TITLE.find(html)?.groupValues?.get(1) - ?: throw IllegalStateException("Email template must have a title tag!") - } + private fun extractEmailTitle(html: String): String = + REGEX_TITLE.find(html)?.groups?.get(1)?.value + ?: throw IllegalStateException("Email template must have a title tag!") ... - private val REGEX_TITLE = Regex("<title>(.+?)</title>") + private val REGEX_TITLE = Regex( + pattern = "<title>([\\s\\S]*?)</title>", + options = setOf(RegexOption.IGNORE_CASE) + )backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateConfig.kt (2)
34-40: Be explicit about template modes and resolver orderSet HTML mode and resolver order to avoid ambiguous resolution across resolvers.
fun templateResolver(): ClassLoaderTemplateResolver { val templateResolver = ClassLoaderTemplateResolver() templateResolver.characterEncoding = "UTF-8" + templateResolver.templateMode = "HTML" templateResolver.prefix = "/email-templates/" templateResolver.suffix = ".html" + templateResolver.order = 2 return templateResolver } ... fun templateEngine( @Qualifier("emailTemplateResolver") templateResolver: ITemplateResolver, ): EmailTemplateEngine { val stringTemplateResolver = StringTemplateResolver() stringTemplateResolver.resolvablePatternSpec.addPattern("<!--@frag-->*") + stringTemplateResolver.templateMode = "HTML" + stringTemplateResolver.order = 1Also applies to: 66-74
51-62: Bean wiring: guard against early engine initialisationSetting templateEngine.emailMessageResolver inside the resolver bean is clever but brittle. Consider documenting this coupling, or add a simple sanity check in EmailTemplateEngine.initializeSpringSpecific to fail fast if emailMessageResolver isn’t set.
override fun initializeSpringSpecific() { - super.initializeSpringSpecific() - setMessageResolver(emailMessageResolver) + super.initializeSpringSpecific() + check(::emailMessageResolver.isInitialized) { "emailMessageResolver not initialized" } + setMessageResolver(emailMessageResolver) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
.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).github/workflows/reportIntermittentTests.yml(2 hunks).github/workflows/test.yml(4 hunks).gitmodules(0 hunks)DEVELOPMENT.md(1 hunks)backend/api/build.gradle(1 hunks)backend/app/build.gradle(3 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(5 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/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/main/kotlin/io/tolgee/pubSub/RedisPubSubReceiver.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)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/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)
💤 Files with no reviewable changes (3)
- .gitmodules
- .github/workflows/preview.yml
- .github/workflows/prerelease-alpha.yml
✅ Files skipped from review due to trivial changes (2)
- backend/data/src/test/resources/email-i18n-test/messages_en.properties
- backend/data/src/test/kotlin/io/tolgee/service/machineTranslation/MtBatchTranslatorTest.kt
🚧 Files skipped from review as they are similar to previous changes (20)
- backend/data/src/main/kotlin/io/tolgee/activity/EntityDescriptionProvider.kt
- backend/data/src/test/kotlin/io/tolgee/email/EmailGlobalVariablesProviderTest.kt
- .github/actions/download-backend-build/action.yaml
- backend/data/src/main/kotlin/io/tolgee/pubSub/RedisPubSubReceiver.kt
- .github/workflows/reportIntermittentTests.yml
- DEVELOPMENT.md
- backend/misc/build.gradle
- backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt
- backend/development/build.gradle
- backend/testing/build.gradle
- backend/ktlint/build.gradle
- backend/security/build.gradle
- backend/data/src/main/kotlin/io/tolgee/configuration/HibernateConfig.kt
- backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/PatControllerTest.kt
- backend/data/src/main/kotlin/io/tolgee/activity/iterceptor/InterceptedEventsManager.kt
- backend/app/build.gradle
- backend/api/build.gradle
- backend/data/build.gradle
- backend/data/src/main/kotlin/io/tolgee/email/EmailGlobalVariablesProvider.kt
- .github/actions/setup-env/action.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: cyyynthia
PR: tolgee/tolgee-platform#1987
File: email/components/translate.ts:36-37
Timestamp: 2025-06-16T20:31:22.217Z
Learning: In email template processing systems like React Email with Thymeleaf integration, build-time processes that run with NODE_ENV === 'production' are short-lived and terminate after completion, so memory accumulation in data structures like Sets is not a concern as the memory is automatically freed when the process ends.
🪛 detekt (1.23.8)
backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateEngine.kt
[warning] 22-22: An empty default constructor can be removed.
(detekt.empty-blocks.EmptyDefaultConstructor)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Ktlint 🪲
- GitHub Check: Frontend static check 🪲
- GitHub Check: Build backend 🏗️
🔇 Additional comments (7)
.github/workflows/release.yml (3)
71-84: Build step depends on fixed tags.Once tags are provided by the metadata fix above, this block looks good. Multi-arch, cache-to/from, SBOM, and provenance flags are correctly set for Buildx.
Please re-run this job after applying the metadata fix to ensure
${{ steps.meta.outputs.tags }}is non-empty (the action fails otherwise).
19-19: Checkout v4 upgrade looks good.Meets current guidance;
contents: readis already granted at job level.
21-27: Verified: setup-env action includes QEMU, Buildx, and Docker Hub login steps..github/workflows/test.yml (2)
27-27: Building test emails in backend-build is fine.This ensures artifacts are reusable across jobs. Just confirm Gradle caches are enabled to amortize the extra work.
74-76: Ensure downstream steps tolerate missing email artifacts.You set SKIP_EMAIL_BUILD=true for tests/e2e. With the upload fix to ignore missing archives, the download/extract steps must also skip gracefully (mirrors your download action changes).
Also applies to: 181-182
backend/app/src/main/kotlin/io/tolgee/Application.kt (1)
8-16: Thymeleaf auto-config removal safe
No Thymeleaf templates, controllers, ModelAndView usage, or view-resolver beans detected in the codebase; excluding ThymeleafAutoConfiguration has no impact.backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateConfig.kt (1)
42-49: No action needed for spring-beans CVE
Thespring-icudependency inbackend/data/build.gradlealready excludesspring-context(and its transitivespring-beans5.2.5.RELEASE) and Spring Boot 3.4.8’s BOM managesspring-beansat Spring Framework 6.x, which is above the vulnerable thresholds. (mvnrepository.com, stackoverflow.com)
| - name: Compress build results | ||
| shell: bash | ||
| env: | ||
| SRC_DIR: ${{ inputs.source-directory }} | ||
| run: | | ||
| tar --zstd -cf ~/backend-api.tar.zst ./backend/api/build | ||
| tar --zstd -cf ~/backend-app.tar.zst ./backend/app/build | ||
| tar --zstd -cf ~/backend-data.tar.zst ./backend/data/build | ||
| tar --zstd -cf ~/backend-misc.tar.zst ./backend/misc/build | ||
| tar --zstd -cf ~/backend-security.tar.zst ./backend/security/build | ||
| tar --zstd -cf ~/backend-testing.tar.zst ./backend/testing/build | ||
| tar --zstd -cf ~/backend-ktlint.tar.zst ./backend/ktlint/build | ||
| tar --zstd -cf ~/backend-development.tar.zst ./backend/development/build | ||
| tar --zstd -cf ~/ee-backend-app.tar.zst ./ee/backend/app/build | ||
| tar --zstd -cf ~/ee-backend-tests.tar.zst ./ee/backend/tests/build | ||
| tar --zstd -cf ~/backend-api.tar.zst $SRC_DIR/backend/api/build | ||
| tar --zstd -cf ~/backend-app.tar.zst $SRC_DIR/backend/app/build | ||
| tar --zstd -cf ~/backend-data.tar.zst $SRC_DIR/backend/data/build | ||
| tar --zstd -cf ~/backend-misc.tar.zst $SRC_DIR/backend/misc/build | ||
| tar --zstd -cf ~/backend-security.tar.zst $SRC_DIR/backend/security/build | ||
| tar --zstd -cf ~/backend-testing.tar.zst $SRC_DIR/backend/testing/build | ||
| tar --zstd -cf ~/backend-ktlint.tar.zst $SRC_DIR/backend/ktlint/build | ||
| tar --zstd -cf ~/backend-development.tar.zst $SRC_DIR/backend/development/build | ||
| tar --zstd -cf ~/ee-backend-app.tar.zst $SRC_DIR/ee/backend/app/build | ||
| tar --zstd -cf ~/ee-backend-tests.tar.zst $SRC_DIR/ee/backend/tests/build | ||
| tar --zstd -cf ~/backend-email.tar.zst $SRC_DIR/email/out |
There was a problem hiding this comment.
Guard tar calls; they fail when directories are absent.
If any of these paths don’t exist (e.g., SKIP_EMAIL_BUILD=true), tar exits non-zero and the step fails.
Apply:
- - name: Compress build results
- shell: bash
- env:
- SRC_DIR: ${{ inputs.source-directory }}
- run: |
- tar --zstd -cf ~/backend-api.tar.zst $SRC_DIR/backend/api/build
- tar --zstd -cf ~/backend-app.tar.zst $SRC_DIR/backend/app/build
- tar --zstd -cf ~/backend-data.tar.zst $SRC_DIR/backend/data/build
- tar --zstd -cf ~/backend-misc.tar.zst $SRC_DIR/backend/misc/build
- tar --zstd -cf ~/backend-security.tar.zst $SRC_DIR/backend/security/build
- tar --zstd -cf ~/backend-testing.tar.zst $SRC_DIR/backend/testing/build
- tar --zstd -cf ~/backend-ktlint.tar.zst $SRC_DIR/backend/ktlint/build
- tar --zstd -cf ~/backend-development.tar.zst $SRC_DIR/backend/development/build
- tar --zstd -cf ~/ee-backend-app.tar.zst $SRC_DIR/ee/backend/app/build
- tar --zstd -cf ~/ee-backend-tests.tar.zst $SRC_DIR/ee/backend/tests/build
- tar --zstd -cf ~/backend-email.tar.zst $SRC_DIR/email/out
+ - name: Compress build results
+ shell: bash
+ env:
+ SRC_DIR: ${{ inputs.source-directory }}
+ run: |
+ set -euo pipefail
+ pack_if_exists() {
+ local out="$1"; shift
+ local dir="$1"; shift
+ if [[ -d "$dir" ]]; then
+ tar --zstd -cf "$out" "$dir"
+ else
+ echo "Skipping: $dir does not exist"
+ fi
+ }
+ pack_if_exists "$HOME/backend-api.tar.zst" "$SRC_DIR/backend/api/build"
+ pack_if_exists "$HOME/backend-app.tar.zst" "$SRC_DIR/backend/app/build"
+ pack_if_exists "$HOME/backend-data.tar.zst" "$SRC_DIR/backend/data/build"
+ pack_if_exists "$HOME/backend-misc.tar.zst" "$SRC_DIR/backend/misc/build"
+ pack_if_exists "$HOME/backend-security.tar.zst" "$SRC_DIR/backend/security/build"
+ pack_if_exists "$HOME/backend-testing.tar.zst" "$SRC_DIR/backend/testing/build"
+ pack_if_exists "$HOME/backend-ktlint.tar.zst" "$SRC_DIR/backend/ktlint/build"
+ pack_if_exists "$HOME/backend-development.tar.zst" "$SRC_DIR/backend/development/build"
+ pack_if_exists "$HOME/ee-backend-app.tar.zst" "$SRC_DIR/ee/backend/app/build"
+ pack_if_exists "$HOME/ee-backend-tests.tar.zst" "$SRC_DIR/ee/backend/tests/build"
+ pack_if_exists "$HOME/backend-email.tar.zst" "$SRC_DIR/email/out"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Compress build results | |
| shell: bash | |
| env: | |
| SRC_DIR: ${{ inputs.source-directory }} | |
| run: | | |
| tar --zstd -cf ~/backend-api.tar.zst ./backend/api/build | |
| tar --zstd -cf ~/backend-app.tar.zst ./backend/app/build | |
| tar --zstd -cf ~/backend-data.tar.zst ./backend/data/build | |
| tar --zstd -cf ~/backend-misc.tar.zst ./backend/misc/build | |
| tar --zstd -cf ~/backend-security.tar.zst ./backend/security/build | |
| tar --zstd -cf ~/backend-testing.tar.zst ./backend/testing/build | |
| tar --zstd -cf ~/backend-ktlint.tar.zst ./backend/ktlint/build | |
| tar --zstd -cf ~/backend-development.tar.zst ./backend/development/build | |
| tar --zstd -cf ~/ee-backend-app.tar.zst ./ee/backend/app/build | |
| tar --zstd -cf ~/ee-backend-tests.tar.zst ./ee/backend/tests/build | |
| tar --zstd -cf ~/backend-api.tar.zst $SRC_DIR/backend/api/build | |
| tar --zstd -cf ~/backend-app.tar.zst $SRC_DIR/backend/app/build | |
| tar --zstd -cf ~/backend-data.tar.zst $SRC_DIR/backend/data/build | |
| tar --zstd -cf ~/backend-misc.tar.zst $SRC_DIR/backend/misc/build | |
| tar --zstd -cf ~/backend-security.tar.zst $SRC_DIR/backend/security/build | |
| tar --zstd -cf ~/backend-testing.tar.zst $SRC_DIR/backend/testing/build | |
| tar --zstd -cf ~/backend-ktlint.tar.zst $SRC_DIR/backend/ktlint/build | |
| tar --zstd -cf ~/backend-development.tar.zst $SRC_DIR/backend/development/build | |
| tar --zstd -cf ~/ee-backend-app.tar.zst $SRC_DIR/ee/backend/app/build | |
| tar --zstd -cf ~/ee-backend-tests.tar.zst $SRC_DIR/ee/backend/tests/build | |
| tar --zstd -cf ~/backend-email.tar.zst $SRC_DIR/email/out | |
| - name: Compress build results | |
| shell: bash | |
| env: | |
| SRC_DIR: ${{ inputs.source-directory }} | |
| run: | | |
| set -euo pipefail | |
| pack_if_exists() { | |
| local out="$1"; shift | |
| local dir="$1"; shift | |
| if [[ -d "$dir" ]]; then | |
| tar --zstd -cf "$out" "$dir" | |
| else | |
| echo "Skipping: $dir does not exist" | |
| fi | |
| } | |
| pack_if_exists "$HOME/backend-api.tar.zst" "$SRC_DIR/backend/api/build" | |
| pack_if_exists "$HOME/backend-app.tar.zst" "$SRC_DIR/backend/app/build" | |
| pack_if_exists "$HOME/backend-data.tar.zst" "$SRC_DIR/backend/data/build" | |
| pack_if_exists "$HOME/backend-misc.tar.zst" "$SRC_DIR/backend/misc/build" | |
| pack_if_exists "$HOME/backend-security.tar.zst" "$SRC_DIR/backend/security/build" | |
| pack_if_exists "$HOME/backend-testing.tar.zst" "$SRC_DIR/backend/testing/build" | |
| pack_if_exists "$HOME/backend-ktlint.tar.zst" "$SRC_DIR/backend/ktlint/build" | |
| pack_if_exists "$HOME/backend-development.tar.zst" "$SRC_DIR/backend/development/build" | |
| pack_if_exists "$HOME/ee-backend-app.tar.zst" "$SRC_DIR/ee/backend/app/build" | |
| pack_if_exists "$HOME/ee-backend-tests.tar.zst" "$SRC_DIR/ee/backend/tests/build" | |
| pack_if_exists "$HOME/backend-email.tar.zst" "$SRC_DIR/email/out" |
🤖 Prompt for AI Agents
.github/actions/upload-backend-build/action.yaml around lines 12 to 27: the tar
commands will cause the step to fail if any source directory is missing; guard
each tar invocation by first checking the target path exists (e.g. test with [
-e ] or [ -d ]) and only run tar when it does, or alternatively use a tar option
that ignores missing inputs if available; ensure the step still creates or skips
artifacts deterministically and preserves exit codes for real failures.
| - name: Prepare Docker metadata | ||
| id: meta | ||
| uses: docker/metadata-action@v5 | ||
| with: | ||
| images: tolgee/tolgee | ||
| tags: | | ||
| type=semver,pattern={{version}} | ||
| type=semver,pattern={{major}}.{{minor}} | ||
| type=semver,pattern={{major}} | ||
There was a problem hiding this comment.
Docker tags will be empty in non-tag runs — pass the version value explicitly.
docker/metadata-action won’t infer a semver from this branch-triggered workflow. Without value=…, ${{ steps.meta.outputs.tags }} will be empty and the build step will fail. Provide the computed VERSION as value (and optionally keep a raw tag).
- name: Prepare Docker metadata
id: meta
uses: docker/metadata-action@v5
with:
images: tolgee/tolgee
tags: |
- type=semver,pattern={{version}}
- type=semver,pattern={{major}}.{{minor}}
- type=semver,pattern={{major}}
+ # tag exactly the resolved version (e.g. v1.2.3)
+ type=raw,value=${{ steps.version.outputs.VERSION }}
+ # derive semver aliases from the same value
+ type=semver,pattern={{version}},value=${{ steps.version.outputs.VERSION }}
+ type=semver,pattern={{major}}.{{minor}},value=${{ steps.version.outputs.VERSION }}
+ type=semver,pattern={{major}},value=${{ steps.version.outputs.VERSION }}🤖 Prompt for AI Agents
.github/workflows/release.yml around lines 61 to 70: the docker/metadata-action
usage builds semver tags without an explicit value so on branch runs the outputs
will be empty; update the tags input to pass the computed VERSION explicitly
(e.g. add value=${{ env.VERSION }} to the semver tag line(s) or add a raw tag
with value=${{ env.VERSION }}) so that steps.meta.outputs.tags is populated on
non-tag runs.
| - name: Publish attestation to GitHub | ||
| uses: actions/attest-build-provenance@v2 | ||
| with: | ||
| subject-name: docker.io/tolgee/tolgee | ||
| subject-digest: ${{ steps.docker-push.outputs.digest }} | ||
| push-to-registry: true |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm Docker Hub attestation support and required perms.
You’re attesting with actions/attest-build-provenance@v2 to Docker Hub (push-to-registry: true). Verify the action currently supports Docker Hub and that job credentials are sufficient (no GHCR-only assumptions).
🌐 Web query:
Does actions/attest-build-provenance@v2 support publishing SLSA build provenance to Docker Hub (OCI registry) and what permissions/tokens are required?
💡 Result:
Short answer: Yes — actions/attest-build-provenance@v2 can push SLSA/in-toto (Sigstore bundle) attestations to an OCI registry (including Docker Hub) using push-to-registry: true. You must provide registry credentials with write access to the target OCI repository and set the workflow permissions GitHub requires to mint Sigstore certs and record attestations.
What to set
- Action inputs: push-to-registry: true, subject-name must be the fully‑qualified registry/repo (for Docker Hub use index.docker.io/OWNER/REPO). [1][2][3]
- Required workflow permissions (minimum for container images):
- id-token: write
- attestations: write
- contents: read
- packages: write
(These allow OIDC signing and persisting attestations). [1][2]
Registry credentials/tokens
- You must log in to the OCI registry (Docker Hub) in your workflow (e.g., docker/login-action) using a username and a registry token/password secret with write/push access to the target repo (store these in GitHub Secrets). The attestation push will use those credentials to write the attestation bundle to the OCI registry. For Docker Hub, use index.docker.io as the registry host. [1][3][4]
References
- GitHub docs: Using artifact attestations / example for container images (permissions + push-to-registry). [1]
- actions/attest-build-provenance repo (inputs/behavior). [2]
- Marketplace / README note about Docker Hub index.docker.io usage. [3]
- Docker Docs on attestations and pushing provenance via docker/build-push-action. [4]
Ensure Docker Hub attestation is correctly configured
- Change
subject-nameto the fully qualified Docker Hub path (e.g.index.docker.io/tolgee/tolgee) - Add a Docker Hub login step (e.g.
docker/login-action) using a secret with write/push rights - Grant these minimum workflow permissions at the top of
.github/workflows/release.yml:
•id-token: write
•attestations: write
•contents: read
•packages: write
🤖 Prompt for AI Agents
.github/workflows/release.yml around lines 85 to 90: update the attestation and
authentication configuration by changing subject-name to the fully qualified
Docker Hub path (e.g. index.docker.io/tolgee/tolgee), add a Docker Hub login
step before pushing (use docker/login-action with a secret that has write/push
rights), and ensure the workflow top-level permissions include id-token: write,
attestations: write, contents: read, and packages: write so the attestation and
push steps have the required access.
| tasks.register('build') { | ||
| dependsOn ':server-app:build' | ||
| dependsOn runE2e | ||
| } | ||
|
|
||
| task check { | ||
| dependsOn project(':server-app').tasks.findByName("check") | ||
| tasks.register('check') { | ||
| dependsOn ':server-app:check' | ||
| dependsOn ktlint | ||
| } |
There was a problem hiding this comment.
Re-registering standard tasks in server-app collides with existing tasks.
server-app already has build/check. Registering them again will fail. Create aggregate tasks at the root or configure existing root tasks.
Apply (aggregate at root):
- tasks.register('build') {
- dependsOn ':server-app:build'
- dependsOn runE2e
- }
+ rootProject.tasks.register('aggregateBuild') {
+ dependsOn ':server-app:build'
+ dependsOn rootProject.tasks.named('runE2e')
+ }And:
- tasks.register('check') {
- dependsOn ':server-app:check'
- dependsOn ktlint
- }
+ rootProject.tasks.register('aggregateCheck') {
+ dependsOn ':server-app:check'
+ dependsOn rootProject.tasks.named('ktlintCheck')
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tasks.register('build') { | |
| dependsOn ':server-app:build' | |
| dependsOn runE2e | |
| } | |
| task check { | |
| dependsOn project(':server-app').tasks.findByName("check") | |
| tasks.register('check') { | |
| dependsOn ':server-app:check' | |
| dependsOn ktlint | |
| } | |
| rootProject.tasks.register('aggregateBuild') { | |
| dependsOn ':server-app:build' | |
| dependsOn rootProject.tasks.named('runE2e') | |
| } | |
| rootProject.tasks.register('aggregateCheck') { | |
| dependsOn ':server-app:check' | |
| dependsOn rootProject.tasks.named('ktlintCheck') | |
| } |
🤖 Prompt for AI Agents
In build.gradle around lines 80-88, you are re-registering the standard "build"
and "check" tasks which collides with existing tasks in :server-app; instead
either configure the existing root tasks or create new aggregate tasks. Replace
the registrations with configuring the existing tasks via tasks.named('build') {
dependsOn(':server-app:build', runE2e) } and tasks.named('check') {
dependsOn(':server-app:check', ktlint) } or, if you prefer not to touch standard
task names, create new aggregate tasks like tasks.register('buildAll') {
dependsOn(':server-app:build', runE2e) } and tasks.register('checkAll') {
dependsOn(':server-app:check', ktlint) } so you avoid re-registering and task
name collisions.
| tasks.register('bootJar') { | ||
| dependsOn ':server-app:bootJar' | ||
| } |
There was a problem hiding this comment.
Don’t shadow server-app’s bootJar task.
Registering bootJar again will collide. Use an aggregate name.
Apply:
- tasks.register('bootJar') {
- dependsOn ':server-app:bootJar'
- }
+ rootProject.tasks.register('aggregateBootJar') {
+ dependsOn ':server-app:bootJar'
+ }🤖 Prompt for AI Agents
In build.gradle around lines 90 to 92, the new tasks.register('bootJar') shadows
the existing server-app:bootJar task and will collide; rename the task to an
aggregate/unique name (e.g., 'aggregateBootJar' or 'serverAppBootJar') and make
it depend on ':server-app:bootJar' instead of re-registering 'bootJar'; update
any references to the task name accordingly so it no longer conflicts with the
subproject task.
| tasks.register('diffChangeLog') { | ||
| project(':server-app').tasks.named("bootRun") { | ||
| mustRunAfter(startDbChangelogContainer) | ||
| } | ||
|
|
||
| finalizedBy = [ | ||
| startDbChangelogContainer, | ||
| project(':server-app').tasks.findByName("bootRun"), | ||
| project(':data').tasks.findByName("diffChangelog"), | ||
| startDbChangelogContainer, | ||
| ':server-app:bootRun', | ||
| ':data:diffChangelog', | ||
| ] | ||
|
|
||
| if (gradle.ext.billingAppDirectory.exists()) { | ||
| def billingDiffChangelog = project(':billing-app').tasks.findByName("diffChangelog") | ||
| finalizedBy.add(billingDiffChangelog) | ||
| finalizedBy.add(':billing-app:diffChangelog') | ||
| } | ||
|
|
||
| if (gradle.ext.eeAppDirectoryExists) { | ||
| finalizedBy.add(project(':ee-app').tasks.findByName("diffChangelog")) | ||
| finalizedBy.add(':ee-app:diffChangelog') | ||
| } | ||
|
|
||
| finalizedBy.add(stopDbChangelogContainer) | ||
|
|
||
|
|
||
| doFirst { | ||
| project(':server-app').bootRun | ||
| .systemProperty('spring.profiles.active', 'dbschema') | ||
| // Use an unlikely-to-be-used port | ||
| .systemProperty('server.port', '61987') | ||
| .systemProperty('spring.profiles.active', 'dbschema') | ||
| // Use an unlikely-to-be-used port | ||
| .systemProperty('server.port', '61987') | ||
| } | ||
| } |
There was a problem hiding this comment.
Misuse of finalizedBy API and void method chaining on systemProperty.
- finalizedBy is a method, not a mutable list; assignments and .add() won’t work.
- JavaExec/BootRun.systemProperty returns void; chaining leads to NPE.
Apply:
- tasks.register('diffChangeLog') {
- project(':server-app').tasks.named("bootRun") {
- mustRunAfter(startDbChangelogContainer)
- }
-
- finalizedBy = [
- startDbChangelogContainer,
- ':server-app:bootRun',
- ':data:diffChangelog',
- ]
-
- if (gradle.ext.billingAppDirectory.exists()) {
- finalizedBy.add(':billing-app:diffChangelog')
- }
-
- if (gradle.ext.eeAppDirectoryExists) {
- finalizedBy.add(':ee-app:diffChangelog')
- }
-
- finalizedBy.add(stopDbChangelogContainer)
-
- doFirst {
- project(':server-app').bootRun
- .systemProperty('spring.profiles.active', 'dbschema')
- // Use an unlikely-to-be-used port
- .systemProperty('server.port', '61987')
- }
- }
+ tasks.register('diffChangeLog') {
+ project(':server-app').tasks.named("bootRun") {
+ mustRunAfter(startDbChangelogContainer)
+ systemProperty 'spring.profiles.active', 'dbschema'
+ // Use an unlikely-to-be-used port
+ systemProperty 'server.port', '61987'
+ }
+ finalizedBy startDbChangelogContainer, ':server-app:bootRun', ':data:diffChangelog'
+ if (gradle.ext.billingAppDirectory.exists()) {
+ finalizedBy ':billing-app:diffChangelog'
+ }
+ if (gradle.ext.eeAppDirectoryExists) {
+ finalizedBy ':ee-app:diffChangelog'
+ }
+ finalizedBy stopDbChangelogContainer
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tasks.register('diffChangeLog') { | |
| project(':server-app').tasks.named("bootRun") { | |
| mustRunAfter(startDbChangelogContainer) | |
| } | |
| finalizedBy = [ | |
| startDbChangelogContainer, | |
| project(':server-app').tasks.findByName("bootRun"), | |
| project(':data').tasks.findByName("diffChangelog"), | |
| startDbChangelogContainer, | |
| ':server-app:bootRun', | |
| ':data:diffChangelog', | |
| ] | |
| if (gradle.ext.billingAppDirectory.exists()) { | |
| def billingDiffChangelog = project(':billing-app').tasks.findByName("diffChangelog") | |
| finalizedBy.add(billingDiffChangelog) | |
| finalizedBy.add(':billing-app:diffChangelog') | |
| } | |
| if (gradle.ext.eeAppDirectoryExists) { | |
| finalizedBy.add(project(':ee-app').tasks.findByName("diffChangelog")) | |
| finalizedBy.add(':ee-app:diffChangelog') | |
| } | |
| finalizedBy.add(stopDbChangelogContainer) | |
| doFirst { | |
| project(':server-app').bootRun | |
| .systemProperty('spring.profiles.active', 'dbschema') | |
| // Use an unlikely-to-be-used port | |
| .systemProperty('server.port', '61987') | |
| .systemProperty('spring.profiles.active', 'dbschema') | |
| // Use an unlikely-to-be-used port | |
| .systemProperty('server.port', '61987') | |
| } | |
| } | |
| tasks.register('diffChangeLog') { | |
| project(':server-app').tasks.named("bootRun") { | |
| mustRunAfter(startDbChangelogContainer) | |
| systemProperty 'spring.profiles.active', 'dbschema' | |
| // Use an unlikely-to-be-used port | |
| systemProperty 'server.port', '61987' | |
| } | |
| finalizedBy startDbChangelogContainer, ':server-app:bootRun', ':data:diffChangelog' | |
| if (gradle.ext.billingAppDirectory.exists()) { | |
| finalizedBy ':billing-app:diffChangelog' | |
| } | |
| if (gradle.ext.eeAppDirectoryExists) { | |
| finalizedBy ':ee-app:diffChangelog' | |
| } | |
| finalizedBy stopDbChangelogContainer | |
| } |
🤖 Prompt for AI Agents
In build.gradle around lines 114 to 141, the block registering the diffChangeLog
task misuses finalizedBy (treating it like a mutable list) and chains
systemProperty on the BootRun JavaExec which returns void; fix by calling
finalizedBy with task references/paths (varargs) instead of assigning or calling
.add(), and for conditional additions call finalizedBy(...) individually inside
the conditionals; for setting system properties obtain the server-app bootRun
task instance and invoke systemProperty on that task (or in its doFirst/doLast
closure) rather than chaining off a void call so each systemProperty call is
invoked on the BootRun task instance.
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
New Features
CI / DevOps
Documentation
Chores
Tests