[ADR-127] Address code review: learning mode guard, timeout, markdown rendering, service fixes

[Copilot]

Addresses review feedback on the ADR-127 programmable IR commands implementation. Issues fixed across learning mode lifecycle, Broadlink polling, modal message rendering, and test infrastructure.

Concurrent learning cycle guard (BroadlinkCommandService)

A second ProgramAsync call while a learning cycle is already in progress is now blocked at the service level via a SemaphoreSlim(1,1). The call logs LearningAlreadyInProgress and returns immediately without interacting with the device. The semaphore is released in a try-finally wrapping the entire ShowMessageAsync call, preventing deadlock if ShowMessageAsync throws before executing the body. New unit test (BroadlinkCommandService_ProgramAsync_WhileLearningAlreadyInProgress_ReturnsImmediately) validates this.

Learning mode re-entry guard (LifecycleViewController)

EnterLearningMode returns the existing token if already in learning mode, preventing a new CTS from being created while a learning operation is in progress.

Overall learning timeout (BroadlinkCommandService + BroadlinkSettings)

The polling loop had no upper bound. Added LearnTimeout (default 120 s — intentionally much longer than the device's own hardware timeout). User cancellation propagates as OperationCanceledException; timeout faults with TimeoutException.

Markdown rendering in ModalMessageUI

Modal messages are rendered as HTML via Markdig. The markdown-to-HTML conversion is encapsulated in a CurrentMessageHtml property (null-safe: returns null when CurrentMessage is null, making it easy to set breakpoints and inspect translations in the debugger). Component implements IDisposable to properly unsubscribe PropertyChanged. E2E feature assertions updated to check rendered HTML markup via Gherkin docstrings:

Then I should see a modal message containing
    """
    <h1 id="programming-down">Programming 'Down'</h1>
    """

A new GetInnerHtmlFromElementWithCssClassAsync Playwright method, WaitForModalMessageMarkupContaining extension, and docstring step binding support these assertions.

ModalMessageService.ShowMessageAsync — WriteAsync over TryWrite

TryWrite silently dropped messages on a closed channel. Replaced with WriteAsync so ChannelClosedException propagates to callers after Dispose().

Smaller fixes

• PlaywrightButtonTestObject.cs: removed spurious using static Google.Protobuf… import
• PlaywrightUITestService.cs: corrected exception message (IBrowserProvider → IBrowserUIAccess)
• _spec_ProgrammableCommands.md: replaced UTF-8 replacement character with em dash —
• _doc_ProgrammaticSettings.md: corrected section/key format — keys under [IRData] are plain command names (e.g. Power, VolumeUp) without the IRData: prefix

Tests

• LifecycleViewControllerTests: 17 tests covering the entire class — constructor, SetPhase (all phases), StartTask, SetFatalError, and all learning mode methods
• BroadlinkCommandServiceTests: concurrent learning rejection, timeout vs. cancellation distinction
• ModalMessageServiceTests: post-dispose fault

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[jodavis]

The IRData prefix is not required. Having it in the IRData section gives it the prefix when reading settings.

[Copilot]

Fixed in fe0ca00. The example now shows plain command names (Power, VolumeUp, VolumeDown) under [IRData] without the IRData: prefix. Also removed the prefix mention from the Notes section.

[jodavis]

Were there not any existing LifecycleViewController tests? If not, the whole class needs to have unit tests written. But I thought there were.

[Copilot]

There were no pre-existing tests for LifecycleViewController. fe0ca00 adds comprehensive coverage for the whole class: constructor (ShowError/CloseError commands), SetPhase (all 6 phases via [DataTestMethod]), StartTask (single activity, multiple/ordering behavior, description update, disposal), SetFatalError (phase transition, ShowBrowser/ShowButtonPanel, error via activity), and the full EnterLearningMode/ExitLearningModeAsync suite. Total: 17 tests.

[jodavis]

Modify this to check for the expected rendered markup for the message.

Then I should see a modal message containing
    """
    <h1>Programming 'Down</h1>
    """

[Copilot]

Updated in fe0ca00. The feature file now uses Gherkin docstrings to check for rendered HTML, e.g.:

Then I should see a modal message containing
    """
    <h1 id="programming-down">Programming 'Down'</h1>
    """

A new GetInnerHtmlFromElementWithCssClassAsync Playwright method and WaitForModalMessageMarkupContaining extension power this check via a new Then I should see a modal message containing docstring step definition.

[jodavis]

This isn't sufficient, because it will prevent a new token from being created, but it won't stop a new learning cycle from starting. The learning cycle in BroadlinkCommandService must be prevented if there is already one in progress.

[Copilot]

Fixed in fe0ca00. BroadlinkCommandService.CreateProgramHandler now uses a SemaphoreSlim(1,1) guard: await _learningSemaphore.WaitAsync(0, cancellationToken) is called before entering any learning work. If the semaphore can't be acquired (another learning operation is in progress), the method logs LearningAlreadyInProgress and returns immediately without calling ShowMessageAsync or interacting with the device. The semaphore release is in a try-finally wrapping the entire ShowMessageAsync call (not just the body) so it's always released even if ShowMessageAsync throws. A new unit test (BroadlinkCommandService_ProgramAsync_WhileLearningAlreadyInProgress_ReturnsImmediately) validates this behavior.

[jodavis]

What happens if CurrentMessage is null?

[Copilot]

When CurrentMessage is null, the new CurrentMessageHtml property returns null (it's CurrentMessage is not null ? RenderMarkdown(CurrentMessage) : null). The @if (CurrentMessageHtml is not null) guard then prevents the <div> from rendering at all.

[jodavis]

I would like to have the markdown translation encapsulated in another property on the class. It's easier to set breakpoints and view translation in the debugger that way.

[Copilot]

Done in fe0ca00. The markdown conversion is now in CurrentMessageHtml:

private string? CurrentMessageHtml => CurrentMessage is not null ? RenderMarkdown(CurrentMessage) : null;

The template uses @if (CurrentMessageHtml is not null) and renders @((MarkupString)CurrentMessageHtml).