Skip to content

Add XAML localization resource regression coverage#534

Open
shanselman wants to merge 1 commit into
masterfrom
shanselman/xaml-resource-smoke-tests
Open

Add XAML localization resource regression coverage#534
shanselman wants to merge 1 commit into
masterfrom
shanselman/xaml-resource-smoke-tests

Conversation

@shanselman
Copy link
Copy Markdown
Contributor

Summary

  • Add static XAML localization validation for unsupported resource properties on x:Uid elements.
  • Catch the AgentEventsPage TextBlock/Content crash class by failing if a TextBlock x:Uid is paired with a .Content resource.
  • Include the one-line AgentEventsPage XAML fix needed for the new master-based regression test to pass.

Validation

  • dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --filter FullyQualifiedName~LocalizationValidationTests.XamlControlsWithXUid_DoNotUseUnsupportedLocalizationProperties
  • .\build.ps1
  • dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore
  • dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore

Base: master.

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

clawsweeper Bot commented May 25, 2026

Codex review: needs real behavior proof before merge. Reviewed May 24, 2026, 8:28 PM ET / 00:28 UTC.

Summary
The PR adds a Tray test that rejects unsupported resource properties on x:Uid elements and removes x:Uid from the AgentEventsPage Clear TextBlock.

Reproducibility: yes. from source inspection: current main binds x:Uid="ClearButton" to a TextBlock while the resource files include ClearButton.Content and ClearButton.Text. I did not run the Windows UI, but the code/resource combination and the PR's proposed test make the crash class clear.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Preserve localization for the Clear label with a supported, non-conflicting x:Uid/resource mapping in every locale.
  • Add after-fix proof, such as terminal/live output or a screenshot/recording showing the Agent Events page behavior with private details redacted.

Proof guidance:
Needs real behavior proof before merge: The PR body lists validation commands but does not include after-fix runtime output, logs, a screenshot, or a recording showing the localization crash is gone; proof should redact private details. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • Merging as-is would make the Agent Events Clear label hard-coded, so non-English users lose an already-present localized string even though the new test passes.
  • The stale ClearButton.Content resources remain in every locale, so the branch avoids the crash by removing the binding rather than proving the visible label still has a supported localization key.
  • The PR lacks after-fix real behavior proof; the body lists commands but does not show runtime output, logs, or a screenshot/recording of the fixed behavior.

Maintainer options:

  1. Preserve the localized Clear label before merge (recommended)
    Update the XAML/resource mapping so the TextBlock uses a supported .Text resource without sharing the stale ClearButton.Content key, then re-check localization coverage.
  2. Accept an English-only Clear label
    Maintainers could intentionally accept the hard-coded fallback, but that would contradict the current localization audit policy for static XAML text.
  3. Pause until proof is added
    If the branch is not updated with both the resource fix and after-fix proof, leave it open for contributor follow-up rather than landing the partial workaround.

Next step before merge
Human follow-up is needed because the branch must preserve localization and add real behavior proof before normal merge review can continue.

Security
Cleared: The diff only changes Tray XAML and a test file; I found no concrete security or supply-chain concern.

Review findings

  • [P2] Keep the Clear label localized — src/OpenClaw.Tray.WinUI/Pages/AgentEventsPage.xaml:55
Review details

Best possible solution:

Keep the regression test, but repair the XAML/resources so the visible Clear label still has a non-conflicting x:Uid with matching .Text resources in every locale.

Do we have a high-confidence way to reproduce the issue?

Yes from source inspection: current main binds x:Uid="ClearButton" to a TextBlock while the resource files include ClearButton.Content and ClearButton.Text. I did not run the Windows UI, but the code/resource combination and the PR's proposed test make the crash class clear.

Is this the best way to solve the issue?

No. The added coverage is useful, but the XAML fix should preserve localization with a supported resource key instead of dropping x:Uid from visible static text.

Full review comments:

  • [P2] Keep the Clear label localized — src/OpenClaw.Tray.WinUI/Pages/AgentEventsPage.xaml:55
    Dropping x:Uid from this TextBlock sidesteps the ClearButton.Content crash, but it also leaves the visible Clear text hard-coded even though locale resources already contain ClearButton.Text translations and the localization docs require x:Uid for static XAML text. Give the label a non-conflicting x:Uid with .Text resources, or otherwise clean up the resource keys, so the new regression test passes without losing localization.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.89

Codex review notes: model gpt-5.5, reasoning high; reviewed against ef6ac8acbab2.

Label changes

Label changes:

  • add P2: This is a normal-priority localization regression fix and test coverage improvement with a limited Tray UI surface.
  • add merge-risk: 🚨 compatibility: The PR removes x:Uid from an existing localized static label, which can regress localized UI behavior for existing non-English users.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists validation commands but does not include after-fix runtime output, logs, a screenshot, or a recording showing the localization crash is gone; proof should redact private details. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: This is a normal-priority localization regression fix and test coverage improvement with a limited Tray UI surface.
  • merge-risk: 🚨 compatibility: The PR removes x:Uid from an existing localized static label, which can regress localized UI behavior for existing non-English users.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists validation commands but does not include after-fix runtime output, logs, a screenshot, or a recording showing the localization crash is gone; proof should redact private details. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Repository policy inspected: AGENTS.md requires build and test validation after changes; this review stayed read-only, so no validation commands were run locally. (AGENTS.md:1, ef6ac8acbab2)
  • Current main has the mismatch the PR targets: AgentEventsPage currently has a TextBlock with x:Uid="ClearButton" and Text="Clear", so resource keys with the ClearButton prefix apply to that TextBlock. (src/OpenClaw.Tray.WinUI/Pages/AgentEventsPage.xaml:55, ef6ac8acbab2)
  • Current resources include both supported and unsupported ClearButton keys: The en-us and fr-fr resource files include ClearButton.Content and ClearButton.Text, which explains why the TextBlock x:Uid can encounter an unsupported Content resource while a localized Text value also exists. (src/OpenClaw.Tray.WinUI/Strings/en-us/Resources.resw:1593, ef6ac8acbab2)
  • PR diff removes localization from the visible label: The branch changes the Clear TextBlock from x:Uid="ClearButton" Text="Clear" to Text="Clear", which avoids the unsupported resource but leaves the static text outside the x:Uid localization path. (src/OpenClaw.Tray.WinUI/Pages/AgentEventsPage.xaml:55, 0a0d4745d106)
  • PR diff adds focused static coverage: The branch adds XamlControlsWithXUid_DoNotUseUnsupportedLocalizationProperties, which checks resource keys for supported localizable properties on known x:Uid element types. (tests/OpenClaw.Tray.Tests/LocalizationValidationTests.cs:372, 0a0d4745d106)
  • Localization docs require x:Uid for static UI text: docs/LOCALIZATION_CHECK.md says XAML uses x:Uid for static UI text and instructs contributors to add x:Uid to the element that owns visible text. (docs/LOCALIZATION_CHECK.md:3, ef6ac8acbab2)

Likely related people:

  • shanselman: Current-main history shows Scott Hanselman introduced the localization validation test and later adjusted the Agent Events Clear button localization path. (role: recent localization validation contributor; confidence: high; commits: bcdb4bce377b, 05d8a3929d6a; files: tests/OpenClaw.Tray.Tests/LocalizationValidationTests.cs, src/OpenClaw.Tray.WinUI/Pages/AgentEventsPage.xaml, src/OpenClaw.Tray.WinUI/Strings/en-us/Resources.resw)
  • bkudiess: The merged Agent Events redesign PR introduced the toolbar Clear button/resource shape that this PR is repairing, and the same author also touched the broader localization pass. (role: recent Agent Events UI contributor; confidence: high; commits: 89aee64058e5, d4b35d4941c0; files: src/OpenClaw.Tray.WinUI/Pages/AgentEventsPage.xaml, src/OpenClaw.Tray.WinUI/Strings/en-us/Resources.resw)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels May 25, 2026
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 25, 2026

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature or rarity is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant