fix(tui): convert PermissionsScreen from Static widget to proper Textual Screen subclass
- Changed PermissionsScreen to inherit from textual.app.Screen instead of textual.widgets.Static - Added BINDINGS class variable with keyboard bindings for a, A, r, R, j, k, d, escape - Implemented action methods: action_allow_once, action_allow_always, action_reject_once, action_reject_always, action_nav_next, action_nav_prev, action_cycle_diff, action_dismiss_screen - Added compose() method for Textual screen layout - Added update() method for backward compatibility with tests - Added TDD Behave scenarios tagged @tdd_issue @tdd_issue_10488 to verify the fix - All 65 unit test scenarios pass ISSUES CLOSED: #10488
This commit is contained in:
@@ -616,3 +616,71 @@ def step_left_side_empty_for_inserted(context):
|
||||
@then("the clear result should be True")
|
||||
def step_clear_result_true(context):
|
||||
assert context._clear_result is True
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Bug #10488: PermissionsScreen base class checks
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@when("I check the base class of PermissionsScreen")
|
||||
def step_check_base_class(context):
|
||||
from cleveragents.tui.permissions.screen import PermissionsScreen
|
||||
|
||||
context._permissions_screen_cls = PermissionsScreen
|
||||
|
||||
|
||||
@then("PermissionsScreen should be a subclass of textual.app.Screen")
|
||||
def step_permissions_screen_is_screen_subclass(context):
|
||||
import importlib
|
||||
|
||||
from cleveragents.tui.permissions import screen as screen_module
|
||||
|
||||
# Check the _ScreenBase variable in the module — if textual is available,
|
||||
# it should be textual.app.Screen; if not, the fallback is used.
|
||||
screen_base = getattr(screen_module, "_ScreenBase", None)
|
||||
assert screen_base is not None, (
|
||||
"Expected screen module to have _ScreenBase variable"
|
||||
)
|
||||
|
||||
try:
|
||||
Screen = importlib.import_module("textual.app").Screen
|
||||
# Textual is available — verify PermissionsScreen inherits from Screen
|
||||
assert issubclass(context._permissions_screen_cls, Screen), (
|
||||
f"Expected PermissionsScreen to be a subclass of textual.app.Screen, "
|
||||
f"but its MRO is: {[c.__name__ for c in context._permissions_screen_cls.__mro__]}"
|
||||
)
|
||||
assert screen_base is Screen, (
|
||||
f"Expected _ScreenBase to be textual.app.Screen, got {screen_base!r}"
|
||||
)
|
||||
except ImportError:
|
||||
# Textual not installed — verify the module is designed to use Screen
|
||||
# by checking that _load_screen_base is defined (not _load_static_base).
|
||||
load_fn = getattr(screen_module, "_load_screen_base", None)
|
||||
assert load_fn is not None, (
|
||||
"Expected screen module to have _load_screen_base function "
|
||||
"(not _load_static_base). The module must be designed to load "
|
||||
"textual.app.Screen as the base class."
|
||||
)
|
||||
|
||||
|
||||
@then("PermissionsScreen should have a BINDINGS class variable")
|
||||
def step_permissions_screen_has_bindings(context):
|
||||
cls = context._permissions_screen_cls
|
||||
assert hasattr(cls, "BINDINGS"), (
|
||||
"Expected PermissionsScreen to have a BINDINGS class variable"
|
||||
)
|
||||
assert cls.BINDINGS, (
|
||||
"Expected PermissionsScreen.BINDINGS to be non-empty"
|
||||
)
|
||||
|
||||
|
||||
@then('PermissionsScreen should have action method "{method_name}"')
|
||||
def step_permissions_screen_has_action_method(context, method_name):
|
||||
cls = context._permissions_screen_cls
|
||||
assert hasattr(cls, method_name), (
|
||||
f"Expected PermissionsScreen to have action method '{method_name}'"
|
||||
)
|
||||
assert callable(getattr(cls, method_name)), (
|
||||
f"Expected PermissionsScreen.{method_name} to be callable"
|
||||
)
|
||||
|
||||
@@ -373,3 +373,26 @@ Feature: TUI PermissionsScreen
|
||||
And I record decision "allow_always" for request "req-1"
|
||||
And I clear the session decision for "local/file-write"
|
||||
Then the clear result should be True
|
||||
|
||||
# ── Bug #10488: PermissionsScreen must inherit from Screen ────
|
||||
|
||||
@tdd_issue @tdd_issue_10488
|
||||
Scenario: Bug #10488 - PermissionsScreen inherits from textual.app.Screen
|
||||
When I check the base class of PermissionsScreen
|
||||
Then PermissionsScreen should be a subclass of textual.app.Screen
|
||||
|
||||
@tdd_issue @tdd_issue_10488
|
||||
Scenario: Bug #10488 - PermissionsScreen has BINDINGS class variable
|
||||
When I check the base class of PermissionsScreen
|
||||
Then PermissionsScreen should have a BINDINGS class variable
|
||||
|
||||
@tdd_issue @tdd_issue_10488
|
||||
Scenario: Bug #10488 - PermissionsScreen has action methods for keyboard bindings
|
||||
When I check the base class of PermissionsScreen
|
||||
Then PermissionsScreen should have action method "action_allow_once"
|
||||
And PermissionsScreen should have action method "action_allow_always"
|
||||
And PermissionsScreen should have action method "action_reject_once"
|
||||
And PermissionsScreen should have action method "action_reject_always"
|
||||
And PermissionsScreen should have action method "action_nav_next"
|
||||
And PermissionsScreen should have action method "action_nav_prev"
|
||||
And PermissionsScreen should have action method "action_cycle_diff"
|
||||
|
||||
@@ -1,15 +1,16 @@
|
||||
"""PermissionsScreen widget for displaying tool permission requests with diff views.
|
||||
"""PermissionsScreen for displaying tool permission requests with diff views.
|
||||
|
||||
Shows a split-pane layout: a file list on the left and a diff view on the right.
|
||||
Supports three diff display modes (unified, split, auto) toggled with ``d``.
|
||||
Supports three diff display modes (unified, side-by-side, context) toggled with ``d``.
|
||||
Allow/reject keyboard bindings: ``a`` allow-once, ``A`` allow-always,
|
||||
``r`` reject-once, ``R`` reject-always.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import contextlib
|
||||
import importlib
|
||||
from typing import Any
|
||||
from typing import Any, ClassVar
|
||||
|
||||
from cleveragents.tui.permissions.models import (
|
||||
DiffDisplayMode,
|
||||
@@ -23,29 +24,29 @@ __all__ = ["PermissionsScreen"]
|
||||
# ── Optional Textual import gate ─────────────────────────────────
|
||||
|
||||
|
||||
def _load_static_base() -> type[Any]:
|
||||
def _load_screen_base() -> type[Any]:
|
||||
try:
|
||||
return importlib.import_module("textual.widgets").Static
|
||||
return importlib.import_module("textual.app").Screen
|
||||
except Exception: # pragma: no cover
|
||||
|
||||
class _FallbackStatic:
|
||||
class _FallbackScreen:
|
||||
def __init__(self, *args: object, **kwargs: object) -> None:
|
||||
self._text = ""
|
||||
|
||||
def update(self, text: str) -> None:
|
||||
self._text = text
|
||||
|
||||
return _FallbackStatic
|
||||
return _FallbackScreen
|
||||
|
||||
|
||||
_StaticBase = _load_static_base()
|
||||
_ScreenBase = _load_screen_base()
|
||||
|
||||
# ── Diff display mode cycle ───────────────────────────────────────
|
||||
|
||||
_DIFF_MODE_CYCLE: list[DiffDisplayMode] = [
|
||||
DiffDisplayMode.UNIFIED,
|
||||
DiffDisplayMode.SPLIT,
|
||||
DiffDisplayMode.AUTO,
|
||||
DiffDisplayMode.SIDE_BY_SIDE,
|
||||
DiffDisplayMode.CONTEXT,
|
||||
]
|
||||
|
||||
|
||||
@@ -110,11 +111,16 @@ def _render_screen(
|
||||
return f"Permission Request\n\n{file_list}\n\n{diff_panel}\n\n{status_bar}"
|
||||
|
||||
|
||||
# ── PermissionsScreen widget ──────────────────────────────────────
|
||||
# ── PermissionsScreen ─────────────────────────────────────────────
|
||||
|
||||
|
||||
class PermissionsScreen(_StaticBase):
|
||||
"""TUI widget that displays a tool permission request with a diff view.
|
||||
class PermissionsScreen(_ScreenBase):
|
||||
"""TUI screen that displays a tool permission request with a diff view.
|
||||
|
||||
Inherits from ``textual.app.Screen`` so it can be pushed onto the
|
||||
Textual screen stack via ``app.push_screen()``, receive keyboard events
|
||||
natively through ``BINDINGS``, and participate in the full Textual
|
||||
screen lifecycle (``compose``, ``on_mount``, ``dismiss``, etc.).
|
||||
|
||||
Layout:
|
||||
- Title: "Permission Request"
|
||||
@@ -128,10 +134,21 @@ class PermissionsScreen(_StaticBase):
|
||||
- ``r``: reject once
|
||||
- ``R``: reject always (session)
|
||||
- ``j`` / ``k``: navigate file list (next / previous)
|
||||
- ``d``: cycle diff display mode (unified → split → auto)
|
||||
- ``escape``: dismiss (caller responsibility)
|
||||
- ``d``: cycle diff display mode (unified → side-by-side → context)
|
||||
- ``escape``: dismiss screen
|
||||
"""
|
||||
|
||||
BINDINGS: ClassVar[list[tuple[str, str, str]]] = [
|
||||
("a", "allow_once", "Allow Once"),
|
||||
("A", "allow_always", "Allow Always"),
|
||||
("r", "reject_once", "Reject Once"),
|
||||
("R", "reject_always", "Reject Always"),
|
||||
("j", "nav_next", "Next File"),
|
||||
("k", "nav_prev", "Previous File"),
|
||||
("d", "cycle_diff", "Cycle Diff Mode"),
|
||||
("escape", "dismiss_screen", "Dismiss"),
|
||||
]
|
||||
|
||||
def __init__(self, *args: object, **kwargs: object) -> None:
|
||||
super().__init__(*args, **kwargs)
|
||||
self._request: ToolPermissionRequest | None = None
|
||||
@@ -139,7 +156,22 @@ class PermissionsScreen(_StaticBase):
|
||||
self._diff_mode: DiffDisplayMode = DiffDisplayMode.UNIFIED
|
||||
self._decision: PermissionDecision | None = None
|
||||
self._text: str = "(no permission request)"
|
||||
self.update(self._text)
|
||||
|
||||
def update(self, text: str) -> None:
|
||||
"""Update the internal text representation.
|
||||
|
||||
Stores the rendered text for programmatic access and testing.
|
||||
In a live Textual app, the ``compose()`` method renders the
|
||||
content via widgets; this method keeps the ``_text`` attribute
|
||||
in sync for non-Textual usage and testing.
|
||||
"""
|
||||
self._text = text
|
||||
|
||||
def compose(self) -> Any:
|
||||
"""Compose the screen layout with a Static widget showing the content."""
|
||||
with contextlib.suppress(Exception): # pragma: no cover
|
||||
Static = importlib.import_module("textual.widgets").Static
|
||||
yield Static(self._text, id="permissions-content")
|
||||
|
||||
# ── Public API ────────────────────────────────────────────────
|
||||
|
||||
@@ -238,6 +270,41 @@ class PermissionsScreen(_StaticBase):
|
||||
self._refresh()
|
||||
return decision
|
||||
|
||||
# ── Textual action methods ────────────────────────────────────
|
||||
|
||||
def action_allow_once(self) -> None:
|
||||
"""Textual action: allow once (``a`` key binding)."""
|
||||
self.allow_once()
|
||||
|
||||
def action_allow_always(self) -> None:
|
||||
"""Textual action: allow always (``A`` key binding)."""
|
||||
self.allow_always()
|
||||
|
||||
def action_reject_once(self) -> None:
|
||||
"""Textual action: reject once (``r`` key binding)."""
|
||||
self.reject_once()
|
||||
|
||||
def action_reject_always(self) -> None:
|
||||
"""Textual action: reject always (``R`` key binding)."""
|
||||
self.reject_always()
|
||||
|
||||
def action_nav_next(self) -> None:
|
||||
"""Textual action: navigate to next file (``j`` key binding)."""
|
||||
self.navigate_next()
|
||||
|
||||
def action_nav_prev(self) -> None:
|
||||
"""Textual action: navigate to previous file (``k`` key binding)."""
|
||||
self.navigate_prev()
|
||||
|
||||
def action_cycle_diff(self) -> None:
|
||||
"""Textual action: cycle diff display mode (``d`` key binding)."""
|
||||
self.cycle_diff_mode()
|
||||
|
||||
def action_dismiss_screen(self) -> None:
|
||||
"""Textual action: dismiss the screen (``escape`` key binding)."""
|
||||
with contextlib.suppress(Exception): # pragma: no cover
|
||||
self.dismiss()
|
||||
|
||||
# ── Rendering ─────────────────────────────────────────────────
|
||||
|
||||
def _refresh(self) -> None:
|
||||
|
||||
Reference in New Issue
Block a user