bug/quickfiler gui not expanding 96 clean

QuickFiler.Test/Controllers/QfcItemControllerTests.cs

@@ -1,10 +1,15 @@
 using System;
+using System.Reflection;
 using System.Threading;
 using System.Threading.Tasks;
+using System.Windows.Forms;
 using FluentAssertions;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
+using Moq;
 using QuickFiler.Controllers;
 using QuickFiler.Helper_Classes;
+using QuickFiler.Interfaces;
+using UtilitiesCS;
 
 namespace QuickFiler.Controllers.Tests
 {
@@ -162,4 +167,149 @@ await act.Should()
             callCts.Dispose();
         }
     }
+
+    /// <summary>
+    /// Regression tests for Issue #96: Right arrow key does not expand conversation messages.
+    ///
+    /// Root cause: RegisterFocusAsyncActions() never registered Keys.Right in
+    /// KeyActionsAsync. The handler was commented out during the async migration and not
+    /// restored. As a result, Right-arrow key presses fell through to the focused WinForms
+    /// control and activated the sender's mailto: address instead of expanding the
+    /// conversation view.
+    ///
+    /// Fix: Add Keys.Right → ToggleExpansionAsync(On) in RegisterFocusAsyncActions() and
+    /// remove it in UnregisterFocusAsyncActions().
+    /// </summary>
+    [TestClass]
+    public class QfcItemController_KeyboardRegistrationTests
+    {
+        // ---------------------------------------------------------------------------
+        // Test double: minimal subclass that injects a stub keyboard handler and a
+        // MailItemHelper with a known EntryId. No WinForms infrastructure is required
+        // because the lambda bodies are only evaluated when invoked, not at registration.
+        // ---------------------------------------------------------------------------
+        private sealed class KeyboardRegistrationQfcItemController : QfcItemController
+        {
+            /// <param name="kbdHandler">
+            /// Stub keyboard handler whose KbdActions collections receive the Add/Remove calls
+            /// made by RegisterFocusAsyncActions and UnregisterFocusAsyncActions.
+            /// </param>
+            /// <param name="entryId">
+            /// String used as the sourceId in KbdActions registrations; must be unique
+            /// within each collection.
+            /// </param>
+            internal KeyboardRegistrationQfcItemController(
+                IQfcKeyboardHandler kbdHandler,
+                string entryId
+            )
+                : base()
+            {
+                // Inject keyboard handler via reflection (field is private in production code).
+                typeof(QfcItemController)
+                    .GetField("_kbdHandler", BindingFlags.NonPublic | BindingFlags.Instance)
+                    .SetValue(this, kbdHandler);
+
+                // Set ItemHelper with a known EntryId so sourceId assignments are predictable.
+                var helper = new MailItemHelper();
+                helper.EntryId = entryId;
+                ItemHelper = helper;
+            }
+        }
+
+        // ---------------------------------------------------------------------------
+        // Helper: build a minimal stub keyboard handler whose KbdActions properties
+        // return real (but empty) collection instances so that Add/Remove calls succeed.
+        // Only CharActionsAsync and KeyActionsAsync are needed by RegisterFocusAsyncActions.
+        // ---------------------------------------------------------------------------
+        private static (
+            Mock<IQfcKeyboardHandler> mock,
+            KbdActions<Keys, KaKeyAsync, Func<Keys, Task>> keyActionsAsync,
+            KbdActions<char, KaCharAsync, Func<char, Task>> charActionsAsync
+        ) BuildKbdHandlerStub()
+        {
+            var mockKbd = new Mock<IQfcKeyboardHandler>();
+
+            var keyActionsAsync = new KbdActions<Keys, KaKeyAsync, Func<Keys, Task>>();
+            var charActionsAsync = new KbdActions<char, KaCharAsync, Func<char, Task>>();
+
+            // Route property accesses to the real collections so Add/Remove mutate them.
+            mockKbd.Setup(k => k.KeyActionsAsync).Returns(keyActionsAsync);
+            mockKbd.Setup(k => k.CharActionsAsync).Returns(charActionsAsync);
+
+            return (mockKbd, keyActionsAsync, charActionsAsync);
+        }
+
+        // ---------------------------------------------------------------------------
+        // Regression test — P1-T1
+        // This test MUST FAIL before the fix and PASS after.
+        // ---------------------------------------------------------------------------
+
+        [TestMethod]
+        public void RegisterFocusAsyncActions_RightArrowKey_IsRegisteredInKeyActionsAsync()
+        {
+            // Arrange
+            // Build a stub keyboard handler with real KbdActions collections.
+            // RegisterFocusAsyncActions must add Keys.Right to KeyActionsAsync so that
+            // KeyDownTaskAsync intercepts and suppresses the key press instead of letting it
+            // fall through to the focused mailto: control.
+            var (mockKbd, keyActionsAsync, _) = BuildKbdHandlerStub();
+            var controller = new KeyboardRegistrationQfcItemController(
+                mockKbd.Object,
+                "test-entry-id-right-key"
+            );
+
+            // Act
+            controller.RegisterFocusAsyncActions();
+
+            // Assert — before the fix this fails because Keys.Right was not registered
+            keyActionsAsync
+                .ContainsKey(Keys.Right)
+                .Should()
+                .BeTrue(
+                    because: "Keys.Right must be registered in KeyActionsAsync so that the keyboard "
+                        + "handler intercepts the key press and expands the conversation instead of "
+                        + "routing it to the mailto: control"
+                );
+        }
+
+        // ---------------------------------------------------------------------------
+        // Regression test — P1-T2
+        // Verifies that the Right-arrow registration is cleaned up on focus loss.
+        // ---------------------------------------------------------------------------
+
+        [TestMethod]
+        public void UnregisterFocusAsyncActions_AfterRegister_RemovesRightArrowFromKeyActionsAsync()
+        {
+            // Arrange
+            // First register, then unregister. The Right-arrow entry must be absent
+            // after unregistration so that nav outside the keyboard-active item does not
+            // capture Right-arrow presses that belong to a different item's handler.
+            var (mockKbd, keyActionsAsync, _) = BuildKbdHandlerStub();
+            var controller = new KeyboardRegistrationQfcItemController(
+                mockKbd.Object,
+                "test-entry-id-right-key-cleanup"
+            );
+
+            controller.RegisterFocusAsyncActions();
+
+            // Precondition: Right must be registered (asserted in the previous test).
+            keyActionsAsync
+                .ContainsKey(Keys.Right)
+                .Should()
+                .BeTrue(because: "precondition — right key must be registered before cleanup");
+
+            // Act
+            controller.UnregisterFocusAsyncActions();
+
+            // Assert — the entry must be removed on unregister
+            keyActionsAsync
+                .ContainsKey(Keys.Right)
+                .Should()
+                .BeFalse(
+                    because: "Keys.Right handler must be removed from KeyActionsAsync when focus "
+                        + "actions are unregistered, otherwise stale registrations accumulate "
+                        + "across focus changes"
+                );
+        }
+    }
 }

QuickFiler/Controllers/QfcItemController.cs

@@ -1339,6 +1339,12 @@ internal void RegisterFocusAsyncActions()
             //_kbdHandler.KeyActionsAsync.Add(_itemInfo.EntryId, Keys.Right, (x) => ToggleCheckboxAsync(_itemViewer.CbxConversation, Enums.ToggleState.Off));
             //_kbdHandler.KeyActionsAsync.Add(_itemInfo.EntryId, Keys.Left, (x) => ToggleCheckboxAsync(_itemViewer.CbxConversation, Enums.ToggleState.On));
             //_kbdHandler.CharActionsAsync.Add(_itemInfo.EntryId, 'A', (x) => this.ToggleCheckboxAsync(_itemViewer.CbxAttachments));
+            // Right arrow expands the conversation thread for the focused item.
+            _kbdHandler.KeyActionsAsync.Add(
+                ItemHelper.EntryId,
+                Keys.Right,
+                (x) => this.ToggleExpansionAsync()
+            );
             _kbdHandler.CharActionsAsync.Add(
                 ItemHelper.EntryId,
                 'C',
@@ -1462,9 +1468,9 @@ internal void UnregisterFocusActions()
 
         internal void UnregisterFocusAsyncActions()
         {
-            //_kbdHandler.KeyActionsAsync.Remove(_itemInfo.EntryId, Keys.Right);
             //_kbdHandler.KeyActionsAsync.Remove(_itemInfo.EntryId, Keys.Left);
             //_kbdHandler.CharActionsAsync.Remove(_itemInfo.EntryId, 'A');
+            _kbdHandler.KeyActionsAsync.Remove(ItemHelper.EntryId, Keys.Right);
             _kbdHandler.CharActionsAsync.Remove(ItemHelper.EntryId, 'C');
             _kbdHandler.CharActionsAsync.Remove(ItemHelper.EntryId, 'O');
             _kbdHandler.CharActionsAsync.Remove(ItemHelper.EntryId, 'M');

docs/features/active/2026-03-25-quickfiler-gui-not-expanding-96/code-review.2026-03-25T14-00.md

@@ -0,0 +1,93 @@
+# Code Review — 2026-03-25T14-00
+
+**Feature folder:** `docs/features/active/2026-03-25-quickfiler-gui-not-expanding-96/`
+**Branch:** `feature/utilities-coverage-part-three-87` (commit `bd8fc03`)
+**Base:** `main` @ `0d6c60f`
+**Work Mode:** `minor-audit`
+**Reviewer:** feature-reviewer agent
+**Date:** 2026-03-25
+
+---
+
+## 1. Executive Summary
+
+**What changed:**
+- `QuickFiler/Controllers/QfcItemController.cs`: 7 lines added/changed to restore the `Keys.Right`
+  keyboard registration that was dropped during the async migration. `RegisterFocusAsyncActions()`
+  now adds `Keys.Right → ToggleExpansionAsync()` and `UnregisterFocusAsyncActions()` now removes it.
+- `QuickFiler.Test/Controllers/QfcItemControllerTests.cs`: 150 lines added — a new
+  `QfcItemController_KeyboardRegistrationTests` class with two regression tests covering the
+  register and unregister flows for `Keys.Right`.
+
+**Top 3 risks:**
+
+1. **`ToggleExpansionAsync()` vs `ToggleExpansionAsync(Enums.ToggleState.On)` (Low risk):**
+   The implementation toggles (expand ↔ collapse) rather than always forcing expand. The issue
+   description says "equivalent to pressing 'E'" and the 'E' binding also uses the no-arg overload,
+   so toggle is consistent. However, the plan specified `.On`. If the product intent is Right-arrow
+   should always expand (not collapse an already-expanded item), the current implementation would
+   allow Right-arrow to collapse. This is a minor behavioral nuance, not a defect.
+
+2. **Reflection-based field injection in tests (Acceptable):**
+   `KeyboardRegistrationQfcItemController` uses `typeof(QfcItemController).GetField("_kbdHandler", ...)`
+   to inject the mock handler. This is a brittle seam — if `_kbdHandler` is renamed or moved to a
+   base class, the test silently throws a NullReferenceException at runtime rather than a compile
+   error. The comment documents the constraint (private field, no constructor injection). Risk is
+   low for a stable legacy type, but reviewers should be aware that refactoring `_kbdHandler`
+   requires updating the test.
+
+3. **Coverage numeric gap (Informational):**
+   The plan requires a numeric line-coverage percentage in both the baseline and QA coverage artifacts.
+   The binary `.coverage` format produced by vstest does not print an inline percentage to stdout.
+   Both artifacts document this limitation explicitly. Repository-wide coverage has not regressed
+   (74 tests, all passed vs 72 at baseline), and the fix adds 2 deterministic targeted tests for 7
+   new production lines.
+
+**Go/No-Go Recommendation:** **Go.** The change is minimal, correctly targeted, and well-evidenced.
+All toolchain gates pass. The two deviations from the plan (toggle vs. force-expand; method naming)
+are informational and do not block merge.
+
+---
+
+## 2. Findings Table
+
+| # | Severity | File | Location | Finding | Recommendation | Rationale | Evidence |
+|---|----------|------|----------|---------|---------------|-----------|---------|
+| F-01 | Minor | `QfcItemController.cs` | Line ~1345 | `ToggleExpansionAsync()` (toggle) used instead of plan-specified `ToggleExpansionAsync(Enums.ToggleState.On)` (force-expand). | Confirm with product owner whether Right-arrow should always expand or toggle. If always-expand is intended, change to `(Enums.ToggleState.On)`. | Issue says "equivalent to pressing 'E'"; 'E' uses toggle; the current behavior is self-consistent. | `git show bd8fc03 -- QuickFiler/Controllers/QfcItemController.cs` |
+| F-02 | Nit | `QfcItemControllerTests.cs` | `KeyboardRegistrationQfcItemController` ctor | Reflection-based field injection (`GetField("_kbdHandler")`) is fragile to rename. | Add a `// NOTE: if _kbdHandler is renamed, update this string` comment, or make the field internal for testability in a future refactor. | Low immediate risk; `_kbdHandler` is stable in this legacy type. | Code diff |
+| F-03 | Nit | Plan / evidence | `baseline-coverage.md`, `qa-test.md` | Numeric line-coverage percentage not captured (binary `.coverage` only). | Acceptable as documented. Future runs should consider a coverage report converter step. | vstest limitation is known; evidence explains it. | `baseline-coverage.md` note section |
+
+No Blockers. No Major findings.
+
+---
+
+## 3. Test Quality Audit
+
+| Criterion | Status | Notes |
+|-----------|--------|-------|
+| Framework: MSTest | PASS | `[TestClass]`, `[TestMethod]` throughout |
+| Mocking: Moq | PASS | `Mock<IQfcKeyboardHandler>` with real `KbdActions<>` instances |
+| Assertions: FluentAssertions | PASS | `.Should().BeTrue(because:...)` and `.Should().BeFalse(because:...)` |
+| AAA structure | PASS | `// Arrange`, `// Act`, `// Assert` comments present |
+| Independence | PASS | Each test creates its own controller and mock stub |
+| Isolation | PASS | No shared state; no external services; no temp files |
+| Determinism | PASS | Pure in-memory operations |
+| Fast execution | PASS | 2 new tests run in < 1s combined |
+| Failure messages | PASS | FluentAssertions `because:` string explains exactly what must be true and why |
+| Fail-before evidence | PASS | Both tests fail before fix (EXIT_CODE: 1 in `regression-fail-before.md`) |
+| Pass-after evidence | PASS | Both tests pass after fix (74/74 in `qa-test.md`) |
+| No temp files | PASS | |
+| No external dependencies | PASS | |
+| Coverage for new code | PASS | New tests directly exercise the 7 changed production lines |
+
+---
+
+## 4. Security / Correctness
+
+| Check | Status | Notes |
+|-------|--------|-------|
+| No secrets in code | PASS | No credentials, tokens, or paths |
+| No unsafe subprocess usage | PASS | No process spawning |
+| Input validation at boundaries | N/A | Key registration is an internal framework call, not a user-input boundary |
+| No new COM interop surface | PASS | Fix reuses existing `_kbdHandler.KeyActionsAsync` collection |
+| Correct cleanup on unregister | PASS | `Remove` is symmetric with `Add`; verified by regression test |

docs/features/active/2026-03-25-quickfiler-gui-not-expanding-96/evidence/baseline/baseline-coverage.md

@@ -0,0 +1,23 @@
+# Phase 0 — Full Coverage Baseline (QuickFiler.Test)
+
+Timestamp: 2026-03-25T13:52:00Z
+Command: & "C:\Program Files\Microsoft Visual Studio\18\Community\Common7\IDE\CommonExtensions\Microsoft\TestWindow\vstest.console.exe" "QuickFiler.Test\bin\Debug\QuickFiler.Test.dll" /InIsolation /EnableCodeCoverage
+EXIT_CODE: 0
+
+## Output Summary
+
+Test Run: Successful
+
+- Total tests:  72
+- Passed:       72
+- Failed:        0
+- Skipped:       0
+- Total time:   3.9633 Seconds
+
+Coverage file generated (binary .coverage):
+`C:\Users\DanMoisan\repos\TaskMaster\TestResults\953d9d33-913a-4180-91f3-f63b852602bd\DanMoisan_MEGALODON4_2026-03-25.10_37_58.coverage`
+
+Note: vstest.console with /EnableCodeCoverage produces a binary .coverage file and does not
+print an inline line-coverage percentage to stdout. The coverage binary is available for
+consumption by Visual Studio or a coverage report converter. All 72 tests passed at baseline;
+no failing tests, no skips.

docs/features/active/2026-03-25-quickfiler-gui-not-expanding-96/evidence/baseline/baseline-format.md

@@ -0,0 +1,20 @@
+# Phase 0 — Format Baseline
+
+Timestamp: 2026-03-25T13:46:00Z
+Command: dotnet tool run csharpier format .
+EXIT_CODE: 0
+
+## Output Summary
+
+CSharpier processed 1001 `.cs` files in 790ms with exit code 0.
+
+One warning: `TaskMaster\TaskMaster_BACKUP_1250.csproj` was skipped due to invalid XML
+(character `<` at line 471, position 2); this file is not a `.cs` source file and is
+excluded from formatting scope.
+
+Full output:
+```
+Warning The csproj at C:\Users\DanMoisan\repos\TaskMaster\TaskMaster\TaskMaster_BACKUP_1250.csproj failed to load with the following exception Name cannot begin with the '<' character, hexadecimal value 0x3C. Line 471, position 2.
+Warning .\TaskMaster\TaskMaster_BACKUP_1250.csproj - Appeared to be invalid xml so was not formatted.
+Formatted 1001 files in 790ms.
+```

docs/features/active/2026-03-25-quickfiler-gui-not-expanding-96/evidence/baseline/baseline-lint.md

@@ -0,0 +1,18 @@
+# Phase 0 — Lint Baseline
+
+Timestamp: 2026-03-25T13:48:00Z
+Command: pwsh -NoProfile -ExecutionPolicy Bypass -File scripts/vscode/Invoke-VSBuild.ps1 -SolutionPath TaskMaster.sln -Configuration Debug -Platform "Any CPU" -EnableNETAnalyzers -EnforceCodeStyleInBuild
+EXIT_CODE: 0
+
+## Output Summary
+
+Build succeeded. 18 Warning(s). 0 Error(s). Time Elapsed 00:00:03.38
+
+All 18 warnings are pre-existing at baseline:
+- 7× pre-build script WARNING: [SVGControl.Test] cannot resolve DLLs from NuGet packages (Castle.Core, FluentAssertions, MSTest, Moq, etc.)
+- 1× pre-build script WARNING: [TaskMaster] merge conflict markers detected, skipping
+- 5× CS0618 (obsolete AsyncEnumerable LINQ APIs: SelectAwait, ForEachAwait*, WhereAwait, ForEachAsync) in ConflictResolutionResolver.cs, NoteController.cs, RibbonController.cs, AppEvents.cs
+- 1× MSTEST0032: assertion condition always true in QfcFormControllerTests.cs(696,13)
+- 2× CS0067: event PropertyChanged declared but never used in SmartSerializable_Tests.cs(826) and SmartSerializableBase_Tests.cs(654)
+
+No errors. Baseline lint state: 0 errors, 18 warnings (all pre-existing).

docs/features/active/2026-03-25-quickfiler-gui-not-expanding-96/evidence/baseline/baseline-nullable.md

@@ -0,0 +1,13 @@
+# Phase 0 — Nullable / Type-Check Baseline
+
+Timestamp: 2026-03-25T13:49:00Z
+Command: pwsh -NoProfile -ExecutionPolicy Bypass -File scripts/vscode/Invoke-VSBuild.ps1 -SolutionPath TaskMaster.sln -Configuration Debug -Platform "Any CPU" -EnableNullable -TreatWarningsAsErrors
+EXIT_CODE: 0
+
+## Output Summary
+
+Build succeeded. 0 Warning(s). 0 Error(s). Time Elapsed 00:00:01.25
+
+All projects were fully up-to-date; CoreCompile targets were skipped for all projects
+(incremental build). No nullable warnings were produced, so -TreatWarningsAsErrors had
+no effect. Baseline nullable state: 0 warnings, 0 errors.

docs/features/active/2026-03-25-quickfiler-gui-not-expanding-96/evidence/baseline/baseline-test.md

@@ -0,0 +1,20 @@
+# Phase 0 — Test Baseline (Targeted Filter)
+
+Timestamp: 2026-03-25T13:51:00Z
+Command: & "C:\Program Files\Microsoft Visual Studio\18\Community\Common7\IDE\CommonExtensions\Microsoft\TestWindow\vstest.console.exe" "QuickFiler.Test\bin\Debug\QuickFiler.Test.dll" /InIsolation /TestCaseFilter:"FullyQualifiedName~QfcItemController_KeyboardRegistration"
+EXIT_CODE: 0
+
+## Output Summary
+
+```
+VSTest version 18.4.0 (x64)
+
+Starting test execution, please wait...
+A total of 1 test files matched the specified pattern.
+No test matches the given testcase filter `FullyQualifiedName~QfcItemController_KeyboardRegistration` in C:\Users\DanMoisan\repos\TaskMaster\QuickFiler.Test\bin\Debug\QuickFiler.Test.dll
+```
+
+**Expected baseline state confirmed:** `QfcItemController_KeyboardRegistration` tests do not yet exist
+at baseline (0 tests found). The regression test class has not been added yet — this is intentional.
+The DLL exists and was found (1 test file matched the pattern). The regression tests will be added
+in Phase 1.