Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 96 additions & 12 deletions UtilitiesCS.Test/Threading/ProgressViewer_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,71 @@ public class ProgressViewer_Tests
private static ProgressViewer CreateHeadlessViewer() =>
(ProgressViewer)FormatterServices.GetUninitializedObject(typeof(ProgressViewer));

/// <summary>
/// Verifies that assigning <see cref="ProgressViewer.CancelSource"/> enables
/// the Cancel button and that selecting it cancels the same source.
/// </summary>
[TestMethod]
public void CancelSource_WhenAssigned_EnablesButtonAndCancelsSameSourceOnClick()
{
var context = new SynchronizationContext();
SynchronizationContext previousContext = SynchronizationContext.Current;
SynchronizationContext.SetSynchronizationContext(context);

try
{
ProgressViewer viewer = new ProgressViewer();

try
{
using var cancelSource = new CancellationTokenSource();
CancellationToken cancellationToken = cancelSource.Token;
FieldInfo cancelButtonField =
typeof(ProgressViewer).GetField(
"ButtonCancel",
BindingFlags.NonPublic | BindingFlags.Instance
)
?? throw new MissingFieldException(nameof(ProgressViewer), "ButtonCancel");
object cancelButtonValue =
cancelButtonField.GetValue(viewer)
?? throw new InvalidOperationException(
"ProgressViewer.ButtonCancel resolved to null."
);
var cancelButton =
cancelButtonValue as System.Windows.Forms.Button
?? throw new InvalidCastException(
"ProgressViewer.ButtonCancel is not a Windows Forms Button."
);

viewer.CancelSource = cancelSource;
viewer.Show();

cancelButton
.Enabled.Should()
.BeTrue("assigning CancelSource must enable cancellation while loading");

cancelButton.PerformClick();

cancellationToken
.IsCancellationRequested.Should()
.BeTrue(
"the enabled button must cancel the source assigned to CancelSource"
);
}
finally
{
if (!viewer.IsDisposed)
{
viewer.Dispose();
}
}
}
finally
{
SynchronizationContext.SetSynchronizationContext(previousContext);
}
}

// ---------------------------------------------------------------------------
// P30-T1: Cancel path transitions the CancellationToken to cancelled
// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -249,20 +314,39 @@ public void UiThreadNumber_SetterAndGetter_RoundTripAssignedValue()
[TestMethod]
public void CancelSource_SetterAndGetter_RoundTripAssignedValue()
{
// Arrange
var viewer = CreateHeadlessViewer();
using var cts = new CancellationTokenSource();
var context = new SynchronizationContext();
SynchronizationContext previousContext = SynchronizationContext.Current;
SynchronizationContext.SetSynchronizationContext(context);

try
{
ProgressViewer viewer = new ProgressViewer();

// Act: use the setter directly (not SetCancellationTokenSource)
viewer.CancelSource = cts;
try
{
using var cts = new CancellationTokenSource();

// Assert: getter must return the same instance
viewer
.CancelSource.Should()
.BeSameAs(
cts,
"the setter must store and the getter must return the assigned CancellationTokenSource"
);
viewer.CancelSource = cts;

viewer
.CancelSource.Should()
.BeSameAs(
cts,
"the setter must store and the getter must return the assigned CancellationTokenSource"
);
}
finally
{
if (!viewer.IsDisposed)
{
viewer.Dispose();
}
}
}
finally
{
SynchronizationContext.SetSynchronizationContext(previousContext);
}
}
}
}
6 changes: 5 additions & 1 deletion UtilitiesCS/Threading/ProgressViewer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,11 @@ public int UiThreadNumber
public CancellationTokenSource CancelSource
{
get => _cancelSource;
set => _cancelSource = value;
set
{
_cancelSource = value;
ButtonCancel.Enabled = value != null;
}
}

public void SetCancellationTokenSource(CancellationTokenSource tokenSource)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
# Code Review: ProgressViewer Cancel Button (#339)

**Review Date:** 2026-07-16
**Reviewer:** feature-review agent
**Feature Folder:** `docs/features/active/2026-07-16-progress-viewer-cancel-button-339`
**Feature Folder Selection Rule:** The supplied active folder matches issue `339` in the branch name and contains the branch scoping document.
**Base Branch:** `bump-release`
**Head Branch:** `bug/progress-viewer-cancel-button-339` at `a22530c11dd9d2f3c94c74531840d889268b8d53`
**Review Type:** Initial review

---

## Executive Summary

The source delta is focused: `ProgressViewer.CancelSource` now derives the Cancel button's enabled state from whether a token source is configured, and the existing test file adds a real-control regression that assigns the source, displays the viewer, selects Cancel, and verifies cancellation on that same source. The reviewed call sites in `ProgressTracker.Initialize()` and `ProgressTrackerAsync.InitializeAsync()` assign `CancelSource` in the UI-thread object initializer before showing the viewer, so the setter change applies to the reported loading state.

The implementation is suitable for normal PR flow after one evidence-only cleanup. Existing C# QA evidence is complete and passing, but `git diff --check bump-release...HEAD` reports six trailing-whitespace lines in a committed diagnostic TRX.

**What changed:**

- `UtilitiesCS/Threading/ProgressViewer.cs`: replaced the expression-bodied setter with assignment plus `ButtonCancel.Enabled = value != null`.
- `UtilitiesCS.Test/Threading/ProgressViewer_Tests.cs`: added a regression using a constructed form and updated an existing headless setter test so it uses a constructed form now that the setter intentionally touches the control.

**Top 3 risks:**

1. The branch currently fails the full-diff whitespace check because of six lines in a diagnostic TRX.
2. The regression accesses a private control through reflection; a designer field rename will require a test update, although the explicit exception provides a direct diagnostic.
3. No end-to-end tracker-specific UI automation was added; correctness for both tracker variants is established by their shared property-assignment call path plus the real-control unit regression.

**PR readiness recommendation:** **Needs Revision** — normalize the six TRX whitespace findings, verify the XML, and repeat the review check; no source-code revision is required.

---

## Findings Table

| Severity | File | Location | Finding | Recommendation | Rationale | Evidence |
|---|---|---|---|---|---|---|
| Minor | `docs/features/active/2026-07-16-progress-viewer-cancel-button-339/evidence/baseline/coverage-timeout-pair.2026-07-16T14-37.trx` | Lines 981, 3844, 3904, 4014, 4029, 5897 | Six generated-text lines contain trailing whitespace, causing the full branch whitespace check to exit 2. | Remove only the trailing whitespace, preserve XML semantics and evidence content, parse the XML, then rerun `git diff --check bump-release...HEAD`. | The branch should pass deterministic diff-integrity checks before PR readiness. | Reviewer command `git diff --check bump-release...HEAD`. |
| Info | `UtilitiesCS/Threading/ProgressViewer.cs` | Lines 53-60 | The targeted setter change enables Cancel for non-null sources and disables it for null sources without changing the public API. | Retain the implementation. | It corrects the exact caller path used by both progress tracker variants with minimal scope. | Source diff and call-site inspection of `ProgressTracker.cs:36-40` and `ProgressTrackerAsync.cs:36-40`. |
| Info | `UtilitiesCS.Test/Threading/ProgressViewer_Tests.cs` | Lines 35-99 | The regression verifies the real button state and same-source cancellation and restores all test-owned state. | Retain the test. | It reproduces the disabled-control defect and verifies token propagation without external dependencies. | Fail-before and pass-after evidence; final 5,468/5,468 test result. |

No Blocker or Major findings were identified.

---

## Implementation Audit

### C# implementation audit

#### What changed well

- The implementation changes the existing configuration boundary instead of adding a second button-state path.
- The setter completes before `ShowProgressViewer(...)` in both reported tracker initializers, so the button is enabled during initial loading.
- Assigning null disables the button, preventing user selection when no cancellation target exists.
- No unrelated production file, public member, dependency, or exception behavior changed.

#### Type safety and API notes

- Analyzer and nullable evidence record 0 warnings and 0 errors in the final ordered pass.
- The existing legacy nullable signature is preserved; the explicit `value != null` guard provides safe control state for null assignment.
- No new public API surface was added.

#### Error handling and logging

- The change introduces no I/O, logging, exception interception, or asynchronous lifecycle.
- The existing click handler cancels the configured source and closes the form. The enabled-state invariant prevents normal UI selection when the source is null.
- The regression disposes the viewer only when needed and always restores the prior synchronization context.

---

## Test Quality Audit

The regression is deterministic and directly covers both outcomes requested by issue `#339`: enabled Cancel state after assignment and cancellation on the same source after selection. It uses the repository's MSTest and FluentAssertions conventions, operates on the real WinForms control, and creates no file, network, clock, or process dependency.

### Reviewed test and QA artifacts

- `evidence/regression-testing/fail-before-339.2026-07-16T12-39.md` — one test failed at the intended enabled-state assertion before the production fix.
- `evidence/regression-testing/pass-after-339.2026-07-16T12-39.md` — the same test passed after the setter fix and verified same-source cancellation.
- `evidence/qa-gates/csharpier-final.2026-07-16T12-39.md` — authoritative final formatting attempt changed zero tracked C# files.
- `evidence/qa-gates/analyzer-final.2026-07-16T12-39.md` — final analyzer build reported 0 warnings and 0 errors.
- `evidence/qa-gates/nullable-final.2026-07-16T12-39.md` — final nullable build reported 0 warnings and 0 errors.
- `evidence/qa-gates/vstest-coverage-final.2026-07-16T12-39.md` — eight isolated runs reported 5,468 passed, 0 failed, and 0 skipped.
- `evidence/qa-gates/coverage-delta-339.2026-07-16T12-39.md` — repository coverage increased from 83.44% to 83.46%; target and changed-line coverage are 100%.

### Quality assessment prompts

- **Determinism:** The test uses fixed in-process objects, a controlled synchronization context, and no timing or external resources.
- **Isolation:** The test owns and restores all mutable context and disposes the form and token source.
- **Speed:** The focused test completed in 237 ms; the focused pass-after run completed in 1.4748 seconds.
- **Diagnostics:** Assertion reasons distinguish the enabled-state requirement from same-source cancellation; reflection failures name the expected private field and control type.

---

## Security / Correctness Checks

| Check | Status | Evidence |
|---|---|---|
| No secrets in code | PASS | The complete two-file source diff contains only UI state, cancellation, reflection-based test access, and resource management. |
| No unsafe subprocess or command construction | N/A | No process invocation or command construction was added. |
| Input validation at boundaries | PASS | Null source assignment disables the control; a non-null source enables it. |
| Error handling remains explicit | PASS | No catch-all handler or swallowed exception was introduced. |
| Configuration / path handling is safe | N/A | No runtime configuration or path handling changed. |
| Same-source cancellation | PASS | The test captures the assigned source's token and verifies it becomes cancellation-requested after `PerformClick()`. |

---

## Research Log

No external research was required. The repository source, canonical PR-context bundle, requirements, and QA evidence fully defined the review scope.

---

## Verdict

The implementation and tests are correct, focused, and supported by passing C# toolchain and coverage evidence. The acceptance behavior is ready.

The branch requires one bounded evidence-only remediation before PR flow: remove the six trailing-whitespace instances in the diagnostic TRX, verify the XML remains parseable, and rerun the full-diff whitespace check. No production or test-code change is recommended.
Loading
Loading