Feature/fluid and email forms #48
Conversation
WalkthroughThe pull request introduces multiple changes primarily focused on package management and feature updates within the Argon API. Key modifications include the addition of a new package reference for Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
src/Argon.Api/Grains/EmailManager.cs (2)
Line range hint
29-36: Add error handling for template renderingThe template rendering lacks proper error handling and validation. Consider these improvements:
- Add try-catch block for template rendering
- Validate template existence
- Consider moving the template name to a constant or configuration
Here's a suggested improvement:
+ private const string OTP_TEMPLATE = "otp"; + public async Task SendOtpCodeAsync(string email, string otpCode, TimeSpan validity) { - var form = formStorage.Render("otp", new Dictionary<string, string> - { - { - "otp", otpCode - }, - { - "validity", $"{(int)Math.Floor(validity.TotalMinutes):D}" - } - }); + string form; + try + { + form = await formStorage.Render(OTP_TEMPLATE, new Dictionary<string, string> + { + { "otp", otpCode }, + { "validity", $"{(int)Math.Floor(validity.TotalMinutes):D}" } + }); + } + catch (Exception ex) + { + logger.LogError(ex, "Failed to render OTP template"); + throw new InvalidOperationException("Failed to generate email content", ex); + }
Line range hint
11-42: Consider migrating from SmtpClient to MailKit
SmtpClientis deprecated by Microsoft. Consider migrating toMailKitwhich offers:
- Better async support
- More secure defaults
- Active maintenance
- Built-in retry policies
Would you like me to provide a code example for the migration to MailKit?
src/Argon.Api/Features/Template/TemplateFeature.cs (1)
8-14: Consider adding configuration options for better flexibilityThe current implementation uses hardcoded service registrations. Consider adding configuration options to allow customization of:
- Template file location
- File patterns
- Parser options
Here's a suggested implementation:
+public class TemplateOptions +{ + public string TemplateDirectory { get; set; } = "./Resources"; + public string FilePattern { get; set; } = "*.html"; + public Action<FluidParser>? ConfigureParser { get; set; } +} public static class TemplateFeature { - public static IServiceCollection AddTemplateEngine(this WebApplicationBuilder builder) + public static IServiceCollection AddTemplateEngine( + this WebApplicationBuilder builder, + Action<TemplateOptions>? configure = null) { - builder.Services.AddSingleton<FluidParser>(); + var options = new TemplateOptions(); + configure?.Invoke(options); + + builder.Services.AddSingleton(options); + builder.Services.AddSingleton(sp => { + var parser = new FluidParser(); + options.ConfigureParser?.Invoke(parser); + return parser; + }); builder.Services.AddHostedService<EMailFormLoader>(); builder.Services.AddSingleton<EMailFormStorage>(); return builder.Services; } }src/Argon.Api/Resources/otp.html (2)
19-19: Consider enhancing visual separation in dark mode.While the current styling works, the container could benefit from better visual separation from the background.
Consider this enhancement:
.container { max-width: 600px; margin: 0 auto; background-color: #2a2a2a; padding: 20px; border-radius: 8px; - box-shadow: 0 2px 8px rgba(0, 0, 0, 0.5); + box-shadow: 0 4px 12px rgba(187, 134, 252, 0.15); + border: 1px solid rgba(187, 134, 252, 0.1); }Also applies to: 22-22
40-50: Consider adding interactive states for better UX.While the styling is consistent, adding hover and active states would improve user experience when the OTP code is interactive.
Consider adding these states:
.otp-code { display: inline-block; font-size: 28px; font-weight: bold; padding: 10px 20px; color: #ffffff; background-color: #6200ea; border-radius: 4px; text-decoration: none; margin-bottom: 20px; + transition: background-color 0.2s ease; +} +.otp-code:hover { + background-color: #7722ff; +} +.otp-code:active { + background-color: #5000d0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/Argon.Api/Argon.Api.csproj(3 hunks)src/Argon.Api/Features/EmailForms/EMailFormStorage.cs(0 hunks)src/Argon.Api/Features/Template/TemplateFeature.cs(1 hunks)src/Argon.Api/Grains/EmailManager.cs(2 hunks)src/Argon.Api/Program.cs(2 hunks)src/Argon.Api/Resources/new_session.html(1 hunks)src/Argon.Api/Resources/otp.html(1 hunks)src/Argon.Api/Resources/reset_pass.html(1 hunks)src/Argon.Entry/Program.cs(1 hunks)
💤 Files with no reviewable changes (1)
- src/Argon.Api/Features/EmailForms/EMailFormStorage.cs
✅ Files skipped from review due to trivial changes (2)
- src/Argon.Api/Resources/new_session.html
- src/Argon.Api/Resources/reset_pass.html
🔇 Additional comments (11)
src/Argon.Api/Grains/EmailManager.cs (1)
5-5: Verify complete migration from EmailForms to Template feature
The import change reflects the architectural shift from EmailForms to Template feature.
Let's verify if there are any remaining references to the old EmailForms feature:
✅ Verification successful
Migration from EmailForms to Template feature appears correct
The references to "emailform" found in the codebase are part of the new Template feature implementation (EMailFormStorage and EMailFormLoader in TemplateFeature.cs), indicating a proper migration where the old functionality has been encapsulated within the new Template feature. The import change in EmailManager.cs is consistent with this architectural change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to EmailForms feature
# that might have been missed during migration
# Search for any remaining references to EmailForms
rg -l "EmailForms" --type-add 'csharp:*.{cs,csproj}' --type csharp
# Search for any remaining AddEMailForms method calls
ast-grep --pattern 'AddEMailForms($$$)'
Length of output: 438
Script:
#!/bin/bash
# Let's do a more thorough search for any remaining references to EmailForms
# including partial matches and case variations to ensure complete migration
# Search for any variations of EmailForms in C# files
rg -i "emailform" --type-add 'csharp:*.{cs,csproj}' --type csharp -l
# Search for Template feature usage to verify proper migration
rg -i "Features\.Template" --type-add 'csharp:*.{cs,csproj}' --type csharp -l
# Look for any form-related classes or interfaces that might need updates
ast-grep --pattern 'class $_ : $$$Form$$$'
ast-grep --pattern 'interface I$_Form'
Length of output: 2963
Script:
#!/bin/bash
# Let's examine the specific changes in the files that still contain emailform references
# to ensure they are just part of the migration and not missed updates
# Check the content of EmailManager.cs for any old EmailForms usage
rg "emailform" -i src/Argon.Api/Grains/EmailManager.cs -C 2
# Check TemplateFeature.cs to understand the migration context
rg "emailform" -i src/Argon.Api/Features/Template/TemplateFeature.cs -C 2
# Look for any template-related method signatures that replaced email forms
ast-grep --pattern 'Render($$$)'
ast-grep --pattern 'CompileAndGetForm($$$)'
Length of output: 1651
src/Argon.Entry/Program.cs (1)
42-42: LGTM! Verify Orleans configuration section exists.
The change from hardcoded cluster configuration to dynamic configuration is a good practice. However, ensure that the "Orleans" configuration section is properly defined in your configuration files.
Let's verify the Orleans configuration section exists:
✅ Verification successful
Orleans configuration section exists and is properly configured ✅
The "Orleans" configuration section is present in both src/Argon.Entry/appsettings.json and src/Argon.Api/appsettings.json with the following configuration:
- ClusterId: "argonapi"
- ServiceId: "argonapi"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Orleans configuration in appsettings files
# Expected: Find configuration files containing Orleans section
# Search for appsettings files
fd appsettings.json$ --exec sh -c 'echo "=== {} ===" && cat {} | jq -r "select(.Orleans != null) | .Orleans"'
Length of output: 350
src/Argon.Api/Features/Template/TemplateFeature.cs (1)
1-5: LGTM! Clean and minimal imports
src/Argon.Api/Resources/otp.html (3)
10-10: LGTM! Color contrast meets accessibility standards.
The background-to-text color contrast ratio of 14.23:1 exceeds WCAG AAA requirements (minimum 7:1), ensuring excellent readability.
Also applies to: 13-13
30-30: LGTM! Consistent with Material Design dark theme.
The purple accent color (#bb86fc) aligns with Material Design dark theme guidelines.
35-39: LGTM! Text remains highly readable.
The message text color provides excellent contrast (9.57:1 ratio) against the container background.
src/Argon.Api/Program.cs (2)
14-14: LGTM! Namespace follows project conventions.
The addition of the Template feature namespace aligns with the architectural changes and follows the project's naming conventions.
52-52: Verify template engine integration with existing email functionality.
The shift from AddEMailForms() to AddTemplateEngine() represents a significant architectural change. While the implementation looks correct, we should verify that all email form consumers have been updated to use the new template engine.
✅ Verification successful
Template engine integration is properly implemented and functional
The verification shows that:
- The new template engine (
TemplateFeature) is a complete replacement that includes the email form functionality EmailManageris properly integrated with the template system throughEMailFormStorage- The system loads HTML templates from the Resources directory and manages them through a concurrent dictionary
- Email sending functionality is properly wired up with template rendering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all email form consumers are updated to use the new template engine
# Search for any remaining references to old email forms
echo "Checking for old email form references..."
rg -l "EMailForm" --type cs
# Search for template engine usage to ensure proper adoption
echo "Checking template engine usage..."
rg -l "Template(Engine|Feature)" --type cs
# Look for any email-related HTML templates
echo "Checking email templates..."
fd -e html . | grep -i "email\|mail\|template"
Length of output: 520
Script:
#!/bin/bash
# Let's examine the actual implementation and usage patterns
# Check the content of EmailManager.cs to see how email forms are used
echo "Checking EmailManager.cs implementation..."
rg -A 5 -B 5 "EMailForm" src/Argon.Api/Grains/EmailManager.cs
# Check TemplateFeature.cs to understand the relationship between old and new systems
echo "Checking TemplateFeature.cs implementation..."
cat src/Argon.Api/Features/Template/TemplateFeature.cs
# Look for actual email sending patterns
echo "Checking email sending patterns..."
rg -A 3 "SendEmail|SendMail" --type cs
Length of output: 3735
src/Argon.Api/Argon.Api.csproj (3)
14-14: Verify template engine implementation
The addition of Fluid.Core aligns with the PR objectives for fluid forms implementation.
#!/bin/bash
# Description: Verify the usage of Fluid.Core template engine
# Expected: Find template-related implementations using Fluid
# Search for Fluid template usage
ast-grep --pattern 'using Fluid;'
# Look for template-related files
fd -e cs -e liquid -e html template27-27: Verify ActualLab.Fusion version compatibility
The update from 9.5.59 to 9.5.64 is a minor version bump. Please ensure there are no breaking changes.
#!/bin/bash
# Description: Check for potential breaking changes in ActualLab.Fusion
# Expected: Find usage patterns that might be affected by the update
# Search for ActualLab.Fusion usage patterns
rg -l "using ActualLab\.Fusion"
# Look for common breaking patterns in Fusion apps
ast-grep --pattern 'IComputed<$_>'
ast-grep --pattern '[ComputeMethod]'
ast-grep --pattern 'IComputeService'48-48: Verify security implications of JWT package update
The JWT package update (8.2.0 → 8.2.1) is a patch version bump. Please verify security implications.
✅ Verification successful
JWT package update is safe to proceed
The version update from 8.2.0 to 8.2.1 is a patch version that does not fall within any vulnerable version ranges identified in the security advisories. The latest security advisories for this package are from January 2024 and affect versions below 7.1.2, which is well before our current version.
The git history shows a stable progression of JWT package versions, with proper dependency management. The update follows semantic versioning principles and represents a safe, backward-compatible patch.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories and JWT usage patterns
# Expected: Find JWT configurations and usage that might need review
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NUGET, package: "System.IdentityModel.Tokens.Jwt") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Look for JWT usage patterns
ast-grep --pattern 'JwtSecurityToken'
ast-grep --pattern 'JwtRegisteredClaimNames'
Length of output: 1698
Script:
#!/bin/bash
# Let's check the specific version changes in the git history and current JWT usage
git log -p --all -S "System.IdentityModel.Tokens.Jwt" -- "**/*.csproj"
# Search for JWT token generation and validation patterns
rg -l "JwtSecurityTokenHandler|TokenValidationParameters|SigningCredentials" --type cs
# Look for JWT configuration in settings
rg -l "JWT|Bearer" --type json
Length of output: 9232
| { | ||
| protected async override Task ExecuteAsync(CancellationToken stoppingToken) | ||
| { | ||
| var formFiles = Directory.EnumerateFiles("./Resources", "*.html").ToList(); |
There was a problem hiding this comment.
💡 Codebase verification
Add security measures for template directory access
The code has potential security issues that need to be addressed:
- The template directory path "./Resources" is hardcoded and accessed without validation
- No path sanitization or access controls are in place
- Directory traversal protection is missing
Required changes:
- Move template directory path to configuration
- Add path validation to ensure templates are accessed within allowed boundaries
- Implement sanitization for template file names
- Consider using
Path.GetFullPath()to resolve relative paths securely
🔗 Analysis chain
Verify template directory access security
The file system operations need additional security checks:
- Verify template directory is within allowed paths
- Check for path traversal in template names
Also applies to: 53-54
Let me gather more information about the template directory security by checking for configuration and validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential security issues in template handling
# Check if template paths contain parent directory traversal
rg -g '*.cs' '(?i)path\.(?:combine|join).*\.\.'
# Check for template directory configuration in settings files
fd -e json -e yaml -e xml | xargs rg -i 'template.*(?:directory|path|location)'
# Look for other file system operations that might need security review
ast-grep --pattern 'File.$$$'
Length of output: 5096
Script:
#!/bin/bash
# Check for template directory configuration and validation
# Look for template directory configuration
rg -g '*.cs' -B 2 -A 2 '"./Resources"'
# Check for any validation or security checks around template access
ast-grep --pattern 'Directory.$$$'
# Check if Resources directory is configured in settings
fd appsettings | xargs rg -i 'resources'
# Look for any path validation or sanitization
rg -g '*.cs' -B 2 -A 2 'Path\.(?:GetFullPath|Combine)'
Length of output: 5174
| public class EMailFormStorage | ||
| { | ||
| private readonly ConcurrentDictionary<string, IFluidTemplate> htmlForms = new(); | ||
|
|
||
| public void Load(string name, IFluidTemplate template) => htmlForms.TryAdd(name, template); | ||
|
|
||
| public IFluidTemplate GetContentFor(string formKey) | ||
| { | ||
| if (htmlForms.TryGetValue(formKey, out var form)) | ||
| return form; | ||
| throw new InvalidOperationException($"No '{formKey}' form found"); | ||
| } | ||
|
|
||
| public string Render(string formKey, Dictionary<string, string> values) | ||
| { | ||
| var template = GetContentFor(formKey); | ||
|
|
||
| var context = new TemplateContext(); | ||
|
|
||
| foreach (var (key, value) in values) | ||
| context.SetValue(key, value); | ||
|
|
||
| return template.Render(context); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling and API design
Several improvements could make this class more robust:
- The storage has no upper bound which could lead to memory issues
- No way to check template existence without exception
- Values dictionary is restricted to strings
Suggested improvements:
public class EMailFormStorage
{
- private readonly ConcurrentDictionary<string, IFluidTemplate> htmlForms = new();
+ private readonly ConcurrentDictionary<string, IFluidTemplate> htmlForms;
+ private readonly int maxTemplates;
+
+ public EMailFormStorage(IConfiguration config)
+ {
+ maxTemplates = config.GetValue<int>("Templates:MaxCount", 1000);
+ htmlForms = new(Environment.ProcessorCount, maxTemplates);
+ }
- public void Load(string name, IFluidTemplate template) => htmlForms.TryAdd(name, template);
+ public bool Load(string name, IFluidTemplate template)
+ {
+ if (htmlForms.Count >= maxTemplates)
+ return false;
+ return htmlForms.TryAdd(name, template);
+ }
+ public bool Exists(string formKey) => htmlForms.ContainsKey(formKey);
public IFluidTemplate GetContentFor(string formKey)
{
if (htmlForms.TryGetValue(formKey, out var form))
return form;
- throw new InvalidOperationException($"No '{formKey}' form found");
+ throw new KeyNotFoundException($"Template '{formKey}' not found");
}
- public string Render(string formKey, Dictionary<string, string> values)
+ public string Render<T>(string formKey, T values) where T : class
{
var template = GetContentFor(formKey);
var context = new TemplateContext();
- foreach (var (key, value) in values)
- context.SetValue(key, value);
+ context.SetValue("Model", values);
return template.Render(context);
}
}📝 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.
| public class EMailFormStorage | |
| { | |
| private readonly ConcurrentDictionary<string, IFluidTemplate> htmlForms = new(); | |
| public void Load(string name, IFluidTemplate template) => htmlForms.TryAdd(name, template); | |
| public IFluidTemplate GetContentFor(string formKey) | |
| { | |
| if (htmlForms.TryGetValue(formKey, out var form)) | |
| return form; | |
| throw new InvalidOperationException($"No '{formKey}' form found"); | |
| } | |
| public string Render(string formKey, Dictionary<string, string> values) | |
| { | |
| var template = GetContentFor(formKey); | |
| var context = new TemplateContext(); | |
| foreach (var (key, value) in values) | |
| context.SetValue(key, value); | |
| return template.Render(context); | |
| } | |
| } | |
| public class EMailFormStorage | |
| { | |
| private readonly ConcurrentDictionary<string, IFluidTemplate> htmlForms; | |
| private readonly int maxTemplates; | |
| public EMailFormStorage(IConfiguration config) | |
| { | |
| maxTemplates = config.GetValue<int>("Templates:MaxCount", 1000); | |
| htmlForms = new(Environment.ProcessorCount, maxTemplates); | |
| } | |
| public bool Load(string name, IFluidTemplate template) | |
| { | |
| if (htmlForms.Count >= maxTemplates) | |
| return false; | |
| return htmlForms.TryAdd(name, template); | |
| } | |
| public bool Exists(string formKey) => htmlForms.ContainsKey(formKey); | |
| public IFluidTemplate GetContentFor(string formKey) | |
| { | |
| if (htmlForms.TryGetValue(formKey, out var form)) | |
| return form; | |
| throw new KeyNotFoundException($"Template '{formKey}' not found"); | |
| } | |
| public string Render<T>(string formKey, T values) where T : class | |
| { | |
| var template = GetContentFor(formKey); | |
| var context = new TemplateContext(); | |
| context.SetValue("Model", values); | |
| return template.Render(context); | |
| } | |
| } |
| public class EMailFormLoader(EMailFormStorage storage, ILogger<EMailFormLoader> logger, FluidParser engine) : BackgroundService | ||
| { | ||
| protected async override Task ExecuteAsync(CancellationToken stoppingToken) | ||
| { | ||
| var formFiles = Directory.EnumerateFiles("./Resources", "*.html").ToList(); | ||
|
|
||
| logger.LogInformation("Found '{count}' email forms", formFiles.Count); | ||
|
|
||
| foreach (var file in formFiles) | ||
| { | ||
| var content = await File.ReadAllTextAsync(file, stoppingToken); | ||
| var name = Path.GetFileNameWithoutExtension(file); | ||
|
|
||
| if (engine.TryParse(content, out var result, out var error)) | ||
| { | ||
| storage.Load(name, result); | ||
| logger.LogInformation("Loaded '{name}' email form", name); | ||
| } | ||
| else | ||
| logger.LogError("Failed load '{name}' email form, error: {error}", name, error); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical improvements needed for robust template loading
Several critical issues need to be addressed:
- Hardcoded path could fail in production
- No way to reload templates without service restart
- Missing error handling for file system operations
- No retry mechanism for failed templates
Here's a suggested implementation with improvements:
-public class EMailFormLoader(EMailFormStorage storage, ILogger<EMailFormLoader> logger, FluidParser engine)
+public class EMailFormLoader(
+ EMailFormStorage storage,
+ ILogger<EMailFormLoader> logger,
+ FluidParser engine,
+ TemplateOptions options)
: BackgroundService
{
+ private readonly FileSystemWatcher? watcher;
+ private readonly HashSet<string> failedTemplates = new();
+ private readonly SemaphoreSlim semaphore = new(1, 1);
+
+ public EMailFormLoader()
+ {
+ if (Directory.Exists(options.TemplateDirectory))
+ {
+ watcher = new(options.TemplateDirectory, options.FilePattern);
+ watcher.Changed += OnTemplateChanged;
+ watcher.Created += OnTemplateChanged;
+ }
+ }
protected async override Task ExecuteAsync(CancellationToken stoppingToken)
{
- var formFiles = Directory.EnumerateFiles("./Resources", "*.html").ToList();
+ await LoadTemplatesAsync(stoppingToken);
+
+ if (watcher != null)
+ {
+ watcher.EnableRaisingEvents = true;
+
+ // Periodically retry failed templates
+ while (!stoppingToken.IsCancellationRequested)
+ {
+ await Task.Delay(TimeSpan.FromMinutes(5), stoppingToken);
+ await RetryFailedTemplatesAsync(stoppingToken);
+ }
+ }
+ }
- logger.LogInformation("Found '{count}' email forms", formFiles.Count);
+ private async Task LoadTemplatesAsync(CancellationToken stoppingToken)
+ {
+ try
+ {
+ var formFiles = Directory.EnumerateFiles(
+ options.TemplateDirectory,
+ options.FilePattern
+ ).ToList();
+ logger.LogInformation(
+ "Found {Count} email forms in {Directory}",
+ formFiles.Count,
+ options.TemplateDirectory
+ );
+ await LoadTemplatesFromFilesAsync(formFiles, stoppingToken);
+ }
+ catch (Exception ex) when (ex is DirectoryNotFoundException or UnauthorizedAccessException)
+ {
+ logger.LogError(
+ ex,
+ "Failed to access template directory {Directory}",
+ options.TemplateDirectory
+ );
+ }
+ }
- foreach (var file in formFiles)
- {
- var content = await File.ReadAllTextAsync(file, stoppingToken);
- var name = Path.GetFileNameWithoutExtension(file);
+ private async Task LoadTemplatesFromFilesAsync(
+ IEnumerable<string> files,
+ CancellationToken stoppingToken)
+ {
+ foreach (var file in files)
+ {
+ await LoadTemplateAsync(file, stoppingToken);
+ }
+ }
+ private async Task LoadTemplateAsync(
+ string file,
+ CancellationToken stoppingToken)
+ {
+ try
+ {
+ var content = await File.ReadAllTextAsync(file, stoppingToken);
+ var name = Path.GetFileNameWithoutExtension(file);
if (engine.TryParse(content, out var result, out var error))
{
- storage.Load(name, result);
+ if (storage.Load(name, result))
+ {
+ failedTemplates.Remove(name);
logger.LogInformation("Loaded '{name}' email form", name);
+ }
+ else
+ {
+ logger.LogWarning(
+ "Failed to load '{Name}' - storage capacity reached",
+ name
+ );
+ }
}
else
+ {
+ failedTemplates.Add(name);
logger.LogError("Failed load '{name}' email form, error: {error}", name, error);
+ }
+ }
+ catch (Exception ex) when (
+ ex is IOException or
+ UnauthorizedAccessException)
+ {
+ logger.LogError(
+ ex,
+ "Failed to read template file {File}",
+ file
+ );
}
}
+ private async void OnTemplateChanged(object sender, FileSystemEventArgs e)
+ {
+ try
+ {
+ await semaphore.WaitAsync();
+ await LoadTemplateAsync(e.FullPath, CancellationToken.None);
+ }
+ finally
+ {
+ semaphore.Release();
+ }
+ }
+ private async Task RetryFailedTemplatesAsync(CancellationToken stoppingToken)
+ {
+ if (failedTemplates.Count == 0) return;
+ logger.LogInformation(
+ "Retrying {Count} failed templates",
+ failedTemplates.Count
+ );
+ var templates = failedTemplates.ToList();
+ foreach (var template in templates)
+ {
+ var file = Path.Combine(
+ options.TemplateDirectory,
+ template + ".html"
+ );
+ await LoadTemplateAsync(file, stoppingToken);
+ }
+ }
+ public override void Dispose()
+ {
+ watcher?.Dispose();
+ semaphore.Dispose();
+ base.Dispose();
+ }
}📝 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.
| public class EMailFormLoader(EMailFormStorage storage, ILogger<EMailFormLoader> logger, FluidParser engine) : BackgroundService | |
| { | |
| protected async override Task ExecuteAsync(CancellationToken stoppingToken) | |
| { | |
| var formFiles = Directory.EnumerateFiles("./Resources", "*.html").ToList(); | |
| logger.LogInformation("Found '{count}' email forms", formFiles.Count); | |
| foreach (var file in formFiles) | |
| { | |
| var content = await File.ReadAllTextAsync(file, stoppingToken); | |
| var name = Path.GetFileNameWithoutExtension(file); | |
| if (engine.TryParse(content, out var result, out var error)) | |
| { | |
| storage.Load(name, result); | |
| logger.LogInformation("Loaded '{name}' email form", name); | |
| } | |
| else | |
| logger.LogError("Failed load '{name}' email form, error: {error}", name, error); | |
| } | |
| } | |
| } | |
| public class EMailFormLoader( | |
| EMailFormStorage storage, | |
| ILogger<EMailFormLoader> logger, | |
| FluidParser engine, | |
| TemplateOptions options) | |
| : BackgroundService | |
| { | |
| private readonly FileSystemWatcher? watcher; | |
| private readonly HashSet<string> failedTemplates = new(); | |
| private readonly SemaphoreSlim semaphore = new(1, 1); | |
| public EMailFormLoader() | |
| { | |
| if (Directory.Exists(options.TemplateDirectory)) | |
| { | |
| watcher = new(options.TemplateDirectory, options.FilePattern); | |
| watcher.Changed += OnTemplateChanged; | |
| watcher.Created += OnTemplateChanged; | |
| } | |
| } | |
| protected async override Task ExecuteAsync(CancellationToken stoppingToken) | |
| { | |
| await LoadTemplatesAsync(stoppingToken); | |
| if (watcher != null) | |
| { | |
| watcher.EnableRaisingEvents = true; | |
| // Periodically retry failed templates | |
| while (!stoppingToken.IsCancellationRequested) | |
| { | |
| await Task.Delay(TimeSpan.FromMinutes(5), stoppingToken); | |
| await RetryFailedTemplatesAsync(stoppingToken); | |
| } | |
| } | |
| } | |
| private async Task LoadTemplatesAsync(CancellationToken stoppingToken) | |
| { | |
| try | |
| { | |
| var formFiles = Directory.EnumerateFiles( | |
| options.TemplateDirectory, | |
| options.FilePattern | |
| ).ToList(); | |
| logger.LogInformation( | |
| "Found {Count} email forms in {Directory}", | |
| formFiles.Count, | |
| options.TemplateDirectory | |
| ); | |
| await LoadTemplatesFromFilesAsync(formFiles, stoppingToken); | |
| } | |
| catch (Exception ex) when (ex is DirectoryNotFoundException or UnauthorizedAccessException) | |
| { | |
| logger.LogError( | |
| ex, | |
| "Failed to access template directory {Directory}", | |
| options.TemplateDirectory | |
| ); | |
| } | |
| } | |
| private async Task LoadTemplatesFromFilesAsync( | |
| IEnumerable<string> files, | |
| CancellationToken stoppingToken) | |
| { | |
| foreach (var file in files) | |
| { | |
| await LoadTemplateAsync(file, stoppingToken); | |
| } | |
| } | |
| private async Task LoadTemplateAsync( | |
| string file, | |
| CancellationToken stoppingToken) | |
| { | |
| try | |
| { | |
| var content = await File.ReadAllTextAsync(file, stoppingToken); | |
| var name = Path.GetFileNameWithoutExtension(file); | |
| if (engine.TryParse(content, out var result, out var error)) | |
| { | |
| if (storage.Load(name, result)) | |
| { | |
| failedTemplates.Remove(name); | |
| logger.LogInformation("Loaded '{name}' email form", name); | |
| } | |
| else | |
| { | |
| logger.LogWarning( | |
| "Failed to load '{Name}' - storage capacity reached", | |
| name | |
| ); | |
| } | |
| } | |
| else | |
| { | |
| failedTemplates.Add(name); | |
| logger.LogError("Failed load '{name}' email form, error: {error}", name, error); | |
| } | |
| } | |
| catch (Exception ex) when ( | |
| ex is IOException or | |
| UnauthorizedAccessException) | |
| { | |
| logger.LogError( | |
| ex, | |
| "Failed to read template file {File}", | |
| file | |
| ); | |
| } | |
| } | |
| private async void OnTemplateChanged(object sender, FileSystemEventArgs e) | |
| { | |
| try | |
| { | |
| await semaphore.WaitAsync(); | |
| await LoadTemplateAsync(e.FullPath, CancellationToken.None); | |
| } | |
| finally | |
| { | |
| semaphore.Release(); | |
| } | |
| } | |
| private async Task RetryFailedTemplatesAsync(CancellationToken stoppingToken) | |
| { | |
| if (failedTemplates.Count == 0) return; | |
| logger.LogInformation( | |
| "Retrying {Count} failed templates", | |
| failedTemplates.Count | |
| ); | |
| var templates = failedTemplates.ToList(); | |
| foreach (var template in templates) | |
| { | |
| var file = Path.Combine( | |
| options.TemplateDirectory, | |
| template + ".html" | |
| ); | |
| await LoadTemplateAsync(file, stoppingToken); | |
| } | |
| } | |
| public override void Dispose() | |
| { | |
| watcher?.Dispose(); | |
| semaphore.Dispose(); | |
| base.Dispose(); | |
| } | |
| } |
| border-radius: 4px; | ||
| text-decoration: none; | ||
| margin-bottom: 20px; | ||
| } | ||
|
|
||
| .footer { | ||
| font-size: 14px; | ||
| color: #777777; | ||
| color: #888888; |
There was a problem hiding this comment.
Fix footer text contrast for accessibility compliance.
The current footer text color (#888888) on background (#2a2a2a) has a contrast ratio of 3.54:1, which fails WCAG AA requirements (minimum 4.5:1 for normal text).
Apply this fix:
- color: #888888;
+ color: #a7a7a7;The new color provides a contrast ratio of 4.52:1, meeting WCAG AA requirements.
📝 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.
| color: #888888; | |
| color: #a7a7a7; |
| .button { | ||
| display: inline-block; | ||
| font-size: 18px; | ||
| font-weight: bold; | ||
| padding: 10px 20px; | ||
| color: #ffffff; | ||
| background-color: #6200ea; | ||
| border-radius: 4px; | ||
| text-decoration: none; | ||
| margin-bottom: 20px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce style duplication with a shared base class.
The .button and .otp-code classes share many properties and could be refactored to follow DRY principles.
Consider this refactor:
+.button-base {
+ display: inline-block;
+ font-weight: bold;
+ padding: 10px 20px;
+ color: #ffffff;
+ background-color: #6200ea;
+ border-radius: 4px;
+ text-decoration: none;
+ margin-bottom: 20px;
+ transition: background-color 0.2s ease;
+}
+
+.button-base:hover {
+ background-color: #7722ff;
+}
+
+.button-base:active {
+ background-color: #5000d0;
+}
+
-.otp-code {
- display: inline-block;
+.otp-code extends .button-base {
font-size: 28px;
- font-weight: bold;
- padding: 10px 20px;
- color: #ffffff;
- background-color: #6200ea;
- border-radius: 4px;
- text-decoration: none;
- margin-bottom: 20px;
}
-.button {
- display: inline-block;
+.button extends .button-base {
font-size: 18px;
- font-weight: bold;
- padding: 10px 20px;
- color: #ffffff;
- background-color: #6200ea;
- border-radius: 4px;
- text-decoration: none;
- margin-bottom: 20px;
}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Chores