fix(quickfiler): close five QfcItemController lifecycle and error-handling defects

[drmoisan]

fix(quickfiler): close five QfcItemController lifecycle and error-handling defects

Summary

• Repairs five defects in the QfcItemController partial classes: a double toggle in ToggleNavigation, a missing event unwiring path, swallowed failures and unobserved cancellation in MoveMailAsync, an undisposed read timer with stale collaborators in Cleanup(), and unguarded inputs in the WebResourceRequested handler.
• Adds a real teardown contract: UnwireEvents() now detaches all 22 subscriptions that the wiring methods attach (16 intent, 6 control-tree) plus the WebResourceRequested delegate, closing an event-handler leak that kept controllers alive after pooling.
• MoveMailAsync now propagates failures instead of swallowing them, preserves InnerException, and routes the user-facing notification through an injectable seam so the path is testable headlessly.
• Adds 21 regression tests (938 to 959 total, all passing) and raises repository line coverage from 84.775% to 84.8323%.
• No public API member is added or removed on any of the four production partials, and the QuickFiler/Interfaces files are untouched.

Why

QfcItemController is pooled and reused. Three of these defects only surface on the second and later use of a recycled controller, which is why they escaped earlier testing:

• Bug: qfc-item-controller-no-event-unwiring-path #481 — WireIntentEvents and WireControlTreeEvents had no inverse. Every Cleanup() left 22 live subscriptions attached, so a pooled controller accumulated handlers and kept its previous ItemViewer reachable.
• Bug: qfc-item-controller-cleanup-timer-and-stale-field-defects #484 — Cleanup() nulled _emailIsReadTimer without disposing it, so an armed timer could fire against a torn-down controller. It also left _mailActions bound to a released collaborator.
• Bug: qfc-item-controller-togglenavigation-double-toggle #480 — ToggleNavigation applied an unconditional toggle in addition to the branch-specific one, so the asynchronous path toggled twice and landed back on its original state.
• Bug: qfc-item-controller-mailactions-error-handling-defects #483 — the catch block logged and returned, so a failed move reported success to the caller and the mail item silently stayed put.
• Bug: qfc-item-controller-webview-handler-unguarded-inputs #485 — the WebResourceRequested lambda parsed its URI and indexed the attachment map without guarding either, so a malformed CID URI threw inside an event handler.

What Changed

Core fixes (4 production partials, +219/-16)

• QfcItemController.EventWiring.cs — adds UnwireEvents(), UnwireIntentEvents(), UnwireControlTreeEvents(), and DetachWebResourceRequestedHandler(), with early-return guards for a null viewer or keyboard handler.
• QfcItemController.ViewerSetup.cs — Cleanup() disposes the read timer before nulling it, unwires events before releasing collaborators, and nulls _mailActions; TryResolveCidResource is extracted as an internal static seam with Uri.TryCreate and map-lookup guards.
• QfcItemController.MailActions.cs — the swallowing catch now rethrows with the inner exception preserved and notifies through the MoveFailureNotifier seam; Token.ThrowIfCancellationRequested() is the first statement of the three async mail actions, outside the try, so cancellation cannot be re-wrapped as a move failure.
• QfcItemController.FocusAndTheme.cs — removes the unconditional toggle; ApplyReadEmailFormat returns early once torn down.

Tests (5 files, +591/-2) — 21 new MSTest cases using Moq and FluentAssertions, covering wire/unwire symmetry by count, teardown statement order, cancellation, error propagation, and CID guard cases.

Documentation — the atomic plan, 50 acceptance criteria, 68 evidence artifacts, and the three review audits.

Architecture / How It Fits Together

Cleanup() is the single teardown entry point and now runs in a fixed order: unwire events, then dispose the timer, then null it, then release collaborators. The order is load-bearing — unwiring after releasing _itemViewer would lose the handler targets, and nulling the timer before disposing it would orphan an armed callback. Two seams were introduced purely to make host-bound paths testable without a live Outlook or WebView2 runtime: MoveFailureNotifier (defaulting to the modal dialog) and the internal static TryResolveCidResource.

Verification

Completed — full CLAUDE.md toolchain, one consecutive clean pass, zero restarts:

Stage	Result
dotnet tool run csharpier format (9 owned files)	exit 0, 0 rewritten
dotnet tool run csharpier check .	exit 0, 1520 checked, 0 unformatted
msbuild TaskMaster.sln /t:Rebuild /m /p:Configuration=Debug "/p:Platform=Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true	exit 0, 0 errors
msbuild TaskMaster.sln /t:Rebuild /m /p:Configuration=Debug "/p:Platform=Any CPU" /p:TreatWarningsAsErrors=true	exit 0, 0 errors
vstest.console.exe QuickFiler.Test.dll /EnableCodeCoverage /InIsolation	exit 0, 959/959 passed

The 5 remaining build warnings are pre-existing System.Reactive packages.config notices, unrelated to this change.

Coverage: repository line rate 84.775% to 84.8323%; changed-line coverage 126/132 added executable lines (95.5%), the 6 uncovered lines being the two authorized WebView2-runtime carve-outs; all five new production members at 100%.

Recommended — re-run the four toolchain commands above.

Backward Compatibility / Migration Notes

No breaking change. Public surface is unchanged; the new members are internal. Behavioral changes callers should know about:

• MoveMailAsync now throws where it previously returned normally on failure. This is the intended fix, but any caller relying on silent failure will now observe an exception.
• ToggleNavigation(async: true) now toggles once rather than twice, so its net effect changes for that path.

Risks and Mitigations

• Teardown order regression — order is pinned by explicit statement-order tests and a dedicated evidence artifact.
• Missed detach — wire and unwire sets are asserted by count (16/16 and 6/6) rather than by inspection.
• Rollback — the change is confined to four QuickFiler/Controllers partials and their tests; reverting the commits restores prior behavior with no schema or config migration.

Review Guide

1. QfcItemController.ViewerSetup.cs — Cleanup() statement order (highest risk).
2. QfcItemController.EventWiring.cs — wire/unwire symmetry.
3. QfcItemController.MailActions.cs — cancellation placement and rethrow.
4. Tests, then evidence and audits (mechanical, high volume).

The 68 evidence files and 3 audits account for most of the line count and can be skimmed.

Follow-ups

Two non-blocking residuals identified in review, recommended for promotion through the potential-to-issue lifecycle:

• A residual TOCTOU window in ApplyReadEmailFormat: the guard re-reads fields after checking them, and parameterless Timer.Dispose() does not wait for an in-flight callback. A local capture would close it.
• A silent skip in MoveMailAsync when OneDrive is absent.

Also surfaced for maintainer resolution: the repository publishes two different line-coverage floors — 80% in CLAUDE.md and 85% in .claude/rules/general-unit-test.md. This branch measures 84.8323%, which clears the first and not the second. The shortfall is pre-existing and repository-wide, and this branch improves it.

GitHub Auto-close

None. The base of this pull request is the epic integration branch rather than the default branch, so no auto-close keyword is emitted here. Issue references in scope: #484, #480, #481, #483, #485.