microsoft / microsoft/microsoft-ui-reactor
[Bug] Search-index generator silently falls through to the next SampleCard when a snippet *comment* trips the placeholder regex — byte-compare gate can't catch it
- Dominant language
- C#
- Stars
- 646
- Forks
- 54
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 84
Description
## Summary
`SearchIndexGenerator.FirstQualifyingSample` picks the first `SampleCard` whose snippet has real code and no placeholder. When a card is disqualified it `continue`s to the **next card, silently**. Because `HasPlaceholder` runs over the **entire snippet text including `//` comment lines**, a card can be disqualified by prose rather than by an actual abbreviation — and the control's index entry then advertises a *different* sample than the one the page leads with.
The `Index_IsUpToDate` byte-compare cannot detect this: the index is regenerated by the same generator, so a wrong-but-self-consistent entry compares clean against itself. **The gate fires on *stale*; it cannot fire on *wrong*.**
## Mechanism
`tools/Reactor.SearchIndex/SearchIndexGenerator.cs`
```csharp
:278 static ExtractedSample? FirstQualifyingSample(ClassDeclarationSyntax cls)
:280 foreach (var inv in cls.DescendantNodes().OfType())
...
:292 if (!HasRealCode(normalized) || HasPlaceholder(normalized)) continue; // <-- silent
:294 return new ExtractedSample(header.Trim(), normalized);
```
`HasRealCode` (:336) only requires that *some* line is non-comment, so a snippet with one real code line plus a prose comment passes it. `HasPlaceholder` (:348-351) then matches over the whole text:
```
,\s*\.\.\.|\.\.\.\s*\)|\(\s*\.\.\.|^\s*\.\.\.\s*$|\.\.\./| setSelectedIndex(i)).Header("Colors")` | True | False | QUALIFIES |
| `// Pick a colour, ... then bind`
+ the same line | True | **True** | **SKIPPED → falls through** |
| `// see Dsl.cs (...)`
+ the same line | True | **True** | **SKIPPED → falls through** |
| `ComboBox(colors, null, i => set(i)).PlaceholderText("Type here...")` | True | False | QUALIFIES |
| `TextBlock("Loading, ...")` | True | **True** | **SKIPPED → falls through** |
| `TextBlock("Please wait (...)")` | True | **True** | **SKIPPED → falls through** |
| `Button("More...", OnMore)` | True | False | QUALIFIES |
**The regex is context-free: it matches inside `//` comments *and* inside string literals.** The comment at `:345` claims the latter is exempt —
> *"Ellipses inside UI strings (`"Type here..."`) are NOT matched, so those snippets still qualify."*
— but that exemption is **accidental adjacency, not context-awareness**. `"Type here..."` survives only because its ellipsis is followed by `"` rather than by `)`, `,`, or `/`. Move the ellipsis and the same string is rejected: rows 5 and 6 above are ordinary display text with no abbreviation at all, and both disqualify their card. So the sentence at `:345` is true of the one example it names and false of the class it generalises to.
That matters for the fix, not just for the comment: **stripping `//` lines alone would leave the string-literal false positive standing.**
## Why existing tests don't cover it
`SearchIndexGeneratorTests` has exactly one fall-through-adjacent assertion:
```csharp
:290 public void SampleOverride_ReplacesRejectedPlaceholderCard()
:294 Assert.Equal("Interactive map", mc.Samples[0].Header);
```
That pins the single **intended** case (`map-control`, whose sole card genuinely abbreviates and is rescued by an editorial `sampleOverride`). There is no general assertion that a control's `samples[0].Header` equals the first `SampleCard` header in its page source, so an **unintended** fall-through is ungated.
## Suggested fix
Either is cheap; the first is the smaller change:
1. **Make the placeholder test context-aware.** Run it over code lines only (`HasRealCode` at `:336` already knows how to identify them) **and** with string-literal contents masked out. Either half alone leaves the other false positive standing. Keeps the abbreviation invariant — `.Click(...)` still rejects — while removing sensitivity to prose and to display text.
2. **Add the missing gate**: assert `samples[0].Header` == first `SampleCard` header in source order, with an explicit allow-list for controls that legitimately fall through (`map-control` today). This turns any future silent fall-through into a build failure and would also have caught this class.
Ideally both — (1) fixes the trigger, (2) makes the next trigger loud.
## Notes
- **No current gallery page is affected — now verified exhaustively, not spot-checked.** Ran the suggested gate (2) by hand over all 93 controls: extract each page''s ordered `SampleCard` headers, compare against the index''s `samples[0].header`. 84 match at position 0; 7 diverge; 2 headers come from editorial overrides (`split-view`, `type-ramp`, both confirmed in `editorial.json`). **All 7 divergences are legitimate**, and they fall into three distinct causes:
| control | cause | line |
|---|---|---|
| `card`, `expander`, `grid`, `list-view`, `tree-view` | genuine `, ...` abbreviation **in code** | `:292` `HasPlaceholder` |
| `spacing` | `sourceCode` is a single `//` line (`"// 4px base unit: 0, 2, 4, ..."`) | `:292` `HasRealCode` — the case the `:333` comment documents |
| `typography` | **`sourceCode` is not a literal** — `string.Join("\n", TypeRamp.Select(...))` | `:289` `code is null` |
- **The third cause is a silent-continue path this issue originally missed.** `FindSourceCodeArgument` → `TryGetStringLiteral` returns `null` for any computed expression, and `:289` `continue`s exactly like the other two. Any allow-list accompanying suggested fix (2) has to cover all three causes, or it will flag `typography` as a false positive on day one.
- Raising this now because **five open PRs carry a regenerated `reactor-search-index.json`** (#1003, #1004, #1005, #1006, #1009). A regeneration that encodes a fall-through would pass CI silently.
- Related shape: #1019 (a snippet class with no gate).
- **Independently re-derived.** The session on #1009 reached suggested fix (2) — pin `samples[0].Header` to the page's first `SampleCard` header — from a different starting point: that `Index_IsUpToDate` regenerates *both* sides of its comparison with the same generator, so a mis-picked card produces a file that agrees with itself perfectly and the byte-compare is structurally incapable of noticing. Same gate, different failure story; recording the convergence here so it lives in one place.
Found while verifying #1006 against issue #981.
Contributor guide
Research direction
Start in tools/Reactor.SearchIndex/SearchIndexGenerator.cs at FirstQualifyingSample, HasRealCode, and HasPlaceholder, then read SearchIndexGeneratorTests, especially SampleOverride_ReplacesRejectedPlaceholderCard. Reproduce the comment and string-literal cases from the issue and inspect how Index_IsUpToDate regenerates the index. Done means the intended sample remains selected, false positives are covered by tests, and the missing first-sample mismatch is made visible.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- build-system, testing-qa, tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 65/100