Skip to content

ApprovalsManagerPage revokes index 0 regardless of selected row #1703

Description

@petabridge-netclaw

What happened

In the netclaw approvals TUI, navigating to a row using ↑/↓ and pressing R or Delete to revoke always deletes the first item (index 0) in the list, regardless of which row is visually highlighted.

Reproduction

  1. Run netclaw approvals
  2. Scroll down using to any row that is NOT the first item (e.g. row Fix reminder Slack notification delivery and failure signaling #181 out of 245)
  3. Press R or Delete to revoke
  4. Observe: the entry that gets deleted is the first item in the list, not the highlighted one

Root Cause

ApprovalsManagerPage.cs delegates arrow key navigation to _approvalList.HandleInput(keyInfo) (line 167), which updates the Termina SelectionListNode's internal visual highlight. However, SelectedIndex on the view model is only ever updated when the user presses Enter via SelectionConfirmed (line 146). Since SelectedIndex defaults to 0 and is never synced during navigation, StartRevoke() (which reads SelectedIndex) always targets index 0.

Relevant Code Paths

ApprovalsManagerPage.cs — key handling for the list state:

// Line 157-167
if (keyInfo.Key == ConsoleKey.R || keyInfo.Key == ConsoleKey.Delete)
{
    ViewModel.StartRevoke();  // SelectedIndex is stale (still 0)
    return;
}

_approvalList?.HandleInput(keyInfo);  // Updates Termina widget's internal focus only
ViewModel.RequestRedraw();

ApprovalsManagerPage.cs — only place where SelectedIndex is set:

// Line 140-149
_approvalList.SelectionConfirmed
    .Subscribe(selected =>
    {
        if (selected.Count == 0) return;
        var idx = rows.IndexOf(selected[0]);
        if (idx < 0) return;
        ViewModel.SelectedIndex = idx;  // ONLY fires on Enter
        ViewModel.StartRevoke();
    })

ApprovalsManagerViewModel.csStartRevoke reads the stale index:

// Line 86-91
public void StartRevoke()
{
    if (CurrentState.Value != ApprovalsManagerState.List) return;
    if (SelectedIndex < 0 || SelectedIndex >= DisplayApprovals.Count) return;

    PendingRevoke = DisplayApprovals[SelectedIndex];  // Always 0
    CurrentState.Value = ApprovalsManagerState.RevokeConfirm;
    StateVersion.Value++;
}

The Correct Pattern

SessionsPage.cs solves this exact same problem using a documented pattern. The page-level comment explains it:

"The scrollable SelectionListNode is focusable (FocusPriority 10), so the page's focus policy hands it keyboard focus and Termina's focus manager would route the arrows and Enter straight into it — moving the list's own highlight while the ViewModel's SelectedIndex (the resume target) never changes. Claim those keys at the page level, which Termina dispatches BEFORE the focus manager, so the list stays a pure renderer driven one-way by .WithHighlightedIndex(SelectedIndex)."

SessionsPage intercepts all navigation at the page level (HandlePageInput) and drives the visual highlight one-way via .WithHighlightedIndex(selectedIndex). ApprovalsManagerPage does the opposite — it delegates navigation to the widget and never syncs SelectedIndex.

Proposed Fix

Two approaches (pick whichever fits the architecture better):

Option A — One-way bind (matches SessionsPage pattern): Use .WithHighlightedIndex(ViewModel.SelectedIndex) on the SelectionListNode and intercept arrow keys at the page level to update SelectedIndex, matching SessionsPage.cs exactly.

Option B — Sync from widget: Subscribe to the SelectionListNode's selection-changed observable (if Termina exposes one) and keep SelectedIndex in sync with the widget's internal state.

Option A is preferable since it's already proven in the codebase.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingtuiTerminal UI (Termina) issues

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions