Repository navigation
fix(#327): MainWindow's 32 bindings were all on reflection — five IsVisible gates could pin a drawer open over the app - #338
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rited-scope wording Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…already owns Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review focus, since this PR is net-additive by design (44 lines of markup comment + 2 attributes, 220 lines of new tests, 60 lines of corrected doc text — no code removed):
|
|
/agentic_review |
PR Summary by QodoCompile-check MainWindow bindings and guard shell wiring
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit ca49135 |
…ut cannot escape it Qodo round 2: the guard collected flyouts window-wide, so moving one out of the pane left both compared sets identical and the test passed while the pane no longer displayed it. Now read out of the pane subtree, with any stray flyout failing first. Re-ran all guard mutations after the change; T7 (a genuine move out of the pane, binding untouched) now fails and nothing previously caught was neutered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit b1677bc |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit b1677bc |
Shepherd pass on
|
| claim | re-derived | how |
|---|---|---|
| 32 bindings | ✅ 29 attribute-syntax + 3 <Binding Path=/> children of the MultiBinding |
XML parse, not a {Binding grep (the grep says 31 — 29 real plus 2 inside the comment) |
8 feed IsVisible, 5 gate a surface |
✅ | parse: the three flyouts, the busy overlay, the settings drawer; the other three are the debug button, the badge and the recovery banner |
| escape hatches: 0 | ✅ x:CompileBindings="False" 0, {ReflectionBinding} 0, exactly one x:DataType (the root) |
parse + raw-text scan; the 9 {TemplateBinding}s are in the ControlThemes and are a different mechanism |
+365 / -10 across 4 files; 44 markup / 261 test / 60 doc |
✅ | git diff --numstat origin/main...HEAD: 44/0, 261/0, 36/4, 24/6 |
| warnings 175 → 175 | ✅ | CI's own cold, all-project totals at both ends: b9bbe2b (this PR's merge base) 175, 5a88aa2 175, main 664d01c 175. My local incremental build reported 155, exactly the trap the body flags |
| 20 views / 770 bindings left on #327 | ✅ self-consistent | 799 − 29 by the table's own accounting, as labelled |
The positive control, re-derived rather than trusted
Not the whole ten-typo matrix — but one typo in the shape earlier PRs had not exercised, plus both
halves of the control, one change at a time, ~/.dotnet/dotnet build of the desktop head each time:
| state | change | result |
|---|---|---|
| this branch | <Binding Path="IsLogSummaryOpenZZZ"/> — a MultiBinding child element |
AVLN2000: Unable to resolve property or method of name 'IsLogSummaryOpenZZZ' on type 'Daqifi.Desktop.ViewModels.DaqifiViewModel' |
root declarations deleted (= main's state for this file) |
same typo | 0 Error(s), no diagnostic naming the file |
| root declarations deleted | MinWidthZZZ="720" on the Window tag |
AVLN2000: Unable to resolve suitable regular or attached property MinWidthZZZ on type Daqifi.Avalonia:Daqifi.Desktop.MainWindow |
The third row is the one that matters: it proves the silence in row 2 was a real compile of this file,
not a skipped or stale one. So the MultiBinding child — the shape {Binding greps cannot see and
that #327's table undercounts — really is covered, and really was not.
And the rejected alternative, tested rather than repeated
The body's design rests on a negative claim ("a ContentControl.ContentTemplate infers nothing, even
with Content="{Binding}" beside it"), which is also why the sixth guard was removed. So I put a
correct member name inside one of the five templates:
<prototype:LiveGraphPane IsVisible="{Binding IsBusy}"/>
→ AVLN2000: Unable to resolve property or method of name 'IsBusy' on type 'XamlX.TypeSystem.XamlPseudoType'
Confirmed on a different view and a different shape from the ones #335 and #327's comment measured.
The five undeclared templates therefore cannot hide a broken binding — any binding added to one is a
hard build error — and the removed "every template that binds declares a scope" guard genuinely could
never have failed. Leaving them undeclared is right, and #327's corrected rule (an explicit
x:DataType overrides, so a plausible-but-wrong one is worse than none) is not contradicted
anywhere in this diff. The working tree was restored and git status is clean; nothing above was
committed.
CI
gh pr checks 338: all 7 green on 5a88aa2 (run 34363288184, head_sha = the current head). The
run was created 2026-09-09T14:23:30Z and main has since taken #355 and #360 — but those touch
App.axaml.cs, Services/MacApplicationMenu.cs and docs only, and this PR touches four entirely
different files, so the sets are disjoint and the PR still reads MERGEABLE. Verification here is
CI's on 5a88aa2, read out of the job logs, not a local re-run of the gate: no code changed, so
re-deriving it locally would buy nothing and take a build lock off a builder who needs it.
Firing my own Qodo round next — the three on record are the builder's, and Qodo is not deterministic
across rounds on the same commit.
|
/agentic_review |
1 similar comment
|
/agentic_review |
Qodo-clean, CI green on
|
Part of #327. Part of #336 — not
Closes: one of the two sites #336 names is stillunfixed, for the reason given under “Second change” below.
MainWindow.axamlis the desktop shell — the toolbar, the notification badge, the tab strip, thebusy overlay, the three flyouts and the settings drawer. Every one of its 32 bindings resolved by
reflection at run time, because the window declared neither
x:DataTypenorx:CompileBindings. Arenamed, moved or deleted view-model member left the markup naming something that no longer existed,
and nothing in either head's build could see it.
What that bought us, concretely. A failed binding does not blank the control — it leaves the
target property at its default, and
IsVisibledefaults totrue. Eight of the 32 bindingshere feed
IsVisible, and five of those gate a whole surface: the settings drawer, the busy overlayand the three flyouts. A typo in any of the five did not hide that surface, it pinned it
permanently open over the running app — and desktop, iOS and every test stayed green. The other
three would have shown the notification badge with no count, the debug console button to every user,
and the config-recovery banner on every launch.
Two attributes on the root make all 32 the compiler's business. No behaviour change, no pixels moved.
Diff is
+365 / -10across 4 files: 44 lines of markup (2 attributes + 1 xmlns + a comment), 261lines of new tests, and 60 lines of corrected documentation.
The measurement
Ten deliberate typos, one at a time, each built on
origin/mainand on this branch:origin/mainWindow's ownHeight0 Error(s), no diagnosticAVLN2000onDaqifiViewModelToggleButton.IsChecked0 Error(s), no diagnosticAVLN2000onDaqifiViewModelStaticResourceconverter0 Error(s), no diagnosticAVLN2000onDaqifiViewModel<Binding Path=/>child of theSplitViewMultiBinding0 Error(s), no diagnosticAVLN2000onDaqifiViewModelAppSettings.HasRecoveryNotice0 Error(s), no diagnosticAVLN2000onDaqifiViewModelAppSettings.IsCommaDelimiter0 Error(s), no diagnosticAVLN2000onDaqifiViewModelCommand0 Error(s), no diagnosticAVLN2000onDaqifiViewModelSelectedIndex0 Error(s), no diagnosticAVLN2000onDaqifiViewModelIsVisible0 Error(s), no diagnosticAVLN2000onDaqifiViewModelFallbackValue0 Error(s), no diagnosticAVLN2000onDaqifiViewModelPositive control on the
mainside, because ten silences are otherwise indistinguishable from askipped or stale compile: a bogus CLR property (
MinWidthZZZ) in the same tag isAVLN2000: Unable to resolve suitable regular or attached property MinWidthZZZ on type … MainWindow.The file was being compiled on every one of those ten runs; it simply had nothing to say about the
bindings. Every source write in the harness is followed by a
touch, so no result is a staleassembly.
There is one binding scope in this view (the root), and the control reached it from ten different
binding shapes, including the two the earlier PRs had not exercised: a
MultiBindingchild element,which
x:CompileBindingsdoes cover, and a binding routed through a converter.No dead bindings. All 32 resolve against
DaqifiViewModel— nothing here was already broken.Escape hatches: zero
x:CompileBindings="False": 0.{ReflectionBinding}: 0. Counted by parsing, and now pinnedby a test, so this view cannot come to read as protected while half of it has opted out. (The nine
{TemplateBinding}s in theControlThemes are a different mechanism and are unaffected either way.)The five
DataTemplates deliberately declare nothingThis is the shape #327's corrected rule says infers nothing, and it is the reason I picked this view.
Measured here on Avalonia 12.1:
ContentControl.ContentTemplatewith a correct member name isAVLN2000 … on type 'XamlX.TypeSystem.XamlPseudoType'— unscoped, not mistyped;Content="{Binding}"gives the compiler theDataContexttypestatically, and it holds with
Content="{Binding AppSettings}"too. The inference does not followthe
Contentbinding at all. This is a stronger result than fix(#327): DeviceLogsView's 35 bindings were all on reflection — two attributes, six scopes, five of them inferred #335's, which only had theContent-typed-objectcase;x:DataType="vm:DaqifiViewModel"the template compiles, and a typo in it isAVLN2000onDaqifiViewModel. The declaration is genuinely load-bearing for this shape.All five templates hold a bare pane element and bind nothing, so they compile today and a
declaration would be pure decoration — while a stale one would be worse than none, since an
explicit
x:DataTypeoverrides and keeps compiling after theContentbeside it is repointed. Theyare left undeclared, and the view comments say what to add if anyone binds in one.
I also re-derived the inference half independently, on
MobileShellView(a different view and adifferent shape) rather than taking #335's report on trust — deleting both template
x:DataTypesthere: correct members still build clean;
{Binding IsSelectedZZZ}isAVLN2000onChannelToggle,the same type the deleted attribute named; and
{Binding ShowDeviceList}— a view-model member — isalso
AVLN2000onChannelToggle. It never falls back to the ancestor scope. That is themeasurement behind the #336 fix below.
Tests: 5 guards, each proven to fail on its own mutation
New class
MainWindowBindingTests(+261 lines), named for this view so it cannot collide with #335'sDeviceLogsViewBindingTests. Member names are not asserted — the compiler owns all 32 at once.Each guard was mutated and re-run, and each mutation still builds, so none of them is duplicating
the compiler:
x:CompileBindingsfrom the tag, leave the literal in the commentDataContextDaqifiViewModelverbatimMultiBindingSplitView.Pane, binding untouchedContent="{Binding}"on the five tab hosts{ReflectionBinding}Two of those are worth a word:
Content="{Binding}"reads like a no-op and is not. It is the only thing puttingDaqifiViewModelinto each pane'sDataContext.LiveGraphPaneandLoggedDataPanePrototypehaveno view model of their own, so deleting it renders those two tabs blank — while the other three,
which replace their own
DataContexton attach, keep working, which is exactly what makes thedeletion look safe when someone tries it.
SplitViewpane, opened by aMultiBindingOR-ing the same threebooleans their
IsVisibles use. Drop one and that flyout is unreachable (visible inside a pane thatnever opens); add one and the pane slides out empty. Both compile. Compared as a set, since the OR
is commutative.
I removed a sixth guard I had written first ("every template that binds declares a scope"): for
this shape the build already fails, so it could never fail on its own, and it would have fired
wrongly on a
ListBox.ItemTemplateadded later, which correctly needs no declaration. Its contentis now a comment where it belongs.
Qodo round 2 found a real hole in the flyout guard: it collected flyouts window-wide, so moving
one out of
SplitView.Paneleft both compared sets identical and the test passed while the panestill ORed a boolean for a flyout it no longer displayed. Fixed in
b1677bc— the flyout side is nowread out of the pane subtree and a stray flyout fails first. I re-ran the whole mutation matrix
after that change rather than only the new case, since narrowing an assertion for a reviewer is
exactly when something else quietly stops failing; all five earlier mutations still fail. The same
fix also made the path extraction tolerate
{Binding X, Mode=OneWay}, which it had been rejecting.Second change: #336, the "inherited scope" wording
BindingFacts.AssertTemplateScopedTo's failure message and doc comment said an undeclaredDataTemplate's bindings "resolve against an inherited scope rather than the item". Measured, thatnever happens — they resolve against an inferred scope, or against nothing at all (
XamlPseudoType,a build error). Corrected in the message and the doc, with the ordered rule, the shapes that infer,
and the reason not to add declarations by reflex.
#336namesDeviceLogsViewBindingTestsas the second site. When this PR was written that file wasnot on
mainyet, so it was left to #335 — that is no longer true. #335 merged asd6e37e1, andthe merge of
maininto this branch (5a88aa2) brought the file here, so the wrong wording is live onmaintoday inDeviceLogsViewBindingTests.Every_data_template_sits_inside_the_list_whose_items_it_renders(both the doc comment and the assertion message). It is deliberately not patched here: that would be
a comment-only commit onto a settled, Qodo-clean, CI-green head, and it is not this view's file.
#336 therefore stays open; the remaining sites are enumerated in a comment on it.
The same wrong wording is on
maintwice more inMobileShellViewBindingTests, so it is fixedthere too, cited to my own measurement above. Docs and assertion-message text only — no guard
behaviour changes. #335 did not touch
BindingFacts.cs, so the merge ofmainbrought no collisionthere.
Verification
dotnet builddesktop: 0 errors.dotnet buildiOSRelease/iossimulator-arm64after ano-RID restore: 0 errors.
dotnet test: 872 passed, 0 failed (7 of them new) on the currenthead
5a88aa2, which is CI's own figure. The 834 first written here was the count atb1677bc,before
mainwas merged in;mainat664d01cruns 865, so the +7 is unchanged. Re-run infull after the round-2 fix, not just the changed test.
--no-incrementalon both sides, because anincremental build here reports 155 and "155 vs 155" would read exactly like a clean result. Both
absolute totals are non-zero. By unique warning line the denominators are first-party 114 → 114
and all-project union (incl. vendored
third_party/oxyplot-avalonia) 122 → 122.git status --short -- '**/packages.lock.json'is empty; the iOS restorewas run without
-r.asserted. The macOS desktop job on this head runs the capture harness five times, captures all 39
screens byte-identically across the five runs, and matches every one against the committed
tools/parity-audit/baselines/macos-arm64.sha256— 39/39, with the denominator equal to thatmanifest's own line count. Nine of those 39 are this window (
desktop-1..9, including the settingsdrawer and each of the three flyouts open) and nine more are the same window at minimum size. A
capture delta here would itself be the finding; there is none.
What remains on #327
20 first-party views after this one. #327's table counts
MainWindow.axamlat 29; the real figureis 32, because the table counts
{Bindingoccurrences and the three<Binding Path=/>children ofthe
MultiBindinghave no such text — so the table undercounts every view that uses aMultiBinding.By the table's own accounting, 770 bindings are left.
The four
View/Prototype/*files (~295 bindings) are still the open question: whether they arereachable at all, which is a bigger question than whether they are checked. Note that
LoggedDataPanePrototypeandLiveGraphPaneare not dead — they are two of the five tabs thiswindow hosts, and this PR pins the binding that feeds them.
🤖 Generated with Claude Code