Skip to content
Merged
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
# breadcrumb-capturecurrentortests-silently-degrades-in-production (Issue #475)

- Date captured: 2026-08-07
- Author: Dan Moisan
- Status: Promoted -> docs/features/active/breadcrumb-capturecurrentortests-silently-degrades-in-production/ (Issue #475)
- Work Mode: full-bug
- Discovered during: preparation research for issue #455 (epic #136, child F13)

- Issue: #475
- Issue URL: https://github.com/drmoisan/TaskMaster/issues/475
- Last Updated: 2026-08-08
## Summary

`BreadcrumbPopupUiOperations.CaptureCurrentOrTests()` inverts a deliberate fail-fast guard into a
silent degradation. When no `SynchronizationContext` is present it falls back to a **test-mode**
dispatcher whose documented contract is to *report* cross-thread work rather than schedule it. Four
production call sites use this method, so on any thread without a synchronization context the
breadcrumb popup silently never opens: no exception, no user-visible error, only a log line.

## Environment

- OS/version: Windows 11 Pro 10.0.26200
- Runtime: .NET Framework 4.8.1 WinForms VSTO add-in with Microsoft WebView2
- Affected path: QuickFiler breadcrumb folder-selector drop-down construction

## Suspected Cause

`QuickFiler/Viewers/BreadcrumbUiDispatcher.cs:43-54` establishes the intended production contract —
fail fast:

```csharp
internal static BreadcrumbUiDispatcher CaptureCurrent()
{
SynchronizationContext context =
SynchronizationContext.Current
?? throw new InvalidOperationException(
"Breadcrumb UI components must be constructed on an owning UI synchronization context."
);
...
}
```

`QuickFiler/Viewers/BreadcrumbPopupUiOperations.cs:86-89` overrides that contract:

```csharp
internal static BreadcrumbPopupUiOperations CaptureCurrentOrTests() =>
SynchronizationContext.Current == null
? CreateForCurrentThreadTests()
: CaptureCurrent();
```

The fallback target is documented at `BreadcrumbUiDispatcher.cs:58-60` as:

> Creates an owner-thread-only boundary for host-neutral unit tests without a UI pump.
> Cross-thread work is reported instead of being scheduled on a generic context.

So the exact condition the production guard was written to reject — a missing synchronization
context — is the condition that silently selects a dispatcher that does not marshal.

## Production Call Sites (verified 2026-08-07)

```
QuickFiler/Viewers/BreadcrumbDropDownHost.cs:98
QuickFiler/Viewers/BreadcrumbDropDownHost.cs:118
QuickFiler/Viewers/ItemViewer.Breadcrumb.cs:156
QuickFiler/Viewers/ItemViewer.Breadcrumb.cs:192
```

None is test-only. `CreateForCurrentThreadTests()` is named for test use but is reachable from all
four.

## Steps to Reproduce

1. Construct the breadcrumb drop-down host from a thread where `SynchronizationContext.Current` is
null — for example a thread-pool continuation, a background worker, or any path that has lost
the WinForms context.
2. Request the drop-down open.
3. Observe that no popup appears, no exception is raised, and the only trace is a reported failure
through the dispatcher's error sink.

## Expected Behavior

Production construction off the owning UI synchronization context is a programming error and should
fail fast with the `InvalidOperationException` that `CaptureCurrent()` already defines. The
test-mode dispatcher should not be reachable from production call sites.

## Actual Behavior

The construction succeeds, the drop-down is wired to a dispatcher that reports rather than
marshals, and the feature silently does nothing.

## Impact / Severity

- [ ] Blocker
- [x] High
- [ ] Medium
- [ ] Low

Severity is High because the failure mode is silent and user-facing: the folder selector simply
does not open, with no diagnostic surfaced to the user and no exception to correlate in a crash
report. It also violates `CLAUDE.md` § "Error Handling" ("fail fast and explicitly; do not silently
ignore errors") and `.claude/rules/general-code-change.md` § "Error Handling and Logging".

There is a secondary design concern: a test-only affordance is reachable from production code. The
repository's determinism guidance expects test seams to be injected by the test, not selected at
runtime by probing ambient state.

## Suggested Remediation

Preferred: delete `CaptureCurrentOrTests()` and have the four production call sites use
`CaptureCurrent()`, restoring fail-fast. Tests construct `BreadcrumbPopupUiOperations` through its
existing injectable constructor (`BreadcrumbPopupUiOperations.cs:62-78`) or supply a fake
`SynchronizationContext`, both of which are already used by the existing test suite — so no test
loses its seam.

Alternative, if some production path genuinely runs without a context: make that path explicit by
passing the dispatcher in, rather than probing `SynchronizationContext.Current` inside a static
factory.

## Why this is not fixed under epic #136

Epic #136 child F13 (issue #455) carries a hard no-behavior-change NFR. Restoring the throw changes
observable behavior on the affected paths, so it belongs in its own issue.

Note also that `ItemViewer.Breadcrumb.cs` is assigned to child F14, not F13, so two of the four call
sites are outside F13's file assignment. This reinforces that the fix belongs in a standalone issue
rather than inside either child.

## Related

- Issue #455 — F13, breadcrumb drop-down and WebView2 host coverage (where this was found).
- Issue #136 — parent epic.
- Issue #462 — breadcrumb drop-down coordinator stale `_closePending`; a second silent-failure mode
in the same open/close path. Worth scheduling together.

## Next Step

- [ ] Promote to GitHub issue
- [ ] Confirm whether any production path legitimately runs without a synchronization context
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
# breadcrumb-dropdown-coordinator-stale-closepending-drops-reopen (Issue #462)

- Date captured: 2026-08-07
- Author: Dan Moisan
- Status: Promoted -> docs/features/active/breadcrumb-dropdown-coordinator-stale-closepending-drops-reopen/ (Issue #462)
- Work Mode: full-bug
- Discovered during: preparation research for issue #455 (epic #136, child F13)

- Issue: #462
- Issue URL: https://github.com/drmoisan/TaskMaster/issues/462
- Last Updated: 2026-08-08
## Summary

`BreadcrumbDropDownOpenCoordinator.CloseCore` never clears its `_closePending` flag on the
**successful** close path. The flag latches `true` after the first close that actually closes the
host and is never reset. `RequestOpen` consults that stale flag and can silently return a
already-closed sentinel task instead of opening the drop-down, dropping a legitimate reopen
request with no error and no log.

## Environment

- OS/version: Windows 11 Pro 10.0.26200
- Runtime: .NET Framework 4.8.1 WinForms VSTO add-in with Microsoft WebView2
- Affected path: QuickFiler item folder-selector breadcrumb drop-down open/close lifetime

## Suspected Cause

`QuickFiler/Viewers/BreadcrumbDropDownOpenCoordinator.cs:237-267`:

```csharp
private bool CloseCore(BreadcrumbDropDownCloseReason reason)
{
lock (_sync)
{
if (_released)
return false;
if (_closePending)
return true;
_closePending = true; // :245 latched here
}
bool closed;
try
{
closed = _host.Close(reason);
}
catch
{
ClearClosePending(); // :254 cleared on throw
throw;
}
if (closed)
{
lock (_sync)
_generation++;
return true; // :261 returns WITHOUT ClearClosePending()
}
ClearClosePending(); // :263 cleared on the not-closed path
...
}
```

Every exit path clears `_closePending` **except** the successful one at `:257-261`. That is the
inverted case: the successful close is exactly the path after which the coordinator should be
ready to accept a new open.

The stale flag is then read by `RequestOpen` at `:92-93`:

```csharp
if (_closePending && _host.IsOpen)
return ClosedTask;
```

## Steps to Reproduce

1. Open the breadcrumb drop-down so `_host.IsOpen` is true.
2. Close it through a path that reaches `CloseCore` and where `_host.Close(reason)` returns `true`.
`_closePending` is now permanently `true`.
3. Cause the host to become open again through a path that does not route through
`CloseCore`/`RequestOpen` — for example `SetDroppedDown(true)` at `:108-112`, where
`_openSelector()` reports no change and `_isSelectorOpen()` is true.
4. Call `RequestOpen`.
5. Observe that the guard at `:92` is satisfied (`_closePending` stale-true and `_host.IsOpen`
true) and `ClosedTask` is returned. The open request is discarded silently.

## Expected Behavior

`_closePending` describes an in-flight close. Once `_host.Close` has completed successfully the
close is no longer pending, so a subsequent `RequestOpen` should proceed and open the drop-down.

## Actual Behavior

`_closePending` remains `true` for the remaining lifetime of the coordinator, so `RequestOpen` can
short-circuit to `ClosedTask` whenever the host is open.

## Impact / Severity

- [ ] Blocker
- [ ] High
- [x] Medium
- [ ] Low

The user-visible symptom is a folder-selector drop-down that intermittently refuses to reopen until
the viewer is recycled. The failure is silent — no exception, no log line — which makes it
expensive to diagnose from a bug report. Severity is Medium because reaching the state requires the
specific reopen path in step 3 rather than the common open/close cycle.

## Additional Note — this is also the file's coverage gap

The same condition is one of the four uncovered branch outcomes measured in this file
(`BreadcrumbDropDownOpenCoordinator.cs`, 98.25% line / 92.05% branch in the committed Cobertura at
`docs/features/active/2026-08-06-quickfiler-high-confidence-queue-init-stall-424/evidence/qa-gates/coverage-final.cobertura.xml`).
The branch is uncovered precisely because no test asserts the post-close reopen contract. The
coverage gap and the defect are the same finding, which is why a coverage-only change cannot close
it: writing the test that covers the branch would assert the current, incorrect behavior.

## Suggested Remediation

Call `ClearClosePending()` on the successful-close path before returning `true` at `:261`, or
restructure so the flag is cleared in a `finally`. Then add a regression test asserting that
`RequestOpen` opens after a successful `CloseCore`.

Related nearby observations, worth reconciling in the same change:

- `_host.IsOpen` is evaluated while holding `_sync` at `:92`, inconsistent with `CloseCore`'s
deliberate decision to call `_host.Close` **outside** the lock at `:250`. That asymmetry is a
lock-ordering hazard (`Coordinator._sync` -> host lock).
- `Close` is a *claim* rather than a completion: `CloseCore` returns `true` at `:243-244` when a
close is already pending, so callers cannot distinguish "closed" from "someone else is closing".

## Why this is not fixed under epic #136

Epic #136 child F13 (issue #455) carries a hard no-behavior-change NFR. Clearing the flag changes
observable drop-down open/close behavior, so it belongs in its own issue.

## Related

- Issue #455 — F13, breadcrumb drop-down and WebView2 host coverage (where this was found).
- Issue #136 — parent epic.
- Issue #440 — open breadcrumb arrow-key navigation bug in adjacent territory; reconcile scheduling.

## Next Step

- [ ] Promote to GitHub issue
- [ ] Reconcile against F13's plan before scheduling, since F13 adds tests over this file
Loading
Loading