Repository navigation
fix(#327): ExportDialog's 22 bindings were all on reflection — one scope, no templates, and IsVisible defaults to true - #372
Merged
Conversation
…ope, no templates
AvaloniaUseCompiledBindingsByDefault is false for this project, so a view that
declares no x:CompileBindings has every {Binding} in it resolved by reflection at
run time: a renamed, moved or deleted view-model member fails silently while both
heads build green.
Measured on origin/main in a SINGLE build carrying two defects at once: a bogus CLR
property on the ProgressBar was AVLN2000 naming this file, proving the compiler read
it, while a {Binding} at a member that does not exist — same file, same compilation —
produced no diagnostic at all.
x:DataType="vm:ExportDialogViewModel" + x:CompileBindings="True" on the root makes
all 22 the compiler's business. The view contains no DataTemplate, so there is
exactly one scope and nothing to declare per-template.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… in ExportDialog Member names are now the compiler's business for all 22 bindings at once, so they are not asserted. What is left, one test each, all four proven to fail under a mutation: - the two root declarations, asserted by PARSING the root element — the comment beside them spells out both literals, so the substring form passes with the attributes deleted from the tag; - the x:DataType CLAIM against the object the dialog is actually handed. The dialog has no DataContext of its own and gets one from ShowDialogAsync<T>(object ownerViewModel, object viewModel), whose view-model parameter is typed object: ShowDialogAsync<ExportDialog>(this, this) COMPILES, and is caught only by this guard; - which attribute each binding feeds. The three state panels share one Grid cell and only IsVisible separates them, and a binding that does not resolve leaves its target at the property default — IsVisible defaults to TRUE, so an ungated panel draws over the state that should be on screen. Same for the ExportSucceeded fork: Open Folder on a failed export opens the folder the export could not write. Also corrects two claims this change makes false: ExportDialogViewModel's remark that IsAverageQuantityValid is "Bound by ExportDialog.axaml by reflection", and ExportResultHonestyTests' markup-half test, whose two AssertExposes calls the compiler now owns. Those were dropped rather than left in place — an assertion the build can no longer let fail reads as coverage and is none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
Ready for review. One view from #327 — The positive control is a single build carrying both defects: one Build gate green on desktop and iOS, 906/906 tests, visual gate 39/39 against a 39-line manifest (denominator checked, not assumed), zero first-party warnings against my own merge-base measurement of |
Contributor
Author
|
/agentic_review |
PR Summary by QodoCompile ExportDialog bindings and guard visibility contracts
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
… stop skipping call shapes
Finding 1 (escape-hatch guard misses valid syntax). Accepted and fixed. The guard
searched raw text for one exact spelling, and XML permits
`x:CompileBindings = 'False'` — spaces around the `=` and single quotes — which means
the hatch could be opened with the guard still green. It was wrong in the other
direction too: a comment explaining the hatch would have failed a clean file. Both
halves now work on the PARSED markup — every descendant's XAML-namespace
CompileBindings attribute, read as a Boolean, and {ReflectionBinding} found by scanning
parsed attribute values and text nodes rather than the file. Same lesson as Qodo round 2
on PR #325, applied to the hatches instead of the declarations.
Finding 2 (type guard skips dialog calls). Accepted; the underlying defect was real —
a call shape the regex could not parse was SILENTLY skipped while the two it understood
kept `callSites.Count > 0` satisfied, so the guard reported coverage it did not have.
Fixed by making that case loud rather than silent: every ShowDialogAsync<ExportDialog>
occurrence is counted across the whole Daqifi.Avalonia project, and the test FAILS when
any occurrence is not in the (owner, local) shape it can follow.
Deliberately NOT the recommended Roslyn semantic model: the test project references no
compiler API, and "fail on a shape I cannot verify" gives the same protection — nothing
unverified passes — at a fraction of the machinery.
Both fixes are mutation-checked, including Qodo's own two bypasses, each of which now
fails and would have passed before:
- x:CompileBindings = 'False' (spaces + single quotes) -> fails
- x:CompileBindings="False" spelled plainly -> fails
- a real {ReflectionBinding} on one binding -> fails
- the literal in a COMMENT on an otherwise clean file -> passes (no false positive)
- a call shape the guard cannot parse -> fails
Test count 906 -> 905: the escape-hatch [Theory] with two InlineData became one [Fact]
that checks strictly more. One fewer test case, more coverage.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
Round 1: both findings accepted and fixed in Both were the same species — a guard that read as protection and provided less — which is the exact failure this PR exists to remove, so they were worth catching here of all places.
Both fixes mutation-checked, including the two bypasses you named. Suite 905/905 (906 → 905 because an escape-hatch |
Contributor
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit a2e01c1 |
This was referenced Sep 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #327 — one view,
Daqifi.Desktop/View/ExportDialog.axaml. The ticket is a checklist and stays open.What was wrong
All 22 of the export dialog's bindings resolved by reflection at run time.
Daqifi.Avalonia.csprojsetsAvaloniaUseCompiledBindingsByDefaulttofalseand the view declared neitherx:DataTypenorx:CompileBindings, so a renamed or deleted member rendered blank — or, for abool, sat at its default, andIsVisibledefaults totrue— while desktop, iOS and every test stayed green.Three lines on the root put all 22 in front of the XAML compiler. No other markup changed — no binding was rewritten, moved or deleted.
That default matters more here than in most views: this dialog is three full-size panels stacked in one
Gridcell, andIsVisibleis the only thing keeping two of the three off the screen. A gate that failed to resolve did not hide a panel — it showed one, on top of the state that should have been visible.The count is 22, re-derived by parsing
#327's table lists this view at 22 and that number holds exactly. Parsed rather than grepped, with a brace-depth scanner so the
StringFormat={}{0}%escape on line 100 is not double-counted and a{Binding}inside a comment would not be counted at all:{Binding …}{MultiBinding}/{TemplateBinding}/{ReflectionBinding}The same parser was validated against the two views whose parsed totals are already on record:
SummaryFlyout.axaml→ 29 (matching #370),MainWindow.axaml→ 29{Binding}+ 9{TemplateBinding}.One scope, not several. The view contains no
DataTemplateat all, so every one of the 22 resolves against the rootx:DataTypeand there is no per-template question to answer here.Escape hatches: accounted for, and now guarded
Grepped and parsed, before and after:
x:CompileBindings="False"on any subtree{ReflectionBinding}Both are now assertions (
The_view_opens_no_escape_hatch), because either one puts part of the file back on reflection with the root declaration still in place and the build still green — so the count above and the guards below would both be measuring less than they claim.Controls
Positive control,
mainside — one build, both defectsThis is the control that makes the negative results below mean something, and it is taken as a single compilation so no second build has to be trusted. On the merge base
c45fe32, with both defects applied toExportDialog.axamlat once:Exactly one error — the bogus CLR property — and not one word about
{Binding ZzzExportResultMessageTypo}twelve lines further down the same file. The XAML compiler demonstrably read this exact file content and chose silence about the binding. A stale or skipped build cannot produce that pair.(Recorded because it nearly cost a false negative elsewhere: after a build fails in the XAML step, a bare re-run can report
Build succeededhaving compiled nothing. Every control here was taken off a fresh edit, and success was read from the process exit code, never from grepping for the word "succeeded" —-tl:onwrites ANSI escapes into that string.)Negative controls, branch side — one typo at a time, four shapes
Each applied alone, each a fresh edit, each its own build. All four are the file's distinct binding shapes rather than four samples of the easiest one:
c45fe32TextpathExportResultMessageZZZAVLN2000 … on type 'Daqifi.Desktop.ViewModels.ExportDialogViewModel'(113,24)IsVisible!IsAverageQuantityValidZZZAVLN2000 … 'ExportDialogViewModel'(76,32)CommandExportLoggingSessionsCommandZZZAVLN2000 … 'ExportDialogViewModel'(91,42)StringFormat={}ExportProgressZZZAVLN2000 … 'ExportDialogViewModel'(100,24)Each names the right type, and each build produced exactly one
AVLN2000for this file.No dead binding in this view
All 22 members exist on
ExportDialogViewModel— the branch builds clean with no markup edits. #327 warns to expect some; this one was clean, like #335 and #370 before it.What the new tests guard, and what they deliberately do not
Member names are now the compiler's business for all 22 at once, so they are not asserted.
ExportDialogBindingTestscovers only what a green build cannot see — and each of the four was run against its own mutation, which it caught:x:CompileBindingsfrom the tagThe_dialog_declares_its_data_typex:DataTypeclaim vs. the object the dialog is handedShowDialogAsync<ExportDialog>(this, this)The_declared_scope_is_the_view_model_the_dialog_is_actually_handedIsVisiblefrom the exporting panelEach_of_the_three_states_is_gated_by_its_own_flagOpen Folder/Try AgaingatesThe_result_buttons_follow_the_resultQodo round 1 found both guards weaker than they read, and both are fixed in
a2e01c1— the reviewer's own two bypasses now fail, and would have passed before:x:CompileBindings = 'False'— spaces and single quotes, valid XMLShowDialogAsync<ExportDialog>(this, (object)vm)— a shape the regex cannot parseBoth guards now work on parsed markup rather than raw text, the call-site scan covers every
.csfile inDaqifi.Avaloniarather than one, and an unparsed call fails loudly instead of being skipped — silent omission was the actual defect. The Roslyn semantic model Qodo recommended was deliberately not taken: the test project references no compiler API, and "fail on a shape I cannot verify" gives the same guarantee for far less machinery.The second one is worth spelling out, because nothing in the type system closes it: the dialog has no
DataContextof its own and is handed one byIDialogService.ShowDialogAsync<T>(object ownerViewModel, object viewModel), whose view-model parameter is typedobject.ShowDialogAsync<ExportDialog>(this, this)compiles, verified — and is caught only by this guard. Anx:DataTypepointed at a type that happens to share the member names compiles too, and renders blank.The fourth is not cosmetic either: Open Folder on a failed export opens the folder the export could not write, and Try Again after a successful one invites a second export over the first. #312's whole subject was this dialog claiming success it had not earned.
Not tested, because the build already rejects it: anything about member names, and a
DataTemplateadded to this view — per the corrected rule in #327's comments,ContentControl.ContentTemplateand resource-dictionary templates infer nothing and are a hardAVLN2000even with correct member names, while anItemsControl/ListBoxItemTemplateinfers correctly and needs no declaration. A test for either would only restate the compiler, which is the opposite of this ticket's point. The view's own comment says which case is which, so the next person does not add anx:DataTypeby reflex — a plausible-but-wrong one overrides the inference and is worse than none.Also here — two claims this change makes false
ExportDialogViewModel.IsAverageQuantityValid's remark said it is "Bound byExportDialog.axamlby reflection". Corrected.ExportResultHonestyTests' markup-half test said "both halves of this fix resolve by reflection", and paired eachAssertBindswith anAssertExposes. The prose is corrected and the twoAssertExposescalls are dropped: renaming either member is now anAVLN2000, so those assertions can no longer fail — and an assertion the build cannot let fail reads as coverage and is none. TheAssertBindshalf stays, because compiled bindings type-check the path and never the target: moving the!IsAverageQuantityValidgate fromIsVisibletoIsEnabledcompiles, and leaves the reason line permanently on screen.BindingFacts' own "inherited scope" wording is #336's ground and is untouched here.Verification
148` first-party warnings`, iOS148`` — measured against my own merge-basec45fe32, both sides force-rebuilt (`--no-incremental`) in this worktree with the same counter. Zero first-party warnings added. No constant quoted.dotnet test: 905 / 905 passing, 9 of them new (ExportDialogBindingTests). Re-derived after the last commit: the count went 906 → 905 in Qodo round 1, when the escape-hatch[Theory]with twoInlineDatabecame one[Fact]that checks strictly more — one fewer test case, more coverage. The two droppedAssertExposescalls were assertions inside an existing[Fact], so they cost no test.dotnet restorerun without-r;git status --short -- '**/packages.lock.json'is empty, so noNU1004lock-file drift.macos-arm64.sha256's 39 lines, and 39 PNGs were actually captured.dialog-export-configure.pnganddialog-export-failed.pngare byte-identical, as are the other 37. This dialog is a captured screen (dialog-export-configure.png,dialog-export-failed.png), and a purex:DataTypeaddition should move zero pixels.c45fe32+ five files (the fifth,BindingFacts.cs, is a one-word visibility change —RepoRootprivate → internal — added in round 1 so the call-site guard can walk the project). Zero file overlap with fix(#327): SummaryFlyout's 29 bindings were all on reflection — two nested template scopes, both inferred #370 (SummaryFlyout.axaml+ its tests) or chore(warning-zero): MVVMTK0034 x5 — read the generated properties, not the [ObservableProperty] backing fields #368 (LoggedSeriesLegendItem.cs+ its test); chore(deps): Bump the minor-and-patch group with 17 updates #363 is Dependabot and red on every head, untouched.🤖 Generated with Claude Code