Skip to content

Conversation

@trmartin4
Copy link
Member

@trmartin4 trmartin4 commented Dec 7, 2025

📔 Objective

We have had several discussions about the process for adding new email assets, which was never documented when we standardized on the assets.bitwarden.com container.

This PR adds a short documentation section on the email asset process in the README for creating email templates, when the assets would be used.

See here for well-formatted reading.

⏰ 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

@trmartin4 trmartin4 marked this pull request as ready for review December 7, 2025 21:01
@trmartin4 trmartin4 requested review from a team, 0xGlitchbyte and willmartian December 7, 2025 21:01
@claude
Copy link
Contributor

claude bot commented Dec 7, 2025

Claude finished @trmartin4's task —— View job


Code Review Complete

Todo List

  • Gather context and read PR changes
  • Conduct thorough code review using bitwarden-code-reviewer agent
  • Post comprehensive review feedback

Review: Email Asset Documentation

I've completed a thorough review of the documentation changes in src/Core/MailTemplates/README.md. This is a valuable addition that addresses a gap in the email development workflow.

Overall Assessment

👍 Great initiative! This documentation correctly identifies the asset URL pattern used throughout the codebase and provides clear guidance for developers on where email assets are hosted and how they should be managed.


Key Findings

⚠️ Missing Ticketing System Details (Lines 88)

The documentation states "enter a ticket for the SRE team" but doesn't specify:

  • Where to enter the ticket (Jira? Internal ticketing system? Slack channel?)
  • What team/project to assign it to
  • What information to include in the ticket
  • Expected turnaround time or SLA

Recommendation: Add specific guidance such as:

If the asset you are adding is not there, create a Jira ticket in the [SPECIFIC PROJECT] project for the SRE team to add the asset to the email asset container. Include the following in your ticket:
- The asset file(s) attached (preferred format: `.png`)
- The desired filename(s)
- The purpose/context of the asset (which email template will use it)
- Any urgency or timeline considerations

🎨 Enhancement: Asset Verification Process (Line 86)

Consider adding guidance about verifying assets work correctly after the SRE team adds them:

After an asset is added by the SRE team, verify:
- The asset loads correctly at the expected URL
- The asset renders properly in email clients (consider using an email testing tool)
- The asset meets email size best practices (typically < 100KB for images)

Minor: Formatting Issues

  1. Line 82: There are two spaces after "SRE team." (should be one)

    -managed by our SRE team.  For example:
    +managed by our SRE team. For example:
  2. Line 88: The file doesn't end with a newline character (common Unix/Linux convention)


🎨 Enhancement: Security Considerations

Given Bitwarden's security focus, consider adding security guidance for email assets:

### Security considerations

- Never include sensitive information in asset filenames or file content
- Assets are publicly accessible at `assets.bitwarden.com` - do not upload confidential images
- Ensure assets don't contain metadata that could expose internal information
- Use generic, professional names for assets (e.g., `icon-login.png` not `internal-auth-flow-2024.png`)

💭 Discussion: Asset Versioning Strategy (Line 82)

The documentation mentions /email/v1 path. Questions to consider:

  • What happens if an asset needs to be updated? Is there a versioning strategy within v1?
  • Are there plans for a v2 path? What would trigger that?
  • Should the documentation mention what to do if you need to update an existing asset versus adding a new one?

This might warrant a brief note about asset immutability or update procedures.


📝 Positive: Accurate URL Pattern (Line 82)

The documented URL pattern https://assets.bitwarden.com/email/v1/ is verified and consistent with actual usage throughout the codebase. I confirmed this pattern is used in:

  • src/Core/MailTemplates/Handlebars/Layouts/*.html.hbs files
  • src/Core/MailTemplates/Mjml/components/*.mjml files
  • src/Core/MailTemplates/Mjml/emails/**/*.mjml files

📝 Positive: Clear Example (Line 82)

The concrete example (mail-github.png) is helpful and references an actual asset that exists in the codebase.


🎨 Enhancement: Link to MJML Asset Usage

Since the MJML documentation already shows asset usage examples, consider cross-referencing it:

For examples of how to reference assets in MJML templates, see the [MJML documentation](./Mjml/README.md#custom-tags).

ℹ️ Note: Environment Consistency (Line 82)

The documentation correctly states "This is the URL for all assets for emails sent from any environment." This is an important clarification since it means:

  • Dev, staging, and production all use the same asset URL
  • Asset changes affect all environments simultaneously
  • Testing should happen before requesting asset additions

Consider making this implication more explicit if it's not obvious to all developers.


Summary

Strengths:

  • ✅ Fills an important documentation gap
  • ✅ Accurate information verified against codebase
  • ✅ Clear, concise writing
  • ✅ Good placement within the existing document structure

Critical Issues: None

Important Improvements:

  • Add specific ticketing system details (where and how to submit SRE requests)
  • Fix minor formatting issues (double space, missing newline)

Suggested Enhancements:

  • Add security considerations for asset management
  • Include asset verification steps
  • Consider adding versioning/update guidance
  • Cross-reference MJML documentation for usage examples

This documentation is a solid foundation that will help developers understand the email asset workflow. With the addition of specific ticketing instructions, it will be immediately actionable.


@trmartin4
Copy link
Member Author

Tagging team-sre because this describes a process of interaction on their team, and team-ui-foundation because this is in the mail template section that they own.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2025

Logo
Checkmarx One – Scan Summary & Detailsa20c8865-4957-48a1-80e2-01bee4a7de10

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

@codecov
Copy link

codecov bot commented Dec 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.64%. Comparing base (3ff5902) to head (65885cc).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6699      +/-   ##
==========================================
+ Coverage   53.60%   53.64%   +0.03%     
==========================================
  Files        1921     1926       +5     
  Lines       85650    85720      +70     
  Branches     7687     7687              
==========================================
+ Hits        45911    45981      +70     
  Misses      37967    37967              
  Partials     1772     1772              

☔ 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.

@trmartin4 trmartin4 merged commit 014376b into main Dec 8, 2025
48 checks passed
@trmartin4 trmartin4 deleted the auth/add-asset-docs branch December 8, 2025 18:47
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