Repository navigation
feat(parity): the capture harness can photograph dialogs, so a dialog change is verifiable - #220
Conversation
… change is verifiable Every Window reached through IDialogService was outside the visual gate: the harness rendered the desktop panes, the drawers and the mobile shell, and constructed no dialog at all. That mattered more here than it would elsewhere, because XAML in this repo is not compile-checked — the views carry no x:DataType, so bindings resolve by reflection at run time and a green build cannot tell you whether a TextBlock rendered, whether its binding resolved, or whether a StaticResource brush exists. #209, #218 and #219 each shipped with an explicit "I did not render this dialog" disclosure. Adds a CaptureDialogs() phase with six seeded states across two dialogs — four connect states (WiFi scanning, USB idle, USB with a connect error, Manual USB with a validation error) and two export states (configure, failed result). All six are deterministic and committed to the macOS baseline, taking the gated set from 18 screens to 24. The reusable parts, each of which was a trap somebody hit by hand: - An indeterminate ProgressBar never settles, so a dialog showing one fails SettledFrame after 40 rounds with a message that says nothing about progress bars. The connect dialog has one behind a "Scanning…" overlay on three of its five tabs, so it is the default state of the dialog most likely to be captured next. SettleIndeterminateProgress now fails by name and offers two remedies: seed the state that hides it (preferred), or opt into FreezeIndeterminateProgress, which stops the animation and logs that it did. The settle rule itself is not loosened for anything — CI caught a real six-pixel race with it in #201. - RequireRenderedText reads a bound element back out of the visual tree by AutomationProperties.AutomationId and reports type, visibility, bounds, foreground and text, separately from the pixels: a TextBlock whose binding silently failed and one that rendered an empty string are indistinguishable in a screenshot. - SelectTab matches on the tab header rather than an index, so selecting and asserting are the same act. Determinism: 24/24 byte-identical across 10 runs, twice (20 runs total, macOS 26.5 25F71, Apple silicon). The manifest change is purely additive — all eighteen existing hashes are unchanged — and --check-captured passes over a fresh determinism set. No scenario may start device discovery, and that is the standing rule rather than an accident of this change: a real SerialDeviceFinder opens every DAQiFi VID/PID COM port on the machine. Scenarios seed the bound collections directly with fixed literals. The README's side-effects section previously guaranteed this by saying the harness never opens the connection dialog, which is no longer true; it now names the actual guarantee. Deliberately not covered, with the reasons recorded in the README: the autosizing alert family (headless does not apply a window's MinWidth/MinHeight — DuplicateDeviceDialog declares 480x280 and lays out at 460x248, so a capture would be a picture of a dialog no user has), and FirmwareDialog (its view-model constructor starts a network fetch that populates a bound ComboBox, so its rendered state races github.com). closes #213 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoGate six dialog states in the Avalonia visual capture harness
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR |
|
Ready for review — not merging. Qodo: clean in one round on CI: all six jobs green. The load-bearing one is Verification method: the dialogs were rendered and looked at, not reasoned about — two are attached to the body. Locally Two dialog families are deliberately excluded with measurements rather than hand-waving: the autosizing alert family (headless lays No board was attached and none was needed — no scenario opens a port. |
Not merging — for review.
closes #213.What a reviewer can now verify that they could not before
Every
Windowthe app shows throughIDialogServicewas outside the visual gate. The harness rendered the desktop panes, the drawers and the mobile shell — 18 screens — and constructed no dialog at all.That gap is worse here than it would be in most codebases: XAML in this repo is not compile-checked. The views carry no
x:DataType, so bindings resolve by reflection at run time. A green build cannot tell you whether a newTextBlockrenders, whether its binding resolves, or whether aStaticResourcebrush exists. Dialogs were therefore the surface where a green build proved least, and the one surface nobody could photograph. #209, #218 and #219 each shipped with an explicit "I did not render this dialog" disclosure.Six dialog states are now gated, byte-for-byte, on every pull request:
dialog-connect-wifi-scanningdialog-connect-usb-idledialog-connect-usb-errorSerialConnectError— the exact surface #209 could not verifydialog-connect-manual-usb-errorManualPortErrordialog-export-configuredialog-export-failedExportSucceededfalseThe attached
dialog-connect-usb-error.pngis the proof the capability works: it is a render of the realConnectionDialogwith a realConnectionDialogViewModel, produced byrun.sh, and it is one of the six now in the committed baseline.Beyond the pixels, the harness reads the element back out of the visual tree and says so, because a
TextBlockwhose binding silently failed and one that rendered an empty string are indistinguishable in a screenshot:That settles three things a green build cannot: the binding resolves,
NotNullToVisdrivesIsVisible, andStatusRedresolves to#fff43f5e.How to invoke it
Unchanged — the existing commands now cover dialogs too:
Adding a dialog state is appending a
DialogScreentoDialogScreens()and its name toExpectedScreens, then re-recording the baseline and bumpingexpected=in the macOS CI step. Documented intools/parity-audit/README.mdunder a new Dialogs section, with the four rules that each cost somebody real time:SerialDeviceFinderopens every DAQiFi VID/PID COM port on the machine. Discovery is a separate call from construction (StartConnectionFinders()), so building a dialog view-model is inert; scenarios seed the bound collections directly with fixed literals.ProgressBarnever settles. It burns all 40 settle rounds and fails with a message that says nothing about progress bars. The connect dialog has one behind a "Scanning…" overlay on three of its five tabs, so this is the default state of the dialog most likely to be captured next.SettleIndeterminateProgressnow fails by name and offers two remedies: seed the state that hides it (preferred — that is a state the app really has), or opt intoFreezeIndeterminateProgress, which stops the animation and logs that it did, so the empty track in the PNG is never read as a rendering bug.RequireRenderedText, byAutomationProperties.AutomationId).SelectTab) — the wrong tab under the right filename passes every check this tool has.Determinism evidence
Determinism is a hard requirement here, so these are inside the baseline set rather than exempt from it, and the six new screens are committed to
baselines/macos-arm64.sha256.--determinismwithDETERMINISM_RUNS=10, run twice:24/24 screens byte-identicalboth times. 20 clean runs in total, on macOS 26.5 build 25F71, Apple silicon, .NET SDK 10.0.302, Avalonia 12.1.1. Every dialog settled in 1 round.shasum -cof the pre-change manifest against a post-change capture: 18 OK, 0 failures. The manifest diff is consequently purely additive — 6 lines added, no existing hash touched — which is the evidence that the new phase moved nothing that was already gated.CaptureDialogs()runs last, after both existing phases, precisely so the 18 are a function of the same sequence that produced their baselines.--check-capturedpasses over a fresh determinism set with the new manifest.Desktop head on macOSjob on this PR is green, including both visual-gate steps: "Visual capture harness must be deterministic" (5 captures, plus its independentexpected=24count assertion) and "Committed baseline must be reproduced on this runner" (--check-capturedagainst the new 24-line manifest). So the six dialog hashes are cross-machine reproducible onmacos-latest, not just on the Mac that recorded them — the same standard tooling: the macos-arm64 baseline is named for OS+arch but has only ever been reproduced on one Mac #188 set for the original eighteen, met on the first run rather than assumed.The settle rule itself is not loosened for anything:
SettleSampleInterval,Quiesceand the render-the-treeEncodeare untouched. CI caught a real six-pixel race with that rule in #201 and it stays load-bearing.What is deliberately NOT covered, and why
Neither is a matter of adding another
DialogScreen; both are recorded in the README.ErrorDialog,SuccessDialog,DuplicateDeviceDialog,MessageDialog). I built and captured these first, then measured that headless does not size them the way a real window does:DuplicateDeviceDialogdeclaresMinWidth="480" MinHeight="280", and headless itsBounds,ClientSize,DesiredSizeandWidth/Heightall come back 460×248 — under its own stated minimum. A capture at that size is a picture of a dialog no user has, and pinning an arbitrary size instead photographs a layout the app never produces. So I removed them rather than ship a screen I could not vouch for.DialogScreen.Sizeisrequiredto keep that decision from being made by accident.FirmwareDialog.FirmwareDialogViewModel's constructor starts a network fetch (LoadFirmwareOptionsAsync) that populates a boundComboBoxasynchronously, so its rendered state is a race against github.com. Capturing it needs an injected stubIFirmwareDownloadService, which makes the picture a picture of a stub.One stale safety claim corrected
The README's side-effects section guaranteed no serial/WiFi discovery by asserting "
StartConnectionFinders()has exactly one caller ... and the harness never opens that dialog." This change makes that sentence false. It now names the actual guarantee — discovery is a separate call from construction, and no capture scenario may start a finder — which is the invariant a future scenario has to preserve. This one is load-bearing: it is what keeps a capture from rendering an attached board's serial number into a screen that has to be byte-reproducible.Build gate
dotnet build Daqifi.Avalonia.Desktopdotnet restore Daqifi.Avalonia.iOS(no-r)dotnet build Daqifi.Avalonia.iOS -c Release -r iossimulator-arm64 --no-restoregit status --short -- '**/packages.lock.json'I make no warning-count claim: I did not build a matched merge-base baseline with cold
third_party/oxyplot-avalonia/objon both sides, so I have no honest delta.Diff: 610 added / 31 removed across 5 files —
AvaloniaCapture/Program.cs(468/8),README.md(123/18),run.sh(8/2),build.yml(5/3),baselines/macos-arm64.sha256(6/0). No test project is touched: this change is test infrastructure, and its own gate is the CI job above.Baseline refresh on other merges
baselines/macos-arm64.sha256is per-commit truth, so any merge that changes rendered pixels needs it re-recorded. Of the PRs open alongside this one:ConnectionDialogViewModel. It touches no AXAML, and none of these scenarios calls the paths it changes: the four connect states seedAvailableSerialDevices/SerialConnectError/ManualPortErrordirectly and never enterConnectSerialAsync,StartSerialDiscoveryor the duplicate-device prompt. Its newConnectionAttemptResultand its "dismissing the duplicate-device prompt leaves the dialog open" change are both downstream of a connect attempt these captures do not make. Expected: no refresh. Whichever of the two lands second should still run--check-capturedto confirm that rather than assume it.ChannelEF entity andIColorable, with a migration) and fix(disk-space): the logging gate measured the root filesystem, not the disk being written to #217 (DiskSpaceMonitorvolume resolution) — if either changes what the Channels pane or the Storage pane renders, they refresh the existing screens, which is true of them with or without this PR.LoggingFleet) — same: only if it changes rendered content.Nothing in this change alters the pixels of the 18 existing screens, so a baseline conflict with any of the four would be a content conflict in the same file, resolved by re-recording once after both land.