feat: well designed transactional e-mails #1987
Conversation
0d73c0f to
3e1496b
Compare
3e1496b to
8fb4d33
Compare
8fb4d33 to
6a361e0
Compare
6a361e0 to
91c5357
Compare
WalkthroughAdds a full email subsystem (React Email templates, extraction, ICUs, Tailwind, and backend Thymeleaf rendering), integrates email build into Gradle and CI, modernizes many Gradle task declarations, refactors email sending to a new EmailService, adjusts tests for async email delivery, and updates E2E email tooling and fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as App code / Sender
participant Tolgee as TolgeeEmailSender
participant Service as EmailService
participant Engine as EmailTemplateEngine (Thymeleaf)
participant Mailer as JavaMailSender
Caller->>Tolgee: request send (EmailParams / template)
Tolgee->>Service: sendEmailTemplate(recipient, template, locale, props...)
Service->>Engine: process(template, context + global vars)
Engine-->>Service: rendered HTML (extract title if needed)
Service->>Mailer: send MimeMessage (attachments, bcc, replyTo)
Mailer-->>Service: sent / exception
Service-->>Tolgee: async completion (returns immediately due to @Async)
sequenceDiagram
participant NPM as npm (email package)
participant Extractor as extractor.ts
participant Gradle as gradle/email.gradle
participant Backend as backend build
NPM->>NPM: build/export templates (out/html)
NPM->>Extractor: extract i18n keys from TSX
Extractor-->>Gradle: writes email/i18n/*.properties
Gradle->>Gradle: copyEmailTemplates & copyEmailLocaleData -> build/generated/resources
Gradle-->>Backend: backend includes generated templates/resources in processResources
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
1777d2c to
bb08e1c
Compare
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 de 8000 fine 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
Co
8000
nfiguration 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/emai 7FA l/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
- e 103CE mail/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) 7F2
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 echo "1) Occurrences of stack.removeLast with context:" echo -e "\n2) Declaration/initialization of 'stack' in EmailMessageResolver.kt:" echo -e "\n3) Any ArrayDeque usage in the file:"
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)
🧰 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)
🔇 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’s MatchResult (it) instead of the outer Matcher, fixing the wrong-tag issue reported in earlier reviews.
Looks good.
67-69: ```shell
#!/bin/bash
set -e
rg -C5 -n "stack.removeLast"
rg -C5 -n "stack =" -n backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt || true
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
Co-authored-by: Daniel Krizan <danyelkrizan@gmail.com>
6707d38 to
7a8ce58
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/task/TaskControllerTest.kt (1)
382-382: Apply the same asynchronous wait pattern to these email assertions.Lines 382 and 398 also check email notifications but don't use
waitForNotThrowing. Since email delivery is asynchronous (as evidenced by the change on lines 104-109), these assertions should use the same pattern to prevent flaky tests.Apply this diff to line 382:
- assertThat(notificationUtil.newestEmailNotification()).contains("has been canceled") + waitForNotThrowing(timeout = 2000, pollTime = 25) { + assertThat(notificationUtil.newestEmailNotification()).contains("has been canceled") + }Apply this diff to line 398:
- assertThat(notificationUtil.newestEmailNotification()).contains("has been finished") + waitForNotThrowing(timeout = 2000, pollTime = 25) { + assertThat(notificationUtil.newestEmailNotification()).contains("has been finished") + }Also applies to: 398-398
♻️ Duplicate comments (22)
e2e/cypress/common/apiCalls/common.ts (7)
394-400: Fix link extraction: supports https and preserves query/hashCurrent regex only matches http and truncates query/hash. Extract href attribute instead.
- return { - verifyEmailLink: r.HTML.replace(/.*(http:\/\/[\w:/]*).*/gs, '$1'), + return { + verifyEmailLink: (r.HTML.match(/href="([^"]+)"/i) || [,''])[1], fromAddress: r.From.Address, toAddress: r.To[0].Address, content: r.HTML, };
403-425: Add bounds check for index and fix fragile link extractionGuard emails[index] and switch to robust href parsing.
export const getParsedEmailVerificationByIndex = (index: number) => { if (index === 0) { return getLatestEmail().then((email) => { return { - verifyEmailLink: email.HTML.replace(/.*(http:\/\/[\w:/]*).*/gs, '$1'), + verifyEmailLink: (email.HTML.match(/href="([^"]+)"/i) || [,''])[1], fromAddress: email.From.Address, toAddress: email.To[0].Address, content: email.HTML, }; }); } else { return getAllEmails().then((emails) => { + if (!emails || index < 0 || index >= emails.length) { + throw new Error(`Email at index ${index} not found. Total: ${emails?.length ?? 0}`); + } return getEmail(emails[index].ID).then((email) => { return { - verifyEmailLink: email.HTML.replace(/.*(http:\/\/[\w:/]*).*/gs, '$1'), + verifyEmailLink: (email.HTML.match(/href="([^"]+)"/i) || [,''])[1], fromAddress: email.From.Address, toAddress: email.To[0].Address, content: email.HTML, }; }); }); } };
428-430: Invitation link: use href extraction-export const getParsedEmailInvitationLink = () => - getLatestEmail().then( - (email) => email.HTML.replace(/.*(http:\/\/[\w:/]*).*/gs, '$1') as string - ); +export const getParsedEmailInvitationLink = () => + getLatestEmail().then((email) => (email.HTML.match(/href="([^"]+)"/i) || [,''])[1] as string);
433-453: Agency links: http-only and truncation; extract anchor hrefs insteadreturn getEmail(email.ID).then((e) => { - const links = Array.from( - e.HTML.matchAll(/(http:\/\/[\w:/]*)/g), - (m) => m[0] - ); + const links = Array.from(e.HTML.matchAll(/href="([^"]+)"/gi), (m) => m[1]); const invitation = links.find((l) => l.includes('accept_invitation')); const project = links.find( (l) => l.includes('/projects/') && !l.includes('/task') ); const tasks = links.filter((l) => l.includes('/task')); return { invitation, project, tasks, }; });
456-476: Order confirmation links: same href extraction issuereturn getEmail(email.ID).then((e) => { - const links = Array.from( - e.HTML.matchAll(/(http:\/\/[\w:/]*)/g), - (m) => m[0] - ); + const links = Array.from(e.HTML.matchAll(/href="([^"]+)"/gi), (m) => m[1]); const project = links.find( (l) => l.includes('/projects/') && !l.includes('/task') ); const tasks = links.filter((l) => l.includes('/task')); return { project, tasks, content: e.HTML, }; });
504-536: Make polling Cypress-native and broaden retries to reduce flakinessAvoid mixing Cypress commands inside Promises. Use a recursive Cypress chain and increase the retry window.
-export const getLatestEmail = (): Cypress.Chainable<EmailSummary> => { - const promise = new Cypress.Promise<EmailSummary>((resolve, reject) => { - const attempt = (count: number) => { - cy.request({ - url: 'http://localhost:21080/api/v1/message/latest', - failOnStatusCode: false, - }).then((r) => { - const body = r.body as EmailSummary | undefined; - const hasMessage = - r.status === 200 && body && (body.HTML || body.Subject); - - if (hasMessage) { - resolve(body!); - return; - } - - if (count < 3) { - cy.wait(250).then(() => attempt(count + 1)); - } else { - reject( - new Error( - `Failed to fetch latest email after ${count + 1} attempt(s).` - ) - ); - } - }); - }; - - attempt(0); - }); - - return cy.wrap(promise); -}; +export const getLatestEmail = ( + retries = 12, + delayMs = 500 +): Cypress.Chainable<EmailSummary> => { + const attempt = (left: number): Cypress.Chainable<EmailSummary> => { + return cy + .request({ url: 'http://localhost:21080/api/v1/message/latest', failOnStatusCode: false }) + .then((r) => { + const body = r.body as EmailSummary | undefined; + const hasMessage = r.status === 200 && body && (body.HTML || body.Subject); + if (hasMessage) return body!; + if (left <= 0) { + throw new Error(`Failed to fetch latest email after ${retries + 1} attempt(s).`); + } + return cy.wait(delayMs).then(() => attempt(left - 1)); + }); + }; + return attempt(retries); +};
549-555: Reset-password link: switch to href extractionreturn { - resetLink: r.HTML.replace(/.*(http:\/\/[\w:/=]*).*/gs, '$1'), + resetLink: (r.HTML.match(/href="([^"]+)"/i) || [,''])[1], fromAddress: r.From.Address, toAddress: r.To[0].Address, };email/components/ImgResource.ts (2)
26-29: Root-walk readability and edge-case safety.The assignment-in-condition trick works but is hard to read. Prefer an explicit 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: Avoid delete operator; destructure to omit fields.Use rest-spread to drop src/resourceName without delete (lint/perf).
- const { resourceName: _a, ...newProps } = props as Props; - delete newProps.src; + const { resourceName: _a, src: _unusedSrc, ...newProps } = props as Props;gradle/liquibase.gradle (1)
44-50: Configure existing Liquibase tasks instead of registering a duplicate.
diffChangeLogis provided by the plugin; registering a new one can collide. Also prefer provider-based dependsOn.- tasks.named('diff') { - dependsOn 'compileKotlin' - } + tasks.named('diff') { + dependsOn tasks.named('compileKotlin') + } - tasks.register('diffChangeLog') { - dependsOn 'compileKotlin' - } + tasks.named('diffChangeLog') { + dependsOn tasks.named('compileKotlin') + }email/HACKING.md (1)
135-135: Correct the security terminology: XSS, not RCE.As previously noted, HTML injection in emails leads to Cross-Site Scripting (XSS), not Remote Code Execution (RCE). XSS executes malicious scripts in the victim's email client/browser context, while RCE executes code on the server.
Apply this diff:
-- `dangerouslyInjectValueAsHtmlWithoutSanitization` (optional): whether to inject this variable as raw HTML. **VERY DANGEROUS. WILL LEAD TO <abbr title="Remote Code Execution">RCE</abbr> IF MISUSED.** Defaults to `false` +- `dangerouslyInjectValueAsHtmlWithoutSanitization` (optional): whether to inject this variable as raw HTML. **VERY DANGEROUS. WILL LEAD TO <abbr title="Cross-Site Scripting">XSS</abbr> ATTACKS IF MISUSED.** Defaults to `false`.github/workflows/release.yml (2)
85-90: Confirm Docker Hub attestation prerequisites are met.The workflow attempts to publish SLSA provenance attestations to Docker Hub using
push-to-registry: true. Verify that:
- The Docker Hub login step (lines 21-27 with
docker-hub: true) properly authenticates with write permissions- The workflow permissions (lines 14-17) include all required scopes
- The
subject-nameuses the fully qualified Docker Hub registry pathThe current configuration appears correct (permissions include
id-token: writeandattestations: write, subject-name usesdocker.io/prefix). However, confirm that thesetup-envaction's Docker Hub login provides write access to thetolgee/tolgeerepository.Run this script to verify the Docker Hub authentication configuration in the setup-env action:
#!/bin/bash # Verify setup-env action configures Docker Hub login with write permissions echo "=== Checking setup-env action for Docker Hub login configuration ===" cat .github/actions/setup-env/action.yaml | grep -A 20 "docker-hub"
61-70: Docker metadata tags will be empty without explicit version value.This workflow is triggered by
workflow_runon branches (not git tags), sodocker/metadata-actioncannot infer semver values from git tag context. Without an explicitvalueparameter,${{ steps.meta.outputs.tags }}will be empty, causing the build step to fail or produce untagged images.Apply this diff to pass the computed VERSION explicitly:
- 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}} + type=raw,value=${{ steps.version.outputs.VERSION }} + 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 }}email/components/layouts/ClassicLayout.tsx (1)
49-63: Accessibility: Social links lack accessible names for screen readers.The anchor created by
Linkcombines a visually hiddenspanwith anaria-hidden={true}image, resulting in an unlabeled link for assistive technology. Screen readers need an accessible name on the anchor itself.Apply this diff to fix the accessibility issue:
function SocialLink({ social, link, resourceName }: SocialLinkProps) { return ( <Text className="text-xs m-0"> - <Link href={link}> - <span className="hidden">{social}</span> + <Link href={link} aria-label={social}> <ImgResource className="mx-auto" resourceName={resourceName} - alt={social} - aria-hidden={true} + alt="" + role="presentation" /> </Link> </Text> ); }gradle/e2e.gradle (4)
101-108: Missing group assignment for consistency.The
printE2eGroupstask lacksgroup = 'e2e'while all other E2E tasks include it, creating inconsistency in task organization.tasks.register('printE2eGroups') { + group = 'e2e' doLast {
12-12: Configuration-time docker resolution lacks error handling and Windows compatibility.Line 12 executes
which dockerat configuration time without validation. This has several issu 32E1 es:
- Silent failure if docker is not in PATH
whichcommand doesn't exist on Windows- Configuration-time execution even when docker tasks aren't needed
Apply this diff to add platform-aware resolution with error handling:
-def dockerPath = ["which", "docker"].execute().text.trim() +def getDockerPath() { + def cmd = System.getProperty("os.name").toLowerCase().contains("windows") + ? ["where", "docker"] + : ["which", "docker"] + def path = cmd.execute().text.trim() + if (!path) { + throw new GradleException("docker executable not found in PATH. Ensure Docker is installed and available.") + } + return path +} +def dockerPath = getDockerPath()
58-58: Hardcoded "docker" bypasses dockerPath variable.Line 58 uses
new ProcessBuilder("docker", "compose", "logs", "-f")instead of thedockerPathvariable resolved at line 12, creating inconsistency with other tasks.Apply this diff:
- def pb = new ProcessBuilder("docker", "compose", "logs", "-f") + def pb = new ProcessBuilder(dockerPath, "compose", "logs", "-f")
110-114: saveServerLogs embeds docker command in bash string.Line 112 uses
"docker compose logs"in a bash string instead of leveraging thedockerPathvariable, bypassing the resolution and breaking on systems where docker isn't in PATH.Refactor to use
dockerPathwith Gradle's built-in output redirection:-tasks.register('saveServerLogs', Exec) { - group = 'e2e' - commandLine "bash", "-c", "docker compose logs > server.log" - workingDir E2E_DIR -} +tasks.register('saveServerLogs', Exec) { + group = 'e2e' + commandLine dockerPath, "compose", "logs" + workingDir E2E_DIR + standardOutput = new FileOutputStream("${E2E_DIR}/server.log") +}build.gradle (4)
71-73: Direct task reference should use lazy provider.Line 72 uses
mustRunAfter copyDistwhich can fail ifcopyDistis not yet configured. Use a lazy task provider instead.tasks.named("docker") { - mustRunAfter copyDist + mustRunAfter tasks.named("copyDist") }
85-93: Re-registering standard tasks causes collisions.Lines 85-93 attempt to register
buildandchecktasks which already exist in:server-app, causing task name collisions. Additionally, line 87 references undefinedrunE2evariable, and line 92 references undefinedktlintvariable.Configure existing tasks or create uniquely named aggregate tasks:
- tasks.register('build') { - dependsOn ':server-app:build' - dependsOn runE2e - } + tasks.named('build') { + dependsOn ':server-app:build' + dependsOn tasks.named('runE2e') + } - tasks.register('check') { - dependsOn ':server-app:check' - dependsOn ktlint - } + tasks.named('check') { + dependsOn ':server-app:check' + dependsOn tasks.named('ktlintCheck') + }Or create aggregate tasks with unique names:
- tasks.register('build') { + tasks.register('aggregateBuild') { dependsOn ':server-app:build' - dependsOn runE2e + dependsOn tasks.named('runE2e') }
95-97: bootJar re-registration shadows subproject task.Line 95 attempts to register
bootJarwhich already exists in:server-app, causing a collision.Use a unique aggregate task name:
- tasks.register('bootJar') { + tasks.register('aggregateBootJar') { dependsOn ':server-app:bootJar' }
119-146: Misuse of finalizedBy API and void method chaining.Lines 124-138 misuse the
finalizedByAPI by treating it as a mutable list with assignment and.add()calls. Lines 141-144 chain.systemProperty()calls which return void, causing compilation or runtime errors.Apply this diff to fix both issues:
tasks.register('diffChangeLog') { project(':server-app').tasks.named("bootRun") { mustRunAfter(startDbChangelogContainer) + systemProperty 'spring.profiles.active', 'dbschema' + systemProperty 'server.port', '61987' } - finalizedBy = [ - startDbChangelogContainer, - ':server-app:bootRun', - ':data:diffChangelog', - ] - + finalizedBy startDbChangelogContainer, ':server-app:bootRun', ':data:diffChangelog' + if (gradle.ext.billingAppDirectory.exists()) { - finalizedBy.add(':billing-app:diffChangelog') + finalizedBy ':billing-app:diffChangelog' } if (gradle.ext.eeAppDirectoryExists) { - finalizedBy.add(':ee-app:diffChangelog') + finalizedBy ':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') - } + finalizedBy stopDbChangelogContainer }
🧹 Nitpick comments (15)
email/env.d.ts (1)
17-24: Module augmentation correctly enables Thymeleaf attributes in React JSX.The implementation allows TypeScript to accept
th:*anddata-th-*attributes (e.g.,th:text,th:if,data-th-utext) in React components, which is necessary for the React Email → Thymeleaf template compilation workflow.Optional improvements:
- Add JSDoc documentation to clarify the purpose for future maintainers:
+/** + * Module augmentation to allow Thymeleaf template attributes in React JSX. + * Enables th:* and data-th-* attributes for templates compiled to Thymeleaf. + */ declare module 'react' { interface Attributes {
- Consider using
stringinstead ofunknownfor better type safety, since Thymeleaf attributes typically contain string expressions:declare module 'react' { interface Attributes { - [k: `th:${string}`]: unknown; - [k: `data-th-${string}`]: unknown; + [k: `th:${string}`]: string; + [k: `data-th-${string}`]: string; } }Note: This augmentation applies globally to all React components in the project, not just email templates. This appears intentional for flexibility across the email workspace.
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/organizationController/OrganizationControllerInvitingTest.kt (1)
165-167: LGTM! Correctly handles asynchronous email delivery.Wrapping
emailTestUtil.verifyEmailSent()in a retry mechanism is the right approach to accommodate asynchronous email processing introduced in this PR. The timeout (2s) and poll interval (25ms) are reasonable for test reliability.Optional: Consider extracting timeout constants.
If these values (2000ms, 25ms) are reused across multiple test files (as suggested by the summary mentioning similar updates in V2UserControllerTest, ProjectsControllerInvitationTest, etc.), consider extracting them as shared constants for easier maintenance.
Example:
// In a shared test utilities file object EmailTestConstants { const val EMAIL_VERIFICATION_TIMEOUT_MS = 2000L const val EMAIL_VERIFICATION_POLL_TIME_MS = 25L } // Usage in tests waitForNotThrowing( timeout = EmailTestConstants.EMAIL_VERIFICATION_TIMEOUT_MS, pollTime = EmailTestConstants.EMAIL_VERIFICATION_POLL_TIME_MS ) { emailTestUtil.verifyEmailSent() }Also applies to: 181-183
backend/app/src/test/kotlin/io/tolgee/service/notification/NotificationServiceTest.kt (1)
84-90: Good improvement for handling asynchronous email delivery.Wrapping the assertion in
waitForNotThrowingmakes the test more robust by polling for the email notification rather than failing immediately. The timeout and poll interval are reasonable.Optional: Consider extracting the timeout and poll time to constants if this pattern is widely used across tests:
companion object { private const val EMAIL_NOTIFICATION_TIMEOUT_MS = 2000L private const val EMAIL_NOTIFICATION_POLL_TIME_MS = 25L }Then use:
- waitForNotThrowing(timeout = 2000, pollTime = 25) { + waitForNotThrowing(timeout = EMAIL_NOTIFICATION_TIMEOUT_MS, pollTime = EMAIL_NOTIFICATION_POLL_TIME_MS) {e2e/cypress/common/apiCalls/common.ts (3)
371-377: Guard To[0] and consider consistent return keyTo avoid OOB when no recipients are present, guard To[0]. Also consider returning a consistent key name (content vs html) across helpers.
return { fromAddress: r.From.Address, - toAddress: r.To[0].Address, + toAddress: r.To?.[0]?.Address ?? '', subject: r.Subject, html: r.HTML, };
381-390: Don’t rely on anchor order; extract hrefs and filter by intentIndexing result[0] and result[2] is brittle. Extract all hrefs and pick by predicate (e.g., contains '/task' vs 'my-tasks').
- const content = r.HTML; - const result = [...content.matchAll(/href="(.*?)"/g)]; - return { - taskLink: result[0][1], - myTasksLink: result[2][1], + const hrefs = Array.from(r.HTML.matchAll(/href="([^"]+)"/gi), (m) => m[1]); + const taskLink = hrefs.find((h) => h.includes('/task')) || hrefs[0] || ''; + const myTasksLink = + hrefs.find((h) => h.includes('my-tasks') || h.includes('/tasks')) || + hrefs.find((h) => h.includes('/task')) || + hrefs[1] || + ''; + return { + taskLink, + myTasksLink, fromAddress: r.From.Address, toAddress: r.To[0].Address, content: r.HTML, };Optional shared helper (outside changed lines):
const extractHrefs = (html: string) => Array.from(html.matchAll(/href="([^"]+)"/gi), (m) => m[1]);
478-490: Tighten types for Email/EmailSummary (avoid any)Define small interfaces for addresses to make downstream code safer.
type Address = { Address: string; Name?: string }; type Email = { ID: string; To: Address[]; From: Address; Subject: string }; type EmailSummary = { HTML: string; Subject: string; To: Address[]; From: Address };.github/workflows/inactive-issues.yml (1)
13-30: Stale action v9: looks good; minor token polish.You can drop the explicit secret and use the built-in token for clarity.
- repo-token: ${{ secrets.GITHUB_TOKEN }} + repo-token: ${{ github.token }}backend/data/src/main/kotlin/io/tolgee/configuration/HibernateConfig.kt (1)
25-31: Guard optional dependency and avoid overriding existing inspector.Add ConditionalOnClass for QueryStackTraceLogger and only set the inspector if absent.
+import org.springframework.boot.autoconfigure.condition.ConditionalOnClass @@ -@Component -@Profile("hibernate-trace") +@Component +@Profile("hibernate-trace") +@ConditionalOnClass(QueryStackTraceLogger::class) class HibernateConfig : HibernatePropertiesCustomizer { override fun customize(hibernateProperties: MutableMap<String, Any>) { - hibernateProperties[AvailableSettings.STATEMENT_INSPECTOR] = QueryStackTraceLogger("io.tolgee") + hibernateProperties.putIfAbsent( + AvailableSettings.STATEMENT_INSPECTOR, + QueryStackTraceLogger("io.tolgee"), + ) } }email/components/ImgResource.ts (2)
50-53: Use correct MIME types (e.g., svg → image/svg+xml, jpg → image/jpeg).Deriving
image/${ext}is wrong for some formats; generate a proper content type.- const ext = extname(file).slice(1); - newProps.src = `data:image/${ext};base64,${blob.toString('base64')}`; + const ext = extname(file).slice(1).toLowerCase(); + const extToMime = (e: string) => + e === 'svg' ? 'image/svg+xml' + : e === 'jpg' ? 'image/jpeg' + : e === 'ico' ? 'image/x-icon' + : `image/${e}`; + const mime = extToMime(ext); + newProps.src = `data:${mime};base64,${blob.toString('base64')}`;
45-53: Optional: cache dev data URLs to avoid repeated disk IO.If the same resource is rendered multiple times, memoize the base64 string.
+const __cache = new Map<string, string>(); @@ - const blob = readFileSync(file); + const cached = __cache.get(file); + const blob = cached ? Buffer.from(cached, 'base64') : readFileSync(file); @@ - newProps.src = `data:${mime};base64,${blob.toString('base64')}`; + const b64 = blob.toString('base64'); + __cache.set(file, b64); + newProps.src = `data:${mime};base64,${b64}`;email/HACKING.md (1)
177-182: Minor: Use markdown link syntax for the URL.The bare URL at line 178 could use markdown link syntax for consistency with the rest of the document.
-- Get SVG from https://simpleicons.org/ +- Get SVG from [Simple Icons](https://simpleicons.org/)email/components/atoms/TolgeeLink.ts (1)
22-28: Consider using JSX syntax for better readability.While
React.createElementis correct, JSX syntax would be more idiomatic and readable. This is optional since the current implementation works correctly.export default function TolgeeLink(props: LinkProps) { const className = props.className ? `${LINK_CLASSES} ${props.className}` : LINK_CLASSES; - return React.createElement(Link, { ...props, className: className }); + return <Link {...props} className={className} />; }backend/data/src/main/kotlin/io/tolgee/email/EmailGlobalVariablesProvider.kt (1)
44-52: Consider logging URI parsing failures for debugging.The
URISyntaxExceptionis caught and swallowed silently. While the fallback toSELF_HOSTED_DEFAULT_QUALIFIERprovides safe default behavior, logging the exception would help administrators debug misconfiguredbackendUrlvalues.Apply this diff to add logging:
+import org.slf4j.LoggerFactory + @Component class EmailGlobalVariablesProvider( // Used to identify whether we're Tolgee Cloud or not private val billingConfigProvider: PublicBillingConfProvider, private val tolgeeProperties: TolgeeProperties, private val frontendUrlProvider: FrontendUrlProvider, ) { + private val logger = LoggerFactory.getLogger(javaClass) + operator fun invoke(): Map<String, Any?> { val isCloud = billingConfigProvider().enabled val backendUrl = tolgeeProperties.backEndUrl ?: frontendUrlProvider.url return mapOf( "isCloud" to isCloud, "instanceQualifier" to if (isCloud) tolgeeProperties.appName else backendUrl.intoQualifier(), "backendUrl" to backendUrl ) } private fun String?.intoQualifier(): String { return this?.let { try { return@let URI(it).host - } catch (_: URISyntaxException) { + } catch (e: URISyntaxException) { + logger.warn("Failed to parse backendUrl for email qualifier: {}", it, e) return@let null } } ?: SELF_HOSTED_DEFAULT_QUALIFIER }backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateConfig.kt (1)
59-61: Bidirectional dependency introduces tight coupling.Lines 59-61 create a circular reference where
EmailMessageResolverreceives thetemplateEngine, and then thetemplateEngineis configured with the resolver via a setter. This pattern creates tight coupling and makes the initialization order fragile.Consider refactoring to:
- Use a post-construct initialization method
- Pass the resolver during engine construction
- Or use a lazy initialization pattern to break the circular dependency
val resolver = EmailMessageResolver(messageResolver, templateEngine) - templateEngine.emailMessageResolver = resolver return resolverAnd handle the wiring in EmailTemplateEngine's initialization or via a separate configuration method that's called after both beans are constructed.
gradle/email.gradle (1)
21-25: Nested static directory may cause confusion.Line 24 uses a nested path
static/static/emailswhich could be confusing. The comment clarifies the intention, but this structure is unusual.Consider documenting this in the email build documentation or simplifying the resource structure if possible. The current approach works but may surprise future maintainers.
📜 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 (91)
.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/api/src/main/kotlin/io/tolgee/controllers/resetPassword/ResetPasswordRequestHandler.kt(2 hunks)backend/app/build.gradle(2 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/app/src/test/kotlin/io/tolgee/api/v2/controllers/V2UserControllerTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/organizationController/OrganizationControllerInvitingTest.kt(2 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ProjectsController/ProjectsControllerInvitationTest.kt(2 hunks)backend/app/src/test/kotlin/io/tolgee/controllers/resetPassword/ResetPasswordControllerTest.kt(2 hunks)backend/app/src/test/kotlin/io/tolgee/service/notification/NotificationServiceTest.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/component/email/EmailVerificationSender.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/component/email/InvitationEmailSender.kt(0 hunks)backend/data/src/main/kotlin/io/tolgee/component/email/TolgeeEmailSender.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/configuration/HibernateConfig.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/TolgeeProperties.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/misc/EmailParams.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/main/kotlin/io/tolgee/service/EmailVerificationService.kt(1 hunks)backend/data/src/main/resources/I18n_en.properties(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)backend/testing/src/main/kotlin/io/tolgee/fixtures/EmailTestUtil.kt(1 hunks)build.gradle(4 hunks)e2e/cypress/common/apiCalls/common.ts(1 hunks)e2e/cypress/e2e/notifications/notifications.cy.ts(2 hunks)e2e/cypress/e2e/organizations/organizationInvitations.cy.ts(3 hunks)e2e/docker-compose.yml(1 hunks)ee/backend/app/build.gradle(2 hunks)ee/backend/tests/build.gradle(2 hunks)ee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/task/TaskControllerTest.kt(2 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/__tests__/test-email.tsx(1 hunks)email/emails/default.tsx(1 hunks)email/emails/registration-confirm.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.properties(1 hunks)gradle/docker.gradle(4 hunks)gradle/e2e.gradle(7 hunks)gradle/email.gradle(1 hunks)gradle/liquibase.gradle(1 hunks)gradle/webapp.gradle(3 hunks)settings.gradle(1 hunks)webapp/.tolgeerc.json(1 hunks)
💤 Files with no reviewable changes (4)
- .gitmodules
- .github/workflows/prerelease-alpha.yml
- .github/workflows/preview.yml
- backend/data/src/main/kotlin/io/tolgee/component/email/InvitationEmailSender.kt
✅ Files skipped from review due to trivial changes (2)
- backend/data/src/main/kotlin/io/tolgee/activity/EntityDescriptionProvider.kt
- backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/PatControllerTest.kt
🚧 Files skipped from review as they are similar to previous changes (41)
- email/tsconfig.json
- backend/data/src/main/kotlin/io/tolgee/activity/iterceptor/InterceptedEventsManager.kt
- backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ProjectsController/ProjectsControllerInvitationTest.kt
- .github/workflows/reportIntermittentTests.yml
- backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/V2UserControllerTest.kt
- ee/backend/tests/src/test/kotlin/io/tolgee/ee/selfHostedLimitsAndReporting/KeyCountLimitTest.kt
- backend/data/src/main/resources/I18n_en.properties
- backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/TolgeeProperties.kt
- gradle.properties
- backend/testing/src/main/kotlin/io/tolgee/fixtures/EmailTestUtil.kt
- email/components/atoms/TolgeeButton.ts
- e2e/cypress/e2e/organizations/organizationInvitations.cy.ts
- backend/data/src/main/kotlin/io/tolgee/dtos/misc/EmailParams.kt
- email/components/Var.ts
- ee/backend/tests/src/test/kotlin/io/tolgee/ee/slack/SlackWithBatchOperationTest.kt
- email/.gitignore
- email/emails/default.tsx
- .github/actions/upload-backend-build/action.yaml
- webapp/.tolgeerc.json
- backend/ktlint/build.gradle
- email/tailwind.config.ts
- backend/data/src/test/resources/email-i18n-test/messages_en.properties
- e2e/docker-compose.yml
- backend/api/src/main/kotlin/io/tolgee/controllers/resetPassword/ResetPasswordRequestHandler.kt
- email/.prettierrc.json
- .github/actions/download-backend-build/action.yaml
- backend/development/build.gradle
- backend/data/src/test/kotlin/io/tolgee/service/machineTranslation/MtBatchTranslatorTest.kt
- backend/data/src/main/kotlin/io/tolgee/pubSub/RedisPubSubReceiver.kt
- backend/data/src/main/kotlin/io/tolgee/component/email/EmailVerificationSender.kt
- email/.eslintrc.json
- backend/data/src/test/kotlin/io/tolgee/email/EmailServiceTest.kt
- backend/app/build.gradle
- e2e/cypress/e2e/notifications/notifications.cy.ts
- backend/testing/build.gradle
- backend/security/build.gradle
- email/components/translate.ts
- backend/data/src/test/kotlin/io/tolgee/email/EmailGlobalVariablesProviderTest.kt
- DEVELOPMENT.md
- email/.config/tolgeerc.json
- backend/misc/build.gradle
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-06-16T20:27:09.537Z
Learnt from: cyyynthia
PR: tolgee/tolgee-platform#1987
File: email/package.json:38-41
Timestamp: 2025-06-16T20:27:09.537Z
Learning: Node.js v16.9.0+ supports wildcard subpath patterns in the "exports" field of package.json. Patterns like "./components/*.ts": "./components/*.ts" are valid syntax and are the recommended approach for exposing multiple files. This replaced the deprecated folder export syntax.
Applied to files:
email/package.json
📚 Learning: 2025-06-16T20:38:38.052Z
Learnt from: cyyynthia
PR: tolgee/tolgee-platform#1987
File: email/components/If.ts:38-47
Timestamp: 2025-06-16T20:38:38.052Z
Learning: React treats null, undefined, and false as "holes" in JSX trees and doesn't render anything in their place. When returning arrays containing null elements, React will skip over the null values during rendering, so filtering them out is unnecessary.
Applied to files:
email/components/If.ts
📚 Learning: 2025-06-16T20:58:15.906Z
Learnt from: cyyynthia
PR: tolgee/tolgee-platform#1987
File: email/components/For.ts:25-38
Timestamp: 2025-06-16T20:58:15.906Z
Learning: In the Tolgee email template system, the For component intentionally uses React.ReactElement type to restrict children to a single child for easier cloning, and uses Math.random() for keys in development mode following a "quick and dirty" approach that's good enough for development-only workloads. The maintainer prefers KISS (Keep It Simple, Stupid) principle over perfect code quality for development-only components.
Applied to files:
email/components/For.ts
📚 Learning: 2025-06-11T16:17:35.696Z
Learnt from: cyyynthia
PR: tolgee/tolgee-platform#3126
File: backend/testing/build.gradle:78-78
Timestamp: 2025-06-11T16:17:35.696Z
Learning: The Gradle module `backend/testing` serves as an internal test pseudo-library; therefore, test-scope dependencies such as `org.springframework.boot:spring-boot-starter-test` are intentionally declared with `implementation`, as the module is only consumed on the test classpath and is not part of production artifacts.
Applied to files:
ee/backend/tests/build.gradlebackend/data/build.gradle
📚 Learning: 2025-06-16T20:29:21.117Z
Learnt from: cyyynthia
PR: tolgee/tolgee-platform#1987
File: email/.config/extractor.ts:114-124
Timestamp: 2025-06-16T20:29:21.117Z
Learning: The t() function in email/.../translate.ts requires both keyName and defaultValue arguments by design. The Tolgee CLI extractor assumes valid usage and does not need to handle cases where the second argument is missing, as such code would be invalid according to the API contract.
Applied to files:
email/.config/extractor.ts
📚 Learning: 2025-06-17T13:37:28.583Z
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.583Z
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.
Applied to files:
backend/data/src/main/kotlin/io/tolgee/email/EmailMessageResolver.kt
🧬 Code graph analysis (6)
email/emails/registration-confirm.tsx (6)
email/components/layouts/ClassicLayout.tsx (1)
ClassicLayout(65-190)email/components/translate.ts (1)
t(203-222)email/components/LocalizedText.ts (1)
LocalizedText(27-29)email/components/If.ts (1)
If(25-70)email/components/atoms/TolgeeButton.ts (1)
TolgeeButton(23-29)email/components/atoms/TolgeeLink.ts (1)
TolgeeLink(22-28)
email/components/layouts/ClassicLayout.tsx (5)
email/components/translate.ts (2)
TranslatedText (24-28)t(203-222)email/components/ImgResource.ts (1)
ImgResource(39-56)email/components/layouts/LayoutCore.tsx (1)
LayoutCore(30-42)email/components/If.ts (1)
If(25-70)email/components/LocalizedText.ts (1)
LocalizedText(27-29)
email/components/layouts/LayoutCore.tsx (1)
email/components/translate.ts (1)
TranslatedText(24-28)
email/emails/__tests__/test-email.tsx (5)
email/components/layouts/ClassicLayout.tsx (1)
ClassicLayout(65-190)email/components/Var.ts (1)
Var(25-39)email/components/atoms/TolgeeLink.ts (1)
TolgeeLink(22-28)email/components/If.ts (1)
If(25-70)email/components/For.ts (1)
For(25-39)
backend/data/src/main/kotlin/io/tolgee/email/EmailService.kt (1)
backend/data/src/main/kotlin/io/tolgee/component/email/TolgeeEmailSender.kt (1)
sendEmail(12-27)
email/components/LocalizedText.ts (1)
email/components/translate.ts (1)
MessageProps(34-37)
🪛 Biome (2.1.2)
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)
🪛 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)
🪛 LanguageTool
email/HACKING.md
[grammar] ~1-~1: There might be a mistake here.
Context: # Guide to writing emails for Tolgee This is a resource helpful for people co...
(QB_NEW_EN)
[grammar] ~2-~2: There might be a mistake here.
Context: ...face the need to create new emails: this document is a quick summary of how to wr...
(QB_NEW_EN)
[grammar] ~3-~3: There might be a mistake here.
Context: ...familiar with the internal tools and the expected way emails should be written. ...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...sing clear JSX syntax, which gets turned into HTML code tailored specifically for...
(QB_NEW_EN)
[grammar] ~13-~13: Use a hyphen to join words.
Context: ...ail/docs/introduction). If you need real world examples, they provide a bunch of ...
(QB_NEW_EN_HYPHEN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...based on real-world emails written using React Email [here](https://demo.react.em...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...see how it looks still without resorting to manual testing within Tolgee itself. ...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...g/doc/tutorials/3.1/usingthymeleaf.html) templates in the out folder that the b...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ...kend at /static/emails. Filenames must be preserved. > [!NOTE] > The backend b...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...f or copy files around. ### TailwindCSS For styles, React Email has a great [Tai...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ....com/) integration that gets turned into email-friendly inline styles. When usin...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...ns, no flexbox, no grid, and pretty much anything that's cool in [CURRENT_YEAR]....
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...ail.com/) is a good resource for what is fine to send and what isn't; basically t...
(QB_NEW_EN)
[grammar] ~43-~43: There might be a mistake here.
Context: ... Row and Column elements for layout. They'll get turned into ugly HTML tables...
(QB_NEW_EN)
[grammar] ~46-~46: There might be a mistake here.
Context: ...e in the good ol' HTML days... > [!TIP] > Recent versions of React Email have an...
(QB_NEW_EN)
[grammar] ~52-~52: There might be a mistake here.
Context: ...ut.tsx`. All emails should use a layout, or at least must use the core layout as ...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ...ect the reason why a given email is sent - Is it because they have an account? Is i...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ...w()` function documented below. The core layout only requires the subject. ## Ut...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...ires the subject. ## Utility components This is note is left here for the lack o...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ...eed a dynamic properties (e.g. href that takes the value of a variable), you can ...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ...th data-th- and setting the value to a [Thymeleaf Standard Expression](https://...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...xt />(ort()when more appropriate). They are equivalent :...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...nslation to be plaintext (no HTML). It's mainly used for the subject part and the...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...e custom message source adds support for FormatJS-like XML within the message. >...
(QB_NEW_EN)
[grammar] ~87-~87: There might be a mistake here.
Context: ...S-like XML within the message. > [!TIP] > b, i, u, em, and strong tags...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ...ed key, such as item.name use a double underscore: item__name. The `<Localiz...
(QB_NEW_EN)
[grammar] ~95-~95: There might be a mistake here.
Context: ...Value` (required): Default string to use - Will be used by the CLI to push default ...
(QB_NEW_EN)
[grammar] ~103-~103: There might be a mistake here.
Context: ...arguments in the order they're described here. > [!WARNING] > When using the dev...
(QB_NEW_EN)
[grammar] ~123-~123: There might be a mistake here.
Context: ... by the previewer, and must be correctly handled by the renderer (by replacing al...
(QB_NEW_EN)
[grammar] ~126-~126: There might be a mistake here.
Context: ...ing, by making sure variables are always escaped by default (via Var or an ICU ...
(QB_NEW_EN)
[grammar] ~138-~138: There might be a mistake here.
Context: ... and then this component should be used. It functions like [React Email's `<Img /...
(QB_NEW_EN)
[grammar] ~139-~139: There might be a mistake here.
Context: ...ponents/image), except it doesn't take a src prop but a resource, that should...
(QB_NEW_EN)
[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)
[grammar] ~145-~145: There might be a mistake here.
Context: ...rably not modified. Doing so would alter previously sent emails, by modifying ima...
(QB_NEW_EN)
[grammar] ~149-~149: There might be a mistake here.
Context: ...ionally showing something else instead). This component takes exactly one or two ...
(QB_NEW_EN)
[grammar] ~158-~158: There might be a mistake here.
Context: ...ach element of the array and produce the inner HTML for each element of the array...
(QB_NEW_EN)
[grammar] ~167-~167: There might be a mistake here.
Context: ...development preview ## Global variables The following global variables are avail...
(QB_NEW_EN)
[grammar] ~169-~169: There might be a mistake here.
Context: ...an): Whether this is Tolgee Cloud or not - instanceQualifier: Either "Tolgee" for Tolgee Cloud, or t...
(QB_NEW_EN)
[grammar] ~170-~170: There might be a mistake here.
Context: ...or the domain name used for the instance - backendUrl: Base URL of the backend They still ne...
(QB_NEW_EN)
[grammar] ~173-~173: There might be a mistake here.
Context: ...ngs as a default value is provided then. The default value can be overridden. ##...
(QB_NEW_EN)
[grammar] ~177-~177: There might be a mistake here.
Context: ...cks How the social icons were generated: - Get SVG from https://simpleicons.org/ - ...
(QB_NEW_EN)
[grammar] ~178-~178: There might be a mistake here.
Context: ... - Get SVG from https://simplei
D8DF
cons.org/ - Write to [file].svg - Add `width="16" ...
(QB_NEW_EN)
[grammar] ~179-~179: There might be a mistake here.
Context: ...rom https://simpleicons.org/ - Write to [file].svg - Add `width="16" height="16" fill="#a0a0a...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
email/HACKING.md
14-14: Link text should be descriptive
(MD059, descriptive-link-text)
16-16: Link text should be descriptive
(MD059, descriptive-link-text)
178-178: Bare URL used
(MD034, no-bare-urls)
⏰ 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). (2)
- GitHub Check: Build backend 🏗️
- GitHub Check: Frontend static check 🪲
| fun sendEmail(params: EmailParams) { | ||
| validateProps() | ||
| val helper = mimeMessageHelperFactory.create() | ||
| helper.setFrom(params.from ?: tolgeeProperties.smtp.from!!) | ||
| helper.setTo(params.to) | ||
| params.replyTo?.let { | ||
| helper.setReplyTo(it) | ||
| } | ||
| if (!params.bcc.isNullOrEmpty()) { | ||
| helper.setBcc(params.bcc!!) | ||
| } | ||
| helper.setSubject(params.subject) | ||
| val content = | ||
| """ | ||
| <html> | ||
| <body style="font-size: 15px"> | ||
| ${params.text}<br/><br/> | ||
| <img style="max-width: 100%; width:120px" src="cid:logo.png" /> | ||
| </body> | ||
| </html> | ||
| """.trimIndent() | ||
| helper.setText(content, true) | ||
|
|
||
| params.attachments.forEach { | ||
| helper.addAttachment(it.name, it.inputStreamSource) | ||
| } | ||
|
|
||
| helper.addInline( | ||
| "logo.png", | ||
| { ClassPathResource("tolgee-logo.png").inputStream }, | ||
| "image/png", | ||
| val properties = mapOf<String, Any>() | ||
| .let { if (params.text != null) it.plus("content" to params.text!!) else it } | ||
| .let { if (params.header != null) it.plus("header" to params.header!!) else it } | ||
| .let { if (params.recipientName != null) it.plus("recipientName" to params.recipientName!!) else it } | ||
| emailService.sendEmailTemplate( | ||
| recipient = params.to, | ||
| subject = params.subject, | ||
| template = params.templateName ?: "default", | ||
| locale = Locale.ENGLISH, | ||
| properties = properties, | ||
| attachments = params.attachments, | ||
| bcc = params.bcc, | ||
| replyTo = params.replyTo, | ||
| ) | ||
|
|
||
| mailSender.send(helper.mimeMessage) | ||
| } | ||
|
|
||
| private fun validateProps() { | ||
| if (tolgeeProperties.smtp.from.isNullOrEmpty()) { | ||
| throw IllegalStateException( | ||
| """tolgee.smtp.from property not provided. | ||
| |You have to configure smtp properties to send an e-mail. | ||
| """.trimMargin(), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| fun getSignature(): String { | ||
| return """ | ||
| <br /><br /> | ||
| Best regards, | ||
| <br /> | ||
| Tolgee Team | ||
| """.trimIndent() | ||
| } |
There was a problem hiding this comment.
Hardcoded locale defeats i18n functionality.
Line 21 hardcodes Locale.ENGLISH for all outbound emails, which prevents the email system from respecting user language preferences. For a platform with comprehensive i18n support (ICU message formatting, translated templates), this is a significant limitation.
Add a locale or languageTag field to EmailParams, or resolve the locale from the request/user context:
fun sendEmail(params: EmailParams) {
val locale = params.locale ?: Locale.ENGLISH // or resolve from context
val properties = mapOf<String, Any>()
.let { if (params.text != null) it.plus("content" to params.text!!) else it }
.let { if (params.header != null) it.plus("header" to params.header!!) else it }
.let { if (params.recipientName != null) it.plus("recipientName" to params.recipientName!!) else it }
emailService.sendEmailTemplate(
recipient = params.to,
subject = params.subject,
template = params.templateName ?: "default",
locale = locale, // Use resolved locale
properties = properties,
attachments = params.attachments,
bcc = params.bcc,
replyTo = params.replyTo,
)
}Update EmailParams in backend/data/src/main/kotlin/io/tolgee/dtos/misc/EmailParams.kt:
data class EmailParams(
// ... existing fields ...
val locale: Locale? = null // Add locale field
)🤖 Prompt for AI Agents
backend/data/src/main/kotlin/io/tolgee/component/email/TolgeeEmailSender.kt
lines 12-27: the code hardcodes Locale.ENGLISH when calling
emailService.sendEmailTemplate; add a nullable locale to EmailParams
(backend/data/src/main/kotlin/io/tolgee/dtos/misc/EmailParams.kt) and use it
here (e.g. val locale = params.locale ?: resolveFromContext() ?: Locale.ENGLISH)
then pass locale to emailService.sendEmailTemplate instead of Locale.ENGLISH;
ensure you import java.util.Locale and update all call sites of EmailParams to
supply locale or rely on default.
| @Bean("emailIcuMessageSource") | ||
| fun messageSource(): MessageSource { | ||
| val messageSource = ICUReloadableResourceBundleMessageSource() | ||
| messageSource.setBasenames("classpath:email-i18n/messages", "email-i18n-test/messages") |
There was a problem hiding this comment.
Test-related basename in production configuration.
The basename "email-i18n-test/messages" appears to include test data in the production message source configuration. This could lead to unintended test messages being available in production or classpath pollution.
Consider either:
- Removing the test basename from production configuration
- Moving test-specific message source configuration to a test-only @configuration class
- messageSource.setBasenames("classpath:email-i18n/messages", "email-i18n-test/messages")
+ messageSource.setBasenames("classpath:email-i18n/messages")📝 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.
| messageSource.setBasenames("classpath:email-i18n/messages", "email-i18n-test/messages") | |
| messageSource.setBasenames("classpath:email-i18n/messages") |
🤖 Prompt for AI Agents
In backend/data/src/main/kotlin/io/tolgee/email/EmailTemplateConfig.kt around
line 45, the messageSource is registering a test-specific basename
("email-i18n-test/messages") in production configuration; remove that test
basename from this production config or move the test message-source
registration into a test-only @Configuration (e.g., a class annotated with
@TestConfiguration or a test resources setup) so production only loads the
intended "classpath:email-i18n/messages".
| function toString(node: Expression | JSXAttrValue): string | null { | ||
| switch (node.type) { | ||
| case 'StringLiteral': | ||
| return node.value; | ||
| case 'JSXText': | ||
| return node.value.trim(); | ||
| case 'JSXExpressionContainer': | ||
| return toString(node.expression); | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename function to avoid shadowing global toString.
The function name toString shadows the global toString property, which can cause confusion. Consider renaming to something more descriptive like nodeToString or extractStringValue.
-function toString(node: Expression | JSXAttrValue): string | null {
+function nodeToString(node: Expression | JSXAttrValue): string | null {
switch (node.type) {
case 'StringLiteral':
return node.value;
case 'JSXText':
return node.value.trim();
case 'JSXExpressionContainer':
- return toString(node.expression);
+ return nodeToString(node.expression);
}
return null;
}Then update the function calls at lines 67 and 68:
- const keyName = toString(keyNameExpr);
- const defaultValue = defaultValueExpr ? toString(defaultValueExpr) : null;
+ const keyName = nodeToString(keyNameExpr);
+ const defaultValue = defaultValueExpr ? nodeToString(defaultValueExpr) : null;Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[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)
🤖 Prompt for AI Agents
In email/.config/extractor.ts around lines 45 to 56, the helper function named
`toString` shadows the global `toString`; rename it to a descriptive name (e.g.,
`nodeToString` or `extractStringValue`) and update its internal references
accordingly, then update the two call sites at lines 67 and 68 to use the new
name so all usages match the renamed function.
| if (children[1].type !== If.Else) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| 'Warning: not using <If.Else /> as second child of <If /> is discouraged.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Guard against undefined access when checking second child.
If only one child is provided, children[1] is undefined, and accessing .type on line 39 will throw a runtime error before the warning can be logged.
Apply this diff to add a length check:
- if (children[1].type !== If.Else) {
+ if (children.length === 2 && children[1].type !== If.Else) {
// eslint-disable-next-line no-console
console.warn(
'Warning: not using <If.Else /> as second child of <If /> is discouraged.'
);
}📝 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.
| if (children[1].type !== If.Else) { | |
| // eslint-disable-next-line no-console | |
| console.warn( | |
| 'Warning: not using <If.Else /> as second child of <If /> is discouraged.' | |
| ); | |
| } | |
| if (children.length === 2 && children[1].type !== If.Else) { | |
| // eslint-disable-next-line no-console | |
| console.warn( | |
| 'Warning: not using <If.Else /> as second child of <If /> is discouraged.' | |
| ); | |
| } |
🤖 Prompt for AI Agents
In email/components/If.ts around lines 39 to 44, the code accesses
children[1].type without ensuring a second child exists which can throw if only
one child is provided; change the check to first normalize/inspect children
safely (e.g. const kids = Array.isArray(children) ? children : [children] or use
React.Children.toArray(children)) and then only access kids[1] after confirming
kids.length > 1 — i.e. if (kids.length > 1 && kids[1].type !== If.Else) {
console.warn(...) } so the warning is logged safely without risking undefined
property access.
| root !== (root = join(root, '..')) | ||
| ); | ||
|
|
||
| const RESOURCES_FOLDER = join(root, 'resources'); |
There was a problem hiding this comment.
Fail fast if resources folder or file is missing.
Currently, if no ancestor has 'resources', RESOURCES_FOLDER points to '/resources' (or 'C:\resources') and readFileSync will throw later. Check existence early with a clear error.
-import { readdirSync, readFileSync } from 'fs';
+import { readdirSync, readFileSync, existsSync } from 'fs';
@@
-const RESOURCES_FOLDER = join(root, 'resources');
+const RESOURCES_FOLDER = join(root, 'resources');
+if (!existsSync(RESOURCES_FOLDER)) {
+ throw new Error(`ImgResource: resources folder not found starting from ${__dirname}`);
+}
@@
- const file = join(RESOURCES_FOLDER, props.resourceName);
+ const file = join(RESOURCES_FOLDER, props.resourceName);
+ if (!existsSync(file)) {
+ throw new Error(`ImgResource: resource "${props.resourceName}" not found in ${RESOURCES_FOLDER}`);
+ }Also applies to: 40-41
🤖 Prompt for AI Agents
In email/components/ImgResource.ts around lines 31 and 40-41, RESOURCES_FOLDER
is computed as join(root, 'resources') but if no ancestor contains a resources
folder it will resolve to the filesystem root (e.g. "/resources") and cause a
cryptic readFileSync error later; update the file to immediately verify that
RESOURCES_FOLDER exists and is a directory and throw a clear, fast-fail error if
not (include the resolved path in the message), and likewise before reading any
specific resource file (lines ~40-41) resolve the full file path, check that the
file exists and is readable, and if not throw a clear error (or wrap
readFileSync in a try/catch that rethrows a descriptive error with the file
path).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/cypress/common/apiCalls/common.ts (1)
333-341: Deletes are not awaited; forEach + cy chain equals flakiness.forEach returns void; callers can proceed before deletes finish.
export const deleteAllProjectApiKeys = (projectId: number) => getAllProjectApiKeys(projectId).then((keys) => { - return keys.forEach((k) => - v2apiFetch(`api-keys/${k.id}`, { - method: 'DELETE', - }) - ); + // Use Cypress chaining to ensure completion + return cy.wrap(keys).each((k) => + v2apiFetch(`api-keys/${k.id}`, { method: 'DELETE' }) + ); });
♻️ Duplicate comments (7)
e2e/cypress/common/apiCalls/common.ts (7)
427-434: Prefer href extraction and filter by path for invitation.Regex is still brittle; preserve https and querystring.
export const getParsedEmailInvitationLink = () => - getLatestEmail().then( - (email) => - email.HTML.replace( - /.*(https?:\/\/[\w:/.-]*accept_invitation[\w:/.-]*).*/gs, - '$1' - ) as string - ); + getLatestEmail().then((email) => { + const links = Array.from(email.HTML.matchAll(/href="([^"]+)"/gi), (m) => m[1]); + return links.find((l) => l.includes('accept_invitation')) || ''; + });
394-401: Regex drops https and query/hash; extract href instead.Current replace() only matches http:// and truncates queries, breaking verification links.
export const getParsedEmailVerification = () => getLatestEmail().then((r) => { return { - verifyEmailLink: r.HTML.replace(/.*(http:\/\/[\w:/]*).*/gs, '$1'), + verifyEmailLink: (r.HTML.match(/href="([^"]+)"/i) || [,''])[1], fromAddress: r.From.Address, toAddress: r.To[0].Address, content: r.HTML, }; });
403-425: Add index bounds check and fix link extraction.Out-of-range index throws; link regex has same http-only bug.
export const getParsedEmailVerificationByIndex = (index: number) => { if (index === 0) { return getLatestEmail().then((email) => { return { - verifyEmailLink: email.HTML.replace(/.*(http:\/\/[\w:/]*).*/gs, '$1'), + verifyEmailLink: (email.HTML.match(/href="([^"]+)"/i) || [,''])[1], fromAddress: email.From.Address, toAddress: email.To[0].Address, content: email.HTML, }; }); } else { - return getAllEmails().then((emails) => { - return getEmail(emails[index].ID).then((email) => { + return getAllEmails().then((emails) => { + if (!emails || index < 0 || index >= emails.length) { + throw new Error(`Email at index ${index} not found. Total: ${emails?.length ?? 0}`); + } + return getEmail(emails[index].ID).then((email) => { return { - verifyEmailLink: email.HTML.replace(/.*(http:\/\/[\w:/]*).*/gs, '$1'), + verifyEmailLink: (email.HTML.match(/href="([^"]+)"/i) || [,''])[1], fromAddress: email.From.Address, toAddress: email.To[0].Address, content: email.HTML, }; }); }); } };
439-457: Guard for missing email and fix link extraction.If the email with expected subject isn’t found, getEmail(email.ID) will crash. Link regex also misses https and params.
export const getAgencyInvitationLinks = () => getAllEmails().then((emails) => { - const email = emails.find((e) => - e.Subject.includes('New translation request') - ); - return getEmail(email.ID).then((e) => { - const links = Array.from( - e.HTML.matchAll(/(http:\/\/[\w:/]*)/g), - (m) => m[0] - ); + const email = emails.find((e) => e.Subject.includes('New translation request')); + if (!email) { + throw new Error('Agency invitation email not found.'); + } + return getEmail(email.ID).then((e) => { + const links = Array.from(e.HTML.matchAll(/href="([^"]+)"/gi), (m) => m[1]); - const invitation = links.find((l) => l.includes('accept_invitation')); - const project = links.find( - (l) => l.includes('/projects/') && !l.includes('/task') - ); - const tasks = links.filter((l) => l.includes('/task')); + const invitation = links.find((l) => l.includes('accept_invitation')); + const project = links.find((l) => l.includes('/projects/') && !l.includes('/task')); + const tasks = links.filter((l) => /\/task(s|\/)/.test(l)); return { invitation, project, tasks, }; }); });
463-479: Same here: guard missing email and use hrefs.Avoid crash on missing subject and keep full URLs.
export const getOrderConfirmation = () => getAllEmails().then((emails) => { - const email = emails.find((e) => - e.Subject.includes('Your translation order to') - ); - return getEmail(email.ID).then((e) => { - const links = Array.from( - e.HTML.matchAll(/(http:\/\/[\w:/]*)/g), - (m) => m[0] - ); + const email = emails.find((e) => e.Subject.includes('Your translation order to')); + if (!email) { + throw new Error('Order confirmation email not found.'); + } + return getEmail(email.ID).then((e) => { + const links = Array.from(e.HTML.matchAll(/href="([^"]+)"/gi), (m) => m[1]); const project = links.find( (l) => l.includes('/projects/') && !l.includes('/task') ); - const tasks = links.filter((l) => l.includes('/task')); + const tasks = links.filter((l) => /\/task(s|\/)/.test(l)); return { project, tasks, content: e.HTML, }; }); });
508-541: Make polling Cypress-native and increase retry window.Avoid cy.* inside a Promise; extend retries to reduce flakiness.
-export const getLatestEmail = (): Cypress.Chainable<EmailSummary> => { - const promise = new Cypress.Promise<EmailSummary>((resolve, reject) => { - const attempt = (count: number) => { - cy.request({ - url: 'http://localhost:21080/api/v1/message/latest', - failOnStatusCode: false, - }).then((r) => { - const body = r.body as EmailSummary | undefined; - const hasMessage = - r.status === 200 && body && (body.HTML || body.Subject); - - if (hasMessage) { - resolve(body!); - return; - } - - if (count < 3) { - cy.wait(250).then(() => attempt(count + 1)); - } else { - reject( - new Error( - `Failed to fetch latest email after ${count + 1} attempt(s).` - ) - ); - } - }); - }; - - attempt(0); - }); - - return cy.wrap(promise); -}; +export const getLatestEmail = ( + retries = 12, + delayMs = 500 +): Cypress.Chainable<EmailSummary> => { + const attempt = (left: number): Cypress.Chainable<EmailSummary> => { + return cy + .request({ url: `${EMAIL_API}/message/latest`, failOnStatusCode: false }) + .then((r) => { + const body = r.body as EmailSummary | undefined; + const hasMessage = r.status === 200 && body && (body.HTML || body.Subject); + if (hasMessage) return body!; + if (left <= 0) { + throw new Error(`Failed to fetch latest email after ${retries + 1} attempt(s).`); + } + return cy.wait(delayMs).then(() => attempt(left - 1)); + }); + }; + return attempt(retries); +};
553-560: Reset-password link has same http-only regex issue.Use href extraction to keep https and query params.
export const getParsedResetPasswordEmail = () => getLatestEmail().then((r) => { return { - resetLink: r.HTML.replace(/.*(http:\/\/[\w:/=]*).*/gs, '$1'), + resetLink: (r.HTML.match(/href="([^"]+)"/i) || [,''])[1], fromAddress: r.From.Address, toAddress: r.To[0].Address, }; });
🧹 Nitpick comments (4)
e2e/cypress/common/apiCalls/common.ts (4)
381-390: Avoid relying on anchor order; pick links by intent.Selecting result[0]/[2] is brittle. Extract hrefs, then choose by path to stabilize tests.
export const getAssignedEmailNotification = () => - getLatestEmail().then((r) => { - const content = r.HTML; - const result = [...content.matchAll(/href="(.*?)"/g)]; - return { - taskLink: result[0][1], - myTasksLink: result[2][1], + getLatestEmail().then((r) => { + const links = Array.from(r.HTML.matchAll(/href="([^"]+)"/gi), (m) => m[1]); + const taskLink = links.find((l) => /\/task(s|\/)/i.test(l)) || ''; + const myTasksLink = links.find((l) => /\/my[-_]tasks/i.test(l)) || ''; + return { + taskLink, + myTasksLink, fromAddress: r.From.Address, toAddress: r.To[0].Address, content: r.HTML, }; });Optional helper (outside this hunk):
const extractHrefs = (html: string) => Array.from(html.matchAll(/href="([^"]+)"/gi), (m) => m[1]);
483-494: Tighten types for Email addresses (optional).To/From as any hides shape; declare a minimal address type.
-type Email = { - ID: string; - To: any; - From: any; - Subject: string; -}; +type EmailAddress = { Address: string }; +type Email = { + ID: string; + To: EmailAddress[]; + From: EmailAddress; + Subject: string; +}; -type EmailSummary = { - HTML: string; - Subject: string; - To: any; - From: any; -}; +type EmailSummary = { + HTML: string; + Subject: string; + To: EmailAddress[]; + From: EmailAddress; +};
496-504: Hard-coded email API base URL; move to Cypress env.Parameterize to run locally and in CI without edits.
+// top-level (outside functions) +const EMAIL_API = Cypress.env('EMAIL_API_URL') ?? 'http://localhost:21080/api/v1'; + function fetchEmails(limit = 0) { - let options = { url: 'http://localhost:21080/api/v1/messages' }; + let options = { url: `${EMAIL_API}/messages` }; if (limit) { options = { ...options, ...{ qs: { limit } } }; } return cy.request(options).then((r) => { return r.body.messages as Email[]; }); } export const getEmail = (id) => - cy - .request({ url: `http://localhost:21080/api/v1/message/${id}` }) + cy + .request({ url: `${EMAIL_API}/message/${id}` }) .then((r) => r.body as EmailSummary); export const deleteAllEmails = () => - cy.request({ - url: 'http://localhost:21080/api/v1/messages', + cy.request({ + url: `${EMAIL_API}/messages`, method: 'DELETE', });Also applies to: 542-546, 548-551
542-546: Minor: type id parameter and return.-export const getEmail = (id) => +export const getEmail = (id: string): Cypress.Chainable<EmailSummary> => cy .request({ url: `${EMAIL_API}/message/${id}` }) .then((r) => r.body as EmailSummary);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e/cypress/common/apiCalls/common.ts(1 hunks)
⏰ 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: Frontend static check 🪲
- GitHub Check: Build backend 🏗️
- GitHub Check: Build frontend 🏗️
🔇 Additional comments (1)
e2e/cypress/common/apiCalls/common.ts (1)
371-377: LGTM for getLastEmail shape.Simple mapping reads clean and matches EmailSummary fields.
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
Bug Fixes
Tests
Documentation