elsa-workflows / elsa-workflows/elsa-foundation
Code-quality nits batch (mediator/events/runtime)
- Dominant language
- C#
- Stars
- 5
- Forks
- 1
- Avg merge
- 3h 18m
- Merged PRs (30d)
- 261
Description
_Filed from an automated code-quality analysis. A batch of small, low-risk items per the "batch trivial nits" guideline. Each can be a separate commit._
### 1. Stray nullable constraint on `IRequestHandler`
`src/Elsa/Mediator/Core/Contracts/IRequestHandler.cs:16` declares `where TRequest : IRequest?`. The `?` allows a nullable request type, which is never intended and is inconsistent with `ICommandHandler`/`IEventHandler`. **Remedy:** drop the `?`.
### 2. Inconsistent reflection result extraction in mediator invokers
`CommandHandlerInvokerMiddleware.cs:58-59` uses `resultProperty?.GetValue(task)` (silently allows null) while `RequestHandlerInvokerMiddleware.cs:43-44` uses `...!` (force-unwrap). **Remedy:** unify — extract the result without reflection (cast/`GetAwaiter().GetResult()` on the typed task) or add an explicit guard with a clear exception; apply the same approach in both.
### 3. Asymmetric default logging: Events vs Commands
`CommandPipeline.cs:16-20` includes `CommandLoggingMiddleware` by default; `EventPipeline.cs:28-31` has no logging middleware. **Remedy:** either add an `EventLoggingMiddleware` to the default event pipeline or make logging opt-in for both, so dispatch observability is symmetric.
### 4. Test double in production source throwing NotImplementedException
`src/Elsa/Workflows/Runtime/JavaScript/Activities/RunJavaScript/TestClasses/ScriptExecutionContext.cs:18-36` implements `IActivityExecutionContext` with `NotImplementedException` members and ships under `src/`. **Remedy:** move it to the test project (or rename to a clearly-scoped fake) so a stub can't be resolved in production.
### 5. (Modernization, low priority) `async void` timer callback
`src/Elsa/Tasks/Schedules/ScheduledTaskExecution.cs:21` uses `async void` for a `System.Threading.Timer` callback. This is the *conventional* pattern for timer callbacks and is defensively wrapped in try/catch here, so it is **not a bug** — but a `PeriodicTimer`-based async loop would avoid `async void` entirely and make exceptions/cancellation easier to reason about. Consider only if the file is touched for other reasons.
## Severity / impact
Low across the batch; #1, #2, #4 are quick correctness/clarity wins.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with the listed IRequestHandler contract, mediator invoker middleware, CommandPipeline and EventPipeline files, then inspect the ScriptExecutionContext test double and its test project. Review the existing handler and pipeline tests before changing these separate items. Done means the nullable constraint, reflection handling, default logging behavior, and production placement are consistent and covered without altering unrelated timer code.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- backend, testing
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100