Task 5 code-review follow-ups:
- I1: ExternalSystemHelper gates its TraceStepScope on a new isTraced ctor arg
(default false for direct construction), not on `_traceSink is not null` — a
traced run without an inline collector still writes durable ScriptLog rows
that must name their step.
- I2 (design §5): an inline shared script now SHARES its caller's ScriptLogger
via the new `sharedLog` ctor param — one line cap, one anchor, one closing
row. Consequence documented on the Log property: a shared script's lines
carry the PARENT run's ExecutionId and land on the parent's ScriptRun node.
- M3: TraceCapturingLogger swallows a throwing formatter — most of what it
captures is best-effort "audit write failed" warnings logged from inside a
catch block, and a formatting exception must not escape that path.
- Tests: ApiCall step attribution, traced-without-collector, shared-script
logger sharing, nested batch/wait scope attribution + unwind, throwing
formatter; test (c) now also pins the anti-recursion property (EmitLog must
log through the undecorated logger).
CompileSurfaceParityTests stays red until Task 7 mirrors Log on ScriptCompileSurface.
Three final-review findings on the two picker pages:
1. DebugView's localStorage auto-reconnect restored the stored instance id with
no membership check, where the ?instanceId= path has one — a stored instance
that is no longer Enabled (or was deleted, or moved sites) is filtered out of
the list the picker is handed, so the field rendered blank while the page held
a non-zero id and auto-connected to it anyway. The guard now mirrors the
deep-link path: forget the stored instance id, tell the operator, and leave the
site selected and loaded so they can pick again.
2. FlowDebugger's _instances kept an OrderBy(UniqueName) from the flat <select>
era; InstanceTreeBuilder sorts areas and instances itself.
3. The deep-link test is joined by one where the instance sits two areas deep, so
the field text is pinned to resolve from the flat list with the panel never
opened.
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
ArrowDown moved focus into the tree, but the picker passed no Selectable /
SelectedKeyChanged, so Enter/Space on a treeitem only moved the roving tabindex —
the pick lived on the label span's @onclick, i.e. mouse only. The tree now runs in
Selectable mode and BOTH gestures arrive at one handler (OnTreeKeyActivatedAsync)
through TreeView's SelectedKeyChanged: an `i:{id}` key selects that instance, an
`a:{id}` key toggles the branch. The label's own @onclick is gone — the click
already bubbles into TreeView's content span, so a second handler there would fire
the callback twice. SelectedKey stays unbound (a combobox shows its pick in the
field, not as a row highlight, and binding it would arm TreeView's
selected-key-not-in-tree reset on every render while the pick is filtered out);
SelectedCssClass is emptied to match.
a11y: the field gains an accessible name via a new AriaLabel parameter (default
"Instance") and an optional InputId so a page can point a <label for> at it;
aria-controls is rendered only while the panel exists, instead of always pointing
at an id that is absent when closed.
Also drops the unused @using of Commons.Entities.Instances from the .razor.
Tests: keyboard activation on an instance row and on an area row, a
one-selection-per-gesture guard across mouse and keyboard, and the labelling
assertions. CentralUI suite 1331 passed / 0 failed; build 0 warnings.
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
Both Flow Debugger and Debug View enable the shared InstancePicker as soon as
_selectedSiteId lands, but Blazor renders at the site-change handler's first
await — at which point the picker is still holding the PREVIOUS site's Areas /
Instances lists. Typing into the field in that window (an operator, or
Playwright's FillAsync, which waits only for "enabled") was silently lost: the
picker compares both lists by reference, so the freshly-assigned lists read as
"the site changed" and it clears the field text and closes the panel.
Both pages now hold an _instancesLoading flag across the whole load — set before
the first await, cleared in a finally so a fault or a site-scope refusal cannot
leave the picker disabled forever — and fold it into the picker's Disabled
expression. InstancePicker itself is unchanged.
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
Pins the site-change behaviour the picker's reference contract exists for: after
picking an instance, switching the site select empties both the page's
_selectedInstanceId and the picker's field text (the new site hands it a
different instance list), and leaves the script select on its placeholder.
Layout: the Instance column goes col-md-2 → col-md-3 so a full instance name
fits — the script-entry branch still sums to 8 of 12 (site 2 + instance 3 +
script 3), and Method never renders beside them — with the placeholder
shortened to "Search instances…" for the narrower field.
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
Task 5 review follow-ups:
- New DebugViewInstancePickerTests.WhileConnected_ThePickerIsDisabled pins that
a live connection locks the picker: both the input and the chevron carry
`disabled`. It marks the page connected by reflection, the way the other
DebugView page tests do (DebugViewAlarmTableTests) — driving the real gRPC
stream is not available to a bUnit render.
- OnInstanceSelectionChanged() was already a no-op and its comment described the
@bind that the picker replaced; deleted, and OnInstancePicked now just records
the id (XML-documented).
- The ItemLabel lambda is hoisted to a private static InstanceLabel(Instance),
passed as a method group.
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
The instance row locator used Locator.GetByText scoped to the panel, which
bypasses the [data-test='flow-instance-row'] contract InstancePicker rows
are documented to carry. Match through the data-test attribute instead,
with an exact-text regex since the test only knows the instance name (not
its id, unlike Debug View's tests).
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
ArrowDown on a picker that has never been opened pins the deferred path where
_tree is still null when the keydown is handled: the panel opens in that render
pass and the roving tab stop lands on the first row in the next one. Escape on a
closed picker is pinned as a no-op.
Two notes, no behaviour change: FirstVisibleInstance records WHY it may trust
TreeView.IsExpanded (Blazor Server's single renderer sync context runs the render
plus the pending ExpandAll/CollapseAll before the next keydown is dispatched — an
await that genuinely yields inside OnAfterRenderAsync would break that), and the
stylesheet warns integrators that the absolutely-positioned panel is clipped by
any ancestor with overflow: hidden/auto.
The ArrowDown focus interop assertion is kept — it was stable across repeated
runs — with a comment marking the tabindex as the behavioural check.
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
The script entry's flat instance <select> is replaced by the shared
InstancePicker combobox (design 2026-09-08 §3): the site's areas load
alongside its instances in LoadInstancesAsync and are held in fields, so the
picker sees one stable list reference per site and only closes its panel on a
real site change. Picking a row raises the id, which OnInstancePickedAsync
applies before running the unchanged OnInstanceChangedAsync — the deep link,
the localStorage restore and the site-scope gate are untouched.
Tests: the five instance-select .Change("7") call sites go through a
PickInstance helper (open the panel, click the data-instance-id row), plus a
new test that areas open collapsed and only reveal the instance once both
levels are expanded, and one that ?instanceId= shows the instance name in the
picker field.
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
The flat instance <select> on Debug View is replaced by the shared
InstancePicker area-tree combobox (design 2026-09-08 §3): the page now loads
the site's areas alongside its Enabled instances in LoadInstancesForSite and
hands both lists to the picker with ItemLabel = "UniqueName (State)",
Disabled="@_connected" and DataTestPrefix="debug-instance".
Both lists are now REPLACED rather than cleared in place wherever the site
changes (LoadInstancesForSite, StartFresh) — the picker compares Areas and
Instances by reference and a new object is what tells it the site changed.
The ?siteId=/?instanceId= deep link and the localStorage reconnect keep
setting _selectedInstanceId directly; the picker reads it on parameter set,
so both paths are unchanged.
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
Flow Debugger's script-entry instance selection and Debug View's instance
selection both go through the shared InstancePicker combobox now instead of
a flat <select>. Update the tests to type into the {prefix}-input, wait for
the matching {prefix}-row, and click it; keep the <select>-polling helper
for the site select, which is unchanged.
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
Note the shared InstancePicker combobox (2026-09-08 design) replacing
the flat instance <select> on both pages, in Component-CentralUI.md's
Flow Debugger and Debug View sections and the CLAUDE.md Flow Debugger
bullet.
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
Joseph Doherty