Never serve sample code that still contains a substitution placeholder - #852
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Value-position substitution tokens were rewritten to the literal "...", which keeps markup well-formed but is not pasteable: a measured 140 attributes in the previous bake were typed properties (StrokeThickness, Height, Width, Orientation, SelectionMode, IsChecked and similar) where "..." is a compile error. Drop the whole attribute instead so the property falls back to its own default, matching how the gallery-side exporter already handles the same case. ExtractInlineCode flattened XAML tokens before CleanGalleryContent ran, which bypassed the position-aware pass entirely. Let the token reach that pass instead. Reorder CleanGalleryContent so the known IsOpen and Severity rewrites run before normalization, so those values survive. Bump CacheVersion to 22 and re-bake: same input, different output, so an existing cache would keep serving the broken attributes this change removes. Broken attributes now measure 0 across gallery, toolkit and reactor, with no scenario loss. Also document xamlOmittedAsMalformed, which the gallery emits on 12 samples but the schema never described, and add codePlaceholdersPresent so a consumer can tell that a sample's C# still carries unresolved tokens. C# cannot use the XAML deletion trick: the tokens appear as identifier fragments, fixed-arity arguments or whole statements, none of which can be removed without a syntax error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
Build Metrics ReportBinary Sizes
Test Results✅ 5816 passed, 18 skipped out of 5834 tests in 1023.9s (+7 tests, +179.6s vs. baseline) Test Coverage✅ 87.5% line coverage, 80.8% branch coverage · ✅ no change vs. baseline CLI Startup Time51ms median (x64, Try This BuildInstalls the MSIX for your architecture, replacing any previously installed build. Needs the GitHub CLI — the command offers to install it and sign you in if it is missing. Switching between builds often?Put the tool on your PATH once: Then this build is just: Run Updated 2026-09-15 19:36:53 UTC · commit |
Zach Teutsch (zateutsch)
left a comment
Zach Teutsch (zateutsch)
left a comment
There was a problem hiding this comment.
Approving with one non blocking item from AI review, we should probably remove this unless we have a reason for having it:
Published gallery schema object has no producer or consumer
-
What is wrong: The PR adds a
sample.galleryobject (xamlPlaceholdersDropped,xamlOmittedAsMalformed,codePlaceholdersPresent) to the published sample-index schema, but nothing writes those fields and nothing reads them. It documents a diagnostic contract that doesn't exist yet. -
Show me:
git grep xamlPlaceholdersDropped→ onlydocs/winui-sample-index.schema.json.SampleIndexSchema.Galleryis referenced only inSampleProperties, which is consumed only by a test parity assertion (SampleIndexTests.cs:500). No producer emits it; the parser suppresses placeholder blocks without reading it. -
Why it matters: A published schema is effectively forever. An external producer may implement these fields believing they're honored, and future maintainers must preserve or explain a contract the code never implemented.
-
Smallest fix: Drop the
galleryobject fromdocs/winui-sample-index.schema.jsonand removeGalleryfromSamplePropertiesuntil a real writer/reader needs it. Keep the pasteability wording changes toxaml/code. -
Location:
docs/winui-sample-index.schema.json:148,src/winapp-CLI/WinApp.Cli/Services/Controls/SampleIndexSchema.cs:69,86
## Description Replaces the WinUI Gallery scraper with the machine-readable sample index now published by microsoft/WinUI-Gallery#2232 at `catalog/windows-samples.json`. The old path reconstructed the corpus by downloading Gallery metadata and hundreds of individual source files. This change consumes the upstream index in one request and deletes `GalleryFetcher.cs` entirely. A shared `SampleIndexFetcher` now handles the Gallery and Reactor indexes. The published index contains Gallery source verbatim, so `GalleryProvider` preserves the pasteability normalization users relied on: Gallery-private page types and namespaces become app-local placeholders, calls to Gallery's internal `UIHelper` are removed, and XAML event attributes are retained only when the emitted C# defines the handler. Samples remain complete rather than being truncated; `find-ui` retrieves one selected sample at a time, and measurement showed recovering a truncated sample costs more tokens than serving it whole. Gallery `xmlnsImports` are now rendered in the `**Setup:**` line instead of being discarded by the Toolkit-only formatter gate. ## Usage Example ```powershell winapp find-ui "animated icon" winapp find-ui --id gallery-animatedicon-2 ``` The selected sample now includes the namespace declaration published by Gallery: ```text **Setup:** `xmlns:animatedvisuals="using:Microsoft.UI.Xaml.Controls.AnimatedVisuals"` ``` A cold corpus refresh fetches the Gallery index once rather than making up to 433 requests. ## Related Issue Closes: #809 Upstream index: microsoft/WinUI-Gallery#2232 Depends on the index-consumption foundation from #852. ## Type of Change - ♻️ Refactor - ⚡ Performance - 🐛 Bug fix ## Checklist - [x] New tests added for the published-index corpus contract and pasteability guards - [x] Tested against the live WinUI Gallery index - [x] Tested locally on Windows - [x] Full build and packaging completed - [x] No command syntax or workflow documentation changes required ## Additional Notes **Before → after:** | Metric | Scraper | Published index | |---|---:|---:| | Gallery HTTP requests on a cold refresh | up to 433 | 1 | | Scenarios | 330 | 327 | | Controls | 114 | 115 | | XAML samples | 295 | 290 | | C# samples | 107 | 116 | | Scenarios with no usable code | 5 | 0 | The three removed scenarios are upstream omissions rather than parser loss. Search enrichment preserves exact tag parity across all 127 controls represented by the shared Gallery tag data. **Validation:** - `scripts/build-cli.ps1 -SkipTests` completed successfully, including NativeAOT, npm, NuGet, and MSIX packaging. - 179/179 find-ui, Gallery, sample-index, snapshot, formatter, Toolkit, and Reactor tests passed. - A fresh live bake produced 327 Gallery, 48 Toolkit, and 95 Reactor scenarios at cache version 23. - Runtime checks confirmed unbacked handlers and Gallery-private helpers are removed, Gallery namespace imports are printed, and Toolkit/Reactor output remains intact. The full CLI suite was not used as the local signal because unrelated network/UI-dependent tests block on this machine; PR CI remains the complete repository gate. ## AI Description <!-- ai-description-start --> _This section is auto-generated by AI when the PR is opened or updated. To opt out, delete this entire section including the marker comments._ <!-- ai-description-end --> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Nikola Metulev <nmetulev@users.noreply.github.com> Copilot-Session: 034ffdc8-3308-4286-96d8-e02406110630 Copilot-Session: 883032eb-8c56-4340-b3d7-4c38bb1916b8
find-uipromises code you can paste. For Gallery-style samples it could not keep that promise: those snippets carry$(Name)substitution placeholders that the Gallery resolves at runtime from live option controls, and nothing downstream was checking for them.The two ways this went wrong
On the scraping path, a placeholder in an attribute value was rewritten to the literal
"...". That keeps the markup well-formed, which is why it survived review as "cosmetic" — but it is not pasteable. Decompressing the previous bake and counting shows 140 attributes set to"...", and they are overwhelmingly typed properties:"..."failsStrokeThickness,Height,Width,RadiusX/Y,ValuedoubleOrientation,SelectionMode,PaneDisplayModeIsCheckedboolEvery one of those is a compile error the moment someone pastes it.
On the index path, nothing looked for placeholders at all, so a sample could be served with its tokens intact.
What this changes
A shared detector,
SampleSubstitutionPlaceholder, is now the single definition of what a placeholder is. Both paths use it.Scraping path.
GalleryFetcher.NormalizeMarkupSubstitutionsresolves a token by the position it occupies rather than blanket-replacing it:<Button $(Background)/>) — the whole attribute is removedValue="$(X)") — the whole attribute is dropped, so the property falls back to its own defaultThe last one is the behaviour change. Dropping the attribute is what the Gallery-side exporter already does, and it is the only option that compiles.
Two supporting fixes were needed to make that actually take effect:
ExtractInlineCodeflattened XAML tokens to"..."beforeCleanGalleryContentran, so the position-aware pass never saw them. It no longer pre-flattens.CleanGalleryContentnow runs the knownIsOpen/Severityrewrites before normalization, so those real values survive instead of having their attribute dropped.Index path.
SampleIndexParsersuppresses a language block that still contains a placeholder and drops the sample only if neither block survives.ScenarioSanitizerenforces the same rule on the way out, so a placeholder cannot reach a caller through either route.Cache.
CacheVersiongoes to22. Same input, different output, so without the bump an existing cache keeps serving the broken attributes this change exists to remove — the trap entry"20"already documents. The snapshot is re-baked accordingly.Result
Measured against a fresh bake, decompressing each snapshot and parsing the JSON:
"..."No content was lost making this safe: gallery XAML went up, 302 → 310, because fragments that previously failed the sanitizer's well-formedness check now survive.
Schema
The contract gains the three
galleryfields the WinUI Gallery exporter actually emits, which were undocumented:xamlPlaceholdersDropped— tokens already removed; the XAML pastes as publishedxamlOmittedAsMalformed— XAML withheld because it did not parse (12 samples upstream)codePlaceholdersPresent— tokens still present incode; it does not compile as publishedRead the first and last as opposites, not companions. One records cleanup already done, the other warns that cleanup was not possible. A consumer conflating them pastes broken C#, which is what the field exists to prevent — the description says so explicitly.
C# gets no deletion equivalent, and that is a property of the language rather than unfinished work: no C# construct yields a default by being absent. The tokens that actually occur are identifier fragments (
BadgeNotificationGlyph.$(SelectedGlyph)), fixed-arity arguments (SetBorderAndTitleBar($(HasBorder), $(HasTitleBar))), and whole statements. None can be cut out without a syntax error or removing the sample's subject.Testing
Full suite: 5437 tests, 12 failures, all pre-existing and environmental — 11 NuGet live-source tests that need
api.nuget.org, and 1 crash-dump test. Same 12 fail on a clean checkout.Directly affected classes re-run green after the schema change:
SampleIndexTests25/25 (includes the schema/reader drift guard),GalleryFetcherCleanContentTests7/7,FindUiSearchTests27/27,ScenarioSanitizerTests23/23,EmbeddedSnapshotTests19/19.New tests cover each token position, the attribute-drop behaviour, and a guard that the
IsOpen/Severityrewrites still win over attribute dropping — the regression the reordering could have caused.Related
Related to microsoft/WinUI-Gallery#2232, which publishes the sample index and guarantees its XAML is placeholder-free. The two are technically independent and can merge in either order — the Gallery's index validates against this repo's schema both before and after this PR, because the contract sets
additionalProperties: trueat every level. Confirmed with ajv (draft 2020-12) against both the currentmainschema and the one here: valid in both cases.What this PR adds is description rather than permission. On
main,sample.gallerydeclares no properties at all, so the three fields that carry the pasteability contract are tolerated but unexplained. Landing this first simply meanscodePlaceholdersPresentis described by the published schema as soon as the Gallery starts emitting it, rather than shortly after.