Skip to content

fix(find-ui): Gallery corpus is upstream-only; ten controls that returned no code now do - #815

Merged
Jaylyn Barbee (Jaylyn-Barbee) merged 13 commits into
mainfrom
jay/find-ui-upstream-truth
Sep 11, 2026
Merged

Jaylyn Barbee (Jaylyn-Barbee) merged 13 commits into
mainfrom
jay/find-ui-upstream-truth

Conversation

@Jaylyn-Barbee

@Jaylyn-Barbee Jaylyn Barbee (Jaylyn-Barbee) commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Description

Two things, one cause.

1. GalleryFetcher presented winapp-authored content as WinUI Gallery's. It post-processed the corpus it fetched, then stamped the results Source = "gallery" — the value find-ui renders as the [gallery] tag and uses to build ids like gallery-itemsrepeater-7.

Mechanism What it did State when found
InjectMissing — CommandBar Appended a hand-written CommandBar sample Dead. Guarded on Gallery having no commandbar sample; Gallery ships one now.
InjectMissing — itemsrepeater-7 Appended a hand-written UniformGridLayout photo grid Live, unconditional. No detection logic at all.
ApplyOverrides — tabview-1 Rewrote upstream's CreateNewTab C# body Live. Guarded on CSharp.Contains("Frame").

All three are gone. FetchAsync is now a passthrough.

This blocks #703: we cannot ask WinUI-Gallery to publish a sample index while merging samples they do not own into their data — still less hand them a generator that does it.

2. The CommandBar injection was papering over a bug winapp owned. Review asked why CommandBar needed a hand-written sample at all. It does not. Upstream's sample is complete and well-formed; winapp's extractor was destroying it.

Gallery samples declare optional attributes and injected markup as $(Name) tokens that the Gallery app expands at runtime. GalleryFetcher flattened every token to "..." regardless of position, which is only valid in one of the three:

upstream   <Button Content="Standard XAML button" Click="Button_Click" $(IsEnabled)/>
flattened  <Button Content="Standard XAML button" Click="Button_Click" .../>
           -> Name cannot begin with the '.' character

ScenarioSanitizer.XamlIsWellFormed then rejected the snippet and Xaml was set to null. The control still appeared with a fetchable id and a "Fetch full code" prompt, and returned a successful-looking result with no code in it.

Surveyed all 480 upstream sample files. Ten were affected:

Button · ToggleButton · RepeatButton · HyperlinkButton · ProgressRing (×2) · CommandBar · AnimatedIcon · PersonPicture · EasingFunction

NormalizeMarkupSubstitutions now resolves tokens by position:

Position Example Handling
Attribute Click="X" $(IsEnabled)/> Removed — stands in for a whole attribute
Element content </AppBarButton>$(MoreCommands) Becomes a comment — all 5 upstream instances inject markup into a property element, never text, so flattening assigned the literal string "..." as a collection's content
Attribute value Value="$(Progress)" Unchanged — flattening here is cosmetic and stays valid

Comments are copied verbatim so a > inside one cannot desynchronize the scanner.

Usage Example

No CLI surface changes.

$ winapp find-ui --id gallery-commandbar-1

Before: header, Important notes, See also — zero XAML, zero C#.

After: upstream's actual CommandBar markup, with the runtime-injection point marked as a comment.

Related Issue

Unblocks #703. Companion to #806, which defines the index contract; this PR makes the data we would generate from that contract honest.

Type of Change

  • 🐛 Bug fix

Checklist

  • New tests added for new functionality
  • Tested locally on Windows
  • Main README.md updated — n/a
  • docs/usage.md updated — n/a, no command surface changed
  • Language-specific guides updated — n/a
  • Sample projects updated — n/a
  • Shipped skills updated in plugins/winapp/skills/ — winapp-find-ui/SKILL.md

Additional Notes

⚠️ This PR re-bakes the corpus — do not apply corpus-rebake-not-needed

snapshot-gallery.json.br, snapshot-reactor.json.br and snapshot-manifest.json are updated, and CacheVersion 19 → 21.

The bump is load-bearing, not housekeeping. Caches match on an exact version string, so without it a user who already fetched keeps being served both the winapp-authored sample and the ten empty scenarios indefinitely — including offline. A re-bake alone never reaches them. CacheVersion.cs states the rule: bump whenever "cleaning logic changes would alter the cached output for the same input data."

Isolating the change from upstream drift

The committed corpus was stale, so a naive before/after would have conflated our removal with upstream's own additions. Baked back to back against the same live upstream: main 323 scenarios, branch 322. The only differences were itemsrepeater-7 removed and tabview-1 restored to upstream's C# (939 → 1064 chars). The CommandBar injection produced no difference — direct confirmation it had gone dead.

Manifest now reads gallery 322 / reactor 95 / toolkit 48.

Known follow-up (not in this PR)

ItemsRepeater's $(SampleCodeLayout) resolves upstream to a UniformGridLayout block stored in a Page.Resources x:String that winapp does not fetch. Resolving those would recover more real upstream markup — and would have covered the image-grid case directly. Worth its own issue.

Verification

  • 4157 tests pass, 17 skipped. The single failure, AnalyzeDumpAsync_ManagedDumpWithDeepStack_DetectsStackOverflowShape, is an ARM64-host/x64-dump architecture mismatch verified as pre-existing by reproducing it on a stashed-clean tree. Network-bound classes (Package/Sign/Nuget/Msix/BuildTools/EndToEnd) are excluded — unreachable api.nuget.org on a corp machine; CI covers them.
  • Three new tests cover the fetcher fix (attribute / value / content position). GalleryCorpus_CarriesNoWinappAuthoredSample still guards against an authored sample reappearing.
  • scripts/build-cli.ps1 clean end to end — 0 warnings, warnings-as-errors on. docs/cli-schema.json unchanged, as expected for a no-surface-change PR.
  • scripts/validate-plugin-package.ps1 clean.
  • Live CLI confirms all ten controls now serve XAML.

…es them

GalleryFetcher post-processed the fetched corpus: ApplyOverrides rewrote the C#
body of gallery-tabview-1, and InjectMissing appended a hand-written ItemsRepeater
photo-grid sample. Both were then stamped Source = "gallery", which is what
find-ui renders as the [gallery] tag and the gallery- id prefix — so winapp
presented its own content to users as WinUI Gallery's.

That is wrong on its own terms, and it blocks #703: we cannot ask WinUI-Gallery to
publish a sample index while quietly merging samples they do not own into their
data, nor hand them a generator that does it.

The guidance those two carried is kept, on a surface that says who wrote it:

- The image-grid pattern moves to Notes.cs, so it appears in the Important notes
  on every ItemsRepeater result rather than masquerading as an upstream sample.
- The TabView advice was already there ("TabViewItem.Content can be ANY UIElement;
  use Frame only for page navigation"), so removing the override loses nothing.

The CommandBar injection is deleted outright. It was guarded on Gallery having no
commandbar scenario, which stopped being true — it had silently become dead code.

Corpus rebaked. Isolating the change from upstream drift, by baking main and this
branch back to back against the same live upstream: 323 -> 322 scenarios, the only
differences being itemsrepeater-7 removed and tabview-1's C# restored to upstream's
(939 -> 1064 chars). Committed counts move 321 -> 322 because upstream added two
samples since the last bake.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 30e85b00-6b59-4df1-b2d7-5642c86c823a

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Moderate issues remain around cache invalidation, search discoverability, and regression coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Removes winapp-authored scenarios and overrides from the Gallery-attributed corpus while retaining ItemsRepeater guidance in winapp-owned notes.

Changes:

  • Removes Gallery sample injection and TabView override logic.
  • Adds ItemsRepeater guidance and a regression test.
  • Updates the embedded snapshot manifest and user guidance.
File summaries
File Review
src/winapp-CLI/WinApp.Cli/Services/Controls/Notes.cs Adds image-grid guidance, but it is not searchable through the documented “photo grid” flow. Moderate (2 votes)
src/winapp-CLI/WinApp.Cli/Services/Controls/GalleryFetcher.cs Removes bespoke mutations. Cache version must be bumped to invalidate legacy content. Documentation also overstates preservation of upstream content. Moderate (1 vote); nit (2 votes)
src/winapp-CLI/WinApp.Cli/Services/Controls/Data/snapshot-manifest.json Updates snapshot metadata and corpus counts.
src/winapp-CLI/WinApp.Cli.Tests/EmbeddedSnapshotTests.cs Guards the removed ItemsRepeater sample but not the former TabView override. Moderate (1 vote)
plugins/winapp/skills/winapp-find-ui/SKILL.md Overstates exact upstream reproduction and incorrectly routes Toolkit reports to WinUI Gallery. Nits (2 votes and 1 vote)
Review details

Suppressed comments (1)

plugins/winapp/skills/winapp-find-ui/SKILL.md:139

  • This section covers both Gallery and Toolkit, but it directs every bad or missing sample report to WinUI-Gallery. A user investigating a [toolkit] result would file against the wrong project. Route reports according to the result's source tag and link both upstream trackers.
If a sample looks wrong or missing, it's upstream's to fix — report it on
[WinUI-Gallery](https://github.com/microsoft/WinUI-Gallery/issues) so every
consumer benefits, not just winapp.
  • Files reviewed: 5/7 changed files
  • Comments generated: 5
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/winapp-CLI/WinApp.Cli.Tests/EmbeddedSnapshotTests.cs Outdated
Comment thread src/winapp-CLI/WinApp.Cli/Services/Controls/GalleryFetcher.cs
Comment thread src/winapp-CLI/WinApp.Cli/Services/Controls/Notes.cs Outdated
Comment thread plugins/winapp/skills/winapp-find-ui/SKILL.md Outdated
Comment thread src/winapp-CLI/WinApp.Cli/Services/Controls/GalleryFetcher.cs Outdated
@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Build Metrics Report

Binary Sizes

Artifact Baseline Current Delta
CLI (ARM64) 46.13 MB 46.12 MB 📉 -14.0 KB (-0.03%)
CLI (x64) 46.04 MB 46.03 MB 📉 -12.5 KB (-0.03%)
MSIX (ARM64) 19.05 MB 19.04 MB 📉 -11.7 KB (-0.06%)
MSIX (x64) 20.19 MB 20.19 MB 📉 -0.6 KB (-0.00%)
NPM Package 39.63 MB 39.61 MB 📉 -15.4 KB (-0.04%)
NuGet Package 39.76 MB 39.75 MB 📉 -8.7 KB (-0.02%)

Test Results

✅ 5809 passed, 18 skipped out of 5827 tests in 937.2s (+5 tests, +35.6s vs. baseline)

Test Coverage

✅ 87.5% line coverage, 80.8% branch coverage · ✅ +0.2% vs. baseline

CLI Startup Time

57ms median (x64, winapp --version) · ✅ no change vs. baseline

Try This Build

Installs 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.

& ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 815
Switching between builds often?

Put the tool on your PATH once:

& ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPath

Then this build is just:

winapp-pr 815

Run winapp-pr with no arguments to pick from a list of open PRs.


Updated 2026-09-11 16:48:32 UTC · commit 4475aeb · workflow run

…antee

Follow-up to review feedback on the injection removal.

Bump CacheVersion 19 -> 20. This is rule 3 in CacheVersion.cs: cleaning logic
changed, so the same upstream input now produces different cached output. Caches
are matched on an exact version string, so without the bump anyone who fetched
with the shipped CLI keeps a "19" cache containing the winapp-authored
ItemsRepeater sample and the rewritten tabview-1 body, and keeps being served
them under the [gallery] tag indefinitely. Re-baking alone never reaches a user
who already has a cache. Corpus re-baked at version 20; the Brotli blobs come out
byte-identical, so the only change is the manifest stamp.

Narrow the provenance wording. The previous text claimed samples are "reproduced
as fetched" and that a result is "exactly what that repository ships", which is
not true: FetchFromGitHub cleans content, compresses C#, truncates both languages
at 2000 chars, and strips event handlers with no emitted code-behind. The skill
also told users to report anything wrong to WinUI-Gallery, so the overstatement
could have sent them upstream to file defects winapp's own extraction introduced.
The honest invariant is narrower and is the one this change actually establishes:
winapp adds no scenarios of its own and applies no bespoke per-sample overrides.
Extraction is still deliberately lossy, uniformly and by rule.

Restore discoverability of the removed sample's guidance. Notes.Get is consulted
only when rendering a selected scenario (SearchEngine.cs:825); notes are never
part of the BM25 document, so moving the image-grid guidance into Notes.cs left
it reachable only to someone who already knew to look at ItemsRepeater. The
sample's header was the only text carrying the token "photo", so its removal made
"photo grid" return Grid and Image and nothing else. Route that vocabulary in
Synonyms instead, which is query-time only and needs no re-bake:

  before: photo grid -> Grid, Image
  after:  photo grid -> GridView, ItemsView, ItemsRepeater

The ItemsRepeater result carries the UniformGridLayout note, so the guidance is
reachable from the query again without a fabricated sample. "thumbnail" already
mapped to Image and now also offers the controls that lay thumbnails out.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 30e85b00-6b59-4df1-b2d7-5642c86c823a
Zach Teutsch (zateutsch) added a commit that referenced this pull request Sep 8, 2026
)

## Description

[#703](#703) asks
WinUI-Gallery and CommunityToolkit to publish machine-readable sample
indexes so `find-ui` stops scraping their repository layouts. Neither
team has ever been asked for this — research found **zero** prior art in
either repo — so appetite is genuinely unknown, and that is the main
risk on the issue.

This is **Phase 0**, which is entirely on our side of the fence. We are
the consumer, so we define the contract first. The point is that each
upstream ask arrives as *"here is the schema, here is the file it
produces, here is the tested code that produced it"* rather than asking
two maintainer teams to design a format for us.

Nothing here changes user-facing behavior.

**What ships:**

| Piece | What it is |
|---|---|
| `docs/winui-sample-index.schema.json` | The published contract. JSON
Schema 2020-12, `schemaVersion` pinned to `const: 1`. |
| `SampleIndexParser` | Generic index → `Scenario[]`. Kind-checks every
element, since this parses untrusted network content. |
| `SampleIndexWriter` | Reference generator core. Deterministic output.
Lives in the **build-only baker**, not the shipped CLI. |
| `ReactorFetcher` | Rewired onto the shared parser: **151 → 67 lines**,
no parsing logic of its own. |
| `--emit-index` | Opt-in flag on the snapshot baker that writes an
index per source. Never committed, never shipped. |

**The vocabulary is deliberately Reactor's.** `microsoft-ui-reactor`
already publishes `reactor-search-index.json`, and this contract
generalizes it rather than competing with it. Field names are the ones
Reactor already ships, which means **Reactor's live index validates
against this schema today with no change on their side** — that is the
proof the contract is adoptable rather than aspirational, and
`ReactorFetcher` reading it through the shared parser is the working
demonstration.

## Usage Example

Not a shipped CLI surface. The generator is opt-in on the build-only
baker:

```bash
# Generate an index per source into a scratch directory (never committed)
dotnet run --project src/winapp-CLI/WinApp.Cli.SnapshotBaker -- --emit-index ./artifacts/indexes
```

Verified live against all three sources. This is a **usable-only**
export: scenarios are sanitized first and any left with neither XAML nor
C# are omitted, so gallery writes 305 of the 323 it fetches. Those 18
are already dropped before search today, so including them would hand
upstream known-broken content to publish. "Lossless" here is a property
of the schema — a scenario that survives is carried field for field —
not a promise that every fetched scenario appears.

| Source | Controls / samples | Size |
|---|---|---|
| gallery | 106 / 305 | 375 KB |
| toolkit | 26 / 48 | 98 KB |
| reactor | 95 / 95 | 83 KB |

## Related Issue

Closes #808 — Phase 0 of #703.

## Type of Change

- ✨ New feature

## Checklist

- [x] New tests added for new functionality — 18 in
`SampleIndexTests.cs`
- [x] Tested locally on Windows
- [ ] Main [README.md](../README.md) updated — n/a, no user-facing
surface
- [ ] [docs/usage.md](../docs/usage.md) updated — n/a, no CLI command
changed; the baker is a build-only tool and, like the existing bake
path, documents itself through its own usage output
- [ ] [Language-specific guides](../docs/guides) updated — n/a
- [ ] [Sample projects updated](../samples) — n/a
- [ ] Shipped skills updated in `plugins/winapp/skills/` — n/a, no CLI
workflow changed

## Additional Notes

### This PR needs the `corpus-rebake-not-needed` label, and here is the
evidence

`find-ui-corpus-check.yml` fires on two changed paths —
`WinApp.Cli.SnapshotBaker/` and `ReactorFetcher.cs` — and this PR
re-bakes nothing. That is correct behavior from the check and the right
question to ask, so it was answered directly rather than waved off:

> The full corpus was baked from `origin/main` and from this branch back
to back, and the decompressed snapshots compared.

```
gallery  main=62016BF907F0  branch=62016BF907F0  720,910 chars  IDENTICAL
toolkit  main=53331F9368D4  branch=53331F9368D4  122,110 chars  IDENTICAL
reactor  main=2D1815A46E13  branch=2D1815A46E13   97,394 chars  IDENTICAL
```

Rewiring `ReactorFetcher` onto the shared parser (−97/+20 lines) is
**behavior-preserving**. Gallery and Toolkit are the control: their
fetchers are untouched and also came out identical, which confirms both
bakes saw the same upstream state, so a reactor difference would have
been attributable to this code rather than to upstream moving mid-test.

Separately and unrelated to this PR: the committed corpus is now behind
upstream (gallery 321 → 323, reactor 93 → 95, baked 2026-08-11). That is
exactly what `find-ui-corpus-drift.yml` exists to report, and it is
deliberately not folded in here.

### Two schema defects that only showed up against real data

Both were found by round-tripping all three **live** corpora, and both
would have shipped as silent quality regressions once a source published
an index.

**1. Curated keywords are a separate ranking signal.** `ProviderData`
carries two dictionaries, not one: `SearchEngine` weights `Keywords` at
**5.0** and `Tags` at **3.0**. Only the Toolkit populates `Keywords`,
from author-written markdown frontmatter. A single `keywords` field
would have folded the Toolkit's best signal down to 3.0 and quietly
re-ranked results.

`keywords` keeps its published meaning (`Tags`, 3.0) because Reactor
already ships that field and re-pointing it would change live rankings
for no reason. The new `curatedKeywords` carries the 5.0 slot. Against
the live Toolkit corpus, **26 of 26** controls populate both — so this
is load-bearing, not hypothetical.

**2. `details` and `xmlnsImports` belong to the sample, not only the
control.** The writer took control metadata from the first scenario in
each group. For `description`, `apiNamespace` and friends that is
correct — they are control facts. For these two it is not: each Toolkit
sample is its own markdown file with its own XAML, so siblings
legitimately disagree. `segmented` has five samples with five
descriptions and differing import counts (2/2/2/2/**3**). Collapsing
them onto the first would have emitted XAML using a prefix the index
never declared.

Both fields are now valid at either level. The writer **hoists them to
the control when every sample agrees** and writes them per sample when
they do not, so the common case stays compact — Gallery, where all
samples share one description, grows 412 bytes; Toolkit grows 5.7 KB;
Reactor not at all. The reader prefers the sample value and falls back
to the control's.

### Two findings about existing data, recorded rather than fixed

- **`gallery/itemsrepeater` has one scenario scraped with no control
metadata at all** — `ApiNamespace`, `ControlDescription`, `Description`,
`RelatedControls` and `Docs` are all empty on `itemsrepeater-7` while
its six siblings have them. Round-tripping *fills* it from the siblings.
That is a repair, not a loss, so the contract asserted in tests is **"a
populated value is never changed or dropped"** rather than strict
equality. Fixing the scrape is out of scope here and moot once Gallery
publishes an index.
- **`gallery-tags.json` holds 13 entries for control ids that have no
scenarios** (`scratchpad`, `listbox`, `wrappanel`, `jumplist-gallery`,
…). `SearchEngine` derives its control list from scenarios, so these are
already dead today. The index cannot carry them and does not need to.

### Design calls worth reviewing

- **`additionalProperties: true` throughout.** Reactor's live index
carries `generatedFrom`, `category` and `galleryRoute`, which this
contract does not model. A closed schema would reject the one real-world
index that already exists, which would be an odd contract to hand
someone.
- **`details` is a new field rather than a redefinition of
`description`.** Reactor's published `description` already means the
one-line summary. Gallery keeps both a short subtitle and a long-form
description, so it needs a second field — taking `description` for the
long form would have broken the one index in the wild.
- **An absent `schemaVersion` is accepted as v1; an unknown one is
refused.** A future v2 index should fail loudly rather than be
half-parsed into wrong results.
- **Determinism is enforced, not incidental.** Controls are sorted by
id, `generatedAtUtc` is omitted by default, and
`JavaScriptEncoder.UnsafeRelaxedJsonEscaping` keeps XAML `<` from
becoming `\u003C`. A generated artifact that churns between runs cannot
be diffed by a drift check and is unreviewable by the maintainers we are
asking to host it.
- **The generator writes on demand and commits nothing.** This follows
the precedent set when #770 merged with only the compressed blobs
checked in. The existing positional bake invocation used by
`build-cli.ps1` and the drift workflow is deliberately byte-identical.

### Review feedback

**The writer moved out of the shipped CLI.** `--emit-index` was always a
baker flag, but `SampleIndexWriter` itself sat in
`WinApp.Cli/Services/Controls/`, so the shipped binary carried code
whose only caller is a build-time tool. It now lives in
`WinApp.Cli.SnapshotBaker/`. The CLI keeps only `SampleIndexSchema` and
`SampleIndexParser` — the parts that survive into a world where upstream
publishes and we only ever *read*. Moving it also took the file out of
the `Services/Controls/` `.editorconfig` subtree, so it now meets the
repo's brace and file-header rules.

**A malformed `language` tag no longer yields C#.** `GetString`
collapsed *absent* and *present-but-not-a-string* into `""`, so
`"language": 42` parsed as C# and would have been offered as paste-ready
code. `IsCSharp` now inspects the `JsonElement` directly and treats a
non-string tag as not-C#. Covered for `42`, `null` and `""`.

**Zach's blocking finding is fixed in #815, not here.** He flagged that
`--emit-index` emits winapp-authored samples presented as Gallery
content, and proposed filtering them at emit time. Investigating it
showed the divergence was not an emit-time problem: `GalleryFetcher` was
injecting and rewriting samples in the *fetch* path and stamping them
`Source = "gallery"`, so they were already reaching users mislabeled,
long before any index was written. Filtering at emit would have hidden
the symptom on the one surface we were about to show upstream. #815
removes the divergence at the source instead, which also makes the
filter unnecessary. This PR is therefore unchanged in that respect and
its byte-identical-corpus evidence below still stands.
### Verification

- **209/209** targeted tests pass (`SampleIndex`, `ReactorFetcher`,
`ScenarioSanitizer`, `Controls`, `FindUi`, `Gallery`, `Toolkit`,
`Search`, `EmbeddedSnapshot`).
- Release build clean — **0 warnings**, warnings-as-errors on.
- A throwaway harness fetched all three sources live and round-tripped
every scenario through writer and reader, asserting scenario-level
equality, no lost live tag, determinism against real upstream data, and
identical sanitizer behavior before and after. It is deleted; its
durable conclusions are three hermetic tests covering per-sample
divergence, control-level hoisting, and reader inheritance.
- `Schema_DeclaresExactlyTheFieldsTheReaderAndWriterUse` asserts
`docs/winui-sample-index.schema.json` and `SampleIndexSchema` cannot
drift apart — the schema file is copied to the test output for that
check.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Zach Teutsch <88554871+zateutsch@users.noreply.github.com>
Copilot-Session: 406ab204-0471-4344-95a7-a91ae84d5a45
Copilot-Session: 30e85b00-6b59-4df1-b2d7-5642c86c823a
@zateutsch

Copy link
Copy Markdown
Contributor

🤖 AI-generated review (winappcli pr-review skill) — verify before acting.

Decision: changes required

The core change — serve WinUI Gallery samples as upstream publishes them and drop the winapp-authored injections/overrides — is well-scoped, cleanly subtractive, and correctly compensated in most places. Two things to address before merge.

Must fix

find-ui "commandbar" advertises a code example but returns nothing

  • What is wrong: Removing InjectMissing dropped the only CommandBar snippet. Upstream's CommandBar has no extractable ControlExample, so the sanitizer strips its code — but the control still appears with a fetchable id and the "Fetch full code" prompt.
  • Show me: winapp find-ui "commandbar" → lists gallery-commandbar-1 + "Fetch full code with: winapp find-ui --id "; then winapp find-ui --id gallery-commandbar-1 → exit 0 with only Important/Family/See-also notes, zero XAML and zero C#.
  • Why it matters: Before this change the same query returned working, pasteable CommandBar code. Now a common control yields a successful-looking result an agent cannot copy — a straight regression to an empty result.
  • Smallest fix (consistent with this PR's thesis): either suppress scenarios that extract to no code from the fetchable results, or move a CommandBar snippet into the attributed Notes/core-pattern mechanism (as done for the ItemsRepeater guidance) rather than back into the gallery corpus.
  • Location: src/winapp-CLI/WinApp.Cli/Services/Controls/GalleryFetcher.cs (removed InjectMissing).

Non-blocking

"image grid" doesn't reach the new collection-layout guidance

  • What is wrong: The compensating synonyms cover photo/gallery/thumbnail/uniformgrid but not the phrase "image grid" — which is literally the header of the deleted sample ("image grid (UniformGridLayout)").
  • Show me: winapp find-ui "image grid" → only Image (nine-grid images, basic image); winapp find-ui "photo grid" → correctly routes to GridView/ItemsView.
  • Why it matters: The most natural phrasing for the exact use case the new note was written for lands on single-image samples instead.
  • Smallest fix: Add image grid / image gallery routing to itemsrepeater/gridview/itemsview in Synonyms.cs.
  • Location: src/winapp-CLI/WinApp.Cli/Services/Controls/Synonyms.cs

gallery-tabview-1 now emits upstream C# that won't compile as-is

  • What is wrong: Dropping ApplyOverrides means the served TabView sample keeps upstream's frame.Navigate(typeof(YourPage)) with an undefined YourPage. That's upstream's own placeholder (deliberately deferred to now), but the new SKILL.md "compiles when pasted" phrasing oversells it.
  • Smallest fix: Scope the compile claim to stripped handlers, or note that some samples reference page types the user must define.
  • Location: plugins/winapp/skills/winapp-find-ui/SKILL.md

What was exercised

  • dotnet build WinApp.Cli.csproj -c Release — succeeded, 0 warnings / 0 errors.
  • Ran the built binary directly: reproduced the empty-code CommandBar result and the "image grid" vs "photo grid" phrasing gap.
  • Cache-version bump (19→20) and re-baked snapshot manifest verified consistent. No security-relevant path changed (fetch stays HTTPS from a Microsoft host).
  • Not run independently: full MSTest suite. The new GalleryCorpus_CarriesNoWinappAuthoredSample test asserts one exact header is absent, so it guards the specific removed sample rather than the general invariant — acceptable but narrow.

@zateutsch Zach Teutsch (zateutsch) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review attached above. Looks like there's a dead sample with the changes.

Upstream's CommandBar sample is a doc-style elided fragment (a literal ... inside
the markup), so it fails structural validation and find-ui emits no code for it.
Put the toolbar shape in Notes.cs as winapp-attributed guidance instead of
re-adding a winapp-authored sample to Gallery's corpus, and route the natural
'image grid' phrasing to the collection controls whose notes carry the
UniformGridLayout pattern.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@nmetulev Nikola Metulev (nmetulev) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not have any authored content int he winapp cli - if content needs to exists, it needs to be pushed upstream to the gallery instead of adding it to winapp - let's use this pr to remove all winapp authored content and only leverage upstream content

github-actions Bot and others added 2 commits September 11, 2026 04:46
…kens

WinUI Gallery samples declare optional attributes and injected markup as
$(Name) tokens that the Gallery app expands at runtime. GalleryFetcher
flattened every token to "..." regardless of where it sat, which produced
invalid markup for two of the three positions:

  <Button Content="Go" Click="Button_Click" $(IsEnabled)/>
    -> Click="Button_Click" .../>   (not well-formed)

ScenarioSanitizer then dropped the XAML, so ten controls advertised a
fetchable id and returned a successful-looking result with no code in it:
Button, ToggleButton, RepeatButton, HyperlinkButton, ProgressRing (x2),
CommandBar, AnimatedIcon, PersonPicture and EasingFunction.

NormalizeMarkupSubstitutions now resolves tokens by position: an
attribute-position token is removed (it stands in for a whole attribute),
an element-content token becomes a comment (every one upstream ships
injects markup into a property element, never text, so flattening assigned
the literal string "..." as a collection's content), and a value-position
token keeps its existing cosmetic flattening. Comments are copied verbatim
so a '>' inside one cannot desynchronize the scanner.

This also removes the need for the winapp-authored CommandBar shape and
ItemsRepeater image-grid notes added earlier on this branch: the CommandBar
gap was this bug, not a missing upstream sample. Per review, winapp carries
no authored sample content — gaps belong upstream and extraction defects
belong in the fetcher.

The image-grid synonyms are kept. Upstream's curated keywords already tag
ItemsRepeater/GridView/ItemsView with image, gallery and grid, but "Image"
and "Grid" are controls in their own right and an exact control-name match
outranks those tags, so "image grid" reached only Image. The routing is
query vocabulary, not served content, and it reaches upstream's samples.

Cache version 20 -> 21 and a re-bake: same input, different output, so an
existing cache would keep serving the empty scenarios.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: cbc27c5a-bc96-486a-8b99-1d533aa68f15
@Jaylyn-Barbee Jaylyn Barbee (Jaylyn-Barbee) changed the title fix(find-ui): serve WinUI Gallery samples exactly as upstream publishes them fix(find-ui): Gallery corpus is upstream-only; ten controls that returned no code now do Sep 11, 2026
- Keep queries **focused** (one feature per query) — the lexical ranker rewards
specific phrasing. Batch multiple focused queries rather than one broad one.

## Upstream is the source of truth

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this in the skill? it sounds like internal mechanics instead of agent guidance

@zateutsch Zach Teutsch (zateutsch) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All clean, lgtm

@Jaylyn-Barbee
Jaylyn Barbee (Jaylyn-Barbee) merged commit a1325be into main Sep 11, 2026
31 checks passed
@Jaylyn-Barbee
Jaylyn Barbee (Jaylyn-Barbee) deleted the jay/find-ui-upstream-truth branch September 11, 2026 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants