Skip to content

Fix mcp permissions render clipping (#1424)#1425

Merged
Aaronontheweb merged 9 commits into
netclaw-dev:devfrom
Aaronontheweb:worktree-fix+1424-mcp-permissions-clipping
Jun 17, 2026
Merged

Fix mcp permissions render clipping (#1424)#1425
Aaronontheweb merged 9 commits into
netclaw-dev:devfrom
Aaronontheweb:worktree-fix+1424-mcp-permissions-clipping

Conversation

@Aaronontheweb

@Aaronontheweb Aaronontheweb commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Root cause (issue netclaw mcp permissions render bug on 0.24.0-beta.5 #1424): ScrollableContainerNode.Render() in Termina 0.12.1 ignores bounds.Y, always writing at context (0,0). When the tool list was tall enough, the scroll container overwrote the page header rows.
  • Fix: Wrap _toolScrollNode in a borderless PanelNode. PanelNode.Render() calls context.CreateSubContext(bounds) before delegating to its content, so the scroll container now receives a properly-offset render context.
  • Footer wrapping fix: Added .NoWrap() + .WidthAuto() to footer TextNodes so they never expand to Fill-width, which caused hint text to wrap on narrower terminals.
  • Regression test: McpToolPermissionsPageTests.ToolGrid_ManyTools_HeaderRowsNotOverwrittenByScrollContent — 15 tools on a 40-row VirtualTerminal, asserts header rows and tool rows are all visible simultaneously.
  • Code-review follow-ups: Stale ScrollOffset reset when MaxScroll drops to 0; .NoWrap() applied to all footer paths; extracted BuildToolGridFooterWithStatus helper.
  • VHS screenshot smoke tape: Added tests/smoke/tapes/screenshots/mcp-permissions.tape and registered it in run-smoke.sh. The approved baseline will be committed after the first CI screenshots run.

Test plan

  • Unit tests: dotnet test --filter McpToolPermissionsPageTests → 13/13 pass
  • Slopwatch: dotnet slopwatch analyze → 0 violations
  • Copyright headers: ./scripts/Add-FileHeaders.ps1 -Verify → all files have headers
  • Smoke flow tape: ./scripts/smoke/run-smoke.sh mcp-permissions passes
  • Screenshots CI run generates mcp-permissions-loading.actual.png → human reviews → commits as tests/smoke/screenshots/mcp-permissions-loading.approved.png

ScrollableContainerNode.Render in Termina 0.12.1 ignores bounds.Y
and writes relative to context (0,0). When hosted directly in a
VerticalLayout the scroll container always writes starting at terminal
row 0, overwriting the header and server-info rows above it.

Fix: wrap the scroll container in a borderless PanelNode. PanelNode.Render
calls context.CreateSubContext(bounds) before passing the context down,
so the inner context is already offset to the correct screen position.

Secondary fix: footer hint TextNodes now use .NoWrap() + .WidthAuto()
on the status indicator so the hints line stays on one row instead of
wrapping when the HorizontalLayout divides width equally.

Regression test added: ToolGrid_ManyTools_HeaderRowsNotOverwrittenByScrollContent
uses 15 tools on a 40-row VirtualTerminal — enough to reproduce the
overwrite — and asserts MCP Permissions, Audience, and Server default
are all visible simultaneously.
Code fixes from review:

- EnsureToolCursorVisible: reset stale ScrollOffset to 0 when MaxScroll
  drops to 0 (e.g. after terminal resize or tool-list shrink). The old
  early return left a nonzero offset in place, clipping the top rows
  even though all tools fit in the viewport.

- BuildFooter: apply .NoWrap() to all footer paths — the _confirmingSave
  branch and the non-ToolGrid fall-through at the end of the lambda were
  missing it. The first commit only applied NoWrap to the three ToolGrid
  status-indicator branches.

- BuildFooter: extract BuildToolGridFooterWithStatus helper to eliminate
  the three near-identical Layouts.Horizontal() blocks. Any future footer
  layout change now only needs to happen in one place.

Screenshot smoke test:

- Add tests/smoke/tapes/screenshots/mcp-permissions.tape — captures the
  mcp-permissions page in whatever state the runner sees (Loading when no
  daemon, ServerList when one is present) as a PNG baseline. Registered
  in run-smoke.sh SHOT_TAPES/SHOT_FRAMES as 'mcp-permissions-loading'.
  The first CI screenshots run will generate the .approved.png baseline.
Generated from first CI screenshots run (no-daemon Loading state):
header panel at row 0, connection-refused error body below it.
Matches the deterministic frame described in mcp-permissions.tape.
Cold-cache runs (fresh Ollama install + 350 MB model pull + ffmpeg apt
deps) consume ~25 min of setup before the VHS tapes even start. The
previous 30-minute limit caused a cancellation on cache-cold runners.
Native Smoke already uses 45 min; align Screenshot Regression to match.
The netclawd image build on a cold runner consistently takes ~19-20 min,
leaving no margin under the previous 20-minute limit. The job was
intermittently cancelled at exactly the timeout boundary. 30 minutes
gives a 10-minute safety margin for cold-cache builds.
@Aaronontheweb Aaronontheweb added tui Terminal UI (Termina) issues bug Something isn't working labels Jun 17, 2026
The previous screenshot tape captured only the no-daemon Loading state,
which showed a header and an error message but none of the actual
permissions UI. Replaced with two meaningful frames:

  mcp-permissions-server-list — ServerList state with smoke-math shown
    as Connected, 3 tools. Confirms the header renders at row 0 with real
    daemon data beneath it.

  mcp-permissions-tool-grid — ToolGrid with all header rows (Server,
    Audience, Server enabled, Server default) and all three tool rows
    (add, echo, record-tasks) visible simultaneously. This directly
    exercises the render fix for issue netclaw-dev#1424: if ScrollableContainerNode
    regresses and ignores bounds.Y, the tool rows will overwrite the header
    rows in the screenshot.

The tape now starts the daemon and registers Netclaw.SmokeMcpServer
(smoke-math, 3 tools) before launching the TUI.

run-native-tape.sh: extend the sed substitution pass to cover both the
preamble and the body file, so tape bodies can use __TOKENS__ too (needed
here for __NETCLAW_SMOKE_MCP_SERVER__).

Baselines removed: the stale mcp-permissions-loading.approved.png is
deleted. CI will generate the two new baselines on the first screenshots
run.
ServerList frame: smoke-math appears as Connected, 3 tools.
ToolGrid frame: Server/Audience/Server-enabled/Server-default header rows
plus add/echo/record-tasks tool rows all visible simultaneously — the
direct regression check for netclaw-dev#1424 (scroll container must not overwrite
header rows).
@Aaronontheweb Aaronontheweb marked this pull request as ready for review June 17, 2026 16:45
@Aaronontheweb Aaronontheweb enabled auto-merge (squash) June 17, 2026 16:45
@Aaronontheweb Aaronontheweb merged commit ad8daa3 into netclaw-dev:dev Jun 17, 2026
15 checks passed
@Aaronontheweb Aaronontheweb deleted the worktree-fix+1424-mcp-permissions-clipping branch June 23, 2026 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working tui Terminal UI (Termina) issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant