Add Papercut SMTP Service Tray Manager #345
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR introduces a new Windows Forms tray notification service for the Papercut SMTP server, addressing the need for server tray icon visibility. It refactors path template handling across the codebase, adds cancellation token support to server operations, consolidates logging configuration, and introduces infrastructure for service-to-tray communication via IPComm. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Tray App<br/>(Program.Main)
participant Container as DI Container<br/>(SimpleContainer)
participant Coord as ServiceTrayCoordinator
participant Status as ServiceStatusService
participant Comm as ServiceCommunicator
participant IPComm as IPCommServer<br/>(TrayService endpoint)
participant WinSvc as Windows<br/>ServiceController
App->>App: BootstrapLogger.CreateBootstrapLogger(AppMeta, args)
App->>App: ApplicationConfiguration.Initialize()
App->>Container: Build PapercutServiceTrayModule
Container->>Container: Register IPComm, Services, Events
App->>Container: Resolve ServiceTrayCoordinator
Container->>Coord: new(LoggingPathConfigurator, ServiceStatusService, ...)
Container->>Status: new(ServiceControllerProvider, ServiceCommunicator)
Container->>Comm: new(IPCommClientFactory, Logger)
Container->>IPComm: StartAsync(TrayService endpoint)
Coord->>Coord: Load tray icon & context menu
Coord->>Status: UpdateStatus()
Status->>WinSvc: GetStatus()
Coord->>Coord: Subscribe to StatusChanged & NewMessageReceived
App->>App: Application.Run(Coordinator)
Note over Coord,Status: User clicks "Start Service"
Coord->>Status: StartService()
Status->>WinSvc: Start()
Status->>Coord: OnStatusChanged(Running)
Coord->>Coord: Update tray icon text
sequenceDiagram
participant Service as SMTP Service<br/>(SmtpServerManager)
participant Event as Event Bus
participant Publish as PublishAppEventsHandler<br/>ToTrayService
participant IPComm as IPCommClient<br/>(ServiceTray)
participant Tray as Tray Service<br/>(Listeners)
Service->>Event: Raise NewMessageEvent(MessageEntryDto)
Event->>Publish: HandleAsync(NewMessageEvent)
Publish->>Publish: PublishAsync(event, token)
Publish->>IPComm: PublishEventServer(event, timeout, token)
IPComm->>Tray: Connect & send event
Tray->>Tray: ServiceCommunicator receives event
Note over Tray: ServiceTrayCoordinator listening
Tray->>Tray: OnNewMessageReceived(event)
Tray->>Tray: Show balloon notification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
Added service status checks in OnOpenWebUI to prevent attempting to open the web UI when double-clicking the tray icon if the service is stopped or not installed. Now shows appropriate warning messages: - "Service Not Installed" if service is not installed - "Service Not Running" if service is stopped This prevents the browser from opening to a non-responsive URL.
Created AppRunOnStartupService to manage Windows registry startup entries and integrated it into the tray context menu. Features: - "Run at Startup" checkbox menu item - Uses Windows registry (HKCU\SOFTWARE\Microsoft\Windows\CurrentVersion\Run) - Automatically checks current state when menu opens - Toggles on/off with visual checkbox feedback - Proper error handling with user-friendly messages - Logs all startup configuration changes Implementation: - AppRunOnStartupService: Manages registry operations - IsRunOnStartupEnabled(): Checks current registry state - SetRunOnStartup(bool): Enables or disables startup entry - ServiceTrayCoordinator: UI integration - Added "Run at Startup" menu item with CheckOnClick - Updates checkbox state in UpdateMenuState() - OnToggleRunOnStartup event handler with error recovery The service uses Environment.ProcessPath to get the executable path and stores it in the registry under the app name from IAppMeta.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
src/Papercut.Service/Infrastructure/Paths/ServerPathTemplateProviderService.cs (1)
25-36: Minor: Variable naming is misleading.The variable
messagePathson line 31 is used for both message and logging paths depending on thetypeparameter. Consider renaming it to something more generic likepathsorpathTemplatesfor clarity.Apply this diff:
- var messagePaths = paths.Split(';') + var pathTemplates = paths.Split(';') .Select(s => s.Trim()) .Where(s => !string.IsNullOrWhiteSpace(s)); - PathTemplates = new ObservableCollection<string>(messagePaths); + PathTemplates = new ObservableCollection<string>(pathTemplates);src/Papercut.Core/Infrastructure/Consoles/ConsoleHelpers.cs (1)
23-39: Consider catching specific exception types.The catch-all exception handler on line 35 swallows all exceptions, which could potentially hide unexpected issues. Consider catching only the specific exceptions that indicate no console is available.
Apply this diff to catch specific exceptions:
try { _ = Console.WindowHeight; return true; } - catch + catch (IOException) { + // Console is not available (e.g., running as Windows Service) return false; }Note:
IOExceptionis thrown when there is no console attached. If other specific exceptions are expected, they can be added to the catch clause.Papercut.sln (1)
78-79: Solution entry and configurations for tray project are completeThe new
Papercut.Service.TrayNotificationproject is correctly added with matching Debug/Release configurations across AnyCPU/x64/x86. From the solution file alone there’s nothing blocking.Optionally, you might want to nest the tray project under an existing solution folder (e.g., alongside the service) for tidier solution organization, but that’s cosmetic.
Also applies to: 318-329
src/Papercut.Service.TrayNotification/CorePathTemplateProvider.cs (1)
27-38: Clarify the logging path template string literal
"%DataDirectory%\\\\Logs"compiles to"%DataDirectory%\\Logs"at runtime (two backslashes). GivenPathTemplateHelperalready normalizes and deduplicates separators, this double separator is unnecessary and a bit confusing.Consider simplifying to a clearer literal, e.g.:
- public ObservableCollection<string> PathTemplates { get; } = new(["%DataDirectory%\\\\Logs"]); + public ObservableCollection<string> PathTemplates { get; } = new(["%DataDirectory%\\Logs"]); + // or + public ObservableCollection<string> PathTemplates { get; } = new([@"%DataDirectory%\Logs"]);This keeps behavior but improves readability.
src/Papercut.Message/MessageRepository.cs (1)
26-35: Constructor and filename-char caching look good; visibility could be tightened
- Switching the ctor to
MessagePathConfiguratorand using the primary-constructor style is consistent with the new path configuration model.- Adding the
entry == nullguard inDeleteMessageand cachingPath.GetInvalidFileNameChars()behindInvalidFileNameChars ??=both improve robustness and performance without changing expected behavior.If
InvalidFileNameCharsis not intended for subclass consumption, consider making itprivate staticinstead ofprotected staticto keep the surface tight, but this is purely optional.Also applies to: 139-147
src/Papercut.Service.TrayNotification/PapercutServiceControllerProvider.cs (1)
34-41: Service controller wrapper looks solid; consider future-proofing IsInstalled/error flowThe wrapper around
ServiceControlleris clean and does the right thing withusingand timeouts for Start/Stop/Restart. If you later consumeIsInstalled()outside the tray status service, you may want to decide explicitly whether access-denied (Win32Exception) should surface as “installed but unauthorized” vs “not installed”, and possibly log those failures similarly toServiceStatusService.UpdateStatus. For now, this is fine as a minimal, focused abstraction.Also applies to: 48-91
src/Papercut.Service.TrayNotification/ServiceStatusService.cs (1)
28-172: Status tracking and control logic are coherent; event payload for “not installed” could be clearerThe status polling and control methods are consistent with the tray’s usage (guards via
CanStart/CanStop/CanRestart, invalidating the web-URL cache on start/restart, and usingIsServiceInstalledto drive UI). One minor clarity concern: on anInvalidOperationExceptionyou raiseStatusChangedwithdefaultServiceControllerStatus, which is0and not a named enum value; current consumers ignore that value and readIsServiceInstalled, but future subscribers might misinterpret it. If you expect more listeners later, consider a custom event args type or documenting that callers should rely onIsServiceInstalledandCurrentStatusrather than the event argument.src/Papercut.Service.TrayNotification/ServiceTrayCoordinator.cs (1)
38-70: Tray coordinator behavior and UX wiring are well-structuredThe coordinator wires the tray icon, periodic status polling, and context menu nicely: menu state is derived from
ServiceStatusService, “Open Web UI” reflects the cached URL when possible, and the Start/Stop/Restart handlers have clear, user-friendly error paths for access denied and service-not-found scenarios. Given this runs on the UI thread, the 2-secondTimerand synchronous service control calls are acceptable for now, but if service operations ever become slow, you might later migrate those to background tasks and just refresh status on completion.Also applies to: 118-224, 231-416
src/Papercut.UI/SettingPathTemplateProvider.cs (1)
28-37: Path template refactor is cleaner and consistent with core/server providersThe shift to a single
PathTemplatescollection keyed byPathTemplateType, plus thePathsswitch, removes duplication between message/logging paths and matches the server-sideServerPathTemplateProviderServicepattern. Logging improvements (_loggerenriched withPathType) andUpdatePathTemplateusingAddRange/RemoveRangegive clear diffs on settings changes. The static AutofacRegisterwiring for two keyedIPathTemplatesProviderinstances also looks correct. Only optional follow-ups would be (a) consideringDateTime-safe or null-safe handling if settings properties can ever be null, and (b) ensuring the relevantusingdirectives or global usings (Autofac,Serilog,IEventHandlernamespace, collection extensions) are present in the project so this file compiles cleanly.Also applies to: 39-56, 66-96, 98-114, 116-132
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/Papercut.Service.TrayNotification/icons/Papercut-icon.icois excluded by!**/*.ico
📒 Files selected for processing (35)
Papercut.sln(3 hunks)src/Papercut.Core/Domain/Paths/IPathTemplatesProvider.cs(1 hunks)src/Papercut.Core/Domain/Paths/MessagePathConfigurator.cs(1 hunks)src/Papercut.Core/Domain/Paths/PathConfiguratorBase.cs(5 hunks)src/Papercut.Core/Domain/Settings/JsonSettingStore.cs(1 hunks)src/Papercut.Core/Infrastructure/Consoles/ConsoleHelpers.cs(1 hunks)src/Papercut.Core/Infrastructure/Container/PapercutCoreModule.cs(1 hunks)src/Papercut.Core/Infrastructure/Logging/BootstrapLogger.cs(2 hunks)src/Papercut.Core/Infrastructure/Logging/RegisterLogger.cs(2 hunks)src/Papercut.Core/Infrastructure/Network/ServiceWebUISettingsExchangeEvent.cs(1 hunks)src/Papercut.Core/Papercut.Core.csproj(2 hunks)src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommConstants.cs(1 hunks)src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommEndpoints.cs(1 hunks)src/Papercut.Infrastructure.IPComm/PapercutIPCommClientFactory.cs(2 hunks)src/Papercut.Infrastructure.Smtp/Papercut.Infrastructure.Smtp.csproj(1 hunks)src/Papercut.Message/MessageRepository.cs(2 hunks)src/Papercut.Message/PapercutMessageModule.cs(1 hunks)src/Papercut.Service.TrayNotification/CorePathTemplateProvider.cs(1 hunks)src/Papercut.Service.TrayNotification/GlobalUsings.cs(1 hunks)src/Papercut.Service.TrayNotification/Papercut.Service.TrayNotification.csproj(1 hunks)src/Papercut.Service.TrayNotification/PapercutServiceControllerProvider.cs(1 hunks)src/Papercut.Service.TrayNotification/PapercutServiceTrayModule.cs(1 hunks)src/Papercut.Service.TrayNotification/Program.cs(1 hunks)src/Papercut.Service.TrayNotification/ServiceCommunicator.cs(1 hunks)src/Papercut.Service.TrayNotification/ServiceStatusService.cs(1 hunks)src/Papercut.Service.TrayNotification/ServiceTrayCoordinator.cs(1 hunks)src/Papercut.Service.TrayNotification/app.manifest(1 hunks)src/Papercut.Service/Infrastructure/IPComm/ReplyWithDefaultMessageSavePathService.cs(2 hunks)src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs(1 hunks)src/Papercut.Service/Infrastructure/Paths/ServerPathTemplateProviderService.cs(1 hunks)src/Papercut.Service/Papercut.Service.csproj(0 hunks)src/Papercut.Service/Program.cs(2 hunks)src/Papercut.UI/PapercutUIModule.cs(1 hunks)src/Papercut.UI/Program.cs(2 hunks)src/Papercut.UI/SettingPathTemplateProvider.cs(3 hunks)
💤 Files with no reviewable changes (1)
- src/Papercut.Service/Papercut.Service.csproj
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-25T16:50:04.096Z
Learnt from: Kissaki
Repo: ChangemakerStudios/Papercut-SMTP PR: 278
File: README.md:32-33
Timestamp: 2024-10-25T16:50:04.096Z
Learning: In future reviews of the Papercut SMTP project, when suggesting changes to the `README.md`, avoid adding version-specific information or mentioning changes between versions. The maintainers prefer to keep the documentation simple and current, with version change details included in commit descriptions.
Applied to files:
src/Papercut.Infrastructure.Smtp/Papercut.Infrastructure.Smtp.csproj
🧬 Code graph analysis (24)
src/Papercut.Infrastructure.IPComm/PapercutIPCommClientFactory.cs (1)
src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommClient.cs (1)
PapercutIPCommClient(26-132)
src/Papercut.Service/Infrastructure/Paths/ServerPathTemplateProviderService.cs (2)
src/Papercut.Service/Domain/SmtpServer/SmtpServerOptions.cs (1)
SmtpServerOptions(23-72)src/Papercut.Service.TrayNotification/CorePathTemplateProvider.cs (1)
Register(35-38)
src/Papercut.Message/PapercutMessageModule.cs (1)
src/Papercut.Message/MessageRepository.cs (1)
MessageRepository(26-200)
src/Papercut.Service.TrayNotification/CorePathTemplateProvider.cs (3)
src/Papercut.Core/Infrastructure/Logging/RegisterLogger.cs (1)
Register(45-117)src/Papercut.Service.TrayNotification/Program.cs (1)
Register(75-78)src/Papercut.Service/Infrastructure/Paths/ServerPathTemplateProviderService.cs (1)
Register(44-51)
src/Papercut.UI/Program.cs (4)
src/Papercut.Core/Domain/Application/ApplicationMeta.cs (1)
ApplicationMeta(23-28)src/Papercut.Core/AppConstants.cs (1)
AppConstants(21-38)src/Papercut.Service.TrayNotification/Program.cs (1)
STAThread(36-71)src/Papercut.Core/Infrastructure/Logging/BootstrapLogger.cs (2)
BootstrapLogger(30-88)BootstrapLogger(32-36)
src/Papercut.Message/MessageRepository.cs (2)
src/Papercut.Message/IMessageRepository.cs (1)
DeleteMessage(25-25)src/Papercut.Core/Domain/Message/MessageEntry.cs (3)
MessageEntry(29-154)MessageEntry(46-74)MessageEntry(76-79)
src/Papercut.Service/Program.cs (2)
src/Papercut.Core/Infrastructure/Consoles/ConsoleHelpers.cs (2)
ConsoleHelpers(21-40)HasConsole(23-39)src/Papercut.Core/Infrastructure/Logging/BootstrapLogger.cs (2)
BootstrapLogger(30-88)BootstrapLogger(32-36)
src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs (2)
src/Papercut.Service.TrayNotification/ServiceCommunicator.cs (1)
Task(50-85)src/Papercut.Service/Infrastructure/IPComm/ReplyWithDefaultMessageSavePathService.cs (2)
Task(26-36)Register(40-44)
src/Papercut.Service.TrayNotification/PapercutServiceControllerProvider.cs (7)
src/Papercut.Service.TrayNotification/ServiceCommunicator.cs (2)
Start(87-96)UsedImplicitly(114-120)src/Papercut.Service.TrayNotification/ServiceTrayCoordinator.cs (1)
UsedImplicitly(429-435)src/Papercut.Service.TrayNotification/ServiceStatusService.cs (1)
UsedImplicitly(176-182)src/Papercut.UI/SettingPathTemplateProvider.cs (1)
UsedImplicitly(122-132)src/Papercut.Service.TrayNotification/CorePathTemplateProvider.cs (1)
Register(35-38)src/Papercut.Service.TrayNotification/Program.cs (1)
Register(75-78)src/Papercut.Service/Program.cs (1)
Register(111-114)
src/Papercut.Core/Infrastructure/Container/PapercutCoreModule.cs (3)
src/Papercut.Core/Infrastructure/Logging/RegisterLogger.cs (1)
Register(45-117)src/Papercut.Service.TrayNotification/CorePathTemplateProvider.cs (1)
Register(35-38)src/Papercut.Service/Infrastructure/Paths/ServerPathTemplateProviderService.cs (1)
Register(44-51)
src/Papercut.Core/Domain/Paths/MessagePathConfigurator.cs (1)
src/Papercut.Core/Domain/Paths/PathConfiguratorBase.cs (2)
PathConfiguratorBase(23-155)PathConfiguratorBase(31-51)
src/Papercut.Service.TrayNotification/ServiceStatusService.cs (2)
src/Papercut.Service.TrayNotification/PapercutServiceControllerProvider.cs (6)
PapercutServiceControllerProvider(28-104)ServiceControllerStatus(37-41)Start(64-69)Stop(74-79)Restart(84-91)UsedImplicitly(95-101)src/Papercut.Service.TrayNotification/ServiceCommunicator.cs (5)
ServiceCommunicator(31-123)Start(87-96)InvalidateCache(102-106)Task(50-85)UsedImplicitly(114-120)
src/Papercut.Core/Infrastructure/Logging/BootstrapLogger.cs (3)
src/Papercut.Core/AppConstants.cs (1)
AppConstants(21-38)src/Papercut.Core/Infrastructure/Logging/EnvironmentEnricher.cs (2)
Enrich(36-56)EnvironmentEnricher(27-57)src/Papercut.Core/Infrastructure/CommandLine/ArgumentParser.cs (1)
ArgumentParser(21-78)
src/Papercut.UI/PapercutUIModule.cs (2)
src/Papercut.Core/Infrastructure/Logging/RegisterLogger.cs (1)
Register(45-117)src/Papercut.UI/Program.cs (1)
Program(29-69)
src/Papercut.Service.TrayNotification/Program.cs (6)
src/Papercut.Core/Domain/Application/ApplicationMeta.cs (1)
ApplicationMeta(23-28)src/Papercut.Core/Infrastructure/Container/SimpleContainer.cs (2)
IContainer(27-35)SimpleContainer(24-36)src/Papercut.Core/Infrastructure/Logging/BootstrapLogger.cs (2)
BootstrapLogger(30-88)BootstrapLogger(32-36)src/Papercut.Core/Infrastructure/Consoles/ConsoleHelpers.cs (2)
ConsoleHelpers(21-40)HasConsole(23-39)src/Papercut.Service.TrayNotification/PapercutServiceTrayModule.cs (1)
PapercutServiceTrayModule(27-43)src/Papercut.Service.TrayNotification/ServiceTrayCoordinator.cs (2)
ServiceTrayCoordinator(28-438)ServiceTrayCoordinator(38-70)
src/Papercut.Service.TrayNotification/ServiceTrayCoordinator.cs (5)
src/Papercut.Service.TrayNotification/ServiceStatusService.cs (10)
ServiceStatusService(28-185)UpdateStatus(43-70)GetStatusText(132-148)CanStart(153-156)CanStop(161-164)CanRestart(169-172)StartService(75-85)StopService(90-99)RestartService(104-114)UsedImplicitly(176-182)src/Papercut.Service.TrayNotification/PapercutServiceControllerProvider.cs (4)
Start(64-69)Stop(74-79)ServiceControllerStatus(37-41)UsedImplicitly(95-101)src/Papercut.Service.TrayNotification/ServiceCommunicator.cs (2)
Start(87-96)UsedImplicitly(114-120)src/Papercut.Core/Infrastructure/Logging/RegisterLogger.cs (1)
Register(45-117)src/Papercut.Service.TrayNotification/CorePathTemplateProvider.cs (1)
Register(35-38)
src/Papercut.Service.TrayNotification/PapercutServiceTrayModule.cs (3)
src/Papercut.UI/PapercutUIModule.cs (2)
Load(51-111)IEnumerable(43-49)src/Papercut.Core/Infrastructure/Container/RegisterMethodExtensions.cs (1)
RegisterStaticMethods(77-87)src/Papercut.Infrastructure.IPComm/PapercutIPCommModule.cs (1)
PapercutIPCommModule(27-39)
src/Papercut.Core/Infrastructure/Network/ServiceWebUISettingsExchangeEvent.cs (1)
src/Papercut.Core/Infrastructure/Network/BaseBindEvent.cs (1)
BaseBindEvent(23-28)
src/Papercut.Service.TrayNotification/ServiceCommunicator.cs (5)
src/Papercut.Infrastructure.IPComm/PapercutIPCommClientFactory.cs (1)
PapercutIPCommClientFactory(30-46)src/Papercut.Service.TrayNotification/ServiceStatusService.cs (1)
Task(119-122)src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs (1)
Task(31-66)src/Papercut.Common/Helper/StringHelpers.cs (1)
IsSet(42-45)src/Papercut.Core/Infrastructure/Async/AsyncHelpers.cs (1)
RunAsync(73-88)
src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommEndpoints.cs (1)
src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommConstants.cs (1)
PapercutIPCommConstants(21-30)
src/Papercut.Core/Infrastructure/Logging/RegisterLogger.cs (1)
src/Papercut.Core/Infrastructure/CommandLine/ArgumentParser.cs (1)
ArgumentParser(21-78)
src/Papercut.Core/Domain/Settings/JsonSettingStore.cs (1)
src/Papercut.Core/Domain/Settings/BaseSettingsStore.cs (1)
BaseSettingsStore(23-67)
src/Papercut.Core/Domain/Paths/PathConfiguratorBase.cs (4)
src/Papercut.Core/Infrastructure/Logging/BootstrapLogger.cs (1)
ILogger(45-81)src/Papercut.Core/Domain/Paths/PathTemplateHelper.cs (3)
RenderPathTemplate(62-99)PathTemplateHelper(25-100)PathTemplateHelper(33-60)src/Papercut.Message/MessageRepository.cs (1)
IEnumerable(90-94)src/Papercut.UI/SettingPathTemplateProvider.cs (1)
IEnumerable(57-64)
src/Papercut.UI/SettingPathTemplateProvider.cs (9)
src/Papercut.Core/Domain/Paths/PathConfiguratorBase.cs (1)
IEnumerable(122-127)src/Papercut.Common/Domain/IEventHandler.cs (1)
Task(24-24)src/Papercut.UI/Domain/Events/SettingsUpdatedEvent.cs (1)
SettingsUpdatedEvent(21-26)src/Papercut.Common/Extensions/CollectionExtensions.cs (5)
RemoveRange(44-47)RemoveRange(49-55)RemoveRange(57-70)AddRange(83-95)AddRange(97-107)src/Papercut.Service.TrayNotification/ServiceTrayCoordinator.cs (1)
UsedImplicitly(429-435)src/Papercut.UI/AppLayer/Rules/RuleService.cs (1)
UsedImplicitly(213-220)src/Papercut.UI/AppLayer/Settings/SaveSettingsOnExitService.cs (1)
UsedImplicitly(53-60)src/Papercut.Service.TrayNotification/CorePathTemplateProvider.cs (1)
Register(35-38)src/Papercut.Service/Infrastructure/Paths/ServerPathTemplateProviderService.cs (1)
Register(44-51)
⏰ 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: Build Papercut SMTP
- GitHub Check: Analyze (csharp)
- GitHub Check: Build Papercut SMTP
🔇 Additional comments (26)
src/Papercut.Infrastructure.Smtp/Papercut.Infrastructure.Smtp.csproj (2)
3-19: Formatting and consolidation changes look good.The indentation adjustments and consolidation of ItemGroup entries improve clarity without altering functionality. ProjectReferences are well-organized and target framework remains at net9.0, which is consistent with PR objectives.
11-13: Serilog dependency is available transitively — no issue found.The removal of Serilog update entries is safe because Serilog is available as a transitive dependency through Papercut.Core, which declares actual Serilog packages. No Serilog using directives were found in the Papercut.Infrastructure.Smtp source code, confirming this project doesn't directly consume Serilog. The project file changes are correct.
src/Papercut.Service/Program.cs (1)
40-45: LGTM! Console detection and bootstrap logging improvements.The change from
Environment.UserInteractivetoConsoleHelpers.HasConsole()provides more robust console detection by actually testing console accessibility. The addition ofAppMetato bootstrap logger initialization enables app-aware logging configuration.src/Papercut.Service.TrayNotification/GlobalUsings.cs (1)
19-21: LGTM! Appropriate global usings for the tray notification project.The global usings for System.Text, Serilog, and JetBrains.Annotations are reasonable choices that will be used throughout the tray notification service.
src/Papercut.Core/Domain/Settings/JsonSettingStore.cs (1)
24-28: LGTM! Clean refactor to primary constructor with app-aware path.The conversion to a primary constructor with
IAppMetaparameter and the property initializer forSettingsFilePathis a clean modernization that aligns with the app-aware configuration pattern introduced throughout the PR.src/Papercut.Service.TrayNotification/app.manifest (1)
19-19: LGTM! Administrator elevation is appropriate for service management.The
requireAdministratorexecution level is necessary for the tray application to start, stop, and manage the Papercut SMTP Windows Service, as these operations require elevated privileges.src/Papercut.Infrastructure.IPComm/PapercutIPCommClientFactory.cs (1)
23-28: LGTM! ServiceTray enum member added correctly.The addition of
ServiceTrayto thePapercutIPCommClientConnectToenum and its corresponding case in the switch statement is implemented correctly and aligns with the new tray notification service.src/Papercut.Core/Papercut.Core.csproj (1)
24-24: Serilog.Sinks.Async version 2.1.0 is current and secure.The latest stable version is 2.1.0 (released Oct 24, 2024), and there are no known security vulnerabilities for this version. The package reference is properly configured.
src/Papercut.Service.TrayNotification/Papercut.Service.TrayNotification.csproj (1)
3-25: Tray app project wiring looks consistent
WinExeoutput type,net9.0-windowsTFM,UseWindowsForms, icon/manifest, and project/package references all look appropriate for the new tray application. Just ensureicons\Papercut-icon.icoandapp.manifestexist in the expected paths in the project.src/Papercut.Core/Infrastructure/Logging/RegisterLogger.cs (1)
51-82: Centralized log path + async sinks look good; check Serilog async wiringUsing
LoggingPathConfigurator.DefaultSavePathforlogFilePathand wrapping File/Trace/Console sinks withWriteTo.Async(...)is a nice improvement and keeps logging output consistent with the configured logging path. Also, switching toEnvironment.GetCommandLineArgs()forReadFrom.KeyValuePairs(...)avoids needing to plumbargsinto this registration and should behave equivalently.Please just confirm:
- The core project references the Serilog async sink package so
WriteTo.Asyncis available at runtime.- There are no environment-specific expectations around synchronous console/trace output that would be affected by buffering.
src/Papercut.Service/Infrastructure/IPComm/ReplyWithDefaultMessageSavePathService.cs (1)
21-46: Namespace/registration changes align with new IPComm patternMoving this handler into
Papercut.Service.Infrastructure.IPCommand makingRegisterprivate static matches the newer module/registration pattern used elsewhere. The event handling logic (message path + IP/Port) remains unchanged and looks correct.Just verify that whatever scans for static registration methods includes non-public
Register(ContainerBuilder)methods so this still gets wired into the container.src/Papercut.UI/Program.cs (1)
19-39: AppMeta-based bootstrap logging integration looks correctIntroducing the static
AppMetaand passing it intoBootstrapLogger.CreateBootstrapLogger(AppMeta, args)cleanly centralizes app name/version for logging and keeps UI startup consistent with the new metadata-aware bootstrap logger.Just confirm
Assembly.GetExecutingAssembly().GetVersion()is the intended source of the displayed version (vs. product/file version) for your UX and logs.src/Papercut.Core/Infrastructure/Container/PapercutCoreModule.cs (1)
52-62: First concern is incorrect; second point remains a valid architectural consideration but isn't blockingThe primary concern—that existing consumers will break because they resolve
IPathConfiguratorwithout a key—is unfounded. Verification shows that no production consumers useIPathConfiguratorat all. Instead, all seven consumers depend directly on the concrete types (MessagePathConfiguratororLoggingPathConfigurator), which are still resolvable via the.AsSelf()registration. The keyedIPathConfiguratorregistration is purely additive and doesn't break any existing code.The second point about logger bootstrap timing is a valid architectural observation:
LoggingPathConfiguratorwill receive anILoggerat registration time (beforeLog.Loggeris reconfigured), so early logs won't go through the final configured logger. However, this appears to be working as designed and doesn't constitute a defect—only a note if you expect those logs to route through the main pipeline.Likely an incorrect or invalid review comment.
src/Papercut.Message/PapercutMessageModule.cs (1)
30-30: Cleaner DI registration via constructor injection.The refactor from factory-based registration to direct type registration simplifies the DI setup and better leverages Autofac's constructor injection capabilities.
src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommConstants.cs (1)
27-27: LGTM!The new tray service port constant follows the existing naming convention and port sequence.
src/Papercut.Service.TrayNotification/PapercutServiceTrayModule.cs (1)
27-42: LGTM!The module follows the established pattern from
PapercutUIModule, providing clean modular composition with IPComm support and static method registration.src/Papercut.Core/Domain/Paths/IPathTemplatesProvider.cs (1)
23-34: Well-structured refactoring to unify path template handling.The introduction of
PathTemplateTypeand the consolidation of separateMessagePathTemplates/LoggingPathTemplatesproperties into a singlePathTemplatescollection with aTypediscriminator simplifies the interface and enables keyed DI registrations.src/Papercut.Service.TrayNotification/Program.cs (1)
37-71: Solid tray application bootstrap with proper error handling.The entry point correctly uses
ConsoleHelpers.HasConsole(), initializes the Windows Forms application, sets up DI via Autofac, and includes comprehensive error handling with log flushing. The parameterlessApplication.Run()call at Line 55 is the appropriate pattern for tray-only applications where theServiceTrayCoordinatormanages the message loop lifecycle.src/Papercut.UI/PapercutUIModule.cs (1)
63-65: LGTM!Centralizing
AppMetain theProgramclass provides a single source of truth for application metadata across the DI container.src/Papercut.Core/Domain/Paths/MessagePathConfigurator.cs (1)
21-21: Excellent use of primary constructor syntax.The concise single-line declaration using C# 12 primary constructor syntax eliminates boilerplate while clearly expressing the class's dependency on
IPathTemplatesProviderandILogger.src/Papercut.Core/Infrastructure/Logging/BootstrapLogger.cs (1)
45-76: Bootstrap logger configuration is consistent and app-awareThe updated
CreateBootstrapLoggercorrectly incorporatesIAppMeta(for file naming and enrichment), conditionally adds Trace/Console sinks, and keeps the compact JSON file sink plus argument-driven settings. This aligns well with the rest of the logging pipeline; no issues from my side.src/Papercut.Service.TrayNotification/PapercutServiceControllerProvider.cs (1)
93-101: DI registration aligns with existing module patternThe static
RegisterusingInstancePerDependencyfor this thin wrapper matches the rest of the tray DI style and keeps per-callServiceControllerlifetime simple.src/Papercut.Service.TrayNotification/ServiceStatusService.cs (1)
174-182: Autofac registration is straightforwardSingleton registration for
ServiceStatusServicevia the staticRegistermatches the other tray services and avoids accidental multiple status pollers.src/Papercut.Service.TrayNotification/ServiceTrayCoordinator.cs (1)
339-380: Open Web UI flow is robust and aligned with cached URL behaviorThe double-check on installation and running status before opening the Web UI, plus catching generic exceptions around
Process.Start, gives a good UX and works cleanly withServiceCommunicator’s caching. This should behave well even when IPComm falls back to the default URL.src/Papercut.Service.TrayNotification/ServiceCommunicator.cs (2)
50-85: Web UI URL probing and caching logic are soundThe async probe via IPComm with a 5-minute cache and a fallback to
http://localhost:8080is straightforward and resilient: failures are logged and the tray continues to function with a reasonable default. TheCachedWebUrlproperty correctly enforces the expiration window.
108-120: DI registration matches other tray servicesRegistering
ServiceCommunicatoras a single instance and advertising bothAsSelf()and implemented interfaces fits the pattern used across the tray module and ensures a single cache/shared IPComm client.
| namespace Papercut.Core.Infrastructure.Network; | ||
|
|
||
| public class ServiceWebUISettingsExchangeEvent : BaseBindEvent; No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
Fix class declaration syntax for ServiceWebUISettingsExchangeEvent
public class ServiceWebUISettingsExchangeEvent : BaseBindEvent; is invalid C# syntax for a class declaration and will not compile. You need an empty class body:
-public class ServiceWebUISettingsExchangeEvent : BaseBindEvent;
+public class ServiceWebUISettingsExchangeEvent : BaseBindEvent
+{
+}I'll verify the C# syntax claim and examine the actual file.
🏁 Script executed:
#!/bin/bash
# Find and display the actual file contents
fd -t f "ServiceWebUISettingsExchangeEvent.cs" --exec cat {}Length of output: 770
🌐 Web query:
C# class declaration syntax semicolon instead of braces valid
💡 Result:
Short answer: No — you cannot replace the braces with a semicolon. In C# a class declaration requires a class_body (the { ... }); a single optional semicolon may appear after the closing brace, but a bare "class C;" (no braces) is invalid. [1][2]
Sources:
- C# Language Spec — class_declaration: class_body optionally followed by ';'. [1]
- Confirming note (Eric Lippert / StackOverflow): trailing semicolon after } is permitted; forward-style "class C;" is not a C# feature. [2]
Fix class declaration syntax for ServiceWebUISettingsExchangeEvent
public class ServiceWebUISettingsExchangeEvent : BaseBindEvent; is invalid C# syntax. Class declarations require a body with braces; a semicolon cannot replace them:
-public class ServiceWebUISettingsExchangeEvent : BaseBindEvent;
+public class ServiceWebUISettingsExchangeEvent : BaseBindEvent
+{
+}📝 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.
| namespace Papercut.Core.Infrastructure.Network; | |
| public class ServiceWebUISettingsExchangeEvent : BaseBindEvent; | |
| namespace Papercut.Core.Infrastructure.Network; | |
| public class ServiceWebUISettingsExchangeEvent : BaseBindEvent | |
| { | |
| } |
🤖 Prompt for AI Agents
In src/Papercut.Core/Infrastructure/Network/ServiceWebUISettingsExchangeEvent.cs
around lines 19 to 21, the class declaration uses a trailing semicolon which is
invalid C# syntax; replace the semicolon with a proper class body (e.g. change
": BaseBindEvent;" to ": BaseBindEvent { }") or add the required
members/constructors inside braces so the class has a valid body.
| public void Start() | ||
| { | ||
| logger.Debug("Startup: Attempting to IPComm to Service to get the Web UI..."); | ||
|
|
||
| var webUrl = this.GetWebUIUrlAsync().RunAsync(); | ||
|
|
||
| logger.Debug("Received WebUrl {WebUrl}", webUrl); | ||
|
|
||
| _cachedWebUrl = webUrl; | ||
| } |
There was a problem hiding this comment.
Start() cannot compile as written and doesn’t need to capture the URL directly
GetWebUIUrlAsync().RunAsync() returns void (per AsyncHelpers.RunAsync(this Task)), so assigning it to webUrl and then using webUrl will not compile, and _cachedWebUrl is already set by GetWebUIUrlAsync() itself. For IStartable.Start you only need to fire the probe and let the async method populate the cache.
public void Start()
{
logger.Debug("Startup: Attempting to IPComm to Service to get the Web UI...");
- var webUrl = this.GetWebUIUrlAsync().RunAsync();
-
- logger.Debug("Received WebUrl {WebUrl}", webUrl);
-
- _cachedWebUrl = webUrl;
+ // Warm the cache synchronously on startup; errors are already handled in GetWebUIUrlAsync.
+ GetWebUIUrlAsync().RunAsync();
+ logger.Debug("Web UI URL probe completed on startup");
}🤖 Prompt for AI Agents
In src/Papercut.Service.TrayNotification/ServiceCommunicator.cs around lines 87
to 96, Start() incorrectly assigns the result of GetWebUIUrlAsync().RunAsync()
to a variable and logs/sets it — RunAsync returns void and GetWebUIUrlAsync
already populates _cachedWebUrl. Fix by removing the webUrl variable and any use
of it: simply call GetWebUIUrlAsync().RunAsync() to fire-and-forget the probe
(optionally log a simple start/trigger message) and do not assign or log the
returned value.
| private Icon LoadIcon() | ||
| { | ||
| try | ||
| { | ||
| var iconPath = Path.Combine(AppContext.BaseDirectory, "icons", "Papercut-icon.ico"); | ||
| if (File.Exists(iconPath)) | ||
| { | ||
| return new Icon(iconPath); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Log.Warning(ex, "Failed to load custom icon, using default"); | ||
| } | ||
|
|
||
| // Fallback to default application icon | ||
| return SystemIcons.Application; | ||
| } |
There was a problem hiding this comment.
Avoid reloading icons on every status update to prevent GDI/resource leaks
LoadIcon() creates a new Icon instance (from disk when available), and UpdateTrayIcon invokes it on every timer tick. That can accumulate undisposed GDI handles over time. Consider caching a single Icon instance (or a small set if you add status-specific icons later) and reusing it, e.g., load once in the constructor and assign in UpdateTrayIcon without re-reading from disk.
- private readonly NotifyIcon _notifyIcon;
+ private readonly NotifyIcon _notifyIcon;
+ private readonly Icon _trayIcon;
@@
- _notifyIcon = new NotifyIcon
- {
- Icon = LoadIcon(),
+ _trayIcon = LoadIcon();
+
+ _notifyIcon = new NotifyIcon
+ {
+ Icon = _trayIcon,
@@
- _notifyIcon.Icon = LoadIcon();
+ _notifyIcon.Icon = _trayIcon;
@@
public void Dispose()
{
+ _trayIcon?.Dispose();
_serviceStatusService.StatusChanged -= OnServiceStatusChanged;Also applies to: 167-174
🤖 Prompt for AI Agents
In src/Papercut.Service.TrayNotification/ServiceTrayCoordinator.cs around lines
99-116 (and similarly 167-174), LoadIcon is creating a new Icon each time
UpdateTrayIcon runs which leaks GDI handles; change to load the icon once (or a
small set for different statuses) into a private readonly/Icon field during
construction (or lazy-init once), reuse that instance in UpdateTrayIcon instead
of calling LoadIcon repeatedly, and ensure the cached Icon is disposed when the
service/shutdown occurs to release GDI resources.
src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Papercut.Service.TrayNotification/ServiceTrayCoordinator.cs (1)
103-120: Cache the tray icon instead of reloading each status update (GDI/resource leak + IO overhead)
LoadIcon()creates a newIconinstance, andUpdateTrayIconassigns a fresh icon on each status update. The previously assigned icons are never disposed, which can leak GDI handles and repeatedly hit disk.This was already raised in an earlier review; consider caching a single (or small set of)
Iconinstance(s) in a readonly field, assigning it in the ctor, reusing it inUpdateTrayIcon, and disposing it inDispose().- private readonly NotifyIcon _notifyIcon; + private readonly NotifyIcon _notifyIcon; + private readonly Icon _trayIcon; @@ - _notifyIcon = new NotifyIcon - { - Icon = LoadIcon(), + _trayIcon = LoadIcon(); + + _notifyIcon = new NotifyIcon + { + Icon = _trayIcon, @@ public void Dispose() { _serviceStatusService.StatusChanged -= OnServiceStatusChanged; _statusUpdateTimer?.Stop(); _statusUpdateTimer?.Dispose(); - _notifyIcon?.Dispose(); + _trayIcon?.Dispose(); + _notifyIcon?.Dispose(); @@ private void UpdateTrayIcon() { var statusText = _serviceStatusService.GetStatusText(); _notifyIcon.Text = $"Papercut SMTP Service ({statusText})"; - // Could add different icons or overlays based on status in the future - _notifyIcon.Icon = LoadIcon(); + // Could add different icons or overlays based on status in the future + _notifyIcon.Icon = _trayIcon; }Also applies to: 180-187
🧹 Nitpick comments (2)
src/Papercut.Service.TrayNotification/AppRunOnStartupService.cs (1)
69-76: Consider creating the Run key if it’s missing instead of failingRight now, if
Registry.CurrentUser.OpenSubKey(AppStartupKey, true)returns null, the method logs an error and returns false. On a clean or unusual profile where the Run key hasn’t been created yet, you could make the feature more robust by falling back toRegistry.CurrentUser.CreateSubKey(AppStartupKey)before giving up.- using var registryKey = Registry.CurrentUser.OpenSubKey(AppStartupKey, true); - - if (registryKey == null) - { - logger.Error("Failed to open registry key {AppStartupKey}", AppStartupKey); - return false; - } + using var registryKey = + Registry.CurrentUser.OpenSubKey(AppStartupKey, writable: true) + ?? Registry.CurrentUser.CreateSubKey(AppStartupKey) + ?? throw new InvalidOperationException($"Unable to open or create registry key {AppStartupKey}");src/Papercut.Service.TrayNotification/ServiceTrayCoordinator.cs (1)
248-354: Deduplicate service control error-handling to reduce repetition
OnStartService,OnStopService, andOnRestartServicerepeat almost the sameWin32Exception/InvalidOperationException/genericExceptionhandling with similar messages. Extracting a small helper (e.g.,HandleServiceControlException(string action, Exception ex)) would reduce duplication and keep the handlers focused on the happy path.- private void OnStartService(object? sender, EventArgs e) - { - try - { - _serviceStatusService.StartService(); - ShowBalloonTip("Service Started", "Papercut SMTP Service is now running.", ToolTipIcon.Info); - } - catch (System.ComponentModel.Win32Exception ex) when (ex.NativeErrorCode == 5) // Access Denied - { - ... - } - catch (InvalidOperationException) - { - ... - } - catch (Exception ex) - { - ... - } - } + private void OnStartService(object? sender, EventArgs e) + { + try + { + _serviceStatusService.StartService(); + ShowBalloonTip("Service Started", "Papercut SMTP Service is now running.", ToolTipIcon.Info); + } + catch (Exception ex) + { + HandleServiceControlException("start", ex); + } + } @@ + private static void HandleServiceControlException(string action, Exception ex) + { + if (ex is System.ComponentModel.Win32Exception win32 && win32.NativeErrorCode == 5) + { + MessageBox.Show( + "This application must be run as Administrator to control the Papercut SMTP Service.\n\n" + + "Please close this application and restart it by right-clicking and selecting 'Run as administrator'.", + "Administrator Privileges Required", + MessageBoxButtons.OK, + MessageBoxIcon.Warning); + return; + } + + if (ex is InvalidOperationException) + { + MessageBox.Show( + "The Papercut SMTP Service could not be found or accessed.\n\n" + + "Please ensure the service is installed.", + "Service Not Found", + MessageBoxButtons.OK, + MessageBoxIcon.Error); + return; + } + + MessageBox.Show( + $"Failed to {action} service: {ex.Message}\n\n" + + "Make sure you have administrator privileges and the service is installed.", + $"Error {char.ToUpper(action[0]) + action[1..]}ing Service", + MessageBoxButtons.OK, + MessageBoxIcon.Error); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Papercut.Service.TrayNotification/AppRunOnStartupService.cs(1 hunks)src/Papercut.Service.TrayNotification/ServiceTrayCoordinator.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Papercut.Service.TrayNotification/ServiceTrayCoordinator.cs (7)
src/Papercut.Service.TrayNotification/ServiceStatusService.cs (10)
ServiceStatusService(28-185)UpdateStatus(43-70)GetStatusText(132-148)CanStart(153-156)CanStop(161-164)CanRestart(169-172)StartService(75-85)StopService(90-99)RestartService(104-114)UsedImplicitly(176-182)src/Papercut.Service.TrayNotification/AppRunOnStartupService.cs (4)
AppRunOnStartupService(32-157)IsRunOnStartupEnabled(39-60)SetRunOnStartup(65-144)UsedImplicitly(148-154)src/Papercut.Service.TrayNotification/PapercutServiceControllerProvider.cs (4)
Start(64-69)Stop(74-79)ServiceControllerStatus(37-41)UsedImplicitly(95-101)src/Papercut.Service.TrayNotification/ServiceCommunicator.cs (2)
Start(87-96)UsedImplicitly(114-120)src/Papercut.Core/Infrastructure/Logging/RegisterLogger.cs (1)
Register(45-117)src/Papercut.Service.TrayNotification/CorePathTemplateProvider.cs (1)
Register(35-38)src/Papercut.Service.TrayNotification/Program.cs (1)
Register(75-78)
src/Papercut.Service.TrayNotification/AppRunOnStartupService.cs (3)
src/Papercut.Core/Infrastructure/Logging/BootstrapLogger.cs (1)
ILogger(45-81)src/Papercut.UI/Helpers/CalburnSerilogBridge.cs (1)
Error(40-43)src/Papercut.Service.TrayNotification/ServiceTrayCoordinator.cs (1)
UsedImplicitly(473-479)
⏰ 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 Papercut SMTP
- GitHub Check: Build Papercut SMTP
🔇 Additional comments (3)
src/Papercut.Service.TrayNotification/AppRunOnStartupService.cs (1)
36-60: Run-on-startup implementation and error handling look solidRegistry access, state detection via
Environment.ProcessPath, and the split between logging-only inIsRunOnStartupEnabledvs. user-facing MessageBox inSetRunOnStartupare all reasonable and consistent with the tray UX. The DI registration pattern matches the rest of the project’s staticRegisterconventions.Also applies to: 65-144
src/Papercut.Service.TrayNotification/ServiceTrayCoordinator.cs (2)
40-74: Coordinator lifecycle wiring and polling strategy look appropriateConstructor wiring (NotifyIcon creation, context menu, status subscription, and 2-second polling timer) and the
Disposecleanup (unsubscribing and disposing timer/icon) are consistent and should behave correctly over the app’s lifetime.Also applies to: 80-82
189-241: Menu state handling and run-on-startup toggle logic are consistentUsing named menu items and
UpdateMenuStateto centralize label text, enablement, and the run-on-startup checkbox state keeps the tray menu logic easy to follow. The toggle handler’s pattern of reverting the checkbox whenSetRunOnStartupfails (or throws) is a nice touch for keeping UI and registry state consistent.Also applies to: 429-454
- Created PublishNewMessageToTrayService in Service project to forward NewMessageEvent to tray - Uses fire-and-forget pattern since tray might not be running - Created NewMessageNotificationService in tray project - Handles NewMessageEvent notifications - Provides NotificationsEnabled property for user control - Updated ServiceTrayCoordinator - Added "Show Notifications" checkbox menu item (default enabled) - Displays balloon tip notification when new messages arrive - Extracts subject from filename format - Added Papercut.Common project reference for IEventHandler interface - Fixed PathConfiguratorBase readonly field issue to allow dynamic path updates
- Load icon once during construction instead of creating new Icon on every UpdateTrayIcon call - Store icon in readonly field _trayIcon and reuse throughout lifetime - Dispose cached icon in Dispose() to release GDI resources - Prevents GDI handle exhaustion from repeated Icon allocations
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
src/Papercut.Core/Domain/Paths/PathConfiguratorBase.cs (2)
116-121: Add logging to path change handler for observability.The constructor logs
DefaultSavePath(lines 44-46) andLoadPaths(line 50) after initialization, but this handler silently updates both properties. Runtime path changes are difficult to diagnose without logs.Apply this diff to add logging:
private void PathTemplatesCollectionChanged(object? sender, NotifyCollectionChangedEventArgs e) { - LoadPaths = RenderLoadPaths().Where(PathExists).ToList(); - DefaultSavePath = GetValidDefaultSavePath(LoadPaths); + var paths = RenderLoadPaths().ToList(); + DefaultSavePath = GetValidDefaultSavePath(paths); + + _logger.Information( + "Default Save Path updated to {DefaultSavePath}", + DefaultSavePath); + + LoadPaths = paths.Where(PathExists).ToArray(); + + _logger.Information("Load Paths updated to {@LoadPaths}", LoadPaths); + OnRefreshLoadPath(); }
130-134: Consider modernizing event invocation pattern.The event invocation pattern can be simplified using the null-conditional operator.
Apply this diff:
protected virtual void OnRefreshLoadPath() { - EventHandler handler = RefreshLoadPath; - handler?.Invoke(this, EventArgs.Empty); + RefreshLoadPath?.Invoke(this, EventArgs.Empty); }src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs (1)
40-42: Consider documenting the first-address selection.The code uses the first address when multiple bindings exist. For most scenarios this is fine, but if Kestrel is bound to multiple addresses (e.g., multiple interfaces or protocols), the first address might not always be the most appropriate choice for the tray client.
You could add a comment explaining this behavior, or consider selecting the most appropriate address (e.g., prefer loopback addresses for local clients, or match the protocol/interface):
if (addressesFeature?.Addresses != null && addressesFeature.Addresses.Any()) { - // Get the first address (typically the primary binding) + // Get the first address (note: if multiple bindings exist, + // the first may not always be most appropriate for local access) var firstAddress = addressesFeature.Addresses.First();Alternatively, prefer localhost/loopback addresses when available:
var addresses = addressesFeature.Addresses.ToList(); var firstAddress = addresses.FirstOrDefault(a => a.Contains("localhost") || a.Contains("127.0.0.1")) ?? addresses.First();src/Papercut.Service.TrayNotification/AppLayer/NewMessageNotificationService.cs (2)
26-40: Public surface and toggle behavior look good; consider a tiny simplificationThe overall shape (event +
NotificationsEnabledflag defaulting to true) is clear and fits a tray‑notification coordinator nicely. If you want to trim a bit of boilerplate, you could makeNotificationsEnabledan auto‑property with an initializer and drop the backing field:- private bool _notificationsEnabled = true; - - public bool NotificationsEnabled - { - get => _notificationsEnabled; - set => _notificationsEnabled = value; - } + public bool NotificationsEnabled { get; set; } = true;Purely cosmetic; current code is fine as is.
41-60:HandleAsyncbehavior is reasonable; minor async/cancellation considerationsThe handler correctly short‑circuits when notifications are disabled, logs the event, raises a .NET event, and swallows/logs any subscriber exceptions so they don’t break the pipeline. A couple of minor points you might optionally tweak:
- The method is declared async‑capable (
Task HandleAsync(..., CancellationToken)), but is entirely synchronous and ignores theCancellationToken. If this stays CPU‑cheap, that’s fine, but you could:
- Either remove the token from the interface (if you control it), or
- At least call
token.ThrowIfCancellationRequested()at the top to respect upstream cancellation paths.- If you expect any heavier work here in the future, it may be worth actually making this
asyncand usingawait(or returning aValueTask) so callers don’t assume it’s always trivial.All of this is optional; functionally it’s sound for the current usage.
If
IEventHandler<NewMessageEvent>has other async implementations in the solution, you may want to double‑check its documentation/usage to ensure it’s OK for handlers to ignoreCancellationTokenand always returnTask.CompletedTask.src/Papercut.Service.TrayNotification/AppLayer/TrayLoggingPathConfigurator.cs (1)
26-37: Normalize logging path template and align static registration with other servicesTwo small improvements here:
- The initial template
"%DataDirectory%\\\\Logs"will produce%DataDirectory%\\Logsat runtime (double backslash). Windows will generally accept it, but"%DataDirectory%\\Logs"is cleaner and more consistent with typical path templates.- Other services’ static
Registermethods (e.g.,ServiceCommunicator,AppRunOnStartupService) use[UsedImplicitly]and guard against a nullContainerBuilder. Mirroring that pattern here keeps the static-registration convention uniform and helps analyzers.A possible tweak:
- public ObservableCollection<string> PathTemplates { get; } = new(["%DataDirectory%\\\\Logs"]); + public ObservableCollection<string> PathTemplates { get; } = new(["%DataDirectory%\\Logs"]); @@ - private static void Register(ContainerBuilder builder) - { - builder.RegisterType<TrayLoggingPathConfigurator>().Keyed<IPathTemplatesProvider>(PathTemplateType.Logging).SingleInstance(); - } + [UsedImplicitly] + private static void Register(ContainerBuilder builder) + { + if (builder == null) throw new ArgumentNullException(nameof(builder)); + + builder.RegisterType<TrayLoggingPathConfigurator>() + .Keyed<IPathTemplatesProvider>(PathTemplateType.Logging) + .SingleInstance(); + }src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs (1)
30-90: Unify logging usage and be aware thatStart()blocks while probing the Web UIThe behavior looks correct overall (5‑minute cache, safe fallback to
http://localhost:8080), but two details are worth tightening:
GetWebUIUrlAsynclogs via staticLog.Warning/Log.DebugwhileStart()uses the injectedlogger. For consistency and easier testing/configuration, it’s cleaner to use the injected logger everywhere:- catch (Exception ex) - { - Log.Warning(ex, "Failed to probe for Papercut Service Listening Web Url"); - } - - // Fallback to default - Log.Debug("Using fallback web URL: {Url}", FallbackWebUrl); + catch (Exception ex) + { + logger.Warning(ex, "Failed to probe for Papercut Service Listening Web Url"); + } + + // Fallback to default + logger.Debug("Using fallback web URL: {Url}", FallbackWebUrl);
Start()callsGetWebUIUrlAsync().RunAsync(), andAsyncHelpers.RunAsyncusestask.Wait(). That means container startup will synchronously wait up to the 2‑second IPComm timeout when the service isn’t reachable. If the intent is a true fire‑and‑forget probe that doesn’t delay tray startup, consider offloading it (e.g., viaTask.Runor another non‑blocking helper) instead ofRunAsync.src/Papercut.Service/Infrastructure/IPComm/PublishNewMessageToTrayService.cs (1)
27-61: SimplifyHandleAsyncasync pattern and consider client disposalThe fire‑and‑forget behavior to avoid blocking the caller makes sense, but a couple of small cleanups are possible:
HandleAsyncis markedasyncbut onlyawaitsTask.CompletedTask; all real async work happens insideTask.Run. You can drop theasync/awaitoverhead and just return a completed task:-public class PublishNewMessageToTrayService( - PapercutIPCommClientFactory ipCommClientFactory, - ILogger logger) : IEventHandler<NewMessageEvent> -{ - public async Task HandleAsync(NewMessageEvent @event, CancellationToken token = default) - { +public class PublishNewMessageToTrayService( + PapercutIPCommClientFactory ipCommClientFactory, + ILogger logger) : IEventHandler<NewMessageEvent> +{ + public Task HandleAsync(NewMessageEvent @event, CancellationToken token = default) + { @@ - catch (Exception ex) - { - logger.Debug(ex, "Failed to get tray client for new message notification"); - } - - await Task.CompletedTask; + catch (Exception ex) + { + logger.Debug(ex, "Failed to get tray client for new message notification"); + } + + return Task.CompletedTask; }
- If
PapercutIPCommClientimplementsIDisposable, it would be safer to disposetrayClientinside theTask.Rundelegate afterPublishEventServercompletes (e.g., via ausingin the lambda) to avoid leaking connections over time.src/Papercut.Service.TrayNotification/Infrastructure/AppRunOnStartupService.cs (1)
64-83: MakeSetRunOnStartupresilient when the Run key is missingThe overall run‑at‑startup logic is sound, but
SetRunOnStartupcurrently returns false ifHKCU\{AppStartupKey}doesn’t exist. On fresh profiles that’s a bit brittle; you can create the key on demand instead:- using var registryKey = Registry.CurrentUser.OpenSubKey(AppStartupKey, true); - - if (registryKey == null) - { - logger.Error("Failed to open registry key {AppStartupKey}", AppStartupKey); - return false; - } + using var registryKey = + Registry.CurrentUser.OpenSubKey(AppStartupKey, writable: true) + ?? Registry.CurrentUser.CreateSubKey(AppStartupKey, writable: true); + + if (registryKey == null) + { + logger.Error("Failed to open or create registry key {AppStartupKey}", AppStartupKey); + return false; + }This keeps the current behavior but avoids failing just because the
Runkey wasn’t present yet.src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceControllerProvider.cs (2)
48-59: Consider broadening installation check diagnostics
IsInstalledcurrently treats onlyInvalidOperationExceptionas “not installed.” Other failures (e.g., access issues) are silently ignored by callers and only surface where you explicitly catch them. Consider either logging non-InvalidOperationExceptionerrors here, or exposing an API that can distinguish “not installed” from “error checking status” if you ever need richer UX around failure modes.
64-91: Service start/stop/restart are synchronous and may block the UI
Start,Stop, andRestartblock until the service reaches the target status or the 30‑second timeout. Since these are invoked from UI event handlers inServiceTrayCoordinator, the tray UI will be unresponsive during long transitions. If you see noticeable freezes, consider offloading these calls to a background task and driving the UI solely viaServiceStatusService.UpdateStatus()updates.src/Papercut.Service.TrayNotification/Infrastructure/ServiceStatusService.cs (1)
132-147: Ambiguity inStatusChangedpayload for “not installed”When the service is not installed,
StatusChangedis raised withdefault(ServiceControllerStatusvalue 0), which is indistinguishable from a true “Stopped” state on the event argument alone. Today, consumers rely onIsServiceInstalledto differentiate, but if more subscribers appear later you may want to either:
- change the event to carry a richer payload (e.g., custom args including
IsServiceInstalled), or- make the event type nullable and send
nullfor “not installed.”
Not urgent, but worth keeping in mind as you add more listeners.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/Papercut.Core/Domain/Paths/PathConfiguratorBase.cs(4 hunks)src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommEndpoints.cs(1 hunks)src/Papercut.Service.TrayNotification/AppLayer/NewMessageNotificationService.cs(1 hunks)src/Papercut.Service.TrayNotification/AppLayer/ServiceTrayCoordinator.cs(1 hunks)src/Papercut.Service.TrayNotification/AppLayer/TrayLoggingPathConfigurator.cs(1 hunks)src/Papercut.Service.TrayNotification/Infrastructure/AppRunOnStartupService.cs(1 hunks)src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceControllerProvider.cs(1 hunks)src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceTrayModule.cs(1 hunks)src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs(1 hunks)src/Papercut.Service.TrayNotification/Infrastructure/ServiceStatusService.cs(1 hunks)src/Papercut.Service.TrayNotification/Papercut.Service.TrayNotification.csproj(1 hunks)src/Papercut.Service.TrayNotification/Program.cs(1 hunks)src/Papercut.Service/Infrastructure/IPComm/PublishNewMessageToTrayService.cs(1 hunks)src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Papercut.Service.TrayNotification/Program.cs
- src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommEndpoints.cs
🧰 Additional context used
🧬 Code graph analysis (10)
src/Papercut.Service/Infrastructure/IPComm/PublishNewMessageToTrayService.cs (3)
src/Papercut.Infrastructure.IPComm/PapercutIPCommClientFactory.cs (1)
PapercutIPCommClientFactory(30-46)src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs (2)
Task(49-84)UsedImplicitly(108-114)src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs (2)
Task(31-66)UsedImplicitly(70-75)
src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs (6)
src/Papercut.Infrastructure.IPComm/PapercutIPCommClientFactory.cs (1)
PapercutIPCommClientFactory(30-46)src/Papercut.Service.TrayNotification/Infrastructure/ServiceStatusService.cs (1)
Task(119-122)src/Papercut.Service/Infrastructure/IPComm/PublishNewMessageToTrayService.cs (1)
Task(31-61)src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs (1)
Task(31-66)src/Papercut.Common/Helper/StringHelpers.cs (1)
IsSet(42-45)src/Papercut.Core/Infrastructure/Async/AsyncHelpers.cs (1)
RunAsync(73-88)
src/Papercut.Service.TrayNotification/AppLayer/ServiceTrayCoordinator.cs (6)
src/Papercut.Service.TrayNotification/Infrastructure/AppRunOnStartupService.cs (4)
AppRunOnStartupService(31-156)IsRunOnStartupEnabled(38-59)SetRunOnStartup(64-143)UsedImplicitly(147-153)src/Papercut.Service.TrayNotification/Infrastructure/ServiceStatusService.cs (10)
ServiceStatusService(28-185)UpdateStatus(43-70)GetStatusText(132-148)CanStart(153-156)CanStop(161-164)CanRestart(169-172)StartService(75-85)StopService(90-99)RestartService(104-114)UsedImplicitly(176-182)src/Papercut.Service.TrayNotification/AppLayer/NewMessageNotificationService.cs (1)
NewMessageNotificationService(29-73)src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceControllerProvider.cs (4)
Start(64-69)Stop(74-79)ServiceControllerStatus(37-41)UsedImplicitly(95-101)src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs (2)
Start(86-90)UsedImplicitly(108-114)src/Papercut.Service.TrayNotification/AppLayer/TrayLoggingPathConfigurator.cs (1)
Register(34-37)
src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceControllerProvider.cs (8)
src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs (2)
Start(86-90)UsedImplicitly(108-114)src/Papercut.Service.TrayNotification/AppLayer/ServiceTrayCoordinator.cs (1)
UsedImplicitly(546-552)src/Papercut.Service.TrayNotification/Infrastructure/AppRunOnStartupService.cs (1)
UsedImplicitly(147-153)src/Papercut.Service.TrayNotification/Infrastructure/ServiceStatusService.cs (1)
UsedImplicitly(176-182)src/Papercut.Service/Infrastructure/IPComm/PublishNewMessageToTrayService.cs (1)
UsedImplicitly(65-70)src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs (1)
UsedImplicitly(70-75)src/Papercut.Service.TrayNotification/AppLayer/TrayLoggingPathConfigurator.cs (1)
Register(34-37)src/Papercut.Service.TrayNotification/Program.cs (1)
Register(79-82)
src/Papercut.Service.TrayNotification/Infrastructure/AppRunOnStartupService.cs (4)
src/Papercut.Message/Helpers/MessageHelper.cs (1)
System(59-70)src/Papercut.Core/Infrastructure/Logging/BootstrapLogger.cs (1)
ILogger(45-81)src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs (1)
UsedImplicitly(108-114)src/Papercut.Service.TrayNotification/Program.cs (1)
Register(79-82)
src/Papercut.Service.TrayNotification/AppLayer/NewMessageNotificationService.cs (2)
src/Papercut.UI/ViewModels/MessageListViewModel.cs (1)
NewMessage(319-322)src/Papercut.UI/Helpers/CalburnSerilogBridge.cs (1)
Error(40-43)
src/Papercut.Service.TrayNotification/Infrastructure/ServiceStatusService.cs (2)
src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceControllerProvider.cs (6)
PapercutServiceControllerProvider(28-104)ServiceControllerStatus(37-41)Start(64-69)Stop(74-79)Restart(84-91)UsedImplicitly(95-101)src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs (5)
ServiceCommunicator(30-117)Start(86-90)InvalidateCache(96-100)Task(49-84)UsedImplicitly(108-114)
src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceTrayModule.cs (2)
src/Papercut.Core/Infrastructure/Container/RegisterMethodExtensions.cs (1)
RegisterStaticMethods(77-87)src/Papercut.Infrastructure.IPComm/PapercutIPCommModule.cs (1)
PapercutIPCommModule(27-39)
src/Papercut.Core/Domain/Paths/PathConfiguratorBase.cs (3)
src/Papercut.Message/MessageRepository.cs (1)
IEnumerable(90-94)src/Papercut.UI/SettingPathTemplateProvider.cs (1)
IEnumerable(57-64)src/Papercut.Core/Domain/Paths/PathTemplateHelper.cs (3)
RenderPathTemplate(62-99)PathTemplateHelper(25-100)PathTemplateHelper(33-60)
src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs (3)
src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs (2)
Task(49-84)UsedImplicitly(108-114)src/Papercut.Service/Infrastructure/IPComm/PublishNewMessageToTrayService.cs (2)
Task(31-61)UsedImplicitly(65-70)src/Papercut.Service/Infrastructure/IPComm/ReplyWithDefaultMessageSavePathService.cs (2)
Task(26-36)Register(40-44)
⏰ 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: Build Papercut SMTP
- GitHub Check: Analyze (csharp)
- GitHub Check: Build Papercut SMTP
🔇 Additional comments (17)
src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs (1)
31-66: Early return fix correctly addresses the previous critical issue.The addition of the early return at line 52 ensures that the fallback values at lines 62-63 are only applied when discovery fails (no addresses found or exception thrown). This correctly preserves the discovered IP and Port values.
However, there is a potential issue with port validation — see the next comment.
src/Papercut.Service.TrayNotification/AppLayer/NewMessageNotificationService.cs (1)
62-70: Autofac registration matches the pattern; singleton lifetime is appropriateThe private
[UsedImplicitly]Registermethod andbuilder .RegisterType<NewMessageNotificationService>() .AsImplementedInterfaces() .AsSelf() .SingleInstance();are consistent with the common pattern in this codebase. Using
SingleInstance()makes sense here since you want a single notifier that others can subscribe to. No changes required.If other tray‑side services are using a different lifetime (e.g., per‑scope), it’s worth confirming that a singleton here aligns with how
IEventHandler<NewMessageEvent>is consumed throughout the app.src/Papercut.Service.TrayNotification/Papercut.Service.TrayNotification.csproj (1)
3-25: Project configuration for the tray app looks consistentTarget framework, WinExe output type, Windows Forms enablement, and icon/manifest wiring all line up with a Windows tray executable; I don’t see anything that needs changing here.
src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceTrayModule.cs (1)
26-41: DI module wiring matches the existing Autofac patternComposing
PapercutIPCommModuleand then callingRegisterStaticMethods(ThisAssembly)lines up with the rest of the container setup; the tray assembly will pick up all its staticRegisterhelpers as intended. No changes needed from my side here.src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceControllerProvider.cs (2)
30-43: Service controller wrapper looks correct and focused
GetStatusandGetControllerfollow the right pattern of creating and disposingServiceControllerinstances, and the fixedServiceNamekeeps the provider simple for this one service. No functional issues spotted here.
93-101: DI registration matches project patternThe static
Registermethod follows the existing Autofac pattern and usesInstancePerDependencyfor a stateless provider, which is appropriate.src/Papercut.Service.TrayNotification/Infrastructure/ServiceStatusService.cs (5)
40-70: Status tracking and eventing are solid
UpdateStatuscleanly handles the normal and “service not installed” paths, only raisesStatusChangedwhen something actually changes, and shields callers from exceptions while logging unexpected failures. This is a good central place for polling and state normalization.
75-114: Service control methods correctly gate on current status
StartService,StopService, andRestartServiceall re-check the current status before calling intoPapercutServiceControllerProvider, which avoids obvious invalid-operation paths, and you invalidate the web‑URL cache whenever the service is started or restarted. That sequencing is sensible and matches howServiceTrayCoordinatoruses the service.
116-147: Web UI URL helpers are a thin, appropriate abstractionForwarding
GetWebUIUrlAsyncand exposingCachedWebUIUrlkeeps tray code from depending directly onServiceCommunicatorwhile still leveraging its cache semantics. This is a clean separation of concerns.
150-172: Status helper methods line up with UI expectations
GetStatusTextand theCanStart/CanStop/CanRestarthelpers give the tray coordinator exactly what it needs without duplicating logic on the UI side. The mappings and conditions are consistent with how the context menu is updated.
174-182: Singleton DI registration is appropriateRegistering
ServiceStatusServiceas aSingleInstancematches its intended role as the central in‑memory source of truth for status and cached URL data.src/Papercut.Service.TrayNotification/AppLayer/ServiceTrayCoordinator.cs (6)
45-87: Tray initialization and disposal look correctYou initialize the icon once, reuse it for the
NotifyIcon, hook up events, start a WinForms timer on the UI thread, and perform an initial status update.Disposecleanly unsubscribes and disposes timer and icon resources, which should avoid GDI handle leaks and dangling tray icons.Also applies to: 89-97
127-144: Status-change handling and URL prefetch are well-structured
OnServiceStatusChangedimmediately refreshes the tray tooltip text and only kicks off the (fire‑and‑forget) web‑URL prefetch when the service reportsRunning. With the internal try/catch around the async call, you avoid unobserved exceptions from thisasync voidhandler.
146-229: Context menu construction and dynamic state updates are cohesive
CreateContextMenusets up a clear, discoverable menu, andUpdateMenuStatecentralizes all the enable/disable and label text logic based onIsServiceInstalled,CurrentStatus, and the notification/startup flags. ReusingCachedWebUIUrlto show the URL inline when available is a nice touch and avoids extra IPComm calls while keeping the UX responsive.Also applies to: 231-296
298-409: Service control handlers provide good UX and error mapping
OnStartService,OnStopService, andOnRestartServicedelegate toServiceStatusServiceand then surface access-denied, missing-service, and generic failures with clear, actionableMessageBoxtext. The success balloon tips give quick feedback without being intrusive.
411-452: Web UI, logs, and toggle handlers are robust
OnOpenWebUIvalidates installation and running state before resolving the URL and defers to the shell for opening it.OnViewLogsensures the logs directory exists before launching Explorer and logs both success and failure. The run‑on‑startup and notifications toggles keep menu state and underlying services in sync, reverting the checkbox when operations fail. All of this is defensive and user-friendly.Also applies to: 454-527
529-553: Exit handling and DI registration are straightforward
OnExitfunnels shutdown throughApplication.Exit(), and the static Autofac registration mirrors the pattern used elsewhere in the tray app, registeringServiceTrayCoordinatoras a singleton orchestration component.
| private void PathTemplatesCollectionChanged(object? sender, NotifyCollectionChangedEventArgs e) | ||
| { | ||
| this.RenderLoadPaths(); | ||
| this.OnRefreshLoadPath(); | ||
| LoadPaths = RenderLoadPaths().Where(PathExists).ToList(); | ||
| DefaultSavePath = GetValidDefaultSavePath(LoadPaths); | ||
| OnRefreshLoadPath(); | ||
| } |
There was a problem hiding this comment.
Fix inconsistent path filtering logic between constructor and handler.
The handler filters paths to only existing directories (line 118) before calling GetValidDefaultSavePath (line 119), but the constructor passes all rendered paths to GetValidDefaultSavePath before filtering (lines 41-48). Since GetValidDefaultSavePath can create non-existent directories via IsSavePathIsValid (line 104), this inconsistency means runtime path template changes won't consider creatable-but-non-existent paths for DefaultSavePath, while initialization does. This could cause unexpected behavior when users update path templates at runtime.
Apply this diff to match constructor logic:
private void PathTemplatesCollectionChanged(object? sender, NotifyCollectionChangedEventArgs e)
{
- LoadPaths = RenderLoadPaths().Where(PathExists).ToList();
- DefaultSavePath = GetValidDefaultSavePath(LoadPaths);
+ var paths = RenderLoadPaths().ToList();
+ DefaultSavePath = GetValidDefaultSavePath(paths);
+ LoadPaths = paths.Where(PathExists).ToArray();
OnRefreshLoadPath();
}📝 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.
| private void PathTemplatesCollectionChanged(object? sender, NotifyCollectionChangedEventArgs e) | |
| { | |
| this.RenderLoadPaths(); | |
| this.OnRefreshLoadPath(); | |
| LoadPaths = RenderLoadPaths().Where(PathExists).ToList(); | |
| DefaultSavePath = GetValidDefaultSavePath(LoadPaths); | |
| OnRefreshLoadPath(); | |
| } | |
| private void PathTemplatesCollectionChanged(object? sender, NotifyCollectionChangedEventArgs e) | |
| { | |
| var paths = RenderLoadPaths().ToList(); | |
| DefaultSavePath = GetValidDefaultSavePath(paths); | |
| LoadPaths = paths.Where(PathExists).ToArray(); | |
| OnRefreshLoadPath(); | |
| } |
🤖 Prompt for AI Agents
In src/Papercut.Core/Domain/Paths/PathConfiguratorBase.cs around lines 116 to
121, the collection-changed handler filters rendered paths for existence before
computing DefaultSavePath, which differs from the constructor behavior; change
the handler to first get the full rendered path list, pass that unfiltered list
to GetValidDefaultSavePath (so creatable-but-non-existent paths are considered),
then set LoadPaths to the subset that exists and call OnRefreshLoadPath.
| private void OnNewMessageReceived(object? sender, Core.Domain.Message.NewMessageEvent e) | ||
| { | ||
| if (!_notificationService.NotificationsEnabled) | ||
| return; | ||
|
|
||
| try | ||
| { | ||
| // Extract subject from filename format: {timestamp} {subject} {randomstring}.eml | ||
| var fileName = Path.GetFileNameWithoutExtension(e.NewMessage.Name); | ||
| var parts = fileName.Split(' ', 3); // Split into timestamp, subject, random | ||
| var subject = parts.Length >= 2 | ||
| ? string.Join(" ", parts.Skip(1).Take(parts.Length - 2)) | ||
| : "(No Subject)"; | ||
|
|
||
| if (string.IsNullOrWhiteSpace(subject)) | ||
| subject = "(No Subject)"; | ||
|
|
||
| ShowBalloonTip( | ||
| "New Email Received", | ||
| $"Subject: {subject}", | ||
| ToolTipIcon.Info); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Log.Error(ex, "Failed to show new message notification"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Subject parsing for new-message notifications drops most of the subject
The logic in OnNewMessageReceived doesn’t match the documented {timestamp} {subject} {randomstring}.eml pattern. With Split(' ', 3), the third element contains "restOfSubject randomstring", and then string.Join with Take(parts.Length - 2) only picks the second element. For multi‑word subjects you end up with just the first word, and for two‑token filenames you fall back to “(No Subject)”.
Consider parsing by stripping the leading timestamp and trailing random token instead, for example:
- var fileName = Path.GetFileNameWithoutExtension(e.NewMessage.Name);
- var parts = fileName.Split(' ', 3); // Split into timestamp, subject, random
- var subject = parts.Length >= 2
- ? string.Join(" ", parts.Skip(1).Take(parts.Length - 2))
- : "(No Subject)";
+ var fileName = Path.GetFileNameWithoutExtension(e.NewMessage.Name);
+
+ string subject;
+ var firstSpaceIndex = fileName.IndexOf(' ');
+ if (firstSpaceIndex > 0)
+ {
+ // Strip leading timestamp
+ var withoutTimestamp = fileName[(firstSpaceIndex + 1)..];
+
+ // Strip trailing random segment (last space-delimited token)
+ var lastSpaceIndex = withoutTimestamp.LastIndexOf(' ');
+ subject = lastSpaceIndex > 0
+ ? withoutTimestamp[..lastSpaceIndex]
+ : withoutTimestamp;
+ }
+ else
+ {
+ subject = "(No Subject)";
+ }The existing string.IsNullOrWhiteSpace(subject) check then still handles degenerate cases.
src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs
Show resolved
Hide resolved
- Check if uri.Port is -1 (no explicit port specified) - Use fallback port 8080 when uri.Port is -1 - Prevents tray service from receiving invalid port value - Keeps IP handling and logging logic unchanged
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
src/Papercut.Core/Domain/Settings/ReadWriteValueExtensions.cs (1)
69-80: Remove redundant null-conditional operator.Line 78 uses
readValue?.Get(key)after already null-checkingreadValueat line 74. IfreadValueis null, the code throws an exception before reaching line 78, making the?operator unnecessary.Apply this diff to simplify:
- var value = readValue?.Get(key); + var value = readValue.Get(key);src/Papercut.Service.TrayNotification/AppLayer/NewMessageNotificationService.cs (1)
30-38: Consider thread-safe access for_notificationsEnabled.The
_notificationsEnabledfield may be accessed from multiple threads (UI thread for toggling, IPComm thread for checking). Consider usingvolatileorInterlockedfor safe concurrent access.Apply this diff to add thread-safe access:
- private bool _notificationsEnabled = true; + private volatile bool _notificationsEnabled = true;Alternatively, for more explicit control:
+ private int _notificationsEnabled = 1; // 1 = enabled, 0 = disabled public bool NotificationsEnabled { - get => _notificationsEnabled; - set => _notificationsEnabled = value; + get => Interlocked.CompareExchange(ref _notificationsEnabled, 0, 0) == 1; + set => Interlocked.Exchange(ref _notificationsEnabled, value ? 1 : 0); }src/Papercut.Service/Infrastructure/Configuration/SmtpServerOptionsMerger.cs (2)
37-47: Consider making lazy accessors private.The
SettingStoreandLoggerproperties areprotected, allowing inheritance, but there's no indication this class is designed for extension. Making themprivatewould better encapsulate the implementation and clarify intent.- protected ISettingStore SettingStore => _settingStore.Value; + private ISettingStore SettingStore => _settingStore.Value; - protected ILogger Logger => _logger.Value; + private ILogger Logger => _logger.Value;
103-106: Partial state on exception.The catch-all exception handler returns the options object after logging a warning, but if an exception occurs partway through the merge (e.g., after applying
IPbut before applyingPort), the options will be in a partially modified state. This may be acceptable for a "best effort" merge, but callers have no indication that the merge was incomplete.Consider one of these approaches:
- Return null or throw on failure to signal incomplete merge:
catch (Exception ex) { Logger.Warning(ex, "Failed to initialize SMTP server options from persisted settings"); + return null; // or throw; }
- Apply all settings in a temporary variable first, then copy to the result only if all succeed:
try { + var tempOptions = new { IP = persistedIp, Port = persistedPort, /* ... */ }; + // Apply all at once after all reads succeed + smtpServerOptions.IP = tempOptions.IP; + // ... }src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommServer.cs (1)
75-98: Async wrappers and cancellation tokens are currently no-opsBoth
StopAsyncandStartAsyncare effectively synchronous and only useawait Task.CompletedTask;to satisfy the async/Task signature, and the newCancellationTokenparameters (including onCreateListener) are not observed.Consider simplifying and making the methods explicitly synchronous while still returning
Task, and/or wiring the token into your shutdown semantics so callers get meaningful cancellation:- public async Task StopAsync(CancellationToken token = default) - { - await Task.CompletedTask; - - if (!this.IsActive) return; + public Task StopAsync(CancellationToken token = default) + { + if (!this.IsActive) return Task.CompletedTask; @@ - } + } + + return Task.CompletedTask; } @@ - public async Task StartAsync(EndpointDefinition endpoint, CancellationToken token = default) - { - await Task.CompletedTask; - - if (this.IsActive) - { - return; - } + public Task StartAsync(EndpointDefinition endpoint, CancellationToken token = default) + { + if (this.IsActive) + { + return Task.CompletedTask; + } @@ - // Create and start new listener socket - this.CreateListener(token); + // Create and start new listener socket + this.CreateListener(token); @@ - } + } + + return Task.CompletedTask; } @@ - protected void CreateListener(CancellationToken token = default) + protected void CreateListener(CancellationToken token = default) { // If the listener isn't null, close before rebinding this.CleanupListener();And if you don’t plan to support cancellation here yet, a brief comment noting that the
tokenis reserved for future use would reduce surprises for consumers.Also applies to: 100-124, 160-184
src/Papercut.Infrastructure.Smtp/PapercutSmtpServer.cs (1)
52-76: CancellationToken parameters extend the API but aren’t yet used
StopAsyncandStartAsyncnow take aCancellationToken, but all control still flows through the internal_tokenSource; the external token is effectively ignored. That’s fine for preserving existing behavior and making the interface cancellation-aware, but callers can’t yet influence shutdown/startup via the token.If you want full cancellation support later, consider:
- Linking the external token into
_tokenSource(e.g., viaCancellationTokenSource.CreateLinkedTokenSource) so cooperative cancellation can propagate from upstream.- Threading the token through to any future async work added inside these methods.
For now, the change looks safe and backward compatible.
Also applies to: 78-124
src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceTrayServer.cs (1)
25-77: Tray IPComm server lifecycle works but cancellation/disposal could be tightenedThe overall pattern (start in
IStartable.Start, wrapPapercutIPCommServerstart/stop in a background task, and register asSingleInstance) makes sense, but a few details are worth tightening:
- The
_cancellationTokenSourceis only passed toTask.Run; it won’t cancelipCommServer.StopAsync()/StartAsync(...)once they’ve started. If you want cooperative shutdown, consider passing the token through to those calls as well:await ipCommServer.StopAsync(_cancellationTokenSource.Token); await ipCommServer.StartAsync(papercutIpCommEndpoints.TrayService, _cancellationTokenSource.Token);Disposecancels the CTS and disposesipCommServer. If Autofac also ownsPapercutIPCommServer’s lifetime (which is typical), this double-dispose is redundant and can be surprising. Letting the container disposeipCommServerand just cancelling/awaiting_serverTaskhere would be cleaner.- The warning message
"After 5 Retries. Failing"no longer matches the implementation (there’s a single attempt and no retry loop). Adjusting the message or reintroducing a retry policy would avoid confusing logs._serverTaskcan beTask?or initialized toTask.CompletedTaskto reflect that it’s optional and avoid nullability warnings; calling.Dispose()on it is generally unnecessary unless you’re explicitly managing wait handles.None of these are blockers, but tightening them up will make the tray server lifecycle more predictable and align better with the new cancellation-aware APIs.
src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (2)
28-55: Verify that persisted SMTP settings are actually being mergedThe new ctor adds a
SmtpServerOptionsMergerdependency and storesSmtpServerOptionsin_smtpServerOptions, but_settingsMergerisn’t used anywhere in this class as shown.If this manager is intended to be the place where persisted settings are merged into the runtime options, you probably want something like:
public SmtpServerManager( PapercutSmtpServer smtpServer, SmtpServerOptionsMerger settingsMerger, SmtpServerOptions smtpServerOptions, IPAllowedList ipAllowedList, ISettingStore settingStore, ILogger logger) { _smtpServer = smtpServer; _settingsMerger = settingsMerger; _smtpServerOptions = _settingsMerger.GetSettings(smtpServerOptions); _ipAllowedList = ipAllowedList; _settingStore = settingStore; _logger = logger; }If the merge happens elsewhere and
_settingsMergeris intentionally unused here, consider removing it from the constructor to keep the dependency surface minimal.
110-138: TLS endpoint construction is robust; consider aligning logs and cancellation semanticsThe new
BindSMTPServer+GetSmtpEndpointflow is a nice improvement: you stop the server, build anEndpointDefinitionwith optional TLS, and fall back to plain SMTP while logging detailed warnings when certificate configuration is invalid (bad enum values, missing or unusable cert).A few refinements to consider:
- Accurate TLS status logging:
tlsStatusis based solely on_smtpServerOptions.CertificateFindValue, butGetSmtpEndpoint()may fall back to a non-TLSEndpointDefinitionif parsing or certificate load fails. In those cases, the log message will still sayTLS=Enabled. You could derive the status fromendpoint.Certificate != nullinstead:var endpoint = GetSmtpEndpoint(); var tlsStatus = endpoint.Certificate != null ? "Enabled" : "Disabled";- Cancellation awareness: Both
HandleAsync(PapercutServiceReadyEvent ...)andHandleAsync(SmtpServerBindEvent ...)receive aCancellationTokenbut callBindSMTPServer()without passing it through. If you want to take advantage of the newPapercutSmtpServer.StartAsync/StopAsynctoken parameters, exposingBindSMTPServer(CancellationToken token)and forwarding the token would make the binding flow cancellable.- Retry log text: Similar to the tray server, the warning log message
"After 5 Retries. Failing"doesn’t match the current implementation (there’s only a single attempt inBindSMTPServer). Either implement a retry loop aroundStartAsyncor update the message to avoid confusion.Functionally this looks solid; these changes would mainly improve observability and consistency with the new cancellation-aware APIs.
Also applies to: 140-213
src/Papercut.Service/Infrastructure/Servers/PapercutServerHostedService.cs (1)
34-57: Consider simplifying StopAsync to avoid redundantWaitAsync
StopAsyncalready passescancellationTokeninto bothsmtpServer.StopAsyncandipCommServer.StopAsync, so those tasks can observe cancellation themselves. WrappingTask.WhenAll(...)in.WaitAsync(cancellationToken)adds an extra cancellation path but doesn’t materially change behavior and slightly obscures intent.You could simplify to:
await Task.WhenAll( smtpServer.StopAsync(cancellationToken), ipCommServer.StopAsync(cancellationToken));which still honors cancellation via the tasks’ own tokens.
Also applies to: 59-64
src/Papercut.Service/Infrastructure/IPComm/PublishAppEventBase.cs (1)
21-52: Fix logging template so the published event is actually capturedThe message template here doesn’t contain a valid placeholder for the event argument, so the structured log won’t include the event payload/type and the extra parameter may be ignored by the logger.
logger.Information( "Publishing {{@{@event.GetType().Name}}} to the Papercut {ConnectTo}", @event, ConnectTo);The escaped braces and inline
@event.GetType().Nameexpression won’t be interpreted as a property. A simpler, conventional template would be:- try - { - logger.Information( - "Publishing {{@{@event.GetType().Name}}} to the Papercut {ConnectTo}", - @event, ConnectTo); + try + { + logger.Information( + "Publishing {@Event} to the Papercut {ConnectTo}", + @event, + ConnectTo);If you also want the event type explicitly in the log, you can add a second placeholder:
logger.Information( "Publishing {@Event} ({EventType}) to the Papercut {ConnectTo}", @event, @event?.GetType().Name, ConnectTo);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
src/Papercut.Core/Domain/Network/IServer.cs(1 hunks)src/Papercut.Core/Domain/Settings/ReadWriteValueExtensions.cs(1 hunks)src/Papercut.Infrastructure.IPComm/ConnectionManager.cs(1 hunks)src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommClient.cs(3 hunks)src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommServer.cs(4 hunks)src/Papercut.Infrastructure.Smtp/PapercutSmtpServer.cs(2 hunks)src/Papercut.Message/MessageRepository.cs(2 hunks)src/Papercut.Message/PapercutMessageModule.cs(1 hunks)src/Papercut.Service.TrayNotification/AppLayer/NewMessageNotificationService.cs(1 hunks)src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceTrayServer.cs(1 hunks)src/Papercut.Service/Infrastructure/Configuration/SmtpServerOptionsInitializer.cs(0 hunks)src/Papercut.Service/Infrastructure/Configuration/SmtpServerOptionsMerger.cs(1 hunks)src/Papercut.Service/Infrastructure/IPComm/PublishAppEventBase.cs(1 hunks)src/Papercut.Service/Infrastructure/IPComm/PublishAppEventsHandlerToClientService.cs(1 hunks)src/Papercut.Service/Infrastructure/IPComm/PublishAppEventsHandlerToTrayService.cs(1 hunks)src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs(1 hunks)src/Papercut.Service/Infrastructure/Servers/PapercutServerHostedService.cs(2 hunks)src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs(4 hunks)src/Papercut.Service/PapercutServiceStartup.cs(1 hunks)src/Papercut.UI/AppLayer/IpComm/BackendServiceCoordinator.cs(1 hunks)test/Papercut.Message.Tests/MessageRepositoryTests.cs(1 hunks)test/Papercut.Rules.Tests/RulesRunnerTests.cs(2 hunks)
💤 Files with no reviewable changes (1)
- src/Papercut.Service/Infrastructure/Configuration/SmtpServerOptionsInitializer.cs
✅ Files skipped from review due to trivial changes (1)
- src/Papercut.Infrastructure.IPComm/ConnectionManager.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs
🧰 Additional context used
🧬 Code graph analysis (16)
src/Papercut.Service/PapercutServiceStartup.cs (2)
src/Papercut.Service/Infrastructure/Configuration/SmtpServerOptionsMerger.cs (3)
SmtpServerOptionsMerger(26-126)SmtpServerOptionsMerger(37-43)SmtpServerOptions(49-109)src/Papercut.Service/Domain/SmtpServer/SmtpServerOptions.cs (1)
SmtpServerOptions(23-72)
src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommClient.cs (4)
src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommProtocol.cs (2)
Task(32-41)Task(43-94)src/Papercut.Infrastructure.IPComm/StreamExtensions.cs (8)
Task(27-38)Task(40-53)Task(55-66)Task(68-75)Task(77-84)Task(86-89)Task(91-102)Task(104-132)src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommSerializer.cs (3)
PapercutIPCommSerializer(23-58)ToJson(35-39)ToJson(47-51)src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommRequest.cs (1)
PapercutIPCommRequest(21-28)
src/Papercut.Service/Infrastructure/Configuration/SmtpServerOptionsMerger.cs (6)
src/Papercut.Core/Infrastructure/Logging/BootstrapLogger.cs (1)
ILogger(45-81)src/Papercut.Core/Domain/Settings/BaseSettingsStore.cs (1)
Get(43-46)src/Papercut.Service.TrayNotification/AppLayer/NewMessageNotificationService.cs (1)
UsedImplicitly(63-69)src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceTrayServer.cs (1)
UsedImplicitly(71-77)src/Papercut.UI/AppLayer/IpComm/BackendServiceCoordinator.cs (1)
UsedImplicitly(245-252)src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (1)
Register(217-221)
src/Papercut.Infrastructure.Smtp/PapercutSmtpServer.cs (2)
src/Papercut.Core/Domain/Network/IServer.cs (2)
Task(34-34)Task(36-36)src/Papercut.Core/Domain/Network/EndpointDefinition.cs (3)
EndpointDefinition(24-146)EndpointDefinition(26-30)EndpointDefinition(32-46)
src/Papercut.Service.TrayNotification/AppLayer/NewMessageNotificationService.cs (5)
src/Papercut.Core/Infrastructure/Logging/BootstrapLogger.cs (1)
ILogger(45-81)src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceTrayServer.cs (1)
UsedImplicitly(71-77)src/Papercut.Service/Infrastructure/IPComm/PublishAppEventsHandlerToTrayService.cs (1)
UsedImplicitly(41-46)src/Papercut.UI/AppLayer/IpComm/BackendServiceCoordinator.cs (1)
UsedImplicitly(245-252)src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (1)
Register(217-221)
src/Papercut.Service/Infrastructure/IPComm/PublishAppEventsHandlerToTrayService.cs (6)
src/Papercut.Infrastructure.IPComm/PapercutIPCommClientFactory.cs (1)
PapercutIPCommClientFactory(30-46)src/Papercut.Service/Infrastructure/IPComm/PublishAppEventBase.cs (2)
PublishAppEventBase(21-53)Task(29-52)src/Papercut.Core/Infrastructure/Lifecycle/PapercutServiceReadyEvent.cs (1)
PapercutServiceReadyEvent(24-27)src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommClient.cs (4)
Task(31-60)Task(62-88)Task(90-109)Task(111-136)src/Papercut.Service.TrayNotification/AppLayer/NewMessageNotificationService.cs (2)
Task(40-59)UsedImplicitly(63-69)src/Papercut.Service/Infrastructure/IPComm/PublishAppEventsHandlerToClientService.cs (3)
Task(29-32)Task(34-37)Register(46-50)
src/Papercut.Core/Domain/Network/IServer.cs (2)
src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (1)
EndpointDefinition(140-213)src/Papercut.Core/Domain/Network/EndpointDefinition.cs (3)
EndpointDefinition(24-146)EndpointDefinition(26-30)EndpointDefinition(32-46)
src/Papercut.Service/Infrastructure/Servers/PapercutServerHostedService.cs (5)
src/Papercut.Core/Domain/Network/IServer.cs (2)
Task(34-34)Task(36-36)src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommClient.cs (4)
Task(31-60)Task(62-88)Task(90-109)Task(111-136)src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommServer.cs (2)
Task(75-98)Task(100-124)src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (3)
Task(57-61)Task(63-108)Task(110-138)src/Papercut.Service/Program.cs (2)
Task(38-48)Task(50-65)
src/Papercut.Message/PapercutMessageModule.cs (2)
src/Papercut.Service/Infrastructure/Paths/ServerPathTemplateProviderService.cs (1)
Register(44-51)src/Papercut.Message/MessageRepository.cs (1)
MessageRepository(26-200)
src/Papercut.Message/MessageRepository.cs (2)
src/Papercut.Message/IMessageRepository.cs (1)
DeleteMessage(25-25)src/Papercut.Core/Domain/Message/MessageEntry.cs (3)
MessageEntry(29-154)MessageEntry(46-74)MessageEntry(76-79)
src/Papercut.Core/Domain/Settings/ReadWriteValueExtensions.cs (4)
src/Papercut.Core/Domain/Settings/BaseSettingsStore.cs (2)
Set(48-51)Get(43-46)src/Papercut.Core/Domain/Settings/IWriteValue.cs (1)
Set(23-23)src/Papercut.Core/Domain/Settings/IReadValue.cs (1)
T(23-23)src/Papercut.Common/Helper/StringHelpers.cs (1)
IsNullOrWhiteSpace(63-66)
src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommServer.cs (3)
src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommClient.cs (4)
Task(31-60)Task(62-88)Task(90-109)Task(111-136)src/Papercut.Infrastructure.Smtp/PapercutSmtpServer.cs (2)
Task(52-76)Task(78-124)src/Papercut.Infrastructure.IPComm/StreamExtensions.cs (8)
Task(27-38)Task(40-53)Task(55-66)Task(68-75)Task(77-84)Task(86-89)Task(91-102)Task(104-132)
src/Papercut.Service/Infrastructure/IPComm/PublishAppEventBase.cs (3)
src/Papercut.Infrastructure.IPComm/PapercutIPCommClientFactory.cs (1)
PapercutIPCommClientFactory(30-46)src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommClient.cs (4)
Task(31-60)Task(62-88)Task(90-109)Task(111-136)src/Papercut.Service/Infrastructure/IPComm/PublishAppEventsHandlerToClientService.cs (3)
Task(29-32)Task(34-37)Task(39-42)
src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceTrayServer.cs (7)
src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommServer.cs (4)
PapercutIPCommServer(29-223)PapercutIPCommServer(39-47)Task(75-98)Task(100-124)src/Papercut.Service.TrayNotification/AppLayer/NewMessageNotificationService.cs (2)
Task(40-59)UsedImplicitly(63-69)src/Papercut.Service/Infrastructure/Configuration/SmtpServerOptionsMerger.cs (1)
UsedImplicitly(117-123)src/Papercut.Service/Infrastructure/IPComm/PublishAppEventsHandlerToTrayService.cs (1)
UsedImplicitly(41-46)src/Papercut.Service/PapercutServiceStartup.cs (1)
UsedImplicitly(70-77)src/Papercut.UI/AppLayer/IpComm/BackendServiceCoordinator.cs (1)
UsedImplicitly(245-252)src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (1)
Register(217-221)
src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (7)
src/Papercut.Core/Infrastructure/Lifecycle/PapercutServiceReadyEvent.cs (1)
PapercutServiceReadyEvent(24-27)src/Papercut.Infrastructure.Smtp/PapercutSmtpServer.cs (1)
PapercutSmtpServer(34-162)src/Papercut.Service/Infrastructure/Configuration/SmtpServerOptionsMerger.cs (3)
SmtpServerOptionsMerger(26-126)SmtpServerOptionsMerger(37-43)SmtpServerOptions(49-109)src/Papercut.Service/Domain/SmtpServer/SmtpServerOptions.cs (1)
SmtpServerOptions(23-72)src/Papercut.Common/Helper/StringHelpers.cs (2)
IsSet(42-45)IsNullOrWhiteSpace(63-66)src/Papercut.Core/Domain/Settings/ReadWriteValueExtensions.cs (1)
Set(26-34)src/Papercut.Core/Domain/Network/EndpointDefinition.cs (4)
ToString(141-145)EndpointDefinition(24-146)EndpointDefinition(26-30)EndpointDefinition(32-46)
src/Papercut.Service/Infrastructure/IPComm/PublishAppEventsHandlerToClientService.cs (4)
src/Papercut.Infrastructure.IPComm/PapercutIPCommClientFactory.cs (1)
PapercutIPCommClientFactory(30-46)src/Papercut.Service/Infrastructure/IPComm/PublishAppEventBase.cs (1)
PublishAppEventBase(21-53)src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommClient.cs (4)
Task(31-60)Task(62-88)Task(90-109)Task(111-136)src/Papercut.Service/Infrastructure/Servers/PapercutServerHostedService.cs (2)
Task(34-57)Task(59-64)
⏰ 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: Build Papercut SMTP
- GitHub Check: Analyze (csharp)
- GitHub Check: Build Papercut SMTP
🔇 Additional comments (16)
src/Papercut.Core/Domain/Settings/ReadWriteValueExtensions.cs (1)
82-91: LGTM!The signature change to
IReadValue<string?>is consistent with nullable reference types adoption and aligns well with the broader settings infrastructure refactoring.src/Papercut.Service.TrayNotification/AppLayer/NewMessageNotificationService.cs (3)
25-28: LGTM! Clean class structure with primary constructor.The class correctly implements
IEventHandler<NewMessageEvent>and uses the modern primary constructor pattern for dependency injection.
40-59: LGTM! Solid error handling and early-return pattern.The cancellation token parameter is unused, which is acceptable given the synchronous nature of the notification logic. The try-catch around event invocation is good defensive programming, ensuring one failing subscriber doesn't crash the handler.
63-69: LGTM! Registration pattern is consistent with the codebase.The registration correctly follows the established pattern with null checking and appropriate lifetime scope (SingleInstance for a notification service).
test/Papercut.Rules.Tests/RulesRunnerTests.cs (2)
238-248: LGTM! Modern async cancellation pattern.The change from
Cancel()toawait CancelAsync()aligns with modern async patterns and ensures cancellation is fully complete before the test proceeds to invokeRunNewMessageRules. The test logic and expectations remain sound.
250-266: LGTM! Async cancellation during execution is well-handled.The change to
await CancelAsync()on line 261 modernizes the cancellation pattern within the dispatch action. While this introduces a minimal timing difference compared to synchronousCancel(), the test logic is preserved: when rules execute in parallel and cancellation is triggered mid-execution, the runner should still throwOperationCanceledException. The async approach is consistent with the broader adoption of async cancellation patterns across the codebase.src/Papercut.Message/MessageRepository.cs (3)
30-30: LGTM: Field visibility and naming improved.The change from
_invalidFileNameChars(implying private) toInvalidFileNameChars(protected) aligns with C# naming conventions for non-private fields.
34-35: LGTM: Defensive null check added.The explicit null check enhances runtime safety and aligns with defensive programming best practices.
147-147: LGTM: Field reference updated consistently.The field reference correctly reflects the rename on Line 30.
test/Papercut.Message.Tests/MessageRepositoryTests.cs (1)
45-45: LGTM: Modern collection expression syntax.The collection expression syntax is appropriate for .NET 9.0 and improves code conciseness.
src/Papercut.Message/PapercutMessageModule.cs (1)
30-31: Code changes verified and approved.All verification checks pass:
PathTemplateType.Messageenum value exists inIPathTemplatesProvider.csMessagePathConfiguratoris correctly registered asKeyed<IPathConfigurator>(PathTemplateType.Message)inPapercutCoreModule.cs(lines 52-56)PapercutMessageModulecorrectly resolves the keyed dependency (line 31)- Module loading sequence ensures
PapercutCoreModuleregisters keyed dependencies beforePapercutMessageModuleis loadedThe keyed resolution pattern is properly implemented with correct registration order.
src/Papercut.Service/Infrastructure/Configuration/SmtpServerOptionsMerger.cs (1)
118-123: LGTM: DI registration follows established patterns.The singleton registration is appropriate for this stateless merger, and the null check and pattern match other registrations in the codebase.
src/Papercut.UI/AppLayer/IpComm/BackendServiceCoordinator.cs (1)
135-155: Good token propagation into IPComm publishForwarding the
CancellationTokenfromPublishSmtpUpdatedAsyncintomessenger.PublishEventServeris exactly what we want with the new cancellation-aware client APIs. It keeps the behavior unchanged while allowing upstream callers to cancel the outbound SMTP binding update.No additional changes needed here.
src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommClient.cs (1)
31-60: Cancellation token propagation in TryConnect/Exchange/Publish looks consistentLinking the per-call timeout CTS with the external token and threading the resulting token through
ConnectAsync,HandlePublishEvent, and the follow‑up reads/writes gives you a single, bounded operation window and cooperative cancellation from callers. The genericFunc<TcpClient, CancellationToken, Task<T>>delegate keeps the connect logic reusable without leaking token plumbing into call sites.Only thing to be aware of: the
connectTimeoutnow effectively bounds the entire publish/exchange (handshake + request + payload + optional response), not just the connect. That matches howPublishTimeoutis used today, but if you later need longer-running exchanges you may want a separate per-operation timeout.Also applies to: 62-88, 90-109, 111-135
src/Papercut.Service/Infrastructure/IPComm/PublishAppEventsHandlerToTrayService.cs (1)
23-48: Tray event handler wiring looks good and consistent with the client handlerDeriving from
PublishAppEventBasewithConnectTo => ServiceTraycleanly routesNewMessageEventandPapercutServiceReadyEventto tray instances, and the static Autofac registration mirrors the existing client service handler pattern. No functional issues spotted here.src/Papercut.Service/Infrastructure/IPComm/PublishAppEventsHandlerToClientService.cs (1)
21-52: Refactor ontoPublishAppEventBasecentralizes UI publish logic nicelyRouting this handler through
PublishAppEventBasewithConnectTo => UIremoves duplicated publish logic, ensures all three lifecycle events share the same timeout/error-handling behavior, and correctly propagates theCancellationTokenviaPublishAsync(@event, token). The DI registration remains equivalent, so consumers shouldn’t see behavioral changes beyond the unified logging/timeout semantics.
| Task StartAsync(EndpointDefinition endpoint, CancellationToken token = default); | ||
|
|
||
| Task StopAsync(); | ||
| Task StopAsync(CancellationToken token = default); |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! CancellationToken parameters enable graceful cancellation.
Adding CancellationToken parameters to async methods is a best practice that enables cooperative cancellation during server startup and shutdown operations. The default parameter values maintain caller compatibility.
Please verify that:
- All implementations of
IServerhave been updated to match these signatures - The
CancellationTokenis actively used in implementations (not just accepted but ignored)
Run the following script to identify all implementations:
🏁 Script executed:
#!/bin/bash
# Find all classes implementing IServer interface
echo "=== Finding IServer implementations ==="
ast-grep --pattern $'class $CLASS : $$$IServer$$$ {
$$$
}'
echo ""
echo "=== Checking StartAsync implementations ==="
ast-grep --pattern $'StartAsync($$$) {
$$$
}'
echo ""
echo "=== Checking StopAsync implementations ==="
ast-grep --pattern $'StopAsync($$$) {
$$$
}'Length of output: 128
🏁 Script executed:
#!/bin/bash
# Search for IServer implementations in C#
echo "=== Finding IServer implementations ==="
rg -nC3 ':\s*IServer' --type=cs
echo ""
echo "=== Finding StartAsync implementations with signatures ==="
rg -nC5 'StartAsync\s*\(' --type=cs
echo ""
echo "=== Finding StopAsync implementations with signatures ==="
rg -nC5 'StopAsync\s*\(' --type=cs
echo ""
echo "=== Checking if CancellationToken is used in method bodies ==="
rg -nC3 'CancellationToken.*token' --type=csLength of output: 87238
🏁 Script executed:
#!/bin/bash
# Get full HttpServer class definition to verify all IServer methods are implemented
echo "=== HttpServer class methods ==="
rg -n 'class HttpServer' -A 60 --type=cs src/Papercut.Service/Web/Hosting/InProcess/HttpServer.cs | head -100
echo ""
echo "=== Search for IServer.StartAsync in HttpServer file ==="
rg -n 'StartAsync' --type=cs src/Papercut.Service/Web/Hosting/InProcess/HttpServer.cs -B2 -A5Length of output: 2368
HttpServer implementation uses wrong StartAsync signature and fails to implement the updated IServer interface.
The interface was updated to use EndpointDefinition as a parameter, but HttpServer still implements the old signature with IHttpApplication<TContext>:
- Interface expects:
Task StartAsync(EndpointDefinition endpoint, CancellationToken token = default); - HttpServer has:
Task IServer.StartAsync<TContext>(IHttpApplication<TContext> application, CancellationToken cancellationToken)
This causes HttpServer to fail the interface contract. Update src/Papercut.Service/Web/Hosting/InProcess/HttpServer.cs line 60 to match the new IServer interface signature (with EndpointDefinition parameter instead of IHttpApplication<TContext>).
🤖 Prompt for AI Agents
In src/Papercut.Service/Web/Hosting/InProcess/HttpServer.cs around line 60, the
HttpServer implementation still declares Task
IServer.StartAsync<TContext>(IHttpApplication<TContext> application,
CancellationToken cancellationToken) instead of implementing the updated IServer
signature Task StartAsync(EndpointDefinition endpoint, CancellationToken token =
default); change the method to remove the generic and IHttpApplication
parameter, accept an EndpointDefinition endpoint and a CancellationToken
(matching default/name from the interface), update the method body to use the
endpoint parameter instead of the old application parameter (and remove any
generic constraints/usages), and ensure method name, accessibility and signature
exactly match the interface so the class correctly implements IServer (also
update any internal callers/tests that passed IHttpApplication to now pass
EndpointDefinition).
| public SmtpServerOptions GetSettings(SmtpServerOptions smtpServerOptions) | ||
| { | ||
| try | ||
| { | ||
| var persistedIp = SettingStore.Get("IP", smtpServerOptions.IP); | ||
| var persistedPort = SettingStore.Get("Port", smtpServerOptions.Port); | ||
| var persistedMessagePath = SettingStore.Get("MessagePath", smtpServerOptions.MessagePath); | ||
| var persistedLoggingPath = SettingStore.Get("LoggingPath", smtpServerOptions.LoggingPath); | ||
|
|
||
| // TLS/Certificate settings | ||
| var persistedCertFindType = SettingStore.Get("CertificateFindType", smtpServerOptions.CertificateFindType); | ||
| var persistedCertFindValue = SettingStore.Get("CertificateFindValue", smtpServerOptions.CertificateFindValue); | ||
| var persistedCertStoreLocation = SettingStore.Get("CertificateStoreLocation", smtpServerOptions.CertificateStoreLocation); | ||
| var persistedCertStoreName = SettingStore.Get("CertificateStoreName", smtpServerOptions.CertificateStoreName); | ||
|
|
||
| // Apply persisted settings | ||
| if (!string.IsNullOrWhiteSpace(persistedIp)) | ||
| { | ||
| smtpServerOptions.IP = persistedIp; | ||
| } | ||
|
|
||
| if (persistedPort > 0) | ||
| { | ||
| smtpServerOptions.Port = persistedPort; | ||
| } | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(persistedMessagePath)) | ||
| { | ||
| smtpServerOptions.MessagePath = persistedMessagePath; | ||
| } | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(persistedLoggingPath)) | ||
| { | ||
| smtpServerOptions.LoggingPath = persistedLoggingPath; | ||
| } | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(persistedCertFindType)) | ||
| { | ||
| smtpServerOptions.CertificateFindType = persistedCertFindType; | ||
| } | ||
|
|
||
| // CertificateFindValue can be empty (TLS disabled), so always apply it | ||
| smtpServerOptions.CertificateFindValue = persistedCertFindValue ?? string.Empty; | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(persistedCertStoreLocation)) | ||
| { | ||
| smtpServerOptions.CertificateStoreLocation = persistedCertStoreLocation; | ||
| } | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(persistedCertStoreName)) | ||
| { | ||
| smtpServerOptions.CertificateStoreName = persistedCertStoreName; | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Logger.Warning(ex, "Failed to initialize SMTP server options from persisted settings"); | ||
| } | ||
|
|
||
| return smtpServerOptions; | ||
| } |
There was a problem hiding this comment.
Mutating input parameter instead of cloning.
GetSettings modifies the smtpServerOptions parameter in place (lines 67, 72, 77, 82, 87, 91, 95, 100) and returns the same instance. This mutation pattern is problematic when the same instance is passed multiple times (as happens in the transient registration in PapercutServiceStartup.cs line 52), leading to repeated modifications and potential non-idempotent behavior.
Refactor to clone the input and return a new instance:
public SmtpServerOptions GetSettings(SmtpServerOptions smtpServerOptions)
{
+ // Clone to avoid mutating the input
+ var result = new SmtpServerOptions
+ {
+ IP = smtpServerOptions.IP,
+ Port = smtpServerOptions.Port,
+ MessagePath = smtpServerOptions.MessagePath,
+ LoggingPath = smtpServerOptions.LoggingPath,
+ CertificateFindType = smtpServerOptions.CertificateFindType,
+ CertificateFindValue = smtpServerOptions.CertificateFindValue,
+ CertificateStoreLocation = smtpServerOptions.CertificateStoreLocation,
+ CertificateStoreName = smtpServerOptions.CertificateStoreName,
+ AllowedIps = smtpServerOptions.AllowedIps
+ };
+
try
{
- var persistedIp = SettingStore.Get("IP", smtpServerOptions.IP);
+ var persistedIp = SettingStore.Get("IP", result.IP);
// ... (update all references to use result instead of smtpServerOptions)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/Papercut.Service.TrayNotification/Program.cs (2)
56-57: Consider storing the coordinator reference or using ApplicationContext.The
ServiceTrayCoordinatoris resolved but not stored, relying on side effects (constructor initialization) and implicit lifetime management through event handlers. While this likely works, it's not immediately clear why the reference is discarded.For clarity and explicit lifetime management, consider:
- var coordinator = Container.Resolve<ServiceTrayCoordinator>(); - Application.Run(); + using var coordinator = Container.Resolve<ServiceTrayCoordinator>(); + Application.Run();Or use
ApplicationContext:public class TrayApplicationContext : ApplicationContext { private readonly ServiceTrayCoordinator _coordinator; public TrayApplicationContext(ServiceTrayCoordinator coordinator) { _coordinator = coordinator; } } // In Main: using var context = new TrayApplicationContext(Container.Resolve<ServiceTrayCoordinator>()); Application.Run(context);
77-80: Add [UsedImplicitly] attribute for consistency.The
Registermethod is invoked via reflection throughbuilder.RegisterStaticMethods(ThisAssembly)(see PapercutServiceTrayModule.cs line 41). Similar methods in the codebase, such asServiceTrayCoordinator.Register, are annotated with[UsedImplicitly]to suppress IDE warnings and document the reflection-based invocation.Apply this diff:
+ [UsedImplicitly] private static void Register(ContainerBuilder builder) { builder.RegisterInstance(AppMeta).As<IAppMeta>().SingleInstance(); }src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs (2)
48-48: UseDateTime.UtcNowfor time comparisons.The code currently uses
DateTime.Nowfor cache expiration checks.DateTime.UtcNowis preferred for time-based comparisons and duration calculations because it avoids time zone and daylight saving time issues.Apply this diff:
public string? CachedWebUrl => - _cachedWebUrl != null && DateTime.Now - _lastUrlCheck < _urlCacheExpiration + _cachedWebUrl != null && DateTime.UtcNow - _lastUrlCheck < _urlCacheExpiration ? _cachedWebUrl : null; public async Task<string> GetWebUIUrlAsync() { // Return cached URL if still valid - if (_cachedWebUrl != null && DateTime.Now - _lastUrlCheck < _urlCacheExpiration) + if (_cachedWebUrl != null && DateTime.UtcNow - _lastUrlCheck < _urlCacheExpiration) { return _cachedWebUrl; } try { var serviceClient = ipCommClientFactory.GetClient(PapercutIPCommClientConnectTo.Service); var exchangeEvent = new ServiceWebUISettingsExchangeEvent(); var serviceWebUiSettings = await serviceClient.ExchangeEventServer(exchangeEvent, TimeSpan.FromSeconds(2)); if (serviceWebUiSettings != null && serviceWebUiSettings.IP.IsSet() && serviceWebUiSettings.Port.HasValue) { _cachedWebUrl = $"http://{serviceWebUiSettings.IP}:{serviceWebUiSettings.Port}"; - _lastUrlCheck = DateTime.Now; + _lastUrlCheck = DateTime.UtcNow; return _cachedWebUrl; } } catch (Exception ex) { Log.Warning(ex, "Failed to probe for Papercut Service Listening Web Url"); } // Fallback to default Log.Debug("Using fallback web URL: {Url}", FallbackWebUrl); _cachedWebUrl = FallbackWebUrl; - _lastUrlCheck = DateTime.Now; + _lastUrlCheck = DateTime.UtcNow; return FallbackWebUrl; }Also applies to: 55-55, 71-71, 84-84
52-87: Consider addingCancellationTokensupport.
GetWebUIUrlAsync()doesn't accept aCancellationTokenparameter, limiting cooperative cancellation. While the IPComm call has a 2-second timeout, adding token support would follow async best practices and allow the operation to be cancelled during application shutdown or on-demand.Apply this diff:
- public async Task<string> GetWebUIUrlAsync() + public async Task<string> GetWebUIUrlAsync(CancellationToken cancellationToken = default) { // Return cached URL if still valid if (_cachedWebUrl != null && DateTime.UtcNow - _lastUrlCheck < _urlCacheExpiration) { return _cachedWebUrl; } try { + cancellationToken.ThrowIfCancellationRequested(); + var serviceClient = ipCommClientFactory.GetClient(PapercutIPCommClientConnectTo.Service); var exchangeEvent = new ServiceWebUISettingsExchangeEvent(); var serviceWebUiSettings = await serviceClient.ExchangeEventServer(exchangeEvent, TimeSpan.FromSeconds(2));Then update the call site in
HandleAsync:public async Task HandleAsync(PapercutServiceReadyEvent @event, CancellationToken token = default) { this.InvalidateCache(); // warm up the cache - var _ = await this.GetWebUIUrlAsync(); + var _ = await this.GetWebUIUrlAsync(token); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Papercut.Service.TrayNotification/AppLayer/ServiceTrayCoordinator.cs(1 hunks)src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceTrayServer.cs(1 hunks)src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs(1 hunks)src/Papercut.Service.TrayNotification/Program.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceTrayServer.cs
- src/Papercut.Service.TrayNotification/AppLayer/ServiceTrayCoordinator.cs
🧰 Additional context used
🧬 Code graph analysis (2)
src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs (6)
src/Papercut.Infrastructure.IPComm/PapercutIPCommClientFactory.cs (1)
PapercutIPCommClientFactory(30-46)src/Papercut.Core/Infrastructure/Lifecycle/PapercutServiceReadyEvent.cs (1)
PapercutServiceReadyEvent(24-27)src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs (1)
Task(31-68)src/Papercut.Service.TrayNotification/Infrastructure/ServiceStatusService.cs (1)
Task(119-122)src/Papercut.Common/Helper/StringHelpers.cs (1)
IsSet(42-45)src/Papercut.Core/Infrastructure/Async/AsyncHelpers.cs (1)
RunAsync(73-88)
src/Papercut.Service.TrayNotification/Program.cs (6)
src/Papercut.Core/Domain/Application/ApplicationMeta.cs (1)
ApplicationMeta(23-28)src/Papercut.Core/Infrastructure/Container/SimpleContainer.cs (2)
IContainer(27-35)SimpleContainer(24-36)src/Papercut.Core/Infrastructure/Logging/BootstrapLogger.cs (2)
BootstrapLogger(30-88)BootstrapLogger(32-36)src/Papercut.Core/Infrastructure/Consoles/ConsoleHelpers.cs (2)
ConsoleHelpers(21-40)HasConsole(23-39)src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceTrayModule.cs (1)
PapercutServiceTrayModule(26-42)src/Papercut.Service.TrayNotification/AppLayer/ServiceTrayCoordinator.cs (2)
ServiceTrayCoordinator(29-541)ServiceTrayCoordinator(45-87)
⏰ 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: Build Papercut SMTP
- GitHub Check: Analyze (csharp)
- GitHub Check: Build Papercut SMTP
🔇 Additional comments (2)
src/Papercut.Service.TrayNotification/Program.cs (2)
45-48: Good defensive console handling.The console availability check before setting the title prevents exceptions in non-console environments.
60-61: Verify that filtering ObjectDisposedException is intentional.The exception filter excludes both
TaskCanceledExceptionandObjectDisposedException. While filteringTaskCanceledExceptionis reasonable for graceful shutdown scenarios, suppressingObjectDisposedExceptionmay hide legitimate disposal-ordering bugs during application teardown.Confirm that
ObjectDisposedExceptionduring startup/shutdown is expected and should be silently ignored. If these exceptions indicate real problems, consider:- catch (Exception ex) when (ex is not TaskCanceledException and not ObjectDisposedException) + catch (Exception ex) when (ex is not TaskCanceledException)
| catch (Exception ex) | ||
| { | ||
| Log.Warning(ex, "Failed to probe for Papercut Service Listening Web Url"); | ||
| } | ||
|
|
||
| // Fallback to default | ||
| Log.Debug("Using fallback web URL: {Url}", FallbackWebUrl); | ||
| _cachedWebUrl = FallbackWebUrl; | ||
| _lastUrlCheck = DateTime.Now; | ||
|
|
||
| return FallbackWebUrl; |
There was a problem hiding this comment.
Use injected logger instead of static Log.
The exception handler and fallback path use the static Log class instead of the injected logger instance. This is inconsistent with line 91 and bypasses the dependency injection pattern, potentially losing logger context enrichment.
Apply this diff:
catch (Exception ex)
{
- Log.Warning(ex, "Failed to probe for Papercut Service Listening Web Url");
+ logger.Warning(ex, "Failed to probe for Papercut Service Listening Web Url");
}
// Fallback to default
- Log.Debug("Using fallback web URL: {Url}", FallbackWebUrl);
+ logger.Debug("Using fallback web URL: {Url}", FallbackWebUrl);
_cachedWebUrl = FallbackWebUrl;📝 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.
| catch (Exception ex) | |
| { | |
| Log.Warning(ex, "Failed to probe for Papercut Service Listening Web Url"); | |
| } | |
| // Fallback to default | |
| Log.Debug("Using fallback web URL: {Url}", FallbackWebUrl); | |
| _cachedWebUrl = FallbackWebUrl; | |
| _lastUrlCheck = DateTime.Now; | |
| return FallbackWebUrl; | |
| catch (Exception ex) | |
| { | |
| logger.Warning(ex, "Failed to probe for Papercut Service Listening Web Url"); | |
| } | |
| // Fallback to default | |
| logger.Debug("Using fallback web URL: {Url}", FallbackWebUrl); | |
| _cachedWebUrl = FallbackWebUrl; | |
| _lastUrlCheck = DateTime.Now; | |
| return FallbackWebUrl; |
🤖 Prompt for AI Agents
In src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs
around lines 76 to 86, replace usages of the static Log class with the injected
logger instance: use logger.Warning(...) in the catch block (preserving
exception and message) and logger.Debug(...) for the fallback web URL line so
logging goes through the injected DI logger and retains context/enrichments;
keep the rest of the fallback logic (_cachedWebUrl, _lastUrlCheck, return
FallbackWebUrl) unchanged.
src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs
Show resolved
Hide resolved
- Subscribe to NotifyIcon.BalloonTipClicked event - Add OnBalloonTipClicked handler that calls OnOpenWebUI - When user clicks new message notification, Web UI opens automatically
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Papercut.Service/PapercutServiceModule.cs (1)
26-44: Address security posture of IP allowlist fallback and fix variable namingThe fallback to
IPAllowedList.AllowAllwhen configuration is invalid is a confirmed security risk. If an operator misconfigures the allowlist (e.g., typos an IP address like "192.168.1.256"), the service silently opens to all IPs instead of failing. The codebase provides a saferIPAllowedList.LocalhostOnlyalternative—consider switching the fallback to deny-by-default or making initialization fail-fast, depending on your backwards-compatibility requirements.Also fix the variable naming mismatch:
smtpServerOptionsshould besmtpServerSettingsto align with the resolved type.src/Papercut.Service/Infrastructure/Rules/RuleService.cs (1)
111-130: Wire the service CancellationTokenSource into periodic rules and cancel/cleanup before base.DisposeAsyncPeriodic background rules are called with CancellationToken.None (src/Papercut.Service/Infrastructure/Rules/RuleService.cs — SetupPeriodicRuleObservable) while the service's _cancellationTokenSource is only cancelled/disposed in DisposeAsync; pass _cancellationTokenSource.Token into RunPeriodicBackgroundRules and perform cancellation/cleanup before awaiting base.DisposeAsync to allow graceful shutdown.
Locations: src/Papercut.Service/Infrastructure/Rules/RuleService.cs — DisposeAsync (lines ~111–129) and SetupPeriodicRuleObservable (lines ~132–140).
Suggested change (DisposeAsync) — keep this diff:
- protected override async ValueTask DisposeAsync(bool disposing) - { - await base.DisposeAsync(disposing); - - if (!disposing) return; - - try - { - await _cancellationTokenSource.CancelAsync(); - _periodicRuleSubscription?.Dispose(); - } - catch (ObjectDisposedException) - { - // ignore - } - finally - { - _cancellationTokenSource.Dispose(); - } - } + protected override async ValueTask DisposeAsync(bool disposing) + { + if (disposing) + { + try + { + await _cancellationTokenSource.CancelAsync(); + _periodicRuleSubscription?.Dispose(); + } + catch (ObjectDisposedException) + { + // ignore + } + finally + { + _cancellationTokenSource.Dispose(); + } + } + + await base.DisposeAsync(disposing); + }Also change SetupPeriodicRuleObservable to call:
- rulesRunner.RunPeriodicBackgroundRules(..., _cancellationTokenSource.Token) instead of CancellationToken.None.
♻️ Duplicate comments (2)
src/Papercut.Service.TrayNotification/AppLayer/ServiceTrayCoordinator.cs (1)
100-126: Subject parsing for new-message notifications still collapses most of the subjectThe filename parsing logic doesn’t correctly follow the
{timestamp} {subject} {randomstring}.emlconvention:
Split(' ', 3)yields:[timestamp, firstWordOfSubject, "restOfSubject random"].string.Join(" ", parts.Skip(1).Take(parts.Length - 2))on length 3 takes only the second element → you lose everything after the first subject word.- For a 2-token filename (
timestamp subject),Take(parts.Length - 2)is empty and you fall back to “(No Subject)”.So multi-word subjects will be truncated to the first word, and simple subjects will often appear as “(No Subject)”. This was already raised on a previous commit and remains unresolved.
You can instead strip the leading timestamp and trailing random token by position:
- try - { - // Extract subject from filename format: {timestamp} {subject} {randomstring}.eml - var fileName = Path.GetFileNameWithoutExtension(e.NewMessage.Name); - var parts = fileName.Split(' ', 3); // Split into timestamp, subject, random - var subject = parts.Length >= 2 - ? string.Join(" ", parts.Skip(1).Take(parts.Length - 2)) - : "(No Subject)"; - - if (string.IsNullOrWhiteSpace(subject)) - subject = "(No Subject)"; + try + { + // Extract subject from filename format: {timestamp} {subject} {randomstring}.eml + var fileName = Path.GetFileNameWithoutExtension(e.NewMessage.Name); + + string subject; + var firstSpaceIndex = fileName.IndexOf(' '); + if (firstSpaceIndex > 0) + { + // Strip leading timestamp + var withoutTimestamp = fileName[(firstSpaceIndex + 1)..]; + + // Strip trailing random segment (last space-delimited token) + var lastSpaceIndex = withoutTimestamp.LastIndexOf(' '); + subject = lastSpaceIndex > 0 + ? withoutTimestamp[..lastSpaceIndex] + : withoutTimestamp; + } + else + { + subject = "(No Subject)"; + } + + if (string.IsNullOrWhiteSpace(subject)) + subject = "(No Subject)";This preserves the full subject portion between the timestamp and the trailing random string.
src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs (1)
78-78: StaticLogusage persists despite past review feedback.Lines 78 and 82 still use the static
Logclass instead of the injectedloggerinstance, which was flagged in a previous review. This bypasses DI, loses context enrichment, and is inconsistent with line 91.Apply this diff:
catch (Exception ex) { - Log.Warning(ex, "Failed to probe for Papercut Service Listening Web Url"); + logger.Warning(ex, "Failed to probe for Papercut Service Listening Web Url"); } // Fallback to default - Log.Debug("Using fallback web URL: {Url}", FallbackWebUrl); + logger.Debug("Using fallback web URL: {Url}", FallbackWebUrl); _cachedWebUrl = FallbackWebUrl;Also applies to: 82-82
🧹 Nitpick comments (16)
src/Papercut.Core/Domain/Network/EndpointDefinition.cs (1)
105-131: Consider simplifying by removing the unnecessary null check.The null check
if (certificate is { } cert2)at line 105 is defensive but unnecessary. Sincecertificatecomes fromcertificates[0]and the collection is verified non-empty at line 90,certificateis guaranteed to be non-null. Removing this conditional would make the code clearer and eliminate the implied possibility that the private key validation and warnings are optional.Apply this diff to simplify the validation logic:
var certificate = certificates[0]; // Validate certificate has private key (required for TLS server) - if (certificate is { } cert2) + if (!certificate.HasPrivateKey) { - if (!cert2.HasPrivateKey) - { - throw new InvalidOperationException( - $"Certificate '{cert2.Subject}' does not have a private key. TLS/STARTTLS requires a certificate with a private key."); - } - - // Log warnings for certificate validity issues - var now = DateTime.Now; - if (cert2.NotBefore > now) - { - Log.Warning( - "Certificate '{Subject}' is not yet valid (NotBefore: {NotBefore}, Current: {Now})", - cert2.Subject, - cert2.NotBefore, - now); - } - else if (cert2.NotAfter < now) - { - Log.Warning( - "Certificate '{Subject}' has expired (NotAfter: {NotAfter}, Current: {Now})", - cert2.Subject, - cert2.NotAfter, - now); - } + throw new InvalidOperationException( + $"Certificate '{certificate.Subject}' does not have a private key. TLS/STARTTLS requires a certificate with a private key."); + } + + // Log warnings for certificate validity issues + var now = DateTime.Now; + if (certificate.NotBefore > now) + { + Log.Warning( + "Certificate '{Subject}' is not yet valid (NotBefore: {NotBefore}, Current: {Now})", + certificate.Subject, + certificate.NotBefore, + now); + } + else if (certificate.NotAfter < now) + { + Log.Warning( + "Certificate '{Subject}' has expired (NotAfter: {NotAfter}, Current: {Now})", + certificate.Subject, + certificate.NotAfter, + now); } return certificate;src/Papercut.Core/Domain/Message/MessageEntryDto.cs (1)
21-34: MessageEntryDto shape and ToEntry() mapping look consistent with MessageEntry.ToDto()DTO fields mirror the computed properties on
MessageEntry, andToEntry()rehydrates fromFile, which is the real source of truth. That’s a reasonable balance for cross-process/event payloads; any additional fields can be recomputed on the consumer side.If you expect consumers to rely heavily on equality/immutability of this payload, you might consider making this a
record(or sealing the class) for clearer semantics, but it’s not required for correctness.src/Papercut.Message/ReceivedDataMessageHandler.cs (1)
26-30: Handler refactor and DTO-based event publishing look correct; a couple of small cleanups possibleThe move to constructor parameters plus publishing
NewMessageEvent(new MessageEntry(file).ToDto())keeps the handler cohesive and aligns it with the new DTO-based event contract; the logging around invalid addresses and fatal publish failures is also appropriate for diagnosing issues.Two small, non-blocking cleanups you might consider:
- Drop the redundant async/await in the SaveMessage callback – it can be simplified without changing behavior:
- file = await messageRepository.SaveMessage(message.Subject ?? string.Empty, async fs => await message.WriteToAsync(fs)); + file = await messageRepository.SaveMessage(message.Subject ?? string.Empty, fs => message.WriteToAsync(fs));
- (Optional) If this path is ever hot, you could avoid constructing
MessageEntrypurely to callToDto()by introducing a helper that buildsMessageEntryDtodirectly from the file path, but the current approach is perfectly acceptable for correctness and readability.Please ensure the signature of
SaveMessageindeed expects aFunc<Stream, Task>so the simplified lambda above remains type-compatible across all call sites.Also applies to: 71-76, 82-87
src/Papercut.Service.TrayNotification/AppLayer/ServiceTrayCoordinator.cs (3)
128-131:OnServiceStatusChangedshould not beasync voidwhen no asynchronous work is done
OnServiceStatusChangedis declaredasync voidbut only callsUpdateTrayIcon()synchronously. This adds unnecessary overhead and can confuse readers expecting asynchronous behavior.You can simplify and avoid
async void(which is generally reserved for true async event handlers) by removingasyncentirely:- private async void OnServiceStatusChanged(object? sender, ServiceControllerStatus status) - { - UpdateTrayIcon(); - } + private void OnServiceStatusChanged(object? sender, ServiceControllerStatus status) + { + UpdateTrayIcon(); + }
295-401: Service start/stop/restart operations block the UI thread while waiting for status transitions
OnStartService,OnStopService, andOnRestartServicecall intoServiceStatusService, which ultimately usesServiceController.WaitForStatus(...). These calls run on the UI thread from the context-menu click handlers, so if the service takes a while to start/stop (or hits the timeout), the entire tray UI will freeze until the operation completes.Functionally it works, but UX can degrade noticeably on slow or failing services. Consider offloading the blocking calls to a background task while keeping the UI responsive, e.g.:
- Use
Task.Runaround the service control call and then marshal back to the UI thread forMessageBox/balloon notifications.- Optionally show a short “Starting…” / “Stopping…” balloon or status indicator while the operation is in progress.
No change required for correctness, but this is a worthwhile improvement for responsiveness.
403-444: Handle empty or invalid Web UI URL more explicitly
OnOpenWebUIassumesGetWebUIUrlAsync()returns a non-empty, valid URL and passes it directly toProcess.Start. If IPComm returns an empty string or some unexpected value, you’ll hit thecatchpath, but the user only sees a generic “Failed to open web UI” message.To make failures clearer and avoid relying on
Process.Startthrowing, you could guard the value first:- try - { - var webUrl = await _serviceStatusService.GetWebUIUrlAsync(); - - Process.Start(new ProcessStartInfo - { - FileName = webUrl, - UseShellExecute = true - }); - } + try + { + var webUrl = await _serviceStatusService.GetWebUIUrlAsync(); + if (string.IsNullOrWhiteSpace(webUrl)) + { + MessageBox.Show( + "The service did not provide a Web UI URL.\n\n" + + "Please verify the server configuration.", + "Web UI URL Unavailable", + MessageBoxButtons.OK, + MessageBoxIcon.Warning); + return; + } + + Process.Start(new ProcessStartInfo + { + FileName = webUrl, + UseShellExecute = true + }); + }Minor, but makes diagnostics clearer if the URL exchange fails.
src/Papercut.Service/Infrastructure/IPComm/PublishAppEventBase.cs (1)
43-45: Add debug logging for swallowed exceptions.Silent exception swallowing makes troubleshooting difficult when cancellation or timeout issues occur. Consider logging at Debug level to aid diagnostics.
Apply this diff:
- catch (Exception ex) when (ex is TaskCanceledException or TimeoutException) - { - } + catch (Exception ex) when (ex is TaskCanceledException or TimeoutException) + { + logger.Debug( + ex, + "Publishing {EventName} to {ConnectTo} was cancelled or timed out", + @event.GetType().Name, + ConnectTo); + }src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs (3)
52-87: Consider adding synchronization for cache access.The class is registered as
SingleInstance, but cache reads/writes (_cachedWebUrl, _lastUrlCheck) lack synchronization. Multiple threads could simultaneously detect an expired cache and all invoke IPComm, leading to redundant network calls and potential inconsistencies.Add a
SemaphoreSlimfor async-safe locking:+ private readonly SemaphoreSlim _cacheLock = new(1, 1); + public async Task<string> GetWebUIUrlAsync() { // Return cached URL if still valid if (_cachedWebUrl != null && DateTime.Now - _lastUrlCheck < _urlCacheExpiration) { return _cachedWebUrl; } + await _cacheLock.WaitAsync(); try { + // Double-check after acquiring lock + if (_cachedWebUrl != null && DateTime.Now - _lastUrlCheck < _urlCacheExpiration) + { + return _cachedWebUrl; + } + var serviceClient = ipCommClientFactory.GetClient(PapercutIPCommClientConnectTo.Service); // ... rest of method } catch (Exception ex) { logger.Warning(ex, "Failed to probe for Papercut Service Listening Web Url"); } + finally + { + _cacheLock.Release(); + } // Fallback logic... }
48-48: PreferDateTime.UtcNowfor cache expiration.Using
DateTime.Nowfor time-based comparisons can cause subtle issues during DST transitions.DateTime.UtcNowis more robust and is the recommended practice for duration-based logic.Apply this diff:
public string? CachedWebUrl => - _cachedWebUrl != null && DateTime.Now - _lastUrlCheck < _urlCacheExpiration + _cachedWebUrl != null && DateTime.UtcNow - _lastUrlCheck < _urlCacheExpiration ? _cachedWebUrl : null; public async Task<string> GetWebUIUrlAsync() { // Return cached URL if still valid - if (_cachedWebUrl != null && DateTime.Now - _lastUrlCheck < _urlCacheExpiration) + if (_cachedWebUrl != null && DateTime.UtcNow - _lastUrlCheck < _urlCacheExpiration) { return _cachedWebUrl; } // ... after successful response _cachedWebUrl = $"http://{serviceWebUiSettings.IP}:{serviceWebUiSettings.Port}"; - _lastUrlCheck = DateTime.Now; + _lastUrlCheck = DateTime.UtcNow; // ... in fallback Log.Debug("Using fallback web URL: {Url}", FallbackWebUrl); _cachedWebUrl = FallbackWebUrl; - _lastUrlCheck = DateTime.Now; + _lastUrlCheck = DateTime.UtcNow;Also applies to: 55-55, 71-71, 84-84
102-102: Simplify discard pattern.
var _ = await ...is unusual; prefer_ = await ...or justawait ...since the result is intentionally discarded.Apply this diff:
- var _ = await this.GetWebUIUrlAsync(); + _ = await this.GetWebUIUrlAsync();Or simply:
- var _ = await this.GetWebUIUrlAsync(); + await this.GetWebUIUrlAsync();src/Papercut.Service/Domain/SmtpServer/SmtpServerOptions.cs (1)
72-81:GetSettingsimplementation looks good; consider small ergonomics tweakThe projection from
SmtpServerOptionsintoSmtpServerSettingsis correct and centralizes the mapping nicely.To make this API a bit safer and easier to reuse for future callers, you could:
- Guard against a null
ipOverrideand fall back toIP, and/or- Provide a parameterless overload that uses the option’s own
IPandPort.For example:
- public SmtpServerSettings GetSettings(string ipOverride, int portOverride) => new(ipOverride, + public SmtpServerSettings GetSettings(string ipOverride, int portOverride) => new( + ipOverride ?? IP, portOverride, CertificateFindType, CertificateFindValue, CertificateStoreLocation, CertificateStoreName, MessagePath, LoggingPath, AllowedIps); + + public SmtpServerSettings GetSettings() => GetSettings(IP, Port);src/Papercut.Service/Infrastructure/Configuration/SmtpServerOptionsProvider.cs (1)
25-53: MakeSmtpServerSettingsDI registration more explicitThe provider behavior and
Settingsprojection look good: you mergeSmtpServerOptionswith persisted IP/Port fromISettingStoreand expose a typed snapshot.For the Autofac registration, the intent is that
SmtpServerSettingscan be directly resolved from the container:builder.Register(p => p.Resolve<ISmtpServerOptionsProvider>().Settings) .AsSelf() .InstancePerDependency();To avoid any ambiguity about how Autofac infers the service type from the delegate, I’d recommend registering it explicitly as
SmtpServerSettings:- builder.Register(p => p.Resolve<ISmtpServerOptionsProvider>().Settings).AsSelf().InstancePerDependency(); + builder + .Register(p => p.Resolve<ISmtpServerOptionsProvider>().Settings) + .As<SmtpServerSettings>() + .InstancePerDependency();This makes the container contract clearer and reduces the chance of surprises if Autofac’s defaults change or if similar registrations are added later.
src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (2)
28-51: Depend onISmtpServerOptionsProviderinstead of concrete provider
SmtpServerManagercurrently takes and stores a concreteSmtpServerOptionsProvider:private readonly SmtpServerOptionsProvider _settingsProvider; public SmtpServerManager( PapercutSmtpServer smtpServer, SmtpServerOptionsProvider settingsProvider, IPAllowedList ipAllowedList, ISettingStore settingStore, ILogger logger) { _smtpServer = smtpServer; _settingsProvider = settingsProvider; ... }Since the class only uses the
Settingsproperty, it can depend on theISmtpServerOptionsProviderinterface instead. That will improve testability and keep the manager decoupled from the infrastructure implementation:- private readonly SmtpServerOptionsProvider _settingsProvider; + private readonly ISmtpServerOptionsProvider _settingsProvider; - public SmtpServerManager(PapercutSmtpServer smtpServer, - SmtpServerOptionsProvider settingsProvider, + public SmtpServerManager( + PapercutSmtpServer smtpServer, + ISmtpServerOptionsProvider settingsProvider, IPAllowedList ipAllowedList, ISettingStore settingStore, ILogger logger)(and the rest of the class stays the same).
124-129: Log message mentions retries that no longer existThe warning message still references “After 5 Retries”:
_logger.Warning( ex, "Unable to Create SMTP Server Listener on {IP}:{Port}. After 5 Retries. Failing", smtpServerSettings.IP, smtpServerSettings.Port);But
BindSMTPServerdoesn’t implement any retry loop. To avoid confusion during troubleshooting, consider updating the text, e.g.:- "Unable to Create SMTP Server Listener on {IP}:{Port}. After 5 Retries. Failing", + "Unable to create SMTP server listener on {IP}:{Port}. Failing without further retries",src/Papercut.Service/Infrastructure/Rules/RuleService.cs (2)
78-100: Guard against multiple periodic subscriptions
SetupPeriodicRuleObservableis called fromHandleAsync(PapercutClientReadyEvent)(Line 97). IfPapercutClientReadyEventcan fire more than once over the service lifetime, each call will create a newObservable.Intervalsubscription and overwrite_periodicRuleSubscription(Line 136), leaving prior subscriptions undisposed and causing multiple periodic rule executions.Consider disposing any existing subscription before assigning a new one, or short‑circuiting if it’s already initialized, e.g.:
- private void SetupPeriodicRuleObservable() - { - _logger.Debug("Setting up Periodic Rule Observable {RunInterval}", PeriodicRunInterval); - - _periodicRuleSubscription = Observable.Interval(PeriodicRunInterval, TaskPoolScheduler.Default) + private void SetupPeriodicRuleObservable() + { + _periodicRuleSubscription?.Dispose(); + + _logger.Debug("Setting up Periodic Rule Observable {RunInterval}", PeriodicRunInterval); + + _periodicRuleSubscription = Observable.Interval(PeriodicRunInterval, TaskPoolScheduler.Default) .SubscribeAsync( async e => await rulesRunner.RunPeriodicBackgroundRules( Rules.OfType<IPeriodicBackgroundRule>().ToArray(), - CancellationToken.None), + CancellationToken.None), ex => { // logging as today });This keeps behavior the same while preventing leaked interval subscriptions if the event is raised multiple times.
153-157: Autofac registration is fine; optional null‑guardThe static
Registermethod (Line 153) correctly registersRuleServiceas both self and implemented interfaces withInstancePerLifetimeScope, consistent with the rest of the codebase.As a minor optional defensive tweak, you could mirror the UI
RuleServicepattern and addArgumentNullException.ThrowIfNull(builder);at the top to fail fast on misconfigured registration calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
examples/appsettings.json(1 hunks)src/Papercut.Core/Domain/Message/MessageEntry.cs(3 hunks)src/Papercut.Core/Domain/Message/MessageEntryDto.cs(1 hunks)src/Papercut.Core/Domain/Message/NewMessageEvent.cs(1 hunks)src/Papercut.Core/Domain/Network/EndpointDefinition.cs(3 hunks)src/Papercut.Message/ReceivedDataMessageHandler.cs(2 hunks)src/Papercut.Service.TrayNotification/AppLayer/ServiceTrayCoordinator.cs(1 hunks)src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs(1 hunks)src/Papercut.Service.TrayNotification/Program.cs(1 hunks)src/Papercut.Service/Domain/ISmtpServerOptionsProvider.cs(1 hunks)src/Papercut.Service/Domain/SmtpServer/SmtpServerOptions.cs(1 hunks)src/Papercut.Service/Domain/SmtpServer/SmtpServerSettings.cs(1 hunks)src/Papercut.Service/Infrastructure/Configuration/SmtpServerOptionsProvider.cs(1 hunks)src/Papercut.Service/Infrastructure/IPComm/PublishAppEventBase.cs(1 hunks)src/Papercut.Service/Infrastructure/IPComm/ReplyWithDefaultMessageSavePathService.cs(2 hunks)src/Papercut.Service/Infrastructure/Paths/ServerPathTemplateProviderService.cs(1 hunks)src/Papercut.Service/Infrastructure/Rules/RuleService.cs(3 hunks)src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs(2 hunks)src/Papercut.Service/PapercutServiceModule.cs(2 hunks)src/Papercut.Service/PapercutServiceStartup.cs(1 hunks)src/Papercut.Service/appsettings.Development.json(1 hunks)src/Papercut.Service/appsettings.Production.json(0 hunks)src/Papercut.Service/appsettings.json(1 hunks)
💤 Files with no reviewable changes (1)
- src/Papercut.Service/appsettings.Production.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Papercut.Service.TrayNotification/Program.cs
- src/Papercut.Service/PapercutServiceStartup.cs
🧰 Additional context used
🧬 Code graph analysis (16)
src/Papercut.Service/Domain/ISmtpServerOptionsProvider.cs (2)
src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (1)
SmtpServerSettings(97-97)src/Papercut.Service/Domain/SmtpServer/SmtpServerOptions.cs (1)
SmtpServerSettings(73-81)
src/Papercut.Core/Domain/Message/MessageEntry.cs (1)
src/Papercut.Core/Domain/Message/MessageEntryDto.cs (2)
MessageEntry(33-33)MessageEntryDto(21-34)
src/Papercut.Service/Domain/SmtpServer/SmtpServerOptions.cs (1)
src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (1)
SmtpServerSettings(97-97)
src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs (5)
src/Papercut.Infrastructure.IPComm/PapercutIPCommClientFactory.cs (1)
PapercutIPCommClientFactory(30-46)src/Papercut.Core/Infrastructure/Lifecycle/PapercutServiceReadyEvent.cs (1)
PapercutServiceReadyEvent(24-27)src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs (1)
Task(31-68)src/Papercut.Service.TrayNotification/Infrastructure/ServiceStatusService.cs (1)
Task(119-122)src/Papercut.Common/Helper/StringHelpers.cs (1)
IsSet(42-45)
src/Papercut.Service/PapercutServiceModule.cs (4)
src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (2)
Register(211-215)SmtpServerSettings(97-97)src/Papercut.Service/Infrastructure/Paths/ServerPathTemplateProviderService.cs (1)
Register(44-51)src/Papercut.Service/Domain/SmtpServer/SmtpServerOptions.cs (1)
SmtpServerSettings(73-81)src/Papercut.Core/Infrastructure/Container/RegisterMethodExtensions.cs (1)
RegisterStaticMethods(77-87)
src/Papercut.Service/Infrastructure/Configuration/SmtpServerOptionsProvider.cs (4)
src/Papercut.Service/Domain/SmtpServer/SmtpServerOptions.cs (2)
SmtpServerOptions(23-82)SmtpServerSettings(73-81)src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (2)
SmtpServerSettings(97-97)Register(211-215)src/Papercut.Service/PapercutServiceStartup.cs (1)
UsedImplicitly(70-77)src/Papercut.Service/Infrastructure/Paths/ServerPathTemplateProviderService.cs (1)
Register(44-51)
src/Papercut.Service/Infrastructure/IPComm/ReplyWithDefaultMessageSavePathService.cs (4)
src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (5)
SmtpServerSettings(97-97)Task(53-57)Task(59-95)Task(99-130)Register(211-215)src/Papercut.Service/Domain/SmtpServer/SmtpServerOptions.cs (1)
SmtpServerSettings(73-81)src/Papercut.Service/Infrastructure/IPComm/ReplyWithWebSettingsService.cs (1)
Task(31-68)src/Papercut.Service/Infrastructure/Paths/ServerPathTemplateProviderService.cs (1)
Register(44-51)
src/Papercut.Service/Domain/SmtpServer/SmtpServerSettings.cs (2)
src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (1)
SmtpServerSettings(97-97)src/Papercut.Service/Domain/SmtpServer/SmtpServerOptions.cs (1)
SmtpServerSettings(73-81)
src/Papercut.Service.TrayNotification/AppLayer/ServiceTrayCoordinator.cs (6)
src/Papercut.Service.TrayNotification/Infrastructure/AppRunOnStartupService.cs (4)
AppRunOnStartupService(31-156)IsRunOnStartupEnabled(38-59)SetRunOnStartup(64-143)UsedImplicitly(147-153)src/Papercut.Service.TrayNotification/Infrastructure/ServiceStatusService.cs (10)
ServiceStatusService(28-185)UpdateStatus(43-70)GetStatusText(132-148)CanStart(153-156)CanStop(161-164)CanRestart(169-172)StartService(75-85)StopService(90-99)RestartService(104-114)UsedImplicitly(176-182)src/Papercut.Service.TrayNotification/AppLayer/NewMessageNotificationService.cs (1)
NewMessageNotificationService(28-72)src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs (2)
Start(89-95)UsedImplicitly(121-127)src/Papercut.Service.TrayNotification/Infrastructure/PapercutServiceControllerProvider.cs (4)
Start(64-69)Stop(74-79)ServiceControllerStatus(37-41)UsedImplicitly(95-101)src/Papercut.Service.TrayNotification/AppLayer/TrayLoggingPathConfigurator.cs (1)
Register(34-37)
src/Papercut.Core/Domain/Message/MessageEntryDto.cs (1)
src/Papercut.Core/Domain/Message/MessageEntry.cs (4)
MessageEntryDto(156-166)MessageEntry(29-167)MessageEntry(46-50)MessageEntry(52-55)
src/Papercut.Service/Infrastructure/Paths/ServerPathTemplateProviderService.cs (2)
src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (1)
SmtpServerSettings(97-97)src/Papercut.Service/Domain/SmtpServer/SmtpServerOptions.cs (1)
SmtpServerSettings(73-81)
src/Papercut.Service/Infrastructure/IPComm/PublishAppEventBase.cs (3)
src/Papercut.Infrastructure.IPComm/PapercutIPCommClientFactory.cs (1)
PapercutIPCommClientFactory(30-46)src/Papercut.Infrastructure.IPComm/Network/PapercutIPCommClient.cs (4)
Task(31-60)Task(62-88)Task(90-109)Task(111-136)src/Papercut.Service/Infrastructure/IPComm/PublishAppEventsHandlerToTrayService.cs (2)
Task(29-32)Task(34-37)
src/Papercut.Core/Domain/Message/NewMessageEvent.cs (3)
src/Papercut.Core/Domain/Message/MessageEntry.cs (1)
MessageEntryDto(156-166)src/Papercut.Core/Domain/Message/MessageEntryDto.cs (1)
MessageEntryDto(21-34)src/Papercut.UI/ViewModels/MessageListViewModel.cs (1)
NewMessage(319-322)
src/Papercut.Message/ReceivedDataMessageHandler.cs (2)
src/Papercut.Core/Domain/Message/MessageEntry.cs (3)
MessageEntry(29-167)MessageEntry(46-50)MessageEntry(52-55)src/Papercut.Core/Domain/Message/MessageEntryDto.cs (1)
MessageEntry(33-33)
src/Papercut.Service/Infrastructure/Rules/RuleService.cs (5)
src/Papercut.UI/AppLayer/Rules/RuleService.cs (10)
RuleService(28-223)Task(43-48)Task(50-53)Task(55-60)Task(62-86)Task(88-142)Task(144-157)Task(173-188)Task(190-205)ValueTask(159-171)src/Papercut.Rules/App/RuleServiceBase.cs (2)
RuleServiceBase(32-94)RuleServiceBase(40-46)src/Papercut.Core/Infrastructure/Async/AsyncHelpers.cs (1)
IDisposable(25-46)src/Papercut.Rules/App/RulesRunner.cs (2)
Task(58-79)Task(81-103)src/Papercut.Core/Infrastructure/BackgroundTasks/BackgroundTaskRunner.cs (3)
Task(60-76)QueueBackgroundTask(50-58)ValueTask(78-108)
src/Papercut.Service/Infrastructure/Servers/SmtpServerManager.cs (6)
src/Papercut.Service/Infrastructure/Configuration/SmtpServerOptionsProvider.cs (1)
SmtpServerOptionsProvider(25-56)src/Papercut.Infrastructure.Smtp/PapercutSmtpServer.cs (3)
PapercutSmtpServer(34-162)Task(52-76)Task(78-124)src/Papercut.Common/Helper/StringHelpers.cs (2)
IsSet(42-45)IsNullOrWhiteSpace(63-66)src/Papercut.Core/Domain/Settings/ReadWriteValueExtensions.cs (1)
Set(26-34)src/Papercut.Core/Domain/Network/EndpointDefinition.cs (4)
ToString(141-145)EndpointDefinition(24-146)EndpointDefinition(26-30)EndpointDefinition(32-46)src/Papercut.Service/Domain/SmtpServer/SmtpServerOptions.cs (1)
SmtpServerSettings(73-81)
⏰ 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: Build Papercut SMTP
- GitHub Check: Analyze (csharp)
- GitHub Check: Build Papercut SMTP
🔇 Additional comments (17)
src/Papercut.Service/PapercutServiceModule.cs (2)
18-18: New domain using is appropriateImporting
Papercut.Service.Domainhere is correct and aligns this module with the newSmtpServerSettings-based domain model.
47-47: Static registration order change seems fine; just confirm no ordering assumptionsMoving
builder.RegisterStaticMethods(ThisAssembly);to the end ofLoadis generally reasonable and can even be clearer (explicit, non-reflection registrations first, then static registration hooks).It’s worth double-checking that none of the static
Registermethods discovered viaRegisterStaticMethodsassume they run before this module’s explicit registrations, or that they override them. If such assumptions exist, the new order could change which registration “wins” for some services.If there are no such assumptions, this change looks good.
src/Papercut.Core/Domain/Network/EndpointDefinition.cs (1)
41-41: LGTM! Good refactoring to static method with non-nullable return type.The changes improve the code by making
LoadCertificateFromStorestatic (since it doesn't access instance state) and using the non-nullableX509Certificate2return type (since the method either returns a valid certificate or throws an exception, never returning null).Also applies to: 69-69
src/Papercut.Core/Domain/Message/NewMessageEvent.cs (1)
23-23: Switch torecordwithMessageEntryDtopayload is a clean event-contract upgradeUsing a positional
recordwithMessageEntryDtomakes the event immutable and better suited for serialization while decoupling consumers from the richMessageEntrytype.Please double-check all event handlers/consumers of
NewMessageEventto ensure they now expectMessageEntryDtoand callToEntry()only where a fullMessageEntryis still required.src/Papercut.Core/Domain/Message/MessageEntry.cs (1)
42-50: Initialization refactor and ToDto() addition preserve behavior and expose a clean DTO boundaryPromoting
_hasBeenSeen/_isSelectedtoprotectedand extractingInitializeFromFileInfo()improve extensibility without changing the semantics of creation time and “seen” status. The newToDto()method mirrors the DTO fields and gives a straightforward projection for messaging/IPComm without leaking UI concerns.Also applies to: 101-124, 156-166
src/Papercut.Service.TrayNotification/AppLayer/ServiceTrayCoordinator.cs (2)
446-501: Run-on-startup and notification toggles are robustly implementedThe handlers for “Run at Startup” and “Show Notifications” both:
- Validate the sender type.
- Defer to dedicated services/state (
AppRunOnStartupService,NewMessageNotificationService).- Log the outcome.
- Revert the checkbox state on failure to avoid UI/model desync.
This is a solid pattern for keeping the UI truthful even under error conditions.
476-525: Exit flow and disposal strategy look correct
OnExitlogs and callsApplication.Exit(), whileServiceTrayCoordinatorimplementsIDisposableto detach event handlers, stop/dispose the timer, and dispose the tray icon. Given the Autofac registration asSingleInstance, disposal on container shutdown should clean up resources and remove the icon cleanly from the tray.No changes needed here.
src/Papercut.Service/Infrastructure/IPComm/PublishAppEventBase.cs (1)
21-27: LGTM! Clean abstraction with good defaults.The base class design follows the template method pattern effectively, with properly injected dependencies and a reasonable default timeout.
src/Papercut.Service/appsettings.Development.json (1)
20-20: Environment-specific SMTP port override is well-scoped.The development configuration correctly overrides the production port 2525 to use the traditional port 25 for local testing. This aligns with the typical dev environment where elevation is available and isolation is assumed.
examples/appsettings.json (1)
4-4: Example client configuration correctly aligned with new server port.The port update to 2525 in the example configuration ensures that clients following this example will use the correct port for the production server. Consistency maintained across configurations.
src/Papercut.Service/appsettings.json (1)
14-14: Incorrect — port change is documented; clarify README and migration note.The SMTP port default of 2525 is already documented (ReleaseNotes.md, DOCKERHUB.md, README.md, Dockerfile, examples) and Production uses non‑privileged port 2525; Development config still uses 25. There is no automatic runtime fallback to 25 — preserve Papercut.Service.Settings.json or set SmtpServer__Port / map host 25:2525 to retain previous behavior. Update src/Papercut.Service/Readme.md (around the line noting “SMTP port: 25”) to state the Production default is 2525, explain the rationale (avoid privilege elevation), and point users to the existing migration options (docker port mapping and SmtpServer__Port).
Likely an incorrect or invalid review comment.
src/Papercut.Service/Infrastructure/Paths/ServerPathTemplateProviderService.cs (2)
38-40: LGTM!Clean property exposure with appropriate getter-only access. The Type discriminator and unified PathTemplates collection align well with the refactored architecture.
44-51: SmtpServerSettings is properly registered and resolvable.Verification confirms that
SmtpServerSettingsis registered in the Autofac container viaSmtpServerOptionsProvider.Register()as a delegate that resolves fromISmtpServerOptionsProvider.Settings. BothServerPathTemplateProviderServiceandSmtpServerOptionsProvideruse Autofac's static registration discovery pattern (RegisterStaticMethods), and the resolution withinServerPathTemplateProviderService.Register()is deferred—occurring at instantiation time when the container is active, not during registration. The keyed registration strategy andSingleInstancescope are appropriate for configuration-driven path template providers.src/Papercut.Service/Infrastructure/IPComm/ReplyWithDefaultMessageSavePathService.cs (1)
40-40: Good encapsulation improvement.Making the registration method private is appropriate since it's only called via reflection by Autofac's module scanning.
src/Papercut.Service/Domain/SmtpServer/SmtpServerSettings.cs (1)
19-30: Immutable settings record fits the usageThe
SmtpServerSettingspositional record cleanly captures the SMTP configuration as an immutable snapshot and matches how you’re consuming it (e.g., in the options provider and server manager). No issues here.src/Papercut.Service/Infrastructure/Rules/RuleService.cs (2)
31-40: Primary constructor + event interfaces look consistentThe primary constructor + implemented
IEventHandler<>interfaces are wired cleanly and align with the patterns used in the UIRuleService(constructor-injected dependencies, baseRuleServiceBaseinitialization, and event handling responsibilities). No issues from a DI or lifetime perspective here.
47-76: NewMessage handling viaIBackgroundTaskRunneris soundUsing
backgroundTaskRunner.QueueBackgroundTaskin Line 53 to offloadRunNewMessageRulesis a good fit with theBackgroundTaskRunnerpattern (seeBackgroundTaskRunner.ProcessTasksAsync). Passingt(the runner’s cancellation token) through toRunNewMessageRules(Line 59–62) ensures new-message rule execution observes shutdown via the background runner.The 2‑second delay and swallowing of
ObjectDisposedException/TaskCanceledException(Lines 58–69) are reasonable to avoid noisy logs during shutdown, given the final catch still logs unexpected failures with context.
| logger.Debug("Startup: Attempting to IPComm to Service to get the Web UI Url..."); | ||
|
|
||
| // fire and forget the initial warm-up | ||
| _ = Task.Run(async () => await this.GetWebUIUrlAsync()); |
There was a problem hiding this comment.
Add error handling to prevent silent failures.
The past review requested "attach a continuation or try/catch to log any exceptions so failures are not lost." The blocking call was fixed, but exceptions thrown by GetWebUIUrlAsync() inside Task.Run will still be silently swallowed.
Apply this diff to log exceptions:
- _ = Task.Run(async () => await this.GetWebUIUrlAsync());
+ _ = Task.Run(async () =>
+ {
+ try
+ {
+ await this.GetWebUIUrlAsync();
+ }
+ catch (Exception ex)
+ {
+ logger.Warning(ex, "Failed to warm up Web UI URL cache on startup");
+ }
+ });🤖 Prompt for AI Agents
In src/Papercut.Service.TrayNotification/Infrastructure/ServiceCommunicator.cs
around line 94, the Task.Run call invokes GetWebUIUrlAsync() but any exception
it throws is silently swallowed; wrap the call in a try/catch or attach a
continuation to log exceptions so failures are not lost — e.g. inside Task.Run
use: try { await this.GetWebUIUrlAsync(); } catch (Exception ex) {
this.logger?.Error(ex, "Failed to get Web UI URL"); } (or call .ContinueWith(t
=> log t.Exception) ensuring AggregateException is handled) so all exceptions
are logged.
| "AllowedIps": "*" | ||
| }, | ||
|
|
||
| "Urls": "http://0.0.0.0:8080", |
There was a problem hiding this comment.
🧩 Analysis chain
Restrict Kestrel binding to prevent unintended network exposure.
The Urls binding to 0.0.0.0:8080 exposes the web UI to all network interfaces, which could allow remote access from the entire network. In production environments, this is a security posture gap. Constrain the binding to localhost or specific trusted IPs.
Verify the appsettings.Production.json configuration constrains this binding appropriately, or apply this diff to use localhost:
- "Urls": "http://0.0.0.0:8080",
+ "Urls": "http://localhost:8080",I'll help verify this review comment by examining the configuration files and deployment context.
🏁 Script executed:
#!/bin/bash
# Find all appsettings configuration files
fd -e json -path "*appsettings*" --type f
echo "---"
# Show content of appsettings.json (first 30 lines to get context)
echo "=== appsettings.json (first 30 lines) ==="
head -n 30 src/Papercut.Service/appsettings.json 2>/dev/null || echo "File not found"
echo ""
echo "=== appsettings.Production.json (if exists) ==="
cat src/Papercut.Service/appsettings.Production.json 2>/dev/null || echo "File not found"
echo ""
echo "=== appsettings.Development.json (if exists) ==="
cat src/Papercut.Service/appsettings.Development.json 2>/dev/null || echo "File not found"Length of output: 1261
Confirm Kestrel binding is constrained in production environment.
The review comment is correct. The base appsettings.json specifies "Urls": "http://0.0.0.0:8080", and appsettings.Production.json is empty—meaning production environments inherit the 0.0.0.0 binding without constraint. In contrast, appsettings.Development.json correctly uses "http://localhost:37408".
This exposes the web UI to all network interfaces in production. Either populate appsettings.Production.json with a constrained binding (e.g., localhost:8080) or confirm environment variable overrides are in place. If relying on environment variables, ensure they're documented.
🤖 Prompt for AI Agents
In src/Papercut.Service/appsettings.json around line 20 the "Urls":
"http://0.0.0.0:8080" setting exposes Kestrel on all interfaces in production;
replace this by constraining the production binding: add an
appsettings.Production.json that sets a safer URL (e.g., localhost:8080) or
remove the Urls entry from base appsettings and move it into
appsettings.Development.json, and if you intend to rely on environment variables
instead, add documentation and ensure the production deployment supplies the
appropriate ASPNETCORE_URLS override so production does not inherit 0.0.0.0.
| namespace Papercut.Service.Domain; | ||
|
|
||
| public interface ISmtpServerOptionsProvider | ||
| { | ||
| SmtpServerSettings Settings { get; } | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure SmtpServerSettings is in scope for this interface
ISmtpServerOptionsProvider lives in Papercut.Service.Domain, but SmtpServerSettings is defined under Papercut.Service.Domain.SmtpServer. Unless there’s a project-level global using for that namespace, this won’t compile.
Even if a global using exists, it’s clearer and more robust to explicitly import or fully qualify the type here:
-namespace Papercut.Service.Domain;
+namespace Papercut.Service.Domain;
+
+using Papercut.Service.Domain.SmtpServer;or:
-public interface ISmtpServerOptionsProvider
-{
- SmtpServerSettings Settings { get; }
-}
+public interface ISmtpServerOptionsProvider
+{
+ SmtpServer.SmtpServerSettings Settings { get; }
+}
---
🏁 Script executed:
```shell
#!/bin/bash
# Find SmtpServerSettings definition
rg -n "class SmtpServerSettings|record SmtpServerSettings|struct SmtpServerSettings" --type cs
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Check ISmtpServerOptionsProvider.cs full content including usings
cat src/Papercut.Service/Domain/ISmtpServerOptionsProvider.csLength of output: 780
🏁 Script executed:
#!/bin/bash
# Look for global usings in the project (typically in .csproj, GlobalUsings.cs, or namespace root)
fd -name "GlobalUsings.cs" -o -name "*.csproj" | head -10 | xargs grep -l "global using" 2>/dev/null || echo "Searching for global using statements..."
rg "global using" --type cs -n | head -20Length of output: 1931
🏁 Script executed:
#!/bin/bash
# Check the namespace structure around Domain directory
fd --type f --name "*.cs" src/Papercut.Service/Domain/ | head -20Length of output: 250
---
🏁 Script executed:
```shell
#!/bin/bash
# View SmtpServerSettings.cs to confirm its namespace
head -30 src/Papercut.Service/Domain/SmtpServer/SmtpServerSettings.cs
Length of output: 974
🏁 Script executed:
#!/bin/bash
# Check if there's a .csproj with ImplicitUsings enabled
find src/Papercut.Service -name "*.csproj" -exec grep -H "ImplicitUsings" {} \;Length of output: 86
🏁 Script executed:
#!/bin/bash
# Check if SmtpServerSettings is re-exported/aliased in Papercut.Service.Domain namespace
rg "using.*SmtpServer|SmtpServerSettings" src/Papercut.Service/Domain/ --type cs -A 2 -B 2Length of output: 1241
🏁 Script executed:
#!/bin/bash
# List all .cs files in Domain directory structure
find src/Papercut.Service/Domain -type f -name "*.cs"Length of output: 178
Add missing using for SmtpServerSettings in ISmtpServerOptionsProvider.cs
The verification confirms this is a real compilation issue. SmtpServerSettings is defined in Papercut.Service.Domain.SmtpServer, while ISmtpServerOptionsProvider is in Papercut.Service.Domain. The file has no using statement for the SmtpServer namespace, and the project's global usings do not include it. C#'s ImplicitUsings feature only adds framework/standard namespaces, not project namespaces.
Add the missing using:
namespace Papercut.Service.Domain;
+using Papercut.Service.Domain.SmtpServer;
+
public interface ISmtpServerOptionsProvider
{
SmtpServerSettings Settings { get; }
}📝 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.
| namespace Papercut.Service.Domain; | |
| public interface ISmtpServerOptionsProvider | |
| { | |
| SmtpServerSettings Settings { get; } | |
| } | |
| using Papercut.Service.Domain.SmtpServer; | |
| namespace Papercut.Service.Domain; | |
| public interface ISmtpServerOptionsProvider | |
| { | |
| SmtpServerSettings Settings { get; } | |
| } |
🤖 Prompt for AI Agents
In src/Papercut.Service/Domain/ISmtpServerOptionsProvider.cs around lines 18 to
23, the file references SmtpServerSettings from the
Papercut.Service.Domain.SmtpServer namespace but lacks a using for it; add
"using Papercut.Service.Domain.SmtpServer;" at the top of the file (below the
file header/namespace declarations as appropriate) so the SmtpServerSettings
type resolves and the file compiles.
|
|
||
| await ipCommClient.PublishEventServer(@event, PublishTimeout, token); | ||
| } | ||
| catch (Exception ex) when (ex is TaskCanceledException or TimeoutException) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that TimeoutException is actually thrown.
Based on the relevant code snippets, the IPComm layer uses CancellationTokenSource with timeout, which throws TaskCanceledException (a subclass of OperationCanceledException), not TimeoutException. The TimeoutException catch clause may be unreachable.
Run the following script to verify if TimeoutException is thrown anywhere in the call chain:
🏁 Script executed:
#!/bin/bash
# Description: Search for TimeoutException throws in the IPComm infrastructure
# Search for TimeoutException being thrown
rg -nP --type=cs -C3 'throw.*TimeoutException' src/Papercut.Infrastructure.IPComm/ src/Papercut.Service/Infrastructure/IPComm/
# Search for methods that might throw TimeoutException (Task.Wait, WaitAll, etc.)
rg -nP --type=cs -C3 '\.(Wait|WaitAll|WaitAny)\s*\(' src/Papercut.Infrastructure.IPComm/ src/Papercut.Service/Infrastructure/IPComm/Length of output: 544
Now I need to examine the actual Connection.cs file more closely to understand the full context:
Remove the TimeoutException catch clause—it cannot be thrown by the code path.
Based on C# specifications and the code found via grep, .Wait() without a timeout parameter returns false when the interval elapses rather than throwing an exception. Line 44 shows this.Protocol.BeginAsync(this).Wait() with no timeout parameter, and CancellationTokenSource.CancelAfter is used for timeouts in async/await, which throws OperationCanceledException, not TimeoutException. The TimeoutException portion of the catch clause at line 43 is unreachable dead code and should be removed.
The catch should handle only TaskCanceledException (or broaden to OperationCanceledException which includes it).
🤖 Prompt for AI Agents
In src/Papercut.Service/Infrastructure/IPComm/PublishAppEventBase.cs around line
43, the catch clause currently specifies "when (ex is TaskCanceledException or
TimeoutException)"; remove the TimeoutException check because the call path
(this.Protocol.BeginAsync(this).Wait() with no timeout and
CancellationTokenSource.CancelAfter) cannot throw TimeoutException—only
cancellation exceptions; change the catch to handle TaskCanceledException alone
or broaden to OperationCanceledException, and update the when-filter accordingly
so the unreachable TimeoutException branch is deleted.
| namespace Papercut.Service.Infrastructure.IPComm; | ||
|
|
||
| public class ReplyWithDefaultMessageSavePathService(MessagePathConfigurator messagePathConfigurator, SmtpServerOptions smtpServerOptions) | ||
| public class ReplyWithDefaultMessageSavePathService(MessagePathConfigurator messagePathConfigurator, SmtpServerSettings smtpServerSettings) |
There was a problem hiding this comment.
🧩 Analysis chain
Type change looks correct; verify settings don't become stale.
The transition from SmtpServerOptions to SmtpServerSettings aligns with the broader refactoring and is appropriate for this service's needs.
However, since SmtpServerSettings is constructor-injected and the service is registered as InstancePerLifetimeScope, verify that SMTP configuration changes at runtime (e.g., via SmtpServerBindEvent handled by SmtpServerManager) don't result in this service replying with outdated IP/Port values. Compare with ReplyWithWebSettingsService, which dynamically reads settings from IServer in its handler method rather than capturing them at construction time.
Run the following script to check how SmtpServerSettings is registered in the DI container and whether it's updated when configuration changes:
Also applies to: 32-33
🏁 Script executed:
#!/bin/bash
# Description: Verify SmtpServerSettings DI registration and update mechanism
# Find SmtpServerSettings registrations in Autofac
echo "=== SmtpServerSettings DI Registrations ==="
rg -n --type=cs -C3 'Register.*SmtpServerSettings|SmtpServerSettings.*Register'
# Check if there's a mechanism to refresh/invalidate services when settings change
echo -e "\n=== Settings Change Handlers ==="
rg -n --type=cs -C3 'SmtpServerBindEvent'
# Look for lifetime scope creation in IPComm event handling
echo -e "\n=== IPComm Event Handler Scopes ==="
rg -n --type=cs -C3 'AppProcessExchangeEvent.*Handle|HandleAsync.*AppProcessExchangeEvent'Length of output: 6924
I'll now search the codebase directly to verify this concern.
🏁 Script executed:
#!/bin/bash
# Description: Find SmtpServerSettings definition, registration, and how it's updated
echo "=== SmtpServerSettings Class Definition ==="
rg -n --type=cs 'class SmtpServerSettings|record SmtpServerSettings' -A5
echo -e "\n=== SmtpServerSettings Properties (Mutable?) ==="
rg -n --type=cs 'class SmtpServerSettings' -A20 | head -40
echo -e "\n=== DI Registration for SmtpServerSettings ==="
rg -n --type=cs 'Register.*SmtpServerSettings|SmtpServerSettings.*Register|builder\.Register' -B2 -A5 | grep -A5 -B5 'SmtpServerSettings'
echo -e "\n=== SmtpServerManager.HandleAsync Implementation ==="
rg -n --type=cs 'class SmtpServerManager' -A100 | grep -A30 'HandleAsync.*SmtpServerBindEvent'Length of output: 6178
Now let me verify the DI registration for ReplyWithDefaultMessageSavePathService and whether new scopes are created for event handling:
Let me search for how ReplyWithDefaultMessageSavePathService is registered and whether scopes are created when handling events:
🏁 Script executed:
#!/bin/bash
# Description: Find ReplyWithDefaultMessageSavePathService registration and event scope handling
echo "=== ReplyWithDefaultMessageSavePathService Registration ==="
rg -n --type=cs 'ReplyWithDefaultMessageSavePathService' -B2 -A2 | head -30
echo -e "\n=== How SmtpServerSettings is Resolved in Event Handlers ==="
rg -n --type=cs 'IEventHandler.*AppProcessExchangeEvent' -B5 -A5
echo -e "\n=== BeginLifetimeScope for Event Handling ==="
rg -n --type=cs 'BeginLifetimeScope' -B2 -A2 | grep -A2 -B2 'event\|Event'
echo -e "\n=== SmtpServerSettings DI Registration (builder.Register context) ==="
rg -n --type=cs 'public.*SmtpServerSettings|private.*SmtpServerSettings' -B2 -A10 | head -50Length of output: 8812
Now let me find the explicit DI registration for SmtpServerSettings:
🏁 Script executed:
#!/bin/bash
# Description: Find how SmtpServerSettings is registered in DI container
echo "=== SmtpServerSettings DI Factory Registration ==="
rg -n --type=cs 'builder\.Register.*SmtpServerSettings|\.Register.*ctx.*SmtpServerSettings' -B3 -A8
echo -e "\n=== SmtpServerSettings Registration Search (broader) ==="
rg -n --type=cs 'RegisterModule\|Load.*builder' -A150 | grep -B10 -A10 'SmtpServerSettings'
echo -e "\n=== SmtpServerOptionsProvider Register Method ==="
rg -n --type=cs 'SmtpServerOptionsProvider' -B2 -A10 | grep -A10 'Register\|static void'
echo -e "\n=== Search for all .Register calls in PapercutServiceModule ==="
cat src/Papercut.Service/PapercutServiceModule.cs | head -150Length of output: 4603
Constructor-injected SmtpServerSettings will become stale; read settings dynamically instead.
Although SmtpServerSettings is registered as InstancePerDependency, ReplyWithDefaultMessageSavePathService is registered as InstancePerLifetimeScope and caches the injected settings at construction time. When SmtpServerBindEvent fires, SmtpServerManager persists new IP/Port values to SettingStore, but the cached service continues using its old settings snapshot. Without new lifetime scopes created for AppProcessExchangeEvent handling, the service will reply with outdated IP/Port until the scope is recreated.
Instead of constructor injection, read settings dynamically from ISmtpServerOptionsProvider (which already queries current SettingStore values) in HandleAsync, similar to how ReplyWithWebSettingsService reads from IServer.
🤖 Prompt for AI Agents
In
src/Papercut.Service/Infrastructure/IPComm/ReplyWithDefaultMessageSavePathService.cs
around line 23, the class currently captures SmtpServerSettings at construction
time which becomes stale; change the service to obtain current settings at
runtime by using ISmtpServerOptionsProvider instead. Remove the
constructor-injected SmtpServerSettings field, inject ISmtpServerOptionsProvider
(or otherwise obtain it via the existing lifetime scope), and inside HandleAsync
call the provider to fetch the latest SMTP options and use those values for
composing the reply (mirroring how ReplyWithWebSettingsService reads from
IServer). Ensure no settings are cached on the service instance so each
HandleAsync uses fresh values from the provider.
| public ServerPathTemplateProviderService(PathTemplateType type, SmtpServerSettings smtpServerSettings) | ||
| { | ||
| var messagePaths = smtpServerOptions.MessagePath.Split(';') | ||
| .Select(s => s.Trim()) | ||
| .Where(s => !string.IsNullOrWhiteSpace(s)); | ||
| Type = type; | ||
|
|
||
| this.MessagePathTemplates = new ObservableCollection<string>(messagePaths); | ||
| var paths = type == PathTemplateType.Message ? smtpServerSettings.MessagePath : smtpServerSettings.LoggingPath; | ||
|
|
||
| var loggingPaths = smtpServerOptions.LoggingPath.Split(';') | ||
| var messagePaths = paths.Split(';') | ||
| .Select(s => s.Trim()) | ||
| .Where(s => !string.IsNullOrWhiteSpace(s)); | ||
|
|
||
| this.LoggingPathTemplates = new ObservableCollection<string>(loggingPaths); | ||
| PathTemplates = new ObservableCollection<string>(messagePaths); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Rename misleading variable and verify null safety.
Two observations:
-
The variable
messagePaths(line 31) is misleading since it holds paths for either message or logging templates depending onType. Consider renaming it topathListorpathEntriesfor clarity. -
Line 29 will throw a
NullReferenceExceptionifMessagePathorLoggingPathis null. Verify thatSmtpServerSettingsguarantees non-null path values, or add defensive null handling.
Apply this diff to improve variable naming:
- var messagePaths = paths.Split(';')
+ var pathEntries = paths.Split(';')
.Select(s => s.Trim())
.Where(s => !string.IsNullOrWhiteSpace(s));
- PathTemplates = new ObservableCollection<string>(messagePaths);
+ PathTemplates = new ObservableCollection<string>(pathEntries);Run the following script to verify null safety:
🏁 Script executed:
#!/bin/bash
# Verify SmtpServerSettings constructor and MessagePath/LoggingPath nullability
# Find SmtpServerSettings definition and check if MessagePath/LoggingPath can be null
ast-grep --pattern $'record SmtpServerSettings($$$) {
$$$
}'
# Check for null handling in SmtpServerSettings usages
rg -nP --type=cs -C3 'SmtpServerSettings\s*\(' -g '!**/bin/**' -g '!**/obj/**'Length of output: 2361
I need to retrieve the complete SmtpServerSettings record definition to verify the null safety of MessagePath and LoggingPath properties. Let me run a focused script to do this.
🏁 Script executed:
#!/bin/bash
# Get the full SmtpServerSettings record definition
cat src/Papercut.Service/Domain/SmtpServer/SmtpServerSettings.csLength of output: 974
I need to verify how SmtpServerSettings is initialized to determine if the null safety concern is valid. Let me search for initialization patterns.
Let me search the codebase for SmtpServerSettings initialization patterns to determine how MessagePath and LoggingPath are loaded.
Based on my verification, I can now provide the rewritten review comment:
Rename misleading variable and add defensive null handling.
Two observations:
-
The variable
messagePaths(line 31) is misleading since it holds paths for either message or logging templates depending onType. Rename it topathEntriesorpathListfor clarity. -
C# nullable reference types don't enforce null checking at runtime, so line 29 could still encounter null values even though
MessagePathandLoggingPathare declared as non-nullablestring. Add a null coalescing operator to provide a safe fallback:var paths = (type == PathTemplateType.Message ? smtpServerSettings.MessagePath : smtpServerSettings.LoggingPath) ?? string.Empty;
Apply this diff:
- var messagePaths = paths.Split(';')
+ var pathEntries = (type == PathTemplateType.Message ? smtpServerSettings.MessagePath : smtpServerSettings.LoggingPath) ?? string.Empty;
+
+ var paths = pathEntries.Split(';')
.Select(s => s.Trim())
.Where(s => !string.IsNullOrWhiteSpace(s));
- PathTemplates = new ObservableCollection<string>(messagePaths);
+ PathTemplates = new ObservableCollection<string>(paths);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/Papercut.Service/Infrastructure/Paths/ServerPathTemplateProviderService.cs
around lines 25 to 36, rename the misleading local variable messagePaths to
something neutral like pathEntries or pathList because it can contain either
message or logging paths, and make path selection defensive by coalescing the
possibly-null MessagePath/LoggingPath: compute paths using the ternary on type
and apply ?? string.Empty before splitting, then trim/filter and assign the
resulting collection to PathTemplates.
| private void SetupPeriodicRuleObservable() | ||
| { | ||
| _logger.Debug("Setting up Periodic Rule Observable {RunInterval}", PeriodicRunInterval); | ||
|
|
||
| _periodicRuleSubscription = Observable.Interval(PeriodicRunInterval, TaskPoolScheduler.Default) | ||
| .SubscribeAsync( | ||
| async e => await rulesRunner.RunPeriodicBackgroundRules( | ||
| Rules.OfType<IPeriodicBackgroundRule>().ToArray(), | ||
| CancellationToken.None), | ||
| ex => | ||
| { | ||
| // Only log if it's not a cancellation exception (which happens during shutdown) | ||
| if (ex is not OperationCanceledException and not TaskCanceledException) | ||
| { | ||
| _logger.Error(ex, "Error Running Periodic Rules"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Pass the service-scoped cancellation token into periodic rules
In SetupPeriodicRuleObservable, RunPeriodicBackgroundRules is currently invoked with CancellationToken.None (Line 140), while RuleService owns a _cancellationTokenSource that’s canceled in DisposeAsync (Line 119). This is a missed opportunity: rules that accept a CancellationToken won’t be able to stop promptly when the service is shutting down.
To align periodic rule behavior with the new‑message path and with the presence of _cancellationTokenSource, consider:
- private void SetupPeriodicRuleObservable()
- {
- _logger.Debug("Setting up Periodic Rule Observable {RunInterval}", PeriodicRunInterval);
-
- _periodicRuleSubscription = Observable.Interval(PeriodicRunInterval, TaskPoolScheduler.Default)
- .SubscribeAsync(
- async e => await rulesRunner.RunPeriodicBackgroundRules(
- Rules.OfType<IPeriodicBackgroundRule>().ToArray(),
- CancellationToken.None),
+ private void SetupPeriodicRuleObservable()
+ {
+ _periodicRuleSubscription?.Dispose();
+
+ _logger.Debug("Setting up Periodic Rule Observable {RunInterval}", PeriodicRunInterval);
+
+ _periodicRuleSubscription = Observable.Interval(PeriodicRunInterval, TaskPoolScheduler.Default)
+ .SubscribeAsync(
+ async _ => await rulesRunner.RunPeriodicBackgroundRules(
+ Rules.OfType<IPeriodicBackgroundRule>().ToArray(),
+ _cancellationTokenSource.Token),
ex =>
{
// Only log if it's not a cancellation exception (which happens during shutdown)
if (ex is not OperationCanceledException and not TaskCanceledException)
{
_logger.Error(ex, "Error Running Periodic Rules");
}
});This makes _cancellationTokenSource purposeful and lets well‑behaved rules participate in graceful shutdown while preserving the existing logging behavior.
I'll verify the review comment by examining the actual codebase. Let me gather the necessary information about the RuleService class structure.
🏁 Script executed:
#!/bin/bash
# Find and examine the RuleService.cs file
fd -t f "RuleService.cs" --path "*Infrastructure/Rules*"Length of output: 233
🏁 Script executed:
#!/bin/bash
# Examine the file structure around the reported lines and _cancellationTokenSource usage
rg -n "_cancellationTokenSource" "src/Papercut.Service/Infrastructure/Rules/RuleService.cs" -A 2 -B 2Length of output: 507
🏁 Script executed:
#!/bin/bash
# Look for the SetupPeriodicRuleObservable method and surrounding context
rg -n "SetupPeriodicRuleObservable" "src/Papercut.Service/Infrastructure/Rules/RuleService.cs" -A 25 -B 2Length of output: 1961
🏁 Script executed:
#!/bin/bash
# Check the DisposeAsync method mentioned at line 119
rg -n "DisposeAsync|Dispose" "src/Papercut.Service/Infrastructure/Rules/RuleService.cs" -A 10 -B 2Length of output: 1365
🏁 Script executed:
#!/bin/bash
# Find the RunPeriodicBackgroundRules method signature to understand parameters
rg -n "RunPeriodicBackgroundRules" "src/Papercut.Service/Infrastructure/Rules/" -A 5 -B 2Length of output: 950
🏁 Script executed:
#!/bin/bash
# Check for other usages of _periodicRuleSubscription to understand disposal impact
rg -n "_periodicRuleSubscription" "src/Papercut.Service/Infrastructure/Rules/RuleService.cs" -B 2 -A 2Length of output: 752
🏁 Script executed:
#!/bin/bash
# Check field declarations for _cancellationTokenSource initialization
rg -n "CancellationTokenSource|private.*_cancellation" "src/Papercut.Service/Infrastructure/Rules/RuleService.cs" -B 2 -A 2Length of output: 231
🏁 Script executed:
#!/bin/bash
# Verify file content around lines 132-148
cat "src/Papercut.Service/Infrastructure/Rules/RuleService.cs" | sed -n '125,155p' | nl -v 125Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check the class declaration and relevant fields
cat "src/Papercut.Service/Infrastructure/Rules/RuleService.cs" | head -80 | tail -60Length of output: 2022
🏁 Script executed:
#!/bin/bash
# Look for any additional context about _periodicRuleSubscription disposal patterns
rg -n "class RuleService" "src/Papercut.Service/Infrastructure/Rules/RuleService.cs" -A 100 | head -120Length of output: 3315
I need to verify the method signature and check if there are other call sites for SetupPeriodicRuleObservable.
🏁 Script executed:
#!/bin/bash
# Find IRulesRunner interface definition to check RunPeriodicBackgroundRules signature
rg -n "interface IRulesRunner|RunPeriodicBackgroundRules" --type cs -A 3 -B 1Length of output: 12081
🏁 Script executed:
#!/bin/bash
# Find all call sites of SetupPeriodicRuleObservable
rg -n "SetupPeriodicRuleObservable" --type cs -B 2 -A 2Length of output: 850
🏁 Script executed:
#!/bin/bash
# Check if RulesUpdatedEvent handler might call SetupPeriodicRuleObservable
rg -n "HandleAsync.*RulesUpdatedEvent" "src/Papercut.Service/Infrastructure/Rules/RuleService.cs" -A 10Length of output: 332
🏁 Script executed:
#!/bin/bash
# Verify _cancellationTokenSource is never recreated after initialization
rg -n "_cancellationTokenSource\s*=" --type csLength of output: 714
🏁 Script executed:
#!/bin/bash
# Check if subscription needs disposal when setup is called multiple times
rg -n "Observable.Interval" --type cs -B 3 -A 10 | head -50Length of output: 2878
Pass the service-scoped cancellation token into periodic rules
The review comment is valid and well-founded. Verification confirms:
-
The
RunPeriodicBackgroundRulesmethod accepts aCancellationTokenparameter, but the code passesCancellationToken.Noneinstead of_cancellationTokenSource.Token. -
Inconsistent behavior: The NewMessageRules handler passes a cancellation token (line 59-62), while PeriodicRules do not—this inconsistency breaks the graceful shutdown pattern.
-
UI precedent: The UI version (src/Papercut.UI/AppLayer/Rules/RuleService.cs line 137) already correctly passes
cancellationSource.Tokeninstead ofCancellationToken.None, demonstrating the correct pattern. -
Disposal safety: While
SetupPeriodicRuleObservableis currently called only once, disposing the previous subscription defensively prevents resource leaks if the method is ever called again (as shown in the UI version's stack pattern).
The suggested changes are appropriate: pass _cancellationTokenSource.Token, add disposal of the previous subscription, and use _ for the unused parameter.
🤖 Prompt for AI Agents
In src/Papercut.Service/Infrastructure/Rules/RuleService.cs around lines 132 to
148, the periodic rules observer currently passes CancellationToken.None,
doesn't dispose any existing subscription, and uses an unused parameter name;
update it to dispose the previous _periodicRuleSubscription if non-null before
assigning a new subscription, change the lambda parameter to '_' for the unused
value, and pass _cancellationTokenSource.Token into RunPeriodicBackgroundRules
so periodic rules honor the service-scoped cancellation token.
| private SmtpServerSettings GetSmtpServerSettings() => _settingsProvider.Settings; | ||
|
|
||
| private async Task BindSMTPServer() | ||
| { | ||
| var smtpServerSettings = GetSmtpServerSettings(); | ||
|
|
||
| try | ||
| { | ||
| await smtpServer.StopAsync(); | ||
| await _smtpServer.StopAsync(); | ||
|
|
||
| EndpointDefinition endpoint; | ||
| var endpoint = GetSmtpEndpoint(); | ||
|
|
||
| var tlsStatus = string.IsNullOrWhiteSpace(smtpServerSettings.CertificateFindValue) | ||
| ? "Disabled" | ||
| : $"Enabled (Cert: {smtpServerSettings.CertificateFindValue})"; | ||
|
|
||
| _logger.Information( | ||
| "SMTP Server Configuration: Address={Address}, Port={Port} TLS={TlsStatus}, Allow={AllowList}", | ||
| endpoint.Address, | ||
| endpoint.Port, | ||
| tlsStatus, | ||
| _ipAllowedList); | ||
|
|
||
| await _smtpServer.StartAsync(endpoint); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.Warning( | ||
| ex, | ||
| "Unable to Create SMTP Server Listener on {IP}:{Port}. After 5 Retries. Failing", | ||
| smtpServerSettings.IP, | ||
| smtpServerSettings.Port); | ||
| } | ||
| } |
There was a problem hiding this comment.
TLS status log can be incorrect when TLS configuration fails
In BindSMTPServer, tlsStatus is derived purely from smtpServerSettings.CertificateFindValue:
var tlsStatus = string.IsNullOrWhiteSpace(smtpServerSettings.CertificateFindValue)
? "Disabled"
: $"Enabled (Cert: {smtpServerSettings.CertificateFindValue})";But GetSmtpEndpoint() may still fall back to a non-TLS EndpointDefinition if any of the enum parses fail or if certificate loading throws. In that case the log line:
_logger.Information(
"SMTP Server Configuration: Address={Address}, Port={Port} TLS={TlsStatus}, Allow={AllowList}",
endpoint.Address,
endpoint.Port,
tlsStatus,
_ipAllowedList);can claim TLS is "Enabled" even though the endpoint is plain SMTP.
Consider basing tlsStatus on the actual endpoint instead, e.g.:
- var smtpServerSettings = GetSmtpServerSettings();
+ var smtpServerSettings = GetSmtpServerSettings();
try
{
await _smtpServer.StopAsync();
- var endpoint = GetSmtpEndpoint();
-
- var tlsStatus = string.IsNullOrWhiteSpace(smtpServerSettings.CertificateFindValue)
- ? "Disabled"
- : $"Enabled (Cert: {smtpServerSettings.CertificateFindValue})";
+ var endpoint = GetSmtpEndpoint();
+
+ var tlsStatus = endpoint.Certificate != null
+ ? $"Enabled (Cert: {smtpServerSettings.CertificateFindValue})"
+ : "Disabled";
_logger.Information(
"SMTP Server Configuration: Address={Address}, Port={Port} TLS={TlsStatus}, Allow={AllowList}",
endpoint.Address,
endpoint.Port,
tlsStatus,
_ipAllowedList);This way the log reflects the actual runtime TLS state instead of just the configuration intent.
Summary
Implements a Windows tray notification application to manage the Papercut SMTP Windows Service, addressing issue #249.
What's New
Papercut.Service.TrayNotification Project
A new Windows Forms tray application that provides:
Architecture
Three-Layer Service Management:
PapercutServiceControllerProvider - Wraps Windows ServiceController API
InstancePerDependencyin AutofacServiceStatusService - Business logic layer
StatusChangedevent for reactive UI updatesServiceTrayCoordinator - UI coordination layer
IPComm Integration
ServiceWebUISettingsExchangeEvent - New IPComm event for web server configuration exchange
IServerAddressesFeatureServiceCommunicator - IPComm client with caching
IStartableCachedWebUrlproperty for instant menu displayReplyWithWebSettingsService - Service-side handler
IServerandIServerAddressesFeatureKey Features
Core Changes
New IPComm Port:
PapercutIPCommConstants.TrayServiceListeningPort= 37404Path Configuration:
TrayLoggingPathConfigurator- Provides logging path template for tray app%DataDirectory%\LogsTechnical Details
icons/Papercut-icon.ico)requireAdministratorTest Plan
Related Issues
Closes #249
Summary by CodeRabbit
New Features
Improvements