elsa-workflows / elsa-workflows/elsa-foundation

Code-quality nits batch (mediator/events/runtime)

Open
#279 1 comment 0 reactions 0 assignees View on GitHub
ready-for-human status:todo
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.