Escape the names the package's inspectors print - #192
Conversation
Why: a theme, font, or command name carrying a bidi override reached the custom inspectors unescaped, so a name read one way in a tooltip and another way in the value the index selected. What changed: - Escape theme, font, command, and asset-path names in all three editors. - Popups show the escaped copy; the index still selects the raw name. - Pin the tooltips with a source gate; no CI lane compiles the editors. How we know: the gate names all five unescaped tooltips on the old sources and passes on the new.
Why: most shipped fonts are Latin-only, and the guide did not say a character the selected font lacks shows a missing-glyph box. What changed: - One paragraph in the theme guide, with the two exceptions measured.
…free Why: the docs claimed no shipped font carries emoji and named two fonts by a name the product never shows; the popup backstop was satisfied by a mention in a comment; the same rule was stated six times. What changed: - Name the asset names and the packs, and bound the emoji claim. - Read the popup backstop from code, so a TODO does not satisfy it. - State the rule once, and say what the gate does not cover. - Cover the null and empty starting copies the callers use.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dc524d2. Configure here.
…ound Why: two of the sanitizer test's assertions could not have run - one would not compile without a using, one compared the wrong array - and the gate's comment removal walked its spans out of order, so a TODO still satisfied the popup backstop. What changed: - Add the using, and assert the literals the null copy must hold. - Order the removed spans, and interleave the control that tests it. - State the rule once, and list what the gate does not see. - Name the shipped fonts as list-fonts prints them, with their packs.
| */ | ||
| public static void SanitizeInto(string[] names, ref string[] copy) | ||
| { | ||
| if (copy == null || copy.Length != names.Length) |
There was a problem hiding this comment.
Can we SSOT length, here and anywhere else?
There was a problem hiding this comment.
DISCLOSURE: LLM-GENERATED TEXT
Done, and swept rather than just patched.
SanitizeInto reads names.Length once now; it sizes the allocation and the loop. The loop writes copy, never names, which is the condition that makes a single read correct here.
I swept every method in Runtime/, Editor/, Tests/, Samples~, and Generator~ for the same shape - one collection's Length/Count read three or more times. Twenty-two methods matched. Nine were genuinely single-source and are now hoisted: BuildUsageHint, Next, Previous, TryEatArgument, KebabCase, RefreshLogs, TryTokenComplete, Ensure, and CatalogEmitter.Emit.
Six I deliberately left, because a single read there would be a bug, not a cleanup:
CyclicBuffer.Resizeand theFix Invalid Fontsloop read the count of the collection they are removing from. The inline re-read is what terminates them.KebabCase's trailingwhileshrinksbuilder.Lengthin its own body.TerminalThemeStyleSheetHelper.GetAvailableThemesreassignsussContentbetween its reads, so the three reads are three different values.TextFieldPaste.TryApply's second read is the post-trim value by design.CollapseCaseInsensitiveDuplicatesalready documents the outer-loop case.
The test sites are also left alone, and one of them is the reason I would not sweep tests mechanically: CountReflectsNumberOfEntries reads history.Count after every Push, and that repetition is the assertion.
One extra find: AbsoluteToUnityRelativePath had a second StartsWith block with the identical condition directly after the first, which always returns. It was unreachable, and it was also wrong - it prefixed Assets/ onto a slice that already begins with Assets/. Deleted; rule 5.
The rule and all three traps are now written down in the hot-path-allocations skill, with the sweep script's logic as the worked example of which sites to leave alone.
There was a problem hiding this comment.
DISCLOSURE: LLM-GENERATED TEXT
Follow-up, because the first reply was not good enough.
I answered "here and anywhere else" with one sweep, quoted the tally, and stopped. A second, independent pass of the same rule found eleven more hoistable sites, one of them 65 lines below a site I had already hoisted in the same file, and two of my classifications were simply wrong:
CollapseCaseInsensitiveDuplicatesI excused because an existing comment said its outerCountread must stay inline - the loop "rewrites entries". An element overwrite cannot change a count. The comment was wrong, I had cited it as a correct example in the skill, and I believed it rather than checking. Hoisted, and the comment now states the real rule.GetAvailableThemesI classified as "reassigned between the reads". All three reads are after the last assignment. Hoistable all along.
So the rule I wrote into the skill was too broad in one direction (it forbade a legal hoist for any element write) and my evidence for completeness was a number I could not defend. Both are fixed: rule 18 and the skill now say the body must change that collection's count, and the skill no longer quotes a tally - a number in a skill file is a claim about the codebase that nobody can audit from the file, and mine did not survive one independent pass.
The guidance now carries the lesson rather than just the rule: treat an inherited justification as a claim to verify, not a fact.
Verification for the second pass: the completion collapse is byte-identical old vs new across twelve corpora built to hit the dedupe (case-variant runs, adjacent duplicates, single entries, non-letters), and the same differential across the other six files is identical. Generator~ was untouched, so the payload still matches.
Why: the sanitizer read names.Length three times, and a $@ tooltip hole was never reported because the check read two characters back from the @. What changed: - Read each count once, in the eight production methods where one value answers every read; leave the six where the loop mutates the collection or the variable is reassigned, and say so at each. - Drop the unreachable second StartsWith block in AbsoluteToUnityRelativePath. - Fix the $@ detection and add a control that fails without the fix.
|
DISCLOSURE: LLM-GENERATED TEXT All three reviewed; two were fixed in
|
Why: the count rule lived only as "never hoist a mutated collection's length", so the safe direction had no guidance and the drift half of it was invisible. What changed: - State one read per method as a drift rule, not a speed one, in hot-path-allocations and in context.md rule 18. - Name the four reasons a second read is a different number, each with the in-repo site that proves it. - Record that the test sites are excluded, and which one breaks if hoisted.
Why: CatalogEmitter gained the hoisted count, so the committed DLL no longer matched its sources and the two-checkout gate failed. How we know: verify-analyzer-payload.ps1 passes for both payloads.
Why: the first sweep hoisted 9 of 22 sites and called the rest traps. A second pass found 11 more hoistable, one 65 lines below a site already hoisted, and one excused by an inherited comment that was itself false. What changed: - Hoist the 11: RunTyped, RouteInvocation, AddArgument, the completion dedupe, the bake loops, RefreshRows, the palette caret, the property path walk, GetAvailableThemes, and the collapse's outer bound. - An element overwrite is not a change of count, so the collapse's "must stay inline" comment was wrong; the loop is now hoisted. - Rule 18 and the skill say "changes that collection's count", and the skill no longer quotes a tally it cannot defend.
1 participant
DISCLOSURE: LLM-GENERATED TEXT
Why:
A theme, font, or command name carrying a bidirectional override reached the package's own inspectors unescaped, so a name read one way in a tooltip or dropdown and another way in the value it selected (#190).
What:
SetTheme,SetFont, and_disabledCommandsare unchanged.How we know:
The gate names all five unescaped tooltips on
2dc1e31and passes on this head; six mutation probes and six scanner tests prove it can still fail.The EditMode sanitizer suite runs here under NUnitLite: 11 of 15 methods pass, and the four that need a live
UnityEngine.Objectare untouched by this change.Note
Low Risk
Display-only escaping in editors with raw values preserved for actions; collateral count-hoisting refactors are mechanical and documented, with low user-visible risk outside inspector labels.
Overview
Inspector display text now matches the console’s escaping rules: theme, font, command, and asset-path names in
TerminalUIEditor,TerminalFontPackEditor, andTerminalThemePackEditorgo throughLogTextSanitizerfor tooltips andEditorGUILayout.Popuplabels, while indices and stored values still use the raw names (SetTheme,SetFont, disabled commands unchanged).LogTextSanitizer.SanitizeIntofills a separate label array in place so per-frame inspector redraws do not allocate a new string array every time. A Node contract suite (tooling~/scripts/tests/display-text.test.mjs) enforces sanitized holes inGUIContentinterpolations and that popup-heavy editor files reference the sanitizer, since Editor assemblies are not compiled in CI.User-facing notes:
CHANGELOG.mdrecords the inspector fix;Documentation~/theming.mddocuments which shipped fonts cover CJK/Hangul and that emoji usually show as missing-glyph boxes.Collateral (same PR): LLM guidance in
.llm/context.mdandhot-path-allocationstightens loop-bound rules (hoist only when count does not change; one count read per method). A repo-wide pass applies that pattern in runtime, editor, and generator code (including hoistingCollapseCaseInsensitiveDuplicates’ outer loop).DirectoryHelperdrops a duplicated path branch.Reviewed by Cursor Bugbot for commit 774cbb1. Bugbot is set up for automated code reviews on this repo. Configure here.