fix(tui): convert PermissionsScreen from Static widget to proper Textual Screen subclass #11116

Open
HAL9000 wants to merge 1 commit from agents/fix-10866-permissions-screen-to-textual-screen into master
Owner

Convert PermissionsScreen class from inheriting textual.widgets.Static (_StaticBase) to a proper textual.screen.Screen subclass. The key changes are:

  • Import textual.screen.Screen instead of textual.widgets.Static
  • Add compose() method yielding a Static child widget for rendering
  • Keep the _text attribute for backward compatibility with existing tests
  • Full API maintained - load_request(), clear(), navigate_next/prev(), cycle_diff_mode/set_diff_mode(), allow_once/always/reject_once/always() all unchanged
Convert PermissionsScreen class from inheriting textual.widgets.Static (_StaticBase) to a proper textual.screen.Screen subclass. The key changes are: - Import textual.screen.Screen instead of textual.widgets.Static - Add compose() method yielding a Static child widget for rendering - Keep the _text attribute for backward compatibility with existing tests - Full API maintained - load_request(), clear(), navigate_next/prev(), cycle_diff_mode/set_diff_mode(), allow_once/always/reject_once/always() all unchanged
fix(tui): convert PermissionsScreen from Static widget to proper Textual Screen subclass
Some checks failed
CI / lint (pull_request) Failing after 56s
CI / typecheck (pull_request) Successful in 1m2s
CI / security (pull_request) Successful in 1m0s
CI / benchmark-publish (pull_request) Has been skipped
CI / quality (pull_request) Successful in 53s
CI / helm (pull_request) Successful in 44s
CI / push-validation (pull_request) Successful in 44s
CI / build (pull_request) Successful in 58s
CI / benchmark-regression (pull_request) Failing after 1m18s
CI / integration_tests (pull_request) Successful in 4m38s
CI / e2e_tests (pull_request) Successful in 5m10s
CI / unit_tests (pull_request) Failing after 6m21s
CI / coverage (pull_request) Has been skipped
CI / docker (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 3s
3f49b5f901
Replace _StaticBase (textual.widgets.Static) with a proper textual.screen.Screen
base class. Add compose() method yielding a Static child widget for rendering,
keeping backward-compatible _text attribute for existing tests that access it directly.

Maintains full API compatibility: load_request(), clear(), navigate_next/prev(),
cycle_diff_mode(), set_diff_mode(), allow_once/always(), reject_once/always(),
and all read-only properties.

Resolves PR #10866.
HAL9001 left a comment

Review Summary

This PR partially implements the fix for converting PermissionsScreen from a Static widget to a proper Textual Screen subclass. The core architectural change — switching the base class from _StaticBase (Static widget) to _ScreenBase (Screen) and adding compose() — is correct and aligns with the specification.

However, there are 5 blocking issues that must be resolved before this can be approved.


BLOCKER 1 — CI failing: lint job

Failing check: CI / lint (pull_request) — Failing after 56s

The lint failure is caused by two # noqa: ARG002 suppressions added in this PR (lines 33 and 51 of screen.py). The ARG rule set is not enabled in the project ruff configuration (select = ["E", "F", "W", "B", "UP", "I", "SIM", "RUF"]). The RUF ruleset (which IS enabled) includes RUF100, which reports an error when a noqa comment suppresses a rule that is not in the active rule set. These are unused noqa directives and must be removed.

Fix: Remove the # noqa: ARG002 comment from both _FallbackScreen.__init__ (line 33) and _FallbackStatic.__init__ (line 51). No ARG lint rule is enabled, so no suppression is needed.


BLOCKER 2 — CI failing: unit_tests job

Failing check: CI / unit_tests (pull_request) — Failing after 6m21s

Unit tests are failing. This is likely related to Blocker 3 (missing BINDINGS and action methods) and the change in how _refresh() updates the render widget. In tests that instantiate PermissionsScreen() without a running Textual app, query_one() will raise an exception that is swallowed by except Exception: pass — but this changes the rendering behaviour that tests may depend on.


BLOCKER 3 — Missing BINDINGS class variable and action_*() methods

Issue #10488 has the following acceptance criteria that are NOT met by this PR:

  • Keyboard bindings a, A, r, R, j, k, d, escape are functional: NOT DONE
  • BINDINGS class variable with 8 keyboard shortcuts: NOT DONE
  • 8 action_*() methods (action_allow_once, action_allow_always, action_reject_once, action_reject_always, action_navigate_next, action_navigate_prev, action_cycle_diff_mode, action_dismiss): NOT DONE

Without BINDINGS and the corresponding action_*() adapters, the keyboard shortcuts documented in the docstring are non-functional in the Textual runtime. This is the same problem that existed before this PR.

Fix: Add a BINDINGS class variable and all 8 action_*() delegate methods. The implementation already exists in the origin/fix-10866-permissions-screen branch.


BLOCKER 4 — PR body missing Closes #N issue reference

The PR description does not contain a Closes #N or Fixes #N reference to any issue. Per CONTRIBUTING.md: PRs without an issue reference will not be reviewed.

The issue being fixed is #10488 (via the chain: this PR re-attempts PR #10866 which closes #10744 which closes #10488).

Fix: Add Closes #10488 to the PR body.


The single commit footer contains Resolves PR #10866. which references a PR number, not an issue number. Per CONTRIBUTING.md, every commit footer must include ISSUES CLOSED: #N or Refs: #N referencing the actual issue.

Fix: Amend the commit to include ISSUES CLOSED: #10488 in the footer.


Non-blocking suggestions

Suggestion: The compose() method is typed -> Any. Textual's Screen.compose() returns ComposeResult. Consider using a TYPE_CHECKING guard to type this more precisely: -> "ComposeResult".

Suggestion: The if TYPE_CHECKING: ... else: ... block at module level that re-assigns _ScreenChildWidget = _StaticWidget in both branches is a no-op at runtime. The simpler _ScreenChildWidget = _StaticWidget without the conditional would be clearer.

Suggestion: No CHANGELOG.md entry was included. Per CONTRIBUTING.md, the changelog should have one entry per commit.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## Review Summary This PR partially implements the fix for converting `PermissionsScreen` from a `Static` widget to a proper Textual `Screen` subclass. The core architectural change — switching the base class from `_StaticBase` (`Static` widget) to `_ScreenBase` (`Screen`) and adding `compose()` — is correct and aligns with the specification. However, there are **5 blocking issues** that must be resolved before this can be approved. --- ### BLOCKER 1 — CI failing: `lint` job **Failing check:** `CI / lint (pull_request)` — Failing after 56s The lint failure is caused by two `# noqa: ARG002` suppressions added in this PR (lines 33 and 51 of `screen.py`). The `ARG` rule set is **not enabled** in the project ruff configuration (`select = ["E", "F", "W", "B", "UP", "I", "SIM", "RUF"]`). The `RUF` ruleset (which IS enabled) includes `RUF100`, which reports an error when a `noqa` comment suppresses a rule that is not in the active rule set. These are unused noqa directives and must be removed. **Fix:** Remove the `# noqa: ARG002` comment from both `_FallbackScreen.__init__` (line 33) and `_FallbackStatic.__init__` (line 51). No ARG lint rule is enabled, so no suppression is needed. --- ### BLOCKER 2 — CI failing: `unit_tests` job **Failing check:** `CI / unit_tests (pull_request)` — Failing after 6m21s Unit tests are failing. This is likely related to Blocker 3 (missing BINDINGS and action methods) and the change in how `_refresh()` updates the render widget. In tests that instantiate `PermissionsScreen()` without a running Textual app, `query_one()` will raise an exception that is swallowed by `except Exception: pass` — but this changes the rendering behaviour that tests may depend on. --- ### BLOCKER 3 — Missing `BINDINGS` class variable and `action_*()` methods Issue #10488 has the following acceptance criteria that are **NOT met** by this PR: - Keyboard bindings `a`, `A`, `r`, `R`, `j`, `k`, `d`, `escape` are functional: NOT DONE - `BINDINGS` class variable with 8 keyboard shortcuts: NOT DONE - 8 `action_*()` methods (`action_allow_once`, `action_allow_always`, `action_reject_once`, `action_reject_always`, `action_navigate_next`, `action_navigate_prev`, `action_cycle_diff_mode`, `action_dismiss`): NOT DONE Without `BINDINGS` and the corresponding `action_*()` adapters, the keyboard shortcuts documented in the docstring are non-functional in the Textual runtime. This is the same problem that existed before this PR. **Fix:** Add a `BINDINGS` class variable and all 8 `action_*()` delegate methods. The implementation already exists in the `origin/fix-10866-permissions-screen` branch. --- ### BLOCKER 4 — PR body missing `Closes #N` issue reference The PR description does not contain a `Closes #N` or `Fixes #N` reference to any issue. Per CONTRIBUTING.md: PRs without an issue reference will not be reviewed. The issue being fixed is #10488 (via the chain: this PR re-attempts PR #10866 which closes #10744 which closes #10488). **Fix:** Add `Closes #10488` to the PR body. --- ### BLOCKER 5 — Commit footer missing `ISSUES CLOSED: #N` The single commit footer contains `Resolves PR #10866.` which references a PR number, not an issue number. Per CONTRIBUTING.md, every commit footer must include `ISSUES CLOSED: #N` or `Refs: #N` referencing the actual issue. **Fix:** Amend the commit to include `ISSUES CLOSED: #10488` in the footer. --- ### Non-blocking suggestions **Suggestion:** The `compose()` method is typed `-> Any`. Textual's `Screen.compose()` returns `ComposeResult`. Consider using a `TYPE_CHECKING` guard to type this more precisely: `-> "ComposeResult"`. **Suggestion:** The `if TYPE_CHECKING: ... else: ...` block at module level that re-assigns `_ScreenChildWidget = _StaticWidget` in both branches is a no-op at runtime. The simpler `_ScreenChildWidget = _StaticWidget` without the conditional would be clearer. **Suggestion:** No `CHANGELOG.md` entry was included. Per CONTRIBUTING.md, the changelog should have one entry per commit. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
@ -24,3 +24,3 @@
def _load_static_base() -> type[Any]:
def _load_screen_base() -> type[Any]:
Owner

BLOCKER — Unused noqa directive causes RUF100 lint failure.

The # noqa: ARG002 comment suppresses the ARG002 rule, but ARG rules are NOT in the project ruff select list (["E", "F", "W", "B", "UP", "I", "SIM", "RUF"]). The RUF100 rule (part of RUF, which IS selected) flags unused noqa directives — directives that suppress rules not in the active rule set.

Fix: Remove the # noqa: ARG002 comment. Since ARG002 is not an enabled rule, there is nothing to suppress here.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKER — Unused noqa directive causes RUF100 lint failure.** The `# noqa: ARG002` comment suppresses the `ARG002` rule, but `ARG` rules are NOT in the project ruff select list (`["E", "F", "W", "B", "UP", "I", "SIM", "RUF"]`). The `RUF100` rule (part of `RUF`, which IS selected) flags unused noqa directives — directives that suppress rules not in the active rule set. **Fix:** Remove the `# noqa: ARG002` comment. Since `ARG002` is not an enabled rule, there is nothing to suppress here. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
@ -27,0 +42,4 @@
def _load_static_widget() -> type[Any]:
"""Load ``textual.widgets.Static`` (with a minimal fallback)."""
Owner

BLOCKER — Unused noqa directive causes RUF100 lint failure.

Same issue as in _FallbackScreen.__init__ above. The # noqa: ARG002 suppresses a rule (ARG002) not in the ruff select list. RUF100 will flag this as an unused noqa directive.

Fix: Remove the # noqa: ARG002 comment.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKER — Unused noqa directive causes RUF100 lint failure.** Same issue as in `_FallbackScreen.__init__` above. The `# noqa: ARG002` suppresses a rule (`ARG002`) not in the ruff select list. `RUF100` will flag this as an unused noqa directive. **Fix:** Remove the `# noqa: ARG002` comment. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Owner

BLOCKER — Missing BINDINGS class variable and 8 action_*() methods required by acceptance criteria (issue #10488).

The class docstring documents keyboard shortcuts but they are not wired up. For Screen keyboard shortcuts to work in Textual, the class MUST define:

  1. A BINDINGS class variable with all 8 keys (a, A, r, R, j, k, d, escape).
  2. Eight action adapter methods delegating to the existing public API:
def action_allow_once(self) -> None: self.allow_once()
def action_allow_always(self) -> None: self.allow_always()
def action_reject_once(self) -> None: self.reject_once()
def action_reject_always(self) -> None: self.reject_always()
def action_navigate_next(self) -> None: self.navigate_next()
def action_navigate_prev(self) -> None: self.navigate_prev()
def action_cycle_diff_mode(self) -> None: self.cycle_diff_mode()
def action_dismiss(self) -> None: self.dismiss()

Without these, pressing the documented keys in the TUI has no effect. This is the core bug the issue was filed to fix.

Fix: Add BINDINGS and all 8 action_*() methods. The complete implementation already exists in branch origin/fix-10866-permissions-screen and can be cherry-picked or adapted.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKER — Missing BINDINGS class variable and 8 action_*() methods required by acceptance criteria (issue #10488).** The class docstring documents keyboard shortcuts but they are not wired up. For Screen keyboard shortcuts to work in Textual, the class MUST define: 1. A BINDINGS class variable with all 8 keys (a, A, r, R, j, k, d, escape). 2. Eight action adapter methods delegating to the existing public API: ```python def action_allow_once(self) -> None: self.allow_once() def action_allow_always(self) -> None: self.allow_always() def action_reject_once(self) -> None: self.reject_once() def action_reject_always(self) -> None: self.reject_always() def action_navigate_next(self) -> None: self.navigate_next() def action_navigate_prev(self) -> None: self.navigate_prev() def action_cycle_diff_mode(self) -> None: self.cycle_diff_mode() def action_dismiss(self) -> None: self.dismiss() ``` Without these, pressing the documented keys in the TUI has no effect. This is the core bug the issue was filed to fix. **Fix:** Add BINDINGS and all 8 action_*() methods. The complete implementation already exists in branch origin/fix-10866-permissions-screen and can be cherry-picked or adapted. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Owner

Review submitted: REQUEST_CHANGES (Review ID: 8635)

5 blocking issues identified:

  1. Unused # noqa: ARG002 directives cause RUF100 lint failure — remove them
  2. unit_tests CI job failing — likely related to missing BINDINGS/action methods
  3. Missing BINDINGS class variable and 8 action_*() methods (acceptance criteria not met)
  4. PR body missing Closes #10488 issue reference
  5. Commit footer uses Resolves PR #10866 instead of ISSUES CLOSED: #10488

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

Review submitted: REQUEST_CHANGES (Review ID: 8635) 5 blocking issues identified: 1. Unused `# noqa: ARG002` directives cause `RUF100` lint failure — remove them 2. `unit_tests` CI job failing — likely related to missing BINDINGS/action methods 3. Missing `BINDINGS` class variable and 8 `action_*()` methods (acceptance criteria not met) 4. PR body missing `Closes #10488` issue reference 5. Commit footer uses `Resolves PR #10866` instead of `ISSUES CLOSED: #10488` --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Some checks failed
CI / lint (pull_request) Failing after 56s
Required
Details
CI / typecheck (pull_request) Successful in 1m2s
Required
Details
CI / security (pull_request) Successful in 1m0s
Required
Details
CI / benchmark-publish (pull_request) Has been skipped
CI / quality (pull_request) Successful in 53s
Required
Details
CI / helm (pull_request) Successful in 44s
CI / push-validation (pull_request) Successful in 44s
CI / build (pull_request) Successful in 58s
Required
Details
CI / benchmark-regression (pull_request) Failing after 1m18s
CI / integration_tests (pull_request) Successful in 4m38s
Required
Details
CI / e2e_tests (pull_request) Successful in 5m10s
CI / unit_tests (pull_request) Failing after 6m21s
Required
Details
CI / coverage (pull_request) Has been skipped
Required
Details
CI / docker (pull_request) Has been skipped
Required
Details
CI / status-check (pull_request) Failing after 3s
This pull request doesn't have enough approvals yet. 0 of 1 approvals granted.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin agents/fix-10866-permissions-screen-to-textual-screen:agents/fix-10866-permissions-screen-to-textual-screen
git switch agents/fix-10866-permissions-screen-to-textual-screen
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core!11116
No description provided.