Skip to content

Conversation

@JimmyVo16
Copy link
Contributor

@JimmyVo16 JimmyVo16 commented Dec 4, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-21742

📔 Objective

While working on this ticket, I discovered this error:
image

This validation error occurs because the build script treats this shared subcomponent as a standalone component. We have this subcomponent here because only the AC team needs it. By moving the files into an AC directory, we can reduce code review friction.

This change will make the build script recognize all “components” directories as non-standalone MJML templates.

Note: This has no impact on production code. We only use this build script during development.

📸 Screenshots

image

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@JimmyVo16 JimmyVo16 self-assigned this Dec 4, 2025
@JimmyVo16 JimmyVo16 requested a review from a team as a code owner December 4, 2025 14:39
@JimmyVo16 JimmyVo16 requested a review from willmartian December 4, 2025 14:39
@claude
Copy link
Contributor

claude bot commented Dec 4, 2025

Claude finished @JimmyVo16's task —— View job


📋 Review Complete ✅

  • Gather context and read changed files
  • Review the MJML build script changes
  • Run comprehensive code review
  • Post review feedback

🔍 Review Summary

This PR correctly fixes the MJML validation error by excluding component files from standalone compilation. The implementation is sound, follows best practices, and has no security concerns.

✅ Strengths

Code Correctness (src/Core/MailTemplates/Mjml/build.js:45-47)

  • The ignore: ['**/components/**'] pattern correctly excludes both root-level and team-specific component directories
  • Properly prevents component files like mobile-app-download.mjml from being compiled as standalone templates
  • Compatible with glob v10.4.5 and follows glob API conventions

Security & Performance

  • No security concerns - build-time only change with hardcoded patterns
  • Minor performance improvement by skipping unnecessary compilation attempts
  • No impact on zero-knowledge architecture or runtime behavior

Code Quality

  • Minimal, focused change that solves the stated problem
  • Clear comment explaining the exclusion
  • Maintains backward compatibility with all existing templates

📝 Suggestions for Enhancement

1. Add Debug Logging (Priority: Medium)
Consider enhancing the trace output to show excluded files:

const mjmlFiles = glob.sync(`${config.inputDir}/**/*.mjml`, {
  ignore: ['**/components/**']
});

if (flags.trace) {
  const excludedFiles = glob.sync(`${config.inputDir}/**/components/**/*.mjml`);
  console.log(`[DEBUG] Excluded ${excludedFiles.length} component file(s):`, excludedFiles);
}

2. Documentation Gap (Priority: Medium)
The README.md should document the components/ directory convention:

  • Files in components/ directories are reusable snippets (not standalone templates)
  • They're included via <mj-include> or registered as custom components in .mjmlconfig
  • Distinction between .mjml includes and custom .js components

3. Testing Considerations (Priority: Low)

  • No automated tests exist for the build script
  • Manual verification: Run npm run build and confirm exactly 9 templates compile (not 10)
  • Consider adding integration tests for MJML compilation in future work

❓ Questions

  1. Have you manually verified that npm run build succeeds without errors after this change?
  2. Have you confirmed that templates using <mj-include path="...components/..."> still compile correctly?

🎯 Recommendation

Approve and merge - This is a well-executed bug fix. The code change is correct and the concerns are minor documentation/testing improvements that can be addressed in follow-up work.

Code Quality Score: 8/10


@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2025

Logo
Checkmarx One – Scan Summary & Details3b4783a5-00b2-4612-ba86-f51f989751f6

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Dec 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.40%. Comparing base (655054a) to head (fbe122e).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6687   +/-   ##
=======================================
  Coverage   53.40%   53.40%           
=======================================
  Files        1917     1917           
  Lines       85466    85466           
  Branches     7667     7667           
=======================================
  Hits        45643    45643           
  Misses      38056    38056           
  Partials     1767     1767           

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

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

@JimmyVo16 JimmyVo16 merged commit d88fff4 into main Dec 4, 2025
45 checks passed
@JimmyVo16 JimmyVo16 deleted the ac/pm-21742/fix-mjml-validation-error branch December 4, 2025 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants