Repository navigation
fix: prevent sidebar toggle icon clipping at display scales - #1690
Conversation
Keep the native WinUI subtle-button style while removing text-button padding from Workspace and Settings pane toggles. Add native layout and markup regression coverage and document the icon-only padding invariant. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3439bb57-3a0e-4b13-b579-25edeafbf01d
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P3
|
Codex review: needs maintainer review before merge. What this changesThe PR removes text-button padding from Workspace and Settings sidebar toggles so their icons have room inside the existing targets. Example: A user opens Workspace at 125% display scaling.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review This PR remains useful: current main retains the cramped button layout, and the patch preserves native interaction behavior without introducing a concrete defect. Collaborator-authored work also falls under the keep-open guard. Priority: P3 Before mergeNone. FindingsNone. Tests
Agent review detailsHow this fits togetherWorkspace hosts native chat navigation, while Settings provides companion configuration pages. Their sidebar buttons open and close navigation panes without replacing the displayed content. flowchart TD
A[User clicks sidebar toggle] --> B{Window}
B --> C[Workspace toggle]
B --> D[Settings toggle]
C --> E[Native button with icon clearance]
D --> E
E --> F[Navigation pane opens or closes]
Technical reviewBest possible solution: Keep the narrow native-button repair without adding a custom template or changing pane ownership. Do we have a high-confidence way to reproduce the issue? Yes for insufficient layout clearance: the supplied native baseline run reports content width 16 for a 16-DIP glyph at raster scale 1.25. Pixel-level clipping itself was not independently reproduced. Is this the best way to solve the issue? Yes. Local padding overrides address the constrained content area while retaining the native template, target size, focus behavior, and existing pane handlers. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 037c17dcb581. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 9, 2026, 2:32 PM ET / 18:32 UTC. |
|
Global triage: HOLD_FOR_AUTHOR. Take confidence 10%; recommendation confidence 99%; effort small; risk high. Reviewed exact head Author: update from current main, resolve conflicts or branch-policy blockers, then rerun exact-head validation and request review. This is a triage classification, not a merge action. No branch changes were made. |
karkarl
added
the
status: 🚢 actively landing
|
CI investigation: the rerun is green on the unchanged head The original UI job stopped at Reran the failed jobs with
Locally, the concurrent Markdown and Workspace layout selection also passed in six fresh ARM64 test hosts (19 cases per run). No timeout increases, skipped assertions, test retries in code, or workflow changes were added. Working tree remains clean. |
karkarl
removed
the
status: 🚢 actively landing
What Problem This Solves
Fixes: the sidebar toggle icon can appear cropped when the app is viewed at a different display scale or resolution.
User Impact
User impact: Workspace collapse/reopen and Settings pane toggles have room for their complete icons without changing their 40-DIP targets or native interaction states.
Why This Change Was Made
These buttons already use WinUI's built-in
SubtleButtonStyle, not a custom template. Its default text-button padding leaves only 16 DIPs for a 16-DIP icon inside the fixed-width target. SetPadding="0"on the three icon-only toggles, keeping the native template, theme resources, focus handling, and pane behavior. Add a markup guard and a mounted native layout test for glyph fit and rounding clearance.Evidence
The three new native layout cases pass with the fix and fail the rounding-clearance assertion against the original XAML at 125% scaling. Seven focused layout cases and four real-app light/dark sidebar and Settings checks passed. Full required validation passed on the final rerun; two earlier queued-chat timing failures were outside this change.
Direct review covered the small diff. A separate rubber-duck review was not used for this three-setter layout fix; no services, navigation logic, or templates change.
Change Type
Scope
winnodeRequired proof pools
windows-winui-interactive: verify visible sidebar toggle glyphs and existing pane interactions in the isolated WinUI app.windows-11-arm64: native ARM64 build, launch, and WinUI layout proof on the validation host.Validation
Run on Windows ARM64 with
OPENCLAW_REPO_ROOTset to this worktree and an isolatedOPENCLAW_TRAY_DATA_DIRfor the required suites:.\build.ps1: passed, all five production projects. Repeated successfully for the manual preview.dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore --verbosity quiet: passed, 4,262 passed, 35 skipped, 0 failed.dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore --verbosity minimal: passed, 3,996 passed, 0 failed.dotnet test .\tests\OpenClaw.Tray.UITests\OpenClaw.Tray.UITests.csproj -r win-arm64 --no-restore -p:BuildProjectReferences=false --filter 'FullyQualifiedName~WorkspaceSessionLayoutTests' --verbosity normal: passed, 7 passed, 0 skipped or failed, using the already-built production references.dotnet test .\tests\OpenClaw.Tray.UITests\OpenClaw.Tray.UITests.csproj -r win-arm64 --no-build --no-restore --filter 'FullyQualifiedName~SidebarActionBackplates_AlignWithNavigationItems|FullyQualifiedName~SettingsChrome_SearchAndBidirectionalHistory_StayBelowNativeTitle' --verbosity normal: passed, 4 passed, 0 skipped or failed.dotnet test .\tests\OpenClaw.Tray.UITests\OpenClaw.Tray.UITests.csproj -r win-arm64 --no-build --no-restore --filter 'FullyQualifiedName~PaneToggle_LeavesRoundingRoomAroundUnconstrainedGlyph' --verbosity normal, withOPENCLAW_REPO_ROOTpointing to an exported copy of the original window markup: 3 expected failures on insufficient rounding clearance.Real Behavior Proof
2bcb1b5d53354ef8ba071479a8d712fe6c7836c2; validation was collected before committing..\run-app-local.ps1 -NoBuild -Isolated -AllowNonMain -Waitfor manual inspection; its new profile opened Setup, which is not itself sidebar proof.Glyph width 16 has no rounding room in content width 16 at raster scale 1.25.Yes/No/N/A): N/A. Screenshots and logs remain local; relevant output is copied above.Security Impact
Yes/No): NoYes/No): NoYes/No): NoYes/No): NoYes/No): NoYes, explain the risk and mitigation: N/ACompatibility and Migration
Yes/No): YesYes/No): NoYes/No): NoReview Conversations