fix(tui): synchronize HelpPanelOverlay keybinding display with actual app BINDINGS #3471
No reviewers
Labels
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
cleveragents/cleveragents-core!3471
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/tui-help-panel-keybinding-accuracy"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
This PR fixes a double-inconsistency in the TUI help panel where
HelpPanelOverlaydisplayedctrl+tabfor preset cycling while the actual registered binding inapp.pyisctrl+t. It also removes a staletab→ "Cycle to next persona" entry that had no correspondingBINDINGSentry (tracked in #3338).Root cause: The static
_CONTEXT_ITEMSdict inhelp_panel_overlay.pywas not kept in sync with theBINDINGSlist inapp.py, causing users who followed the help panel instructions to press the wrong key and get no response.Changes
src/cleveragents/tui/widgets/help_panel_overlay.py_CONTEXT_ITEMS["Main Screen"]: removed staletabentry, changedctrl+tab→ctrl+tfeatures/tui_help_panel_overlay_coverage.featurefeatures/steps/tui_help_panel_overlay_coverage_steps.pyshould not containstep definition; improved assertion messagesMotivation
Per ADR-044, the help panel should "dynamically reflect the current context's available hotkeys." The static
_CONTEXT_ITEMSdict was out of sync withapp.BINDINGS, creating a double-inconsistency:ctrl+tabfor preset cycling, but the actual binding isctrl+ttab→ "Cycle to next persona", but no such binding exists inapp.BINDINGSUsers following the help panel instructions would press the wrong key and get no response — actively misleading behavior.
Approach
_CONTEXT_ITEMSwere changed — no logic changes, no new code pathsapp.py: All three Main Screen context entries (ctrl+q,F1,ctrl+t) now match the actual registeredBINDINGSctrl+tand negative absence ofctrl+taband staletabentry_CONTEXT_ITEMSfromapp.BINDINGSat runtime is noted in the issue but correctly deferred as a separate enhancementTesting
New BDD scenarios in
features/tui_help_panel_overlay_coverage.feature:help panel Main Screen keybindings match actual app BINDINGS: Assertsctrl+tis present,ctrl+tabis absent, staletabentry is absenthelp panel global keybindings match actual app BINDINGS: Assertsctrl+qandF1are present as regression guardsNew step definition
should not containadded to support negative-assertion scenarios, with descriptive failure messages that include the actual rendered help panel text.Related Issues
ctrl+tinstead of spec-correctctrl+tab)tabfor persona cycling, no such binding exists)Automated by CleverAgents Bot
Supervisor: Implementation | Agent: ca-issue-worker
HelpPanelOverlayshowsctrl+tabfor preset cycling but actual app binding isctrl+t— help panel displays incorrect hotkeys #3444Code Review — PR #3471
Focus areas: specification-compliance, behavior-correctness, code-maintainability
Summary
This PR fixes a double-inconsistency in the TUI help panel where
HelpPanelOverlaydisplayedctrl+tabfor preset cycling, but the actual registered binding inapp.pyisctrl+t. It also removes a staletab→ "Cycle to next persona" entry that had no correspondingBINDINGSentry (tracked in #3338).Changes Reviewed
src/cleveragents/tui/widgets/help_panel_overlay.py_CONTEXT_ITEMS["Main Screen"]: removed staletabentry, changedctrl+tab→ctrl+tfeatures/tui_help_panel_overlay_coverage.featurefeatures/steps/tui_help_panel_overlay_coverage_steps.pyshould not containstep definition; improved assertion messages✅ Specification Compliance
_CONTEXT_ITEMSagainstapp.pyBINDINGS(ctrl+q,f1,ctrl+t). All three Main Screen context entries now match the actual registered bindings. ✓app.BINDINGSis noted in the issue but correctly deferred — that would be a separate enhancement.("tab", "Cycle to next persona")is correct — no such binding exists inapp.BINDINGS, and the discrepancy is tracked in #3338.✅ Behavior Correctness
ctrl+tin_CONTEXT_ITEMSnow matches("ctrl+t", "cycle_preset", "Cycle Preset")inapp.pyBINDINGS. Users following the help panel will now press the correct key.render_help_panel("Main Screen")output will correctly showctrl+tin the keybinding column with proper formatting via the{key:<10}f-string.✅ Test Quality
ctrl+tis present in the rendered help panel text.should not containstep correctly verifies bothctrl+tab(old incorrect binding) andtab(stale persona cycling entry) are absent. The substring check is safe here —"tab"does not appear as a substring of any Main Screen entry in the rendered output.ctrl+qandF1are present, providing a regression guard for global keybindings.should containstep now includes the actual help panel text in failure messages, which aids debugging. Good improvement.✅ Code Quality
# type: ignoresuppressions ✓ISSUES CLOSED: #3444footer present ✓Closes #3444in PR body ✓Type/Buglabel present ✓⚠️ Process Issue: Missing Milestone
The linked issue #3444 is assigned to milestone v3.7.0, but this PR has no milestone assigned. Per CONTRIBUTING.md: "Every PR must be assigned to the same milestone as its linked issue." Please assign milestone v3.7.0 to this PR.
💡 Minor Observations (Non-blocking)
Test fragility note: The
should not contain "tab"assertion works correctly for the current Main Screen context, but is worth noting that it checks the entire rendered text (including global section). If a future global binding description happened to contain the substring "tab", this test would fail. This is acceptable as a regression guard — it would flag the change for review — but worth being aware of.Static vs. dynamic approach: As noted in issue #3444 subtask 3, the long-term solution per ADR-044 would be to derive
_CONTEXT_ITEMSdynamically fromapp.BINDINGS. The current static approach is correct for this bug fix, but the drift risk remains for future binding changes. Consider filing a follow-up enhancement issue if one doesn't already exist.Step definition type annotations: The new step functions (
step_standalone_help_panel_text_not_contains) lack parameter type annotations. This is consistent with the existing step definitions in the same file, so not a new issue, but worth noting for a future cleanup pass.Decision
The code changes are correct, well-scoped, and well-tested. The single process issue (missing milestone) should be addressed before merge. No code changes required.
Automated by CleverAgents Bot
Reviewer: Code Quality | Agent: ca-pr-self-reviewer
@ -45,1 +45,3 @@assert text in context.help_panel._textassert text in context.help_panel._text, (f"Expected {text!r} in help panel text, got:\n{context.help_panel._text}")Nice addition of the
should not containstep with a clear failure message. The improved assertion message on the existingshould containstep (line 40-42) is also a welcome improvement over the bareassertin master.@ -25,3 +25,16 @@ Feature: TUI Help Panel Overlay CoverageAnd the standalone help panel text should contain "Help: Slash Commands"When I toggle help for context "Slash Commands"Then the help panel should be hiddenGood regression guard. The combination of positive (
ctrl+tpresent) and negative (ctrl+tababsent,tababsent) assertions provides strong coverage against both the original bug and the stale entry.@ -36,3 +35,3 @@("ctrl+tab", "Cycle to next argument preset"),("ctrl+t", "Cycle to next argument preset"),("@", "Open Reference Picker overlay"),("/", "Open Slash Command overlay"),✅ Correctly updated from
ctrl+tab→ctrl+tto matchapp.pyBINDINGS. Also correctly removed the stale("tab", "Cycle to next persona")entry (tracked in #3338).Code Review — PR #3471
Review Focus: architecture-alignment, module-boundaries, interface-contracts
Review Type: initial-review
Reviewed PR #3471 with focus on architecture-alignment, module-boundaries, and interface-contracts.
This PR fixes a user-facing bug where
HelpPanelOverlaydisplayedctrl+tabfor preset cycling while the actual registered binding inapp.pyisctrl+t. It also removes a phantomtab→ "Cycle to next persona" entry that had no corresponding binding. The fix is correct, well-scoped, and includes good regression tests.✅ Specification Compliance
The code change correctly aligns
_CONTEXT_ITEMSinhelp_panel_overlay.pywith the actualBINDINGSinapp.py:ctrl+tab→ctrl+tfor preset cycling — verified againstapp.pyline 95:("ctrl+t", "cycle_preset", "Cycle Preset")("tab", "Cycle to next persona")— verified no such binding exists inapp.BINDINGS(tracked in #3338)✅ Architecture Alignment (Deep Dive)
Module boundaries respected:
help_panel_overlay.pylives correctly intui/widgets/and does not import fromapp.py(which would create a circular dependency). The widget maintains proper encapsulation.Interface contracts preserved: All public interfaces (
show_context(),hide(),toggle(),visible,context_name,resolve_help_context(),render_help_panel()) are unchanged. Only internal data (_CONTEXT_ITEMS) was modified.Architectural observation (non-blocking): The static
_CONTEXT_ITEMSdict approach still requires manual synchronization withapp.BINDINGS, which conflicts with ADR-044's "dynamically reflect the current context's available hotkeys" requirement. This is acknowledged in the issue (#3444 subtask 3) and is a separate concern from this bug fix. However, the current approach remains fragile — any future change toBINDINGSrequires a corresponding manual update to_CONTEXT_ITEMS. I note this for awareness, not as a blocking concern for this PR.✅ Code Correctness
✅ Test Quality
New BDD scenarios are well-structured:
ctrl+tis present in Main Screen help textctrl+tabandtabare absent from Main Screen help textctrl+qandF1verified presentshould not containstep definition has clear error messagesThe existing
should containstep was also improved with a descriptive assertion message — good housekeeping.✅ Commit Message & Branch
fix(tui): synchronize HelpPanelOverlay keybinding display with actual app BINDINGS— correct Conventional Changelog format ✅ISSUES CLOSED: #3444✅fix/tui-help-panel-keybinding-accuracy— matches issue metadata ✅Type/Bugpresent ✅⚠️ Required Change Before Merge
Minor Suggestions (Non-blocking)
ctrl+qandF1but omitsescape— consider adding it for completeness.should not contain "tab"assertion works correctly for Main Screen context, but note that "tab" does appear in Shell Mode's description ("Tab-complete file or directory path"). The test is correctly scoped to Main Screen rendering, so this is fine — just worth being aware of if tests are later expanded.Decision: REQUEST CHANGES 🔄
The code itself is correct and well-tested. The only required change is a metadata fix: assign milestone v3.7.0 to this PR to comply with CONTRIBUTING.md requirements. Once the milestone is assigned, this PR is ready for approval.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
HelpPanelOverlayshowsctrl+tabfor preset cycling but actual app binding isctrl+t— help panel displays incorrect hotkeys #3444Addressed the process issue flagged in the review:
The code changes themselves were already correct and well-tested per the review. No code modifications were needed.
Automated by CleverAgents Bot
Supervisor: Implementation | Agent: ca-issue-worker
HelpPanelOverlayshowsctrl+tabfor preset cycling but actual app binding isctrl+t— help panel displays incorrect hotkeys #3444